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

engine/networking overhaul #17176

Merged
merged 26 commits into from
Jun 5, 2023
Merged

Conversation

dvdksn
Copy link
Contributor

@dvdksn dvdksn commented Apr 24, 2023

Proposed changes

Complete overhaul of networking documentation: drivers, ipv6, iptables/firewalls

@netlify
Copy link

netlify bot commented Apr 24, 2023

Deploy Preview for docsdocker ready!

Name Link
🔨 Latest commit 2be681d
🔍 Latest deploy log https://app.netlify.com/sites/docsdocker/deploys/647dd2d210e9f70009755fcc
😎 Deploy Preview https://deploy-preview-17176--docsdocker.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@dvdksn dvdksn force-pushed the engine/networking-overhaul branch 3 times, most recently from c5ad914 to 1b14ec3 Compare May 4, 2023 12:08
@dvdksn dvdksn marked this pull request as ready for review May 4, 2023 19:03
Copy link
Member

@akerouanton akerouanton left a comment

Choose a reason for hiding this comment

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

We also need a section in the firewalling docs about iptables-nftables. Currently, dockerd only supports iptables-legacy. On some OS, users have to explicitly switch to that (by creating the proper symlink, or by using update-alternatives) or dockerd might break in unexpected ways (including at startup).

network/drivers/index.md Outdated Show resolved Hide resolved
network/drivers/index.md Outdated Show resolved Hide resolved
network/drivers/index.md Outdated Show resolved Hide resolved
network/drivers/index.md Outdated Show resolved Hide resolved
network/drivers/index.md Outdated Show resolved Hide resolved
network/iptables.md Outdated Show resolved Hide resolved
network/iptables.md Outdated Show resolved Hide resolved
network/iptables.md Outdated Show resolved Hide resolved
network/iptables.md Outdated Show resolved Hide resolved
network/index.md Outdated Show resolved Hide resolved
@dvdksn dvdksn force-pushed the engine/networking-overhaul branch 3 times, most recently from 6fd5cfa to 277a4e7 Compare May 12, 2023 13:42
network/index.md Outdated Show resolved Hide resolved
@dvdksn dvdksn force-pushed the engine/networking-overhaul branch 2 times, most recently from 69b40b9 to c90454b Compare May 12, 2023 15:32
dvdksn added a commit to dvdksn/cli that referenced this pull request May 12, 2023
File location changes in docker/docs#17176

Signed-off-by: David Karlsson <david.karlsson@docker.com>
engine/install/debian.md Outdated Show resolved Hide resolved
engine/install/raspbian.md Show resolved Hide resolved
engine/install/ubuntu.md Show resolved Hide resolved
network/drivers/bridge.md Outdated Show resolved Hide resolved
_data/toc.yaml Outdated Show resolved Hide resolved
_data/toc.yaml Outdated Show resolved Hide resolved
network/drivers/index.md Outdated Show resolved Hide resolved
network/index.md Outdated Show resolved Hide resolved
network/index.md Outdated Show resolved Hide resolved
aevesdocker
aevesdocker previously approved these changes May 16, 2023
@akerouanton
Copy link
Member

akerouanton commented May 24, 2023

@dvdksn The command shown in network/drivers/none.md is weird and its output is wrong. I'd rather replace the three steps with a single one:

$ docker run --rm --network none alpine:latest ip link show
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN qlen 1000
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00

EDIT: maybe we can actually add this command too and add a note that say no IPv6 loopback address is configured when using none 🤔

$ docker run --rm --network none --name no-net-alpine alpine:latest ip addr show
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN qlen 1000
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
    inet 127.0.0.1/8 scope host lo
       valid_lft forever preferred_lft forever

network/drivers/index.md Outdated Show resolved Hide resolved
network/drivers/bridge.md Outdated Show resolved Hide resolved
network/drivers/index.md Outdated Show resolved Hide resolved
@dvdksn
Copy link
Contributor Author

dvdksn commented May 24, 2023

@akerouanton ptal!

@akerouanton
Copy link
Member

akerouanton commented May 28, 2023

I can't edit the original description, so let me dump links to related issues here:

@thaJeztah ISTR we closed an issue not so long ago about IPv6 resolver ending up in containers' /etc/resolv.conf, which was leading to flaky DNS resolution under Alpine-based images. I can't find it anymore, do you have any idea which one it is? Following commits are related to that:

@neersighted neersighted self-requested a review May 30, 2023 22:32
@dvdksn
Copy link
Contributor Author

dvdksn commented May 31, 2023

@akerouanton @thaJeztah I don't think we should add anything more to this PR -- so unless you have any change requests I suggest we get this merged. I have couple of follow-ups to come.

