From 357174e3510a07a3332030e2dbb54c363cb5da27 Mon Sep 17 00:00:00 2001 From: Tom Wilkie Date: Wed, 23 Sep 2015 03:11:12 +0000 Subject: [PATCH 1/2] DNS lookups should not be case sensitive. - On-the-wire gossip messages are sorted case sensitive, but in memory they are sorted case-insensitive. - Add test for case insensitive match on multiple names. --- nameserver/entry.go | 97 +++++++++++++++++++-------- nameserver/nameserver.go | 6 +- test/245_dns_case_insensitive_test.sh | 31 +++++++++ test/config.sh | 2 +- 4 files changed, 105 insertions(+), 31 deletions(-) create mode 100755 test/245_dns_case_insensitive_test.sh diff --git a/nameserver/entry.go b/nameserver/entry.go index 6a750cd493..a29765071e 100644 --- a/nameserver/entry.go +++ b/nameserver/entry.go @@ -5,6 +5,7 @@ import ( "encoding/gob" "fmt" "sort" + "strings" "time" "github.com/weaveworks/weave/net/address" @@ -23,8 +24,26 @@ type Entry struct { } type Entries []Entry - -func (e1 *Entry) equal(e2 *Entry) bool { +type CaseSensitive Entries +type CaseInsensitive Entries +type SortableEntries interface { + sort.Interface + Get(i int) Entry +} + +// Gossip messages are sorted in a case sensitive order... +func (es CaseSensitive) Len() int { return len(es) } +func (es CaseSensitive) Swap(i, j int) { es[i], es[j] = es[j], es[i] } +func (es CaseSensitive) Get(i int) Entry { return es[i] } +func (es CaseSensitive) Less(i, j int) bool { return es[i].less(&es[j]) } + +// ... but we store entries in a case insensitive order. +func (es CaseInsensitive) Len() int { return len(es) } +func (es CaseInsensitive) Swap(i, j int) { es[i], es[j] = es[j], es[i] } +func (es CaseInsensitive) Get(i int) Entry { return es[i] } +func (es CaseInsensitive) Less(i, j int) bool { return es[i].insensitiveLess(&es[j]) } + +func (e1 Entry) equal(e2 Entry) bool { return e1.ContainerID == e2.ContainerID && e1.Origin == e2.Origin && e1.Addr == e2.Addr && @@ -48,6 +67,24 @@ func (e1 *Entry) less(e2 *Entry) bool { } } +func (e1 *Entry) insensitiveLess(e2 *Entry) bool { + // Entries are kept sorted by Hostname, Origin, ContainerID then address + e1Hostname, e2Hostname := strings.ToLower(e1.Hostname), strings.ToLower(e2.Hostname) + switch { + case e1Hostname != e2Hostname: + return e1Hostname < e2Hostname + + case e1.Origin != e2.Origin: + return e1.Origin < e2.Origin + + case e1.ContainerID != e2.ContainerID: + return e1.ContainerID < e2.ContainerID + + default: + return e1.Addr < e2.Addr + } +} + // returns true to indicate a change func (e1 *Entry) merge(e2 *Entry) bool { // we know container id, origin, add and hostname are equal @@ -63,37 +100,37 @@ func (e1 *Entry) String() string { return fmt.Sprintf("%s -> %s", e1.Hostname, e1.Addr.String()) } -func (es Entries) Len() int { return len(es) } -func (es Entries) Swap(i, j int) { panic("Swap") } -func (es Entries) Less(i, j int) bool { return es[i].less(&es[j]) } - -func (es Entries) check() error { +func check(es SortableEntries) error { if !sort.IsSorted(es) { return fmt.Errorf("Not sorted!") } - for i := 1; i < len(es); i++ { - if es[i].equal(&es[i-1]) { - return fmt.Errorf("Duplicate entry: %d:%v and %d:%v", i-1, es[i-1], i, es[i]) + for i := 1; i < es.Len(); i++ { + if es.Get(i).equal(es.Get(i - 1)) { + return fmt.Errorf("Duplicate entry: %d:%v and %d:%v", i-1, es.Get(i-1), i, es.Get(i)) } } return nil } -func (es *Entries) checkAndPanic() { - if err := es.check(); err != nil { +func checkAndPanic(es SortableEntries) { + if err := check(es); err != nil { panic(err) } } +func (es *Entries) checkAndPanic() *Entries { + checkAndPanic(CaseInsensitive(*es)) + return es +} + func (es *Entries) add(hostname, containerid string, origin router.PeerName, addr address.Address) Entry { - es.checkAndPanic() - defer es.checkAndPanic() + defer es.checkAndPanic().checkAndPanic() entry := Entry{Hostname: hostname, Origin: origin, ContainerID: containerid, Addr: addr} i := sort.Search(len(*es), func(i int) bool { - return !(*es)[i].less(&entry) + return !(*es)[i].insensitiveLess(&entry) }) - if i < len(*es) && (*es)[i].equal(&entry) { + if i < len(*es) && (*es)[i].equal(entry) { if (*es)[i].Tombstone > 0 { (*es)[i].Tombstone = 0 (*es)[i].Version++ @@ -107,16 +144,16 @@ func (es *Entries) add(hostname, containerid string, origin router.PeerName, add } func (es *Entries) merge(incoming Entries) Entries { - es.checkAndPanic() - defer es.checkAndPanic() + defer es.checkAndPanic().checkAndPanic() + newEntries := Entries{} i := 0 for _, entry := range incoming { - for i < len(*es) && (*es)[i].less(&entry) { + for i < len(*es) && (*es)[i].insensitiveLess(&entry) { i++ } - if i < len(*es) && (*es)[i].equal(&entry) { + if i < len(*es) && (*es)[i].equal(entry) { if (*es)[i].merge(&entry) { newEntries = append(newEntries, entry) } @@ -133,8 +170,8 @@ func (es *Entries) merge(incoming Entries) Entries { // f returning true means keep the entry. func (es *Entries) tombstone(ourname router.PeerName, f func(*Entry) bool) Entries { - es.checkAndPanic() - defer es.checkAndPanic() + defer es.checkAndPanic().checkAndPanic() + tombstoned := Entries{} for i, e := range *es { if f(&e) && e.Origin == ourname { @@ -148,8 +185,8 @@ func (es *Entries) tombstone(ourname router.PeerName, f func(*Entry) bool) Entri } func (es *Entries) filter(f func(*Entry) bool) { - es.checkAndPanic() - defer es.checkAndPanic() + defer es.checkAndPanic().checkAndPanic() + i := 0 for _, e := range *es { if !f(&e) { @@ -163,15 +200,17 @@ func (es *Entries) filter(f func(*Entry) bool) { func (es Entries) lookup(hostname string) Entries { es.checkAndPanic() + + lowerHostname := strings.ToLower(hostname) i := sort.Search(len(es), func(i int) bool { - return es[i].Hostname >= hostname + return strings.ToLower(es[i].Hostname) >= lowerHostname }) - if i >= len(es) || es[i].Hostname != hostname { + if i >= len(es) || strings.ToLower(es[i].Hostname) != lowerHostname { return Entries{} } j := sort.Search(len(es)-i, func(j int) bool { - return es[i+j].Hostname > hostname + return strings.ToLower(es[i+j].Hostname) > lowerHostname }) return es[i : i+j] @@ -179,6 +218,7 @@ func (es Entries) lookup(hostname string) Entries { func (es *Entries) first(f func(*Entry) bool) (*Entry, error) { es.checkAndPanic() + for _, e := range *es { if f(&e) { return &e, nil @@ -193,6 +233,8 @@ type GossipData struct { } func (g *GossipData) Merge(o router.GossipData) { + checkAndPanic(CaseSensitive(g.Entries)) + defer func() { checkAndPanic(CaseSensitive(g.Entries)) }() other := o.(*GossipData) g.Entries.merge(other.Entries) if g.Timestamp < other.Timestamp { @@ -201,6 +243,7 @@ func (g *GossipData) Merge(o router.GossipData) { } func (g *GossipData) Encode() [][]byte { + checkAndPanic(CaseSensitive(g.Entries)) buf := &bytes.Buffer{} if err := gob.NewEncoder(buf).Encode(g); err != nil { panic(err) diff --git a/nameserver/nameserver.go b/nameserver/nameserver.go index 356e61e0f0..97797f8448 100644 --- a/nameserver/nameserver.go +++ b/nameserver/nameserver.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/gob" "fmt" + "sort" "sync" "time" @@ -198,6 +199,7 @@ func (n *Nameserver) Gossip() router.GossipData { Timestamp: now(), } copy(gossip.Entries, n.entries) + sort.Sort(CaseSensitive(gossip.Entries)) return gossip } @@ -215,9 +217,7 @@ func (n *Nameserver) receiveGossip(msg []byte) (router.GossipData, router.Gossip return nil, nil, fmt.Errorf("clock skew of %d detected", delta) } - if err := gossip.Entries.check(); err != nil { - return nil, nil, err - } + sort.Sort(CaseInsensitive(gossip.Entries)) n.Lock() defer n.Unlock() diff --git a/test/245_dns_case_insensitive_test.sh b/test/245_dns_case_insensitive_test.sh new file mode 100755 index 0000000000..be14440cc5 --- /dev/null +++ b/test/245_dns_case_insensitive_test.sh @@ -0,0 +1,31 @@ +#! /bin/bash + +. ./config.sh + +C1=10.2.0.78 +C2=10.2.0.79 +C3=10.2.0.80 + +start_suite "DNS lookup case (in)sensitivity" + +weave_on $HOST1 launch + +start_container_with_dns $HOST1 --name=test + +start_container $HOST1 $C1/24 --name=seeone +assert_dns_record $HOST1 test seeone.weave.local $C1 +assert_dns_record $HOST1 test SeeOne.weave.local $C1 +assert_dns_record $HOST1 test SEEONE.weave.local $C1 + +start_container $HOST1 $C2/24 --name=SeEtWo +assert_dns_record $HOST1 test seetwo.weave.local $C2 +assert_dns_record $HOST1 test SeeTwo.weave.local $C2 +assert_dns_record $HOST1 test SEETWO.weave.local $C2 + +start_container $HOST1 $C3/24 --name=seetwo +assert_dns_record $HOST1 test seetwo.weave.local $C2 $C3 +assert_dns_record $HOST1 test SeeTwo.weave.local $C2 $C3 +assert_dns_record $HOST1 test SEETWO.weave.local $C2 $C3 +assert "exec_on $HOST1 test dig +short seetwo.weave.local A | grep -v ';;' | wc -l" 2 + +end_suite diff --git a/test/config.sh b/test/config.sh index a26b89e3f3..d6471b2f00 100644 --- a/test/config.sh +++ b/test/config.sh @@ -188,7 +188,7 @@ assert_dns_record() { [ -z "$DEBUG" ] || greyly echo "Checking whether the IPs '$@' exists at $host:$container" for ip in "$@" ; do - assert "exec_on $host $container getent hosts $ip | tr -s ' '" "$ip $name" + assert "exec_on $host $container getent hosts $ip | tr -s ' ' | tr '[:upper:]' '[:lower:]'" "$(echo $ip $name | tr '[:upper:]' '[:lower:]')" done } From e5316221bbf443e336cbf6f9764a11ab08531689 Mon Sep 17 00:00:00 2001 From: Tom Wilkie Date: Thu, 1 Oct 2015 14:14:15 +0000 Subject: [PATCH 2/2] Review feedback --- test/245_dns_case_insensitive_test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/245_dns_case_insensitive_test.sh b/test/245_dns_case_insensitive_test.sh index be14440cc5..9d032a8998 100755 --- a/test/245_dns_case_insensitive_test.sh +++ b/test/245_dns_case_insensitive_test.sh @@ -10,7 +10,7 @@ start_suite "DNS lookup case (in)sensitivity" weave_on $HOST1 launch -start_container_with_dns $HOST1 --name=test +start_container_with_dns $HOST1 --name=test start_container $HOST1 $C1/24 --name=seeone assert_dns_record $HOST1 test seeone.weave.local $C1