From 1d3cff55e309e85d71f87356156405ef3b950885 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Mon, 12 Oct 2026 12:09:27 -0500 Subject: [PATCH 01/19] Configure private DNS using resource names and registration labels --- api/v1alpha1/internaldns_project_types.go | 63 +++--- api/v1alpha1/internaldns_types_test.go | 7 +- api/v1alpha1/zz_generated.deepcopy.go | 33 ++- ...orking.miloapis.com_dnsnamingpolicies.yaml | 207 ++++++++++-------- ...king.miloapis.com_dnszoneassociations.yaml | 71 +----- .../internal-dns/examples/private-zone.yaml | 38 ++++ internal/internaldns/admission/handler.go | 43 +++- .../internaldns/admission/handler_test.go | 96 ++++++++ .../access_recovery_envtest_test.go | 3 + internal/internaldns/controlplane/compiler.go | 5 +- .../internaldns/controlplane/compiler_test.go | 2 +- .../controlplane/context_reconciler.go | 148 +++++++------ .../controlplane/context_reconciler_test.go | 9 +- .../controlplane/managed_domain_test.go | 6 +- .../controlplane/named_reference_api_test.go | 62 ++++++ .../controlplane/named_reference_test.go | 132 +++++++++++ internal/internaldns/controlplane/naming.go | 25 ++- .../controlplane/naming_selector_test.go | 139 ++++++++++++ .../internaldns/controlplane/naming_test.go | 4 +- .../publication_reconciler_test.go | 8 +- .../runtime/deployment_permissions_test.go | 8 +- internal/internaldns/serving/verify_test.go | 15 +- test/internaldns/kubernetes/qualify/main.go | 2 +- 23 files changed, 825 insertions(+), 301 deletions(-) create mode 100644 config/internal-dns/examples/private-zone.yaml create mode 100644 internal/internaldns/controlplane/named_reference_api_test.go create mode 100644 internal/internaldns/controlplane/named_reference_test.go create mode 100644 internal/internaldns/controlplane/naming_selector_test.go diff --git a/api/v1alpha1/internaldns_project_types.go b/api/v1alpha1/internaldns_project_types.go index 97685c5b..c5586283 100644 --- a/api/v1alpha1/internaldns_project_types.go +++ b/api/v1alpha1/internaldns_project_types.go @@ -22,15 +22,19 @@ type DNSObjectReference struct { Generation int64 `json:"generation,omitempty"` } -// +kubebuilder:validation:XValidation:rule="(has(self.vpcRef) && has(self.vpcRef.name) && self.vpcRef.name.size() > 0 && has(self.vpcRef.uid) && self.vpcRef.uid.size() > 0) != (has(self.resolverContextRef) && has(self.resolverContextRef.name) && self.resolverContextRef.name.size() > 0 && has(self.resolverContextRef.uid) && self.resolverContextRef.uid.size() > 0)",message="exactly one UID-pinned vpcRef or resolverContextRef is required" -// +kubebuilder:validation:XValidation:rule="!has(self.vpcRef) || ((self.vpcRef.name.size() == 0 && !has(self.vpcRef.uid) && (!has(self.vpcRef.generation) || self.vpcRef.generation == 0)) || (self.vpcRef.name.size() > 0 && has(self.vpcRef.uid) && self.vpcRef.uid.size() > 0))",message="a nonempty vpcRef must pin name and UID" -// +kubebuilder:validation:XValidation:rule="!has(self.resolverContextRef) || ((self.resolverContextRef.name.size() == 0 && !has(self.resolverContextRef.uid) && (!has(self.resolverContextRef.generation) || self.resolverContextRef.generation == 0)) || (self.resolverContextRef.name.size() > 0 && has(self.resolverContextRef.uid) && self.resolverContextRef.uid.size() > 0))",message="a nonempty resolverContextRef must pin name and UID" -// +kubebuilder:validation:XValidation:rule="has(self.dnsZoneRef.uid) && self.dnsZoneRef.uid.size() > 0",message="dnsZoneRef must pin the zone UID" +// DNSNameReference selects a resource in the same project namespace. +// DNS records its resolved lifetime in controller-owned status. +type DNSNameReference struct { + Name string `json:"name"` +} + +// +kubebuilder:validation:XValidation:rule="(has(self.vpcRef) && has(self.vpcRef.name) && self.vpcRef.name.size() > 0) != (has(self.resolverContextRef) && has(self.resolverContextRef.name) && self.resolverContextRef.name.size() > 0)",message="exactly one named vpcRef or resolverContextRef is required" +// +kubebuilder:validation:XValidation:rule="self.dnsZoneRef.name.size() > 0",message="dnsZoneRef requires a name" type DNSZoneAssociationSpec struct { - DNSZoneRef DNSObjectReference `json:"dnsZoneRef"` - VPCRef DNSObjectReference `json:"vpcRef,omitempty"` - ResolverContextRef DNSObjectReference `json:"resolverContextRef,omitempty"` - Managed bool `json:"managed,omitempty"` + DNSZoneRef DNSNameReference `json:"dnsZoneRef"` + VPCRef DNSNameReference `json:"vpcRef,omitempty"` + ResolverContextRef DNSNameReference `json:"resolverContextRef,omitempty"` + Managed bool `json:"managed,omitempty"` } type DNSZoneAssociationStatus struct { @@ -323,36 +327,23 @@ type DNSRecordContributionList struct { Items []DNSRecordContribution `json:"items"` } -// +kubebuilder:validation:XValidation:rule="(has(self.vpcRef) && has(self.vpcRef.name) && self.vpcRef.name.size() > 0 && has(self.vpcRef.uid) && self.vpcRef.uid.size() > 0) != (has(self.resolverContextRef) && has(self.resolverContextRef.name) && self.resolverContextRef.name.size() > 0 && has(self.resolverContextRef.uid) && self.resolverContextRef.uid.size() > 0)",message="exactly one UID-pinned vpcRef or resolverContextRef is required" -// +kubebuilder:validation:XValidation:rule="!has(self.vpcRef) || ((self.vpcRef.name.size() == 0 && !has(self.vpcRef.uid) && (!has(self.vpcRef.generation) || self.vpcRef.generation == 0)) || (self.vpcRef.name.size() > 0 && has(self.vpcRef.uid) && self.vpcRef.uid.size() > 0))",message="a nonempty vpcRef must pin name and UID" -// +kubebuilder:validation:XValidation:rule="!has(self.resolverContextRef) || ((self.resolverContextRef.name.size() == 0 && !has(self.resolverContextRef.uid) && (!has(self.resolverContextRef.generation) || self.resolverContextRef.generation == 0)) || (self.resolverContextRef.name.size() > 0 && has(self.resolverContextRef.uid) && self.resolverContextRef.uid.size() > 0))",message="a nonempty resolverContextRef must pin name and UID" -// +kubebuilder:validation:XValidation:rule="!has(self.dnsZoneRef) || self.dnsZoneRef.name.size() == 0 || (has(self.dnsZoneRef.uid) && self.dnsZoneRef.uid.size() > 0)",message="a selected dnsZoneRef must pin the zone UID" +// +kubebuilder:validation:XValidation:rule="(has(self.vpcRef) && has(self.vpcRef.name) && self.vpcRef.name.size() > 0) != (has(self.resolverContextRef) && has(self.resolverContextRef.name) && self.resolverContextRef.name.size() > 0)",message="exactly one named vpcRef or resolverContextRef is required" type DNSNamingPolicySpec struct { - VPCRef DNSObjectReference `json:"vpcRef,omitempty"` - ResolverContextRef DNSObjectReference `json:"resolverContextRef,omitempty"` + VPCRef DNSNameReference `json:"vpcRef,omitempty"` + ResolverContextRef DNSNameReference `json:"resolverContextRef,omitempty"` AdditionalNames []DNSAdditionalNameRule `json:"additionalNames"` - // Legacy single-rule fields are retained during the v1alpha1 transition. - DNSZoneRef DNSObjectReference `json:"dnsZoneRef,omitempty"` - Product string `json:"product,omitempty"` - ScopePrefix string `json:"scopePrefix,omitempty"` - Priority int32 `json:"priority,omitempty"` + DNSZoneRef DNSNameReference `json:"dnsZoneRef,omitempty"` + ScopePrefix string `json:"scopePrefix,omitempty"` + Priority int32 `json:"priority,omitempty"` } -// +kubebuilder:validation:Enum=InstanceIdentity;ServiceDiscovery;ServiceVIP;ServiceExport -type DNSRegistrationClass string - -const ( - DNSRegistrationClassInstanceIdentity DNSRegistrationClass = "InstanceIdentity" - DNSRegistrationClassServiceDiscovery DNSRegistrationClass = "ServiceDiscovery" - DNSRegistrationClassServiceVIP DNSRegistrationClass = "ServiceVIP" - DNSRegistrationClassServiceExport DNSRegistrationClass = "ServiceExport" -) - -// +kubebuilder:validation:XValidation:rule="has(self.dnsZoneRef.uid) && self.dnsZoneRef.uid.size() > 0",message="dnsZoneRef must pin the zone UID" +// +kubebuilder:validation:XValidation:rule="self.dnsZoneRef.name.size() > 0",message="dnsZoneRef requires a name" type DNSAdditionalNameRule struct { - RegistrationClass DNSRegistrationClass `json:"registrationClass"` - DNSZoneRef DNSObjectReference `json:"dnsZoneRef"` - NamePrefix string `json:"namePrefix"` + // Selector matches DNSRegistration metadata labels within this DNS context. + // An omitted or empty selector matches every registration in the context. + Selector *metav1.LabelSelector `json:"selector,omitempty"` + DNSZoneRef DNSNameReference `json:"dnsZoneRef"` + NamePrefix string `json:"namePrefix"` } type DNSNamingPolicyStatus struct { @@ -364,9 +355,9 @@ type DNSNamingPolicyStatus struct { } type DNSResolvedAdditionalNameRule struct { - RegistrationClass DNSRegistrationClass `json:"registrationClass"` - DNSZoneRef DNSObjectReference `json:"dnsZoneRef"` - NamePrefix string `json:"namePrefix"` + Selector *metav1.LabelSelector `json:"selector,omitempty"` + DNSZoneRef DNSObjectReference `json:"dnsZoneRef"` + NamePrefix string `json:"namePrefix"` } // +kubebuilder:object:root=true diff --git a/api/v1alpha1/internaldns_types_test.go b/api/v1alpha1/internaldns_types_test.go index 0929a7fc..21140890 100644 --- a/api/v1alpha1/internaldns_types_test.go +++ b/api/v1alpha1/internaldns_types_test.go @@ -6,12 +6,11 @@ import ( ) // Value-typed references with omitempty encode their unselected peer as {}. -// The CRD CEL deliberately tests a complete name/UID pin, rather than mere -// property presence, so this normal Go client representation remains valid. +// The CRD CEL tests a nonempty selected name so Go clients can omit its peer. func TestConsumerSelectorJSONUsesEmptyUnselectedReference(t *testing.T) { for name, value := range map[string]any{ - "association context": DNSZoneAssociationSpec{ResolverContextRef: DNSObjectReference{Name: "ctx", UID: "ctx-uid"}}, - "naming legacy": DNSNamingPolicySpec{VPCRef: DNSObjectReference{Name: "vpc", UID: "vpc-uid"}}, + "association context": DNSZoneAssociationSpec{ResolverContextRef: DNSNameReference{Name: "ctx"}}, + "naming legacy": DNSNamingPolicySpec{VPCRef: DNSNameReference{Name: "vpc"}}, } { t.Run(name, func(t *testing.T) { data, err := json.Marshal(value) diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index aed412fe..2b6b6624 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -91,6 +91,11 @@ func (in *CNAMERecordSpec) DeepCopy() *CNAMERecordSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *DNSAdditionalNameRule) DeepCopyInto(out *DNSAdditionalNameRule) { *out = *in + if in.Selector != nil { + in, out := &in.Selector, &out.Selector + *out = new(v1.LabelSelector) + (*in).DeepCopyInto(*out) + } out.DNSZoneRef = in.DNSZoneRef } @@ -369,6 +374,21 @@ func (in *DNSManagedNamespaceStatus) DeepCopy() *DNSManagedNamespaceStatus { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *DNSNameReference) DeepCopyInto(out *DNSNameReference) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new DNSNameReference. +func (in *DNSNameReference) DeepCopy() *DNSNameReference { + if in == nil { + return nil + } + out := new(DNSNameReference) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *DNSNamingPolicy) DeepCopyInto(out *DNSNamingPolicy) { *out = *in @@ -436,7 +456,9 @@ func (in *DNSNamingPolicySpec) DeepCopyInto(out *DNSNamingPolicySpec) { if in.AdditionalNames != nil { in, out := &in.AdditionalNames, &out.AdditionalNames *out = make([]DNSAdditionalNameRule, len(*in)) - copy(*out, *in) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } } out.DNSZoneRef = in.DNSZoneRef } @@ -460,7 +482,9 @@ func (in *DNSNamingPolicyStatus) DeepCopyInto(out *DNSNamingPolicyStatus) { if in.ResolvedAdditionalNames != nil { in, out := &in.ResolvedAdditionalNames, &out.ResolvedAdditionalNames *out = make([]DNSResolvedAdditionalNameRule, len(*in)) - copy(*out, *in) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } } if in.Conditions != nil { in, out := &in.Conditions, &out.Conditions @@ -1174,6 +1198,11 @@ func (in *DNSRegistrationStatus) DeepCopy() *DNSRegistrationStatus { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *DNSResolvedAdditionalNameRule) DeepCopyInto(out *DNSResolvedAdditionalNameRule) { *out = *in + if in.Selector != nil { + in, out := &in.Selector, &out.Selector + *out = new(v1.LabelSelector) + (*in).DeepCopyInto(*out) + } out.DNSZoneRef = in.DNSZoneRef } diff --git a/config/crd/bases/dns.networking.miloapis.com_dnsnamingpolicies.yaml b/config/crd/bases/dns.networking.miloapis.com_dnsnamingpolicies.yaml index c5df3cd2..3d508e60 100644 --- a/config/crd/bases/dns.networking.miloapis.com_dnsnamingpolicies.yaml +++ b/config/crd/bases/dns.networking.miloapis.com_dnsnamingpolicies.yaml @@ -42,62 +42,78 @@ spec: properties: dnsZoneRef: description: |- - DNSObjectReference pins a namespaced object by API-assigned UID. Generation - is used where a writer must also be fenced against policy changes. + DNSNameReference selects a resource in the same project namespace. + DNS records its resolved lifetime in controller-owned status. properties: - generation: - format: int64 - minimum: 0 - type: integer name: - maxLength: 253 - type: string - uid: - description: |- - UID is a type that holds unique ID values, including UUIDs. Because we - don't ONLY use UUIDs, this is an alias to string. Being a type captures - intent and helps make sure that UIDs and names do not get conflated. - maxLength: 128 type: string required: - name type: object namePrefix: type: string - registrationClass: - enum: - - InstanceIdentity - - ServiceDiscovery - - ServiceVIP - - ServiceExport - type: string + selector: + description: |- + Selector matches DNSRegistration metadata labels within this DNS context. + An omitted or empty selector matches every registration in the context. + properties: + matchExpressions: + description: matchExpressions is a list of label selector + requirements. The requirements are ANDed. + items: + description: |- + A label selector requirement is a selector that contains values, a key, and an operator that + relates the key and values. + properties: + key: + description: key is the label key that the selector + applies to. + type: string + operator: + description: |- + operator represents a key's relationship to a set of values. + Valid operators are In, NotIn, Exists and DoesNotExist. + type: string + values: + description: |- + values is an array of string values. If the operator is In or NotIn, + the values array must be non-empty. If the operator is Exists or DoesNotExist, + the values array must be empty. This array is replaced during a strategic + merge patch. + items: + type: string + type: array + x-kubernetes-list-type: atomic + required: + - key + - operator + type: object + type: array + x-kubernetes-list-type: atomic + matchLabels: + additionalProperties: + type: string + description: |- + matchLabels is a map of {key,value} pairs. A single {key,value} in the matchLabels + map is equivalent to an element of matchExpressions, whose key field is "key", the + operator is "In", and the values array contains only "value". The requirements are ANDed. + type: object + type: object + x-kubernetes-map-type: atomic required: - dnsZoneRef - namePrefix - - registrationClass type: object x-kubernetes-validations: - - message: dnsZoneRef must pin the zone UID - rule: has(self.dnsZoneRef.uid) && self.dnsZoneRef.uid.size() > - 0 + - message: dnsZoneRef requires a name + rule: self.dnsZoneRef.name.size() > 0 type: array dnsZoneRef: - description: Legacy single-rule fields are retained during the v1alpha1 - transition. + description: |- + DNSNameReference selects a resource in the same project namespace. + DNS records its resolved lifetime in controller-owned status. properties: - generation: - format: int64 - minimum: 0 - type: integer name: - maxLength: 253 - type: string - uid: - description: |- - UID is a type that holds unique ID values, including UUIDs. Because we - don't ONLY use UUIDs, this is an alias to string. Being a type captures - intent and helps make sure that UIDs and names do not get conflated. - maxLength: 128 type: string required: - name @@ -105,26 +121,12 @@ spec: priority: format: int32 type: integer - product: - type: string resolverContextRef: description: |- - DNSObjectReference pins a namespaced object by API-assigned UID. Generation - is used where a writer must also be fenced against policy changes. + DNSNameReference selects a resource in the same project namespace. + DNS records its resolved lifetime in controller-owned status. properties: - generation: - format: int64 - minimum: 0 - type: integer name: - maxLength: 253 - type: string - uid: - description: |- - UID is a type that holds unique ID values, including UUIDs. Because we - don't ONLY use UUIDs, this is an alias to string. Being a type captures - intent and helps make sure that UIDs and names do not get conflated. - maxLength: 128 type: string required: - name @@ -133,22 +135,10 @@ spec: type: string vpcRef: description: |- - DNSObjectReference pins a namespaced object by API-assigned UID. Generation - is used where a writer must also be fenced against policy changes. + DNSNameReference selects a resource in the same project namespace. + DNS records its resolved lifetime in controller-owned status. properties: - generation: - format: int64 - minimum: 0 - type: integer name: - maxLength: 253 - type: string - uid: - description: |- - UID is a type that holds unique ID values, including UUIDs. Because we - don't ONLY use UUIDs, this is an alias to string. Being a type captures - intent and helps make sure that UIDs and names do not get conflated. - maxLength: 128 type: string required: - name @@ -157,26 +147,10 @@ spec: - additionalNames type: object x-kubernetes-validations: - - message: exactly one UID-pinned vpcRef or resolverContextRef is required + - message: exactly one named vpcRef or resolverContextRef is required rule: (has(self.vpcRef) && has(self.vpcRef.name) && self.vpcRef.name.size() - > 0 && has(self.vpcRef.uid) && self.vpcRef.uid.size() > 0) != (has(self.resolverContextRef) - && has(self.resolverContextRef.name) && self.resolverContextRef.name.size() - > 0 && has(self.resolverContextRef.uid) && self.resolverContextRef.uid.size() - > 0) - - message: a nonempty vpcRef must pin name and UID - rule: '!has(self.vpcRef) || ((self.vpcRef.name.size() == 0 && !has(self.vpcRef.uid) - && (!has(self.vpcRef.generation) || self.vpcRef.generation == 0)) - || (self.vpcRef.name.size() > 0 && has(self.vpcRef.uid) && self.vpcRef.uid.size() - > 0))' - - message: a nonempty resolverContextRef must pin name and UID - rule: '!has(self.resolverContextRef) || ((self.resolverContextRef.name.size() - == 0 && !has(self.resolverContextRef.uid) && (!has(self.resolverContextRef.generation) - || self.resolverContextRef.generation == 0)) || (self.resolverContextRef.name.size() - > 0 && has(self.resolverContextRef.uid) && self.resolverContextRef.uid.size() - > 0))' - - message: a selected dnsZoneRef must pin the zone UID - rule: '!has(self.dnsZoneRef) || self.dnsZoneRef.name.size() == 0 || - (has(self.dnsZoneRef.uid) && self.dnsZoneRef.uid.size() > 0)' + > 0) != (has(self.resolverContextRef) && has(self.resolverContextRef.name) + && self.resolverContextRef.name.size() > 0) status: properties: conditions: @@ -262,17 +236,58 @@ spec: type: object namePrefix: type: string - registrationClass: - enum: - - InstanceIdentity - - ServiceDiscovery - - ServiceVIP - - ServiceExport - type: string + selector: + description: |- + A label selector is a label query over a set of resources. The result of matchLabels and + matchExpressions are ANDed. An empty label selector matches all objects. A null + label selector matches no objects. + properties: + matchExpressions: + description: matchExpressions is a list of label selector + requirements. The requirements are ANDed. + items: + description: |- + A label selector requirement is a selector that contains values, a key, and an operator that + relates the key and values. + properties: + key: + description: key is the label key that the selector + applies to. + type: string + operator: + description: |- + operator represents a key's relationship to a set of values. + Valid operators are In, NotIn, Exists and DoesNotExist. + type: string + values: + description: |- + values is an array of string values. If the operator is In or NotIn, + the values array must be non-empty. If the operator is Exists or DoesNotExist, + the values array must be empty. This array is replaced during a strategic + merge patch. + items: + type: string + type: array + x-kubernetes-list-type: atomic + required: + - key + - operator + type: object + type: array + x-kubernetes-list-type: atomic + matchLabels: + additionalProperties: + type: string + description: |- + matchLabels is a map of {key,value} pairs. A single {key,value} in the matchLabels + map is equivalent to an element of matchExpressions, whose key field is "key", the + operator is "In", and the values array contains only "value". The requirements are ANDed. + type: object + type: object + x-kubernetes-map-type: atomic required: - dnsZoneRef - namePrefix - - registrationClass type: object type: array resolvedDNSZoneRef: diff --git a/config/crd/bases/dns.networking.miloapis.com_dnszoneassociations.yaml b/config/crd/bases/dns.networking.miloapis.com_dnszoneassociations.yaml index 8cbd7304..29aaf416 100644 --- a/config/crd/bases/dns.networking.miloapis.com_dnszoneassociations.yaml +++ b/config/crd/bases/dns.networking.miloapis.com_dnszoneassociations.yaml @@ -40,22 +40,10 @@ spec: properties: dnsZoneRef: description: |- - DNSObjectReference pins a namespaced object by API-assigned UID. Generation - is used where a writer must also be fenced against policy changes. + DNSNameReference selects a resource in the same project namespace. + DNS records its resolved lifetime in controller-owned status. properties: - generation: - format: int64 - minimum: 0 - type: integer name: - maxLength: 253 - type: string - uid: - description: |- - UID is a type that holds unique ID values, including UUIDs. Because we - don't ONLY use UUIDs, this is an alias to string. Being a type captures - intent and helps make sure that UIDs and names do not get conflated. - maxLength: 128 type: string required: - name @@ -64,44 +52,20 @@ spec: type: boolean resolverContextRef: description: |- - DNSObjectReference pins a namespaced object by API-assigned UID. Generation - is used where a writer must also be fenced against policy changes. + DNSNameReference selects a resource in the same project namespace. + DNS records its resolved lifetime in controller-owned status. properties: - generation: - format: int64 - minimum: 0 - type: integer name: - maxLength: 253 - type: string - uid: - description: |- - UID is a type that holds unique ID values, including UUIDs. Because we - don't ONLY use UUIDs, this is an alias to string. Being a type captures - intent and helps make sure that UIDs and names do not get conflated. - maxLength: 128 type: string required: - name type: object vpcRef: description: |- - DNSObjectReference pins a namespaced object by API-assigned UID. Generation - is used where a writer must also be fenced against policy changes. + DNSNameReference selects a resource in the same project namespace. + DNS records its resolved lifetime in controller-owned status. properties: - generation: - format: int64 - minimum: 0 - type: integer name: - maxLength: 253 - type: string - uid: - description: |- - UID is a type that holds unique ID values, including UUIDs. Because we - don't ONLY use UUIDs, this is an alias to string. Being a type captures - intent and helps make sure that UIDs and names do not get conflated. - maxLength: 128 type: string required: - name @@ -110,25 +74,12 @@ spec: - dnsZoneRef type: object x-kubernetes-validations: - - message: exactly one UID-pinned vpcRef or resolverContextRef is required + - message: exactly one named vpcRef or resolverContextRef is required rule: (has(self.vpcRef) && has(self.vpcRef.name) && self.vpcRef.name.size() - > 0 && has(self.vpcRef.uid) && self.vpcRef.uid.size() > 0) != (has(self.resolverContextRef) - && has(self.resolverContextRef.name) && self.resolverContextRef.name.size() - > 0 && has(self.resolverContextRef.uid) && self.resolverContextRef.uid.size() - > 0) - - message: a nonempty vpcRef must pin name and UID - rule: '!has(self.vpcRef) || ((self.vpcRef.name.size() == 0 && !has(self.vpcRef.uid) - && (!has(self.vpcRef.generation) || self.vpcRef.generation == 0)) - || (self.vpcRef.name.size() > 0 && has(self.vpcRef.uid) && self.vpcRef.uid.size() - > 0))' - - message: a nonempty resolverContextRef must pin name and UID - rule: '!has(self.resolverContextRef) || ((self.resolverContextRef.name.size() - == 0 && !has(self.resolverContextRef.uid) && (!has(self.resolverContextRef.generation) - || self.resolverContextRef.generation == 0)) || (self.resolverContextRef.name.size() - > 0 && has(self.resolverContextRef.uid) && self.resolverContextRef.uid.size() - > 0))' - - message: dnsZoneRef must pin the zone UID - rule: has(self.dnsZoneRef.uid) && self.dnsZoneRef.uid.size() > 0 + > 0) != (has(self.resolverContextRef) && has(self.resolverContextRef.name) + && self.resolverContextRef.name.size() > 0) + - message: dnsZoneRef requires a name + rule: self.dnsZoneRef.name.size() > 0 status: properties: bindingRef: diff --git a/config/internal-dns/examples/private-zone.yaml b/config/internal-dns/examples/private-zone.yaml new file mode 100644 index 00000000..b20e20ec --- /dev/null +++ b/config/internal-dns/examples/private-zone.yaml @@ -0,0 +1,38 @@ +apiVersion: dns.networking.miloapis.com/v1alpha1 +kind: DNSZone +metadata: + name: production + namespace: project +spec: + visibility: Private + domainName: prod.internal + dnsZoneClassName: datum-internal-dns +--- +apiVersion: dns.networking.miloapis.com/v1alpha1 +kind: DNSZoneAssociation +metadata: + name: application-production + namespace: project +spec: + # Select resources by name within this project namespace. + dnsZoneRef: + name: production + resolverContextRef: + name: application-network +--- +apiVersion: dns.networking.miloapis.com/v1alpha1 +kind: DNSNamingPolicy +metadata: + name: application-production-names + namespace: project +spec: + resolverContextRef: + name: application-network + additionalNames: + # Match registration labels supplied by any authorized publisher. + - selector: + matchLabels: + app: storefront + dnsZoneRef: + name: production + namePrefix: storefront diff --git a/internal/internaldns/admission/handler.go b/internal/internaldns/admission/handler.go index 5df7263c..bc8ea6d8 100644 --- a/internal/internaldns/admission/handler.go +++ b/internal/internaldns/admission/handler.go @@ -258,15 +258,19 @@ func (h *Handler) association(ctx context.Context, cl client.Client, req kubeadm if err := decode(req, &a); err != nil { return err } - if _, err := currentPrivateZone(ctx, cl, req.Namespace, a.Spec.DNSZoneRef); err != nil { + if err := validateNamedPrivateZone(ctx, cl, req.Namespace, a.Spec.DNSZoneRef); err != nil { return err } if err := validateConsumerSelector(a.Spec.VPCRef, a.Spec.ResolverContextRef, h.ResolverContextsEnabled); err != nil { return err } if h.ResolverContextsEnabled { - if a.Spec.ResolverContextRef.UID == "" { - return fmt.Errorf("association must pin resolver context UID") + var resolverContext dnsv1alpha1.DNSResolverContext + if err := cl.Get(ctx, client.ObjectKey{Namespace: req.Namespace, Name: a.Spec.ResolverContextRef.Name}, &resolverContext); err != nil { + return fmt.Errorf("resolver context lookup: %w", err) + } + if !resolverContext.DeletionTimestamp.IsZero() { + return fmt.Errorf("resolver context is being deleted") } return nil } @@ -282,25 +286,25 @@ func (h *Handler) namingPolicy(ctx context.Context, cl client.Client, req kubead return err } if p.Spec.DNSZoneRef.Name != "" { - if _, err := currentPrivateZone(ctx, cl, req.Namespace, p.Spec.DNSZoneRef); err != nil { + if err := validateNamedPrivateZone(ctx, cl, req.Namespace, p.Spec.DNSZoneRef); err != nil { return err } } for _, rule := range p.Spec.AdditionalNames { - if _, err := currentPrivateZone(ctx, cl, req.Namespace, rule.DNSZoneRef); err != nil { + if _, err := metav1.LabelSelectorAsSelector(rule.Selector); err != nil { + return fmt.Errorf("invalid naming selector: %w", err) + } + if err := validateNamedPrivateZone(ctx, cl, req.Namespace, rule.DNSZoneRef); err != nil { return err } } if h.ResolverContextsEnabled { - if p.Spec.ResolverContextRef.Name == "" || p.Spec.ResolverContextRef.UID == "" { - return fmt.Errorf("naming policy must pin resolver context name and UID") - } var resolverContext dnsv1alpha1.DNSResolverContext if err := cl.Get(ctx, client.ObjectKey{Namespace: req.Namespace, Name: p.Spec.ResolverContextRef.Name}, &resolverContext); err != nil { return fmt.Errorf("resolver context lookup: %w", err) } ready := apimeta.FindStatusCondition(resolverContext.Status.Conditions, "Ready") - if resolverContext.UID != p.Spec.ResolverContextRef.UID || !resolverContext.DeletionTimestamp.IsZero() || ready == nil || ready.Status != metav1.ConditionTrue || ready.ObservedGeneration != resolverContext.Generation { + if !resolverContext.DeletionTimestamp.IsZero() || ready == nil || ready.Status != metav1.ConditionTrue || ready.ObservedGeneration != resolverContext.Generation { return fmt.Errorf("resolver context lifetime is not ready") } return nil @@ -308,9 +312,10 @@ func (h *Handler) namingPolicy(ctx context.Context, cl client.Client, req kubead return nil } -func validateConsumerSelector(vpcRef, contextRef dnsv1alpha1.DNSObjectReference, contextMode bool) error { - hasVPC := vpcRef.Name != "" || vpcRef.UID != "" || vpcRef.Generation != 0 - hasContext := contextRef.Name != "" || contextRef.UID != "" || contextRef.Generation != 0 +func validateConsumerSelector(vpcRef, contextRef dnsv1alpha1.DNSNameReference, contextMode bool) error { + hasVPC := vpcRef.Name != "" + hasContext := contextRef.Name != "" + if contextMode && !hasContext { return fmt.Errorf("resolver context mode requires resolverContextRef") } @@ -320,6 +325,20 @@ func validateConsumerSelector(vpcRef, contextRef dnsv1alpha1.DNSObjectReference, return nil } +func validateNamedPrivateZone(ctx context.Context, cl client.Client, namespace string, ref dnsv1alpha1.DNSNameReference) error { + if ref.Name == "" { + return fmt.Errorf("zone reference requires a name") + } + var zone dnsv1alpha1.DNSZone + if err := cl.Get(ctx, client.ObjectKey{Namespace: namespace, Name: ref.Name}, &zone); err != nil { + return fmt.Errorf("private zone lookup: %w", err) + } + if !zone.DeletionTimestamp.IsZero() || zone.Spec.Visibility != dnsv1alpha1.DNSZoneVisibilityPrivate { + return fmt.Errorf("internal references require a live private zone") + } + return nil +} + func decode(req kubeadmission.Request, obj any) error { data := req.Object.Raw if req.Operation == admissionv1.Delete { diff --git a/internal/internaldns/admission/handler_test.go b/internal/internaldns/admission/handler_test.go index 02996320..af0da3a5 100644 --- a/internal/internaldns/admission/handler_test.go +++ b/internal/internaldns/admission/handler_test.go @@ -5,17 +5,21 @@ package admission import ( "context" "encoding/json" + "strings" "testing" "time" dnsv1alpha1 "go.miloapis.com/dns-operator/api/v1alpha1" admissionv1 "k8s.io/api/admission/v1" authenticationv1 "k8s.io/api/authentication/v1" + authorizationv1 "k8s.io/api/authorization/v1" apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" kubeadmission "sigs.k8s.io/controller-runtime/pkg/webhook/admission" ) @@ -197,6 +201,98 @@ func TestResolverAccessRequiresCurrentContextAndBoundedLease(t *testing.T) { } } +func TestConsumerSelectorUsesNamesAndRejectsAmbiguity(t *testing.T) { + vpc := dnsv1alpha1.DNSNameReference{Name: "vpc"} + resolverContext := dnsv1alpha1.DNSNameReference{Name: "ctx"} + for _, contextMode := range []bool{false, true} { + for _, tc := range []struct { + name string + vpc, context dnsv1alpha1.DNSNameReference + }{{"empty", dnsv1alpha1.DNSNameReference{}, dnsv1alpha1.DNSNameReference{}}, {"both", vpc, resolverContext}} { + t.Run(fmt.Sprintf("context=%v/%s", contextMode, tc.name), func(t *testing.T) { + if err := validateConsumerSelector(tc.vpc, tc.context, contextMode); err == nil { + t.Fatal("ambiguous selector accepted") + } + }) + } + } + if err := validateConsumerSelector(vpc, dnsv1alpha1.DNSNameReference{}, false); err != nil { + t.Fatalf("network selector rejected: %v", err) + } + if err := validateConsumerSelector(dnsv1alpha1.DNSNameReference{}, resolverContext, true); err != nil { + t.Fatalf("context selector rejected: %v", err) + } + if validateConsumerSelector(vpc, dnsv1alpha1.DNSNameReference{}, true) == nil || validateConsumerSelector(dnsv1alpha1.DNSNameReference{}, resolverContext, false) == nil { + t.Fatal("selector accepted in the wrong integration mode") + } +} + +func TestCustomerNamedReferencesRequireZoneAndContextAuthorization(t *testing.T) { + scheme := runtime.NewScheme() + if err := dnsv1alpha1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + zone := &dnsv1alpha1.DNSZone{ObjectMeta: metav1.ObjectMeta{Name: "production", Namespace: "project", UID: "zone"}, Spec: dnsv1alpha1.DNSZoneSpec{Visibility: dnsv1alpha1.DNSZoneVisibilityPrivate}} + resolver := &dnsv1alpha1.DNSResolverContext{ObjectMeta: metav1.ObjectMeta{Name: "application", Namespace: "project", UID: "context", Generation: 1}} + setAdmissionCondition(&resolver.Status.Conditions, "Ready", metav1.ConditionTrue, resolver.Generation, time.Now()) + deniedResource := "" + reviewed := map[string]string{} + cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(zone, resolver).WithInterceptorFuncs(interceptor.Funcs{Create: func(ctx context.Context, cl client.WithWatch, obj client.Object, opts ...client.CreateOption) error { + if review, ok := obj.(*authorizationv1.SubjectAccessReview); ok { + attributes := review.Spec.ResourceAttributes + reviewed[attributes.Resource] = attributes.Verb + "/" + attributes.Name + review.Status.Allowed = attributes.Resource != deniedResource + return nil + } + return cl.Create(ctx, obj, opts...) + }}).Build() + h := Handler{Client: cl, ResolverContextsEnabled: true} + zoneRef := dnsv1alpha1.DNSNameReference{Name: zone.Name} + contextRef := dnsv1alpha1.DNSNameReference{Name: resolver.Name} + for resource, object := range map[string]any{ + "dnszoneassociations": &dnsv1alpha1.DNSZoneAssociation{Spec: dnsv1alpha1.DNSZoneAssociationSpec{DNSZoneRef: zoneRef, ResolverContextRef: contextRef}}, + "dnsnamingpolicies": &dnsv1alpha1.DNSNamingPolicy{Spec: dnsv1alpha1.DNSNamingPolicySpec{ResolverContextRef: contextRef, AdditionalNames: []dnsv1alpha1.DNSAdditionalNameRule{{DNSZoneRef: zoneRef, NamePrefix: "hosts"}}}}, + } { + t.Run(resource, func(t *testing.T) { + for _, denied := range []string{"", "dnszones", "dnsresolvercontexts"} { + deniedResource = denied + reviewed = map[string]string{} + response := h.Handle(context.Background(), request(resource, "customer", object, nil)) + if response.Allowed != (denied == "") { + t.Fatalf("denied=%q allowed=%v: %v", denied, response.Allowed, response.Result) + } + if denied == "" && (reviewed["dnszones"] != "update/production" || reviewed["dnsresolvercontexts"] != "use/application") { + t.Fatalf("references did not authorize both named resources: %v", reviewed) + } + } + req := request(resource, "customer", object, nil) + req.SubResource = "status" + if h.Handle(context.Background(), req).Allowed { + t.Fatal("customer can modify DNS-owned lifetime pins") + } + }) + } +} + +func TestNamingPolicyRejectsMalformedSelectors(t *testing.T) { + scheme := runtime.NewScheme() + if err := dnsv1alpha1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + h := Handler{Client: fake.NewClientBuilder().WithScheme(scheme).Build(), ResolverContextsEnabled: true} + for _, selector := range []*metav1.LabelSelector{ + {MatchLabels: map[string]string{"bad key": "value"}}, + {MatchExpressions: []metav1.LabelSelectorRequirement{{Key: "app", Operator: "Unsupported"}}}, + {MatchExpressions: []metav1.LabelSelectorRequirement{{Key: "app", Operator: metav1.LabelSelectorOpIn}}}, + } { + policy := &dnsv1alpha1.DNSNamingPolicy{Spec: dnsv1alpha1.DNSNamingPolicySpec{ResolverContextRef: dnsv1alpha1.DNSNameReference{Name: "application"}, AdditionalNames: []dnsv1alpha1.DNSAdditionalNameRule{{Selector: selector, DNSZoneRef: dnsv1alpha1.DNSNameReference{Name: "production"}, NamePrefix: "application"}}}} + response := h.Handle(context.Background(), request("dnsnamingpolicies", "customer", policy, nil)) + if response.Allowed || response.Result == nil || !strings.Contains(response.Result.Message, "invalid naming selector") { + t.Fatalf("malformed selector was not rejected by validation: %v", response.Result) + } + } +} + func setAdmissionCondition(conditions *[]metav1.Condition, conditionType string, status metav1.ConditionStatus, generation int64, now time.Time) { apimeta.SetStatusCondition(conditions, metav1.Condition{Type: conditionType, Status: status, Reason: "Test", ObservedGeneration: generation, LastTransitionTime: metav1.NewTime(now)}) } diff --git a/internal/internaldns/controlplane/access_recovery_envtest_test.go b/internal/internaldns/controlplane/access_recovery_envtest_test.go index 3f4c0d3a..4fd1edc2 100644 --- a/internal/internaldns/controlplane/access_recovery_envtest_test.go +++ b/internal/internaldns/controlplane/access_recovery_envtest_test.go @@ -82,6 +82,9 @@ func TestAccessRecoveryPreservesSourceFencesWithCEL(t *testing.T) { requireAccessTestSuccess(t, cl.Status().Update(ctx, c)) } readyContext(resolverContext) + t.Run("name-only customer references", func(t *testing.T) { + verifyNamedReferencesWithAPI(t, cl) + }) deadline := metav1.NewTime(now.Add(time.Minute + 22357123*time.Nanosecond)) access := &dnsv1alpha1.DNSResolverAccessBinding{ObjectMeta: metav1.ObjectMeta{Name: "access", Namespace: "project"}, Spec: dnsv1alpha1.DNSResolverAccessBindingSpec{ContextRef: dnsv1alpha1.DNSObjectReference{Name: resolverContext.Name, UID: resolverContext.UID}, Region: "central", QueryIdentity: dnsv1alpha1.DNSResolverQueryIdentity{Type: dnsv1alpha1.DNSResolverQueryIdentityDestinationAddress, Value: "fd70:100::10"}, Port: 53, Transports: []string{"UDP", "TCP"}, Authorization: dnsv1alpha1.DNSResolverAccessAuthorization{WriterEpoch: 1, Sequence: 2, ValidUntil: deadline}}} access.APIVersion = dnsv1alpha1.GroupVersion.String() diff --git a/internal/internaldns/controlplane/compiler.go b/internal/internaldns/controlplane/compiler.go index e938ed27..1ed1c5e9 100644 --- a/internal/internaldns/controlplane/compiler.go +++ b/internal/internaldns/controlplane/compiler.go @@ -11,8 +11,6 @@ import ( dnsv1alpha1 "go.miloapis.com/dns-operator/api/v1alpha1" "go.miloapis.com/dns-operator/internal/internaldns/model" - apimeta "k8s.io/apimachinery/pkg/api/meta" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" ) @@ -518,9 +516,8 @@ func associatedContextUIDs(zone *dnsv1alpha1.DNSZone, associations []dnsv1alpha1 vpcs := map[types.UID]struct{}{} for i := range associations { a := &associations[i] - accepted := apimeta.FindStatusCondition(a.Status.Conditions, "Accepted") consumerUID := a.Status.ResolvedResolverContextRef.UID - if a.DeletionTimestamp.IsZero() && accepted != nil && accepted.Status == metav1.ConditionTrue && refMatches(a.Status.ResolvedDNSZoneRef, zone.Name, zone.UID, zone.Generation) && consumerUID != "" { + if associationMatchesZone(a, zone) && consumerUID != "" { vpcs[consumerUID] = struct{}{} } } diff --git a/internal/internaldns/controlplane/compiler_test.go b/internal/internaldns/controlplane/compiler_test.go index db936bde..545debea 100644 --- a/internal/internaldns/controlplane/compiler_test.go +++ b/internal/internaldns/controlplane/compiler_test.go @@ -211,7 +211,7 @@ func TestCompileSRVTargetUsesAbsoluteZoneIdentity(t *testing.T) { func fixture(now time.Time) CompileInput { zone := &dnsv1alpha1.DNSZone{ObjectMeta: metav1.ObjectMeta{Name: "private", UID: "zone-a", Generation: 2}, Spec: dnsv1alpha1.DNSZoneSpec{DomainName: "corp.internal", Visibility: dnsv1alpha1.DNSZoneVisibilityPrivate}} assoc := func(name string, vpc types.UID) dnsv1alpha1.DNSZoneAssociation { - return dnsv1alpha1.DNSZoneAssociation{ObjectMeta: metav1.ObjectMeta{Name: name, UID: types.UID(name)}, Spec: dnsv1alpha1.DNSZoneAssociationSpec{DNSZoneRef: dnsv1alpha1.DNSObjectReference{Name: zone.Name, UID: zone.UID}, ResolverContextRef: dnsv1alpha1.DNSObjectReference{Name: name, UID: vpc}}, Status: dnsv1alpha1.DNSZoneAssociationStatus{ResolvedDNSZoneRef: dnsv1alpha1.DNSObjectReference{Name: zone.Name, UID: zone.UID, Generation: zone.Generation}, ResolvedResolverContextRef: dnsv1alpha1.DNSObjectReference{Name: name, UID: vpc}, Conditions: []metav1.Condition{{Type: "Accepted", Status: metav1.ConditionTrue}}}} + return dnsv1alpha1.DNSZoneAssociation{ObjectMeta: metav1.ObjectMeta{Name: name, UID: types.UID(name)}, Spec: dnsv1alpha1.DNSZoneAssociationSpec{DNSZoneRef: dnsv1alpha1.DNSNameReference{Name: zone.Name}, ResolverContextRef: dnsv1alpha1.DNSNameReference{Name: name}}, Status: dnsv1alpha1.DNSZoneAssociationStatus{ResolvedDNSZoneRef: dnsv1alpha1.DNSObjectReference{Name: zone.Name, UID: zone.UID, Generation: zone.Generation}, ResolvedResolverContextRef: dnsv1alpha1.DNSObjectReference{Name: name, UID: vpc}, Conditions: []metav1.Condition{{Type: "Accepted", Status: metav1.ConditionTrue}}}} } reg := dnsv1alpha1.DNSRegistration{ObjectMeta: metav1.ObjectMeta{Name: "api", UID: "registration-a", Generation: 3}, Spec: dnsv1alpha1.DNSRegistrationSpec{DNSZoneRef: dnsv1alpha1.DNSObjectReference{Name: zone.Name, UID: zone.UID, Generation: zone.Generation}, Name: "api", RecordTypes: []dnsv1alpha1.RRType{dnsv1alpha1.RRTypeAAAA}, PublicationPolicy: dnsv1alpha1.DNSPublicationPolicyEligibleContributions, TTLSeconds: 30}} grant := dnsv1alpha1.DNSContributionGrant{ObjectMeta: metav1.ObjectMeta{Name: "compute", UID: "grant-a", Generation: 2}, Spec: dnsv1alpha1.DNSContributionGrantSpec{RegistrationRef: dnsv1alpha1.DNSObjectReference{Name: reg.Name, UID: reg.UID, Generation: reg.Generation}, ProducerID: "compute-east", RecordTypes: []dnsv1alpha1.RRType{dnsv1alpha1.RRTypeAAAA}}, Status: dnsv1alpha1.DNSContributionGrantStatus{ActiveWriterEpoch: 7, ObservedGrantGeneration: 2, ObservedRegistrationGeneration: 3}} diff --git a/internal/internaldns/controlplane/context_reconciler.go b/internal/internaldns/controlplane/context_reconciler.go index 68b9c06b..89bb130c 100644 --- a/internal/internaldns/controlplane/context_reconciler.go +++ b/internal/internaldns/controlplane/context_reconciler.go @@ -27,55 +27,79 @@ import ( func jsonEqual(a, b any) bool { return reflect.DeepEqual(a, b) } +// pinNamedReference preserves a resolved lifetime while the selected name is unchanged. +// Recreate the association or policy to attach a replacement with the same name. +func pinNamedReference(name string, previous, current dnsv1alpha1.DNSObjectReference) (dnsv1alpha1.DNSObjectReference, bool) { + if name == "" { + return dnsv1alpha1.DNSObjectReference{}, false + } + if previous.Name != name { + previous = dnsv1alpha1.DNSObjectReference{Name: name} + } + if current.Name != name || current.UID == "" || (previous.UID != "" && previous.UID != current.UID) { + return previous, false + } + return current, true +} + func (r *Reconciler) reconcileNamingPolicies(ctx context.Context, items []dnsv1alpha1.DNSNamingPolicy, assocs []dnsv1alpha1.DNSZoneAssociation, zones map[string]*dnsv1alpha1.DNSZone, now time.Time) error { for i := range items { p := &items[i] - vpcUID, contextUID, contextName := types.UID(""), types.UID(""), "" - valid := false - + base := p.DeepCopy() var resolverContext dnsv1alpha1.DNSResolverContext err := r.Get(ctx, client.ObjectKey{Namespace: p.Namespace, Name: p.Spec.ResolverContextRef.Name}, &resolverContext) + current := dnsv1alpha1.DNSObjectReference{} + if err == nil && resolverContext.DeletionTimestamp.IsZero() { + current = dnsv1alpha1.DNSObjectReference{Name: resolverContext.Name, UID: resolverContext.UID} + } + contextPin, contextCurrent := pinNamedReference(p.Spec.ResolverContextRef.Name, p.Status.ResolvedResolverContextRef, current) ready := apimeta.FindStatusCondition(resolverContext.Status.Conditions, "Ready") - valid = err == nil && p.Spec.ResolverContextRef.UID != "" && p.Spec.ResolverContextRef.UID == resolverContext.UID && resolverContext.DeletionTimestamp.IsZero() && ready != nil && ready.Status == metav1.ConditionTrue && ready.ObservedGeneration == resolverContext.Generation - if valid { - vpcUID = types.UID(resolverContext.Spec.ConsumerID) - contextUID = resolverContext.UID - contextName = resolverContext.Name + valid := contextCurrent && p.DeletionTimestamp.IsZero() && ready != nil && ready.Status == metav1.ConditionTrue && ready.ObservedGeneration == resolverContext.Generation + p.Status.ResolvedResolverContextRef = contextPin + if contextCurrent { + p.Status.ResolvedVPCRef = dnsv1alpha1.DNSObjectReference{Name: resolverContext.Name, UID: types.UID(resolverContext.Spec.ConsumerID)} } - resolved := []dnsv1alpha1.DNSResolvedAdditionalNameRule{} + previousZones := map[string]dnsv1alpha1.DNSObjectReference{} + for _, rule := range base.Status.ResolvedAdditionalNames { + previousZones[rule.DNSZoneRef.Name] = rule.DNSZoneRef + } + resolved := make([]dnsv1alpha1.DNSResolvedAdditionalNameRule, 0, len(p.Spec.AdditionalNames)) seen := map[string]bool{} for _, rule := range p.Spec.AdditionalNames { z := zones[rule.DNSZoneRef.Name] - associated := associatedWithContext(assocs, zUID(z), contextUID) - - if z == nil || z.Spec.Visibility != dnsv1alpha1.DNSZoneVisibilityPrivate || !refMatches(rule.DNSZoneRef, z.Name, z.UID, z.Generation) || !associated { + currentZone := dnsv1alpha1.DNSObjectReference{} + if z != nil && z.DeletionTimestamp.IsZero() && z.Spec.Visibility == dnsv1alpha1.DNSZoneVisibilityPrivate { + currentZone = dnsv1alpha1.DNSObjectReference{Name: z.Name, UID: z.UID, Generation: z.Generation} + } + zonePin, zoneCurrent := pinNamedReference(rule.DNSZoneRef.Name, previousZones[rule.DNSZoneRef.Name], currentZone) + if !zoneCurrent || !contextCurrent || !associatedWithContext(assocs, zonePin.UID, contextPin.UID) { valid = false - continue } - key := string(rule.RegistrationClass) + "\x00" + canonicalName(rule.NamePrefix) + selector, selectorErr := namingSelector(rule.Selector) + selectorKey := "" + if selectorErr != nil { + valid = false + } else { + selectorKey = selector.String() + } + key := rule.DNSZoneRef.Name + "\x00" + canonicalName(rule.NamePrefix) + "\x00" + selectorKey if rule.NamePrefix == "" || seen[key] { valid = false - continue } seen[key] = true - resolved = append(resolved, dnsv1alpha1.DNSResolvedAdditionalNameRule{RegistrationClass: rule.RegistrationClass, DNSZoneRef: dnsv1alpha1.DNSObjectReference{Name: z.Name, UID: z.UID, Generation: z.Generation}, NamePrefix: canonicalName(rule.NamePrefix)}) + resolved = append(resolved, dnsv1alpha1.DNSResolvedAdditionalNameRule{Selector: rule.Selector.DeepCopy(), DNSZoneRef: zonePin, NamePrefix: canonicalName(rule.NamePrefix)}) } - base := p.DeepCopy() - p.Status.ResolvedVPCRef = dnsv1alpha1.DNSObjectReference{Name: p.Spec.VPCRef.Name, UID: vpcUID} - - p.Status.ResolvedVPCRef.Name = contextName - p.Status.ResolvedResolverContextRef = dnsv1alpha1.DNSObjectReference{Name: contextName, UID: contextUID} - p.Status.ResolvedAdditionalNames = resolved status := metav1.ConditionTrue reason, msg := dnsValueAccepted, "naming policy is authorized" if !valid { status = metav1.ConditionFalse reason = "InvalidPolicy" - msg = "all rules require an associated private zone and unique class/prefix" + msg = "rules require current resource lifetimes, associated private zones, valid label selectors, and unique naming rules" } - if setCondition(&p.Status.Conditions, dnsValueAccepted, status, reason, msg, p.Generation, now) { + setCondition(&p.Status.Conditions, dnsValueAccepted, status, reason, msg, p.Generation, now) + if !jsonEqual(base.Status, p.Status) { if err := r.Status().Patch(ctx, p, client.MergeFrom(base)); err != nil { return err } @@ -84,17 +108,18 @@ func (r *Reconciler) reconcileNamingPolicies(ctx context.Context, items []dnsv1a return nil } -func zUID(z *dnsv1alpha1.DNSZone) types.UID { - if z == nil { - return "" - } - return z.UID +func associationAccepted(a *dnsv1alpha1.DNSZoneAssociation) bool { + accepted := apimeta.FindStatusCondition(a.Status.Conditions, dnsValueAccepted) + return a.DeletionTimestamp.IsZero() && accepted != nil && accepted.Status == metav1.ConditionTrue && accepted.ObservedGeneration == a.Generation +} + +func associationMatchesZone(a *dnsv1alpha1.DNSZoneAssociation, z *dnsv1alpha1.DNSZone) bool { + return z != nil && associationAccepted(a) && refMatches(a.Status.ResolvedDNSZoneRef, z.Name, z.UID, z.Generation) } func associatedWithContext(assocs []dnsv1alpha1.DNSZoneAssociation, zoneUID, contextUID types.UID) bool { for _, a := range assocs { - c := apimeta.FindStatusCondition(a.Status.Conditions, dnsValueAccepted) - if c != nil && c.Status == metav1.ConditionTrue && a.Status.ResolvedDNSZoneRef.UID == zoneUID && a.Status.ResolvedResolverContextRef.UID == contextUID { + if associationAccepted(&a) && a.Status.ResolvedDNSZoneRef.UID == zoneUID && a.Status.ResolvedResolverContextRef.UID == contextUID { return true } } @@ -132,14 +157,13 @@ func (r *Reconciler) reconcileManagedNamespaces(ctx context.Context, ns string, return err } assocName := zoneName + "-vpc" - assocSpec := dnsv1alpha1.DNSZoneAssociationSpec{DNSZoneRef: dnsv1alpha1.DNSObjectReference{Name: zone.Name, UID: zone.UID}, VPCRef: m.Spec.VPCRef, Managed: true} + assocSpec := dnsv1alpha1.DNSZoneAssociationSpec{DNSZoneRef: dnsv1alpha1.DNSNameReference{Name: zone.Name}, Managed: true} - assocSpec.VPCRef = dnsv1alpha1.DNSObjectReference{} var contextObject dnsv1alpha1.DNSResolverContext if err := r.Get(ctx, client.ObjectKey{Namespace: ns, Name: m.Spec.VPCRef.Name}, &contextObject); err != nil { return err } - assocSpec.ResolverContextRef = dnsv1alpha1.DNSObjectReference{Name: contextObject.Name, UID: contextObject.UID} + assocSpec.ResolverContextRef = dnsv1alpha1.DNSNameReference{Name: contextObject.Name} assoc := &dnsv1alpha1.DNSZoneAssociation{ObjectMeta: metav1.ObjectMeta{Name: assocName, Namespace: ns, Labels: map[string]string{"dns.networking.miloapis.com/managed": "true"}}, Spec: assocSpec} if r.Scheme != nil { @@ -252,48 +276,45 @@ func (r *Reconciler) reconcileResolverContexts(ctx context.Context, ns string, n func (r *Reconciler) reconcileAssociations(ctx context.Context, items []dnsv1alpha1.DNSZoneAssociation, zones map[string]*dnsv1alpha1.DNSZone, now time.Time) error { type apexKey struct { - vpc types.UID - apex string + context types.UID + apex string } owners := map[apexKey]types.UID{} sort.Slice(items, func(i, j int) bool { return items[i].Name < items[j].Name }) for i := range items { a := &items[i] + base := a.DeepCopy() z := zones[a.Spec.DNSZoneRef.Name] + currentZone := dnsv1alpha1.DNSObjectReference{} + if z != nil && z.DeletionTimestamp.IsZero() && z.Spec.Visibility == dnsv1alpha1.DNSZoneVisibilityPrivate { + currentZone = dnsv1alpha1.DNSObjectReference{Name: z.Name, UID: z.UID, Generation: z.Generation} + } + zonePin, zoneCurrent := pinNamedReference(a.Spec.DNSZoneRef.Name, a.Status.ResolvedDNSZoneRef, currentZone) + var resolverContext dnsv1alpha1.DNSResolverContext + lookupErr := r.Get(ctx, client.ObjectKey{Namespace: a.Namespace, Name: a.Spec.ResolverContextRef.Name}, &resolverContext) + currentContext := dnsv1alpha1.DNSObjectReference{} + if lookupErr == nil && resolverContext.DeletionTimestamp.IsZero() { + currentContext = dnsv1alpha1.DNSObjectReference{Name: resolverContext.Name, UID: resolverContext.UID} + } + contextPin, contextCurrent := pinNamedReference(a.Spec.ResolverContextRef.Name, a.Status.ResolvedResolverContextRef, currentContext) + a.Status.ResolvedDNSZoneRef = zonePin + a.Status.ResolvedResolverContextRef = contextPin + a.Status.ResolvedVPCRef = dnsv1alpha1.DNSObjectReference{} status := metav1.ConditionTrue reason, msg := dnsValueAccepted, "association accepted" - vpcUID, vpcErr := types.UID(""), error(nil) - - var resolverContext dnsv1alpha1.DNSResolverContext - vpcErr = r.Get(ctx, client.ObjectKey{Namespace: a.Namespace, Name: a.Spec.ResolverContextRef.Name}, &resolverContext) - if vpcErr == nil && (a.Spec.ResolverContextRef.UID == "" || a.Spec.ResolverContextRef.UID != resolverContext.UID || !resolverContext.DeletionTimestamp.IsZero()) { - vpcErr = errors.New("resolver context lifetime is not current") - } - if vpcErr == nil { - vpcUID = resolverContext.UID - } - - if !a.DeletionTimestamp.IsZero() || z == nil || z.Spec.Visibility != dnsv1alpha1.DNSZoneVisibilityPrivate || vpcErr != nil || !refMatches(a.Spec.DNSZoneRef, z.Name, z.UID, z.Generation) { + if !a.DeletionTimestamp.IsZero() || !zoneCurrent || !contextCurrent { status = metav1.ConditionFalse reason = "InvalidReference" - msg = "private zone and pinned VPC/zone UIDs are required" - } else if old := owners[apexKey{vpcUID, canonicalName(z.Spec.DomainName)}]; old != "" && old != z.UID { + msg = "private zone and resolver context must match their resolved lifetimes; recreate the association to reattach replacements" + } else if old := owners[apexKey{contextPin.UID, canonicalName(z.Spec.DomainName)}]; old != "" && old != z.UID { status = metav1.ConditionFalse reason = "ApexConflict" - msg = "another zone owns this apex in the VPC" + msg = "another zone owns this apex in the DNS context" } else { - owners[apexKey{vpcUID, canonicalName(z.Spec.DomainName)}] = z.UID + owners[apexKey{contextPin.UID, canonicalName(z.Spec.DomainName)}] = z.UID } - base := a.DeepCopy() - a.Status.ResolvedVPCRef = dnsv1alpha1.DNSObjectReference{Name: a.Spec.VPCRef.Name, UID: vpcUID} - - a.Status.ResolvedVPCRef = dnsv1alpha1.DNSObjectReference{} - a.Status.ResolvedResolverContextRef = dnsv1alpha1.DNSObjectReference{Name: a.Spec.ResolverContextRef.Name, UID: vpcUID} - - if z != nil { - a.Status.ResolvedDNSZoneRef = dnsv1alpha1.DNSObjectReference{Name: z.Name, UID: z.UID, Generation: z.Generation} - } - if setCondition(&a.Status.Conditions, dnsValueAccepted, status, reason, msg, a.Generation, now) { + setCondition(&a.Status.Conditions, dnsValueAccepted, status, reason, msg, a.Generation, now) + if !jsonEqual(base.Status, a.Status) { if err := r.Status().Patch(ctx, a, client.MergeFrom(base)); err != nil { return err } @@ -333,8 +354,7 @@ func (r *Reconciler) reconcileContextBindings(ctx context.Context, ns string, as zonesByContext := map[types.UID][]types.UID{} for _, a := range assocs { z := zones[a.Spec.DNSZoneRef.Name] - accepted := apimeta.FindStatusCondition(a.Status.Conditions, dnsValueAccepted) - if z != nil && accepted != nil && accepted.Status == metav1.ConditionTrue { + if associationMatchesZone(&a, z) { zonesByContext[a.Status.ResolvedResolverContextRef.UID] = append(zonesByContext[a.Status.ResolvedResolverContextRef.UID], z.UID) } } diff --git a/internal/internaldns/controlplane/context_reconciler_test.go b/internal/internaldns/controlplane/context_reconciler_test.go index e86b0c01..8da2e5b6 100644 --- a/internal/internaldns/controlplane/context_reconciler_test.go +++ b/internal/internaldns/controlplane/context_reconciler_test.go @@ -79,7 +79,7 @@ func TestContextAccessProjectsExactAuthorizationAndExpiresClosed(t *testing.T) { contextObject := &dnsv1alpha1.DNSResolverContext{ObjectMeta: metav1.ObjectMeta{Name: "ctx", Namespace: "project", UID: "context-uid", Generation: 1}, Spec: dnsv1alpha1.DNSResolverContextSpec{ConsumerID: "consumer-uid"}, Status: dnsv1alpha1.DNSResolverContextStatus{AccessWriterEpoch: 4}} setCondition(&contextObject.Status.Conditions, "Ready", metav1.ConditionTrue, "Ready", "", 1, now) zone := &dnsv1alpha1.DNSZone{ObjectMeta: metav1.ObjectMeta{Name: "zone", Namespace: "project", UID: "zone-uid", Generation: 1}, Spec: dnsv1alpha1.DNSZoneSpec{Visibility: dnsv1alpha1.DNSZoneVisibilityPrivate}} - association := &dnsv1alpha1.DNSZoneAssociation{ObjectMeta: metav1.ObjectMeta{Name: "assoc", Namespace: "project", UID: "assoc-uid", Generation: 1}, Spec: dnsv1alpha1.DNSZoneAssociationSpec{DNSZoneRef: dnsv1alpha1.DNSObjectReference{Name: "zone", UID: "zone-uid"}, ResolverContextRef: dnsv1alpha1.DNSObjectReference{Name: "ctx", UID: "context-uid"}}, Status: dnsv1alpha1.DNSZoneAssociationStatus{ResolvedDNSZoneRef: dnsv1alpha1.DNSObjectReference{Name: "zone", UID: "zone-uid", Generation: 1}, ResolvedResolverContextRef: dnsv1alpha1.DNSObjectReference{Name: "ctx", UID: "context-uid"}}} + association := &dnsv1alpha1.DNSZoneAssociation{ObjectMeta: metav1.ObjectMeta{Name: "assoc", Namespace: "project", UID: "assoc-uid", Generation: 1}, Spec: dnsv1alpha1.DNSZoneAssociationSpec{DNSZoneRef: dnsv1alpha1.DNSNameReference{Name: "zone"}, ResolverContextRef: dnsv1alpha1.DNSNameReference{Name: "ctx"}}, Status: dnsv1alpha1.DNSZoneAssociationStatus{ResolvedDNSZoneRef: dnsv1alpha1.DNSObjectReference{Name: "zone", UID: "zone-uid", Generation: 1}, ResolvedResolverContextRef: dnsv1alpha1.DNSObjectReference{Name: "ctx", UID: "context-uid"}}} setCondition(&association.Status.Conditions, "Accepted", metav1.ConditionTrue, "Accepted", "", 1, now) until := metav1.NewTime(now.Add(2 * time.Minute)) access := &dnsv1alpha1.DNSResolverAccessBinding{ObjectMeta: metav1.ObjectMeta{Name: "access", Namespace: "project", UID: "access-uid", Generation: 1}, Spec: dnsv1alpha1.DNSResolverAccessBindingSpec{ContextRef: dnsv1alpha1.DNSObjectReference{Name: "ctx", UID: "context-uid"}, Region: "central", QueryIdentity: dnsv1alpha1.DNSResolverQueryIdentity{Type: dnsv1alpha1.DNSResolverQueryIdentityDestinationAddress, Value: "fd70:100::10"}, Port: 53, Transports: []string{"UDP", "TCP"}, Authorization: dnsv1alpha1.DNSResolverAccessAuthorization{WriterEpoch: 4, Sequence: 12, ValidUntil: until}}} @@ -113,8 +113,8 @@ func TestContextAccessProjectsExactAuthorizationAndExpiresClosed(t *testing.T) { extraZone.UID = "extra-uid" extraAssociation := association.DeepCopy() extraAssociation.Name = "extra-assoc" - extraAssociation.Spec.DNSZoneRef = dnsv1alpha1.DNSObjectReference{Name: extraZone.Name, UID: extraZone.UID} - extraAssociation.Status.ResolvedDNSZoneRef = extraAssociation.Spec.DNSZoneRef + extraAssociation.Spec.DNSZoneRef = dnsv1alpha1.DNSNameReference{Name: extraZone.Name} + extraAssociation.Status.ResolvedDNSZoneRef = dnsv1alpha1.DNSObjectReference{Name: extraZone.Name, UID: extraZone.UID, Generation: extraZone.Generation} if err := r.reconcileContextBindings(ctx, "project", []dnsv1alpha1.DNSZoneAssociation{*association, *extraAssociation}, map[string]*dnsv1alpha1.DNSZone{"zone": zone, "extra": extraZone}, now); err != nil { t.Fatal(err) } @@ -252,9 +252,10 @@ func TestContextNamingPolicyResolvesMultipleZonesWithoutNetworking(t *testing.T) z2 := dnsv1alpha1.DNSZone{ObjectMeta: metav1.ObjectMeta{Name: "services", Namespace: "project", UID: "zone-2", Generation: 3}, Spec: dnsv1alpha1.DNSZoneSpec{Visibility: dnsv1alpha1.DNSZoneVisibilityPrivate}} associations := []dnsv1alpha1.DNSZoneAssociation{{Status: dnsv1alpha1.DNSZoneAssociationStatus{ResolvedDNSZoneRef: dnsv1alpha1.DNSObjectReference{UID: z1.UID}, ResolvedResolverContextRef: dnsv1alpha1.DNSObjectReference{UID: resolverContext.UID}}}, {Status: dnsv1alpha1.DNSZoneAssociationStatus{ResolvedDNSZoneRef: dnsv1alpha1.DNSObjectReference{UID: z2.UID}, ResolvedResolverContextRef: dnsv1alpha1.DNSObjectReference{UID: resolverContext.UID}}}} for i := range associations { + associations[i].Generation = 1 setCondition(&associations[i].Status.Conditions, "Accepted", metav1.ConditionTrue, "Accepted", "", 1, now) } - policy := &dnsv1alpha1.DNSNamingPolicy{ObjectMeta: metav1.ObjectMeta{Name: "names", Namespace: "project", UID: "policy-uid", Generation: 1}, Spec: dnsv1alpha1.DNSNamingPolicySpec{ResolverContextRef: dnsv1alpha1.DNSObjectReference{Name: resolverContext.Name, UID: resolverContext.UID}, AdditionalNames: []dnsv1alpha1.DNSAdditionalNameRule{{RegistrationClass: dnsv1alpha1.DNSRegistrationClassInstanceIdentity, DNSZoneRef: dnsv1alpha1.DNSObjectReference{Name: z1.Name, UID: z1.UID, Generation: z1.Generation}, NamePrefix: "instances"}, {RegistrationClass: dnsv1alpha1.DNSRegistrationClassServiceDiscovery, DNSZoneRef: dnsv1alpha1.DNSObjectReference{Name: z2.Name, UID: z2.UID, Generation: z2.Generation}, NamePrefix: "services"}}}} + policy := &dnsv1alpha1.DNSNamingPolicy{ObjectMeta: metav1.ObjectMeta{Name: "names", Namespace: "project", UID: "policy-uid", Generation: 1}, Spec: dnsv1alpha1.DNSNamingPolicySpec{ResolverContextRef: dnsv1alpha1.DNSNameReference{Name: resolverContext.Name}, AdditionalNames: []dnsv1alpha1.DNSAdditionalNameRule{{DNSZoneRef: dnsv1alpha1.DNSNameReference{Name: z1.Name}, NamePrefix: "instances"}, {DNSZoneRef: dnsv1alpha1.DNSNameReference{Name: z2.Name}, NamePrefix: "services"}}}} c := fake.NewClientBuilder().WithScheme(s).WithObjects(resolverContext, policy).WithStatusSubresource(&dnsv1alpha1.DNSResolverContext{}, &dnsv1alpha1.DNSResolverAccessBinding{}, &dnsv1alpha1.DNSNamingPolicy{}).Build() r := &Reconciler{Client: c, Options: ReconcilerOptions{}} if err := r.reconcileNamingPolicies(ctx, []dnsv1alpha1.DNSNamingPolicy{*policy}, associations, map[string]*dnsv1alpha1.DNSZone{z1.Name: &z1, z2.Name: &z2}, now); err != nil { diff --git a/internal/internaldns/controlplane/managed_domain_test.go b/internal/internaldns/controlplane/managed_domain_test.go index b2c56402..deed10d8 100644 --- a/internal/internaldns/controlplane/managed_domain_test.go +++ b/internal/internaldns/controlplane/managed_domain_test.go @@ -54,7 +54,11 @@ func TestManagedDomainIsSharedWithSeparateContextZones(t *testing.T) { } for i := range associations.Items { a := &associations.Items[i] - a.Status.ResolvedDNSZoneRef = a.Spec.DNSZoneRef + for _, z := range zones.Items { + if z.Name == a.Spec.DNSZoneRef.Name { + a.Status.ResolvedDNSZoneRef = dnsv1alpha1.DNSObjectReference{Name: z.Name, UID: z.UID, Generation: z.Generation} + } + } setCondition(&a.Status.Conditions, "Accepted", metav1.ConditionTrue, "Ready", "", a.Generation, now) if err := c.Status().Update(ctx, a); err != nil { t.Fatal(err) diff --git a/internal/internaldns/controlplane/named_reference_api_test.go b/internal/internaldns/controlplane/named_reference_api_test.go new file mode 100644 index 00000000..96a4ac67 --- /dev/null +++ b/internal/internaldns/controlplane/named_reference_api_test.go @@ -0,0 +1,62 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package controlplane + +import ( + "context" + "testing" + "time" + + dnsv1alpha1 "go.miloapis.com/dns-operator/api/v1alpha1" + apimeta "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +func verifyNamedReferencesWithAPI(t *testing.T, cl client.Client) { + t.Helper() + ctx := context.Background() + now := time.Now().UTC() + resolver := &dnsv1alpha1.DNSResolverContext{ObjectMeta: metav1.ObjectMeta{Name: "named-context", Namespace: "project"}, Spec: dnsv1alpha1.DNSResolverContextSpec{ConsumerID: "named-consumer"}} + requireAccessTestSuccess(t, cl.Create(ctx, resolver)) + setCondition(&resolver.Status.Conditions, "Ready", metav1.ConditionTrue, "Ready", "", resolver.Generation, now) + requireAccessTestSuccess(t, cl.Status().Update(ctx, resolver)) + zone := &dnsv1alpha1.DNSZone{ObjectMeta: metav1.ObjectMeta{Name: "named-zone", Namespace: "project"}, Spec: dnsv1alpha1.DNSZoneSpec{DomainName: "named.internal", Visibility: dnsv1alpha1.DNSZoneVisibilityPrivate, DNSZoneClassName: "private"}} + requireAccessTestSuccess(t, cl.Create(ctx, zone)) + association := &dnsv1alpha1.DNSZoneAssociation{ObjectMeta: metav1.ObjectMeta{Name: "named-association", Namespace: "project"}, Spec: dnsv1alpha1.DNSZoneAssociationSpec{DNSZoneRef: dnsv1alpha1.DNSNameReference{Name: zone.Name}, ResolverContextRef: dnsv1alpha1.DNSNameReference{Name: resolver.Name}}} + policy := &dnsv1alpha1.DNSNamingPolicy{ObjectMeta: metav1.ObjectMeta{Name: "named-policy", Namespace: "project"}, Spec: dnsv1alpha1.DNSNamingPolicySpec{ResolverContextRef: dnsv1alpha1.DNSNameReference{Name: resolver.Name}, AdditionalNames: []dnsv1alpha1.DNSAdditionalNameRule{{Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"app": "storefront"}}, DNSZoneRef: dnsv1alpha1.DNSNameReference{Name: zone.Name}, NamePrefix: "hosts"}}}} + requireAccessTestSuccess(t, cl.Create(ctx, association)) + requireAccessTestSuccess(t, cl.Create(ctx, policy)) + r := &Reconciler{Client: cl} + reconcile := func(want metav1.ConditionStatus) { + t.Helper() + requireAccessTestSuccess(t, cl.Get(ctx, client.ObjectKeyFromObject(association), association)) + associations := []dnsv1alpha1.DNSZoneAssociation{*association} + zones := map[string]*dnsv1alpha1.DNSZone{zone.Name: zone} + requireAccessTestSuccess(t, r.reconcileAssociations(ctx, associations, zones, now)) + requireAccessTestSuccess(t, cl.Get(ctx, client.ObjectKeyFromObject(policy), policy)) + requireAccessTestSuccess(t, r.reconcileNamingPolicies(ctx, []dnsv1alpha1.DNSNamingPolicy{*policy}, associations, zones, now)) + requireAccessTestSuccess(t, cl.Get(ctx, client.ObjectKeyFromObject(association), association)) + requireAccessTestSuccess(t, cl.Get(ctx, client.ObjectKeyFromObject(policy), policy)) + for name, conditions := range map[string][]metav1.Condition{"association": association.Status.Conditions, "policy": policy.Status.Conditions} { + if condition := apimeta.FindStatusCondition(conditions, "Accepted"); condition == nil || condition.Status != want { + t.Fatalf("%s expected %s: %v", name, want, condition) + } + } + } + reconcile(metav1.ConditionTrue) + if selector := policy.Status.ResolvedAdditionalNames[0].Selector; selector == nil || selector.MatchLabels["app"] != "storefront" { + t.Fatal("API did not preserve the registration label selector") + } + originalUID := zone.UID + requireAccessTestSuccess(t, cl.Delete(ctx, zone)) + zone = &dnsv1alpha1.DNSZone{ObjectMeta: metav1.ObjectMeta{Name: zone.Name, Namespace: "project"}, Spec: zone.Spec} + requireAccessTestSuccess(t, cl.Create(ctx, zone)) + if zone.UID == originalUID { + t.Fatal("replacement did not receive a new API-assigned UID") + } + reconcile(metav1.ConditionFalse) + if association.Status.ResolvedDNSZoneRef.UID != originalUID || policy.Status.ResolvedAdditionalNames[0].DNSZoneRef.UID != originalUID { + t.Fatal("API-backed replacement transferred lifetime pins") + } +} diff --git a/internal/internaldns/controlplane/named_reference_test.go b/internal/internaldns/controlplane/named_reference_test.go new file mode 100644 index 00000000..c030a356 --- /dev/null +++ b/internal/internaldns/controlplane/named_reference_test.go @@ -0,0 +1,132 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package controlplane + +import ( + "context" + "testing" + "time" + + dnsv1alpha1 "go.miloapis.com/dns-operator/api/v1alpha1" + apimeta "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func TestNamedReferencesPreserveLifetimesAcrossReplacement(t *testing.T) { + for _, replace := range []string{"zone", "context"} { + t.Run(replace, func(t *testing.T) { + ctx := context.Background() + now := time.Now().UTC() + scheme := runtime.NewScheme() + if err := dnsv1alpha1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + resolver := &dnsv1alpha1.DNSResolverContext{ObjectMeta: metav1.ObjectMeta{Name: "application", Namespace: "project", UID: "context-original", Generation: 1}, Spec: dnsv1alpha1.DNSResolverContextSpec{ConsumerID: "consumer"}} + setCondition(&resolver.Status.Conditions, "Ready", metav1.ConditionTrue, "Ready", "", resolver.Generation, now) + zone := &dnsv1alpha1.DNSZone{ObjectMeta: metav1.ObjectMeta{Name: "production", Namespace: "project", UID: "zone-original", Generation: 1}, Spec: dnsv1alpha1.DNSZoneSpec{DomainName: "prod.internal", Visibility: dnsv1alpha1.DNSZoneVisibilityPrivate}} + association := &dnsv1alpha1.DNSZoneAssociation{ObjectMeta: metav1.ObjectMeta{Name: "production", Namespace: "project", Generation: 1}, Spec: dnsv1alpha1.DNSZoneAssociationSpec{DNSZoneRef: dnsv1alpha1.DNSNameReference{Name: zone.Name}, ResolverContextRef: dnsv1alpha1.DNSNameReference{Name: resolver.Name}}} + policy := &dnsv1alpha1.DNSNamingPolicy{ObjectMeta: metav1.ObjectMeta{Name: "production", Namespace: "project", Generation: 1}, Spec: dnsv1alpha1.DNSNamingPolicySpec{ResolverContextRef: dnsv1alpha1.DNSNameReference{Name: resolver.Name}, AdditionalNames: []dnsv1alpha1.DNSAdditionalNameRule{{DNSZoneRef: dnsv1alpha1.DNSNameReference{Name: zone.Name}, NamePrefix: "hosts"}}}} + cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(resolver, zone, association, policy).WithStatusSubresource(association, policy).Build() + r := &Reconciler{Client: cl} + zones := map[string]*dnsv1alpha1.DNSZone{zone.Name: zone} + reconcile := func() { + t.Helper() + if err := cl.Get(ctx, client.ObjectKeyFromObject(association), association); err != nil { + t.Fatal(err) + } + associations := []dnsv1alpha1.DNSZoneAssociation{*association} + if err := r.reconcileAssociations(ctx, associations, zones, now); err != nil { + t.Fatal(err) + } + if err := cl.Get(ctx, client.ObjectKeyFromObject(policy), policy); err != nil { + t.Fatal(err) + } + if err := r.reconcileNamingPolicies(ctx, []dnsv1alpha1.DNSNamingPolicy{*policy}, associations, zones, now); err != nil { + t.Fatal(err) + } + if err := cl.Get(ctx, client.ObjectKeyFromObject(association), association); err != nil { + t.Fatal(err) + } + if err := cl.Get(ctx, client.ObjectKeyFromObject(policy), policy); err != nil { + t.Fatal(err) + } + } + assertAccepted := func(want bool) { + t.Helper() + for name, conditions := range map[string][]metav1.Condition{"association": association.Status.Conditions, "policy": policy.Status.Conditions} { + condition := apimeta.FindStatusCondition(conditions, "Accepted") + if condition == nil || (condition.Status == metav1.ConditionTrue) != want { + t.Fatalf("%s accepted=%v, want %v", name, condition, want) + } + } + } + reconcile() + assertAccepted(true) + if association.Status.ResolvedDNSZoneRef.UID != zone.UID || policy.Status.ResolvedResolverContextRef.UID != resolver.UID { + t.Fatal("name-only references did not pin lifetimes") + } + zone.Generation++ + reconcile() + assertAccepted(true) + if association.Status.ResolvedDNSZoneRef.Generation != zone.Generation || policy.Status.ResolvedAdditionalNames[0].DNSZoneRef.Generation != zone.Generation { + t.Fatal("resolved generations were not persisted for the same zone lifetime") + } + originalZoneUID := zone.UID + originalContextUID := resolver.UID + var replaced client.Object = zone + if replace == "context" { + replaced = resolver + } + if err := cl.Delete(ctx, replaced); err != nil { + t.Fatal(err) + } + if replace == "zone" { + delete(zones, zone.Name) + } + reconcile() + assertAccepted(false) + if association.Status.ResolvedDNSZoneRef.UID != originalZoneUID || policy.Status.ResolvedResolverContextRef.UID != originalContextUID { + t.Fatal("deletion cleared lifetime pins") + } + replaced.SetResourceVersion("") + replaced.SetUID("replacement") + if err := cl.Create(ctx, replaced); err != nil { + t.Fatal(err) + } + if replace == "zone" { + zones[zone.Name] = zone + } + reconcile() + assertAccepted(false) + if association.Status.ResolvedDNSZoneRef.UID != originalZoneUID || policy.Status.ResolvedAdditionalNames[0].DNSZoneRef.UID != originalZoneUID || policy.Status.ResolvedResolverContextRef.UID != originalContextUID { + t.Fatal("replacement inherited lifetime pins") + } + // Recreate both customer resources to explicitly attach the replacement. + if err := cl.Delete(ctx, association); err != nil { + t.Fatal(err) + } + association.ResourceVersion = "" + association.Status = dnsv1alpha1.DNSZoneAssociationStatus{} + if err := cl.Create(ctx, association); err != nil { + t.Fatal(err) + } + reconcile() + if condition := apimeta.FindStatusCondition(policy.Status.Conditions, "Accepted"); condition == nil || condition.Status != metav1.ConditionFalse { + t.Fatal("reattaching an association silently rebound the policy") + } + if err := cl.Delete(ctx, policy); err != nil { + t.Fatal(err) + } + policy.ResourceVersion = "" + policy.Status = dnsv1alpha1.DNSNamingPolicyStatus{} + if err := cl.Create(ctx, policy); err != nil { + t.Fatal(err) + } + reconcile() + assertAccepted(true) + }) + } +} diff --git a/internal/internaldns/controlplane/naming.go b/internal/internaldns/controlplane/naming.go index 52ce3ca7..af6aae30 100644 --- a/internal/internaldns/controlplane/naming.go +++ b/internal/internaldns/controlplane/naming.go @@ -11,6 +11,7 @@ import ( dnsv1alpha1 "go.miloapis.com/dns-operator/api/v1alpha1" apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -24,7 +25,8 @@ type ResolvedName struct { // ResolveNaming returns the stable platform namespace first and authorized // additive policy names after it. It never selects an arbitrary associated // customer zone. -func ResolveNaming(ctx context.Context, c client.Client, namespace string, projectUID, vpcUID types.UID, class dnsv1alpha1.DNSRegistrationClass, allocatedName string) ([]ResolvedName, error) { +// Selectors match DNSRegistration metadata labels supplied by the publisher. +func ResolveNaming(ctx context.Context, c client.Client, namespace string, projectUID, vpcUID types.UID, registrationLabels map[string]string, allocatedName string) ([]ResolvedName, error) { label := dnsLabel(allocatedName) if label == "" { return nil, fmt.Errorf("allocated name %q has no DNS-safe label", allocatedName) @@ -45,6 +47,7 @@ func ResolveNaming(ctx context.Context, c client.Client, namespace string, proje if len(out) == 0 { return nil, fmt.Errorf("managed DNS namespace for VPC %s is not ready", vpcUID) } + seen := map[string]bool{string(out[0].DNSZoneRef.UID) + "\x00" + out[0].Name: true} var policies dnsv1alpha1.DNSNamingPolicyList if err := c.List(ctx, &policies, client.InNamespace(namespace)); err != nil { return nil, err @@ -58,18 +61,32 @@ func ResolveNaming(ctx context.Context, c client.Client, namespace string, proje for i := range policies.Items { p := &policies.Items[i] accepted := apimeta.FindStatusCondition(p.Status.Conditions, dnsValueAccepted) - if p.Status.ResolvedVPCRef.UID != vpcUID || accepted == nil || accepted.Status != metav1.ConditionTrue { + if p.Status.ResolvedVPCRef.UID != vpcUID || !p.DeletionTimestamp.IsZero() || accepted == nil || accepted.Status != metav1.ConditionTrue || accepted.ObservedGeneration != p.Generation { continue } for _, rule := range p.Status.ResolvedAdditionalNames { - if rule.RegistrationClass == class { - out = append(out, ResolvedName{DNSZoneRef: rule.DNSZoneRef, Name: label + "." + canonicalName(rule.NamePrefix)}) + selector, err := namingSelector(rule.Selector) + if err != nil { + return nil, fmt.Errorf("naming policy %s has an invalid selector: %w", p.Name, err) + } + name := label + "." + canonicalName(rule.NamePrefix) + key := string(rule.DNSZoneRef.UID) + "\x00" + name + if selector.Matches(labels.Set(registrationLabels)) && !seen[key] { + out = append(out, ResolvedName{DNSZoneRef: rule.DNSZoneRef, Name: name}) + seen[key] = true } } } return out, nil } +func namingSelector(selector *metav1.LabelSelector) (labels.Selector, error) { + if selector == nil { + return labels.Everything(), nil + } + return metav1.LabelSelectorAsSelector(selector) +} + func dnsLabel(s string) string { s = strings.ToLower(strings.TrimSpace(s)) var b strings.Builder diff --git a/internal/internaldns/controlplane/naming_selector_test.go b/internal/internaldns/controlplane/naming_selector_test.go new file mode 100644 index 00000000..22dd2fe7 --- /dev/null +++ b/internal/internaldns/controlplane/naming_selector_test.go @@ -0,0 +1,139 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package controlplane + +import ( + "context" + "reflect" + "testing" + "time" + + dnsv1alpha1 "go.miloapis.com/dns-operator/api/v1alpha1" + apimeta "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func TestNamingSelectorsMatchRegistrationLabelsWithinContext(t *testing.T) { + scheme := runtime.NewScheme() + if err := dnsv1alpha1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + managed := &dnsv1alpha1.DNSManagedNamespace{ObjectMeta: metav1.ObjectMeta{Name: "managed", Namespace: "project"}, Spec: dnsv1alpha1.DNSManagedNamespaceSpec{ProjectUID: "project-a", VPCRef: dnsv1alpha1.DNSObjectReference{UID: "consumer-a"}}, Status: dnsv1alpha1.DNSManagedNamespaceStatus{DNSZoneRef: dnsv1alpha1.DNSObjectReference{Name: "canonical", UID: "canonical-zone"}, Conditions: []metav1.Condition{{Type: "Accepted", Status: metav1.ConditionTrue}}}} + zone := dnsv1alpha1.DNSObjectReference{Name: "custom", UID: "custom-zone"} + storefront := &metav1.LabelSelector{MatchLabels: map[string]string{"app": "storefront"}} + policy := &dnsv1alpha1.DNSNamingPolicy{ObjectMeta: metav1.ObjectMeta{Name: "names", Namespace: "project", Generation: 1}, Status: dnsv1alpha1.DNSNamingPolicyStatus{ResolvedVPCRef: dnsv1alpha1.DNSObjectReference{UID: "consumer-a"}, Conditions: []metav1.Condition{{Type: "Accepted", Status: metav1.ConditionTrue, ObservedGeneration: 1}}, ResolvedAdditionalNames: []dnsv1alpha1.DNSResolvedAdditionalNameRule{ + {DNSZoneRef: zone, NamePrefix: "all"}, + {Selector: &metav1.LabelSelector{}, DNSZoneRef: zone, NamePrefix: "empty"}, + {Selector: storefront, DNSZoneRef: zone, NamePrefix: "storefront"}, + {Selector: &metav1.LabelSelector{MatchExpressions: []metav1.LabelSelectorRequirement{{Key: "app", Operator: metav1.LabelSelectorOpIn, Values: []string{"storefront"}}}}, DNSZoneRef: zone, NamePrefix: "storefront"}, + {Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"app": "storefront"}, MatchExpressions: []metav1.LabelSelectorRequirement{{Key: "tier", Operator: metav1.LabelSelectorOpIn, Values: []string{"backend", "worker"}}, {Key: "disabled", Operator: metav1.LabelSelectorOpDoesNotExist}}}, DNSZoneRef: zone, NamePrefix: "backend"}, + }}} + other := policy.DeepCopy() + other.Name = "other-context" + other.Status.ResolvedVPCRef.UID = "consumer-b" + other.Status.ResolvedAdditionalNames = []dnsv1alpha1.DNSResolvedAdditionalNameRule{{DNSZoneRef: zone, NamePrefix: "other-context"}} + stale := policy.DeepCopy() + stale.Name = "stale-policy" + stale.Generation++ + stale.Status.ResolvedAdditionalNames = []dnsv1alpha1.DNSResolvedAdditionalNameRule{{DNSZoneRef: zone, NamePrefix: "stale"}} + foreign := policy.DeepCopy() + foreign.Name = "other-namespace" + foreign.Namespace = "other-project" + foreign.Status.ResolvedAdditionalNames = []dnsv1alpha1.DNSResolvedAdditionalNameRule{{DNSZoneRef: zone, NamePrefix: "foreign"}} + cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(managed, policy, other, stale, foreign).Build() + for _, tc := range []struct { + name string + labels map[string]string + want []string + }{ + {"unlabeled", nil, []string{"web-01", "web-01.all", "web-01.empty"}}, + {"matching publisher one", map[string]string{"app": "storefront", "publisher": "one"}, []string{"web-01", "web-01.all", "web-01.empty", "web-01.storefront"}}, + {"matching publisher two", map[string]string{"app": "storefront", "publisher": "two"}, []string{"web-01", "web-01.all", "web-01.empty", "web-01.storefront"}}, + {"different app", map[string]string{"app": "billing"}, []string{"web-01", "web-01.all", "web-01.empty"}}, + {"set expression", map[string]string{"app": "storefront", "tier": "worker"}, []string{"web-01", "web-01.all", "web-01.empty", "web-01.storefront", "web-01.backend"}}, + {"excluded label", map[string]string{"app": "storefront", "tier": "worker", "disabled": "true"}, []string{"web-01", "web-01.all", "web-01.empty", "web-01.storefront"}}, + } { + t.Run(tc.name, func(t *testing.T) { + names, err := ResolveNaming(context.Background(), cl, "project", "project-a", "consumer-a", tc.labels, "Web 01") + if err != nil { + t.Fatal(err) + } + got := make([]string, len(names)) + for i, n := range names { + got[i] = n.Name + } + if !reflect.DeepEqual(got, tc.want) { + t.Fatalf("got %v want %v", got, tc.want) + } + if !names[0].Canonical { + t.Fatal("default name was not retained") + } + }) + } + t.Run("malformed accepted selector", func(t *testing.T) { + if err := cl.Get(context.Background(), client.ObjectKeyFromObject(policy), policy); err != nil { + t.Fatal(err) + } + policy.Status.ResolvedAdditionalNames[0].Selector = &metav1.LabelSelector{MatchExpressions: []metav1.LabelSelectorRequirement{{Key: "app", Operator: "Unsupported"}}} + if err := cl.Update(context.Background(), policy); err != nil { + t.Fatal(err) + } + names, err := ResolveNaming(context.Background(), cl, "project", "project-a", "consumer-a", nil, "Web 01") + if err == nil || len(names) != 0 { + t.Fatalf("malformed accepted selector produced names: %v, %v", names, err) + } + }) +} + +func TestNamingPolicyValidatesSelectorsAndDuplicateRules(t *testing.T) { + ctx := context.Background() + now := time.Now() + scheme := runtime.NewScheme() + if err := dnsv1alpha1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + resolver := &dnsv1alpha1.DNSResolverContext{ObjectMeta: metav1.ObjectMeta{Name: "context", Namespace: "project", UID: "context-uid", Generation: 1}, Spec: dnsv1alpha1.DNSResolverContextSpec{ConsumerID: "consumer"}} + setCondition(&resolver.Status.Conditions, "Ready", metav1.ConditionTrue, "Ready", "", 1, now) + zone := &dnsv1alpha1.DNSZone{ObjectMeta: metav1.ObjectMeta{Name: "zone", UID: "zone-uid", Generation: 1}, Spec: dnsv1alpha1.DNSZoneSpec{Visibility: dnsv1alpha1.DNSZoneVisibilityPrivate}} + association := dnsv1alpha1.DNSZoneAssociation{ObjectMeta: metav1.ObjectMeta{Generation: 1}, Status: dnsv1alpha1.DNSZoneAssociationStatus{ResolvedDNSZoneRef: dnsv1alpha1.DNSObjectReference{UID: zone.UID}, ResolvedResolverContextRef: dnsv1alpha1.DNSObjectReference{UID: resolver.UID}}} + setCondition(&association.Status.Conditions, "Accepted", metav1.ConditionTrue, "Accepted", "", 1, now) + for _, tc := range []struct { + name string + selectors []*metav1.LabelSelector + valid bool + }{ + {"omitted", []*metav1.LabelSelector{nil}, true}, + {"empty", []*metav1.LabelSelector{{}}, true}, + {"labels", []*metav1.LabelSelector{{MatchLabels: map[string]string{"app": "storefront"}}}, true}, + {"invalid-key", []*metav1.LabelSelector{{MatchLabels: map[string]string{"bad key": "value"}}}, false}, + {"invalid-operator", []*metav1.LabelSelector{{MatchExpressions: []metav1.LabelSelectorRequirement{{Key: "app", Operator: "Unsupported"}}}}, false}, + {"missing-values", []*metav1.LabelSelector{{MatchExpressions: []metav1.LabelSelectorRequirement{{Key: "app", Operator: metav1.LabelSelectorOpIn}}}}, false}, + {"duplicate", []*metav1.LabelSelector{nil, {}}, false}, + {"overlapping", []*metav1.LabelSelector{nil, {MatchLabels: map[string]string{"app": "storefront"}}}, true}, + } { + t.Run(tc.name, func(t *testing.T) { + policy := &dnsv1alpha1.DNSNamingPolicy{ObjectMeta: metav1.ObjectMeta{Name: tc.name, Namespace: "project", Generation: 1}, Spec: dnsv1alpha1.DNSNamingPolicySpec{ResolverContextRef: dnsv1alpha1.DNSNameReference{Name: resolver.Name}}} + for _, selector := range tc.selectors { + policy.Spec.AdditionalNames = append(policy.Spec.AdditionalNames, dnsv1alpha1.DNSAdditionalNameRule{Selector: selector, DNSZoneRef: dnsv1alpha1.DNSNameReference{Name: zone.Name}, NamePrefix: "application"}) + } + cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(resolver, policy).WithStatusSubresource(policy).Build() + r := &Reconciler{Client: cl} + if err := r.reconcileNamingPolicies(ctx, []dnsv1alpha1.DNSNamingPolicy{*policy}, []dnsv1alpha1.DNSZoneAssociation{association}, map[string]*dnsv1alpha1.DNSZone{zone.Name: zone}, now); err != nil { + t.Fatal(err) + } + if err := cl.Get(ctx, client.ObjectKeyFromObject(policy), policy); err != nil { + t.Fatal(err) + } + condition := apimeta.FindStatusCondition(policy.Status.Conditions, "Accepted") + if condition == nil || (condition.Status == metav1.ConditionTrue) != tc.valid { + t.Fatalf("accepted=%v want %v", condition, tc.valid) + } + if policy.Status.ResolvedAdditionalNames[0].DNSZoneRef.UID != zone.UID { + t.Fatal("selector validation lost the zone lifetime pin") + } + }) + } +} diff --git a/internal/internaldns/controlplane/naming_test.go b/internal/internaldns/controlplane/naming_test.go index e5992041..1e00f3e1 100644 --- a/internal/internaldns/controlplane/naming_test.go +++ b/internal/internaldns/controlplane/naming_test.go @@ -16,9 +16,9 @@ func TestResolveNamingReturnsCanonicalThenAuthorizedAliases(t *testing.T) { t.Fatal(err) } managed := &dnsv1alpha1.DNSManagedNamespace{ObjectMeta: metav1.ObjectMeta{Name: "managed", Namespace: "project"}, Spec: dnsv1alpha1.DNSManagedNamespaceSpec{ProjectUID: "project-a", VPCRef: dnsv1alpha1.DNSObjectReference{Name: "prod", UID: "vpc-a"}}, Status: dnsv1alpha1.DNSManagedNamespaceStatus{DNSZoneRef: dnsv1alpha1.DNSObjectReference{Name: "canonical", UID: "zone-c"}, Conditions: []metav1.Condition{{Type: "Accepted", Status: metav1.ConditionTrue}}}} - policy := &dnsv1alpha1.DNSNamingPolicy{ObjectMeta: metav1.ObjectMeta{Name: "custom", Namespace: "project"}, Spec: dnsv1alpha1.DNSNamingPolicySpec{Priority: 10}, Status: dnsv1alpha1.DNSNamingPolicyStatus{ResolvedVPCRef: dnsv1alpha1.DNSObjectReference{Name: "prod", UID: "vpc-a"}, ResolvedAdditionalNames: []dnsv1alpha1.DNSResolvedAdditionalNameRule{{RegistrationClass: dnsv1alpha1.DNSRegistrationClassInstanceIdentity, DNSZoneRef: dnsv1alpha1.DNSObjectReference{Name: "custom", UID: "zone-x"}, NamePrefix: "instances"}}, Conditions: []metav1.Condition{{Type: "Accepted", Status: metav1.ConditionTrue}}}} + policy := &dnsv1alpha1.DNSNamingPolicy{ObjectMeta: metav1.ObjectMeta{Name: "custom", Namespace: "project"}, Spec: dnsv1alpha1.DNSNamingPolicySpec{Priority: 10}, Status: dnsv1alpha1.DNSNamingPolicyStatus{ResolvedVPCRef: dnsv1alpha1.DNSObjectReference{Name: "prod", UID: "vpc-a"}, ResolvedAdditionalNames: []dnsv1alpha1.DNSResolvedAdditionalNameRule{{DNSZoneRef: dnsv1alpha1.DNSObjectReference{Name: "custom", UID: "zone-x"}, NamePrefix: "instances"}}, Conditions: []metav1.Condition{{Type: "Accepted", Status: metav1.ConditionTrue}}}} c := fake.NewClientBuilder().WithScheme(s).WithObjects(managed, policy).Build() - names, err := ResolveNaming(context.Background(), c, "project", "project-a", "vpc-a", dnsv1alpha1.DNSRegistrationClassInstanceIdentity, "Web 01") + names, err := ResolveNaming(context.Background(), c, "project", "project-a", "vpc-a", nil, "Web 01") if err != nil { t.Fatal(err) } diff --git a/internal/internaldns/controlplane/publication_reconciler_test.go b/internal/internaldns/controlplane/publication_reconciler_test.go index 47d72c8b..77f0f649 100644 --- a/internal/internaldns/controlplane/publication_reconciler_test.go +++ b/internal/internaldns/controlplane/publication_reconciler_test.go @@ -84,8 +84,8 @@ func TestQuiescentSurvivorRenewsOwnershipBeyondLeaseWindow(t *testing.T) { assoc := input.Associations[0].DeepCopy() assoc.Namespace = "project" assoc.Name = "zone-prod" - assoc.Spec.VPCRef = dnsv1alpha1.DNSObjectReference{} - assoc.Spec.ResolverContextRef = dnsv1alpha1.DNSObjectReference{Name: "prod", UID: "vpc-a"} + assoc.Spec.VPCRef = dnsv1alpha1.DNSNameReference{} + assoc.Spec.ResolverContextRef = dnsv1alpha1.DNSNameReference{Name: "prod"} assoc.Status = dnsv1alpha1.DNSZoneAssociationStatus{} reg := input.Registrations[0].DeepCopy() reg.Namespace = "project" @@ -292,8 +292,8 @@ func TestReconcilerPersistsFencedMultiRegionOutbox(t *testing.T) { assoc := fixture(now).Associations[0].DeepCopy() assoc.Namespace = "project" assoc.Name = "zone-prod" - assoc.Spec.VPCRef = dnsv1alpha1.DNSObjectReference{} - assoc.Spec.ResolverContextRef = dnsv1alpha1.DNSObjectReference{Name: "prod", UID: "vpc-a"} + assoc.Spec.VPCRef = dnsv1alpha1.DNSNameReference{} + assoc.Spec.ResolverContextRef = dnsv1alpha1.DNSNameReference{Name: "prod"} assoc.Status = dnsv1alpha1.DNSZoneAssociationStatus{} reg := fixture(now).Registrations[0].DeepCopy() reg.Namespace = "project" diff --git a/internal/internaldns/runtime/deployment_permissions_test.go b/internal/internaldns/runtime/deployment_permissions_test.go index ac3b622b..1c1cefc8 100644 --- a/internal/internaldns/runtime/deployment_permissions_test.go +++ b/internal/internaldns/runtime/deployment_permissions_test.go @@ -285,10 +285,10 @@ func verifyProjectReferenceSchemas(t *testing.T, ctx context.Context, cl client. {"registration-valid", &dnsv1alpha1.DNSRegistration{Spec: dnsv1alpha1.DNSRegistrationSpec{DNSZoneRef: ref, Name: "api", RecordTypes: []dnsv1alpha1.RRType{dnsv1alpha1.RRTypeA}, TTLSeconds: 30, PublicationPolicy: dnsv1alpha1.DNSPublicationPolicyEligibleContributions}}, true}, {"registration-unpinned", &dnsv1alpha1.DNSRegistration{Spec: dnsv1alpha1.DNSRegistrationSpec{DNSZoneRef: dnsv1alpha1.DNSObjectReference{Name: zone.Name}, Name: "api", RecordTypes: []dnsv1alpha1.RRType{dnsv1alpha1.RRTypeA}, TTLSeconds: 30, PublicationPolicy: dnsv1alpha1.DNSPublicationPolicyEligibleContributions}}, false}, {"registration-ttl31", &dnsv1alpha1.DNSRegistration{Spec: dnsv1alpha1.DNSRegistrationSpec{DNSZoneRef: ref, Name: "api", RecordTypes: []dnsv1alpha1.RRType{dnsv1alpha1.RRTypeA}, TTLSeconds: 31, PublicationPolicy: dnsv1alpha1.DNSPublicationPolicyEligibleContributions}}, false}, - {"association-valid", &dnsv1alpha1.DNSZoneAssociation{Spec: dnsv1alpha1.DNSZoneAssociationSpec{DNSZoneRef: ref, ResolverContextRef: contextRef}}, true}, - {"association-unpinned", &dnsv1alpha1.DNSZoneAssociation{Spec: dnsv1alpha1.DNSZoneAssociationSpec{DNSZoneRef: dnsv1alpha1.DNSObjectReference{Name: zone.Name}, ResolverContextRef: contextRef}}, false}, - {"naming-valid", &dnsv1alpha1.DNSNamingPolicy{Spec: dnsv1alpha1.DNSNamingPolicySpec{ResolverContextRef: contextRef, AdditionalNames: []dnsv1alpha1.DNSAdditionalNameRule{{RegistrationClass: dnsv1alpha1.DNSRegistrationClassServiceDiscovery, DNSZoneRef: ref, NamePrefix: "services"}}}}, true}, - {"naming-unpinned", &dnsv1alpha1.DNSNamingPolicy{Spec: dnsv1alpha1.DNSNamingPolicySpec{ResolverContextRef: contextRef, AdditionalNames: []dnsv1alpha1.DNSAdditionalNameRule{{RegistrationClass: dnsv1alpha1.DNSRegistrationClassServiceDiscovery, DNSZoneRef: dnsv1alpha1.DNSObjectReference{Name: zone.Name}, NamePrefix: "services"}}}}, false}, + {"association-valid", &dnsv1alpha1.DNSZoneAssociation{Spec: dnsv1alpha1.DNSZoneAssociationSpec{DNSZoneRef: dnsv1alpha1.DNSNameReference{Name: ref.Name}, ResolverContextRef: dnsv1alpha1.DNSNameReference{Name: contextRef.Name}}}, true}, + {"association-empty-zone", &dnsv1alpha1.DNSZoneAssociation{Spec: dnsv1alpha1.DNSZoneAssociationSpec{DNSZoneRef: dnsv1alpha1.DNSNameReference{}, ResolverContextRef: dnsv1alpha1.DNSNameReference{Name: contextRef.Name}}}, false}, + {"naming-valid", &dnsv1alpha1.DNSNamingPolicy{Spec: dnsv1alpha1.DNSNamingPolicySpec{ResolverContextRef: dnsv1alpha1.DNSNameReference{Name: contextRef.Name}, AdditionalNames: []dnsv1alpha1.DNSAdditionalNameRule{{DNSZoneRef: dnsv1alpha1.DNSNameReference{Name: ref.Name}, NamePrefix: "services"}}}}, true}, + {"naming-empty-zone", &dnsv1alpha1.DNSNamingPolicy{Spec: dnsv1alpha1.DNSNamingPolicySpec{ResolverContextRef: dnsv1alpha1.DNSNameReference{Name: contextRef.Name}, AdditionalNames: []dnsv1alpha1.DNSAdditionalNameRule{{DNSZoneRef: dnsv1alpha1.DNSNameReference{}, NamePrefix: "services"}}}}, false}, } for _, tc := range cases { tc.object.SetName(tc.name) diff --git a/internal/internaldns/serving/verify_test.go b/internal/internaldns/serving/verify_test.go index e44120b6..6650cc9a 100644 --- a/internal/internaldns/serving/verify_test.go +++ b/internal/internaldns/serving/verify_test.go @@ -321,7 +321,18 @@ func TestZeroZoneContextProvesRecursionOverUDPAndTCP(t *testing.T) { if err := (DNSVerifier{Config: DNSVerifierConfig{Timeout: time.Second}}).Verify(context.Background(), binding); err != nil { t.Fatal(err) } - if len(seen) != 2 { - t.Fatalf("probe count=%d", len(seen)) + deadline := time.NewTimer(time.Second) + defer deadline.Stop() + transports := make(map[string]bool, 2) + for range 2 { + select { + case transport := <-seen: + transports[transport] = true + case <-deadline.C: + t.Fatalf("timed out waiting for UDP and TCP probes: %v", transports) + } + } + if !transports[networkUDP] || !transports[networkTCP] { + t.Fatalf("probe transports = %v, want UDP and TCP", transports) } } diff --git a/test/internaldns/kubernetes/qualify/main.go b/test/internaldns/kubernetes/qualify/main.go index 16ee1bca..e84f7c9d 100644 --- a/test/internaldns/kubernetes/qualify/main.go +++ b/test/internaldns/kubernetes/qualify/main.go @@ -364,7 +364,7 @@ func (s *suite) prepare(p *project) error { } func (s *suite) associate(p *project, zone *dnsv1.DNSZone, name string) error { - association := &dnsv1.DNSZoneAssociation{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: p.Namespace}, Spec: dnsv1.DNSZoneAssociationSpec{DNSZoneRef: objectRef(zone), ResolverContextRef: objectRef(&p.resolverContext)}} + association := &dnsv1.DNSZoneAssociation{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: p.Namespace}, Spec: dnsv1.DNSZoneAssociationSpec{DNSZoneRef: dnsv1.DNSNameReference{Name: zone.Name}, ResolverContextRef: dnsv1.DNSNameReference{Name: p.resolverContext.Name}}} if err := p.customer.Create(s.ctx, association); err != nil { return err } From 2b2ea81c72c8f75564faa4291ad4df558c4e0f9f Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Mon, 12 Oct 2026 16:15:58 -0500 Subject: [PATCH 02/19] Validate registration selector expressions in the API schema --- ...ernaldns_naming_validation_envtest_test.go | 79 +++++++++++++++++++ api/v1alpha1/internaldns_project_types.go | 55 ++++++++++--- .../internaldns_validation_envtest_test.go | 1 + api/v1alpha1/zz_generated.deepcopy.go | 51 +++++++++++- ...orking.miloapis.com_dnsnamingpolicies.yaml | 46 +++++------ ...king.miloapis.com_dnszoneassociations.yaml | 3 + internal/internaldns/admission/handler.go | 2 +- .../internaldns/admission/handler_test.go | 20 +---- .../controlplane/context_reconciler.go | 4 +- .../controlplane/named_reference_api_test.go | 2 +- .../controlplane/naming_selector_test.go | 18 ++--- 11 files changed, 220 insertions(+), 61 deletions(-) create mode 100644 api/v1alpha1/internaldns_naming_validation_envtest_test.go diff --git a/api/v1alpha1/internaldns_naming_validation_envtest_test.go b/api/v1alpha1/internaldns_naming_validation_envtest_test.go new file mode 100644 index 00000000..e63e42aa --- /dev/null +++ b/api/v1alpha1/internaldns_naming_validation_envtest_test.go @@ -0,0 +1,79 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package v1alpha1 + +import ( + "context" + "strings" + "testing" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +func validateNamingAPISchemas(t *testing.T, cl client.Client) { + t.Helper() + ctx := context.Background() + for _, tc := range []struct { + name string + selector *DNSRegistrationSelector + allowed bool + }{ + {"omitted-selector", nil, true}, + {"empty-selector", &DNSRegistrationSelector{}, true}, + {"in-selector", &DNSRegistrationSelector{MatchExpressions: []DNSRegistrationSelectorRequirement{{Key: "app", Operator: metav1.LabelSelectorOpIn, Values: []string{"web"}}}}, true}, + {"exists-selector", &DNSRegistrationSelector{MatchExpressions: []DNSRegistrationSelectorRequirement{{Key: "app", Operator: metav1.LabelSelectorOpExists}}}, true}, + {"invalid-operator", &DNSRegistrationSelector{MatchExpressions: []DNSRegistrationSelectorRequirement{{Key: "app", Operator: "Unsupported"}}}, false}, + {"missing-values", &DNSRegistrationSelector{MatchExpressions: []DNSRegistrationSelectorRequirement{{Key: "app", Operator: metav1.LabelSelectorOpIn}}}, false}, + {"unexpected-values", &DNSRegistrationSelector{MatchExpressions: []DNSRegistrationSelectorRequirement{{Key: "app", Operator: metav1.LabelSelectorOpExists, Values: []string{"web"}}}}, false}, + {"invalid-value", &DNSRegistrationSelector{MatchExpressions: []DNSRegistrationSelectorRequirement{{Key: "app", Operator: metav1.LabelSelectorOpIn, Values: []string{"invalid value"}}}}, false}, + {"long-value", &DNSRegistrationSelector{MatchExpressions: []DNSRegistrationSelectorRequirement{{Key: "app", Operator: metav1.LabelSelectorOpIn, Values: []string{strings.Repeat("x", 64)}}}}, false}, + } { + t.Run(tc.name, func(t *testing.T) { + p := &DNSNamingPolicy{ObjectMeta: metav1.ObjectMeta{Name: tc.name, Namespace: "validation"}, Spec: DNSNamingPolicySpec{ResolverContextRef: DNSNameReference{Name: "context"}, AdditionalNames: []DNSAdditionalNameRule{{DNSZoneRef: DNSNameReference{Name: "zone"}, Selector: tc.selector, NamePrefix: "hosts"}}}} + err := cl.Create(ctx, p) + if tc.allowed { + if err != nil { + t.Fatal(err) + } + } else if !apierrors.IsInvalid(err) { + t.Fatalf("expected API validation rejection, got %v", err) + } + }) + } + t.Run("selector-expression-limit", func(t *testing.T) { + p := &DNSNamingPolicy{ObjectMeta: metav1.ObjectMeta{Name: "expression-limit", Namespace: "validation"}, Spec: DNSNamingPolicySpec{ResolverContextRef: DNSNameReference{Name: "context"}, AdditionalNames: []DNSAdditionalNameRule{{DNSZoneRef: DNSNameReference{Name: "zone"}, NamePrefix: "hosts", Selector: &DNSRegistrationSelector{}}}}} + for range 64 { + p.Spec.AdditionalNames[0].Selector.MatchExpressions = append(p.Spec.AdditionalNames[0].Selector.MatchExpressions, DNSRegistrationSelectorRequirement{Key: "app", Operator: metav1.LabelSelectorOpExists}) + } + if err := cl.Create(ctx, p); err != nil { + t.Fatal(err) + } + p.Spec.AdditionalNames[0].Selector.MatchExpressions = append(p.Spec.AdditionalNames[0].Selector.MatchExpressions, DNSRegistrationSelectorRequirement{Key: "app", Operator: metav1.LabelSelectorOpExists}) + if err := cl.Update(ctx, p); !apierrors.IsInvalid(err) { + t.Fatalf("expected expression limit rejection, got %v", err) + } + }) + for _, tc := range []struct { + name string + vpc, context DNSNameReference + allowed bool + }{ + {"one-context", DNSNameReference{}, DNSNameReference{Name: "context"}, true}, + {"no-consumer", DNSNameReference{}, DNSNameReference{}, false}, + {"both-consumers", DNSNameReference{Name: "vpc"}, DNSNameReference{Name: "context"}, false}, + } { + t.Run(tc.name, func(t *testing.T) { + a := &DNSZoneAssociation{ObjectMeta: metav1.ObjectMeta{Name: tc.name, Namespace: "validation"}, Spec: DNSZoneAssociationSpec{DNSZoneRef: DNSNameReference{Name: "zone"}, VPCRef: tc.vpc, ResolverContextRef: tc.context}} + err := cl.Create(ctx, a) + if tc.allowed { + if err != nil { + t.Fatal(err) + } + } else if !apierrors.IsInvalid(err) { + t.Fatalf("expected API validation rejection, got %v", err) + } + }) + } +} diff --git a/api/v1alpha1/internaldns_project_types.go b/api/v1alpha1/internaldns_project_types.go index c5586283..b4114de6 100644 --- a/api/v1alpha1/internaldns_project_types.go +++ b/api/v1alpha1/internaldns_project_types.go @@ -25,6 +25,7 @@ type DNSObjectReference struct { // DNSNameReference selects a resource in the same project namespace. // DNS records its resolved lifetime in controller-owned status. type DNSNameReference struct { + // +kubebuilder:validation:MaxLength=253 Name string `json:"name"` } @@ -329,21 +330,57 @@ type DNSRecordContributionList struct { // +kubebuilder:validation:XValidation:rule="(has(self.vpcRef) && has(self.vpcRef.name) && self.vpcRef.name.size() > 0) != (has(self.resolverContextRef) && has(self.resolverContextRef.name) && self.resolverContextRef.name.size() > 0)",message="exactly one named vpcRef or resolverContextRef is required" type DNSNamingPolicySpec struct { - VPCRef DNSNameReference `json:"vpcRef,omitempty"` - ResolverContextRef DNSNameReference `json:"resolverContextRef,omitempty"` - AdditionalNames []DNSAdditionalNameRule `json:"additionalNames"` - DNSZoneRef DNSNameReference `json:"dnsZoneRef,omitempty"` - ScopePrefix string `json:"scopePrefix,omitempty"` - Priority int32 `json:"priority,omitempty"` + VPCRef DNSNameReference `json:"vpcRef,omitempty"` + ResolverContextRef DNSNameReference `json:"resolverContextRef,omitempty"` + // +kubebuilder:validation:MaxItems=256 + AdditionalNames []DNSAdditionalNameRule `json:"additionalNames"` + DNSZoneRef DNSNameReference `json:"dnsZoneRef,omitempty"` + ScopePrefix string `json:"scopePrefix,omitempty"` + Priority int32 `json:"priority,omitempty"` } // +kubebuilder:validation:XValidation:rule="self.dnsZoneRef.name.size() > 0",message="dnsZoneRef requires a name" type DNSAdditionalNameRule struct { // Selector matches DNSRegistration metadata labels within this DNS context. // An omitted or empty selector matches every registration in the context. - Selector *metav1.LabelSelector `json:"selector,omitempty"` - DNSZoneRef DNSNameReference `json:"dnsZoneRef"` - NamePrefix string `json:"namePrefix"` + Selector *DNSRegistrationSelector `json:"selector,omitempty"` + DNSZoneRef DNSNameReference `json:"dnsZoneRef"` + NamePrefix string `json:"namePrefix"` +} + +// DNSRegistrationSelector matches registration labels using Kubernetes selector semantics. +type DNSRegistrationSelector struct { + // +kubebuilder:validation:MaxProperties=64 + MatchLabels map[string]string `json:"matchLabels,omitempty"` + // +kubebuilder:validation:MaxItems=64 + MatchExpressions []DNSRegistrationSelectorRequirement `json:"matchExpressions,omitempty"` +} + +// +kubebuilder:validation:XValidation:rule="(self.operator in ['In', 'NotIn']) ? (has(self.values) && self.values.size() > 0) : (!has(self.values) || self.values.size() == 0)",message="selector operator requires compatible values" +type DNSRegistrationSelectorRequirement struct { + // +kubebuilder:validation:MinLength=1 + // +kubebuilder:validation:MaxLength=317 + Key string `json:"key"` + // +kubebuilder:validation:Enum=In;NotIn;Exists;DoesNotExist + Operator metav1.LabelSelectorOperator `json:"operator"` + // +kubebuilder:validation:MaxItems=64 + // +kubebuilder:validation:items:MaxLength=63 + // +kubebuilder:validation:items:Pattern=`^([A-Za-z0-9]([-A-Za-z0-9_.]{0,61}[A-Za-z0-9])?)?$` + Values []string `json:"values,omitempty"` +} + +// AsLabelSelector converts the API selector for Kubernetes label matching. +func (s *DNSRegistrationSelector) AsLabelSelector() *metav1.LabelSelector { + if s == nil { + return nil + } + out := &metav1.LabelSelector{MatchLabels: s.MatchLabels} + for _, requirement := range s.MatchExpressions { + out.MatchExpressions = append(out.MatchExpressions, metav1.LabelSelectorRequirement{ + Key: requirement.Key, Operator: requirement.Operator, Values: requirement.Values, + }) + } + return out } type DNSNamingPolicyStatus struct { diff --git a/api/v1alpha1/internaldns_validation_envtest_test.go b/api/v1alpha1/internaldns_validation_envtest_test.go index 4426872b..95c8c01a 100644 --- a/api/v1alpha1/internaldns_validation_envtest_test.go +++ b/api/v1alpha1/internaldns_validation_envtest_test.go @@ -144,5 +144,6 @@ func TestPrivateDNSValidationAtAPIServer(t *testing.T) { requireInvalid(t, cl.Update(ctx, a)) }) validateServiceDNSContracts(t, cl) + validateNamingAPISchemas(t, cl) } diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 2b6b6624..ce42c420 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -93,7 +93,7 @@ func (in *DNSAdditionalNameRule) DeepCopyInto(out *DNSAdditionalNameRule) { *out = *in if in.Selector != nil { in, out := &in.Selector, &out.Selector - *out = new(v1.LabelSelector) + *out = new(DNSRegistrationSelector) (*in).DeepCopyInto(*out) } out.DNSZoneRef = in.DNSZoneRef @@ -1147,6 +1147,55 @@ func (in *DNSRegistrationList) DeepCopyObject() runtime.Object { return nil } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *DNSRegistrationSelector) DeepCopyInto(out *DNSRegistrationSelector) { + *out = *in + if in.MatchLabels != nil { + in, out := &in.MatchLabels, &out.MatchLabels + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } + if in.MatchExpressions != nil { + in, out := &in.MatchExpressions, &out.MatchExpressions + *out = make([]DNSRegistrationSelectorRequirement, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new DNSRegistrationSelector. +func (in *DNSRegistrationSelector) DeepCopy() *DNSRegistrationSelector { + if in == nil { + return nil + } + out := new(DNSRegistrationSelector) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *DNSRegistrationSelectorRequirement) DeepCopyInto(out *DNSRegistrationSelectorRequirement) { + *out = *in + if in.Values != nil { + in, out := &in.Values, &out.Values + *out = make([]string, len(*in)) + copy(*out, *in) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new DNSRegistrationSelectorRequirement. +func (in *DNSRegistrationSelectorRequirement) DeepCopy() *DNSRegistrationSelectorRequirement { + if in == nil { + return nil + } + out := new(DNSRegistrationSelectorRequirement) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *DNSRegistrationSpec) DeepCopyInto(out *DNSRegistrationSpec) { *out = *in diff --git a/config/crd/bases/dns.networking.miloapis.com_dnsnamingpolicies.yaml b/config/crd/bases/dns.networking.miloapis.com_dnsnamingpolicies.yaml index 3d508e60..6c0d2294 100644 --- a/config/crd/bases/dns.networking.miloapis.com_dnsnamingpolicies.yaml +++ b/config/crd/bases/dns.networking.miloapis.com_dnsnamingpolicies.yaml @@ -46,6 +46,7 @@ spec: DNS records its resolved lifetime in controller-owned status. properties: name: + maxLength: 253 type: string required: - name @@ -58,48 +59,45 @@ spec: An omitted or empty selector matches every registration in the context. properties: matchExpressions: - description: matchExpressions is a list of label selector - requirements. The requirements are ANDed. items: - description: |- - A label selector requirement is a selector that contains values, a key, and an operator that - relates the key and values. properties: key: - description: key is the label key that the selector - applies to. + maxLength: 317 + minLength: 1 type: string operator: - description: |- - operator represents a key's relationship to a set of values. - Valid operators are In, NotIn, Exists and DoesNotExist. + description: A label selector operator is the set + of operators that can be used in a selector requirement. + enum: + - In + - NotIn + - Exists + - DoesNotExist type: string values: - description: |- - values is an array of string values. If the operator is In or NotIn, - the values array must be non-empty. If the operator is Exists or DoesNotExist, - the values array must be empty. This array is replaced during a strategic - merge patch. items: + maxLength: 63 + pattern: ^([A-Za-z0-9]([-A-Za-z0-9_.]{0,61}[A-Za-z0-9])?)?$ type: string + maxItems: 64 type: array - x-kubernetes-list-type: atomic required: - key - operator type: object + x-kubernetes-validations: + - message: selector operator requires compatible values + rule: '(self.operator in [''In'', ''NotIn'']) ? (has(self.values) + && self.values.size() > 0) : (!has(self.values) || + self.values.size() == 0)' + maxItems: 64 type: array - x-kubernetes-list-type: atomic matchLabels: additionalProperties: type: string - description: |- - matchLabels is a map of {key,value} pairs. A single {key,value} in the matchLabels - map is equivalent to an element of matchExpressions, whose key field is "key", the - operator is "In", and the values array contains only "value". The requirements are ANDed. + maxProperties: 64 type: object type: object - x-kubernetes-map-type: atomic required: - dnsZoneRef - namePrefix @@ -107,6 +105,7 @@ spec: x-kubernetes-validations: - message: dnsZoneRef requires a name rule: self.dnsZoneRef.name.size() > 0 + maxItems: 256 type: array dnsZoneRef: description: |- @@ -114,6 +113,7 @@ spec: DNS records its resolved lifetime in controller-owned status. properties: name: + maxLength: 253 type: string required: - name @@ -127,6 +127,7 @@ spec: DNS records its resolved lifetime in controller-owned status. properties: name: + maxLength: 253 type: string required: - name @@ -139,6 +140,7 @@ spec: DNS records its resolved lifetime in controller-owned status. properties: name: + maxLength: 253 type: string required: - name diff --git a/config/crd/bases/dns.networking.miloapis.com_dnszoneassociations.yaml b/config/crd/bases/dns.networking.miloapis.com_dnszoneassociations.yaml index 29aaf416..15161d08 100644 --- a/config/crd/bases/dns.networking.miloapis.com_dnszoneassociations.yaml +++ b/config/crd/bases/dns.networking.miloapis.com_dnszoneassociations.yaml @@ -44,6 +44,7 @@ spec: DNS records its resolved lifetime in controller-owned status. properties: name: + maxLength: 253 type: string required: - name @@ -56,6 +57,7 @@ spec: DNS records its resolved lifetime in controller-owned status. properties: name: + maxLength: 253 type: string required: - name @@ -66,6 +68,7 @@ spec: DNS records its resolved lifetime in controller-owned status. properties: name: + maxLength: 253 type: string required: - name diff --git a/internal/internaldns/admission/handler.go b/internal/internaldns/admission/handler.go index bc8ea6d8..e1fb31f6 100644 --- a/internal/internaldns/admission/handler.go +++ b/internal/internaldns/admission/handler.go @@ -291,7 +291,7 @@ func (h *Handler) namingPolicy(ctx context.Context, cl client.Client, req kubead } } for _, rule := range p.Spec.AdditionalNames { - if _, err := metav1.LabelSelectorAsSelector(rule.Selector); err != nil { + if _, err := metav1.LabelSelectorAsSelector(rule.Selector.AsLabelSelector()); err != nil { return fmt.Errorf("invalid naming selector: %w", err) } if err := validateNamedPrivateZone(ctx, cl, req.Namespace, rule.DNSZoneRef); err != nil { diff --git a/internal/internaldns/admission/handler_test.go b/internal/internaldns/admission/handler_test.go index af0da3a5..daa7121d 100644 --- a/internal/internaldns/admission/handler_test.go +++ b/internal/internaldns/admission/handler_test.go @@ -201,21 +201,9 @@ func TestResolverAccessRequiresCurrentContextAndBoundedLease(t *testing.T) { } } -func TestConsumerSelectorUsesNamesAndRejectsAmbiguity(t *testing.T) { +func TestConsumerSelectorRequiresConfiguredIntegrationMode(t *testing.T) { vpc := dnsv1alpha1.DNSNameReference{Name: "vpc"} resolverContext := dnsv1alpha1.DNSNameReference{Name: "ctx"} - for _, contextMode := range []bool{false, true} { - for _, tc := range []struct { - name string - vpc, context dnsv1alpha1.DNSNameReference - }{{"empty", dnsv1alpha1.DNSNameReference{}, dnsv1alpha1.DNSNameReference{}}, {"both", vpc, resolverContext}} { - t.Run(fmt.Sprintf("context=%v/%s", contextMode, tc.name), func(t *testing.T) { - if err := validateConsumerSelector(tc.vpc, tc.context, contextMode); err == nil { - t.Fatal("ambiguous selector accepted") - } - }) - } - } if err := validateConsumerSelector(vpc, dnsv1alpha1.DNSNameReference{}, false); err != nil { t.Fatalf("network selector rejected: %v", err) } @@ -280,10 +268,10 @@ func TestNamingPolicyRejectsMalformedSelectors(t *testing.T) { t.Fatal(err) } h := Handler{Client: fake.NewClientBuilder().WithScheme(scheme).Build(), ResolverContextsEnabled: true} - for _, selector := range []*metav1.LabelSelector{ + for _, selector := range []*dnsv1alpha1.DNSRegistrationSelector{ {MatchLabels: map[string]string{"bad key": "value"}}, - {MatchExpressions: []metav1.LabelSelectorRequirement{{Key: "app", Operator: "Unsupported"}}}, - {MatchExpressions: []metav1.LabelSelectorRequirement{{Key: "app", Operator: metav1.LabelSelectorOpIn}}}, + {MatchExpressions: []dnsv1alpha1.DNSRegistrationSelectorRequirement{{Key: "app", Operator: "Unsupported"}}}, + {MatchExpressions: []dnsv1alpha1.DNSRegistrationSelectorRequirement{{Key: "app", Operator: metav1.LabelSelectorOpIn}}}, } { policy := &dnsv1alpha1.DNSNamingPolicy{Spec: dnsv1alpha1.DNSNamingPolicySpec{ResolverContextRef: dnsv1alpha1.DNSNameReference{Name: "application"}, AdditionalNames: []dnsv1alpha1.DNSAdditionalNameRule{{Selector: selector, DNSZoneRef: dnsv1alpha1.DNSNameReference{Name: "production"}, NamePrefix: "application"}}}} response := h.Handle(context.Background(), request("dnsnamingpolicies", "customer", policy, nil)) diff --git a/internal/internaldns/controlplane/context_reconciler.go b/internal/internaldns/controlplane/context_reconciler.go index 89bb130c..f554999f 100644 --- a/internal/internaldns/controlplane/context_reconciler.go +++ b/internal/internaldns/controlplane/context_reconciler.go @@ -76,7 +76,7 @@ func (r *Reconciler) reconcileNamingPolicies(ctx context.Context, items []dnsv1a if !zoneCurrent || !contextCurrent || !associatedWithContext(assocs, zonePin.UID, contextPin.UID) { valid = false } - selector, selectorErr := namingSelector(rule.Selector) + selector, selectorErr := namingSelector(rule.Selector.AsLabelSelector()) selectorKey := "" if selectorErr != nil { valid = false @@ -88,7 +88,7 @@ func (r *Reconciler) reconcileNamingPolicies(ctx context.Context, items []dnsv1a valid = false } seen[key] = true - resolved = append(resolved, dnsv1alpha1.DNSResolvedAdditionalNameRule{Selector: rule.Selector.DeepCopy(), DNSZoneRef: zonePin, NamePrefix: canonicalName(rule.NamePrefix)}) + resolved = append(resolved, dnsv1alpha1.DNSResolvedAdditionalNameRule{Selector: rule.Selector.AsLabelSelector().DeepCopy(), DNSZoneRef: zonePin, NamePrefix: canonicalName(rule.NamePrefix)}) } p.Status.ResolvedAdditionalNames = resolved status := metav1.ConditionTrue diff --git a/internal/internaldns/controlplane/named_reference_api_test.go b/internal/internaldns/controlplane/named_reference_api_test.go index 96a4ac67..bda1cb8f 100644 --- a/internal/internaldns/controlplane/named_reference_api_test.go +++ b/internal/internaldns/controlplane/named_reference_api_test.go @@ -24,7 +24,7 @@ func verifyNamedReferencesWithAPI(t *testing.T, cl client.Client) { zone := &dnsv1alpha1.DNSZone{ObjectMeta: metav1.ObjectMeta{Name: "named-zone", Namespace: "project"}, Spec: dnsv1alpha1.DNSZoneSpec{DomainName: "named.internal", Visibility: dnsv1alpha1.DNSZoneVisibilityPrivate, DNSZoneClassName: "private"}} requireAccessTestSuccess(t, cl.Create(ctx, zone)) association := &dnsv1alpha1.DNSZoneAssociation{ObjectMeta: metav1.ObjectMeta{Name: "named-association", Namespace: "project"}, Spec: dnsv1alpha1.DNSZoneAssociationSpec{DNSZoneRef: dnsv1alpha1.DNSNameReference{Name: zone.Name}, ResolverContextRef: dnsv1alpha1.DNSNameReference{Name: resolver.Name}}} - policy := &dnsv1alpha1.DNSNamingPolicy{ObjectMeta: metav1.ObjectMeta{Name: "named-policy", Namespace: "project"}, Spec: dnsv1alpha1.DNSNamingPolicySpec{ResolverContextRef: dnsv1alpha1.DNSNameReference{Name: resolver.Name}, AdditionalNames: []dnsv1alpha1.DNSAdditionalNameRule{{Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"app": "storefront"}}, DNSZoneRef: dnsv1alpha1.DNSNameReference{Name: zone.Name}, NamePrefix: "hosts"}}}} + policy := &dnsv1alpha1.DNSNamingPolicy{ObjectMeta: metav1.ObjectMeta{Name: "named-policy", Namespace: "project"}, Spec: dnsv1alpha1.DNSNamingPolicySpec{ResolverContextRef: dnsv1alpha1.DNSNameReference{Name: resolver.Name}, AdditionalNames: []dnsv1alpha1.DNSAdditionalNameRule{{Selector: &dnsv1alpha1.DNSRegistrationSelector{MatchLabels: map[string]string{"app": "storefront"}}, DNSZoneRef: dnsv1alpha1.DNSNameReference{Name: zone.Name}, NamePrefix: "hosts"}}}} requireAccessTestSuccess(t, cl.Create(ctx, association)) requireAccessTestSuccess(t, cl.Create(ctx, policy)) r := &Reconciler{Client: cl} diff --git a/internal/internaldns/controlplane/naming_selector_test.go b/internal/internaldns/controlplane/naming_selector_test.go index 22dd2fe7..d1e46bdc 100644 --- a/internal/internaldns/controlplane/naming_selector_test.go +++ b/internal/internaldns/controlplane/naming_selector_test.go @@ -102,17 +102,17 @@ func TestNamingPolicyValidatesSelectorsAndDuplicateRules(t *testing.T) { setCondition(&association.Status.Conditions, "Accepted", metav1.ConditionTrue, "Accepted", "", 1, now) for _, tc := range []struct { name string - selectors []*metav1.LabelSelector + selectors []*dnsv1alpha1.DNSRegistrationSelector valid bool }{ - {"omitted", []*metav1.LabelSelector{nil}, true}, - {"empty", []*metav1.LabelSelector{{}}, true}, - {"labels", []*metav1.LabelSelector{{MatchLabels: map[string]string{"app": "storefront"}}}, true}, - {"invalid-key", []*metav1.LabelSelector{{MatchLabels: map[string]string{"bad key": "value"}}}, false}, - {"invalid-operator", []*metav1.LabelSelector{{MatchExpressions: []metav1.LabelSelectorRequirement{{Key: "app", Operator: "Unsupported"}}}}, false}, - {"missing-values", []*metav1.LabelSelector{{MatchExpressions: []metav1.LabelSelectorRequirement{{Key: "app", Operator: metav1.LabelSelectorOpIn}}}}, false}, - {"duplicate", []*metav1.LabelSelector{nil, {}}, false}, - {"overlapping", []*metav1.LabelSelector{nil, {MatchLabels: map[string]string{"app": "storefront"}}}, true}, + {"omitted", []*dnsv1alpha1.DNSRegistrationSelector{nil}, true}, + {"empty", []*dnsv1alpha1.DNSRegistrationSelector{{}}, true}, + {"labels", []*dnsv1alpha1.DNSRegistrationSelector{{MatchLabels: map[string]string{"app": "storefront"}}}, true}, + {"invalid-key", []*dnsv1alpha1.DNSRegistrationSelector{{MatchLabels: map[string]string{"bad key": "value"}}}, false}, + {"invalid-operator", []*dnsv1alpha1.DNSRegistrationSelector{{MatchExpressions: []dnsv1alpha1.DNSRegistrationSelectorRequirement{{Key: "app", Operator: "Unsupported"}}}}, false}, + {"missing-values", []*dnsv1alpha1.DNSRegistrationSelector{{MatchExpressions: []dnsv1alpha1.DNSRegistrationSelectorRequirement{{Key: "app", Operator: metav1.LabelSelectorOpIn}}}}, false}, + {"duplicate", []*dnsv1alpha1.DNSRegistrationSelector{nil, {}}, false}, + {"overlapping", []*dnsv1alpha1.DNSRegistrationSelector{nil, {MatchLabels: map[string]string{"app": "storefront"}}}, true}, } { t.Run(tc.name, func(t *testing.T) { policy := &dnsv1alpha1.DNSNamingPolicy{ObjectMeta: metav1.ObjectMeta{Name: tc.name, Namespace: "project", Generation: 1}, Spec: dnsv1alpha1.DNSNamingPolicySpec{ResolverContextRef: dnsv1alpha1.DNSNameReference{Name: resolver.Name}}} From cd43bb8213cb21b727449ab493bc0214f7a54c93 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Mon, 12 Oct 2026 16:35:29 -0500 Subject: [PATCH 03/19] Validate named DNS references without duplicating authorization --- .../internaldns/admission/handler_test.go | 52 +++++-------------- 1 file changed, 13 insertions(+), 39 deletions(-) diff --git a/internal/internaldns/admission/handler_test.go b/internal/internaldns/admission/handler_test.go index daa7121d..3a840af7 100644 --- a/internal/internaldns/admission/handler_test.go +++ b/internal/internaldns/admission/handler_test.go @@ -12,14 +12,11 @@ import ( dnsv1alpha1 "go.miloapis.com/dns-operator/api/v1alpha1" admissionv1 "k8s.io/api/admission/v1" authenticationv1 "k8s.io/api/authentication/v1" - authorizationv1 "k8s.io/api/authorization/v1" apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" - "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" - "sigs.k8s.io/controller-runtime/pkg/client/interceptor" kubeadmission "sigs.k8s.io/controller-runtime/pkg/webhook/admission" ) @@ -215,50 +212,27 @@ func TestConsumerSelectorRequiresConfiguredIntegrationMode(t *testing.T) { } } -func TestCustomerNamedReferencesRequireZoneAndContextAuthorization(t *testing.T) { +func TestCustomerNamedReferencesValidateLiveResources(t *testing.T) { scheme := runtime.NewScheme() if err := dnsv1alpha1.AddToScheme(scheme); err != nil { t.Fatal(err) } - zone := &dnsv1alpha1.DNSZone{ObjectMeta: metav1.ObjectMeta{Name: "production", Namespace: "project", UID: "zone"}, Spec: dnsv1alpha1.DNSZoneSpec{Visibility: dnsv1alpha1.DNSZoneVisibilityPrivate}} - resolver := &dnsv1alpha1.DNSResolverContext{ObjectMeta: metav1.ObjectMeta{Name: "application", Namespace: "project", UID: "context", Generation: 1}} - setAdmissionCondition(&resolver.Status.Conditions, "Ready", metav1.ConditionTrue, resolver.Generation, time.Now()) - deniedResource := "" - reviewed := map[string]string{} - cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(zone, resolver).WithInterceptorFuncs(interceptor.Funcs{Create: func(ctx context.Context, cl client.WithWatch, obj client.Object, opts ...client.CreateOption) error { - if review, ok := obj.(*authorizationv1.SubjectAccessReview); ok { - attributes := review.Spec.ResourceAttributes - reviewed[attributes.Resource] = attributes.Verb + "/" + attributes.Name - review.Status.Allowed = attributes.Resource != deniedResource - return nil - } - return cl.Create(ctx, obj, opts...) - }}).Build() - h := Handler{Client: cl, ResolverContextsEnabled: true} - zoneRef := dnsv1alpha1.DNSNameReference{Name: zone.Name} - contextRef := dnsv1alpha1.DNSNameReference{Name: resolver.Name} + zone := &dnsv1alpha1.DNSZone{ObjectMeta: metav1.ObjectMeta{Name: "production", Namespace: "project"}, Spec: dnsv1alpha1.DNSZoneSpec{Visibility: dnsv1alpha1.DNSZoneVisibilityPrivate}} + resolver := &dnsv1alpha1.DNSResolverContext{ObjectMeta: metav1.ObjectMeta{Name: "application", Namespace: "project", Generation: 1}} + setAdmissionCondition(&resolver.Status.Conditions, "Ready", metav1.ConditionTrue, 1, time.Now()) + h := Handler{Client: fake.NewClientBuilder().WithScheme(scheme).WithObjects(zone, resolver).Build(), ResolverContextsEnabled: true} + zoneRef, contextRef := dnsv1alpha1.DNSNameReference{Name: zone.Name}, dnsv1alpha1.DNSNameReference{Name: resolver.Name} for resource, object := range map[string]any{ "dnszoneassociations": &dnsv1alpha1.DNSZoneAssociation{Spec: dnsv1alpha1.DNSZoneAssociationSpec{DNSZoneRef: zoneRef, ResolverContextRef: contextRef}}, "dnsnamingpolicies": &dnsv1alpha1.DNSNamingPolicy{Spec: dnsv1alpha1.DNSNamingPolicySpec{ResolverContextRef: contextRef, AdditionalNames: []dnsv1alpha1.DNSAdditionalNameRule{{DNSZoneRef: zoneRef, NamePrefix: "hosts"}}}}, } { - t.Run(resource, func(t *testing.T) { - for _, denied := range []string{"", "dnszones", "dnsresolvercontexts"} { - deniedResource = denied - reviewed = map[string]string{} - response := h.Handle(context.Background(), request(resource, "customer", object, nil)) - if response.Allowed != (denied == "") { - t.Fatalf("denied=%q allowed=%v: %v", denied, response.Allowed, response.Result) - } - if denied == "" && (reviewed["dnszones"] != "update/production" || reviewed["dnsresolvercontexts"] != "use/application") { - t.Fatalf("references did not authorize both named resources: %v", reviewed) - } - } - req := request(resource, "customer", object, nil) - req.SubResource = "status" - if h.Handle(context.Background(), req).Allowed { - t.Fatal("customer can modify DNS-owned lifetime pins") - } - }) + if response := h.Handle(context.Background(), request(resource, "rbac-authorized-customer", object, nil)); !response.Allowed { + t.Fatalf("valid %s denied: %v", resource, response.Result) + } + missing := Handler{Client: fake.NewClientBuilder().WithScheme(scheme).Build(), ResolverContextsEnabled: true} + if missing.Handle(context.Background(), request(resource, "rbac-authorized-customer", object, nil)).Allowed { + t.Fatalf("%s accepted missing references", resource) + } } } From 0bebb1d27f8a8851016c97296c8d945ac7a28589 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Tue, 13 Oct 2026 10:03:04 -0500 Subject: [PATCH 04/19] test: Scope naming fixtures to DNS contexts --- internal/internaldns/controlplane/naming_selector_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/internaldns/controlplane/naming_selector_test.go b/internal/internaldns/controlplane/naming_selector_test.go index d1e46bdc..8b7d0117 100644 --- a/internal/internaldns/controlplane/naming_selector_test.go +++ b/internal/internaldns/controlplane/naming_selector_test.go @@ -21,10 +21,10 @@ func TestNamingSelectorsMatchRegistrationLabelsWithinContext(t *testing.T) { if err := dnsv1alpha1.AddToScheme(scheme); err != nil { t.Fatal(err) } - managed := &dnsv1alpha1.DNSManagedNamespace{ObjectMeta: metav1.ObjectMeta{Name: "managed", Namespace: "project"}, Spec: dnsv1alpha1.DNSManagedNamespaceSpec{ProjectUID: "project-a", VPCRef: dnsv1alpha1.DNSObjectReference{UID: "consumer-a"}}, Status: dnsv1alpha1.DNSManagedNamespaceStatus{DNSZoneRef: dnsv1alpha1.DNSObjectReference{Name: "canonical", UID: "canonical-zone"}, Conditions: []metav1.Condition{{Type: "Accepted", Status: metav1.ConditionTrue}}}} + managed := &dnsv1alpha1.DNSManagedNamespace{ObjectMeta: metav1.ObjectMeta{Name: "managed", Namespace: "project"}, Spec: dnsv1alpha1.DNSManagedNamespaceSpec{ProjectUID: "project-a", ResolverContextRef: dnsv1alpha1.DNSObjectReference{UID: "consumer-a"}}, Status: dnsv1alpha1.DNSManagedNamespaceStatus{DNSZoneRef: dnsv1alpha1.DNSObjectReference{Name: "canonical", UID: "canonical-zone"}, Conditions: []metav1.Condition{{Type: "Accepted", Status: metav1.ConditionTrue}}}} zone := dnsv1alpha1.DNSObjectReference{Name: "custom", UID: "custom-zone"} storefront := &metav1.LabelSelector{MatchLabels: map[string]string{"app": "storefront"}} - policy := &dnsv1alpha1.DNSNamingPolicy{ObjectMeta: metav1.ObjectMeta{Name: "names", Namespace: "project", Generation: 1}, Status: dnsv1alpha1.DNSNamingPolicyStatus{ResolvedVPCRef: dnsv1alpha1.DNSObjectReference{UID: "consumer-a"}, Conditions: []metav1.Condition{{Type: "Accepted", Status: metav1.ConditionTrue, ObservedGeneration: 1}}, ResolvedAdditionalNames: []dnsv1alpha1.DNSResolvedAdditionalNameRule{ + policy := &dnsv1alpha1.DNSNamingPolicy{ObjectMeta: metav1.ObjectMeta{Name: "names", Namespace: "project", Generation: 1}, Status: dnsv1alpha1.DNSNamingPolicyStatus{ResolvedResolverContextRef: dnsv1alpha1.DNSObjectReference{UID: "consumer-a"}, Conditions: []metav1.Condition{{Type: "Accepted", Status: metav1.ConditionTrue, ObservedGeneration: 1}}, ResolvedAdditionalNames: []dnsv1alpha1.DNSResolvedAdditionalNameRule{ {DNSZoneRef: zone, NamePrefix: "all"}, {Selector: &metav1.LabelSelector{}, DNSZoneRef: zone, NamePrefix: "empty"}, {Selector: storefront, DNSZoneRef: zone, NamePrefix: "storefront"}, @@ -33,7 +33,7 @@ func TestNamingSelectorsMatchRegistrationLabelsWithinContext(t *testing.T) { }}} other := policy.DeepCopy() other.Name = "other-context" - other.Status.ResolvedVPCRef.UID = "consumer-b" + other.Status.ResolvedResolverContextRef.UID = "consumer-b" other.Status.ResolvedAdditionalNames = []dnsv1alpha1.DNSResolvedAdditionalNameRule{{DNSZoneRef: zone, NamePrefix: "other-context"}} stale := policy.DeepCopy() stale.Name = "stale-policy" From aa0e7555161524a2028825e39d3203788a88c349 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Tue, 13 Oct 2026 15:58:03 -0500 Subject: [PATCH 05/19] feat: Discover internal DNS project sources Discover ready Milo projects and reconcile every source namespace without restarting the DNS service. Keep active sources during discovery outages and resolve admission requests against live project identity. Route serving artifacts by resource type and scope access withdrawal to its source namespace so shared namespace names stay isolated. --- config/internal-dns/README.md | 9 + .../internal-dns/control-plane.example.yaml | 25 ++- .../controlplane/context_reconciler.go | 23 ++- .../controlplane/context_reconciler_test.go | 49 +++++ .../internaldns/controlplane/reconciler.go | 2 +- internal/internaldns/platform/acks.go | 6 +- internal/internaldns/runtime/config.go | 33 +++- internal/internaldns/runtime/config_test.go | 20 ++ .../internaldns/runtime/project_discovery.go | 181 ++++++++++++++++++ .../runtime/project_discovery_test.go | 157 +++++++++++++++ .../internaldns/runtime/routing_client.go | 28 +-- .../runtime/routing_client_test.go | 25 ++- internal/internaldns/runtime/run.go | 59 ++++-- 13 files changed, 561 insertions(+), 56 deletions(-) create mode 100644 internal/internaldns/runtime/project_discovery.go create mode 100644 internal/internaldns/runtime/project_discovery_test.go diff --git a/config/internal-dns/README.md b/config/internal-dns/README.md index 173c462b..559ef363 100644 --- a/config/internal-dns/README.md +++ b/config/internal-dns/README.md @@ -38,6 +38,15 @@ identity and storage; do not run two pods against one checkpoint. ## Control-plane and API boundaries +Control-plane replicas discover ready Milo projects through the core API and +watch every namespace in each project control plane. New projects are included +without restarting the service. `projectDiscovery.kubeconfig` authenticates +discovery and project API access; the process `--kubeconfig` authenticates the +DNS service project. Generated kubeconfig Secrets use the `value` key. +Unavailable discovery retains active sources and applied DNS configuration. +Admission resolves the authenticated project parent against its live identity, +and acknowledgement projection follows the discovered source clients. + Control-plane replicas can concurrently watch the platform API and source projects. Publication and shard ownership use resource-version checks and increasing writer epochs on takeover. Each source project has an authenticated diff --git a/config/internal-dns/control-plane.example.yaml b/config/internal-dns/control-plane.example.yaml index 3f257fbf..6cecc1cb 100644 --- a/config/internal-dns/control-plane.example.yaml +++ b/config/internal-dns/control-plane.example.yaml @@ -10,14 +10,11 @@ data: "shard": "shared-0", "platformNamespace": "internal-dns-system", "platformKubeAPI": {"qps":50,"burst":100,"requestTimeoutSeconds":10}, - "projects": [ - { - "name": "example-project", - "projectUID": "00000000-0000-0000-0000-000000000001", - "namespace": "example-project", - "kubeAPI": {"qps":20,"burst":40,"requestTimeoutSeconds":10} - } - ], + "projectDiscovery": { + "kubeconfig": "/var/run/internal-dns/discovery/kubeconfig", + "intervalSeconds": 10, + "kubeAPI": {"qps":20,"burst":40,"requestTimeoutSeconds":10} + }, "nodeBackends": [ {"memberID":"node-bind-0","address":"fd70:10::10","port":5300} ], @@ -82,7 +79,7 @@ spec: containers: - name: control-plane image: example.invalid/dns-operator/internal-dns:VERSION - args: ["--role=control-plane","--config=/etc/internal-dns/config.json"] + args: ["--role=control-plane","--config=/etc/internal-dns/config.json","--kubeconfig=/var/run/internal-dns/platform/kubeconfig"] ports: - {name: admission, containerPort: 9443} securityContext: @@ -91,10 +88,20 @@ spec: runAsNonRoot: true capabilities: {drop: [ALL]} volumeMounts: + - {name: discovery, mountPath: /var/run/internal-dns/discovery, readOnly: true} + - {name: platform, mountPath: /var/run/internal-dns/platform, readOnly: true} - {name: config, mountPath: /etc/internal-dns, readOnly: true} - {name: nats, mountPath: /var/run/internal-dns/nats, readOnly: true} - {name: admission, mountPath: /var/run/internal-dns/admission, readOnly: true} volumes: + - name: discovery + secret: + secretName: internal-dns-discovery-kubeconfig + items: [{key: value, path: kubeconfig}] + - name: platform + secret: + secretName: internal-dns-service-kubeconfig + items: [{key: value, path: kubeconfig}] - name: config configMap: {name: internal-dns-control-plane-config} - name: nats diff --git a/internal/internaldns/controlplane/context_reconciler.go b/internal/internaldns/controlplane/context_reconciler.go index eb0bbcfc..2475f147 100644 --- a/internal/internaldns/controlplane/context_reconciler.go +++ b/internal/internaldns/controlplane/context_reconciler.go @@ -368,7 +368,7 @@ func (r *Reconciler) reconcileContextBindings(ctx context.Context, ns string, as return err } } - binding := &dnsv1alpha1.DNSResolverBinding{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: r.Options.PlatformNamespace, Labels: map[string]string{"internal-dns.miloapis.com/context-uid": string(contextObject.UID), "internal-dns.miloapis.com/access-uid": string(a.UID), "internal-dns.miloapis.com/region": model.SafeToken(spec.Region), "internal-dns.miloapis.com/shard": model.SafeToken(r.shardForContextRegion(contextObject.UID, spec.Region, existing.Items))}}, Spec: dnsv1alpha1.DNSResolverBindingSpec{Source: dnsv1alpha1.DNSResolverBindingSource{ProjectUID: r.Options.ProjectUID, ResolverContextRef: dnsv1alpha1.DNSObjectReference{Name: contextObject.Name, UID: contextObject.UID}, AccessBindingRef: dnsv1alpha1.DNSObjectReference{Name: a.Name, UID: a.UID, Generation: a.Generation}}, Placement: dnsv1alpha1.DNSResolverBindingPlacement{Region: spec.Region, Shard: r.shardForContextRegion(contextObject.UID, spec.Region, existing.Items)}, Configuration: dnsv1alpha1.DNSResolverBindingConfiguration{Generation: generation, Revision: revision, Query: dnsv1alpha1.DNSResolverBindingQuery{NodeAddress: spec.DestinationAddress, RegionalAddress: cluster}, ZoneRefs: zoneReferences(zids, zones), ZoneApices: zoneApices(zids, zones)}, Version: version}} + binding := &dnsv1alpha1.DNSResolverBinding{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: r.Options.PlatformNamespace, Labels: map[string]string{"internal-dns.miloapis.com/source-namespace": ns, "internal-dns.miloapis.com/context-uid": string(contextObject.UID), "internal-dns.miloapis.com/access-uid": string(a.UID), "internal-dns.miloapis.com/region": model.SafeToken(spec.Region), "internal-dns.miloapis.com/shard": model.SafeToken(r.shardForContextRegion(contextObject.UID, spec.Region, existing.Items))}}, Spec: dnsv1alpha1.DNSResolverBindingSpec{Source: dnsv1alpha1.DNSResolverBindingSource{ProjectUID: r.Options.ProjectUID, ResolverContextRef: dnsv1alpha1.DNSObjectReference{Name: contextObject.Name, UID: contextObject.UID}, AccessBindingRef: dnsv1alpha1.DNSObjectReference{Name: a.Name, UID: a.UID, Generation: a.Generation}}, Placement: dnsv1alpha1.DNSResolverBindingPlacement{Region: spec.Region, Shard: r.shardForContextRegion(contextObject.UID, spec.Region, existing.Items)}, Configuration: dnsv1alpha1.DNSResolverBindingConfiguration{Generation: generation, Revision: revision, Query: dnsv1alpha1.DNSResolverBindingQuery{NodeAddress: spec.DestinationAddress, RegionalAddress: cluster}, ZoneRefs: zoneReferences(zids, zones), ZoneApices: zoneApices(zids, zones)}, Version: version}} if !exists { if err := r.Create(ctx, binding); err != nil { return err @@ -376,7 +376,7 @@ func (r *Reconciler) reconcileContextBindings(ctx context.Context, ns string, as if err := r.Get(ctx, client.ObjectKeyFromObject(binding), binding); err != nil { return err } - } else if !bindingEqual(old.Spec, binding.Spec) { + } else if !bindingEqual(old.Spec, binding.Spec) || old.Labels["internal-dns.miloapis.com/source-namespace"] != ns { baseOld := old.DeepCopy() old.Spec = binding.Spec old.Labels = binding.Labels @@ -408,6 +408,25 @@ func (r *Reconciler) reconcileContextBindings(ctx context.Context, ns string, as if b.Labels["internal-dns.miloapis.com/context-uid"] == "" { continue } + sourceNamespace := b.Labels["internal-dns.miloapis.com/source-namespace"] + if sourceNamespace != "" && sourceNamespace != ns { + continue + } + if sourceNamespace == "" && r.Options.ProjectNamespace == "" { + var sourceAccesses dnsv1alpha1.DNSResolverAccessBindingList + if err := r.sourceReader().List(ctx, &sourceAccesses); err != nil { + return err + } + for j := range sourceAccesses.Items { + if sourceAccesses.Items[j].UID == b.Spec.Source.AccessBindingRef.UID { + sourceNamespace = sourceAccesses.Items[j].Namespace + break + } + } + if sourceNamespace != "" && sourceNamespace != ns { + continue + } + } withdraw, err := r.confirmAccessWithdrawal(ctx, ns, b) if err != nil { return err diff --git a/internal/internaldns/controlplane/context_reconciler_test.go b/internal/internaldns/controlplane/context_reconciler_test.go index ca44d51c..ef6bd741 100644 --- a/internal/internaldns/controlplane/context_reconciler_test.go +++ b/internal/internaldns/controlplane/context_reconciler_test.go @@ -14,6 +14,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" ) @@ -309,3 +310,51 @@ func TestAccessPlacementMatchesContextPublicationTargets(t *testing.T) { } } } + +func TestNamespaceReconciliationCannotWithdrawAnotherNamespace(t *testing.T) { + for _, labeled := range []bool{false, true} { + t.Run(fmt.Sprintf("namespace-label-%t", labeled), func(t *testing.T) { + ctx := context.Background() + scheme := runtime.NewScheme() + if err := addReferenceStateToScheme(scheme); err != nil { + t.Fatal(err) + } + access := &dnsv1alpha1.DNSResolverAccessBinding{ObjectMeta: metav1.ObjectMeta{Name: "access", Namespace: "other", UID: "other-access"}} + binding := &dnsv1alpha1.DNSResolverBinding{ObjectMeta: metav1.ObjectMeta{Name: "binding", Namespace: "platform", Labels: map[string]string{"internal-dns.miloapis.com/context-uid": "other-context"}}, Spec: dnsv1alpha1.DNSResolverBindingSpec{Source: dnsv1alpha1.DNSResolverBindingSource{ProjectUID: "project", AccessBindingRef: dnsv1alpha1.DNSObjectReference{Name: access.Name, UID: access.UID}, ResolverContextRef: dnsv1alpha1.DNSObjectReference{Name: "context", UID: "other-context"}}}} + if labeled { + binding.Labels["internal-dns.miloapis.com/source-namespace"] = access.Namespace + } + cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(access, binding).Build() + reconciler := &Reconciler{Client: cl, APIReader: cl, Options: ReconcilerOptions{ProjectUID: "project", PlatformNamespace: "platform"}} + if err := reconciler.reconcileContextBindings(ctx, "different", nil, nil, time.Now()); err != nil { + t.Fatal(err) + } + if err := cl.Get(ctx, client.ObjectKeyFromObject(binding), binding); err != nil { + t.Fatal(err) + } + if binding.Spec.Tombstone { + t.Fatal("another source namespace was withdrawn") + } + }) + } +} + +func TestAllNamespaceReconcileIncludesPlatformNamedSourceNamespace(t *testing.T) { + ctx := context.Background() + scheme := runtime.NewScheme() + if err := addReferenceStateToScheme(scheme); err != nil { + t.Fatal(err) + } + contextObject := &dnsv1alpha1.DNSResolverContext{ObjectMeta: metav1.ObjectMeta{Name: "context", Namespace: "platform", UID: "context", Generation: 1}, Spec: dnsv1alpha1.DNSResolverContextSpec{ConsumerID: "network"}} + cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(contextObject).WithStatusSubresource(contextObject).Build() + r := &Reconciler{Client: cl, APIReader: cl, Options: ReconcilerOptions{ProjectUID: "project", PlatformNamespace: "platform"}} + if _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(contextObject)}); err != nil { + t.Fatal(err) + } + if err := cl.Get(ctx, client.ObjectKeyFromObject(contextObject), contextObject); err != nil { + t.Fatal(err) + } + if !apimeta.IsStatusConditionTrue(contextObject.Status.Conditions, "Ready") { + t.Fatal("source namespace was excluded because it shares the managed namespace name") + } +} diff --git a/internal/internaldns/controlplane/reconciler.go b/internal/internaldns/controlplane/reconciler.go index dfbbafce..7138147e 100644 --- a/internal/internaldns/controlplane/reconciler.go +++ b/internal/internaldns/controlplane/reconciler.go @@ -104,7 +104,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu if ns == "" { ns = req.Namespace } - if ns == "" || ns == r.Options.PlatformNamespace { + if ns == "" { return ctrl.Result{}, nil } return r.reconcileProject(ctx, ns) diff --git a/internal/internaldns/platform/acks.go b/internal/internaldns/platform/acks.go index bca7ea0a..d185bab0 100644 --- a/internal/internaldns/platform/acks.go +++ b/internal/internaldns/platform/acks.go @@ -36,6 +36,7 @@ type AckSink struct { Namespace string ProjectNamespaces []string Projects []ProjectScope + ProjectScopes func() []ProjectScope PublicationMembers []model.Member PublicationRegions map[string][]model.Member Region string @@ -182,6 +183,9 @@ func (s *AckSink) serving(ctx context.Context, a model.MemberAck, row dnsv1alpha func (s *AckSink) projectPublication(ctx context.Context, m *dnsv1alpha1.DNSPublicationManifest, complete bool, now time.Time) error { projects := append([]ProjectScope(nil), s.Projects...) + if s.ProjectScopes != nil { + projects = append(projects, s.ProjectScopes()...) + } for _, ns := range s.ProjectNamespaces { projects = append(projects, ProjectScope{Client: s.Client, Namespace: ns}) } @@ -230,7 +234,7 @@ func (s *AckSink) projectPublicationForScope(ctx context.Context, project Projec } } var regs dnsv1alpha1.DNSRegistrationList - if err := projectClient.List(ctx, ®s, client.InNamespace(ns)); err != nil { + if err := projectClient.List(ctx, ®s, client.InNamespace(z.Namespace)); err != nil { projectErrors = errors.Join(projectErrors, fmt.Errorf("project %s list registrations: %w", ns, err)) continue } diff --git a/internal/internaldns/runtime/config.go b/internal/internaldns/runtime/config.go index b07f539e..4997be4c 100644 --- a/internal/internaldns/runtime/config.go +++ b/internal/internaldns/runtime/config.go @@ -35,6 +35,12 @@ type ProjectConfig struct { KubeAPI KubeAPIClientConfig `json:"kubeAPI,omitempty"` } +type ProjectDiscoveryConfig struct { + Kubeconfig string `json:"kubeconfig"` + IntervalSeconds int64 `json:"intervalSeconds,omitempty"` + KubeAPI KubeAPIClientConfig `json:"kubeAPI,omitempty"` +} + type KubeAPIClientConfig struct { QPS float32 `json:"qps,omitempty"` Burst int `json:"burst,omitempty"` @@ -59,7 +65,8 @@ type Config struct { Shard string `json:"shard"` PlatformNamespace string `json:"platformNamespace"` PlatformKubeAPI KubeAPIClientConfig `json:"platformKubeAPI,omitempty"` - Projects []ProjectConfig `json:"projects"` + Projects []ProjectConfig `json:"projects,omitempty"` + ProjectDiscovery *ProjectDiscoveryConfig `json:"projectDiscovery,omitempty"` NodeBackends []model.Backend `json:"nodeBackends,omitempty"` ClusterBackends []model.Backend `json:"clusterBackends,omitempty"` Members []model.Member `json:"members,omitempty"` @@ -118,6 +125,12 @@ func (c *Config) defaults() { } defaultKubeAPI(&c.PlatformKubeAPI, 50, 100) + if c.ProjectDiscovery != nil { + defaultKubeAPI(&c.ProjectDiscovery.KubeAPI, 20, 40) + if c.ProjectDiscovery.IntervalSeconds == 0 { + c.ProjectDiscovery.IntervalSeconds = 10 + } + } for i := range c.Projects { defaultKubeAPI(&c.Projects[i].KubeAPI, 20, 40) } @@ -190,12 +203,26 @@ func (c *Config) Validate(role string) error { } return c.Agent.Validate() } - if c.PlatformNamespace == "" || len(c.Projects) == 0 { - return errors.New("control-plane requires platformNamespace and projects") + if c.PlatformNamespace == "" || (len(c.Projects) == 0 && c.ProjectDiscovery == nil) { + return errors.New("control-plane requires platformNamespace and project discovery or projects") } if err := validateKubeAPI("platformKubeAPI", c.PlatformKubeAPI); err != nil { return err } + if c.ProjectDiscovery != nil { + if len(c.Projects) != 0 { + return errors.New("projectDiscovery and projects are mutually exclusive") + } + if !filepath.IsAbs(c.ProjectDiscovery.Kubeconfig) { + return errors.New("projectDiscovery.kubeconfig must be absolute") + } + if c.ProjectDiscovery.IntervalSeconds < 1 || c.ProjectDiscovery.IntervalSeconds > 300 { + return errors.New("projectDiscovery.intervalSeconds must be between 1 and 300") + } + if err := validateKubeAPI("projectDiscovery.kubeAPI", c.ProjectDiscovery.KubeAPI); err != nil { + return err + } + } names := map[string]bool{} uids := map[types.UID]bool{} namespaces := map[string]bool{} diff --git a/internal/internaldns/runtime/config_test.go b/internal/internaldns/runtime/config_test.go index 635824db..6ce6a902 100644 --- a/internal/internaldns/runtime/config_test.go +++ b/internal/internaldns/runtime/config_test.go @@ -205,3 +205,23 @@ func TestAgentRestoresCheckpointWhileBrokerUnavailable(t *testing.T) { t.Fatal("agent did not stop") } } + +func TestControlPlaneProjectDiscoveryValidation(t *testing.T) { + base := Config{Region: "central", Shard: "one", NATS: transport.Config{URL: "nats://fixture:4222", AllowInsecure: true}, Stream: transport.StreamConfig{Name: "DNS", SnapshotsBucket: "DNS_BOOTSTRAP"}, Members: []model.Member{{MemberID: "regional", Role: "regional"}}, ProjectDiscovery: &ProjectDiscoveryConfig{Kubeconfig: "/var/run/discovery/kubeconfig"}} + if err := base.Validate(roleControlPlane); err != nil { + t.Fatal(err) + } + if base.ProjectDiscovery.IntervalSeconds != 10 || base.ProjectDiscovery.KubeAPI.RequestTimeoutSeconds != 10 { + t.Fatal("discovery defaults missing") + } + mixed := base + mixed.Projects = []ProjectConfig{{ProjectUID: "project", Namespace: "project"}} + if err := mixed.Validate(roleControlPlane); err == nil { + t.Fatal("mixed discovery and static sources accepted") + } + relative := base + relative.ProjectDiscovery = &ProjectDiscoveryConfig{Kubeconfig: "relative"} + if err := relative.Validate(roleControlPlane); err == nil { + t.Fatal("relative discovery credential accepted") + } +} diff --git a/internal/internaldns/runtime/project_discovery.go b/internal/internaldns/runtime/project_discovery.go new file mode 100644 index 00000000..43d63e9e --- /dev/null +++ b/internal/internaldns/runtime/project_discovery.go @@ -0,0 +1,181 @@ +package runtime + +import ( + "context" + "errors" + "fmt" + "net/url" + "path" + "sort" + "sync" + + "go.miloapis.com/dns-operator/internal/internaldns/platform" + resourcemanagerv1alpha1 "go.miloapis.com/milo/pkg/apis/resourcemanager/v1alpha1" + apimeta "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/client-go/rest" + "sigs.k8s.io/controller-runtime/pkg/cache" + "sigs.k8s.io/controller-runtime/pkg/client" + kubeadmission "sigs.k8s.io/controller-runtime/pkg/webhook/admission" +) + +type directoryProject struct { + projectRuntime + cancel context.CancelFunc +} + +type projectDirectory struct { + mu sync.RWMutex + projects map[string]directoryProject + discover client.Reader +} + +func newProjectDirectory(projects []projectRuntime) *projectDirectory { + d := &projectDirectory{projects: map[string]directoryProject{}} + for _, p := range projects { + d.projects[p.config.Name] = directoryProject{projectRuntime: p} + } + return d +} + +func (d *projectDirectory) scopes() []platform.ProjectScope { + d.mu.RLock() + defer d.mu.RUnlock() + scopes := make([]platform.ProjectScope, 0, len(d.projects)) + names := make([]string, 0, len(d.projects)) + for name := range d.projects { + names = append(names, name) + } + sort.Strings(names) + for _, name := range names { + p := d.projects[name] + scopes = append(scopes, platform.ProjectScope{Client: p.direct, Namespace: p.config.Namespace, RequestTimeout: p.config.KubeAPI.requestTimeout()}) + } + return scopes +} + +func (d *projectDirectory) selectProject(ctx context.Context, req kubeadmission.Request) (client.Client, error) { + parents := req.UserInfo.Extra[parentNameExtra] + d.mu.RLock() + if d.discover == nil { + projects := make([]projectRuntime, 0, len(d.projects)) + for _, p := range d.projects { + projects = append(projects, p.projectRuntime) + } + d.mu.RUnlock() + return projectSelector(projects)(ctx, req) + } + reader := d.discover + d.mu.RUnlock() + if len(parents) != 1 || req.Namespace == "" { + return nil, errors.New("authenticated project parent is missing or ambiguous") + } + var project resourcemanagerv1alpha1.Project + if err := reader.Get(ctx, client.ObjectKey{Name: parents[0]}, &project); err != nil { + return nil, fmt.Errorf("observe authenticated project: %w", err) + } + if !project.DeletionTimestamp.IsZero() || !apimeta.IsStatusConditionTrue(project.Status.Conditions, resourcemanagerv1alpha1.ProjectReady) { + return nil, errors.New("authenticated project is unavailable") + } + d.mu.RLock() + defer d.mu.RUnlock() + p, found := d.projects[parents[0]] + if !found || p.config.ProjectUID != project.UID { + return nil, errors.New("authenticated project source is not ready") + } + return p.direct, nil +} + +type projectDiscovery struct { + client client.Client + directory *projectDirectory + rest *rest.Config + kubeAPI KubeAPIClientConfig + makeProject func(ProjectConfig, *rest.Config) (projectRuntime, error) +} + +func (d *projectDiscovery) step(ctx context.Context) error { + d.directory.mu.Lock() + d.directory.discover = d.client + d.directory.mu.Unlock() + var projects resourcemanagerv1alpha1.ProjectList + if err := d.client.List(ctx, &projects); err != nil { + return err + } + seen := map[string]bool{} + var failures error + for i := range projects.Items { + p := &projects.Items[i] + seen[p.Name] = true + if p.UID == "" { + failures = errors.Join(failures, fmt.Errorf("project %s has no identity", p.Name)) + continue + } + d.directory.mu.RLock() + existing, exists := d.directory.projects[p.Name] + d.directory.mu.RUnlock() + ready := p.DeletionTimestamp.IsZero() && apimeta.IsStatusConditionTrue(p.Status.Conditions, resourcemanagerv1alpha1.ProjectReady) + if exists && (existing.config.ProjectUID != p.UID || !ready) { + d.remove(p.Name) + exists = false + } + if !ready || exists { + continue + } + projectREST, err := projectRESTConfig(d.rest, p.Name) + if err != nil { + failures = errors.Join(failures, err) + continue + } + worker, err := d.makeProject(ProjectConfig{Name: p.Name, ProjectUID: p.UID, KubeAPI: d.kubeAPI}, projectREST) + if err != nil { + failures = errors.Join(failures, err) + continue + } + sourceCtx, cancel := context.WithCancel(ctx) + d.directory.mu.Lock() + d.directory.projects[p.Name] = directoryProject{projectRuntime: worker, cancel: cancel} + d.directory.mu.Unlock() + go runProjectSource(sourceCtx, worker) + } + d.directory.mu.RLock() + names := make([]string, 0, len(d.directory.projects)) + for name := range d.directory.projects { + names = append(names, name) + } + d.directory.mu.RUnlock() + for _, name := range names { + if !seen[name] { + d.remove(name) + } + } + return failures +} + +func (d *projectDiscovery) remove(name string) { + d.directory.mu.Lock() + defer d.directory.mu.Unlock() + if p, found := d.directory.projects[name]; found { + if p.cancel != nil { + p.cancel() + } + delete(d.directory.projects, name) + } +} + +func projectRESTConfig(base *rest.Config, name string) (*rest.Config, error) { + endpoint, err := url.Parse(base.Host) + if err != nil || endpoint.Scheme == "" || endpoint.Host == "" { + return nil, fmt.Errorf("invalid project discovery API endpoint %q", base.Host) + } + endpoint.Path = path.Join(endpoint.Path, "apis/resourcemanager.miloapis.com/v1alpha1/projects", name, "control-plane") + config := rest.CopyConfig(base) + config.Host = endpoint.String() + return config, nil +} + +func projectCache(namespace string) cache.Options { + if namespace == "" { + return cache.Options{} + } + return cache.Options{DefaultNamespaces: map[string]cache.Config{namespace: {}}} +} diff --git a/internal/internaldns/runtime/project_discovery_test.go b/internal/internaldns/runtime/project_discovery_test.go new file mode 100644 index 00000000..6fa884e4 --- /dev/null +++ b/internal/internaldns/runtime/project_discovery_test.go @@ -0,0 +1,157 @@ +package runtime + +import ( + "context" + "errors" + "strings" + "testing" + "time" + + dnsv1 "go.miloapis.com/dns-operator/api/v1alpha1" + resourcemanagerv1alpha1 "go.miloapis.com/milo/pkg/apis/resourcemanager/v1alpha1" + admissionv1 "k8s.io/api/admission/v1" + authenticationv1 "k8s.io/api/authentication/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/rest" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + kubeadmission "sigs.k8s.io/controller-runtime/pkg/webhook/admission" +) + +type discoveryFixtureClient struct { + client.Client + unavailable bool +} + +func (c *discoveryFixtureClient) List(ctx context.Context, obj client.ObjectList, opts ...client.ListOption) error { + if c.unavailable { + return errors.New("discovery unavailable") + } + return c.Client.List(ctx, obj, opts...) +} + +func TestProjectDiscoveryAddsSourcesAndPreservesThemDuringOutage(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + scheme, err := newScheme() + if err != nil { + t.Fatal(err) + } + first := readyDiscoveryProject("first", "first-uid") + core := &discoveryFixtureClient{Client: fake.NewClientBuilder().WithScheme(scheme).WithObjects(first).Build()} + directory := newProjectDirectory(nil) + starts := make(chan types.UID, 4) + d := &projectDiscovery{client: core, directory: directory, rest: &rest.Config{Host: "https://milo.example:6443"}, makeProject: func(config ProjectConfig, sourceREST *rest.Config) (projectRuntime, error) { + if config.Namespace != "" || !strings.HasSuffix(sourceREST.Host, "/projects/"+config.Name+"/control-plane") { + t.Errorf("unexpected discovery scope: %#v %s", config, sourceREST.Host) + } + direct := fake.NewClientBuilder().WithScheme(scheme).Build() + return projectRuntime{config: config, direct: direct, start: func(workerCtx context.Context) error { + starts <- config.ProjectUID + <-workerCtx.Done() + return workerCtx.Err() + }}, nil + }} + if err := d.step(ctx); err != nil { + t.Fatal(err) + } + awaitDiscoveredSource(t, starts, first.UID) + second := readyDiscoveryProject("second", "second-uid") + if err := core.Create(ctx, second); err != nil { + t.Fatal(err) + } + if err := d.step(ctx); err != nil { + t.Fatal(err) + } + awaitDiscoveredSource(t, starts, second.UID) + scopes := directory.scopes() + if len(scopes) != 2 || scopes[0].Namespace != "" || scopes[1].Namespace != "" { + t.Fatalf("ACK scopes did not follow discovery: %#v", scopes) + } + core.unavailable = true + if err := d.step(ctx); err == nil { + t.Fatal("expected discovery error") + } + if len(directory.scopes()) != 2 { + t.Fatal("discovery outage discarded existing sources") + } + core.unavailable = false + request := kubeadmission.Request{AdmissionRequest: admissionv1.AdmissionRequest{Namespace: "internal-dns-platform", UserInfo: authenticationv1.UserInfo{Extra: map[string]authenticationv1.ExtraValue{parentNameExtra: {second.Name}}}}} + selected, err := directory.selectProject(ctx, request) + if err != nil || selected != scopes[1].Client { + t.Fatalf("dynamic admission did not map all namespaces: %v", err) + } + if err := core.Delete(ctx, second); err != nil { + t.Fatal(err) + } + if _, err := directory.selectProject(ctx, request); err == nil { + t.Fatal("deleted project passed admission") + } + recreated := readyDiscoveryProject(second.Name, "replacement-uid") + if err := core.Create(ctx, recreated); err != nil { + t.Fatal(err) + } + if _, err := directory.selectProject(ctx, request); err == nil { + t.Fatal("recreated project used previous source") + } + if err := d.step(ctx); err != nil { + t.Fatal(err) + } + awaitDiscoveredSource(t, starts, recreated.UID) + if _, err := directory.selectProject(ctx, request); err != nil { + t.Fatal(err) + } + if err := core.Delete(ctx, recreated); err != nil { + t.Fatal(err) + } + if err := d.step(ctx); err != nil { + t.Fatal(err) + } + if len(directory.scopes()) != 1 { + t.Fatal("ACK scopes retained deleted source") + } +} + +func readyDiscoveryProject(name string, uid types.UID) *resourcemanagerv1alpha1.Project { + return &resourcemanagerv1alpha1.Project{ObjectMeta: metav1.ObjectMeta{Name: name, UID: uid}, Status: resourcemanagerv1alpha1.ProjectStatus{Conditions: []metav1.Condition{{Type: resourcemanagerv1alpha1.ProjectReady, Status: metav1.ConditionTrue, Reason: "Ready", LastTransitionTime: metav1.Now()}}}} +} +func awaitDiscoveredSource(t *testing.T, starts <-chan types.UID, expected types.UID) { + t.Helper() + select { + case actual := <-starts: + if actual != expected { + t.Fatalf("started %s want %s", actual, expected) + } + case <-time.After(time.Second): + t.Fatal("new project source did not start") + } +} + +func TestAllNamespaceRoutingPreservesListsAndStatus(t *testing.T) { + scheme, err := newScheme() + if err != nil { + t.Fatal(err) + } + zone := &dnsv1.DNSZone{ObjectMeta: metav1.ObjectMeta{Name: "same-name", Namespace: "internal-dns-platform"}} + other := &dnsv1.DNSZone{ObjectMeta: metav1.ObjectMeta{Name: zone.Name, Namespace: "other"}} + source := fake.NewClientBuilder().WithScheme(scheme).WithObjects(zone, other).WithStatusSubresource(zone, other).Build() + managed := fake.NewClientBuilder().WithScheme(scheme).Build() + routed := &RoutingClient{Client: managed, Source: source, PlatformNamespace: zone.Namespace} + var zones dnsv1.DNSZoneList + if err := routed.List(context.Background(), &zones); err != nil { + t.Fatal(err) + } + if len(zones.Items) != 2 { + t.Fatalf("source namespaces were dropped: %#v", zones.Items) + } + current := &zones.Items[0] + base := current.DeepCopy() + current.Status.Conditions = []metav1.Condition{{Type: "Published", Status: metav1.ConditionTrue, Reason: "Ready", LastTransitionTime: metav1.Now()}} + if err := routed.Status().Patch(context.Background(), current, client.MergeFrom(base)); err != nil { + t.Fatal(err) + } + if err := source.Get(context.Background(), client.ObjectKeyFromObject(current), current); err != nil || len(current.Status.Conditions) != 1 { + t.Fatalf("status did not reach source: %v", err) + } +} diff --git a/internal/internaldns/runtime/routing_client.go b/internal/internaldns/runtime/routing_client.go index 38211767..e1e2c53e 100644 --- a/internal/internaldns/runtime/routing_client.go +++ b/internal/internaldns/runtime/routing_client.go @@ -3,6 +3,8 @@ package runtime import ( "context" + dnsv1 "go.miloapis.com/dns-operator/api/v1alpha1" + k8sruntime "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -16,24 +18,23 @@ type RoutingClient struct { PlatformNamespace string } -func (c *RoutingClient) target(obj client.Object) client.Client { - if obj != nil && obj.GetNamespace() == c.PlatformNamespace { - return c.Client - } - return c.Source -} -func (c *RoutingClient) targetNamespace(namespace string) client.Client { - if namespace == c.PlatformNamespace { +func (c *RoutingClient) target(obj k8sruntime.Object) client.Client { + switch obj.(type) { + case *dnsv1.DNSZone, *dnsv1.DNSZoneList, *dnsv1.DNSZoneAssociation, *dnsv1.DNSZoneAssociationList, + *dnsv1.DNSRecordSet, *dnsv1.DNSRecordSetList, *dnsv1.DNSResolverContext, *dnsv1.DNSResolverContextList, + *dnsv1.DNSResolverAccessBinding, *dnsv1.DNSResolverAccessBindingList, + *dnsv1.DNSRegistration, *dnsv1.DNSRegistrationList, *dnsv1.DNSNamingPolicy, *dnsv1.DNSNamingPolicyList, + *dnsv1.DNSRecordContribution, *dnsv1.DNSRecordContributionList: + return c.Source + default: return c.Client } - return c.Source } func (c *RoutingClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { - return c.targetNamespace(key.Namespace).Get(ctx, key, obj, opts...) + return c.target(obj).Get(ctx, key, obj, opts...) } func (c *RoutingClient) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { - o := (&client.ListOptions{}).ApplyOptions(opts) - return c.targetNamespace(o.Namespace).List(ctx, list, opts...) + return c.target(list).List(ctx, list, opts...) } func (c *RoutingClient) Apply(ctx context.Context, obj k8sruntime.ApplyConfiguration, opts ...client.ApplyOption) error { return c.Client.Apply(ctx, obj, opts...) @@ -51,8 +52,7 @@ func (c *RoutingClient) Patch(ctx context.Context, obj client.Object, p client.P return c.target(obj).Patch(ctx, obj, p, opts...) } func (c *RoutingClient) DeleteAllOf(ctx context.Context, obj client.Object, opts ...client.DeleteAllOfOption) error { - o := (&client.DeleteAllOfOptions{}).ApplyOptions(opts) - return c.targetNamespace(o.Namespace).DeleteAllOf(ctx, obj, opts...) + return c.target(obj).DeleteAllOf(ctx, obj, opts...) } func (c *RoutingClient) Status() client.SubResourceWriter { return &routingSubresource{parent: c, name: "status"} diff --git a/internal/internaldns/runtime/routing_client_test.go b/internal/internaldns/runtime/routing_client_test.go index a9363dd0..63defced 100644 --- a/internal/internaldns/runtime/routing_client_test.go +++ b/internal/internaldns/runtime/routing_client_test.go @@ -6,6 +6,7 @@ import ( "context" "testing" + dnsv1 "go.miloapis.com/dns-operator/api/v1alpha1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -18,11 +19,14 @@ func TestRoutingClientKeepsSourceAndPlatformObjectsSeparate(t *testing.T) { if err := corev1.AddToScheme(scheme); err != nil { t.Fatal(err) } + if err := dnsv1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } source := fake.NewClientBuilder().WithScheme(scheme).Build() platform := fake.NewClientBuilder().WithScheme(scheme).Build() routed := &RoutingClient{Client: platform, Source: source, PlatformNamespace: "internal-dns-system"} ctx := context.Background() - projectObject := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "input", Namespace: "project-a"}} + projectObject := &dnsv1.DNSZone{ObjectMeta: metav1.ObjectMeta{Name: "input", Namespace: "internal-dns-system"}} platformObject := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "manifest", Namespace: "internal-dns-system"}} if err := routed.Create(ctx, projectObject); err != nil { t.Fatal(err) @@ -31,18 +35,19 @@ func TestRoutingClientKeepsSourceAndPlatformObjectsSeparate(t *testing.T) { t.Fatal(err) } for _, tc := range []struct { - name string - cl client.Client - key client.ObjectKey - found bool + name string + cl client.Client + key client.ObjectKey + found bool + object client.Object }{ - {"project in source", source, client.ObjectKeyFromObject(projectObject), true}, - {"project absent from platform", platform, client.ObjectKeyFromObject(projectObject), false}, - {"artifact in platform", platform, client.ObjectKeyFromObject(platformObject), true}, - {"artifact absent from source", source, client.ObjectKeyFromObject(platformObject), false}, + {"project in source", source, client.ObjectKeyFromObject(projectObject), true, &dnsv1.DNSZone{}}, + {"project absent from platform", platform, client.ObjectKeyFromObject(projectObject), false, &dnsv1.DNSZone{}}, + {"artifact in platform", platform, client.ObjectKeyFromObject(platformObject), true, &corev1.ConfigMap{}}, + {"artifact absent from source", source, client.ObjectKeyFromObject(platformObject), false, &corev1.ConfigMap{}}, } { t.Run(tc.name, func(t *testing.T) { - err := tc.cl.Get(ctx, tc.key, &corev1.ConfigMap{}) + err := tc.cl.Get(ctx, tc.key, tc.object) if (err == nil) != tc.found { t.Fatalf("found=%v want=%v err=%v", err == nil, tc.found, err) } diff --git a/internal/internaldns/runtime/run.go b/internal/internaldns/runtime/run.go index cabb2808..d4a8d043 100644 --- a/internal/internaldns/runtime/run.go +++ b/internal/internaldns/runtime/run.go @@ -22,6 +22,7 @@ import ( "go.miloapis.com/dns-operator/internal/internaldns/serving" "go.miloapis.com/dns-operator/internal/internaldns/transport" "go.miloapis.com/dns-operator/internal/internaldns/watchdog" + resourcemanagerv1alpha1 "go.miloapis.com/milo/pkg/apis/resourcemanager/v1alpha1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" clientgoscheme "k8s.io/client-go/kubernetes/scheme" @@ -29,7 +30,7 @@ import ( "k8s.io/client-go/tools/clientcmd" "k8s.io/client-go/util/flowcontrol" ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/cache" + "sigs.k8s.io/controller-runtime/pkg/client" metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" "sigs.k8s.io/controller-runtime/pkg/webhook" @@ -85,26 +86,26 @@ func runControlPlane(ctx context.Context, cfg Config, kubeconfig string) error { } identity := holderIdentity() - projects := make([]projectRuntime, 0, len(cfg.Projects)) - for _, p := range cfg.Projects { - projectREST := rest.CopyConfig(platformREST) + makeProject := func(p ProjectConfig, sourceREST *rest.Config) (projectRuntime, error) { + projectREST := rest.CopyConfig(sourceREST) if p.Kubeconfig != "" { - projectREST, err = restConfig(p.Kubeconfig) - if err != nil { - return fmt.Errorf("load project %q kubeconfig: %w", p.Name, err) + loaded, loadErr := restConfig(p.Kubeconfig) + if loadErr != nil { + return projectRuntime{}, fmt.Errorf("load project %q kubeconfig: %w", p.Name, loadErr) } + projectREST = loaded } applyKubeAPILimit(projectREST, p.KubeAPI) direct, err := client.New(projectREST, client.Options{Scheme: scheme}) if err != nil { - return fmt.Errorf("create project %q direct client: %w", p.Name, err) + return projectRuntime{}, fmt.Errorf("create project %q direct client: %w", p.Name, err) } direct = &deadlineClient{Client: direct, Timeout: p.KubeAPI.requestTimeout()} project := p start := func(startCtx context.Context) error { mgr, err := ctrl.NewManager(projectREST, ctrl.Options{ Scheme: scheme, - Cache: cache.Options{DefaultNamespaces: map[string]cache.Config{project.Namespace: {}}}, + Cache: projectCache(project.Namespace), Metrics: metricsserver.Options{BindAddress: "0"}, HealthProbeBindAddress: "0", LeaderElection: false, @@ -129,7 +130,30 @@ func runControlPlane(ctx context.Context, cfg Config, kubeconfig string) error { } return mgr.Start(startCtx) } - projects = append(projects, projectRuntime{config: project, direct: direct, start: start}) + return projectRuntime{config: project, direct: direct, start: start}, nil + } + projects := make([]projectRuntime, 0, len(cfg.Projects)) + for _, p := range cfg.Projects { + project, err := makeProject(p, platformREST) + if err != nil { + return err + } + projects = append(projects, project) + } + directory := newProjectDirectory(projects) + var discovery *projectDiscovery + if cfg.ProjectDiscovery != nil { + discoveryREST, loadErr := restConfig(cfg.ProjectDiscovery.Kubeconfig) + if loadErr != nil { + return fmt.Errorf("load project discovery kubeconfig: %w", loadErr) + } + applyKubeAPILimit(discoveryREST, cfg.ProjectDiscovery.KubeAPI) + discoveryClient, clientErr := client.New(discoveryREST, client.Options{Scheme: scheme}) + if clientErr != nil { + return fmt.Errorf("create project discovery client: %w", clientErr) + } + discoveryClient = &deadlineClient{Client: discoveryClient, Timeout: cfg.ProjectDiscovery.KubeAPI.requestTimeout()} + discovery = &projectDiscovery{client: discoveryClient, directory: directory, rest: discoveryREST, kubeAPI: cfg.ProjectDiscovery.KubeAPI, makeProject: makeProject} } var admissionManager ctrl.Manager @@ -145,13 +169,16 @@ func runControlPlane(ctx context.Context, cfg Config, kubeconfig string) error { return fmt.Errorf("create admission manager: %w", err) } handler := &dnsadmission.Handler{ - ClientForRequest: projectSelector(projects), + ClientForRequest: directory.selectProject, } admissionManager.GetWebhookServer().Register(dnsadmission.Path, &kubeadmission.Webhook{Handler: handler}) } errCh := make(chan error, len(projects)+len(planners)*3+8) startProjectSources(ctx, projects) + if discovery != nil { + go retryLoop(ctx, time.Duration(cfg.ProjectDiscovery.IntervalSeconds)*time.Second, "project discovery", discovery.step, errCh) + } if admissionManager != nil { go func() { errCh <- admissionManager.Start(ctx) }() } @@ -161,10 +188,7 @@ func runControlPlane(ctx context.Context, cfg Config, kubeconfig string) error { for _, planner := range planners { publicationRegions[planner.Region+"/"+planner.Shard] = append([]model.Member(nil), planner.Members...) } - projectScopes := make([]platform.ProjectScope, 0, len(projects)) - for _, p := range projects { - projectScopes = append(projectScopes, platform.ProjectScope{Client: p.direct, Namespace: p.config.Namespace, RequestTimeout: p.config.KubeAPI.requestTimeout()}) - } + sinks := map[string]*platform.AckSink{} for _, plannerCfg := range planners { pc := plannerCfg @@ -174,7 +198,7 @@ func runControlPlane(ctx context.Context, cfg Config, kubeconfig string) error { interval = time.Second } go retryLoop(ctx, interval, "planner "+pc.Region+"/"+pc.Shard, planner.Step, errCh) - sink := &platform.AckSink{Client: platformClient, Namespace: cfg.PlatformNamespace, Projects: projectScopes, + sink := &platform.AckSink{Client: platformClient, Namespace: cfg.PlatformNamespace, ProjectScopes: directory.scopes, PublicationMembers: publicationMembers, PublicationRegions: publicationRegions, Region: pc.Region, Shard: pc.Shard, Members: pc.Members} sinks[pc.Region+"/"+pc.Shard] = sink go retryLoop(ctx, 2*time.Second, "ack refresh "+pc.Region+"/"+pc.Shard, sink.Refresh, errCh) @@ -383,6 +407,9 @@ func newScheme() (*runtime.Scheme, error) { if err := clientgoscheme.AddToScheme(scheme); err != nil { return nil, err } + if err := resourcemanagerv1alpha1.AddToScheme(scheme); err != nil { + return nil, err + } if err := corev1.AddToScheme(scheme); err != nil { return nil, err } From d7d08a939e5b864a0f7ba395e5dde1cd514dabf6 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Wed, 14 Oct 2026 17:16:39 -0500 Subject: [PATCH 06/19] test: qualify named references with CoreDNS --- .../runtime/deployment_permissions_test.go | 14 +++++++------- test/internaldns/kubernetes/qualify/main.go | 14 +++++++------- test/internaldns/kubernetes/qualify/scenarios.go | 10 +++++----- 3 files changed, 19 insertions(+), 19 deletions(-) diff --git a/internal/internaldns/runtime/deployment_permissions_test.go b/internal/internaldns/runtime/deployment_permissions_test.go index 224d03de..ed105150 100644 --- a/internal/internaldns/runtime/deployment_permissions_test.go +++ b/internal/internaldns/runtime/deployment_permissions_test.go @@ -265,13 +265,13 @@ func verifyProjectReferenceSchemas(t *testing.T, ctx context.Context, cl client. object client.Object valid bool }{ - {"registration-valid", &dnsv1alpha1.DNSRegistration{Spec: dnsv1alpha1.DNSRegistrationSpec{DNSZoneRef: ref, Name: "api", RecordTypes: []dnsv1alpha1.RRType{dnsv1alpha1.RRTypeA}, TTLSeconds: 30}}, true}, - {"registration-unpinned", &dnsv1alpha1.DNSRegistration{Spec: dnsv1alpha1.DNSRegistrationSpec{DNSZoneRef: dnsv1alpha1.DNSObjectReference{Name: zone.Name}, Name: "api", RecordTypes: []dnsv1alpha1.RRType{dnsv1alpha1.RRTypeA}, TTLSeconds: 30}}, false}, - {"registration-ttl31", &dnsv1alpha1.DNSRegistration{Spec: dnsv1alpha1.DNSRegistrationSpec{DNSZoneRef: ref, Name: "api", RecordTypes: []dnsv1alpha1.RRType{dnsv1alpha1.RRTypeA}, TTLSeconds: 31}}, false}, - {"association-valid", &dnsv1alpha1.DNSZoneAssociation{Spec: dnsv1alpha1.DNSZoneAssociationSpec{DNSZoneRef: ref, ResolverContextRef: contextRef}}, true}, - {"association-unpinned", &dnsv1alpha1.DNSZoneAssociation{Spec: dnsv1alpha1.DNSZoneAssociationSpec{DNSZoneRef: dnsv1alpha1.DNSObjectReference{Name: zone.Name}, ResolverContextRef: contextRef}}, false}, - {"naming-valid", &dnsv1alpha1.DNSNamingPolicy{Spec: dnsv1alpha1.DNSNamingPolicySpec{ResolverContextRef: contextRef, AdditionalNames: []dnsv1alpha1.DNSAdditionalNameRule{{Selector: nil, DNSZoneRef: ref, NamePrefix: "services"}}}}, true}, - {"naming-unpinned", &dnsv1alpha1.DNSNamingPolicy{Spec: dnsv1alpha1.DNSNamingPolicySpec{ResolverContextRef: contextRef, AdditionalNames: []dnsv1alpha1.DNSAdditionalNameRule{{Selector: nil, DNSZoneRef: dnsv1alpha1.DNSObjectReference{Name: zone.Name}, NamePrefix: "services"}}}}, false}, + {"registration-name-only", &dnsv1alpha1.DNSRegistration{Spec: dnsv1alpha1.DNSRegistrationSpec{DNSZoneRef: dnsv1alpha1.DNSNameReference{Name: zone.Name}, Name: "api", RecordTypes: []dnsv1alpha1.RRType{dnsv1alpha1.RRTypeA}, TTLSeconds: 30}}, true}, + {"registration-empty-zone", &dnsv1alpha1.DNSRegistration{Spec: dnsv1alpha1.DNSRegistrationSpec{DNSZoneRef: dnsv1alpha1.DNSNameReference{}, Name: "api", RecordTypes: []dnsv1alpha1.RRType{dnsv1alpha1.RRTypeA}, TTLSeconds: 30}}, false}, + {"registration-ttl31", &dnsv1alpha1.DNSRegistration{Spec: dnsv1alpha1.DNSRegistrationSpec{DNSZoneRef: dnsv1alpha1.DNSNameReference{Name: ref.Name}, Name: "api", RecordTypes: []dnsv1alpha1.RRType{dnsv1alpha1.RRTypeA}, TTLSeconds: 31}}, false}, + {"association-valid", &dnsv1alpha1.DNSZoneAssociation{Spec: dnsv1alpha1.DNSZoneAssociationSpec{DNSZoneRef: dnsv1alpha1.DNSNameReference{Name: ref.Name}, ResolverContextRef: dnsv1alpha1.DNSNameReference{Name: contextRef.Name}}}, true}, + {"association-empty-zone", &dnsv1alpha1.DNSZoneAssociation{Spec: dnsv1alpha1.DNSZoneAssociationSpec{DNSZoneRef: dnsv1alpha1.DNSNameReference{}, ResolverContextRef: dnsv1alpha1.DNSNameReference{Name: contextRef.Name}}}, false}, + {"naming-valid", &dnsv1alpha1.DNSNamingPolicy{Spec: dnsv1alpha1.DNSNamingPolicySpec{ResolverContextRef: dnsv1alpha1.DNSNameReference{Name: contextRef.Name}, AdditionalNames: []dnsv1alpha1.DNSAdditionalNameRule{{DNSZoneRef: dnsv1alpha1.DNSNameReference{Name: ref.Name}, NamePrefix: "services"}}}}, true}, + {"naming-empty-zone", &dnsv1alpha1.DNSNamingPolicy{Spec: dnsv1alpha1.DNSNamingPolicySpec{ResolverContextRef: dnsv1alpha1.DNSNameReference{Name: contextRef.Name}, AdditionalNames: []dnsv1alpha1.DNSAdditionalNameRule{{DNSZoneRef: dnsv1alpha1.DNSNameReference{}, NamePrefix: "services"}}}}, false}, } for _, tc := range cases { tc.object.SetName(tc.name) diff --git a/test/internaldns/kubernetes/qualify/main.go b/test/internaldns/kubernetes/qualify/main.go index bfeb22b1..50095d2e 100644 --- a/test/internaldns/kubernetes/qualify/main.go +++ b/test/internaldns/kubernetes/qualify/main.go @@ -290,9 +290,9 @@ func (s *suite) run(path string) error { if a.resolverContext.UID == b.resolverContext.UID || a.zone.UID == b.zone.UID || a.resolverContext.Status.ManagedNamespace.DNSZoneRef.Name == b.resolverContext.Status.ManagedNamespace.DNSZoneRef.Name { return errors.New("source projects lost their distinct context, zone, or managed namespace identity") } - for _, p := range s.env.Projects { - if p.resolverContext.Status.ManagedNamespace.Suffix != "datum.internal" { - return errors.New("managed namespace did not use the default domain") + for _, p := range []*project{a, b} { + if p.resolverContext.Status.ManagedNamespace.Suffix != p.resolverContext.Spec.ManagedNamespace.DomainSuffix { + return fmt.Errorf("%s managed suffix does not match its requested domain", p.Name) } } s.pass("independent projects retain distinct context, zone, and managed namespace identities", map[string]string{"a": string(a.resolverContext.UID), "b": string(b.resolverContext.UID)}) @@ -324,13 +324,13 @@ func (s *suite) permissions(p *project) error { } func (s *suite) prepare(p *project) error { - p.resolverContext = dnsv1.DNSResolverContext{ObjectMeta: metav1.ObjectMeta{Name: "application-network", Namespace: p.Namespace}, Spec: dnsv1.DNSResolverContextSpec{ConsumerID: "qualification/" + p.Name, ManagedNamespace: dnsv1.DNSResolverContextManagedNamespace{Enabled: true}}} + p.resolverContext = dnsv1.DNSResolverContext{ObjectMeta: metav1.ObjectMeta{Name: "application-network", Namespace: p.Namespace}, Spec: dnsv1.DNSResolverContextSpec{ConsumerID: "qualification/" + p.Name, ManagedNamespace: dnsv1.DNSResolverContextManagedNamespace{Enabled: true, DomainSuffix: "datum.internal"}}} if err := p.integration.Create(s.ctx, &p.resolverContext); err != nil { return err } if err := s.wait(p.Name+" managed namespace", 120*time.Second, func() (bool, error) { err := p.product.Get(s.ctx, client.ObjectKeyFromObject(&p.resolverContext), &p.resolverContext) - return condition(p.resolverContext.Status.Conditions, "Ready", p.resolverContext.Generation) && p.resolverContext.Status.ManagedNamespace.DNSZoneRef.UID != "", err + return condition(p.resolverContext.Status.Conditions, "Ready", p.resolverContext.Generation) && p.resolverContext.Status.ManagedNamespace.DNSZoneRef.Name != "", err }); err != nil { return err } @@ -354,7 +354,7 @@ func (s *suite) prepare(p *project) error { } func (s *suite) associate(p *project, zone *dnsv1.DNSZone, name string) error { - association := &dnsv1.DNSZoneAssociation{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: p.Namespace}, Spec: dnsv1.DNSZoneAssociationSpec{DNSZoneRef: objectRef(zone), ResolverContextRef: objectRef(&p.resolverContext)}} + association := &dnsv1.DNSZoneAssociation{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: p.Namespace}, Spec: dnsv1.DNSZoneAssociationSpec{DNSZoneRef: dnsv1.DNSNameReference{Name: zone.Name}, ResolverContextRef: dnsv1.DNSNameReference{Name: p.resolverContext.Name}}} if err := p.customer.Create(s.ctx, association); err != nil { return err } @@ -385,7 +385,7 @@ func recordSets(name, address, ipv6 string) []dnsv1.DNSContributionRecordSet { return sets } -func (s *suite) publish(p *project, zone dnsv1.DNSObjectReference, prefix, name, address, ipv6 string) (*publication, error) { +func (s *suite) publish(p *project, zone dnsv1.DNSNameReference, prefix, name, address, ipv6 string) (*publication, error) { pub := &publication{registration: dnsv1.DNSRegistration{ObjectMeta: metav1.ObjectMeta{Name: prefix + "-registration", Namespace: p.Namespace}, Spec: dnsv1.DNSRegistrationSpec{DNSZoneRef: zone, Name: name, RecordTypes: []dnsv1.RRType{dnsv1.RRTypeA}, TTLSeconds: 2}}} if ipv6 != "" { pub.registration.Spec.RecordTypes = append(pub.registration.Spec.RecordTypes, dnsv1.RRTypeAAAA) diff --git a/test/internaldns/kubernetes/qualify/scenarios.go b/test/internaldns/kubernetes/qualify/scenarios.go index 5945d566..b97c6eee 100644 --- a/test/internaldns/kubernetes/qualify/scenarios.go +++ b/test/internaldns/kubernetes/qualify/scenarios.go @@ -35,11 +35,11 @@ func (s *suite) scenarios(a, b *project) error { return err } } - pa, err := s.publish(a, objectRef(&a.zone), "common", "common", "10.42.0.10", "fd42::10") + pa, err := s.publish(a, dnsv1.DNSNameReference{Name: a.zone.Name}, "common", "common", "10.42.0.10", "fd42::10") if err != nil { return err } - pb, err := s.publish(b, objectRef(&b.zone), "common", "common", "10.42.0.20", "fd42::20") + pb, err := s.publish(b, dnsv1.DNSNameReference{Name: b.zone.Name}, "common", "common", "10.42.0.20", "fd42::20") if err != nil { return err } @@ -86,7 +86,7 @@ func (s *suite) scenarios(a, b *project) error { return err } s.pass("an unbound destination cannot resolve either tenant's private records", nil) - if _, err := s.publish(a, objectRef(&a.zone), "only-a", "only-a", "10.42.0.20", ""); err != nil { + if _, err := s.publish(a, dnsv1.DNSNameReference{Name: a.zone.Name}, "only-a", "only-a", "10.42.0.20", ""); err != nil { return err } if err := s.expect(a, "only-a.prod.internal", "A", false, dns.RcodeSuccess, "10.42.0.20"); err != nil { @@ -125,7 +125,7 @@ func (s *suite) scenarios(a, b *project) error { if err := s.associate(a, apps, "apps"); err != nil { return err } - if _, err := s.publish(a, objectRef(apps), "apps", "api", "10.42.0.12", ""); err != nil { + if _, err := s.publish(a, dnsv1.DNSNameReference{Name: apps.Name}, "apps", "api", "10.42.0.12", ""); err != nil { return err } if err := s.expect(a, "api.apps.internal", "A", true, dns.RcodeSuccess, "10.42.0.12"); err != nil { @@ -173,7 +173,7 @@ func (s *suite) scenarios(a, b *project) error { if err := s.observe(b, pb, true); err != nil { return err } - deleted, err := s.publish(a, objectRef(&a.zone), "deleted", "deleted", "10.42.0.14", "") + deleted, err := s.publish(a, dnsv1.DNSNameReference{Name: a.zone.Name}, "deleted", "deleted", "10.42.0.14", "") if err != nil { return err } From 4ec3c8c7e6adbe328a56c867a80bcba605ee7e28 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Thu, 15 Oct 2026 07:31:29 -0500 Subject: [PATCH 07/19] fix: detect native dnsdist console clients Use the supported client startup predicate in the shared configuration. --- internal/internaldns/serving/render.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/internaldns/serving/render.go b/internal/internaldns/serving/render.go index ceec28e5..8a4ad7f3 100644 --- a/internal/internaldns/serving/render.go +++ b/internal/internaldns/serving/render.go @@ -202,7 +202,7 @@ func collectBackends(bindings []model.Binding, get func(model.Binding) []model.B func renderDNSDist(c DNSDistRenderConfig, backends []model.Backend, bindings []model.Binding, active map[string]bool, cluster bool, eligible map[string]map[string]bool, zones map[string]map[string]map[string]bool) []byte { var out bytes.Buffer endpoint := dnsdistConsole(c.Path) - fmt.Fprint(&out, "local client = isClient()\nif client or not internalDNSInitialized then\n") + fmt.Fprint(&out, "local client = inClientStartup()\nif client or not internalDNSInitialized then\n") fmt.Fprintf(&out, " local key\n if os.getenv(\"INTERNAL_DNS_VALIDATE\") == \"1\" then key = string.rep(\"A\",43)..\"=\" else\n local handle = assert(io.open(%q,\"r\"))\n key = handle:read(\"*l\"); handle:close()\n end\n setKey(key)\n controlSocket(%q)\n setConsoleACL({\"127.0.0.1/32\"})\nend\nif not client then\n", DNSDistKeyPath, endpoint) topology, _ := json.Marshal(struct { Listen string From 82edb3aa2405854184fa5a7d0c0820ea1634169f Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Wed, 21 Oct 2026 18:42:57 -0500 Subject: [PATCH 08/19] fix: scope DNS publication status to its project --- .../controlplane/publication_reconciler.go | 2 +- internal/internaldns/platform/acks.go | 8 +++++++- .../internaldns/platform/platform_test.go | 20 +++++++++++++++++++ .../internaldns/runtime/project_discovery.go | 2 +- 4 files changed, 29 insertions(+), 3 deletions(-) diff --git a/internal/internaldns/controlplane/publication_reconciler.go b/internal/internaldns/controlplane/publication_reconciler.go index 4abdbaf1..63b4039d 100644 --- a/internal/internaldns/controlplane/publication_reconciler.go +++ b/internal/internaldns/controlplane/publication_reconciler.go @@ -413,7 +413,7 @@ func (r *Reconciler) persistPublication(ctx context.Context, z *dnsv1alpha1.DNSZ for _, target := range servingTargets { apiTargets = append(apiTargets, dnsv1alpha1.DNSPublicationTarget{Region: target.Region, Shard: target.Shard}) } - m := &dnsv1alpha1.DNSPublicationManifest{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: r.Options.PlatformNamespace, Labels: map[string]string{"internal-dns.miloapis.com/zone-uid": string(z.UID)}}, Spec: dnsv1alpha1.DNSPublicationManifestSpec{ZoneRef: dnsv1alpha1.DNSObjectReference{Name: z.Name, UID: z.UID}, ZoneApex: z.Spec.DomainName, ContextUIDs: contextUIDs, WriterEpoch: epoch, Revision: revision, PreviousRevision: previous, Tombstone: tombstone, Chunks: refs, ContentHash: hash, GeneratedAt: metav1.NewTime(now), ContributionFences: apiFences, RegistrationFences: apiRegistrations, ServingTargets: apiTargets}} + m := &dnsv1alpha1.DNSPublicationManifest{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: r.Options.PlatformNamespace, Labels: map[string]string{"internal-dns.miloapis.com/zone-uid": string(z.UID), "internal-dns.miloapis.com/project-uid": string(r.Options.ProjectUID)}}, Spec: dnsv1alpha1.DNSPublicationManifestSpec{ZoneRef: dnsv1alpha1.DNSObjectReference{Name: z.Name, UID: z.UID}, ZoneApex: z.Spec.DomainName, ContextUIDs: contextUIDs, WriterEpoch: epoch, Revision: revision, PreviousRevision: previous, Tombstone: tombstone, Chunks: refs, ContentHash: hash, GeneratedAt: metav1.NewTime(now), ContributionFences: apiFences, RegistrationFences: apiRegistrations, ServingTargets: apiTargets}} if err := r.createImmutable(ctx, m); err != nil { return nil, err } diff --git a/internal/internaldns/platform/acks.go b/internal/internaldns/platform/acks.go index d185bab0..37f87e77 100644 --- a/internal/internaldns/platform/acks.go +++ b/internal/internaldns/platform/acks.go @@ -26,6 +26,7 @@ const dnsValueNode = "node" // to its own AckSubject. It also verifies membership, region, resource lifetime, // revision and freshness before changing project-facing publication status. type ProjectScope struct { + ProjectUID string Client client.Client Namespace string RequestTimeout time.Duration @@ -190,7 +191,12 @@ func (s *AckSink) projectPublication(ctx context.Context, m *dnsv1alpha1.DNSPubl projects = append(projects, ProjectScope{Client: s.Client, Namespace: ns}) } results := make(chan error, len(projects)) + started := 0 for _, project := range projects { + if owner := m.Labels["internal-dns.miloapis.com/project-uid"]; owner != "" && project.ProjectUID != owner { + continue + } + started++ go func() { projectCtx := ctx @@ -203,7 +209,7 @@ func (s *AckSink) projectPublication(ctx context.Context, m *dnsv1alpha1.DNSPubl }() } var projectErrors error - for range projects { + for i := 0; i < started; i++ { projectErrors = errors.Join(projectErrors, <-results) } return projectErrors diff --git a/internal/internaldns/platform/platform_test.go b/internal/internaldns/platform/platform_test.go index 41f8d630..a991a761 100644 --- a/internal/internaldns/platform/platform_test.go +++ b/internal/internaldns/platform/platform_test.go @@ -555,3 +555,23 @@ func TestServingAvailabilityDoesNotRequireEveryReplica(t *testing.T) { t.Fatal("stale observations claimed a serving outage") } } + +func TestPublicationProjectionOnlyReadsOwningProject(t *testing.T) { + cl := testClient(t) + unrelated := &blockingListClient{Client: cl, started: make(chan struct{})} + manifest := &dnsv1alpha1.DNSPublicationManifest{ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{"internal-dns.miloapis.com/project-uid": "owner"}, + }} + sink := AckSink{Client: cl, Projects: []ProjectScope{ + {ProjectUID: "unrelated", Client: unrelated, RequestTimeout: 20 * time.Millisecond}, + {ProjectUID: "owner", Client: cl}, + }} + if err := sink.projectPublication(context.Background(), manifest, false, time.Now()); err != nil { + t.Fatal(err) + } + select { + case <-unrelated.started: + t.Fatal("publication contacted an unrelated project") + default: + } +} diff --git a/internal/internaldns/runtime/project_discovery.go b/internal/internaldns/runtime/project_discovery.go index 62cb1349..940adb0e 100644 --- a/internal/internaldns/runtime/project_discovery.go +++ b/internal/internaldns/runtime/project_discovery.go @@ -49,7 +49,7 @@ func (d *projectDirectory) scopes() []platform.ProjectScope { sort.Strings(names) for _, name := range names { p := d.projects[name] - scopes = append(scopes, platform.ProjectScope{Client: p.direct, Namespace: p.config.Namespace, RequestTimeout: p.config.KubeAPI.requestTimeout()}) + scopes = append(scopes, platform.ProjectScope{ProjectUID: string(p.config.ProjectUID), Client: p.direct, Namespace: p.config.Namespace, RequestTimeout: p.config.KubeAPI.requestTimeout()}) } return scopes } From 65755a53960ec931a6ce5fcae204485a52ddfd77 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Wed, 21 Oct 2026 18:54:16 -0500 Subject: [PATCH 09/19] fix: process fleet acknowledgments concurrently Use a bounded set of consumers so healthy fleet reports remain current while Kubernetes status updates are in flight. Optimistic writes and observation ordering retain protection against stale acknowledgments. --- internal/internaldns/runtime/run.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/internal/internaldns/runtime/run.go b/internal/internaldns/runtime/run.go index 7b4643c3..06d6389d 100644 --- a/internal/internaldns/runtime/run.go +++ b/internal/internaldns/runtime/run.go @@ -312,11 +312,14 @@ func runControlPlaneRegion(ctx context.Context, cfg Config, region string, plann continue } workerCtx, cancel := context.WithCancel(ctx) - localDone := make(chan error, len(consumers)+1) + const ackWorkers = 4 + localDone := make(chan error, len(consumers)*ackWorkers+1) outbox := &platform.Outbox{Client: platformClient, Namespace: cfg.PlatformNamespace, Region: region, Publisher: route.publisher, Bootstrap: route.bootstrap, PendingOnly: true} go retryLoop(workerCtx, 500*time.Millisecond, "outbox "+region, outbox.Step, localDone) for key, consumer := range consumers { - go consumeAcks(workerCtx, consumer, sinks[key], localDone) + for range ackWorkers { + go consumeAcks(workerCtx, consumer, sinks[key], localDone) + } } <-ctx.Done() cancel() From 513598766d1af25cbfe50eedbfd84d82dc143c23 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Wed, 21 Oct 2026 19:43:48 -0500 Subject: [PATCH 10/19] fix: stabilize DNS health reconciliation Preserve access readiness transition timestamps when serving health is\nunchanged. Continue waiting after an idle NATS pull expires while retaining\nconnection and heartbeat errors. --- .../controlplane/context_reconciler.go | 3 +- .../controlplane/context_reconciler_test.go | 24 +++++++++ internal/internaldns/transport/jetstream.go | 17 +++++-- .../internaldns/transport/jetstream_test.go | 51 ++++++++++++++++++- 4 files changed, 90 insertions(+), 5 deletions(-) diff --git a/internal/internaldns/controlplane/context_reconciler.go b/internal/internaldns/controlplane/context_reconciler.go index 2475f147..d48c7d8b 100644 --- a/internal/internaldns/controlplane/context_reconciler.go +++ b/internal/internaldns/controlplane/context_reconciler.go @@ -388,11 +388,12 @@ func (r *Reconciler) reconcileContextBindings(ctx context.Context, ns string, as binding = &old } a.Status.ObservedGeneration = a.Generation - setCondition(&a.Status.Conditions, "Ready", metav1.ConditionFalse, "PendingReplicaVerification", "serving fleet acknowledgement is pending", a.Generation, now) if c := apimeta.FindStatusCondition(binding.Status.Conditions, "Serving"); c != nil && c.Status == metav1.ConditionTrue && binding.Status.ObservedConfigurationRevision == binding.Spec.Configuration.Revision && bindingAckCurrent(binding, now) { setCondition(&a.Status.Conditions, "Ready", metav1.ConditionTrue, "Ready", "serving capacity applied this access generation", a.Generation, now) } else if c != nil && c.Status == metav1.ConditionUnknown { setCondition(&a.Status.Conditions, "Ready", metav1.ConditionUnknown, "ObservationUnavailable", "current serving health is unavailable", a.Generation, now) + } else { + setCondition(&a.Status.Conditions, "Ready", metav1.ConditionFalse, "PendingReplicaVerification", "serving fleet acknowledgement is pending", a.Generation, now) } if !jsonEqual(base.Status, a.Status) { if err := r.Status().Patch(ctx, a, client.MergeFromWithOptions(base, client.MergeFromWithOptimisticLock{})); err != nil { diff --git a/internal/internaldns/controlplane/context_reconciler_test.go b/internal/internaldns/controlplane/context_reconciler_test.go index e07a7197..898e05d8 100644 --- a/internal/internaldns/controlplane/context_reconciler_test.go +++ b/internal/internaldns/controlplane/context_reconciler_test.go @@ -95,6 +95,30 @@ func TestContextAccessPersistsUntilExplicitWithdrawal(t *testing.T) { if access.Status.ObservedGeneration != 1 { t.Fatalf("source generation not observed: %#v", access.Status) } + until := metav1.NewTime(now.Add(time.Minute)) + b.Status.ObservedConfigurationRevision = b.Spec.Configuration.Revision + b.Status.MemberAcknowledgements = []dnsv1alpha1.DNSApplyAcknowledgement{{Phase: string(model.AckVerified), Revision: b.Spec.Configuration.Revision, AccessEpoch: b.Spec.Version.Epoch, AccessSequence: b.Spec.Version.Sequence, ValidUntil: &until}} + setCondition(&b.Status.Conditions, "Serving", metav1.ConditionTrue, "Ready", "", b.Generation, now) + if err := c.Status().Update(ctx, &b); err != nil { + t.Fatal(err) + } + for i := 0; i < 2; i++ { + if err := r.reconcileContextBindings(ctx, "project", []dnsv1alpha1.DNSZoneAssociation{*association}, map[string]*dnsv1alpha1.DNSZone{"zone": zone}, now.Add(time.Duration(i)*time.Second)); err != nil { + t.Fatal(err) + } + previousVersion := access.ResourceVersion + if err := c.Get(ctx, client.ObjectKeyFromObject(access), access); err != nil { + t.Fatal(err) + } + ready := apimeta.FindStatusCondition(access.Status.Conditions, "Ready") + if ready == nil || ready.Status != metav1.ConditionTrue || !ready.LastTransitionTime.Time.Equal(now) { + t.Fatalf("unchanged serving state changed its transition: %#v", ready) + } + if i > 0 && access.ResourceVersion != previousVersion { + t.Fatal("unchanged serving state wrote access status") + } + } + extraZone := zone.DeepCopy() extraZone.Name = "extra" extraZone.UID = "extra-uid" diff --git a/internal/internaldns/transport/jetstream.go b/internal/internaldns/transport/jetstream.go index 8f300b8a..da5b3c1c 100644 --- a/internal/internaldns/transport/jetstream.go +++ b/internal/internaldns/transport/jetstream.go @@ -285,9 +285,20 @@ func (j *JetStream) Consumer(ctx context.Context, c ConsumerConfig) (*Consumer, // Next returns one event. Malformed envelopes are terminated so a poison event // cannot block every later revision on an ordered durable. func (c *Consumer) Next(ctx context.Context) (Message, error) { - m, err := c.c.Next(jetstream.FetchContext(ctx)) - if err != nil { - return nil, err + var m jetstream.Msg + for { + if err := ctx.Err(); err != nil { + return nil, err + } + var err error + m, err = c.c.Next(jetstream.FetchContext(ctx)) + if errors.Is(err, nats.ErrTimeout) { + continue + } + if err != nil { + return nil, err + } + break } var env model.Envelope if err := json.Unmarshal(m.Data(), &env); err != nil { diff --git a/internal/internaldns/transport/jetstream_test.go b/internal/internaldns/transport/jetstream_test.go index 8a9e1456..9aa35b6e 100644 --- a/internal/internaldns/transport/jetstream_test.go +++ b/internal/internaldns/transport/jetstream_test.go @@ -1,6 +1,12 @@ package transport -import "testing" +import ( + "context" + "errors" + "github.com/nats-io/nats.go" + "github.com/nats-io/nats.go/jetstream" + "testing" +) func TestProductionTransportRequiresTLSAndOneAuthMethod(t *testing.T) { t.Parallel() @@ -53,3 +59,46 @@ func TestBrokerMessageIDIsNamespacedBySubject(t *testing.T) { t.Fatal("message ID is not stable for an ambiguous retry") } } + +type nextResultConsumer struct { + jetstream.Consumer + next func() (jetstream.Msg, error) +} + +func (c nextResultConsumer) Next(...jetstream.FetchOpt) (jetstream.Msg, error) { + return c.next() +} + +func TestConsumerWaitsThroughIdlePulls(t *testing.T) { + for _, finalError := range []error{jetstream.ErrNoHeartbeat, nats.ErrConnectionClosed} { + t.Run(finalError.Error(), func(t *testing.T) { + calls := 0 + c := Consumer{c: nextResultConsumer{next: func() (jetstream.Msg, error) { + calls++ + if calls == 1 { + return nil, nats.ErrTimeout + } + return nil, finalError + }}} + _, err := c.Next(context.Background()) + if !errors.Is(err, finalError) || calls != 2 { + t.Fatalf("got %v after %d pulls; want %v after idle pull", err, calls, finalError) + } + }) + } +} + +func TestConsumerIdlePullHonorsCancellation(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + calls := 0 + c := Consumer{c: nextResultConsumer{next: func() (jetstream.Msg, error) { + calls++ + cancel() + return nil, nats.ErrTimeout + }}} + _, err := c.Next(ctx) + if !errors.Is(err, context.Canceled) || calls != 1 { + t.Fatalf("got %v after %d pulls; want cancellation", err, calls) + } +} From 5e0148c7cec9374a552c98d18330b51be77f43e1 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Wed, 21 Oct 2026 19:51:06 -0500 Subject: [PATCH 11/19] feat: expose internal DNS operation metrics Expose an optional aggregate control-plane metrics endpoint. Report loop\nerrors and successful completion timestamps so monitoring can distinguish\nhealthy Pods from stalled configuration delivery. --- config/internal-dns/README.md | 18 +++++++ internal/internaldns/runtime/config.go | 1 + internal/internaldns/runtime/metrics.go | 33 ++++++++++++ internal/internaldns/runtime/metrics_test.go | 54 ++++++++++++++++++++ internal/internaldns/runtime/run.go | 23 +++++++-- 5 files changed, 126 insertions(+), 3 deletions(-) create mode 100644 internal/internaldns/runtime/metrics.go create mode 100644 internal/internaldns/runtime/metrics_test.go diff --git a/config/internal-dns/README.md b/config/internal-dns/README.md index 3ff577cd..58fa67d4 100644 --- a/config/internal-dns/README.md +++ b/config/internal-dns/README.md @@ -226,3 +226,21 @@ reports `CapacityAvailable=False` for the affected binding while healthy contexts and withdrawals continue. A pending zone fails only queries beneath its private apex; its names never fall through to public recursion. Bindings, publication targets, and context status use the same stable shard assignment. + +### Control-plane monitoring + +Set `metricsBindAddress` to an internal listener, such as `:8080`, to expose +`/metrics` on the control-plane process. The endpoint is disabled when the +setting is omitted or set to `0`. Restrict access to the monitoring network. + +The endpoint includes controller-runtime reconciliation metrics and: + +- `internal_dns_operations_total{operation,result}`: completed operations, + with `result` set to `success` or `error`. +- `internal_dns_operation_last_success_timestamp_seconds{operation}`: the + latest successful completion time. Errors do not advance this timestamp. + +Operations identify configured discovery, planner, acknowledgement refresh, +transport outbox, and acknowledgement processing loops. A successful loop +can be idle; it does not prove that an endpoint is serving. Combine these +metrics with binding conditions, broker backlog, and DNS queries from a VPC. diff --git a/internal/internaldns/runtime/config.go b/internal/internaldns/runtime/config.go index d4ea47b3..e429e592 100644 --- a/internal/internaldns/runtime/config.go +++ b/internal/internaldns/runtime/config.go @@ -62,6 +62,7 @@ type RegionalTransportConfig struct { } type Config struct { + MetricsBindAddress string `json:"metricsBindAddress,omitempty"` Region string `json:"region"` Shard string `json:"shard"` PlatformNamespace string `json:"platformNamespace"` diff --git a/internal/internaldns/runtime/metrics.go b/internal/internaldns/runtime/metrics.go new file mode 100644 index 00000000..d1c6e079 --- /dev/null +++ b/internal/internaldns/runtime/metrics.go @@ -0,0 +1,33 @@ +package runtime + +import ( + "time" + + "github.com/prometheus/client_golang/prometheus" + "sigs.k8s.io/controller-runtime/pkg/metrics" +) + +var operationResults = prometheus.NewCounterVec(prometheus.CounterOpts{ + Name: "internal_dns_operations_total", + Help: "Completed internal DNS operations by configured loop and result.", +}, []string{"operation", "result"}) + +var operationLastSuccess = prometheus.NewGaugeVec(prometheus.GaugeOpts{ + Name: "internal_dns_operation_last_success_timestamp_seconds", + Help: "Unix timestamp of the latest successful internal DNS operation.", +}, []string{"operation"}) + +func init() { + metrics.Registry.MustRegister(operationResults, operationLastSuccess) +} + +func observeOperation(operation string, err error) { + lastSuccess := operationLastSuccess.WithLabelValues(operation) + result := "success" + if err != nil { + result = "error" + } else { + lastSuccess.Set(float64(time.Now().Unix())) + } + operationResults.WithLabelValues(operation, result).Inc() +} diff --git a/internal/internaldns/runtime/metrics_test.go b/internal/internaldns/runtime/metrics_test.go new file mode 100644 index 00000000..08da5071 --- /dev/null +++ b/internal/internaldns/runtime/metrics_test.go @@ -0,0 +1,54 @@ +package runtime + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/prometheus/client_golang/prometheus/testutil" +) + +func TestRetryLoopReportsFailureAndRecovery(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + operation := t.Name() + defer operationResults.DeleteLabelValues(operation, "error") + defer operationResults.DeleteLabelValues(operation, "success") + defer operationLastSuccess.DeleteLabelValues(operation) + done := make(chan error, 1) + calls := 0 + go retryLoop(ctx, time.Millisecond, operation, func(context.Context) error { + calls++ + if calls == 1 { + return errors.New("source unavailable") + } + cancel() + return nil + }, done) + select { + case <-done: + case <-time.After(time.Second): + t.Fatal("loop did not stop") + } + if got := testutil.ToFloat64(operationResults.WithLabelValues(operation, "error")); got != 1 { + t.Fatalf("error count = %v", got) + } + if got := testutil.ToFloat64(operationResults.WithLabelValues(operation, "success")); got != 1 { + t.Fatalf("success count = %v", got) + } + if got := testutil.ToFloat64(operationLastSuccess.WithLabelValues(operation)); got == 0 { + t.Fatal("successful recovery did not update freshness") + } +} + +func TestFailedOperationDoesNotRenewSuccess(t *testing.T) { + operation := t.Name() + defer operationResults.DeleteLabelValues(operation, "error") + defer operationLastSuccess.DeleteLabelValues(operation) + operationLastSuccess.WithLabelValues(operation).Set(123) + observeOperation(operation, errors.New("failed")) + if got := testutil.ToFloat64(operationLastSuccess.WithLabelValues(operation)); got != 123 { + t.Fatalf("failure renewed successful operation timestamp: %v", got) + } +} diff --git a/internal/internaldns/runtime/run.go b/internal/internaldns/runtime/run.go index 06d6389d..2fc3657f 100644 --- a/internal/internaldns/runtime/run.go +++ b/internal/internaldns/runtime/run.go @@ -186,6 +186,13 @@ func runControlPlane(ctx context.Context, cfg Config, kubeconfig string) error { } errCh := make(chan error, len(projects)+len(planners)*3+8) + if cfg.MetricsBindAddress != "" && cfg.MetricsBindAddress != "0" { + server, err := metricsserver.NewServer(metricsserver.Options{BindAddress: cfg.MetricsBindAddress}, platformREST, nil) + if err != nil { + return fmt.Errorf("create metrics server: %w", err) + } + go func() { errCh <- server.Start(ctx) }() + } startProjectSources(ctx, projects) if discovery != nil { go retryLoop(ctx, time.Duration(cfg.ProjectDiscovery.IntervalSeconds)*time.Second, "project discovery", discovery.step, errCh) @@ -473,7 +480,11 @@ func retryLoop(ctx context.Context, interval time.Duration, name string, step fu ticker := time.NewTicker(interval) defer ticker.Stop() for { - if err := step(ctx); err != nil && !errors.Is(err, context.Canceled) { + err := step(ctx) + if !errors.Is(err, context.Canceled) { + observeOperation(name, err) + } + if err != nil && !errors.Is(err, context.Canceled) { log.Error(err, "operation failed; retrying") } select { @@ -494,16 +505,22 @@ func consumeAcks(ctx context.Context, source *transport.Consumer, sink *platform terminal <- nil return } + observeOperation("ack read "+sink.Region+"/"+sink.Shard, err) log.Error(err, "read acknowledgement failed; retrying") time.Sleep(time.Second) continue } - if err := sink.Apply(ctx, message.Subject(), message.Envelope()); err != nil { + observeOperation("ack read "+sink.Region+"/"+sink.Shard, nil) + err = sink.Apply(ctx, message.Subject(), message.Envelope()) + observeOperation("ack apply "+sink.Region+"/"+sink.Shard, err) + if err != nil { log.Error(err, "apply acknowledgement failed; retrying") _ = message.Retry(time.Second) continue } - if err := message.Ack(ctx); err != nil { + err = message.Ack(ctx) + observeOperation("ack commit "+sink.Region+"/"+sink.Shard, err) + if err != nil { log.Error(err, "acknowledgement commit was ambiguous; awaiting redelivery") } } From 332b243c4a75744a2179e4a6e841ad026ab04639 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Wed, 21 Oct 2026 19:58:02 -0500 Subject: [PATCH 12/19] fix: preserve DNS reference recovery boundaries --- config/internal-dns/README.md | 8 +++ .../controlplane/reference_state.go | 44 +++++++++++++- .../controlplane/reference_state_test.go | 59 +++++++++++++++++++ 3 files changed, 109 insertions(+), 2 deletions(-) diff --git a/config/internal-dns/README.md b/config/internal-dns/README.md index 58fa67d4..a882916e 100644 --- a/config/internal-dns/README.md +++ b/config/internal-dns/README.md @@ -244,3 +244,11 @@ Operations identify configured discovery, planner, acknowledgement refresh, transport outbox, and acknowledgement processing loops. A successful loop can be idle; it does not prove that an endpoint is serving. Combine these metrics with binding conditions, broker backlog, and DNS queries from a VPC. + +### Reference-state recovery + +An observed association, naming policy, or registration requires its private +reference state to retain the target object's identity. If that state is +missing, DNS rejects the source instead of attaching it to a same-name target. +The rejection persists across retries. Restore a verified reference-state +backup, or recreate the source resource to select its targets explicitly. diff --git a/internal/internaldns/controlplane/reference_state.go b/internal/internaldns/controlplane/reference_state.go index 56a44ba0..d4358733 100644 --- a/internal/internaldns/controlplane/reference_state.go +++ b/internal/internaldns/controlplane/reference_state.go @@ -14,6 +14,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -23,6 +24,7 @@ const referenceStateDataKey = "references.json" const defaultPlatformNamespace = "internal-dns-system" type referenceState struct { + Unverified bool `json:"unverified,omitempty"` SourceNamespace string `json:"sourceNamespace"` SourceKind string `json:"sourceKind"` Source dnsv1.DNSObjectReference `json:"source"` @@ -63,6 +65,9 @@ func (r *Reconciler) loadReferenceState(ctx context.Context, source client.Objec object := r.referenceStateObject(source) state := referenceState{SourceKind: referenceKind(source), SourceNamespace: source.GetNamespace(), Source: dnsv1.DNSObjectReference{Name: source.GetName(), UID: source.GetUID()}, References: map[string]dnsv1.DNSObjectReference{}} if err := r.Get(ctx, client.ObjectKeyFromObject(object), object); err != nil { + if apierrors.IsNotFound(err) { + state.Unverified = hasResolvedReferenceStatus(source) + } return object, state, client.IgnoreNotFound(err) } if err := json.Unmarshal([]byte(object.Data[referenceStateDataKey]), &state); err != nil { @@ -79,6 +84,19 @@ func (r *Reconciler) pinReferences(ctx context.Context, source client.Object, se if err != nil { return nil, false, err } + if state.Unverified { + if object.ResourceVersion == "" { + payload, err := json.Marshal(state) + if err != nil { + return nil, false, err + } + object.Data = map[string]string{referenceStateDataKey: string(payload)} + if err := r.Create(ctx, object); err != nil { + return nil, false, err + } + } + return state.References, false, nil + } current := true resolved := map[string]dnsv1.DNSObjectReference{} for key, ref := range selected { @@ -122,7 +140,8 @@ func (r *Reconciler) collectReferenceStates(ctx context.Context, sources []clien if !live[states.Items[i].Name] { var state referenceState if err := json.Unmarshal([]byte(states.Items[i].Data[referenceStateDataKey]), &state); err != nil { - return err + ctrl.LoggerFrom(ctx).Error(err, "skip malformed DNS reference state", "name", states.Items[i].Name) + continue } var source client.Object switch state.SourceKind { @@ -133,7 +152,15 @@ func (r *Reconciler) collectReferenceStates(ctx context.Context, sources []clien case "DNSRegistration": source = &dnsv1.DNSRegistration{} default: - return fmt.Errorf("unknown DNS reference source kind %q", state.SourceKind) + ctrl.LoggerFrom(ctx).Error(fmt.Errorf("unknown source kind %q", state.SourceKind), "skip DNS reference state", "name", states.Items[i].Name) + continue + } + source.SetNamespace(state.SourceNamespace) + source.SetName(state.Source.Name) + source.SetUID(state.Source.UID) + if r.referenceStateObject(source).Name != states.Items[i].Name { + ctrl.LoggerFrom(ctx).Error(fmt.Errorf("source identity mismatch"), "skip DNS reference state", "name", states.Items[i].Name) + continue } err := r.Get(ctx, client.ObjectKey{Namespace: state.SourceNamespace, Name: state.Source.Name}, source) if err != nil && !apierrors.IsNotFound(err) { @@ -166,3 +193,16 @@ func (r *Reconciler) associationReferences(ctx context.Context, associations []d } return result, nil } + +func hasResolvedReferenceStatus(source client.Object) bool { + switch object := source.(type) { + case *dnsv1.DNSZoneAssociation: + return object.Status.ResolvedDNSZoneRef.Name != "" || object.Status.ResolvedResolverContextRef.Name != "" || len(object.Status.Conditions) != 0 + case *dnsv1.DNSNamingPolicy: + return object.Status.ResolvedResolverContextRef.Name != "" || len(object.Status.Conditions) != 0 + case *dnsv1.DNSRegistration: + return object.Status.CanonicalFQDN != "" || object.Status.ObservedGeneration != 0 || len(object.Status.Conditions) != 0 + default: + return false + } +} diff --git a/internal/internaldns/controlplane/reference_state_test.go b/internal/internaldns/controlplane/reference_state_test.go index 0709106d..188a7c00 100644 --- a/internal/internaldns/controlplane/reference_state_test.go +++ b/internal/internaldns/controlplane/reference_state_test.go @@ -23,6 +23,15 @@ func addReferenceStateToScheme(scheme *runtime.Scheme) error { func seedReferenceState(t *testing.T, r *Reconciler, source client.Object, selected map[string]dnsv1alpha1.DNSObjectReference) { t.Helper() + source = source.DeepCopyObject().(client.Object) + switch object := source.(type) { + case *dnsv1alpha1.DNSZoneAssociation: + object.Status = dnsv1alpha1.DNSZoneAssociationStatus{} + case *dnsv1alpha1.DNSNamingPolicy: + object.Status = dnsv1alpha1.DNSNamingPolicyStatus{} + case *dnsv1alpha1.DNSRegistration: + object.Status = dnsv1alpha1.DNSRegistrationStatus{} + } if _, valid, err := r.pinReferences(context.Background(), source, selected); err != nil || !valid { t.Fatalf("reference state: valid=%t err=%v", valid, err) } @@ -80,3 +89,53 @@ func TestReferenceStateIsPrivateScopedAndCollected(t *testing.T) { t.Fatalf("deleted source retained private state: count=%d err=%v", len(states.Items), err) } } + +func TestMissingReferenceStateDoesNotReattachObservedResource(t *testing.T) { + scheme := runtime.NewScheme() + if err := addReferenceStateToScheme(scheme); err != nil { + t.Fatal(err) + } + ctx := context.Background() + cl := fake.NewClientBuilder().WithScheme(scheme).Build() + r := &Reconciler{Client: cl, Options: Options{ProjectUID: "project", PlatformNamespace: "platform"}} + source := &dnsv1alpha1.DNSRegistration{ObjectMeta: metav1.ObjectMeta{Name: "web", Namespace: "project", UID: "registration"}, Status: dnsv1alpha1.DNSRegistrationStatus{CanonicalFQDN: "web.datum.internal."}} + selected := map[string]dnsv1alpha1.DNSObjectReference{"zone": {Name: "private", UID: "replacement-zone"}} + for range 2 { + pins, valid, err := r.pinReferences(ctx, source, selected) + if err != nil || valid || pins["zone"].UID != "" { + t.Fatalf("unverified reference reattached: pins=%v valid=%t err=%v", pins, valid, err) + } + source.Status = dnsv1alpha1.DNSRegistrationStatus{} + } + source.UID = "recreated-registration" + if _, valid, err := r.pinReferences(ctx, source, selected); err != nil || !valid { + t.Fatalf("new registration could not establish references: valid=%t err=%v", valid, err) + } +} + +func TestReferenceCollectionSkipsMalformedOrForeignObjects(t *testing.T) { + scheme := runtime.NewScheme() + if err := addReferenceStateToScheme(scheme); err != nil { + t.Fatal(err) + } + ctx := context.Background() + cl := fake.NewClientBuilder().WithScheme(scheme).Build() + r := &Reconciler{Client: cl, Options: Options{ProjectUID: "project", PlatformNamespace: "platform"}} + source := &dnsv1alpha1.DNSRegistration{ObjectMeta: metav1.ObjectMeta{Name: "web", Namespace: "project", UID: "registration"}} + seedReferenceState(t, r, source, map[string]dnsv1alpha1.DNSObjectReference{"zone": {Name: "private", UID: "zone"}}) + for name, data := range map[string]string{"broken": "not-json", "foreign": `{"sourceKind":"DNSRegistration","sourceNamespace":"project","source":{"name":"web","uid":"foreign"}}`, "unknown": `{"sourceKind":"Other"}`} { + object := r.referenceStateObject(source) + object.Name = name + object.Data = map[string]string{referenceStateDataKey: data} + if err := cl.Create(ctx, object); err != nil { + t.Fatal(err) + } + } + if err := r.collectReferenceStates(ctx, nil); err != nil { + t.Fatalf("unrelated reference state blocked collection: %v", err) + } + var states corev1.ConfigMapList + if err := cl.List(ctx, &states, client.InNamespace("platform")); err != nil || len(states.Items) != 3 { + t.Fatalf("valid retired state not collected, or foreign state deleted: count=%d err=%v", len(states.Items), err) + } +} From f0e0600cd00b6bed4234ed1db067d3fa80ae5ff1 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Wed, 21 Oct 2026 20:00:04 -0500 Subject: [PATCH 13/19] test: verify concurrent DNS health projection --- config/internal-dns/README.md | 6 ++ internal/internaldns/platform/acks.go | 10 +-- .../internaldns/platform/platform_test.go | 79 +++++++++++++++++++ 3 files changed, 90 insertions(+), 5 deletions(-) diff --git a/config/internal-dns/README.md b/config/internal-dns/README.md index a882916e..a5405c00 100644 --- a/config/internal-dns/README.md +++ b/config/internal-dns/README.md @@ -252,3 +252,9 @@ reference state to retain the target object's identity. If that state is missing, DNS rejects the source instead of attaching it to a same-name target. The rejection persists across retries. Restore a verified reference-state backup, or recreate the source resource to select its targets explicitly. + +Registration write permission is the publication boundary within a project. +Naming-policy selectors organize names; they do not authorize publishers. +A publisher that can change registration labels can select every matching +rule for the registration's associated context. Restrict registration writes +with RBAC when publishers must not share that authority. diff --git a/internal/internaldns/platform/acks.go b/internal/internaldns/platform/acks.go index 37f87e77..d855efd4 100644 --- a/internal/internaldns/platform/acks.go +++ b/internal/internaldns/platform/acks.go @@ -35,7 +35,6 @@ type ProjectScope struct { type AckSink struct { Client client.Client Namespace string - ProjectNamespaces []string Projects []ProjectScope ProjectScopes func() []ProjectScope PublicationMembers []model.Member @@ -187,12 +186,14 @@ func (s *AckSink) projectPublication(ctx context.Context, m *dnsv1alpha1.DNSPubl if s.ProjectScopes != nil { projects = append(projects, s.ProjectScopes()...) } - for _, ns := range s.ProjectNamespaces { - projects = append(projects, ProjectScope{Client: s.Client, Namespace: ns}) - } results := make(chan error, len(projects)) started := 0 + var projectErrors error for _, project := range projects { + if m.Labels["internal-dns.miloapis.com/project-uid"] != "" && project.ProjectUID == "" { + projectErrors = errors.Join(projectErrors, fmt.Errorf("publication projection scope %q requires a project identity", project.Namespace)) + continue + } if owner := m.Labels["internal-dns.miloapis.com/project-uid"]; owner != "" && project.ProjectUID != owner { continue } @@ -208,7 +209,6 @@ func (s *AckSink) projectPublication(ctx context.Context, m *dnsv1alpha1.DNSPubl results <- s.projectPublicationForScope(projectCtx, project, m, complete, now) }() } - var projectErrors error for i := 0; i < started; i++ { projectErrors = errors.Join(projectErrors, <-results) } diff --git a/internal/internaldns/platform/platform_test.go b/internal/internaldns/platform/platform_test.go index a991a761..c2da035a 100644 --- a/internal/internaldns/platform/platform_test.go +++ b/internal/internaldns/platform/platform_test.go @@ -14,6 +14,7 @@ import ( "go.miloapis.com/dns-operator/internal/internaldns/model" "go.miloapis.com/dns-operator/internal/internaldns/transport" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -575,3 +576,81 @@ func TestPublicationProjectionOnlyReadsOwningProject(t *testing.T) { default: } } + +type heldManifestRead struct { + client.Client + read chan struct{} + release chan struct{} +} + +func (c *heldManifestRead) Get(ctx context.Context, key client.ObjectKey, object client.Object, opts ...client.GetOption) error { + if err := c.Client.Get(ctx, key, object, opts...); err != nil { + return err + } + if _, ok := object.(*dnsv1alpha1.DNSPublicationManifest); ok { + close(c.read) + select { + case <-c.release: + case <-ctx.Done(): + return ctx.Err() + } + } + return nil +} + +func TestConcurrentAckProjectionPreservesNewerObservation(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + now := time.Now().UTC() + manifest := &dnsv1alpha1.DNSPublicationManifest{ObjectMeta: metav1.ObjectMeta{Name: "manifest", Namespace: "platform"}, Spec: dnsv1alpha1.DNSPublicationManifestSpec{ZoneRef: dnsv1alpha1.DNSObjectReference{UID: "zone"}, WriterEpoch: 1, Revision: 2}} + owner := &dnsv1alpha1.DNSPublicationOwnership{ObjectMeta: metav1.ObjectMeta{Name: "owner-" + model.OpaqueToken("zone")[:20], Namespace: "platform"}, Spec: dnsv1alpha1.DNSPublicationOwnershipSpec{ZoneUID: "zone", WriterEpoch: 1, ActiveManifestName: manifest.Name}} + cl := testClient(t, owner, manifest) + held := &heldManifestRead{Client: cl, read: make(chan struct{}), release: make(chan struct{})} + sink := AckSink{Client: cl, Namespace: "platform", Region: "east", Shard: "shared", Members: []model.Member{{MemberID: "auth", ReplicaID: "replica", Role: "regional"}}, Now: func() time.Time { return now }} + olderSink := sink + olderSink.Client = held + ack := model.MemberAck{MemberID: "auth", ReplicaID: "replica", ResourceUID: "zone", Kind: model.KindPublicationManifest, Epoch: 1, Revision: 2, Phase: model.AckVerified, ObservedAt: now.Add(-time.Second), ValidUntil: now.Add(time.Minute)} + older, err := model.NewEnvelope(model.KindMemberAck, "older", "east", "shared", "zone", 1, 2, now, ack) + if err != nil { + t.Fatal(err) + } + ack.Phase = model.AckRejected + ack.ObservedAt = now + newer, err := model.NewEnvelope(model.KindMemberAck, "newer", "east", "shared", "zone", 1, 2, now, ack) + if err != nil { + t.Fatal(err) + } + subject := model.AckSubject("east", "shared", "auth") + done := make(chan error, 1) + go func() { done <- olderSink.Apply(ctx, subject, older) }() + select { + case <-held.read: + case <-ctx.Done(): + t.Fatal("older acknowledgement did not reach manifest") + } + if err := sink.Apply(ctx, subject, newer); err != nil { + close(held.release) + t.Fatal(err) + } + close(held.release) + if err := <-done; !apierrors.IsConflict(err) { + t.Fatalf("racing older projection did not reject stale write: %v", err) + } + if err := sink.Apply(ctx, subject, older); err != nil { + t.Fatalf("older acknowledgement redelivery failed: %v", err) + } + if err := cl.Get(ctx, client.ObjectKeyFromObject(manifest), manifest); err != nil { + t.Fatal(err) + } + if len(manifest.Status.ReplicaAcknowledgements) != 1 || manifest.Status.ReplicaAcknowledgements[0].Phase != string(model.AckRejected) || manifest.Status.ReplicaAcknowledgements[0].ObservedUnixNano != now.UnixNano() { + t.Fatalf("late verified acknowledgement overwrote newer rejection: %+v", manifest.Status.ReplicaAcknowledgements) + } +} + +func TestPublicationProjectionReportsMissingProjectIdentity(t *testing.T) { + manifest := &dnsv1alpha1.DNSPublicationManifest{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"internal-dns.miloapis.com/project-uid": "project-owner"}}} + sink := AckSink{Projects: []ProjectScope{{Namespace: "default"}}} + if err := sink.projectPublication(context.Background(), manifest, true, time.Now()); err == nil { + t.Fatal("missing projection identity was silently skipped") + } +} From d84be0277f66d54c4a23237b6364c07affb0a03d Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Wed, 21 Oct 2026 20:00:51 -0500 Subject: [PATCH 14/19] style: group DNS transport test imports --- internal/internaldns/transport/jetstream_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/internaldns/transport/jetstream_test.go b/internal/internaldns/transport/jetstream_test.go index 9aa35b6e..724914ec 100644 --- a/internal/internaldns/transport/jetstream_test.go +++ b/internal/internaldns/transport/jetstream_test.go @@ -3,9 +3,10 @@ package transport import ( "context" "errors" + "testing" + "github.com/nats-io/nats.go" "github.com/nats-io/nats.go/jetstream" - "testing" ) func TestProductionTransportRequiresTLSAndOneAuthMethod(t *testing.T) { From 03752e55c5eaeeba19b40439474a803e59c23a63 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Wed, 21 Oct 2026 20:01:39 -0500 Subject: [PATCH 15/19] build: include metrics test dependency --- go.mod | 1 + 1 file changed, 1 insertion(+) diff --git a/go.mod b/go.mod index e6bc0765..6d9958d1 100644 --- a/go.mod +++ b/go.mod @@ -66,6 +66,7 @@ require ( github.com/hashicorp/golang-lru/v2 v2.0.7 // indirect github.com/jpillora/backoff v1.0.0 // indirect github.com/klauspost/compress v1.20.0 // indirect + github.com/kylelemons/godebug v1.1.0 // indirect github.com/lufia/plan9stats v0.0.0-20260216142805-b3301c5f2a88 // indirect github.com/magiconair/properties v1.8.10 // indirect github.com/mdlayher/socket v0.6.0 // indirect From 037fff635820aa32f210f434fb0df509d5210fc8 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Wed, 21 Oct 2026 20:26:52 -0500 Subject: [PATCH 16/19] test: use pinned references in compiler regression fixtures --- internal/internaldns/controlplane/compiler_test.go | 3 +++ .../internaldns/controlplane/publication_reconciler_test.go | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/internal/internaldns/controlplane/compiler_test.go b/internal/internaldns/controlplane/compiler_test.go index 52b650e1..fd119610 100644 --- a/internal/internaldns/controlplane/compiler_test.go +++ b/internal/internaldns/controlplane/compiler_test.go @@ -347,6 +347,8 @@ func TestCompileRejectedRegistrationDoesNotClaimOtherTypes(t *testing.T) { valid.CreationTimestamp = metav1.NewTime(time.Unix(3, 0)) valid.Spec.RecordTypes = []dnsv1alpha1.RRType{dnsv1alpha1.RRTypeA} in.Registrations = []dnsv1alpha1.DNSRegistration{rejected, valid, original} + in.RegistrationZoneReferences[rejected.UID] = in.RegistrationZoneReferences[original.UID] + in.RegistrationZoneReferences[valid.UID] = in.RegistrationZoneReferences[original.UID] got, err := Compile(in) if err != nil { t.Fatal(err) @@ -369,6 +371,7 @@ func TestCompileInvalidSRVDoesNotBlockWithdrawal(t *testing.T) { reg.Name, reg.UID, reg.Spec.Name = "srv", "srv", "_grpc._tcp.svc" reg.Spec.RecordTypes = []dnsv1alpha1.RRType{dnsv1alpha1.RRTypeSRV} in.Registrations = append(in.Registrations, reg) + in.RegistrationZoneReferences[reg.UID] = in.RegistrationZoneReferences[in.Registrations[0].UID] c := *in.Contributions[0].DeepCopy() c.Name, c.UID, c.Spec.Eligible = "srv", "srv-contribution", true c.Spec.RegistrationRef = dnsv1alpha1.DNSObjectReference{Name: reg.Name, UID: reg.UID, Generation: reg.Generation} diff --git a/internal/internaldns/controlplane/publication_reconciler_test.go b/internal/internaldns/controlplane/publication_reconciler_test.go index 1a0b97e0..d5d4e2a9 100644 --- a/internal/internaldns/controlplane/publication_reconciler_test.go +++ b/internal/internaldns/controlplane/publication_reconciler_test.go @@ -365,7 +365,7 @@ func TestReconcilerPersistsFencedMultiRegionOutbox(t *testing.T) { func TestProjectContinuesAfterZonePublicationFailure(t *testing.T) { ctx := context.Background() scheme := runtime.NewScheme() - if err := dnsv1alpha1.AddToScheme(scheme); err != nil { + if err := addReferenceStateToScheme(scheme); err != nil { t.Fatal(err) } bad := fixture(time.Now()).Zone.DeepCopy() From 63d53b78c5090712374a98545419b9e1f85209fa Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Wed, 21 Oct 2026 20:56:43 -0500 Subject: [PATCH 17/19] fix: refresh DNS backend transport before startup --- internal/internaldns/serving/dnsdist.go | 16 +++++++++++++--- internal/internaldns/serving/dnsdist_test.go | 19 ++++++++++++++++--- internal/internaldns/serving/render.go | 14 +++++++++----- 3 files changed, 38 insertions(+), 11 deletions(-) diff --git a/internal/internaldns/serving/dnsdist.go b/internal/internaldns/serving/dnsdist.go index 071ff5f9..ea8cd458 100644 --- a/internal/internaldns/serving/dnsdist.go +++ b/internal/internaldns/serving/dnsdist.go @@ -3,6 +3,7 @@ package serving import ( + "bytes" "crypto/rand" "encoding/base64" "errors" @@ -55,13 +56,22 @@ func prepareDNSDistRouter(router DNSDistRenderConfig) error { if err := os.MkdirAll(filepath.Dir(router.Path), 0750); err != nil { return err } - if _, err := os.Stat(router.Path); err == nil { - return nil + startup := renderDNSDist(router, router.Backends, nil, nil, false, nil, nil) + retained, err := os.ReadFile(router.Path) + if err == nil { + // Refresh immutable process settings while retaining accepted tenant rules. + boundary := []byte(dnsdistRulesBoundary) + _, rules, ok := bytes.Cut(retained, boundary) + if !ok || !bytes.HasSuffix(rules, []byte("setRules(rules)\nend\n")) { + return errors.New("retained dnsdist configuration has no complete rules section") + } + header, _, _ := bytes.Cut(startup, boundary) + startup = append(append(header, boundary...), rules...) } else if !errors.Is(err, os.ErrNotExist) { return err } temporary := router.Path + ".bootstrap" - if err := writeDurable(temporary, renderDNSDist(router, router.Backends, nil, nil, false, nil, nil), 0644); err != nil { + if err := writeDurable(temporary, startup, 0644); err != nil { return err } if err := os.Rename(temporary, router.Path); err != nil { diff --git a/internal/internaldns/serving/dnsdist_test.go b/internal/internaldns/serving/dnsdist_test.go index 485311ba..9963eb51 100644 --- a/internal/internaldns/serving/dnsdist_test.go +++ b/internal/internaldns/serving/dnsdist_test.go @@ -65,14 +65,27 @@ func TestDNSDistBootstrapPublishesOnlyCompleteConfiguration(t *testing.T) { if err != nil || !strings.Contains(string(data), "DNSAction.Refused") || !strings.Contains(string(data), "setRules(rules)") { t.Fatal("bootstrap must refuse unknown contexts", err) } - if err := os.WriteFile(router.Path, []byte("accepted snapshot"), 0644); err != nil { + accepted := strings.Replace(string(data), "local bindings = {", "local bindings = {\n [\"fd53::a\"] = {context=\"tenant-a\", ready=true},", 1) + accepted = strings.ReplaceAll(accepted, "tcpOnly=true,checkTCP=true,", "") + if err := os.WriteFile(router.Path, []byte(accepted), 0644); err != nil { t.Fatal(err) } if err := prepareDNSDistRouter(router); err != nil { t.Fatal(err) } retained, err := os.ReadFile(router.Path) - if err != nil || string(retained) != "accepted snapshot" { - t.Fatal("bootstrap replaced accepted routing", err) + if err != nil { + t.Fatal(err) + } + _, acceptedRules, _ := strings.Cut(accepted, dnsdistRulesBoundary) + _, retainedRules, _ := strings.Cut(string(retained), dnsdistRulesBoundary) + if retainedRules != acceptedRules || !strings.Contains(string(retained), "tcpOnly=true,checkTCP=true") { + t.Fatal("startup must refresh transport and preserve accepted tenant rules") + } + if err := os.WriteFile(router.Path, []byte("incomplete configuration"), 0644); err != nil { + t.Fatal(err) + } + if err := prepareDNSDistRouter(router); err == nil { + t.Fatal("incomplete retained rules must block startup") } } diff --git a/internal/internaldns/serving/render.go b/internal/internaldns/serving/render.go index 6756d9a2..491e3bc2 100644 --- a/internal/internaldns/serving/render.go +++ b/internal/internaldns/serving/render.go @@ -199,16 +199,20 @@ func collectBackends(bindings []model.Binding, get func(model.Binding) []model.B return out } +const dnsdistRulesBoundary = "local rules = {}\n" + func renderDNSDist(c DNSDistRenderConfig, backends []model.Backend, bindings []model.Binding, active map[string]bool, cluster bool, eligible map[string]map[string]bool, zones map[string]map[string]map[string]bool) []byte { var out bytes.Buffer endpoint := dnsdistConsole(c.Path) fmt.Fprint(&out, "local client = inClientStartup()\nif client or not internalDNSInitialized then\n") fmt.Fprintf(&out, " local key\n if os.getenv(\"INTERNAL_DNS_VALIDATE\") == \"1\" then key = string.rep(\"A\",43)..\"=\" else\n local handle = assert(io.open(%q,\"r\"))\n key = handle:read(\"*l\"); handle:close()\n end\n setKey(key)\n controlSocket(%q)\n setConsoleACL({\"127.0.0.1/32\"})\nend\nif not client then\n", DNSDistKeyPath, endpoint) topology, _ := json.Marshal(struct { - Listen string - ACLs []string - Backends []model.Backend - }{c.ListenAddress, c.ACLs, backends}) + Listen string + ACLs []string + Backends []model.Backend + Pool string + Transport string + }{c.ListenAddress, c.ACLs, backends, c.PoolName, "proxy-v2-tcp"}) fingerprint := model.OpaqueToken(string(topology)) fmt.Fprint(&out, "if not internalDNSInitialized then\n") fmt.Fprintf(&out, "setLocal(%q)\nsetACL({", c.ListenAddress) @@ -223,7 +227,7 @@ func renderDNSDist(c DNSDistRenderConfig, backends []model.Backend, bindings []m fmt.Fprintf(&out, "internalDNSServers[%q] = newServer({address=%q,name=%q,pool=%q,useProxyProtocol=true,tcpOnly=true,checkTCP=true,checkName=\".\",checkType=\"SOA\",mustResolve=false})\n", backend.MemberID, net.JoinHostPort(backend.Address, strconv.Itoa(int(backend.Port))), "b_"+model.OpaqueToken(backend.MemberID), c.PoolName) } fmt.Fprintf(&out, "internalDNSTopology = %q\ninternalDNSInitialized = true\nend\nassert(internalDNSTopology == %q,\"dnsdist topology requires a replacement member\")\n", fingerprint, fingerprint) - fmt.Fprint(&out, "local rules = {}\nlocal function appendRule(rule,action) rules[#rules+1] = newRuleAction(rule,action) end\n") + fmt.Fprint(&out, dnsdistRulesBoundary+"local function appendRule(rule,action) rules[#rules+1] = newRuleAction(rule,action) end\n") if cluster { pools := map[string]map[string]bool{} for _, binding := range bindings { From 186e3270cded6bb25e4e2398e96031a8a1add680 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Wed, 21 Oct 2026 21:11:05 -0500 Subject: [PATCH 18/19] fix: preserve accepted routing during DNS process upgrades --- internal/internaldns/serving/continuity.go | 29 +++++++++++++ internal/internaldns/serving/dnsdist.go | 22 ++++++---- internal/internaldns/serving/dnsdist_test.go | 43 ++++++++++++++++++++ 3 files changed, 87 insertions(+), 7 deletions(-) diff --git a/internal/internaldns/serving/continuity.go b/internal/internaldns/serving/continuity.go index 26035e80..49c9d611 100644 --- a/internal/internaldns/serving/continuity.go +++ b/internal/internaldns/serving/continuity.go @@ -106,6 +106,9 @@ func (a *Agent) restoreActive(ctx context.Context) error { if !a.canRetainActive() { return a.failClosed(ctx) } + if err := a.refreshActiveDNSDist(); err != nil { + return err + } files := a.state.Active.Files if !activationPending(a.cfg.StateDir) && installedFilesMatch(files) { if err := a.probeLocalDaemons(ctx); err == nil { @@ -129,3 +132,29 @@ func (a *Agent) restoreActive(ctx context.Context) error { } return a.tx.commitActivation() } + +func (a *Agent) refreshActiveDNSDist() error { + role := a.cfg.Render.Role + var routers []DNSDistRenderConfig + if role == "" || role == RenderBoth || role == RenderNode { + routers = append(routers, a.cfg.Render.NodeDNSDist) + } + if role == "" || role == RenderBoth || role == RenderCluster || role == RenderRegionalDNSDist { + routers = append(routers, a.cfg.Render.ClusterDNSDist) + } + for _, router := range routers { + if len(router.Backends) == 0 { + continue + } + retained, ok := a.state.Active.Files[router.Path] + if !ok { + continue + } + current, err := refreshDNSDistConfiguration(router, retained) + if err != nil { + return err + } + a.state.Active.Files[router.Path] = current + } + return nil +} diff --git a/internal/internaldns/serving/dnsdist.go b/internal/internaldns/serving/dnsdist.go index ea8cd458..e7fc5711 100644 --- a/internal/internaldns/serving/dnsdist.go +++ b/internal/internaldns/serving/dnsdist.go @@ -59,14 +59,10 @@ func prepareDNSDistRouter(router DNSDistRenderConfig) error { startup := renderDNSDist(router, router.Backends, nil, nil, false, nil, nil) retained, err := os.ReadFile(router.Path) if err == nil { - // Refresh immutable process settings while retaining accepted tenant rules. - boundary := []byte(dnsdistRulesBoundary) - _, rules, ok := bytes.Cut(retained, boundary) - if !ok || !bytes.HasSuffix(rules, []byte("setRules(rules)\nend\n")) { - return errors.New("retained dnsdist configuration has no complete rules section") + startup, err = refreshDNSDistConfiguration(router, retained) + if err != nil { + return err } - header, _, _ := bytes.Cut(startup, boundary) - startup = append(append(header, boundary...), rules...) } else if !errors.Is(err, os.ErrNotExist) { return err } @@ -83,6 +79,18 @@ func prepareDNSDistRouter(router DNSDistRenderConfig) error { return nil } +// Refresh immutable process settings while retaining accepted tenant rules. +func refreshDNSDistConfiguration(router DNSDistRenderConfig, retained []byte) ([]byte, error) { + boundary := []byte(dnsdistRulesBoundary) + _, rules, ok := bytes.Cut(retained, boundary) + if !ok || !bytes.HasSuffix(rules, []byte("setRules(rules)\nend\n")) { + return nil, errors.New("retained dnsdist configuration has no complete rules section") + } + startup := renderDNSDist(router, router.Backends, nil, nil, false, nil, nil) + header, _, _ := bytes.Cut(startup, boundary) + return append(append(header, boundary...), rules...), nil +} + func prepareDNSDistKey(path string) error { if err := os.MkdirAll(filepath.Dir(path), 0750); err != nil { return err diff --git a/internal/internaldns/serving/dnsdist_test.go b/internal/internaldns/serving/dnsdist_test.go index 9963eb51..01d41ad9 100644 --- a/internal/internaldns/serving/dnsdist_test.go +++ b/internal/internaldns/serving/dnsdist_test.go @@ -3,6 +3,8 @@ package serving import ( + "bytes" + "context" "encoding/base64" "os" "path/filepath" @@ -89,3 +91,44 @@ func TestDNSDistBootstrapPublishesOnlyCompleteConfiguration(t *testing.T) { t.Fatal("incomplete retained rules must block startup") } } + +func TestRestartRetainsRoutingAcrossProcessConfigurationRefresh(t *testing.T) { + cfg := regionalTestConfig(t) + cfg.Mode = ModeResolver + cfg.Render.Role = RenderNode + cfg.Render.NodeDNSDist.Path = filepath.Join(t.TempDir(), "node-dnsdist.conf") + _, snapshot := renderFixture(t) + data := renderDNSDist(cfg.Render.NodeDNSDist, cfg.Render.NodeDNSDist.Backends, snapshot.Bindings, map[string]bool{"bind-a": true}, false, nil, nil) + retained := []byte(strings.ReplaceAll(string(data), "tcpOnly=true,checkTCP=true,", "")) + if err := os.WriteFile(cfg.Render.NodeDNSDist.Path, retained, 0644); err != nil { + t.Fatal(err) + } + agent, err := NewAgent(cfg, Dependencies{Runner: &recordingRunner{}}) + if err != nil { + t.Fatal(err) + } + agent.state.Snapshot = &snapshot + agent.state.BindingFences, _ = advanceBindingFences(nil, snapshot) + agent.state.Active = &activeConfiguration{Snapshot: snapshot, Files: map[string][]byte{cfg.Render.NodeDNSDist.Path: retained}} + agent.state.RenderedHash = hashFiles(agent.state.Active.Files) + if err := agent.store.Save(agent.state); err != nil { + t.Fatal(err) + } + if err := prepareDNSDistRouter(cfg.Render.NodeDNSDist); err != nil { + t.Fatal(err) + } + runner := &recordingRunner{} + restarted, err := NewAgent(cfg, Dependencies{Runner: runner}) + if err != nil { + t.Fatal(err) + } + if err := restarted.restoreActive(context.Background()); err != nil { + t.Fatal(err) + } + if len(runner.calls) != 0 { + t.Fatalf("restoration reloaded a running daemon with matching accepted routing: %v", runner.calls) + } + if !installedFilesMatch(restarted.state.Active.Files) || !bytes.Equal(restarted.state.Active.Files[cfg.Render.NodeDNSDist.Path], data) { + t.Fatal("accepted checkpoint and current process configuration differ") + } +} From 6f66e14a22bcc7430b53488a382a7c76dbf78007 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Wed, 21 Oct 2026 21:28:28 -0500 Subject: [PATCH 19/19] fix: retry concurrent DNS acknowledgement projections --- internal/internaldns/platform/acks.go | 16 ++++++++++++++++ internal/internaldns/platform/platform_test.go | 8 ++++---- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/internal/internaldns/platform/acks.go b/internal/internaldns/platform/acks.go index d855efd4..99f31d60 100644 --- a/internal/internaldns/platform/acks.go +++ b/internal/internaldns/platform/acks.go @@ -17,6 +17,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -46,6 +47,15 @@ type AckSink struct { } func (s *AckSink) Apply(ctx context.Context, subject string, env model.Envelope) error { + return retry.RetryOnConflict(retry.DefaultRetry, func() error { + if err := ctx.Err(); err != nil { + return err + } + return s.apply(ctx, subject, env) + }) +} + +func (s *AckSink) apply(ctx context.Context, subject string, env model.Envelope) error { if env.Kind != model.KindMemberAck || env.Region != s.Region || env.Shard != s.Shard { return fmt.Errorf("ack belongs to another kind, region or shard") } @@ -127,6 +137,9 @@ func (s *AckSink) publication(ctx context.Context, a model.MemberAck, row dnsv1a m.Status.ReplicaAcknowledgements = upsertAck(m.Status.ReplicaAcknowledgements, row) complete := s.publicationVerified(&m, now) condition(&m.Status.Conditions, "Published", complete, "ReplicaVerification", m.Generation, now) + if jsonEqualStatus(base.Status, m.Status) { + return nil + } if err := s.Client.Status().Patch(ctx, &m, client.MergeFromWithOptions(base, client.MergeFromWithOptimisticLock{})); err != nil { return err } @@ -173,6 +186,9 @@ func (s *AckSink) serving(ctx context.Context, a model.MemberAck, row dnsv1alpha binding.Status.Phase = dnsValuePending } s.bindingConditions(binding, e, snapshot, now, complete) + if jsonEqualStatus(base.Status, binding.Status) { + continue + } if err := s.Client.Status().Patch(ctx, binding, client.MergeFromWithOptions(base, client.MergeFromWithOptimisticLock{})); err != nil { return err } diff --git a/internal/internaldns/platform/platform_test.go b/internal/internaldns/platform/platform_test.go index c2da035a..119f9fc3 100644 --- a/internal/internaldns/platform/platform_test.go +++ b/internal/internaldns/platform/platform_test.go @@ -14,7 +14,6 @@ import ( "go.miloapis.com/dns-operator/internal/internaldns/model" "go.miloapis.com/dns-operator/internal/internaldns/transport" corev1 "k8s.io/api/core/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -579,6 +578,7 @@ func TestPublicationProjectionOnlyReadsOwningProject(t *testing.T) { type heldManifestRead struct { client.Client + once sync.Once read chan struct{} release chan struct{} } @@ -588,7 +588,7 @@ func (c *heldManifestRead) Get(ctx context.Context, key client.ObjectKey, object return err } if _, ok := object.(*dnsv1alpha1.DNSPublicationManifest); ok { - close(c.read) + c.once.Do(func() { close(c.read) }) select { case <-c.release: case <-ctx.Done(): @@ -633,8 +633,8 @@ func TestConcurrentAckProjectionPreservesNewerObservation(t *testing.T) { t.Fatal(err) } close(held.release) - if err := <-done; !apierrors.IsConflict(err) { - t.Fatalf("racing older projection did not reject stale write: %v", err) + if err := <-done; err != nil { + t.Fatalf("racing older projection did not retry the optimistic conflict: %v", err) } if err := sink.Apply(ctx, subject, older); err != nil { t.Fatalf("older acknowledgement redelivery failed: %v", err)