[go: up one dir, main page]

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

Enforce requirement for iptables/ip6tables to implement ICC=false #48494

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

robmry
Copy link
Contributor
@robmry robmry commented Sep 13, 2024

- What I did

With "--icc=false" (no inter-container comms on the default bridge), the daemon will produce an error and fail to start if iptables/ip6tables are disabled and ipv4/ipv6 are enabled for the network.

There's no such check for user-defined networks. (And I don't think we test ICC=false in user-defined networks.)

- How I did it

Make "network create" fail if ICC=false can't be implemented.

- How to verify it

New tests.

- Description for the changelog

Bridge driver option `com.docker.network.bridge.enable_icc=false` is implemented using iptables
rules, "network create" now fails with an error if the option is used when iptables or ip6tables is
required but disabled.

@robmry robmry added kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. area/networking impact/changelog labels Sep 13, 2024
@robmry robmry added this to the 28.0.0 milestone Sep 13, 2024
@robmry robmry self-assigned this Sep 13, 2024
@robmry robmry changed the title Enforce requirement for iptables/ip6tables to implement ICC Enforce requirement for iptables/ip6tables to implement ICC=false Sep 13, 2024
@robmry robmry force-pushed the validate_icc branch 3 times, most recently from a8eb017 to 2dea933 Compare September 17, 2024 08:23
Disabling Inter-Container Communication (ICC) in the bridge
driver is implemented using iptables.

The bridge driver has not been checking that iptables/ip6tables
are enabled if required for ICC=false, and some unit tests do
not set up consistent config.

Supplying "netlabel.GenericData" in "options.Generic" form
("map[string]any") does not set up "EnableICC: false". But,
suppling it in "map[string]string" form does - and that's
the form used when a driver option is supplied via the API
as, for example "-o com.docker.network.bridge.enable_icc=false".
So, do that in more tests.

Set EnableIPTables/EnableIP6Tables in the driver config for
other tests, and explicitly enable/disable EnableICC in others.

Signed-off-by: Rob Murray <rob.murray@docker.com>
The integration test helpers like NewSwarm that start a daemon
with swarm enabled have been setting "--iptables=false". Now
deleted comments from 2018 say "avoid networking conflicts".

But, when a swarm network needs a "docker_gwbridge" network,
it's always created with ICC=false, and that can't be implemented
with iptables disabled - the tests have silently been running
with ICC enabled on gateway networks (as well as any other
effects running without iptables may have had).

Non-swarm tests that start their own daemons don't disable
iptables, and CI is happy with iptables enabled.

So, don't disable iptables.

Signed-off-by: Rob Murray <rob.murray@docker.com>
With "-icc=false", no inter-container comms on the default bridge,
the daemon will produce an error and fail to start if iptables or
ip6tables are disabled (and ipv4/ipv6 are enabled for the network).

Add a similar check for user-defined networks, so that "network
create" fails if ICC can't be implemented.

Along with the new errors, test that ICC can be disabled for a
user-defined network.

Signed-off-by: Rob Murray <rob.murray@docker.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/networking impact/changelog kind/enhancement Enhancements are not bugs or new features but can improve usability or performance.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant