From 655c349696bce24b36cc7756c3b2e1162ae396d6 Mon Sep 17 00:00:00 2001 From: Michael Nairn Date: Fri, 25 Oct 2024 16:51:02 +0100 Subject: [PATCH 1/3] dnspolicy section name support Signed-off-by: Michael Nairn --- api/v1alpha1/dnspolicy_types.go | 24 ++- api/v1alpha1/topology.go | 6 +- api/v1alpha1/zz_generated.deepcopy.go | 2 +- ...adrant-operator.clusterserviceversion.yaml | 2 +- bundle/manifests/kuadrant.io_dnspolicies.yaml | 19 +++ .../templates/manifests.yaml | 19 +++ config/crd/bases/kuadrant.io_dnspolicies.yaml | 19 +++ .../dnspolicy/dnspolicy_controller_test.go | 142 +++++++++++++++++- .../target_status_controller_test.go | 2 +- 9 files changed, 221 insertions(+), 14 deletions(-) diff --git a/api/v1alpha1/dnspolicy_types.go b/api/v1alpha1/dnspolicy_types.go index bac852131..0a2fecbb7 100644 --- a/api/v1alpha1/dnspolicy_types.go +++ b/api/v1alpha1/dnspolicy_types.go @@ -26,6 +26,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" @@ -58,7 +59,7 @@ type DNSPolicySpec struct { // targetRef identifies an API object to apply policy to. // +kubebuilder:validation:XValidation:rule="self.group == 'gateway.networking.k8s.io'",message="Invalid targetRef.group. The only supported value is 'gateway.networking.k8s.io'" // +kubebuilder:validation:XValidation:rule="self.kind == 'Gateway'",message="Invalid targetRef.kind. The only supported values are 'Gateway'" - TargetRef gatewayapiv1alpha2.LocalPolicyTargetReference `json:"targetRef"` + TargetRef gatewayapiv1alpha2.LocalPolicyTargetReferenceWithSectionName `json:"targetRef"` // +optional HealthCheck *dnsv1alpha1.HealthCheckSpec `json:"healthCheck,omitempty"` @@ -190,7 +191,7 @@ func (p *DNSPolicy) GetRulesHostnames() []string { } func (p *DNSPolicy) GetTargetRef() gatewayapiv1alpha2.LocalPolicyTargetReference { - return p.Spec.TargetRef + return p.Spec.TargetRef.LocalPolicyTargetReference } func (p *DNSPolicy) GetStatus() kuadrantgatewayapi.PolicyStatus { @@ -252,7 +253,7 @@ func NewDNSPolicy(name, ns string) *DNSPolicy { } } -func (p *DNSPolicy) WithTargetRef(targetRef gatewayapiv1alpha2.LocalPolicyTargetReference) *DNSPolicy { +func (p *DNSPolicy) WithTargetRef(targetRef gatewayapiv1alpha2.LocalPolicyTargetReferenceWithSectionName) *DNSPolicy { p.Spec.TargetRef = targetRef return p } @@ -290,13 +291,22 @@ func (p *DNSPolicy) WithExcludeAddresses(excluded []string) *DNSPolicy { //TargetRef func (p *DNSPolicy) WithTargetGateway(gwName string) *DNSPolicy { - return p.WithTargetRef(gatewayapiv1alpha2.LocalPolicyTargetReference{ - Group: gatewayapiv1.GroupName, - Kind: "Gateway", - Name: gatewayapiv1.ObjectName(gwName), + return p.WithTargetRef(gatewayapiv1alpha2.LocalPolicyTargetReferenceWithSectionName{ + LocalPolicyTargetReference: gatewayapiv1alpha2.LocalPolicyTargetReference{ + Group: gatewayapiv1.GroupName, + Kind: "Gateway", + Name: gatewayapiv1.ObjectName(gwName), + }, + SectionName: nil, }) } +func (p *DNSPolicy) WithTargetGatewayListener(gwName string, lName string) *DNSPolicy { + p.WithTargetGateway(gwName) + p.Spec.TargetRef.SectionName = ptr.To(gatewayapiv1.SectionName(lName)) + return p +} + //HealthCheck func (p *DNSPolicy) WithHealthCheckFor(endpoint string, port int, protocol string, failureThreshold int) *DNSPolicy { diff --git a/api/v1alpha1/topology.go b/api/v1alpha1/topology.go index 0ab5156bf..2fe36e218 100644 --- a/api/v1alpha1/topology.go +++ b/api/v1alpha1/topology.go @@ -18,9 +18,9 @@ var _ machinery.Policy = &DNSPolicy{} func (p *DNSPolicy) GetTargetRefs() []machinery.PolicyTargetReference { return []machinery.PolicyTargetReference{ - machinery.LocalPolicyTargetReference{ - LocalPolicyTargetReference: p.Spec.TargetRef, - PolicyNamespace: p.Namespace, + machinery.LocalPolicyTargetReferenceWithSectionName{ + LocalPolicyTargetReferenceWithSectionName: p.Spec.TargetRef, + PolicyNamespace: p.Namespace, }, } } diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 1ca43e956..aa08cd87c 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -130,7 +130,7 @@ func (in *DNSPolicyList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *DNSPolicySpec) DeepCopyInto(out *DNSPolicySpec) { *out = *in - out.TargetRef = in.TargetRef + in.TargetRef.DeepCopyInto(&out.TargetRef) if in.HealthCheck != nil { in, out := &in.HealthCheck, &out.HealthCheck *out = new(apiv1alpha1.HealthCheckSpec) diff --git a/bundle/manifests/kuadrant-operator.clusterserviceversion.yaml b/bundle/manifests/kuadrant-operator.clusterserviceversion.yaml index 0860c05dc..afbe5a2f3 100644 --- a/bundle/manifests/kuadrant-operator.clusterserviceversion.yaml +++ b/bundle/manifests/kuadrant-operator.clusterserviceversion.yaml @@ -106,7 +106,7 @@ metadata: capabilities: Basic Install categories: Integration & Delivery containerImage: quay.io/kuadrant/kuadrant-operator:latest - createdAt: "2024-10-22T09:01:33Z" + createdAt: "2024-10-25T15:27:18Z" description: A Kubernetes Operator to manage the lifecycle of the Kuadrant system operators.operatorframework.io/builder: operator-sdk-v1.32.0 operators.operatorframework.io/project_layout: go.kubebuilder.io/v3 diff --git a/bundle/manifests/kuadrant.io_dnspolicies.yaml b/bundle/manifests/kuadrant.io_dnspolicies.yaml index 42dfb30db..98b4226b0 100644 --- a/bundle/manifests/kuadrant.io_dnspolicies.yaml +++ b/bundle/manifests/kuadrant.io_dnspolicies.yaml @@ -194,6 +194,25 @@ spec: maxLength: 253 minLength: 1 type: string + sectionName: + description: |- + SectionName is the name of a section within the target resource. When + unspecified, this targetRef targets the entire resource. In the following + resources, SectionName is interpreted as the following: + + + * Gateway: Listener name + * HTTPRoute: HTTPRouteRule name + * Service: Port name + + + If a SectionName is specified, but does not exist on the targeted object, + the Policy must fail to attach, and the policy implementation should record + a `ResolvedRefs` or similar Condition in the Policy's status. + maxLength: 253 + minLength: 1 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ + type: string required: - group - kind diff --git a/charts/kuadrant-operator/templates/manifests.yaml b/charts/kuadrant-operator/templates/manifests.yaml index 3c2c33014..b34da88b7 100644 --- a/charts/kuadrant-operator/templates/manifests.yaml +++ b/charts/kuadrant-operator/templates/manifests.yaml @@ -6962,6 +6962,25 @@ spec: maxLength: 253 minLength: 1 type: string + sectionName: + description: |- + SectionName is the name of a section within the target resource. When + unspecified, this targetRef targets the entire resource. In the following + resources, SectionName is interpreted as the following: + + + * Gateway: Listener name + * HTTPRoute: HTTPRouteRule name + * Service: Port name + + + If a SectionName is specified, but does not exist on the targeted object, + the Policy must fail to attach, and the policy implementation should record + a `ResolvedRefs` or similar Condition in the Policy's status. + maxLength: 253 + minLength: 1 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ + type: string required: - group - kind diff --git a/config/crd/bases/kuadrant.io_dnspolicies.yaml b/config/crd/bases/kuadrant.io_dnspolicies.yaml index ab686e3a6..d08a38f69 100644 --- a/config/crd/bases/kuadrant.io_dnspolicies.yaml +++ b/config/crd/bases/kuadrant.io_dnspolicies.yaml @@ -193,6 +193,25 @@ spec: maxLength: 253 minLength: 1 type: string + sectionName: + description: |- + SectionName is the name of a section within the target resource. When + unspecified, this targetRef targets the entire resource. In the following + resources, SectionName is interpreted as the following: + + + * Gateway: Listener name + * HTTPRoute: HTTPRouteRule name + * Service: Port name + + + If a SectionName is specified, but does not exist on the targeted object, + the Policy must fail to attach, and the policy implementation should record + a `ResolvedRefs` or similar Condition in the Policy's status. + maxLength: 253 + minLength: 1 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ + type: string required: - group - kind diff --git a/tests/common/dnspolicy/dnspolicy_controller_test.go b/tests/common/dnspolicy/dnspolicy_controller_test.go index 196e04c66..4771f597e 100644 --- a/tests/common/dnspolicy/dnspolicy_controller_test.go +++ b/tests/common/dnspolicy/dnspolicy_controller_test.go @@ -840,7 +840,7 @@ var _ = Describe("DNSPolicy controller", func() { })), ) g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: wildcardRecordName, Namespace: testNamespace}, currentWildcardRec)).To(Succeed()) - g.Expect(currentRec.Status.Conditions).To( + g.Expect(currentWildcardRec.Status.Conditions).To( ContainElements( MatchFields(IgnoreExtras, Fields{ "Type": Equal(string(kuadrantdnsv1alpha1.ConditionTypeReady)), @@ -1071,6 +1071,146 @@ var _ = Describe("DNSPolicy controller", func() { }) }) + Context("section name", func() { + It("should handle policy with section name", func(ctx SpecContext) { + containReadyCondition := ContainElements( + MatchFields(IgnoreExtras, Fields{ + "Type": Equal(string(kuadrantdnsv1alpha1.ConditionTypeReady)), + "Status": Equal(metav1.ConditionTrue), + })) + + rootEndpointMatcher := func(dnsName, recordType, setID string, ttl int64, targets ...string) types.GomegaMatcher { + return PointTo(MatchFields(IgnoreExtras, Fields{ + "DNSName": Equal(dnsName), + "Targets": ConsistOf(targets), + "RecordType": Equal(recordType), + "SetIdentifier": Equal(setID), + "RecordTTL": Equal(externaldns.TTL(ttl)), + })) + } + + //listener 1 & 2 - Default gateway policy has a loadbalancing section so will create loadbalanced records with CNAME Records for the rootHost + currentRecRootEndpoint := rootEndpointMatcher(tests.HostOne(domain), "CNAME", "", 300, "klb.test."+domain) + currentWildcardRecRootEndpoint := rootEndpointMatcher(tests.HostWildcard(domain), "CNAME", "", 300, "klb."+domain) + + //get the current dnsrecord and wildcard dnsrecord + currentRec := &kuadrantdnsv1alpha1.DNSRecord{} + currentWildcardRec := &kuadrantdnsv1alpha1.DNSRecord{} + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: recordName, Namespace: testNamespace}, currentRec)).To(Succeed()) + g.Expect(currentRec.Status.Conditions).To(containReadyCondition) + g.Expect(currentRec.Spec.Endpoints).To(ContainElement(currentRecRootEndpoint)) + + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: wildcardRecordName, Namespace: testNamespace}, currentWildcardRec)).To(Succeed()) + g.Expect(currentWildcardRec.Status.Conditions).To(containReadyCondition) + g.Expect(currentWildcardRec.Spec.Endpoints).To(ContainElement(currentWildcardRecRootEndpoint)) + }, tests.TimeoutLong, time.Second).Should(BeNil()) + + By("creating a dnspolicy with section name for listener one") + dnsPolicyWithSection := tests.NewDNSPolicy("test-dns-policy-with-section-name", testNamespace). + WithProviderSecret(*dnsProviderSecret). + WithTargetGatewayListener(tests.GatewayName, tests.ListenerNameOne) + Expect(k8sClient.Create(ctx, dnsPolicyWithSection)).To(Succeed()) + + //listener 1 - Listener policy has no loadbalancing section so will create simple records with A Records for the rootHost + newRecRootEndpoint := rootEndpointMatcher(tests.HostOne(domain), "A", "", 60, tests.IPAddressOne, tests.IPAddressTwo) + //listener 2 - Default gateway policy has a loadbalancing section so will create loadbalanced records with CNAME Records for the rootHost + newWildcardRecRootEndpoint := currentWildcardRecRootEndpoint + + //get the dnsrecord again and verify it's no longer the same DNSRecord resource and the record type for the root host has changed + //get the wildcard dnsrecord again and verify the DNSRecord resource is unchanged + Eventually(func(g Gomega) { + newRec := &kuadrantdnsv1alpha1.DNSRecord{} + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: recordName, Namespace: testNamespace}, newRec)).To(Succeed()) + g.Expect(newRec.Spec.RootHost).To(Equal(currentRec.Spec.RootHost)) + g.Expect(newRec.UID).ToNot(Equal(currentRec.UID)) // if/when we remove the need for record re-creation on policy changes, these assertions can be removed + g.Expect(newRec.Status.Conditions).To(containReadyCondition) + g.Expect(newRec.Spec.Endpoints).To(ContainElement(newRecRootEndpoint)) + + newWildcardRec := &kuadrantdnsv1alpha1.DNSRecord{} + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: wildcardRecordName, Namespace: testNamespace}, newWildcardRec)).To(Succeed()) + g.Expect(newWildcardRec.Spec.RootHost).To(Equal(currentWildcardRec.Spec.RootHost)) + g.Expect(newWildcardRec.UID).To(Equal(currentWildcardRec.UID)) + g.Expect(newWildcardRec.Status.Conditions).To(containReadyCondition) + g.Expect(newWildcardRec.Spec.Endpoints).To(ContainElement(newWildcardRecRootEndpoint)) + + //ToDo Add checks for policy affected by on gateway when possible, will require discoverability changes + + currentRec = newRec + currentWildcardRec = newWildcardRec + }, tests.TimeoutLong, time.Second).Should(BeNil()) + + By("updating dnspolicy section name to listener two") + Eventually(func() error { + existingDNSpolicy := &v1alpha1.DNSPolicy{} + if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(dnsPolicyWithSection), existingDNSpolicy); err != nil { + return err + } + patch := client.MergeFrom(existingDNSpolicy.DeepCopy()) + existingDNSpolicy.Spec.TargetRef.SectionName = ptr.To(gatewayapiv1.SectionName(tests.ListenerNameWildcard)) + return k8sClient.Patch(ctx, existingDNSpolicy, patch) + }, tests.TimeoutMedium, time.Second).Should(Succeed()) + + //listener 1 - Default gateway policy has a loadbalancing section so will create loadbalanced records with CNAME Records for the rootHost + newRecRootEndpoint = rootEndpointMatcher(tests.HostOne(domain), "CNAME", "", 300, "klb.test."+domain) + //listener 2 - Listener policy has no loadbalancing section so will create simple records with A Records for the rootHost + newWildcardRecRootEndpoint = rootEndpointMatcher(tests.HostWildcard(domain), "A", "", 60, tests.IPAddressOne, tests.IPAddressTwo) + + //get the dnsrecord again and verify it's no longer the same DNSRecord resource and the record type for the root host has changed + //get the wildcard dnsrecord and verify it's no longer the same DNSRecord resource and the record type for the root host has changed + Eventually(func(g Gomega) { + newRec := &kuadrantdnsv1alpha1.DNSRecord{} + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: recordName, Namespace: testNamespace}, newRec)).To(Succeed()) + g.Expect(newRec.Spec.RootHost).To(Equal(currentRec.Spec.RootHost)) + g.Expect(newRec.UID).ToNot(Equal(currentRec.UID)) + g.Expect(newRec.Status.Conditions).To(containReadyCondition) + g.Expect(newRec.Spec.Endpoints).To(ContainElement(newRecRootEndpoint)) + + newWildcardRec := &kuadrantdnsv1alpha1.DNSRecord{} + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: wildcardRecordName, Namespace: testNamespace}, newWildcardRec)).To(Succeed()) + g.Expect(newWildcardRec.Spec.RootHost).To(Equal(currentWildcardRec.Spec.RootHost)) + g.Expect(newWildcardRec.UID).ToNot(Equal(currentWildcardRec.UID)) + g.Expect(newWildcardRec.Status.Conditions).To(containReadyCondition) + g.Expect(newWildcardRec.Spec.Endpoints).To(ContainElement(newWildcardRecRootEndpoint)) + + //ToDo Add checks for policy affected by on gateway when possible, will require discoverability changes + + currentRec = newRec + currentWildcardRec = newWildcardRec + }, tests.TimeoutLong, time.Second).Should(BeNil()) + + By("deleting the dnspolicy with section name") + Expect(k8sClient.Delete(ctx, dnsPolicyWithSection)).To(Succeed()) + + //listener 1 & 2 - Default gateway policy has a loadbalancing section so will create loadbalanced records with CNAME Records for the rootHost + newRecRootEndpoint = rootEndpointMatcher(tests.HostOne(domain), "CNAME", "", 300, "klb.test."+domain) + newWildcardRecRootEndpoint = rootEndpointMatcher(tests.HostWildcard(domain), "CNAME", "", 300, "klb."+domain) + + //get the dnsrecord again and verify the DNSRecord resource is unchanged + //get the wildcard dnsrecord and verify it's no longer the same DNSRecord resource and the record type for the root host has changed + Eventually(func(g Gomega) { + newRec := &kuadrantdnsv1alpha1.DNSRecord{} + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: recordName, Namespace: testNamespace}, newRec)).To(Succeed()) + g.Expect(newRec.Spec.RootHost).To(Equal(currentRec.Spec.RootHost)) + g.Expect(newRec.UID).To(Equal(currentRec.UID)) + g.Expect(newRec.Status.Conditions).To(containReadyCondition) + g.Expect(newRec.Spec.Endpoints).To(ContainElement(newRecRootEndpoint)) + + newWildcardRec := &kuadrantdnsv1alpha1.DNSRecord{} + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: wildcardRecordName, Namespace: testNamespace}, newWildcardRec)).To(Succeed()) + g.Expect(newWildcardRec.Spec.RootHost).To(Equal(currentWildcardRec.Spec.RootHost)) + g.Expect(newWildcardRec.UID).ToNot(Equal(currentWildcardRec.UID)) + g.Expect(newWildcardRec.Status.Conditions).To(containReadyCondition) + g.Expect(newWildcardRec.Spec.Endpoints).To(ContainElement(newWildcardRecRootEndpoint)) + + //ToDo Add checks for policy affected by on gateway when possible, will require discoverability changes + + currentRec = newRec + currentWildcardRec = newWildcardRec + }, tests.TimeoutLong, time.Second).Should(BeNil()) + }) + }) + }) Context("cel validation", func() { diff --git a/tests/common/targetstatus/target_status_controller_test.go b/tests/common/targetstatus/target_status_controller_test.go index 6635a017b..0b0f41bb0 100644 --- a/tests/common/targetstatus/target_status_controller_test.go +++ b/tests/common/targetstatus/target_status_controller_test.go @@ -481,7 +481,7 @@ var _ = Describe("Target status reconciler", func() { policyAcceptedAndTargetsAffected := func(ctx context.Context, policy *kuadrantv1alpha1.DNSPolicy) func() bool { return func() bool { policyKey := client.ObjectKeyFromObject(policy) - return isDNSPolicyAccepted(ctx, policyKey) && targetsAffected(ctx, policyKey, policyAffectedCondition, policy.Spec.TargetRef) + return isDNSPolicyAccepted(ctx, policyKey) && targetsAffected(ctx, policyKey, policyAffectedCondition, policy.Spec.TargetRef.LocalPolicyTargetReference) } } From ec52eae213b8342ac89c9ee92e7bec81f983c636 Mon Sep 17 00:00:00 2001 From: Michael Nairn Date: Wed, 30 Oct 2024 08:54:16 +0000 Subject: [PATCH 2/3] fix dsnpolicy: Don't change owner ref on update DNSRecords are currently owned by the DNSPolicy that created them. We can't just update the ownership on policy change since the new policy may not be compatible with the current DNSRecord, instead we must re-create the DNSRecord resource. This is not ideal, issue to look into changing this https://github.com/Kuadrant/dns-operator/issues/287 Signed-off-by: Michael Nairn --- .../effective_dnspolicies_reconciler.go | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/controllers/effective_dnspolicies_reconciler.go b/controllers/effective_dnspolicies_reconciler.go index 0b70cc579..d5bcaebbe 100644 --- a/controllers/effective_dnspolicies_reconciler.go +++ b/controllers/effective_dnspolicies_reconciler.go @@ -138,7 +138,22 @@ func (r *EffectiveDNSPoliciesReconciler) reconcile(ctx context.Context, _ []cont existingRecord := existingRecordObj.(*controller.RuntimeObject).Object.(*kuadrantdnsv1alpha1.DNSRecord) - //Deal with the potential deletion of a record first + // Deal with the potential deletion of a record first + // + // A DNSRecord can't currently support record type changes due to a limitation of the dns operator + // https://github.com/Kuadrant/dns-operator/issues/287 + // If the policy changes, delete the existing record and break out to create the new one. + // Note: There should only ever be a single DNSPolicy targeting a unique Gateway/Listener until we + // resolve the above issue. + pPoliciesLocs := lo.Map(topology.Policies().Parents(existingRecordObj), func(item machinery.Policy, _ int) string { + return item.GetLocator() + }) + if !lo.Contains(pPoliciesLocs, policy.GetLocator()) { + rLogger.V(1).Info("parent policy has changed, deleting record for listener") + r.deleteRecord(ctx, existingRecordObj) + break + } + if !hasAttachedRoute || len(desiredRecord.Spec.Endpoints) == 0 { if !hasAttachedRoute { rLogger.V(1).Info("listener has no attached routes, deleting record for listener") @@ -156,13 +171,11 @@ func (r *EffectiveDNSPoliciesReconciler) reconcile(ctx context.Context, _ []cont break } - if reflect.DeepEqual(existingRecord.Spec, desiredRecord.Spec) && - reflect.DeepEqual(existingRecord.OwnerReferences, desiredRecord.OwnerReferences) { + if reflect.DeepEqual(existingRecord.Spec, desiredRecord.Spec) { rLogger.V(1).Info("dns record is up to date, nothing to do") continue } existingRecord.Spec = desiredRecord.Spec - existingRecord.OwnerReferences = desiredRecord.OwnerReferences un, err := controller.Destruct(existingRecord) if err != nil { From 504d443115c5bc15df35f6c1cdcd7555eda83acc Mon Sep 17 00:00:00 2001 From: Michael Nairn Date: Wed, 30 Oct 2024 16:31:15 +0000 Subject: [PATCH 3/3] Add canUpdateDNSRecord function Returns true if an existing record can knowingly be updated to a desired state based on the differences of the specs. Current known reasons for not being able to update are: * RootHost updates * Endpoint record type changes (A -> CNAME etc..) In both these cases, and any others that may be added to `canUpdateDNSRecord` the current record should be deleted before doing a create of the desired record. Signed-off-by: Michael Nairn --- .../effective_dnspolicies_reconciler.go | 49 ++++--- .../effective_dnspolicies_reconciler_test.go | 135 ++++++++++++++++++ 2 files changed, 165 insertions(+), 19 deletions(-) create mode 100644 controllers/effective_dnspolicies_reconciler_test.go diff --git a/controllers/effective_dnspolicies_reconciler.go b/controllers/effective_dnspolicies_reconciler.go index d5bcaebbe..07b106d10 100644 --- a/controllers/effective_dnspolicies_reconciler.go +++ b/controllers/effective_dnspolicies_reconciler.go @@ -139,21 +139,6 @@ func (r *EffectiveDNSPoliciesReconciler) reconcile(ctx context.Context, _ []cont existingRecord := existingRecordObj.(*controller.RuntimeObject).Object.(*kuadrantdnsv1alpha1.DNSRecord) // Deal with the potential deletion of a record first - // - // A DNSRecord can't currently support record type changes due to a limitation of the dns operator - // https://github.com/Kuadrant/dns-operator/issues/287 - // If the policy changes, delete the existing record and break out to create the new one. - // Note: There should only ever be a single DNSPolicy targeting a unique Gateway/Listener until we - // resolve the above issue. - pPoliciesLocs := lo.Map(topology.Policies().Parents(existingRecordObj), func(item machinery.Policy, _ int) string { - return item.GetLocator() - }) - if !lo.Contains(pPoliciesLocs, policy.GetLocator()) { - rLogger.V(1).Info("parent policy has changed, deleting record for listener") - r.deleteRecord(ctx, existingRecordObj) - break - } - if !hasAttachedRoute || len(desiredRecord.Spec.Endpoints) == 0 { if !hasAttachedRoute { rLogger.V(1).Info("listener has no attached routes, deleting record for listener") @@ -164,10 +149,9 @@ func (r *EffectiveDNSPoliciesReconciler) reconcile(ctx context.Context, _ []cont continue } - if desiredRecord.Spec.RootHost != existingRecord.Spec.RootHost { - rLogger.V(1).Info("listener hostname has changed, deleting record for listener") + if !canUpdateDNSRecord(ctx, existingRecord, desiredRecord) { + rLogger.V(1).Info("unable to update record, deleting record for listener and re-creating") r.deleteRecord(ctx, existingRecordObj) - //Break to allow it to try the creation of the desired record break } @@ -183,7 +167,7 @@ func (r *EffectiveDNSPoliciesReconciler) reconcile(ctx context.Context, _ []cont continue } - rLogger.V(1).Info("updating DNS record for listener") + rLogger.V(1).Info("updating record for listener") if _, uErr := resource.Update(ctx, un, metav1.UpdateOptions{}); uErr != nil { rLogger.Error(uErr, "unable to update dns record") } @@ -313,3 +297,30 @@ func (r *EffectiveDNSPoliciesReconciler) deleteRecord(ctx context.Context, obj m logger.Error(err, "failed to delete DNSRecord", "record", obj.GetLocator()) } } + +// canUpdateDNSRecord returns true if the current record can be updated to the desired. +func canUpdateDNSRecord(ctx context.Context, current, desired *kuadrantdnsv1alpha1.DNSRecord) bool { + logger := controller.LoggerFromContext(ctx) + + // DNSRecord doesn't currently support rootHost changes + if current.Spec.RootHost != desired.Spec.RootHost { + logger.V(1).Info("root host for existing record has changed") + return false + } + + // DNSRecord doesn't currently support record type changes due to a limitation of the dns operator + // https://github.com/Kuadrant/dns-operator/issues/287 + for _, curEp := range current.Spec.Endpoints { + for _, desEp := range desired.Spec.Endpoints { + if curEp.DNSName == desEp.DNSName { + if curEp.RecordType != desEp.RecordType { + logger.V(1).Info("record type for existing endpoint has changed", + "dnsName", curEp.DNSName, "current", curEp.RecordType, "desired", desEp.RecordType) + return false + } + } + } + } + + return true +} diff --git a/controllers/effective_dnspolicies_reconciler_test.go b/controllers/effective_dnspolicies_reconciler_test.go new file mode 100644 index 000000000..59cee7771 --- /dev/null +++ b/controllers/effective_dnspolicies_reconciler_test.go @@ -0,0 +1,135 @@ +package controllers + +import ( + "context" + "testing" + + externaldns "sigs.k8s.io/external-dns/endpoint" + + kuadrantdnsv1alpha1 "github.com/kuadrant/dns-operator/api/v1alpha1" +) + +func Test_canUpdateDNSRecord(t *testing.T) { + tests := []struct { + name string + current *kuadrantdnsv1alpha1.DNSRecord + desired *kuadrantdnsv1alpha1.DNSRecord + want bool + }{ + { + name: "different root hosts", + current: &kuadrantdnsv1alpha1.DNSRecord{ + Spec: kuadrantdnsv1alpha1.DNSRecordSpec{ + RootHost: "foo.example.com", + }, + }, + desired: &kuadrantdnsv1alpha1.DNSRecord{ + Spec: kuadrantdnsv1alpha1.DNSRecordSpec{ + RootHost: "bar.example.com", + }, + }, + want: false, + }, + { + name: "same root hosts", + current: &kuadrantdnsv1alpha1.DNSRecord{ + Spec: kuadrantdnsv1alpha1.DNSRecordSpec{ + RootHost: "foo.example.com", + }, + }, + desired: &kuadrantdnsv1alpha1.DNSRecord{ + Spec: kuadrantdnsv1alpha1.DNSRecordSpec{ + RootHost: "foo.example.com", + }, + }, + want: true, + }, + { + name: "different record type same dnsnames", + current: &kuadrantdnsv1alpha1.DNSRecord{ + Spec: kuadrantdnsv1alpha1.DNSRecordSpec{ + Endpoints: []*externaldns.Endpoint{ + { + DNSName: "foo.example.com", + RecordType: "A", + }, + }, + }, + }, + desired: &kuadrantdnsv1alpha1.DNSRecord{ + Spec: kuadrantdnsv1alpha1.DNSRecordSpec{ + Endpoints: []*externaldns.Endpoint{ + { + DNSName: "foo.example.com", + RecordType: "CNAME", + }, + }, + }, + }, + want: false, + }, + { + name: "same record type same dnsnames", + current: &kuadrantdnsv1alpha1.DNSRecord{ + Spec: kuadrantdnsv1alpha1.DNSRecordSpec{ + Endpoints: []*externaldns.Endpoint{ + { + DNSName: "foo.example.com", + RecordType: "A", + }, + }, + }, + }, + desired: &kuadrantdnsv1alpha1.DNSRecord{ + Spec: kuadrantdnsv1alpha1.DNSRecordSpec{ + Endpoints: []*externaldns.Endpoint{ + { + DNSName: "foo.example.com", + RecordType: "A", + }, + }, + }, + }, + want: true, + }, + { + name: "multiple endpoints", + current: &kuadrantdnsv1alpha1.DNSRecord{ + Spec: kuadrantdnsv1alpha1.DNSRecordSpec{ + Endpoints: []*externaldns.Endpoint{ + { + DNSName: "foo.example.com", + RecordType: "A", + }, + { + DNSName: "baz.example.com", + RecordType: "CNAME", + }, + }, + }, + }, + desired: &kuadrantdnsv1alpha1.DNSRecord{ + Spec: kuadrantdnsv1alpha1.DNSRecordSpec{ + Endpoints: []*externaldns.Endpoint{ + { + DNSName: "foo.example.com", + RecordType: "A", + }, + { + DNSName: "bar.example.com", + RecordType: "CNAME", + }, + }, + }, + }, + want: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := canUpdateDNSRecord(context.Background(), tt.current, tt.desired); got != tt.want { + t.Errorf("canUpdateDNSRecord() = %v, want %v", got, tt.want) + } + }) + } +}