Forward PTR record lookup from kubedns to upstream server. #220

grayluck · 2018-03-23T00:19:13Z

Fixes #198

Approach: Let kubedns sync config to skydns nameservers, allowing skydns to do the request forwarding.

grayluck · 2018-03-28T21:52:40Z

/assign @MrHohn

MrHohn · 2018-03-29T22:45:10Z

/ok-to-test

MrHohn · 2018-03-30T00:38:18Z

@grayluck Seems like travis failed on this:

--- FAIL: TestConfigSync (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x68 pc=0x11963f6]
goroutine 38 [running]:
testing.tRunner.func1(0xc4204651d0)
	/usr/local/go/src/testing/testing.go:711 +0x2d2
panic(0x12e4380, 0x1cb8c40)
	/usr/local/go/src/runtime/panic.go:491 +0x283
k8s.io/dns/pkg/dns.(*KubeDNS).updateConfig(0xc42027f200, 0xc4201a5950)
	/go/src/k8s.io/dns/pkg/dns/dns.go:160 +0x346
k8s.io/dns/pkg/dns.(*KubeDNS).startConfigMapSync(0xc42027f200)
	/go/src/k8s.io/dns/pkg/dns/dns.go:214 +0x20a
k8s.io/dns/pkg/dns.TestConfigSync(0xc4204651d0)
	/go/src/k8s.io/dns/pkg/dns/dns_test.go:683 +0x164
testing.tRunner(0xc4204651d0, 0x14b7ff8)
	/usr/local/go/src/testing/testing.go:746 +0xd0
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:789 +0x2de
FAIL	k8s.io/dns/pkg/dns	0.125s

grayluck · 2018-03-31T00:15:12Z

/assign @bowei

bowei · 2018-03-31T00:15:35Z

Makefile

+#     dnsmasq-nanny \
+#     kube-dns \
+#     sidecar
+CONTAINER_BINARIES := kube-dns


bowei · 2018-03-31T00:15:48Z

Makefile


 # List of images to build (contained in images/)
 IMAGES := dnsmasq
 # Registry to push to.
-REGISTRY ?= staging-k8s.gcr.io
+# REGISTRY ?= gcr.io
+REGISTRY ?= gcr.io/yankaiz-kube-test


bowei · 2018-03-31T00:18:12Z

cmd/kube-dns/app/server.go

@@ -163,40 +162,6 @@ func setupSignalHandlers() {
 	}()
 }

