Add advertise_addrs. #1004

hanshasselberg · 2015-06-05T11:46:29Z

Fixes #550.
This will make it possible to configure the advertised adresses for SerfLan, SerfWan and RPC. It will enable multiple consul clients on a single host which is very useful in a container environment.

This option might override advertise_addr and advertise_addr_wan depending on the configuration.

It will be configureable with advertise_addrs. Example:

{
  "advertise_addrs": {
    "serf_lan": "10.0.120.91:4424",
    "serf_wan": "201.20.10.61:4423",
    "rpc": "10.20.10.61:4424"
  }
}

ryanuber · 2015-06-05T17:31:29Z

command/agent/agent.go

+			base.SerfWANConfig.MemberlistConfig.AdvertiseAddr = addr.IP.String()
+			base.SerfWANConfig.MemberlistConfig.AdvertisePort = addr.Port
+		}
+	}


Instead of printing a message for both of the above and allowing Consul to start even with a bad advertise address specified in its config, can we store the *net.TCPAddrs in the AdvertiseAddrs struct and parse the address while we are decoding? This would also make a config merge pretty easy for these fields. We do a similar thing with parsing time values.

ryanuber · 2015-06-05T17:35:01Z

Hey @i0rek, I took a quick look through what you have so far and I think you're on the right track. We are missing a few things. See my comment above. We will also need to add the new config fields to the Merge method in a *Config, so that the new config options may be specified in any configuration file and not just the first one on the list.

hanshasselberg · 2015-06-06T05:27:36Z

Thank you very much for the suggestions @ryanuber! I will take care of them beginning next week.

hanshasselberg · 2015-06-08T16:00:46Z

@ryanuber I think I added everything you suggested. Still have to test it though.

ryanuber · 2015-06-08T20:04:46Z

command/agent/config.go

+		addr, err := net.ResolveTCPAddr("tcp", result.AdvertiseAddrs.SerfLanRaw)
+		if err != nil {
+			return nil, fmt.Errorf("AdvertiseAddrs.SerfLan is invalid: %v", err)
+		} else {


Minor nit-pick but let's drop these else statements, since we already return in the error path above.

I am sorry, I should've paid more attention to the other code.

ryanuber · 2015-06-08T20:29:38Z

@i0rek this is looking good. Ping me when you feel it is complete and I'll give it another once-over!

hanshasselberg · 2015-06-09T05:36:00Z

In theory this could be necessary for CLI RPC, HTTP API and DNS Interface too. What do you think? When any of the above available from the outside it would be good to include them in the advertise_addrs configuration.

hanshasselberg · 2015-06-09T11:56:12Z

Ok, it works fine for our usecase. Which is letting consul run in two containers on one host, connecting to a remote server. This is the configuration I used:

{ "advertise_addr": "10.111.111.112", "advertise_addrs": { "serf_lan": "10.111.111.112:9301" } }

I don't think RPC works yet, but I could add that afterwards.

docker1            10.111.111.112:9301    alive   client  0.5.2  2
docker2            10.111.111.112:8301    alive   client  0.5.2  2

hanshasselberg · 2015-06-09T12:32:24Z

@ryanuber I added RPC to the mix, because that was missing. I also added a little bit of documentation, not sure how you handle that. I was planning to rebase a last time after you looked at it so that you have a nice commit you can merge. I am not sure how you handle that either.

ryanuber · 2015-06-12T18:52:32Z

command/agent/config.go

+	}
+	if b.AdvertiseAddrs.RPC != nil {
+		result.AdvertiseAddrs.RPC = b.AdvertiseAddrs.RPC
+	}


Looks like we are missing tests for this. Let's definitely make sure this is tested.

ryanuber · 2015-06-12T18:56:21Z

@i0rek this looks good, just some minor notes about test coverage but overall this is really close! The documentation looks right. Also, no need to rebase this, since it can be cleanly applied against master in its current state.

hanshasselberg · 2015-06-13T22:40:29Z

Thanks for the review @ryanuber. I added the tests.

ryanuber · 2015-06-14T00:42:18Z

Great! Will review again soon!

On June 13, 2015 3:40:35 PM PDT, Hans Hasselberg notifications@github.com wrote:

Thanks for the review @ryanuber. I added the tests.

Reply to this email directly or view it on GitHub:
#1004 (comment)

Sent from my Android device with K-9 Mail. Please excuse my brevity.

hanshasselberg · 2015-06-19T08:23:32Z

When I said rebasing, I meant squashing. I couldn't resist.

hanshasselberg · 2015-06-19T09:30:02Z

I added tests to agent_test to make sure the configured values are used.

hanshasselberg · 2015-06-23T17:46:28Z

@ryanuber any progress on this? Seems ready to merge to me. Thanks

ryanuber · 2015-06-23T18:00:59Z

website/source/docs/agent/options.html.markdown

