controller: Check if IPTables is enabled for arrangeUserFilterRule by phyber · Pull Request #2339 · moby/libnetwork

phyber · 2019-02-20T14:53:14Z

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 · 2019-04-25T03:23:10Z

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.

controller.go

arkodg · 2019-04-26T21:29:58Z

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

elboulangero · 2019-04-29T00:51:41Z

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.

This allows the `--iptables=false` argument to the `dockerd` to actually work. Signed-off-by: David O'Rourke <[email protected]>

GordonTheTurtle · 2019-04-29T12:50:55Z

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" [email protected]: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.

Signed-off-by: David O'Rourke <[email protected]>

phyber · 2019-04-29T14:38:01Z

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 · 2019-04-29T22:04:14Z

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 · 2019-05-10T15:48:17Z

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 · 2019-06-04T16:27:38Z

@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 · 2019-06-04T16:35:34Z

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.

full diffs: - moby/libnetwork@fc5a7d9...62a13ae - vishvananda/netlink@b2de5d1...v1.0.0 - vishvananda/netns@604eaf1...13995c7 notable changes in libnetwork: - moby/libnetwork#2366 Bump vishvananda/netlink to 1.0.0 - moby/libnetwork#2339 controller: Check if IPTables is enabled for arrangeUserFilterRule - addresses moby/libnetwork#2158 dockerd when run with --iptables=false modifies iptables by adding DOCKER-USER - addresses moby#35777 With iptables=false dockerd still creates DOCKER-USER chain and rules - addresses docker/for-linux#136 dockerd --iptables=false adds DOCKER-USER chain and modify FORWARD chain anyway - moby/libnetwork#2394 Make DNS records and queries case-insensitive - addresses moby#28689 Embedded DNS is case-sensitive - addresses moby#21169 hostnames with new networking are case-sensitive Signed-off-by: Sebastiaan van Stijn <[email protected]>

full diffs: - moby/libnetwork@fc5a7d9...62a13ae - vishvananda/netlink@b2de5d1...v1.0.0 - vishvananda/netns@604eaf1...13995c7 notable changes in libnetwork: - moby/libnetwork#2366 Bump vishvananda/netlink to 1.0.0 - moby/libnetwork#2339 controller: Check if IPTables is enabled for arrangeUserFilterRule - addresses moby/libnetwork#2158 dockerd when run with --iptables=false modifies iptables by adding DOCKER-USER - addresses moby/moby#35777 With iptables=false dockerd still creates DOCKER-USER chain and rules - addresses docker/for-linux#136 dockerd --iptables=false adds DOCKER-USER chain and modify FORWARD chain anyway - moby/libnetwork#2394 Make DNS records and queries case-insensitive - addresses moby/moby#28689 Embedded DNS is case-sensitive - addresses moby/moby#21169 hostnames with new networking are case-sensitive Signed-off-by: Sebastiaan van Stijn <[email protected]> Upstream-commit: 344b093258fcb2195fa393081e5224a6c766c798 Component: engine

full diffs: - moby/libnetwork@fc5a7d9...62a13ae - vishvananda/netlink@b2de5d1...v1.0.0 - vishvananda/netns@604eaf1...13995c7 notable changes in libnetwork: - moby/libnetwork#2366 Bump vishvananda/netlink to 1.0.0 - moby/libnetwork#2339 controller: Check if IPTables is enabled for arrangeUserFilterRule - addresses moby/libnetwork#2158 dockerd when run with --iptables=false modifies iptables by adding DOCKER-USER - addresses moby#35777 With iptables=false dockerd still creates DOCKER-USER chain and rules - addresses docker/for-linux#136 dockerd --iptables=false adds DOCKER-USER chain and modify FORWARD chain anyway - moby/libnetwork#2394 Make DNS records and queries case-insensitive - addresses moby#28689 Embedded DNS is case-sensitive - addresses moby#21169 hostnames with new networking are case-sensitive Signed-off-by: Sebastiaan van Stijn <[email protected]> (cherry picked from commit 344b093) Signed-off-by: Sebastiaan van Stijn <[email protected]>