config/daemon/ipv6.md Outdated Show resolved Hide resolved
network/index.md Outdated Show resolved Hide resolved
Comment on lines 34 to 35
The `ipv6` and `fixed-cidr-v6` parameters are optional.
They assign an IPv6 subnet to the default bridge network.
Copy link
Contributor

Choose a reason for hiding this comment

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

My grammar isn't great, so I could be mistaken if a comma properly connects the two statements 😅

Suggested change
The `ipv6` and `fixed-cidr-v6` parameters are optional.
They assign an IPv6 subnet to the default bridge network.
The `ipv6` and `fixed-cidr-v6` parameters are optional,
they assign an IPv6 subnet to the default bridge network.

polarathene
polarathene previously approved these changes Jun 2, 2023
```

Upon restart, the daemon assigns IPv6 addresses to containers connected to the
default bridge network, and to user-defined networks configured with an IPv6 subnet.
Copy link
Member

Choose a reason for hiding this comment

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

Users need to set default-address-pools to let the daemon assign IPv6 addresses to their custom network automatically (as described below).

Suggested change
default bridge network, and to user-defined networks configured with an IPv6 subnet.
default bridge network.

Copy link
Contributor

Choose a reason for hiding this comment

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

Users need to set default-address-pools to let the daemon assign IPv6 addresses

docker network create --ipv6 --subnet fd00:d0ca::/112?

You can assign an IPv6 subnet explicitly for user-defined networks without default-address-pools being adjusted.

The statement doesn't say anything about automatically configuring IPv6 subnet for a user-defined network, just that containers with a user-defined network that has already been configured with an IPv6 subnet would enable the container to be assigned an IPv6 address from it.

Copy link
Member

Choose a reason for hiding this comment

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

The statement doesn't say anything about automatically configuring IPv6 subnet for a user-defined network

It talks about automatically assigning IPv6 addresses to containers:

Upon restart, the daemon assigns IPv6 addresses to containers connected [...] to user-defined networks configured with an IPv6 subnet.

My point is: experimental and ip6tables are needed to have the same experience with IPv6 as with IPv4 (wrt. isolation & port mapping), whether users rely on static or dyanmic allocation. But ipv6 and fixed-cidr-v6 are only required to let the daemon assign IPv6 addresses to containers connected to the default bridge network. Above options won't turn on dynamic allocation for custom networks, hence my suggestion.

Given the state of IPv6 support currently, and the fact old networking mode (with the default bridge) still exist, I also prefer to just tell users to configure ipv6 and fixed-cidr-v6 such that the default bridge and custom networks just work the same way.

Copy link
Contributor

Choose a reason for hiding this comment

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

It talks about automatically assigning IPv6 addresses to containers:

Which is correct. That happens when the network (default bridge, or user-defined nework) has IPv6 subnet already configured?

My point is: experimental and ip6tables are needed to have the same experience with IPv6 as with IPv4 (wrt. isolation & port mapping), whether users rely on static or dyanmic allocation.

Yes these two settings are quite important. It's unfortunate ip6tables remains opt-in while userland-proxy: true is the default due to surprises that causes on an IPv6 host even when docker networks are IPv4 only.

Above options won't turn on dynamic allocation for custom networks, hence my suggestion.

I tried to communicate this distinction earlier in review, the two settings from the earlier JSON snippet could belong in their own section that enables IPv6 for the default (legacy) bridge.

A section that follows after it could then cover the same for user-defined networks with an explicit subnet given with docker network create or in the compose.yaml. After that, you detail implicit subnet assignment which almost works like creating IPv4 networks, but still requires opt-in for IPv6 (--ipv6 / enable_ipv6: true).

I also prefer to just tell users to configure ipv6 and fixed-cidr-v6 such that the default bridge and custom networks just work the same way.

I understand :)

I just think it's worth the distinction that the two settings are optional, only related to enabling the support on the bridge. Some users aren't aware of that in the past and think it's required even when using compose.yaml.

Copy link
Member

Choose a reason for hiding this comment

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

Which is correct. That happens when the network (default bridge, or user-defined nework) has IPv6 subnet already configured?

Right, but that's unrelated to the daemon parameters we're advertising in this section.

I tried to communicate this distinction earlier in review, the two settings from the earlier JSON snippet could belong in their own section that enables IPv6 for the default (legacy) bridge.

Got you. @dvdksn WDYT about what @polarathene proposes:

  • Adding a section dedicated to the default bridge and moving ipv6 and fixed-cidr-v6 there ;
  • Adding a section dedicated to static allocation ;

We can discuss this proposal on Slack if you're unsure what should go in these new sections.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, I think this would help clear things up. I'll give it it a go!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ptal 2be681d

config/daemon/ipv6.md Show resolved Hide resolved
config/daemon/ipv6.md Outdated Show resolved Hide resolved
```

