Add a dnsBindAddress configuration to the node #12805

smarterclayton · 2017-02-03T19:59:05Z

Allows the installer to start node DNS without having to set a config
flag. BindAddress is consistent with others of this type.

@sdodson this would be how you could enable DNS and bind it (you could set 0.0.0.0:54, and then configure dnsmasq)

[test]

smarterclayton · 2017-02-06T04:42:56Z

@liggitt this needs review now. I forgot that node config isn't generated anything like master config so had to plumb the new flag in.

This will enable @sdodson to start the node DNS even when DNSmasq is on so that we can get correct PTR record support (right now it's broken because the master isn't recursing and dnsmasq doesn't try other endpoints on failure).

smarterclayton · 2017-02-06T04:44:40Z

Required to fix openshift/openshift-ansible#3017

sdodson · 2017-02-06T15:18:51Z

My only concern is how will the node's dns server know which upstream servers to use when doing recursion if /etc/resolv.conf points to the node's dnsmasq and dnsmasq is sending all in-addr.arpa back to the node's dns server.

smarterclayton · 2017-02-07T01:24:14Z

I may have to accept name servers or an alternate config file for skydns to bootstrap. Will look at that tomorrow morning.

smarterclayton · 2017-02-08T00:23:18Z

Updated to add a dnsNameservers config variable that overrides the default behavior (reads from /etc/resolv.conf). Required to be set if the local proxy is in the resolve.conf.

liggitt · 2017-02-08T16:33:58Z

pkg/cmd/server/api/types.go

@@ -112,12 +112,20 @@ type NodeConfig struct {
 	// MasterClientConnectionOverrides provides overrides to the client connection used to connect to the master.
 	MasterClientConnectionOverrides *ClientConnectionOverrides

+	// DNSBindAddress is the ip:port to serve on. If this is not set, the DNS server will not be started.
+	DNSBindAddress string


group DNSNameservers with this param and lay out the two setup scenarios really clearly

liggitt · 2017-02-08T16:36:12Z

pkg/cmd/server/start/node_args.go

@@ -37,12 +37,12 @@ func NewNodeComponentFlag() *utilflags.ComponentFlag {
 	return utilflags.NewComponentFlag(
 		map[string][]string{ComponentGroupNetwork: {ComponentProxy, ComponentPlugins}},
 		ComponentKubelet, ComponentProxy, ComponentPlugins, ComponentDNS,
-	).DefaultDisable(ComponentDNS)
+	).DefaultEnable(ComponentDNS)


aren't all enabled by default?

liggitt · 2017-02-08T16:36:20Z

pkg/cmd/server/start/node_args.go

 }

 // NewNodeComponentFlag returns a flag capable of handling enabled components for the network
 func NewNetworkComponentFlag() *utilflags.ComponentFlag {
-	return utilflags.NewComponentFlag(nil, ComponentProxy, ComponentPlugins, ComponentDNS).DefaultDisable(ComponentDNS)
+	return utilflags.NewComponentFlag(nil, ComponentProxy, ComponentPlugins, ComponentDNS).DefaultEnable(ComponentDNS)


same question

liggitt · 2017-02-08T16:37:09Z

pkg/cmd/server/start/start_allinone.go

@@ -248,6 +248,7 @@ func (o *AllInOneOptions) Complete() error {
 		if dnsAddr.Provided {
 			if dnsAddr.Port == 53 {
 				// the user has set the DNS port to 53, which is the effective default (node on 53, master on 8053)
+				o.NodeArgs.DNSBindAddr = dnsAddr.URL.Host


doesn't this need a port?

Host in a URL is host:port or host.

smarterclayton · 2017-02-08T22:14:52Z

Flaked on kubernetes/kubernetes#40953

liggitt · 2017-02-09T20:14:33Z

pkg/cmd/server/admin/create_nodeconfig.go

@@ -86,6 +87,7 @@ func NewCommandNodeConfig(commandName string, fullName string, out io.Writer) *c
 	flags.StringVar(&options.ImageTemplate.Format, "images", options.ImageTemplate.Format, "When fetching the network container image, use this format. The latest release will be used by default.")
 	flags.BoolVar(&options.ImageTemplate.Latest, "latest-images", options.ImageTemplate.Latest, "If true, attempt to use the latest images for the cluster instead of the latest release.")
 	flags.BoolVar(&options.AllowDisabledDocker, "allow-disabled-docker", options.AllowDisabledDocker, "Allow the node to start without docker being available.")
+	flags.StringVar(&options.DNSBindAddress, "dns-bind-address", options.DNSBindAddress, "An address to bind DNS to.")


do we need to surface it as a flag, or only in config?

We could avoid adding it as a flag, it just means that openshift start node will be unable to start DNS.

do we ever run openshift start node without a config?

liggitt · 2017-02-09T20:15:30Z

pkg/cmd/server/api/types.go

 	DNSDomain string

-	// DNSIP holds the IP
+	// DNSIP is the IP address that the DNS server will listen on and which will be reported into pods. Defaults


"DNSIP is the IP address that the DNS server will listen on" isn't accurate, right?

hmm... looks like we try to listen on port 53 of this IP unless DNSBindAddress is specified, in which case it stomps? that is not clear

We never listen on this IP. I'll update the comment.

liggitt · 2017-02-09T20:16:55Z

pkg/cmd/server/api/types.go

 	DNSIP string

+	// DNSBindAddress is the ip:port to serve DNS on. If this is not set, the DNS server will not be started.
+	// Because most DNS resolvers will only listen on port 53, if you select an alternative port you will need
+	// a DNS proxy like dnsmasq to answer queries for containers. A common configuration is dnsmasq configured


"if you select an alternative port you will need a DNS proxy like dnsmasq to answer queries for containers, and to point DNSIP at that DNS proxy", right?

Because we say that in the previous field I don't know that it is necessary here (reordered to make that clear).

liggitt · 2017-02-09T20:17:47Z

pkg/cmd/server/api/types.go

+	// queries to the host environments nameservers.
+	DNSBindAddress string
+
+	// DNSNameservers is a list of ip:port values of recursive nameservers to forward queries to.


only used by the DNS server started if DNSBindAddress is set

liggitt · 2017-02-09T20:19:10Z

pkg/cmd/server/api/types.go

+	DNSBindAddress string
+
+	// DNSNameservers is a list of ip:port values of recursive nameservers to forward queries to.
+	// If this value is empty, the DNS server will default to the nameservers listed in /etc/resolv.conf.


"the DNS server will delegate to nameservers listed in /etc/resolv.conf for queries it cannot satisfy itself"?

liggitt · 2017-02-09T20:19:32Z

pkg/cmd/server/api/validation/node.go

 	if len(config.DNSIP) > 0 {
 		validationResults.AddErrors(ValidateSpecifiedIP(config.DNSIP, fldPath.Child("dnsIP"))...)
 	}
+	for i, nameserver := range config.DNSNameservers {


error to specify this and not specify DNSBindAddress?

Do we do that often? It means if you want to temporarily stop listening you have to change a bunch of fields, rather than just one.

I guess not... just don't want people (more) confused. not a big deal either way

liggitt · 2017-02-09T20:19:52Z

pkg/cmd/server/api/validation/node.go

 	if len(config.DNSIP) > 0 {
 		validationResults.AddErrors(ValidateSpecifiedIP(config.DNSIP, fldPath.Child("dnsIP"))...)
 	}
+	for i, nameserver := range config.DNSNameservers {
+		validationResults.AddErrors(ValidateSpecifiedIPPort(nameserver, fldPath.Child("dnsNameserver").Index(i))...)


"dnsNameservers"

liggitt · 2017-02-09T20:24:45Z

pkg/cmd/server/kubernetes/node_config.go

@@ -317,8 +317,14 @@ func BuildKubernetesNodeConfig(options configapi.NodeConfig, enableProxy, enable
 		if len(options.DNSIP) > 0 {


if we don't start the dns server unless DNSBindAddress is specified, what's the point of setting it to DNSIP initially?

are there old configs that just set DNSIP that will break (not start node DNS) if they upgrade and don't update their config to also set DNSBindAddress?

No one has ever started node DNS by default, so only someone using the feature experimentally (outside of our control) would be in that setup.

Before, you had to pass --enable=dns to get DNS. Now you don't. I think we can remove this check.

smarterclayton · 2017-02-09T21:02:34Z

Another commit up.

liggitt · 2017-02-09T21:06:31Z

LGTM

sdodson · 2017-02-09T22:48:21Z

So it's on by default now and we need to set the bindaddress to avoid port conflicts? Recursion in the node's dns server was enabled to start with or do I need to do something to enable that as well?

Allows the installer to start node DNS without having to set a config flag. BindAddress is consistent with others of this type.

smarterclayton · 2017-02-09T23:32:55Z

No, it's not on by default, you have to set dns-bind-address.

smarterclayton · 2017-02-09T23:33:06Z

Node is recursion by default and you can't disable it.

smarterclayton · 2017-02-09T23:33:12Z

[merge]

openshift-bot · 2017-02-09T23:37:33Z

Evaluated for origin test up to f97f728

sdodson · 2017-02-10T00:09:46Z

Sounds good, thanks.

…

On Feb 9, 2017 6:37 PM, "OpenShift Bot" ***@***.***> wrote: continuous-integration/openshift-jenkins/test Running ( https://ci.openshift.redhat.com/jenkins/job/test_pull_ requests_origin_future/62/) (Base Commit: 898266a <898266a> ) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12805 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAC8IX6uhu4B7avjgpjtHfsuSCJ6tUZyks5ra6NOgaJpZM4L2vlW> .

openshift-bot · 2017-02-10T01:29:25Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/62/) (Base Commit: 898266a)

smarterclayton · 2017-02-10T17:47:36Z

Known flakes [merge]

openshift-bot · 2017-02-10T17:53:24Z

Evaluated for origin merge up to f97f728

smarterclayton · 2017-02-10T20:31:11Z

[merge]

openshift-bot · 2017-02-11T10:53:25Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/132/) (Base Commit: 4e8a130) (Image: devenv-rhel7_5898)

smarterclayton force-pushed the dns_args branch 4 times, most recently from 162c5c0 to b3f3316 Compare February 6, 2017 02:50

smarterclayton force-pushed the dns_args branch from b3f3316 to 47a0d88 Compare February 8, 2017 00:22

smarterclayton force-pushed the dns_args branch from 47a0d88 to 36bf0f1 Compare February 8, 2017 02:42

liggitt reviewed Feb 8, 2017

View reviewed changes

smarterclayton force-pushed the dns_args branch 3 times, most recently from d0548f2 to 790a538 Compare February 8, 2017 20:28

liggitt reviewed Feb 9, 2017

View reviewed changes

Swagger generation uses a hardcoded directory

a9d1e79

smarterclayton force-pushed the dns_args branch from a00ba63 to f97f728 Compare February 9, 2017 23:29

Add a dnsBindAddress configuration to the node

f97f728

Allows the installer to start node DNS without having to set a config flag. BindAddress is consistent with others of this type.

openshift-bot merged commit 8ac875e into openshift:master Feb 11, 2017

		@@ -317,8 +317,14 @@ func BuildKubernetesNodeConfig(options configapi.NodeConfig, enableProxy, enable
		if len(options.DNSIP) > 0 {

Add a dnsBindAddress configuration to the node #12805

Add a dnsBindAddress configuration to the node #12805

Conversation

smarterclayton commented Feb 3, 2017

smarterclayton commented Feb 6, 2017

smarterclayton commented Feb 6, 2017

sdodson commented Feb 6, 2017

smarterclayton commented Feb 7, 2017

smarterclayton commented Feb 8, 2017

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

smarterclayton commented Feb 8, 2017

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

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

smarterclayton commented Feb 9, 2017

liggitt commented Feb 9, 2017

sdodson commented Feb 9, 2017

smarterclayton commented Feb 9, 2017

smarterclayton commented Feb 9, 2017

smarterclayton commented Feb 9, 2017

openshift-bot commented Feb 9, 2017

sdodson commented Feb 10, 2017 via email

openshift-bot commented Feb 10, 2017

smarterclayton commented Feb 10, 2017

openshift-bot commented Feb 10, 2017

smarterclayton commented Feb 10, 2017

openshift-bot commented Feb 11, 2017 • edited Loading

openshift-bot commented Feb 11, 2017 •

edited

Loading