Fixes docker icc + firewalld docker zone#46190
Fixes docker icc + firewalld docker zone#46190msilveirabr wants to merge 3 commits intomoby:masterfrom
Conversation
- Fixes when docker should add the interface to the firewalld docker zone - FollowUP of Issue moby#45126 / PR moby#45127 Signed-off-by: Mauricio Silveira <mauricio@livreti.com.br>
libnetwork/iptables/firewalld.go
Outdated
|
|
||
| func SetupInterfaceFirewalldZone(bridgeName string, enable bool) error { | ||
| // Either add or remove the interface from the firewalld zone | ||
| if firewalldRunning { |
There was a problem hiding this comment.
Perhaps we should move the firewalldRunning check into AddInterfaceFirewall and DelInterfaceFirewalld functions
(note to self: I want to get rid of directly using that package-level variable, as it depends on a side-effect of other functions being called, so it's not guaranteed to always be set)
There was a problem hiding this comment.
If we do, we can inline the Add.., Del... calls in the code, which (I think) makes it more transparent what we're doing (not having to pass that awkward insert boolean);
func setIcc(version iptables.IPVersion, bridgeIface string, iccEnable, insert bool) error {
iptable := iptables.GetIptable(version)
var (
table = iptables.Filter
chain = "FORWARD"
args = []string{"-i", bridgeIface, "-o", bridgeIface, "-j"}
acceptArgs = append(args, "ACCEPT")
dropArgs = append(args, "DROP")
)
if insert {
if !iccEnable {
iptable.Raw(append([]string{"-D", chain}, acceptArgs...)...)
if !iptable.Exists(table, chain, dropArgs...) {
if err := iptable.RawCombinedOutput(append([]string{"-A", chain}, dropArgs...)...); err != nil {
return fmt.Errorf("Unable to prevent intercontainer communication: %s", err.Error())
}
}
} else {
iptable.Raw(append([]string{"-D", chain}, dropArgs...)...)
if !iptable.Exists(table, chain, acceptArgs...) {
if err := iptable.RawCombinedOutput(append([]string{"-I", chain}, acceptArgs...)...); err != nil {
return fmt.Errorf("Unable to allow intercontainer communication: %s", err.Error())
}
}
// Ensure bridge interface is added to the docker zone if firewalld is running.
if err := iptables.AddInterfaceFirewalld(bridgeName); err != nil {
return err
}
}
} else {
// Remove any ICC rule.
if !iccEnable {
if iptable.Exists(table, chain, dropArgs...) {
iptable.Raw(append([]string{"-D", chain}, dropArgs...)...)
}
} else {
if iptable.Exists(table, chain, acceptArgs...) {
iptable.Raw(append([]string{"-D", chain}, acceptArgs...)...)
}
// Remove bridge interface from the docker zone if firewalld is running.
if err := iptables.DelInterfaceFirewalld(bridgeName); err != nil {
return err
}
}
}
return nil
}There was a problem hiding this comment.
Agreed, but I have no idea what should be done with firewalldRunning check. I believe it might already be in the right place, other functions are using this check
libnetwork/iptables/firewalld.go
Outdated
| return err | ||
| } | ||
| } else { | ||
| if err := DelInterfaceFirewalld(bridgeName); err != nil { |
There was a problem hiding this comment.
This is not new, but I'm wondering if this is a bug; should this action be idempotent? What I mean is: should we actually produce an error if the interface was not part of the zone ?
moby/libnetwork/iptables/firewalld.go
Lines 266 to 275 in e0da5cb
If it wasn't part of the zone, it feels like we can ignore the error ("we're done; nothing to do here, move on move on.. ").
However, for that to work, we probably need to update AddInterfaceFirewalld and DelInterfaceFirewalld to return properly types errors, and return a errdefs.ErrNotFound, so that we can handle "not found" errors differently when calling those functions.
Which would look something like;
// Remove bridge interface from the docker zone if it exists, and if firewalld is running.
if err := DelInterfaceFirewalld(bridgeName); err != nil && !errdefs.IsNotFound(err) {
return err
}There was a problem hiding this comment.
Not really sure about how the error handling should be done but I'll give it a try.
There was a problem hiding this comment.
I tried to remove the check for firewalld running, but it resulted in errors: 2023/08/14 19:26:22 http: panic serving @: runtime error: invalid memory address or nil pointer dereference and many other lines.
I agree that checking if firewalld is running is unnecessary when removing interface, I just have no idea how to code to remove this dependency AND I wonder what side effects ignoring this check would cause.
It took me long enough to figure out how to do what I already did.
libnetwork/iptables/iptables.go
Outdated
| // Either add or remove the interface from the firewalld zone | ||
| if firewalldRunning { | ||
| if enable { | ||
| if err := AddInterfaceFirewalld(bridgeName); err != nil { | ||
| return err | ||
| } | ||
| } else { | ||
| if err := DelInterfaceFirewalld(bridgeName); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we safely remove this code here? Mostly wondering if there's code-paths we could hit where this logic is still needed. For example, I noticed there's a n.registerIptCleanFunc() used;
moby/libnetwork/drivers/bridge/setup_ip_tables.go
Lines 183 to 185 in e0da5cb
Which I think would mean that these things are no longer cleaned up on deleteNetwork() 🤔
There was a problem hiding this comment.
I tried using DelInterfaceFirewalld inside deleteNetwork in the previous PR and it didn't work, not sure why...
Anyway, all the CI tests are good, as well as my tests.
- Moved AddInterfaceFirewalld and DelInterfaceFirewalld into setup_iptables.go - Renamed CheckRunning to FirewalldCheckRunning in firewalld.go Signed-off-by: Mauricio Silveira <mauricio@livreti.com.br>
Signed-off-by: Mauricio Silveira <mauricio@livreti.com.br>
I had to open this new PR because the old one was autoclosed when I synched my fork with main repo.
After 2 failed attempts to fix the issue, and thanks to all the feedback, I decided to create a new test environment with an Ubuntu-22.04LTS VM and a Fedora38 VM for further testing/investigation. This helped clear my mind about what icc and internal really means to docker (hopefully).
After extensive testing, my conclusion was simple: any bridge interface with icc enabled should be added to the docker firewalld zone. The internal option is being handled correctly.
After re-analysing the code, came up with a new possible solution.
Here's some rationale about how I see docker bridges should behave:Internal is meant to define wether the container should communicate with other hosts, and icc is meant to allow communication between containers. Those concepts were a bit mixed in my mind before.Anyway, the current interaction of docker with firewalld does not result in the desired state, at least in an default install on Ubuntu/Fedora.I have created a test script and a compose to automate the tests, and the network section of the compose file is this:considering that the default value for internal is false AND com.docker.network.bridge.enable_icc is true, the default_bridge_settings, icc_enabled and icc_enabled_internal are the same, and icc_disabled is equal to icc_disabled_nointernal.
The only useful declarations are for icc_enabled_internal, where the network only works between the attached containers and icc_disabled/icc_disabled_nointernal where the container should communicate with external hosts only, not with other containers connected to the same bridge.
icc_enabled_internal configuration is meant to create a private network to be used between a backend and frontend container, where the frontend container should be connected to another network with the icc_disabled config type to the outide world or another private network such as an nginx proxy, which is then part of another network of config icc_disabled.
AND, from my observation, I believe those features only works when iptables is enabled in docker config.
Here's my test results:
A few notes about the above results:
Thanks for all the feedback / orientation