From 5112c4358eff4dd8659396b2ede384ffebaa450f Mon Sep 17 00:00:00 2001 From: Tao Yi Date: Mon, 24 Jun 2024 18:31:00 +0800 Subject: [PATCH] fix(translator): Re-fill client certificates of services after merging certificate (#6228) (#6232) (cherry picked from commit d410be40f1e93dbfd6a2c9c8421bbba0009ca378) --- CHANGELOG.md | 8 + internal/dataplane/parser/parser.go | 40 ++++- internal/dataplane/parser/parser_test.go | 36 ++++- .../dataplane/parser/translate_certs_test.go | 144 ++++++++++++++++++ 4 files changed, 224 insertions(+), 4 deletions(-) create mode 100644 internal/dataplane/parser/translate_certs_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index dc1ebe3d26..e9ae4fd025 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -79,6 +79,14 @@ Adding a new version? You'll need three changes: - [0.0.5](#005) - [0.0.4 and prior](#004-and-prior) +## Unreleased + +### Fixed + +- Services using `Secret`s containing the same certificate as client certificates + by annotation `konghq.com/client-cert` can be correctly translated. + [#6228](https://github.com/Kong/kubernetes-ingress-controller/pull/6228) + ## [2.12.4] > Release date: 2024-04-30 diff --git a/internal/dataplane/parser/parser.go b/internal/dataplane/parser/parser.go index 1184a7fdaa..160487e413 100644 --- a/internal/dataplane/parser/parser.go +++ b/internal/dataplane/parser/parser.go @@ -256,7 +256,19 @@ func (p *Parser) BuildKongConfig() KongConfigBuildingResult { ingressCerts := p.getCerts(ingressRules.SecretNameToSNIs) gatewayCerts := p.getGatewayCerts() // note that ingress-derived certificates will take precedence over gateway-derived certificates for SNI assignment - result.Certificates = mergeCerts(p.logger, ingressCerts, gatewayCerts) + var certIDsSeen certIDToMergedCertID + result.Certificates, certIDsSeen = mergeCerts(p.logger, ingressCerts, gatewayCerts) + + // re-fill client certificate IDs of services after certificates are merged. + for i, s := range result.Services { + if s.ClientCertificate != nil && s.ClientCertificate.ID != nil { + certID := s.ClientCertificate.ID + mergedCertID := certIDsSeen[*certID] + result.Services[i].ClientCertificate = &kong.Certificate{ + ID: kong.String(mergedCertID), + } + } + } // populate CA certificates in Kong result.CACertificates = p.getCACerts() @@ -657,9 +669,18 @@ func (p *Parser) registerResourceFailureNotSupportedForExpressionRoutes(obj clie } } -func mergeCerts(log logrus.FieldLogger, certLists ...[]certWrapper) []kongstate.Certificate { +type certIDToMergedCertID map[string]string + +type identicalCertIDSet struct { + mergedCertID string + certIDs []string +} + +func mergeCerts(log logrus.FieldLogger, certLists ...[]certWrapper) ([]kongstate.Certificate, certIDToMergedCertID) { snisSeen := make(map[string]string) certsSeen := make(map[string]certWrapper) + certIDSets := make(map[string]identicalCertIDSet) + for _, cl := range certLists { for _, cw := range cl { current, ok := certsSeen[cw.identifier] @@ -700,6 +721,12 @@ func mergeCerts(log logrus.FieldLogger, certLists ...[]certWrapper) []kongstate. } } certsSeen[current.identifier] = current + + idSet := certIDSets[current.identifier] + idSet.mergedCertID = *current.cert.ID + idSet.certIDs = append(idSet.certIDs, *cw.cert.ID) + certIDSets[current.identifier] = idSet + } } var res []kongstate.Certificate @@ -709,7 +736,14 @@ func mergeCerts(log logrus.FieldLogger, certLists ...[]certWrapper) []kongstate. }) res = append(res, kongstate.Certificate{Certificate: cw.cert}) } - return res + + idToMergedID := certIDToMergedCertID{} + for _, idSet := range certIDSets { + for _, certID := range idSet.certIDs { + idToMergedID[certID] = idSet.mergedCertID + } + } + return res, idToMergedID } func getServiceEndpoints( diff --git a/internal/dataplane/parser/parser_test.go b/internal/dataplane/parser/parser_test.go index f8e2309fb4..45387bda6b 100644 --- a/internal/dataplane/parser/parser_test.go +++ b/internal/dataplane/parser/parser_test.go @@ -859,6 +859,17 @@ func TestServiceClientCertificate(t *testing.T) { }, }, }, + { + Path: "/bar", + Backend: netv1.IngressBackend{ + Service: &netv1.IngressServiceBackend{ + Name: "bar-svc", + Port: netv1.ServiceBackendPort{ + Number: 80, + }, + }, + }, + }, }, }, }, @@ -886,6 +897,17 @@ func TestServiceClientCertificate(t *testing.T) { "tls.key": key, }, }, + { + ObjectMeta: metav1.ObjectMeta{ + UID: k8stypes.UID("ffaabbcc-180b-4702-a91f-61351a33c6e4"), + Name: "secret2", + Namespace: "default", + }, + Data: map[string][]byte{ + "tls.crt": crt, + "tls.key": key, + }, + }, } services := []*corev1.Service{ { @@ -897,6 +919,16 @@ func TestServiceClientCertificate(t *testing.T) { }, }, }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "bar-svc", + Namespace: "default", + Annotations: map[string]string{ + "konghq.com/client-cert": "secret2", + "konghq.com/protocol": "https", + }, + }, + }, } store, err := store.NewFakeStore(store.FakeObjects{ IngressesV1: ingresses, @@ -914,9 +946,11 @@ func TestServiceClientCertificate(t *testing.T) { assert.Equal("7428fb98-180b-4702-a91f-61351a33c6e4", *state.Certificates[0].ID) - assert.Equal(1, len(state.Services)) + assert.Equal(2, len(state.Services)) assert.Equal("7428fb98-180b-4702-a91f-61351a33c6e4", *state.Services[0].ClientCertificate.ID) + assert.Equal("7428fb98-180b-4702-a91f-61351a33c6e4", + *state.Services[1].ClientCertificate.ID) }) t.Run("client-cert secret doesn't exist", func(t *testing.T) { ingresses := []*netv1.Ingress{ diff --git a/internal/dataplane/parser/translate_certs_test.go b/internal/dataplane/parser/translate_certs_test.go new file mode 100644 index 0000000000..c908bae62c --- /dev/null +++ b/internal/dataplane/parser/translate_certs_test.go @@ -0,0 +1,144 @@ +package parser + +import ( + "sort" + "testing" + + "github.com/kong/go-kong/kong" + "github.com/sirupsen/logrus" + "github.com/stretchr/testify/require" + + "github.com/kong/kubernetes-ingress-controller/v2/internal/dataplane/kongstate" + "github.com/kong/kubernetes-ingress-controller/v2/test/helpers/certificate" +) + +func TestMergeCerts(t *testing.T) { + crt1, key1 := certificate.MustGenerateSelfSignedCertPEMFormat(certificate.WithCommonName("foo.com")) + crt2, key2 := certificate.MustGenerateSelfSignedCertPEMFormat(certificate.WithCommonName("bar.com")) + testCases := []struct { + name string + certs []certWrapper + mergedCerts []kongstate.Certificate + idToMergedID certIDToMergedCertID + }{ + { + name: "single certificate", + certs: []certWrapper{ + { + identifier: string(crt1) + string(key1), + cert: kong.Certificate{ + ID: kong.String("certificate-1"), + Cert: kong.String(string(crt1)), + Key: kong.String(string(key1)), + }, + snis: []string{"foo.com"}, + }, + }, + mergedCerts: []kongstate.Certificate{ + { + Certificate: kong.Certificate{ + ID: kong.String("certificate-1"), + Cert: kong.String(string(crt1)), + Key: kong.String(string(key1)), + SNIs: kong.StringSlice("foo.com"), + }, + }, + }, + idToMergedID: certIDToMergedCertID{"certificate-1": "certificate-1"}, + }, + { + name: "multiple different certifcates", + certs: []certWrapper{ + { + identifier: string(crt1) + string(key1), + cert: kong.Certificate{ + ID: kong.String("certificate-1"), + Cert: kong.String(string(crt1)), + Key: kong.String(string(key1)), + }, + snis: []string{"foo.com"}, + }, + { + identifier: string(crt2) + string(key2), + cert: kong.Certificate{ + ID: kong.String("certificate-2"), + Cert: kong.String(string(crt2)), + Key: kong.String(string(key2)), + }, + snis: []string{"bar.com"}, + }, + }, + mergedCerts: []kongstate.Certificate{ + { + Certificate: kong.Certificate{ + ID: kong.String("certificate-1"), + Cert: kong.String(string(crt1)), + Key: kong.String(string(key1)), + SNIs: kong.StringSlice("foo.com"), + }, + }, + { + Certificate: kong.Certificate{ + ID: kong.String("certificate-2"), + Cert: kong.String(string(crt2)), + Key: kong.String(string(key2)), + SNIs: kong.StringSlice("bar.com"), + }, + }, + }, + idToMergedID: certIDToMergedCertID{ + "certificate-1": "certificate-1", + "certificate-2": "certificate-2", + }, + }, + { + name: "multiple certs with same content should be merged", + certs: []certWrapper{ + { + identifier: string(crt1) + string(key1), + cert: kong.Certificate{ + ID: kong.String("certificate-1"), + Cert: kong.String(string(crt1)), + Key: kong.String(string(key1)), + }, + snis: []string{"foo.com"}, + }, + { + identifier: string(crt1) + string(key1), + cert: kong.Certificate{ + ID: kong.String("certificate-1-1"), + Cert: kong.String(string(crt1)), + Key: kong.String(string(key1)), + }, + snis: []string{"baz.com"}, + }, + }, + mergedCerts: []kongstate.Certificate{ + { + Certificate: kong.Certificate{ + ID: kong.String("certificate-1"), + Cert: kong.String(string(crt1)), + Key: kong.String(string(key1)), + // SNIs should be sorted + SNIs: kong.StringSlice("baz.com", "foo.com"), + }, + }, + }, + idToMergedID: certIDToMergedCertID{ + "certificate-1": "certificate-1", + "certificate-1-1": "certificate-1", + }, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + mergedCerts, idToMergedID := mergeCerts(logrus.StandardLogger(), tc.certs) + // sort certs by their IDs to make a stable order of the result merged certs. + sort.SliceStable(mergedCerts, func(i, j int) bool { + return *mergedCerts[i].Certificate.ID < *mergedCerts[j].Certificate.ID + }) + require.Equal(t, tc.mergedCerts, mergedCerts) + require.Equal(t, tc.idToMergedID, idToMergedID) + }) + } +}