Repository: coredns/coredns (go)

Issue:
plugin/k8s_external
<!-- 
When reporting bugs and requesting assistance, please include:
* the version of CoreDNS you are using
* your Corefile
* logs, if applicable

Thanks for using CoreDNS!
-->
using coredns 1.3.1 and the k8s_external plugin i miss some A records in the dns subzone

<pre>
.:53 {
    errors
    health
    kubernetes k8s.cluster.local in-addr.arpa ip6.arpa {
       pods insecure
       upstream
       fallthrough in-addr.arpa ip6.arpa
       ttl 30
    }
    k8s_external k9s.cluster.local in-addr.arpa ip6.arpa {
       ttl 30
    }
    prometheus :9153
    forward . /etc/resolv.conf
    cache 30
    loop
    reload
    loadbalance
}
</pre>

the documentation notes that there is a stub/apex zone. But i'm missing an NS record and an A record
<pre>
dig NS k9s.cluster.local => invalid dns package
dig NS dns.k9s.cluster.local =>  no entry
dig A ns1.dns.k9s.cluster.local => no entry
</pre>

The kubernetes plugin handles them like
<pre>
dig NS k8s.cluster.local => ns.dns.k8s.cluster.local.
dig NS dns.k8s.cluster.local => no entry
dig A ns.dns.k8s.cluster.local => 10.96.x.y
</pre>

Pull request description:
plugin/k8s_external/kubernetes: handle NS records
### 1. Why is this pull request needed and what does it do?

Fixes #3064:
* don't SERVFAIL when querying NS  `cluster.local.` in k8s_external
* reply with a record for `ns1.dns.cluster.local.`  in k8s_external

While reworking `nsAddr()` to fix #3064, I made the following related fixes/improvements which apply to both k8s_external and kubernetes:
* optimization to use svc object indexes (previously iterated entire service list, which is expensive)
* handle the ipv6 case (where coredns service is ipv6 - previously ipv4 was assumed)
* handle case where more than one k8s service targets the coredns endpoint
* handle corner case when a headless k8s service points to the coredns endpoint

### 2. Which issues (if any) are related?
#3064

### 3. Which documentation changes (if any) need to be made?
None

### 4. Does this introduce a backward incompatible change or deprecation?
No

