diff --git a/internal/common/helper.go b/internal/common/helper.go index 3fdc36b5..53fbd0a2 100644 --- a/internal/common/helper.go +++ b/internal/common/helper.go @@ -1,8 +1,10 @@ package common import ( + "reflect" "time" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/rand" ) @@ -33,3 +35,40 @@ func Owns(owner, object metav1.Object) bool { } return false } + +func EnsureOwnerRef(owner, owned metav1.Object, blockDelete bool) error { + ownerType, err := meta.TypeAccessor(owner) + if err != nil { + return err + } + + ownerRef := metav1.OwnerReference{ + APIVersion: ownerType.GetAPIVersion(), + Kind: ownerType.GetKind(), + Name: owner.GetName(), + UID: owner.GetUID(), + BlockOwnerDeletion: &blockDelete, + } + + // check for existing ref + for i, ref := range owned.GetOwnerReferences() { + if ref.UID == owner.GetUID() { + if reflect.DeepEqual(ref, ownerRef) { + // no changes to make, return + return nil + } + // we need to update the ownerRef, remove the existing one + if len(owned.GetOwnerReferences()) == 1 { + owned.SetOwnerReferences([]metav1.OwnerReference{}) + } else { + owned.SetOwnerReferences(append(owned.GetOwnerReferences()[:i], owner.GetOwnerReferences()[i+1:]...)) + } + break + } + } + + // add ownerRef to object + owned.SetOwnerReferences(append(owned.GetOwnerReferences(), ownerRef)) + + return nil +} diff --git a/internal/common/helper_test.go b/internal/common/helper_test.go index 0353abf2..9faf20d2 100644 --- a/internal/common/helper_test.go +++ b/internal/common/helper_test.go @@ -178,3 +178,170 @@ func TestOwns(t *testing.T) { }) } } + +func TestEnsureOwnerRef(t *testing.T) { + RegisterTestingT(t) + testCases := []struct { + Name string + Owned metav1.Object + Owner metav1.Object + BlockDelete bool + Verify func(t *testing.T, err error, obj metav1.Object) + }{ + { + Name: "Owner is added", + Owned: &v1alpha1.DNSRecord{}, + Owner: &v1alpha1.ManagedZone{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-zone", + UID: "unique-uid", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "ManagedZone", + APIVersion: "v1beta1", + }, + }, + BlockDelete: true, + Verify: func(t *testing.T, err error, obj metav1.Object) { + Expect(err).NotTo(HaveOccurred()) + Expect(len(obj.GetOwnerReferences())).To(Equal(1)) + + expectedOwnerRef := metav1.OwnerReference{ + APIVersion: "v1beta1", + Kind: "ManagedZone", + Name: "test-zone", + UID: "unique-uid", + BlockOwnerDeletion: ptr.To(true), + } + Expect(obj.GetOwnerReferences()[0]).To(Equal(expectedOwnerRef)) + }, + }, + { + Name: "Does not duplicate owner ref", + Owned: &v1alpha1.DNSRecord{ + ObjectMeta: metav1.ObjectMeta{ + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "v1beta1", + Kind: "ManagedZone", + Name: "test-zone", + UID: "unique-uid", + BlockOwnerDeletion: ptr.To(true), + }, + }, + }, + }, + Owner: &v1alpha1.ManagedZone{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-zone", + UID: "unique-uid", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "ManagedZone", + APIVersion: "v1beta1", + }, + }, + BlockDelete: true, + Verify: func(t *testing.T, err error, obj metav1.Object) { + Expect(err).NotTo(HaveOccurred()) + Expect(len(obj.GetOwnerReferences())).To(Equal(1)) + + expectedOwnerRef := metav1.OwnerReference{ + APIVersion: "v1beta1", + Kind: "ManagedZone", + Name: "test-zone", + UID: "unique-uid", + BlockOwnerDeletion: ptr.To(true), + } + Expect(obj.GetOwnerReferences()[0]).To(Equal(expectedOwnerRef)) + }, + }, + { + Name: "Does update owner ref", + Owned: &v1alpha1.DNSRecord{ + ObjectMeta: metav1.ObjectMeta{ + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "v1beta1", + Kind: "ManagedZone", + Name: "test-zone", + UID: "unique-uid", + BlockOwnerDeletion: ptr.To(false), + }, + }, + }, + }, + Owner: &v1alpha1.ManagedZone{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-zone", + UID: "unique-uid", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "ManagedZone", + APIVersion: "v1beta1", + }, + }, + BlockDelete: true, + Verify: func(t *testing.T, err error, obj metav1.Object) { + Expect(err).NotTo(HaveOccurred()) + Expect(len(obj.GetOwnerReferences())).To(Equal(1)) + + expectedOwnerRef := metav1.OwnerReference{ + APIVersion: "v1beta1", + Kind: "ManagedZone", + Name: "test-zone", + UID: "unique-uid", + BlockOwnerDeletion: ptr.To(true), + } + Expect(obj.GetOwnerReferences()[0]).To(Equal(expectedOwnerRef)) + }, + }, + { + Name: "Does append owner ref", + Owned: &v1alpha1.DNSRecord{ + ObjectMeta: metav1.ObjectMeta{ + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "v1", + Kind: "OtherThing", + Name: "otherName", + UID: "other-unique-uid", + BlockOwnerDeletion: ptr.To(false), + }, + }, + }, + }, + Owner: &v1alpha1.ManagedZone{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-zone", + UID: "unique-uid", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "ManagedZone", + APIVersion: "v1beta1", + }, + }, + BlockDelete: true, + Verify: func(t *testing.T, err error, obj metav1.Object) { + Expect(err).NotTo(HaveOccurred()) + Expect(len(obj.GetOwnerReferences())).To(Equal(2)) + + expectedOwnerRef := metav1.OwnerReference{ + APIVersion: "v1beta1", + Kind: "ManagedZone", + Name: "test-zone", + UID: "unique-uid", + BlockOwnerDeletion: ptr.To(true), + } + Expect(obj.GetOwnerReferences()[1]).To(Equal(expectedOwnerRef)) + }, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.Name, func(t *testing.T) { + err := EnsureOwnerRef(testCase.Owned, testCase.Owner, testCase.BlockDelete) + testCase.Verify(t, err, testCase.Owned) + }) + } +} diff --git a/internal/controller/dnsrecord_controller.go b/internal/controller/dnsrecord_controller.go index 1ee1aa1b..449af6db 100644 --- a/internal/controller/dnsrecord_controller.go +++ b/internal/controller/dnsrecord_controller.go @@ -150,7 +150,7 @@ func (r *DNSRecordReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( } if !common.Owns(managedZone, dnsRecord) { - err = controllerutil.SetOwnerReference(managedZone, dnsRecord, r.Scheme) + err = common.EnsureOwnerRef(managedZone, dnsRecord, true) if err != nil { return ctrl.Result{}, err } diff --git a/internal/controller/dnsrecord_controller_test.go b/internal/controller/dnsrecord_controller_test.go index 0bd5f3a1..fc05c978 100644 --- a/internal/controller/dnsrecord_controller_test.go +++ b/internal/controller/dnsrecord_controller_test.go @@ -99,15 +99,15 @@ var _ = Describe("DNSRecordReconciler", func() { Expect(client.IgnoreNotFound(err)).ToNot(HaveOccurred()) } if dnsRecord2 != nil { - err := k8sClient.Delete(ctx, dnsRecord) + err := k8sClient.Delete(ctx, dnsRecord2) Expect(client.IgnoreNotFound(err)).ToNot(HaveOccurred()) } if managedZone != nil { - err := k8sClient.Delete(ctx, managedZone) + err := k8sClient.Delete(ctx, managedZone, client.PropagationPolicy(metav1.DeletePropagationForeground)) Expect(client.IgnoreNotFound(err)).ToNot(HaveOccurred()) } if brokenZone != nil { - err := k8sClient.Delete(ctx, brokenZone) + err := k8sClient.Delete(ctx, brokenZone, client.PropagationPolicy(metav1.DeletePropagationForeground)) Expect(client.IgnoreNotFound(err)).ToNot(HaveOccurred()) } }) @@ -162,64 +162,7 @@ var _ = Describe("DNSRecordReconciler", func() { g.Expect(dnsRecord.Finalizers).To(ContainElement(DNSRecordFinalizer)) }, TestTimeoutMedium, time.Second).Should(Succeed()) }) - It("maintains the finalizer on a deleting DNS record with a deleted managed zone", func(ctx SpecContext) { - dnsRecord = &v1alpha1.DNSRecord{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo.example.com", - Namespace: testNamespace, - }, - Spec: v1alpha1.DNSRecordSpec{ - OwnerID: "owner1", - RootHost: "foo.example.com", - ManagedZoneRef: &v1alpha1.ManagedZoneReference{ - Name: managedZone.Name, - }, - Endpoints: getDefaultTestEndpoints(), - HealthCheck: nil, - }, - } - Expect(k8sClient.Create(ctx, dnsRecord)).To(Succeed()) - Eventually(func(g Gomega) { - err := k8sClient.Get(ctx, client.ObjectKeyFromObject(dnsRecord), dnsRecord) - g.Expect(err).NotTo(HaveOccurred()) - g.Expect(dnsRecord.Status.Conditions).To( - ContainElement(MatchFields(IgnoreExtras, Fields{ - "Type": Equal(string(v1alpha1.ConditionTypeReady)), - "Status": Equal(metav1.ConditionTrue), - "Reason": Equal("ProviderSuccess"), - "Message": Equal("Provider ensured the dns record"), - "ObservedGeneration": Equal(dnsRecord.Generation), - })), - ) - g.Expect(dnsRecord.Finalizers).To(ContainElement(DNSRecordFinalizer)) - }, TestTimeoutMedium, time.Second).Should(Succeed()) - - Expect(k8sClient.Delete(ctx, managedZone)).To(Succeed()) - Eventually(func(g Gomega) { - err := k8sClient.Get(ctx, client.ObjectKeyFromObject(dnsRecord), dnsRecord) - g.Expect(err).NotTo(HaveOccurred()) - g.Expect(dnsRecord.Status.Conditions).To( - ContainElement(MatchFields(IgnoreExtras, Fields{ - "Type": Equal(string(v1alpha1.ConditionTypeReady)), - "Status": Equal(metav1.ConditionFalse), - "Reason": Equal("ManagedZoneError"), - "Message": Equal("The managedZone could not be loaded: ManagedZone.kuadrant.io \"mz-example-com\" not found"), - "ObservedGeneration": Equal(dnsRecord.Generation), - })), - ) - g.Expect(common.Owns(managedZone, dnsRecord)).To(BeTrue()) - g.Expect(dnsRecord.Finalizers).To(ContainElement(DNSRecordFinalizer)) - }, TestTimeoutMedium, time.Second).Should(Succeed()) - - err := k8sClient.Delete(ctx, dnsRecord) - Expect(err).ToNot(HaveOccurred()) - - Consistently(func(g Gomega, ctx context.Context) { - err := k8sClient.Get(ctx, client.ObjectKeyFromObject(dnsRecord), dnsRecord) - g.Expect(err).ToNot(HaveOccurred()) - }, TestTimeoutMedium, time.Second, ctx).Should(Succeed()) - }) It("can delete a record with an valid managed zone", func(ctx SpecContext) { dnsRecord = &v1alpha1.DNSRecord{ ObjectMeta: metav1.ObjectMeta{ diff --git a/internal/controller/managedzone_controller.go b/internal/controller/managedzone_controller.go index 00263977..fdd502f6 100644 --- a/internal/controller/managedzone_controller.go +++ b/internal/controller/managedzone_controller.go @@ -36,6 +36,7 @@ import ( externaldns "sigs.k8s.io/external-dns/endpoint" "github.com/kuadrant/dns-operator/api/v1alpha1" + "github.com/kuadrant/dns-operator/internal/common" "github.com/kuadrant/dns-operator/internal/metrics" "github.com/kuadrant/dns-operator/internal/provider" ) @@ -84,6 +85,15 @@ func (r *ManagedZoneReconciler) Reconcile(ctx context.Context, req ctrl.Request) logger.Error(err, "Failed to delete parent Zone NS Record") return ctrl.Result{}, err } + + if recordsExist, err := r.hasOwnedRecords(ctx, managedZone); err != nil { + logger.Error(err, "Failed to check owned records") + return ctrl.Result{}, err + } else if recordsExist { + logger.Info("ManagedZone deletion awaiting removal of owned DNS records") + return ctrl.Result{Requeue: true}, nil + } + if err := r.deleteManagedZone(ctx, managedZone); err != nil { logger.Error(err, "Failed to delete ManagedZone") return ctrl.Result{}, err @@ -252,7 +262,7 @@ func (r *ManagedZoneReconciler) deleteManagedZone(ctx context.Context, managedZo } err = dnsProvider.DeleteManagedZone(managedZone) if err != nil { - if strings.Contains(err.Error(), "was not found") || strings.Contains(err.Error(), "notFound") { + if strings.Contains(err.Error(), "not found") || strings.Contains(err.Error(), "notFound") { logger.Info("ManagedZone was not found, continuing") return nil } @@ -398,6 +408,20 @@ func (r *ManagedZoneReconciler) parentZoneNSRecordReady(ctx context.Context, man return nil } +func (r *ManagedZoneReconciler) hasOwnedRecords(ctx context.Context, zone *v1alpha1.ManagedZone) (bool, error) { + records := &v1alpha1.DNSRecordList{} + if err := r.List(ctx, records, client.InNamespace(zone.GetNamespace())); err != nil { + return false, err + } + + for _, record := range records.Items { + if common.Owns(zone, &record) { + return true, nil + } + } + return false, nil +} + // setManagedZoneCondition adds or updates a given condition in the ManagedZone status. func setManagedZoneCondition(managedZone *v1alpha1.ManagedZone, conditionType string, status metav1.ConditionStatus, reason, message string) { cond := metav1.Condition{ diff --git a/internal/controller/managedzone_controller_test.go b/internal/controller/managedzone_controller_test.go index 0937333d..8c6b1f47 100644 --- a/internal/controller/managedzone_controller_test.go +++ b/internal/controller/managedzone_controller_test.go @@ -38,6 +38,7 @@ var _ = Describe("ManagedZoneReconciler", func() { Context("testing ManagedZone controller", func() { var dnsProviderSecret *v1.Secret var managedZone *v1alpha1.ManagedZone + var dnsRecord *v1alpha1.DNSRecord var testNamespace string BeforeEach(func() { @@ -46,21 +47,41 @@ var _ = Describe("ManagedZoneReconciler", func() { dnsProviderSecret = testBuildInMemoryCredentialsSecret("inmemory-credentials", testNamespace) managedZone = testBuildManagedZone("mz-example-com", testNamespace, "example.com", dnsProviderSecret.Name) + dnsRecord = &v1alpha1.DNSRecord{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo.example.com", + Namespace: testNamespace, + }, + Spec: v1alpha1.DNSRecordSpec{ + OwnerID: "owner1", + RootHost: "foo.example.com", + ManagedZoneRef: &v1alpha1.ManagedZoneReference{ + Name: managedZone.Name, + }, + Endpoints: getDefaultTestEndpoints(), + }, + } + Expect(k8sClient.Create(ctx, dnsProviderSecret)).To(Succeed()) }) AfterEach(func() { + if dnsRecord != nil { + err := k8sClient.Delete(ctx, dnsRecord) + Expect(client.IgnoreNotFound(err)).ToNot(HaveOccurred()) + } + // Clean up managedZones mzList := &v1alpha1.ManagedZoneList{} err := k8sClient.List(ctx, mzList, client.InNamespace(testNamespace)) Expect(err).NotTo(HaveOccurred()) for _, mz := range mzList.Items { - err = k8sClient.Delete(ctx, &mz) + err = k8sClient.Delete(ctx, &mz, client.PropagationPolicy(metav1.DeletePropagationForeground)) Expect(client.IgnoreNotFound(err)).NotTo(HaveOccurred()) } if dnsProviderSecret != nil { - err := k8sClient.Delete(ctx, dnsProviderSecret) + err := k8sClient.Delete(ctx, dnsProviderSecret, client.PropagationPolicy(metav1.DeletePropagationForeground)) Expect(client.IgnoreNotFound(err)).ToNot(HaveOccurred()) } }) @@ -246,5 +267,50 @@ var _ = Describe("ManagedZoneReconciler", func() { }, TestTimeoutMedium, time.Second).Should(Succeed()) }) + It("Should block deletion of a managed zone when it still owns DNS Records", func(ctx SpecContext) { + Expect(k8sClient.Create(ctx, managedZone)).To(Succeed()) + Eventually(func(g Gomega) { + err := k8sClient.Get(ctx, client.ObjectKeyFromObject(managedZone), managedZone) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(managedZone.Status.Conditions).To( + ContainElement(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(string(v1alpha1.ConditionTypeReady)), + "Status": Equal(metav1.ConditionTrue), + })), + ) + g.Expect(managedZone.Finalizers).To(ContainElement(ManagedZoneFinalizer)) + }, TestTimeoutMedium, time.Second).Should(Succeed()) + + // create DNS Record + Expect(k8sClient.Create(ctx, dnsRecord)).To(Succeed()) + Eventually(func(g Gomega) { + err := k8sClient.Get(ctx, client.ObjectKeyFromObject(dnsRecord), dnsRecord) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(dnsRecord.Finalizers).To(ContainElement(DNSRecordFinalizer)) + }, TestTimeoutMedium, time.Second).Should(Succeed()) + + // Delete managed zone + Expect(k8sClient.Delete(ctx, managedZone)).To(Succeed()) + + // confirm DNS Record and managed zone have not deleted + Consistently(func(g Gomega) { + err := k8sClient.Get(ctx, client.ObjectKeyFromObject(dnsRecord), dnsRecord) + g.Expect(err).To(BeNil()) + + err = k8sClient.Get(ctx, client.ObjectKeyFromObject(managedZone), managedZone) + g.Expect(err).To(BeNil()) + }, TestTimeoutMedium, time.Second).Should(Succeed()) + + // delete the DNS Record + Expect(k8sClient.Delete(ctx, dnsRecord)).To(Succeed()) + + Eventually(func(g Gomega) { + err := k8sClient.Get(ctx, client.ObjectKeyFromObject(dnsRecord), dnsRecord) + g.Expect(errors.IsNotFound(err)).To(BeTrue()) + + err = k8sClient.Get(ctx, client.ObjectKeyFromObject(managedZone), managedZone) + g.Expect(errors.IsNotFound(err)).To(BeTrue()) + }, TestTimeoutMedium, time.Second).Should(Succeed()) + }) }) })