Add pod os field #104693
Merged
Add pod os field #104693
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-ci-robot
added
do-not-merge/work-in-progress
|
/remove-sig api-machinery |
k8s-ci-robot
removed
the
sig/api-machinery
ravisantoshgudimetla
force-pushed
the
add-podOS-field
branch
from
a5d6d60
to
668bc4b
Compare
k8s-ci-robot
added
the
sig/api-machinery
|
/remove-sig api-machinery |
k8s-ci-robot
added
sig/api-machinery
marosset
reviewed
Sep 16, 2021
| } | ||
| // OS based podSecurityContext validation | ||
| // TODO: Think if we need to relax this restriction or some of the restrictions | ||
| if os != nil && os.Name == "windows" { |
There was a problem hiding this comment.
Should we add these to the list?
- runAsUser
- runAsGroup
- supplementalGroups
Also note that runAsNonRoot IS supported for Windows.
marosset
reviewed
Sep 16, 2021
| // There is some naming overlap between Windows and Linux Security Contexts but all the Windows Specific options | ||
| // are set via securityContext.WindowsOptions which we validate below | ||
| // TODO: Think if we need to relax this restriction or some of the restrictions | ||
| if os != nil && os.Name == "windows" { |
There was a problem hiding this comment.
Should we add these?
- runAsUser
- runAsGroup
|
/remove-sig api-machinery |
k8s-ci-robot
added
sig/api-machinery
|
/retest |
ravisantoshgudimetla
force-pushed
the
add-podOS-field
branch
from
5eb528e
to
b0af59c
Compare
k8s-ci-robot
added
the
approved
marosset
reviewed
Oct 27, 2021
| allErrs = append(allErrs, field.Forbidden(fldPath.Child("allowPrivilegeEscalation"), "cannot be set for windows pods")) | ||
| } | ||
| if sc.ProcMount != nil { | ||
| allErrs = append(allErrs, field.Forbidden(fldPath.Child("procMount"), "procMount cannot be set for windows pods")) |
There was a problem hiding this comment.
Suggested change
| allErrs = append(allErrs, field.Forbidden(fldPath.Child("procMount"), "procMount cannot be set for windows pods")) | |
| allErrs = append(allErrs, field.Forbidden(fldPath.Child("procMount"), "cannot be set for windows pods")) |
nit
marosset
reviewed
Oct 27, 2021
| sc := c.SecurityContext | ||
| if sc != nil && sc.WindowsOptions != nil { | ||
| fldPath := cFldPath.Child("securityContext") | ||
| allErrs = append(allErrs, field.Forbidden(fldPath.Child("windowsOptions"), "windows options cannot be set to a linux pod")) |
There was a problem hiding this comment.
Suggested change
| allErrs = append(allErrs, field.Forbidden(fldPath.Child("windowsOptions"), "windows options cannot be set to a linux pod")) | |
| allErrs = append(allErrs, field.Forbidden(fldPath.Child("windowsOptions"), "windows options cannot be set on a linux pod")) |
Should this error message be either 'windows options cannot be set for a linux pod' or 'windows options cannot be set on a linux pod'?
'windows options cannot be set on to a linux pod' is a bit confusing to me.
(if we change this we'll need to change the line above and unit tests too)
There was a problem hiding this comment.
agree either "on" or "for" is better. @ravisantoshgudimetla please change that on all the messages
marosset
reviewed
Oct 27, 2021
| return allErrs | ||
| } | ||
| if !opts.AllowOSField { | ||
| return append(allErrs, field.Forbidden(fldPath, "cannot be set when IdentifyPodOS feature is not set")) |
There was a problem hiding this comment.
Suggested change
| return append(allErrs, field.Forbidden(fldPath, "cannot be set when IdentifyPodOS feature is not set")) | |
| return append(allErrs, field.Forbidden(fldPath, "cannot be set when IdentifyPodOS feature is not enabled")) |
very minor nit (feel free to close/ignore)
|
I have a couple of minor comments regarding error messages but other than that this LGTM. |
k8s-ci-robot
added
the
needs-rebase
ravisantoshgudimetla
force-pushed
the
add-podOS-field
branch
from
e93e6de
to
8a65330
Compare
k8s-ci-robot
removed
the
needs-rebase
|
/lgtm Thanks for the updates @ravisantoshgudimetla |
k8s-ci-robot
added
the
lgtm
|
/test pull-kubernetes-unit |
This was referenced Nov 5, 2021
ravisantoshgudimetla
added a commit
to ravisantoshgudimetla/kubernetes
that referenced
this pull request
Nov 9, 2021
Once kubernetes#104613 and kubernetes#104693 merge, we'll have OS field in pod spec. Kubelet should start rejecting pods where pod.Spec.OS and node's OS(using runtime.GOOS) won't match
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: