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

@@ -472,6 +472,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
 		}

test/e2e/crds/v2/tls.go

@@ -310,5 +310,108 @@ spec:
 			assert.Equal(GinkgoT(), int64(10), *tls[0].Client.Depth, "client depth should be 10")
 			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,21 +72,22 @@ spec:
 
 			// Create GatewayClass
 			Expect(s.CreateResourceFromString(s.GetGatewayClassYaml())).NotTo(HaveOccurred(), "creating GatewayClass")
+			Expect(s.CreateResourceFromString(s.GetGatewayClassYaml())).
+				NotTo(HaveOccurred(), "creating GatewayClass")
 
 			// Create Gateway with TCP listener
 			Expect(s.CreateResourceFromString(fmt.Sprintf(tcpGateway, s.Namespace(), s.Namespace()))).
 				NotTo(HaveOccurred(), "creating Gateway")
 		})
 
 		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")
 			time.Sleep(2 * time.Second)
 
 			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(),
@@ -101,7 +102,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)
 		})
 	})
 })

test/e2e/scaffold/assertion.go

@@ -200,35 +200,58 @@ func (s *Scaffold) HTTPOverTCPConnectAssert(shouldRespond bool, timeout time.Dur
 		defer func() {
 			_ = conn.Close()
 		}()
-		_, _ = fmt.Fprintf(conn, "GET /get HTTP/1.1\r\nHost: localhost\r\n\r\n")
+		_, _ = fmt.Fprintf(conn, "GET /get HTTP/1.1\r\nHost: localhost\r\nConnection: close\r\n\r\n")
+
+		// Read response - loop until EOF or error
+		// Set deadline for total read time (not per-read)
+		_ = conn.SetReadDeadline(time.Now().Add(10 * time.Second))
+		var responseBuilder strings.Builder
+		buf := make([]byte, 4096)
+
+		for {
+			n, err := conn.Read(buf)
+			if n > 0 {
+				responseBuilder.Write(buf[:n])
+			}
 
-		// Read response
-		_ = conn.SetReadDeadline(time.Now().Add(2 * time.Second))
-		buf := make([]byte, 1024)
-		n, err := conn.Read(buf)
+			// EOF is expected with Connection: close
+			if err == io.EOF {
+				break
+			}
+
+			// Other errors (including timeout)
+			if err != nil {
+				// If we haven't received any data yet, this is an error
+				if responseBuilder.Len() == 0 {
+					return fmt.Errorf("read error before receiving data: %v", err)
+				}
+				// If we have partial data, treat timeout as potential issue
+				if strings.Contains(err.Error(), "timeout") {
+					break // Use whatever we've received so far
+				}
+				return fmt.Errorf("read error: %v", err)
+			}
+		}
+
+		response := responseBuilder.String()
 
 		if shouldRespond {
 			// Should get a response (HTTP 200 from httpbin)
-			if err != nil || n == 0 {
-				return fmt.Errorf("expected response but got error: %v or empty response", err)
+			if len(response) == 0 {
+				return fmt.Errorf("expected response but got empty response")
 			}
 			// Check if we got a valid HTTP response
-			response := string(buf[:n])
 			if !strings.Contains(response, "HTTP/1.1") {
 				return fmt.Errorf("expected HTTP response but got: %s", response)
 			}
 		} else {
 			// Should get no response or connection reset
-			if err == nil && n > 0 {
-				return fmt.Errorf("expected no response but got: %s", string(buf[:n]))
-			}
-			// EOF or timeout is expected when no route is configured
-			if err != io.EOF && !strings.Contains(err.Error(), "timeout") {
-				return fmt.Errorf("expected EOF or timeout but got: %v", err)
+			if len(response) > 0 {
+				return fmt.Errorf("expected no response but got: %s", response)
 			}
 		}
 		return nil
-	}).WithTimeout(timeout).WithPolling(2 * time.Second).Should(Succeed())
+	}).WithTimeout(timeout).WithPolling(10 * time.Second).Should(Succeed())
 }
 
 func (s *Scaffold) RequestAssert(r *RequestAssert) bool {