Skip to content

Commit

Permalink
DNS Records block MZ deletion
Browse files Browse the repository at this point in the history
  • Loading branch information
@philbrookes
philbrookes committed Jun 14, 2024
1 parent 67370a6 commit 805eef4
Show file tree
Hide file tree
Showing 6 changed files with 303 additions and 64 deletions.
39 changes: 39 additions & 0 deletions internal/common/helper.go
View file
Original file line number Diff line number Diff line change
@@ -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"
)
Expand Down Expand Up @@ -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
}
167 changes: 167 additions & 0 deletions internal/common/helper_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
}
}
2 changes: 1 addition & 1 deletion internal/controller/dnsrecord_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
63 changes: 3 additions & 60 deletions internal/controller/dnsrecord_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}
})
Expand Down Expand Up @@ -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{
Expand Down
26 changes: 25 additions & 1 deletion internal/controller/managedzone_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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{
Expand Down
Loading

0 comments on commit 805eef4

Please sign in to comment.