diff --git a/CHANGELOG.md b/CHANGELOG.md index 56aedfd67..2f2ddef8d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -173,3 +173,4 @@ * [BUGFIX] `MetricFamiliesPerTenant.SendMaxOfGauges` will no longer return 0 if some tenant doesn't have the gauge in their registry, and other tenants have negative values only. #368 * [BUGFIX] `MetricFamiliesPerTenant.SendMaxOfGaugesPerTenant` no longer includes tenants, which do not have specified gauge. #368 * [BUGFIX] Correctly format `host:port` addresses when using IPv6. #388 +* [BUGFIX] Memberlist's TCP transport will now reject bind addresses that are not IP addresses, such as "localhost", rather than silently converting these to 0.0.0.0 and therefore listening on all addresses. #396 diff --git a/kv/memberlist/http_status_handler_test.go b/kv/memberlist/http_status_handler_test.go index 0752e01c0..aae066d6d 100644 --- a/kv/memberlist/http_status_handler_test.go +++ b/kv/memberlist/http_status_handler_test.go @@ -11,6 +11,7 @@ import ( func TestPage(t *testing.T) { conf := memberlist.DefaultLANConfig() + conf.BindAddr = getLocalhostAddr() ml, err := memberlist.Create(conf) require.NoError(t, err) diff --git a/kv/memberlist/memberlist_client_test.go b/kv/memberlist/memberlist_client_test.go index 500c3979a..30ba6e623 100644 --- a/kv/memberlist/memberlist_client_test.go +++ b/kv/memberlist/memberlist_client_test.go @@ -234,6 +234,24 @@ func casWithErr(ctx context.Context, kv *Client, key string, updateFn func(*data return kv.CAS(ctx, key, fn) } +func getLocalhostAddr() string { + return getLocalhostAddrs()[0] +} + +var addrsOnce sync.Once +var localhostIP string + +func getLocalhostAddrs() []string { + addrsOnce.Do(func() { + ip, err := net.ResolveIPAddr("ip4", "localhost") + if err != nil { + localhostIP = "127.0.0.1" // this is the most common answer, try it + } + localhostIP = ip.String() + }) + return []string{localhostIP} +} + func TestBasicGetAndCas(t *testing.T) { c := dataCodec{} @@ -241,7 +259,7 @@ func TestBasicGetAndCas(t *testing.T) { var cfg KVConfig flagext.DefaultValues(&cfg) cfg.TCPTransport = TCPTransportConfig{ - BindAddrs: []string{"localhost"}, + BindAddrs: getLocalhostAddrs(), } cfg.Codecs = []codec.Codec{c} @@ -302,7 +320,9 @@ func withFixtures(t *testing.T, testFN func(t *testing.T, kv *Client)) { var cfg KVConfig flagext.DefaultValues(&cfg) - cfg.TCPTransport = TCPTransportConfig{} + cfg.TCPTransport = TCPTransportConfig{ + BindAddrs: getLocalhostAddrs(), + } cfg.Codecs = []codec.Codec{c} mkv := NewKV(cfg, log.NewNopLogger(), &dnsProviderMock{}, prometheus.NewPedanticRegistry()) @@ -456,6 +476,9 @@ func TestMultipleCAS(t *testing.T) { var cfg KVConfig flagext.DefaultValues(&cfg) + cfg.TCPTransport = TCPTransportConfig{ + BindAddrs: getLocalhostAddrs(), + } cfg.Codecs = []codec.Codec{c} mkv := NewKV(cfg, log.NewNopLogger(), &dnsProviderMock{}, prometheus.NewPedanticRegistry()) @@ -544,7 +567,7 @@ func defaultKVConfig(i int) KVConfig { cfg.PushPullInterval = 5 * time.Second cfg.TCPTransport = TCPTransportConfig{ - BindAddrs: []string{"localhost"}, + BindAddrs: getLocalhostAddrs(), BindPort: 0, // randomize ports } @@ -842,7 +865,7 @@ func TestJoinMembersWithRetryBackoff(t *testing.T) { cfg.AbortIfJoinFails = true cfg.TCPTransport = TCPTransportConfig{ - BindAddrs: []string{"localhost"}, + BindAddrs: getLocalhostAddrs(), BindPort: port, } @@ -927,11 +950,11 @@ func TestMemberlistFailsToJoin(t *testing.T) { cfg.AbortIfJoinFails = true cfg.TCPTransport = TCPTransportConfig{ - BindAddrs: []string{"localhost"}, + BindAddrs: getLocalhostAddrs(), BindPort: 0, } - cfg.JoinMembers = []string{net.JoinHostPort("127.0.0.1", strconv.Itoa(ports[0]))} + cfg.JoinMembers = []string{net.JoinHostPort(getLocalhostAddr(), strconv.Itoa(ports[0]))} cfg.Codecs = []codec.Codec{c} @@ -951,7 +974,7 @@ func TestMemberlistFailsToJoin(t *testing.T) { func getFreePorts(count int) ([]int, error) { var ports []int for i := 0; i < count; i++ { - addr, err := net.ResolveTCPAddr("tcp", "127.0.0.1:0") + addr, err := net.ResolveTCPAddr("tcp", net.JoinHostPort(getLocalhostAddr(), "0")) if err != nil { return nil, err } @@ -997,7 +1020,7 @@ func runClient(kv *Client, name string, ringKey string, portToConnect int, casIn // let's join the first member if portToConnect > 0 { - _, err := kv.kv.JoinMembers([]string{net.JoinHostPort("127.0.0.1", strconv.Itoa(portToConnect))}) + _, err := kv.kv.JoinMembers([]string{net.JoinHostPort(getLocalhostAddr(), strconv.Itoa(portToConnect))}) if err != nil { return fmt.Errorf("%s failed to join the cluster: %f", name, err) } @@ -1099,7 +1122,7 @@ func TestMultipleCodecs(t *testing.T) { var cfg KVConfig flagext.DefaultValues(&cfg) cfg.TCPTransport = TCPTransportConfig{ - BindAddrs: []string{"localhost"}, + BindAddrs: getLocalhostAddrs(), BindPort: 0, // randomize } @@ -1190,7 +1213,7 @@ func TestRejoin(t *testing.T) { var cfg1 KVConfig flagext.DefaultValues(&cfg1) cfg1.TCPTransport = TCPTransportConfig{ - BindAddrs: []string{"localhost"}, + BindAddrs: getLocalhostAddrs(), BindPort: ports[0], } @@ -1251,7 +1274,11 @@ func TestMessageBuffer(t *testing.T) { func TestNotifyMsgResendsOnlyChanges(t *testing.T) { codec := dataCodec{} - cfg := KVConfig{} + cfg := KVConfig{ + TCPTransport: TCPTransportConfig{ + BindAddrs: getLocalhostAddrs(), + }, + } // We will be checking for number of messages in the broadcast queue, so make sure to use known retransmit factor. cfg.RetransmitMult = 1 cfg.Codecs = append(cfg.Codecs, codec) @@ -1316,7 +1343,11 @@ func TestNotifyMsgResendsOnlyChanges(t *testing.T) { func TestSendingOldTombstoneShouldNotForwardMessage(t *testing.T) { codec := dataCodec{} - cfg := KVConfig{} + cfg := KVConfig{ + TCPTransport: TCPTransportConfig{ + BindAddrs: getLocalhostAddrs(), + }, + } // We will be checking for number of messages in the broadcast queue, so make sure to use known retransmit factor. cfg.RetransmitMult = 1 cfg.LeftIngestersTimeout = 5 * time.Minute @@ -1409,7 +1440,7 @@ func TestFastJoin(t *testing.T) { var cfg KVConfig flagext.DefaultValues(&cfg) cfg.TCPTransport = TCPTransportConfig{ - BindAddrs: []string{"localhost"}, + BindAddrs: getLocalhostAddrs(), BindPort: 0, // randomize } @@ -1459,6 +1490,9 @@ func TestDelegateMethodsDontCrashBeforeKVStarts(t *testing.T) { cfg := KVConfig{} cfg.Codecs = append(cfg.Codecs, codec) + cfg.TCPTransport = TCPTransportConfig{ + BindAddrs: getLocalhostAddrs(), + } kv := NewKV(cfg, log.NewNopLogger(), &dnsProviderMock{}, prometheus.NewPedanticRegistry()) diff --git a/kv/memberlist/tcp_transport.go b/kv/memberlist/tcp_transport.go index bd82c585a..751ad1163 100644 --- a/kv/memberlist/tcp_transport.go +++ b/kv/memberlist/tcp_transport.go @@ -39,7 +39,7 @@ const colonColon = "::" // TCPTransportConfig is a configuration structure for creating new TCPTransport. type TCPTransportConfig struct { - // BindAddrs is a list of addresses to bind to. + // BindAddrs is a list of IP addresses to bind to. BindAddrs flagext.StringSlice `yaml:"bind_addr"` // BindPort is the port to listen on, for each address above. @@ -147,6 +147,9 @@ func NewTCPTransport(config TCPTransportConfig, logger log.Logger, registerer pr port := config.BindPort for _, addr := range config.BindAddrs { ip := net.ParseIP(addr) + if ip == nil { + return nil, fmt.Errorf("could not parse bind addr %q as IP address", addr) + } tcpAddr := &net.TCPAddr{IP: ip, Port: port} diff --git a/kv/memberlist/tcp_transport_test.go b/kv/memberlist/tcp_transport_test.go index 9abad52dc..2eeafa363 100644 --- a/kv/memberlist/tcp_transport_test.go +++ b/kv/memberlist/tcp_transport_test.go @@ -20,14 +20,14 @@ func TestTCPTransport_WriteTo_ShouldNotLogAsWarningExpectedFailures(t *testing.T unexpectedLogs string }{ "should not log 'connection refused' by default": { - remoteAddr: "localhost:12345", + remoteAddr: "127.0.0.1:12345", unexpectedLogs: "connection refused", }, "should log 'connection refused' if debug log level is enabled": { setup: func(t *testing.T, cfg *TCPTransportConfig) { cfg.TransportDebug = true }, - remoteAddr: "localhost:12345", + remoteAddr: "127.0.0.1:12345", expectedLogs: "connection refused", }, } @@ -39,7 +39,7 @@ func TestTCPTransport_WriteTo_ShouldNotLogAsWarningExpectedFailures(t *testing.T cfg := TCPTransportConfig{} flagext.DefaultValues(&cfg) - cfg.BindAddrs = []string{"localhost"} + cfg.BindAddrs = []string{"127.0.0.1"} cfg.BindPort = 0 if testData.setup != nil { testData.setup(t, &cfg) @@ -69,7 +69,7 @@ func TestFinalAdvertiseAddr(t *testing.T) { }{ "should not fail with local address specified": { advertiseAddr: "127.0.0.1", - bindAddrs: []string{"localhost"}, + bindAddrs: []string{"127.0.0.1"}, bindPort: 0, }, } @@ -95,3 +95,9 @@ func TestFinalAdvertiseAddr(t *testing.T) { }) } } + +func TestNonIPsAreRejected(t *testing.T) { + cfg := TCPTransportConfig{BindAddrs: flagext.StringSlice{"localhost"}} + _, err := NewTCPTransport(cfg, nil, nil) + require.EqualError(t, err, `could not parse bind addr "localhost" as IP address`) +}