OCPBUGS-65626: add service account to guard pod by ehearne-redhat · Pull Request #2076 · openshift/library-go

ehearne-redhat · 2026-01-08T16:57:16Z

This change adds a bespoke service account to the guard pod. It also handles service account cleanup, and includes additional fields in tests and basic service account testing.

The reason for the change is that we should opt to use a bespoke service account rather than default.

openshift-ci-robot · 2026-01-08T16:57:26Z

@ehearne-redhat: This pull request references Jira Issue OCPBUGS-65626, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.22.0) matches configured target version for branch (4.22.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @wangke19

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

openshift-ci-robot · 2026-01-08T17:16:56Z

@ehearne-redhat: This pull request references Jira Issue OCPBUGS-65626, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.22.0) matches configured target version for branch (4.22.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @wangke19

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

This change adds a bespoke service account to the guard pod. It also handles service account cleanup, and includes additional fields in tests and basic service account testing.

The reason for the change is that we should opt to use a bespoke service account rather than default.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

ehearne-redhat · 2026-01-09T13:13:23Z

@wangke19 @p0lyn0mial this PR is ready to review. Could you please take a look? :)

p0lyn0mial · 2026-01-28T15:54:07Z

/assign @ingvagabund

@ingvagabund please take a look at this PR. I think you are the best person to take a look since you created the controller. Thanks!

ehearne-redhat · 2026-01-29T17:08:31Z

Hey @ingvagabund - thanks so much for your review! I have amended the changes and added unit tests. Tests are passing locally.

Please take a look when you have the chance :)

ingvagabund · 2026-02-02T12:34:53Z

The PR looks good in overall. Thank you. Just few more nits.

ingvagabund · 2026-02-02T12:36:47Z

Can you also please open evidence PRs for the corresponding operators as wel? KCM-o, KA-o, KS-o. To see the CI goes green to avoid any hidden corners.

ehearne-redhat · 2026-02-03T12:31:32Z

@ingvagabund thanks so much for your review - I have completed the evidence PRs. I'll re-ping for review when the tests come back. :)

ehearne-redhat · 2026-02-04T14:28:18Z

Hey @ingvagabund the evidence PR's openshift/cluster-kube-controller-manager-operator#905 , openshift/cluster-kube-apiserver-operator#2026 , and openshift/cluster-kube-scheduler-operator#610 are now ready to review for evidence. I have attached proof of guard pods using their own service accounts in each.

Please take a look when you have the chance. :)

ingvagabund · 2026-02-05T15:47:09Z

Brilliant :) Thank you.
/lgtm

ingvagabund · 2026-02-05T15:47:51Z

@p0lyn0mial for the final approval

ingvagabund · 2026-02-05T15:48:33Z

/lgtm cancel

@ehearne-redhat can you please squash the commits before merging?

p0lyn0mial · 2026-02-25T15:21:54Z

 		pdbGetter:                     pdbGetter,
 		pdbLister:                     kubeInformersForTargetNamespace.Policy().V1().PodDisruptionBudgets().Lister(),
+		saGetter:                      saGetter,
+		saLister:                      saLister,


let's take the lister from kubeInformersForTargetNamespace

then let's add the informer to factory.New().WithInformers() below.

p0lyn0mial · 2026-02-26T13:54:48Z

