Fix external name not solving by reloading resolv.conf. #225

grayluck · 2018-04-14T00:10:55Z

Now skydns will be configured with resolv.conf when nameserver is empty.

Fixes#224 (#224)

MrHohn · 2018-04-14T00:38:54Z

/ok-to-test

MrHohn · 2018-04-14T00:46:08Z

pkg/dns/dns.go

-		kd.SkyDNSConfig.Nameservers = nameServers
+		if len(nameServers) == 0 {
+			kd.SkyDNSConfig.Nameservers = kd.loadDefaultNameserver()
+		}


Seems like kd.SkyDNSConfig.Nameservers = nameServers is still needed?

Now skydns will be configured with resolv.conf when nameserver is empty.

MrHohn · 2018-04-14T01:22:28Z

pkg/dns/dns.go

@@ -139,6 +140,19 @@ func NewKubeDNS(client clientset.Interface, clusterDomain string, timeout time.D
 	return kd
 }

+func (kd *KubeDNS) loadDefaultNameserver() []string {
+	nameservers := []string{}
+	if c, err := dns.ClientConfigFromFile("/etc/resolv.conf"); !os.IsNotExist(err) {


How is not exist different from parse error in this case? I think we should return empty array in both cases (feel like there are some redundant logic)?

Can we log if ClientConfigFromFile returns error?

Logged using verbose level 1.

MrHohn · 2018-04-16T18:20:43Z

pkg/dns/dns.go

+	}
+	return nameservers
+}
+
 func (kd *KubeDNS) updateConfig(nextConfig *config.Config) {


Is it possible to add a unit test for updating nameservers?

Unit test for updateConfig added.

MrHohn · 2018-04-16T20:13:55Z

Keep @bowei in the loop.

grayluck · 2018-04-16T20:21:47Z

Done. Unit test added. PTAL thx

MrHohn · 2018-04-16T20:22:55Z

pkg/dns/dns.go

+			nameservers = append(nameservers, net.JoinHostPort(s, c.Port))
+		}
+	} else {
+		glog.V(1).Infof("Load nameserver from resolv.conf failed: %v", err)


We should use glog.Errorf() here. Can we move the error case up and early return?

if c, err := dns.ClientConfigFromFile(defaultResolvFile); err != nil {
// log error
return
}

// do the rest

MrHohn · 2018-04-16T20:24:38Z

pkg/dns/dns.go

@@ -148,12 +164,20 @@ func (kd *KubeDNS) updateConfig(nextConfig *config.Config) {
 		for _, nameServer := range nextConfig.UpstreamNameservers {
 			if ip, port, err := util.ValidateNameserverIpAndPort(nameServer); err != nil {
 				glog.V(1).Infof("Invalid nameserver %q: %v", nameServer, err)


glog.Errorf()?

Now skyDNS nameservers will fall back to resolv.conf whenever it's empty.

MrHohn · 2018-04-16T20:29:00Z

pkg/dns/dns.go

+	if c, err := dns.ClientConfigFromFile(defaultResolvFile); err != nil {
+		glog.Errorf("Load nameserver from resolv.conf failed: %v", err)
+		return []string{}
+	} else {


The else block seems unnecessary.

I see, we are using thec.

No worries, let keep this as is.

MrHohn · 2018-04-16T20:39:42Z

pkg/dns/dns.go

+				glog.Errorf("Invalid nameserver %q: %v", nameServer, err)
+				if len(kd.SkyDNSConfig.Nameservers) == 0 {
+					// Fall back to resolv.conf on initialization failed.
+					kd.SkyDNSConfig.Nameservers = kd.loadDefaultNameserver()


I think we should fall through here --- append whatever valid nameservers are passed in, and reuse the 0 length detection logic below:

if ip, port, err := util.ValidateNameserverIpAndPort(nameServer); err != nil { glog.Errorf("Invalid nameserver %q: %v", nameServer, err) } else { nameServers = append(nameServers, net.JoinHostPort(ip, port)) } ...

The idea is, we don't want to pollute kd.config with invalid input. (But the truth is, kd.config.UpstreamNameservers are never used so it's okay to be polluted)

Sounds good, though having duplicated logic here seems bad. What about:

if kd.SkyDNSConfig != nil { var nameServers []string for _, nameServer := range nextConfig.UpstreamNameservers { if ip, port, err := util.ValidateNameserverIpAndPort(nameServer); err != nil { glog.Errorf("Invalid nameserver %q: %v", nameServer, err) // Don't populate nameServers if invalid input is detected. nameServers = []string{} break } else { nameServers = append(nameServers, net.JoinHostPort(ip, port)) } } if len(nameServers) == 0 { ...

Sorry what I wrote is wrong. I got what you were saying.

But we can still do the partially update and leave kd.config untouched. Do you want me to do that?
I am kind of against this since kd.SkyDNSConfig is partially updated and kd.config is not which might be confusing in some extreme cases.

I am kind of against this since kd.SkyDNSConfig is partially updated and kd.config is not which might be confusing in some extreme cases.

Agreed, that sounds bad. Let's keep this.

MrHohn

/lgtm

MrHohn · 2018-04-16T21:16:24Z

pkg/dns/dns.go

-				glog.V(1).Infof("Invalid nameserver %q: %v", nameServer, err)
+				glog.Errorf("Invalid nameserver %q: %v", nameServer, err)
+				if len(kd.SkyDNSConfig.Nameservers) == 0 {
+					// Fall back to resolv.conf on initialization failed.


nit: initialization failed -> initialization failure

MrHohn · 2018-04-16T21:23:51Z

pkg/dns/dns.go

+				glog.Errorf("Invalid nameserver %q: %v", nameServer, err)
+				if len(kd.SkyDNSConfig.Nameservers) == 0 {
+					// Fall back to resolv.conf on initialization failed.
+					kd.SkyDNSConfig.Nameservers = kd.loadDefaultNameserver()


I am kind of against this since kd.SkyDNSConfig is partially updated and kd.config is not which might be confusing in some extreme cases.

Agreed, that sounds bad. Let's keep this.

grayluck · 2018-04-16T21:28:14Z

Thanks for review!

MrHohn

/lgtm

MrHohn · 2018-04-16T21:36:52Z

Will merge this once travis passes.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 14, 2018

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 14, 2018

MrHohn reviewed Apr 14, 2018

View reviewed changes

Fix external name not solving by reloading resolv.conf.

faf4849

Now skydns will be configured with resolv.conf when nameserver is empty.

grayluck force-pushed the externalname branch from cb6059f to faf4849 Compare April 14, 2018 00:50

MrHohn reviewed Apr 14, 2018

View reviewed changes

MrHohn reviewed Apr 16, 2018

View reviewed changes

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 16, 2018

grayluck force-pushed the externalname branch 2 times, most recently from 4834ebb to bf3f6e8 Compare April 16, 2018 20:20

MrHohn reviewed Apr 16, 2018

View reviewed changes

Add unittest for kubedns update config.

2660e2d

Now skyDNS nameservers will fall back to resolv.conf whenever it's empty.

grayluck force-pushed the externalname branch from bf3f6e8 to 2660e2d Compare April 16, 2018 20:27

MrHohn reviewed Apr 16, 2018

View reviewed changes

k8s-ci-robot assigned MrHohn Apr 16, 2018

MrHohn approved these changes Apr 16, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 16, 2018

MrHohn approved these changes Apr 16, 2018

View reviewed changes

nit: initialization failed -> initialization failure

969920f

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 16, 2018

MrHohn approved these changes Apr 16, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 16, 2018

MrHohn merged commit 80fdd88 into kubernetes:master Apr 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix external name not solving by reloading resolv.conf. #225

Fix external name not solving by reloading resolv.conf. #225

grayluck commented Apr 14, 2018

MrHohn commented Apr 14, 2018

MrHohn Apr 14, 2018

grayluck Apr 14, 2018

MrHohn Apr 14, 2018

grayluck Apr 16, 2018

MrHohn Apr 16, 2018

grayluck Apr 16, 2018

MrHohn commented Apr 16, 2018

grayluck commented Apr 16, 2018

MrHohn Apr 16, 2018

grayluck Apr 16, 2018

MrHohn Apr 16, 2018

grayluck Apr 16, 2018

MrHohn Apr 16, 2018

MrHohn Apr 16, 2018

MrHohn Apr 16, 2018

MrHohn Apr 16, 2018

grayluck Apr 16, 2018

MrHohn Apr 16, 2018

MrHohn Apr 16, 2018

grayluck Apr 16, 2018

MrHohn Apr 16, 2018

MrHohn left a comment

MrHohn Apr 16, 2018

grayluck Apr 16, 2018

MrHohn Apr 16, 2018

grayluck commented Apr 16, 2018

MrHohn left a comment

MrHohn commented Apr 16, 2018

Fix external name not solving by reloading resolv.conf. #225

Fix external name not solving by reloading resolv.conf. #225

Conversation

grayluck commented Apr 14, 2018

MrHohn commented Apr 14, 2018

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

MrHohn commented Apr 16, 2018

grayluck commented Apr 16, 2018

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

MrHohn left a 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

grayluck commented Apr 16, 2018

MrHohn left a comment

Choose a reason for hiding this comment

MrHohn commented Apr 16, 2018