-
Notifications
You must be signed in to change notification settings - Fork 421
fix(nodeimage): add SecurityContext to image creation and monitor containers #2139
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
base: main
Are you sure you want to change the base?
Conversation
…pods Signed-off-by: Ben Dronen <[email protected]>
WalkthroughAdds SecurityContext configurations to the node-joiner and node-joiner-monitor containers in the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
Assessment against linked issues
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dronenb 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 |
|
Hi @dronenb. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
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: 0
🧹 Nitpick comments (1)
pkg/cli/admin/nodeimage/monitor.go (1)
290-296: LGTM! Security context correctly addresses PodSecurity violations.The SecurityContext configuration properly satisfies the "restricted:latest" PodSecurity requirements identified in issue #2138. All three mandatory fields are present.
Consider extracting the identical SecurityContext configuration shared with
create.go(lines 748-754) into a common helper function to reduce duplication.Optionally, the boolean pointer pattern
&[]bool{false}[0]could be replaced with more idiomatic Go:SecurityContext: &corev1.SecurityContext{ AllowPrivilegeEscalation: ptr.To(false), RunAsNonRoot: ptr.To(true), Capabilities: &corev1.Capabilities{ Drop: []corev1.Capability{"ALL"}, }, },This requires importing
k8s.io/utils/ptr.
📜 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 (2)
pkg/cli/admin/nodeimage/create.go(1 hunks)pkg/cli/admin/nodeimage/monitor.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/cli/admin/nodeimage/create.gopkg/cli/admin/nodeimage/monitor.go
🔇 Additional comments (1)
pkg/cli/admin/nodeimage/create.go (1)
748-754: LGTM! Security context matches monitor.go implementation.The SecurityContext correctly addresses the PodSecurity violations for the node-joiner container. See the review comment on
monitor.golines 290-296 for suggested refactors regarding code duplication and the boolean pointer pattern.
Fixes #2138