:sparkles: Add ServiceAccount validation to ClusterExtension reconciliation by perdasilva · Pull Request #2488 · operator-framework/operator-controller

perdasilva · 2026-02-05T14:19:52Z

Summary

Introduces an extensible validation framework for ClusterExtension reconciliation that validates configuration before attempting bundle installation. The first validator ensures the specified ServiceAccount exists.

Problem

Previously, ServiceAccount validation happened during revision state retrieval, which meant:

Configuration errors were discovered late in the reconciliation process
Error handling was mixed with business logic
No way to validate multiple aspects and report all errors at once

Solution

New Validation Framework

ValidateClusterExtension(validators ...ClusterExtensionValidator)

Orchestrates multiple validators in a single reconcile step
Executes ALL validators even if some fail (collects all errors)
Sets appropriate status conditions on failure:
- Installed: Unknown
- Progressing: True (Reason: Retrying)
Returns aggregated errors with clear user-facing messages

ServiceAccountValidator(saClient)

Validates ServiceAccount existence via direct CoreV1 API Get call
Returns friendly error: service account "name" not found in namespace "ns"

Integration Points

Added as a reconcile step (after finalizer handling, before revision state retrieval) in both:

Boxcutter runtime configuration
Helm runtime configuration

RBAC Changes

Added get permission for serviceaccounts resources to the operator-controller manager ClusterRole
Regenerated all deployment manifests (standard, experimental, e2e variants)

Cleanup

Removed ServiceAccount-specific error handling from RetrieveRevisionStates since validation now happens earlier in the pipeline
Removed authentication.ServiceAccountNotFoundError dependency from reconcile steps and tests

Testing

Unit Tests (`clusterextension_controller_test.go`)

TestValidateClusterExtension - 7 table-driven scenarios:

All validators pass
Single validator fails
Multiple validators collect all failures
Multiple validators all pass
Mixed pass/fail validators
ServiceAccount not found
ServiceAccount found

E2E Tests (`test/e2e/features/install.feature`)

New scenario: "Report validation error when ServiceAccount does not exist"

Files Changed

