IPv6 only: Allow IPv4 and IPv6 host-gateway-ip addresses by robmry · Pull Request #48807 · moby/moby

robmry · 2024-10-31T16:09:50Z

- What I did

Running a container with --add-host blah:host-gateway adds an /etc/hosts entry for host blah and an address on the docker host - to give the container a convenient way of reaching the host.

If no --host-gateway-ip option is supplied, the IPv4 address of the default bridge is used - and that's been fine until now, it's a host address we know will exist. But, in a container that's only connected to IPv6-only networks, that doesn't work.

So:

if the default bridge has an IPv6 address, create an additional /etc/hosts entry with that address
allow two --host-gateway-ip options
- at most one IPv4 and one IPv6 address
in daemon.json, allow a JSON array value --host-gateway-ips (plural).
- for a single address, a JSON string is also allowed

For example:
--host-gateway-ip 192.0.2.1 --host-gateway-ip 2001:db8::1111
And the daemon.json version would be:
"host-gateway-ips": ["192.0.2.1", "2001:db8::1111"]
But, this is also still valid:
"host-gateway-ip": "192.0.2.1"

- How I did it

Notes:

The /etc/hosts entries follow the usual rules...
- If IPv6 is disabled in a container (by sysctl, or lack of kernel support), IPv6 addresses are not included in the file.
- In other cases, IPv4 and IPv6 addresses will both be included, whether or not the container currently has network endpoints that support IPv4 or IPv6.
buildx has its own code to interpret the host-gateway-ip option.
- When it's updated to understand two addresses, moby will need to pass it both. For now, it passes an IPv4 address if there is one, else IPv6.

- How to verify it

- Description for the changelog

- Modifications to `host-gateway`, for compatibility with IPv6-only networks.
  - When special value `host-gateway` is used in an `--add-host` option in place of an address, it's
    replaced by an address on the docker host to make it possible to refer to the host by name.
    The address used belongs to the default bridge (normally `docker0`). Until now it's always been
    an IPv4 address, because all containers on bridge networks had IPv4 addresses.
  - Now, if IPv6 is enabled on the default bridge network, `/etc/hosts` entries will be created for
    IPv4 and IPv6 addresses. So, it's possible for a container that's only connected to IPv6-only
    networks to access the host by name.
  - The `--host-gateway-ip` option overrides the address used to replace `host-gateway`. Two of
    these options are now allowed on the command line, for one IPv4 gateway and one IPv6.
    - In the `daemon.json` file, to provide two addresses, use a JSON array in the `"host-gateway-ips"`
      field. For example, `"host-gateway-ips": ["192.0.2.1", "2001:db8::1111"]`.

thaJeztah

It'd be nice to have a daemon.json "host-gateway-ips" that takes a []string, rather than the sightly-odd comma separated list in a single string. But...
We have to keep the "host-gateway-ip field for existing config files.

I think we should be able to achieve this, but it would require users to migrate to the new field if they need to have options for multiple;

Keep the old, single-value field
Add the new, multi-value field
Users are allowed to set on or the other, but not BOTH
Setting BOTH is a hard failure
Setting the old one propagates the new field with a single value, and logs a warning (possibly warning in docker info)
After X release(s) we can deprecate the old field (hard failure when used).

Some questions;

