: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	`70e5028`
🔍 Latest deploy log	https://app.netlify.com/projects/olmv1/deploys/699c8038c66ee80008612e18
😎 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.

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.

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.24%. Comparing base (1ef820f) to head (70e5028).
⚠️ Report is 4 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.27% 73.24% -0.04% 
==========================================
Files 103 103 Lines 8516 8539 +23 ==========================================
+ Hits 6240 6254 +14 - Misses 1801 1808 +7 - Partials 475 477 +2

Flag	Coverage Δ
e2e	`45.43% <85.71%> (-0.39%)`	⬇️
experimental-e2e	`53.78% <85.71%> (+0.31%)`	⬆️
unit	`57.98% <75.00%> (+0.03%)`	⬆️

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.

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

 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.

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.

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.

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

Pull request overview

Copilot reviewed 10 out of 10 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.

camilamacedo86

I am OK with.
Until we go with easy mode makes sense to me, to validate it in the code.

/approved

pedjak · 2026-02-23T13:28:21Z

+	return func(ctx context.Context, state *reconcileState, ext *ocv1.ClusterExtension) (*ctrl.Result, error) {
+ l := log.FromContext(ctx)
+
+ l.Info("validating cluster extension")


I think we should lower this do debug level.

pedjak · 2026-02-23T13:30:26Z

+ }
+
+ // Set status conditions with the validation errors
+ err := fmt.Errorf("installation cannot proceed due to the following validation error(s): %w", errors.Join(validationErrors...))


can we make a bit more focused or generic message, because we cannot proceed with the installation or an update, right?

pedjak · 2026-02-23T13:38:08Z

+ if apierrors.IsNotFound(err) {
+ return fmt.Errorf("service account %q not found in namespace %q", ext.Spec.ServiceAccount.Name, ext.Spec.Namespace)
+ }
+ return fmt.Errorf("error validating service account: %w", err)


given that we just try to access the service accound at the line 105, the error message error validating service account is confusing because we did not perform any validation, just tried to access it.

…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 no new comments.

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

camilamacedo86

/approved

@pedjak WDYT? Could you LGTM this one?

pedjak

/lgtm

rashmigottipati

/lgtm

tmshort · 2026-02-23T21:10:14Z

/approve

openshift-ci · 2026-02-23T21:10:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rashmigottipati, tmshort

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:

~~OWNERS~~ [tmshort]

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

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

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

Copilotstarted 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 reconciliationFeb 5, 2026

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

openshift-ciBot 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

CopilotAI reviewed Feb 5, 2026
View reviewed changes

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

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

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

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

CopilotAI reviewed Feb 5, 2026
View reviewed changes

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

perdasilva commented Feb 9, 2026
View reviewed changes

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

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

Copilotstarted 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-ciBot 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-ciBot requested a review from pedjak February 9, 2026 09:33

CopilotAI reviewed Feb 9, 2026
View reviewed changes

Comment threadinternal/operator-controller/controllers/clusterextension_controller_test.go Outdated
Comment threadinternal/operator-controller/controllers/clusterextension_controller_test.go

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

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

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

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

CopilotAI reviewed Feb 9, 2026
View reviewed changes

Comment threadinternal/operator-controller/controllers/clusterextension_controller_test.go Outdated

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

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

CopilotAI reviewed Feb 19, 2026
View reviewed changes

Comment threadinternal/operator-controller/controllers/clusterextension_controller_test.go
Comment threadcmd/operator-controller/main.go

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

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

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

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

CopilotAI reviewed Feb 19, 2026
View reviewed changes

Comment threadinternal/operator-controller/controllers/clusterextension_reconcile_steps.go Outdated
Comment threadinternal/operator-controller/controllers/clusterextension_controller_test.go Outdated

perdasilva force-pushed the validation-stage branch from 7cd5abe to d82a364Compare February 23, 2026 08:27

CopilotAI review requested due to automatic review settings February 23, 2026 09:36

perdasilva force-pushed the validation-stage branch from d82a364 to 82ad75aCompare February 23, 2026 09:36

Copilotstarted reviewing on behalf of perdasilva February 23, 2026 09:36 View session

CopilotAI reviewed Feb 23, 2026
View reviewed changes

camilamacedo86 reviewed Feb 23, 2026
View reviewed changes

pedjak reviewed Feb 23, 2026
View reviewed changes

perdasilva force-pushed the validation-stage branch from 82ad75a to 6e72e47Compare February 23, 2026 15:13

CopilotAI review requested due to automatic review settings February 23, 2026 16:28

perdasilva force-pushed the validation-stage branch from 6e72e47 to 70e5028Compare February 23, 2026 16:28

Copilotstarted reviewing on behalf of perdasilva February 23, 2026 16:31 View session

CopilotAI reviewed Feb 23, 2026
View reviewed changes

camilamacedo86 reviewed Feb 23, 2026
View reviewed changes

pedjak reviewed Feb 23, 2026
View reviewed changes

openshift-ciBot assigned pedjak Feb 23, 2026

openshift-ciBot added the lgtm Indicates that a PR is ready to be merged. label Feb 23, 2026

rashmigottipati approved these changes Feb 23, 2026
View reviewed changes

openshift-ciBot assigned rashmigottipati Feb 23, 2026

openshift-ciBot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 23, 2026

openshift-merge-botBot merged commit ddf921f into operator-framework:mainFeb 23, 2026
32 checks passed

Uh oh!

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!

netlifyBot 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!

CopilotAI 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!

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

Uh oh!

Codecov Report

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

perdasilvaFeb 9, 2026

Choose a reason for hiding this comment

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

camilamacedo86 left a comment

Choose a reason for hiding this comment

Uh oh!

pedjakFeb 23, 2026

Choose a reason for hiding this comment

Uh oh!

pedjakFeb 23, 2026

perdasilva commented Feb 5, 2026•
edited
Loading

Unit Tests (`clusterextension_controller_test.go`)

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

netlifyBot commented Feb 5, 2026•
edited
Loading

codecovBot commented Feb 5, 2026•
edited
Loading