Skip to content
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

IPv6 iptables config option #41622

Merged
merged 2 commits into from Dec 7, 2020
Merged

IPv6 iptables config option #41622

merged 2 commits into from Dec 7, 2020

Conversation

bboehmke
Copy link
Contributor

@bboehmke bboehmke commented Nov 2, 2020

Note: This requires pull request #41604 to be merged first (already merged)

- What I did
Added an config option to enable the IPv6 iptables rule creation based on moby/libnetwork#2572.

This should close #25407.
fixes #21614

- How I did it
Add a config option ip6tables (like iptables for IPv4) which can be used to enable the IPv6 iptables rule creation.
By default this option is disabled too keep backward compatibility.

- How to verify it
To enable this functionality it is required to enable ipv6, set a fixed cidr and enable the ip6itpables.

This can be done for example via CLI parameters on the daemon:

dockerd --ipv6 --fixed-cidr-v6 fd00:dead:beef::/48 --ip6tables

or by setting the configurations inside the daemon.json.

Now a container can be started and if a port is published the IPv6 NAT iptables rules are also created.

- Description for the changelog

--ip6tables enables IPv6 iptables rules (only if experimental)

Signed-off-by: Benjamin Böhmke <benjamin@boehmke.net>
@bboehmke bboehmke marked this pull request as ready for review November 5, 2020 15:20
@bboehmke
Copy link
Contributor Author

bboehmke commented Nov 5, 2020

@arkodg @thaJeztah PTAL

Also I am not sure if there is also changes for the documentation needed.

@arkodg
Copy link
Contributor

arkodg commented Nov 18, 2020

I see a make swagger-docs that might be needed to generate docs

@bboehmke
Copy link
Contributor Author

The swagger part seems to be more for the API of the docker daemon which should not be changed here.

The the documentation that maybe have to be updated are this 2 sections in the docker daemon documentation:

If you can give me a hint how this is updated I can also add a change for this.

@arkodg
Copy link
Contributor

arkodg commented Nov 18, 2020

ah good point
there a Request doc changes link to the right
https://github.com/docker/docker.github.io/issues/new?body=File:%20[engine/reference/commandline/dockerd.md](https://docs.docker.com/engine/reference/commandline/dockerd/)
this flag would need to get consumed by libnetwork which would then need to get vendored in and that version would need to get released in the docker-ce package, before the doc changes go live

@arkodg
Copy link
Contributor

arkodg commented Nov 18, 2020

ptal @tiborvass @thaJeztah

@bboehmke
Copy link
Contributor Author

@arkodg I directly created a PR for the documentation change -> docker/cli#2846

I hope this is enough (and it was the right position for the change).

@arkodg
Copy link
Contributor

arkodg commented Nov 18, 2020

yah that should be good enough imo, thanks for the proactive change

@thaJeztah thaJeztah added area/daemon area/networking impact/changelog kind/feature Functionality or other elements that the project doesn't currently have. Features are new and shiny status/2-code-review labels Nov 19, 2020
@thaJeztah
Copy link
Member

@arkodg does this feature require additional dependencies to be defined in the packages, or can we assume that if iptables is present, ip6tables is as well?

Also; does the check-config.sh need additional checks for troubleshooting? https://github.com/moby/moby/blob/master/contrib/check-config.sh#L191-L197

@thaJeztah thaJeztah added this to In progress in 20.10 planning via automation Nov 19, 2020
@bboehmke
Copy link
Contributor Author

bboehmke commented Nov 19, 2020

I did a quick check for some linux distributions.

ip6tables included in iptables package for the following:

ip6tables NOT included in iptables package for the following:

I hope this is useful

@thaJeztah
Copy link
Member

Thanks for checking!

Discussing this in the maintainers meeting, and we're considering to mark the feature as "experimental" for the initial implementation, as it didn't have testing yet, so there could still be issues with it.

@ell1e
Copy link

ell1e commented Nov 19, 2020

Will it be possible to specify a single IPv6 address for --fixed-cidr-v6 if --ip6tables is enabled? (Or alternatively can be --fixed-cidr-v6 omitted entirely in that case?) Sorry if this is nonsense, but in my head it feels like that should be possible to allow IPv6 NAT operation behind a single predetermined public ip similar to IPv4 NAT which wasn't possible before, but which I would at least expect to work now

@tianon
Copy link
Member

tianon commented Nov 19, 2020

Very interesting idea, @ell1e -- are you thinking the containers would just have use link-local IPs instead?

(I'm just clarifying the idea, the question itself is likely better for libnetwork maintainers as to whether it's possible 😅)

@ell1e
Copy link

ell1e commented Nov 19, 2020

Yes, in fact I would have assumed that would be naturally the case with --ip6tables (since otherwise what is the point of the NAT if the containers are still publicly reachable through other routes?)

@bboehmke
Copy link
Contributor Author

bboehmke commented Nov 19, 2020

@ell1e I currently only tested with a single public IPv6 by setting the fixed-cidr-v6to the private subnet fd00:dead:beef::/48.
This should then work exactly like the IPv4 NAT.

The only thing is that it is currently required to define a fixed-cidr-v6 otherwise it will not work (because there is no default subnet specified).

Edit: This should of course also work with docker networks.

@ell1e
Copy link

ell1e commented Nov 19, 2020

Ok, so wouldn't it make sense if omitting --fixed-cidr-v6 made it default to a private subnet automatically if --ip6tables is specified? To mirror the IPv4 NAT default behavior. I would then also make the docs suggest to omit the option/remove it if docker ipv6 was previously used without the NAT, so nobody exposes their NAT'ed containers to the public again by accident. (If that is not already what the docs suggest)

@arkodg
Copy link
Contributor

arkodg commented Nov 19, 2020

yah in the future we could possibly use a subnet within fd00::/8 as a default Ipv6 network which eliminates the need to explicitly add --fixed-cidr-v6

@ell1e
Copy link

ell1e commented Nov 19, 2020

I wonder, maybe it might even be worth having docker print a warning if a non-private v6 subnet is specified in combination with the NAT6....? In case some folks migrate their old config keeping their --fixed-cidr-v6 set to a public range and just adding the NAT6 option without realizing that will kind of circumvent the NAT in a way. I think there is some great value in being able to omit this option from the start, so users are conceptually trained to let go of their old configs and do it right.

Edit: please note I don't think it's a good idea to make --fixed-cidr-v6 generally default to a private subnet (unless that was already the case up to now), but rather only when --ip6tables is specified. Otherwise it would mess with old configs again

@arkodg
Copy link
Contributor

arkodg commented Nov 20, 2020

just realized that we have some iptables related integration tests here

func TestHostIPv4BridgeLabel(t *testing.T) {

could you maybe add one more (with this dockerd setting enabled) to give the feature more life :)

@tiborvass
Copy link
Collaborator

Per the comments above, I removed c2b421e072586bfd9be64cf8fa2b17718d9686ed

Copy link
Member

@tianon tianon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ell1e
Copy link

ell1e commented Dec 4, 2020

I think it would be better to by default implicity set --ipv6 and --fixed-cidr-v6 (the latter defaulting to a private range then) when just putting --ip6tables. I don't think it's a good user experience to do something most people do not want to do when forgetting to add those, since I assume the common use case will be to just mirror how the IPv4 setup for docker works. If I put just --ip6tables and nothing else, this is IMHO what should happen and not something more obscure.

Rather, I think it would make more sense to require those to use --fixed-cidr-v6 with something else manually then, and to have something like --no-ipv6 in addition, to allow what was described above as sometimes required but what I would assume a corner case.

Otherwise, the options are just going to be an even more wild unusable mess than they already are now IMHO. I mean, sure documentation can help and it's great the feature is now there at all, but I think if you're not optimizing the options for the likely most common use then you're setting yourself up for confused tickets for years (since now is probably the latest moment in time where this can still be changed without messing up everyone's launch options in real deployments later).

Edit: different idea which I didn't put at first because it will break more people's pre-existing stuff, but I think this is how it would be in an ideal world: 1. rename --ipv6 to --ip6routes or something, 2. implement --ip6tables as it is now with no implicit other options set, 3. introduce a new --ipv6 that sets --ip6routes, --ip6tables, and --fixed-cidr-ipv6 to a private range if not manually specified. Then people could just put --ipv6 and it would work, and the longer more explicit options could be used instead by those who want a special config - and ignored by everyone else.

