Merge pull request #17 from datum-cloud/split-listener-certificates

Program downstream gateways to use a separate secret for each listener certificate

Author: joshlreese

internal/controller/gateway_controller.go

@@ -34,6 +34,7 @@ import (
 
 	"go.datum.net/network-services-operator/internal/config"
 	downstreamclient "go.datum.net/network-services-operator/internal/downstreamclient"
+	"go.datum.net/network-services-operator/internal/util/resourcename"
 )
 
 const gatewayControllerFinalizer = "gateway.networking.datumapis.com/gateway-controller"
@@ -104,13 +105,15 @@ func (r *GatewayReconciler) Reconcile(ctx context.Context, req mcreconcile.Reque
 	downstreamStrategy := downstreamclient.NewMappedNamespaceResourceStrategy(req.ClusterName, cl.GetClient(), r.DownstreamCluster.GetClient())
 
 	if !gateway.DeletionTimestamp.IsZero() {
-		if result := r.finalizeGateway(ctx, cl.GetClient(), &gateway, downstreamStrategy); result.ShouldReturn() {
-			return result.Complete(ctx)
-		}
+		if controllerutil.ContainsFinalizer(&gateway, gatewayControllerFinalizer) {
+			if result := r.finalizeGateway(ctx, cl.GetClient(), &gateway, downstreamStrategy); result.ShouldReturn() {
+				return result.Complete(ctx)
+			}
 
-		controllerutil.RemoveFinalizer(&gateway, gatewayControllerFinalizer)
-		if err := cl.GetClient().Update(ctx, &gateway); err != nil {
-			return ctrl.Result{}, fmt.Errorf("failed to remove finalizer from gateway: %w", err)
+			controllerutil.RemoveFinalizer(&gateway, gatewayControllerFinalizer)
+			if err := cl.GetClient().Update(ctx, &gateway); err != nil {
+				return ctrl.Result{}, fmt.Errorf("failed to remove finalizer from gateway: %w", err)
+			}
 		}
 
 		return ctrl.Result{}, nil
@@ -198,12 +201,6 @@ func (r *GatewayReconciler) ensureDownstreamGateway(
 		}
 		var listeners []gatewayv1.Listener
 		for _, l := range upstreamGateway.Spec.Listeners {
-
-			// TODO(jreese) this approach actually leads to request coalescing, resulting
-			// in the `OverlappingTLSConfig` condition being set to true on the downstream
-			// gateway, and HTTP2 disabled. We may need to either handle our own
-			// certificate requests for each listener, or create a gateway for each
-			// unique hostname that needs TLS.
 			if l.TLS != nil && l.TLS.Options[certificateIssuerTLSOption] != "" {
 				if r.Config.Gateway.PerGatewayCertificateIssuer {
 					downstreamGateway.Annotations["cert-manager.io/issuer"] = downstreamGateway.Name
@@ -230,7 +227,7 @@ func (r *GatewayReconciler) ensureDownstreamGateway(
 						// See: https://cert-manager.io/docs/usage/certificate/#cleaning-up-secrets-when-certificates-are-deleted
 						CertificateRefs: []gatewayv1.SecretObjectReference{
 							{
-								Name: gatewayv1.ObjectName(downstreamGateway.Name),
+								Name: gatewayv1.ObjectName(resourcename.GetValidDNS1123Name(fmt.Sprintf("%s-%s", downstreamGateway.Name, l.Name))),
 							},
 						},
 					}
@@ -267,7 +264,7 @@ func (r *GatewayReconciler) ensureDownstreamGateway(
 					// See: https://cert-manager.io/docs/usage/certificate/#cleaning-up-secrets-when-certificates-are-deleted
 					CertificateRefs: []gatewayv1.SecretObjectReference{
 						{
-							Name: gatewayv1.ObjectName(downstreamGateway.Name),
+							Name: gatewayv1.ObjectName(resourcename.GetValidDNS1123Name(fmt.Sprintf("%s-%s", downstreamGateway.Name, name))),
 						},
 					},
 				}

internal/util/resourcename/resourcename.go

@@ -0,0 +1,44 @@
+package resourcename
+
+import (
+	"crypto/md5"
+	"fmt"
+
+	validation "k8s.io/apimachinery/pkg/util/validation"
+)
+
+// GetValidDNS1123Name returns a name compliant with Kubernetes DNS-1123
+// subdomain length (253).
+// It truncates and appends an MD5 hash if the input name is too long.
+func GetValidDNS1123Name(name string) string {
+	return optionalTruncateAndHash(name, validation.DNS1123SubdomainMaxLength)
+}
+
+// GetValidDNS1035Name returns a name compliant with Kubernetes DNS-1035
+// label length (63).
+// It truncates and appends an MD5 hash if the input name is too long.
+func GetValidDNS1035Name(name string) string {
+	return optionalTruncateAndHash(name, validation.DNS1035LabelMaxLength)
+}
+
+// optionalTruncateAndHash ensures a name fits maxLength. If longer, it
+// truncates the name and appends an MD5 hash of the original name. Panics if
+// maxLength is too small for the hash suffix (33 chars).
+func optionalTruncateAndHash(name string, maxLength int) string {
+	if len(name) <= maxLength {
+		return name
+	}
+
+	hash := md5.Sum([]byte(name))
+
+	prefixLen := maxLength - 33
+	if prefixLen <= 0 {
+		panic(fmt.Sprintf("maxLength is too small: %d", maxLength))
+	}
+
+	if len(name) > prefixLen {
+		name = name[0:prefixLen]
+	}
+
+	return fmt.Sprintf("%s-%x", name, hash)
+}

internal/util/resourcename/resourcename_test.go

@@ -0,0 +1,48 @@
+package resourcename
+
+import (
+	"strings"
+	"testing"
+)
+
+func TestOptionalTruncateAndHash(t *testing.T) {
+	tests := []struct {
+		name  string
+		fn    func(string) string
+		input string
+		want  string
+	}{
+		{
+			name:  "DNS1123 no truncation",
+			fn:    GetValidDNS1123Name,
+			input: "short",
+			want:  "short",
+		},
+		{
+			name:  "DNS1123 truncation",
+			fn:    GetValidDNS1123Name,
+			input: "long" + strings.Repeat("a", 253),
+			want:  "long" + strings.Repeat("a", 253-33-4) + "-ac7d16f7520c0ebcf3269a2d2dbda4da",
+		},
+		{
+			name:  "DNS1035 no truncation",
+			fn:    GetValidDNS1035Name,
+			input: "short",
+			want:  "short",
+		},
+		{
+			name:  "DNS1035 truncation",
+			fn:    GetValidDNS1035Name,
+			input: "long" + strings.Repeat("a", 63),
+			want:  "long" + strings.Repeat("a", 63-33-4) + "-95b923195bff3e889498eca200310834",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := tt.fn(tt.input); got != tt.want {
+				t.Errorf("got %q, want %q", got, tt.want)
+			}
+		})
+	}
+}

test/e2e/gateway/chainsaw-test.yaml

@@ -329,7 +329,7 @@ spec:
                     certificateRefs:
                     - group: ""
                       kind: Secret
-                      name: test-gateway
+                      name: test-gateway-https-test-e2e
                     mode: Terminate
                 - allowedRoutes:
                     namespaces:
@@ -349,7 +349,7 @@ spec:
                     certificateRefs:
                     - group: ""
                       kind: Secret
-                      name: test-gateway
+                      name: test-gateway-https-0
                     mode: Terminate
                 - allowedRoutes:
                     namespaces:
@@ -369,7 +369,7 @@ spec:
                     certificateRefs:
                     - group: ""
                       kind: Secret
-                      name: test-gateway
+                      name: test-gateway-https-1
                     mode: Terminate
                 - allowedRoutes:
                     namespaces:
@@ -389,7 +389,7 @@ spec:
                     certificateRefs:
                     - group: ""
                       kind: Secret
-                      name: test-gateway
+                      name: test-gateway-https-2
                     mode: Terminate
               status:
                 conditions: