Add CoreDNS as feature in kubeadm #52501

rajansandeep · 2017-09-14T20:06:08Z

What this PR does / why we need it:
This PR adds CoreDNS as a DNS plugin via the feature-gate option in Kubeadm init.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged):
Fixes kubernetes/kubeadm#446

Special notes for your reviewer:

Release note:

kubeadm: Add an experimental mode to deploy CoreDNS instead of KubeDNS

/cc @johnbelamaric

k8s-ci-robot · 2017-09-14T20:06:09Z

@rajansandeep: GitHub didn't allow me to request PR reviews from the following users: johnbelamaric.

Note that only kubernetes members can review this PR, and authors cannot review their own PRs.

In response to this:

What this PR does / why we need it:
This PR adds CoreDNS as a DNS plugin via the feature-gate option in Kubeadm init.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged):
Fixes kubernetes/enhancements#427

Special notes for your reviewer:

Release note:
/cc @johnbelamaric

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.

k8s-ci-robot · 2017-09-14T20:06:15Z

Hi @rajansandeep. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

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

rajansandeep · 2017-09-14T20:09:34Z

@johnbelamaric @chrisohaver

luxas · 2017-09-15T21:00:46Z

/ok-to-test
/release-note

cc @kubernetes/sig-cluster-lifecycle-pr-reviews
/assign

luxas · 2017-09-15T21:01:37Z

(I'll take a look at this later when I get to it)

Thanks for this PR!!

mattmoyer

This is really great to see, thanks! I left a few comments.

I have one question for @luxas or someone else in @kubernetes/sig-cluster-lifecycle-feature-requests: do we want to create a way to switch from kube-dns to CoreDNS in a running cluster, or is that out of scope for kubeadm? It feels similar to upgrade but not quite the same.

mattmoyer · 2017-09-15T20:10:29Z

cmd/kubeadm/app/phases/addons/dns/manifests.go

+        effect: NoSchedule
+      containers:
+      - name: coredns
+        image: coredns/coredns:{{ .Version }}


We probably need to add {{ .ImageRepository }} and {{.Arch}} here, similar to the existing kube-dns manifests.

I think this probably also means mirroring CoreDNS into the gcr.io/google_containers registry. Is this an option? I'm not sure who owns that decision.

coredns/coredns should be built as a manifest list (ref: k8s multiarch proposal)

Then we don't need to pass {{ .Arch }} at all, docker will just pull the right variant of the image.

Reach out to me if you want to know how to build a manifest list: https://docs.docker.com/registry/spec/manifest-v2-2/

ping @rajansandeep @johnbelamaric on the manifest list building

mattmoyer · 2017-09-15T20:13:15Z

cmd/kubeadm/app/phases/addons/dns/manifests.go

+            name: coredns
+            items:
+            - key: Corefile
+              path: Corefile


If we add multi-architecture support above, we need the beta.kubernetes.io/arch nodeAffinity selector here to make sure it schedules to an appropriate node.

not needed anymore as the latest image is a manifest list

mattmoyer · 2017-09-15T20:15:47Z

cmd/kubeadm/app/phases/addons/dns/manifests.go

+        errors
+        log stdout
+        health
+        kubernetes {{ .DNSDomain }} {{ .Servicecidr }}


nit: maybe s/Servicecidr/ServiceCIDR/ for consistency?

mattmoyer · 2017-09-15T20:28:36Z

cmd/kubeadm/app/phases/addons/dns/dns.go

+	return nil
+}
+
+//convSubnet fetches the servicecidr and modifies the mask to the nearest class


I wonder if this should error out when the configured CIDR doesn't match a full class instead of trying to fix it. Any idea how kube-dns handles this case?

Could this lead to answering reverse DNS queries with the wrong response if they were for IPs that were part of the service subnet, but not in the nearest class?

Default is a /12 so it would error out by default 😉

This is a general DNS problem. We're looking at some solutions here coredns/coredns#1074

kube-dns today captures ALL PTRs. Leading to things like kubernetes/dns#124 and (I think) the ability to hijack the PTR of any IP in the world.

Even if we nail it down to just the service CIDR, we still have problems with PTR and manually added endpoints in K8s, especially in a multi-tenant deployment.

mattmoyer · 2017-09-15T21:05:42Z

cmd/kubeadm/app/phases/addons/dns/versions.go

