Enable DNS for services #2224

thockin · 2014-11-07T04:49:59Z

Open issues as of 11/15

Needs e2e testing

Question: Can services exist above "default"? I.e. do we need to search (domain) and (ns).(domain) or just (ns).(domain) ?

Should default.(domain) be in everyone's search list?

TODO: plumb DNS to minion's resolv.conf for use from host

TODO: fix portals for UDP from host (works from a container)

source address is wrong on response from host:
OUTPUT -> -A KUBE-PROXY-LOCAL -d 10.0.0.10/32 -p udp -m comment --comment skydns -m udp --dport 53 -j DNAT --to-destination 10.240.239.33:41504

TODO: Need a specific version tag on the skydns, kube2sky, and etcd containers.

Addresses #1261.

derekwaynecarr · 2014-11-07T17:53:32Z

LGTM

jbeda · 2014-11-07T17:58:14Z

cluster/kube-up.sh

@@ -35,4 +35,16 @@ kube-up

 "${KUBE_ROOT}/cluster/validate-cluster.sh"

+if [[ "${ENABLE_DNS}" == "y" ]]; then


I think this looks workable. As we get more of these we should think about (a) using namespaces and (b) having a directory that is 'services-enabled' similar to how apache/nginx work. We'd rip through those and do a 'kubectl create' for each of those.

jbeda · 2014-11-07T17:59:09Z

We probably want an e2e test that makes sure this continues to work? Do you want to get that in with this PR or should we get it in in a future one?

thockin · 2014-11-07T18:04:05Z

I will have an e2e test when all the plumbing is done. As it is, this is a
useless PR, and I could hold off on commit.

Agree 100% that we need a plugin-like system for turning these one. Just
discussing this side-band. We have at least 3 or 4 such things:

DNS has: "run a pod", "configure node resolv.conf", and "configure docker
daemon" aspects
logging has: "run an agent on every machine" and "run a pod in the cluster"
components
heapster has: "run a pod in the cluster" aspects (and per-node, but we just
defaulted that already)
docker-registry has: "run a pod" and "configure docker daemon" aspects

On Fri, Nov 7, 2014 at 9:59 AM, Joe Beda notifications@github.com wrote:

We probably want an e2e test that makes sure this continues to work? Do
you want to get that in with this PR or should we get it in in a future one?

Reply to this email directly or view it on GitHub
#2224 (comment)
.

jbeda · 2014-11-07T18:13:41Z

I'll hold off util more of it is there.

brendandburns · 2014-11-11T05:19:18Z

I can haz rebase? Then you can haz mergez ;)

thockin · 2014-11-11T05:24:14Z

No point merging this until a few other pieces fall into place. I'll be
working on this off and on the rest of this week, on planes and such. I
know how to do the rest and I have a TODO list.

On Mon, Nov 10, 2014 at 9:19 PM, Brendan Burns notifications@github.com
wrote:

I can haz rebase? Then you can haz mergez ;)

Reply to this email directly or view it on GitHub
#2224 (comment)
.

thockin · 2014-11-16T06:00:32Z

I have re-titled this and added some comments about work in progress. This is far enough along to warrant some review. It may even be good enough to go in soon.

@derekwaynecarr
@lavalamp
@bketelsen

satnam6502 · 2014-11-16T06:16:51Z

contrib/dns/README.md

+helper container called `kube2sky` also runs in the pod and acts a bridge
+between Kubernetes and SkyDNS.  It finds the Kubernetes master through the
+`kubernetes-ro` service, pulls service info from it, and write that to etcd for
+SkyDNS to find.


If it finds.., it pulls..., and writes that

lavalamp · 2014-11-17T18:49:08Z

pkg/api/v1beta2/types.go