full diffs: - moby/libnetwork@fc5a7d9...62a13ae - vishvananda/netlink@b2de5d1...v1.0.0 - vishvananda/netns@604eaf1...13995c7 notable changes in libnetwork: - moby/libnetwork#2366 Bump vishvananda/netlink to 1.0.0 - moby/libnetwork#2339 controller: Check if IPTables is enabled for arrangeUserFilterRule - addresses moby/libnetwork#2158 dockerd when run with --iptables=false modifies iptables by adding DOCKER-USER - addresses moby/moby#35777 With iptables=false dockerd still creates DOCKER-USER chain and rules - addresses docker/for-linux#136 dockerd --iptables=false adds DOCKER-USER chain and modify FORWARD chain anyway - moby/libnetwork#2394 Make DNS records and queries case-insensitive - addresses moby/moby#28689 Embedded DNS is case-sensitive - addresses moby/moby#21169 hostnames with new networking are case-sensitive Signed-off-by: Sebastiaan van Stijn <[email protected]> (cherry picked from commit 344b093258fcb2195fa393081e5224a6c766c798) Signed-off-by: Sebastiaan van Stijn <[email protected]> Upstream-commit: f3e1aff81df959e9178433b77e7f3364c22aee59 Component: engine

This PR fixes the regression introduced by moby#2339 to correctly insert the DOCKER-USER chain if iptables=true is set in the Daemon config Signed-off-by: Arko Dasgupta <[email protected]>

This PR fixes the regression introduced by moby#2339 to correctly insert the DOCKER-USER chain if iptables=true is set in the Daemon config Addresses : docker/for-linux#810 Signed-off-by: Arko Dasgupta <[email protected]>

This reverts commit 820deef, reversing changes made to 19e372a.

This reverts commit 820deef, reversing changes made to 19e372a. Signed-off-by: Arko Dasgupta <[email protected]>

Revert "Merge pull request #2339 from phyber/iptables-check"

Revert "Merge pull request moby#2339 from phyber/iptables-check" (cherry picked from commit 90afbb0) Signed-off-by: Arko Dasgupta <[email protected]>

full diffs: - moby/libnetwork@fc5a7d9...62a13ae - vishvananda/netlink@b2de5d1...v1.0.0 - vishvananda/netns@604eaf1...13995c7 notable changes in libnetwork: - moby/libnetwork#2366 Bump vishvananda/netlink to 1.0.0 - moby/libnetwork#2339 controller: Check if IPTables is enabled for arrangeUserFilterRule - addresses moby/libnetwork#2158 dockerd when run with --iptables=false modifies iptables by adding DOCKER-USER - addresses moby#35777 With iptables=false dockerd still creates DOCKER-USER chain and rules - addresses docker/for-linux#136 dockerd --iptables=false adds DOCKER-USER chain and modify FORWARD chain anyway - moby/libnetwork#2394 Make DNS records and queries case-insensitive - addresses moby#28689 Embedded DNS is case-sensitive - addresses moby#21169 hostnames with new networking are case-sensitive Signed-off-by: Sebastiaan van Stijn <[email protected]> Signed-off-by: zach <[email protected]>

arkodg reviewed Apr 26, 2019

View reviewed changes

controller.go Outdated Show resolved Hide resolved

controller: Check if IPTables is enabled for arrangeUserFilterRule

5e34060

This allows the `--iptables=false` argument to the `dockerd` to actually work. Signed-off-by: David O'Rourke <[email protected]>

phyber force-pushed the iptables-check branch from 85e5a55 to eb0c593 Compare April 29, 2019 12:50

GordonTheTurtle added the dco/no label Apr 29, 2019

Move hasIPTablesEnabled check into firewall_linux.go

a232658

Signed-off-by: David O'Rourke <[email protected]>

phyber force-pushed the iptables-check branch from eb0c593 to a232658 Compare April 29, 2019 12:51

