Ensure Consul is IPv6 compliant (#5468)

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",
+				Name:     "Connect Proxy Listening",
-				TCP: fmt.Sprintf("%s:%d", chkAddr,
+				TCP:      ipaddr.FormatAddressPort(chkAddr, bindPort),
-					proxyCfg["bind_port"]),
 				Interval: 10 * time.Second,
 			},
 		}

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()
+			defer a.Shutdown()
-	testrpc.WaitForTestAgent(t, a.RPC, "dc1")
+			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
+				// Note the uppercase P is important here - it ensures translation works
-		// correctly in case-insensitive way. Without it this test can pass even
+				// correctly in case-insensitive way. Without it this test can pass even
-		// when translation is broken for other valid inputs.
+				// when translation is broken for other valid inputs.
-		`"Proxy": {
+				`"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()
+			rr := httptest.NewRecorder()
-	obj, err := a.srv.AgentRegisterService(rr, req)
+			obj, err := a.srv.AgentRegisterService(rr, req)
-	require.NoError(t, err)
+			require.NoError(t, err)
-	require.Nil(t, obj)
+			require.Nil(t, obj)
-	require.Equal(t, 200, rr.Code, "body: %s", rr.Body)
+			require.Equal(t, 200, rr.Code, "body: %s", rr.Body)
 
-	svc := &structs.NodeService{
+			svc := &structs.NodeService{
-		ID:      "test",
+				ID:      "test",
-		Service: "test",
+				Service: "test",
-		Meta: map[string]string{
+				Meta: map[string]string{
-			"some":                "meta",
+					"some":                "meta",
-			"enable_tag_override": "meta is 'opaque' so should not get translated",
+					"enable_tag_override": "meta is 'opaque' so should not get translated",
-		},
+				},
-		Port:              8000,
+				Port:              8000,
-		EnableTagOverride: true,
+				EnableTagOverride: true,
-		Weights:           &structs.Weights{Passing: 16, Warning: 0},
+				Weights:           &structs.Weights{Passing: 16, Warning: 0},
-		Kind:              structs.ServiceKindConnectProxy,
+				Kind:              structs.ServiceKindConnectProxy,
-		Proxy: structs.ConnectProxyConfig{
+				Proxy: structs.ConnectProxyConfig{
-			DestinationServiceName: "web",
+					DestinationServiceName: "web",
-			DestinationServiceID:   "web",
+					DestinationServiceID:   "web",
-			LocalServiceAddress:    "127.0.0.1",
+					LocalServiceAddress:    tt.ip,
-			LocalServicePort:       1234,
+					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,
 					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,
-		Connect: structs.ServiceConnect{
+							DestinationName:      "db",
-			Proxy: &structs.ServiceDefinitionConnectProxy{
+							DestinationNamespace: "default",
-				ExecMode: "script",
+							LocalBindAddress:     tt.ip,
-				Config: map[string]interface{}{
+							LocalBindPort:        1234,
-					"destination_type": "connect.proxy.config is 'opaque' so should not get translated",
+							Config: map[string]interface{}{
-				},
+								"destination_type": "proxy.upstreams.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,
-						Config: map[string]interface{}{
-							"destination_type": "connect.proxy.upstreams.config is 'opaque' so should not get translated",
 						},
 					},
 				},
-			},
+				Connect: structs.ServiceConnect{
-			// The sidecar service is nilled since it is only config sugar and
+					Proxy: &structs.ServiceDefinitionConnectProxy{
-			// shouldn't be represented in state. We assert that the translations
+						ExecMode: "script",
-			// there worked by inspecting the registered sidecar below.
+						Config: map[string]interface{}{
-			SidecarService: nil,
+							"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,
+				},
+			}
 
-	got := a.State.Service("test")
+			got := a.State.Service("test")
-	require.Equal(t, svc, got)
+			require.Equal(t, svc, got)
 
-	sidecarSvc := &structs.NodeService{
+			sidecarSvc := &structs.NodeService{
-		Kind:    structs.ServiceKindConnectProxy,
+				Kind:    structs.ServiceKindConnectProxy,
-		ID:      "test-sidecar-proxy",
+				ID:      "test-sidecar-proxy",
-		Service: "test-proxy",
+				Service: "test-proxy",
-		Meta: map[string]string{
+				Meta: map[string]string{
-			"some":                "meta",
+					"some":                "meta",
-			"enable_tag_override": "sidecar_service.meta is 'opaque' so should not get translated",
+					"enable_tag_override": "sidecar_service.meta is 'opaque' so should not get translated",
-		},
+				},
-		Port:                       8001,
+				Port:                       8001,
-		EnableTagOverride:          true,
+				EnableTagOverride:          true,
-		Weights:                    &structs.Weights{Passing: 1, Warning: 1},
+				Weights:                    &structs.Weights{Passing: 1, Warning: 1},
-		LocallyRegisteredAsSidecar: true,
+				LocallyRegisteredAsSidecar: true,
-		Proxy: structs.ConnectProxyConfig{
+				Proxy: structs.ConnectProxyConfig{
-			DestinationServiceName: "test",
+					DestinationServiceName: "test",
-			DestinationServiceID:   "test",
+					DestinationServiceID:   "test",
-			LocalServiceAddress:    "127.0.0.1",
+					LocalServiceAddress:    tt.ip,
-			LocalServicePort:       4321,
+					LocalServicePort:       4321,
-			Upstreams: structs.Upstreams{
+					Upstreams: structs.Upstreams{
-				{
+						{
-					DestinationType:      structs.UpstreamDestTypeService,
+							DestinationType:      structs.UpstreamDestTypeService,
-					DestinationName:      "db",
+							DestinationName:      "db",
-					DestinationNamespace: "default",
+							DestinationNamespace: "default",
-					LocalBindAddress:     "127.0.0.1",
+							LocalBindAddress:     tt.ip,
-					LocalBindPort:        1234,
+							LocalBindPort:        1234,
-					Config: map[string]interface{}{
+							Config: map[string]interface{}{
-						"destination_type": "sidecar_service.proxy.upstreams.config is 'opaque' so should not get translated",
+								"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) {

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 {

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{

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()

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) {

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, "")

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 {

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 {

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)
+				}
+			}
+		})
+	}
+}