federation: Updating KubeDNS to try finding a local service first for federation query #27708

nikhiljindal · 2016-06-20T07:50:37Z

Updating KubeDNS to try to find a local service first for federation query.
Without this change, KubeDNS always returns the DNS hostname, even if a local service exists.

Have updated the code to first remove federation name from path if it exists, so that the default search for local service happens. If we dont find a local service, then we try to find the DNS hostname.

Will appreciate a strong review since this is my first change to KubeDNS.
#25727 was the original PR that added federation support to KubeDNS.

cc @kubernetes/sig-cluster-federation @quinton-hoole @madhusudancs @bprashanth @mml

nikhiljindal · 2016-06-20T07:55:47Z

pkg/dns/dns.go

+	path := strings.Split(trimmed, ".")
+	isFederationQuery := false
+	federationPath := []string{}
+	if !exact && kd.isFederationQuery(path) {


I am not sure about this exact parameter.
Looking at the existing code, it seems that we should be checking !exact here.
Can remove it if its not required.

@nikhiljindal See #25727 (comment) for context.

ghost · 2016-06-20T17:26:05Z

Thanks @nikhiljindal - I'm going to review this now.

madhusudancs · 2016-06-20T19:16:33Z

pkg/dns/dns.go

@@ -420,8 +420,20 @@ func (kd *KubeDNS) newHeadlessService(service *kapi.Service) error {
 func (kd *KubeDNS) Records(name string, exact bool) (retval []skymsg.Service, err error) {
 	glog.Infof("Received DNS Request:%s, exact:%v", name, exact)
 	trimmed := strings.TrimRight(name, ".")
-	segments := strings.Split(trimmed, ".")
-	path := reverseArray(segments)
+	path := strings.Split(trimmed, ".")


Call this segments for terminology consistency. path is the reverse segments.

madhusudancs · 2016-06-20T19:18:28Z

LGTM other than a minor nit. Feel free to apply the label after fixing it.

ghost · 2016-06-20T19:56:31Z

pkg/dns/dns.go

+		federationPath = append(federationPath, path...)
+		// To try local service, remove federation name from path.
+		// Federation name is 3rd in the path (after service name and namespace).
+		path = append(path[:2], path[3:]...)


@nikhiljindal @madhusudancs I'm probably overlooking something, but its not clear to me where we ensure that we avoid returning the cluster IP of a local service that has no healthy endpoints backing it?

It works when there is no endpoint for the local service (I have a test for it).
But I just tried deleting endpoints from an existing service and it still returned me the endpoint (which I deleted). Am debugging that.

Well that was just an error in the way I was testing.
What I said above was only true for headless services. Updated the code to handle normal services as well (which is what federation will create)

nikhiljindal · 2016-06-21T05:06:49Z

Updated the code to check for endpoints of a service before returning its cluster IP for a federation query.
We return the DNS hostname instead if there are no endpoints.

Added tests as well.
I have also added a test for headless service (since I wrote that first), but realized that we wont be creating headless services in federation. So its not strictly required, but I have let it be.

PTAL

ghost · 2016-06-21T05:40:01Z

This is a pretty substantial change that will need to be carefully reviewed and tested. If we break KubeDNS, the cluster is broken, so that's really bad. Do we understand why it appeared to work, at least in a limited fashion, before these changes?

To be clear, I've not reviewed to code extensively yet. But I'm very nervous about making changes to such a key component of every Kubernetes cluster so late before a major release, unless we are super-confident about the proposed changes.

Are we super-confident @nikhiljindal @madhusudancs ? @ArtfulCoder could you also please take a look? More eyes are better.

ghost · 2016-06-21T05:41:59Z

PS: I would ideally like us to bang hard on this for a few days in a soak test cluster before declaring it good. I can assist in making that happen.

nikhiljindal · 2016-06-21T06:26:53Z

Totally agree @quinton-hoole
This is the first time I am making a change to kubeDNS so I am definitely not super confident :)

I have restricted the changes to be only when the query is for federation service. And have added extensive tests. The existing code has good tests as well.
But yes, this is a critical component.

Will wait until someone familiar with this code has had a chance to take a look.
@bprashanth maybe?

nikhiljindal · 2016-06-21T06:29:29Z

@k8s-bot node e2e test this issue: #27668

nikhiljindal · 2016-06-21T17:23:20Z

cc @girishkalele who I am told is the new KubeDNS owner :)

nikhiljindal · 2016-06-21T20:19:55Z

Added a check to validate that federation names dont have dots '.' as per discussion with @ArtfulCoder and @madhusudancs

ArtfulCoder · 2016-06-21T21:03:15Z

TODOs left in the PR:

make sure its thread safe when the new map is accessed.
removed the reverse lookup map
ensure that federation name does not have dots.
check for wildcard support and impact on wildcards with the new federation segment.
Consider scenarios where the federation name could be an existing namespace/service name.

(I spoke with Nikhil and he knows what the above mean code-wise)..

@mml

Automatic merge from submit-queue federation e2e: Adding retries to fetching secret in controller manager Ref: #27708 Trying to fix the following failure in fed controller manager: ``` error in fetching secret: Get https://10.0.0.1:443/api/v1/namespaces/federation/secrets/federation-apiserver-secret: dial tcp 10.0.0.1:443: i/o timeout ``` I am not sure why that error is happening in the first place. kube-proxy should have been configured before fed controller manager pod comes up. I didnt find anything wrong in kube-proxy logs. The request never reaches fed apiserver. Lets see if adding retries helps. cc @kubernetes/sig-cluster-federation @mml @colhom

ghost · 2016-06-22T20:38:05Z

@nikhiljindal Still needs rebase, and the verification error is below. Hopefully the rebase will sort that out too.

FAIL: changes needed but not made due to --verify
/go/src/k8s.io/kubernetes/docs/ is out of date. Please run hack/update-munge-docs.sh
FAILED   ./hack/../hack/verify-munge-docs.sh    2s

ghost · 2016-06-23T20:23:37Z

pkg/dns/dns.go

@@ -558,6 +637,9 @@ func getSkyMsg(ip string, port int) (*skymsg.Service, string) {
 //   5. Fourth segment is exactly "svc"
 //   6. The remaining segments match kd.domainPath.
 //   7. And federation must be one of the listed federations in the config.
+//   Note: Because of the above conditions, this method will treat wildcard queries such as
+//   *.mysvc.myns.myfederation.svc.domain.path as non-federation queries.
+//   We can add support for wildcard queries later, if needed.


Yes, I would like us to support wildcard queries for federated services, as this will be an easy way for clients to discover all shards of a service globally, with a single DNS query. Pretty cool. Can wait until post-1.3.

@quinton-hoole How are the clients expected to format the wildcard-names? Specifically, where do you expect them to put the *?

Also, what do you expect the KubeDNS response to be?

nikhiljindal · 2016-06-23T20:27:55Z

Tests on this PR ran with the updated KubeDNS image (with my code changes). Great to see that they all passed.

Consider scenarios where the federation name could be an existing namespace/service name.

Yea. Need to think a bit more about the case when federation name is same as namespace name. Will get back on this.

I discussed this with @quinton-hoole and we discussed a few potential solutions, but decided not to fix it in 1.3. Have filed #27969 to keep track.

Also, @quinton-hoole suggested returning a CNAME record for the local service (if it exists) instead of the A records directly in response to a federation DNS query. Making that change now.

ghost · 2016-06-23T20:46:55Z

pkg/dns/dns_test.go

+	assert.Equal(t, a, records[0].Host)
+}
+
+// Verifies that quering KubeDNS for a headless federation service returns the DNS hostname when a local service does not exist and returns the endpoint IP when a local service exists.


As discussed, for both headless and non-headless, we should return a CNAME (to the local service's DNS name if the service exists and has endoints, or to the zone-local DNS name if the local service has no endpoints).

yifan-gu · 2016-06-23T21:48:14Z

@nikhiljindal Could you update the manifest under cluster/gce/coreos/kube-manifest/addons as well? Thank you!

nikhiljindal · 2016-06-23T22:53:21Z

Sure @yifan-gu done.

Update code as per comments. Added a commit to return the DNS hostname of local service rather than returning its endpoints directly. PTAL.

nikhiljindal · 2016-06-23T22:56:13Z

Have also rebased to include other DNS changes, specifically: #27891, #27896 and #27960.

Pushed the updated image to gcr.io/google_containers/kubedns-amd64:1.5 to run tests

girishkalele · 2016-06-24T00:17:06Z

pkg/dns/dns.go

+	isFederationQuery := false
+	federationSegments := []string{}
+	if !exact && kd.isFederationQuery(segments) {
+		glog.Infof("federation service query: Received federation query. Going to try to find local service first")


Move this to a lower log level ? Seems chatty with no specific information.

the information this gives is that this query was interpreted as a federation query.
I would like to keep it for debugging.
Note that we wont see this in the logs unless someone has federation enabled and is sending federation queries.

nikhiljindal · 2016-06-24T00:30:29Z

I see a new GKE e2e here which is failing with some auth problems unrelated to this PR. I dont see any issue filed.
cc @fejta ?

girishkalele · 2016-06-24T00:39:06Z

@nikhiljindal

Both gke e2e seem to be known flakes.
Is this the kubernetes-e2e-gke-slow/5723 run ?

[Fail] [k8s.io] Pod Disks [It] should schedule a pod w/ a readonly PD on two hosts, then remove both. [Slow] 
/go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/test/e2e/pd.go:172

or kubernetes-e2e-gke/9788 ?

[Fail] [k8s.io] Proxy version v1 [It] should proxy through a service and a pod [Conformance] 
/go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/test/e2e/proxy.go:259

girishkalele · 2016-06-24T00:41:42Z

As far as I can tell, there is no non-federation functionality affected.
2 Major comments:

Logs are very noisy, I ran against kubedns:1.5 and see very chatty logs. Please reduce log levels.
Squash all commits.

nikhiljindal · 2016-06-24T00:51:42Z

Squashed into 2 commits (dns changes and YAML file changes).
As discussed the logs are same as in 1.4

girishkalele · 2016-06-24T00:53:20Z

LGTM - lets get it in.

nikhiljindal · 2016-06-24T00:55:03Z

thx @girishkalele

k8s-bot · 2016-06-24T01:47:49Z

GCE e2e build/test passed for commit 7be4293.

k8s-github-robot · 2016-06-24T02:23:41Z

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

ghost · 2016-06-24T02:26:42Z

@nikhiljindal I suggest that you follow @girishkalele 's suggestions re logging verbosity. Rather log as glog.V(4) and set --v=4 on the binary while you're debugging? Will that work for you?

ghost · 2016-06-24T02:28:30Z

@nikhiljindal @fejta PS that new GKE smoke test is failing on #27972 also.

k8s-bot · 2016-06-24T03:30:57Z

GCE e2e build/test passed for commit 7be4293.

k8s-github-robot · 2016-06-24T03:30:58Z

Automatic merge from submit-queue

nikhiljindal · 2016-06-24T06:32:33Z

Pushed and verified that 1.5 image now exists on all platforms:
https://gcr.io/v2/google_containers/kubedns-arm64/tags/list
https://gcr.io/v2/google_containers/kubedns-ppc64le/tags/list
https://gcr.io/v2/google_containers/kubedns-arm/tags/list
https://gcr.io/v2/google_containers/kubedns-amd64/tags/list

googlebot added the cla: yes label Jun 20, 2016

nikhiljindal added the area/cluster-federation label Jun 20, 2016

nikhiljindal added this to the v1.3 milestone Jun 20, 2016

nikhiljindal added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jun 20, 2016

nikhiljindal assigned madhusudancs, ghost and bprashanth Jun 20, 2016

k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 20, 2016

nikhiljindal reviewed Jun 20, 2016
View reviewed changes

nikhiljindal force-pushed the dnsHealthCheck branch from bb0b8f6 to 8abccd3 Compare June 20, 2016 08:51

madhusudancs reviewed Jun 20, 2016
View reviewed changes

ghost reviewed Jun 20, 2016
View reviewed changes

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 21, 2016

nikhiljindal force-pushed the dnsHealthCheck branch from d98b9f6 to 5ed5ccc Compare June 21, 2016 05:02

girishkalele self-assigned this Jun 21, 2016

nikhiljindal mentioned this pull request Jun 22, 2016

federation e2e: Adding retries to fetching secret in controller manager #27823

Merged

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

ghost reviewed Jun 23, 2016
View reviewed changes

ghost added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Jun 23, 2016

nikhiljindal force-pushed the dnsHealthCheck branch from 75d3a5c to 778c49c Compare June 23, 2016 22:50

girishkalele reviewed Jun 24, 2016
View reviewed changes

nikhiljindal force-pushed the dnsHealthCheck branch from 778c49c to 1e87b9b Compare June 24, 2016 00:40

nikhiljindal added 2 commits June 23, 2016 17:50

Updating KubeDNS to try a local service first for federation query

61f43c5

Updating the Makefile and YAML files to use the updated image

7be4293

nikhiljindal force-pushed the dnsHealthCheck branch from 1e87b9b to 7be4293 Compare June 24, 2016 00:50

girishkalele added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 24, 2016

k8s-github-robot merged commit a27fd4b into kubernetes:master Jun 24, 2016

dimmykumont unassigned girishkalele Apr 22, 2019

federation: Updating KubeDNS to try finding a local service first for federation query #27708

federation: Updating KubeDNS to try finding a local service first for federation query #27708

Conversation

nikhiljindal commented Jun 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jun 20, 2016

Choose a reason for hiding this comment

madhusudancs commented Jun 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikhiljindal commented Jun 21, 2016

ghost commented Jun 21, 2016

ghost commented Jun 21, 2016

nikhiljindal commented Jun 21, 2016

nikhiljindal commented Jun 21, 2016

nikhiljindal commented Jun 21, 2016

nikhiljindal commented Jun 21, 2016

ArtfulCoder commented Jun 21, 2016 • edited Loading

ghost commented Jun 22, 2016

ghost Jun 23, 2016 • edited by ghost Loading

Choose a reason for hiding this comment

madhusudancs Jun 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikhiljindal commented Jun 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yifan-gu commented Jun 23, 2016

nikhiljindal commented Jun 23, 2016

nikhiljindal commented Jun 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikhiljindal commented Jun 24, 2016

girishkalele commented Jun 24, 2016

girishkalele commented Jun 24, 2016

nikhiljindal commented Jun 24, 2016

girishkalele commented Jun 24, 2016

nikhiljindal commented Jun 24, 2016

k8s-bot commented Jun 24, 2016

k8s-github-robot commented Jun 24, 2016

ghost commented Jun 24, 2016

ghost commented Jun 24, 2016

k8s-bot commented Jun 24, 2016

k8s-github-robot commented Jun 24, 2016

nikhiljindal commented Jun 24, 2016

ArtfulCoder commented Jun 21, 2016 •

edited

Loading

ghost Jun 23, 2016 •

edited by ghost

Loading

madhusudancs Jun 23, 2016 •

edited

Loading