diff --git a/internal/controller/gateway_controller.go b/internal/controller/gateway_controller.go index 14288c9e..25333a72 100644 --- a/internal/controller/gateway_controller.go +++ b/internal/controller/gateway_controller.go @@ -1506,6 +1506,43 @@ func managedGatewayHostnameFromStatus( return "" } +// isIPFamilyHostname reports whether hostname is a platform v4./v6. alias of +// the gateway canonical name. Those aliases are programmed as CNAME/ALIAS +// DNSRecordSets by Gateway DNS; ExternalDNS must not also emit A/AAAA for them +// or PowerDNS rejects the CNAME with a 422 conflict. +func isIPFamilyHostname(hostname string) bool { + return strings.HasPrefix(hostname, "v4.") || strings.HasPrefix(hostname, "v6.") +} + +// gatewayDNSEndpointEntries builds ExternalDNS endpoint entries for the apex +// (and any other non-IP-family) hostnames only. v4./v6. aliases are omitted so +// Gateway DNS remains the sole writer for those names. +func gatewayDNSEndpointEntries(hostnames []string, v4IPs, v6IPs []any) []any { + endpoints := []any{} + for _, hostname := range hostnames { + if isIPFamilyHostname(hostname) { + continue + } + if len(v4IPs) > 0 { + endpoints = append(endpoints, map[string]any{ + "dnsName": hostname, + "targets": v4IPs, + "recordType": "A", + "recordTTL": int64(300), + }) + } + if len(v6IPs) > 0 { + endpoints = append(endpoints, map[string]any{ + "dnsName": hostname, + "targets": v6IPs, + "recordType": "AAAA", + "recordTTL": int64(300), + }) + } + } + return endpoints +} + func (r *GatewayReconciler) ensureDownstreamGatewayDNSEndpoints( ctx context.Context, downstreamGateway *gatewayv1.Gateway, @@ -1547,7 +1584,7 @@ func (r *GatewayReconciler) ensureDownstreamGatewayDNSEndpoints( return result } - endpoints := []any{} + endpoints := gatewayDNSEndpointEntries(hostnames, v4IPs, v6IPs) var gatewayDNSEndpoint unstructured.Unstructured gatewayDNSEndpoint.SetGroupVersionKind(schema.GroupVersionKind{ Group: "externaldns.k8s.io", @@ -1557,28 +1594,6 @@ func (r *GatewayReconciler) ensureDownstreamGatewayDNSEndpoints( gatewayDNSEndpoint.SetNamespace(downstreamGateway.Namespace) gatewayDNSEndpoint.SetName(downstreamGateway.Name) - for _, hostname := range hostnames { - if len(v4IPs) > 0 && !strings.HasPrefix(hostname, "v6") { - // v4 specific hostname, or hostname that includes both v4 and v6 - endpoints = append(endpoints, map[string]any{ - "dnsName": hostname, - "targets": v4IPs, - "recordType": "A", - "recordTTL": int64(300), - }) - } - - if len(v6IPs) > 0 && !strings.HasPrefix(hostname, "v4") { - // v6 specific hostname, or hostname that includes both v4 and v6 - endpoints = append(endpoints, map[string]any{ - "dnsName": hostname, - "targets": v6IPs, - "recordType": "AAAA", - "recordTTL": int64(300), - }) - } - } - if _, err := controllerutil.CreateOrUpdate(ctx, downstreamStrategy.GetClient(), &gatewayDNSEndpoint, func() error { if err := controllerutil.SetControllerReference(downstreamGateway, &gatewayDNSEndpoint, downstreamStrategy.GetClient().Scheme()); err != nil { return err diff --git a/internal/controller/gateway_dns_controller.go b/internal/controller/gateway_dns_controller.go index 0d6ca179..4f91bb0c 100644 --- a/internal/controller/gateway_dns_controller.go +++ b/internal/controller/gateway_dns_controller.go @@ -82,12 +82,15 @@ func (r *GatewayReconciler) ensureDNSRecordSets( desiredRecordSetNames := map[string]bool{} for _, hostname := range claimedHostnames { - // Skip the platform-managed canonical hostname – it is handled by external-dns. + // Skip the platform-managed canonical (apex) hostname - ExternalDNS + // programs its A/AAAA via the downstream DNSEndpoint. v4./v6. aliases + // are NOT skipped: Gateway DNS owns those as CNAME/ALIAS records. if hostname == canonicalHostname { continue } hs := networkingv1alpha.HostnameStatus{Hostname: hostname} + recordSetName := dnsRecordSetName(upstreamGateway.Name, hostname) // Get all possible zone names from most specific to least specific. zoneNames := possibleZoneNames(hostname) @@ -190,6 +193,9 @@ func (r *GatewayReconciler) ensureDNSRecordSets( switch { case unverifiedDomain != nil: + // Hostname is still claimed - retain any existing DNSRecordSet so a + // transient verification flap does not GC a record we still want. + desiredRecordSetNames[recordSetName] = true apimeta.SetStatusCondition(&hs.Conditions, metav1.Condition{ Type: networkingv1alpha.HostnameConditionDNSRecordProgrammed, Status: metav1.ConditionFalse, @@ -198,6 +204,9 @@ func (r *GatewayReconciler) ensureDNSRecordSets( ObservedGeneration: upstreamGateway.Generation, }) case noAuthorityDomain != nil: + // Same retain rule for Domain NS flaps (HasDNSAuthority false): pause + // programming without deleting the Gateway's DNSRecordSet. + desiredRecordSetNames[recordSetName] = true msg := fmt.Sprintf("Domain %q is verified but Datum DNS does not have authority", noAuthorityDomain.Name) if noAuthorityZone != nil { if !apimeta.IsStatusConditionTrue(noAuthorityZone.Status.Conditions, conditionTypeAccepted) || @@ -241,11 +250,12 @@ func (r *GatewayReconciler) ensureDNSRecordSets( rrType = dnsv1alpha1.RRTypeALIAS } - recordSetName := dnsRecordSetName(upstreamGateway.Name, hostname) desiredRecordSetNames[recordSetName] = true - // Conflict detection: list existing DNSRecordSets with the same - // hostname annotation in this namespace that reference this zone. + // Conflict detection: any other DNSRecordSet claiming the same FQDN in + // this zone blocks programming - including peers with the same + // managed-by (e.g. a leftover ExternalDNS-shaped record that still + // carries dns.datumapis.com/managed). var existingList dnsv1alpha1.DNSRecordSetList if err := upstreamClient.List(ctx, &existingList, client.InNamespace(upstreamGateway.Namespace), @@ -257,21 +267,22 @@ func (r *GatewayReconciler) ensureDNSRecordSets( for _, existing := range existingList.Items { if existing.Name == recordSetName { - // This is our own record; skip conflict check. continue } if existing.Annotations[annotationDNSHostname] == hostname && - existing.Spec.DNSZoneRef.Name == dnsZone.Name && - existing.Labels[labelManagedBy] != labelManagedByValue { - // Conflict: a record for this hostname exists that we don't own. + existing.Spec.DNSZoneRef.Name == dnsZone.Name { + managedBy := existing.Labels[labelManagedBy] + if managedBy == "" { + managedBy = "unknown" + } conflictMsg := fmt.Sprintf( "Existing DNSRecordSet %q for hostname %q is managed by %q", - existing.Name, hostname, existing.Labels[labelManagedBy], + existing.Name, hostname, managedBy, ) logger.Info("DNS record conflict detected", "hostname", hostname, "conflicting_record", existing.Name, - "managed_by", existing.Labels[labelManagedBy], + "managed_by", managedBy, ) apimeta.SetStatusCondition(&hs.Conditions, metav1.Condition{ Type: networkingv1alpha.HostnameConditionDNSRecordProgrammed, diff --git a/internal/controller/gateway_dns_controller_test.go b/internal/controller/gateway_dns_controller_test.go index 3f2d13ff..f74e81ad 100644 --- a/internal/controller/gateway_dns_controller_test.go +++ b/internal/controller/gateway_dns_controller_test.go @@ -383,7 +383,31 @@ func TestEnsureDNSRecordSets(t *testing.T) { {Hostname: "ns1.otherprovider.com"}, {Hostname: "ns2.otherprovider.com"}, } - return []client.Object{d, newDNSZone(ns, "example-com", "example.com")} + // Pre-existing platform record that must be retained across the + // authority miss (Domain NS flap must not GC wanted CNAMEs). + existingRS := &dnsv1alpha1.DNSRecordSet{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns, + Name: dnsRecordSetName("test-gw", "api.example.com"), + UID: uuid.NewUUID(), + Labels: map[string]string{ + labelManagedBy: labelManagedByValue, + labelDNSManaged: "true", + labelDNSSourceKind: "Gateway", + labelDNSSourceName: "test-gw", + labelDNSSourceNS: ns, + }, + Annotations: map[string]string{ + annotationDNSHostname: "api.example.com", + }, + }, + Spec: dnsv1alpha1.DNSRecordSetSpec{ + DNSZoneRef: corev1.LocalObjectReference{Name: "example-com"}, + RecordType: dnsv1alpha1.RRTypeCNAME, + Records: []dnsv1alpha1.RecordEntry{{Name: "api.example.com."}}, + }, + } + return []client.Object{d, newDNSZone(ns, "example-com", "example.com"), existingRS} }(), assertStatuses: func(t *testing.T, statuses []networkingv1alpha.HostnameStatus) { require.Len(t, statuses, 1) @@ -396,7 +420,8 @@ func TestEnsureDNSRecordSets(t *testing.T) { assertRecords: func(t *testing.T, cl client.Client) { var list dnsv1alpha1.DNSRecordSetList require.NoError(t, cl.List(context.Background(), &list, client.InNamespace(ns))) - assert.Empty(t, list.Items, "no DNSRecordSet should be created when DNS authority is missing") + require.Len(t, list.Items, 1, "existing DNSRecordSet must be retained when DNS authority is missing") + assert.Equal(t, dnsRecordSetName("test-gw", "api.example.com"), list.Items[0].Name) }, }, { @@ -523,6 +548,83 @@ func TestEnsureDNSRecordSets(t *testing.T) { assert.Contains(t, c.Message, "some-other-actor") }, }, + { + name: "conflict with same-FQDN peer even when managed-by matches platform", + claimedHostnames: []string{"api.example.com"}, + upstreamObjects: func() []client.Object { + // Leftover A-record-shaped DNSRecordSet at the same FQDN (e.g. from + // the former ExternalDNS dual-write path) still labeled as managed. + existingRS := &dnsv1alpha1.DNSRecordSet{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns, + Name: "v4-api-example-com-a-leftover", + UID: uuid.NewUUID(), + Labels: map[string]string{ + labelDNSManaged: "true", + labelManagedBy: labelManagedByValue, + }, + Annotations: map[string]string{ + annotationDNSHostname: "api.example.com", + }, + }, + Spec: dnsv1alpha1.DNSRecordSetSpec{ + DNSZoneRef: corev1.LocalObjectReference{Name: "example-com"}, + RecordType: dnsv1alpha1.RRTypeA, + Records: []dnsv1alpha1.RecordEntry{{ + Name: "api.example.com.", + A: &dnsv1alpha1.ARecordSpec{Content: "203.0.113.10"}, + }}, + }, + } + return []client.Object{ + newVerifiedDNSZoneDomain(ns, "example.com", false), + newDNSZone(ns, "example-com", "example.com"), + existingRS, + } + }(), + assertStatuses: func(t *testing.T, statuses []networkingv1alpha.HostnameStatus) { + require.Len(t, statuses, 1) + c := apimeta.FindStatusCondition(statuses[0].Conditions, networkingv1alpha.HostnameConditionDNSRecordProgrammed) + require.NotNil(t, c) + assert.Equal(t, metav1.ConditionFalse, c.Status) + assert.Equal(t, networkingv1alpha.DNSRecordReasonConflict, c.Reason) + assert.Contains(t, c.Message, "v4-api-example-com-a-leftover") + }, + assertRecords: func(t *testing.T, cl client.Client) { + var list dnsv1alpha1.DNSRecordSetList + require.NoError(t, cl.List(context.Background(), &list, client.InNamespace(ns))) + require.Len(t, list.Items, 1, "conflict must not create a second DNSRecordSet") + assert.Equal(t, "v4-api-example-com-a-leftover", list.Items[0].Name) + }, + }, + { + name: "v4 and v6 aliases create CNAME records to the canonical hostname", + claimedHostnames: []string{"v4.11111111111111111111111111111111.gateways.test.local", "v6.11111111111111111111111111111111.gateways.test.local"}, + upstreamObjects: []client.Object{ + newVerifiedDNSZoneDomain(ns, "gateways.test.local", false), + newDNSZone(ns, "gateways-zone", "gateways.test.local"), + }, + assertStatuses: func(t *testing.T, statuses []networkingv1alpha.HostnameStatus) { + require.Len(t, statuses, 2) + for _, hs := range statuses { + c := apimeta.FindStatusCondition(hs.Conditions, networkingv1alpha.HostnameConditionDNSRecordProgrammed) + require.NotNil(t, c) + assert.Equal(t, metav1.ConditionTrue, c.Status) + assert.Equal(t, networkingv1alpha.DNSRecordReasonCreated, c.Reason) + } + }, + assertRecords: func(t *testing.T, cl client.Client) { + var list dnsv1alpha1.DNSRecordSetList + require.NoError(t, cl.List(context.Background(), &list, client.InNamespace(ns))) + require.Len(t, list.Items, 2) + for _, rs := range list.Items { + assert.Equal(t, dnsv1alpha1.RRTypeCNAME, rs.Spec.RecordType) + require.Len(t, rs.Spec.Records, 1) + require.NotNil(t, rs.Spec.Records[0].CNAME) + assert.Equal(t, "11111111111111111111111111111111.gateways.test.local.", rs.Spec.Records[0].CNAME.Content) + } + }, + }, { name: "canonical hostname is skipped (handled by external-dns)", claimedHostnames: []string{}, // empty; we add the canonical hostname below in test setup @@ -607,7 +709,8 @@ func TestEnsureDNSRecordSets(t *testing.T) { s := newDNSTestScheme(t) gw := newTestGatewayForDNS(ns, "test-gw") - if tt.name == "legacy canonical hostname remains canonical when status already set" { + if tt.name == "legacy canonical hostname remains canonical when status already set" || + tt.name == "v4 and v6 aliases create CNAME records to the canonical hostname" { gw.UID = types.UID("11111111-1111-1111-1111-111111111111") gw.Status.Addresses = []gatewayv1.GatewayStatusAddress{ { diff --git a/internal/controller/gateway_dns_endpoint_test.go b/internal/controller/gateway_dns_endpoint_test.go new file mode 100644 index 00000000..ce4162b7 --- /dev/null +++ b/internal/controller/gateway_dns_endpoint_test.go @@ -0,0 +1,53 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package controller + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestIsIPFamilyHostname(t *testing.T) { + assert.True(t, isIPFamilyHostname("v4.example.com")) + assert.True(t, isIPFamilyHostname("v6.example.com")) + assert.False(t, isIPFamilyHostname("example.com")) + assert.False(t, isIPFamilyHostname("v4example.com")) + assert.False(t, isIPFamilyHostname("vv4.example.com")) +} + +func TestGatewayDNSEndpointEntries_OmitsIPFamilyAliases(t *testing.T) { + v4IPs := []any{"203.0.113.10"} + v6IPs := []any{"2001:db8::1"} + hostnames := []string{ + "aabbccddeeff00112233445566778899.gateways.test.local", + "v4.aabbccddeeff00112233445566778899.gateways.test.local", + "v6.aabbccddeeff00112233445566778899.gateways.test.local", + } + + endpoints := gatewayDNSEndpointEntries(hostnames, v4IPs, v6IPs) + require.Len(t, endpoints, 2, "only apex A and AAAA; v4./v6. aliases must not get ExternalDNS endpoints") + + seen := map[string]string{} + for _, ep := range endpoints { + m, ok := ep.(map[string]any) + require.True(t, ok) + name, _ := m["dnsName"].(string) + rtype, _ := m["recordType"].(string) + seen[rtype] = name + assert.Equal(t, hostnames[0], name) + assert.False(t, isIPFamilyHostname(name)) + } + assert.Equal(t, hostnames[0], seen["A"]) + assert.Equal(t, hostnames[0], seen["AAAA"]) +} + +func TestGatewayDNSEndpointEntries_EmptyWhenOnlyIPFamilyHostnames(t *testing.T) { + endpoints := gatewayDNSEndpointEntries( + []string{"v4.example.com", "v6.example.com"}, + []any{"203.0.113.10"}, + []any{"2001:db8::1"}, + ) + assert.Empty(t, endpoints) +} diff --git a/test/e2e/gateway/chainsaw-test.yaml b/test/e2e/gateway/chainsaw-test.yaml index 1253d66d..4b435f90 100644 --- a/test/e2e/gateway/chainsaw-test.yaml +++ b/test/e2e/gateway/chainsaw-test.yaml @@ -407,7 +407,8 @@ spec: value: ($downstreamGateway.status.addresses[?contains(value, ':')].value) # Ensure DNSEndpoint is defined and that targets match the addresses - # provisioned to the downstream gateway. + # provisioned to the downstream gateway. Apex only: v4./v6. aliases are + # owned by Gateway DNS as CNAMEs (see #315), not ExternalDNS A/AAAA. - assert: # timeout: 5s cluster: nso-infra @@ -427,14 +428,6 @@ spec: targets: ($downstreamGatewayV6Addresses) recordType: AAAA recordTTL: 300 - - dnsName: ($v4IPFamilyHostname) - targets: ($downstreamGatewayV4Addresses) - recordType: A - recordTTL: 300 - - dnsName: ($v6IPFamilyHostname) - targets: ($downstreamGatewayV6Addresses) - recordType: AAAA - recordTTL: 300 # Ensure the upstream gateway's status is updated as expected - assert: