From 18032015b9b5d3674b9fe7e71dbc9e9f73ee101b Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Tue, 14 Jul 2015 16:06:05 +0100 Subject: [PATCH] Revert "Resolve unqualified names in the local WeaveDNS domain" --- nameserver/dns.go | 4 --- nameserver/names.go | 28 ---------------- nameserver/names_test.go | 34 ------------------- nameserver/server.go | 57 ++++++++++++++------------------ nameserver/server_test.go | 16 +++------ nameserver/zone_lookup.go | 7 ++-- nameserver/zone_lookup_test.go | 30 ----------------- test/230_dns_unqualified_test.sh | 14 ++++---- test/config.sh | 5 ++- 9 files changed, 43 insertions(+), 152 deletions(-) delete mode 100644 nameserver/names.go delete mode 100644 nameserver/names_test.go diff --git a/nameserver/dns.go b/nameserver/dns.go index e6b4b6a4..c232d45b 100644 --- a/nameserver/dns.go +++ b/nameserver/dns.go @@ -80,10 +80,6 @@ func makeDNSFailResponse(r *dns.Msg) *dns.Msg { return m } -func failHandleFunc(w dns.ResponseWriter, r *dns.Msg) { - w.WriteMsg(makeDNSFailResponse(r)) -} - // get the maximum UDP-reply length func getMaxReplyLen(r *dns.Msg, proto dnsProtocol) int { maxLen := minUDPSize diff --git a/nameserver/names.go b/nameserver/names.go deleted file mode 100644 index edf227ef..00000000 --- a/nameserver/names.go +++ /dev/null @@ -1,28 +0,0 @@ -package nameserver - -import ( - "strings" - - "github.com/miekg/dns" -) - -// Get the number of components of a name -// For example, "redis" is 1, "redis.weave.local" is 3... -func nameNumComponents(name string) int { - if len(name) == 0 || name == "." { - return 0 - } - return strings.Count(dns.Fqdn(name), ".") -} - -// normalize a name by -// a) adding the local domain when it is a single component name -// b) converting the result to FQDN -func normalizeName(n string, domain string) string { - // we should never add anything to a FQDN like "redis.", but that's the only thing that - // some broken resolvers will do: to convert the name to FQDN... - if nameNumComponents(n) == 1 { - n = dns.Fqdn(n) + domain - } - return dns.Fqdn(n) -} diff --git a/nameserver/names_test.go b/nameserver/names_test.go deleted file mode 100644 index 9af2539c..00000000 --- a/nameserver/names_test.go +++ /dev/null @@ -1,34 +0,0 @@ -package nameserver - -import ( - "testing" - - "github.com/stretchr/testify/require" - . "github.com/weaveworks/weave/common" -) - -func TestNamesNumComponents(t *testing.T) { - EnableDebugLogging(testing.Verbose()) - - assertNumComps := func(n string, e int) { require.Equal(t, e, nameNumComponents(n), "name num components failed") } - - assertNumComps("", 0) - assertNumComps(".", 0) - assertNumComps("something", 1) - assertNumComps("something.", 1) - assertNumComps("something.local", 2) - assertNumComps("something.local.", 2) -} - -func TestNamesNormalization(t *testing.T) { - EnableDebugLogging(testing.Verbose()) - - assertNormalization := func(s, d, e string) { require.Equal(t, e, normalizeName(s, d), "name normalization failed") } - - assertNormalization("", "domain.local.", ".") - assertNormalization(".", "domain.local.", ".") - assertNormalization("something", "domain.local.", "something.domain.local.") - assertNormalization("something.", "domain.local.", "something.domain.local.") - assertNormalization("something.domain.local", "domain.local.", "something.domain.local.") - assertNormalization("something.domain.local.", "domain.local.", "something.domain.local.") -} diff --git a/nameserver/server.go b/nameserver/server.go index 4ff42e4a..6f36aaf4 100644 --- a/nameserver/server.go +++ b/nameserver/server.go @@ -279,6 +279,9 @@ func (s *DNSServer) Stop() error { // We must create two DNS request multiplexers, depending on the protocol used by // clients (as we use the same protocol for asking upstream servers) func (s *DNSServer) createMux(proto dnsProtocol) *dns.ServeMux { + failFallback := func(w dns.ResponseWriter, r *dns.Msg) { + w.WriteMsg(makeDNSFailResponse(r)) + } notUsHandler := s.notUsHandler(proto) notUsFallback := func(w dns.ResponseWriter, r *dns.Msg) { Log.Infof("[dns msgid %d] -> sending to fallback server", r.MsgHdr.Id) @@ -288,7 +291,7 @@ func (s *DNSServer) createMux(proto dnsProtocol) *dns.ServeMux { // create the multiplexer m := dns.NewServeMux() m.HandleFunc(s.Zone.Domain(), s.localHandler(proto, "Query", dns.TypeA, - s.Zone.DomainLookupName, makeAddressReply, s.Zone.ObserveName, failHandleFunc)) + s.Zone.DomainLookupName, makeAddressReply, s.Zone.ObserveName, failFallback)) m.HandleFunc(RDNSDomain, s.localHandler(proto, "Reverse query", dns.TypePTR, s.Zone.DomainLookupInaddr, makePTRReply, s.Zone.ObserveInaddr, notUsFallback)) m.HandleFunc(".", s.notUsHandler(proto)) @@ -370,42 +373,32 @@ func (s *DNSServer) localHandler(proto dnsProtocol, kind string, qtype uint16, func (s *DNSServer) notUsHandler(proto dnsProtocol) dns.HandlerFunc { dnsClient := proto.GetNewClient(DefaultUDPBuflen, s.timeout) - localNameResolver := s.localHandler(proto, "Query", dns.TypeA, - s.Zone.DomainLookupName, makeAddressReply, s.Zone.ObserveName, failHandleFunc) - return func(w dns.ResponseWriter, r *dns.Msg) { q := r.Question[0] - nc := nameNumComponents(q.Name) - if nc == 1 { - Log.Debugf("[dns msgid %d] Unqualified query: '%+v' ", r.MsgHdr.Id, q) - localNameResolver(w, r) // short names (eg, "redis.") will follow a local resolution process - return - } else if nc > 1 { - // announce our max payload size as the max payload our client supports - maxLen := getMaxReplyLen(r, proto) - rcopy := r - rcopy.SetEdns0(uint16(maxLen), false) - Log.Debugf("[dns msgid %d] Fallback query: %+v [%s, max:%d bytes]", rcopy.MsgHdr.Id, q, proto, maxLen) - for _, server := range s.Upstream.Servers { - reply, _, err := dnsClient.Exchange(rcopy, fmt.Sprintf("%s:%s", server, s.Upstream.Port)) - if err != nil { - Log.Debugf("[dns msgid %d] Network error trying %s (%s)", - r.MsgHdr.Id, server, err) - continue - } - if reply != nil && reply.Rcode != dns.RcodeSuccess { - Log.Debugf("[dns msgid %d] Failure reported by %s for query %s", - r.MsgHdr.Id, server, q.Name) - continue - } - Log.Debugf("[dns msgid %d] Given answer by %s for query %s", - r.MsgHdr.Id, server, q.Name) - w.WriteMsg(reply) - return + // announce our max payload size as the max payload our client supports + maxLen := getMaxReplyLen(r, proto) + rcopy := r + rcopy.SetEdns0(uint16(maxLen), false) + + Log.Debugf("[dns msgid %d] Fallback query: %+v [%s, max:%d bytes]", rcopy.MsgHdr.Id, q, proto, maxLen) + for _, server := range s.Upstream.Servers { + reply, _, err := dnsClient.Exchange(rcopy, fmt.Sprintf("%s:%s", server, s.Upstream.Port)) + if err != nil { + Log.Debugf("[dns msgid %d] Network error trying %s (%s)", + r.MsgHdr.Id, server, err) + continue } + if reply != nil && reply.Rcode != dns.RcodeSuccess { + Log.Debugf("[dns msgid %d] Failure reported by %s for query %s", + r.MsgHdr.Id, server, q.Name) + continue + } + Log.Debugf("[dns msgid %d] Given answer by %s for query %s", + r.MsgHdr.Id, server, q.Name) + w.WriteMsg(reply) + return } - Log.Warningf("[dns msgid %d] Failed lookup for external name %s", r.MsgHdr.Id, q.Name) w.WriteMsg(makeDNSFailResponse(r)) } diff --git a/nameserver/server_test.go b/nameserver/server_test.go index 9ec63986..9c4a1976 100644 --- a/nameserver/server_test.go +++ b/nameserver/server_test.go @@ -32,12 +32,11 @@ func TestUDPDNSServer(t *testing.T) { setupForTest(t) const ( - successTestName = "test1.weave.local." - successTestNameShort = "test1." - failTestName = "fail.weave.local." - nonLocalName = "weave.works." - testAddr1 = "10.2.2.1" - containerID = "somecontainer" + successTestName = "test1.weave.local." + failTestName = "fail.weave.local." + nonLocalName = "weave.works." + testAddr1 = "10.2.2.1" + containerID = "somecontainer" ) testCIDR1 := testAddr1 + "/24" @@ -106,11 +105,6 @@ func TestUDPDNSServer(t *testing.T) { require.IsType(t, (*dns.A)(nil), r.Answer[0], "DNS record") require.Equal(t, testAddr1, r.Answer[0].(*dns.A).A.String(), "IP address") - // Assert that we can get the same result when asking for the same name but with no domain - _, r = assertExchange(t, successTestNameShort, dns.TypeA, testPort, 1, 1, 0) - require.IsType(t, (*dns.A)(nil), r.Answer[0], "DNS record") - require.Equal(t, testAddr1, r.Answer[0].(*dns.A).A.String(), "IP address") - assertExchange(t, failTestName, dns.TypeA, testPort, 0, 0, dns.RcodeNameError) _, r = assertExchange(t, testRDNSsuccess, dns.TypePTR, testPort, 1, 1, 0) diff --git a/nameserver/zone_lookup.go b/nameserver/zone_lookup.go index 32486131..6e515b62 100644 --- a/nameserver/zone_lookup.go +++ b/nameserver/zone_lookup.go @@ -4,6 +4,7 @@ import ( "net" "time" + "github.com/miekg/dns" . "github.com/weaveworks/weave/common" ) @@ -61,7 +62,7 @@ func (zone *ZoneDb) LookupName(name string) (res []ZoneRecord, err error) { defer zone.mx.RUnlock() // note: LookupName() is usually called from the mDNS server, so we do not touch the name - name = normalizeName(name, zone.domain) + name = dns.Fqdn(name) Log.Debugf("[zonedb] Looking for name '%s' in local database", name) return zone.lookup(name, func(ns *nameSet) []*recordEntry { return ns.getEntriesForName(name) }) } @@ -116,7 +117,7 @@ func (zone *ZoneDb) domainLookup(target string, lfun ZoneLookupFunc) (res []Zone // Perform a lookup for a name in the zone // The name can be resolved locally with the local database or with some other resolution method (eg, a mDNS query) func (zone *ZoneDb) DomainLookupName(name string) (res []ZoneRecord, err error) { - name = normalizeName(name, zone.domain) + name = dns.Fqdn(name) Log.Debugf("[zonedb] Looking for name '%s' in local(&remote) database", name) zone.mx.Lock() @@ -266,7 +267,7 @@ func (zone *ZoneDb) updater(name string) (nextTime time.Time) { } // perform the refresh for this name - fullName := normalizeName(name, zone.domain) + fullName := dns.Fqdn(name) startTime := zone.clock.Now() Log.Debugf("[zonedb] Refreshing name '%s' with mDNS...", fullName) res, _ := zone.mdnsCli.InsistentLookupName(fullName) diff --git a/nameserver/zone_lookup_test.go b/nameserver/zone_lookup_test.go index 784d2612..31789133 100644 --- a/nameserver/zone_lookup_test.go +++ b/nameserver/zone_lookup_test.go @@ -9,36 +9,6 @@ import ( . "github.com/weaveworks/weave/common" ) -func TestZoneLookups(t *testing.T) { - var ( - id = "foobar" - name = "foo.weave.local" - addr1 = "10.2.2.3/24" - addr2 = "10.2.7.8/24" - ) - - EnableDebugLogging(testing.Verbose()) - - zone, err := NewZoneDb(ZoneConfig{}) - require.NoError(t, err) - err = zone.Start() - require.NoError(t, err) - defer zone.Stop() - - for _, addr := range []string{addr1, addr2} { - ip, _, _ := net.ParseCIDR(addr) - err := zone.AddRecord(id, name, ip) - require.NoError(t, err) - } - - // Check we can query for the name in many different ways... - for _, qName := range []string{"foo.weave.local", "foo.weave.local.", "foo", "foo."} { - res, err := zone.LookupName(qName) - require.NoError(t, err) - require.Equal(t, len(res), 2, "Unexpected number of responses") - } -} - // Check that the refreshing mechanism works as expected func TestZoneRefresh(t *testing.T) { const ( diff --git a/test/230_dns_unqualified_test.sh b/test/230_dns_unqualified_test.sh index 1bbffe83..38e30043 100755 --- a/test/230_dns_unqualified_test.sh +++ b/test/230_dns_unqualified_test.sh @@ -11,19 +11,19 @@ NAME=seeone.$DOMAIN start_suite "Resolve unqualified names" -launch_dns_on $HOST1 10.2.254.1/24 --debug +launch_dns_on $HOST1 10.2.254.1/24 start_container $HOST1 $C1/24 --name=c1 -h $NAME start_container_with_dns $HOST1 $C2/24 --name=c2 -h seetwo.$DOMAIN start_container_with_dns $HOST1 $C3/24 --name=c3 --dns-search=$DOMAIN container=$(start_container_with_dns $HOST1 $C4/24) -assert_dns_a_record $HOST1 c2 seeone $C1 $NAME -assert_dns_a_record $HOST1 c3 seeone $C1 $NAME -assert_dns_a_record $HOST1 "$container" seeone $C1 $NAME +check() { + assert "exec_on $HOST1 $1 getent hosts seeone | tr -s ' '" "$C1 $NAME" +} -# check that unqualified names are automatically qualified for broken resolvers -weave_on $HOST1 dns-add $C1 c1 -h mysql.weave.local -assert_dns_a_record $HOST1 c1 mysql $C1 +check c2 +check c3 +check "$container" end_suite diff --git a/test/config.sh b/test/config.sh index 05fd419d..3d136999 100644 --- a/test/config.sh +++ b/test/config.sh @@ -188,10 +188,9 @@ assert_no_dns_record() { assert_raises "exec_on $host $container getent hosts $name" 2 } -# assert_dns_a_record [] +# assert_dns_a_record assert_dns_a_record() { - exp_name=${5:-$3} - assert "exec_on $1 $2 getent hosts $3 | tr -s ' ' | cut -d ' ' -f 1,2" "$4 $exp_name" + assert "exec_on $1 $2 getent hosts $3 | tr -s ' '" "$4 $3" } # assert_dns_ptr_record