+
+// GetCoreDNSVersion returns the right CoreDNS version for a specific k8s version
+func GetCoreDNSVersion(kubeVersion *version.Version) string {
+	// v1.7.0+ uses CoreDNS-011, just return that here


Should be v1.8.0+ (here and again below).

luxas · 2017-09-15T21:15:45Z

do we want to create a way to switch from kube-dns to CoreDNS in a running cluster, or is that out of scope for kubeadm? It feels similar to upgrade but not quite the same.

Possibly in phases/upgrade/postupgrade.go... If the feature gate is enabled and if we think that's worth the effort.

Thanks for the review @mattmoyer!

timothysc · 2017-09-18T22:59:27Z

Please squash the PRs. Given that @luxas and @mattmoyer are on it I'll defer to them.

bradbeam · 2017-09-19T15:38:59Z

cmd/kubeadm/app/phases/addons/dns/manifests.go

+        errors
+        log stdout
+        health
+        kubernetes {{ .DNSDomain }} {{ .Servicecidr }}


Should prometheus be added in as part of the default to align with the rest of Kubernetes components?

yes, that makes sense

kad · 2017-09-25T10:03:53Z

cmd/kubeadm/app/phases/addons/dns/dns.go

+	if err != nil {
+		log.Fatal(err)
+	}
+	servicecidr = ipv4Net.String()


Maybe it would be good to test that functionality with IPv6 as well ?
IPv6 is coming soon anyway, doesn't make sense to write IPv4 code only.

luxas

some quick feedback.
We have a couple of open questions still:

Should this be seen as a "drop-in" replacement called kube-dns still or is it a "net-new" replacment for kube-dns?
Should RBAC ClusterRole & Binding be auto-bootstrapped?
Should the service still be called kube-dns?

Action items:

Make sure it's possible to "upgrade" any v1.8 cluster to using coredns. Make sure that kubeadm upgrade does the right things, shows the right text, removes the old kube-dns if not needed, etc.
The coredns image must be a manifest list. Please make the next release a manifest list so we can use it. Reach out to me if you need help with doing that.

luxas · 2017-10-11T19:23:10Z

cmd/kubeadm/app/cmd/init.go

@@ -382,11 +382,6 @@ func (i *Init) Run(out io.Writer) error {
 		return err
 	}

-	// Create/update RBAC rules that makes the nodes to rotate certificates and get their CSRs approved automatically


Why remove this?

luxas · 2017-10-11T19:24:06Z

cmd/kubeadm/app/cmd/init.go

+			return err
+		}
+	} else {
+		if err := dnsaddonphase.EnsureDNSAddon(i.cfg, client); err != nil {


Can you move this logic inside of EnsureDNSAddon? (i.e. the logic whether to use kube- or core-dns)

luxas · 2017-10-11T19:30:00Z

cmd/kubeadm/app/phases/addons/dns/manifests.go

+        effect: NoSchedule
+      containers:
+      - name: coredns
+        image: coredns/coredns:{{ .Version }}


coredns/coredns should be built as a manifest list (ref: k8s multiarch proposal)

Then we don't need to pass {{ .Arch }} at all, docker will just pull the right variant of the image.

Reach out to me if you want to know how to build a manifest list: https://docs.docker.com/registry/spec/manifest-v2-2/

luxas · 2017-10-11T19:31:06Z

cmd/kubeadm/app/phases/addons/dns/manifests.go

+    protocol: TCP
+`
+
+	//ConfigMap is the CoreDNS ConfigMap manifest


needs to be a godoc comment

luxas · 2017-10-11T19:31:25Z

cmd/kubeadm/app/phases/addons/dns/manifests.go

+kind: ClusterRole
+metadata:
+  labels:
+    kubernetes.io/bootstrapping: rbac-defaults


should not be here

luxas · 2017-10-11T19:34:17Z

cmd/kubeadm/app/phases/addons/dns/versions.go

@@ -38,3 +39,18 @@ func GetKubeDNSManifest(kubeVersion *version.Version) string {
 	// In the future when the kube-dns version is bumped at HEAD; add conditional logic to return the right manifest
 	return v170AndAboveKubeDNSDeployment
 }
+
+// GetCoreDNSVersion returns the right CoreDNS version for a specific k8s version
+func GetCoreDNSVersion(kubeVersion *version.Version) string {


Let's use the same functions -- don't create a new one. Instead, choose what version to return based on DNS provider.
This way the right values will be shown for upgrades as well.

luxas · 2017-10-11T19:35:05Z

cmd/kubeadm/app/phases/addons/dns/manifests.go

+        k8s-app: coredns
+      annotations:
+        scheduler.alpha.kubernetes.io/critical-pod: ''
+        scheduler.alpha.kubernetes.io/tolerations: '[{"key":"CriticalAddonsOnly", "operator":"Exists"}]'


This annotation isn't respected anymore, tolerations are now set on the PodSpec. See other manifests in cmd/kubeadm

luxas · 2017-10-11T19:36:10Z

cmd/kubeadm/app/phases/addons/dns/dns.go

@@ -127,6 +131,161 @@ func createKubeDNSAddon(deploymentBytes, serviceBytes []byte, client clientset.I
 	return nil
 }

+func EnsureCoreDNSAddon(cfg *kubeadmapi.MasterConfiguration, client clientset.Interface) error {


make this private and call from EnsureDNSAddon

luxas · 2017-10-11T19:36:56Z

cmd/kubeadm/app/phases/addons/dns/manifests.go

+	// CoreDNSService is the CoreDNS Service manifest
+	CoreDNSService = `
+apiVersion: v1
+kind: Service


What did we think here? Would it be possible to keep the same kube-dns service?

ping @kubernetes/sig-network-pr-reviews should we keep the kube-dns name or not?
In some way, it would be cool to keep the name to signal "this is the DNS service for Kubernetes, regardless of implementation"

That sounds reasonable to me

luxas · 2017-10-11T19:37:37Z

cmd/kubeadm/app/phases/addons/dns/dns.go

+
+//convSubnet fetches the serviceCIDR and modifies the mask to the nearest class
+//CoreDNS requires CIDR notations for reverse zones as classful.
+func convSubnet(cidr string) (serviceCIDR string) {


unit tests please

rajansandeep · 2017-11-07T17:18:21Z

@luxas I fixed the golint errors. I am not able to verify the other error which is failing the test.

kad · 2017-11-07T17:23:22Z

@rajansandeep you need to run hack/update-bazel.sh it will update cmd/kubeadm/app/phases/addons/dns/BUILD file, which you need to add to your squashed commit.

rajansandeep · 2017-11-07T19:22:15Z

/test pull-kubernetes-unit

fturib · 2017-11-07T20:11:02Z

/retest

rajansandeep · 2017-11-07T21:11:03Z

Did the test flake? @luxas @kad

fturib · 2017-11-07T21:38:55Z

Looks like the very same test were ok on the precedent launch (that was ignored because I did this /retest before the end). So it seems to be a flaky test .. let's retry once..

fturib · 2017-11-07T21:39:26Z

/retest

luxas · 2017-11-07T21:46:36Z

/lgtm

Yes, flaked

k8s-github-robot · 2017-11-07T21:47:06Z

[APPROVALNOTIFIER] This PR is APPROVED

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

Associated issue: 427

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

Needs approval from an approver in each of these OWNERS Files:

~~cmd/kubeadm/OWNERS~~ [luxas]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

rajansandeep · 2017-11-08T00:08:52Z

/retest

fejta-bot · 2017-11-08T02:27:15Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

fejta-bot · 2017-11-08T06:36:47Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

kad · 2017-11-08T07:09:19Z

bazel builds seems to be legitimate errors:

W1011 19:16:09.889] 2017/10/11 19:16:09 missing strict dependencies:
W1011 19:16:09.890] 	cmd/kubeadm/app/phases/addons/dns/dns.go: import of k8s.io/api/rbac/v1, which is not a direct dependency

and

W1023 21:46:32.637] ERROR: /go/src/k8s.io/kubernetes/cmd/kubeadm/app/phases/addons/dns/BUILD:28:1: GoCompile cmd/kubeadm/app/phases/addons/dns/~normal~go_default_library~/k8s.io/kubernetes/cmd/kubeadm/app/phases/addons/dns.a failed (Exit 1): compile failed: error executing command 
W1023 21:46:32.643] 2017/10/23 21:46:32 missing strict dependencies:
W1023 21:46:32.644] 	cmd/kubeadm/app/phases/addons/dns/dns.go: import of k8s.io/kubernetes/cmd/kubeadm/app/features, which is not a direct dependency
W1023 21:46:32.645] 	cmd/kubeadm/app/phases/addons/dns/versions.go: import of k8s.io/kubernetes/cmd/kubeadm/app/features, which is not a direct dependency

