Add support for a custom service account for hub templates #236

mprahl · 2024-08-01T15:10:27Z

A new field of spec.hubTemplateOptions.serviceAccountName can be set to leverage a service account in the root policy namespace to resolve hub templates instead of the restricted hub templating of referencing only objects in the root policy namespace.

This leverages the TokenRequest API to get short-lived tokens of the service accounts and periodically refreshes them before expiration.

Relates:
https://issues.redhat.com/browse/ACM-12790

mprahl · 2024-08-01T15:11:10Z

api/v1/policy_types.go

@@ -98,6 +102,9 @@ type PolicySpec struct {
 	// PolicyDependencies is a list of dependency objects detailed with extra considerations for
 	// compliance that should be fulfilled before applying the policies to the managed clusters.
 	Dependencies []PolicyDependency `json:"dependencies,omitempty"`
+
+	// HubTemplateOptions changes the default behavior of hub templates.
+	HubTemplateOptions *HubTemplateOptions `json:"hubTemplateOptions,omitempty"`


This is a pointer so that the omitempty has effect rather than returning an empty struct when hubTemplateOptions is unset.

Do we need to create HubTemplateOptions.ServiceAccountName? because it is only one property.

We don't need it but I chose it in case we make other things customizable and I thought a hubTemplateServiceAccountName field was too long lol.

Grouping more pretty and this is good for future maybe

yiraeChristineKim · 2024-08-01T15:46:55Z

controllers/propagator/replicatedpolicy_controller.go

+	if errors.Is(tmplErr, ErrRetryable) {
+		// Return the error if it's retryable, which will utilize controller-runtime's exponential backoff.
+		return reconcile.Result{}, tmplErr
+	} else if errors.Is(tmplErr, ErrSAMissing) {


else is not needed?

It's not because template errors that are not retryable must continue so that the replicated policy has the hub template error annotation set.

mprahl · 2024-08-01T15:56:33Z

I'm not sure what's going on with the test. It's failing every time in CI but working every time locally...

controllers/propagator/template_utils.go

test/resources/case9_templates/case9-token-sa.yaml

test/e2e/case9_templates_test.go

yiraeChristineKim · 2024-08-02T16:56:16Z

test/e2e/case9_templates_test.go

+				g.Expect(err).ToNot(HaveOccurred())
+				g.Expect(refreshedToken).ToNot(BeNil())
+				g.Expect(bytes.Equal(token, refreshedToken)).To(BeFalse(), "Expected the token to have been refreshed")
+			}, 70, 3).Should(Succeed())


Why 70 not 60?

Just because it could be exactly 60 seconds before the token refreshes, so I gave it a bit of a buffer. I could have made it 65 seconds though.

I don't think it is a big deal as long as they change it. I don't think no one complain that it is over 1min

controllers/propagator/template_utils.go

yiraeChristineKim · 2024-08-02T18:14:52Z

controllers/propagator/template_utils.go

+}
+
+// RemovePolicy will clean up watches on the current template resolver (service account used in the last call to
+// GetResolver) and if this was the last policy using this template resolver, the template resolver will be cleaned up.


root policy or replicated policy? It is confusing. Base on referenceCount description it is a replicated policy right?

Yup, it's a replicated policy. I can rename the method or clarify the description. Any preference?

RemoveRePolicy? I don't know Dale is good at this lol

yiraeChristineKim · 2024-08-02T18:39:39Z

controllers/propagator/template_utils.go

+	globalLock sync.RWMutex
+	// The default template resolver when a replicated policy does not specify a service account.
+	defaultTemplateResolver *templates.TemplateResolver
+	// A map of service acconts to template resolvers. Access to this must leverage globalLock.


Suggested change

// A map of service acconts to template resolvers. Access to this must leverage globalLock.

// A map of service acconts to templateResolverWithCancel. Access to this must leverage globalLock.

??

or template resolver single

yiraeChristineKim · 2024-08-02T19:29:22Z

This is amazing! Yare are the best expert Go concurrency

mprahl · 2024-08-02T20:21:05Z

/hold for reviews

mprahl · 2024-08-02T20:22:37Z

controllers/propagator/propagation.go

@@ -408,7 +432,5 @@ func (r *ReplicatedPolicyReconciler) processTemplates(
 		}
 	}

-	log.V(1).Info("Successfully processed templates")


This log message is removed because processTemplates is now always called and when no templates are present, it does nothing. This is to clean up watches if hub templates used to be present but are now removed.

JustinKuli

Seems about right 😅

controllers/propagator/propagation.go

JustinKuli · 2024-08-05T15:36:06Z

controllers/propagator/replicatedpolicy_controller.go

+	var returnErr error
+
+	// Retry template errors due to permission issues. This isn't ideal, but there's no good event driven way to
+	// be notified when the permissions are given to the service account.
+	if k8serrors.IsForbidden(tmplErr) {
+		returnErr = tmplErr
+	}
+
 	if watcherErr != nil {
 		log.Info("Requeueing for the dynamic watcher error")
+
+		returnErr = watcherErr
 	}

-	return reconcile.Result{}, watcherErr
+	return reconcile.Result{}, returnErr


