OLS-3087: Replace wildcard RBAC verb on namespace-scoped roles/rolebindings#1788
OLS-3087: Replace wildcard RBAC verb on namespace-scoped roles/rolebindings#1788xrajesh wants to merge 2 commits into
Conversation
…ndings The namespace-scoped Role in openshift-lightspeed granted verb '*' on roles and rolebindings, which implicitly includes 'escalate' and 'bind'. Replace with the explicit verbs the operator actually uses: get, list, create, update, delete. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@xrajesh: This pull request references OLS-3087 which is a valid jira issue. DetailsIn 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. |
📝 WalkthroughWalkthroughThis change updates a kubebuilder RBAC annotation in the OLS config controller, narrowing the permitted verbs for ChangesRBAC Annotation Update
Estimated code review effort: 1 (Trivial) | ~2 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/controller/olsconfig_controller.go`:
- Line 132: The RBAC marker for the OlsConfig controller is missing the watch
verb for roles and rolebindings, while listOwnedResources() uses r.List() on
both resources. Update the kubebuilder RBAC annotation in OlsConfig controller
metadata to include watch for roles;rolebindings and avoid adding patch since it
is not used.
🪄 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: 57a1c8d4-fd9a-4bfa-ba25-24a8fcb21cf7
⛔ Files ignored due to path filters (1)
config/rbac/role.yamlis excluded by!config/rbac/role.yaml
📒 Files selected for processing (1)
internal/controller/olsconfig_controller.go
| // +kubebuilder:rbac:groups=operator.openshift.io,resources=consoles,verbs=watch;list;get;update | ||
| // +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterroles;clusterrolebindings;rolebindings,verbs=get;list;create;update;patch;delete;watch | ||
| // +kubebuilder:rbac:groups=rbac.authorization.k8s.io,namespace=openshift-lightspeed,resources=roles;rolebindings,verbs=* | ||
| // +kubebuilder:rbac:groups=rbac.authorization.k8s.io,namespace=openshift-lightspeed,resources=roles;rolebindings,verbs=get;list;create;update;delete |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate reconciler code touching Role/RoleBinding and inspect the client calls used.
rg -nP --type=go -C3 '\b(rbacv1\.(Role|RoleBinding)|Role\{|RoleBinding\{)' internal/controller | head -100
rg -nP --type=go -C2 '\.(Patch|Update)\(ctx' internal/controller | rg -B5 -A2 'Role'Repository: openshift/lightspeed-operator
Length of output: 8072
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Role/RoleBinding client usage in internal/controller ---'
rg -n --type=go '\b(rbacv1\.(Role|RoleBinding)|RoleList|RoleBindingList)\b|controllerutil\.(CreateOrPatch|CreateOrUpdate)|\.(Get|List|Create|Update|Patch|Delete)\(' internal/controller/olsconfig_controller.go internal/controller -g'*.go' | sed -n '1,220p'
echo
echo '--- SetupWithManager and cache/client options around OLSConfig controller ---'
sed -n '1,220p' internal/controller/olsconfig_controller.goRepository: openshift/lightspeed-operator
Length of output: 40647
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- SetupWithManager / watches / owns in internal/controller/olsconfig_controller.go ---'
rg -n --type=go '\bSetupWithManager\b|Owns\(|Watches\(' internal/controller/olsconfig_controller.go
sed -n '820,980p' internal/controller/olsconfig_controller.goRepository: openshift/lightspeed-operator
Length of output: 6883
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- SetupWithManager / watches / owns in internal/controller/olsconfig_controller.go ---'
rg -n --type=go '\bSetupWithManager\b|Owns\(|Watches\(' internal/controller/olsconfig_controller.go
echo
sed -n '820,980p' internal/controller/olsconfig_controller.goRepository: openshift/lightspeed-operator
Length of output: 6884
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Role/RoleBinding listOwnedResources usage ---'
rg -n --type=go 'listOwnedResources\(' internal/controller/olsconfig_controller.go internal/controller -g'*.go'
echo
echo '--- appserver role/rolebinding reconciliation calls ---'
sed -n '250,310p' internal/controller/appserver/reconciler.goRepository: openshift/lightspeed-operator
Length of output: 3397
Add watch to the roles;rolebindings RBAC marker listOwnedResources() calls r.List() on both types, so the marker needs watch; patch isn’t used here.
🤖 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/controller/olsconfig_controller.go` at line 132, The RBAC marker for
the OlsConfig controller is missing the watch verb for roles and rolebindings,
while listOwnedResources() uses r.List() on both resources. Update the
kubebuilder RBAC annotation in OlsConfig controller metadata to include watch
for roles;rolebindings and avoid adding patch since it is not used.
The initial wildcard-to-explicit verb replacement omitted watch and patch, which are required by controller-runtime informers and CreateOrUpdate. Without them the operator cannot reconcile OLSConfig into a working deployment, causing all e2e tests to cascade-fail. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@xrajesh: The following test failed, say
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. |
2 participants
Summary
*verb on the namespace-scoped Role forrolesandrolebindingsinopenshift-lightspeedwith explicit verbs:get,list,create,update,deleteescalateandbindpermissions that the operator never uses (SAR finding OLS-3087)make manifests; all unit tests passTest plan
make manifestsregeneratesconfig/rbac/role.yamlcorrectlymake testpasses — all controller packages green🤖 Generated with Claude Code
Summary by CodeRabbit