IFF we'd introduce a new field, and if you'd design your ideal would that be only a []string, or are there other potential options to expect (i.e, would a []struct{"field": "value", "field": "value} be something desirable?)
We currently limit to the special host-gateway value; I recall at some point we discussed having an option to add custom DNS entries for containers; mostly mentioning here, as possibly related to this effort (but not sure if that should be "same feature" or "other feature")

thaJeztah · 2024-11-08T14:21:28Z

@@ -0,0 +1,65 @@
+package opts


If this one's only gonna be used by the daemon, would it be ok to move this to internal/opts ? The opts package used to be somewhat shared between the CLI and Daemon, but trying to reduce public API surface if we don't need it exported.

Oh, yes - hadn't spotted that package. I'll move it, thank you.

robmry · 2024-11-08T19:37:26Z

We have to keep the "host-gateway-ip field for existing config files.

I think we should be able to achieve this, but it would require users to migrate to the new field if they need to have options for multiple;

@akerouanton suggested handling it in the JSON unmarshal, so in daemon.json we'd allow any of:

"host-gateway-ip": "192.0.2.1"
"host-gateway-ip": [ "192.0.2.1" ]
"host-gateway-ip": [ "192.0.2.1", "2001:db8::1" ]

And, on the command line, it'd just be two --host-gateway-ip options.

That seems like a good plan, I think it handles everything we need - I'll make the change.

IFF we'd introduce a new field, and if you'd design your ideal would that be only a []string, or are there other potential options to expect (i.e, would a []struct{"field": "value", "field": "value} be something desirable?)

Just the IP address I think.

It'll only allow one IPv4 and one IPv6 address - so we could make it a JSON object and require something like {"v4": "192.0.2.1", "v6": "2001:db8::1"} ... but I think that'd just be harder to remember and easier to mistype, without really adding much.

I suppose we could make it possible to name a host interface, and look up its addresses (not saying we will do that, we'd probably have to monitor for address changes and make /etc/hosts updates) ... in that case, we'd be able to spot something interface-name-like in the string rather than an IP address, and do something sensible with it.

Or, worst case, if we do find a need for a more complicated type - we could pull the same trick again, and accept an object as well as a string/list.

We currently limit to the special host-gateway value; I recall at some point we discussed having an option to add custom DNS entries for containers; mostly mentioning here, as possibly related to this effort (but not sure if that should be "same feature" or "other feature")

I'm not sure - we could put the host-gateway-ip in the internal DNS instead of /etc/hosts (then it'd be a little easier to make it dynamic if we wanted to monitor host addresses on an interface) ... is that the sort of thing you're thinking of? If so, it's probably "other feature" - I don't think this change would rule anything out.

vvoland · 2024-11-12T15:33:44Z

+		{
+			name:   "ipv6 and ipv4",
+			input:  []string{"fdb8:3037:2cb2::1", "10.1.1.1"},
+			expStr: "10.1.1.1,fdb8:3037:2cb2::1",


Shouldn't it be a JSON array?

Suggested change

expStr: "10.1.1.1,fdb8:3037:2cb2::1",

expStr: `["10.1.1.1","fdb8:3037:2cb2::1"]`,

It'll never be used as JSON, so I went for something unambiguous and minimal for error messages etc. At-least opts/address_pools.go takes the same approach.

The two addresses will need to be passed to buildx, once it understands two addresses. Either version would be ok for that.

Can change it if needed though?

Was mostly concerned if it should match the input in daemon.json it was parsed from.
If it doesn't matter though, either is fine.

akerouanton · 2024-11-18T15:32:21Z

 	"bytes"
 	"encoding/json"
 	"fmt"
+	"github.com/docker/docker/internal/testutils/networking"


You need to put this import in the block of imports below. I think that's why the CI is complaining.

Yes, ta - I'll do that once we pick an approach (just didn't want to break the links I sent to reviewers by changing the commit numbers, and this PR is going to need at least one more push anyway).

akerouanton

Since you did the extra work to add --host-gateway-ips to the daemon, I think I'm in favor of that approach. It makes this flag follows the pattern used by other plural flags, etc... So this sounds better.

Is there any particular downside you see with this approach?

robmry · 2024-11-18T16:08:24Z

Since you did the extra work to add --host-gateway-ips to the daemon, I think I'm in favor of that approach. It makes this flag follows the pattern used by other plural flags, etc... So this sounds better.

Is there any particular downside you see with this approach?

Right, I agree it fits better and probably wins on that basis. If forced to look for downsides, nothing too bad, but...

it makes the command-line/config-file merge a little more complicated (and it was already complicated enough!)
it creates a deprecated field in the config
IP addresses are stored as a []string rather than a pair of netip.Addr, and
handling of host-gateway-ip is more scattered.

thaJeztah · 2024-11-21T12:24:32Z

+	DNS            []net.IP `json:"dns,omitempty"`
+	DNSOptions     []string `json:"dns-opts,omitempty"`
+	DNSSearch      []string `json:"dns-search,omitempty"`
+	HostGatewayIP  net.IP   `json:"host-gateway-ip,omitempty"` // Deprecated: this single-IP is migrated to HostGatewayIPs


Looks like this would require it to be a pointer for the omitempty to work (also see my example below; https://go.dev/play/p/m-9f_mVwuGW)

The only change here is to deprecate the existing field.

I don't think this struct is ever marshalled - just unmarshalled from the config file ... so I'm not sure why any of the fields have omitempty.

Yeah, not super critical probably; I think we use the JSON format to log it when reloading the config; perhaps I should try if that would work with structured logs (although it can be useful to have the log log the config in the format used for config.json 🤔

moby/daemon/reload.go

Lines 118 to 129 in 803534f

jsonString, _ := json.Marshal(&struct {

*config.Config

config.Proxies `json:"proxies"`

}{

Config: &newCfg.Config,

Proxies: config.Proxies{

HTTPProxy: config.MaskCredentials(newCfg.HTTPProxy),

HTTPSProxy: config.MaskCredentials(newCfg.HTTPSProxy),

NoProxy: config.MaskCredentials(newCfg.NoProxy),

},

})

log.G(context.TODO()).Infof("Reloaded configuration: %s", jsonString)

thaJeztah · 2024-11-21T12:28:04Z

+	DNSOptions     []string `json:"dns-opts,omitempty"`
+	DNSSearch      []string `json:"dns-search,omitempty"`
+	HostGatewayIP  net.IP   `json:"host-gateway-ip,omitempty"` // Deprecated: this single-IP is migrated to HostGatewayIPs
+	HostGatewayIPs []string `json:"host-gateway-ips,omitempty"`


Curious; was there a reason not to use []net.IP for this? From some quick testing, marshalling / unmarshaling should work for that? See https://go.dev/play/p/m-9f_mVwuGW

package main import ( "encoding/json" "net" "testing" ) type config struct { HostGatewayIP net.IP `json:"host-gateway-ip,omitempty"` // Deprecated: this single-IP is migrated to HostGatewayIPs HostGatewayIPs []net.IP `json:"host-gateway-ips,omitempty"` } func TestGatewayIPS(t *testing.T) { cfg1 := config{ HostGatewayIPs: []net.IP{ net.ParseIP("2001:db8::1234"), net.ParseIP("192.0.2.1"), }, } out, err := json.Marshal(cfg1) if err != nil { t.Fatal(err) } t.Log(string(out)) const configJSON = `{"host-gateway-ips": ["2001:db8::1234", "192.0.2.1"]}` var cfg config err = json.Unmarshal([]byte(configJSON), &cfg) if err != nil { t.Fatal(err) } t.Logf("%+v\n", cfg.HostGatewayIPs) }

Because we don't have any separation between config read from the file, config from the command line, and the structs used to represent config internally.

A []string can be used in flags.Var(opts.NewNamedListOptsRef("host-gateway-ips", &conf.HostGatewayIPs, opts.ValidateIPAddress) ... it'd be possible to write a []net.IP (or, rather, []netip.Addr) version of that. Perhaps it could also be rewritten to use generic types. But, I didn't do any of that! ... it wouldn't buy much in this case, the addresses are only used as strings, and they're validated as IP addresses.

Ah, right, so there is a IPSliceVar (and related) option in spf13/pflag. Haven't checked what's needed to wrap that into a "NamedListOpt" (or whatever it's named) though;

moby/vendor/github.com/spf13/pflag/ip_slice.go

Lines 151 to 160 in 803534f

// IPSliceVar defines a []net.IP flag with specified name, default value, and usage string.

// The argument p points to a []net.IP variable in which to store the value of the flag.

func IPSliceVar(p *[]net.IP, name string, value []net.IP, usage string) {

CommandLine.VarP(newIPSliceValue(value, p), name, "", usage)

}

// IPSliceVarP is like IPSliceVar, but accepts a shorthand letter that can be used after a single dash.

func IPSliceVarP(p *[]net.IP, name, shorthand string, value []net.IP, usage string) {

CommandLine.VarP(newIPSliceValue(value, p), name, shorthand, usage)

}

It doesn't quite fit together like that, IPSliceVar takes a *[]net.IP, which can't be a NamedOption (so it can't implement the equivalence between command-line --host-gateway-ip and config file "host-gateway-ips").

So, I've added a NamedIPListOpts, which stores values in a []netip.Addr and implements interface NamedOption.

I added it as a third commit to make it easier to see the change ... how's it looking?

Signed-off-by: Rob Murray <[email protected]>

Running a container with "--add-host blah:host-gateway" adds an /etc/hosts entry for host "blah" and an address on the docker host - to give the container a convenient way of reaching the host. If no --host-gateway-ip option is supplied, the IPv4 address of the default bridge is used - and that's been fine until now, it's a host address we know will exist. But, in a container that's only connected to IPv6-only networks, that doesn't work. So: - if the default bridge has an IPv6 address, create an additional /etc/hosts entry with that adddress - allow two --host-gateway-ip options - at most one IPv4 and one IPv6 address - in daemon.json, allow a JSON array value in --host-gateway-ips (plural) - for a single address, a JSON string is also allowed For example: --host-gateway-ip 192.0.2.1 --host-gateway-ip 2001:db8::1111 And the daemon.json version would be: "host-gateway-ips": ["192.0.2.1", "2001:db8::1111"] But, this is also still valid: "host-gateway-ip": "192.0.2.1" Note that the /etc/hosts entries follow the usual rules. If IPv6 is disabled in a container (by sysctl, or lack of kernel support), IPv6 addresses are not included in the file. In other cases, IPv4 and IPv6 addresses will both be included, whether or not the container currently has network endpoints that support IPv4 or IPv6. buildx has its own code to interpret the host-gateway-ip option. When it's updated to understand two addresses, moby will need to pass it both. For now, it passes an IPv4 address if there is one, else IPv6. Signed-off-by: Rob Murray <[email protected]>

Signed-off-by: Rob Murray <[email protected]>

thaJeztah

LGTM, thanks!

robmry · 2024-11-26T17:50:11Z

Edit: ignore this comment - too many PRs, and this one isn't involved! I'll deal with it separately.

I've just found a problem with this, worth fixing before it's merged - disabling IPv6 on an endpoint means it can't be an IPv6 gateway ...

# docker network create --ipv6 b46
# docker run --rm -ti --network name=b46,driver-opt=com.docker.network.endpoint.sysctls=net.ipv6.conf.IFNAME.disable_ipv6=1 busybox
docker: Error response from daemon: failed to set IPv6 gateway while updating gateway: route for the gateway fd11:9190:ed6e::1 could not be found: network is unreachable.

robmry self-assigned this Oct 31, 2024

robmry added kind/feature Functionality or other elements that the project doesn't currently have. Features are new and shiny area/networking Networking impact/changelog impact/documentation area/networking/ipv6 Networking area/networking/d/bridge Networking labels Oct 31, 2024

robmry added this to the 28.0.0 milestone Oct 31, 2024

This was referenced Oct 31, 2024

IPv6 only: use random MAC addresses #48808

Merged

[epic] IPv6 improvements for moby 28.0 #48814

Closed

robmry force-pushed the v6only/host_gateway_ip branch 2 times, most recently from c008b37 to 0e5047e Compare November 7, 2024 18:56

robmry marked this pull request as ready for review November 7, 2024 20:08

robmry requested a review from tonistiigi as a code owner November 7, 2024 20:08

robmry requested a review from akerouanton November 7, 2024 20:08

thaJeztah reviewed Nov 8, 2024

View reviewed changes

robmry force-pushed the v6only/host_gateway_ip branch from 0e5047e to 4c4a4f9 Compare November 11, 2024 11:51

robmry mentioned this pull request Nov 11, 2024

Docs: host-gateway-ip daemon option IPv4+IPv6 docker/cli#5607

Merged

robmry requested a review from thaJeztah November 11, 2024 12:02

vvoland reviewed Nov 12, 2024

View reviewed changes

robmry force-pushed the v6only/host_gateway_ip branch 2 times, most recently from 0afc4a5 to ab81e12 Compare November 13, 2024 17:31

akerouanton reviewed Nov 18, 2024

View reviewed changes

thaJeztah reviewed Nov 21, 2024

View reviewed changes

robmry force-pushed the v6only/host_gateway_ip branch 3 times, most recently from a80e24d to 4717ef0 Compare November 25, 2024 11:05

robmry force-pushed the v6only/host_gateway_ip branch from 4717ef0 to 937311f Compare November 25, 2024 19:32

robmry added 3 commits November 26, 2024 11:47

Move Linux-only integration/network tests into Linux-only file

01e2168

Signed-off-by: Rob Murray <[email protected]>

Test host-gateway addresses from docker0

c9a1e4d

Signed-off-by: Rob Murray <[email protected]>

robmry force-pushed the v6only/host_gateway_ip branch from 937311f to c9a1e4d Compare November 26, 2024 12:18

thaJeztah approved these changes Nov 26, 2024

View reviewed changes

akerouanton approved these changes Nov 27, 2024

View reviewed changes

thaJeztah merged commit c565f74 into moby:master Nov 27, 2024

robmry deleted the v6only/host_gateway_ip branch November 27, 2024 19:23

thaJeztah mentioned this pull request Dec 15, 2024

Support host.docker.internal in dockerd on Linux #40007

Merged

	expStr: "10.1.1.1,fdb8:3037:2cb2::1",
	expStr: `["10.1.1.1","fdb8:3037:2cb2::1"]`,

	jsonString, _ := json.Marshal(&struct {
	*config.Config
	config.Proxies `json:"proxies"`
	}{
	Config: &newCfg.Config,
	Proxies: config.Proxies{
	HTTPProxy: config.MaskCredentials(newCfg.HTTPProxy),
	HTTPSProxy: config.MaskCredentials(newCfg.HTTPSProxy),
	NoProxy: config.MaskCredentials(newCfg.NoProxy),
	},
	})
	log.G(context.TODO()).Infof("Reloaded configuration: %s", jsonString)

	// IPSliceVar defines a []net.IP flag with specified name, default value, and usage string.
	// The argument p points to a []net.IP variable in which to store the value of the flag.
	func IPSliceVar(p *[]net.IP, name string, value []net.IP, usage string) {
	CommandLine.VarP(newIPSliceValue(value, p), name, "", usage)
	}

	// IPSliceVarP is like IPSliceVar, but accepts a shorthand letter that can be used after a single dash.
	func IPSliceVarP(p *[]net.IP, name, shorthand string, value []net.IP, usage string) {
	CommandLine.VarP(newIPSliceValue(value, p), name, shorthand, usage)
	}

Conversation

robmry commented Oct 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robmry commented Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akerouanton left a comment

Choose a reason for hiding this comment

Uh oh!

robmry commented Nov 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

robmry commented Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

robmry commented Oct 31, 2024 •

edited

Loading

robmry commented Nov 8, 2024 •

edited

Loading

robmry commented Nov 26, 2024 •

edited

Loading