fix(translator): Re-fill client certificates of services after mergin…

…g certificate (#6228) (#6232)

(cherry picked from commit d410be4)

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

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(

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{

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)
+		})
+	}
+}