This change is 

@k8s-bot gce etcd3 e2e test this

@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).

@k8s-bot gce etcd3 e2e test this

ok @madhusudancs, targetting 1.7 should be fine.

@k8s-bot gce etcd3 e2e test this

/assign @madhusudancs

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 :-)

@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.

/sig federation

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

@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):

@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):

Renamed to populateStubDomainsIfRequired().

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

Done

Comments from Reviewable

/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):

Ok. I stand corrected.

Comments from Reviewable

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@shashidharatd: The following test(s) failed:

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.

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