controller: Check if IPTables is enabled for arrangeUserFilterRule

[phyber]

This allows the --iptables=false argument to the dockerd to work correctly
by checking if IPTables is enabled before creating the user filter rules
in the controller.

This probably fixes:

• dockerd when run with --iptables=false modifies iptables by adding DOCKER-USER #2158
• With iptables=false dockerd still creates DOCKER-USER chain and rules moby#35777
• dockerd --iptables=false adds DOCKER-USER chain and modify FORWARD chain anyway docker/for-linux#136

and possibly more. I didn't dig too deep.

[elboulangero]

Hey upstream, that would be great to have some feedback on that if you have a chance.

I'd like to import this patch in Debian to solve https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=903635. This bug is a blocker for buster release, because basically docker breaks the firewall of its users when they use iptables, and that's a serious problem.

[arkodg]

@elboulangero this should not break Debian anymore
5ce033c fixed the issue and it recently got vendored in via moby/moby@bcaa613

[elboulangero]

5ce033c fixed the issue and it recently got vendored in via moby/moby@bcaa613

@arkodg Thanks for the pointer, I was not aware of that! The Debian bug related to the fix you mention is #921600, and I will close it then.

However there's also #903635, which is really about --iptables=false, and is what is discussed here.

[GordonTheTurtle]

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "iptables-check" git@github.com:phyber/libnetwork.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354571648
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

[phyber]

I've moved the option check and if statement into the more appropriate firewall_linux.go.
However, I'm unable to add any tests because I simply cannot work out how this would even be tested.

I also don't know how to deal with the case where arrangeUserFilterRule is called as a function (rather than via the method on the controller), so there is a very real chance that dockerd will at some point still interfere with firewall rules.

[selansen]

Service_linux.go below functions uses IPTables APIs that might create/update IPChain.
fwMarker()
redirector()
programIngress()

with the PR, I don't think we are covering these code path. we might end up leaving the daemon iptable configuration in faulty state.

[arkodg]

LGTM
@mavenugo this should fix the issue for docker run, I've raised #2375 to add the extra fixes needed for docker service

[selansen]

I am really concerned with half fix. This will lead us into unexpected behavior when user start creating service/etc. just my 2 cents.

[mavenugo]

@selansen can you pls explain your point more ?
Setting --iptables=false on a specific node will certainly break many features especially those in swarm mode where the task placement will NOT consider the --iptables settings...
When the user sets this flag, I would assume that they know what they are doing and infact that is the case today.
Hence am trying to understand what you mean by "half fix" here .

[selansen]

my point is there is few other places where we invoke iptable chain commands ( I mentioned files info in the previous comment) that are not covered as part of this fix.
If that code path gets executed , we will still have few ipchain insert on behalf of libnetwork which is not desirable. That made me to believe this is partial fix but not complete one.