Don't autoadvertise private ip if bind=localhost

A slight improvement to #2399 - if bind is localhost, return an error
instead of advertising a private ip. The advertised ip isn't valid and
will just cause errors on use. It's better to fail with an error message
instructing users how to fix the problem.

command/agent/config.go

@@ -800,9 +800,8 @@ func parseSingleIPTemplate(ipTmpl string) (string, error) {
 func normalizeBind(addr, bind string) (string, error) {
 	if addr == "" {
 		return bind, nil
-	} else {
-		return parseSingleIPTemplate(addr)
 	}
+	return parseSingleIPTemplate(addr)
 }
 
 // normalizeAdvertise returns a normalized advertise address.
@@ -825,16 +824,17 @@ func normalizeAdvertise(addr string, bind string, defport int, dev bool) (string
 
 	if addr != "" {
 		// Default to using manually configured address
-		host, port, err := net.SplitHostPort(addr)
+		_, _, err = net.SplitHostPort(addr)
 		if err != nil {
 			if !isMissingPort(err) {
 				return "", fmt.Errorf("Error parsing advertise address %q: %v", addr, err)
 			}
-			host = addr
-			port = strconv.Itoa(defport)
+
+			// missing port, append the default
+			return net.JoinHostPort(addr, strconv.Itoa(defport)), nil
 		}
 
-		return net.JoinHostPort(host, port), nil
+		return addr, nil
 	}
 
 	// Fallback to bind address first, and then try resolving the local hostname
@@ -843,18 +843,21 @@ func normalizeAdvertise(addr string, bind string, defport int, dev bool) (string
 		return "", fmt.Errorf("Error resolving bind address %q: %v", bind, err)
 	}
 
-	// Return the first unicast address
+	// Return the first non-localhost unicast address
 	for _, ip := range ips {
 		if ip.IsLinkLocalUnicast() || ip.IsGlobalUnicast() {
 			return net.JoinHostPort(ip.String(), strconv.Itoa(defport)), nil
 		}
-		if !ip.IsLoopback() || (ip.IsLoopback() && dev) {
-			// loopback is fine for dev mode
-			return net.JoinHostPort(ip.String(), strconv.Itoa(defport)), nil
+		if ip.IsLoopback() {
+			if dev {
+				// loopback is fine for dev mode
+				return net.JoinHostPort(ip.String(), strconv.Itoa(defport)), nil
+			}
+			return "", fmt.Errorf("Defaulting advertise to localhost is unsafe, please set advertise manually")
 		}
 	}
 
-	// Otherwise, default to the private IP address
+	// Bind is not localhost but not a valid advertise IP, use first private IP
 	addr, err = parseSingleIPTemplate("{{ GetPrivateIP }}")
 	if err != nil {
 		return "", fmt.Errorf("Unable to parse default advertise address: %v", err)

command/agent/config_test.go

@@ -571,7 +571,7 @@ func TestConfig_normalizeAddrs(t *testing.T) {
 		t.Fatalf("expected unset Serf advertise address, got %s", c.AdvertiseAddrs.Serf)
 	}
 
-	// default to non-localhost address in non-dev mode
+	// fail if no valid advertise address available in non-dev mode
 	c = &Config{
 		BindAddr: "127.0.0.1",
 		Ports: &Ports{
@@ -584,8 +584,8 @@ func TestConfig_normalizeAddrs(t *testing.T) {
 		DevMode:        false,
 	}
 
-	if err := c.normalizeAddrs(); err != nil {
-		t.Fatalf("unable to normalize addresses: %s", err)
+	if err := c.normalizeAddrs(); err == nil {
+		t.Fatalf("expected an error when no valid advertise address is available")
 	}
 
 	if c.AdvertiseAddrs.HTTP == "127.0.0.1:4646" {
@@ -600,6 +600,40 @@ func TestConfig_normalizeAddrs(t *testing.T) {
 		t.Fatalf("expected non-localhost Serf advertise address, got %s", c.AdvertiseAddrs.Serf)
 	}
 
+	// allow localhost for advertise addrs without dev mode if they're excplicitly set
+	c = &Config{
+		BindAddr: "127.0.0.1",
+		Ports: &Ports{
+			HTTP: 4646,
+			RPC:  4647,
+			Serf: 4648,
+		},
+		Addresses: &Addresses{},
+		AdvertiseAddrs: &AdvertiseAddrs{
+			HTTP: "127.0.0.1",
+			RPC:  "127.0.0.1",
+			Serf: "127.0.0.1",
+		},
+		DevMode: false,
+		Server:  &ServerConfig{Enabled: true},
+	}
+
+	if err := c.normalizeAddrs(); err != nil {
+		t.Fatalf("unexpected error when manually setting bind mode: %v", err)
+	}
+
+	if c.AdvertiseAddrs.HTTP != "127.0.0.1:4646" {
+		t.Errorf("expected localhost HTTP advertise address, got %s", c.AdvertiseAddrs.HTTP)
+	}
+
+	if c.AdvertiseAddrs.RPC != "127.0.0.1:4647" {
+		t.Errorf("expected localhost RPC advertise address, got %s", c.AdvertiseAddrs.RPC)
+	}
+
+	if c.AdvertiseAddrs.Serf != "127.0.0.1:4648" {
+		t.Errorf("expected localhost Serf advertise address, got %s", c.AdvertiseAddrs.Serf)
+	}
+
 	c = &Config{
 		BindAddr: "169.254.1.5",
 		Ports: &Ports{

website/source/docs/agent/configuration/index.html.md

@@ -99,8 +99,8 @@ testing.
   configurations such as NAT. This configuration is optional, and defaults to
   the bind address of the specific network service if it is not provided. Any
   values configured in this stanza take precedence over the default
-  [bind_addr](#bind_addr). If the bind address is `0.0.0.0` then the hostname
-  is advertised. You may advertise an alternate port as well.
+  [bind_addr](#bind_addr). If the bind address is `0.0.0.0` then the first
+  private IP found is advertised. You may advertise an alternate port as well.
   The values support [go-sockaddr/template format][go-sockaddr/template].
 
   - `http` - The address to advertise for the HTTP interface. This should be