refactor: effective tls policies reconciler #927

KevFan · 2024-10-09T17:59:19Z

Description

Refactor the logic for the management of TLSPolicy Certificates into a workflow task
Re-add validation for TLS Policy to check for if Issuer kind is correct and if the issuer is present on cluster, with the addition of integrations tests to test for this

Verification

Passing integration tests should be enough to test that nothing is broken from this change

KevFan · 2024-10-11T14:23:53Z

controllers/effective_tls_policies_reconciler.go

+		if policy.DeletionTimestamp != nil {
+			logger.V(1).Info("policy is marked for deletion, nothing to do", "name", policy.Name, "namespace", policy.Namespace, "uid", policy.GetUID())
+			continue
+		}


No need for finalizer since associated Certificate's will also be deleted by owner ref

maleck13 · 2024-10-15T09:58:24Z

controllers/effective_tls_policies_reconciler.go

+	logger := controller.LoggerFromContext(ctx).WithName("EffectiveTLSPoliciesReconciler").WithName("Reconcile")
+
+	// Get all TLS Policies
+	policies := lo.Filter(topology.Policies().Items(), func(item machinery.Policy, index int) bool {


So are we reconciling every policy on the cluster with every change to the subscribed kinds? Not quite understanding how this is working

If so do we intend to have a way to not need to do this? Like if a certificate changes, there is a link between that certificate and the TLSPolicy that ownes it so we shouldn't need to reconcile all TLSPolicies because a certificate changed right. Or am I misunderstanding?

That's fair. There's is a way to do reconcile only affected tls policies instead by using the events received. I've pushed a commit for this so this should be addressed now cd35907

Signed-off-by: KevFan <chfan@redhat.com>

…get ref Signed-off-by: KevFan <chfan@redhat.com>

…rts" This reverts commit 552d9f3. Signed-off-by: KevFan <chfan@redhat.com>

Signed-off-by: KevFan <chfan@redhat.com>

guicassolato · 2024-10-16T22:49:42Z

controllers/tls_workflow.go

+			g := machinery.Gateway{Gateway: ob.(*gwapiv1.Gateway)}
+
+			affectedPolicies = append(affectedPolicies, lo.Filter(policies, func(item machinery.Policy, index int) bool {
+				for _, tg := range item.GetTargetRefs() {
+					if g.GetLocator() == tg.GetLocator() {


Have you considered using g.Policies()?

Hmm, dont think i'm able to since g is a new machinery.Gateway instance, so the policies will be nil unless you're saying to get the machinery.Gateway instance elsewhere? 🤔

guicassolato · 2024-10-16T22:54:23Z

controllers/tls_workflow.go

+			affectedPolicies = append(affectedPolicies, lo.Filter(policies, func(item machinery.Policy, index int) bool {
+				return item.GetName() == ob.GetName() && item.GetNamespace() == ob.GetNamespace()
+			})...)


I understand this will filter out all TLSPolicies that have been deleted. Unless that's what indeed you want, could we simply cast ob maybe?

Suggested change

affectedPolicies = append(affectedPolicies, lo.Filter(policies, func(item machinery.Policy, index int) bool {

return item.GetName() == ob.GetName() && item.GetNamespace() == ob.GetNamespace()

})...)

p, _ := ob.(*TLSPolicy)

affectedPolicies = append(affectedPolicies, p)

Hmm, I didn't intend to do this, but that is simpler indeed. Why it's working as is due to there's no work to be done on tlspolicies marked for deletion 🤔

guicassolato · 2024-10-16T23:04:34Z

controllers/tls_workflow.go

+			affectedPolicies = append(affectedPolicies, lo.Filter(policies, func(item machinery.Policy, index int) bool {
+				p := item.(*kuadrantv1alpha1.TLSPolicy)
+				return utils.IsOwnedBy(ob, p)
+			})...)


I believe you could leverage the topology links here too:

Suggested change

affectedPolicies = append(affectedPolicies, lo.Filter(policies, func(item machinery.Policy, index int) bool {

p := item.(*kuadrantv1alpha1.TLSPolicy)

return utils.IsOwnedBy(ob, p)

})...)

c, _ := ob.(*controller.RuntimeObject).Object.(*certmanagerv1.Certificate)

listeners := topology.Targetables().Parents(machinery.LocatorFromObject(c))

affectedPolicies = append(affectedPolicies, lo.FlatMap(listeners, func(l machinery.Targetable) ([]machinery.Policy, bool) {

return l.(*machinery.Listener).Gateway.Policies()

})...)

In this particular case, not sure if it's better or worse. I'm just thinking about avoiding filtering the same list of policies so many times.

Hmm, that's true. If you feel strongly on this I can update, but using the owner refernence looks a lot more readable unless filtering the list of policies should be avoided in general? 🤔

guicassolato · 2024-10-16T23:09:45Z

controllers/tls_workflow.go

 	}
+
+	// Return only unique policies as there can be duplicates from multiple events
+	return lo.Uniq(affectedPolicies)


Hmm. Maybe specify a uniqueness key and thus avoid possible problems with pointer comparison:

Suggested change

return lo.Uniq(affectedPolicies)

return lo.UniqBy(affectedPolicies, func(p machinery.Policy) string { return p.GetLocator() } )

guicassolato · 2024-10-16T23:23:21Z

controllers/effective_tls_policies_reconciler.go

+		// TODO: Update when targeting by section name is allowed, the listener will contain the policy rather than the gateway
+		listeners := lo.FilterMap(topology.Targetables().Items(), func(t machinery.Targetable, index int) (*machinery.Listener, bool) {
+			l, ok := t.(*machinery.Listener)
+			return l, ok && lo.Contains(l.Gateway.Policies(), p)


I'm actually surprised equality (==) between two policy works.

Yeah, I think it compares the pointers between the two policies

Signed-off-by: KevFan <chfan@redhat.com>

KevFan self-assigned this Oct 11, 2024

KevFan added the kind/enhancement New feature or request label Oct 11, 2024

KevFan commented Oct 11, 2024

View reviewed changes

maleck13 reviewed Oct 15, 2024

View reviewed changes

KevFan force-pushed the effective-tls-reconciler branch 5 times, most recently from af8c44c to cd35907 Compare October 16, 2024 09:21

KevFan marked this pull request as ready for review October 16, 2024 09:24

KevFan requested review from guicassolato, Boomatang and mikenairn October 16, 2024 11:45

KevFan added 10 commits October 16, 2024 17:27

refactor: effective tls policies reconciler

c2bdfb6

Signed-off-by: KevFan <chfan@redhat.com>

refactor: use gateways again for now to check for expected certs

c25ebb7

Signed-off-by: KevFan <chfan@redhat.com>

refactor: tests for new invalid conditions for tls policy

faf1e7f

Signed-off-by: KevFan <chfan@redhat.com>

refactor: validation methods

a6c1f0d

Signed-off-by: KevFan <chfan@redhat.com>

fixup: deletes certs owned by policy when invalid instead

31cc9c2

Signed-off-by: KevFan <chfan@redhat.com>

fixup: delete orphaned certs from change in target ref -> invalid tar…

86796f7

…get ref Signed-off-by: KevFan <chfan@redhat.com>

Revert "refactor: use gateways again for now to check for expected ce…

ef61dee

…rts" This reverts commit 552d9f3. Signed-off-by: KevFan <chfan@redhat.com>

fixup: log messages

637425e

Signed-off-by: KevFan <chfan@redhat.com>

fixup: conventions naming and move functions

4c99d04

Signed-off-by: KevFan <chfan@redhat.com>

refactor: only reconcile tls policies that were affected by events

58e608c

Signed-off-by: KevFan <chfan@redhat.com>

guicassolato reviewed Oct 16, 2024

View reviewed changes

refactor: integration tests for orphaned tests & address some comments

2c1093b

Signed-off-by: KevFan <chfan@redhat.com>

KevFan force-pushed the effective-tls-reconciler branch from cd35907 to 2c1093b Compare October 17, 2024 19:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: effective tls policies reconciler #927

refactor: effective tls policies reconciler #927

KevFan commented Oct 9, 2024 •

edited

Loading

KevFan Oct 11, 2024

maleck13 Oct 15, 2024 •

edited

Loading

maleck13 Oct 15, 2024 •

edited

Loading

KevFan Oct 16, 2024

guicassolato Oct 16, 2024

KevFan Oct 17, 2024

guicassolato Oct 16, 2024

KevFan Oct 17, 2024

guicassolato Oct 16, 2024

KevFan Oct 17, 2024

guicassolato Oct 16, 2024

guicassolato Oct 16, 2024

KevFan Oct 17, 2024

-			affectedPolicies = append(affectedPolicies, lo.Filter(policies, func(item machinery.Policy, index int) bool {
-				p := item.(*kuadrantv1alpha1.TLSPolicy)
-				return utils.IsOwnedBy(ob, p)
-			})...)
+			c, _ := ob.(*controller.RuntimeObject).Object.(*certmanagerv1.Certificate)
+			listeners := topology.Targetables().Parents(machinery.LocatorFromObject(c))
+			affectedPolicies = append(affectedPolicies, lo.FlatMap(listeners, func(l machinery.Targetable) ([]machinery.Policy, bool) {
+				return l.(*machinery.Listener).Gateway.Policies()
+			})...)

	return lo.Uniq(affectedPolicies)
	return lo.UniqBy(affectedPolicies, func(p machinery.Policy) string { return p.GetLocator() } )

refactor: effective tls policies reconciler #927

Are you sure you want to change the base?

refactor: effective tls policies reconciler #927

Conversation

KevFan commented Oct 9, 2024 • edited Loading

Description

Verification

Choose a reason for hiding this comment

maleck13 Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

maleck13 Oct 15, 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

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

KevFan commented Oct 9, 2024 •

edited

Loading

maleck13 Oct 15, 2024 •

edited

Loading

maleck13 Oct 15, 2024 •

edited

Loading