From 91b78d256d5a6471a96cf4d5733c2f8f8407c742 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Fri, 12 May 2017 13:53:35 -0700 Subject: [PATCH] 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 | 25 +++++++----- command/agent/config_test.go | 40 +++++++++++++++++-- .../docs/agent/configuration/index.html.md | 4 +- 3 files changed, 53 insertions(+), 16 deletions(-) diff --git a/command/agent/config.go b/command/agent/config.go index 27930111089..3ede98b5ae0 100644 --- a/command/agent/config.go +++ b/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) diff --git a/command/agent/config_test.go b/command/agent/config_test.go index 270ffe23b62..7eb5dc53627 100644 --- a/command/agent/config_test.go +++ b/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{ diff --git a/website/source/docs/agent/configuration/index.html.md b/website/source/docs/agent/configuration/index.html.md index c9adc2c9f8c..e9ae41cb09d 100644 --- a/website/source/docs/agent/configuration/index.html.md +++ b/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