cmd/operator-controller/main.go - Added validation step with ServiceAccountValidator to both boxcutter and helm reconciler configurations
internal/operator-controller/controllers/clusterextension_reconcile_steps.go - New validation framework (ClusterExtensionValidator type, ValidateClusterExtension, ServiceAccountValidator), removed SA error handling from RetrieveRevisionStates
internal/operator-controller/controllers/clusterextension_controller_test.go - Replaced TestClusterExtensionServiceAccountNotFound with comprehensive TestValidateClusterExtension table-driven tests (7 scenarios)
internal/operator-controller/controllers/suite_test.go - Added Validators field to test deps and included ValidateClusterExtension in default reconcile steps
helm/olmv1/templates/rbac/clusterrole-operator-controller-manager-role.yml - Added get permission for serviceaccounts
manifests/*.yaml - Regenerated manifests reflecting RBAC changes
test/e2e/features/install.feature - E2E validation scenario for missing ServiceAccount

🤖 Generated with Claude Code

netlify · 2026-02-05T14:19:57Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`7cd5abe`
🔍 Latest deploy log	https://app.netlify.com/projects/olmv1/deploys/69972be1337d17000821fb61
😎 Deploy Preview	https://deploy-preview-2488--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

openshift-ci · 2026-02-05T14:19:58Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign oceanc80 for approval. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

OWNERS

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

Copilot

Pull request overview

This PR adds ServiceAccount validation to the ClusterExtension reconciliation pipeline. The validation checks whether the ServiceAccount specified in .spec.serviceAccount exists on the cluster before proceeding with bundle installation, moving this check from RetrieveRevisionStates to an earlier validation stage.

Changes:

Introduces a new validation framework with ValidateClusterExtension orchestrator and ServiceAccountValidator implementation
Moves ServiceAccount existence checks earlier in the reconciliation pipeline (after finalizer handling)
Updates both Boxcutter and Helm reconciler configurations to include the validation step

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
`internal/operator-controller/controllers/clusterextension_reconcile_steps.go`	Adds validation framework (ValidateClusterExtension, ClusterExtensionValidator, ServiceAccountValidator) and removes ServiceAccount error handling from RetrieveRevisionStates
`internal/operator-controller/controllers/clusterextension_reconcile_steps_test.go`	Adds comprehensive unit tests for validator functions (10 test cases total)
`internal/operator-controller/controllers/clusterextension_controller_test.go`	Updates existing ServiceAccount test to use new validation approach with mock TokenGetter
`cmd/operator-controller/main.go`	Integrates validation step into both Boxcutter and Helm reconciler configurations
`test/e2e/features/install.feature`	Adds E2E Gherkin scenario for ServiceAccount validation error case

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

internal/operator-controller/controllers/clusterextension_controller_test.go

internal/operator-controller/controllers/clusterextension_reconcile_steps.go

internal/operator-controller/controllers/clusterextension_reconcile_steps_test.go

internal/operator-controller/controllers/clusterextension_reconcile_steps.go

internal/operator-controller/controllers/clusterextension_reconcile_steps_test.go

codecov · 2026-02-05T14:38:38Z

Codecov Report

❌ Patch coverage is 96.42857% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 73.13%. Comparing base (553e210) to head (7cd5abe).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...er/controllers/clusterextension_reconcile_steps.go	95.45%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2488      +/-   ##
==========================================
- Coverage   73.21%   73.13%   -0.09%     
==========================================
  Files         102      103       +1     
  Lines        8505     8539      +34     
==========================================
+ Hits         6227     6245      +18     
- Misses       1799     1813      +14     
- Partials      479      481       +2

Flag	Coverage Δ
e2e	`45.73% <85.71%> (+0.14%)`	⬆️
experimental-e2e	`53.60% <85.71%> (-0.01%)`	⬇️
unit	`57.94% <75.00%> (+0.05%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

internal/operator-controller/controllers/clusterextension_reconcile_steps.go

internal/operator-controller/controllers/clusterextension_reconcile_steps_test.go

internal/operator-controller/controllers/clusterextension_reconcile_steps.go

perdasilva · 2026-02-09T09:25:32Z

internal/operator-controller/controllers/clusterextension_reconcile_steps.go

 		revisionStates, err := r.GetRevisionStates(ctx, ext)
 		if err != nil {
 			setInstallStatus(ext, nil)
-			var saerr *authentication.ServiceAccountNotFoundError


this got split between the service account validator and the cluster extension validator

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

internal/operator-controller/controllers/clusterextension_controller_test.go

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

internal/operator-controller/controllers/clusterextension_controller_test.go

perdasilva · 2026-02-18T16:43:49Z

internal/operator-controller/controllers/clusterextension_controller_test.go

 	require.NoError(t, cl.DeleteAllOf(ctx, &ocv1.ClusterExtension{}))
 }

-func TestClusterExtensionServiceAccountNotFound(t *testing.T) {


I moved this use-case into a more general test against validation

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

pedjak · 2026-02-18T21:18:31Z

internal/operator-controller/controllers/clusterextension_reconcile_steps.go

+// validators sequentially. All validators are executed even if some fail,
+// and all errors are collected and returned as a joined error.
+// This provides complete validation feedback in a single reconciliation cycle.
+func ValidateClusterExtension(validators ...ClusterExtensionValidator) ReconcileStepFunc {


it would be nice to add some unit tests for the new reconcile step.

I think I've covered those here, no?

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

internal/operator-controller/controllers/clusterextension_controller_test.go

cmd/operator-controller/main.go

…erExtension Introduces an extensible validation framework that runs early in the ClusterExtension reconciliation pipeline (after finalizer handling, before revision state retrieval) to catch configuration errors before expensive operations begin. Key changes: - New ClusterExtensionValidator type and ValidateClusterExtension reconcile step that executes all validators and collects all errors (no fail-fast behavior) - ServiceAccountValidator checks SA existence via direct CoreV1 API Get call, providing clear "not found" feedback - Sets Installed=Unknown and Progressing=Retrying conditions on validation failure - Removed SA-specific error handling from RetrieveRevisionStates, since validation now catches these errors earlier - Added RBAC permission (get serviceaccounts) to the operator-controller manager ClusterRole and regenerated manifests Testing: - Table-driven unit tests covering 7 scenarios (all pass, single fail, multiple fail, mixed, SA not found, SA found) - E2E scenario for missing ServiceAccount validation error Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com> Signed-off-by: Per G. da Silva <pegoncal@redhat.com>

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-19T15:35:28Z

internal/operator-controller/controllers/clusterextension_reconcile_steps.go

+			return nil, nil
+		}
+
+		// Set status condition with the validation error for other validation failures


The comment "Set status condition with the validation error for other validation failures" is misleading. The word "other" suggests there are different types of validation failures handled differently, but this code path handles all validation failures uniformly. Consider changing to "Set status conditions with the validation errors" for clarity.

Suggested change

// Set status condition with the validation error for other validation failures

// Set status conditions with the validation errors

Copilot · 2026-02-19T15:35:28Z

internal/operator-controller/controllers/clusterextension_controller_test.go

+				serviceAccountValidatorWithFakeClient(&corev1.ServiceAccount{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "missing-sa",


The test setup is confusing because it creates a ServiceAccount named "missing-sa" in the fake client, but the test name "service account not found" and the expected error message suggest we're testing for "test-sa" not being found. While the test logic is correct (it verifies that when "test-sa" doesn't exist, we get the expected error), the setup could be clearer. Consider either: (1) Renaming the ServiceAccount to clarify it's a decoy (e.g., "other-sa"), (2) Adding a comment explaining that this creates a different SA to ensure "test-sa" is not found, or (3) Passing nil to serviceAccountValidatorWithFakeClient to make it explicit that no SA exists.

Suggested change

serviceAccountValidatorWithFakeClient(&corev1.ServiceAccount{

ObjectMeta: metav1.ObjectMeta{

Name: "missing-sa",

// Create a different ServiceAccount to ensure "test-sa" is not found.

serviceAccountValidatorWithFakeClient(&corev1.ServiceAccount{

ObjectMeta: metav1.ObjectMeta{

Name: "other-sa",

Copilot AI review requested due to automatic review settings February 5, 2026 14:19

openshift-ci bot requested review from anik120 and grokspawn February 5, 2026 14:19

Copilot started reviewing on behalf of perdasilva February 5, 2026 14:20 View session

perdasilva changed the title ~~feat: Add ServiceAccount validation to ClusterExtension reconciliation~~ ✨ Add ServiceAccount validation to ClusterExtension reconciliation Feb 5, 2026

perdasilva marked this pull request as draft February 5, 2026 14:20

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 Feb 5, 2026

Copilot AI reviewed Feb 5, 2026

View reviewed changes

perdasilva force-pushed the validation-stage branch from 26b9052 to 58f8e47 Compare February 5, 2026 14:46

Copilot AI review requested due to automatic review settings February 5, 2026 16:54

perdasilva force-pushed the validation-stage branch from 58f8e47 to 64a00b6 Compare February 5, 2026 16:54

Copilot started reviewing on behalf of perdasilva February 5, 2026 16:56 View session

Copilot AI reviewed Feb 5, 2026

View reviewed changes

perdasilva force-pushed the validation-stage branch from 64a00b6 to 8e814fb Compare February 6, 2026 13:40

perdasilva commented Feb 9, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings February 9, 2026 09:31

perdasilva force-pushed the validation-stage branch from 8e814fb to cf4cdb1 Compare February 9, 2026 09:31

Copilot started reviewing on behalf of perdasilva February 9, 2026 09:31 View session

perdasilva marked this pull request as ready for review February 9, 2026 09:33

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 Feb 9, 2026

openshift-ci bot requested a review from pedjak February 9, 2026 09:33

Copilot AI reviewed Feb 9, 2026

View reviewed changes

internal/operator-controller/controllers/clusterextension_controller_test.go Outdated Show resolved Hide resolved

internal/operator-controller/controllers/clusterextension_controller_test.go Show resolved Hide resolved

perdasilva force-pushed the validation-stage branch from cf4cdb1 to a946319 Compare February 9, 2026 13:28

Copilot AI review requested due to automatic review settings February 9, 2026 14:09

perdasilva force-pushed the validation-stage branch from a946319 to 81c68c3 Compare February 9, 2026 14:09

Copilot started reviewing on behalf of perdasilva February 9, 2026 14:10 View session

Copilot AI reviewed Feb 9, 2026

View reviewed changes

internal/operator-controller/controllers/clusterextension_controller_test.go Outdated Show resolved Hide resolved

perdasilva force-pushed the validation-stage branch from 81c68c3 to 6d40acc Compare February 9, 2026 14:31

Copilot AI review requested due to automatic review settings February 18, 2026 16:39

perdasilva force-pushed the validation-stage branch 2 times, most recently from 18ede8a to 6bad46a Compare February 18, 2026 16:42

Copilot started reviewing on behalf of perdasilva February 18, 2026 16:43 View session

perdasilva commented Feb 18, 2026

View reviewed changes

Copilot AI reviewed Feb 18, 2026

View reviewed changes

pedjak reviewed Feb 18, 2026

View reviewed changes

perdasilva force-pushed the validation-stage branch from 6bad46a to 773c8f2 Compare February 19, 2026 09:31

Copilot AI review requested due to automatic review settings February 19, 2026 11:47

perdasilva force-pushed the validation-stage branch from 773c8f2 to a352ccc Compare February 19, 2026 11:47

Copilot started reviewing on behalf of perdasilva February 19, 2026 11:47 View session

Copilot AI reviewed Feb 19, 2026

View reviewed changes

internal/operator-controller/controllers/clusterextension_controller_test.go Show resolved Hide resolved

cmd/operator-controller/main.go Show resolved Hide resolved

perdasilva force-pushed the validation-stage branch from a352ccc to 8638859 Compare February 19, 2026 12:21

Copilot AI review requested due to automatic review settings February 19, 2026 15:27

perdasilva force-pushed the validation-stage branch from 8638859 to 7cd5abe Compare February 19, 2026 15:27

Copilot started reviewing on behalf of perdasilva February 19, 2026 15:28 View session

Copilot AI reviewed Feb 19, 2026

View reviewed changes

	// Set status condition with the validation error for other validation failures
	// Set status conditions with the validation errors

Conversation

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

Uh oh!

Summary

Problem

Solution

New Validation Framework

Integration Points

RBAC Changes

Cleanup

Testing

Unit Tests (clusterextension_controller_test.go)

E2E Tests (test/e2e/features/install.feature)

Files Changed

Uh oh!

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

Uh oh!

✅ Deploy Preview for olmv1 ready!

Uh oh!

openshift-ci bot commented Feb 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

perdasilva Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

perdasilva Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

pedjak Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

perdasilva Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

perdasilva commented Feb 5, 2026 •

edited

Loading

Unit Tests (`clusterextension_controller_test.go`)

E2E Tests (`test/e2e/features/install.feature`)

netlify bot commented Feb 5, 2026 •

edited

Loading

codecov bot commented Feb 5, 2026 •

edited

Loading