MG-61: minimal possible get/list RBAC for ServiceAccount running the Job#264
Draft
swghosh wants to merge 4 commits intoopenshift:masterfrom
Draft
MG-61: minimal possible get/list RBAC for ServiceAccount running the Job#264swghosh wants to merge 4 commits intoopenshift:masterfrom
swghosh wants to merge 4 commits intoopenshift:masterfrom
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: swghosh 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
Signed-off-by: Swarup Ghosh <swghosh@redhat.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #264 +/- ##
=======================================
Coverage 38.02% 38.02%
=======================================
Files 7 7
Lines 476 476
=======================================
Hits 181 181
Misses 292 292
Partials 3 3 🚀 New features to boost your workflow:
|
swghosh
commented
Sep 5, 2025
Comment on lines
299
to
298
| - create # For temporary perf-node-gather-daemonset | ||
| - delete # For cleanup of temporary daemonset |
Member
Author
There was a problem hiding this comment.
DaemonSet "create/delete" is too risky.
Comment on lines
30
to
32
| - apiGroups: [""] | ||
| resources: | ||
| - pods/exec | ||
| verbs: | ||
| - create # Required for oc exec |
Member
Author
There was a problem hiding this comment.
pods/exec with "create" verb could be risky as well.
Comment on lines
328
to
333
| - apiGroups: ["security.openshift.io"] | ||
| resources: | ||
| - securitycontextconstraints | ||
| verbs: | ||
| - get | ||
| - list |
Member
Author
There was a problem hiding this comment.
A "get/list" over the SCC object does not make the cluster-policy-controller to escalate the default security context constraint label on the namespace where Job is run.
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughRenames the ClusterRole from must-gather-admin to must-gather-minimal, replaces a wildcard-allow rule with granular RBAC across many API groups/resources, adds limited non-resource URL access, and updates corresponding ClusterRoleBindings. Introduces an example ClusterRole manifest mirroring the granular permissions and adjusts the example binding to reference it. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing touches🧪 Generate unit tests
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: 10
♻️ Duplicate comments (3)
examples/other_resources/05_must-gather-minimal.ClusterRole.yaml (1)
286-299: DaemonSet create/delete is high-risk; split into a separate opt-in roleCreating/deleting DaemonSets cluster-wide is powerful and not “minimal.” Prior feedback already flagged this.
Apply this to remove the lifecycle verbs here:
- daemonsets verbs: - get - list - - create # For temporary perf-node-gather-daemonset - - delete # For cleanup of temporary daemonsetCreate a separate ClusterRole (e.g., must-gather-perf) with only apps/daemonsets {create,delete,get,list} that operators can bind temporarily when running the perf gather.
deploy/05_must-gather-admin.ClusterRole.yaml (2)
34-37: pods/portforward comment incorrect; verify actual needoc cp uses exec, not port-forward. Drop or fix comment and rule if unused.
286-299: DaemonSet create/delete is not minimalRemove here; create an optional, separate role for perf gather.
🧹 Nitpick comments (11)
examples/other_resources/05_must-gather-minimal.ClusterRole.yaml (5)
34-37: pods/portforward “Required for oc cp” is incorrect; verify necessitykubectl/oc cp uses exec, not port-forward. If port-forward isn’t actually used by the job, drop this rule; otherwise fix the comment.
Suggested change:
- - create # Required for oc cp + - create # Required only if oc port-forward is usedOr remove the whole pods/portforward rule if not needed.
300-306: NonResourceURLs scope is cluster-wide; consider isolating/metrics and /debug/api_priority_and_fairness/* grant apiserver-wide access. If not mandatory for the baseline gather, move them to a separate optional role and bind only when required.
24-25: Duplicate serviceaccounts ruleserviceaccounts get/list appears twice. Keep one.
Apply either removal:
- - serviceaccountsor
- - apiGroups: [""] - resources: - - serviceaccounts - verbs: - - get - - listAlso applies to: 359-364
286-291: Duplicate coverage of apps/deploymentsdeployments are granted at 286-291 and again at 380-388. Consolidate to one block.
Apply:
- - apiGroups: ["apps"] - resources: - - deployments - verbs: - - get # To find insights operator deploymentKeep the consolidated apps block later.
Also applies to: 380-388
321-327: users and groups read can expose sensitive identity dataValidate that must-gather truly needs cluster-wide get/list on users and groups. If not essential, remove.
deploy/05_must-gather-admin.ClusterRole.yaml (4)
300-306: NonResourceURLs: consider isolatingMove /metrics and debug endpoints to an optional role if not strictly required.
24-25: Duplicate serviceaccounts ruleDeduplicate.
Also applies to: 359-364
286-291: Duplicate deployments coverageConsolidate to a single apps block.
Also applies to: 380-388
321-327: users/groups read—validate necessityConfirm it’s required; otherwise remove to reduce data exposure.
deploy/06_must-gather-admin.ClusterRoleBinding.yaml (1)
7-7: Binding now targets must-gather-minimal—ensure role exists at apply timeVerify deploy order so the ClusterRole is created before this binding; otherwise apply will fail.
You may also consider renaming metadata.name (and the ServiceAccount name) to align with “-minimal” to avoid confusion, keeping a legacy binding only if needed.
examples/other_resources/06_must-gather-admin.ClusterRoleBinding.yaml (1)
7-7: Example binding points to must-gather-minimal—validate SA/namespaceEnsure the referenced ServiceAccount and namespace match the example deployment, or adjust for clarity.
Optionally rename the binding to must-gather-minimal for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (4)
deploy/05_must-gather-admin.ClusterRole.yaml(1 hunks)deploy/06_must-gather-admin.ClusterRoleBinding.yaml(1 hunks)examples/other_resources/05_must-gather-minimal.ClusterRole.yaml(1 hunks)examples/other_resources/06_must-gather-admin.ClusterRoleBinding.yaml(1 hunks)
🔇 Additional comments (3)
examples/other_resources/05_must-gather-minimal.ClusterRole.yaml (1)
328-334: SCC read-only is fineget/list on securitycontextconstraints is safe and matches prior guidance that it won’t escalate defaults.
deploy/05_must-gather-admin.ClusterRole.yaml (2)
2-4: Principle-of-least-privilege improvementRenaming and replacing the wildcard “//*” with granular rules is a solid step toward minimal RBAC.
328-334: SCC read-only is appropriateLGTM for visibility without escalation.
Comment on lines
18
to
20
| - configmaps | ||
| - secrets # Limited to service account tokens | ||
| - events |
There was a problem hiding this comment.
Broad secrets read; tighten
Same concern as the examples file: this grants get/list on all Secrets. Remove and use namespaced Roles + RoleBindings where specific Secrets are needed.
Apply:
- configmaps
- - secrets # Limited to service account tokens
- events📝 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.
Suggested change
| - configmaps | |
| - secrets # Limited to service account tokens | |
| - events | |
| # in deploy/05_must-gather-admin.ClusterRole.yaml, around lines 18–20 | |
| resources: | |
| - configmaps | |
| - events |
🤖 Prompt for AI Agents
In deploy/05_must-gather-admin.ClusterRole.yaml around lines 18 to 20, the
ClusterRole currently grants broad get/list on all Secrets (the "- secrets"
entry); remove that blanket secret permission and instead restrict access by
creating namespaced Roles that grant get/list only on the specific Secret names
required, then bind them with RoleBindings to the service accounts in those
namespaces; if cluster-wide read of certain secrets is truly required, replace
the wildcard with a tightly-scoped rule listing only the exact secret
resourceNames and document why each is necessary.
Comment on lines
45
to
55
| resources: | ||
| - storageclasses | ||
| - volumeattachments | ||
| - csidrivers | ||
| - csinodes | ||
| - volumesnapshotclasses | ||
| - volumesnapshotcontents | ||
| - csistoragecapacities | ||
| verbs: | ||
| - get | ||
| - list |
There was a problem hiding this comment.
Fix VolumeSnapshot API group
Move volumesnapshot* to snapshot.storage.k8s.io as in prior comment.
🤖 Prompt for AI Agents
In deploy/05_must-gather-admin.ClusterRole.yaml around lines 45-55, the
VolumeSnapshot resources are listed without their API group; change
volumesnapshotclasses and volumesnapshotcontents to include the
snapshot.storage.k8s.io API group (e.g.
volumesnapshotclasses.snapshot.storage.k8s.io and
volumesnapshotcontents.snapshot.storage.k8s.io) so RBAC targets the correct
resources; keep the same verbs.
Comment on lines
75
to
81
| - apiGroups: [""] | ||
| resources: | ||
| - projects # OpenShift namespaces | ||
| verbs: | ||
| - get | ||
| - list | ||
| - apiGroups: ["config.openshift.io"] |
There was a problem hiding this comment.
Projects API group correction
Use project.openshift.io for projects.
🤖 Prompt for AI Agents
In deploy/05_must-gather-admin.ClusterRole.yaml around lines 75 to 81, the RBAC
rule currently uses the wrong API group for projects; replace the existing
apiGroups entry (either "" or "config.openshift.io") for the projects resource
with apiGroups: ["project.openshift.io"] so the rule targets the correct
OpenShift projects API, keeping resources: ["projects"] and verbs:
["get","list"] unchanged.
Comment on lines
341
to
352
| resources: | ||
| - configmaps | ||
| - secrets | ||
| resourceNames: | ||
| - cluster-monitoring-config | ||
| - alertmanager-main | ||
| verbs: | ||
| - get | ||
| namespaces: | ||
| - openshift-monitoring | ||
| - openshift-user-workload-monitoring | ||
| - apiGroups: ["*"] |
There was a problem hiding this comment.
Invalid “namespaces” selector in PolicyRule
RBAC PolicyRule doesn’t support namespaces. Use namespaced Roles + RoleBindings.
🤖 Prompt for AI Agents
In deploy/05_must-gather-admin.ClusterRole.yaml around lines 341-352, the
PolicyRule incorrectly includes a "namespaces" selector (RBAC PolicyRule doesn't
support namespaces). Remove the "namespaces" field from this ClusterRole and
instead create two separate namespaced Role objects (one for
openshift-monitoring and one for openshift-user-workload-monitoring) that
contain the same resources/resourceNames/verbs, then create RoleBindings in each
namespace to bind the Role to the appropriate subjects; alternatively, if
cluster-wide access is intended, simply omit the "namespaces" key entirely from
the ClusterRole so only supported fields remain.
Comment on lines
352
to
358
| - apiGroups: ["*"] | ||
| resources: | ||
| - "*/status" | ||
| - "*/scale" | ||
| verbs: | ||
| - get | ||
| - apiGroups: [""] |
There was a problem hiding this comment.
Invalid wildcard subresource patterns
Remove “/status” and “/scale” or enumerate explicit resources.
🤖 Prompt for AI Agents
In deploy/05_must-gather-admin.ClusterRole.yaml around lines 352 to 358, the
ClusterRole uses invalid wildcard subresource patterns ("*/status" and
"*/scale"); remove those wildcard subresources or replace them with explicit
resource/subresource entries (e.g., specify apiGroups and exact resources like
"deployments/status", "deployments/scale", "replicasets/status", etc.) and
ensure apiGroups is set correctly for each entry rather than using a wildcard;
update the resources block to list only valid explicit resource or
resource/subresource strings and adjust apiGroups accordingly.
Comment on lines
18
to
20
| - configmaps | ||
| - secrets # Limited to service account tokens | ||
| - events |
There was a problem hiding this comment.
Broad read on secrets; comment is misleading and over-privileged
This grants get/list on all Secrets cluster-wide. The inline note “Limited to service account tokens” is not enforced by RBAC and is incorrect. This is sensitive and exceeds “minimal.”
Apply this diff to drop the broad Secrets read from the core resources block:
- configmaps
- - secrets # Limited to service account tokens
- eventsIf you truly need two specific Secrets in monitoring namespaces, model them with namespace-scoped Roles + RoleBindings per namespace instead of a ClusterRole rule. Example (outside this file):
# Role in openshift-monitoring
kind: Role
apiVersion: rbac.authorization.k8s.io/v1
metadata:
name: must-gather-monitoring-read
namespace: openshift-monitoring
rules:
- apiGroups: [""]
resources: ["configmaps","secrets"]
resourceNames: ["cluster-monitoring-config","alertmanager-main"]
verbs: ["get"]Bind the SA to that Role in each namespace via RoleBinding.
🤖 Prompt for AI Agents
In examples/other_resources/05_must-gather-minimal.ClusterRole.yaml around lines
18-20 the ClusterRole grants get/list on all Secrets (the inline note “Limited
to service account tokens” is misleading and unenforced); remove secrets from
the core resources block to eliminate cluster-wide Secret access and delete the
misleading comment, and if access to specific Secrets in monitoring namespaces
is required, replace the ClusterRole rule with namespace-scoped Roles that list
only the exact resourceNames and bind the service account to those Roles via
RoleBindings in each target namespace.
examples/other_resources/05_must-gather-minimal.ClusterRole.yaml
Outdated
Show resolved
Hide resolved
examples/other_resources/05_must-gather-minimal.ClusterRole.yaml
Outdated
Show resolved
Hide resolved
examples/other_resources/05_must-gather-minimal.ClusterRole.yaml
Outdated
Show resolved
Hide resolved
Comment on lines
352
to
358
| - apiGroups: ["*"] | ||
| resources: | ||
| - "*/status" | ||
| - "*/scale" | ||
| verbs: | ||
| - get | ||
| - apiGroups: [""] |
There was a problem hiding this comment.
Invalid wildcard subresource patterns (“/status”, “/scale”)
RBAC doesn’t support wildcard prefix for resource names with subresources. Enumerate needed resources explicitly (e.g., deployments/scale) or drop this rule if not required.
Apply:
- - apiGroups: ["*"]
- resources:
- - "*/status"
- - "*/scale"
- verbs:
- - getIf you need scale for specific workloads, add explicit rules like apps/deployments/scale: get.
📝 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.
Suggested change
| - apiGroups: ["*"] | |
| resources: | |
| - "*/status" | |
| - "*/scale" | |
| verbs: | |
| - get | |
| - apiGroups: [""] | |
| - apiGroups: [""] |
🤖 Prompt for AI Agents
In examples/other_resources/05_must-gather-minimal.ClusterRole.yaml around lines
352-358, the ClusterRole uses invalid wildcard subresource patterns ("*/status",
"*/scale") which RBAC does not support; replace these wildcard subresource
entries by either removing the subresource rule entirely if not needed or
enumerating explicit resource/subresource pairs with correct apiGroups (for
example add resources like "deployments/scale" under apiGroups: ["apps"] and
"pods/status" under apiGroups: [""] as required), ensure apiGroups lines
correspond to each explicit resource, and update verbs as needed (e.g., get for
scale/status).
swghosh
force-pushed
the
minimal-rbac
branch
2 times, most recently
from
b0c5725 to
ca97b2f
Compare
and rm secrets access Signed-off-by: Swarup Ghosh <swghosh@redhat.com>
swghosh
force-pushed
the
minimal-rbac
branch
from
ca97b2f to
0508ae4
Compare
for must-gather-minimal CR, CRB Signed-off-by: Swarup Ghosh <swghosh@redhat.com>
Signed-off-by: Swarup Ghosh <swghosh@redhat.com>
Member
Author
|
using the must-gather-minimal ClusterRole applied (per https://github.com/swghosh/must-gather-operator/blob/ae6d67944b3b2e2c3c6e2a23038d50833018fec0/deploy/05_must-gather-admin.ClusterRole.yaml), Detailspod logs from gather containergather image: registry.redhat.io/openshift4/ose-must-gather-rhel9@sha256:16c67cfcae221ea9db9dcfce1c2efc0a6bfcdcff1c589513d7826cd91c37ca11 (v4.19) notably a possible restricted SCC violation occurs: |
swghosh
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@swghosh: This pull request references MG-61 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the spike to target the "4.21.0" version, but no target version was set. 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. |
Contributor
|
@swghosh: 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
experimental ClusterRole with minimally sufficient RBAC privileges to run an ose-must-gather from the Job's Pod.