Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

sotw: dnspolicy #937

Merged
merged 6 commits into from
Oct 25, 2024
Merged

sotw: dnspolicy #937

merged 6 commits into from
Oct 25, 2024

Conversation

mikenairn
Copy link
Member

@mikenairn mikenairn commented Oct 14, 2024 •
edited
Loading

Adds state of the world tasks for DNSPolicy.

  • Init
  • DNSPolicy Validation (DNSPoliciesValidator)
  • Policy Reconciliation (EffectiveDNSPoliciesReconciler)
    • Reconcile resources
    • Orphan record deletion
  • Status Updater (DNSPolicyStatusUpdater)
  • Cleanup old controller files (Left as is for easier reviews, will do this in a separate PR after this one with no logic changes)

closes #818 #823

@mikenairn mikenairn force-pushed the sotw_dnspolicy branch 2 times, most recently from d558ea1 to 18f3056 Compare October 14, 2024 09:16
mikenairn
mikenairn commented Oct 14, 2024
View reviewed changes
go.mod Outdated Show resolved Hide resolved
maleck13
maleck13 reviewed Oct 14, 2024
View reviewed changes
controllers/dnspolicies_validator.go Outdated
}

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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

maleck13
maleck13 reviewed Oct 14, 2024
View reviewed changes
controllers/effective_dnspolicies_reconciler.go Outdated
return nil, false
})