But I'm not sure how it would be possible to deprecate the old --ipv6 without ruining everyone's day. I guess one way to do this would be to also guard this renaming of --ipv6 with the experimental flag, and add --ip6routes also in non-experimental mode and make docker spill out a warning if --ipv6 is used with the experimental flag off in the "old" usage style to make people rename their option? But I'm not sure how many people would actually notice the warnings...

Edit 2: just to spell out the latest, "ideal world" idea, in full detail, this is how it would work:

Experimental features off:

  • --ip6routes is added, does what --ipv6 does now
  • --ipv6 is an alias for --ip6routes, but using it adds a warning that this option will change meaning in a future release and that it should be renamed in the local launch options (to the new --ip6routes) to avoid any undesired effects
  • --ip6tables is unavailable

Experimental features on:

  • --ip6routes is added, does what --ipv6 does now
  • --ipv6 is an entirely new option, which sets --ip6routes, --ip6tables, and --fixed-cidr-ipv6 to a private subnet if not set
  • --ip6tables is also available for separate use with any or none of --ip6routes and --fixed-cidr-ipv6 combined for granular configurations

@tomalok
Copy link

tomalok commented Dec 4, 2020

Let's not over-complicate things if possible.

What's the current state of things? As I understand it, those who want to use IPv6 must use both --ipv6 and --fixed-cidr-v6 in order for it to work?

If that's true (and I am by no means an expert), then to use IPv6 without any specified CIDR I would expect --ipv6 to be sufficient. It should pick a suitable non-routable IPv6 block, and do NAT with ip6tables just like how IPv4 works today.

To use a specific IPv6 CIDR, use of --fixed-cidr-v6 could also imply --ipv6 but not error out if --ipv6 is also specified. No ip6tables would be managed/manipulated, in line with what (i think) is the current behavior.

If someone wants to specify their own IPv6 CIDR and do NAT, then there should be some new way to explicitly turn it on, like --ipv6-nat would be specified alongside the --fixed-cidr-v6.

It would be okay to specify --ipv6-nat with --ipv6 -- since that's the default behavior without the --fixed-cidr-v6.

Summarized, I think this is simplest...

  • --ipv6 [--ipv6-nat] - turn on IPv6 support, default non-routable IPv6 CIDR, use NAT/ip6tables
  • [--ipv6] --fixed-cidr-v6 <cidr> - turn on IPv6 support, using specified IPv6 CIDR, do not use NAT/ip6tables
  • [--ipv6] --fixed-cidr-v6 <cidr> --ip6-nat - turn on IPv6 support, use specified IPv6 CIDR, use NAT/ip6tables

@tianon
Copy link
Member

tianon commented Dec 5, 2020