@@ -322,6 +322,16 @@ definitions support being updated during a reload.
 * <a name="advertise_addr"></a><a href="#advertise_addr">`advertise_addr`</a> Equivalent to
  the [`-advertise` command-line flag](#_advertise).

+* <a name="advertise_addrs"></a><a href="#advertise_addrs">`advertise_addrs`</a> Allows to set
+  the advertised addresses for SerfLan, SerfLan and RPC together with the port. This gives


I think we meant for this to be SerfLan, SerfWan, and RPC ?

ryanuber · 2015-06-23T18:01:27Z

@i0rek sorry for the delay! It's been busy. Left a super-minor comment but otherwise let's get this merged. Great work!

Fixes #550. This will make it possible to configure the advertised adresses for SerfLan, SerfWan and RPC. It will enable multiple consul clients on a single host which is very useful in a container environment. This option might override advertise_addr and advertise_addr_wan depending on the configuration. It will be configureable with advertise_addrs. Example: { "advertise_addrs": { "serf_lan": "10.0.120.91:4424", "serf_wan": "201.20.10.61:4423", "rpc": "10.20.10.61:4424" } }

hanshasselberg · 2015-06-23T19:24:14Z

@ryanuber no worries. I changed the typo!

ryanuber · 2015-06-23T19:32:04Z

LGTM, thanks again!

Add advertise_addrs.

rotatingJazz · 2015-06-23T19:33:23Z

BIG props!!! 👍

hanshasselberg · 2015-06-23T19:36:46Z

❤️

ryanuber reviewed Jun 5, 2015
View reviewed changes

hanshasselberg changed the title ~~Add advertise_addrs.~~ [HOLD] Add advertise_addrs. Jun 8, 2015

ryanuber reviewed Jun 8, 2015
View reviewed changes

hanshasselberg changed the title ~~[HOLD] Add advertise_addrs.~~ [WIP] Add advertise_addrs. Jun 9, 2015

hanshasselberg changed the title ~~[WIP] Add advertise_addrs.~~ Add advertise_addrs. Jun 9, 2015

ryanuber reviewed Jun 12, 2015
View reviewed changes

ryanuber reviewed Jun 23, 2015
View reviewed changes

ryanuber added a commit that referenced this pull request Jun 23, 2015

Merge pull request #1004 from i0rek/advertise_addrs

d0348d1

Add advertise_addrs.

ryanuber merged commit d0348d1 into hashicorp:master Jun 23, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add advertise_addrs. #1004

Add advertise_addrs. #1004

hanshasselberg commented Jun 5, 2015

ryanuber Jun 5, 2015

ryanuber commented Jun 5, 2015

hanshasselberg commented Jun 6, 2015

hanshasselberg commented Jun 8, 2015

ryanuber Jun 8, 2015

hanshasselberg Jun 8, 2015

ryanuber commented Jun 8, 2015

hanshasselberg commented Jun 9, 2015

hanshasselberg commented Jun 9, 2015

hanshasselberg commented Jun 9, 2015

ryanuber Jun 12, 2015

ryanuber commented Jun 12, 2015

hanshasselberg commented Jun 13, 2015

ryanuber commented Jun 14, 2015

hanshasselberg commented Jun 19, 2015

hanshasselberg commented Jun 19, 2015

hanshasselberg commented Jun 23, 2015

ryanuber Jun 23, 2015

ryanuber commented Jun 23, 2015

hanshasselberg commented Jun 23, 2015

ryanuber commented Jun 23, 2015

rotatingJazz commented Jun 23, 2015

hanshasselberg commented Jun 23, 2015

Add advertise_addrs. #1004

Add advertise_addrs. #1004

Conversation

hanshasselberg commented Jun 5, 2015

ryanuber Jun 5, 2015

Choose a reason for hiding this comment

ryanuber commented Jun 5, 2015

hanshasselberg commented Jun 6, 2015

hanshasselberg commented Jun 8, 2015

ryanuber Jun 8, 2015

Choose a reason for hiding this comment

hanshasselberg Jun 8, 2015

Choose a reason for hiding this comment

ryanuber commented Jun 8, 2015

hanshasselberg commented Jun 9, 2015

hanshasselberg commented Jun 9, 2015

hanshasselberg commented Jun 9, 2015

ryanuber Jun 12, 2015

Choose a reason for hiding this comment

ryanuber commented Jun 12, 2015

hanshasselberg commented Jun 13, 2015

ryanuber commented Jun 14, 2015

hanshasselberg commented Jun 19, 2015

hanshasselberg commented Jun 19, 2015

hanshasselberg commented Jun 23, 2015

ryanuber Jun 23, 2015

Choose a reason for hiding this comment

ryanuber commented Jun 23, 2015

hanshasselberg commented Jun 23, 2015

ryanuber commented Jun 23, 2015

rotatingJazz commented Jun 23, 2015

hanshasselberg commented Jun 23, 2015