OPRUN-4666,OPRUN-4671,OPRUN-4672,OPRUN-4673,OPRUN-4674,OPRUN-4659: Synchronize From Upstream Repositories + Remove spec.serviceAccount tests + remove demo e2e

[tmshort]

Summary by CodeRabbit

• New Features

  • Added dynamic GraphQL catalog queries with pagination, schema discovery, caching, request timeouts, and query-complexity safeguards.
  • Added end-to-end demos with recorded terminal sessions and SVG conversion.
  • Added admission controls for fine-grained ClusterExtension API access.

• Changes

  • ClusterExtension serviceAccount configuration is now optional, deprecated, and ignored; setting it produces a warning.
  • Installation examples no longer require dedicated installer permissions.

• Documentation

  • Updated guides, API references, navigation, and quickstart demos to reflect the revised installation and access model.

[openshift-ci-robot]

@tmshort: This pull request references OPRUN-4666 which is a valid jira issue.

This pull request references OPRUN-4671 which is a valid jira issue.

This pull request references OPRUN-4672 which is a valid jira issue.

This pull request references OPRUN-4673 which is a valid jira issue.

This pull request references OPRUN-4674 which is a valid jira issue.

This pull request references OPRUN-4659 which is a valid jira issue.

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]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

• DOWNSTREAM_OWNERS [tmshort]

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

[coderabbitai]

Walkthrough

The change deprecates and ignores ClusterExtension.spec.serviceAccount, removes service-account-scoped installation and preflight authorization, grants the controller cluster-admin access, adds persisted dynamic GraphQL loading and query limits, and introduces end-to-end demo recording and admission-policy coverage.

Changes

OLMv1 service-account model

