Choose safer default advertise address #1955

schmichael · 2016-11-08T01:10:13Z

-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.

schmichael · 2016-11-08T01:10:52Z

command/agent/agent.go

@@ -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)


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

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

dadgar · 2016-11-08T01:24:01Z

command/agent/agent.go

@@ -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)


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

dadgar · 2016-11-08T01:24:27Z

command/agent/agent.go

@@ -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)


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

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.

dadgar · 2016-11-08T01:26:10Z

command/agent/command.go

+	// Use getAdvertise(&config.AdvertiseAddrs.<Service>, <Port>) to
+	// retrieve a default address for advertising if one isn't set.
+	defaultAdvertise := ""
+	getAdvertise := func(addr *string, port int) bool {


make it its own function func(addr string, port int) (string, error) so that it can be tested

dadgar · 2016-11-08T01:31:01Z

nomad/server.go

@@ -234,6 +234,7 @@ func NewServer(config *Config, consulSyncer *consul.Syncer, logger *log.Logger)
 	}

 	// Initialize the RPC layer
+


dadgar · 2016-11-08T01:31:18Z

command/agent/command.go

+		}
+
+		// Autoconfigure using previously set default
+		if defaultAdvertise != "" {


After its own function think you can remove this

dadgar · 2016-11-08T01:32:11Z

command/agent/command.go

+	// retrieve a default address for advertising if one isn't set.
+	defaultAdvertise := ""
+	getAdvertise := func(addr *string, port int) bool {
+		if *addr != "" {


Should still be an error if they explicitly advertise 0.0.0.0 or localhost (in any of its ipv4 or v6 forms, or via resolution)

dadgar · 2016-11-08T01:32:51Z

command/agent/command.go

+	defaultAdvertise := ""
+	getAdvertise := func(addr *string, port int) bool {
+		if *addr != "" {
+			// Default to using manually configured address


We should be able to append the default port (see original behavior)

* -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.

dadgar · 2016-11-09T00:31:49Z

command/agent/agent_test.go

-	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)


dadgar · 2016-11-09T00:32:50Z

command/agent/config.go

+//
+// 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 {


Can we return an error instead?

dadgar · 2016-11-09T00:38:05Z

command/agent/config.go

+
+	// Set the default port for this service to the bound port to keep
+	// configuration consistent.
+	if port != *defport {


What are you doing here? Have to say not really a fan of taking a pointer to the value. More idiomatic to just return the new value and not modify the caller

Store address+port in an unexported field for ease-of-use

dadgar · 2016-11-09T22:43:25Z

command/agent/config.go

@@ -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


Comment is a little wrong

dadgar · 2016-11-09T22:50:52Z

command/agent/config.go

+	if err != nil {
+		return fmt.Errorf("Failed to parse HTTP advertise address: %v", err)
+	}
+	c.AdvertiseAddrs.HTTP = addr


Fine for this PR but I would have imagined having normalizedBindAddrs and normalizedAdvertiseAddrs and never modifying the user supplied configs.

Made an issue and assigned to me - #1971

dadgar · 2016-11-09T22:52:16Z

command/agent/config.go

+//
+// 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 != "" {


What if given a host name?

[summarizing offline discussion here]

Uses it here but hard fails later in rpc/serf code that resolves advertise addresses.

This matches 0.4.1 behavior, and trying to fix rpc & serf so that they don't resolve advertise addresses is out of scope for 0.5.

Leaving as-is.

Update address docs to match behavior in #1955

github-actions · 2023-04-14T02:08:16Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

schmichael commented Nov 8, 2016

View reviewed changes

dadgar reviewed Nov 8, 2016

View reviewed changes

schmichael added 3 commits November 8, 2016 11:17

Choose safer default advertise address

4e7587d

* -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.

Failure to resolve advertise address isn't fatal

3dab503

Refactor getAdvertise into a func

2c4b19f

schmichael force-pushed the b-fix-default-advertise branch from 4c2e6cc to 2c4b19f Compare November 8, 2016 21:07

schmichael added 8 commits November 8, 2016 13:44

Handle missing ports in addresses

581d6a5

Match old error messages

9420e73

Move config normalization into config.go to ease testing

5cc435a

Fix int pointer formatting and server config test

eb89fd2

Normalize configs for agent tests

4f15cac

Restore sync/atomic use for consistency

4aed5ba

Fix test that fails now that advertise can't be localhost

1e0b4d7

Enable dev mode to allow using localhost in tests

1893772

dadgar requested changes Nov 9, 2016

View reviewed changes

schmichael added 3 commits November 9, 2016 11:49

Addresses are just addresses - no ports

1304ba8

Store address+port in an unexported field for ease-of-use

Add unit test for missing port helper func

5fa84d5

Fix typo in test

3b4fe9a

dadgar approved these changes Nov 9, 2016

View reviewed changes

Fix comment

778dfef

schmichael merged commit fa67fb6 into master Nov 10, 2016

schmichael deleted the b-fix-default-advertise branch November 10, 2016 00:03

schmichael added a commit that referenced this pull request Nov 10, 2016

Update address docs to match behavior in #1955

333ae6f

schmichael added a commit that referenced this pull request Nov 10, 2016

Merge pull request #1972 from hashicorp/b-update-advertise-docs

019e244

Update address docs to match behavior in #1955

devendram pushed a commit to pubnub/nomad that referenced this pull request Nov 14, 2016

Update address docs to match behavior in hashicorp#1955

552a86f

github-actions bot locked as resolved and limited conversation to collaborators Apr 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Choose safer default advertise address #1955

Choose safer default advertise address #1955

schmichael commented Nov 8, 2016

schmichael Nov 8, 2016

dadgar Nov 8, 2016

dadgar Nov 8, 2016

dadgar Nov 8, 2016

schmichael Nov 8, 2016

dadgar Nov 8, 2016

dadgar Nov 8, 2016

dadgar Nov 8, 2016

dadgar Nov 8, 2016

dadgar Nov 8, 2016

dadgar Nov 9, 2016

dadgar Nov 9, 2016

dadgar Nov 9, 2016

dadgar Nov 9, 2016

dadgar Nov 9, 2016

schmichael Nov 10, 2016

dadgar Nov 9, 2016

schmichael Nov 9, 2016

github-actions bot commented Apr 14, 2023

		@@ -234,6 +234,7 @@ func NewServer(config Config, consulSyncer consul.Syncer, logger *log.Logger)
		}

		// Initialize the RPC layer

Choose safer default advertise address #1955

Choose safer default advertise address #1955

Conversation

schmichael commented Nov 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 14, 2023