I agree there's a ton of room for improvement/adjustment in Docker's IPv6 support (and I want to see it!), but this PR is just adding a flag to enable one bit of optional new behavior that wasn't previously available, so I don't think it makes sense to make any drastic changes in this context (and is why I've added my own "LGTM" to this PR, especially with the new flag under experimental).

I think the overall experience improvement discussion should probably move to a separate issue(/PR).

@ell1e
Copy link

ell1e commented Dec 5, 2020

Summarized, I think this is simplest...

--ipv6 [--ipv6-nat] - turn on IPv6 support, default non-routable IPv6 CIDR, use NAT/ip6tables
[--ipv6] --fixed-cidr-v6 - turn on IPv6 support, using specified IPv6 CIDR, do not use NAT/ip6tables
[--ipv6] --fixed-cidr-v6 --ip6-nat - turn on IPv6 support, use specified IPv6 CIDR, use NAT/ip6tables

I like this idea. While I think the options I suggested conceptually make slightly more sense, this one is very practical to add in without breaking old configs while addressing my main concern (which is that --ipv6 should be transitioned to something where it "just works" like IPv4 without extra options, which is I think what people usually would expect).

@tianon is it common to rename options around again of experimental stuff, though? There might be some merit to get it right the first time, maybe. (I'm not familiar enough with how docker is commonly maintained here.) But I'd be happy to put it into a separate issue, I'm just wondering if that means it won't be addressed for another ten years because then many are already used to the "weird" options.

@bboehmke
Copy link
Contributor Author

bboehmke commented Dec 5, 2020

There are currently two options to use ipv6:

  1. via default bridge via --ipv6 and --fixed-cidr-v6
  2. via user defined network created with --ipv6

Based on this it is possible to enable ipv6 only for specific networks without enabling it for the default bridge.

The --ip6tables would only enable the ip6tables NAT rule creation but only for ipv6 enabled networks.
Because the change ip6tables is working like the iptables (ipv4) it is not possible to enable it only for a single network (like --iptables)

Maybe this can be changed in the future but I have no idea if this is possible at all.

In my opinion the --ip6tables enables only the iptables rule creation for IPv6 enabled networks without requiring a configured network at startup.

@ell1e
Copy link

ell1e commented Dec 5, 2020

Based on this it is possible to enable ipv6 only for specific networks without enabling it for the default bridge. [...] In my opinion the --ip6tables enables only the iptables rule creation for IPv6 enabled networks without requiring a configured network at startup.

Interesting, is there a specific reason why it needs to be like that? Ideally I think the most useful way for this new thing to work would be to have one easy option to enable it for everything, exactly like IPv4, including for the default bridge and other places. This would also work nicely with @tomalok 's option suggestion that promotes a single --ipv6 with no extra option to do all this magic in the future, so people can just fire it up and then just focus on making their microservices and images work well with it and not spend all this time on infrastructure details they possibly don't care about. (That is what extra options would be best for, IMHO.)

@tianon
Copy link
Member

tianon commented Dec 5, 2020

@tianon is it common to rename options around again of experimental stuff, though? There might be some merit to get it right the first time, maybe. (I'm not familiar enough with how docker is commonly maintained here.) But I'd be happy to put it into a separate issue, I'm just wondering if that means it won't be addressed for another ten years because then many are already used to the "weird" options.

Absolutely - the whole point of requiring "experimental" for flags is to let users test them early while they're still completely subject to change (or even removal).

@bboehmke
Copy link
Contributor Author

bboehmke commented Dec 5, 2020

@ell1e The current IPv6 implementation is build this way, that if you set this option on the daemon only the default network will have IPv6 and the user created ones not (at least if not configured explicit on network creation).

In some setups (especially for the first steps with IPv6) it can be a good idea to do not change the default bridge but do first tests with a separate network with IPv6 enabled (this was the way I started with IPv6).

Maybe default values the IPv6 cidr would make the situation a bit easier but this is not present so far.

@tiborvass
Copy link
Collaborator

So are there any objections for merging this as-is?

@arkodg
Copy link
Contributor

arkodg commented Dec 6, 2020

lets continue the discussions of default IPv6 flags in a separate PR / issue since reaching a consensus for it here is gating the IPv6 NAT feature
@tiborvass can we please merge this PR, so this feature (still experimental) can be released soon, thanks

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; let's merge this one

Could someone open a ticket to describe and track improvements that we want to make? I'm trying to follow the discussion, but perhaps someone could write up something 🤗

@thaJeztah thaJeztah merged commit cf31b96 into moby:master Dec 7, 2020
1 check passed
20.10 planning automation moved this from Reviewer approved to Done Dec 7, 2020
@ell1e
Copy link

ell1e commented Dec 7, 2020

@thaJeztah I made a ticket here, I hope that will be suitable: #41754 (if not feel free to close it & replace it with a better one)

@thaJeztah
Copy link
Member

Thanks!

@WallaceSUI
Copy link

Hey team,

Just wonder when (by date or by release) you plan to make this implementation as non-experimental (remove parameter "experimental")?

Thanks a lot.

@akerouanton
Copy link
Contributor

For the record, I answered @WallaceSUI question here: #45384.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/daemon area/networking impact/changelog kind/feature Functionality or other elements that the project doesn't currently have. Features are new and shiny status/2-code-review
Projects
No open projects
9 participants