-
Notifications
You must be signed in to change notification settings - Fork 170
ROX-32636: Set allowPrivilegeEscalation for empty securityContext #18468
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: master
Are you sure you want to change the base?
ROX-32636: Set allowPrivilegeEscalation for empty securityContext #18468
Conversation
|
Skipping CI for Draft Pull Request. |
|
Images are ready for the commit at 566a083. To use with deploy scripts, first |
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.
Hey - I've left some high level feedback:
- In the tests where
defaultSecurityContextis shared across multiple containers, consider creating a fresh&storage.SecurityContext{AllowPrivilegeEscalation: true}per container instead of reusing a single pointer to avoid subtle coupling if tests or code ever mutate the security context. - In
TestSecurityContext, the conditional reassignment ofAllowPrivilegeEscalationafter initializingspec := v1.PodSpec{Containers: []v1.Container{{SecurityContext: testCase.securityContext}}}is redundant and can be removed, since the test cases already fully define theSecurityContextinstances.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the tests where `defaultSecurityContext` is shared across multiple containers, consider creating a fresh `&storage.SecurityContext{AllowPrivilegeEscalation: true}` per container instead of reusing a single pointer to avoid subtle coupling if tests or code ever mutate the security context.
- In `TestSecurityContext`, the conditional reassignment of `AllowPrivilegeEscalation` after initializing `spec := v1.PodSpec{Containers: []v1.Container{{SecurityContext: testCase.securityContext}}}` is redundant and can be removed, since the test cases already fully define the `SecurityContext` instances.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #18468 +/- ##
==========================================
- Coverage 48.92% 48.92% -0.01%
==========================================
Files 2629 2633 +4
Lines 197814 197988 +174
==========================================
+ Hits 96786 96871 +85
- Misses 93644 93728 +84
- Partials 7384 7389 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
clickboo
left a 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.
Left a comment.
Description
Container.SecurityContextmight be nil and allowPrivilegeEscalation should be set to true explicitly for such cases. Otherwise allowPrivilegeEscalation policy will not trigger on container without securityContext. See example in the ticket description.User-facing documentation
Testing and quality
Automated testing
How I validated my change
Added unit test