Layer / File(s)	Summary
API, CRD, and admission contract api/v1/*, helm/olmv1/..., manifests/*, openshift/operator-controller/*	spec.serviceAccount becomes optional, deprecated, and ignored; populated values produce warnings, while names remain immutable once set but may be cleared.
Controller and applier wiring cmd/operator-controller/main.go, internal/operator-controller/applier/*, internal/operator-controller/controllers/*, internal/operator-controller/revision_engine_factory.go	Service-account token, impersonation, pre-authorization, and content-manager paths are removed; shared tracking-cache wiring replaces per-extension cache setup.
OpenShift and E2E migration openshift/tests-extension/*, test/e2e/features/*, test/extension-developer-e2e/*	ClusterExtension fixtures and scenarios stop creating service accounts and RBAC bindings, and new deprecation-warning and API-access policy scenarios are added.
Documentation and generated examples docs/*, README.md, config/samples/*, mkdocs.yml	Service-account installation guidance is removed or revised, API-access protection guidance is added, and demo embeds use Asciinema/SVG assets.

Catalogd GraphQL and image handling

Layer / File(s)	Summary
Dynamic schema and query execution internal/catalogd/graphql/*, internal/catalogd/service/*, internal/catalogd/server/*	Schema discovery uses bounded sampling and serializable metadata; resolvers load objects on demand, query complexity is bounded, and execution uses context-aware provider-backed caching.
Persistent catalog loading internal/catalogd/storage/*	Catalog storage persists GraphQL schemas, exposes schema byte sections, and loads requested JSONL objects through indexed offsets.
Image and Helm cleanup internal/shared/util/image/*	Helm-specific image-layer detection, chart loading, and chart cache handling are removed; image layers use the standard archive path.

Demo and build workflow

Layer / File(s)	Summary
Demo recording and execution Makefile, test/e2e/features/demos.feature, test/e2e/steps/*	Tagged demo scenarios record Asciinema casts, query catalogs through port-forwarding, validate install modes and webhook behavior, and convert casts to SVG.
Build, installation, and dependency updates .gitignore, scripts/install.tpl.sh, requirements.txt, go.mod, openshift/Makefile	Generated demo output is ignored, legacy RBAC resources are cleaned up during installation, demo tags are excluded from normal E2E runs, and module/tool versions are refreshed.

Generated annotations

Layer / File(s)	Summary
Deployment metadata propagation internal/operator-controller/rukpak/render/registryv1/generators/*, test/regression/convert/testdata/*	Generated deployment templates now include olm.operatorNamespace derived from the installation namespace, with updated expected manifests.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

• openshift/operator-framework-operator-controller#770: Updates overlapping OpenShift E2E helpers and templates to remove ServiceAccount and ClusterRoleBinding provisioning.
• openshift/operator-framework-operator-controller#771: Modifies the same demo workflow, E2E tag filtering, timeout handling, and service-account deprecation paths.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

[tmshort]

/test unit

[coderabbitai]

Actionable comments posted: 9

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

internal/catalogd/service/graphql_service.go (1)

47-80: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

ctx passed into GetSchema is never used — schema build has no timeout.

GetSchema accepts a context.Context but never reads it; the underlying buildGroup.Do/buildSchema/provider.LoadCatalogSchema/NewObjectLoader calls take no context either (verified golang.org/x/sync/singleflight.Do itself has no context-aware overload). ExecuteQuery only wraps graphql.Do in a 5s timeout (Line 96) — the GetSchema(ctx, catalog) call right before it (Line 91) can block indefinitely if catalog loading is slow, with no deadline enforcement despite the surrounding code clearly intending timeout-bounded execution.

Wrap the schema-build phase with its own timeout (e.g., derive it from ctx similarly to the query timeout) or thread the context through to the provider so cancellation/deadlines actually apply to catalog IO.

As per path instructions for **/*.go, **/*.go: "context.Context for cancellation and timeouts" — the unused ctx parameter here doesn't fulfill that guidance.

Also applies to: 82-106

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/catalogd/service/graphql_service.go` around lines 47 - 80, Make
GetSchema honor its ctx by applying a bounded timeout or cancellation to the
schema-build operation, including the buildGroup.Do/buildSchema path, so slow
catalog loads cannot block indefinitely. Prefer threading ctx through
buildSchema and the provider.LoadCatalogSchema/NewObjectLoader calls; otherwise
enforce timeout completion around buildGroup.Do. Preserve cache behavior and
return context cancellation/deadline errors.

Source: Path instructions

🧹 Nitpick comments (2)

test/e2e/features/revision.feature (1)

9-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Drop the unused pvc-probe-sa setup
These scenarios no longer reference pvc-probe-sa in the ClusterObjectSet manifest, so the shared Given ServiceAccount ... step now creates an unused ServiceAccount/RBAC pair. Switch to the namespace-only setup used by the migrated scenarios below.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/e2e/features/revision.feature` around lines 9 - 16, Remove the unused
“Given ServiceAccount "pvc-probe-sa" with needed permissions” step from the
affected scenarios in revision.feature, and use the namespace-only setup
established by the migrated scenarios below. Keep the ClusterObjectSet manifests
and scenario behavior unchanged.

test/e2e/steps/asciicast_recorder.go (1)

139-147: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Unhandled error from f.Close().

defer f.Close() discards the close error. For a file write path, a failed close can mean data wasn't flushed to disk, so this should ideally be captured with a named return / explicit check rather than silently dropped.

As per path instructions for **/*.go: "Never ignore error returns."

♻️ Proposed fix

 	f, err := os.Create(r.castPath)
 	if err != nil {
 		return fmt.Errorf("failed to create cast file %s: %w", r.castPath, err)
 	}
-	defer f.Close()
+	defer func() {
+		if cerr := f.Close(); cerr != nil && err == nil {
+			err = fmt.Errorf("failed to close cast file %s: %w", r.castPath, cerr)
+		}
+	}()

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/e2e/steps/asciicast_recorder.go` around lines 139 - 147, Update
AsciiCastRecorder.Stop to handle the error returned by f.Close instead of
discarding it via defer. Use a named return or explicit close handling so close
failures are propagated while preserving existing errors from directory
creation, file creation, and recording.

Source: Path instructions

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/howto/how-to-protect-olmv1-api-access.md`:
- Around line 39-44: Update the RBAC verb lists in the installer role
documentation to include patch for both resource entries, alongside the existing
create, update, delete, get, list, and watch verbs. Ensure both resources
support declarative kubectl apply workflows.
- Line 71: Update the Markdown link in the ValidatingAdmissionPolicy discussion
to replace the vague “here” anchor with descriptive text identifying the
Kubernetes ValidatingAdmissionPolicy documentation, while preserving the
existing destination URL.

