From 4e7587de17da119adf1e1f5371433c3296fb3074 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Fri, 4 Nov 2016 15:24:28 -0700 Subject: [PATCH 01/15] Choose safer default advertise address * -dev mode defaults bind & advertise to localhost * Normal mode defaults bind to 0.0.0.0 & advertise to the resolved hostname. If the hostname resolves to localhost it will refuse to start and advertise must be manually set. --- command/agent/agent.go | 164 ++++++--------------------------------- command/agent/command.go | 74 +++++++++++++++++- command/agent/config.go | 3 +- command/agent/http.go | 2 +- 4 files changed, 97 insertions(+), 146 deletions(-) diff --git a/command/agent/agent.go b/command/agent/agent.go index 1b5c19e8cb6..4d1da5e1a87 100644 --- a/command/agent/agent.go +++ b/command/agent/agent.go @@ -11,7 +11,6 @@ import ( "strconv" "strings" "sync" - "sync/atomic" "time" "github.com/hashicorp/nomad/client" @@ -45,13 +44,9 @@ type Agent struct { // consulSyncer registers the Nomad agent with the Consul Agent consulSyncer *consul.Syncer - client *client.Client - clientHTTPAddr string + client *client.Client - server *nomad.Server - serverHTTPAddr string - serverRPCAddr string - serverSerfAddr string + server *nomad.Server shutdown bool shutdownCh chan struct{} @@ -115,7 +110,7 @@ func (a *Agent) serverConfig() (*nomad.Config, error) { if a.config.Server.BootstrapExpect == 1 { conf.Bootstrap = true } else { - atomic.StoreInt32(&conf.BootstrapExpect, int32(a.config.Server.BootstrapExpect)) + conf.BootstrapExpect = int32(a.config.Server.BootstrapExpect) } } if a.config.DataDir != "" { @@ -135,11 +130,11 @@ func (a *Agent) serverConfig() (*nomad.Config, error) { } // Set up the bind addresses - rpcAddr, err := a.getRPCAddr(true) + rpcAddr, err := net.ResolveTCPAddr("tcp", a.config.Addresses.RPC) if err != nil { return nil, err } - serfAddr, err := a.getSerfAddr(true) + serfAddr, err := net.ResolveTCPAddr("tcp", a.config.Addresses.Serf) if err != nil { return nil, err } @@ -149,21 +144,14 @@ func (a *Agent) serverConfig() (*nomad.Config, error) { conf.SerfConfig.MemberlistConfig.BindAddr = serfAddr.IP.String() // Set up the advertise addresses - httpAddr, err := a.getHTTPAddr(false) + rpcAddr, err = net.ResolveTCPAddr("tcp", a.config.AdvertiseAddrs.RPC) if err != nil { return nil, err } - rpcAddr, err = a.getRPCAddr(false) + serfAddr, err = net.ResolveTCPAddr("tcp", a.config.AdvertiseAddrs.Serf) if err != nil { return nil, err } - serfAddr, err = a.getSerfAddr(false) - if err != nil { - return nil, err - } - a.serverHTTPAddr = net.JoinHostPort(httpAddr.IP.String(), strconv.Itoa(httpAddr.Port)) - a.serverRPCAddr = net.JoinHostPort(rpcAddr.IP.String(), strconv.Itoa(rpcAddr.Port)) - a.serverSerfAddr = net.JoinHostPort(serfAddr.IP.String(), strconv.Itoa(serfAddr.Port)) conf.RPCAdvertise = rpcAddr conf.SerfConfig.MemberlistConfig.AdvertiseAddr = serfAddr.IP.String() conf.SerfConfig.MemberlistConfig.AdvertisePort = serfAddr.Port @@ -267,12 +255,7 @@ func (a *Agent) clientConfig() (*clientconfig.Config, error) { conf.Node.NodeClass = a.config.Client.NodeClass // Set up the HTTP advertise address - httpAddr, err := a.selectAddr(a.getHTTPAddr, false) - if err != nil { - return nil, err - } - conf.Node.HTTPAddr = httpAddr - a.clientHTTPAddr = httpAddr + conf.Node.HTTPAddr = a.config.Addresses.HTTP // Reserve resources on the node. r := conf.Node.Reserved @@ -330,18 +313,14 @@ func (a *Agent) setupServer() error { } a.server = server - // Resolve consul check addresses. Always use advertise address for services - httpCheckAddr, err := a.selectAddr(a.getHTTPAddr, !a.config.Consul.ChecksUseAdvertise) - if err != nil { - return err - } - rpcCheckAddr, err := a.selectAddr(a.getRPCAddr, !a.config.Consul.ChecksUseAdvertise) - if err != nil { - return err - } - serfCheckAddr, err := a.selectAddr(a.getSerfAddr, !a.config.Consul.ChecksUseAdvertise) - if err != nil { - return err + // Consul check addresses default to bind but can be toggled to use advertise + httpCheckAddr := a.config.Addresses.HTTP + rpcCheckAddr := a.config.Addresses.RPC + serfCheckAddr := a.config.Addresses.Serf + if a.config.Consul.ChecksUseAdvertise { + httpCheckAddr = a.config.AdvertiseAddrs.HTTP + rpcCheckAddr = a.config.AdvertiseAddrs.RPC + serfCheckAddr = a.config.AdvertiseAddrs.Serf } // Create the Nomad Server services for Consul @@ -349,7 +328,7 @@ func (a *Agent) setupServer() error { if a.config.Consul.AutoAdvertise { httpServ := &structs.Service{ Name: a.config.Consul.ServerServiceName, - PortLabel: a.serverHTTPAddr, + PortLabel: a.config.AdvertiseAddrs.HTTP, Tags: []string{consul.ServiceTagHTTP}, Checks: []*structs.ServiceCheck{ &structs.ServiceCheck{ @@ -365,7 +344,7 @@ func (a *Agent) setupServer() error { } rpcServ := &structs.Service{ Name: a.config.Consul.ServerServiceName, - PortLabel: a.serverRPCAddr, + PortLabel: a.config.AdvertiseAddrs.RPC, Tags: []string{consul.ServiceTagRPC}, Checks: []*structs.ServiceCheck{ &structs.ServiceCheck{ @@ -378,8 +357,8 @@ func (a *Agent) setupServer() error { }, } serfServ := &structs.Service{ - PortLabel: a.serverSerfAddr, Name: a.config.Consul.ServerServiceName, + PortLabel: a.config.AdvertiseAddrs.Serf, Tags: []string{consul.ServiceTagSerf}, Checks: []*structs.ServiceCheck{ &structs.ServiceCheck{ @@ -458,9 +437,9 @@ func (a *Agent) setupClient() error { a.client = client // Resolve the http check address - httpCheckAddr, err := a.selectAddr(a.getHTTPAddr, !a.config.Consul.ChecksUseAdvertise) - if err != nil { - return err + httpCheckAddr := a.config.Addresses.HTTP + if a.config.Consul.ChecksUseAdvertise { + httpCheckAddr = a.config.AdvertiseAddrs.HTTP } // Create the Nomad Client services for Consul @@ -469,7 +448,7 @@ func (a *Agent) setupClient() error { if a.config.Consul.AutoAdvertise { httpServ := &structs.Service{ Name: a.config.Consul.ClientServiceName, - PortLabel: a.clientHTTPAddr, + PortLabel: a.config.AdvertiseAddrs.HTTP, Tags: []string{consul.ServiceTagHTTP}, Checks: []*structs.ServiceCheck{ &structs.ServiceCheck{ @@ -493,103 +472,6 @@ func (a *Agent) setupClient() error { return nil } -// Defines the selector interface -type addrSelector func(bool) (*net.TCPAddr, error) - -// selectAddr returns the right address given a selector, and return it as a PortLabel -// preferBind is a weak preference, and will skip 0.0.0.0 -func (a *Agent) selectAddr(selector addrSelector, preferBind bool) (string, error) { - addr, err := selector(preferBind) - if err != nil { - return "", err - } - - if preferBind && addr.IP.String() == "0.0.0.0" { - addr, err = selector(false) - if err != nil { - return "", err - } - } - - address := net.JoinHostPort(addr.IP.String(), strconv.Itoa(addr.Port)) - return address, nil -} - -// getHTTPAddr returns the HTTP address to use based on the clients -// configuration. If bind is true, an address appropriate for binding is -// returned, otherwise an address for advertising is returned. Skip 0.0.0.0 -// unless returning a bind address, since that's the only time it's useful. -func (a *Agent) getHTTPAddr(bind bool) (*net.TCPAddr, error) { - advertAddr := a.config.AdvertiseAddrs.HTTP - bindAddr := a.config.Addresses.HTTP - globalBindAddr := a.config.BindAddr - port := a.config.Ports.HTTP - return pickAddress(bind, globalBindAddr, advertAddr, bindAddr, port, "HTTP") -} - -// getRPCAddr returns the HTTP address to use based on the clients -// configuration. If bind is true, an address appropriate for binding is -// returned, otherwise an address for advertising is returned. Skip 0.0.0.0 -// unless returning a bind address, since that's the only time it's useful. -func (a *Agent) getRPCAddr(bind bool) (*net.TCPAddr, error) { - advertAddr := a.config.AdvertiseAddrs.RPC - bindAddr := a.config.Addresses.RPC - globalBindAddr := a.config.BindAddr - port := a.config.Ports.RPC - return pickAddress(bind, globalBindAddr, advertAddr, bindAddr, port, "RPC") -} - -// getSerfAddr returns the Serf address to use based on the clients -// configuration. If bind is true, an address appropriate for binding is -// returned, otherwise an address for advertising is returned. Skip 0.0.0.0 -// unless returning a bind address, since that's the only time it's useful. -func (a *Agent) getSerfAddr(bind bool) (*net.TCPAddr, error) { - advertAddr := a.config.AdvertiseAddrs.Serf - bindAddr := a.config.Addresses.Serf - globalBindAddr := a.config.BindAddr - port := a.config.Ports.Serf - return pickAddress(bind, globalBindAddr, advertAddr, bindAddr, port, "Serf") -} - -// pickAddress is a shared helper to pick the address to either bind to or -// advertise. -func pickAddress(bind bool, globalBindAddr, advertiseAddr, bindAddr string, port int, service string) (*net.TCPAddr, error) { - var serverAddr string - if advertiseAddr != "" && !bind { - serverAddr = advertiseAddr - - // Check if the advertise has a port - if host, pport, err := net.SplitHostPort(advertiseAddr); err == nil { - if parsed, err := strconv.Atoi(pport); err == nil { - serverAddr = host - port = parsed - } - } - } else if bindAddr != "" && !(bindAddr == "0.0.0.0" && !bind) { - serverAddr = bindAddr - } else if globalBindAddr != "" && !(globalBindAddr == "0.0.0.0" && !bind) { - serverAddr = globalBindAddr - } else { - serverAddr = "127.0.0.1" - } - - ip := net.ParseIP(serverAddr) - if ip == nil { - joined := net.JoinHostPort(serverAddr, strconv.Itoa(port)) - addr, err := net.ResolveTCPAddr("tcp", joined) - if err == nil { - return addr, nil - } - - return nil, fmt.Errorf("Failed to parse %s %q as IP and failed to resolve address: %v", service, serverAddr, err) - } - - return &net.TCPAddr{ - IP: ip, - Port: port, - }, nil -} - // reservePortsForClient reserves a range of ports for the client to use when // it creates various plugins for log collection, executors, drivers, etc func (a *Agent) reservePortsForClient(conf *clientconfig.Config) error { diff --git a/command/agent/command.go b/command/agent/command.go index 9b1348c94f4..dff1adfc28b 100644 --- a/command/agent/command.go +++ b/command/agent/command.go @@ -5,6 +5,7 @@ import ( "fmt" "io" "log" + "net" "os" "os/signal" "path/filepath" @@ -201,9 +202,71 @@ func (c *Command) readConfig() *Config { config.Version = c.Version config.VersionPrerelease = c.VersionPrerelease - if dev { - // Skip validation for dev mode - return config + // Normalize addresses + if config.Addresses.HTTP == "" { + config.Addresses.HTTP = fmt.Sprintf("%s:%d", config.BindAddr, config.Ports.HTTP) + } + if config.Addresses.RPC == "" { + config.Addresses.RPC = fmt.Sprintf("%s:%d", config.BindAddr, config.Ports.RPC) + } + if config.Addresses.Serf == "" { + config.Addresses.Serf = fmt.Sprintf("%s:%d", config.BindAddr, config.Ports.Serf) + } + + // Use getAdvertise(&config.AdvertiseAddrs., ) to + // retrieve a default address for advertising if one isn't set. + defaultAdvertise := "" + getAdvertise := func(addr *string, port int) bool { + if *addr != "" { + // Default to using manually configured address + return true + } + + // Autoconfigure using previously set default + if defaultAdvertise != "" { + newaddr := fmt.Sprintf("%s:%d", defaultAdvertise, port) + *addr = newaddr + return true + } + + // Fallback to Bind address if it's not 0.0.0.0 + if config.BindAddr != "0.0.0.0" { + defaultAdvertise = config.BindAddr + newaddr := fmt.Sprintf("%s:%d", defaultAdvertise, port) + *addr = newaddr + return true + } + + // As a last resort resolve the hostname and use it if it's not + // localhost (as localhost is never a sensible default) + host, err := os.Hostname() + if err != nil { + c.Ui.Error(fmt.Sprintf("Unable to get hostname to set advertise address: %v", err)) + return false + } + ip, err := net.ResolveIPAddr("ip", host) + if err != nil { + c.Ui.Error(fmt.Sprintf("Unable to resolve hostname to set advertise address: %v", err)) + return false + } + if ip.IP.IsLoopback() && !dev { + c.Ui.Error("Unable to select default advertise address as hostname resolves to localhost") + return false + } + defaultAdvertise = ip.IP.String() + newaddr := fmt.Sprintf("%s:%d", defaultAdvertise, port) + *addr = newaddr + return true + } + + if ok := getAdvertise(&config.AdvertiseAddrs.HTTP, config.Ports.HTTP); !ok { + return nil + } + if ok := getAdvertise(&config.AdvertiseAddrs.RPC, config.Ports.RPC); !ok { + return nil + } + if ok := getAdvertise(&config.AdvertiseAddrs.Serf, config.Ports.Serf); !ok { + return nil } if config.Server.EncryptKey != "" { @@ -217,6 +280,11 @@ func (c *Command) readConfig() *Config { } } + if dev { + // Skip validation for dev mode + return config + } + // Parse the RetryInterval. dur, err := time.ParseDuration(config.Server.RetryInterval) if err != nil { diff --git a/command/agent/config.go b/command/agent/config.go index b9878469055..0a31e93bb8c 100644 --- a/command/agent/config.go +++ b/command/agent/config.go @@ -431,6 +431,7 @@ func (r *Resources) ParseReserved() error { // DevConfig is a Config that is used for dev mode of Nomad. func DevConfig() *Config { conf := DefaultConfig() + conf.BindAddr = "127.0.0.1" conf.LogLevel = "DEBUG" conf.Client.Enabled = true conf.Server.Enabled = true @@ -459,7 +460,7 @@ func DefaultConfig() *Config { LogLevel: "INFO", Region: "global", Datacenter: "dc1", - BindAddr: "127.0.0.1", + BindAddr: "0.0.0.0", Ports: &Ports{ HTTP: 4646, RPC: 4647, diff --git a/command/agent/http.go b/command/agent/http.go index 2d0c0ed2d7b..fe3c417cfec 100644 --- a/command/agent/http.go +++ b/command/agent/http.go @@ -49,7 +49,7 @@ type HTTPServer struct { // NewHTTPServer starts new HTTP server over the agent func NewHTTPServer(agent *Agent, config *Config, logOutput io.Writer) (*HTTPServer, error) { // Start the listener - lnAddr, err := agent.getHTTPAddr(true) + lnAddr, err := net.ResolveTCPAddr("tcp", config.Addresses.HTTP) if err != nil { return nil, err } From 3dab50318ef975b0df8e780b9e4d83ae64d8b10d Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Mon, 7 Nov 2016 17:25:34 -0800 Subject: [PATCH 02/15] Failure to resolve advertise address isn't fatal --- command/agent/command.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/command/agent/command.go b/command/agent/command.go index dff1adfc28b..0321b4b0136 100644 --- a/command/agent/command.go +++ b/command/agent/command.go @@ -246,8 +246,12 @@ func (c *Command) readConfig() *Config { } ip, err := net.ResolveIPAddr("ip", host) if err != nil { - c.Ui.Error(fmt.Sprintf("Unable to resolve hostname to set advertise address: %v", err)) - return false + // Just because this node can't resolve its advertise + // address doesn't mean it's a bad address; use it + defaultAdvertise = host + newaddr := fmt.Sprintf("%s:%d", defaultAdvertise, port) + *addr = newaddr + return true } if ip.IP.IsLoopback() && !dev { c.Ui.Error("Unable to select default advertise address as hostname resolves to localhost") From 2c4b19f1a64e3316e45390fca26270cc3d855476 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Tue, 8 Nov 2016 13:06:56 -0800 Subject: [PATCH 03/15] Refactor getAdvertise into a func --- command/agent/command.go | 104 ++++++++++++++++++--------------------- 1 file changed, 48 insertions(+), 56 deletions(-) diff --git a/command/agent/command.go b/command/agent/command.go index 0321b4b0136..bed19ab2292 100644 --- a/command/agent/command.go +++ b/command/agent/command.go @@ -203,73 +203,27 @@ func (c *Command) readConfig() *Config { config.VersionPrerelease = c.VersionPrerelease // Normalize addresses + bind := config.BindAddr if config.Addresses.HTTP == "" { - config.Addresses.HTTP = fmt.Sprintf("%s:%d", config.BindAddr, config.Ports.HTTP) + config.Addresses.HTTP = fmt.Sprintf("%s:%d", bind, config.Ports.HTTP) } if config.Addresses.RPC == "" { - config.Addresses.RPC = fmt.Sprintf("%s:%d", config.BindAddr, config.Ports.RPC) + config.Addresses.RPC = fmt.Sprintf("%s:%d", bind, config.Ports.RPC) } if config.Addresses.Serf == "" { - config.Addresses.Serf = fmt.Sprintf("%s:%d", config.BindAddr, config.Ports.Serf) + config.Addresses.Serf = fmt.Sprintf("%s:%d", bind, config.Ports.Serf) } - // Use getAdvertise(&config.AdvertiseAddrs., ) to - // retrieve a default address for advertising if one isn't set. - defaultAdvertise := "" - getAdvertise := func(addr *string, port int) bool { - if *addr != "" { - // Default to using manually configured address - return true - } - - // Autoconfigure using previously set default - if defaultAdvertise != "" { - newaddr := fmt.Sprintf("%s:%d", defaultAdvertise, port) - *addr = newaddr - return true - } - - // Fallback to Bind address if it's not 0.0.0.0 - if config.BindAddr != "0.0.0.0" { - defaultAdvertise = config.BindAddr - newaddr := fmt.Sprintf("%s:%d", defaultAdvertise, port) - *addr = newaddr - return true - } - - // As a last resort resolve the hostname and use it if it's not - // localhost (as localhost is never a sensible default) - host, err := os.Hostname() - if err != nil { - c.Ui.Error(fmt.Sprintf("Unable to get hostname to set advertise address: %v", err)) - return false - } - ip, err := net.ResolveIPAddr("ip", host) - if err != nil { - // Just because this node can't resolve its advertise - // address doesn't mean it's a bad address; use it - defaultAdvertise = host - newaddr := fmt.Sprintf("%s:%d", defaultAdvertise, port) - *addr = newaddr - return true - } - if ip.IP.IsLoopback() && !dev { - c.Ui.Error("Unable to select default advertise address as hostname resolves to localhost") - return false - } - defaultAdvertise = ip.IP.String() - newaddr := fmt.Sprintf("%s:%d", defaultAdvertise, port) - *addr = newaddr - return true - } - - if ok := getAdvertise(&config.AdvertiseAddrs.HTTP, config.Ports.HTTP); !ok { + if err := getAdvertise(&config.AdvertiseAddrs.HTTP, bind, config.Ports.HTTP, dev); err != nil { + c.Ui.Error(err.Error()) return nil } - if ok := getAdvertise(&config.AdvertiseAddrs.RPC, config.Ports.RPC); !ok { + if err := getAdvertise(&config.AdvertiseAddrs.RPC, bind, config.Ports.RPC, dev); err != nil { + c.Ui.Error(err.Error()) return nil } - if ok := getAdvertise(&config.AdvertiseAddrs.Serf, config.Ports.Serf); !ok { + if err := getAdvertise(&config.AdvertiseAddrs.Serf, bind, config.Ports.Serf, dev); err != nil { + c.Ui.Error(err.Error()) return nil } @@ -1042,3 +996,41 @@ Atlas Options: ` return strings.TrimSpace(helpText) } + +// Use getAdvertise(&config.AdvertiseAddrs., ) to +// retrieve a default address for advertising if one isn't set. +func getAdvertise(addr *string, bind string, defport int, dev bool) error { + if *addr != "" { + // Default to using manually configured address + return nil + } + + // Fallback to Bind address if it's not 0.0.0.0 + if bind != "0.0.0.0" { + newaddr := fmt.Sprintf("%s:%d", bind, defport) + *addr = newaddr + return nil + } + + // As a last resort resolve the hostname and use it if it's not + // localhost (as localhost is never a sensible default) + host, err := os.Hostname() + if err != nil { + return fmt.Errorf("Unable to get hostname to set advertise address: %v", err) + } + ip, err := net.ResolveIPAddr("ip", host) + if err != nil { + //TODO It'd be nice if we could advertise unresolvable + // addresses for configurations where this process may have + // different name resolution than the rest of the cluster. + // Unfortunately both serf/memberlist and Nomad's raft layer + // require resovlable advertise addresses. + return fmt.Errorf("Unable to resolve hostname to set advertise address: %v", err) + } + if ip.IP.IsLoopback() && !dev { + return fmt.Errorf("Unable to select default advertise address as hostname resolves to localhost") + } + newaddr := fmt.Sprintf("%s:%d", ip.IP.String(), defport) + *addr = newaddr + return nil +} From 581d6a5320fe1cfa762e6e4f1468cb9362d80766 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Tue, 8 Nov 2016 13:44:41 -0800 Subject: [PATCH 04/15] Handle missing ports in addresses --- command/agent/command.go | 77 ++++++++++++++++++++++++++++++++++------ 1 file changed, 66 insertions(+), 11 deletions(-) diff --git a/command/agent/command.go b/command/agent/command.go index bed19ab2292..bcf7fb17619 100644 --- a/command/agent/command.go +++ b/command/agent/command.go @@ -204,25 +204,28 @@ func (c *Command) readConfig() *Config { // Normalize addresses bind := config.BindAddr - if config.Addresses.HTTP == "" { - config.Addresses.HTTP = fmt.Sprintf("%s:%d", bind, config.Ports.HTTP) + if err := normalizeBind(&config.Addresses.HTTP, bind, &config.Ports.HTTP); err != nil { + c.Ui.Error(err.Error()) + return nil } - if config.Addresses.RPC == "" { - config.Addresses.RPC = fmt.Sprintf("%s:%d", bind, config.Ports.RPC) + if err := normalizeBind(&config.Addresses.RPC, bind, &config.Ports.RPC); err != nil { + c.Ui.Error(err.Error()) + return nil } - if config.Addresses.Serf == "" { - config.Addresses.Serf = fmt.Sprintf("%s:%d", bind, config.Ports.Serf) + if err := normalizeBind(&config.Addresses.Serf, bind, &config.Ports.Serf); err != nil { + c.Ui.Error(err.Error()) + return nil } - if err := getAdvertise(&config.AdvertiseAddrs.HTTP, bind, config.Ports.HTTP, dev); err != nil { + if err := normalizeAdvertise(&config.AdvertiseAddrs.HTTP, bind, config.Ports.HTTP, dev); err != nil { c.Ui.Error(err.Error()) return nil } - if err := getAdvertise(&config.AdvertiseAddrs.RPC, bind, config.Ports.RPC, dev); err != nil { + if err := normalizeAdvertise(&config.AdvertiseAddrs.RPC, bind, config.Ports.RPC, dev); err != nil { c.Ui.Error(err.Error()) return nil } - if err := getAdvertise(&config.AdvertiseAddrs.Serf, bind, config.Ports.Serf, dev); err != nil { + if err := normalizeAdvertise(&config.AdvertiseAddrs.Serf, bind, config.Ports.Serf, dev); err != nil { c.Ui.Error(err.Error()) return nil } @@ -997,11 +1000,22 @@ Atlas Options: return strings.TrimSpace(helpText) } -// Use getAdvertise(&config.AdvertiseAddrs., ) to +// Use normalizeAdvertise(&config.AdvertiseAddrs., ) to // retrieve a default address for advertising if one isn't set. -func getAdvertise(addr *string, bind string, defport int, dev bool) error { +func normalizeAdvertise(addr *string, bind string, defport int, dev bool) error { if *addr != "" { // Default to using manually configured address + _, _, err := net.SplitHostPort(*addr) + if err != nil { + if !isMissingPort(err) { + return fmt.Errorf("Error parsing advertise address %q: %v", *addr, err) + } + + // missing port, append the default + newaddr := fmt.Sprintf("%s:%d", *addr, defport) + *addr = newaddr + return nil + } return nil } @@ -1034,3 +1048,44 @@ func getAdvertise(addr *string, bind string, defport int, dev bool) error { *addr = newaddr return nil } + +func normalizeBind(addr *string, bind string, defport *int) error { + if *addr == "" { + newaddr := fmt.Sprintf("%s:%d", bind, *defport) + *addr = newaddr + return nil + } + + _, portstr, err := net.SplitHostPort(*addr) + if err != nil { + if !isMissingPort(err) { + return fmt.Errorf("Error parsing bind address %q: %v", err, *addr) + } + + // missing port, add default + newaddr := fmt.Sprintf("%s:%d", *addr, defport) + *addr = newaddr + return nil + } + + port, err := strconv.Atoi(portstr) + if err != nil { + return fmt.Errorf("Error parsing bind address's port: %q: %v", portstr, *addr) + } + + // Set the default port for this service to the bound port to keep + // configuration consistent. + if port != *defport { + *defport = port + } + + return nil +} + +// isMissingPort returns true if an error is a "missing port" error from +// net.SplitHostPort. +func isMissingPort(err error) bool { + // matches error const in net/ipsock.go + const missingPort = "missing port in address" + return err != nil && strings.HasPrefix(err.Error(), missingPort) +} From 9420e73e7ec4a5364ff18484530e3119f8ab11b0 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Tue, 8 Nov 2016 15:25:29 -0800 Subject: [PATCH 05/15] Match old error messages --- command/agent/agent.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/command/agent/agent.go b/command/agent/agent.go index 4d1da5e1a87..043e313c1e4 100644 --- a/command/agent/agent.go +++ b/command/agent/agent.go @@ -132,11 +132,11 @@ func (a *Agent) serverConfig() (*nomad.Config, error) { // Set up the bind addresses rpcAddr, err := net.ResolveTCPAddr("tcp", a.config.Addresses.RPC) if err != nil { - return nil, err + return nil, fmt.Errorf("Failed to parse RPC address %q: %v", a.config.Addresses.RPC, err) } serfAddr, err := net.ResolveTCPAddr("tcp", a.config.Addresses.Serf) if err != nil { - return nil, err + return nil, fmt.Errorf("Failed to parse Serf address %q: %v", a.config.Addresses.Serf, err) } conf.RPCAddr.Port = rpcAddr.Port conf.RPCAddr.IP = rpcAddr.IP @@ -146,11 +146,11 @@ func (a *Agent) serverConfig() (*nomad.Config, error) { // Set up the advertise addresses rpcAddr, err = net.ResolveTCPAddr("tcp", a.config.AdvertiseAddrs.RPC) if err != nil { - return nil, err + return nil, fmt.Errorf("Failed to parse RPC advertise address %q: %v", a.config.AdvertiseAddrs.RPC, err) } serfAddr, err = net.ResolveTCPAddr("tcp", a.config.AdvertiseAddrs.Serf) if err != nil { - return nil, err + return nil, fmt.Errorf("Failed to parse Serf advertise address %q: %v", a.config.AdvertiseAddrs.Serf, err) } conf.RPCAdvertise = rpcAddr conf.SerfConfig.MemberlistConfig.AdvertiseAddr = serfAddr.IP.String() From 5cc435ab3e84def191e1bf933f52278d7adae127 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Tue, 8 Nov 2016 15:44:10 -0800 Subject: [PATCH 06/15] Move config normalization into config.go to ease testing --- command/agent/command.go | 133 +----------------------------------- command/agent/config.go | 141 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 142 insertions(+), 132 deletions(-) diff --git a/command/agent/command.go b/command/agent/command.go index bcf7fb17619..5253db25956 100644 --- a/command/agent/command.go +++ b/command/agent/command.go @@ -5,7 +5,6 @@ import ( "fmt" "io" "log" - "net" "os" "os/signal" "path/filepath" @@ -197,49 +196,9 @@ func (c *Command) readConfig() *Config { // Merge any CLI options over config file options config = config.Merge(cmdConfig) - // Set the version info - config.Revision = c.Revision - config.Version = c.Version - config.VersionPrerelease = c.VersionPrerelease - - // Normalize addresses - bind := config.BindAddr - if err := normalizeBind(&config.Addresses.HTTP, bind, &config.Ports.HTTP); err != nil { - c.Ui.Error(err.Error()) - return nil - } - if err := normalizeBind(&config.Addresses.RPC, bind, &config.Ports.RPC); err != nil { - c.Ui.Error(err.Error()) - return nil - } - if err := normalizeBind(&config.Addresses.Serf, bind, &config.Ports.Serf); err != nil { - c.Ui.Error(err.Error()) - return nil - } - - if err := normalizeAdvertise(&config.AdvertiseAddrs.HTTP, bind, config.Ports.HTTP, dev); err != nil { - c.Ui.Error(err.Error()) - return nil - } - if err := normalizeAdvertise(&config.AdvertiseAddrs.RPC, bind, config.Ports.RPC, dev); err != nil { - c.Ui.Error(err.Error()) + if ok := config.normalize(c.Ui.Error, dev); !ok { return nil } - if err := normalizeAdvertise(&config.AdvertiseAddrs.Serf, bind, config.Ports.Serf, dev); err != nil { - c.Ui.Error(err.Error()) - return nil - } - - if config.Server.EncryptKey != "" { - if _, err := config.Server.EncryptBytes(); err != nil { - c.Ui.Error(fmt.Sprintf("Invalid encryption key: %s", err)) - return nil - } - keyfile := filepath.Join(config.DataDir, serfKeyring) - if _, err := os.Stat(keyfile); err == nil { - c.Ui.Error("WARNING: keyring exists but -encrypt given, using keyring") - } - } if dev { // Skip validation for dev mode @@ -999,93 +958,3 @@ Atlas Options: ` return strings.TrimSpace(helpText) } - -// Use normalizeAdvertise(&config.AdvertiseAddrs., ) to -// retrieve a default address for advertising if one isn't set. -func normalizeAdvertise(addr *string, bind string, defport int, dev bool) error { - if *addr != "" { - // Default to using manually configured address - _, _, err := net.SplitHostPort(*addr) - if err != nil { - if !isMissingPort(err) { - return fmt.Errorf("Error parsing advertise address %q: %v", *addr, err) - } - - // missing port, append the default - newaddr := fmt.Sprintf("%s:%d", *addr, defport) - *addr = newaddr - return nil - } - return nil - } - - // Fallback to Bind address if it's not 0.0.0.0 - if bind != "0.0.0.0" { - newaddr := fmt.Sprintf("%s:%d", bind, defport) - *addr = newaddr - return nil - } - - // As a last resort resolve the hostname and use it if it's not - // localhost (as localhost is never a sensible default) - host, err := os.Hostname() - if err != nil { - return fmt.Errorf("Unable to get hostname to set advertise address: %v", err) - } - ip, err := net.ResolveIPAddr("ip", host) - if err != nil { - //TODO It'd be nice if we could advertise unresolvable - // addresses for configurations where this process may have - // different name resolution than the rest of the cluster. - // Unfortunately both serf/memberlist and Nomad's raft layer - // require resovlable advertise addresses. - return fmt.Errorf("Unable to resolve hostname to set advertise address: %v", err) - } - if ip.IP.IsLoopback() && !dev { - return fmt.Errorf("Unable to select default advertise address as hostname resolves to localhost") - } - newaddr := fmt.Sprintf("%s:%d", ip.IP.String(), defport) - *addr = newaddr - return nil -} - -func normalizeBind(addr *string, bind string, defport *int) error { - if *addr == "" { - newaddr := fmt.Sprintf("%s:%d", bind, *defport) - *addr = newaddr - return nil - } - - _, portstr, err := net.SplitHostPort(*addr) - if err != nil { - if !isMissingPort(err) { - return fmt.Errorf("Error parsing bind address %q: %v", err, *addr) - } - - // missing port, add default - newaddr := fmt.Sprintf("%s:%d", *addr, defport) - *addr = newaddr - return nil - } - - port, err := strconv.Atoi(portstr) - if err != nil { - return fmt.Errorf("Error parsing bind address's port: %q: %v", portstr, *addr) - } - - // Set the default port for this service to the bound port to keep - // configuration consistent. - if port != *defport { - *defport = port - } - - return nil -} - -// isMissingPort returns true if an error is a "missing port" error from -// net.SplitHostPort. -func isMissingPort(err error) bool { - // matches error const in net/ipsock.go - const missingPort = "missing port in address" - return err != nil && strings.HasPrefix(err.Error(), missingPort) -} diff --git a/command/agent/config.go b/command/agent/config.go index 0a31e93bb8c..e18bf4d4e62 100644 --- a/command/agent/config.go +++ b/command/agent/config.go @@ -658,6 +658,147 @@ func (c *Config) Merge(b *Config) *Config { return &result } +// normalize config to set defaults and prevent nil derefence panics. +// +// Returns true if config is ok and false on errors. Since some normalization +// issues aren't fatal a logger must be provided to emit warnings. +func (c *Config) normalize(logger func(string), dev bool) bool { + // Set the version info + c.Revision = c.Revision + c.Version = c.Version + c.VersionPrerelease = c.VersionPrerelease + + // Normalize addresses + bind := c.BindAddr + if err := normalizeBind(&c.Addresses.HTTP, bind, &c.Ports.HTTP); err != nil { + logger(err.Error()) + return false + } + if err := normalizeBind(&c.Addresses.RPC, bind, &c.Ports.RPC); err != nil { + logger(err.Error()) + return false + } + if err := normalizeBind(&c.Addresses.Serf, bind, &c.Ports.Serf); err != nil { + logger(err.Error()) + return false + } + + if err := normalizeAdvertise(&c.AdvertiseAddrs.HTTP, bind, c.Ports.HTTP, dev); err != nil { + logger(err.Error()) + return false + } + if err := normalizeAdvertise(&c.AdvertiseAddrs.RPC, bind, c.Ports.RPC, dev); err != nil { + logger(err.Error()) + return false + } + if err := normalizeAdvertise(&c.AdvertiseAddrs.Serf, bind, c.Ports.Serf, dev); err != nil { + logger(err.Error()) + return false + } + + if c.Server.EncryptKey != "" { + if _, err := c.Server.EncryptBytes(); err != nil { + logger(fmt.Sprintf("Invalid encryption key: %s", err)) + return false + } + keyfile := filepath.Join(c.DataDir, serfKeyring) + if _, err := os.Stat(keyfile); err == nil { + logger("WARNING: keyring exists but -encrypt given, using keyring") + } + } + return true +} + +// Use normalizeAdvertise(&config.AdvertiseAddrs., ) to +// retrieve a default address for advertising if one isn't set. +func normalizeAdvertise(addr *string, bind string, defport int, dev bool) error { + if *addr != "" { + // Default to using manually configured address + _, _, err := net.SplitHostPort(*addr) + if err != nil { + if !isMissingPort(err) { + return fmt.Errorf("Error parsing advertise address %q: %v", *addr, err) + } + + // missing port, append the default + newaddr := fmt.Sprintf("%s:%d", *addr, defport) + *addr = newaddr + return nil + } + return nil + } + + // Fallback to Bind address if it's not 0.0.0.0 + if bind != "0.0.0.0" { + newaddr := fmt.Sprintf("%s:%d", bind, defport) + *addr = newaddr + return nil + } + + // As a last resort resolve the hostname and use it if it's not + // localhost (as localhost is never a sensible default) + host, err := os.Hostname() + if err != nil { + return fmt.Errorf("Unable to get hostname to set advertise address: %v", err) + } + ip, err := net.ResolveIPAddr("ip", host) + if err != nil { + //TODO It'd be nice if we could advertise unresolvable + // addresses for configurations where this process may have + // different name resolution than the rest of the cluster. + // Unfortunately both serf/memberlist and Nomad's raft layer + // require resovlable advertise addresses. + return fmt.Errorf("Unable to resolve hostname to set advertise address: %v", err) + } + if ip.IP.IsLoopback() && !dev { + return fmt.Errorf("Unable to select default advertise address as hostname resolves to localhost") + } + newaddr := fmt.Sprintf("%s:%d", ip.IP.String(), defport) + *addr = newaddr + return nil +} + +func normalizeBind(addr *string, bind string, defport *int) error { + if *addr == "" { + newaddr := fmt.Sprintf("%s:%d", bind, *defport) + *addr = newaddr + return nil + } + + _, portstr, err := net.SplitHostPort(*addr) + if err != nil { + if !isMissingPort(err) { + return fmt.Errorf("Error parsing bind address %q: %v", err, *addr) + } + + // missing port, add default + newaddr := fmt.Sprintf("%s:%d", *addr, defport) + *addr = newaddr + return nil + } + + port, err := strconv.Atoi(portstr) + if err != nil { + return fmt.Errorf("Error parsing bind address's port: %q: %v", portstr, *addr) + } + + // Set the default port for this service to the bound port to keep + // configuration consistent. + if port != *defport { + *defport = port + } + + return nil +} + +// isMissingPort returns true if an error is a "missing port" error from +// net.SplitHostPort. +func isMissingPort(err error) bool { + // matches error const in net/ipsock.go + const missingPort = "missing port in address" + return err != nil && strings.HasPrefix(err.Error(), missingPort) +} + // Merge is used to merge two server configs together func (a *ServerConfig) Merge(b *ServerConfig) *ServerConfig { result := *a From eb89fd2793d846932d3b908731d0551364b34e95 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Tue, 8 Nov 2016 16:02:20 -0800 Subject: [PATCH 07/15] Fix int pointer formatting and server config test --- command/agent/agent_test.go | 107 +++++++++++++++++++----------------- command/agent/config.go | 2 +- 2 files changed, 58 insertions(+), 51 deletions(-) diff --git a/command/agent/agent_test.go b/command/agent/agent_test.go index d4ba5bcdd37..49be3ba95f1 100644 --- a/command/agent/agent_test.go +++ b/command/agent/agent_test.go @@ -94,6 +94,10 @@ func TestAgent_RPCPing(t *testing.T) { func TestAgent_ServerConfig(t *testing.T) { conf := DefaultConfig() a := &Agent{config: conf} + testlogger := func(s string) { + t.Log(s) + } + dev := false // Returns error on bad serf addr conf.AdvertiseAddrs.Serf = "nope" @@ -113,6 +117,9 @@ func TestAgent_ServerConfig(t *testing.T) { conf.AdvertiseAddrs.HTTP = "10.10.11.1:4005" // Parses the advertise addrs correctly + if ok := conf.normalize(testlogger, dev); !ok { + t.Fatalf("failed to normalize config") + } out, err := a.serverConfig() if err != nil { t.Fatalf("err: %s", err) @@ -125,20 +132,27 @@ func TestAgent_ServerConfig(t *testing.T) { if serfPort != 4000 { t.Fatalf("expected 4000, got: %d", serfPort) } - if addr := out.RPCAdvertise; addr.IP.String() != "127.0.0.1" || addr.Port != 4001 { + + // Assert addresses weren't changed + if addr := conf.AdvertiseAddrs.RPC; addr != "127.0.0.1:4001" { t.Fatalf("bad rpc advertise addr: %#v", addr) } - if addr := a.serverHTTPAddr; addr != "10.10.11.1:4005" { + if addr := conf.AdvertiseAddrs.HTTP; addr != "10.10.11.1:4005" { t.Fatalf("expect 10.11.11.1:4005, got: %v", addr) } - if addr := a.serverRPCAddr; addr != "127.0.0.1:4001" { - t.Fatalf("expect 127.0.0.1:4001, got: %v", addr) + if addr := conf.Addresses.RPC; addr != "0.0.0.0:4647" { + t.Fatalf("expect 0.0.0.0:4001, got: %v", addr) } // Sets up the ports properly + conf.Addresses.RPC = "" + conf.Addresses.Serf = "" conf.Ports.RPC = 4003 conf.Ports.Serf = 4004 + if ok := conf.normalize(testlogger, dev); !ok { + t.Fatalf("failed to normalize config") + } out, err = a.serverConfig() if err != nil { t.Fatalf("err: %s", err) @@ -152,93 +166,83 @@ func TestAgent_ServerConfig(t *testing.T) { // Prefers advertise over bind addr conf.BindAddr = "127.0.0.3" + conf.Addresses.HTTP = "127.0.0.2" conf.Addresses.RPC = "127.0.0.2" conf.Addresses.Serf = "127.0.0.2" - conf.Addresses.HTTP = "127.0.0.2" conf.AdvertiseAddrs.HTTP = "10.0.0.10" conf.AdvertiseAddrs.RPC = "" conf.AdvertiseAddrs.Serf = "10.0.0.12:4004" + if ok := conf.normalize(testlogger, dev); !ok { + t.Fatalf("failed to normalize config") + } out, err = a.serverConfig() + fmt.Println(conf.Addresses.RPC) if err != nil { t.Fatalf("err: %s", err) } if addr := out.RPCAddr.IP.String(); addr != "127.0.0.2" { t.Fatalf("expect 127.0.0.2, got: %s", addr) } + if port := out.RPCAddr.Port; port != 4003 { + t.Fatalf("expect 4647, got: %d", port) + } if addr := out.SerfConfig.MemberlistConfig.BindAddr; addr != "127.0.0.2" { t.Fatalf("expect 127.0.0.2, got: %s", addr) } - if addr := a.serverHTTPAddr; addr != "10.0.0.10:4646" { - t.Fatalf("expect 10.0.0.10:4646, got: %s", addr) - } - // NOTE: AdvertiseAddr > Addresses > BindAddr > Defaults - if addr := a.serverRPCAddr; addr != "127.0.0.2:4003" { - t.Fatalf("expect 127.0.0.2:4003, got: %s", addr) - } - if addr := a.serverSerfAddr; addr != "10.0.0.12:4004" { - t.Fatalf("expect 10.0.0.12:4004, got: %s", addr) + if port := out.SerfConfig.MemberlistConfig.BindPort; port != 4004 { + t.Fatalf("expect 4648, got: ^d", port) } - - // It correctly identifies the bind and advertise address when requested - if addr, err := a.selectAddr(a.getHTTPAddr, true); addr != "127.0.0.2:4646" || err != nil { + if addr := conf.Addresses.HTTP; addr != "127.0.0.2:4646" { t.Fatalf("expect 127.0.0.2:4646, got: %s", addr) } - - if addr, err := a.selectAddr(a.getHTTPAddr, false); addr != "10.0.0.10:4646" || err != nil { - t.Fatalf("expect 10.0.0.10:4646, got: %s", addr) - } - - if addr, err := a.selectAddr(a.getRPCAddr, true); addr != "127.0.0.2:4003" || err != nil { + if addr := conf.Addresses.RPC; addr != "127.0.0.2:4003" { t.Fatalf("expect 127.0.0.2:4003, got: %s", addr) } - - if addr, err := a.selectAddr(a.getRPCAddr, false); addr != "127.0.0.2:4003" || err != nil { - t.Fatalf("expect 127.0.0.2:4003, got: %s", addr) - } - - if addr, err := a.selectAddr(a.getSerfAddr, true); addr != "127.0.0.2:4004" || err != nil { - t.Fatalf("expect 127.0.0.2:4004, got: %s", addr) - } - - if addr, err := a.selectAddr(a.getSerfAddr, false); addr != "10.0.0.12:4004" || err != nil { + if addr := conf.Addresses.Serf; addr != "127.0.0.2:4004" { t.Fatalf("expect 10.0.0.12:4004, got: %s", addr) } - - // We don't resolve 0.0.0.0 unless we're asking for bind - conf.Addresses.HTTP = "0.0.0.0" - conf.AdvertiseAddrs.HTTP = "" - if addr, err := a.getHTTPAddr(false); addr.IP.String() != "127.0.0.3" || err != nil { - t.Fatalf("expect 127.0.0.3, got: %s", addr.IP.String()) + if addr := conf.AdvertiseAddrs.HTTP; addr != "10.0.0.10:4646" { + t.Fatalf("expect 10.0.0.10:4646, got: %s", addr) } - - // We still get 0.0.0.0 when explicitly asking for bind - if addr, err := a.getHTTPAddr(true); addr.IP.String() != "0.0.0.0" || err != nil { - t.Fatalf("expect 0.0.0.0, got: %s", addr.IP.String()) + if addr := conf.AdvertiseAddrs.RPC; addr != "127.0.0.3:4003" { + t.Fatalf("expect 127.0.0.3:4003, got: %s", addr) } - - // selectAddr does not return 0.0.0.0 with preferBind - if addr, err := a.selectAddr(a.getHTTPAddr, true); addr != "127.0.0.3:4646" || err != nil { - t.Fatalf("expect 127.0.0.3:4646, got: %s", addr) + if addr := conf.AdvertiseAddrs.Serf; addr != "10.0.0.12:4004" { + t.Fatalf("expect 10.0.0.12:4004, got: %s", addr) } conf.Server.NodeGCThreshold = "42g" + if ok := conf.normalize(testlogger, dev); !ok { + t.Fatalf("failed to normalize config") + } out, err = a.serverConfig() if err == nil || !strings.Contains(err.Error(), "unknown unit") { t.Fatalf("expected unknown unit error, got: %#v", err) } + conf.Server.NodeGCThreshold = "10s" + if ok := conf.normalize(testlogger, dev); !ok { + t.Fatalf("failed to normalize config") + } out, err = a.serverConfig() if threshold := out.NodeGCThreshold; threshold != time.Second*10 { t.Fatalf("expect 10s, got: %s", threshold) } conf.Server.HeartbeatGrace = "42g" + if ok := conf.normalize(testlogger, dev); !ok { + t.Fatalf("failed to normalize config") + } out, err = a.serverConfig() if err == nil || !strings.Contains(err.Error(), "unknown unit") { t.Fatalf("expected unknown unit error, got: %#v", err) } + conf.Server.HeartbeatGrace = "37s" + if ok := conf.normalize(testlogger, dev); !ok { + t.Fatalf("failed to normalize config") + } out, err = a.serverConfig() if threshold := out.HeartbeatGrace; threshold != time.Second*37 { t.Fatalf("expect 37s, got: %s", threshold) @@ -254,6 +258,9 @@ func TestAgent_ServerConfig(t *testing.T) { conf.Ports.HTTP = 4646 conf.Ports.RPC = 4647 conf.Ports.Serf = 4648 + if ok := conf.normalize(testlogger, dev); !ok { + t.Fatalf("failed to normalize config") + } out, err = a.serverConfig() if err != nil { t.Fatalf("err: %s", err) @@ -264,13 +271,13 @@ func TestAgent_ServerConfig(t *testing.T) { if addr := out.SerfConfig.MemberlistConfig.BindAddr; addr != "127.0.0.3" { t.Fatalf("expect 127.0.0.3, got: %s", addr) } - if addr := a.serverHTTPAddr; addr != "127.0.0.3:4646" { + if addr := conf.Addresses.HTTP; addr != "127.0.0.3:4646" { t.Fatalf("expect 127.0.0.3:4646, got: %s", addr) } - if addr := a.serverRPCAddr; addr != "127.0.0.3:4647" { + if addr := conf.Addresses.RPC; addr != "127.0.0.3:4647" { t.Fatalf("expect 127.0.0.3:4647, got: %s", addr) } - if addr := a.serverSerfAddr; addr != "127.0.0.3:4648" { + if addr := conf.Addresses.Serf; addr != "127.0.0.3:4648" { t.Fatalf("expect 127.0.0.3:4648, got: %s", addr) } diff --git a/command/agent/config.go b/command/agent/config.go index e18bf4d4e62..bd0ad60b1da 100644 --- a/command/agent/config.go +++ b/command/agent/config.go @@ -772,7 +772,7 @@ func normalizeBind(addr *string, bind string, defport *int) error { } // missing port, add default - newaddr := fmt.Sprintf("%s:%d", *addr, defport) + newaddr := fmt.Sprintf("%s:%d", *addr, *defport) *addr = newaddr return nil } From 4f15cac3ca67f79b8c9b4a9739949a13f1067000 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Tue, 8 Nov 2016 16:13:09 -0800 Subject: [PATCH 08/15] Normalize configs for agent tests --- command/agent/agent_test.go | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/command/agent/agent_test.go b/command/agent/agent_test.go index 49be3ba95f1..5ef4ab3fb7c 100644 --- a/command/agent/agent_test.go +++ b/command/agent/agent_test.go @@ -13,6 +13,14 @@ import ( sconfig "github.com/hashicorp/nomad/nomad/structs/config" ) +// getTestLogger returns a log func appropriate for passing to +// Config.normalize() +func getTestLogger(t testing.TB) func(string) { + return func(s string) { + t.Log(s) + } +} + func getPort() int { addr, err := net.ResolveTCPAddr("tcp", "localhost:0") if err != nil { @@ -72,6 +80,9 @@ func makeAgent(t testing.TB, cb func(*Config)) (string, *Agent) { cb(conf) } + if ok := conf.normalize(getTestLogger(t), config.DevMode); !ok { + t.Fatalf("error normalizing config") + } agent, err := NewAgent(conf, os.Stderr) if err != nil { os.RemoveAll(dir) @@ -94,9 +105,7 @@ func TestAgent_RPCPing(t *testing.T) { func TestAgent_ServerConfig(t *testing.T) { conf := DefaultConfig() a := &Agent{config: conf} - testlogger := func(s string) { - t.Log(s) - } + testlogger := getTestLogger(t) dev := false // Returns error on bad serf addr From 4aed5ba1e216b4e4097427973a7eb726b060fecc Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Tue, 8 Nov 2016 16:21:52 -0800 Subject: [PATCH 09/15] Restore sync/atomic use for consistency --- command/agent/agent.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/command/agent/agent.go b/command/agent/agent.go index 043e313c1e4..60e7f027628 100644 --- a/command/agent/agent.go +++ b/command/agent/agent.go @@ -11,6 +11,7 @@ import ( "strconv" "strings" "sync" + "sync/atomic" "time" "github.com/hashicorp/nomad/client" @@ -110,7 +111,7 @@ func (a *Agent) serverConfig() (*nomad.Config, error) { if a.config.Server.BootstrapExpect == 1 { conf.Bootstrap = true } else { - conf.BootstrapExpect = int32(a.config.Server.BootstrapExpect) + atomic.StoreInt32(&conf.BootstrapExpect, int32(a.config.Server.BootstrapExpect)) } } if a.config.DataDir != "" { From 1e0b4d76e8376b6b9ecf966f1e51d77cfa38f087 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Tue, 8 Nov 2016 16:22:04 -0800 Subject: [PATCH 10/15] Fix test that fails now that advertise can't be localhost --- command/agent/command_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/command/agent/command_test.go b/command/agent/command_test.go index 9e6d750ef5f..449fd6caccd 100644 --- a/command/agent/command_test.go +++ b/command/agent/command_test.go @@ -59,6 +59,9 @@ func TestCommand_Args(t *testing.T) { ShutdownCh: shutdownCh, } + // To prevent test failures on hosts whose hostname resolves to + // a loopback address, we must append a bind address + tc.args = append(tc.args, "-bind=127.0.0.1") if code := cmd.Run(tc.args); code != 1 { t.Fatalf("args: %v\nexit: %d\n", tc.args, code) } From 1893772f4b629118d80ce4659f80d46bb4dcfbd3 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Tue, 8 Nov 2016 16:35:11 -0800 Subject: [PATCH 11/15] Enable dev mode to allow using localhost in tests --- command/agent/agent_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/command/agent/agent_test.go b/command/agent/agent_test.go index 5ef4ab3fb7c..6e385d1c2a4 100644 --- a/command/agent/agent_test.go +++ b/command/agent/agent_test.go @@ -318,11 +318,16 @@ func TestAgent_ServerConfig(t *testing.T) { func TestAgent_ClientConfig(t *testing.T) { conf := DefaultConfig() + // enabled just to allow using localhost for all addresses + conf.DevMode = true a := &Agent{config: conf} conf.Client.Enabled = true conf.Addresses.HTTP = "127.0.0.1" conf.Ports.HTTP = 5678 + if ok := conf.normalize(getTestLogger(t), conf.DevMode); !ok { + t.Fatalf("error normalizing config") + } c, err := a.clientConfig() if err != nil { t.Fatalf("got err: %v", err) @@ -334,10 +339,14 @@ func TestAgent_ClientConfig(t *testing.T) { } conf = DefaultConfig() + conf.DevMode = true a = &Agent{config: conf} conf.Client.Enabled = true conf.Addresses.HTTP = "127.0.0.1" + if ok := conf.normalize(getTestLogger(t), conf.DevMode); !ok { + t.Fatalf("error normalizing config") + } c, err = a.clientConfig() if err != nil { t.Fatalf("got err: %v", err) From 1304ba8b2dd5cd56c67eb16df43879c1c9ed691c Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Wed, 9 Nov 2016 11:37:41 -0800 Subject: [PATCH 12/15] Addresses are just addresses - no ports Store address+port in an unexported field for ease-of-use --- command/agent/agent.go | 18 +-- command/agent/agent_test.go | 106 +++++++++--------- command/agent/command.go | 20 +++- command/agent/command_test.go | 2 +- command/agent/config.go | 204 ++++++++++++++++------------------ command/agent/http.go | 2 +- 6 files changed, 175 insertions(+), 177 deletions(-) diff --git a/command/agent/agent.go b/command/agent/agent.go index 60e7f027628..5c8399270d1 100644 --- a/command/agent/agent.go +++ b/command/agent/agent.go @@ -131,13 +131,13 @@ func (a *Agent) serverConfig() (*nomad.Config, error) { } // Set up the bind addresses - rpcAddr, err := net.ResolveTCPAddr("tcp", a.config.Addresses.RPC) + rpcAddr, err := net.ResolveTCPAddr("tcp", a.config.normalizedAddrs.RPC) if err != nil { - return nil, fmt.Errorf("Failed to parse RPC address %q: %v", a.config.Addresses.RPC, err) + return nil, fmt.Errorf("Failed to parse RPC address %q: %v", a.config.normalizedAddrs.RPC, err) } - serfAddr, err := net.ResolveTCPAddr("tcp", a.config.Addresses.Serf) + serfAddr, err := net.ResolveTCPAddr("tcp", a.config.normalizedAddrs.Serf) if err != nil { - return nil, fmt.Errorf("Failed to parse Serf address %q: %v", a.config.Addresses.Serf, err) + return nil, fmt.Errorf("Failed to parse Serf address %q: %v", a.config.normalizedAddrs.Serf, err) } conf.RPCAddr.Port = rpcAddr.Port conf.RPCAddr.IP = rpcAddr.IP @@ -256,7 +256,7 @@ func (a *Agent) clientConfig() (*clientconfig.Config, error) { conf.Node.NodeClass = a.config.Client.NodeClass // Set up the HTTP advertise address - conf.Node.HTTPAddr = a.config.Addresses.HTTP + conf.Node.HTTPAddr = a.config.AdvertiseAddrs.HTTP // Reserve resources on the node. r := conf.Node.Reserved @@ -315,9 +315,9 @@ func (a *Agent) setupServer() error { a.server = server // Consul check addresses default to bind but can be toggled to use advertise - httpCheckAddr := a.config.Addresses.HTTP - rpcCheckAddr := a.config.Addresses.RPC - serfCheckAddr := a.config.Addresses.Serf + httpCheckAddr := a.config.normalizedAddrs.HTTP + rpcCheckAddr := a.config.normalizedAddrs.RPC + serfCheckAddr := a.config.normalizedAddrs.Serf if a.config.Consul.ChecksUseAdvertise { httpCheckAddr = a.config.AdvertiseAddrs.HTTP rpcCheckAddr = a.config.AdvertiseAddrs.RPC @@ -438,7 +438,7 @@ func (a *Agent) setupClient() error { a.client = client // Resolve the http check address - httpCheckAddr := a.config.Addresses.HTTP + httpCheckAddr := a.config.normalizedAddrs.HTTP if a.config.Consul.ChecksUseAdvertise { httpCheckAddr = a.config.AdvertiseAddrs.HTTP } diff --git a/command/agent/agent_test.go b/command/agent/agent_test.go index 6e385d1c2a4..9615c027ac1 100644 --- a/command/agent/agent_test.go +++ b/command/agent/agent_test.go @@ -13,14 +13,6 @@ import ( sconfig "github.com/hashicorp/nomad/nomad/structs/config" ) -// getTestLogger returns a log func appropriate for passing to -// Config.normalize() -func getTestLogger(t testing.TB) func(string) { - return func(s string) { - t.Log(s) - } -} - func getPort() int { addr, err := net.ResolveTCPAddr("tcp", "localhost:0") if err != nil { @@ -80,8 +72,8 @@ func makeAgent(t testing.TB, cb func(*Config)) (string, *Agent) { cb(conf) } - if ok := conf.normalize(getTestLogger(t), config.DevMode); !ok { - t.Fatalf("error normalizing config") + if err := conf.normalizeAddrs(); err != nil { + t.Fatalf("error normalizing config: %v", err) } agent, err := NewAgent(conf, os.Stderr) if err != nil { @@ -104,30 +96,16 @@ func TestAgent_RPCPing(t *testing.T) { func TestAgent_ServerConfig(t *testing.T) { conf := DefaultConfig() + conf.DevMode = true // allow localhost for advertise addrs a := &Agent{config: conf} - testlogger := getTestLogger(t) - dev := false - // Returns error on bad serf addr - conf.AdvertiseAddrs.Serf = "nope" - _, err := a.serverConfig() - if err == nil || !strings.Contains(err.Error(), "Failed to parse Serf") { - t.Fatalf("expected serf address error, got: %#v", err) - } conf.AdvertiseAddrs.Serf = "127.0.0.1:4000" - - // Returns error on bad rpc addr - conf.AdvertiseAddrs.RPC = "nope" - _, err = a.serverConfig() - if err == nil || !strings.Contains(err.Error(), "Failed to parse RPC") { - t.Fatalf("expected rpc address error, got: %#v", err) - } conf.AdvertiseAddrs.RPC = "127.0.0.1:4001" conf.AdvertiseAddrs.HTTP = "10.10.11.1:4005" // Parses the advertise addrs correctly - if ok := conf.normalize(testlogger, dev); !ok { - t.Fatalf("failed to normalize config") + if err := conf.normalizeAddrs(); err != nil { + t.Fatalf("error normalizing config: %v", err) } out, err := a.serverConfig() if err != nil { @@ -149,8 +127,8 @@ func TestAgent_ServerConfig(t *testing.T) { if addr := conf.AdvertiseAddrs.HTTP; addr != "10.10.11.1:4005" { t.Fatalf("expect 10.11.11.1:4005, got: %v", addr) } - if addr := conf.Addresses.RPC; addr != "0.0.0.0:4647" { - t.Fatalf("expect 0.0.0.0:4001, got: %v", addr) + if addr := conf.Addresses.RPC; addr != "0.0.0.0" { + t.Fatalf("expect 0.0.0.0, got: %v", addr) } // Sets up the ports properly @@ -159,8 +137,8 @@ func TestAgent_ServerConfig(t *testing.T) { conf.Ports.RPC = 4003 conf.Ports.Serf = 4004 - if ok := conf.normalize(testlogger, dev); !ok { - t.Fatalf("failed to normalize config") + if err := conf.normalizeAddrs(); err != nil { + t.Fatalf("error normalizing config: %v", err) } out, err = a.serverConfig() if err != nil { @@ -182,8 +160,8 @@ func TestAgent_ServerConfig(t *testing.T) { conf.AdvertiseAddrs.RPC = "" conf.AdvertiseAddrs.Serf = "10.0.0.12:4004" - if ok := conf.normalize(testlogger, dev); !ok { - t.Fatalf("failed to normalize config") + if err := conf.normalizeAddrs(); err != nil { + t.Fatalf("error normalizing config: %v", err) } out, err = a.serverConfig() fmt.Println(conf.Addresses.RPC) @@ -202,28 +180,37 @@ func TestAgent_ServerConfig(t *testing.T) { if port := out.SerfConfig.MemberlistConfig.BindPort; port != 4004 { t.Fatalf("expect 4648, got: ^d", port) } - if addr := conf.Addresses.HTTP; addr != "127.0.0.2:4646" { + if addr := conf.Addresses.HTTP; addr != "127.0.0.2" { + t.Fatalf("expect 127.0.0.2, got: %s", addr) + } + if addr := conf.Addresses.RPC; addr != "127.0.0.2" { + t.Fatalf("expect 127.0.0.2, got: %s", addr) + } + if addr := conf.Addresses.Serf; addr != "127.0.0.2" { + t.Fatalf("expect 10.0.0.12, got: %s", addr) + } + if addr := conf.normalizedAddrs.HTTP; addr != "127.0.0.2:4646" { t.Fatalf("expect 127.0.0.2:4646, got: %s", addr) } - if addr := conf.Addresses.RPC; addr != "127.0.0.2:4003" { + if addr := conf.normalizedAddrs.RPC; addr != "127.0.0.2:4003" { t.Fatalf("expect 127.0.0.2:4003, got: %s", addr) } - if addr := conf.Addresses.Serf; addr != "127.0.0.2:4004" { + if addr := conf.normalizedAddrs.Serf; addr != "127.0.0.2:4004" { t.Fatalf("expect 10.0.0.12:4004, got: %s", addr) } if addr := conf.AdvertiseAddrs.HTTP; addr != "10.0.0.10:4646" { t.Fatalf("expect 10.0.0.10:4646, got: %s", addr) } - if addr := conf.AdvertiseAddrs.RPC; addr != "127.0.0.3:4003" { - t.Fatalf("expect 127.0.0.3:4003, got: %s", addr) + if addr := conf.AdvertiseAddrs.RPC; addr != "127.0.0.2:4003" { + t.Fatalf("expect 127.0.0.2:4003, got: %s", addr) } if addr := conf.AdvertiseAddrs.Serf; addr != "10.0.0.12:4004" { t.Fatalf("expect 10.0.0.12:4004, got: %s", addr) } conf.Server.NodeGCThreshold = "42g" - if ok := conf.normalize(testlogger, dev); !ok { - t.Fatalf("failed to normalize config") + if err := conf.normalizeAddrs(); err != nil { + t.Fatalf("error normalizing config: %v", err) } out, err = a.serverConfig() if err == nil || !strings.Contains(err.Error(), "unknown unit") { @@ -231,8 +218,8 @@ func TestAgent_ServerConfig(t *testing.T) { } conf.Server.NodeGCThreshold = "10s" - if ok := conf.normalize(testlogger, dev); !ok { - t.Fatalf("failed to normalize config") + if err := conf.normalizeAddrs(); err != nil { + t.Fatalf("error normalizing config: %v", err) } out, err = a.serverConfig() if threshold := out.NodeGCThreshold; threshold != time.Second*10 { @@ -240,8 +227,8 @@ func TestAgent_ServerConfig(t *testing.T) { } conf.Server.HeartbeatGrace = "42g" - if ok := conf.normalize(testlogger, dev); !ok { - t.Fatalf("failed to normalize config") + if err := conf.normalizeAddrs(); err != nil { + t.Fatalf("error normalizing config: %v", err) } out, err = a.serverConfig() if err == nil || !strings.Contains(err.Error(), "unknown unit") { @@ -249,8 +236,8 @@ func TestAgent_ServerConfig(t *testing.T) { } conf.Server.HeartbeatGrace = "37s" - if ok := conf.normalize(testlogger, dev); !ok { - t.Fatalf("failed to normalize config") + if err := conf.normalizeAddrs(); err != nil { + t.Fatalf("error normalizing config: %v", err) } out, err = a.serverConfig() if threshold := out.HeartbeatGrace; threshold != time.Second*37 { @@ -267,8 +254,8 @@ func TestAgent_ServerConfig(t *testing.T) { conf.Ports.HTTP = 4646 conf.Ports.RPC = 4647 conf.Ports.Serf = 4648 - if ok := conf.normalize(testlogger, dev); !ok { - t.Fatalf("failed to normalize config") + if err := conf.normalizeAddrs(); err != nil { + t.Fatalf("error normalizing config: %v", err) } out, err = a.serverConfig() if err != nil { @@ -280,13 +267,22 @@ func TestAgent_ServerConfig(t *testing.T) { if addr := out.SerfConfig.MemberlistConfig.BindAddr; addr != "127.0.0.3" { t.Fatalf("expect 127.0.0.3, got: %s", addr) } - if addr := conf.Addresses.HTTP; addr != "127.0.0.3:4646" { + if addr := conf.Addresses.HTTP; addr != "127.0.0.3" { + t.Fatalf("expect 127.0.0.3, got: %s", addr) + } + if addr := conf.Addresses.RPC; addr != "127.0.0.3" { + t.Fatalf("expect 127.0.0.3, got: %s", addr) + } + if addr := conf.Addresses.Serf; addr != "127.0.0.3" { + t.Fatalf("expect 127.0.0.3, got: %s", addr) + } + if addr := conf.normalizedAddrs.HTTP; addr != "127.0.0.3:4646" { t.Fatalf("expect 127.0.0.3:4646, got: %s", addr) } - if addr := conf.Addresses.RPC; addr != "127.0.0.3:4647" { + if addr := conf.normalizedAddrs.RPC; addr != "127.0.0.3:4647" { t.Fatalf("expect 127.0.0.3:4647, got: %s", addr) } - if addr := conf.Addresses.Serf; addr != "127.0.0.3:4648" { + if addr := conf.normalizedAddrs.Serf; addr != "127.0.0.3:4648" { t.Fatalf("expect 127.0.0.3:4648, got: %s", addr) } @@ -325,8 +321,8 @@ func TestAgent_ClientConfig(t *testing.T) { conf.Addresses.HTTP = "127.0.0.1" conf.Ports.HTTP = 5678 - if ok := conf.normalize(getTestLogger(t), conf.DevMode); !ok { - t.Fatalf("error normalizing config") + if err := conf.normalizeAddrs(); err != nil { + t.Fatalf("error normalizing config: %v", err) } c, err := a.clientConfig() if err != nil { @@ -344,8 +340,8 @@ func TestAgent_ClientConfig(t *testing.T) { conf.Client.Enabled = true conf.Addresses.HTTP = "127.0.0.1" - if ok := conf.normalize(getTestLogger(t), conf.DevMode); !ok { - t.Fatalf("error normalizing config") + if err := conf.normalizeAddrs(); err != nil { + t.Fatalf("error normalizing config: %v", err) } c, err = a.clientConfig() if err != nil { diff --git a/command/agent/command.go b/command/agent/command.go index 5253db25956..c0ce3b78fc7 100644 --- a/command/agent/command.go +++ b/command/agent/command.go @@ -196,7 +196,14 @@ func (c *Command) readConfig() *Config { // Merge any CLI options over config file options config = config.Merge(cmdConfig) - if ok := config.normalize(c.Ui.Error, dev); !ok { + // Set the version info + config.Revision = c.Revision + config.Version = c.Version + config.VersionPrerelease = c.VersionPrerelease + + // Normalize binds, ports, addresses, and advertise + if err := config.normalizeAddrs(); err != nil { + c.Ui.Error(err.Error()) return nil } @@ -205,6 +212,17 @@ func (c *Command) readConfig() *Config { return config } + if config.Server.EncryptKey != "" { + if _, err := config.Server.EncryptBytes(); err != nil { + c.Ui.Error(fmt.Sprintf("Invalid encryption key: %s", err)) + return nil + } + keyfile := filepath.Join(config.DataDir, serfKeyring) + if _, err := os.Stat(keyfile); err == nil { + c.Ui.Warn("WARNING: keyring exists but -encrypt given, using keyring") + } + } + // Parse the RetryInterval. dur, err := time.ParseDuration(config.Server.RetryInterval) if err != nil { diff --git a/command/agent/command_test.go b/command/agent/command_test.go index 449fd6caccd..248ef118524 100644 --- a/command/agent/command_test.go +++ b/command/agent/command_test.go @@ -61,7 +61,7 @@ func TestCommand_Args(t *testing.T) { // To prevent test failures on hosts whose hostname resolves to // a loopback address, we must append a bind address - tc.args = append(tc.args, "-bind=127.0.0.1") + tc.args = append(tc.args, "-bind=169.254.0.1") if code := cmd.Run(tc.args); code != 1 { t.Fatalf("args: %v\nexit: %d\n", tc.args, code) } diff --git a/command/agent/config.go b/command/agent/config.go index bd0ad60b1da..e9b1cdfe809 100644 --- a/command/agent/config.go +++ b/command/agent/config.go @@ -46,8 +46,13 @@ type Config struct { Ports *Ports `mapstructure:"ports"` // Addresses is used to override the network addresses we bind to. + // + // Use normalizedAddrs if you need the host+port to bind to. Addresses *Addresses `mapstructure:"addresses"` + // normalizedAddr is set to the Address+Port by normalizeAddrs() + normalizedAddrs *Addresses + // AdvertiseAddrs is used to control the addresses we advertise. AdvertiseAddrs *AdvertiseAddrs `mapstructure:"advertise"` @@ -335,8 +340,8 @@ type Telemetry struct { CirconusBrokerSelectTag string `mapstructure:"circonus_broker_select_tag"` } -// Ports is used to encapsulate the various ports we bind to for network -// services. If any are not specified then the defaults are used instead. +// Ports encapsulates the various ports we bind to for network services. If any +// are not specified then the defaults are used instead. type Ports struct { HTTP int `mapstructure:"http"` RPC int `mapstructure:"rpc"` @@ -352,8 +357,8 @@ type Addresses struct { } // AdvertiseAddrs is used to control the addresses we advertise out for -// different network services. Not all network services support an -// advertise address. All are optional and default to BindAddr. +// different network services. All are optional and default to BindAddr and +// their default Port. type AdvertiseAddrs struct { HTTP string `mapstructure:"http"` RPC string `mapstructure:"rpc"` @@ -658,137 +663,116 @@ func (c *Config) Merge(b *Config) *Config { return &result } -// normalize config to set defaults and prevent nil derefence panics. +// normalize Addresses and AdvertiseAddrs to always be initialized and have +// sane defaults. +func (c *Config) normalizeAddrs() error { + c.Addresses.HTTP = normalizeBind(c.Addresses.HTTP, c.BindAddr) + c.Addresses.RPC = normalizeBind(c.Addresses.RPC, c.BindAddr) + c.Addresses.Serf = normalizeBind(c.Addresses.Serf, c.BindAddr) + c.normalizedAddrs = &Addresses{ + HTTP: fmt.Sprintf("%s:%d", c.Addresses.HTTP, c.Ports.HTTP), + RPC: fmt.Sprintf("%s:%d", c.Addresses.RPC, c.Ports.RPC), + Serf: fmt.Sprintf("%s:%d", c.Addresses.Serf, c.Ports.Serf), + } + + addr, err := normalizeAdvertise(c.AdvertiseAddrs.HTTP, c.Addresses.HTTP, c.Ports.HTTP, c.DevMode) + if err != nil { + return fmt.Errorf("Failed to parse HTTP advertise address: %v", err) + } + c.AdvertiseAddrs.HTTP = addr + + addr, err = normalizeAdvertise(c.AdvertiseAddrs.RPC, c.Addresses.RPC, c.Ports.RPC, c.DevMode) + if err != nil { + return fmt.Errorf("Failed to parse RPC advertise address: %v", err) + } + c.AdvertiseAddrs.RPC = addr + + addr, err = normalizeAdvertise(c.AdvertiseAddrs.Serf, c.Addresses.Serf, c.Ports.Serf, c.DevMode) + if err != nil { + return fmt.Errorf("Failed to parse Serf advertise address: %v", err) + } + c.AdvertiseAddrs.Serf = addr + + return nil +} + +// normalizeBind returns a normalized bind address. // -// Returns true if config is ok and false on errors. Since some normalization -// issues aren't fatal a logger must be provided to emit warnings. -func (c *Config) normalize(logger func(string), dev bool) bool { - // Set the version info - c.Revision = c.Revision - c.Version = c.Version - c.VersionPrerelease = c.VersionPrerelease - - // Normalize addresses - bind := c.BindAddr - if err := normalizeBind(&c.Addresses.HTTP, bind, &c.Ports.HTTP); err != nil { - logger(err.Error()) - return false - } - if err := normalizeBind(&c.Addresses.RPC, bind, &c.Ports.RPC); err != nil { - logger(err.Error()) - return false - } - if err := normalizeBind(&c.Addresses.Serf, bind, &c.Ports.Serf); err != nil { - logger(err.Error()) - return false - } - - if err := normalizeAdvertise(&c.AdvertiseAddrs.HTTP, bind, c.Ports.HTTP, dev); err != nil { - logger(err.Error()) - return false - } - if err := normalizeAdvertise(&c.AdvertiseAddrs.RPC, bind, c.Ports.RPC, dev); err != nil { - logger(err.Error()) - return false - } - if err := normalizeAdvertise(&c.AdvertiseAddrs.Serf, bind, c.Ports.Serf, dev); err != nil { - logger(err.Error()) - return false - } - - if c.Server.EncryptKey != "" { - if _, err := c.Server.EncryptBytes(); err != nil { - logger(fmt.Sprintf("Invalid encryption key: %s", err)) - return false - } - keyfile := filepath.Join(c.DataDir, serfKeyring) - if _, err := os.Stat(keyfile); err == nil { - logger("WARNING: keyring exists but -encrypt given, using keyring") - } +// If addr is set it is used, if not the default bind address is used. +func normalizeBind(addr, bind string) string { + if addr == "" { + return bind } - return true + return addr } -// Use normalizeAdvertise(&config.AdvertiseAddrs., ) to -// retrieve a default address for advertising if one isn't set. -func normalizeAdvertise(addr *string, bind string, defport int, dev bool) error { - if *addr != "" { +// normalizeAdvertise returns a normalized advertise address. +// +// If addr is set, it is used and the default port is appended if no port is +// set. +// +// If addr is not set and bind is a valid address, the returned string is the +// bind+port. +// +// If addr is not set and bind is not a valid advertise address, the hostname +// is resolved and returned with the port. +// +// Loopback is only considered a valid advertise address in dev mode. +func normalizeAdvertise(addr string, bind string, defport int, dev bool) (string, error) { + if addr != "" { // Default to using manually configured address - _, _, 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) + return "", fmt.Errorf("Error parsing advertise address %q: %v", addr, err) } // missing port, append the default - newaddr := fmt.Sprintf("%s:%d", *addr, defport) - *addr = newaddr - return nil + return fmt.Sprintf("%s:%d", addr, defport), nil } - return nil - } - - // Fallback to Bind address if it's not 0.0.0.0 - if bind != "0.0.0.0" { - newaddr := fmt.Sprintf("%s:%d", bind, defport) - *addr = newaddr - return nil + return addr, nil } - // As a last resort resolve the hostname and use it if it's not - // localhost (as localhost is never a sensible default) - host, err := os.Hostname() + // Fallback to bind address first, and then try resolving the local hostname + ips, err := net.LookupIP(bind) if err != nil { - return fmt.Errorf("Unable to get hostname to set advertise address: %v", err) + return "", fmt.Errorf("Error resolving bind address %q: %v", bind, err) } - ip, err := net.ResolveIPAddr("ip", host) - if err != nil { - //TODO It'd be nice if we could advertise unresolvable - // addresses for configurations where this process may have - // different name resolution than the rest of the cluster. - // Unfortunately both serf/memberlist and Nomad's raft layer - // require resovlable advertise addresses. - return fmt.Errorf("Unable to resolve hostname to set advertise address: %v", err) - } - if ip.IP.IsLoopback() && !dev { - return fmt.Errorf("Unable to select default advertise address as hostname resolves to localhost") - } - newaddr := fmt.Sprintf("%s:%d", ip.IP.String(), defport) - *addr = newaddr - return nil -} -func normalizeBind(addr *string, bind string, defport *int) error { - if *addr == "" { - newaddr := fmt.Sprintf("%s:%d", bind, *defport) - *addr = newaddr - return nil + // Return the first unicast address + for _, ip := range ips { + if ip.IsLinkLocalUnicast() || ip.IsGlobalUnicast() { + return fmt.Sprintf("%s:%d", ip, defport), nil + } + if ip.IsLoopback() && dev { + // loopback is fine for dev mode + return fmt.Sprintf("%s:%d", ip, defport), nil + } } - _, portstr, err := net.SplitHostPort(*addr) + // As a last resort resolve the hostname and use it if it's not + // localhost (as localhost is never a sensible default) + host, err := os.Hostname() if err != nil { - if !isMissingPort(err) { - return fmt.Errorf("Error parsing bind address %q: %v", err, *addr) - } - - // missing port, add default - newaddr := fmt.Sprintf("%s:%d", *addr, *defport) - *addr = newaddr - return nil + return "", fmt.Errorf("Unable to get hostname to set advertise address: %v", err) } - port, err := strconv.Atoi(portstr) + ips, err = net.LookupIP(host) if err != nil { - return fmt.Errorf("Error parsing bind address's port: %q: %v", portstr, *addr) + return "", fmt.Errorf("Error resolving hostname %q for advertise address: %v", host, err) } - // Set the default port for this service to the bound port to keep - // configuration consistent. - if port != *defport { - *defport = port + // Return the first unicast address + for _, ip := range ips { + if ip.IsLinkLocalUnicast() || ip.IsGlobalUnicast() { + return fmt.Sprintf("%s:%d", ip, defport), nil + } + if ip.IsLoopback() && dev { + // loopback is fine for dev mode + return fmt.Sprintf("%s:%d", ip, defport), nil + } } - - return nil + return "", fmt.Errorf("No valid advertise addresses, please set `advertise` manually") } // isMissingPort returns true if an error is a "missing port" error from diff --git a/command/agent/http.go b/command/agent/http.go index fe3c417cfec..90fbbcdbb00 100644 --- a/command/agent/http.go +++ b/command/agent/http.go @@ -49,7 +49,7 @@ type HTTPServer struct { // NewHTTPServer starts new HTTP server over the agent func NewHTTPServer(agent *Agent, config *Config, logOutput io.Writer) (*HTTPServer, error) { // Start the listener - lnAddr, err := net.ResolveTCPAddr("tcp", config.Addresses.HTTP) + lnAddr, err := net.ResolveTCPAddr("tcp", config.normalizedAddrs.HTTP) if err != nil { return nil, err } From 5fa84d5658bf1de74ff6cd7b5706dd8483cbfb06 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Wed, 9 Nov 2016 11:55:10 -0800 Subject: [PATCH 13/15] Add unit test for missing port helper func --- command/agent/config_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/command/agent/config_test.go b/command/agent/config_test.go index 40c9eca2771..2c8571fa815 100644 --- a/command/agent/config_test.go +++ b/command/agent/config_test.go @@ -2,6 +2,7 @@ package agent import ( "io/ioutil" + "net" "os" "path/filepath" "reflect" @@ -541,3 +542,14 @@ func TestResources_ParseReserved(t *testing.T) { } } + +func TestIsMissingPort(t *testing.T) { + _, _, err := net.SplitHostPort("localhost") + if missing := isMissingPort(err); !missing { + t.Errorf("expected missing port error, but got %v", err) + } + _, _, err = net.SplitHostPort("localhost:9000") + if missing := isMissingPort(err); missing { + t.Errorf("expected no error, but got %v", err) + } +} From 3b4fe9a95be4affc97cb51d093ca67157cdcedf1 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Wed, 9 Nov 2016 13:16:56 -0800 Subject: [PATCH 14/15] Fix typo in test --- command/agent/agent_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/agent/agent_test.go b/command/agent/agent_test.go index 9615c027ac1..71283dcef02 100644 --- a/command/agent/agent_test.go +++ b/command/agent/agent_test.go @@ -178,7 +178,7 @@ func TestAgent_ServerConfig(t *testing.T) { t.Fatalf("expect 127.0.0.2, got: %s", addr) } if port := out.SerfConfig.MemberlistConfig.BindPort; port != 4004 { - t.Fatalf("expect 4648, got: ^d", port) + t.Fatalf("expect 4648, got: %d", port) } if addr := conf.Addresses.HTTP; addr != "127.0.0.2" { t.Fatalf("expect 127.0.0.2, got: %s", addr) From 778dfeffc4d44ae4d9098d0ef4b46ac75ade4e50 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Wed, 9 Nov 2016 15:51:00 -0800 Subject: [PATCH 15/15] Fix comment --- command/agent/config.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/command/agent/config.go b/command/agent/config.go index e9b1cdfe809..95f26b21a2a 100644 --- a/command/agent/config.go +++ b/command/agent/config.go @@ -663,8 +663,8 @@ func (c *Config) Merge(b *Config) *Config { return &result } -// normalize Addresses and AdvertiseAddrs to always be initialized and have -// sane defaults. +// normalizeAddrs normalizes Addresses and AdvertiseAddrs to always be +// initialized and have sane defaults. func (c *Config) normalizeAddrs() error { c.Addresses.HTTP = normalizeBind(c.Addresses.HTTP, c.BindAddr) c.Addresses.RPC = normalizeBind(c.Addresses.RPC, c.BindAddr)