Patch:
diff --git a/ADOPTERS.md b/ADOPTERS.md
index a67917f54..22abc9aef 100644
--- a/ADOPTERS.md
+++ b/ADOPTERS.md
@@ -9,7 +9,7 @@
 * [Z Lab](https://zlab.co.jp) uses CoreDNS in production combination with Consul and Kubernetes Clusters.
 * [Serpro/estaleiro](estaleiro.serpro.gov.br) uses CoreDNS as Kubernetes' DNS Server, in production with tuned Kubernetes plugin options
 * [Lumo](https://thinklumo.com) uses CoreDNS as Kubernetes' DNS Server, in production and lab with default configuration
-* [Booming Games](https://booming-games.com) uses CoreDNS in multiple Kubernetes clusters, with Federation plugin. expect going to production soon.
+* [Booming Games](https://booming-games.com) uses CoreDNS in multiple Kubernetes clusters, with Federation plugin. expect to go into production soon.
 * [Sodimac](https://www.sodimac.cl) uses CoreDNS with Kubernetes in production with default configuration.
 * [Bose](https://www.bose.com/) uses CoreDNS with Kubernetes in production on very large cluster (over 250 nodes)
 * [farmotive](https://farmotive.io) uses CoreDNS in Kubernetes using default configuration, in its Lab. Expect to be in production soon.
diff --git a/notes/coredns-1.0.1.md b/notes/coredns-1.0.1.md
index 216afd46e..e3cb55898 100644
--- a/notes/coredns-1.0.1.md
+++ b/notes/coredns-1.0.1.md
@@ -16,7 +16,7 @@ One new plugin was added: *nsid*, that implements [RFC 5001](https://tools.ietf.
 ## Plugins
 * *file* fixes a crash when an request with a DO bit (pretty much the default) hits an unsigned zone. The default configuration should recover the go-routine, but this is nonetheless serious. *file* received some other fixes when returning (secure) delegations.
 * *dnstap* plugin is now 50% faster.
-* *metrics* fixed the start time bucket for duration.
+* *metrics* fixed the start time bucket for the duration.
 
 ## Contributors
 
diff --git a/plugin/dnstap/dnstapio/io.go b/plugin/dnstap/dnstapio/io.go
index 65e2e222e..9a4c26042 100644
--- a/plugin/dnstap/dnstapio/io.go
+++ b/plugin/dnstap/dnstapio/io.go
@@ -70,7 +70,7 @@ func (dio *dnstapIO) newConnect() error {
 	return dio.enc.resetWriter(dio.conn)
 }
 
-// Connect connects to the dnstop endpoint.
+// Connect connects to the dnstap endpoint.
 func (dio *dnstapIO) Connect() {
 	if err := dio.newConnect(); err != nil {
 		log.Error("No connection to dnstap endpoint")
diff --git a/plugin/etcd/msg/path.go b/plugin/etcd/msg/path.go
index c90798035..bfa458863 100644
--- a/plugin/etcd/msg/path.go
+++ b/plugin/etcd/msg/path.go
@@ -29,7 +29,7 @@ func Domain(s string) string {
 	return dnsutil.Join(l[1 : len(l)-1]...)
 }
 
-// PathWithWildcard ascts as Path, but if a name contains wildcards (* or any), the name will be
+// PathWithWildcard acts as Path, but if a name contains wildcards (* or any), the name will be
 // chopped of before the (first) wildcard, and we do a higher level search and
 // later find the matching names.  So service.*.skydns.local, will look for all
 // services under skydns.local and will later check for names that match
diff --git a/plugin/forward/forward.go b/plugin/forward/forward.go
index da2b2beb4..4abd80bae 100644
--- a/plugin/forward/forward.go
+++ b/plugin/forward/forward.go
@@ -89,7 +89,7 @@ func (f *Forward) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg
 			if fails < len(f.proxies) {
 				continue
 			}
-			// All upstream proxies are dead, assume healtcheck is completely broken and randomly
+			// All upstream proxies are dead, assume healthcheck is completely broken and randomly
 			// select an upstream to connect to.
 			r := new(random)
 			proxy = r.List(f.proxies)[0]
diff --git a/plugin/kubernetes/external.go b/plugin/kubernetes/external.go
index 4b1104460..42495a430 100644
--- a/plugin/kubernetes/external.go
+++ b/plugin/kubernetes/external.go
@@ -85,8 +85,10 @@ func (k *Kubernetes) External(state request.Request) ([]msg.Service, int) {
 
 // ExternalAddress returns the external service address(es) for the CoreDNS service.
 func (k *Kubernetes) ExternalAddress(state request.Request) []dns.RR {
-	// This is probably wrong, because of all the fallback behavior of k.nsAddr, i.e. can get
-	// an address that isn't reachable from outside the cluster.
-	rrs := []dns.RR{k.nsAddr()}
-	return rrs
+	// If CoreDNS is running inside the Kubernetes cluster: k.nsAddrs() will return the external IPs of the services
+	// targeting the CoreDNS Pod.
+	// If CoreDNS is running outside of the Kubernetes cluster: k.nsAddrs() will return the first non-loopback IP
+	// address seen on the local system it is running on. This could be the wrong answer if coredns is using the *bind*
+	// plugin to bind to a different IP address.
+	return k.nsAddrs(true, state.Zone)
 }
diff --git a/plugin/kubernetes/kubernetes.go b/plugin/kubernetes/kubernetes.go
index a02c35698..e7b36917e 100644
--- a/plugin/kubernetes/kubernetes.go
+++ b/plugin/kubernetes/kubernetes.go
@@ -107,25 +107,33 @@ func (k *Kubernetes) Services(ctx context.Context, state request.Request, exact
 
 	case dns.TypeNS:
 		// We can only get here if the qname equals the zone, see ServeDNS in handler.go.
-		ns := k.nsAddr()
-		svc := msg.Service{Host: ns.A.String(), Key: msg.Path(state.QName(), coredns), TTL: k.ttl}
-		return []msg.Service{svc}, nil
+		nss := k.nsAddrs(false, state.Zone)
+		var svcs []msg.Service
+		for _, ns := range nss {
+			if ns.Header().Rrtype == dns.TypeA {
+				svcs = append(svcs, msg.Service{Host: ns.(*dns.A).A.String(), Key: msg.Path(ns.Header().Name, coredns), TTL: k.ttl})
+				continue
+			}
+			if ns.Header().Rrtype == dns.TypeAAAA {
+				svcs = append(svcs, msg.Service{Host: ns.(*dns.AAAA).AAAA.String(), Key: msg.Path(ns.Header().Name, coredns), TTL: k.ttl})
+			}
+		}
+		return svcs, nil
 	}
 
 	if isDefaultNS(state.Name(), state.Zone) {
-		ns := k.nsAddr()
-
-		isIPv4 := ns.A.To4() != nil
-
-		if !((state.QType() == dns.TypeA && isIPv4) || (state.QType() == dns.TypeAAAA && !isIPv4)) {
-			// NODATA
-			return nil, nil
+		nss := k.nsAddrs(false, state.Zone)
+		var svcs []msg.Service
+		for _, ns := range nss {
+			if ns.Header().Rrtype == dns.TypeA && state.QType() == dns.TypeA {
+				svcs = append(svcs, msg.Service{Host: ns.(*dns.A).A.String(), Key: msg.Path(state.QName(), coredns), TTL: k.ttl})
+				continue
+			}
+			if ns.Header().Rrtype == dns.TypeAAAA && state.QType() == dns.TypeAAAA {
+				svcs = append(svcs, msg.Service{Host: ns.(*dns.AAAA).AAAA.String(), Key: msg.Path(state.QName(), coredns), TTL: k.ttl})
+			}
 		}
-
-		// If this is an A request for "ns.dns", respond with a "fake" record for coredns.
-		// SOA records always use this hardcoded name
-		svc := msg.Service{Host: ns.A.String(), Key: msg.Path(state.QName(), coredns), TTL: k.ttl}
-		return []msg.Service{svc}, nil
+		return svcs, nil
 	}
 
 	s, e := k.Records(ctx, state, false)
diff --git a/plugin/kubernetes/local.go b/plugin/kubernetes/local.go
index c5918a306..199af6f0d 100644
--- a/plugin/kubernetes/local.go
+++ b/plugin/kubernetes/local.go
@@ -12,11 +12,14 @@ func localPodIP() net.IP {
 
 	for _, addr := range addrs {
 		ip, _, _ := net.ParseCIDR(addr.String())
-		ip = ip.To4()
-		if ip == nil || ip.IsLoopback() {
-			continue
+		ip4 := ip.To4()
+		if ip4 != nil && !ip4.IsLoopback() {
+			return ip4
+		}
+		ip6 := ip.To16()
+		if ip6 != nil && !ip6.IsLoopback() {
+			return ip6
 		}
-		return ip
 	}
 	return nil
 }
diff --git a/plugin/kubernetes/ns.go b/plugin/kubernetes/ns.go
index f3d33ee22..6d18c0b77 100644
--- a/plugin/kubernetes/ns.go
+++ b/plugin/kubernetes/ns.go
@@ -4,6 +4,7 @@ import (
 	"net"
 	"strings"
 
+	"github.com/coredns/coredns/plugin/kubernetes/object"
 	"github.com/miekg/dns"
 	api "k8s.io/api/core/v1"
 )
@@ -12,54 +13,74 @@ func isDefaultNS(name, zone string) bool {
 	return strings.Index(name, defaultNSName) == 0 && strings.Index(name, zone) == len(defaultNSName)
 }
 
-// nsAddr return the A record for the CoreDNS service in the cluster. If it fails that it fallsback
-// on the local address of the machine we're running on.
-//
-// This function is rather expensive to run.
-func (k *Kubernetes) nsAddr() *dns.A {
+// nsAddrs returns the A or AAAA records for the CoreDNS service in the cluster. If the service cannot be found,
+// it returns a record for the the local address of the machine we're running on.
+func (k *Kubernetes) nsAddrs(external bool, zone string) []dns.RR {
 	var (
-		svcName      string
-		svcNamespace string
+		svcNames []string
+		svcIPs   []net.IP
 	)
 
-	rr := new(dns.A)
+	// Find the CoreDNS Endpoint
 	localIP := k.interfaceAddrsFunc()
-	rr.A = localIP
+	endpoints := k.APIConn.EpIndexReverse(localIP.String())
 
-FindEndpoint:
-	for _, ep := range k.APIConn.EpIndexReverse(localIP.String()) {
-		for _, eps := range ep.Subsets {
-			for _, addr := range eps.Addresses {
-				if localIP.Equal(net.ParseIP(addr.IP)) {
-					svcNamespace = ep.Namespace
-					svcName = ep.Name
-					break FindEndpoint
+	// If the CoreDNS Endpoint is not found, use the locally bound IP address
+	if len(endpoints) == 0 {
+		svcNames = []string{defaultNSName + zone}
+		svcIPs = []net.IP{localIP}
+	} else {
+		// Collect IPs for all Services of the Endpoints
+		for _, endpoint := range endpoints {
+			svcs := k.APIConn.SvcIndex(object.ServiceKey(endpoint.Name, endpoint.Namespace))
+			for _, svc := range svcs {
+				if external {
+					svcName := strings.Join([]string{svc.Name, svc.Namespace, zone}, ".")
+					for _, exIP := range svc.ExternalIPs {
+						svcNames = append(svcNames, svcName)
+						svcIPs = append(svcIPs, net.ParseIP(exIP))
+					}
+					continue
+				}
+				svcName := strings.Join([]string{svc.Name, svc.Namespace, Svc, zone}, ".")
+				if svc.ClusterIP == api.ClusterIPNone {
+					// For a headless service, use the endpoints IPs
+					for _, s := range endpoint.Subsets {
+						for _, a := range s.Addresses {
+							svcNames = append(svcNames, endpointHostname(a, k.endpointNameMode)+"."+svcName)
+							svcIPs = append(svcIPs, net.ParseIP(a.IP))
+						}
+					}
+				} else {
+					svcNames = append(svcNames, svcName)
+					svcIPs = append(svcIPs, net.ParseIP(svc.ClusterIP))
 				}
 			}
 		}
 	}
 
-	if len(svcName) == 0 {
-		rr.Hdr.Name = defaultNSName
-		rr.A = localIP
-		return rr
-	}
-
-FindService:
-	for _, svc := range k.APIConn.ServiceList() {
-		if svcName == svc.Name && svcNamespace == svc.Namespace {
-			if svc.ClusterIP == api.ClusterIPNone {
-				rr.A = localIP
-			} else {
-				rr.A = net.ParseIP(svc.ClusterIP)
-			}
-			break FindService
+	// Create an RR slice of collected IPs
+	var rrs []dns.RR
+	rrs = make([]dns.RR, len(svcIPs))
+	for i, ip := range svcIPs {
+		if ip.To4() == nil {
+			rr := new(dns.AAAA)
+			rr.Hdr.Class = dns.ClassINET
+			rr.Hdr.Rrtype = dns.TypeAAAA
+			rr.Hdr.Name = svcNames[i]
+			rr.AAAA = ip
+			rrs[i] = rr
+			continue
 		}
+		rr := new(dns.A)
+		rr.Hdr.Class = dns.ClassINET
+		rr.Hdr.Rrtype = dns.TypeA
+		rr.Hdr.Name = svcNames[i]
+		rr.A = ip
+		rrs[i] = rr
 	}
 
-	rr.Hdr.Name = strings.Join([]string{svcName, svcNamespace, "svc."}, ".")
-
-	return rr
+	return rrs
 }
 
 const defaultNSName = "ns.dns."
diff --git a/plugin/pkg/fuzz/do.go b/plugin/pkg/fuzz/do.go
index e2bc0edee..054c4298a 100644
--- a/plugin/pkg/fuzz/do.go
+++ b/plugin/pkg/fuzz/do.go
@@ -13,15 +13,19 @@ import (
 // Do will fuzz p - used by gofuzz. See Makefile.fuzz for comments and context.
 func Do(p plugin.Handler, data []byte) int {
 	ctx := context.TODO()
-	ret := 1
 	r := new(dns.Msg)
 	if err := r.Unpack(data); err != nil {
-		ret = 0
+		return 0 // plugin will never be called when this happens.
+	}
+	// If the data unpack into a dns msg, but does not have a proper question section discard it.
+	// The server parts make sure this is true before calling the plugins; mimic this behavior.
+	if len(r.Question) == 0 {
+		return 0
 	}
 
 	if _, err := p.ServeDNS(ctx, &test.ResponseWriter{}, r); err != nil {
-		ret = 1
+		return 1
 	}
 
-	return ret
+	return 0
 }
diff --git a/plugin/reload/reload.go b/plugin/reload/reload.go
index 59e040479..0e01f63b4 100644
--- a/plugin/reload/reload.go
+++ b/plugin/reload/reload.go
@@ -99,7 +99,7 @@ func hook(event caddy.EventName, info interface{}) error {
 					// Let not try to restart with the same file, even though it is wrong.
 					md5sum = s
 					// now lets consider that plugin will not be reload, unless appear in next config file
-					// change status iof usage will be reset in setup if the plugin appears in config file
+					// change status of usage will be reset in setup if the plugin appears in config file
 					r.setUsage(maybeUsed)
 					_, err := instance.Restart(corefile)
 					if err != nil {


Test patch:
diff --git a/plugin/kubernetes/kubernetes_test.go b/plugin/kubernetes/kubernetes_test.go
index 57b735d7b..4fd8e22a6 100644
--- a/plugin/kubernetes/kubernetes_test.go
+++ b/plugin/kubernetes/kubernetes_test.go
@@ -327,7 +327,7 @@ func TestServicesAuthority(t *testing.T) {
 
 		state := request.Request{
 			Req:  &dns.Msg{Question: []dns.Question{{Name: test.qname, Qtype: test.qtype}}},
-			Zone: "interwebs.test.", // must match from k.Zones[0]
+			Zone: k.Zones[0],
 		}
 		svcs, e := k.Services(context.TODO(), state, false, plugin.Options{})
 		if e != nil {
diff --git a/plugin/kubernetes/ns_test.go b/plugin/kubernetes/ns_test.go
index df7cf5f83..af057c254 100644
--- a/plugin/kubernetes/ns_test.go
+++ b/plugin/kubernetes/ns_test.go
@@ -1,9 +1,11 @@
 package kubernetes
 
 import (
+	"net"
 	"testing"
 
 	"github.com/coredns/coredns/plugin/kubernetes/object"
+	"github.com/miekg/dns"
 
 	api "k8s.io/api/core/v1"
 )
@@ -14,12 +16,23 @@ func (APIConnTest) HasSynced() bool                          { return true }
 func (APIConnTest) Run()                                     { return }
 func (APIConnTest) Stop() error                              { return nil }
 func (APIConnTest) PodIndex(string) []*object.Pod            { return nil }
-func (APIConnTest) SvcIndex(string) []*object.Service        { return nil }
 func (APIConnTest) SvcIndexReverse(string) []*object.Service { return nil }
 func (APIConnTest) EpIndex(string) []*object.Endpoints       { return nil }
 func (APIConnTest) EndpointsList() []*object.Endpoints       { return nil }
 func (APIConnTest) Modified() int64                          { return 0 }
 
+func (a APIConnTest) SvcIndex(s string) []*object.Service {
+	switch s {
+	case "dns-service.kube-system":
+		return []*object.Service{a.ServiceList()[0]}
+	case "hdls-dns-service.kube-system":
+		return []*object.Service{a.ServiceList()[1]}
+	case "dns6-service.kube-system":
+		return []*object.Service{a.ServiceList()[2]}
+	}
+	return nil
+}
+
 func (APIConnTest) ServiceList() []*object.Service {
 	svcs := []*object.Service{
 		{
@@ -27,18 +40,31 @@ func (APIConnTest) ServiceList() []*object.Service {
 			Namespace: "kube-system",
 			ClusterIP: "10.0.0.111",
 		},
+		{
+			Name:      "hdls-dns-service",
+			Namespace: "kube-system",
+			ClusterIP: api.ClusterIPNone,
+		},
+		{
+			Name:      "dns6-service",
+			Namespace: "kube-system",
+			ClusterIP: "10::111",
+		},
 	}
 	return svcs
 }
 
-func (APIConnTest) EpIndexReverse(string) []*object.Endpoints {
+func (APIConnTest) EpIndexReverse(ip string) []*object.Endpoints {
+	if ip != "10.244.0.20" {
+		return nil
+	}
 	eps := []*object.Endpoints{
 		{
 			Subsets: []object.EndpointSubset{
 				{
 					Addresses: []object.EndpointAddress{
 						{
-							IP: "127.0.0.1",
+							IP: "10.244.0.20",
 						},
 					},
 				},
@@ -46,6 +72,32 @@ func (APIConnTest) EpIndexReverse(string) []*object.Endpoints {
 			Name:      "dns-service",
 			Namespace: "kube-system",
 		},
+		{
+			Subsets: []object.EndpointSubset{
+				{
+					Addresses: []object.EndpointAddress{
+						{
+							IP: "10.244.0.20",
+						},
+					},
+				},
+			},
+			Name:      "hdls-dns-service",
+			Namespace: "kube-system",
+		},
+		{
+			Subsets: []object.EndpointSubset{
+				{
+					Addresses: []object.EndpointAddress{
+						{
+							IP: "10.244.0.20",
+						},
+					},
+				},
+			},
+			Name:      "dns6-service",
+			Namespace: "kube-system",
+		},
 	}
 	return eps
 }
@@ -55,19 +107,43 @@ func (APIConnTest) GetNamespaceByName(name string) (*api.Namespace, error) {
 	return &api.Namespace{}, nil
 }
 
-func TestNsAddr(t *testing.T) {
+func TestNsAddrs(t *testing.T) {
 
 	k := New([]string{"inter.webs.test."})
 	k.APIConn = &APIConnTest{}
+	k.interfaceAddrsFunc = func() net.IP { return net.ParseIP("10.244.0.20") }
 
-	cdr := k.nsAddr()
-	expected := "10.0.0.111"
+	cdrs := k.nsAddrs(false, k.Zones[0])
 
-	if cdr.A.String() != expected {
-		t.Errorf("Expected A to be %q, got %q", expected, cdr.A.String())
+	if len(cdrs) != 3 {
+		t.Fatalf("Expected 3 results, got %v", len(cdrs))
+
+	}
+	cdr := cdrs[0]
+	expected := "10.0.0.111"
+	if cdr.(*dns.A).A.String() != expected {
+		t.Errorf("Expected 1st A to be %q, got %q", expected, cdr.(*dns.A).A.String())
+	}
+	expected = "dns-service.kube-system.svc.inter.webs.test."
+	if cdr.Header().Name != expected {
+		t.Errorf("Expected 1st Header Name to be %q, got %q", expected, cdr.Header().Name)
+	}
+	cdr = cdrs[1]
+	expected = "10.244.0.20"
+	if cdr.(*dns.A).A.String() != expected {
+		t.Errorf("Expected 2nd A to be %q, got %q", expected, cdr.(*dns.A).A.String())
+	}
+	expected = "10-244-0-20.hdls-dns-service.kube-system.svc.inter.webs.test."
+	if cdr.Header().Name != expected {
+		t.Errorf("Expected 2nd Header Name to be %q, got %q", expected, cdr.Header().Name)
+	}
+	cdr = cdrs[2]
+	expected = "10::111"
+	if cdr.(*dns.AAAA).AAAA.String() != expected {
+		t.Errorf("Expected AAAA to be %q, got %q", expected, cdr.(*dns.A).A.String())
 	}
-	expected = "dns-service.kube-system.svc."
-	if cdr.Hdr.Name != expected {
-		t.Errorf("Expected Hdr.Name to be %q, got %q", expected, cdr.Hdr.Name)
+	expected = "dns6-service.kube-system.svc.inter.webs.test."
+	if cdr.Header().Name != expected {
+		t.Errorf("Expected AAAA Header Name to be %q, got %q", expected, cdr.Header().Name)
 	}
 }
diff --git a/plugin/kubernetes/parse_test.go b/plugin/kubernetes/parse_test.go
index 0ce4b7d98..6fc635477 100644
--- a/plugin/kubernetes/parse_test.go
+++ b/plugin/kubernetes/parse_test.go
@@ -37,7 +37,7 @@ func TestParseRequest(t *testing.T) {
 		}
 		rs := r.String()
 		if rs != tc.expected {
-			t.Errorf("Test %d, expected (stringyfied) recordRequest: %s, got %s", i, tc.expected, rs)
+			t.Errorf("Test %d, expected (stringified) recordRequest: %s, got %s", i, tc.expected, rs)
 		}
 	}
 }