I wonder if customizing the retry interval for Forbidden errors might be nice. So maybe something like this:

if watcherErr != nil { log.Info("Requeueing for the dynamic watcher error") return reconcile.Result{}, watcherErr } // Retry template errors due to permission issues. This isn't ideal, but there's no good event driven way to // be notified when the permissions are given to the service account. if k8serrors.IsForbidden(tmplErr) { log.Info("Requeueing in 2 minutes for the permission issue") // returning the error would override the RequeueAfter setting return reconcile.Result{RequeueAfter: 2 * time.Minute}, nil } return reconicile.Result{}, nil

controllers/propagator/rootpolicy_setup.go

controllers/propagator/template_utils.go

JustinKuli · 2024-08-05T16:03:28Z

controllers/propagator/template_utils.go

+	// the template resolver will be cleaned up. A pointer is used since the reference count can be copied to a new
+	// templateResolverWithCancel instance if the TemplateResolver instance was unexpectedly canceled and was
+	// restarted.
+	referenceCount *atomic.Uint32


I can't shake the feeling that us doing our own reference counting means we're missing some other way to do this

The only other way I could think of that would work other than periodically checking to see if any root policy still references this service account was to have a map of service accounts to replicated policies. I thought the reference count was better to avoid locking around a shared map. It also saves a bit of memory.

Another way that came to mind was using a read and write lock for each template resolver and all replicated policies have a read lock and then a separate goroutine waits for a write lock, which would indicate there are no more readers left to then clean up the template resolver. The problem is that the read and write lock in the Go standard library doesn't work like that as it prioritizes giving the write lock when it can, which makes sense.

controllers/propagator/template_utils.go

A new field of spec.hubTemplateOptions.serviceAccountName can be set to leverage a service account in the root policy namespace to resolve hub templates instead of the restricted hub templating of referencing only objects in the root policy namespace. This leverages the TokenRequest API to get short-lived tokens of the service accounts and periodically refreshes them before expiration. Relates: https://issues.redhat.com/browse/ACM-12790 Signed-off-by: mprahl <[email protected]>

This was covering up an error during test development. Signed-off-by: mprahl <[email protected]>

JustinKuli

Seems good to me.

openshift-ci · 2024-08-05T19:47:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JustinKuli, mprahl

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JustinKuli,mprahl]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mprahl · 2024-08-05T20:07:01Z

/unhold

openshift-ci bot requested review from dhaiducek and gparvin August 1, 2024 15:10

openshift-ci bot added dco-signoff: yes approved labels Aug 1, 2024

mprahl commented Aug 1, 2024

View reviewed changes

mprahl requested review from JustinKuli and yiraeChristineKim August 1, 2024 15:18

yiraeChristineKim reviewed Aug 1, 2024

View reviewed changes

controllers/propagator/template_utils.go Show resolved Hide resolved

yiraeChristineKim reviewed Aug 1, 2024

View reviewed changes

test/resources/case9_templates/case9-token-sa.yaml Show resolved Hide resolved

yiraeChristineKim reviewed Aug 1, 2024

View reviewed changes

test/e2e/case9_templates_test.go Show resolved Hide resolved

mprahl requested a review from yiraeChristineKim August 2, 2024 15:04

yiraeChristineKim reviewed Aug 2, 2024

View reviewed changes

controllers/propagator/template_utils.go Show resolved Hide resolved

yiraeChristineKim reviewed Aug 2, 2024

View reviewed changes

mprahl requested a review from yiraeChristineKim August 2, 2024 20:20

openshift-ci bot added the do-not-merge/hold label Aug 2, 2024

mprahl commented Aug 2, 2024

View reviewed changes

JustinKuli reviewed Aug 5, 2024

View reviewed changes

mprahl added 2 commits August 5, 2024 13:28

Wait for kubectl to complete before returning in tests

ba42aa3

This was covering up an error during test development. Signed-off-by: mprahl <[email protected]>

mprahl requested a review from JustinKuli August 5, 2024 17:29

JustinKuli approved these changes Aug 5, 2024

View reviewed changes

openshift-ci bot assigned JustinKuli Aug 5, 2024

openshift-ci bot added the lgtm label Aug 5, 2024

openshift-ci bot removed the do-not-merge/hold label Aug 5, 2024

openshift-merge-bot bot merged commit fe30aeb into open-cluster-management-io:main Aug 5, 2024
13 checks passed

magic-mirror-bot bot mentioned this pull request Aug 5, 2024

😿 Failed to sync the upstream PRs: #236 stolostron/governance-policy-propagator#806

Closed

	// A map of service acconts to template resolvers. Access to this must leverage globalLock.
	// A map of service acconts to templateResolverWithCancel. Access to this must leverage globalLock.

Add support for a custom service account for hub templates #236

Add support for a custom service account for hub templates #236

Uh oh!

Conversation

mprahl commented Aug 1, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mprahl commented Aug 1, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yiraeChristineKim commented Aug 2, 2024

Uh oh!

mprahl commented Aug 2, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JustinKuli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JustinKuli left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Aug 5, 2024

Uh oh!

mprahl commented Aug 5, 2024

Uh oh!

Uh oh!

Uh oh!