Skip to content

Add internal n/w bridge to firewalld docker zone#47293

Merged
akerouanton merged 1 commit intomoby:masterfrom
robmry:47229-internal-bridge-firewalld
Feb 2, 2024
Merged

Add internal n/w bridge to firewalld docker zone#47293
akerouanton merged 1 commit intomoby:masterfrom
robmry:47229-internal-bridge-firewalld

Conversation

@robmry
Copy link
Contributor

@robmry robmry commented Feb 1, 2024

Fixes #47229

- What I did

Containers attached to an 'internal' bridge network are unable to communicate when the host is running firewalld.

Non-internal bridges are added to a trusted 'docker' firewalld zone, but internal bridges were not - they are now.

- How I did it

For a non-internal network, the Add/Del functions are called by iptables.ProgramChain (multiple times per bridge). There's there's no equivalent in the iptables package for an internal network. So, this change calls them from the bridge driver, in the function that's used for create and cleanup of an internal bridge's rules.

- How to verify it

Tested a local build on openSUSE, using the example in the original ticket.

Without this change, pings and curls over the internal network fail, they hit a 'reject with icmpx admin-prohibited' firewalld rule.

With the change, communication between containers works. The DOCKER-ISOLATION iptables rules are still configured for an internal network, they block traffic to/from addresses outside the network's subnet (checked by looking at counters in iptables -nvL -t filter). The bridge still does not get an IP address, so there's no route for communication with the host. So, it is still an internal network.

It should be covered by existing tests in 'integration/networking/bridge_test.go' ... but I don't think we run CI on any hosts that use firewalld?

- Description for the changelog

Include 'internal' network bridges in the 'docker' zone for firewalld, so that containers can communicate.

Containers attached to an 'internal' bridge network are unable to
communicate when the host is running firewalld.

Non-internal bridges are added to a trusted 'docker' firewalld zone, but
internal bridges were not.

DOCKER-ISOLATION iptables rules are still configured for an internal
network, they block traffic to/from addresses outside the network's subnet.

Signed-off-by: Rob Murray <rob.murray@docker.com>
@robmry robmry self-assigned this Feb 1, 2024
@robmry robmry marked this pull request as ready for review February 1, 2024 13:55
@robmry robmry requested review from akerouanton and corhere February 1, 2024 13:55
var version iptables.IPVersion
var inDropRule, outDropRule iptRule

// Either add or remove the interface from the firewalld zone, if firewalld is running.
Copy link
Member

Choose a reason for hiding this comment

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

It seems ProgramChain is only called by the bridge driver. Do you think it'd make sense to move the calls to Add/Del methods out of the iptables package to setupIPTables (ie. setupInternalNetworkRules caller), such that we don't duplicate these in two different places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I don't think the firewalld calls belong with the rest of ProgramChain, and should be called separately (and once) by the bridge driver.

It looked like a bit of a mess though. And, particularly for firewalld, not well regression tested. So, I figured a minimal change would be more likely to make it into 25.0.3.

And, it's the usual problem with changing an exported package? Maybe that doesn't matter? Or, if it does, we could split ProgramChain, and make it call new functions that we use from the bridge driver?

But perhaps that should be a separate refactoring PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Separate refactoring PR, please, because I already have one: #46666

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excellent! Thanks Cory.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Containers cannot communicate with each other when using an internal network

4 participants