sotw: dnspolicy #937

mikenairn · 2024-10-14T08:45:31Z

Add basic setup for DNSPolicy state of the world tasks, dnsrecord types, watcher and linker function (Listener -> DNSRecord)

sotw: dnspolicy delete orphan records

Move all logic to delete orphan dnsrecord resources for a DNSPolicy to the sotw reconciler and based all decisions on the current topology.

Orphan record is one that no longer has a valid path between it's owner DNSPolicy and itself in the topology. This covers the following scenarios:

Listener is deleted from the Gateway
Gateway is deleted
Policy is deleted(K8s will also deal with this due to the owner relationship)
Policy ref is changed

Does not deal with the removal of records based on the state of the gateway, these aren't considered "orphans" and will be dealt with separately.

Requires: Kuadrant/policy-machinery#42

mikenairn · 2024-10-14T09:26:10Z

controllers/dns_workflow.go

+//+kubebuilder:rbac:groups=kuadrant.io,resources=dnspolicies/finalizers,verbs=update
+
+//+kubebuilder:rbac:groups=kuadrant.io,resources=dnsrecords,verbs=get;list;watch;create;update;patch;delete
+//+kubebuilder:rbac:groups=kuadrant.io,resources=dnsrecords/status,verbs=get


Moved all these to here for now, but will need to review these permissions again. I'm not sure why we need the namespace permissions for instance.

mikenairn · 2024-10-14T09:29:56Z

controllers/effective_dnspolicies_reconciler.go

+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
+}


This should all go somewhere else, state_of_the_world.go maybe? This also, if we agree it seems like a reasonable way to link an OwnerReference to a Policy could be reused by at least the TLSPolicy.

go.mod

controllers/effective_dnspolicies_reconciler.go

controllers/dns_workflow.go

maleck13 · 2024-10-14T13:42:04Z

controllers/dnspolicies_validator.go

