Fixes internal bridge with icc in firewalld #45126#45127
Fixes internal bridge with icc in firewalld #45126#45127msilveirabr wants to merge 0 commit intomoby:masterfrom msilveirabr:master
Conversation
|
@thaJeztah |
There was a problem hiding this comment.
Thank you for your contribution towards fixing the internal bridge with icc in Firewalld. Through your contribution, you addressed the issue raised in this Github Issue - moby/libnetwork#2647.
I have reviewed the changes you submitted and have the following feedback:
- The code additions make sense since they resolve the Firewalld issue with nftables when the docker is internally set and icc is enabled.
- I appreciate the comment in the code explaining that the new function added, SetupInternalICC, either adds or removes the internal interface from the firewalld zone depending on whether icc is enabled or disabled.
- The new function added, SetupInternalICC, might affect the performance of the system since it needs to check on whether Firewalld is running, which adds overhead that might slow down the system. You might want to re-evaluate where it is added or how this detection is being handled.
- I didn't notice any issues with the code quality, though I recommend following the best cost-effective practices to mitigate potential issues.
Thank you for contributing to the open-source community.
| return err | ||
| } | ||
| //Add internal interface to docker zone if ICC is enabled | ||
| if icc { |
There was a problem hiding this comment.
I suggest renaming this parameter to isIccEnabled for clarity
libnetwork/iptables/iptables.go
Outdated
| } | ||
|
|
||
| // SetupInternalICC is used to add or remove the internal interface from the firewalld zone ( ICC ) | ||
| // https://github.com/moby/libnetwork/issues/2647 |
There was a problem hiding this comment.
I suggest adding a comment to clarify the purpose of this method to avoid confusion in the future
| iiptable.SetupInternalICC(bridgeIface, insert) | ||
| } | ||
| // Set Inter Container Communication. | ||
| return setIcc(version, bridgeIface, icc, insert) |
There was a problem hiding this comment.
Have yet to look at the code in context, but wondering if the changes should be part of this function. IIUC, the new changes would mean that things become less atomic; first some iptables changes are made, then we call setIcc(); probably those two steps should either "both fail" or "both success", and not "one of them succeed (or fail)"
There was a problem hiding this comment.
become less atomic
I wouldn't worry too much about that. If any part of bridge network creation fails, the driver throws out all its state about the network without invoking any of the registered cleanup callbacks. No aspect of bridge network creation is atomic.
libnetwork/iptables/iptables.go
Outdated
|
|
||
| // SetupInternalICC is used to add or remove the internal interface from the firewalld zone ( ICC ) | ||
| // https://github.com/moby/libnetwork/issues/2647 | ||
| func (iptable IPTable) SetupInternalICC(bridgeIface string, include bool) error { |
There was a problem hiding this comment.
Was there a reason to add this as a method on iptable? As (from the code below) it doesn't appear to be using iptable as part of the function 🤔
(FWIW, the whole "iptable" interface is not ideal; it's trying to shoehorn / abstracting many things into an interface that's not always a good fit for the things done, so trying to prevent digging in further into adding more exported methods)
|
cc @akerouanton |
| iiptable.SetupInternalICC(bridgeIface, insert) | ||
| } | ||
| // Set Inter Container Communication. | ||
| return setIcc(version, bridgeIface, icc, insert) |
There was a problem hiding this comment.
become less atomic
I wouldn't worry too much about that. If any part of bridge network creation fails, the driver throws out all its state about the network without invoking any of the registered cleanup callbacks. No aspect of bridge network creation is atomic.
libnetwork/iptables/iptables.go
Outdated
| if firewalldRunning { | ||
| if include { | ||
| if err := AddInterfaceFirewalld(bridgeIface); err != nil { | ||
| return fmt.Errorf("Failed to add interface %s to firewalld zone: %s", bridgeIface, err.Error()) | ||
| } | ||
| } else { | ||
| if err := DelInterfaceFirewalld(bridgeIface); err != nil { | ||
| return fmt.Errorf("Failed to remove interface %s from firewalld zone: %s", bridgeIface, err.Error()) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This code is a near-identical copy of the code at the top of (IPTable).ProgramChain. Notably, the only references to ProgramChain are in the (*bridgeNetwork).setupIPTables function, specifically the !config.Internal branch. And the only caller of setupInternalNetworkRules is the config.Internal branch of (*bridgeNetwork).setupIPTables. Given that adding/removing the interface to the firewalld zone is required irrespective of the value of config.Internal, I don't think duplicating the same code in both branches is the right solution. I think a less-awful solution would be to make (*bridgeNetwork).setupIPTables call AddInterfaceFirewalld/DelInterfaceFirewalld directly, and move the if firewalldRunning checks into AddInterfaceFirewalld and DelInterfaceFirewalld. There isn't any clean abstraction layer between the bridge driver and package iptables anyway so I see no value in fabricating a half-baked, leaky abstraction which makes no sense unless you know what's behind the abstraction anyway, just to maintain a fiction that there is an abstraction.
|
It looks like one or more commits are missing a DCO sign-off |
|
Well, sorry for the delayed response. Having some issues with lack of focus, life happened... diff --git a/libnetwork/drivers/bridge/setup_ip_tables.go b/libnetwork/drivers/bridge/setup_ip_tables.go
index f6e631d612..7ca84a7225 100644
--- a/libnetwork/drivers/bridge/setup_ip_tables.go
+++ b/libnetwork/drivers/bridge/setup_ip_tables.go
@@ -291,6 +291,22 @@ func programChainRule(version iptables.IPVersion, rule iptRule, ruleDescr string
return nil
}
+// SetupInternalICC is used to add or remove the internal interface to/from the firewalld docker zone ( ICC )
+// https://github.com/moby/libnetwork/issues/2647
+func SetupInternalICC(bridgeIface string, include bool) error {
+ // Either add or remove the interface from the firewalld docker zone
+ if include {
+ if err := iptables.AddInterfaceFirewalld(bridgeIface); err != nil {
+ return fmt.Errorf("Failed to add interface %s to firewalld docker zone: %s", bridgeIface, err.Error())
+ }
+ } else {
+ if err := iptables.DelInterfaceFirewalld(bridgeIface); err != nil {
+ return fmt.Errorf("Failed to remove interface %s from firewalld docker zone: %s", bridgeIface, err.Error())
+ }
+ }
+ return nil
+}
+
func setIcc(version iptables.IPVersion, bridgeIface string, iccEnable, insert bool) error {
iptable := iptables.GetIptable(version)
var (
@@ -413,6 +429,11 @@ func setupInternalNetworkRules(bridgeIface string, addr *net.IPNet, icc, insert
if err := programChainRule(version, outDropRule, "DROP OUTGOING", insert); err != nil {
return err
}
+ if icc {
+ if err := SetupInternalICC(bridgeIface, insert); err != nil {
+ return err
+ }
+ }
// Set Inter Container Communication.
return setIcc(version, bridgeIface, icc, insert)
}
Discard previous commits. Now I wonder what should be the best approach here: Create e new commit, reverting previous changes + include the changes of the above patch, or create a new pull request to avoid "polluted" git history, since I believe this pull request has too many commits? ( As I stated before, I'm a sysadmin with some programming skills, not a senior programmer ) But first and foremost, is the above code OK? |
Don't worry about that right now (unless the reviewers say otherwise). With pull requests the review is the important part to get right first. So apply whatever change / updates you need to as new commits and review will be based on the merge diff: https://github.com/moby/moby/pull/45127/files Once the reviewers are happy with the PR, the commit history can be adjusted with an interactive rebase to tidy it up 👍 |
Looks like your question earlier got missed by the reviewers. Since the current commit history doesn't provide much value, squashing it down to a single commit should be fine, and you can add the DCO sign-off to that commit message. No need to create a new PR just for that 😎 I'm not a maintainer here, so you might want to hold off before rewriting history (that said, the PR diff is small and I have seen lengthy PRs in the past force-push frequently in response to review feedback. I believe the previous commit history and review also remains accessible too). |
|
Rewriting history and force-pushing is fine, especially when fine-tuning a small PR. The previous commit history is not really accessible, though it is rarely an issue in my experience. |
| if include { | ||
| if err := iptables.AddInterfaceFirewalld(bridgeIface); err != nil { | ||
| return fmt.Errorf("Failed to add interface %s to firewalld docker zone: %s", bridgeIface, err.Error()) | ||
| } | ||
| } else { | ||
| if err := iptables.DelInterfaceFirewalld(bridgeIface); err != nil { | ||
| return fmt.Errorf("Failed to remove interface %s from firewalld docker zone: %s", bridgeIface, err.Error()) | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm nearly positive that iptables.AddInterfaceFirewalld and iptables.DelInterfaceFirewalld will panic if called when firewalld is not running. See my earlier review comments for suggestions on how to fix this.
|
|
||
| // SetupInternalICC is used to add or remove the internal interface to/from the firewalld docker zone ( ICC ) | ||
| // https://github.com/moby/libnetwork/issues/2647 | ||
| func SetupInternalICC(bridgeIface string, include bool) error { |
There was a problem hiding this comment.
While necessary to make ICC work on internal bridge networks with firewalld, this function is misleadingly named as it does not do any of the other things which are necessary to set up ICC on an internal bridge network. Since there is only one caller, I recommend inlining the code into setupInternalNetworkRules instead of factoring it out into a function.
| if err := programChainRule(version, outDropRule, "DROP OUTGOING", insert); err != nil { | ||
| return err | ||
| } | ||
| if icc { |
There was a problem hiding this comment.
Why add the bridge interface to firewalld only when ICC is enabled? For non-internal networks, the bridge interface is added to firewalld irrespective of the ICC setting. What's special about internal bridge networks with ICC disabled?
|
@msilveirabr did you accidentally close this PR you updated? |
Yes, it closed as soon as I synched my fork... Something went wrong with merge fix. |
|
I've never dealt with this situation before. It seems like this PR is still active in the CI/CD flow. |
|
UPDATE: Checked your fork, I think your concern about a hanging job is a non-issue now? If so you should still be able to update your If you didn't explicitly close this PR, I'm not quite sure how that happened. I haven't experienced it before 😅
Do you mean a CI job is still running? I wouldn't worry about that. If you mean on your own fork, it's running the Github Actions probably setup from this repo, you can disable those when you fork in the repos actions tab IIRC, so they won't run on your fork if they aren't useful (I do this for a project I maintain but raise PRs from branches of my own clone as I only care about the Github Actions on the official repo). Especially if the actions rely on secrets you don't have access to (like publishing / deploying an artifact), there's no need for those on your fork 😅
If it's doing what I described above, it shouldn't matter. I see that your PR here is using your forks master branch thouogh, and perhaps that triggered those extra CI jobs that weren't necessary. If you do delete your current fork and do it again, perhaps branch from master this time before you add your new commits. You could call your branch Since the current CI job is running on your fork, you should be able to cancel that as you have ownership over the fork. You can have an overview of CI jobs for your fork here: https://github.com/msilveirabr/moby/actions I think whatever you saw as running seems like it's already done? |
|
Oh I see... your PR no longer has any changes... you've erased them :\ The PR would have implicitly closed because there is nothing to review/contribute now. |
|
You force pushed (rewriting history) your forks I've got the prior commit before you did that here: 2449e0a So some of your changes are salvageable 🙏 It seems we can access the old history for now via this: https://github.com/moby/moby/commits/2449e0a61608251a970cd780489061e877404054 You could try using I assume you meant to rebase your changes, or just merge the latest upstream commits into your fork/PR. |
I don't really care about the code itself up to the auto-close event ( I have all the changes here ). My new commit will be a short, clean change. My only concern is whether this could cause some problem to the master origin moby/moby. I think my best action right now ( considering all the mess) is to delete my current fork and create a new fork, then apply my diff to my fork and do the PR asap. |
Don't worry, it won't cause any problem.
You don't need to delete and recreate your fork. But you should definitely create a dedicated branch for your change to make sure next time you pull moby/moby's master branch into your fork, it won't erase your work and auto-close your PR. Once your dedicated branch is ready, you can submit a new PR from that branch. Please mention this PR in your new PR's description such that we can keep track of the original review comments. |
- 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>
Fixes issue #45126
moby/libnetwork#2647
Firewalld with nftables requires docker to add bridge interface to docker zone when set to internal and icc is enabled to work properly.
- What I did
Fixed the issue
- How I did it
Added function in iptables code and bridge driver code to insert/remove network interface from docker zone
- How to verify it
As explained in issue
- Description for the changelog
Fix for internal network ICC when using systems with firewalld with nftables as backend.
It works as expected in systems with iptables backend for firewalld.
- A picture of a cute animal (not mandatory but encouraged)