for i := range orphanRecords {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
maleck13 reviewed Oct 14, 2024
View reviewed changes
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{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are each of these dispatched in their own routine?

maleck13
maleck13 reviewed Oct 14, 2024
View reviewed changes
Copy link
Collaborator

@maleck13 maleck13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mikenairn mikenairn force-pushed the sotw_dnspolicy branch 2 times, most recently from 4c58b9a to 76839c3 Compare October 16, 2024 12:57
Boomatang
Boomatang reviewed Oct 17, 2024
View reviewed changes
Copy link
Contributor

@Boomatang Boomatang left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

controllers/state_of_the_world.go Outdated Show resolved Hide resolved
@mikenairn mikenairn force-pushed the sotw_dnspolicy branch 2 times, most recently from 45d301e to 76839c3 Compare October 18, 2024 07:25
@mikenairn mikenairn changed the title Sotw dnspolicy sotw: dnspolicy Oct 18, 2024
mikenairn
mikenairn commented Oct 18, 2024
View reviewed changes
controllers/common.go
"app.kubernetes.io/name": KuadrantAppName,
"app.kubernetes.io/part-of": KuadrantAppName,
}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@guicassolato guicassolato Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been adding a few common vars and helper funcs for things like labeling internal resources we create, etc, including one to identify the resource as "managed-by" kuadrant, here:

kuadrant-operator/controllers/state_of_the_world.go

Line 45 in b6494ca

kuadrantManagedLabelKey = "kuadrant.io/managed"

(and also at the top of specific workflow files, when the var is not just as common as these ones.)

Happy to move those here once this is merged, if you believe it would help having them all in one place rather than separated by area.

@mikenairn mikenairn force-pushed the sotw_dnspolicy branch 6 times, most recently from b55dbdf to c6fbbcf Compare October 22, 2024 10:18
maleck13
maleck13 approved these changes Oct 23, 2024
View reviewed changes
Copy link
Collaborator

@maleck13 maleck13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few minor things. But generally changes look ok and approachable. Will try it out locally but happy to approve

@maleck13
Copy link
Collaborator

maleck13 commented Oct 24, 2024 •
edited
Loading

@mikenairn I did a bunch of addiitonal manual tests with multiple gateways, multiple httproutes, deleting httproutes, deleting gateways, no credentials etc all worked as expected

👍
suggest we merge this ASAP and any issues can be done in follow ups?
/lgtm

KevFan
KevFan approved these changes Oct 24, 2024
View reviewed changes
Copy link
Contributor

@KevFan KevFan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

@mikenairn mikenairn force-pushed the sotw_dnspolicy branch 2 times, most recently from 4191c5c to e5393eb Compare October 24, 2024 15:14
@mikenairn
sotw: dnspolicy init
50884df 
Add basic setup for DNSPolicy state of the world tasks, dnsrecord types,
watcher and linker function (Listener -> DNSRecord)

* Update dns policy validator in preparation 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

Signed-off-by: Michael Nairn <mnairn@redhat.com>
@mikenairn
sotw: dnspolicy delete orphan records
d1120cd 
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>
@mikenairn
sotw dnspolicy: status
90a8d03 
Signed-off-by: Michael Nairn <mnairn@redhat.com>
@mikenairn
sotw dnspolicy: dnspolicies reconciliation
b40fe8b 
Signed-off-by: Michael Nairn <mnairn@redhat.com>
@mikenairn
sotw dnspolicy: cross cutting updates
46f5400 
* Add dnsPolicyTypeFilterFunc for reuse across dns policy tasks
* Add Validate method to DNSPolicy
* Add context specific errors to state during reconciliation and append
them to the enforced message on status update.

Signed-off-by: Michael Nairn <mnairn@redhat.com>
@mikenairn
Bump policy-machinery v0.6.1
2469b62 
Requires this fix Kuadrant/policy-machinery#45

Signed-off-by: Michael Nairn <mnairn@redhat.com>
@mikenairn mikenairn mentioned this pull request Oct 25, 2024
Closed
@mikenairn mikenairn merged commit 334ece5 into Kuadrant:main Oct 25, 2024
24 checks passed
@mikenairn mikenairn deleted the sotw_dnspolicy branch October 25, 2024 11:19
@mikenairn mikenairn mentioned this pull request Oct 25, 2024
Closed
R-Lawton pushed a commit to R-Lawton/kuadrant-operator that referenced this pull request Oct 29, 2024
@mikenairn @R-Lawton
sotw: dnspolicy (Kuadrant#937)
ae2844e 
* sotw: dnspolicy init

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

* Update dns policy validator in preparation 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

* 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.

* sotw dnspolicy: status and dnspolicies reconciliation

* Bump policy-machinery v0.6.1

---------

Signed-off-by: Michael Nairn <mnairn@redhat.com>
Signed-off-by: R-Lawton <rlawton@redhat.com>
mikenairn added a commit to mikenairn/kuadrant-operator that referenced this pull request Oct 31, 2024
@mikenairn
chore: Tidy up dnspolicy controller code and files
3dac1ed 
Follow on from Kuadrant#937 to
cleanup controller files to bring them more inline with the current sotw
approach.

There are no logical changes in this commit, existing code is just being
moved.

Signed-off-by: Michael Nairn <mnairn@redhat.com>
@mikenairn mikenairn mentioned this pull request Oct 31, 2024
Merged
mikenairn added a commit to mikenairn/kuadrant-operator that referenced this pull request Oct 31, 2024
@mikenairn
chore: Tidy up dnspolicy controller code and files
3aa9e84 
Follow on from Kuadrant#937 to
cleanup controller files to bring them more inline with the current sotw
approach.

There are no logical changes in this commit, existing code is just being
moved.

Signed-off-by: Michael Nairn <mnairn@redhat.com>
mikenairn added a commit that referenced this pull request Oct 31, 2024
@mikenairn
chore: Tidy up dnspolicy controller code and files (#967)
853577f 
Follow on from #937 to
cleanup controller files to bring them more inline with the current sotw
approach.

There are no logical changes in this commit, existing code is just being
moved.

Signed-off-by: Michael Nairn <mnairn@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maleck13 maleck13 maleck13 approved these changes

@guicassolato guicassolato guicassolato left review comments

@Boomatang Boomatang Boomatang left review comments

@maksymvavilov maksymvavilov maksymvavilov left review comments

@KevFan KevFan KevFan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[state-of-the-world reconciler] Effective policy DNS
6 participants
@mikenairn @maleck13 @guicassolato @Boomatang @KevFan @maksymvavilov