Skip to content

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

Closed
arkodg wants to merge 3 commits into
moby:masterfrom
arkodg:fix-user-chain
Closed

[WIP] Add DOCKER-USER chain when iptables=true is set ENGCORE-1114#2464
arkodg wants to merge 3 commits into
moby:masterfrom
arkodg:fix-user-chain

Conversation

@arkodg

@arkodgarkodg commented Oct 9, 2019•
edited by atlassian Bot
Loading

Copy link
Copy Markdown
Contributor

This PR fixes the regression introduced by
#2339 to

  1. correctly insert the DOCKER-USER chain if iptables=true is set in the Daemon config
  2. To make sure DOCKER-USER is the first chain and DOCKER-INGRESS is the second chain in forwarding
  3. To make sure we do not create DOCKER-INGRESS and DOCKER-USER if iptables=false

All the logic has been moved to the bridge driver since EnableIPTables is a bridge specific configuration

Addresses : docker/for-linux#810ENGCORE-1114

Signed-off-by: Arko Dasgupta arko.dasgupta@docker.com

@arkodg

arkodg commented Oct 9, 2019

Copy link
Copy Markdown
ContributorAuthor

PTAL @selansen@mavenugo@joeabbey

@selansen

selansen commented Oct 9, 2019

Copy link
Copy Markdown
Contributor

@arkodg , before this change, I noticed arrangeUserFilterRule is getting called white creating NewNetwork(). Now I see we invoke only for bridge driver. Any specific reason why this has been moved from generic code base to inside bridge driver ?

@arkodg

arkodg commented Oct 9, 2019

Copy link
Copy Markdown
ContributorAuthor

@selansen because this piece of code correctly deciphers whether enableIPTables is set or not and this is where we take care of fundamental things like enable ip_forwarding

selansen
selansen approved these changes Oct 10, 2019
View reviewed changes

@selansenselansen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@selansen

selansen commented Oct 10, 2019

Copy link
Copy Markdown
Contributor

@mavenugo , PTAL .

suwang48404
suwang48404 reviewed Oct 10, 2019
View reviewed changes
Comment threaddrivers/bridge/bridge.go
iptables.OnReloaded(func() { logrus.Debugf("Recreating iptables chains on firewall reload"); setupIPChains(config) })

// Add DOCKER-USER chain
arrangeUserFilterRule()

@suwang48404suwang48404Oct 10, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now arrangeUserFIlterRule is called on all systems, linux/windows/bsd etc, is this by intention?

@arkodgarkodgOct 10, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK bridge is for linux only

@suwang48404suwang48404Oct 10, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose for any *nix, we expect to create bridge. I see somewhere there are _bsd specific imple. So I am guessing at some point before at least bsd is supported?

suwang48404
suwang48404 reviewed Oct 10, 2019
View reviewed changes
Comment threaddrivers/bridge/bridge.go
vethLen = 7
defaultContainerVethPrefix = "eth"
maxAllocatePortAttempts = 10
userChain = "DOCKER-USER"

@suwang48404suwang48404Oct 10, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have similar questions as Elango, why move to bridge.go, may be the real problem is that controller.hasIPTableEnabled() isn't the correct value?

@suwang48404suwang48404Oct 10, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by moving arrangeUserFilterRule to bridge only network, it means unless a bridge network is created, there won't be DOCKER_USER chain. It will work as dockerd always creates docker9 bridge, but for me the original approach is cleaner.

@arkodgarkodgOct 10, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK from https://github.com/moby/moby/blob/ad1b781e44fa1e44b9e654e5078929aec56aed66/daemon/config/config_unix.go#L50 and 9c6ab12
EnableIPTables is specific to bridge

@guillaumerose

guillaumerose commented Oct 10, 2019

Copy link
Copy Markdown

Is it possible to add an e2e test somewhere that checks that after dockerd startup sequence the output of iptables -L is fine ?

@euanh
euanh self-requested a review October 10, 2019 12:58
euanh
euanh suggested changes Oct 10, 2019
View reviewed changes

@euanheuanh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm uncomfortable merging anything else in this area until we have some sort of regression test. It doesn't need to be fancy - just something that checks that the iptables setup looks sane, as @guillaumerose suggested.

@arkodg

arkodg commented Oct 10, 2019

Copy link
Copy Markdown
ContributorAuthor

yah, I wanted to decide where to put the code before I added a TC, added one now

@arkodg
arkodg requested a review from euanhOctober 10, 2019 18:50
@dougburksdougburks mentioned this pull request Oct 10, 2019
Closed
3 tasks
@arkodgarkodg changed the title Add DOCKER-USER chain when iptables=true is set[DO NOT MERGE] Add DOCKER-USER chain when iptables=true is setOct 11, 2019
@arkodgarkodg changed the title [DO NOT MERGE] Add DOCKER-USER chain when iptables=true is set[WIP] Add DOCKER-USER chain when iptables=true is setOct 11, 2019
Arko Dasgupta added 2 commits October 10, 2019 19:57
Arko Dasgupta
Add DOCKER-USER chain when iptables=true is set
083c739
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 <arko.dasgupta@docker.com>
Arko Dasgupta
Add TC for making sure DOCKER-USER exists
a8417b6
Signed-off-by: Arko Dasgupta <arko.dasgupta@docker.com>
Arko Dasgupta
Move arrangeIngressFilterRule to bridge driver
1434743
Signed-off-by: Arko Dasgupta <arko.dasgupta@docker.com>
@andrewhsuandrewhsu changed the title [WIP] Add DOCKER-USER chain when iptables=true is set[WIP] Add DOCKER-USER chain when iptables=true is set ENGCORE-1114Oct 11, 2019
@arkodgarkodg closed this Oct 15, 2019
@rwdimrwdim mentioned this pull request Jul 29, 2020
Open
2 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@euanheuanhAwaiting requested review from euanh
2 more reviewers
@selansenselansenselansen approved these changes
@suwang48404suwang48404suwang48404 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@arkodg@selansen@guillaumerose@euanh@suwang48404