From a84c26d78b45e63d13903a888b8c693d0daea9f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carl-Magnus=20Bj=C3=B6rkell?= Date: Mon, 28 Jan 2019 17:38:27 +0100 Subject: [PATCH] plugin/etcd: Filter empty host field by qtype (#2499) When a query, different from a TXT lookup is performed, all services with a missing `Host` field should be filtered out, as these otherwize cause a line in the answer section with a single dot (`.`) as the result. This behavior manifests for example when a TXT record is present on a domain, eg. an A or SRV lookup is performed on said domain. If there are no services containing a `Host` field, a `NODATA` response should be given. If there are other Services, these alone should be returned for the query. Filter any service that has an empty Host field from all lookup types other than TXT to solve this issue. At the same time the check for empty `Text` fields in TXT queries are also moved to the same check in the etcd ServiceBackend. --- plugin/backend_lookup.go | 3 --- plugin/etcd/cname_test.go | 8 ++++++++ plugin/etcd/etcd.go | 19 ++++++++++++++++--- plugin/etcd/lookup_test.go | 9 +++++++++ 4 files changed, 33 insertions(+), 6 deletions(-) diff --git a/plugin/backend_lookup.go b/plugin/backend_lookup.go index f3bba50c1..66550ad01 100644 --- a/plugin/backend_lookup.go +++ b/plugin/backend_lookup.go @@ -331,9 +331,6 @@ func TXT(b ServiceBackend, zone string, state request.Request, opt Options) (rec } for _, serv := range services { - if serv.Text == "" { - continue - } records = append(records, serv.NewTXT(state.QName())) } return records, nil diff --git a/plugin/etcd/cname_test.go b/plugin/etcd/cname_test.go index d478a0f5c..3d571077f 100644 --- a/plugin/etcd/cname_test.go +++ b/plugin/etcd/cname_test.go @@ -58,6 +58,7 @@ var servicesCname = []*msg.Service{ {Host: "cname6.region2.skydns.test", Key: "cname5.region2.skydns.test."}, {Host: "endpoint.region2.skydns.test", Key: "cname6.region2.skydns.test."}, {Host: "mainendpoint.region2.skydns.test", Key: "region2.skydns.test."}, + {Host: "", Key: "region3.skydns.test.", Text: "SOME-RECORD-TEXT"}, {Host: "10.240.0.1", Key: "endpoint.region2.skydns.test."}, } @@ -83,4 +84,11 @@ var dnsTestCasesCname = []test.Case{ test.CNAME("region2.skydns.test. 300 IN CNAME mainendpoint.region2.skydns.test."), }, }, + { + Qname: "region3.skydns.test.", Qtype: dns.TypeCNAME, + Rcode: dns.RcodeSuccess, + Ns: []dns.RR{ + test.SOA("skydns.test. 303 IN SOA ns.dns.skydns.test. hostmaster.skydns.test. 1546424605 7200 1800 86400 30"), + }, + }, } diff --git a/plugin/etcd/etcd.go b/plugin/etcd/etcd.go index 17422e347..c75261b41 100644 --- a/plugin/etcd/etcd.go +++ b/plugin/etcd/etcd.go @@ -78,7 +78,7 @@ func (e *Etcd) Records(state request.Request, exact bool) ([]msg.Service, error) return nil, err } segments := strings.Split(msg.Path(name, e.PathPrefix), "/") - return e.loopNodes(r.Kvs, segments, star) + return e.loopNodes(r.Kvs, segments, star, state.QType()) } func (e *Etcd) get(path string, recursive bool) (*etcdcv3.GetResponse, error) { @@ -115,7 +115,7 @@ func (e *Etcd) get(path string, recursive bool) (*etcdcv3.GetResponse, error) { return r, nil } -func (e *Etcd) loopNodes(kv []*mvccpb.KeyValue, nameParts []string, star bool) (sx []msg.Service, err error) { +func (e *Etcd) loopNodes(kv []*mvccpb.KeyValue, nameParts []string, star bool, qType uint16) (sx []msg.Service, err error) { bx := make(map[msg.Service]struct{}) Nodes: for _, n := range kv { @@ -149,7 +149,10 @@ Nodes: if serv.Priority == 0 { serv.Priority = priority } - sx = append(sx, *serv) + + if shouldInclude(serv, qType) { + sx = append(sx, *serv) + } } return sx, nil } @@ -173,3 +176,13 @@ func (e *Etcd) TTL(kv *mvccpb.KeyValue, serv *msg.Service) uint32 { } return serv.TTL } + +// shouldInclude returns true if the service should be included in a list of records, given the qType. For all the +// currently supported lookup types, the only one to allow for an empty Host field in the service are TXT records. +// Similarly, the TXT record in turn requires the Text field to be set. +func shouldInclude(serv *msg.Service, qType uint16) bool { + if qType == dns.TypeTXT { + return serv.Text != "" + } + return serv.Host != "" +} \ No newline at end of file diff --git a/plugin/etcd/lookup_test.go b/plugin/etcd/lookup_test.go index 41d031863..ba2bb04f1 100644 --- a/plugin/etcd/lookup_test.go +++ b/plugin/etcd/lookup_test.go @@ -27,6 +27,8 @@ var services = []*msg.Service{ {Host: "10.0.0.1", Port: 8080, Key: "a.server1.prod.region1.skydns.test."}, {Host: "10.0.0.2", Port: 8080, Key: "b.server1.prod.region1.skydns.test."}, {Host: "::1", Port: 8080, Key: "b.server6.prod.region1.skydns.test."}, + // TXT record in server1. + {Host: "", Port: 8080, Text: "sometext", Key: "txt.server1.prod.region1.skydns.test."}, // Unresolvable internal name. {Host: "unresolvable.skydns.test", Key: "cname.prod.region1.skydns.test."}, // Priority. @@ -127,6 +129,13 @@ var dnsTestCases = []test.Case{ Qname: "cname.prod.region1.skydns.test.", Qtype: dns.TypeA, Ns: []dns.RR{test.SOA("skydns.test. 30 SOA ns.dns.skydns.test. hostmaster.skydns.test. 0 0 0 0 0")}, }, + // TXT Test + { + Qname: "server1.prod.region1.skydns.test.", Qtype: dns.TypeTXT, + Answer: []dns.RR{ + test.TXT("server1.prod.region1.skydns.test. 303 IN TXT sometext"), + }, + }, // Wildcard Test { Qname: "*.region1.skydns.test.", Qtype: dns.TypeSRV,