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
Identify Pod's OS during API Server admission #2803
Identify Pod's OS during API Server admission #2803
Changes from 1 commit
6ea449b
11724d3
d51cbbc
263e3ea
4bb1d17
493b097
fc68598
c7c8359
0981dc4
03f3dd8
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.
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 think we mean to standardize this webhook into Kubernetes/core itself, right?
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.
Nope, the admission webhooks can be outside of core Kube too.
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 think this field was primarily the result of the distinction between
Since a pod targeting a windows node could still be a linux pod, we needed a way to express scheduling distinct from the pod OS. The pod OS informs what combination of fields can be specified and what values need to be enforced during pod security admission.
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 did not mention this as support for LCOW in Kubernetes is questionable. I included it as I assume you want the reasoning behind to be mentioned in the KEP.
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.
maybe it's just me, but it's not clear if "the OS on which pod intends to run" refers to the node OS or the container OS. "the OS of the containers specified in the pod" would be clearer.
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.
But it is not at the container field level in the pod right? It'd just be at the pod level?
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.
We support all of these? solaris?
When the API is created, I think a strict enumeration is required.
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 assume since OCI spec is supporting those OS'es, and if the container runtime is supporting and confirms to CRI spec, we should be good from node perspective? AFAIK, node binaries are available for linux, mac and windows as part of our standard release process
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.
current node platforms:
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.
that said, I'm not necessarily opposed to accepting more OS values, I just think that means more work to define API validation and PodSecurity treatment of each enumerated value
when we add validation for this, we should probably bake in toleration of an existing unknown value on an update, and explicitly note that future values could be added in the future, and how a client should handle an unknown enum value (xref https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md#new-enum-value-in-existing-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.
Ahh I see, I was doing something like this in the validation stage - kubernetes/kubernetes@852f442#diff-c713e8919642d873fdf48fe8fb6d43e5cb2f53fd601066ff53580ea655948f0dR6158. I believe I need to add some toleration for the unknown updates.
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.
the node platforms are distinct from supported operating system value. i had linked @ravisantoshgudimetla to the OCI spec for reference as a potential future set, but we should only support
linux
orwindows
at this time which maps to present support.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.
When/if linux containers on windows are supported, this will need to be updated, correct? I think it's still ok to add this since old kubelets won't be able to correctly manage such pods, but I want to call out our future debt.
Perhaps it is better phrased as, "kubelets should reject admitting pod if the kubelet cannot honor the pod.spec.os.name. For instance, if the os.name does not match the host 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.
Agree, matching the
kubernetes.io/os
node label is an implementation detail. For the KEP we can use the phasing mentioned above.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.
Beta -> GA?
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 prefer IdentifyPodOS
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 don't have a strong preference. If that is the name everyone is fine with, I can use that name.
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.
if pod validation changes to allow or require some fields to be empty, the update validation will need to need to be adjusted to accommodate missing fields or pods will become un-updateable. I'd to explicitly call out the need for unit tests and manual tests ensuring this compatibility.
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.
seems like pod creation and pod update rejections are worth watching.
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.
Agreed and added a metric which we can use to track the pod status however I couldn't find rejections from the admission controller rejection metric unless I misunderstood you.
I am thinking of adding a metric which can benefit us - https://github.com/kubernetes/enhancements/pull/2803/files#diff-0bd86b6b4704c4e10a453c7f15e74e31aacb53891087cf069ae11b8bf5293a30R320
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.
@deads2k suggested that I can use 400 series errors to capture the rejection at the admission plugin. So, we won't need a new metric. I mentioned the metric we can use to track the 400 series errors 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.
The most likely failure mode is rejection either by the kube-apiserver on pod create/update or by the kubelet during kubelet admission. I suspect both of those have metrics associated them. I think you should find those metrics and list 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.
kube-apiserver and kubelet, right?
It's important to list these because a kubelet is often mismatched to a kube-apiserver and so the change won't fully function until kubelets are updated.