-
Notifications
You must be signed in to change notification settings - Fork 2k
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.
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
Service registration for IPv6 docker addresses (Fixes #3785) #3790
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great PR! Are you able to add a test to docker_test.go
or does that require properly configuring ipv6 and dockerd on the host running the tests?
@@ -355,6 +355,11 @@ The `docker` driver supports the following configuration in the job spec. Only | |||
] | |||
} | |||
``` | |||
* `use_ipv6_address` - (Optional) `true` or `false` (default). Use IPv6 Address | |||
will use the containers IPv6 address (GlobalIPv6Address) when registering service checks and using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the container's IPv6 address (
GlobalIPv6Address
in Docker) when registering ...
client/driver/docker.go
Outdated
@@ -884,6 +888,9 @@ func (d *DockerDriver) detectIP(c *docker.Container) (string, bool) { | |||
} | |||
|
|||
ip = net.IPAddress | |||
if d.driverConfig.UseIPv6Address { | |||
ip = net.GlobalIPv6Address |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, it seems like we probably want to set auto = true
here as well. While this complicates the already unfortunately confusing advertise = "auto"
logic, I think it will do what users expect by automatically advertising the routable address for the container when it's possible to discover it.
Perhaps a better question is: It should be exceedingly rare that someone wants use_ipv6_address = true
and advertise = "host"
, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(this has been addressed; leaving for historical purposes)
@@ -104,6 +104,9 @@ does not automatically enable service discovery. | |||
`address_mode="driver"`. Numeric ports may be used when in driver addressing | |||
mode. | |||
|
|||
Docker and IPv6 containers: This setting is required if you want to register | |||
the port of the (IPv6) service. See [below for examples.](#IPv6 docker containers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we set auto=true
when use_ipv6_address=true
this will no longer be necessary.
@@ -463,6 +477,62 @@ In this case Nomad doesn't need to assign Redis any host ports. The `service` | |||
and `check` stanzas can both specify the port number to advertise and check | |||
directly since Nomad isn't managing any port assignments. | |||
|
|||
### IPv6 docker containers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docker -> Docker
Discussed it internally and unless you have some concerns I think setting Since Nomad only uses this address in service advertisements and checks:
|
Regarding the docker_test, yes it needs Well the downside of using But setting See below for both examples.
|
* Set autoadvertise to true. * Update documentation.
Updated with the changes from your feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So no hope of a portable test?
The [Docker](/docs/drivers/docker.html#use_ipv6_address) driver support the | ||
`use_ipv6_address` parameter in it's configuration. | ||
For the `service`stanza is no explicit `address_mode` required. | ||
Services default to the `auto` address mode. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Services will automatically advertise the IPv6 address when
advertise_ipv6_address
is used.
So I've added some extra commits.
On my local vagrant setup this test works, we'll see if travis succeeds. |
Looks fantastic! Thanks for sticking with this @42wim! I pushed a small test tweak to skip the test on hosts without IPv6 enabled but otherwise will merge once Travis goes green again! |
💥 Merged 🎈 Thanks again @42wim! |
Whoops I appear to have commented on the PR instead of the issue, please find my comment on the issue for this PR here #3785 |
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. |
This adds a
use_ipv6_address
boolean which allows IPv6 docker containers to be registered using theservice
stanza.An example job on how to configure this can be seen below. Documentation has been updated too.