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 8 commits
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
172 changes: 27 additions & 145 deletions command/agent/agent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (
"strconv"
"strings"
"sync"
"sync/atomic"
"time"

"github.com/hashicorp/nomad/client"
Expand Down Expand Up @@ -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{}
Expand Down Expand Up @@ -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)
Copy link
Member Author

Choose a reason for hiding this comment

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

This code is never called concurrently with any other readers/writers (it's before the server is started), so I removed the atomic.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would just use the atomics. It is easier to just know that all users of the atomic variable are accessing it atomically

}
}
if a.config.DataDir != "" {
Expand All @@ -135,35 +130,28 @@ 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Mentioned offline that it isn't always the case that the client can resolve its advertise address

Copy link
Member Author

Choose a reason for hiding this comment

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

After some investigation and discussion I discovered RPC and Serf advertise addresses are resolved later on in much harder to fix places, so we're not going to worry about it in this PR.

if err != nil {
return nil, err
return nil, fmt.Errorf("Failed to parse RPC address %q: %v", a.config.Addresses.RPC, err)
}
serfAddr, err := a.getSerfAddr(true)
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
conf.SerfConfig.MemberlistConfig.BindPort = serfAddr.Port
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
return nil, fmt.Errorf("Failed to parse RPC advertise address %q: %v", a.config.AdvertiseAddrs.RPC, err)
}
rpcAddr, err = a.getRPCAddr(false)
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)
}
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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -330,26 +313,22 @@ 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
// TODO re-introduce HTTP/S checks when Consul 0.7.1 comes out
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{
Expand All @@ -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{
Expand All @@ -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{
Expand Down Expand Up @@ -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
Expand All @@ -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{
Expand All @@ -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 {
Expand Down