Fix memcached shards being inconsistent when looking up hosts by SRV (e…

…nvoyproxy#316)

SRV records return hosts in an arbitrary order, but the memcached
library in use relies on the order of the hosts for sharding (i.e.
the i-th host passed to the client will host shard i). If multiple
ratelimitinstances looked up the same set of memcached hosts via
SRV they could recieve them in different orders and begin sharding
descriptor keys to separate hosts, leading to non-global rate
limiting (as the sum of hits would be split accross hosts, meaning
each ratelimit instance would have an incomplete total of hits).

This fixes the issue by sorting the hosts returned by the SRV
lexicographically. Both weight and priority are ignored; weight
was not previously handled and priority is not possible to handle
(as the memcached library cannot take a priority into account when
determining shards).

Signed-off-by: Peter Marsh <[email protected]>

src/srv/srv.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"net"
 	"regexp"
+	"sort"
 
 	logger "github.com/sirupsen/logrus"
 )
@@ -17,6 +18,8 @@ type SrvResolver interface {
 
 type DnsSrvResolver struct{}
 
+type addrsLookup func(service, proto, name string) (cname string, addrs []*net.SRV, err error)
+
 func ParseSrv(srv string) (string, string, string, error) {
 	matches := srvRegex.FindStringSubmatch(srv)
 	if matches == nil {
@@ -28,13 +31,17 @@ func ParseSrv(srv string) (string, string, string, error) {
 }
 
 func (dnsSrvResolver DnsSrvResolver) ServerStringsFromSrv(srv string) ([]string, error) {
+	return lookupServerStringsFromSrv(srv, net.LookupSRV)
+}
+
+func lookupServerStringsFromSrv(srv string, addrsLookup addrsLookup) ([]string, error) {
 	service, proto, name, err := ParseSrv(srv)
 	if err != nil {
 		logger.Errorf("failed to parse SRV: %s", err)
 		return nil, err
 	}
 
-	_, srvs, err := net.LookupSRV(service, proto, name)
+	_, srvs, err := addrsLookup(service, proto, name)
 	if err != nil {
 		logger.Errorf("failed to lookup SRV: %s", err)
 		return nil, err
@@ -49,5 +56,9 @@ func (dnsSrvResolver DnsSrvResolver) ServerStringsFromSrv(srv string) ([]string,
 		serversFromSrv[i] = fmt.Sprintf("%s:%v", srv.Target, srv.Port)
 	}
 
+	// we sort the server strings (host:port) to make sure we ge a consistent order as
+	// bradfitz/gomemcache uses assigns shards based on order of given hosts
+	sort.Strings(serversFromSrv)
+
 	return serversFromSrv, nil
 }

src/srv/srv_test.go

@@ -0,0 +1,18 @@
+package srv
+
+import (
+	"net"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func mockAddrsLookup(service, proto, name string) (cname string, addrs []*net.SRV, err error) {
+	return "ignored", []*net.SRV{{"z", 1, 0, 0}, {"z", 0, 0, 0}, {"a", 9001, 0, 0}}, nil
+}
+
+func TestLookupServerStringsFromSrvReturnsServersSorted(t *testing.T) {
+	targets, err := lookupServerStringsFromSrv("_something._tcp.example.org.", mockAddrsLookup)
+	assert.Nil(t, err)
+	assert.Equal(t, targets, []string{"a:9001", "z:0", "z:1"})
+}