Skip to content
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

Make CoreDNS the default DNS server #7919

Merged
merged 1 commit into from
Dec 15, 2020

Conversation

rajansandeep
Copy link
Contributor

This will install CoreDNS as the default DNS server instead of kube-dns

Fixes #6500

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 12, 2019
@rajansandeep
Copy link
Contributor Author

This minimum change should do the trick.
I hope I'm not missing something obvious!

@zetaab
Copy link
Member

zetaab commented Nov 12, 2019

seems okay for me, please fix tests

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 12, 2019
Copy link
Member

@zetaab zetaab left a comment

Choose a reason for hiding this comment

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

Thanks @rajansandeep
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 13, 2019
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 22, 2019
@hakman
Copy link
Member

hakman commented Jan 5, 2020

/test pull-kops-verify-staticcheck

@rifelpet
Copy link
Member

rifelpet commented Feb 6, 2020

@justinsb can probably weigh in more here, but typically with changes like this we try to avoid modifying existing k8s clusters that arent also upgrading their k8s version. This way if someone updates Kops and runs update cluster without changing anything in their manifest, there should be minimal changes reported.

To make this kind of change we do one or both of two things:

  1. Update the default for newly created clusters only. This means that kops create cluster would populate the provider field with CoreDNS but update cluster wouldn't change anything.
  2. Update the default for newer k8s versions only. If we were targeting this change for Kops 1.17, we could have a check that only defaults to CoreDNS if the cluster's k8s version is >= 1.17. This makes the change more explicit since it is tied to a k8s version.

Thoughts?

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 6, 2020
@rajansandeep
Copy link
Contributor Author

For a start, I've rebased the PR.
I'll make further changes after we decide on the proposal made by @rifelpet

@@ -232,7 +232,7 @@ func (b *BootstrapChannelBuilder) buildAddons() *channelsapi.Addons {
}

kubeDNS := b.cluster.Spec.KubeDNS
if kubeDNS.Provider == "KubeDNS" || kubeDNS.Provider == "" {
Copy link
Member

Choose a reason for hiding this comment

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

We discussed this during office hours yesterday and determined that it will be best to default to CoreDNS only for clusters >= kubernetes 1.18. This way way upgrading kops by itself will not change the default, only upgrading kubernetes itself.

Heres an example of how you could get that information:

kv, err := k8sversion.Parse(cluster.Spec.KubernetesVersion)
if err != nil {
return err
}
// check if we should recommend turning off anonymousAuth on k8s versions gte than 1.10
// we do 1.10 since this is a really critical issues and 1.10 has it
if kv.IsGTE("1.10") {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rifelpet okay, I'll update the PR to reflect these changes.

Copy link
Member

Choose a reason for hiding this comment

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

How is kv, err := k8sversion.Parse(cluster.Spec.KubernetesVersion); kv.IsGTE vs b.cluster.IsKubernetesGTE?
I tend to prefer the latter.

@rifelpet
Copy link
Member

I'm wondering about the upgrade path for this. Given the note in the docs regarding switching from KubeDNS to CoreDNS, and to my knowledge kops doesnt have a simple way of removing manifests and knowing when it is safe to remove the KubeDNS deployment, we might need to resort to having a Required Action item in the 1.18 release notes.

@olemarkus
Copy link
Member

Any updates on this? Would be nice to get this in soon.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 11, 2020
@k8s-ci-robot k8s-ci-robot added area/documentation and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 14, 2020
@rajansandeep rajansandeep force-pushed the corednsdefault branch 2 times, most recently from dd2608f to bda60cf Compare April 14, 2020 18:45
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 15, 2020
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 15, 2020
@rajansandeep
Copy link
Contributor Author

Sorry @rifelpet had to push a change due to a small nit.

@rifelpet
Copy link
Member

/lgtm
/assign @justinsb

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 15, 2020
@rajansandeep
Copy link
Contributor Author

Any updates?

@rdrgmnzs
Copy link
Contributor

From the Kops office hours 05/08 we decided to wait to make CoreDNS the default DNS until Kops 1.19 and to introduce node-local-dns along with it to help avoid some of the iptables DNAT issues that can pop up.

@rifelpet rifelpet added this to the v1.19 milestone May 22, 2020
@olemarkus
Copy link
Member

By introducing node-local-dns we mean make it enabled by default?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 18, 2020
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Dec 14, 2020

@rajansandeep: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kops-verify-cloudformation bb1fb76 link /test pull-kops-verify-cloudformation
pull-kops-verify-hashes bb1fb76 link /test pull-kops-verify-hashes

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/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 15, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rajansandeep, zetaab

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 Dec 15, 2020
@hakman
Copy link
Member

hakman commented Dec 15, 2020

Did a quick rebase in case anyone feels that this should be merged into 1.19.

@olemarkus
Copy link
Member

It should definitely make it into 1.20.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 15, 2020
@k8s-ci-robot k8s-ci-robot merged commit 3cc34fd into kubernetes:master Dec 15, 2020
@k8s-ci-robot k8s-ci-robot modified the milestones: v1.19, v1.20 Dec 15, 2020
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. area/documentation cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make CoreDNS the default DNS in Kops
8 participants