-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
KEP-4742: Expose Node Labels via Downward API #4747
base: master
Are you sure you want to change the base?
Conversation
docandrew
commented
Jul 1, 2024
- Adding new KEP 4742: Expose Node Labels via Downward API
- Issue link: Expose Node labels via downward API #4742
- Other comments: In response to discussion here: Consider exposing node labels via downward api kubernetes#40610
Welcome @docandrew! |
Hi @docandrew. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: docandrew The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/cc @kerthcet |
As discussed at kubernetes/kubernetes#40610 |
I think we can do this in a safe way. Let me know if the reply makes sense. Happy to work through this and give examples. |
After reading the entire conversation on the KEP issue and related links, I am ok with this now. As NFD maintainer I am eager to help here, as NFD creates labels for the underlying infra |
/ok-to-test |
/sig auth need opinion on security and potential abuse |
some concerns to address: kubernetes/kubernetes#40610 (comment) |
|
||
The initial design includes: | ||
|
||
* Being able to pass node labels to volumes with fieldPath of `node.metadata.labels` where only labels containing `topology.k8s.io/` are passed through |
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.
Is this overloading the namespace of topology.k8s.io? Are we encouraging users to overload it with their own labels (toplogy.k8s.io/hostuefipassword=foo
)? Will users accidentally see side effects because they use the same fields inside the topology.k8s.io namespace for another purpose?
Maybe it would be better to either use a config-controlled allow-list within the kubelet(?) config or a limited allow-list of well-known labels plus a new namespace like alpha.exposednodelabels.k8s.io/*
(feel free to come up with a better 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.
Maybe we could follow a similar logic to https://kubernetes-sigs.github.io/node-feature-discovery/v0.16/usage/features.html#built-in-labels
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 we think "topology.k8s.io" in an extensibility mechanism, we need to spec that very clearly.
e.g. x.topology.k8s.io/<anything>
is for extensions, while topology.k8s.io/<anything>
is reserved for k8s use.
And even then we need to define "local". Can vendors define topology extensions?
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'm looking to update this a KEP a bit, but i think I am a little lost. So we need to strictly document and define through code what the <anything>
flag can be? Is this the ask?
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.
flag as in part of the label that would be applied to the pod
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'm looking for the KEP to say SOMETHING LIKE the below. Note that this is a suggested approach, we should consider alternatives:
"""
In KEP 1659, the following labels are defined:
* topology.kubernetes.io/region
* topology.kubernetes.io/zone
In addition to those, KEP 1659 declares the entire topology.kubernetes.io
prefix space as reserved for use by the Kubernetes project.
This KEP expands upon KEP1659 in the following ways:
- The
x.topology.kubernetes.io
prefix is allocated for use by end users. The kubernetes project itself will not define any standard labels with that prefix. - The
<domain>.x.topology.kubernetes.io
prefix is likewise allocated for use by end users or third-parties. The<domain>
portion is treated the same as a "normal" label prefix. For example,example.com.x.topology.k8s.io/label-name
. - All labels using the
topology.kubernetes.io
or*.topology.kubernetes.io
prefix spaces are considered "safe" for workloads. A workload may be exposed to the values of these labels which directly apply to the workload. For example, a pod may learn the topology of the node on which it is running.
"""
Then this KEP can describe how it will expose those labels from nodes to pods (is it a literal copy or a virtual one, who wins in case of conflict, etc).
If we think there is a need for ANOTHER prefix which is "safe" but isn't "topology", we can discuss doing a similar carve-out for another prefix. But I'd argue it stops at 2.
|
||
Workarounds today typically involve using an initContainer to query the | ||
Kubernetes API and then pass data via shared volume to other containers within | ||
the same pod. This adds additional demand on the API server and is burdensome |
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 want to be really clear about my position on this:
- DownwardAPI for anything inside the Pod object seems OK (we opened that door a long time ago).
- DownwardAPI for things outside of the Pod object is not OK unless it follows authz rules (pod's SA).
- The fact that workarounds exist means we should NOT consider this an extraoridnary need and should NOT be bending rules.
- Arguments based on reducing apiserver load need to show proof.
|
||
The initial design includes: | ||
|
||
* Being able to pass node labels to volumes with fieldPath of `node.metadata.labels` where only labels containing `topology.k8s.io/` are passed through |
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 we think "topology.k8s.io" in an extensibility mechanism, we need to spec that very clearly.
e.g. x.topology.k8s.io/<anything>
is for extensions, while topology.k8s.io/<anything>
is reserved for k8s use.
And even then we need to define "local". Can vendors define topology extensions?
|
||
The initial design includes: | ||
|
||
* Being able to pass node labels to volumes with fieldPath of `node.metadata.labels` where only labels containing `topology.k8s.io/` are passed through |
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.
fieldpaths are supposed to be paths to fields, not magic words.
What I proposed in kubernetes/kubernetes#40610 (comment) was to copy (literally copy the data) from node to pod, and define the behavior of that.
Then we don't need a new fieldpath at all. We just need to decide WHO copies that data, and whether it is literally (and pushed back to apiserver) or virtually copied by kubelet (and kept local, like service link env vars).
I think this will be net simpler than having new magic words.
attained through a configmap or secret mounted to a pod, being passed on | ||
creation but not guaranteed to be immutable and thus should be treated as so. | ||
|
||
* Can potentially be featuregated to make this feature strictly opt-in, if |
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.
feature gates are not long-term flags -- if you need a mechanism for admisn to disable this entirely, that's not a feature gate (and I would argue it indicates a deeper problem)
## Motivation | ||
|
||
We’d like to change the runtime behavior of containers based on node labels. | ||
In our case, we’re using a CNI with DaemonSets to perform network setup, and |
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.
For very specific cases, we have workarounds like sidecars. If these DaemonSets need more info, that's their escape path.
Notes I got from the meeting so they can be fixed and address later when there is time:
updated since we are not considering using kubelet flags |
I am -2 on kubelet flags. In most managed providers, those flags are not accessible to users or admins, and differeing opinions between providers will make portability WORSE, not better. We'd do better to define a small set of extension mechanisms and leave it fixed at that. |
We’d like to change the runtime behavior of containers based on node labels. | ||
In our case, we’re using a CNI with DaemonSets to perform network setup, and | ||
would like to configure the network differently based on the presence of a node | ||
label (e.g. `topology.k8s.io/vpn=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.
topology.k8s.io/vpn
is NOT a standard label. If you (abstract you) make up labels with "k8s.io" or "kubernetes.io" keys, you are asking to get broken. To the best of my knowledge, we have not declared that "topology.k8s.io" in an extensibility mechanism -- am I misremembering?
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 used this based off the suggestion made in kubernetes/kubernetes#40610 (comment) that these labels could be copied - but I think the comment there was addressing the very specific use case of things like being AZ-aware, and not the broader need to make certain node labels visible in pods.
Whatever the scheme, I think I'd like to stick with a convention for labels (e.g. downwardapi.k8s.io/*
or shared.k8s.io/*
) that get copied rather than specify which labels get copied via something like kubelet args or CRDs.
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 we started this conversation, I assumed (I know, my mistake) that we would only be copying topology labels, and those topology labels are few in number and well defined by the project.
I am okay if we want to define a way for admins to extend that using our topology label space, as long as we make it clear what's an extension and what is standard. But we have to define that somewhere.
I am also okay, I guess, to define a different label space, which explicitly means that "these labels are safe to copy to pods". But again we have to define that somewhere.
I really really really do not want this to be something that is configurable, either through flags or crds or something else. Until and unless we can show that something simple and well defined absolutely cannot work, let's just do the simple thing.
Just as an after thought kubernetes-sigs/node-feature-discovery#1779 |
Signed-off-by: Case Wylie <cmwylie19@defenseunicorns.com>
Signed-off-by: Case Wylie <cmwylie19@defenseunicorns.com>
@docandrew: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
--> | ||
|
||
## Alternatives | ||
|
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.
Reading @thockin comments about kubelet-sidecar. Maybe you can comment here about some alternatives that have been purusued or discussed?