+}
+
+func (r *DNSPoliciesValidator) validate(_ context.Context, _ []controller.ResourceEvent, topology *machinery.Topology, _ error, state *sync.Map) error {
+	policies := topology.Policies().Items(func(o machinery.Object) bool {


night be a nice option in the future to have a topology.Policies().ForGVK(...) ?

controllers/dnspolicies_validator.go

maleck13 · 2024-10-14T14:44:08Z

controllers/effective_dnspolicies_reconciler.go

+		return nil, false
+	})
+
+	for i := range orphanRecords {


open question. If we are using lo should we not end up using it for slices also? https://github.com/samber/lo?tab=readme-ov-file#foreach @guicassolato ?

maleck13 · 2024-10-14T14:52:25Z

controllers/effective_dnspolicies_reconciler.go

+	var ownerRef *PolicyOwnerReference
+	for _, o := range item.(*controller.RuntimeObject).GetOwnerReferences() {
+		oGV, err := schema.ParseGroupVersion(o.APIVersion)
+		if err != nil {


guessing we skip the error here as it would be none kuadrant owner ref that would cause this error?

maleck13 · 2024-10-14T14:58:27Z

controllers/state_of_the_world.go

+
+	return opts
+}
+
 func (b *BootOptionsBuilder) Reconciler() controller.ReconcileFunc {
 	mainWorkflow := &controller.Workflow{
 		Precondition: initWorkflow(b.client).Run,
 		Tasks: []controller.ReconcileFunc{


are each of these dispatched in their own routine?

maleck13

These changes all look pretty good to me. Couple of minor comments but nothing major.

Add basic setup for DNSPolicy state of the world tasks, dnsrecord types, watcher and linker function (Listener -> DNSRecord) Signed-off-by: Michael Nairn <mnairn@redhat.com>

Move all logic to delete orphan dnsrecord resources for a DNSPolicy to the sotw reconciler and based all decisions on the current topology. Orphan record is one that no longer has a valid path between it's owner DNSPolicy and itself in the topology. This covers the following scenarios: * Listener is deleted from the Gateway * Gateway is deleted * Policy is deleted(K8s will also deal with this due to the owner relationship) * Policy ref is changed Does not deal with the removal of records based on the state of the gateway. Signed-off-by: Michael Nairn <mnairn@redhat.com>

controllers/dns_workflow.go

controllers/effective_dnspolicies_reconciler.go

Boomatang

I have removed so of my previous comments. After looking at this a second time last night, I had a misunderstanding of what was going on here. There is something going on here that seems out of place I have quite point my finger on it yet.

Boomatang · 2024-10-17T16:52:07Z

controllers/state_of_the_world.go

+func (b *BootOptionsBuilder) getDNSOperatorOptions() []controller.ControllerOption {
+	var opts []controller.ControllerOption
+	opts = append(opts,
+		controller.WithRunnable("dnsrecord watcher", controller.Watch(&kuadrantdnsv1alpha1.DNSRecord{}, DNSRecordResource, metav1.NamespaceAll)),


I think this needs address as it brings in results that are not created by kuadrant and kuadrant has no right to know about.

Yes, so i actually wasn't aware until you commented and i looked more closely at the controller code that there was a way to filter out the objects that get added to the topology. This is definitely something i think we should do as we don't want to interfere with resources that weren't created by a policy as you correctly pointed out.

The only way i see to do that with the current implementation of the controller is by using a label filter, and ensuring that all resources we create on behalf of the kuadrant operator are consistently labelled. I had a go at that here 8e1afd2.

If this is the approach we want to take, i think we should do it for all resources that we create on behalf of any policy. I noticed for instance TLS Certificates and Issuers that are not created by a TLSPolicy are also added to the topoloy.

@guicassolato @KevFan @Boomatang wdyt about common labelling for all created resources?

* Update dns policy validator in prepration for status updates, adds correct errors for acceptance. * Add common labels that get applied to all dnsrecord resources created by the kuadrant operator * Add filter to topology for dnsrecords to only add records that contain the correct label. * Adds predicates to only trigger on events to owned policies (might be pointless) Signed-off-by: Michael Nairn <mnairn@redhat.com>

* go mod updaate (main) Signed-off-by: Michael Nairn <mnairn@redhat.com>

mikenairn · 2024-10-18T08:46:54Z

controllers/common.go

+		"app.kubernetes.io/name":       KuadrantAppName,
+		"app.kubernetes.io/part-of":    KuadrantAppName,
+	}
+}


Copied the layout and labels from https://github.com/Kuadrant/kuadrant-operator/blob/main/pkg/openshift/consoleplugin/common.go

mikenairn · 2024-10-18T09:01:50Z

controllers/state_of_the_world.go

+		controller.WithRunnable("dnsrecord watcher", controller.Watch(
+			&kuadrantdnsv1alpha1.DNSRecord{}, DNSRecordResource, metav1.NamespaceAll,
+			controller.FilterResourcesByLabel[*kuadrantdnsv1alpha1.DNSRecord](fmt.Sprintf("%s=%s", AppLabelKey, AppLabelValue)),
+			controller.WithPredicates(ctrlruntimepredicate.NewTypedPredicateFuncs(isDNSRecordOwnedByDNSPolicy)))),


Added this predicate based on what had been added for TLS but I'm unsure of its usefulness when we are using a label filter. The label filter gets added as a predicate internally anyway. I'm thinking we can probably just drop it entirely and rely solely on the label filter.

mikenairn force-pushed the sotw_dnspolicy branch 2 times, most recently from d558ea1 to 18f3056 Compare October 14, 2024 09:16

mikenairn commented Oct 14, 2024

View reviewed changes

go.mod Outdated Show resolved Hide resolved

mikenairn commented Oct 14, 2024

View reviewed changes

controllers/effective_dnspolicies_reconciler.go Outdated Show resolved Hide resolved

maleck13 reviewed Oct 14, 2024

View reviewed changes

controllers/dns_workflow.go Show resolved Hide resolved

maleck13 reviewed Oct 14, 2024

View reviewed changes

controllers/dns_workflow.go Outdated Show resolved Hide resolved

maleck13 reviewed Oct 14, 2024

View reviewed changes

mikenairn commented Oct 14, 2024

View reviewed changes

controllers/dnspolicies_validator.go Show resolved Hide resolved

maleck13 reviewed Oct 14, 2024

View reviewed changes

mikenairn force-pushed the sotw_dnspolicy branch from 18f3056 to 4c58b9a Compare October 16, 2024 12:34

mikenairn added 2 commits October 16, 2024 13:57

sotw: dnspolicy init

26d3a47

Add basic setup for DNSPolicy state of the world tasks, dnsrecord types, watcher and linker function (Listener -> DNSRecord) Signed-off-by: Michael Nairn <mnairn@redhat.com>

mikenairn force-pushed the sotw_dnspolicy branch from 4c58b9a to 76839c3 Compare October 16, 2024 12:57

Boomatang reviewed Oct 17, 2024

View reviewed changes

controllers/dns_workflow.go Outdated Show resolved Hide resolved

Boomatang reviewed Oct 17, 2024

View reviewed changes

controllers/effective_dnspolicies_reconciler.go Outdated Show resolved Hide resolved

Boomatang reviewed Oct 17, 2024

View reviewed changes

mikenairn force-pushed the sotw_dnspolicy branch 2 times, most recently from 45d301e to 76839c3 Compare October 18, 2024 07:25

mikenairn added 2 commits October 18, 2024 09:42

sotw dnspolicy: delete orphan records updates

fb6c6a3

* go mod updaate (main) Signed-off-by: Michael Nairn <mnairn@redhat.com>

mikenairn changed the title ~~Sotw dnspolicy~~ sotw: dnspolicy Oct 18, 2024

mikenairn commented Oct 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sotw: dnspolicy #937

sotw: dnspolicy #937

mikenairn commented Oct 14, 2024 •

edited

Loading

mikenairn Oct 14, 2024

mikenairn Oct 14, 2024

maleck13 Oct 14, 2024

maleck13 Oct 14, 2024

maleck13 Oct 14, 2024

maleck13 Oct 14, 2024

maleck13 left a comment

Boomatang left a comment •

edited

Loading

Boomatang Oct 17, 2024

mikenairn Oct 18, 2024 •

edited

Loading

mikenairn Oct 18, 2024

mikenairn Oct 18, 2024

sotw: dnspolicy #937

Are you sure you want to change the base?

sotw: dnspolicy #937

Conversation

mikenairn commented Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maleck13 left a comment

Choose a reason for hiding this comment

Boomatang left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikenairn Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikenairn commented Oct 14, 2024 •

edited

Loading

Boomatang left a comment •

edited

Loading

mikenairn Oct 18, 2024 •

edited

Loading