-
Notifications
You must be signed in to change notification settings - Fork 881
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
Add docker interfaces to firewalld docker zone #2548
Conversation
PTAL @yrro @cpuguy83 @thaJeztah |
7378ec4
to
f4b58e7
Compare
IMO, Docker should follow libvirt's example and create its own I think you're modifying firewalld's runtime configuration only, so interfaces will be moved back to the default zone when firewalld is reloaded or restarted. libvirt handles this by listening to firewalld's Reloaded signal, and the normal D-Bus |
It occurs to me that a user might want different interfaces to be created in different zones. A |
f4b58e7
to
cd39849
Compare
@yrro thanks for taking a look !
|
ed91f60
to
d0d4036
Compare
updated the PR to add docker interfaces to a new docker zone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First, thanks for adding firewalld integration. This will please users. In general this looks functional. I have various cleanup type comments. But those are non-blocking.
I realize that masquerading and port forwarding is still done via iptables. In the future I think firewalld could be used for those as well. Upstream is currently working on support for forward/output filtering.
|
||
settings := getDockerZoneSettings() | ||
// Permanent | ||
if err := connection.sysConfObj.Call(dbusInterface+".config.addZone", 0, dockerZone, settings).Err; err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note that the addZone()
method has been deprecated. The next firewalld feature release introduces addZone2()
which is dictionary based and doesn't require all fields be filled. That being said, docker has to support older versions of firewalld so this is the correct one to call. In the future support for addZone2()
should be considered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, libvirt ships a static zone definition as part of the package. Therefore they avoid the addZone()
and reload()
. They also allow other things; dhcp, dns, icmp, ipv6-icmp. But they're also filtering (denying) other packets destined to the host.
// Check if interface is already added to the zone | ||
if err := connection.sysObj.Call(dbusInterface+".zone.getInterfaces", 0, dockerZone).Store(&intfs); err != nil { | ||
return err | ||
} | ||
// Return if interface is already part of the zone | ||
if contains(intfs, intf) { | ||
logrus.Infof("Firewalld: interface %s already part of %s zone, returning", intf, dockerZone) | ||
return nil | ||
} | ||
|
||
logrus.Debugf("Firewalld: adding %s interface to %s zone", intf, dockerZone) | ||
// Runtime | ||
if err := connection.sysObj.Call(dbusInterface+".zone.addInterface", 0, dockerZone, intf).Err; err != nil { | ||
return err | ||
} | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're probably aware, but this is racey. It's possible the interface gets deleted/added by another entity/user between the getInterfaces()
and addInterface()
calls. It may be simpler to always call addInterface()
and handle the error codes appropriately. e.g. ALREADY_ENABLED
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to keep it this way, since AFAIK I would need to compare the error string to handle the above case, which might change in the future
if err := connection.sysObj.Call(dbusInterface+".zone.getInterfaces", 0, dockerZone).Store(&intfs); err != nil { | ||
return err | ||
} | ||
// Remove interface if it exists | ||
if !contains(intfs, intf) { | ||
return fmt.Errorf("Firewalld: unable to find interface %s in %s zone", intf, dockerZone) | ||
} | ||
|
||
logrus.Debugf("Firewalld: removing %s interface from %s zone", intf, dockerZone) | ||
// Runtime | ||
if err := connection.sysObj.Call(dbusInterface+".zone.removeInterface", 0, dockerZone, intf).Err; err != nil { | ||
return err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar comment as above. Maybe always call removeInterface()
and handle the errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above
57b7633
to
902a280
Compare
If firewalld is running, create a new docker zone and add the docker interfaces to the docker zone to allow container networking for distros with firewalld enabled Fixes: moby#2496 Signed-off-by: Arko Dasgupta <[email protected]>
902a280
to
7a72092
Compare
What happens here if someone already has a pre-configured |
If a user manually created a docker zone, libnetwork would just reuse that and insert interfaces into that zone. However exact behavior of networking depends on the zone settings (which libnetwork will not try and override) |
So I believe that should happen if the user sets Edit - tested this with |
I think the idea was to not have to set iptables=false but allow users to enable the specific firewalld enhancement (or rather disable it as needed), for example cases where they are already managing this. I just know that firewall changes Docker does always ends up being problematic. |
@cpuguy83 even w/o this change, we have been pushing iptables rules via firewalld |
We've been pushing rules such that firewalld ensures they exist in iptables, but this is changing how firewalld is configured entirely. |
@cpuguy83 do you prefer having a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
full diff: moby/libnetwork@2e24aed...9e99af2 - moby/libnetwork#2548 Add docker interfaces to firewalld docker zone - fixes docker/for-linux#957 DNS Not Resolving under Network [CentOS8] - fixes moby/libnetwork#2496 Port Forwarding does not work on RHEL 8 with Firewalld running with FirewallBackend=nftables - store.getNetworksFromStore() remove unused error return - moby/libnetwork#2554 Fix 'failed to get network during CreateEndpoint' - fixes/addresses docker/for-linux#888 failed to get network during CreateEndpoint - moby/libnetwork#2558 [master] bridge: disable IPv6 router advertisements - moby/libnetwork#2563 log error instead if disabling IPv6 router advertisement failed - fixes docker/for-linux#1033 Shouldn't be fatal: Unable to disable IPv6 router advertisement: open /proc/sys/net/ipv6/conf/docker0/accept_ra: read-only file system Signed-off-by: Sebastiaan van Stijn <[email protected]>
full diff: moby/libnetwork@2e24aed...9e99af2 - moby/libnetwork#2548 Add docker interfaces to firewalld docker zone - fixes docker/for-linux#957 DNS Not Resolving under Network [CentOS8] - fixes moby/libnetwork#2496 Port Forwarding does not work on RHEL 8 with Firewalld running with FirewallBackend=nftables - store.getNetworksFromStore() remove unused error return - moby/libnetwork#2554 Fix 'failed to get network during CreateEndpoint' - fixes/addresses docker/for-linux#888 failed to get network during CreateEndpoint - moby/libnetwork#2558 [master] bridge: disable IPv6 router advertisements - moby/libnetwork#2563 log error instead if disabling IPv6 router advertisement failed - fixes docker/for-linux#1033 Shouldn't be fatal: Unable to disable IPv6 router advertisement: open /proc/sys/net/ipv6/conf/docker0/accept_ra: read-only file system Signed-off-by: Sebastiaan van Stijn <[email protected]> Upstream-commit: 219e7e7ddcf5f0314578d2a517fc0832f03622c1 Component: engine
https://build.opensuse.org/request/show/851816 by user mrostecki + dimstar_suse - Add a patch which makes Docker compatible with firewalld with nftables backend. Backport of moby/libnetwork#2548 (boo#1178801, SLE-16460) * boo1178801-0001-Add-docker-interfaces-to-firewalld-docker-zone.patch
Rocky 9
All docker containers listening on 0.0.0.0:PORT are accessible from the outside via servername:PORT |
This is expected. This what docker requests. I recently wrote a blog about this and how to do strict filtering of docker containers. |
If firewalld is running, create a new docker zone and
add the docker interfaces to the docker zone to allow
container networking for distros with Firewalld enabled
Fixes: #2496
Addresses docker/for-linux#957
Debug output
Signed-off-by: Arko Dasgupta [email protected]