Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Choose safer default advertise address #1955

Merged
merged 15 commits into from
Nov 10, 2016
Merged

Choose safer default advertise address #1955

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
4e7587d
Choose safer default advertise address
schmichael Nov 4, 2016
3dab503
Failure to resolve advertise address isn't fatal
schmichael Nov 8, 2016
2c4b19f
Refactor getAdvertise into a func
schmichael Nov 8, 2016
581d6a5
Handle missing ports in addresses
schmichael Nov 8, 2016
9420e73
Match old error messages
schmichael Nov 8, 2016
5cc435a
Move config normalization into config.go to ease testing
schmichael Nov 8, 2016
eb89fd2
Fix int pointer formatting and server config test
schmichael Nov 9, 2016
4f15cac
Normalize configs for agent tests
schmichael Nov 9, 2016
4aed5ba
Restore sync/atomic use for consistency
schmichael Nov 9, 2016
1e0b4d7
Fix test that fails now that advertise can't be localhost
schmichael Nov 9, 2016
1893772
Enable dev mode to allow using localhost in tests
schmichael Nov 9, 2016
1304ba8
Addresses are just addresses - no ports
schmichael Nov 9, 2016
5fa84d5
Add unit test for missing port helper func
schmichael Nov 9, 2016
3b4fe9a
Fix typo in test
schmichael Nov 9, 2016
778dfef
Fix comment
schmichael Nov 9, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 9 additions & 9 deletions command/agent/agent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down
106 changes: 51 additions & 55 deletions command/agent/agent_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand All @@ -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
Expand All @@ -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 {
Expand All @@ -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)
Expand All @@ -202,55 +180,64 @@ func TestAgent_ServerConfig(t *testing.T) {
if port := out.SerfConfig.MemberlistConfig.BindPort; port != 4004 {
t.Fatalf("expect 4648, got: ^d", port)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

%d

}
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") {
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")
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 {
t.Fatalf("expect 10s, got: %s", threshold)
}

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") {
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")
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 {
Expand All @@ -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 {
Expand All @@ -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)
}

Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down
20 changes: 19 additions & 1 deletion command/agent/command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand All @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion command/agent/command_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down