sotw dnspolicy: delete orphan records updates

sotw delete: updates
* Create link between policy and records using ownerrefs
* Update orphan record deletion
* go mod updaate (fix_policy_linking)

Signed-off-by: Michael Nairn <[email protected]>

controllers/dns_workflow.go

@@ -11,6 +11,9 @@ import (
 	"github.com/kuadrant/policy-machinery/machinery"
 
 	kuadrantdnsv1alpha1 "github.com/kuadrant/dns-operator/api/v1alpha1"
+
+	"github.com/kuadrant/kuadrant-operator/api/v1alpha1"
+	"github.com/kuadrant/kuadrant-operator/pkg/library/utils"
 )
 
 const (
@@ -61,3 +64,20 @@ func LinkListenerToDNSRecord(objs controller.Store) machinery.LinkFunc {
 		},
 	}
 }
+
+func LinkDNSPolicyToDNSRecord(objs controller.Store) machinery.LinkFunc {
+	policies := lo.Map(objs.FilterByGroupKind(v1alpha1.DNSPolicyGroupKind), controller.ObjectAs[*v1alpha1.DNSPolicy])
+
+	return machinery.LinkFunc{
+		From: v1alpha1.DNSPolicyGroupKind,
+		To:   DNSRecordGroupKind,
+		Func: func(child machinery.Object) []machinery.Object {
+			if dnsRecord, ok := child.(*controller.RuntimeObject).Object.(*kuadrantdnsv1alpha1.DNSRecord); ok {
+				return lo.FilterMap(policies, func(dnsPolicy *v1alpha1.DNSPolicy, _ int) (machinery.Object, bool) {
+					return dnsPolicy, utils.IsOwnedBy(dnsRecord, dnsPolicy)
+				})
+			}
+			return nil
+		},
+	}
+}

controllers/effective_dnspolicies_reconciler.go

