Add proposal for configurable pod resolv.conf #1276
Conversation
k8s-ci-robot
added
size/L
|
@kubernetes/sig-network-api-reviews |
k8s-ci-robot
added
sig/network
bowei
force-pushed
the
pod-resolv-conf
branch
3 times, most recently
from
085419f
to
f617f96
Compare
bowei
force-pushed
the
pod-resolv-conf
branch
5 times, most recently
from
8c8d83f
to
6eb18c0
Compare
| image: example | ||
| dnsPolicy: Custom | ||
| dnsParams: | ||
| searchPaths: |
| dnsPolicy: Custom | ||
| dnsParams: | ||
| searchPaths: | ||
| - $NAMESPACE.svc.$CLUSTER |
There was a problem hiding this comment.
We should be very careful with templating syntax. This could set precedent.
I would suggest $(NAMESPACE) similar to other places we do expansion. Needs REALLY good docs on what expansions are available and what happens if the user uses an invalid one, and how to get a literal value from the escape.
There was a problem hiding this comment.
Maybe you can get away without templating, at least initially? My guess is that any user who needs such an advanced feature can figure out how to template this themselves, or already have a templating layer anyway (we do).
This would uncouple the DNS configurability from all the complexity of adding templating.
If it turns out to be really indispensable, it can still be added later, but once it's in you can't get it out and you'll have to explain why it's not available everywhere.
There was a problem hiding this comment.
I added more detail to the psuedocode and exposition. The system works by substituting labels if the substitution value matches and is not intended to be a generic template mechanism.
@matthiasr we can leave out the substitution but suspect most people would want to be able to write YAMLs portable across namespaces without forcing use of another layer.
| - my.dns.search.suffix | ||
| - $HOST | ||
| options: | ||
| - ndots: 2 |
There was a problem hiding this comment.
unclear what the type behind this is? Is it a map of string-> string? I think you meant it to just be []string, but I bet this doesn't unmarshal properly (since you wrote a []struct{ndots int}).
you either need to say:
options:
- "ndots: 2"
or we need to actually make a schema (which I don't think we should do) or we need to make it a map (which we generally frown upon) or we need to make it:
options:
- name: ndots
value: "2"
which I dislike. Assuming no other major issues, we can discuss this as an API issue with API guardians.
There was a problem hiding this comment.
Want to raise that not all options are {key}:{value}, options like rotate and debug are just flags. I think []string might be the best because the intention here is to capture the options, and no additional processing is assumed.
There was a problem hiding this comment.
It is []string (see API code), I updated the YAML to make this clear (it's actually a YAML syntax error)
|
|
||
| ```bash | ||
| nameserver 1.2.3.4 | ||
| search foo.com |
There was a problem hiding this comment.
if you make this hold 2 entries it will be clearer what $(HOST) means
| metadata: | ||
| namespace: ns1 | ||
| name: example | ||
| spec: |
There was a problem hiding this comment.
no field for nameservers ? E.g. if a user wants to not use cluster DNS at all?
There was a problem hiding this comment.
I'm wary of adding in nameserver configuration due to differences between common libc (glibc and musl). glibc will try the nameservers in sequence whereas musl sends requests in parallel.
There was a problem hiding this comment.
If a user REALLY wants to customize, they might choose to bypass kube-DNS entirely?
| The following is a Pod dnsParams that only contains the host search paths: | ||
|
|
||
| ```yaml | ||
| dnsParams: |
There was a problem hiding this comment.
How does this map to Windows? Do the concepts apply?
There was a problem hiding this comment.
According to the documentation online, Windows Containers do not support search path https://docs.microsoft.com/en-us/virtualization/windowscontainers/manage-containers/container-networking.
Or maybe the support doc has not yet been updated: microsoft/hcsshim#98.
The user space kube-proxy for Windows actually captures the DNS request and implements the search path expansion there.
There was a problem hiding this comment.
oh puke.
can anyone from @kubernetes/sig-windows-misc comment on this? I'd like to not make the API any more non-windows-friendly, but I don't know how to express this in a windows-friendly way
There was a problem hiding this comment.
DNS Search option is already supported for Windows, the document needs to be updated.
| ``` | ||
|
|
||
| #### Substitutions | ||
| `Substitute` will replace labels that begin with `$` with values for the given Pod: |
There was a problem hiding this comment.
is $(NAMESPACE)12345 going to => foobar12345 or is it an error?
There was a problem hiding this comment.
updated, the subst will be label for label.
| - name: example | ||
| image: example | ||
| dnsPolicy: Custom | ||
| dnsParams: |
There was a problem hiding this comment.
Maybe
dnsParams:
custom:
search:
so we leave room for other params over time?
There was a problem hiding this comment.
I don't think dnsParams is the best key name for this. The customization is around resolv.conf, which is used specifically by some resolver implementations. I think it should be referencing resolv.conf customization. This should also address the concern that how is this applicable to Windows.
There was a problem hiding this comment.
@thockin I updated
@phsiao It seems to be asymmetric to have:
dnsPolicy: Custom
dnsResolvConf:
...
or on Windows
dnsPolicy: CustomWindows
dnsWindows:
...
on Foo platform:
dnsPolicy: CustomFoo
etc
The search path expansion seems to be something that all platforms should probably support in some way. As for options, by its nature, I would expect that to be non-portable.
There was a problem hiding this comment.
How does windows configure name resolution? We need an answer to that before we merge this, really.
There was a problem hiding this comment.
Yeah, I want this to be non linux specific. I really, really, really don't want to embed resolvconf in our API design.
I'd prefer for people who need to deep configure resolve.conf themselves supporting a configmap mounted at /etc/resolv.conf. If we had single file injection we could solve this problem.
| * Implementation owner: Bowei Du <[bowei@google.com](mailto:bowei@google.com)>, Zihong Zheng <[zihongz@google.com](mailto:zihongz@google.com)> | ||
|
|
||
| # Overview | ||
| The `/etc/resolv.conf` in a pod is managed by Kubelet and its contents are generated based on `pod.dnsPolicy`. For `dnsPolicy: Default`, `resolv.conf` is copied from the node where the pod is running. If the `dnsPolicy` is `ClusterFirst`, the contents of the resolv.conf is the hosts `resolv.conf` augmented with the following options: |
There was a problem hiding this comment.
The statement "For dnsPolicy: Default, resolv.conf is copied from the node where the pod is running" is incorrect. It only copies nameserver and search. I'd love to see it actually does that but it is not, and I think the statement should be reflecting that.
There was a problem hiding this comment.
I will make the text more clear.
| - my.dns.search.suffix | ||
| - $HOST | ||
| options: | ||
| - ndots: 2 |
There was a problem hiding this comment.
Want to raise that not all options are {key}:{value}, options like rotate and debug are just flags. I think []string might be the best because the intention here is to capture the options, and no additional processing is assumed.
| ```bash | ||
| nameserver 1.2.3.4 | ||
| search foo.com | ||
| options: ndots: 1 |
There was a problem hiding this comment.
This is not a valid resolv.conf syntax, should be options ndots:1.
| # Populated from searchPaths | ||
| search ns1.svc.cluster.local my.dns.search.suffix foo.com | ||
| # Populated from options | ||
| options ndots: 2 |
There was a problem hiding this comment.
Don't know if it is correct to have a space between : and the value, (e.g., ndots:" "2), but I have not seen it being used this way.
| DNSParams *PodDNSParams | ||
| } | ||
|
|
||
| type PodDNSParams struct { |
There was a problem hiding this comment.
while we are on this, should we also add domain and sortlist that are also supported in resolv.conf?
There was a problem hiding this comment.
I am trying to be as parsimonious as possible in the initial allowed options.
There was a problem hiding this comment.
It should be easy to add the other fields in the future...
| - name: example | ||
| image: example | ||
| dnsPolicy: Custom | ||
| dnsParams: |
There was a problem hiding this comment.
I don't think dnsParams is the best key name for this. The customization is around resolv.conf, which is used specifically by some resolver implementations. I think it should be referencing resolv.conf customization. This should also address the concern that how is this applicable to Windows.
|
Updated, please take a look. |
There was a problem hiding this comment.
This seems OK, but we also have kubernetes/kubernetes#54773 which seems to just want to change options in addition to ClusterFirst.
Given the PR in its current state, we could add a ClusterFirst struct { Options []string } block, but that starts to smell funny.
Thought experiment here - bear with me. What if DnsPolicy was adjusted to mean "initial settings" and we simply took these new fields as "appends".
e.g.
ClusterFirst means "start with the in-cluster config, add the host's config, and then add customiztions".
Default means "start with the host's config, and then add customizations"
Custom means "start with nothing, and then add customizations".
Then people could tweak options in ClusterFirst mode. They could maybe even override ndots (not sure what happens if you list an option twice).
We could keep (or not) the templating. Or revisit that later?
If we eventually rejigger the DNS schema, we can retain the structure, but add new policy starting points.
Also, unrelated to that, we should consider whether this belongs in PodPreset. @droot
| metadata: | ||
| namespace: ns1 | ||
| name: example | ||
| spec: |
There was a problem hiding this comment.
If a user REALLY wants to customize, they might choose to bypass kube-DNS entirely?
| - name: example | ||
| image: example | ||
| dnsPolicy: Custom | ||
| dnsParams: |
There was a problem hiding this comment.
How does windows configure name resolution? We need an answer to that before we merge this, really.
| The following is a Pod dnsParams that only contains the host search paths: | ||
|
|
||
| ```yaml | ||
| dnsParams: |
There was a problem hiding this comment.
oh puke.
can anyone from @kubernetes/sig-windows-misc comment on this? I'd like to not make the API any more non-windows-friendly, but I don't know how to express this in a windows-friendly way
| const ( | ||
| DNSParamsSearchPathNamespace = "$(NAMESPACE)" | ||
| DNSParamsSearchPathClusterDomain = "$(CLUSTER)" | ||
| DNSParamsSearchPathHostPaths = "$(HOST)" |
There was a problem hiding this comment.
k8s-ci-robot
added
the
sig/windows
| dnsParams: | ||
| custom: | ||
| search: | ||
| - $(NAMESPACE).svc.$(CLUSTER) |
There was a problem hiding this comment.
What's "$(CLUSTER)" here? New magic downward API?
There was a problem hiding this comment.
I don't think its appropriate to overlap our existing downward API with a new source of values. Will users be allowed to do downward API injection of env vars? What happens when they clash? Do we define three magic ones as part of the API (note in every other spot we require the user to define NAMESPACE as a downward envFrom for pod.namespace)?
There was a problem hiding this comment.
The point was to provide a small number of magic values without any other scaffolding. Users can't define new magic here, nor does this magic overlap with env or anything else. That may not sway the conversation, but it is context.
| | Substitution | Description | | ||
| | ---- | ---- | | ||
| | `$(NAMESPACE)` | Namespace of the Pod | | ||
| | `$(CLUSTER)` | Kubernetes cluster domain (e.g. `cluster.local`) | |
There was a problem hiding this comment.
The moment we add this people will ask for it in other places. This should be part of the downward API if we want to add it.
There was a problem hiding this comment.
And it needs a better name, it's not CLUSTER it's CLUSTER_SUBDOMAIN or similar.
There was a problem hiding this comment.
Making it part of downward API is an interesting idea. Do we have other places where downward API pulls values from thin air? ISTR most of them are pulled from pod fields. Namespace could be metadata.namespace (if it isn't already), but we have no field for cluster suffix.
This proposal was doing a limited-set of substitutions outside of downward API because, as we know, the downward API requires more structure than simple templating. These were not intended to be generic env-vars that expand, just a few magic names in this particular piece of the API.
It's hacky, and I figured it would draw attention, but I hoped we could keep it simple (since there's no risk of collision - we control all the variables, unlike env).
We could make this a generic env expansion, which would require users to touch both env and dnsPolicy. That feels like overkill.
There was a problem hiding this comment.
The last time I looked at making this part of the downward API, this seems the first in terms of values being passed that are not part of a declared spec.
| | ---- | ---- | | ||
| | `$(NAMESPACE)` | Namespace of the Pod | | ||
| | `$(CLUSTER)` | Kubernetes cluster domain (e.g. `cluster.local`) | | ||
| | `$(HOST)` | Host search paths. This is a special substitution that MUST ONLY appear at the end of the `searchPaths` list | |
There was a problem hiding this comment.
If it can only be at the end why not just have a boolean includeHostSearchPaths?
There was a problem hiding this comment.
Making this default to adding the host search paths to make it easier for users:
excludeHostSearchPaths
Thoughts?
| actual result is found. This order of magnitude increase of query rate imposes a | ||
| large load on the kube-dns service. At the same time, there are user | ||
| applications do not need the convenience of the name aliases and do not wish to | ||
| pay this performance cost. |
There was a problem hiding this comment.
Add the security implication of our current default search paths as well. @thockin is this our chance to fix that one?
There was a problem hiding this comment.
I think the schematic rejigger is orthogonal to this, mostly. I'm pretty sure that to make that work, we'll need a new DNSPolicy that enacts the new search paths, opt-in, and the DNS server will either serve both styles or we will need 2 DNS servers.
We could make the default become admission control or PodPreset, so admins could flip it to new mode for the whole cluster eventually. The old mode basically can't go away for a long time.
| ... | ||
| DNSPolicy string | ||
| DNSParams *PodDNSParams | ||
| ... |
| ``` | ||
|
|
||
| Override `ndots` and add custom search path. Note that overriding the ndot may | ||
| break the functionality of some of the search paths the |
| dnsParams: | ||
| custom: | ||
| searchPaths: | ||
| - $HOST |
| custom: | ||
| searchPaths: | ||
| - my.custom.suffix | ||
| - $HOST |
| // This will not appear in types.go but is here for explication purposes. | ||
| type DNSParamsSubstitution string | ||
| const ( | ||
| DNSParamsSearchPathNamespace = "$(NAMESPACE)" |
There was a problem hiding this comment.
DNSParamsSearchPathNamespace DNSParamsSubstitution = "$(NAMESPACE)"
|
@thockin regarding the "append" model, this seems to be not so useful as:
|
bowei
force-pushed
the
pod-resolv-conf
branch
from
9cead22
to
eb79043
Compare
|
We're not doing alphas by annotation any more. Now we use fields and alpha gates to hide those fields. I'm approving this, but I think we should change the name |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. |
Automatic merge from submit-queue (batch tested with PRs 55812, 55752, 55447, 55848, 50984). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Add Custom Pod DNS to Kubernetes API **What this PR does / why we need it**: Ref: - Feature issue: kubernetes/enhancements#504 - Proposal: kubernetes/community#1276 This PR adds the relevant APIs, validation check and the underlying kubelet changes. **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: Fixes #NONE **Special notes for your reviewer**: /sig network @kubernetes/sig-network-api-reviews /assign @bowei @thockin **Release note**: ```release-note Add DNSConfig field to PodSpec and support "None" mode for DNSPolicy (Alpha). ```
Automatic merge from submit-queue (batch tested with PRs 55812, 55752, 55447, 55848, 50984). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Add Custom Pod DNS to Kubernetes API **What this PR does / why we need it**: Ref: - Feature issue: kubernetes/enhancements#504 - Proposal: kubernetes/community#1276 This PR adds the relevant APIs, validation check and the underlying kubelet changes. **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: Fixes #NONE **Special notes for your reviewer**: /sig network @kubernetes/sig-network-api-reviews /assign @bowei @thockin **Release note**: ```release-note Add DNSConfig field to PodSpec and support "None" mode for DNSPolicy (Alpha). ``` Kubernetes-commit: 8a5cf788b0560c176bf219f26e893d97a19de1e2
Automatic merge from submit-queue (batch tested with PRs 55812, 55752, 55447, 55848, 50984). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Add Custom Pod DNS to Kubernetes API **What this PR does / why we need it**: Ref: - Feature issue: kubernetes/enhancements#504 - Proposal: kubernetes/community#1276 This PR adds the relevant APIs, validation check and the underlying kubelet changes. **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: Fixes #NONE **Special notes for your reviewer**: /sig network @kubernetes/sig-network-api-reviews /assign @bowei @thockin **Release note**: ```release-note Add DNSConfig field to PodSpec and support "None" mode for DNSPolicy (Alpha). ``` Kubernetes-commit: 8a5cf788b0560c176bf219f26e893d97a19de1e2
Automatic merge from submit-queue (batch tested with PRs 55812, 55752, 55447, 55848, 50984). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Add Custom Pod DNS to Kubernetes API **What this PR does / why we need it**: Ref: - Feature issue: kubernetes/enhancements#504 - Proposal: kubernetes/community#1276 This PR adds the relevant APIs, validation check and the underlying kubelet changes. **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: Fixes #NONE **Special notes for your reviewer**: /sig network @kubernetes/sig-network-api-reviews /assign @bowei @thockin **Release note**: ```release-note Add DNSConfig field to PodSpec and support "None" mode for DNSPolicy (Alpha). ``` Kubernetes-commit: 8a5cf788b0560c176bf219f26e893d97a19de1e2
Automatic merge from submit-queue (batch tested with PRs 55812, 55752, 55447, 55848, 50984). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Add Custom Pod DNS to Kubernetes API **What this PR does / why we need it**: Ref: - Feature issue: kubernetes/enhancements#504 - Proposal: kubernetes/community#1276 This PR adds the relevant APIs, validation check and the underlying kubelet changes. **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: Fixes #NONE **Special notes for your reviewer**: /sig network @kubernetes/sig-network-api-reviews /assign @bowei @thockin **Release note**: ```release-note Add DNSConfig field to PodSpec and support "None" mode for DNSPolicy (Alpha). ``` Kubernetes-commit: 8a5cf788b0560c176bf219f26e893d97a19de1e2
Automatic merge from submit-queue (batch tested with PRs 55812, 55752, 55447, 55848, 50984). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Add Custom Pod DNS to Kubernetes API **What this PR does / why we need it**: Ref: - Feature issue: kubernetes/enhancements#504 - Proposal: kubernetes/community#1276 This PR adds the relevant APIs, validation check and the underlying kubelet changes. **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: Fixes #NONE **Special notes for your reviewer**: /sig network @kubernetes/sig-network-api-reviews /assign @bowei @thockin **Release note**: ```release-note Add DNSConfig field to PodSpec and support "None" mode for DNSPolicy (Alpha). ``` Kubernetes-commit: 8a5cf788b0560c176bf219f26e893d97a19de1e2
Automatic merge from submit-queue (batch tested with PRs 55812, 55752, 55447, 55848, 50984). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Add Custom Pod DNS to Kubernetes API **What this PR does / why we need it**: Ref: - Feature issue: kubernetes/enhancements#504 - Proposal: kubernetes/community#1276 This PR adds the relevant APIs, validation check and the underlying kubelet changes. **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: Fixes #NONE **Special notes for your reviewer**: /sig network @kubernetes/sig-network-api-reviews /assign @bowei @thockin **Release note**: ```release-note Add DNSConfig field to PodSpec and support "None" mode for DNSPolicy (Alpha). ``` Kubernetes-commit: 8a5cf788b0560c176bf219f26e893d97a19de1e2
Automatic merge from submit-queue (batch tested with PRs 55812, 55752, 55447, 55848, 50984). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Add Custom Pod DNS to Kubernetes API **What this PR does / why we need it**: Ref: - Feature issue: kubernetes/enhancements#504 - Proposal: kubernetes/community#1276 This PR adds the relevant APIs, validation check and the underlying kubelet changes. **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: Fixes #NONE **Special notes for your reviewer**: /sig network @kubernetes/sig-network-api-reviews /assign @bowei @thockin **Release note**: ```release-note Add DNSConfig field to PodSpec and support "None" mode for DNSPolicy (Alpha). ``` Kubernetes-commit: 8a5cf788b0560c176bf219f26e893d97a19de1e2
Automatic merge from submit-queue (batch tested with PRs 55812, 55752, 55447, 55848, 50984). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Add Custom Pod DNS to Kubernetes API **What this PR does / why we need it**: Ref: - Feature issue: kubernetes/enhancements#504 - Proposal: kubernetes/community#1276 This PR adds the relevant APIs, validation check and the underlying kubelet changes. **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: Fixes #NONE **Special notes for your reviewer**: /sig network @kubernetes/sig-network-api-reviews /assign @bowei @thockin **Release note**: ```release-note Add DNSConfig field to PodSpec and support "None" mode for DNSPolicy (Alpha). ``` Kubernetes-commit: 8a5cf788b0560c176bf219f26e893d97a19de1e2
cc: @thockin @MrHohn