Advertise driver-specific addresses #2709

schmichael · 2017-06-15T23:30:25Z

Summary

DriverNetwork struct returned by Driver.Prestart and Driver.Start containing the PortMap and IP set by the Driver.
NOMAD_{HOST,DRIVER}_{ADDR,IP,PORT}_<label> env vars
service.address_mode to determine what IP to advertise

DriverNetwork

Ideally this would only need to be returned by Prestart, but Docker doesn't assign a container an IP until the container starts.

I could have implemented it such that Prestart returns PortMap and Start returns IP/AutoAdvertise, but here's why I didn't:

DriverNetwork is a really nice way to wrap up all of the container network metadata for passing through TaskRunner to Consul ServiceClient. Even if the Driver didn't use it I'd probably keep it around in TaskRunner and Consul.
In theory other drivers may work differently and provide the IP in Prestart. I couldn't actually find where Docker documented their behavior, so it's conceivable they'll even change someday.
This mirrors Docker's own Inspect function: it returns networking information whether it's populated or not.

The last point raises an interesting question: Should I just create Driver.Inspect and have it return `DriverNetwork? TaskRunner would just call it as needed and we wouldn't need to persist DriverNetwork.

Might make for a good followup PR.

`NOMAD_{HOST,DRIVER}_{ADDR,IP,PORT}_<label>` env vars

This was the Cambrian explosion of networking env vars. My work revealed something interesting: NOMAD_ADDR_<label> isn't reliable. If a port_map is set it concatenates the host IP with the driver port. This can only ever be right by pure happenstance.

So I marked it as deprecated and introduced explicit env vars.

The big caveat here is that the DRIVER_IP (and therefore ADDR) env vars can't be populated until the driver returns them -- which right now only happens after Start. This makes it only really useful for script checks. Everything else happens before Start is called.

In theory if other drivers return an IP from Prestart then it would be available in Start (eg args interpolation).

I tried to document this well, but it feels like a pretty big footgun.

`service.address_mode`

Turned out as we hoped! auto, host, or driver where auto means host unless you're using a Docker networking plugin.

Checks always use host IP+Port as we discussed. A bit of a gotcha, but I think it's always the right choice. If it needs to be configurable for esoteric setups we can add that later.

Naming (Driver vs Plugin)

As always I'm willing to changes any names! That being said I went with driver everywhere as it just made sense. It's the network defined by the driver. I think using "plugin" right now would just leave people hunting around for what these "plugins" are -- or conflate it with Docker plugins which is kind of true now but would probably lead to confusion in the future?

dadgar · 2017-06-21T16:54:33Z

nomad/structs/structs.go

@@ -2467,6 +2498,11 @@ func (s *Service) Copy() *Service {
 // Canonicalize interpolates values of Job, Task Group and Task in the Service
 // Name. This also generates check names, service id and check ids.
 func (s *Service) Canonicalize(job string, taskGroup string, task string) {
+	// Default to AddressModeAuto
+	if s.AddressMode == "" {


Can you actually move this to the api pkg. The defaulting should be happening there now.

dadgar · 2017-06-21T16:55:05Z

api/tasks.go

+	Tags        []string
+	PortLabel   string `mapstructure:"port"`
+	AddressMode string `mapstructure:"address_mode"`
+	Checks      []ServiceCheck


Canonicalize below

dadgar · 2017-06-21T16:58:46Z

client/driver/docker.go

+		// This should only happen if there's been a coding error (such
+		// as not calling InspetContainer after CreateContainer). Code
+		// defensively in case the Docker API changes subtly.
+		d.logger.Printf("[WARN] driver.docker: no network settings for container %s", c.ID)


May want to bump the log level

dadgar · 2017-06-21T17:14:53Z

client/driver/docker.go

+		// Don't auto-advertise bridge IPs
+		if name != "bridge" {
+			auto = true
+		}


Just break?

dadgar · 2017-06-21T17:42:41Z

client/driver/qemu.go

@@ -276,7 +276,7 @@ func (d *QemuDriver) Start(ctx *ExecContext, task *structs.Task) (DriverHandle,
 		waitCh:         make(chan *dstructs.WaitResult, 1),
 	}
 	go h.run()
-	return h, nil
+	return &StartResponse{Handle: h}, nil


Does this not need to expose the port map?

We've only ever propagated the port map for the Docker driver despite rkt and qemu also having port maps. Should I go ahead and add that as part of this PR? Should Just Work but would be a new feature (or bugfix depending on perspective).

From offline discussion: going to have rkt and qemu return their port maps.

dadgar · 2017-06-21T17:57:26Z

nomad/structs/structs.go

+const (
+	AddressModeAuto   = "auto"
+	AddressModeHost   = "host"
+	AddressModeDriver = "driver" //FIXME plugin? I forget what we decided because like 1000 other things have gone wrong since then


Remove FIXME

dadgar · 2017-06-21T18:35:46Z

client/driver/env/env.go

+	// allocations to tasks with the driver's IP address if on exists.
+	// Service's will advertise this address if address_mode=true or
+	// address_mode=auto and the driver determines it should be used.
+	DriverAddrPrefix = "NOMAD_DRIVER_ADDR_"


dadgar · 2017-06-21T18:36:16Z

client/driver/env/env.go

+	// Deprecated: Use NOMAD_HOST_ADDR_ or NOMAD_DRIVER_ADDR_ instead as
+	// this environment variable will only ever use the host IP. If a port
+	// map is used this variable will be set to the Host IP and Driver's
+	// Port which likely won't work in either context.


Make it always be host_ip:host_port and don't say deprecated

dadgar · 2017-06-21T18:36:35Z

client/driver/env/env.go

 	AddrPrefix = "NOMAD_ADDR_"

-	// IpPrefix is the prefix for passing the IP of a port allocation to a task.
+	// IpPrefix is the prefix for passing the IP of a port allocation to a
+	// task. This may not be the host's address depending on task


Always host IP

dadgar · 2017-06-21T18:36:57Z

client/driver/env/env.go

 	IpPrefix = "NOMAD_IP_"

 	// PortPrefix is the prefix for passing the port allocation to a task.
 	PortPrefix = "NOMAD_PORT_"

+	// HostAddrPrefix is the prefix for passing both dynamic and static
+	// port allocations to tasks with the host's IP address for cases where
+	// the task advertises a different address.


Remove these

Ideally DriverNetwork would be fully populated in Driver.Prestart, but Docker doesn't assign the container's IP until you start the container. However, it's important to setup the port env vars before calling Driver.Start, so Prestart should populate that.

Also make NOMAD_ADDR_* use host ip:port for consistency. NOMAD_PORT_* varies based on port map and the driver IP isn't exposed as an env var as the only place it can be used is in script checks anyway.

schmichael · 2017-06-22T00:21:27Z

@dadgar Ready for round 2.

dadgar · 2017-06-23T19:30:05Z

client/driver/docker.go

+	return resp, nil
+}
+
+func (d *DockerDriver) detectIP(c *docker.Container) (string, bool) {


Put a comment on what this does and the fields it returns

dadgar · 2017-06-23T19:30:35Z

client/driver/docker.go

+		d.logger.Printf("[ERROR] driver.docker: no network settings for container %s", c.ID)
+		return "", false
+	}
+	ip, ipName := "", ""


Add blank spaces between blocks. Here and line 643

dadgar · 2017-06-23T19:31:58Z

client/driver/env/env.go

+// buildNetworkEnv env vars in the given map.
+//
+//	Auto:   NOMAD_{IP,PORT,ADDR}_<label>
+//	Host:   NOMAD_HOST_{IP,PORT,ADDR}_<label>


Update comment

dadgar · 2017-06-23T19:36:15Z

nomad/structs/structs.go

@@ -2430,6 +2450,12 @@ func (sc *ServiceCheck) Hash(serviceID string) string {
 	return fmt.Sprintf("%x", h.Sum(nil))
 }

+const (
+	AddressModeAuto   = "auto"


dadgar · 2017-06-23T19:36:34Z

website/source/docs/drivers/docker.html.md

+`NOMAD_DRIVER_IP_<label>` environment variable for use in script checks.
+
+The ports specified in the `port_map` are exposed via the
+`NOMAD_DRIVER_PORT_<label>` environment variables. Unlike the IP, ports are


Simplifies things greatly!

Looks like I accidently dropped them when combining env var listings in PR #2709

github-actions · 2023-03-31T02:13:17Z

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 force-pushed the f-advertise-docker-ips branch 2 times, most recently from 6dfd590 to 43a326c Compare June 20, 2017 16:42

schmichael requested a review from dadgar June 20, 2017 16:58

schmichael changed the title ~~[WIP] Advertise driver-specific addresses~~ Advertise driver-specific addresses Jun 20, 2017

dadgar requested changes Jun 21, 2017

View reviewed changes

schmichael force-pushed the f-advertise-docker-ips branch from 5a8964e to 82c89f0 Compare June 22, 2017 00:19

schmichael added 16 commits June 21, 2017 17:19

Test driver network advertisement and checks

8a7df57

Fix Service.AddressMode changes during task updates

4117c8b

Consolidate envvars in a partial template

57b6f6e

Document address_mode

ee5e02f

Skip DRIVER env vars for labels without a port mapping

cda2db2

Fix lxc tests

8ed23d4

Remove debug logging

a464ca3

Add script to demo weave in vagrant

e6552e1

Remove readme

2023946

Bump error log level

81b942e

Simplify Docker Networks processing

deffe5b

Move caonicalization from nomad/structs/ to api/

3f37be3

Have Qemu return PortMap

b5fdf1d

Remove DRIVER env vars

5d7fba4

Also make NOMAD_ADDR_* use host ip:port for consistency. NOMAD_PORT_* varies based on port map and the driver IP isn't exposed as an env var as the only place it can be used is in script checks anyway.

Fixup example

fe3bb07

schmichael force-pushed the f-advertise-docker-ips branch from 82c89f0 to fe3bb07 Compare June 22, 2017 00:19

Remove DRIVER env vars from docs

9ef4a42

dadgar requested changes Jun 23, 2017

View reviewed changes

schmichael added 4 commits June 23, 2017 15:20

Style and comments

a70ad9b

Remove defunct DRIVER references in docs

cc29d74

Simplifies things greatly!

Fix some tests still expecting reverted behavior

8c32582

Fix test error formats

85860dc

This was referenced Jun 29, 2017

Docker exposed port registering with Consul #1124

Closed

Nomad Docker bridge, provide support for Fan Networking #1800

Closed

schmichael merged commit 6b3ae9a into master Jul 3, 2017

schmichael deleted the f-advertise-docker-ips branch July 3, 2017 21:04

schmichael added a commit that referenced this pull request Jul 3, 2017

Add #2654 #2695 #2702 #2709 #2729 #2735 #2746 #2750 #2753

bde854c

schmichael added a commit that referenced this pull request Aug 4, 2017

Document <task> network env vars

29951ab

Looks like I accidently dropped them when combining env var listings in PR #2709

schmichael mentioned this pull request Aug 4, 2017

Document <task> network env vars #2968

Merged

schmichael added a commit that referenced this pull request Aug 17, 2017

Document <task> network env vars

2c24daf

Looks like I accidently dropped them when combining env var listings in PR #2709

42wim mentioned this pull request Jan 23, 2018

Service registration for IPv6 docker addresses #3785

Closed

github-actions bot locked as resolved and limited conversation to collaborators Mar 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Advertise driver-specific addresses #2709

Advertise driver-specific addresses #2709

schmichael commented Jun 15, 2017 •

edited

Loading

dadgar Jun 21, 2017

dadgar Jun 21, 2017

schmichael Jun 21, 2017

dadgar Jun 21, 2017

dadgar Jun 21, 2017

schmichael Jun 21, 2017

dadgar Jun 21, 2017

schmichael Jun 21, 2017 •

edited

Loading

schmichael Jun 21, 2017

dadgar Jun 21, 2017

dadgar Jun 21, 2017

dadgar Jun 21, 2017

dadgar Jun 21, 2017

dadgar Jun 21, 2017

schmichael commented Jun 22, 2017

dadgar Jun 23, 2017

dadgar Jun 23, 2017

dadgar Jun 23, 2017

dadgar Jun 23, 2017

dadgar Jun 23, 2017

github-actions bot commented Mar 31, 2023

Advertise driver-specific addresses #2709

Advertise driver-specific addresses #2709

Conversation

schmichael commented Jun 15, 2017 • edited Loading

Summary

DriverNetwork

NOMAD_{HOST,DRIVER}_{ADDR,IP,PORT}_<label> env vars

service.address_mode

Naming (Driver vs Plugin)

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

schmichael Jun 21, 2017 • edited Loading

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

schmichael commented Jun 22, 2017

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 Mar 31, 2023

schmichael commented Jun 15, 2017 •

edited

Loading

`NOMAD_{HOST,DRIVER}_{ADDR,IP,PORT}_<label>` env vars

`service.address_mode`

schmichael Jun 21, 2017 •

edited

Loading