@@ -2,14 +2,12 @@ package controllers
 
 import (
 	"context"
-	"fmt"
 	"sync"
 
 	"github.com/samber/lo"
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/client-go/dynamic"
 
 	kuadrantdnsv1alpha1 "github.com/kuadrant/dns-operator/api/v1alpha1"
@@ -40,94 +38,59 @@ func (r *EffectiveDNSPoliciesReconciler) Subscription() controller.Subscription
 
 func (r *EffectiveDNSPoliciesReconciler) reconcile(ctx context.Context, _ []controller.ResourceEvent, topology *machinery.Topology, _ error, _ *sync.Map) error {
 	//ToDo Implement DNSRecord reconcile
-	return r.deleteOrphanDNSPolicyRecords(ctx, topology)
+	return r.deleteOrphanDNSRecords(ctx, topology)
 }
 
-// deleteOrphanDNSPolicyRecords deletes any DNSRecord resources created by a DNSPolicy but no longer have a valid path in the topology to that policy.
-func (r *EffectiveDNSPoliciesReconciler) deleteOrphanDNSPolicyRecords(ctx context.Context, topology *machinery.Topology) error {
-	logger := controller.LoggerFromContext(ctx).WithName("effectiveDNSPoliciesReconciler")
-	logger.Info("deleting orphan policy DNS records")
+// deleteOrphanDNSRecords deletes any DNSRecord resources that exist in the topology but have no parent targettable, policy or path back to the policy.
+func (r *EffectiveDNSPoliciesReconciler) deleteOrphanDNSRecords(ctx context.Context, topology *machinery.Topology) error {
+	logger := controller.LoggerFromContext(ctx).WithName("EffectiveDNSPoliciesReconciler")
 
-	orphanRecords := lo.FilterMap(topology.Objects().Items(), func(item machinery.Object, _ int) (machinery.Object, bool) {
+	orphanRecords := lo.Filter(topology.Objects().Items(), func(item machinery.Object, _ int) bool {
 		if item.GroupVersionKind().GroupKind() == DNSRecordGroupKind {
-			policyOwnerRef := getObjectPolicyOwnerReference(item, kuadrantv1alpha1.DNSPolicyGroupKind)
+			pTargettables := topology.Targetables().Parents(item)
+			pPolicies := topology.Policies().Parents(item)
+
+			if logger.V(1).Enabled() {
+				pPoliciesLocs := lo.Map(pPolicies, func(item machinery.Policy, _ int) string {
+					return item.GetLocator()
+				})
+				pTargetablesLocs := lo.Map(pTargettables, func(item machinery.Targetable, _ int) string {
+					return item.GetLocator()
+				})
+				logger.V(1).Info("dns record parents", "record", item.GetLocator(), "targetables", pTargetablesLocs, "polices", pPoliciesLocs)
+			}
+
+			//Target removed from topology
+			if len(pTargettables) == 0 {
+				return true
+			}
 
-			// Ignore all DNSRecords that weren't created by a DNSPolicy
-			if policyOwnerRef == nil {
-				return nil, false
+			//Policy removed from topology
+			if len(pPolicies) == 0 {
+				return true
 			}
 
-			// Any DNSRecord that does not have a link in the topology back to its owner DNSPolicy should be removed
-			if len(topology.All().Paths(policyOwnerRef, item)) == 0 {
-				logger.Info(fmt.Sprintf("dnsrecord object is no longer linked to it's policy owner, dnsrecord: %v, policy: %v", item.GetLocator(), policyOwnerRef.GetLocator()))
-				return item, true
+			//Policy target ref changes
+			if len(topology.All().Paths(pPolicies[0], item)) == 1 { //There will always be at least one DNSPolicy -> DNSRecord
+				return true
 			}
+
+			return false
 		}
-		return nil, false
+		return false
 	})
 
-	for i := range orphanRecords {
-		record := orphanRecords[i].(*controller.RuntimeObject).Object.(*kuadrantdnsv1alpha1.DNSRecord)
+	for _, obj := range orphanRecords {
+		record := obj.(*controller.RuntimeObject).Object.(*kuadrantdnsv1alpha1.DNSRecord)
 		if record.GetDeletionTimestamp() != nil {
 			continue
 		}
-		logger.Info(fmt.Sprintf("deleting DNSRecord: %v", orphanRecords[i].GetLocator()))
+		logger.Info("deleting orphan dns record", "record", obj.GetLocator())
 		resource := r.client.Resource(DNSRecordResource).Namespace(record.GetNamespace())
 		if err := resource.Delete(ctx, record.GetName(), metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) {
-			logger.Error(err, "failed to delete DNSRecord")
+			logger.Error(err, "failed to delete DNSRecord", "record", obj.GetLocator())
 		}
 	}
 
 	return nil
 }
-
-type PolicyOwnerReference struct {
-	metav1.OwnerReference
-	PolicyNamespace string
-	GVK             schema.GroupVersionKind
-}
-
-func (o *PolicyOwnerReference) SetGroupVersionKind(gvk schema.GroupVersionKind) {
-	o.GVK = gvk
-}
-
-func (o *PolicyOwnerReference) GroupVersionKind() schema.GroupVersionKind {
-	return o.GVK
-}
-
-func (o *PolicyOwnerReference) GetNamespace() string {
-	return o.PolicyNamespace
-}
-
-func (o *PolicyOwnerReference) GetName() string {
-	return o.OwnerReference.Name
-}
-
-func (o *PolicyOwnerReference) GetLocator() string {
-	return machinery.LocatorFromObject(o)
-}
-
-var _ machinery.PolicyTargetReference = &PolicyOwnerReference{}
-
-func getObjectPolicyOwnerReference(item machinery.Object, gk schema.GroupKind) *PolicyOwnerReference {
-	var ownerRef *PolicyOwnerReference
-	for _, o := range item.(*controller.RuntimeObject).GetOwnerReferences() {
-		oGV, err := schema.ParseGroupVersion(o.APIVersion)
-		if err != nil {
-			continue
-		}
-
-		if oGV.Group == gk.Group && o.Kind == gk.Kind {
-			ownerRef = &PolicyOwnerReference{
-				OwnerReference:  o,
-				PolicyNamespace: item.GetNamespace(),
-				GVK: schema.GroupVersionKind{
-					Group: gk.Group,
-					Kind:  gk.Kind,
-				},
-			}
-			break
-		}
-	}
-	return ownerRef
-}

controllers/state_of_the_world.go

@@ -311,6 +311,7 @@ func (b *BootOptionsBuilder) getDNSOperatorOptions() []controller.ControllerOpti
 		),
 		controller.WithObjectLinks(
 			LinkListenerToDNSRecord,
+			LinkDNSPolicyToDNSRecord,
 		),
 	)
 

go.mod

@@ -14,7 +14,7 @@ require (
 	github.com/kuadrant/authorino-operator v0.11.1
 	github.com/kuadrant/dns-operator v0.0.0-20241018131559-f2ce8b6aaaef
 	github.com/kuadrant/limitador-operator v0.9.0
-	github.com/kuadrant/policy-machinery v0.5.1-0.20241014081934-0c6af68f7cfe
+	github.com/kuadrant/policy-machinery v0.5.1-0.20241020183426-2dc665c31409
 	github.com/martinlindhe/base36 v1.1.1
 	github.com/onsi/ginkgo/v2 v2.20.2
 	github.com/onsi/gomega v1.34.1

go.sum

@@ -262,10 +262,8 @@ github.com/kuadrant/dns-operator v0.0.0-20241018131559-f2ce8b6aaaef h1:6P2pC1kOP
 github.com/kuadrant/dns-operator v0.0.0-20241018131559-f2ce8b6aaaef/go.mod h1:LGG4R3KEz93Ep0CV1/tziCmRk+VtojWUHR9mXkOHZks=
 github.com/kuadrant/limitador-operator v0.9.0 h1:hTQ6CFPayf/sL7cIzwWjCoU8uTn6fzWdsJgKbDlnFts=
 github.com/kuadrant/limitador-operator v0.9.0/go.mod h1:DQOlg9qFOcnWPrwO529JRCMLLOEXJQxkmOes952S/Hw=
-github.com/kuadrant/policy-machinery v0.5.0 h1:hTllNYswhEOFrS/uj8kY4a4wq2W1xL2hagHeftn9TTY=
-github.com/kuadrant/policy-machinery v0.5.0/go.mod h1:ZV4xS0CCxPgu/Xg6gz+YUaS9zqEXKOiAj33bZ67B6Lo=
-github.com/kuadrant/policy-machinery v0.5.1-0.20241014081934-0c6af68f7cfe h1:7967AfN8dtv94BAKAFTwbKXXeiZuTGehBh/g8eHNIyQ=
-github.com/kuadrant/policy-machinery v0.5.1-0.20241014081934-0c6af68f7cfe/go.mod h1:ZV4xS0CCxPgu/Xg6gz+YUaS9zqEXKOiAj33bZ67B6Lo=
+github.com/kuadrant/policy-machinery v0.5.1-0.20241020183426-2dc665c31409 h1:fGHNfFjsDDRNUApVgHz6+ZCTMPFHl6OWDhK/BtqeaOY=
+github.com/kuadrant/policy-machinery v0.5.1-0.20241020183426-2dc665c31409/go.mod h1:ZV4xS0CCxPgu/Xg6gz+YUaS9zqEXKOiAj33bZ67B6Lo=
 github.com/kylelemons/godebug v1.1.0 h1:RPNrshWIDI6G2gRW9EHilWtl7Z6Sb1BR0xunSBf0SNc=
 github.com/kylelemons/godebug v1.1.0/go.mod h1:9/0rRGxNHcop5bhtWyNeEfOS8JIWk580+fNqagV/RAw=
 github.com/lann/builder v0.0.0-20180802200727-47ae307949d0 h1:SOEGU9fKiNWd/HOJuq6+3iTQz8KNCLtVX6idSoTLdUw=

pkg/library/utils/k8s_utils.go

@@ -86,6 +86,10 @@ func StatusConditionsMarshalJSON(input []metav1.Condition) ([]byte, error) {
 // The version of the owner reference is not checked in this implementation.
 // Returns true if the owned object is owned by the owner object, false otherwise.
 func IsOwnedBy(owned, owner client.Object) bool {
+	if owned.GetNamespace() != owner.GetNamespace() {
+		return false
+	}
+
 	ownerGVK := owner.GetObjectKind().GroupVersionKind()
 
 	for _, o := range owned.GetOwnerReferences() {

pkg/library/utils/k8s_utils_test.go

@@ -466,6 +466,58 @@ func TestIsOwnedBy(t *testing.T) {
 			},
 			expected: false,
 		},
+		{
+			name: "when owned object has owner reference and in same namespace then return true",
+			owned: &corev1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: "ns1",
+					OwnerReferences: []metav1.OwnerReference{
+						{
+							APIVersion: "v1",
+							Kind:       "Deployment",
+							Name:       "my-deployment",
+						},
+					},
+				},
+			},
+			owner: &appsv1.Deployment{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "Deployment",
+					APIVersion: "v1",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "my-deployment",
+					Namespace: "ns1",
+				},
+			},
+			expected: true,
+		},
+		{
+			name: "when owned object has owner reference but in different namespace then return false",
+			owned: &corev1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: "ns1",
+					OwnerReferences: []metav1.OwnerReference{
+						{
+							APIVersion: "v1",
+							Kind:       "Deployment",
+							Name:       "my-deployment",
+						},
+					},
+				},
+			},
+			owner: &appsv1.Deployment{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "Deployment",
+					APIVersion: "v1",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "my-deployment",
+					Namespace: "ns2",
+				},
+			},
+			expected: false,
+		},
 	}
 
 	for _, tc := range testCases {