From ccf9c37476f24df4d96ea1a2f71665786ae48561 Mon Sep 17 00:00:00 2001 From: Michael Nairn Date: Fri, 18 Oct 2024 09:44:04 +0100 Subject: [PATCH] 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 --- controllers/dns_workflow.go | 20 ++++ .../effective_dnspolicies_reconciler.go | 107 ++++++------------ controllers/state_of_the_world.go | 1 + go.mod | 2 +- go.sum | 6 +- pkg/library/utils/k8s_utils.go | 4 + pkg/library/utils/k8s_utils_test.go | 52 +++++++++ 7 files changed, 115 insertions(+), 77 deletions(-) diff --git a/controllers/dns_workflow.go b/controllers/dns_workflow.go index d0a8205cb..bd5bf26da 100644 --- a/controllers/dns_workflow.go +++ b/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 + }, + } +} diff --git a/controllers/effective_dnspolicies_reconciler.go b/controllers/effective_dnspolicies_reconciler.go index 5fba1b911..59c4cb612 100644 --- a/controllers/effective_dnspolicies_reconciler.go +++ b/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 -} diff --git a/controllers/state_of_the_world.go b/controllers/state_of_the_world.go index f6ee58c49..11111d1ed 100644 --- a/controllers/state_of_the_world.go +++ b/controllers/state_of_the_world.go @@ -311,6 +311,7 @@ func (b *BootOptionsBuilder) getDNSOperatorOptions() []controller.ControllerOpti ), controller.WithObjectLinks( LinkListenerToDNSRecord, + LinkDNSPolicyToDNSRecord, ), ) diff --git a/go.mod b/go.mod index 486141e3e..445222eab 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index 937d36b6d..b7f27b56d 100644 --- a/go.sum +++ b/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= diff --git a/pkg/library/utils/k8s_utils.go b/pkg/library/utils/k8s_utils.go index 6c42e85ff..192aa8676 100644 --- a/pkg/library/utils/k8s_utils.go +++ b/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() { diff --git a/pkg/library/utils/k8s_utils_test.go b/pkg/library/utils/k8s_utils_test.go index fcc291ead..1b64cdae9 100644 --- a/pkg/library/utils/k8s_utils_test.go +++ b/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 {