In `@internal/catalogd/graphql/validation.go`:
- Around line 16-21: Update query complexity traversal around
queryComplexity.visited and the fragment-recursion logic in validateQuery to
treat visited as a per-path set: mark a fragment before traversing its
definition, remove it after recursion completes, and skip only fragments already
active on the current path. Preserve cycle protection while allowing each
non-overlapping fragment spread to contribute its fields and aliases to the
operation totals.

In `@internal/catalogd/storage/localdir_test.go`:
- Around line 711-712: Update the test setup around NewLocalDirV1 and the
corresponding setup at the other URL parsing occurrence to capture the error
returned by url.Parse and assert it with require.NoError before using rootURL.
Do not discard the parsing error, including for constant URLs.

In `@internal/catalogd/storage/localdir.go`:
- Around line 344-356: Validate offset and limit before slicing sections in the
surrounding schema-section retrieval method, returning a validation error when
either pagination value is negative. Preserve the existing nil returns for
missing or exhausted sections and the current slicing behavior for non-negative
values.
- Around line 97-106: Update storeAtomicSwap’s schema warm-up flow to avoid
deleting the previously healthy catalog before validation completes. Either
validate the temporary catalog before replacing the existing directory, or
retain the old directory and restore it when graphqlSvc.GetSchema fails; ensure
rollback removes the new directory and leaves the prior catalog available.
- Around line 124-128: Update the cleanup paths in the local catalog storage
flow, including the deferred RemoveAll around MkdirTemp and the f.Close path
referenced by the comment, so their returned errors are explicitly handled
rather than discarded. Propagate cleanup or close failures when the surrounding
function can return an error; otherwise log them using the existing logging
mechanism, while preserving the current successful cleanup behavior.
- Around line 332-340: Update NewObjectLoader so the closure no longer captures
an index loaded before locking; acquire the snapshot RLock first, then load the
index through the lock-held helper before reading the catalog file. Preserve the
existing error handling and ensure both index and JSONL data come from the same
snapshot.

In `@openshift/operator-controller/manifests.yaml`:
- Around line 736-740: Replace the cluster-admin role reference in the
operator-controller-cluster-admin-rolebinding with a least-privilege custom or
aggregated ClusterRole that grants only the manager’s required permissions in
openshift/operator-controller/manifests.yaml:736-740 and
openshift/operator-controller/manifests-experimental.yaml:887-891. Apply the
same scoped RBAC change in both standard and experimental manifests, preserving
the Controller ServiceAccount binding.

---

Outside diff comments:
In `@internal/catalogd/service/graphql_service.go`:
- Around line 47-80: Make GetSchema honor its ctx by applying a bounded timeout
or cancellation to the schema-build operation, including the
buildGroup.Do/buildSchema path, so slow catalog loads cannot block indefinitely.
Prefer threading ctx through buildSchema and the
provider.LoadCatalogSchema/NewObjectLoader calls; otherwise enforce timeout
completion around buildGroup.Do. Preserve cache behavior and return context
cancellation/deadline errors.

---

Nitpick comments:
In `@test/e2e/features/revision.feature`:
- Around line 9-16: Remove the unused “Given ServiceAccount "pvc-probe-sa" with
needed permissions” step from the affected scenarios in revision.feature, and
use the namespace-only setup established by the migrated scenarios below. Keep
the ClusterObjectSet manifests and scenario behavior unchanged.

In `@test/e2e/steps/asciicast_recorder.go`:
- Around line 139-147: Update AsciiCastRecorder.Stop to handle the error
returned by f.Close instead of discarding it via defer. Use a named return or
explicit close handling so close failures are propagated while preserving
existing errors from directory creation, file creation, and recording.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Grant patch to the installer role.

