OCPBUGS-62799: Add required-scc annotation to node-joiner pod #2230
Conversation
openshift-ci-robot
added
jira/severity-important
|
@cetinerdev: This pull request references Jira Issue OCPBUGS-62799, 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 Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 12 minutes and 6 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughAdds the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @cetinerdev. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
|
/retest-required |
rwsu
left a comment
rwsu
left a comment
There was a problem hiding this comment.
Some minor typos to correct. Otherwise looks good.
| remoteExecOutput: "0", | ||
| }, | ||
| { | ||
| name: "node-joiner pod has required-scc annotation ", |
There was a problem hiding this comment.
| name: "node-joiner pod has required-scc annotation ", | |
| name: "node-joiner pod has required-scc annotation", |
| expected := "restricted-v2" | ||
| got := pod.Annotations["openshift.io/required-scc"] | ||
| if got != expected { | ||
| t.Errorf("annoation openshift.io/required-scc = %q, want %q", got, expected) |
There was a problem hiding this comment.
| t.Errorf("annoation openshift.io/required-scc = %q, want %q", got, expected) | |
| t.Errorf("annotation openshift.io/required-scc = %q, want %q", got, expected) |
| got := pod.Annotations["openshift.io/required-scc"] | ||
| if got != expected { | ||
| t.Errorf("annoation openshift.io/required-scc = %q, want %q", got, expected) | ||
| } |
| Labels: map[string]string{ | ||
| "app": "node-joiner-monitor", | ||
| }, | ||
| Annotations: map[string]string{ |
There was a problem hiding this comment.
Would be great if a similar test for monitor can be created.
There was a problem hiding this comment.
added missing test , and test is verified
cetinerdev
force-pushed
the
custom-scc-prevention
branch
from
e920e51 to
62434fc
Compare
|
Addressed all review comments: fixed trailing space, typo, and mixed tabs/spaces in create_test.go. Added similar test for monitor in monitor_test.go. |
|
@cetinerdev: This pull request references Jira Issue OCPBUGS-62799, which is invalid:
Comment 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cli/admin/nodeimage/monitor_test.go (1)
77-97:⚠️ Potential issue | 🟡 MinorInvoke
expectedPod; the new assertion currently never runs.Line 77 adds the hook and Lines 90-96 define the annotation check, but
tc.expectedPodis never called aftero.Run(). This means the new test passes even if the monitor pod annotation is removed.🧪 Proposed fix
if tc.expectedError == "" { if fakeLogContent != logContents.String() { t.Errorf("expected %v, actual %v", fakeLogContent, logContents.String()) } + if tc.expectedPod != nil { + if o.nodeJoinerPod == nil { + t.Fatalf("expected node-joiner monitor pod to be created") + } + tc.expectedPod(t, o.nodeJoinerPod) + } }Also applies to: 143-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cli/admin/nodeimage/monitor_test.go` around lines 77 - 97, The test defines an expectedPod hook (tc.expectedPod) but never invokes it, so the annotation assertion never runs; after invoking the command runner (the call to o.Run(...) or equivalent in the test loop), add a conditional call like if tc.expectedPod != nil { tc.expectedPod(t, createdPod) } so the provided validation runs — do this in the table-driven test where the test cases are iterated (the block that calls o.Run) and also apply the same fix for the second table at lines ~143-149 so both sets of cases execute their expectedPod callbacks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/cli/admin/nodeimage/monitor_test.go`:
- Around line 77-97: The test defines an expectedPod hook (tc.expectedPod) but
never invokes it, so the annotation assertion never runs; after invoking the
command runner (the call to o.Run(...) or equivalent in the test loop), add a
conditional call like if tc.expectedPod != nil { tc.expectedPod(t, createdPod) }
so the provided validation runs — do this in the table-driven test where the
test cases are iterated (the block that calls o.Run) and also apply the same fix
for the second table at lines ~143-149 so both sets of cases execute their
expectedPod callbacks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 9566281a-6de4-493e-828f-1172bbeb5e51
📒 Files selected for processing (4)
pkg/cli/admin/nodeimage/create.gopkg/cli/admin/nodeimage/create_test.gopkg/cli/admin/nodeimage/monitor.gopkg/cli/admin/nodeimage/monitor_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/cli/admin/nodeimage/monitor.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/cli/admin/nodeimage/create_test.go
- pkg/cli/admin/nodeimage/create.go
…ent third-party SCC interference When a third-party SCC (e.g. Pure Storage CSI) with readOnlyRootFilesystem: true and higher priority is broadly accessible via RBAC, the SCC admission controller may assign it to the node-joiner pod instead of restricted-v2. This causes the node-joiner tool to fail with 'read-only file system' errors when writing to /tmp. Adding the openshift.io/required-scc annotation ensures restricted-v2 is always assigned regardless of other SCCs' priority or restrictiveness, as required by the custom SCC preemption prevention enhancement: https://github.com/openshift/enhancements/blob/master/enhancements/authentication/custom-scc-preemption-prevention.md
cetinerdev
force-pushed
the
custom-scc-prevention
branch
from
62434fc to
d1931c3
Compare
|
/jira refresh |
|
@cetinerdev: This pull request references Jira Issue OCPBUGS-62799, which is invalid:
Comment 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@cetinerdev: This pull request references Jira Issue OCPBUGS-62799, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cetinerdev, rwsu 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
|
/verified by unit tests: node-joiner pod has required-scc annotation |
openshift-ci-robot
added
the
verified
|
@rwsu: This PR has been marked as verified by 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. |
|
@cetinerdev: 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. |
|
@cetinerdev: Jira Issue Verification Checks: Jira Issue OCPBUGS-62799 Jira Issue OCPBUGS-62799 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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. |
|
/cherry-pick release-4.22 |
|
@rwsu: new pull request created: #2266 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 kubernetes-sigs/prow repository. |
|
Fix included in release 5.0.0-0.nightly-2026-05-06-052707 |
6 participants
When a third-party SCC (e.g. Pure Storage CSI) with readOnlyRootFilesystem: true and higher priority is broadly accessible via RBAC, the SCC admission controller may assign it to the node-joiner pod instead of restricted-v2. This causes the node-joiner tool to fail with 'read-only file system' errors when writing to /tmp.
Adding the openshift.io/required-scc annotation ensures restricted-v2 is always assigned regardless of other SCCs' priority or restrictiveness, as required by the custom SCC preemption prevention enhancement:
https://github.com/openshift/enhancements/blob/master/enhancements/authentication/custom-scc-preemption-prevention.md
Summary by CodeRabbit
Bug Fixes
Tests