[Federation] Automate configuring nameserver in cluster-dns for CoreDNS provider

[shashidharatd]

Addresses issue #42894 #42822

Release note:

[Federation] CoreDNS server will be automatically added to nameserver resolv.conf chain When using CoreDNS as dns provider for federation during federation join.

cc @madhusudancs @kubernetes/sig-federation-bugs

[k8s-reviewable]

This change is 

[shashidharatd]

@k8s-bot gce etcd3 e2e test this

[madhusudancs]

@shashidharatd as we discussed yesterday, this is targetted for v1.7 right? (I have set the milestone to 1.7 already, but let me know if you think this is for v1.6).

[shashidharatd]

@k8s-bot gce etcd3 e2e test this

[shashidharatd]

ok @madhusudancs, targetting 1.7 should be fine.

[shashidharatd]

@k8s-bot gce etcd3 e2e test this

[shashidharatd]

/assign @madhusudancs

[ghost]

I don't yet know enough about how kubedns is deployed in various environments to do a proper review of this, so I'm going to remove myself from the assignee list. If you want me to study up on kubedns deployments, let me know.

It does strike me as odd that we're adding CoreDNS-specific logic when it's not necessary for other DNS providers. Is it not possible to use the standard DNS recursion logic here instead, like the other DNS providers? I'm way out of my depth here, so will discuss it with you in person rather that confuse things here :-)

[shashidharatd]

@quinton-hoole, Sorry for the delay in responding to the concern raised by you. Below is the explanation. Hope you agree otherwise would follow the suggestions if any.

It does strike me as odd that we're adding CoreDNS-specific logic when it's not necessary for other DNS providers. Is it not possible to use the standard DNS recursion logic here instead, like the other DNS providers? I'm way out of my depth here, so will discuss it with you in person rather that confuse things here :-)

CoreDNS is private nameserver and we need additional measures to make sure the federation domain lookups by pods are answered by the CoreDNS server. Here is an blog explaining how to do this currently in kubernetes http://blog.kubernetes.io/2017/04/configuring-private-dns-zones-upstream-nameservers-kubernetes.html

Regarding the CoreDNS-specific logic in kubefed, We did discuss the alternatives previously on this PR. Alternative-1: Implement generically to add a private nameserver to DNS lookup chain (via additional flag to kubefed during cluster join.)
Alternative-2: Implicitly add the CoreDNS server to the DNS lookup chain during cluster join(if the CoreDNS DNS provider is used). Here user need not explicitly take care of this, but this introduces little additional burden to kubefed developers.

@madhusudancs, preferred to use Alternative-2 as it is better user experience and User's preferences should always come first than developer's preferences.

[shashidharatd]

/sig federation

[madhusudancs]

Reviewed 1 of 8 files at r1, 7 of 7 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

federation/pkg/kubefed/join.go, line 278 at r3 (raw file):

		}
		newConfigMap = appendStubDomains(newConfigMap,
			cmDep.Annotations[util.FedDNSProvider],

I think this isn't sufficient. kubefed panics if they keys don't exist. So you need the equivalent of

provider, ok := cmDep.Annotations[util.FedDNSProvider]
if !ok {
    ...
}

for all the three keys. Instead of doing that here, I am Ok if oyu pass cmDep.Annotations to the function and then do the check there.

---

federation/pkg/kubefed/join.go, line 401 at r3 (raw file):

}

func appendStubDomains(configMap *api.ConfigMap, dnsProvider, dnsZoneName, nameServer string) *api.ConfigMap {

Can we call this populateStubDomainsIfRequired() or something like that?

---

federation/pkg/kubefed/join_test.go, line 297 at r3 (raw file):

	deploymentList.Items = append(deploymentList.Items, deployment)

	if dnsProvider == util.FedDNSProviderCoreDNS {

Write this block before appending to the list for clarity.

Also, it's not necessary to initialize the list and append in two separate statements? They can be combined?

---

Comments from Reviewable

[shashidharatd]

@madhusudancs, have handled the comments. PTAL. Thanks!

---

Review status: 4 of 8 files reviewed at latest revision, 3 unresolved discussions.

---

federation/pkg/kubefed/join.go, line 278 at r3 (raw file):

Previously, madhusudancs (Madhusudan.C.S) wrote…

I think this isn't sufficient. kubefed panics if they keys don't exist. So you need the equivalent of

provider, ok := cmDep.Annotations[util.FedDNSProvider]
if !ok {
    ...
}

for all the three keys. Instead of doing that here, I am Ok if oyu pass cmDep.Annotations to the function and then do the check there.

@madhusudancs, there won't be panic, if key does not exist. the variable would be initialized with zero value for the type. Here is the reference.

I re-factored the function to take complete annotations as suggested.

---

federation/pkg/kubefed/join.go, line 401 at r3 (raw file):

Previously, madhusudancs (Madhusudan.C.S) wrote…

Can we call this populateStubDomainsIfRequired() or something like that?

Renamed to populateStubDomainsIfRequired().

---

federation/pkg/kubefed/join_test.go, line 297 at r3 (raw file):

Previously, madhusudancs (Madhusudan.C.S) wrote…

Write this block before appending to the list for clarity.

Also, it's not necessary to initialize the list and append in two separate statements? They can be combined?

Done

---

Comments from Reviewable

[madhusudancs]

/lgtm

---

Reviewed 2 of 4 files at r4.
Review status: 6 of 8 files reviewed at latest revision, 1 unresolved discussion.

---

federation/pkg/kubefed/join.go, line 278 at r3 (raw file):

Previously, shashidharatd (Shashidhara T D) wrote…

@madhusudancs, there won't be panic, if key does not exist. the variable would be initialized with zero value for the type. Here is the reference.

I re-factored the function to take complete annotations as suggested.

Ok. I stand corrected.

---

Comments from Reviewable

[k8s-github-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: madhusudancs, shashidharatd

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

• federation/OWNERS [madhusudancs]

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

[k8s-github-robot]

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

[k8s-ci-robot]

@shashidharatd: The following test(s) failed:

Test name	Commit	Details	Rerun command
pull-kubernetes-federation-e2e-gce	005e11b	link	@k8s-bot pull-kubernetes-federation-e2e-gce test this
Jenkins kops AWS e2e	005e11b	link	@k8s-bot kops aws e2e test this

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-github-robot]

Automatic merge from submit-queue (batch tested with PRs 42895, 45940)