@@ -739,6 +739,8 @@ type ContainerManifest struct {
 	Volumes       []Volume      `yaml:"volumes" json:"volumes"`
 	Containers    []Container   `yaml:"containers" json:"containers"`
 	RestartPolicy RestartPolicy `json:"restartPolicy,omitempty" yaml:"restartPolicy,omitempty"`
+	// Optional: Disable cluster-local DNS. Defaults to false.
+	DisableClusterDNS bool `json:"disableClusterDNS" yaml:"disableClusterDNS"`


Can we not name options in the negative?

What is the use case for this option, anyway?

Should the option be about publishing in general, instead of about DNS specifically?

Oh. I see from the kubelet code that this option is about giving the pod access to cluster DNS, not about not publishing the pod. Maybe the name can be improved. Doubly unsure about the use case now, though.

I would much rather name this "EnableClusterDNS" and default to true, but the way we do defaulting, I can not tell if the user set it to False or expected the default.

This exists primarily so that SkyDNS does not try to use itself for DNS lookups. Maybe we don't NEED it (would have to try), but it feels scary to have this self-loop.

Then make it an enum, DNS, with the values "clusterProvided" and "defaultExternal". That's better than a bool in any case, gives us more flexibility later.

Use case sounds... surprisingly valid, OK.

lavalamp · 2014-11-18T18:22:54Z

LGTM with api change from bool->enum.

thockin · 2014-11-20T03:42:44Z

New push is up. I have not properly tested it by spinning up a whole cluster - will do that ASAP.

derekwaynecarr · 2014-11-20T21:00:49Z

@thockin - I will look to spin up a vagrant cluster and report back.

derekwaynecarr · 2014-11-20T21:39:45Z

@thockin - can you rebase this? right now, i cant bring up the cluster in this branch because of bug fixes that have since been fixed.

lavalamp · 2014-12-24T01:27:22Z

LGTM will merge on green

thockin · 2014-12-24T01:28:19Z

I think all comments are addressed. Re-running e2e.

lavalamp · 2014-12-24T18:00:03Z

Did e2e pass?

thockin · 2014-12-24T18:06:12Z

e2e passed several times, but I saw it flake once. Will retry and see if I can figure it out.

This adds --cluster_dns and --cluster_domain flags to kubelet. If non-empty, kubelet will set docker --dns and --dns-search flags based on these. It uses the cluster DNS and appends the hosts's DNS servers. Likewise for DNS search domains. This also adds API support to bypass cluster DNS entirely, needed to bootstrap DNS.

Rather than have to keep SkyDNS up to date with Kubernetes, use a buddy-container "kube2sky" to watch kubernetes and sync Service records into etcd for SkyDNS ot find. This also adds namespace support.

After this DNS is resolvable from the host, if the DNS server is targetted explicitly. This does NOT add the cluster DNS to the host's resolv.conf. That is a larger problem, with distro-specific tie-ins and circular deps.

thockin · 2014-12-29T18:11:19Z

I re-ran e2e dozens of times (including setup and teardown) and could not get it to flake. I think we should commit this.

@jbeda (oncall)

Enable DNS for services

thockin · 2014-12-29T18:47:51Z

w00t!!

On Mon, Dec 29, 2014 at 10:28 AM, Joe Beda notifications@github.com wrote:

Merged #2224 #2224
.

Reply to this email directly or view it on GitHub
#2224 (comment)
.

lavalamp · 2014-12-29T18:48:34Z

Yay!

bgrant0607 · 2014-12-29T19:20:44Z

Awesome!

brendandburns · 2014-12-29T23:24:11Z

Cool!
On Dec 29, 2014 11:21 AM, "bgrant0607" notifications@github.com wrote:

Awesome!

—
Reply to this email directly or view it on GitHub
#2224 (comment)
.

SIG Cloud Provider KEP: Reporting Conformance Test Results to Testgrid

k8s-ci-robot · 2024-12-11T03:02:06Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Elno5, thockin
Once this PR has been reviewed and has the lgtm label, please assign liggitt for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

~~OWNERS~~ [thockin]
cluster/OWNERS
~~cmd/OWNERS~~ [thockin]
~~hack/OWNERS~~ [thockin]
~~pkg/OWNERS~~ [thockin]
~~pkg/api/OWNERS~~ [thockin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jbeda reviewed Nov 7, 2014
View reviewed changes

thockin force-pushed the dns branch from 3a8abf0 to 662df5d Compare November 7, 2014 18:04

bgrant0607 assigned jbeda Nov 8, 2014

thockin force-pushed the dns branch 2 times, most recently from a3d1787 to e39eca0 Compare November 16, 2014 05:53

thockin changed the title ~~Make DNS an option for cluster turnup~~ Enable DNS for services Nov 16, 2014

satnam6502 reviewed Nov 16, 2014
View reviewed changes

thockin force-pushed the dns branch 2 times, most recently from 85039c4 to 332f976 Compare November 17, 2014 05:25

lavalamp reviewed Nov 17, 2014
View reviewed changes

thockin force-pushed the dns branch from 332f976 to 060bebc Compare November 18, 2014 06:25

thockin force-pushed the dns branch 2 times, most recently from dd723b0 to aecf766 Compare November 20, 2014 03:42

thockin force-pushed the dns branch from be47f75 to 7494ab5 Compare December 24, 2014 01:25

lavalamp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 24, 2014

thockin added 7 commits December 29, 2014 09:18

add contrib/dns/MAINTAINERS

2a110b0

Use a helper bridge between k8s and SkyDNS

1291c71

Rather than have to keep SkyDNS up to date with Kubernetes, use a buddy-container "kube2sky" to watch kubernetes and sync Service records into etcd for SkyDNS ot find. This also adds namespace support.

Make DNS an option for cluster turnup

59164ca

Split portals into host and container

e045c6c

After this DNS is resolvable from the host, if the DNS server is targetted explicitly. This does NOT add the cluster DNS to the host's resolv.conf. That is a larger problem, with distro-specific tie-ins and circular deps.

Use a kubernetes-hosted minimal container for skydns

b0268f3

Add an e2e test for DNS from pods.

32a5947

thockin force-pushed the dns branch from 7494ab5 to 32a5947 Compare December 29, 2014 17:18

jbeda added a commit that referenced this pull request Dec 29, 2014

Merge pull request #2224 from thockin/dns

a2e58d4

Enable DNS for services

jbeda merged commit a2e58d4 into kubernetes:master Dec 29, 2014

thockin deleted the dns branch January 12, 2015 14:21

zmerlynn mentioned this pull request Jan 16, 2015

Proposal for Deferred Creation of Addon Objects #3579

Closed

google-admin unassigned jbeda May 6, 2015

seans3 pushed a commit to seans3/kubernetes that referenced this pull request Apr 10, 2019

Merge pull request kubernetes#2224 from andrewsykim/kep-provider-e2e

74acb1a

SIG Cloud Provider KEP: Reporting Conformance Test Results to Testgrid

haosdent mentioned this pull request Sep 8, 2022

kubelet doesn't merge multiple 'options' lines from node /etc/resolv.conf #112323

Closed

Elno5 approved these changes Dec 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable DNS for services #2224

Enable DNS for services #2224

thockin commented Nov 7, 2014

derekwaynecarr commented Nov 7, 2014

jbeda Nov 7, 2014

jbeda commented Nov 7, 2014

thockin commented Nov 7, 2014

jbeda commented Nov 7, 2014

brendandburns commented Nov 11, 2014

thockin commented Nov 11, 2014

thockin commented Nov 16, 2014

satnam6502 Nov 16, 2014

thockin Nov 17, 2014

lavalamp Nov 17, 2014

lavalamp Nov 17, 2014

lavalamp Nov 17, 2014

thockin Nov 18, 2014

lavalamp Nov 18, 2014

lavalamp commented Nov 18, 2014

thockin commented Nov 20, 2014

derekwaynecarr commented Nov 20, 2014

derekwaynecarr commented Nov 20, 2014

lavalamp commented Dec 24, 2014

thockin commented Dec 24, 2014

lavalamp commented Dec 24, 2014

thockin commented Dec 24, 2014

thockin commented Dec 29, 2014

thockin commented Dec 29, 2014

lavalamp commented Dec 29, 2014

bgrant0607 commented Dec 29, 2014

brendandburns commented Dec 29, 2014

k8s-ci-robot commented Dec 11, 2024

		@@ -35,4 +35,16 @@ kube-up

		"${KUBE_ROOT}/cluster/validate-cluster.sh"

		if [[ "${ENABLE_DNS}" == "y" ]]; then

Enable DNS for services #2224

Enable DNS for services #2224

Conversation

thockin commented Nov 7, 2014

derekwaynecarr commented Nov 7, 2014

Choose a reason for hiding this comment

jbeda commented Nov 7, 2014

thockin commented Nov 7, 2014

jbeda commented Nov 7, 2014

brendandburns commented Nov 11, 2014

thockin commented Nov 11, 2014

thockin commented Nov 16, 2014

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

lavalamp commented Nov 18, 2014

thockin commented Nov 20, 2014

derekwaynecarr commented Nov 20, 2014

derekwaynecarr commented Nov 20, 2014

lavalamp commented Dec 24, 2014

thockin commented Dec 24, 2014

lavalamp commented Dec 24, 2014

thockin commented Dec 24, 2014

thockin commented Dec 29, 2014

thockin commented Dec 29, 2014

lavalamp commented Dec 29, 2014

bgrant0607 commented Dec 29, 2014

brendandburns commented Dec 29, 2014

k8s-ci-robot commented Dec 11, 2024