Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Restricted Pod E2E tests #109946
Restricted Pod E2E tests #109946
Changes from all commits
ccc69b1
702ab97
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
does changing from nginx to pause change the lifecycle characteristics of the pod (w.r.t. handling shutdown signals)? same question applies in multiple places
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 question. Both the pause container and nginx handle SIGINT and SIGTERM as a fast shutdown, which I think is what containerd uses. nginx additionally handles SIGQUIT, but I don't see any references to SIGQUIT in k/k, containerd, or cri-o.
Maybe @SergeyKanzhelev or @endocrimes have additional context.
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 test doesn't cover any corner cases of shutdown so this seems ok to me 👍
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 do windows nodes do if runAsUser is non-nil? do they ignore it or fail?
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.
It is ignored by kubelet:
kubernetes/pkg/kubelet/kuberuntime/security_context_windows.go
Lines 46 to 49 in 68fc207
With PodOS feature and OS field is set (in beta in 1.24) the adminsion controller would reject it:
kubernetes/staging/src/k8s.io/api/core/v1/types.go
Lines 3286 to 3307 in 976a940
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.
ok, there's a TODO to avoid setting RunAsUser in MustMixinRestrictedPodSecurity for explicitly windows pods as part of the PodOS work, I just wanted to make sure this wasn't going to fail in a windows kubelet for pods that didn't explicitly indicate their OS
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.
So, this did break the tests because of #102849
https://prow.k8s.io/view/gs/kubernetes-jenkins/logs/ci-kubernetes-e2e-capz-master-containerd-windows/1529769915945324544
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 isn't setting RunAsUserName, but does the windows kubelet have similar problems with RunAsUser?
if so, that seems like a good reason to drop RunAsUser here
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.
Yes the title is slightly misleading but it states in the issue:
In addition if a Windows Pod is created with a Projected Volume and RunAsUser set, the Pod will be stuck at ContainerCreating:
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.
ok. @tallclair, that's a good reason not to set RunAsUser here for pods that could run on Windows. @jsturtevant, do you mind opening a PR to drop the RunAsUser field?
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.
linking here for anyone following the thread #110235