diff --git a/agent/agent.go b/agent/agent.go index 945a7b268b..b6c0286e76 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -2537,11 +2537,14 @@ func (a *Agent) addProxyLocked(proxy *structs.ConnectManagedProxy, persist, From chkAddr := a.resolveProxyCheckAddress(proxyCfg) chkTypes := []*structs.CheckType{} if chkAddr != "" { + bindPort, ok := proxyCfg["bind_port"].(int) + if !ok { + return fmt.Errorf("Cannot convert bind_port=%v to an int for creating TCP Check for address %s", proxyCfg["bind_port"], chkAddr) + } chkTypes = []*structs.CheckType{ &structs.CheckType{ - Name: "Connect Proxy Listening", - TCP: fmt.Sprintf("%s:%d", chkAddr, - proxyCfg["bind_port"]), + Name: "Connect Proxy Listening", + TCP: ipaddr.FormatAddressPort(chkAddr, bindPort), Interval: 10 * time.Second, }, } diff --git a/agent/agent_endpoint_test.go b/agent/agent_endpoint_test.go index 25bff03187..1aa8db7084 100644 --- a/agent/agent_endpoint_test.go +++ b/agent/agent_endpoint_test.go @@ -2479,17 +2479,26 @@ func TestAgent_RegisterService(t *testing.T) { func TestAgent_RegisterService_TranslateKeys(t *testing.T) { t.Parallel() - a := NewTestAgent(t, t.Name(), ` + tests := []struct { + ip string + expectedTCPCheckStart string + }{ + {"127.0.0.1", "127.0.0.1:"}, // private network address + {"::1", "[::1]:"}, // shared address space + } + for _, tt := range tests { + t.Run(tt.ip, func(t *testing.T) { + a := NewTestAgent(t, t.Name(), ` connect { proxy { allow_managed_api_registration = true } } `) - defer a.Shutdown() - testrpc.WaitForTestAgent(t, a.RPC, "dc1") + defer a.Shutdown() + testrpc.WaitForTestAgent(t, a.RPC, "dc1") - json := ` + json := ` { "name":"test", "port":8000, @@ -2499,14 +2508,14 @@ func TestAgent_RegisterService_TranslateKeys(t *testing.T) { "enable_tag_override": "meta is 'opaque' so should not get translated" }, "kind": "connect-proxy",` + - // Note the uppercase P is important here - it ensures translation works - // correctly in case-insensitive way. Without it this test can pass even - // when translation is broken for other valid inputs. - `"Proxy": { + // Note the uppercase P is important here - it ensures translation works + // correctly in case-insensitive way. Without it this test can pass even + // when translation is broken for other valid inputs. + `"Proxy": { "destination_service_name": "web", "destination_service_id": "web", "local_service_port": 1234, - "local_service_address": "127.0.0.1", + "local_service_address": "` + tt.ip + `", "config": { "destination_type": "proxy.config is 'opaque' so should not get translated" }, @@ -2515,7 +2524,7 @@ func TestAgent_RegisterService_TranslateKeys(t *testing.T) { "destination_type": "service", "destination_namespace": "default", "destination_name": "db", - "local_bind_address": "127.0.0.1", + "local_bind_address": "` + tt.ip + `", "local_bind_port": 1234, "config": { "destination_type": "proxy.upstreams.config is 'opaque' so should not get translated" @@ -2534,7 +2543,7 @@ func TestAgent_RegisterService_TranslateKeys(t *testing.T) { "destination_type": "service", "destination_namespace": "default", "destination_name": "db", - "local_bind_address": "127.0.0.1", + "local_bind_address": "` + tt.ip + `", "local_bind_port": 1234, "config": { "destination_type": "connect.proxy.upstreams.config is 'opaque' so should not get translated" @@ -2555,13 +2564,13 @@ func TestAgent_RegisterService_TranslateKeys(t *testing.T) { "destination_service_name": "test", "destination_service_id": "test", "local_service_port": 4321, - "local_service_address": "127.0.0.1", + "local_service_address": "` + tt.ip + `", "upstreams": [ { "destination_type": "service", "destination_namespace": "default", "destination_name": "db", - "local_bind_address": "127.0.0.1", + "local_bind_address": "` + tt.ip + `", "local_bind_port": 1234, "config": { "destination_type": "sidecar_service.proxy.upstreams.config is 'opaque' so should not get translated" @@ -2575,109 +2584,121 @@ func TestAgent_RegisterService_TranslateKeys(t *testing.T) { "passing": 16 } }` - req, _ := http.NewRequest("PUT", "/v1/agent/service/register", strings.NewReader(json)) + req, _ := http.NewRequest("PUT", "/v1/agent/service/register", strings.NewReader(json)) - rr := httptest.NewRecorder() - obj, err := a.srv.AgentRegisterService(rr, req) - require.NoError(t, err) - require.Nil(t, obj) - require.Equal(t, 200, rr.Code, "body: %s", rr.Body) + rr := httptest.NewRecorder() + obj, err := a.srv.AgentRegisterService(rr, req) + require.NoError(t, err) + require.Nil(t, obj) + require.Equal(t, 200, rr.Code, "body: %s", rr.Body) - svc := &structs.NodeService{ - ID: "test", - Service: "test", - Meta: map[string]string{ - "some": "meta", - "enable_tag_override": "meta is 'opaque' so should not get translated", - }, - Port: 8000, - EnableTagOverride: true, - Weights: &structs.Weights{Passing: 16, Warning: 0}, - Kind: structs.ServiceKindConnectProxy, - Proxy: structs.ConnectProxyConfig{ - DestinationServiceName: "web", - DestinationServiceID: "web", - LocalServiceAddress: "127.0.0.1", - LocalServicePort: 1234, - Config: map[string]interface{}{ - "destination_type": "proxy.config is 'opaque' so should not get translated", - }, - Upstreams: structs.Upstreams{ - { - DestinationType: structs.UpstreamDestTypeService, - DestinationName: "db", - DestinationNamespace: "default", - LocalBindAddress: "127.0.0.1", - LocalBindPort: 1234, + svc := &structs.NodeService{ + ID: "test", + Service: "test", + Meta: map[string]string{ + "some": "meta", + "enable_tag_override": "meta is 'opaque' so should not get translated", + }, + Port: 8000, + EnableTagOverride: true, + Weights: &structs.Weights{Passing: 16, Warning: 0}, + Kind: structs.ServiceKindConnectProxy, + Proxy: structs.ConnectProxyConfig{ + DestinationServiceName: "web", + DestinationServiceID: "web", + LocalServiceAddress: tt.ip, + LocalServicePort: 1234, Config: map[string]interface{}{ - "destination_type": "proxy.upstreams.config is 'opaque' so should not get translated", + "destination_type": "proxy.config is 'opaque' so should not get translated", + }, + Upstreams: structs.Upstreams{ + { + DestinationType: structs.UpstreamDestTypeService, + DestinationName: "db", + DestinationNamespace: "default", + LocalBindAddress: tt.ip, + LocalBindPort: 1234, + Config: map[string]interface{}{ + "destination_type": "proxy.upstreams.config is 'opaque' so should not get translated", + }, + }, }, }, - }, - }, - Connect: structs.ServiceConnect{ - Proxy: &structs.ServiceDefinitionConnectProxy{ - ExecMode: "script", - Config: map[string]interface{}{ - "destination_type": "connect.proxy.config is 'opaque' so should not get translated", - }, - Upstreams: structs.Upstreams{ - { - DestinationType: structs.UpstreamDestTypeService, - DestinationName: "db", - DestinationNamespace: "default", - LocalBindAddress: "127.0.0.1", - LocalBindPort: 1234, + Connect: structs.ServiceConnect{ + Proxy: &structs.ServiceDefinitionConnectProxy{ + ExecMode: "script", Config: map[string]interface{}{ - "destination_type": "connect.proxy.upstreams.config is 'opaque' so should not get translated", + "destination_type": "connect.proxy.config is 'opaque' so should not get translated", + }, + Upstreams: structs.Upstreams{ + { + DestinationType: structs.UpstreamDestTypeService, + DestinationName: "db", + DestinationNamespace: "default", + LocalBindAddress: tt.ip, + LocalBindPort: 1234, + Config: map[string]interface{}{ + "destination_type": "connect.proxy.upstreams.config is 'opaque' so should not get translated", + }, + }, }, }, + // The sidecar service is nilled since it is only config sugar and + // shouldn't be represented in state. We assert that the translations + // there worked by inspecting the registered sidecar below. + SidecarService: nil, }, - }, - // The sidecar service is nilled since it is only config sugar and - // shouldn't be represented in state. We assert that the translations - // there worked by inspecting the registered sidecar below. - SidecarService: nil, - }, - } + } - got := a.State.Service("test") - require.Equal(t, svc, got) + got := a.State.Service("test") + require.Equal(t, svc, got) - sidecarSvc := &structs.NodeService{ - Kind: structs.ServiceKindConnectProxy, - ID: "test-sidecar-proxy", - Service: "test-proxy", - Meta: map[string]string{ - "some": "meta", - "enable_tag_override": "sidecar_service.meta is 'opaque' so should not get translated", - }, - Port: 8001, - EnableTagOverride: true, - Weights: &structs.Weights{Passing: 1, Warning: 1}, - LocallyRegisteredAsSidecar: true, - Proxy: structs.ConnectProxyConfig{ - DestinationServiceName: "test", - DestinationServiceID: "test", - LocalServiceAddress: "127.0.0.1", - LocalServicePort: 4321, - Upstreams: structs.Upstreams{ - { - DestinationType: structs.UpstreamDestTypeService, - DestinationName: "db", - DestinationNamespace: "default", - LocalBindAddress: "127.0.0.1", - LocalBindPort: 1234, - Config: map[string]interface{}{ - "destination_type": "sidecar_service.proxy.upstreams.config is 'opaque' so should not get translated", + sidecarSvc := &structs.NodeService{ + Kind: structs.ServiceKindConnectProxy, + ID: "test-sidecar-proxy", + Service: "test-proxy", + Meta: map[string]string{ + "some": "meta", + "enable_tag_override": "sidecar_service.meta is 'opaque' so should not get translated", + }, + Port: 8001, + EnableTagOverride: true, + Weights: &structs.Weights{Passing: 1, Warning: 1}, + LocallyRegisteredAsSidecar: true, + Proxy: structs.ConnectProxyConfig{ + DestinationServiceName: "test", + DestinationServiceID: "test", + LocalServiceAddress: tt.ip, + LocalServicePort: 4321, + Upstreams: structs.Upstreams{ + { + DestinationType: structs.UpstreamDestTypeService, + DestinationName: "db", + DestinationNamespace: "default", + LocalBindAddress: tt.ip, + LocalBindPort: 1234, + Config: map[string]interface{}{ + "destination_type": "sidecar_service.proxy.upstreams.config is 'opaque' so should not get translated", + }, + }, }, }, - }, - }, + } + gotSidecar := a.State.Service("test-sidecar-proxy") + hasNoCorrectTCPCheck := true + for _, v := range a.checkTCPs { + if strings.HasPrefix(v.TCP, tt.expectedTCPCheckStart) { + hasNoCorrectTCPCheck = false + break + } + fmt.Println("TCP Check:= ", v) + } + if hasNoCorrectTCPCheck { + t.Fatalf("Did not find the expected TCP Healtcheck '%s' in %#v ", tt.expectedTCPCheckStart, a.checkTCPs) + } + require.Equal(t, sidecarSvc, gotSidecar) + }) } - - gotSidecar := a.State.Service("test-sidecar-proxy") - require.Equal(t, sidecarSvc, gotSidecar) } func TestAgent_RegisterService_ACLDeny(t *testing.T) { diff --git a/agent/dns.go b/agent/dns.go index 9306d5730d..a4bffa18ec 100644 --- a/agent/dns.go +++ b/agent/dns.go @@ -19,6 +19,7 @@ import ( "github.com/hashicorp/consul/agent/consul" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/api" + "github.com/hashicorp/consul/ipaddr" "github.com/hashicorp/consul/lib" "github.com/miekg/dns" ) @@ -264,7 +265,7 @@ func recursorAddr(recursor string) (string, error) { START: _, _, err := net.SplitHostPort(recursor) if ae, ok := err.(*net.AddrError); ok && ae.Err == "missing port in address" { - recursor = fmt.Sprintf("%s:%d", recursor, 53) + recursor = ipaddr.FormatAddressPort(recursor, 53) goto START } if err != nil { diff --git a/agent/sidecar_service.go b/agent/sidecar_service.go index 75782c334a..83d6c1d5e6 100644 --- a/agent/sidecar_service.go +++ b/agent/sidecar_service.go @@ -4,6 +4,8 @@ import ( "fmt" "time" + "github.com/hashicorp/consul/ipaddr" + "github.com/hashicorp/consul/agent/structs" ) @@ -171,7 +173,7 @@ func (a *Agent) sidecarServiceFromNodeService(ns *structs.NodeService, token str Name: "Connect Sidecar Listening", // Default to localhost rather than agent/service public IP. The checks // can always be overridden if a non-loopback IP is needed. - TCP: fmt.Sprintf("127.0.0.1:%d", sidecar.Port), + TCP: ipaddr.FormatAddressPort(sidecar.Proxy.LocalServiceAddress, sidecar.Port), Interval: 10 * time.Second, }, &structs.CheckType{ diff --git a/connect/proxy/config.go b/connect/proxy/config.go index 22df11bad5..3678f7d3db 100644 --- a/connect/proxy/config.go +++ b/connect/proxy/config.go @@ -10,6 +10,7 @@ import ( "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/api/watch" "github.com/hashicorp/consul/connect" + "github.com/hashicorp/consul/ipaddr" "github.com/hashicorp/consul/lib" ) @@ -242,7 +243,7 @@ func (w *AgentConfigWatcher) handler(blockVal watch.BlockingParamVal, } cfg.PublicListener.BindAddress = resp.Address cfg.PublicListener.BindPort = resp.Port - cfg.PublicListener.LocalServiceAddress = fmt.Sprintf("%s:%d", + cfg.PublicListener.LocalServiceAddress = ipaddr.FormatAddressPort( resp.Proxy.LocalServiceAddress, resp.Proxy.LocalServicePort) cfg.PublicListener.applyDefaults() diff --git a/connect/proxy/listener.go b/connect/proxy/listener.go index e0f20c8097..7077e93932 100644 --- a/connect/proxy/listener.go +++ b/connect/proxy/listener.go @@ -4,7 +4,6 @@ import ( "context" "crypto/tls" "errors" - "fmt" "log" "net" "sync" @@ -14,6 +13,7 @@ import ( metrics "github.com/armon/go-metrics" "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/connect" + "github.com/hashicorp/consul/ipaddr" ) const ( @@ -58,7 +58,7 @@ type Listener struct { // connections and proxy them to the configured local application over TCP. func NewPublicListener(svc *connect.Service, cfg PublicListenerConfig, logger *log.Logger) *Listener { - bindAddr := fmt.Sprintf("%s:%d", cfg.BindAddress, cfg.BindPort) + bindAddr := ipaddr.FormatAddressPort(cfg.BindAddress, cfg.BindPort) return &Listener{ Service: svc, listenFunc: func() (net.Listener, error) { @@ -96,7 +96,7 @@ func NewUpstreamListener(svc *connect.Service, client *api.Client, func newUpstreamListenerWithResolver(svc *connect.Service, cfg UpstreamConfig, resolverFunc func(UpstreamConfig) (connect.Resolver, error), logger *log.Logger) *Listener { - bindAddr := fmt.Sprintf("%s:%d", cfg.LocalBindAddress, cfg.LocalBindPort) + bindAddr := ipaddr.FormatAddressPort(cfg.LocalBindAddress, cfg.LocalBindPort) return &Listener{ Service: svc, listenFunc: func() (net.Listener, error) { diff --git a/connect/proxy/listener_test.go b/connect/proxy/listener_test.go index f893a0575a..3e7a683ae4 100644 --- a/connect/proxy/listener_test.go +++ b/connect/proxy/listener_test.go @@ -16,6 +16,7 @@ import ( agConnect "github.com/hashicorp/consul/agent/connect" "github.com/hashicorp/consul/connect" + "github.com/hashicorp/consul/ipaddr" "github.com/hashicorp/consul/sdk/freeport" ) @@ -204,7 +205,7 @@ func TestUpstreamListener(t *testing.T) { // Proxy and fake remote service are running, play the part of the app // connecting to a remote connect service over TCP. conn, err := net.Dial("tcp", - fmt.Sprintf("%s:%d", cfg.LocalBindAddress, cfg.LocalBindPort)) + ipaddr.FormatAddressPort(cfg.LocalBindAddress, cfg.LocalBindPort)) require.NoError(t, err) TestEchoConn(t, conn, "") diff --git a/connect/resolver.go b/connect/resolver.go index 90d58d669f..054ebe70d3 100644 --- a/connect/resolver.go +++ b/connect/resolver.go @@ -8,6 +8,7 @@ import ( "github.com/hashicorp/consul/agent/connect" "github.com/hashicorp/consul/api" + "github.com/hashicorp/consul/ipaddr" ) // Resolver is the interface implemented by a service discovery mechanism to get @@ -156,7 +157,7 @@ func (cr *ConsulResolver) resolveServiceEntry(entry *api.ServiceEntry) (string, Service: service, } - return fmt.Sprintf("%s:%d", addr, port), certURI, nil + return ipaddr.FormatAddressPort(addr, port), certURI, nil } func (cr *ConsulResolver) queryOptions(ctx context.Context) *api.QueryOptions { diff --git a/ipaddr/ipaddr.go b/ipaddr/ipaddr.go index 6755682cd3..321c8d9b1d 100644 --- a/ipaddr/ipaddr.go +++ b/ipaddr/ipaddr.go @@ -4,8 +4,14 @@ import ( "fmt" "net" "reflect" + "strconv" ) +// FormatAddressPort Helper for net.JoinHostPort that takes int for port +func FormatAddressPort(address string, port int) string { + return net.JoinHostPort(address, strconv.Itoa(port)) +} + // IsAny checks if the given ip address is an IPv4 or IPv6 ANY address. ip // can be either a *net.IP or a string. It panics on another type. func IsAny(ip interface{}) bool { diff --git a/ipaddr/ipaddr_test.go b/ipaddr/ipaddr_test.go new file mode 100644 index 0000000000..e08bfe266e --- /dev/null +++ b/ipaddr/ipaddr_test.go @@ -0,0 +1,56 @@ +package ipaddr + +import ( + "fmt" + "testing" +) + +func TestIsIPv6(t *testing.T) { + tests := []struct { + ip string + ipv6 bool + }{ + // IPv4 private addresses + {"10.0.0.1", false}, // private network address + {"100.64.0.1", false}, // shared address space + {"172.16.0.1", false}, // private network address + {"192.168.0.1", false}, // private network address + {"192.0.0.1", false}, // IANA address + {"192.0.2.1", false}, // documentation address + {"127.0.0.1", false}, // loopback address + {"169.254.0.1", false}, // link local address + + // IPv4 public addresses + {"1.2.3.4", false}, + + // IPv6 private addresses + {"::1", true}, // loopback address + {"fe80::1", true}, // link local address + {"fc00::1", true}, // unique local address + {"fec0::1", true}, // site local address + {"2001:db8::1", true}, // documentation address + + // IPv6 public addresses + {"2004:db6::1", true}, + + // hostname + {"example.com", false}, + {"localhost", false}, + {"1.257.0.1", false}, + } + for _, tt := range tests { + t.Run(tt.ip, func(t *testing.T) { + port := 1234 + formated := FormatAddressPort(tt.ip, port) + if tt.ipv6 { + if fmt.Sprintf("[%s]:%d", tt.ip, port) != formated { + t.Fatalf("Wrong format %s for %s", formated, tt.ip) + } + } else { + if fmt.Sprintf("%s:%d", tt.ip, port) != formated { + t.Fatalf("Wrong format %s for %s", formated, tt.ip) + } + } + }) + } +}