Fix tests

Add a whole bunch of comments to document what we are trying to do.

middleware/kubernetes/handler_test.go

@@ -12,7 +12,6 @@ import (
 )
 
 var dnsTestCases = map[string](test.Case){
-	// *.any.svc-1-a.*.svc.cluster.local.,
 	"A Service": {
 		Qname: "svc1.testns.svc.cluster.local.", Qtype: dns.TypeA,
 		Rcode: dns.RcodeSuccess,
@@ -24,29 +23,26 @@ var dnsTestCases = map[string](test.Case){
 		Qname: "svc1.*.svc.cluster.local.", Qtype: dns.TypeA,
 		Rcode: dns.RcodeSuccess,
 		Answer: []dns.RR{
-			test.A("svc1.testns.svc.cluster.local.  0       IN      A       10.0.0.1"),
+			test.A("svc1.*.svc.cluster.local.  0       IN      A       10.0.0.1"),
 		},
 	},
+	"SRV Service (wildcard)": {
+		Qname: "svc1.*.svc.cluster.local.", Qtype: dns.TypeSRV,
+		Rcode: dns.RcodeSuccess,
+		Answer: []dns.RR{test.SRV("svc1.*.svc.cluster.local.	0	IN	SRV	0 100 80 svc1.testns.svc.cluster.local.")},
+		Extra: []dns.RR{test.A("svc1.testns.svc.cluster.local.  0       IN      A       10.0.0.1")},
+	},
 	"SRV Service (wildcards)": {
 		Qname: "*.any.svc1.*.svc.cluster.local.", Qtype: dns.TypeSRV,
 		Rcode: dns.RcodeSuccess,
-		Answer: []dns.RR{
+		Answer: []dns.RR{test.SRV("*.any.svc1.*.svc.cluster.local.	0	IN	SRV	0 100 80 svc1.testns.svc.cluster.local.")},
-			test.A("svc1.testns.svc.cluster.local.  0       IN      A       10.0.0.1"),
+		Extra: []dns.RR{test.A("svc1.testns.svc.cluster.local.  0       IN      A       10.0.0.1")},
-		},
 	},
 	"A Service (wildcards)": {
 		Qname: "*.any.svc1.*.svc.cluster.local.", Qtype: dns.TypeA,
 		Rcode: dns.RcodeSuccess,
 		Answer: []dns.RR{
-			test.A("svc1.testns.svc.cluster.local.  0       IN      A       10.0.0.1"),
+			test.A("*.any.svc1.*.svc.cluster.local.  0       IN      A       10.0.0.1"),
-		},
-	},
-	"A Service (Headless)": {
-		Qname: "hdls1.testns.svc.cluster.local.", Qtype: dns.TypeA,
-		Rcode: dns.RcodeSuccess,
-		Answer: []dns.RR{
-			test.A("hdls1.testns.svc.cluster.local.	0	IN	A	172.0.0.2"),
-			test.A("hdls1.testns.svc.cluster.local.	0	IN	A	172.0.0.3"),
 		},
 	},
 	"SRV Service": {
@@ -59,6 +55,14 @@ var dnsTestCases = map[string](test.Case){
 			test.A("svc1.testns.svc.cluster.local.	0	IN	A	10.0.0.1"),
 		},
 	},
+	"A Service (Headless)": {
+		Qname: "hdls1.testns.svc.cluster.local.", Qtype: dns.TypeA,
+		Rcode: dns.RcodeSuccess,
+		Answer: []dns.RR{
+			test.A("hdls1.testns.svc.cluster.local.	0	IN	A	172.0.0.2"),
+			test.A("hdls1.testns.svc.cluster.local.	0	IN	A	172.0.0.3"),
+		},
+	},
 	"SRV Service (Headless)": {
 		Qname: "_http._tcp.hdls1.testns.svc.cluster.local.", Qtype: dns.TypeSRV,
 		Rcode: dns.RcodeSuccess,
@@ -71,7 +75,6 @@ var dnsTestCases = map[string](test.Case){
 			test.A("172-0-0-3.hdls1.testns.svc.cluster.local.	0	IN	A	172.0.0.3"),
 		},
 	},
-	// TODO A External
 	"CNAME External": {
 		Qname: "external.testns.svc.cluster.local.", Qtype: dns.TypeCNAME,
 		Rcode: dns.RcodeSuccess,
@@ -175,13 +178,12 @@ func TestServeDNS(t *testing.T) {
 	ctx := context.TODO()
 	runServeDNSTests(ctx, t, dnsTestCases, k)
 
-	//Set PodMode to Disabled
 	k.PodMode = PodModeDisabled
 	runServeDNSTests(ctx, t, podModeDisabledCases, k)
-	//Set PodMode to Insecure
+
 	k.PodMode = PodModeInsecure
 	runServeDNSTests(ctx, t, podModeInsecureCases, k)
-	//Set PodMode to Verified
+
 	k.PodMode = PodModeVerified
 	runServeDNSTests(ctx, t, podModeVerifiedCases, k)
 }

middleware/kubernetes/kubernetes.go

@@ -450,13 +450,19 @@ func (k *Kubernetes) findServices(r recordRequest) ([]kService, error) {
 				if ep.ObjectMeta.Name != svc.Name || ep.ObjectMeta.Namespace != svc.Namespace {
 					continue
 				}
+
 				for _, eps := range ep.Subsets {
 					for _, addr := range eps.Addresses {
-						for _, p := range eps.Ports {
+
-							ephostname := endpointHostname(addr)
+						// See comments in parse.go parseRequest about the endpoint handling.
-							if r.endpoint != "" && r.endpoint != ephostname {
+
+						if r.endpoint != "" {
+							if !match(r.endpoint, endpointHostname(addr)) {
 								continue
 							}
+						}
+
+						for _, p := range eps.Ports {
 							if !(match(r.port, p.Name) && match(r.protocol, string(p.Protocol))) {
 								continue
 							}

middleware/kubernetes/kubernetes_test.go

@@ -217,24 +217,26 @@ func TestServices(t *testing.T) {
 		{qname: "external.testns.svc.interwebs.test.", qtype: dns.TypeCNAME, answer: svcAns{host: "coredns.io", key: "/coredns/test/interwebs/svc/testns/external"}},
 	}
 
-	for _, test := range tests {
+	for i, test := range tests {
 		state := request.Request{
 			Req:  &dns.Msg{Question: []dns.Question{{Name: test.qname, Qtype: test.qtype}}},
 			Zone: "interwebs.test.", // must match from k.Zones[0]
 		}
 		svcs, _, e := k.Services(state, false, middleware.Options{})
 		if e != nil {
-			t.Errorf("Query '%v' got error '%v'", test.qname, e)
+			t.Errorf("Test %d: got error '%v'", i, e)
+			continue
 		}
 		if len(svcs) != 1 {
-			t.Errorf("Query %v %v: expected expected 1 answer, got %v", test.qname, dns.TypeToString[test.qtype], len(svcs))
+			t.Errorf("Test %d, expected expected 1 answer, got %v", i, len(svcs))
-		} else {
+			continue
+		}
+
 		if test.answer.host != svcs[0].Host {
-				t.Errorf("Query %v %v: expected host '%v', got '%v'", test.qname, dns.TypeToString[test.qtype], test.answer.host, svcs[0].Host)
+			t.Errorf("Test %d, expected host '%v', got '%v'", i, test.answer.host, svcs[0].Host)
 		}
 		if test.answer.key != svcs[0].Key {
-				t.Errorf("Query %v %v: expected key '%v', got '%v'", test.qname, dns.TypeToString[test.qtype], test.answer.key, svcs[0].Key)
+			t.Errorf("Test %d, expected key '%v', got '%v'", i, test.answer.key, svcs[0].Key)
-			}
 		}
 	}
 }

middleware/kubernetes/parse.go

@@ -24,15 +24,15 @@ type recordRequest struct {
 }
 
 // parseRequest parses the qname to find all the elements we need for querying k8s. Anything
-// that is not parsed will have the wildcard "*" value. Potential underscores are stripped
+// that is not parsed will have the wildcard "*" value (except r.endpoint).
-// from _port and _protocol.
+// Potential underscores are stripped from _port and _protocol.
 func (k *Kubernetes) parseRequest(state request.Request) (r recordRequest, err error) {
 	// 3 Possible cases:
-	// o SRV Request: _port._protocol.service.namespace.pod|svc.zone
+	// 1. _port._protocol.service.namespace.pod|svc.zone
-	// o A Request (endpoint): endpoint.service.namespace.pod|svc.zone
+	// 2. (endpoint): endpoint.service.namespace.pod|svc.zone
-	// o A Request (service): service.namespace.pod|svc.zone
+	// 3. (service): service.namespace.pod|svc.zone
 	//
-	// Federations are handled in the federation middleware.
+	// Federations are handled in the federation middleware. And aren't parsed here.
 
 	base, _ := dnsutil.TrimZone(state.Name(), state.Zone)
 	segs := dns.SplitDomainName(base)
@@ -40,8 +40,9 @@ func (k *Kubernetes) parseRequest(state request.Request) (r recordRequest, err e
 	r.port = "*"
 	r.protocol = "*"
 	r.service = "*"
-	r.endpoint = "" // TODO(miek): dangerous; should just work with "*", but "" is checked in k.get()
 	r.namespace = "*"
+	// r.endpoint is the odd one out, we need to know if it has been set or not. If it is
+	// empty we should skip the endpoint check in k.get(). Hence we cannot set if to "*".
 
 	// start at the right and fill out recordRequest with the bits we find, so we look for
 	// pod|svc.namespace.service and then either
@@ -70,27 +71,31 @@ func (k *Kubernetes) parseRequest(state request.Request) (r recordRequest, err e
 		return r, nil
 	}
 
-	if segs[last][0] == '_' {
+	// Becuase of ambiquity we check the labels left: 1: an endpoint. 2: port and protocol.
-		r.protocol = segs[last][1:]
+	// Anything else is a query that is too long to answer and can safely be delegated to return an nxdomain.
-	} else {
+	switch last {
+
+	case 0: // endpoint only
 		r.endpoint = segs[last]
-	}
+	case 1: // service and port
-	last--
+		r.protocol = stripUnderscore(segs[last])
-	if last < 0 {
+		r.port = stripUnderscore(segs[last-1])
-		return r, nil
-	}
 
-	if segs[last][0] == '_' {
+	default: // too long
-		r.port = segs[last][1:]
-	}
-
-	if last > 0 { // Too long, so NXDOMAIN these.
 		return r, errInvalidRequest
-
 	}
+
 	return r, nil
 }
 
+// stripUnderscore removes a prefixed underscore from s.
+func stripUnderscore(s string) string {
+	if s[0] != '_' {
+		return s
+	}
+	return s[1:]
+}
+
 // String return a string representation of r, it just returns all fields concatenated with dots.
 // This is mostly used in tests.
 func (r recordRequest) String() string {

middleware/kubernetes/parse_test.go

@@ -24,7 +24,7 @@ func TestParseRequest(t *testing.T) {
 		{
 			// wildcard acceptance
 			"*.any.*.any.svc.inter.webs.test.", dns.TypeSRV,
-			"*.*.any.*.any.svc",
+			"*.any..*.any.svc",
 		},
 		{
 			// A request of endpoint