-
Notifications
You must be signed in to change notification settings - Fork 39.8k
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
kuberuntime: check the value of RunAsNonRoot when verifying #47009
Conversation
/cc @feiskyer |
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.
👍 LGTM
@liggitt this should fix the kubelet issue we discussed earlier. |
LGTM, would like a second from @pweil- |
The verification function is fixed to check the value of RunAsNonRoot, not just the existence of it. Also adds unit tests to verify the correct behavior.
I pushed again just to remove the two redundant lines (48-49 in security_context_test.go) from the original test. Nothing else was changed. |
@@ -72,7 +72,8 @@ func (m *kubeGenericRuntimeManager) determineEffectiveSecurityContext(pod *v1.Po | |||
// verifyRunAsNonRoot verifies RunAsNonRoot. | |||
func verifyRunAsNonRoot(pod *v1.Pod, container *v1.Container, uid int64) error { | |||
effectiveSc := securitycontext.DetermineEffectiveSecurityContext(pod, container) | |||
if effectiveSc == nil || effectiveSc.RunAsNonRoot == nil { | |||
// If the option is not set, or if running as root is allowed, return nil. | |||
if effectiveSc == nil || effectiveSc.RunAsNonRoot == nil || !*effectiveSc.RunAsNonRoot { |
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.
effectiveSc.GetRunAsNonRoot
?
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.
This is a k8s api pod object. I don't think we have getters for them.
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.
ACK. Confused initially.
@@ -244,7 +244,7 @@ func TestGenerateContainerConfig(t *testing.T) { | |||
Command: []string{"testCommand"}, | |||
WorkingDir: "testWorkingDir", | |||
SecurityContext: &v1.SecurityContext{ | |||
RunAsNonRoot: &RunAsNonRoot, | |||
RunAsNonRoot: &runAsNonRootTrue, |
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.
What is the following expectedConfig
used for?
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.
Probably can be removed. @feiskyer
But I don't want to turn this into a test cleanup PR. That can be done in a separate PR.
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.
Missing one assertion
assert.Equal(t, expectedConfig, containerConfig, "generate container config for kubelet runtime v1.")
errStr: "container's runAsUser breaks non-root policy", | ||
}, | ||
{ | ||
desc: "Fail if image's user is root and RunAsNonRoot is true", |
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.
Add case Pass if image's user is non-root and RunAsNonRoot is true
?
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.
Done.
@@ -46,59 +46,59 @@ func TestVerifyRunAsNonRoot(t *testing.T) { | |||
} | |||
|
|||
err := verifyRunAsNonRoot(pod, &pod.Spec.Containers[0], int64(0)) | |||
assert.NoError(t, err) | |||
assert.NoError(t, err, "should pass if SecurityContext is not set") |
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.
Why not merge this case into the test table?
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.
I removed this before you left the 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.
ACK
RunAsNonRoot: &runAsNonRootTrue, | ||
RunAsUser: &rootUser, | ||
}, | ||
errStr: "container's runAsUser breaks non-root policy", |
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.
Hmm, not sure whether we want to check error string.
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.
I didn't want to change the original test behavior too much, but remove it any way. Done.
@k8s-bot pull-kubernetes-unit test this |
LGTM |
LGTM |
/assign |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: feiskyer, yujuhong Associated issue: 46996 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
Automatic merge from submit-queue (batch tested with PRs 46775, 47009) |
I haven't squashed yet... :-| |
sorry, didn't notice there is another commit 😢 |
The verification function is fixed to check the value of RunAsNonRoot,
not just the existence of it. Also adds unit tests to verify the correct
behavior.
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close that issue when PR gets merged): fixes #46996Release note: