OCPBUGS-94182: narrow down RBAC permissions aggressively#4639
Conversation
Reduce the permission range by pinning RBAC rules to the object names we actually work with. AI-attribution: AIA PAI Ce Hin R claude-4.6-opus-1M v1.0 Signed-off-by: Francesco Romani <fromani@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ffromani 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 |
openshift-ci
Bot
added
the
approved
ffromani
changed the title
|
@ffromani: This pull request references Jira Issue OCPBUGS-94182, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
Warning Review limit reached
Next review available in: 45 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: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe change narrows operator and scheduler RBAC, removes controller event recording, configures manager cache namespaces, removes the legacy scheduler binding from desired manifests, and adds startup cleanup for that binding. ChangesOperator RBAC and scheduler lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Manager
participant RunnableFunc
participant schedstate.Cleanup
participant KubernetesAPI
Manager->>RunnableFunc: Register cleanup runnable
RunnableFunc->>schedstate.Cleanup: Cleanup(ctx, client)
schedstate.Cleanup->>KubernetesAPI: Delete secondary-scheduler ClusterRoleBinding
KubernetesAPI-->>schedstate.Cleanup: Return success or not-found
schedstate.Cleanup-->>RunnableFunc: Return cleanup result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (1)
pkg/objectupdate/rte/rte.go (1)
311-317: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
isPodStatusRuledeletes entire rule on partial match.If a
PolicyRuleever combinespodsandpods/statusin the sameResourceslist,slices.DeleteFuncremoves the whole rule, unintentionally droppingpodspermissions. This is unlikely given how the vendored manifest structures rules, but consider filtering only thepods/statusentry fromrule.Resourcesinstead of deleting the entire rule, or add a test case covering mixed 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 `@pkg/objectupdate/rte/rte.go` around lines 311 - 317, Update RoleRules so it removes only the "pods/status" entry from each PolicyRule.Resources list, preserving other resources such as "pods"; avoid deleting the entire rule when isPodStatusRule matches a mixed-resource rule, and retain rules only when their resources remain valid.
🤖 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 `@cmd/main.go`:
- Around line 426-431: Update the manager cleanup runnable registration around
manager.RunnableFunc so schedstate.Cleanup errors are logged as best-effort
failures and the runnable returns nil instead of propagating them through
mgr.Start. Preserve the existing startup error handling for failures from
mgr.Add itself.
- Around line 345-349: Update the ctrl.NewManager cache configuration in
cmd/main.go to include openshift-config-managed alongside namespace, or change
the kubeletconfig controller’s cached List to use the manager’s APIReader.
Ensure the HyperShift ConfigMap lookup in the kubeletconfig reconciliation path
can read from openshift-config-managed without failing due to an uncached
namespace.
---
Nitpick comments:
In `@pkg/objectupdate/rte/rte.go`:
- Around line 311-317: Update RoleRules so it removes only the "pods/status"
entry from each PolicyRule.Resources list, preserving other resources such as
"pods"; avoid deleting the entire rule when isPodStatusRule matches a
mixed-resource rule, and retain rules only when their resources remain valid.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 09f21087-2ec7-4b97-9025-eb9bb105d753
📒 Files selected for processing (21)
bundle/manifests/numaresources-operator.clusterserviceversion.yamlcmd/main.goconfig/rbac/role.yamlhack/verify-generated.shinternal/controller/kubeletconfig_controller.gointernal/controller/kubeletconfig_controller_test.gointernal/controller/numaresourcesoperator_controller.gointernal/controller/numaresourcesoperator_controller_test.gointernal/controller/numaresourcesscheduler_controller.gointernal/controller/operand_rbac_rte.gointernal/controller/operand_rbac_sched.gopkg/numaresourcesscheduler/manifests/manifests.gopkg/numaresourcesscheduler/manifests/manifests_test.gopkg/numaresourcesscheduler/manifests/sched/sched.gopkg/numaresourcesscheduler/manifests/yaml/clusterrole.nrt.yamlpkg/numaresourcesscheduler/manifests/yaml/clusterrolebinding.yamlpkg/numaresourcesscheduler/objectstate/sched/sched.gopkg/numaresourcesscheduler/objectstate/sched/sched_test.gopkg/objectupdate/rte/rte.gopkg/objectupdate/rte/rte_test.gopkg/objectupdate/sched/sched.go
💤 Files with no reviewable changes (6)
- pkg/objectupdate/sched/sched.go
- pkg/numaresourcesscheduler/manifests/sched/sched.go
- pkg/numaresourcesscheduler/manifests/yaml/clusterrolebinding.yaml
- pkg/numaresourcesscheduler/manifests/manifests.go
- pkg/numaresourcesscheduler/manifests/manifests_test.go
- internal/controller/numaresourcesoperator_controller_test.go
Tal-or
left a comment
Tal-or
left a comment
There was a problem hiding this comment.
Some changes required to avoid breaking operation on HyperShift.
The u/s CI passing doesn't provide a strong evidence here because it's not using the updated RBAC rules.
from the must-gather extra I pulled:
"apiVersion": "rbac.authorization.k8s.io/v1",
"kind": "Role",
"metadata": {
"creationTimestamp": "2026-07-13T17:18:28Z",
"labels": {
"olm.managed": "true",
"olm.owner": "numaresources-operator.v5.0.999-snapshot",
"olm.owner.kind": "ClusterServiceVersion",
"olm.owner.namespace": "numaresources-operator",
...
"operators.coreos.com/numaresources-operator.numaresources-operator": ""
},
...
"rules": [
...
{
"apiGroups": [
""
],
"resources": [
"events"
],
"verbs": [
"create",
"patch"
]
},
Which IIRC this PR removes events completely
| } | ||
|
|
||
| // Namespace Scoped | ||
| //+kubebuilder:rbac:groups="",resources=configmaps,verbs=create;delete;get;list;update;watch,namespace="numaresources" |
There was a problem hiding this comment.
That would break NROP on HyperShift, we should add the openshift-config-managed namespace.
There was a problem hiding this comment.
thanks for pointing this out. I think I addressed in the last commit.
quite some permissions we need are actually namespaced scope; narrow down accordingly AI-attribution: AIA PAI Ce Hin R claude-4.6-opus-1M v1.0 Signed-off-by: Francesco Romani <fromani@redhat.com>
At the beginning of this project, we added events attempting to improve UX. But events are actually useful only if you either interactive watch them while the operator is doing stuff or if you store somewhere durable. So the usefulness of the events - especially how we use them, was already questionable. In addition, we need permissions on the `default` namespace, as byproduct of the machinery we use to send events. We can fix the latter with new faciities provided by the controller-runtime, but given our usage of events were already questionable in the first place, we take the chance to just remove them, reducing both the code and the RBAC footprint. AI-attribution: AIA PAI Ce Hin R claude-4.6-opus-1M v1.0 Signed-off-by: Francesco Romani <fromani@redhat.com>
Up until now, the scheduler was bound to 2 cluster roles: 1. the standard `system:kube-scheduler` granting baseline permissions for scheduler-compatible operation 2. the custom `topology-aware-scheduler` enabling the numa-aware scheduler plugin NodeTopologyMatch. While this worked, it created unnecessary large permissions requirements _and_ coupling to the main scheduler RBAC rules. With this change, we 1. merge the kube-scheduler clusterrole permissions into our custom clusterrole permissions. The scheduler is now independent and easier to audit 2. narrow down as much as possible the clusterrole permissions, matching only the objects which the profile we support requires. 3. make sure the old clusterrole binding to kube-scheduler is deleted This enables us to have a less privileged scheduler with a much smaller RBAC footprint. It must be noted that to work at all the scheduler still needs a quite large set of RBAC write permissions, but this is unavoidable and driven by how secondary scheduler work in general, e.g. is not numa-aware specific. AI-attribution: AIA PAI Ce Hin R claude-4.6-opus-1M v1.0 Signed-off-by: Francesco Romani <fromani@redhat.com>
run: `make generate bundle manifests` Signed-off-by: Francesco Romani <fromani@redhat.com>
The operator needs to hold permissions actually required by its operand in order to be able to grant them. But we can at very least split them in separate files to clearly distinguish the RBAC (through the kubebuilder annotations) the operator truly needs from what it needs just to pass through the operands. AI-attribution: AIA PAI Ce Hin R claude-4.6-opus-1M v1.0 Signed-off-by: Francesco Romani <fromani@redhat.com>
we need to capture and bubble up the actual exit value of git diff vs rely on the output or lack thereof. Signed-off-by: Francesco Romani <fromani@redhat.com>
069c6ea to
25572ae
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
config/rbac/role.yaml (1)
119-131: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDrop
deletecollectionfromleasecandidates(and the matching scheduler ClusterRole rule).deleteis enough here, and the bulk verb widens the RBAC surface unnecessarily.🤖 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 `@config/rbac/role.yaml` around lines 119 - 131, Remove the deletecollection verb from the leasecandidates rule in the role configuration and from the matching scheduler ClusterRole rule, while preserving the existing delete permission and all other verbs.
🤖 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.
Nitpick comments:
In `@config/rbac/role.yaml`:
- Around line 119-131: Remove the deletecollection verb from the leasecandidates
rule in the role configuration and from the matching scheduler ClusterRole rule,
while preserving the existing delete permission and all other verbs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: f452a659-c1c9-4a0f-9d9e-fe1f0866a89c
📒 Files selected for processing (19)
bundle/manifests/numaresources-operator.clusterserviceversion.yamlcmd/main.goconfig/rbac/role.yamlhack/verify-generated.shinternal/controller/kubeletconfig_controller.gointernal/controller/kubeletconfig_controller_test.gointernal/controller/numaresourcesoperator_controller.gointernal/controller/numaresourcesoperator_controller_test.gointernal/controller/numaresourcesscheduler_controller.gointernal/controller/operand_rbac_rte.gointernal/controller/operand_rbac_sched.gopkg/numaresourcesscheduler/manifests/manifests.gopkg/numaresourcesscheduler/manifests/manifests_test.gopkg/numaresourcesscheduler/manifests/sched/sched.gopkg/numaresourcesscheduler/manifests/yaml/clusterrole.nrt.yamlpkg/numaresourcesscheduler/manifests/yaml/clusterrolebinding.yamlpkg/numaresourcesscheduler/objectstate/sched/sched.gopkg/numaresourcesscheduler/objectstate/sched/sched_test.gopkg/objectupdate/sched/sched.go
💤 Files with no reviewable changes (6)
- pkg/numaresourcesscheduler/manifests/manifests_test.go
- pkg/objectupdate/sched/sched.go
- pkg/numaresourcesscheduler/manifests/manifests.go
- pkg/numaresourcesscheduler/manifests/sched/sched.go
- pkg/numaresourcesscheduler/manifests/yaml/clusterrolebinding.yaml
- internal/controller/numaresourcesoperator_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (10)
- internal/controller/operand_rbac_rte.go
- hack/verify-generated.sh
- pkg/numaresourcesscheduler/objectstate/sched/sched_test.go
- internal/controller/numaresourcesscheduler_controller.go
- bundle/manifests/numaresources-operator.clusterserviceversion.yaml
- pkg/numaresourcesscheduler/objectstate/sched/sched.go
- pkg/numaresourcesscheduler/manifests/yaml/clusterrole.nrt.yaml
- internal/controller/kubeletconfig_controller.go
- cmd/main.go
- internal/controller/kubeletconfig_controller_test.go
On HyperShift, we need to read configmaps from different namespaces, namely from the `openshift-config-managed` namespace. The only viable option is to move back the ConfigMap read permission as cluster role, while keeping the write/mutation permission as namespaced role. Ideally, we would have injected a specific role+rolebinding in the manifest bundle, but turns out that OLM always overwrites the namespace in the namespaced manifests (and that would perfectly make sense) so we cna't add permissions about a different namespace. AI-attribution: AIA PAI Ce Hin R claude-4.6-opus-1M v1.0 Signed-off-by: Francesco Romani <fromani@redhat.com>
25572ae to
8bdd0a7
Compare
|
/retest-required |
|
/retest |
3 participants
Stronger version of #4230.
Narrow down more aggressively the RBAC footprint, narrowing down cluster to namespace permissions, listing verbs explicitly and pinning as much as possible to resource names.
Two key decisions made with the intent to narrow down RBAC further