Enable 'ip6tables' by default, don't require 'experimental'. #47747
+165
−190
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
robmry
force-pushed
the
non-experimental-ip6tables
branch
3 times, most recently
from
1106f19
to
fe1dd01
Compare
Tests that start a daemon disable iptables, to avoid conflicts with other tests running in parallel and also creating iptables chains. Do the same for ip6tables, in prep for them being enabled by-default. Signed-off-by: Rob Murray <rob.murray@docker.com>
The bridge driver's setupIPChains() had an initial sanity check that "--iptables=true". But, it's called with "version=IPv6" when "--iptables=false" and "--ip6tables=true" - the sanity test needed to allow for that. Signed-off-by: Rob Murray <rob.murray@docker.com>
bridgeNetwork.isolateNetwork() checks "--iptables=true" and "--ip6tables=true" before doing anything with IPv4 and IPv6 respectively. But, it was only called if "--iptables=true". Now, it's called if "--ip6tables=true", even if "--iptables=false". Signed-off-by: Rob Murray <rob.murray@docker.com>
The code to enable "bridge-nf-call-iptables" or "bridge-nf-call-ip6tables" was gated on "--iptables=true", it didn't check "--ip6tables=true". So, split the top level call into IPv4/IPv6 so that the iptables-enable settings can be checked independently, and simplfied the implementation. Signed-off-by: Rob Murray <rob.murray@docker.com>
robmry
force-pushed
the
non-experimental-ip6tables
branch
from
fe1dd01
to
880c7b4
Compare
robmry
added
status/2-code-review
kind/enhancement
robmry
force-pushed
the
non-experimental-ip6tables
branch
from
880c7b4
to
8a9f4fb
Compare
akerouanton
reviewed
May 7, 2024
| assert.Check(t, is.Contains(string(res), bridgeName)) | ||
| } else { | ||
| assert.Check(t, !strings.Contains(string(res), subnet), | ||
| fmt.Sprintf("Didn't expect to find '%s' in '%s'", subnet, string(res))) |
There was a problem hiding this comment.
res is going to be a multiline string, so this error won't be easy to read. I'd rather put ... in ip6tables-save output.
There was a problem hiding this comment.
I temporarily added a false && to make the test fail - the output looks like this ...
=== RUN TestNoIP6Tables/ip6tables_off
bridge_test.go:782: assertion failed: expression is false: false && !strings.Contains(string(res), subnet): Didn't expect to find 'fdb3:2511:e851:34a9::/64' in '# Generated by ip6tables-save v1.8.9 on Tue May 7 10:34:13 2024
*nat
:PREROUTING ACCEPT [0:0]
:INPUT ACCEPT [0:0]
:OUTPUT ACCEPT [0:0]
:POSTROUTING ACCEPT [0:0]
:DOCKER - [0:0]
-A PREROUTING -m addrtype --dst-type LOCAL -j DOCKER
-A OUTPUT ! -d ::1/128 -m addrtype --dst-type LOCAL -j DOCKER
COMMIT
# Completed on Tue May 7 10:34:13 2024
# Generated by ip6tables-save v1.8.9 on Tue May 7 10:34:13 2024
*filter
:INPUT ACCEPT [0:0]
:FORWARD DROP [0:0]
:OUTPUT ACCEPT [2:232]
:DOCKER - [0:0]
:DOCKER-ISOLATION-STAGE-1 - [0:0]
:DOCKER-ISOLATION-STAGE-2 - [0:0]
:DOCKER-USER - [0:0]
-A FORWARD -j DOCKER-USER
-A FORWARD -j DOCKER-ISOLATION-STAGE-1
-A DOCKER-ISOLATION-STAGE-1 -i docker0 ! -o docker0 -j DOCKER-ISOLATION-STAGE-2
-A DOCKER-ISOLATION-STAGE-1 -j RETURN
-A DOCKER-ISOLATION-STAGE-2 -o docker0 -j DROP
-A DOCKER-ISOLATION-STAGE-2 -j RETURN
-A DOCKER-USER -j RETURN
COMMIT
# Completed on Tue May 7 10:34:13 2024
'
--- FAIL: TestNoIP6Tables (8.31s)
So, I think it's readable - and probably useful to see the full output.
There was a problem hiding this comment.
Okay, let's keep it as is then.
daemon/daemon_unix.go
Outdated
| if !conf.BridgeConfig.EnableIPTables { | ||
| return fmt.Errorf("You specified --iptables=false with --icc=false. ICC=false uses iptables to function. Please set --icc or --iptables to true") | ||
| } | ||
| if !conf.BridgeConfig.EnableIP6Tables { |
There was a problem hiding this comment.
If you disable IPv6 at the kernel level and explicitly set EnableIP6Tables=false and InterContainerCommunication=false, then this check will fail. I think we should make this an error only if IPv6 has been enabled on the default bridge (ie. when EnableIPv6 is true).
FWIW, we don't barf out if a custom network is created with icc=false, ipv6=true and ip6tables disabled. We should probably make config validation more consistent.
There was a problem hiding this comment.
Done.
(Is it possible to set icc=false for a custom network?)
There was a problem hiding this comment.
(Is it possible to set icc=false for a custom network?)
Yeah, you can create one with: docker network create --opt=com.docker.network.bridge.enable_icc=false
Check forwarding, then set bridge-nf-call-ip6tables, on a bridge if IPv6 is enabled - even if no IPv6 address has been assigned. Signed-off-by: Rob Murray <rob.murray@docker.com>
In setupIPv6BridgeNetFiltering(), the bridge should always be named. Don't fall back to checking the "default" setting for a new bridge. Signed-off-by: Rob Murray <rob.murray@docker.com>
Signed-off-by: Rob Murray <rob.murray@docker.com>
Tests no longer need to use "--experimental --ip6tables", now ip6tables is the default behaviour. Signed-off-by: Rob Murray <rob.murray@docker.com>
robmry
force-pushed
the
non-experimental-ip6tables
branch
from
8a9f4fb
to
2319f51
Compare
akerouanton
approved these changes
May 7, 2024
corhere
approved these changes
May 8, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
- What I did
Enable
--ip6tablesby-default, don't require--experimental.Note that this enables NAT/masquerading by default for IPv6 network. This is likely to remain the default, as it's a lowest-common-denominator that'll ensure bridge networks get IPv6 connectivity on an IPv6 host. In a follow-up PR, I'll make it possible to enable/disable masquerading separately for IPv4/IPv6.
With IPv6 masquerading disabled, running ip6tables by default means containers on bridge networks with routed IPv6 addresses only expose explicitly exposed/forwarded ports.
- How I did it
A series of commits - it's probably easiest to review them separately.
- How to verify it
Existing and new integration tests.
- Description for the changelog