-
Notifications
You must be signed in to change notification settings - Fork 880
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
Limited bridge netfilter application. #2497
base: master
Are you sure you want to change the base?
Conversation
Please sign your commits following these rules: $ git clone -b "limited-bridge-netfilter" git@github.com:tomkcook/libnetwork.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f Amending updates the existing PR. You DO NOT need to open a new one. |
Thanks for raising a PR @tomkcook
|
@@ -64,11 +64,12 @@ func checkBridgeNetFiltering(config *networkConfiguration, i *bridgeInterface) e | |||
if err != nil { | |||
logrus.Warnf("failed to check %s forwarding: %v", ipVerName, err) | |||
} else if enabled { | |||
enabled, err := getKernelBoolParam(getBridgeNFKernelParam(ipVer)) | |||
bridgeName := i.Link.Attrs().Name |
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 see that we already have something similar in https://github.com/docker/libnetwork/blob/feeff4f0a3fd2a2bb19cf67c826082c66ffaaed9/drivers/bridge/setup_bridgenetfiltering.go#L55
Can we use any one way of deriving the bridgeName
and reference it everywhere else
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.
Done. It got mixed up with the requested signoff and commit comment changes so has ended up in the same commit, apologies.
2605b47
to
5aa4008
Compare
libnetwork uses the br-netfilter module to allow filtering of packets passing through a bridge. To do so, it sets /proc/sys/net/bridge/bridge-nf-call-ip[6]tables to 1, forcing iptables for every bridge on the system, whether this is desired or not. This overrides anything set in /etc/sysctl.conf. This change prevents libnetwork from setting the system-wide /proc/sys/net/bridge/bridge-nf-call-ip[6]tables and instead sets /sys/class/net/<bridge-name>/bridge/nf_call_ip[6]tables for each bridge which has ICC disabled. Note that this does introduce a change in the behaviour of docker. For a default network configuration, with the existing behaviour, both `docker_gwbridge` and `docker0` bridges have iptables enabled while this change results in `docker_gwbridge` having iptables enabled but `docker0` having iptables disabled, because ICC is enabled by default. As far as I can tell, iptables should not be enabled on the `docker0` bridge when ICC is enabled (the code which implements this seems to assume that iptables is enabled per-bridge and not systemwide) so I think this change is correct, but it is still a change in behaviour. Signed-off-by: Tom Cook <tom.k.cook@gmail.com>
5aa4008
to
446b688
Compare
Done. |
these changes look good @tomkcook |
switch ipVer { | ||
case ipv4: | ||
return "/proc/sys/net/bridge/bridge-nf-call-iptables" | ||
return fmt.Sprintf("/sys/class/net/%s/bridge/nf_call_iptables", bridgeName) |
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.
Should probably use path.Join()
return fmt.Sprintf("/sys/class/net/%s/bridge/nf_call_iptables", bridgeName) | |
return path.Join("/sys/class/net", bridgeName, "bridge/nf_call_iptables") |
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'm not exactly clear how one is better than the other; both versions still have assumptions about what the path separator is and, anyway, the whole thing is linux-specific. Does swapping to path.Join
improve this in some way?
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.
We're constructing a path, so we should use a function that's designed for that.
Looking at this, we should also validate bridgeName
before use: this value is passed as a label, and we likely don't want ../../
to be used
case ipv6: | ||
return "/proc/sys/net/bridge/bridge-nf-call-ip6tables" | ||
return fmt.Sprintf("/sys/class/net/%s/bridge/nf_call_ip6tables", bridgeName) |
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 here;
return fmt.Sprintf("/sys/class/net/%s/bridge/nf_call_ip6tables", bridgeName) | |
return path.Join("/sys/class/net", bridgeName, "bridge/nf_call_ip6tables") |
Would that remove the |
@thaJeztah no, only suggesting we rewrite it to
|
Unrelated to this PR directly, but I noticed that the func checkBridgeNetFiltering(config *networkConfiguration, i *bridgeInterface) error { It was added in https://github.com/docker/libnetwork/pull/336/files, and looks to be because it needs to satisfy the (could probably be changed to |
@tomkcook can incorporate https://github.com/docker/libnetwork/issues/2488#issuecomment-571746499 (remove the |
Note we have migrated this codebase over to github.com/moby/moby/libnetwork. |
Fixes moby/moby#47127.