rajansandeep · 2017-11-08T12:46:51Z

The BUILD file seems to already include the missing dependencies. Am I missing something?
Also, the dates in the error logs seem old.

luxas · 2017-11-08T13:54:46Z

I think these are just flakes. Bazel seems fine. The TestCRD unit test fails right now. IIRC someone is working on fixing the flakiness there already /retest

…

On 08 Nov 2017, at 14:47, Sandeep Rajan ***@***.***> wrote: The BUILD file seems to already include the missing dependencies. Am I missing something? Also, the dates in the error logs seem old. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

rajansandeep · 2017-11-08T15:15:57Z

/retest

rajansandeep · 2017-11-08T15:28:40Z

The command for retest isn't restarting the test.

luxas · 2017-11-08T15:32:53Z

/test all

rajansandeep · 2017-11-08T18:10:44Z

/retest

k8s-ci-robot · 2017-11-08T18:55:56Z

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

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce-bazel	`8902354`	link	`/test pull-kubernetes-e2e-gce-bazel`
pull-kubernetes-e2e-gce-gpu	a64ce7b3a96a480a40ea47d23a9f39a72487cc59	link	`/test pull-kubernetes-e2e-gce-gpu`

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.

rajansandeep · 2017-11-08T18:59:25Z

/retest