kubectl apply and other declarative clients use PATCH for existing resources, so this role cannot perform the documented modification workflow. Add patch for both resources.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/howto/how-to-protect-olmv1-api-access.md` around lines 39 - 44, Update
the RBAC verb lists in the installer role documentation to include patch for
both resource entries, alongside the existing create, update, delete, get, list,
and watch verbs. Ensure both resources support declarative kubectl apply
workflows.

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Use descriptive link text.

Replace “here” with text such as “Kubernetes ValidatingAdmissionPolicy documentation.”

🧰 Tools

🪛 markdownlint-cli2 (0.23.0)

[warning] 71-71: Link text should be descriptive

(MD059, descriptive-link-text)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/howto/how-to-protect-olmv1-api-access.md` at line 71, Update the
Markdown link in the ValidatingAdmissionPolicy discussion to replace the vague
“here” anchor with descriptive text identifying the Kubernetes
ValidatingAdmissionPolicy documentation, while preserving the existing
destination URL.

Source: Linters/SAST tools

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Fragment-spread dedup lets query cost bypass the complexity limits it's meant to enforce.

c.visited is a single map reused for the whole operation and a fragment name is never unmarked once counted (Lines 79-84). This only guards against fragments that self-reference (cycles), but it also silently skips re-counting a fragment the second, third, ... Nth time it's spread in different, non-overlapping branches of the same query — even though the GraphQL executor (graphql.Do) will actually resolve that fragment's fields at every one of those spread sites. An attacker can define one deep/wide fragment and reuse it across many sibling fields to keep the reported fields/aliases counts under maxQueryFields/maxQueryAliases while the real execution cost multiplies — defeating the DoS protection this validator exists to provide.

Use a per-path "currently visiting" set (added before recursing into a fragment, removed after) instead of a permanent per-operation set, so cycles are still blocked but each distinct spread occurrence is fully counted.

🔒 Proposed fix: track visiting fragments per traversal path

 type queryComplexity struct {
 	aliases   int
 	fields    int
 	fragments map[string]*ast.FragmentDefinition
-	visited   map[string]bool
 }
