fix: generate unique SSL IDs to prevent certificate conflicts across different hosts (#2592)

Signed-off-by: Ashing Zheng <[email protected]>

Author: ronething

api/adc/types.go

@@ -473,6 +473,19 @@ func (n Upstream) MarshalJSON() ([]byte, error) {
 	return json.Marshal((Alias)(n))
 }
 
+func ComposeSSLName(kind, namespace, name string) string {
+	p := make([]byte, 0, len(kind)+len(namespace)+len(name)+2)
+	buf := bytes.NewBuffer(p)
+
+	buf.WriteString(kind)
+	buf.WriteByte('_')
+	buf.WriteString(namespace)
+	buf.WriteByte('_')
+	buf.WriteString(name)
+
+	return buf.String()
+}
+
 // ComposeRouteName uses namespace, name and rule name to compose
 // the route name.
 func ComposeRouteName(namespace, name string, rule string) string {

internal/adc/translator/apisixtls.go

@@ -27,6 +27,7 @@ import (
 	"github.com/apache/apisix-ingress-controller/internal/controller/label"
 	"github.com/apache/apisix-ingress-controller/internal/id"
 	"github.com/apache/apisix-ingress-controller/internal/provider"
+	internaltypes "github.com/apache/apisix-ingress-controller/internal/types"
 )
 
 func (t *Translator) TranslateApisixTls(tctx *provider.TranslateContext, tls *apiv2.ApisixTls) (*TranslateResult, error) {
@@ -57,7 +58,7 @@ func (t *Translator) TranslateApisixTls(tctx *provider.TranslateContext, tls *ap
 	// Create SSL object
 	ssl := &adctypes.SSL{
 		Metadata: adctypes.Metadata{
-			ID:     id.GenID(tls.Namespace + "_" + tls.Name),
+			ID:     id.GenID(adctypes.ComposeSSLName(internaltypes.KindApisixTls, tls.Namespace, tls.Name)),
 			Labels: label.GenLabel(tls),
 		},
 		Certificates: []adctypes.Certificate{

internal/adc/translator/gateway.go

@@ -22,7 +22,6 @@ import (
 	"encoding/json"
 	"encoding/pem"
 	"fmt"
-	"slices"
 
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
@@ -50,7 +49,6 @@ func (t *Translator) TranslateGateway(tctx *provider.TranslateContext, obj *gate
 			result.SSL = append(result.SSL, ssl...)
 		}
 	}
-	result.SSL = mergeSSLWithSameID(result.SSL)
 
 	rk := utils.NamespacedNameKind(obj)
 	gatewayProxy, ok := tctx.GatewayProxies[rk]
@@ -80,7 +78,7 @@ func (t *Translator) translateSecret(tctx *provider.TranslateContext, listener g
 	sslObjs := make([]*adctypes.SSL, 0)
 	switch *listener.TLS.Mode {
 	case gatewayv1.TLSModeTerminate:
-		for _, ref := range listener.TLS.CertificateRefs {
+		for refIndex, ref := range listener.TLS.CertificateRefs {
 			ns := obj.GetNamespace()
 			if ref.Namespace != nil {
 				ns = string(*ref.Namespace)
@@ -122,8 +120,7 @@ func (t *Translator) translateSecret(tctx *provider.TranslateContext, listener g
 					}
 					sslObj.Snis = append(sslObj.Snis, hosts...)
 				}
-				// Note: use cert as id to avoid duplicate certificate across ssl objects
-				sslObj.ID = id.GenID(string(cert))
+				sslObj.ID = id.GenID(fmt.Sprintf("%s_%s_%d", adctypes.ComposeSSLName(internaltypes.KindGateway, obj.Namespace, obj.Name), listener.Name, refIndex))
 				t.Log.V(1).Info("generated ssl id", "ssl id", sslObj.ID, "secret", secretNN.String())
 				sslObj.Labels = label.GenLabel(obj)
 				sslObjs = append(sslObjs, sslObj)
@@ -241,47 +238,3 @@ func (t *Translator) fillPluginMetadataFromGatewayProxy(pluginMetadata adctypes.
 		pluginMetadata[pluginName] = pluginConfig
 	}
 }
-
-// mergeSSLWithSameID merge ssl with same id
-func mergeSSLWithSameID(sslList []*adctypes.SSL) []*adctypes.SSL {
-	if len(sslList) <= 1 {
-		return sslList
-	}
-
-	// create a map to store ssl with same id
-	sslMap := make(map[string]*adctypes.SSL)
-	for _, ssl := range sslList {
-		if existing, exists := sslMap[ssl.ID]; exists {
-			// if ssl with same id exists, merge their snis
-			// use map to deduplicate
-			sniMap := make(map[string]struct{})
-			// add existing snis
-			for _, sni := range existing.Snis {
-				sniMap[sni] = struct{}{}
-			}
-			// add new snis
-			for _, sni := range ssl.Snis {
-				sniMap[sni] = struct{}{}
-			}
-			// rebuild deduplicated snis list
-			newSnis := make([]string, 0, len(sniMap))
-			for sni := range sniMap {
-				newSnis = append(newSnis, sni)
-			}
-
-			slices.Sort(newSnis)
-			// update existing ssl object
-			existing.Snis = newSnis
-		} else {
-			slices.Sort(ssl.Snis)
-			// if new ssl id, add to map
-			sslMap[ssl.ID] = ssl
-		}
-	}
-
-	mergedSSL := make([]*adctypes.SSL, 0, len(sslMap))
-	for _, ssl := range sslMap {
-		mergedSSL = append(mergedSSL, ssl)
-	}
-	return mergedSSL
-}

internal/adc/translator/ingress.go

@@ -33,7 +33,7 @@ import (
 	internaltypes "github.com/apache/apisix-ingress-controller/internal/types"
 )
 
-func (t *Translator) translateIngressTLS(ingressTLS *networkingv1.IngressTLS, secret *corev1.Secret, labels map[string]string) (*adctypes.SSL, error) {
+func (t *Translator) translateIngressTLS(namespace, name string, tlsIndex int, ingressTLS *networkingv1.IngressTLS, secret *corev1.Secret, labels map[string]string) (*adctypes.SSL, error) {
 	// extract the key pair from the secret
 	cert, key, err := extractKeyPair(secret, true)
 	if err != nil {
@@ -64,7 +64,7 @@ func (t *Translator) translateIngressTLS(ingressTLS *networkingv1.IngressTLS, se
 		},
 		Snis: hosts,
 	}
-	ssl.ID = id.GenID(string(cert))
+	ssl.ID = id.GenID(fmt.Sprintf("%s_%d", adctypes.ComposeSSLName(internaltypes.KindIngress, namespace, name), tlsIndex))
 
 	return ssl, nil
 }
@@ -75,7 +75,7 @@ func (t *Translator) TranslateIngress(tctx *provider.TranslateContext, obj *netw
 	labels := label.GenLabel(obj)
 
 	// handle TLS configuration, convert to SSL objects
-	for _, tls := range obj.Spec.TLS {
+	for tlsIndex, tls := range obj.Spec.TLS {
 		if tls.SecretName == "" {
 			continue
 		}
@@ -86,7 +86,7 @@ func (t *Translator) TranslateIngress(tctx *provider.TranslateContext, obj *netw
 		if secret == nil {
 			continue
 		}
-		ssl, err := t.translateIngressTLS(&tls, secret, labels)
+		ssl, err := t.translateIngressTLS(obj.Namespace, obj.Name, tlsIndex, &tls, secret, labels)
 		if err != nil {
 			return nil, err
 		}

internal/provider/init/init.go

@@ -18,11 +18,12 @@
 package init
 
 import (
+	"github.com/go-logr/logr"
+
 	"github.com/apache/apisix-ingress-controller/internal/controller/status"
 	"github.com/apache/apisix-ingress-controller/internal/manager/readiness"
 	"github.com/apache/apisix-ingress-controller/internal/provider"
 	"github.com/apache/apisix-ingress-controller/internal/provider/apisix"
-	"github.com/go-logr/logr"
 )
 
 func init() {

internal/provider/register.go

@@ -21,9 +21,10 @@ import (
 	"fmt"
 	"net/http"
 
+	"github.com/go-logr/logr"
+
 	"github.com/apache/apisix-ingress-controller/internal/controller/status"
 	"github.com/apache/apisix-ingress-controller/internal/manager/readiness"
-	"github.com/go-logr/logr"
 )
 
 type RegisterHandler interface {

test/e2e/crds/v2/tls.go

@@ -336,5 +336,107 @@ spec:
 			assert.Contains(GinkgoT(), tls[0].Client.SkipMtlsURIRegex, skipMtlsUriRegex, "skip_mtls_uri_regex should be set")
 		})
 
+		It("ApisixTls and Ingress with same certificate but different hosts", func() {
+			By("create shared TLS secret")
+			err := s.NewKubeTlsSecret("shared-tls-secret", Cert, Key)
+			Expect(err).NotTo(HaveOccurred(), "creating shared TLS secret")
+
+			const apisixTlsSpec = `
+apiVersion: apisix.apache.org/v2
+kind: ApisixTls
+metadata:
+  name: test-apisixtls-shared
+spec:
+  ingressClassName: %s
+  hosts:
+  - api6.com
+  secret:
+    name: shared-tls-secret
+    namespace: %s
+`
+
+			By("apply ApisixTls with api6.com")
+			var apisixTls apiv2.ApisixTls
+			tlsSpec := fmt.Sprintf(apisixTlsSpec, s.Namespace(), s.Namespace())
+			applier.MustApplyAPIv2(types.NamespacedName{Namespace: s.Namespace(), Name: "test-apisixtls-shared"}, &apisixTls, tlsSpec)
+
+			const ingressYamlWithTLS = `
+apiVersion: networking.k8s.io/v1
+kind: Ingress
+metadata:
+  name: test-ingress-tls-shared
+spec:
+  ingressClassName: %s
+  tls:
+  - hosts:
+    - api7.com
+    secretName: shared-tls-secret
+  rules:
+  - host: api7.com
+    http:
+      paths:
+      - path: /
+        pathType: Prefix
+        backend:
+          service:
+            name: httpbin-service-e2e-test
+            port:
+              number: 80
+`
+
+			By("apply Ingress with api7.com using same certificate")
+			err = s.CreateResourceFromString(fmt.Sprintf(ingressYamlWithTLS, s.Namespace()))
+			Expect(err).NotTo(HaveOccurred(), "creating Ingress")
+
+			By("verify two SSL objects exist in control plane")
+			Eventually(func() bool {
+				tls, err := s.DefaultDataplaneResource().SSL().List(context.Background())
+				if err != nil {
+					return false
+				}
+				return len(tls) == 2
+			}).WithTimeout(30 * time.Second).ProbeEvery(1 * time.Second).Should(BeTrue())
+
+			tls, err := s.DefaultDataplaneResource().SSL().List(context.Background())
+			assert.Nil(GinkgoT(), err, "list tls error")
+			assert.Len(GinkgoT(), tls, 2, "should have exactly 2 SSL objects")
+
+			By("verify SSL objects have different IDs and SNIs")
+			sniFound := make(map[string]bool)
+
+			for i := range tls {
+				// Check certificate content is the same
+				assert.Len(GinkgoT(), tls[i].Certificates, 1, "each SSL should have 1 certificate")
+				assert.Equal(GinkgoT(), Cert, tls[i].Certificates[0].Certificate, "certificate should match")
+
+				// Track SNIs
+				for _, sni := range tls[i].Snis {
+					sniFound[sni] = true
+				}
+			}
+
+			By("verify both hosts are covered")
+			assert.True(GinkgoT(), sniFound["api6.com"], "api6.com should be in SNIs")
+			assert.True(GinkgoT(), sniFound["api7.com"], "api7.com should be in SNIs")
+
+			By("test HTTPS request to api6.com")
+			Eventually(func() int {
+				return s.NewAPISIXHttpsClient("api6.com").
+					GET("/get").
+					WithHost("api6.com").
+					Expect().
+					Raw().StatusCode
+			}).WithTimeout(30 * time.Second).ProbeEvery(1 * time.Second).Should(Equal(http.StatusOK))
+
+			By("test HTTPS request to api7.com")
+			Eventually(func() int {
+				return s.NewAPISIXHttpsClient("api7.com").
+					GET("/get").
+					WithHost("api7.com").
+					Expect().
+					Raw().StatusCode
+			}).WithTimeout(30 * time.Second).ProbeEvery(1 * time.Second).Should(Equal(http.StatusOK))
+		})
+
 	})
 })

test/e2e/gatewayapi/gateway.go

@@ -284,8 +284,8 @@ spec:
 			Eventually(func() error {
 				tls, err := s.DefaultDataplaneResource().SSL().List(context.Background())
 				Expect(err).NotTo(HaveOccurred(), "list ssl")
-				if len(tls) != 1 {
-					return fmt.Errorf("expect 1 ssl, got %d", len(tls))
+				if len(tls) != 2 {
+					return fmt.Errorf("expect 2 ssl, got %d", len(tls))
 				}
 				if len(tls[0].Certificates) != 1 {
 					return fmt.Errorf("expect 1 certificate, got %d", len(tls[0].Certificates))
@@ -305,7 +305,7 @@ spec:
 			Eventually(func() string {
 				tls, err := s.DefaultDataplaneResource().SSL().List(context.Background())
 				Expect(err).NotTo(HaveOccurred(), "list ssl")
-				if len(tls) < 1 {
+				if len(tls) != 2 {
 					return ""
 				}
 				if len(tls[0].Certificates) < 1 {

test/e2e/gatewayapi/tcproute.go

@@ -72,33 +72,25 @@ spec:
 				NotTo(HaveOccurred(), "creating GatewayProxy")
 
 			// Create GatewayClass
-			gatewayClassName := s.Namespace()
 			Expect(s.CreateResourceFromString(s.GetGatewayClassYaml())).
 				NotTo(HaveOccurred(), "creating GatewayClass")
-			gcyaml, _ := s.GetResourceYaml("GatewayClass", gatewayClassName)
-			s.ResourceApplied("GatewayClass", gatewayClassName, gcyaml, 1)
 
 			// Create Gateway with TCP listener
-			gatewayName := s.Namespace()
-			Expect(s.CreateResourceFromString(fmt.Sprintf(tcpGateway, gatewayName, gatewayClassName))).
+			Expect(s.CreateResourceFromString(fmt.Sprintf(tcpGateway, s.Namespace(), s.Namespace()))).
 				NotTo(HaveOccurred(), "creating Gateway")
-
-			gwyaml, _ := s.GetResourceYaml("Gateway", gatewayName)
-			s.ResourceApplied("Gateway", gatewayName, gwyaml, 1)
 		})
 
 		It("should route TCP traffic to backend service", func() {
-			gatewayName := s.Namespace()
 			By("creating TCPRoute")
-			Expect(s.CreateResourceFromString(fmt.Sprintf(tcpRoute, gatewayName))).
+			Expect(s.CreateResourceFromString(fmt.Sprintf(tcpRoute, s.Namespace()))).
 				NotTo(HaveOccurred(), "creating TCPRoute")
 
 			// Verify TCPRoute status becomes programmed
 			routeYaml, _ := s.GetResourceYaml("TCPRoute", "tcp-app-1")
 			s.ResourceApplied("TCPRoute", "tcp-app-1", routeYaml, 1)
 
 			By("verifying TCPRoute is functional")
-			s.HTTPOverTCPConnectAssert(true, time.Minute*5) // should be able to connect
+			s.HTTPOverTCPConnectAssert(true, time.Minute*3) // should be able to connect
 			By("sending TCP traffic to verify routing")
 			s.RequestAssert(&scaffold.RequestAssert{
 				Client:   s.NewAPISIXClientOnTCPPort(),
@@ -113,7 +105,7 @@ spec:
 			Expect(s.DeleteResource("TCPRoute", "tcp-app-1")).
 				NotTo(HaveOccurred(), "deleting TCPRoute")
 
-			s.HTTPOverTCPConnectAssert(false, time.Minute*5)
+			s.HTTPOverTCPConnectAssert(false, time.Minute*3)
 		})
 	})
 })