Merge pull request #414 from containeroo/fix/ingress

fix(ingress): delete orphaned dns records

Author: rxbn

internal/controller/ingress_controller.go

@@ -79,6 +79,16 @@ func (r *IngressReconciler) reconcileIngress(ctx context.Context, ingress *netwo
 
 	annotations := ingress.GetAnnotations()
 
+	if annotations["cloudflare-operator.io/content"] == "" && annotations["cloudflare-operator.io/ip-ref"] == "" {
+		for _, record := range dnsRecords.Items {
+			if err := r.Delete(ctx, &record); err != nil {
+				log.Error(err, "Failed to delete DNSRecord", "name", record.Name)
+			}
+		}
+
+		return ctrl.Result{}, nil
+	}
+
 	dnsRecordSpec := r.parseAnnotations(annotations)
 	existingRecords := make(map[string]cloudflareoperatoriov1.DNSRecord)
 	for _, record := range dnsRecords.Items {

internal/controller/ingress_controller_test.go

@@ -25,6 +25,7 @@ import (
 	. "github.com/onsi/gomega"
 
 	networkingv1 "k8s.io/api/networking/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
@@ -77,6 +78,23 @@ func TestIngressReconciler_reconcileIngress(t *testing.T) {
 		g.Expect(dnsRecord.Spec).To(HaveField("Content", Equal("1.1.1.1")))
 	})
 
+	t.Run("change dnsrecord spec when annotations change", func(t *testing.T) {
+		g := NewWithT(t)
+		ingress.Annotations = map[string]string{
+			"cloudflare-operator.io/content": "2.2.2.2",
+		}
+
+		_, err := r.reconcileIngress(context.TODO(), ingress)
+		g.Expect(err).NotTo(HaveOccurred())
+
+		dnsRecord := &cloudflareoperatoriov1.DNSRecord{}
+		err = r.Get(context.TODO(), client.ObjectKey{Namespace: "default", Name: "ingtest-containeroo-test-org"}, dnsRecord)
+		g.Expect(err).NotTo(HaveOccurred())
+
+		g.Expect(dnsRecord.Spec).To(HaveField("Name", Equal("ingtest.containeroo-test.org")))
+		g.Expect(dnsRecord.Spec).To(HaveField("Content", Equal("2.2.2.2")))
+	})
+
 	t.Run("reconcile ingress wildcard", func(t *testing.T) {
 		g := NewWithT(t)
 		ingress.Spec = networkingv1.IngressSpec{
@@ -92,7 +110,19 @@ func TestIngressReconciler_reconcileIngress(t *testing.T) {
 		g.Expect(err).NotTo(HaveOccurred())
 
 		g.Expect(dnsRecord.Spec).To(HaveField("Name", Equal("*.containeroo-test.org")))
-		g.Expect(dnsRecord.Spec).To(HaveField("Content", Equal("1.1.1.1")))
+		g.Expect(dnsRecord.Spec).To(HaveField("Content", Equal("2.2.2.2")))
+	})
+
+	t.Run("remove dnsrecord when annotations are absent", func(t *testing.T) {
+		g := NewWithT(t)
+		ingress.Annotations = map[string]string{}
+
+		_, err := r.reconcileIngress(context.TODO(), ingress)
+		g.Expect(err).NotTo(HaveOccurred())
+
+		dnsRecord := &cloudflareoperatoriov1.DNSRecord{}
+		err = r.Get(context.TODO(), client.ObjectKey{Namespace: "default", Name: "wildcard-containeroo-test-org"}, dnsRecord)
+		g.Expect(apierrors.IsNotFound(err)).To(BeTrue())
 	})
 
 	t.Run("ingress annotation parsing", func(t *testing.T) {

internal/predicates/predicates.go

@@ -18,6 +18,7 @@ package predicates
 
 import (
 	"reflect"
+	"slices"
 
 	cloudflareoperatoriov1 "github.com/containeroo/cloudflare-operator/api/v1"
 	networkingv1 "k8s.io/api/networking/v1"
@@ -46,13 +47,28 @@ func (DNSFromIngressPredicate) Update(e event.UpdateEvent) bool {
 	oldAnnotations := e.ObjectOld.GetAnnotations()
 	newAnnotations := e.ObjectNew.GetAnnotations()
 
+	oldObjectHadRequiredAnnotations := ingressHasRequiredAnnotations(oldAnnotations)
+	newObjectHasRequiredAnnotations := ingressHasRequiredAnnotations(newAnnotations)
+
 	oldObj := e.ObjectOld.(*networkingv1.Ingress)
 	newObj := e.ObjectNew.(*networkingv1.Ingress)
 
 	annotationsChanged := !reflect.DeepEqual(oldAnnotations, newAnnotations)
-	rulesChanged := !reflect.DeepEqual(oldObj.Spec.Rules, newObj.Spec.Rules)
+	oldHosts := []string{}
+	for _, rule := range oldObj.Spec.Rules {
+		oldHosts = append(oldHosts, rule.Host)
+	}
+	newHosts := []string{}
+	for _, rule := range newObj.Spec.Rules {
+		newHosts = append(newHosts, rule.Host)
+	}
+
+	slices.Sort(oldHosts)
+	slices.Sort(newHosts)
+
+	hostsChanged := !reflect.DeepEqual(oldHosts, newHosts)
 
-	return ingressHasRequiredAnnotations(newAnnotations) && (annotationsChanged || rulesChanged)
+	return (newObjectHasRequiredAnnotations && (annotationsChanged || hostsChanged)) || (oldObjectHadRequiredAnnotations && !newObjectHasRequiredAnnotations)
 }
 
 func (DNSFromIngressPredicate) Delete(e event.DeleteEvent) bool {

test/e2e/e2e_test.go

@@ -202,4 +202,24 @@ var _ = Describe("controller", Ordered, func() {
 		Eventually(utils.VerifyDNSRecordContent, time.Minute, time.Second).
 			WithArguments("ingress-containeroo-test-org", "145.145.145.145").Should(Succeed())
 	})
+
+	It("should recreate dnsrecord when it gets deleted", func() {
+		cmd := exec.Command("kubectl", "-n", namespace, "delete", "dnsrecord", "ingress-containeroo-test-org")
+		_, err := utils.Run(cmd)
+		Expect(err).NotTo(HaveOccurred())
+
+		Eventually(utils.VerifyObjectReady, time.Minute, time.Second).
+			WithArguments("dnsrecord", "ingress-containeroo-test-org").Should(Succeed())
+	})
+
+	It("should delete dnsrecord when ingress annotations are absent", func() {
+		cmd := exec.Command(
+			"kubectl", "-n", namespace, "patch", "ingress", "ingress-sample",
+			"--type=json", "-p", `[{"op": "remove", "path": "/metadata/annotations"}]`)
+		_, err := utils.Run(cmd)
+		Expect(err).NotTo(HaveOccurred())
+
+		Eventually(utils.VerifyDNSRecordAbsent, time.Minute, time.Second).
+			WithArguments("ingress-containeroo-test-org").Should(Succeed())
+	})
 })

test/utils/utils.go

@@ -148,3 +148,22 @@ func VerifyDNSRecordContent(objName, expectedContent string) error {
 	}
 	return nil
 }
+
+func VerifyDNSRecordAbsent(objName string) error {
+	cmd := exec.Command(
+		"kubectl",
+		"get",
+		"dnsrecord",
+		objName,
+		"-n",
+		"cloudflare-operator-system",
+	)
+	status, err := Run(cmd)
+	if err != nil {
+		if strings.Contains(string(status), "NotFound") {
+			return nil
+		}
+		return err
+	}
+	return fmt.Errorf("dnsrecord %s still exists", objName)
+}