+		if errors.IsNotFound(err) {
+			_, _, err = resourceapply.ApplyServiceAccount(ctx, c.saGetter, syncCtx.Recorder(), serviceAccount)
+			if err != nil {
+				klog.Errorf("Unable to create service account %v for Guard Pods: %v", serviceAccount.Name, err)


won't this ere be logged because we use WithSyncDegradedOnError ?
if yes, maybe we should skip the additional logging. (applies to the other places)

OK, I will remove the redundant klog.Errorf from my changes.

p0lyn0mial · 2026-02-26T14:02:01Z

+			_, _, err = resourceapply.ApplyServiceAccount(ctx, c.saGetter, syncCtx.Recorder(), serviceAccount)
+			if err != nil {
+				klog.Errorf("Unable to create service account %v for Guard Pods: %v", serviceAccount.Name, err)
+				return fmt.Errorf("Unable to create service account %v for Guard Pods: %v", serviceAccount.Name, err)


also, I think that in go in general errors should not be capitalized.

Yes, this is true. I was just following the convention from the other errors. I will remove those capital starts.

p0lyn0mial

Maybe for consistency we should change the existing code so that it uses lower-case for errors, uses %w to wrap errors, and doesn't log and return the same error. We could do that in a new commit for easier review.

p0lyn0mial · 2026-03-03T08:36:01Z

 	pdbClient := b.kubeClient.PolicyV1()
+	saClient := b.kubeClient.CoreV1()
 	operandInformers := b.kubeNamespaceInformers.InformersFor(b.operandNamespace)
+	saLister := operandInformers.Core().V1().ServiceAccounts().Lister()


we take the lister from kubeInformersForTargetNamespace.Core().V1().ServiceAccounts().Lister() this is not needed.

p0lyn0mial · 2026-03-03T08:36:15Z

 			podClient,
 			pdbClient,
+			saClient,
+			saLister,


we take the lister from kubeInformersForTargetNamespace.Core().V1().ServiceAccounts().Lister() this is not needed.

p0lyn0mial · 2026-03-03T08:36:26Z

 	podGetter corev1client.PodsGetter,
 	pdbGetter policyclientv1.PodDisruptionBudgetsGetter,
+	saGetter corev1client.ServiceAccountsGetter,
+	saLister corelisterv1.ServiceAccountLister,


we take the lister from kubeInformersForTargetNamespace.Core().V1().ServiceAccounts().Lister() this is not needed.

p0lyn0mial · 2026-03-03T08:38:14Z

+		if err == nil {
+			_, _, err = resourceapply.DeleteServiceAccount(ctx, c.saGetter, syncCtx.Recorder(), serviceAccount)
+			if err != nil {
+				klog.Errorf("unable to delete Service Account: %v", err)


use %w for err wrapping.

I'm getting this message when I try to use error wrapping
k8s.io/klog/v2.Errorf does not support error-wrapping directive %w

won't this ere be logged because we use WithSyncDegradedOnError ?
if yes, maybe we should skip the additional logging. (applies to the other places

p0lyn0mial · 2026-03-03T08:38:22Z

+				errs = append(errs, err)
+			}
+		} else if !apierrors.IsNotFound(err) {
+			klog.Errorf("unable to get service account %v from lister: %v", serviceAccount.Name, err)


use %w for err wrapping.

Same here
k8s.io/klog/v2.Errorf does not support error-wrapping directive %w

won't this ere be logged because we use WithSyncDegradedOnError ?
if yes, maybe we should skip the additional logging. (applies to the other places)

we could use fmt if we want to enhance the context and then add it to the errs

p0lyn0mial · 2026-03-03T08:38:33Z

+		if errors.IsNotFound(err) {
+			_, _, err = resourceapply.ApplyServiceAccount(ctx, c.saGetter, syncCtx.Recorder(), serviceAccount)
+			if err != nil {
+				return fmt.Errorf("Unable to create service account %v for Guard Pods: %v", serviceAccount.Name, err)


use %w for err wrapping.

This one I can do. :)

p0lyn0mial · 2026-03-03T08:45:38Z

 	}
 }

+func TestGuardServiceAccountManifestStability(t *testing.T) {


could this test be updated to something like:

sa := resourceread.ReadServiceAccountV1OrDie(serviceAccountTemplate) expected := &corev1.ServiceAccount{ ObjectMeta: metav1.ObjectMeta{ Name: "guard-sa", Labels: map[string]string{"app": "guard"}, }, } if !equality.Semantic.DeepEqual(sa, expected) { t.Fatalf("guard-pod-sa.yaml manifest has changed. " + "The controller only creates the SA, it does not update existing ones. " + "If the manifest changed, add update logic and update the expected value in this test.") }

?

openshift-ci · 2026-03-03T11:43:25Z

@ehearne-redhat: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-aws-encryption	`1763302`	link	true	`/test e2e-aws-encryption`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

p0lyn0mial · 2026-03-04T10:37:01Z

+				errs = append(errs, err)
+			}
+		} else if !apierrors.IsNotFound(err) {
+			klog.Errorf("unable to get service account %v from lister: %v", serviceAccount.Name, err)


won't this ere be logged because we use WithSyncDegradedOnError ?
if yes, maybe we should skip the additional logging. (applies to the other places)

p0lyn0mial · 2026-03-04T10:38:55Z

+				errs = append(errs, err)
+			}
+		} else if !apierrors.IsNotFound(err) {
+			klog.Errorf("unable to get service account %v from lister: %v", serviceAccount.Name, err)


we could use fmt if we want to enhance the context and then add it to the errs

p0lyn0mial · 2026-03-04T10:39:28Z

+		guardServiceAccount   *corev1.ServiceAccount
 		createConditionalFunc func() (bool, bool, error)
 		withArbiter           bool
+		expectSADeleted       bool // true if service account should be deleted (cleanup mode)


let's rm the comment, not sure it is helpful.

p0lyn0mial · 2026-03-04T10:42:37Z

+		if err == nil {
+			_, _, err = resourceapply.DeleteServiceAccount(ctx, c.saGetter, syncCtx.Recorder(), serviceAccount)
+			if err != nil {
+				klog.Errorf("unable to delete Service Account: %v", err)


won't this ere be logged because we use WithSyncDegradedOnError ?
if yes, maybe we should skip the additional logging. (applies to the other places

p0lyn0mial · 2026-03-04T10:43:30Z

+		if errors.IsNotFound(err) {
+			_, _, err = resourceapply.ApplyServiceAccount(ctx, c.saGetter, syncCtx.Recorder(), serviceAccount)
+			if err != nil {
+				return fmt.Errorf("Unable to create service account %v for Guard Pods: %w", serviceAccount.Name, err)


isn't the go convention to start err with a lowercase letter ?

p0lyn0mial · 2026-03-04T10:45:33Z

+		// The guard pod uses automountServiceAccountToken: false.
+		// There's nothing meaningful to update. So update logic isn't needed right now.
+		_, err = c.saLister.ServiceAccounts(serviceAccount.Namespace).Get(serviceAccount.Name)
+		if errors.IsNotFound(err) {


let's use apierrors and remove"k8s.io/apimachinery/pkg/api/errors" from the imports.

p0lyn0mial · 2026-03-04T10:50:50Z

+		if err == nil {
+			_, _, err = resourceapply.DeleteServiceAccount(ctx, c.saGetter, syncCtx.Recorder(), serviceAccount)
+			if err != nil {
+				klog.Errorf("unable to delete Service Account: %v", err)


the other log msg use "service account" not "Service Account", let's be consistent.

I believe this error log has been removed based on previous comment above. The consistency has been resolved anyway. :)

This change add a bespoke service account to a guard pod, and introduces checks to ensure proper service account cleanup. Tests are included to test behaviour of service account in different scenarios. Test is included to ensure developers review code in case of Service Account manifest change.

p0lyn0mial · 2026-03-06T15:23:43Z

/approve

/hold
for the evidence PRs to pass

ingvagabund · 2026-03-11T09:37:07Z

/lgtm

ingvagabund · 2026-03-11T09:37:12Z

/hold cancel

openshift-ci · 2026-03-11T09:37:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ehearne-redhat, ingvagabund, p0lyn0mial

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/operator/staticpod/OWNERS~~ [p0lyn0mial]

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

openshift-ci-robot · 2026-03-11T09:41:45Z

@ehearne-redhat: Jira Issue OCPBUGS-65626: All pull requests linked via external trackers have merged:

openshift/library-go#2076

Jira Issue OCPBUGS-65626 has been moved to the MODIFIED state.

Details

In response to this:

This change adds a bespoke service account to the guard pod. It also handles service account cleanup, and includes additional fields in tests and basic service account testing.

The reason for the change is that we should opt to use a bespoke service account rather than default.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 8, 2026

openshift-ci bot requested review from dgrisonnet, p0lyn0mial and wangke19 January 8, 2026 16:57

ehearne-redhat changed the title ~~[WIP] OCPBUGS-65626: add service account to guard pod~~ OCPBUGS-65626: add service account to guard pod Jan 8, 2026

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 8, 2026

openshift-ci bot assigned ingvagabund Jan 28, 2026

ingvagabund reviewed Jan 29, 2026

View reviewed changes

Comment thread pkg/operator/staticpod/controller/guard/guard_controller.go Outdated

Comment thread pkg/operator/staticpod/controller/guard/guard_controller.go Outdated

Comment thread pkg/operator/staticpod/controller/guard/guard_controller.go

ingvagabund reviewed Feb 2, 2026

View reviewed changes

Comment thread pkg/operator/staticpod/controller/guard/guard_controller.go Outdated

Comment thread pkg/operator/staticpod/controller/guard/guard_controller.go Outdated

ehearne-redhat force-pushed the add-sa-guard-pod branch from ce096db to 1763302 Compare February 6, 2026 09:20

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2026

ehearne-redhat force-pushed the add-sa-guard-pod branch from 1763302 to 2ebfe9b Compare February 6, 2026 09:28

ehearne-redhat force-pushed the add-sa-guard-pod branch from 9a4deb2 to 895b5de Compare February 24, 2026 14:24

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 24, 2026

p0lyn0mial reviewed Feb 25, 2026

View reviewed changes

ehearne-redhat force-pushed the add-sa-guard-pod branch 2 times, most recently from 4cad71f to 5c9bf94 Compare February 25, 2026 13:25

p0lyn0mial reviewed Feb 26, 2026

View reviewed changes

Comment thread pkg/operator/staticpod/controller/guard/guard_controller.go

p0lyn0mial reviewed Feb 26, 2026

View reviewed changes

Comment thread pkg/operator/staticpod/controller/guard/guard_controller.go

ehearne-redhat force-pushed the add-sa-guard-pod branch from 5c9bf94 to eb3b963 Compare March 2, 2026 16:32

p0lyn0mial reviewed Mar 3, 2026

View reviewed changes

ehearne-redhat force-pushed the add-sa-guard-pod branch from eb3b963 to 9ec4053 Compare March 3, 2026 11:34

ehearne-redhat force-pushed the add-sa-guard-pod branch 2 times, most recently from 6d44615 to 19a0412 Compare March 3, 2026 11:48

p0lyn0mial reviewed Mar 4, 2026

View reviewed changes

ehearne-redhat force-pushed the add-sa-guard-pod branch from 19a0412 to 546c960 Compare March 4, 2026 11:02

ehearne-redhat force-pushed the add-sa-guard-pod branch from 546c960 to c56f4c3 Compare March 4, 2026 11:03

openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 6, 2026

openshift-ci bot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Mar 11, 2026

openshift-merge-bot bot merged commit ac826d1 into openshift:master Mar 11, 2026
4 checks passed

openshift-ci-robot mentioned this pull request Mar 21, 2026

OCPBUGS-65626: upgrade library-go to latest changes openshift/cluster-etcd-operator#1566

Merged

Conversation

ehearne-redhat commented Jan 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Jan 8, 2026

Uh oh!

openshift-ci-robot commented Jan 8, 2026

Uh oh!

ehearne-redhat commented Jan 9, 2026

Uh oh!

p0lyn0mial commented Jan 28, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ehearne-redhat commented Jan 29, 2026

Uh oh!

Uh oh!

Uh oh!

ingvagabund commented Feb 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ingvagabund commented Feb 2, 2026

Uh oh!

ehearne-redhat commented Feb 3, 2026

Uh oh!

ehearne-redhat commented Feb 4, 2026

Uh oh!

ingvagabund commented Feb 5, 2026

Uh oh!

ingvagabund commented Feb 5, 2026

Uh oh!

ingvagabund commented Feb 5, 2026

Uh oh!

Uh oh!

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!

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!

Uh oh!

Uh oh!

p0lyn0mial left a comment

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!

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!

ehearne-redhat commented Jan 8, 2026 •

edited

Loading

ingvagabund commented Feb 2, 2026 •

edited

Loading

openshift-ci bot commented Mar 3, 2026 •

edited

Loading