-
Notifications
You must be signed in to change notification settings - Fork 182
NO-JIRA: scc: give all authenticated users access to restricted-v3 #1935
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NO-JIRA: scc: give all authenticated users access to restricted-v3 #1935
Conversation
WalkthroughAdds a new Kubernetes RBAC manifest that creates a ClusterRoleBinding named Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 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 (1)
bindata/bootkube/scc-manifests/0000_20_kube-apiserver-operator_00_crb-systemauthenticated-scc-restricted-v3.yaml(1 hunks)
🔇 Additional comments (1)
bindata/bootkube/scc-manifests/0000_20_kube-apiserver-operator_00_crb-systemauthenticated-scc-restricted-v3.yaml (1)
8-12: ClusterRole existence and unique binding verified – system:openshift:scc:restricted-v3 is defined and only referenced by a single ClusterRoleBinding; no duplicates found.
| subjects: | ||
| - apiGroup: rbac.authorization.k8s.io | ||
| kind: Group | ||
| name: system:authenticated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR objective mismatch: unauthenticated group not granted.
Title/objective says to grant restricted-v3 to unauthenticated users, but the binding only targets system:authenticated. Add system:unauthenticated (or switch to it) to meet the objective. Safer: include both to avoid regressions.
Apply one of the following diffs.
Option A (add unauthenticated, keep authenticated):
subjects:
- apiGroup: rbac.authorization.k8s.io
kind: Group
name: system:authenticated
+- apiGroup: rbac.authorization.k8s.io
+ kind: Group
+ name: system:unauthenticatedOption B (if intent is only unauthenticated):
subjects:
- apiGroup: rbac.authorization.k8s.io
kind: Group
- name: system:authenticated
+ name: system:unauthenticatedOptional: If you choose Option A, consider renaming the file to drop “systemauthenticated” from the filename for clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch robot. I am confused how restricted-v2 gets access to system:unauthenticated (I adapted this from the restricted-v2 one)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all restricted-v2 ClusterRoleBinding manifests
echo "=== Finding restricted-v2 ClusterRoleBinding files ==="
fd -e yaml -e yml . bindata/bootkube/scc-manifests/ | xargs grep -l "restricted-v2" | head -20
echo -e "\n=== Examining restricted-v2 ClusterRoleBinding content ==="
fd -e yaml -e yml . bindata/bootkube/scc-manifests/ --exec grep -l "restricted-v2" | xargs cat
echo -e "\n=== Searching for any unauthenticated bindings to restricted SCCs ==="
rg -n "system:unauthenticated" bindata/bootkube/scc-manifests/
Length of output: 4967
🏁 Script executed:
#!/bin/bash
# Search entire repo for unauthenticated SCC bindings or related configuration
echo "=== Searching entire repo for system:unauthenticated references ==="
rg -n "system:unauthenticated" --type yaml
echo -e "\n=== Searching for restricted SCC with unauthenticated ==="
rg -n -B5 -A5 "restricted.*unauthenticated|unauthenticated.*restricted"
echo -e "\n=== Looking at PR description and related files ==="
# Check if there's a PR description file or related documentation
fd -e md README
echo -e "\n=== Checking for any default SCC assignments or documentation ==="
rg -n "default.*scc|scc.*default" --type md
echo -e "\n=== Looking for issue or enhancement references ==="
rg -n "OCPBUGS|JIRA|https://issues" bindata/bootkube/scc-manifests/
Length of output: 5193
🏁 Script executed:
#!/bin/bash
# Get PR description and details
gh pr view 1935 --json title,body,author,number
Length of output: 189
@haircommander You correctly adapted from restricted-v2! I found that the restricted-v2 ClusterRoleBinding also only grants access to system:authenticated:
# bindata/bootkube/scc-manifests/0000_20_kube-apiserver-operator_00_crb-systemauthenticated-scc-restricted-v2.yaml
subjects:
- apiGroup: rbac.authorization.k8s.io
kind: Group
name: system:authenticatedSo restricted-v2 does not grant unauthenticated users access either, based on the manifests in this repository.
Since your PR title says "give unauthenticated users access to restricted-v3", there's a mismatch:
- PR objective: Grant access to unauthenticated users
- Current implementation: Grants access to authenticated users only
To fix this, you'll need to decide:
- If the title is correct and you want unauthenticated users to have access, apply the bot's suggested Option A to add
system:unauthenticatedas an additional subject. - If the title is incorrect and you only want authenticated users (matching restricted-v2), update the PR title to say "authenticated" instead of "unauthenticated".
Which approach matches your actual requirements?
|
/hold I'm trying to figure out how unauthenticated users get access to restricted-v2 |
From what I understand, it's available to all authenticated users? At least that's mentioned in the docs. |
Signed-off-by: Peter Hunt <[email protected]>
48978cc to
721c337
Compare
|
🤦 right that's what we want /unhold this is ready, sorry for the noise |
|
/retest-required |
2 similar comments
|
/retest-required |
|
/retest-required |
|
There is a broken test now: |
|
/lgtm |
|
/lgtm cancel we need to fix the failing tests - #1935 (comment) |
|
@tchap: This PR was included in a payload test run from openshift/origin#30384 |
1 similar comment
|
@tchap: This PR was included in a payload test run from openshift/origin#30384 |
|
@tchap: This PR was included in a payload test run from openshift/origin#30384
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/d6b1e380-aa85-11f0-95a7-712e44658b77-0 |
|
/retitle NO-JIRA: scc: give all authenticated users access to restricted-v3 |
|
@haircommander: This pull request explicitly references no jira issue. 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. |
|
/payload-with-prs 4.21 ci informing openshift/origin#30384 |
|
@tchap: trigger 1 job(s) of type informing for the ci release of OCP 4.21
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/33e53000-aaa1-11f0-86f9-7907cfa77396-0 |
|
/retest |
1 similar comment
|
/retest |
|
/retest I'm assuming openshift/origin#30384 will allow this test to pass for the presubmit now. Do you have a follow-up PR to origin ready? |
|
/lgtm cancel Moved to #1944. |
|
@benluddy: Closed this PR. 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 kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: haircommander The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
No description provided.