-func validateHostAndPort(hostAndPort string) error {


I think this should just go into config/ package instead of creating a new package util

It's now in config package.
I am making it public for now in testing purpose. Will change it to private method very soon on Monday.

Two validation logic merged. Now skydns's nameserver is synced/guarded by kubedns.

bowei · 2018-03-31T00:19:30Z

pkg/dns/dns.go

@@ -57,6 +58,9 @@ type KubeDNS struct {
 	// to get Endpoints and Service objects.
 	kubeClient clientset.Interface

+	// skydns points to the skydns server instance for configuration syncing.
+	skydnsConfig *server.Config


Let's just make this a Public field and call set directly

SkyDNSConfig *server.Config

bowei · 2018-03-31T00:19:39Z

pkg/dns/dns.go

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

+// BondSkydnsConfig bonds the configuration of skydns for config sync. The
+// function encapsulates skydnsConfig inside KubeDNS struct.
+func (kd *KubeDNS) BondSkydnsConfig(config *server.Config) {


delete this...

bowei · 2018-03-31T00:22:43Z

pkg/dns/util/util.go

@@ -87,3 +89,37 @@ func HashServiceRecord(msg *msg.Service) string {
 	h.Write([]byte(s))
 	return fmt.Sprintf("%x", h.Sum32())
 }
+
+func ValidateHostAndPort(hostAndPort string) error {


Move unit tests as well. Are there any unit tests?

bowei · 2018-03-31T00:23:17Z

pkg/e2e/options.go

-	hyperkubeImage = "k8s.gcr.io/hyperkube:v1.5.1"
+	etcdImage       = "quay.io/coreos/etcd:v3.0.14"
+	hyperkubeImage  = "k8s.gcr.io/hyperkube:v1.5.1"
+	dnsmasqPtrImage = "gcr.io/kubernetes-e2e-test-images/dnsmasq-ptr:1.0"


what is this?

Now using k8s.gcr.io/k8s-dns-dnsmasq-amd64:1.14.5 instead.

bowei · 2018-03-31T00:23:47Z

test/e2e/kubedns/kubedns.go

@@ -55,4 +59,69 @@ var _ = Describe("kube-dns", func() {
 			kubeDNS.Stop()
 		})
 	})
+
+	It("should forward missed ptr lookup to upstream server", func() {


should forward PTR queries to the upstream server

bowei · 2018-03-31T00:24:03Z

test/e2e/kubedns/kubedns.go

+
+		kubeDNS.Start("kube-dns-ptrfwd", "-v=4", "--config-dir="+configDir)
+
+		By("Get answer without upstream server")


bowei · 2018-03-31T00:25:33Z

test/e2e/kubedns/kubedns.go

+
+func doPtrQuery(kubeDNS *e2edns.KubeDNS) error {
+	time.Sleep(1 * time.Second)
+	names, err := kubeDNS.Query("4.3.2.1.in-addr.arpa.", dns.TypePTR)


You can use an IP from the testbed range to guarantee no one will reply. 1.2.3.4 may be someone's address.

192.0.2.0/24

https://en.wikipedia.org/wiki/Reserved_IP_addresses

IP changed. Now using numb upstream server when test starts and then switch to a upstream server with the desired PTR record.

bowei · 2018-04-03T06:16:20Z

pkg/dns/config/config.go

+			return err
+		}
+		if ip := net.ParseIP(host); ip == nil {
+			return fmt.Errorf("bad IP address: %s", host)


bowei · 2018-04-03T06:16:26Z

pkg/dns/config/config.go

+			return fmt.Errorf("bad IP address: %s", host)
+		}
+		if p, _ := strconv.Atoi(port); p < 1 || p > 65535 {
+			return fmt.Errorf("bad port number %s", port)


bowei · 2018-04-03T06:22:12Z

pkg/dns/dns.go

+
+	if kd.SkyDNSConfig != nil {
+		var nameServers []string
+		for _, nameServer := range kd.config.UpstreamNameservers {


is there a reason why this isn't match the parsing routine in config.go?

config.go only validates the address of nameservers. Here a default port is appended to the address if the address doesn't have a port.

bowei · 2018-04-04T01:13:01Z

test/e2e/kubedns/kubedns.go

+
+		configDir := workDir + "/kube-dns-config"
+		if err := os.MkdirAll(configDir, 0744); err != nil {
+			panic(err)


Expect("foo").To(Equal("foo"))

bowei · 2018-04-05T07:01:59Z

pkg/dns/util/util.go

+		return "", "", err
+	}
+	if ip := net.ParseIP(host); ip == nil {
+		return "", "", fmt.Errorf("bad IP address: %s", host)


For invalid strings, you should use %q so the string is quoted.

bowei · 2018-04-05T07:02:08Z

pkg/dns/util/util.go

+		return "", "", fmt.Errorf("bad IP address: %s", host)
+	}
+	if p, _ := strconv.Atoi(port); p < 1 || p > 65535 {
+		return "", "", fmt.Errorf("bad port number %s", port)


For invalid strings, you should use %q so the string is quoted.

check err as well

if p, err := strconv.Atoi(port); err != nil || p < 1 || p > 65535 {

bowei · 2018-04-05T07:04:39Z

pkg/dns/util/util_test.go

+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"


Don't use testify, we are trying to move away from it.

bowei · 2018-04-05T07:06:47Z

pkg/dns/util/util_test.go

+		{false, "asdf", "", ""},
+	} {
+		ip, port, err := ValidateNameserverIpAndPort(tc.nameserver)
+		if tc.isValid {


gotErr := err != nil if gotErr != tc.wantErr { t.Errorf("ValidateNameserverIpAndPort(%q) = %q, %q, %v; gotErr = %t, want %t", ip, port, err, gotErr, tc.wantErr) }

bowei · 2018-04-05T07:09:00Z

pkg/dns/util/util_test.go

+		} else {
+			assert.Error(t, err, "%s should not be valid nameserver", tc.nameserver)
+		}
+		assert.Equal(t, tc.expectedIp, ip, "Ip doesn't match.")


if ip != tc.ip || port != tc.port { t.Errorf("ValidateNameserverIpAndPort(%q) = %q, %q, nil; want %q, %q, nil", ip, port, tc.ip, tc.port) }

bowei · 2018-04-05T07:12:28Z

pkg/dns/util/util_test.go

+		{false, "1.1.1.1:abc", "", ""},
+		{false, "1.1.1.1:123456789", "", ""},
+		{false, "1.1.1.1:", "", ""},
+		{false, "asdf", "", ""},


add case `{wantErr: true, ns:"invalidip:80"},

bowei · 2018-04-05T07:14:45Z

pkg/e2e/dns/dns.go

 	cmd       *exec.Cmd
 	isRunning bool
 }

 // Start kube DNS, passing in extra arguments
-func (kd *KubeDNS) Start(args ...string) {
+func (kd *KubeDNS) Start(name string, args ...string) {


document name

Doc updated.

bowei · 2018-04-05T07:14:55Z

pkg/dns/util/util_test.go

@@ -0,0 +1,49 @@
+/*
+Copyright 2016 The Kubernetes Authors.


bowei · 2018-04-05T07:16:05Z

pkg/dns/dns.go

+	}
+	kd.config = nextConfig
+	glog.V(2).Infof("Configuration updated: %+v", *kd.config)
+


bowei · 2018-04-05T07:23:21Z

cmd/kube-dns/app/server.go

 func (d *KubeDNSServer) startSkyDNSServer() {
 	glog.V(0).Infof("Starting SkyDNS server (%v:%v)", d.dnsBindAddress, d.dnsPort)
 	skydnsConfig := &server.Config{
 		Domain:  d.domain,
 		DnsAddr: fmt.Sprintf("%s:%d", d.dnsBindAddress, d.dnsPort),
 	}
-	if d.nameServers != "" {


Where doe d.nameServers get initialized now?

It will get initialized when kubeDNS initialize its configuration through Once.
Let's say the user provides nameservers as commandline parameters. In the case of using configMap or configDir, the nameserver from command line will be overwriten when kubedns initializes its config through sync once. As for no configmap or configdir, both kubedns and skydns will use the nameserver from commandline parameter after kubedns sync once.

https://github.com/grayluck/dns/blob/acd9f258a0a65dcc27ef03d6cf059969fb4f602f/cmd/kube-dns/app/server.go#L74

bowei · 2018-04-05T17:47:46Z

Ok, can you squash all of the commits and clean up the commit message?

git rebase -i upstream/master

then we can merge.

grayluck · 2018-04-05T17:53:26Z

Squashed. Also changed the title of this PR.

Fixes issue kubernetes#198. e2e test for PTR lookup forwarding also included. Skydns syncs config with kubeDNS. Now Skydns will ignore nameservers parameter from the commandline if either configmap or configdir is given. Merged hostname validation logic and moved them to util. Unittest is added for this.

bowei · 2018-04-05T19:41:19Z

/lgtm

bowei · 2018-04-05T19:41:26Z

/approved

grayluck mentioned this pull request Mar 23, 2018

Reverse IP lookup of external IPs not working with kube-dns upstreamNameservers #198

Closed

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 28, 2018

grayluck force-pushed the fwdup branch from fb96fdc to 387d73c Compare March 28, 2018 21:50

grayluck changed the title ~~[WIP] Forward external IP from kubedns to upstream server~~ Forward external IP from kubedns to upstream server Mar 28, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 28, 2018

k8s-ci-robot assigned MrHohn Mar 28, 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 Mar 29, 2018

k8s-ci-robot assigned bowei Mar 31, 2018

bowei reviewed Mar 31, 2018

View reviewed changes

grayluck force-pushed the fwdup branch 2 times, most recently from 33bbedf to a34f511 Compare April 2, 2018 00:13

bowei reviewed Apr 3, 2018

View reviewed changes

bowei reviewed Apr 4, 2018

View reviewed changes

grayluck mentioned this pull request Apr 4, 2018

Add e2e test for kubedns to forward PTR records to upstream server. kubernetes/kubernetes#62042

Merged

bowei reviewed Apr 5, 2018

View reviewed changes

grayluck changed the title ~~Forward external IP from kubedns to upstream server~~ Forward PTR record lookup from kubedns to upstream server. Apr 5, 2018

grayluck force-pushed the fwdup branch from acd9f25 to 69cfa27 Compare April 5, 2018 17:52

grayluck force-pushed the fwdup branch from 69cfa27 to ac1eec9 Compare April 5, 2018 19:35

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

bowei approved these changes Apr 5, 2018

View reviewed changes

bowei merged commit acd6b1e into kubernetes:master Apr 5, 2018

grayluck deleted the fwdup branch April 9, 2018 18:19

MrHohn mentioned this pull request Apr 13, 2018

DNS resolution for externalName services broken in v1.14.9 #224

Closed


		kubeDNS.Start("kube-dns-ptrfwd", "-v=4", "--config-dir="+configDir)

		By("Get answer without upstream server")

Forward PTR record lookup from kubedns to upstream server. #220

Forward PTR record lookup from kubedns to upstream server. #220

Conversation

grayluck commented Mar 23, 2018 • edited Loading

grayluck commented Mar 28, 2018

MrHohn commented Mar 29, 2018

MrHohn commented Mar 30, 2018

grayluck commented Mar 31, 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

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

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

grayluck Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

bowei commented Apr 5, 2018

grayluck commented Apr 5, 2018

bowei commented Apr 5, 2018

bowei commented Apr 5, 2018

grayluck commented Mar 23, 2018 •

edited

Loading

grayluck Apr 5, 2018 •

edited

Loading