-
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-4969: Cluster Domain Downward API #4972
base: master
Are you sure you want to change the base?
Conversation
nightkr
commented
Nov 21, 2024
- One-line PR description: Initial KEP draft
- Issue link: Cluster Domain Downward API #4969
- Other comments:
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: nightkr 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 |
Welcome @nightkr! |
Hi @nightkr. 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. |
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.
just two minor nitpicks/typos
Currently there is no way for cluster workloads to query for this domain name, | ||
leaving them either use relative domain names or take it as manual configuration. |
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.
Currently there is no way for cluster workloads to query for this domain name, | |
leaving them either use relative domain names or take it as manual configuration. | |
Currently, there is no way for cluster workloads to query this domain name, | |
leaving them to either use relative domain names or configure it manually. |
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 left "query for this" as-is, because I read the revised variant as "ask what the domain name (that I already know) means" rather than "ask what the domain name is".
Currently there is no way for cluster workloads to query for this domain name, | ||
leaving them either use relative domain names or take it as manual configuration. | ||
|
||
This KEP proposes adding a new Downward API for that workloads can use to request it. |
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 KEP proposes adding a new Downward API for that workloads can use to request it. | |
This KEP proposes adding a new Downward API that workloads can use to request it. |
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.
- `nodePropertyRef` (@aojea) | ||
- `runtimeConfigs` (@thockin) |
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 guess this is out of scope of this KEP, but I guess this change sets a precedent of other configs that could be passed down into the Pod.
I wonder if someone with more experience than me has an idea/vision of what that could look like, which may then determine what name to decide on?
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.
Looking forward, I suspect that both cases will become relevant eventually. This property just occupies an awkward spot since it doesn't really have one clean owner.
/ok-to-test |
/retest |
<!-- | ||
This section is incredibly important for producing high-quality, user-focused | ||
documentation such as release notes or a development roadmap. It should be | ||
possible to collect this information before implementation begins, in order to | ||
avoid requiring implementors to split their attention between writing release | ||
notes and implementing the feature itself. KEP editors and SIG Docs | ||
should help to ensure that the tone and content of the `Summary` section is | ||
useful for a wide audience. | ||
|
||
A good summary is probably at least a paragraph in length. | ||
|
||
Both in this section and below, follow the guidelines of the [documentation | ||
style guide]. In particular, wrap lines to a reasonable length, to make it | ||
easier for reviewers to cite specific portions, and to minimize diff churn on | ||
updates. | ||
|
||
[documentation style guide]: https://github.com/kubernetes/community/blob/master/contributors/guide/style-guide.md | ||
--> |
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 section is incredibly important for producing high-quality, user-focused | |
documentation such as release notes or a development roadmap. It should be | |
possible to collect this information before implementation begins, in order to | |
avoid requiring implementors to split their attention between writing release | |
notes and implementing the feature itself. KEP editors and SIG Docs | |
should help to ensure that the tone and content of the `Summary` section is | |
useful for a wide audience. | |
A good summary is probably at least a paragraph in length. | |
Both in this section and below, follow the guidelines of the [documentation | |
style guide]. In particular, wrap lines to a reasonable length, to make it | |
easier for reviewers to cite specific portions, and to minimize diff churn on | |
updates. | |
[documentation style guide]: https://github.com/kubernetes/community/blob/master/contributors/guide/style-guide.md | |
--> |
clusterPropertyRef: clusterDomain | ||
``` | ||
|
||
`foo` can now perform the query by running `curl http://bar.$NAMESPACE.svc.$CLUSTER_DOMAIN/`. |
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.
Extra credit: define a sidecar that relies on interpolating $(CLUSTER_DOMAIN)
environments, since `node-b` might not be able to resolve `cluster.local` | ||
FQDNs correctly. | ||
|
||
For this KEP to make sense, this would have to be explicitly prohibited. |
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 know if that's true.
For Pod consumes, the downward API is implemented by the kubelet, so each kubelet can expose its local view of the cluster domain.
We would still strongly recommend against having multiple cluster domains defined across your cluster - anything else sounds really unwise - but technically it can be made to work.
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.
Yeah, it's one of those.. it would be implementable without making such a declaration, but it would likely be another thing leading to pretty confusing behaviour. Maybe the language can be softened somewhat.
<!-- | ||
What are the caveats to the proposal? | ||
What are some important details that didn't come across above? | ||
Go in to as much detail as necessary here. | ||
This might be a good place to talk about core concepts and how they relate. | ||
--> |
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.
A related detail.
- A kubelet that is formally aware of its cluster domain could report back either the cluster domain value, or a hash of it, via
.status
. - A kubelet that is formally aware of its cluster domain could report back either the cluster domain value, or a hash of it, via a somethingz HTTP endpoint.
- A kubelet that is formally aware of its cluster domain could report back either the cluster domain value, or a hash of it, via a Prometheus static-series label
egkubelet_cluster_domain{domain_name="cluster.example"} 1
If we decide to make the API server aware of cluster domain, adding that info could help with troubleshooting and general observability.
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's already available via the kubelet's /configz (mentioned in Alternatives). I don't have a strong opinion either way on adding it to the Node status.
Co-authored-by: Tim Bannister <[email protected]>
Co-authored-by: Tim Bannister <[email protected]>
The ConfigMap written by k3s[^prior-art-k3s] could be blessed, requiring that | ||
all other distributions also provide it. However, this would require additional | ||
migration effort from each distribution. | ||
|
||
Additionally, this would be problematic to query for: users would have to query | ||
it manually using the Kubernetes API (since ConfigMaps cannot be mounted across | ||
Namespaces), and users would require RBAC permission to query wherever it is stored. |
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 thought centralized management was a non goal though? I recommend highlighting that this alternative isn't aligned with this KEP's goals.
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.
In terms of the non-goal I was mostly referring to configuring the kubelet itself (which would have been a retread of #281). Maybe that could be clarified.
This roughly shares the arguments for/against as [the ConfigMap alternative](#alternative-configmap), | ||
although it would allow more precise RBAC policy targeting. |
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 thought centralized management was a non goal though? I recommend highlighting that this alternative isn't aligned with this KEP's goals.
# The following PRR answers are required at alpha release | ||
# List the feature gate name and the components for which it must be enabled | ||
feature-gates: | ||
- name: MyFeature |
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 could pick the feature gate name even at provisional stage.
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.
Sure. PodClusterDomain
would align with PodHostIPs
(the only other downward API feature gate listed on https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates/), but happy to hear other takes.
|
||
## Design Details | ||
|
||
A new Downward API `clusterPropertyRef: clusterDomain` would be introduced, which can be projected into an environment variable or a volume file. |
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's this a reference to?
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 was mostly trying to be consistent with fieldRef
and resourceFieldRef
, which also don't quite correspond to the pod/container objects (since they're not quite 1:1), but I'm certainly not married to it.
Co-authored-by: Tim Bannister <[email protected]>
Co-authored-by: Tim Bannister <[email protected]>