Upon restart, the daemon assigns IPv6 addresses to containers connected to the
default bridge network, and to user-defined networks configured with an IPv6 subnet.
Copy link
Member

Choose a reason for hiding this comment

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

The statement doesn't say anything about automatically configuring IPv6 subnet for a user-defined network

It talks about automatically assigning IPv6 addresses to containers:

Upon restart, the daemon assigns IPv6 addresses to containers connected [...] to user-defined networks configured with an IPv6 subnet.

My point is: experimental and ip6tables are needed to have the same experience with IPv6 as with IPv4 (wrt. isolation & port mapping), whether users rely on static or dyanmic allocation. But ipv6 and fixed-cidr-v6 are only required to let the daemon assign IPv6 addresses to containers connected to the default bridge network. Above options won't turn on dynamic allocation for custom networks, hence my suggestion.

Given the state of IPv6 support currently, and the fact old networking mode (with the default bridge) still exist, I also prefer to just tell users to configure ipv6 and fixed-cidr-v6 such that the default bridge and custom networks just work the same way.

Comment on lines 63 to 72
> **Note**
>
> Be aware that the following known limitations exist:
>
> - Supernets can't have a size larger than 80. This is due to an integer
> overflow in the Docker daemon. See
> [moby/moby#42801](https://github.com/moby/moby/issues/42801)
> - The difference between the supernet length and the pool size can't be
> larger than 24. Otherwise, the daemon consumes all available memory. See
> [moby/moby#40275](https://github.com/moby/moby/issues/40275)
Copy link
Member

Choose a reason for hiding this comment

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

I think you can create a pool of subnets larger than 2^24, it just depends on the memory the system has available.

You can, but it grows exponentially. A difference of 24 bits between the pool size and the desired subnet size already consumes 250MiB. 28 bits will consume 4GiB and 32 bits, 64GiB. Hence my initial recommendation I asked @dvdksn to put here.

Also, I prefer to precisely describe the limitation instead of vaguely stating it, as the latter case implies users have to discover it by themselves.

config/daemon/ipv6.md Outdated Show resolved Hide resolved
network/drivers/overlay.md Outdated Show resolved Hide resolved
Containers that attach to the default `bridge` network receive a copy of this file.
Containers that attach to a
[custom network](network-tutorial-standalone.md#use-user-defined-bridge-networks)
use Docker's embedded DNS server.
Copy link
Member

Choose a reason for hiding this comment

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

@dvdksn Can you add an item on your todo list about that please ⬆️?

network/packet-filtering-firewalls.md Outdated Show resolved Hide resolved
network/packet-filtering-firewalls.md Outdated Show resolved Hide resolved
network/index.md Outdated Show resolved Hide resolved
@dvdksn dvdksn force-pushed the engine/networking-overhaul branch from 742edfe to 584b15f Compare June 5, 2023 12:10
dvdksn and others added 6 commits June 5, 2023 14:19
Signed-off-by: David Karlsson <david.karlsson@docker.com>
Signed-off-by: David Karlsson <david.karlsson@docker.com>
Ports published on the localhost IP are still accessible
by other systems connected to the same local network.

Signed-off-by: David Karlsson <david.karlsson@docker.com>
Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
Signed-off-by: David Karlsson <david.karlsson@docker.com>
Signed-off-by: David Karlsson <david.karlsson@docker.com>
- One section for using IPv6 with custom networks
- One section for using IPv6 with the default bridge

Signed-off-by: David Karlsson <david.karlsson@docker.com>
@dvdksn dvdksn force-pushed the engine/networking-overhaul branch from 584b15f to 2be681d Compare June 5, 2023 12:19
Copy link
Member

@akerouanton akerouanton left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

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; are there commits that need to be squashed before merging? (the rename commits probably must be kept separate)

also kept one comment about the "experimental" part, but we can look at that in a follow-up (I think we need to slightly make that more clear)

@dvdksn dvdksn merged commit fb2f039 into docker:main Jun 5, 2023
9 checks passed
thaJeztah pushed a commit to thaJeztah/cli that referenced this pull request Jun 27, 2023
File location changes in docker/docs#17176

Signed-off-by: David Karlsson <david.karlsson@docker.com>
(cherry picked from commit 035e26f)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

missing command in the documentation of enabling ipv6 in docker
8 participants