From d410be40f1e93dbfd6a2c9c8421bbba0009ca378 Mon Sep 17 00:00:00 2001 From: Tao Yi Date: Sat, 22 Jun 2024 04:57:52 +0800 Subject: [PATCH] fix(translator): Re-fill client certificates of services after merging certificate (#6228) --- CHANGELOG.md | 8 + .../dataplane/translator/translate_certs.go | 26 +++- .../translator/translate_certs_test.go | 144 ++++++++++++++++++ internal/dataplane/translator/translator.go | 14 +- .../dataplane/translator/translator_test.go | 36 ++++- 5 files changed, 224 insertions(+), 4 deletions(-) create mode 100644 internal/dataplane/translator/translate_certs_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 96a6341146..c4cb761fc6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -89,6 +89,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) + ## 3.2.0 > Release date: 2024-06-12 diff --git a/internal/dataplane/translator/translate_certs.go b/internal/dataplane/translator/translate_certs.go index d42fcfdb55..c79fee523d 100644 --- a/internal/dataplane/translator/translate_certs.go +++ b/internal/dataplane/translator/translate_certs.go @@ -172,9 +172,18 @@ func (t *Translator) getCerts(secretsToSNIs SecretNameToSNIs) []certWrapper { return certs } -func mergeCerts(logger logr.Logger, certLists ...[]certWrapper) []kongstate.Certificate { +type certIDToMergedCertID map[string]string + +type identicalCertIDSet struct { + mergedCertID string + certIDs []string +} + +func mergeCerts(logger logr.Logger, 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] @@ -214,6 +223,12 @@ func mergeCerts(logger logr.Logger, certLists ...[]certWrapper) []kongstate.Cert } } 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 + } } res := make([]kongstate.Certificate, 0, len(certsSeen)) @@ -225,5 +240,12 @@ func mergeCerts(logger logr.Logger, certLists ...[]certWrapper) []kongstate.Cert Certificate: cw.cert, }) } - return res + + idToMergedID := certIDToMergedCertID{} + for _, idSet := range certIDSets { + for _, certID := range idSet.certIDs { + idToMergedID[certID] = idSet.mergedCertID + } + } + return res, idToMergedID } diff --git a/internal/dataplane/translator/translate_certs_test.go b/internal/dataplane/translator/translate_certs_test.go new file mode 100644 index 0000000000..54e7c2b3ae --- /dev/null +++ b/internal/dataplane/translator/translate_certs_test.go @@ -0,0 +1,144 @@ +package translator + +import ( + "sort" + "testing" + + "github.com/go-logr/logr" + "github.com/kong/go-kong/kong" + "github.com/stretchr/testify/require" + + "github.com/kong/kubernetes-ingress-controller/v3/internal/dataplane/kongstate" + "github.com/kong/kubernetes-ingress-controller/v3/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(logr.Discard(), 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) + }) + } +} diff --git a/internal/dataplane/translator/translator.go b/internal/dataplane/translator/translator.go index bac5e82f50..5c6da0ef76 100644 --- a/internal/dataplane/translator/translator.go +++ b/internal/dataplane/translator/translator.go @@ -221,7 +221,19 @@ func (t *Translator) BuildKongConfig() KongConfigBuildingResult { ingressCerts := t.getCerts(ingressRules.SecretNameToSNIs) gatewayCerts := t.getGatewayCerts() // note that ingress-derived certificates will take precedence over gateway-derived certificates for SNI assignment - result.Certificates = mergeCerts(t.logger, ingressCerts, gatewayCerts) + var certIDsSeen certIDToMergedCertID + result.Certificates, certIDsSeen = mergeCerts(t.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 = t.getCACerts() diff --git a/internal/dataplane/translator/translator_test.go b/internal/dataplane/translator/translator_test.go index 8fca543c98..e05d3e22fc 100644 --- a/internal/dataplane/translator/translator_test.go +++ b/internal/dataplane/translator/translator_test.go @@ -860,6 +860,17 @@ func TestServiceClientCertificate(t *testing.T) { }, }, }, + { + Path: "/bar", + Backend: netv1.IngressBackend{ + Service: &netv1.IngressServiceBackend{ + Name: "bar-svc", + Port: netv1.ServiceBackendPort{ + Number: 80, + }, + }, + }, + }, }, }, }, @@ -887,6 +898,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{ { @@ -899,6 +921,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, @@ -916,9 +948,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{