diff --git a/operator/src/internal/controller/certificate_controller.go b/operator/src/internal/controller/certificate_controller.go index 3335f986d..a88b608d1 100644 --- a/operator/src/internal/controller/certificate_controller.go +++ b/operator/src/internal/controller/certificate_controller.go @@ -189,7 +189,7 @@ func (r *CertificateReconciler) ensureCertManagerManagedCert(ctx context.Context secretName = ddb.Name + "-gateway-cert-tls" } - serviceBase := util.DOCUMENTDB_SERVICE_PREFIX + ddb.Name + serviceBase := util.GetDocumentDBServiceName(ddb.Name) baseDNS := []string{serviceBase, serviceBase + "." + ddb.Namespace, serviceBase + "." + ddb.Namespace + ".svc"} dnsSet := map[string]struct{}{} finalDNS := []string{} @@ -289,7 +289,7 @@ func (r *CertificateReconciler) ensureSelfSignedCert(ctx context.Context, ddb *d } } - serviceBase := util.DOCUMENTDB_SERVICE_PREFIX + ddb.Name + serviceBase := util.GetDocumentDBServiceName(ddb.Name) dnsNames := []string{ serviceBase, serviceBase + "." + namespace, diff --git a/operator/src/internal/controller/certificate_controller_test.go b/operator/src/internal/controller/certificate_controller_test.go index 9753fc249..c0ac228d8 100644 --- a/operator/src/internal/controller/certificate_controller_test.go +++ b/operator/src/internal/controller/certificate_controller_test.go @@ -5,6 +5,7 @@ package controller import ( "context" + "strings" "testing" "time" @@ -126,7 +127,7 @@ func TestEnsureCertManagerManagedCert(t *testing.T) { } require.Contains(t, cert.Spec.DNSNames, "custom.example") // Should include service DNS names - serviceBase := util.DOCUMENTDB_SERVICE_PREFIX + ddb.Name + serviceBase := util.GetDocumentDBServiceName(ddb.Name) require.Contains(t, cert.Spec.DNSNames, serviceBase) // Simulate readiness condition then invoke ensure again (mimic reconcile loop) @@ -165,6 +166,34 @@ func TestEnsureSelfSignedCert(t *testing.T) { require.NotEmpty(t, ddb.Status.TLS.SecretName) } +func TestGatewayCertSANsMatchTruncatedServiceName(t *testing.T) { + ctx := context.Background() + // 19-char prefix + this name exceeds the 63-character Service name limit, so the + // operator truncates the Service name. The cert SANs must follow that truncation, + // otherwise the hostname published in status.connectionString fails strict TLS + // verification. Regression test for the SAN/Service-name divergence. + longName := strings.Repeat("a", 60) + ddb := baseDocumentDB(longName, "default") + ddb.Spec.TLS = &dbpreview.TLSConfiguration{Gateway: &dbpreview.GatewayTLS{Mode: "SelfSigned"}} + ddb.Status.TLS = &dbpreview.TLSStatus{} + r := buildCertificateReconciler(t, ddb) + + _, err := r.reconcileCertificates(ctx, ddb) + require.NoError(t, err) + + cert := &cmapi.Certificate{} + require.NoError(t, r.Client.Get(ctx, types.NamespacedName{Name: longName + "-gateway-cert", Namespace: "default"}, cert)) + + serviceName := util.GetDocumentDBServiceName(ddb.Name) + require.Len(t, serviceName, 63, "test fixture should exercise the truncation path") + + require.Contains(t, cert.Spec.DNSNames, serviceName) + require.Contains(t, cert.Spec.DNSNames, serviceName+".default") + require.Contains(t, cert.Spec.DNSNames, serviceName+".default.svc") + require.NotContains(t, cert.Spec.DNSNames, util.DOCUMENTDB_SERVICE_PREFIX+ddb.Name, + "cert must not carry a SAN for the untruncated service name") +} + func TestReconcileCertificatesDoesNotManagePostgresCertificates(t *testing.T) { ctx := context.Background() ddb := baseDocumentDB("ddb-pg", "default") diff --git a/operator/src/internal/utils/util.go b/operator/src/internal/utils/util.go index 10511696b..798ce79f9 100644 --- a/operator/src/internal/utils/util.go +++ b/operator/src/internal/utils/util.go @@ -28,6 +28,23 @@ import ( dbpreview "github.com/documentdb/documentdb-operator/api/preview" ) +// GetDocumentDBServiceName returns the name of the Service published for a DocumentDB +// instance, truncated to the 63-character RFC 1123 label limit enforced by Kubernetes. +// +// This is the single source of truth for that name. Anything that has to agree with the +// Service name — most importantly the gateway certificate SANs, which must match the +// hostname published in status.connectionString for strict TLS verification to succeed — +// must derive it from here rather than concatenating the prefix and the DocumentDB name +// itself, otherwise long names silently diverge. +func GetDocumentDBServiceName(documentDBName string) string { + serviceName := DOCUMENTDB_SERVICE_PREFIX + documentDBName + if len(serviceName) > 63 { + serviceName = serviceName[:63] + } + // A label may not end in a non-alphanumeric character, which truncation can produce. + return strings.TrimRight(serviceName, "-.") +} + // GetDocumentDBServiceDefinition returns the LoadBalancer Service definition for a given DocumentDB instance func GetDocumentDBServiceDefinition(documentdb *dbpreview.DocumentDB, replicationContext *ReplicationContext, namespace string, serviceType corev1.ServiceType) *corev1.Service { // If no local HA, these two should be empty @@ -41,11 +58,7 @@ func GetDocumentDBServiceDefinition(documentdb *dbpreview.DocumentDB, replicatio } } - // Ensure service name doesn't exceed 63 characters (Kubernetes limit) - serviceName := DOCUMENTDB_SERVICE_PREFIX + documentdb.Name - if len(serviceName) > 63 { - serviceName = serviceName[:63] - } + serviceName := GetDocumentDBServiceName(documentdb.Name) service := &corev1.Service{ ObjectMeta: metav1.ObjectMeta{ @@ -107,16 +120,28 @@ func getEnvironmentSpecificAnnotations(environment string) map[string]string { } } -// EnsureServiceIP ensures that the Service has an IP assigned and returns it, or returns an error if not available +// EnsureServiceIP ensures that the Service has a reachable endpoint and returns +// the address clients should connect to, or an error if it is not available yet. +// +// For ClusterIP services this returns the in-cluster DNS name (..svc) +// rather than the raw ClusterIP. The DNS name resolves from any pod in the cluster, stays +// valid when the Service is recreated with a different ClusterIP, and matches the SANs on +// the gateway certificate, so strict TLS verification succeeds. A raw ClusterIP has no +// matching SAN and fails hostname verification once TLS is trusted. +// +// For LoadBalancer services the externally reachable ingress IP/hostname is returned, +// since cluster-internal DNS is not resolvable by outside clients. +// +// NOTE: the returned value is therefore not always an IP address, despite the name. func EnsureServiceIP(ctx context.Context, service *corev1.Service) (string, error) { if service == nil { return "", fmt.Errorf("service is nil") } - // For ClusterIP services, return the ClusterIP directly + // For ClusterIP services, return the stable in-cluster DNS name if service.Spec.Type == corev1.ServiceTypeClusterIP { if service.Spec.ClusterIP != "" && service.Spec.ClusterIP != "None" { - return service.Spec.ClusterIP, nil + return fmt.Sprintf("%s.%s.svc", service.Name, service.Namespace), nil } return "", fmt.Errorf("ClusterIP not assigned") } diff --git a/operator/src/internal/utils/util_test.go b/operator/src/internal/utils/util_test.go index b0977add2..ab01543e6 100644 --- a/operator/src/internal/utils/util_test.go +++ b/operator/src/internal/utils/util_test.go @@ -696,12 +696,57 @@ func TestGenerateServiceName_PublicFunction(t *testing.T) { } } +func TestGetDocumentDBServiceName(t *testing.T) { + tests := []struct { + name string + documentDBName string + expected string + }{ + { + name: "short name is prefixed verbatim", + documentDBName: "test-db", + expected: "documentdb-service-test-db", + }, + { + name: "name at the limit is not truncated", + documentDBName: strings.Repeat("a", 44), + expected: "documentdb-service-" + strings.Repeat("a", 44), + }, + { + name: "long name is truncated to 63 characters", + documentDBName: strings.Repeat("a", 80), + expected: "documentdb-service-" + strings.Repeat("a", 44), + }, + { + name: "truncation does not leave a trailing hyphen", + documentDBName: strings.Repeat("a", 43) + "-" + strings.Repeat("b", 20), + expected: "documentdb-service-" + strings.Repeat("a", 43), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := GetDocumentDBServiceName(tt.documentDBName) + if result != tt.expected { + t.Errorf("GetDocumentDBServiceName(%q) = %q; expected %q", tt.documentDBName, result, tt.expected) + } + if len(result) > 63 { + t.Errorf("GetDocumentDBServiceName(%q) returned %d characters, exceeding the 63-character limit", tt.documentDBName, len(result)) + } + if strings.HasSuffix(result, "-") || strings.HasSuffix(result, ".") { + t.Errorf("GetDocumentDBServiceName(%q) = %q, which is not a valid RFC 1123 label", tt.documentDBName, result) + } + }) + } +} + func TestEnsureServiceIP(t *testing.T) { tests := []struct { - name string - service *corev1.Service - expectError bool - errorMsg string + name string + service *corev1.Service + expectError bool + errorMsg string + expectedResult string }{ { name: "nil service returns error", @@ -710,14 +755,19 @@ func TestEnsureServiceIP(t *testing.T) { errorMsg: "service is nil", }, { - name: "ClusterIP service with valid IP", + name: "ClusterIP service returns in-cluster DNS name, not the raw IP", service: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "documentdb-service-test-db", + Namespace: "test-namespace", + }, Spec: corev1.ServiceSpec{ Type: corev1.ServiceTypeClusterIP, ClusterIP: "10.0.0.1", }, }, - expectError: false, + expectError: false, + expectedResult: "documentdb-service-test-db.test-namespace.svc", }, { name: "ClusterIP service with None returns error", @@ -741,6 +791,40 @@ func TestEnsureServiceIP(t *testing.T) { expectError: true, errorMsg: "ClusterIP not assigned", }, + { + name: "LoadBalancer service returns external IP", + service: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "documentdb-service-test-db", + Namespace: "test-namespace", + }, + Spec: corev1.ServiceSpec{Type: corev1.ServiceTypeLoadBalancer}, + Status: corev1.ServiceStatus{ + LoadBalancer: corev1.LoadBalancerStatus{ + Ingress: []corev1.LoadBalancerIngress{{IP: "203.0.113.10"}}, + }, + }, + }, + expectError: false, + expectedResult: "203.0.113.10", + }, + { + name: "LoadBalancer service returns external hostname", + service: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "documentdb-service-test-db", + Namespace: "test-namespace", + }, + Spec: corev1.ServiceSpec{Type: corev1.ServiceTypeLoadBalancer}, + Status: corev1.ServiceStatus{ + LoadBalancer: corev1.LoadBalancerStatus{ + Ingress: []corev1.LoadBalancerIngress{{Hostname: "lb.example.com"}}, + }, + }, + }, + expectError: false, + expectedResult: "lb.example.com", + }, } for _, tt := range tests { @@ -758,8 +842,8 @@ func TestEnsureServiceIP(t *testing.T) { if err != nil { t.Errorf("Unexpected error: %v", err) } - if result == "" { - t.Error("Expected non-empty result") + if result != tt.expectedResult { + t.Errorf("EnsureServiceIP() = %q; expected %q", result, tt.expectedResult) } } }) diff --git a/test/e2e/pkg/e2eutils/portforward/portforward.go b/test/e2e/pkg/e2eutils/portforward/portforward.go index f0323dfaa..3cabc2458 100644 --- a/test/e2e/pkg/e2eutils/portforward/portforward.go +++ b/test/e2e/pkg/e2eutils/portforward/portforward.go @@ -23,6 +23,7 @@ import ( "errors" "fmt" "io" + "strings" "github.com/cloudnative-pg/cloudnative-pg/tests/utils/environment" "github.com/cloudnative-pg/cloudnative-pg/tests/utils/forwardconnection" @@ -42,7 +43,9 @@ const GatewayPort = 10260 const ServiceNamePrefix = "documentdb-service-" // GatewayServiceName returns the Service name the operator creates for -// the given DocumentDB CR. +// the given DocumentDB CR. It must stay in sync with +// util.GetDocumentDBServiceName in the operator, including the trailing +// separator trim that keeps a truncated name a valid RFC 1123 label. func GatewayServiceName(dd *previewv1.DocumentDB) string { if dd == nil { return "" @@ -51,7 +54,7 @@ func GatewayServiceName(dd *previewv1.DocumentDB) string { if len(name) > 63 { name = name[:63] } - return name + return strings.TrimRight(name, "-.") } // OpenWithErr establishes a port-forward from localPort on the caller's diff --git a/test/e2e/pkg/e2eutils/portforward/portforward_test.go b/test/e2e/pkg/e2eutils/portforward/portforward_test.go index 8eaa78e71..b57ea6d6b 100644 --- a/test/e2e/pkg/e2eutils/portforward/portforward_test.go +++ b/test/e2e/pkg/e2eutils/portforward/portforward_test.go @@ -30,6 +30,11 @@ func TestGatewayServiceName(t *testing.T) { // 19 (prefix) + 44 xs = 63 "documentdb-service-" + strings.Repeat("x", 44), }, + { + "truncation trims a trailing hyphen", + &previewv1.DocumentDB{ObjectMeta: metav1.ObjectMeta{Name: strings.Repeat("x", 43) + "-" + strings.Repeat("y", 20)}}, + "documentdb-service-" + strings.Repeat("x", 43), + }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) {