Fix: Grant execution SA base read access via shared ClusterRoleBinding#282
Fix: Grant execution SA base read access via shared ClusterRoleBinding#282blublinsky wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (19)
📝 WalkthroughWalkthroughThis PR adds reader ClusterRoleBinding subject sync for execution ServiceAccounts, updates the controller RBAC to allow binding updates, and adjusts tests/fixtures to include the shared reader binding. ChangesShared reader ClusterRoleBinding subject sync
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Reconciler
participant ensureExecutionSA
participant addReaderSubject
participant cleanupExecutionRBAC
participant removeReaderSubject
participant ClusterRoleBinding
Reconciler->>ensureExecutionSA: create execution ServiceAccount
ensureExecutionSA->>addReaderSubject: add ServiceAccount subject
addReaderSubject->>ClusterRoleBinding: fetch and update subjects
Reconciler->>cleanupExecutionRBAC: delete proposal RBAC
cleanupExecutionRBAC->>removeReaderSubject: remove ServiceAccount subject
removeReaderSubject->>ClusterRoleBinding: fetch and update subjects
Related issues: None provided. Related PRs: None provided. Suggested labels: rbac, controller Suggested reviewers: None provided. Poem: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@controller/proposal/rbac.go`:
- Around line 250-253: The `removeReaderSubject` callsite in `proposal/rbac.go`
is redundantly wrapping an error that already includes `ErrRemoveReaderSubject`,
which can duplicate the prefix in the final message. Update the
`executionSAName`/`removeReaderSubject` block to return the error directly,
matching the `addReaderSubject` callsite style for consistency, and leave the
existing wrapping inside `removeReaderSubject` as the single source of context.
In `@controller/proposal/reconciler.go`:
- Line 48: The manager RBAC on the proposal reconciler is missing permissions
needed by addReaderSubject/removeReaderSubject to modify the shared reader
binding. Update the kubebuilder RBAC annotation in the reconciler that defines
the manager ClusterRole so it includes update on clusterrolebindings, and add
bind on the shared cluster-reader ClusterRole if the operator service account
may need to attach subjects to it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7dfb8b7f-7f68-4b93-acaf-b5f7baefc56f
📒 Files selected for processing (2)
controller/proposal/rbac.gocontroller/proposal/reconciler.go
blublinsky
force-pushed
the
execution-sa-fix
branch
from
13095d2 to
833a119
Compare
blublinsky
force-pushed
the
execution-sa-fix
branch
2 times, most recently
from
5fea87a to
01b8282
Compare
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@controller/proposal/rbac.go`:
- Around line 148-156: `removeReaderSubject` only checks `readerRoleBinding` and
returns early on NotFound, so it can miss a renamed binding and leave the
subject behind. Update `removeReaderSubject` to mirror `addReaderSubject` by
using `discoverReaderBinding` as a fallback when the default binding name is not
found, then retry the removal against the discovered ClusterRoleBinding before
returning. Ensure the lookup and cleanup logic in `removeReaderSubject` stays
consistent with `addReaderSubject` and the shared `readerRoleBinding` handling.
- Line 38: The shared readerRoleBinding state is unsafe because
discoverReaderBinding writes it while addReaderSubject and removeReaderSubject
read it during concurrent reconciles, causing a data race and stale process-wide
binding names. Fix this by removing the package-level mutable variable in
rbac.go and making the binding name local to each reconcile flow, or by guarding
all access with synchronization such as a mutex or atomic. Update the call paths
in discoverReaderBinding, addReaderSubject, and removeReaderSubject to pass the
binding name explicitly rather than relying on shared global state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f0fd90d8-ba5e-41dd-aa43-4ac8e046199b
⛔ Files ignored due to path filters (1)
config/rbac/role.yamlis excluded by!config/rbac/role.yaml
📒 Files selected for processing (4)
controller/proposal/rbac.gocontroller/proposal/rbac_test.gocontroller/proposal/reconciler.gocontroller/proposal/reconciler_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/lightspeed-agentic-sandbox(manual)
✅ Files skipped from review due to trivial changes (1)
- controller/proposal/reconciler.go
| ) | ||
|
|
||
| // +kubebuilder:rbac:groups="",resources=serviceaccounts,verbs=create;delete;get | ||
| var readerRoleBinding = defaultReaderClusterRoleBinding |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- controller/proposal/rbac.go ---'
cat -n controller/proposal/rbac.go | sed -n '1,240p'
echo
echo '--- search MaxConcurrentReconciles ---'
rg -n "MaxConcurrentReconciles|controller\.Options|NewControllerManagedBy|Owns\(" controller config .ai -g '!**/vendor/**'
echo
echo '--- search readerRoleBinding usages ---'
rg -n "readerRoleBinding|discoverReaderBinding|addReaderSubject|removeReaderSubject" controller/proposal -g '!**/vendor/**'Repository: openshift/lightspeed-agentic-operator
Length of output: 13841
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- controller/proposal/reconciler.go (around controller setup) ---'
cat -n controller/proposal/reconciler.go | sed -n '230,280p'
echo
echo '--- search maxConcurrent source ---'
rg -n "maxConcurrent" controller main cmd . -g '!**/vendor/**'Repository: openshift/lightspeed-agentic-operator
Length of output: 2795
Guard readerRoleBinding before concurrent reconciles race here. readerRoleBinding is written in discoverReaderBinding and read in addReaderSubject / removeReaderSubject with no synchronization. controller/proposal/reconciler.go sets MaxConcurrentReconciles from ApprovalPolicy, so this is an active data race, and the discovered binding name can also stick as stale process-wide state for later proposals. Make the binding name per-reconcile or protect it with a mutex/atomic.
🤖 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 `@controller/proposal/rbac.go` at line 38, The shared readerRoleBinding state
is unsafe because discoverReaderBinding writes it while addReaderSubject and
removeReaderSubject read it during concurrent reconciles, causing a data race
and stale process-wide binding names. Fix this by removing the package-level
mutable variable in rbac.go and making the binding name local to each reconcile
flow, or by guarding all access with synchronization such as a mutex or atomic.
Update the call paths in discoverReaderBinding, addReaderSubject, and
removeReaderSubject to pass the binding name explicitly rather than relying on
shared global state.
| func removeReaderSubject(ctx context.Context, c client.Client, saName, namespace string) error { | ||
| for attempts := 0; attempts < 3; attempts++ { | ||
| crb := &rbacv1.ClusterRoleBinding{} | ||
| if err := c.Get(ctx, client.ObjectKey{Name: readerRoleBinding}, crb); err != nil { | ||
| if apierrors.IsNotFound(err) { | ||
| return nil | ||
| } | ||
| return fmt.Errorf("%s: %w", ErrRemoveReaderSubject, err) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
removeReaderSubject has no discovery fallback, unlike addReaderSubject — orphaned subjects possible.
addReaderSubject calls discoverReaderBinding when the default binding name isn't found (Lines 119-124), but removeReaderSubject simply returns nil on NotFound (Lines 152-154). If the reader binding was ever renamed (triggering discovery during setup) and the process later restarts — resetting readerRoleBinding back to defaultReaderClusterRoleBinding — cleanup will silently no-op against a binding name that doesn't exist, leaving the execution SA subject dangling on the real (renamed) binding indefinitely.
♻️ Proposed fix to mirror `addReaderSubject`'s fallback
if err := c.Get(ctx, client.ObjectKey{Name: readerRoleBinding}, crb); err != nil {
if apierrors.IsNotFound(err) {
- return nil
+ if discoverErr := discoverReaderBinding(ctx, c, namespace); discoverErr != nil {
+ // Binding genuinely doesn't exist anywhere — nothing to remove.
+ return nil
+ }
+ continue
}
return fmt.Errorf("%s: %w", ErrRemoveReaderSubject, err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func removeReaderSubject(ctx context.Context, c client.Client, saName, namespace string) error { | |
| for attempts := 0; attempts < 3; attempts++ { | |
| crb := &rbacv1.ClusterRoleBinding{} | |
| if err := c.Get(ctx, client.ObjectKey{Name: readerRoleBinding}, crb); err != nil { | |
| if apierrors.IsNotFound(err) { | |
| return nil | |
| } | |
| return fmt.Errorf("%s: %w", ErrRemoveReaderSubject, err) | |
| } | |
| func removeReaderSubject(ctx context.Context, c client.Client, saName, namespace string) error { | |
| for attempts := 0; attempts < 3; attempts++ { | |
| crb := &rbacv1.ClusterRoleBinding{} | |
| if err := c.Get(ctx, client.ObjectKey{Name: readerRoleBinding}, crb); err != nil { | |
| if apierrors.IsNotFound(err) { | |
| if discoverErr := discoverReaderBinding(ctx, c, namespace); discoverErr != nil { | |
| // Binding genuinely doesn't exist anywhere — nothing to remove. | |
| return nil | |
| } | |
| continue | |
| } | |
| return fmt.Errorf("%s: %w", ErrRemoveReaderSubject, err) | |
| } |
🤖 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 `@controller/proposal/rbac.go` around lines 148 - 156, `removeReaderSubject`
only checks `readerRoleBinding` and returns early on NotFound, so it can miss a
renamed binding and leave the subject behind. Update `removeReaderSubject` to
mirror `addReaderSubject` by using `discoverReaderBinding` as a fallback when
the default binding name is not found, then retry the removal against the
discovered ClusterRoleBinding before returning. Ensure the lookup and cleanup
logic in `removeReaderSubject` stays consistent with `addReaderSubject` and the
shared `readerRoleBinding` handling.
01b8282 to
5071a3d
Compare
|
@blublinsky: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
/approve |
openshift-ci
Bot
added
the
approved
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: blublinsky The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
2 participants
Summary
OLS-3248 introduced per-proposal ServiceAccounts for execution RBAC isolation, but inadvertently dropped base read access. The previous implementation used
lightspeed-agentSA (which already hadcluster-readerbound) for both analysis and execution. After OLS-3248, execution uses a fresh SA with only write permissions from the analysis RBAC output — breaking any execution script that reads resources (e.g.,kubectl get,kubectl rollout status).This fix adds the per-proposal execution SA as a subject to the existing
lightspeed-agent-cluster-readerClusterRoleBinding, giving it the same read access as the analysis SA. Subjects are added on execution start and removed on cleanup.Changes
controller/proposal/rbac.goaddReaderSubject/removeReaderSubjecthelpers that manage subjects on the sharedlightspeed-agent-cluster-readerClusterRoleBinding. Both retry on conflict (optimistic concurrency). Called fromensureExecutionSAandcleanupExecutionRBAC.controller/proposal/reconciler.goupdateverb to the ClusterRoleBinding RBAC marker.Execution SA permissions (before → after)
cluster-readervia shared bindingTest plan
go build ./...— compilesmake manifests—role.yamlincludesupdateon clusterrolebindingsmake test— unit tests pass