k8s-github-robot · 2017-11-08T22:19:27Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-11-08T23:41:14Z

Automatic merge from submit-queue (batch tested with PRs 54493, 52501, 55172, 54780, 54819). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 14, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 14, 2017

k8s-github-robot assigned krousey and timothysc Sep 14, 2017

k8s-github-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Sep 14, 2017

johnbelamaric mentioned this pull request Sep 15, 2017

Add an option to use CoreDNS instead of KubeDNS in v1.9 kubernetes/kubeadm#446

Closed

luxas added this to the v1.9 milestone Sep 15, 2017

k8s-ci-robot assigned luxas Sep 15, 2017

mattmoyer reviewed Sep 15, 2017

View reviewed changes

timothysc removed their assignment Sep 18, 2017

timothysc added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 18, 2017

bradbeam reviewed Sep 19, 2017

View reviewed changes

amitkumarj441 mentioned this pull request Sep 24, 2017

Bump version #52941

Closed

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 24, 2017

kad reviewed Sep 25, 2017

View reviewed changes

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 3, 2017

luxas reviewed Oct 11, 2017

View reviewed changes

k8s-ci-robot added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Oct 11, 2017

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Nov 7, 2017

adding coredns as a featuregate

f3dc622

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2017

k8s-github-robot merged commit d42be07 into kubernetes:master Nov 8, 2017

rajansandeep deleted the featurecoredns branch November 10, 2017 18:49

Add CoreDNS as feature in kubeadm #52501

Add CoreDNS as feature in kubeadm #52501

Conversation

rajansandeep commented Sep 14, 2017 • edited by luxas

k8s-ci-robot commented Sep 14, 2017

k8s-ci-robot commented Sep 14, 2017

rajansandeep commented Sep 14, 2017

luxas commented Sep 15, 2017

luxas commented Sep 15, 2017 • edited

mattmoyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luxas commented Sep 15, 2017

timothysc commented Sep 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luxas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajansandeep commented Nov 7, 2017

kad commented Nov 7, 2017

rajansandeep commented Nov 7, 2017

fturib commented Nov 7, 2017

rajansandeep commented Nov 7, 2017

fturib commented Nov 7, 2017

fturib commented Nov 7, 2017

luxas commented Nov 7, 2017

k8s-github-robot commented Nov 7, 2017

rajansandeep commented Nov 8, 2017

fejta-bot commented Nov 8, 2017

fejta-bot commented Nov 8, 2017

kad commented Nov 8, 2017

rajansandeep commented Nov 8, 2017

luxas commented Nov 8, 2017 via email

rajansandeep commented Nov 8, 2017

rajansandeep commented Nov 8, 2017

luxas commented Nov 8, 2017

rajansandeep commented Nov 8, 2017

k8s-ci-robot commented Nov 8, 2017 • edited

rajansandeep commented Nov 8, 2017

k8s-github-robot commented Nov 8, 2017

k8s-github-robot commented Nov 8, 2017

rajansandeep commented Sep 14, 2017 •

edited by luxas

luxas commented Sep 15, 2017 •

edited

k8s-ci-robot commented Nov 8, 2017 •

edited