@@
 	for _, def := range doc.Definitions {
 		if op, ok := def.(*ast.OperationDefinition); ok {
-			// Reset per operation so shared fragments are counted for each operation.
-			c.visited = make(map[string]bool)
-			if err := c.walkSelectionSet(op.SelectionSet, 1); err != nil {
+			if err := c.walkSelectionSet(op.SelectionSet, 1, make(map[string]bool)); err != nil {
 				return err
 			}
 		}
 	}
@@
-func (c *queryComplexity) walkSelectionSet(ss *ast.SelectionSet, depth int) error {
+func (c *queryComplexity) walkSelectionSet(ss *ast.SelectionSet, depth int, visiting map[string]bool) error {
 	...
-			if err := c.walkSelectionSet(s.SelectionSet, depth+1); err != nil {
+			if err := c.walkSelectionSet(s.SelectionSet, depth+1, visiting); err != nil {
 				return err
 			}
 		case *ast.InlineFragment:
-			if err := c.walkSelectionSet(s.SelectionSet, depth+1); err != nil {
+			if err := c.walkSelectionSet(s.SelectionSet, depth+1, visiting); err != nil {
 				return err
 			}
 		case *ast.FragmentSpread:
 			name := s.Name.Value
-			if c.visited[name] {
+			if visiting[name] {
 				continue
 			}
-			c.visited[name] = true
+			visiting[name] = true
 			if frag, ok := c.fragments[name]; ok {
-				if err := c.walkSelectionSet(frag.SelectionSet, depth+1); err != nil {
+				if err := c.walkSelectionSet(frag.SelectionSet, depth+1, visiting); err != nil {
 					return err
 				}
 			}
+			delete(visiting, name)
 		}

Also applies to: 41-42, 79-89

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/catalogd/graphql/validation.go` around lines 16 - 21, Update query
complexity traversal around queryComplexity.visited and the fragment-recursion
logic in validateQuery to treat visited as a per-path set: mark a fragment
before traversing its definition, remove it after recursion completes, and skip
only fragments already active on the current path. Preserve cycle protection
while allowing each non-overlapping fragment spread to contribute its fields and
aliases to the operation totals.

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Check the URL parsing errors.

Do not discard url.Parse errors, even for constants; assert them with require.NoError. As per path instructions, “Never ignore error returns.”

Also applies to: 764-765

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/catalogd/storage/localdir_test.go` around lines 711 - 712, Update
the test setup around NewLocalDirV1 and the corresponding setup at the other URL
parsing occurrence to capture the error returned by url.Parse and assert it with
require.NoError before using rootURL. Do not discard the parsing error,
including for constant URLs.

Source: Path instructions

[coderabbitai]

🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Preserve the previous catalog if schema warm-up fails.

storeAtomicSwap has already deleted the prior directory. This rollback only deletes the new directory, leaving a previously healthy catalog unavailable. Retain and restore the old directory, or validate the temporary catalog before replacing it.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/catalogd/storage/localdir.go` around lines 97 - 106, Update
storeAtomicSwap’s schema warm-up flow to avoid deleting the previously healthy
catalog before validation completes. Either validate the temporary catalog
before replacing the existing directory, or retain the old directory and restore
it when graphqlSvc.GetSchema fails; ensure rollback removes the new directory
and leaves the prior catalog available.

[coderabbitai]

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Handle deferred cleanup and close errors.

os.RemoveAll and f.Close errors are discarded. Propagate or log them so failed cleanup does not silently accumulate stale catalog data. As per path instructions, “Never ignore error returns.”

Also applies to: 358-362

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/catalogd/storage/localdir.go` around lines 124 - 128, Update the
cleanup paths in the local catalog storage flow, including the deferred
RemoveAll around MkdirTemp and the f.Close path referenced by the comment, so
their returned errors are explicitly handled rather than discarded. Propagate
cleanup or close failures when the surrounding function can return an error;
otherwise log them using the existing logging mechanism, while preserving the
current successful cleanup behavior.

Source: Path instructions

[coderabbitai]

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Load the index under the same snapshot lock as the catalog file.

The closure captures idx before it takes RLock. A loader retained from the previous cached schema can therefore apply old byte offsets to newly swapped JSONL data, returning incorrect objects or read errors. Load the index after acquiring the closure lock, using a lock-held helper.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/catalogd/storage/localdir.go` around lines 332 - 340, Update
NewObjectLoader so the closure no longer captures an index loaded before
locking; acquire the snapshot RLock first, then load the index through the
lock-held helper before reading the catalog file. Preserve the existing error
handling and ensure both index and JSONL data come from the same snapshot.

[coderabbitai]

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Reject negative pagination values before slicing.

offset = -1 reaches sections[offset:], and limit = -1 reaches sections[:limit]; both panic. Return a validation error when either value is negative.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/catalogd/storage/localdir.go` around lines 344 - 356, Validate
offset and limit before slicing sections in the surrounding schema-section
retrieval method, returning a validation error when either pagination value is
negative. Preserve the existing nil returns for missing or exhausted sections
and the current slicing behavior for non-negative values.

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

Controller ServiceAccount bound to cluster-admin in both standard and experimental manifests. The scoped operator-controller-manager-role binding was replaced by a cluster-admin ClusterRoleBinding. This is a deliberate consequence of removing SA-scoped installs (documented in the CRD deprecation text), but it broadens the workload's blast radius and conflicts with the least-privilege guideline. Confirm this is the accepted design and, if possible, scope to a custom/aggregated ClusterRole instead of cluster-admin.

• openshift/operator-controller/manifests.yaml#L736-L740: operator-controller-cluster-admin-rolebinding binds the manager SA to cluster-admin.
• openshift/operator-controller/manifests-experimental.yaml#L887-L891: identical cluster-admin binding for the experimental feature-set.

As per path instructions: "RBAC: least privilege; no cluster-admin for workloads".

📍 Affects 2 files

• openshift/operator-controller/manifests.yaml#L736-L740 (this comment)
• openshift/operator-controller/manifests-experimental.yaml#L887-L891

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@openshift/operator-controller/manifests.yaml` around lines 736 - 740, Replace
the cluster-admin role reference in the
operator-controller-cluster-admin-rolebinding with a least-privilege custom or
aggregated ClusterRole that grants only the manager’s required permissions in
openshift/operator-controller/manifests.yaml:736-740 and
openshift/operator-controller/manifests-experimental.yaml:887-891. Apply the
same scoped RBAC change in both standard and experimental manifests, preserving
the Controller ServiceAccount binding.

Source: Path instructions

[tmshort]

Closing in favor of merged #767