Skip to content

Conversation

@gjkim42
Copy link
Member

@gjkim42 gjkim42 commented Apr 2, 2021

Enhancement issue: #2595

previous discussion: kubernetes/kubernetes#100583

@k8s-ci-robot k8s-ci-robot requested review from dcbw and thockin April 2, 2021 03:17
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/network Categorizes an issue or PR as relevant to SIG Network. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 2, 2021
@gjkim42 gjkim42 mentioned this pull request Apr 2, 2021
12 tasks
@gjkim42 gjkim42 changed the title Add expanded DNS configuration KEP WIP: Add expanded DNS configuration KEP Apr 2, 2021
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 2, 2021
@gjkim42 gjkim42 force-pushed the expanded-dns-config branch 8 times, most recently from a346c44 to e78ec0f Compare April 2, 2021 11:12
@gjkim42 gjkim42 changed the title WIP: Add expanded DNS configuration KEP Add expanded DNS configuration KEP Apr 2, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 2, 2021
@gjkim42
Copy link
Member Author

gjkim42 commented Apr 2, 2021

/cc @aojea @thockin @liggitt

@k8s-ci-robot k8s-ci-robot requested review from aojea and liggitt April 2, 2021 11:35
Comment on lines +360 to +143
#### Alpha -> Beta Graduation

- Address feedback from alpha
- Sufficient testing

#### Beta -> GA Graduation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we go to beta directly and enable it by default?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be ok to enable it by default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the relaxed validation cannot allow new expanded data into objects by default in the first release, since it would allow data the previous API server version would choke on

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liggitt
Isn't it enough to wrap the feature into the feature gate to make the user can disable it if there is such a case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no. that means that, by default, the first version of the API server including this feature allows storing unsafe data. we must default safe.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah... I think you are right. Let's go to alpha and disable the feature by default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. We can very likely skip beta, but we can't skip alpha

status: implementable
creation-date: 2021-04-02
reviewers:
- TBD
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@claudiubelu do you know the implications of this resolv.conf changes in windows?

-->

In HA(highly-available) clusters, all kube-apiservers should enable or disable
expanded DNS configuration feature.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there should be no problem meanwhile they don't create pods with an expanded DNSConfig

Copy link
Member Author

@gjkim42 gjkim42 Apr 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should avoid the situation that there is a HA cluster with 1.22 apiserver and 1.21 apiserver having a pod with expanded DNS configuration due to partial roll-back.
If a user updates the pod, it can be updated by 1.22 apiserver or blocked by 1.21 apiserver. That will be an inconsistent behavior.

@aojea
Copy link
Member

aojea commented Apr 2, 2021

LGTM, this was previously discussed here

the problem was discussed here kubernetes/kubernetes#100583
and the implementation is not a big change
kubernetes/kubernetes#100622

the KEP and the feature gate will help to document the feature and give us the possibility to avoid surprises :)

@gjkim42 gjkim42 force-pushed the expanded-dns-config branch from e78ec0f to 8b37167 Compare April 2, 2021 15:11
@thockin
Copy link
Member

thockin commented Apr 30, 2021

What does this need in terms of docs?

@gjkim42
Copy link
Member Author

gjkim42 commented Apr 30, 2021

I think we need some documentation updates like

Linux's libc (a.k.a. glibc) has a limit for the DNS nameserver records to 3 by default. What's more, for the glibc versions which are older than glibc-2.17-222 (the new versions update see this issue), the allowed number of DNS search records has been limited to 6 (see this bug from 2005). Kubernetes needs to consume 1 nameserver record and 3 search records. This means that if a local installation already uses 3 nameservers or uses more than 3 searches while your glibc version is in the affected list, some of those settings will be lost. To work around the DNS nameserver records limit, the node can run dnsmasq, which will provide more nameserver entries. You can also use kubelet's --resolv-conf flag. To fix the DNS search records limit, consider upgrading your linux distribution or upgrading to an unaffected version of glibc.

@gjkim42 gjkim42 force-pushed the expanded-dns-config branch from fedcf67 to fce09c5 Compare May 1, 2021 05:37
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 1, 2021
@gjkim42
Copy link
Member Author

gjkim42 commented May 1, 2021

Only updated the release signoff checklist to the desired state.


### Version Skew Strategy

In clusters with a replicated control plane, all kube-apiservers should enable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more about kubelets. It is valid to have kubelets be a (slightly) lower version than apiserver. Looking at kubelet code I can see this will throw warnings (pkg/kubelet/network/dns/dns.go Configurer.CheckLimitsForResolvConf()) but that does not hard-fail. Still worth noting here.

In the same file, Configurer.formDNSSearchFitsLimits(), it does truncate. That is what is important to note here (and to test-confirm) -- back-rev kubelet should be fine, it will just truncate the results.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

thanks @thockin

@gjkim42
Copy link
Member Author

gjkim42 commented May 7, 2021

@johnbelamaric
Would you please take a look and give a PRR? I hope we can catch the v1.22 enhancement freeze date (May 13th).
Thanks :)

@gjkim42
Copy link
Member Author

gjkim42 commented May 8, 2021

@johnbelamaric
Thanks!
Updated the PRR questionnaire section.

@gjkim42 gjkim42 force-pushed the expanded-dns-config branch 2 times, most recently from 2ca6725 to d7713c8 Compare May 8, 2021 15:33
@gjkim42 gjkim42 force-pushed the expanded-dns-config branch 2 times, most recently from 4852c0e to 19a2876 Compare May 10, 2021 04:43
@gjkim42
Copy link
Member Author

gjkim42 commented May 10, 2021

@thockin @johnbelamaric
Added graduation criteria for alpha.
Could you PTAL? Thanks

@gjkim42 gjkim42 force-pushed the expanded-dns-config branch from 19a2876 to ce72cc7 Compare May 10, 2021 04:52
@johnbelamaric
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gjkim42, johnbelamaric, thockin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 10, 2021
@johnbelamaric
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 10, 2021
@k8s-ci-robot k8s-ci-robot merged commit 5422920 into kubernetes:master May 10, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone May 10, 2021
@gjkim42 gjkim42 deleted the expanded-dns-config branch May 10, 2021 17:02

Yes, the feature can be disabled by disabling the feature gate.

Once the feature is disabled, kube-apiserver will reject the pod having expanded
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gjkim42 what happens to existing pods in the API when the feature gate is disabled, and then something tries to update non-DNS podSpec fields? Do those things just their update denied, even though they didn't change any relevant fields?

Copy link
Member Author

@gjkim42 gjkim42 May 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens to existing pods in the API when the feature gate is disabled

The existing pods will be running with the expanded DNS config.

and then something tries to update non-DNS podSpec fields? Do those things just their update denied, even though they didn't change any relevant fields?

In v1.22 or higher, you can update non-DNSConfig podSpec fields, because we will add the exception.
(See the PR in progress.)
In v1.21 or lower(downgrade), any update requests will be denied, if the pod already had expanded DNS config.

@ritpanjw
Copy link

Hello @gjkim42 👋 , 1.22 Docs Shadow here.

This enhancement is marked as Needs Docs for 1.22 release.
Please follow the steps detailed in the documentation to open a PR against dev-1.22 branch in the k/website repo. This PR can be just a placeholder at this time and must be created before Fri July 9, 11:59 PM PDT.
Also, take a look at Documenting for a release to familiarize yourself with the docs requirement for the release.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/network Categorizes an issue or PR as relevant to SIG Network. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants