From c772cecaab69eb0430e70b8f756f06eda6c2e288 Mon Sep 17 00:00:00 2001 From: Frank Schroeder Date: Wed, 3 May 2017 23:47:25 +0200 Subject: [PATCH] Do not modify config after creation II Move code for finding the advertise address via a template into consulConfig() so that the config object is not modified after creation. --- command/agent/agent.go | 115 +++++++++++++-------------- command/agent/agent_endpoint_test.go | 1 + command/agent/agent_test.go | 42 ++++++---- consul/server_test.go | 1 + 4 files changed, 84 insertions(+), 75 deletions(-) diff --git a/command/agent/agent.go b/command/agent/agent.go index cebd973a2..7f80c246f 100644 --- a/command/agent/agent.go +++ b/command/agent/agent.go @@ -133,8 +133,7 @@ type Agent struct { // Create is used to create a new Agent. Returns // the agent or potentially an error. -func Create(config *Config, logOutput io.Writer, logWriter *logger.LogWriter, - reloadCh chan chan error) (*Agent, error) { +func Create(config *Config, logOutput io.Writer, logWriter *logger.LogWriter, reloadCh chan chan error) (*Agent, error) { // Ensure we have a log sink if logOutput == nil { logOutput = os.Stderr @@ -148,54 +147,6 @@ func Create(config *Config, logOutput io.Writer, logWriter *logger.LogWriter, return nil, fmt.Errorf("Must configure a DataDir") } - // Try to get an advertise address - if config.AdvertiseAddr != "" { - ipStr, err := parseSingleIPTemplate(config.AdvertiseAddr) - if err != nil { - return nil, fmt.Errorf("Advertise address resolution failed: %v", err) - } - config.AdvertiseAddr = ipStr - - if ip := net.ParseIP(config.AdvertiseAddr); ip == nil { - return nil, fmt.Errorf("Failed to parse advertise address: %v", config.AdvertiseAddr) - } - } else if config.BindAddr != "0.0.0.0" && config.BindAddr != "" && config.BindAddr != "[::]" { - config.AdvertiseAddr = config.BindAddr - } else { - var err error - var ip net.IP - if config.BindAddr == "[::]" { - ip, err = consul.GetPublicIPv6() - } else { - ip, err = consul.GetPrivateIP() - } - if err != nil { - return nil, fmt.Errorf("Failed to get advertise address: %v", err) - } - config.AdvertiseAddr = ip.String() - } - - // Try to get an advertise address for the wan - if config.AdvertiseAddrWan != "" { - ipStr, err := parseSingleIPTemplate(config.AdvertiseAddrWan) - if err != nil { - return nil, fmt.Errorf("Advertise WAN address resolution failed: %v", err) - } - config.AdvertiseAddrWan = ipStr - - if ip := net.ParseIP(config.AdvertiseAddrWan); ip == nil { - return nil, fmt.Errorf("Failed to parse advertise address for wan: %v", config.AdvertiseAddrWan) - } - } else { - config.AdvertiseAddrWan = config.AdvertiseAddr - } - - // Create the default set of tagged addresses. - config.TaggedAddresses = map[string]string{ - "lan": config.AdvertiseAddr, - "wan": config.AdvertiseAddrWan, - } - agent := &Agent{ config: config, logger: log.New(logOutput, "", log.LstdFlags), @@ -288,7 +239,7 @@ func Create(config *Config, logOutput io.Writer, logWriter *logger.LogWriter, } // consulConfig is used to return a consul configuration -func (a *Agent) consulConfig() *consul.Config { +func (a *Agent) consulConfig() (*consul.Config, error) { // Start with the provided config or default config base := consul.DefaultConfig() if a.config.ConsulConfig != nil { @@ -342,6 +293,52 @@ func (a *Agent) consulConfig() *consul.Config { if a.config.SerfWanBindAddr != "" { base.SerfWANConfig.MemberlistConfig.BindAddr = a.config.SerfWanBindAddr } + // Try to get an advertise address + switch { + case a.config.AdvertiseAddr != "": + ipStr, err := parseSingleIPTemplate(a.config.AdvertiseAddr) + if err != nil { + return nil, fmt.Errorf("Advertise address resolution failed: %v", err) + } + if net.ParseIP(ipStr) == nil { + return nil, fmt.Errorf("Failed to parse advertise address: %v", ipStr) + } + a.config.AdvertiseAddr = ipStr + + case a.config.BindAddr != "0.0.0.0" && a.config.BindAddr != "" && a.config.BindAddr != "[::]": + a.config.AdvertiseAddr = a.config.BindAddr + + default: + ip, err := consul.GetPrivateIP() + if a.config.BindAddr == "[::]" { + ip, err = consul.GetPublicIPv6() + } + if err != nil { + return nil, fmt.Errorf("Failed to get advertise address: %v", err) + } + a.config.AdvertiseAddr = ip.String() + } + + // Try to get an advertise address for the wan + if a.config.AdvertiseAddrWan != "" { + ipStr, err := parseSingleIPTemplate(a.config.AdvertiseAddrWan) + if err != nil { + return nil, fmt.Errorf("Advertise WAN address resolution failed: %v", err) + } + if net.ParseIP(ipStr) == nil { + return nil, fmt.Errorf("Failed to parse advertise address for WAN: %v", ipStr) + } + a.config.AdvertiseAddrWan = ipStr + } else { + a.config.AdvertiseAddrWan = a.config.AdvertiseAddr + } + + // Create the default set of tagged addresses. + a.config.TaggedAddresses = map[string]string{ + "lan": a.config.AdvertiseAddr, + "wan": a.config.AdvertiseAddrWan, + } + if a.config.AdvertiseAddr != "" { base.SerfLANConfig.MemberlistConfig.AdvertiseAddr = a.config.AdvertiseAddr base.SerfWANConfig.MemberlistConfig.AdvertiseAddr = a.config.AdvertiseAddr @@ -476,7 +473,7 @@ func (a *Agent) consulConfig() *consul.Config { // Setup the loggers base.LogOutput = a.logOutput - return base + return base, nil } // parseSingleIPTemplate is used as a helper function to parse out a single IP @@ -588,12 +585,13 @@ func (a *Agent) resolveTmplAddrs() error { // setupServer is used to initialize the Consul server func (a *Agent) setupServer() error { - config := a.consulConfig() - + config, err := a.consulConfig() + if err != nil { + return err + } if err := a.setupKeyrings(config); err != nil { return fmt.Errorf("Failed to configure keyring: %v", err) } - server, err := consul.NewServer(config) if err != nil { return fmt.Errorf("Failed to start Consul server: %v", err) @@ -604,12 +602,13 @@ func (a *Agent) setupServer() error { // setupClient is used to initialize the Consul client func (a *Agent) setupClient() error { - config := a.consulConfig() - + config, err := a.consulConfig() + if err != nil { + return err + } if err := a.setupKeyrings(config); err != nil { return fmt.Errorf("Failed to configure keyring: %v", err) } - client, err := consul.NewClient(config) if err != nil { return fmt.Errorf("Failed to start Consul client: %v", err) diff --git a/command/agent/agent_endpoint_test.go b/command/agent/agent_endpoint_test.go index 9a85547dc..fe6eedc86 100644 --- a/command/agent/agent_endpoint_test.go +++ b/command/agent/agent_endpoint_test.go @@ -337,6 +337,7 @@ func TestAgent_Reload(t *testing.T) { args := []string{ "-server", + "-advertise", "127.0.0.1", "-data-dir", tmpDir, "-http-port", fmt.Sprintf("%d", conf.Ports.HTTP), "-config-file", tmpFile.Name(), diff --git a/command/agent/agent_test.go b/command/agent/agent_test.go index 2c4abde79..d8b8f2125 100644 --- a/command/agent/agent_test.go +++ b/command/agent/agent_test.go @@ -195,12 +195,12 @@ func TestAgent_CheckSerfBindAddrsSettings(t *testing.T) { defer os.RemoveAll(dir) defer agent.Shutdown() - serfWanBind := agent.consulConfig().SerfWANConfig.MemberlistConfig.BindAddr + serfWanBind := consulConfig(agent).SerfWANConfig.MemberlistConfig.BindAddr if serfWanBind != ip { t.Fatalf("SerfWanBindAddr is should be a non-loopback IP not %s", serfWanBind) } - serfLanBind := agent.consulConfig().SerfLANConfig.MemberlistConfig.BindAddr + serfLanBind := consulConfig(agent).SerfLANConfig.MemberlistConfig.BindAddr if serfLanBind != ip { t.Fatalf("SerfLanBindAddr is should be a non-loopback IP not %s", serfWanBind) } @@ -214,23 +214,23 @@ func TestAgent_CheckAdvertiseAddrsSettings(t *testing.T) { defer os.RemoveAll(dir) defer agent.Shutdown() - serfLanAddr := agent.consulConfig().SerfLANConfig.MemberlistConfig.AdvertiseAddr + serfLanAddr := consulConfig(agent).SerfLANConfig.MemberlistConfig.AdvertiseAddr if serfLanAddr != "127.0.0.42" { t.Fatalf("SerfLan is not properly set to '127.0.0.42': %s", serfLanAddr) } - serfLanPort := agent.consulConfig().SerfLANConfig.MemberlistConfig.AdvertisePort + serfLanPort := consulConfig(agent).SerfLANConfig.MemberlistConfig.AdvertisePort if serfLanPort != 1233 { t.Fatalf("SerfLan is not properly set to '1233': %d", serfLanPort) } - serfWanAddr := agent.consulConfig().SerfWANConfig.MemberlistConfig.AdvertiseAddr + serfWanAddr := consulConfig(agent).SerfWANConfig.MemberlistConfig.AdvertiseAddr if serfWanAddr != "127.0.0.43" { t.Fatalf("SerfWan is not properly set to '127.0.0.43': %s", serfWanAddr) } - serfWanPort := agent.consulConfig().SerfWANConfig.MemberlistConfig.AdvertisePort + serfWanPort := consulConfig(agent).SerfWANConfig.MemberlistConfig.AdvertisePort if serfWanPort != 1234 { t.Fatalf("SerfWan is not properly set to '1234': %d", serfWanPort) } - rpc := agent.consulConfig().RPCAdvertise + rpc := consulConfig(agent).RPCAdvertise if rpc != c.AdvertiseAddrs.RPC { t.Fatalf("RPC is not properly set to %v: %s", c.AdvertiseAddrs.RPC, rpc) } @@ -253,7 +253,7 @@ func TestAgent_CheckPerformanceSettings(t *testing.T) { defer agent.Shutdown() raftMult := time.Duration(consul.DefaultRaftMultiplier) - r := agent.consulConfig().RaftConfig + r := consulConfig(agent).RaftConfig def := raft.DefaultConfig() if r.HeartbeatTimeout != raftMult*def.HeartbeatTimeout || r.ElectionTimeout != raftMult*def.ElectionTimeout || @@ -271,7 +271,7 @@ func TestAgent_CheckPerformanceSettings(t *testing.T) { defer agent.Shutdown() const raftMult time.Duration = 99 - r := agent.consulConfig().RaftConfig + r := consulConfig(agent).RaftConfig def := raft.DefaultConfig() if r.HeartbeatTimeout != raftMult*def.HeartbeatTimeout || r.ElectionTimeout != raftMult*def.ElectionTimeout || @@ -288,12 +288,12 @@ func TestAgent_ReconnectConfigSettings(t *testing.T) { defer os.RemoveAll(dir) defer agent.Shutdown() - lan := agent.consulConfig().SerfLANConfig.ReconnectTimeout + lan := consulConfig(agent).SerfLANConfig.ReconnectTimeout if lan != 3*24*time.Hour { t.Fatalf("bad: %s", lan.String()) } - wan := agent.consulConfig().SerfWANConfig.ReconnectTimeout + wan := consulConfig(agent).SerfWANConfig.ReconnectTimeout if wan != 3*24*time.Hour { t.Fatalf("bad: %s", wan.String()) } @@ -307,12 +307,12 @@ func TestAgent_ReconnectConfigSettings(t *testing.T) { defer os.RemoveAll(dir) defer agent.Shutdown() - lan := agent.consulConfig().SerfLANConfig.ReconnectTimeout + lan := consulConfig(agent).SerfLANConfig.ReconnectTimeout if lan != 24*time.Hour { t.Fatalf("bad: %s", lan.String()) } - wan := agent.consulConfig().SerfWANConfig.ReconnectTimeout + wan := consulConfig(agent).SerfWANConfig.ReconnectTimeout if wan != 36*time.Hour { t.Fatalf("bad: %s", wan.String()) } @@ -327,7 +327,7 @@ func TestAgent_setupNodeID(t *testing.T) { defer agent.Shutdown() // The auto-assigned ID should be valid. - id := agent.consulConfig().NodeID + id := consulConfig(agent).NodeID if _, err := uuid.ParseUUID(string(id)); err != nil { t.Fatalf("err: %v", err) } @@ -337,7 +337,7 @@ func TestAgent_setupNodeID(t *testing.T) { if err := agent.setupNodeID(c); err != nil { t.Fatalf("err: %v", err) } - if newID := agent.consulConfig().NodeID; id != newID { + if newID := consulConfig(agent).NodeID; id != newID { t.Fatalf("bad: %q vs %q", id, newID) } @@ -357,7 +357,7 @@ func TestAgent_setupNodeID(t *testing.T) { if err := agent.setupNodeID(c); err != nil { t.Fatalf("err: %v", err) } - if id := agent.consulConfig().NodeID; string(id) != newID { + if id := consulConfig(agent).NodeID; string(id) != newID { t.Fatalf("bad: %q vs. %q", id, newID) } @@ -380,7 +380,7 @@ func TestAgent_setupNodeID(t *testing.T) { if err := agent.setupNodeID(c); err != nil { t.Fatalf("err: %v", err) } - if id := agent.consulConfig().NodeID; string(id) != "adf4238a-882b-9ddc-4a9d-5b6758e4159e" { + if id := consulConfig(agent).NodeID; string(id) != "adf4238a-882b-9ddc-4a9d-5b6758e4159e" { t.Fatalf("bad: %q vs. %q", id, newID) } } @@ -1985,3 +1985,11 @@ func TestAgent_GetCoordinate(t *testing.T) { check(true) check(false) } + +func consulConfig(a *Agent) *consul.Config { + c, err := a.consulConfig() + if err != nil { + panic(err) + } + return c +} diff --git a/consul/server_test.go b/consul/server_test.go index ecdb087e5..bca08b9e0 100644 --- a/consul/server_test.go +++ b/consul/server_test.go @@ -47,6 +47,7 @@ func testServerConfig(t *testing.T, NodeName string) (string, *Config) { IP: []byte{127, 0, 0, 1}, Port: getPort(), } + config.RPCAdvertise = config.RPCAddr nodeID, err := uuid.GenerateUUID() if err != nil { t.Fatal(err)