GordonTheTurtle removed the dco/no label Apr 29, 2019

arkodg approved these changes May 10, 2019

View reviewed changes

selansen approved these changes Jun 21, 2019

View reviewed changes

euanh merged commit 820deef into moby:master Jun 21, 2019

thaJeztah mentioned this pull request Jun 25, 2019

bump libnetwork. vishvananda/netlink 1.0, vishvananda/netns moby/moby#39054

Merged

thaJeztah mentioned this pull request Aug 22, 2019

[19.03 backport] bump libnetwork to 92d1fbe1eb0883cf11d283cea8e658275146411d docker-archive/engine#330

Merged

arkodg mentioned this pull request Oct 9, 2019

[WIP] Add DOCKER-USER chain when iptables=true is set ENGCORE-1114 #2464

Closed

arkodg pushed a commit to arkodg/libnetwork that referenced this pull request Oct 11, 2019

Revert "Merge pull request moby#2339 from phyber/iptables-check"

aa34b95

This reverts commit 820deef, reversing changes made to 19e372a.

arkodg pushed a commit to arkodg/libnetwork that referenced this pull request Oct 11, 2019

Revert "Merge pull request moby#2339 from phyber/iptables-check"

8db595c

This reverts commit 820deef, reversing changes made to 19e372a. Signed-off-by: Arko Dasgupta <[email protected]>

selansen added a commit that referenced this pull request Oct 11, 2019

Merge pull request #2466 from arkodg/revert-iptables-docker-user

90afbb0

Revert "Merge pull request #2339 from phyber/iptables-check"

This was referenced Oct 11, 2019

Revert "Merge pull request #2339 from phyber/iptables-check" #2466

Merged

Revert alternatives #2467

Closed

This was referenced Oct 11, 2019

Merge pull request #2466 from arkodg/revert-iptables-docker-user #2468

Closed

Merge pull request #2466 from arkodg/revert-iptables-docker-user #2469

Closed

This was referenced Oct 11, 2019

[19.03] revert controller: Check if IPTables is enabled for arrangeUserFilterRule ENGCORE-1114 #2470

Merged

[19.03 backport] revert controller: Check if IPTables is enabled for arrangeUserFilterRule ENGCORE-1114 docker-archive/engine#404

Merged

selansen mentioned this pull request Nov 10, 2019

DOCKER-USER chain not created when IPTableEnable=false. #2471

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

controller: Check if IPTables is enabled for arrangeUserFilterRule#2339

controller: Check if IPTables is enabled for arrangeUserFilterRule#2339
euanh merged 2 commits intomoby:masterfrom
phyber:iptables-check

phyber commented Feb 20, 2019

Uh oh!

elboulangero commented Apr 25, 2019

Uh oh!

Uh oh!

arkodg commented Apr 26, 2019

Uh oh!

elboulangero commented Apr 29, 2019 •

edited

Loading

Uh oh!

GordonTheTurtle commented Apr 29, 2019

Uh oh!

phyber commented Apr 29, 2019

Uh oh!

selansen commented Apr 29, 2019 •

edited

Loading

Uh oh!

arkodg left a comment

Uh oh!

selansen commented May 10, 2019

Uh oh!

mavenugo commented Jun 4, 2019

Uh oh!

selansen commented Jun 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

phyber commented Feb 20, 2019

Uh oh!

elboulangero commented Apr 25, 2019

Uh oh!

Uh oh!

arkodg commented Apr 26, 2019

Uh oh!

elboulangero commented Apr 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GordonTheTurtle commented Apr 29, 2019

Uh oh!

phyber commented Apr 29, 2019

Uh oh!

selansen commented Apr 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arkodg left a comment

Choose a reason for hiding this comment

Uh oh!

selansen commented May 10, 2019

Uh oh!

mavenugo commented Jun 4, 2019

Uh oh!

selansen commented Jun 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

elboulangero commented Apr 29, 2019 •

edited

Loading

selansen commented Apr 29, 2019 •

edited

Loading