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

suwang48404 · 2019-10-15T04:24:13Z

This fix addresses https://docker.atlassian.net/browse/ENGCORE-1115
Expected behaviors upon docker engine restarts:

IPTableEnable=true, DOCKER-USER chain present
-- no change to DOCKER-USER chain
IPTableEnable=true, DOCKER-USER chain not present
-- DOCKER-USER chain created and inserted top of FORWARD
chain.
IPTableEnable=false, DOCKER-USER chain present
-- no change to DOCKER-USER chain
the rational is that DOCKER-USER is populated
and may be used by end-user for purpose other than
filtering docker container traffic. Thus even if
IPTableEnable=false, docker engine does not touch
pre-existing DOCKER-USER chain.
IPTableEnable=false, DOCKER-USER chain not present
-- DOCKER-USER chain is not created.

GordonTheTurtle · 2019-10-15T04:24:15Z

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 "master" git@github.com:suwang48404/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.

suwang48404 · 2019-10-15T04:58:47Z

@arkodg @selansen @euanh @chiragtayal

suwang48404 · 2019-10-15T04:58:57Z

@joeabbey

arkodg · 2019-10-15T23:28:39Z

Some useful documentation in this area -
https://docs.docker.com/network/iptables/

This wont take care of the cases where DOCKER-INGRESS is created if there are services running

IMHO a clean way to fix this could be to incorporate this flag into
https://github.com/docker/libnetwork/blob/master/iptables/iptables.go and error out from functions such as NewChain or ProgramChain if the flag is disabled with an error saying iptables is disabled in config

suwang48404 · 2019-10-16T00:04:09Z

Some useful documentation in this area -
https://docs.docker.com/network/iptables/

This wont take care of the cases where DOCKER-INGRESS is created if there are services running

IMHO a clean way to fix this could be to incorporate this flag into
https://github.com/docker/libnetwork/blob/master/iptables/iptables.go and error out from functions such as NewChain or ProgramChain if the flag is disabled with an error saying iptables is disabled in config

Arko,

I did spend sometime considering the approach, and feel this commit addressed user's need while not potentially causing unexpected behavior.

-- iptables=false (a.k.a prevent docker manipulate iptables) has a limited use cases for conrtainers that with (network mode=null, host, macvlan), in these uses cases this commit allows DOCKER-USER not created, and DOCKER-INGRESS chain will also not be created as there is no swarm service.
-- in most common container use cases (network mode=bridge, overlay), I feel "iptables=false" should be ignore even if it is set. This is because without docker engine manipulate iptables for NATting/forwarding rules, connectivity to those container are broken.

In another word, I ask in what circumstances where a user (correctly) wants to disable iptable manipulation by docker engine? maybe when containers need no network connectivities (network mode =null), or container expose network port/ip directly (network mode = host, macvlan). In these cases, DOCKER-USER/DOCKER-INGRESS will not be created.

Thx, Su

thaJeztah · 2019-10-16T02:15:06Z

-- in most common container use cases (network mode=bridge, overlay), I feel "iptables=false" should be ignore even if it is set. This is because without docker engine manipulate iptables for NATting/forwarding rules, connectivity to those container are broken.

I know that there's definitely users running with --iptables=false; their use case is that they don't want docker to automatically expose ports / traffic to the containers, and they manually set up rules to make networking work

suwang48404 · 2019-10-16T04:49:41Z

-- in most common container use cases (network mode=bridge, overlay), I feel "iptables=false" should be ignore even if it is set. This is because without docker engine manipulate iptables for NATting/forwarding rules, connectivity to those container are broken.

I know that there's definitely users running with --iptables=false; their use case is that they don't want docker to automatically expose ports / traffic to the containers, and they manually set up rules to make networking work

Hi Sebastian,

thx for the heads-up with regard to iptable=false use case, i did not know that ...

The question is are we comfortable changing existing docker engine ibehavior and strictly enforcing iptable=false, i.e no iptable programming at all, or error on caution?

Thx, Su

suwang48404 · 2019-11-05T17:41:12Z

@thaJeztah

Hi Sebastiaan, can u please help review. As this will eventually be moved to dockerd?
thx, Su

thaJeztah

left some comments; still trying to figure out what the best approach would be (not a fan of the current global variable / sync.once)

controller.go

thaJeztah · 2019-11-06T17:24:45Z

firewall_test.go

+		{enable: true, insert: true},
+	}
+
+	for _, i := range tests {


can you use a different name for this variable? i is more commonly used for the index, than for the value

firewall_test.go

thaJeztah · 2019-11-06T17:50:21Z

firewall_test.go

+
+const (
+	fwdChainName = "FORWARD"
+	usrChainName = "DOCKER-USER"


Looks like there's an existing const for this in the actual (non-testing) code; https://github.com/docker/libnetwork/blob/90afbb01e1d8acacb505a092744ea42b9f167377/firewall_linux.go#L8

firewall_test.go

thaJeztah · 2019-11-06T18:42:19Z

firewall_test.go

+	}
+	rules := strings.Split(string(output), "\n")
+	if !enabled {
+		if len(rules)-1 != 1 {


Having to use len(rules)-1 everywhere makes it a bit difficult to read; IIUC, the -1 is to compensate for the trailing newline; perhaps add a small helper function that reads all rules in a chain, and drops the last one?

Something like;

func getRules(t *testing.T, chain string) []string { t.Helper() output, err := iptables.Raw("-S", chain) assert.NilError(t, err, "chain %s: failed to get rules", chain) rules := strings.Split(string(output), "\n") if len(rules) > 0 { rules = rules[:len(rules)-1] } return rules }

Also, perhaps instead of checking number of rules, we should add an expected []string to the tests, to check that the expected rules are created 🤔

take ur firewall_test.go instead

controller.go

thaJeztah · 2019-11-07T02:15:57Z

firewall_test.go

+		t.Fatal(err)
+	}
+	if insert {
+		_, err = iptables.Raw("-A", fwdChainName, "-j", "DROP")


I'm trying to understand what the insert option is testing, as we don't have a combination iptables=false, and insert=true

the reason to have insert is that in case there is already a rule in FORWARD chain, DOCKER-CHAIN rule would be placed ahead of that rule when iptable=true; if iptable=false, it is moot because we do add DOCKER-CHAIN rule to FORWARD chain.

Right, so the thing that worries me a bit in the test, is that we're creating a new code-path just for the test. The production code does not have check wether or not this should be called. (The production code would (currently) call setupArrangeUserFilterRule and arrangeUserFilterRule when calling controller.NewNetwork(), which is not the code-path we're testing.

And because of userChainOnce and ctrl being global, we can't test different permutations of iptables being enabled/disabled etc.

firewall_test.go

thaJeztah · 2019-11-07T02:58:43Z

firewall_linux.go

-	iptables.OnReloaded(func() {
+var (
+	ctrl          *controller = nil
+	userChainOnce sync.Once


Wondering why these are implemented as global variables, and not as a property on the controller

IIUC, currently, the DOCKER-USER chain is created when first calling NewNetwork - should we create it on New() (so when constructing the controller?) Or perhaps have an exported function that can be called from dockerd when starting the daemon.

I think the intent is not to cal this only once, but to make it idempotent, correct?

yes, cal_ once is the mean to goal of not setup DOCKER-USER chain multiple times.

Right, but effectively we're making controller a singleton(-like) construct, which makes the testing difficult (see above), but also makes the first controller that's created "magic" as that's the controller that now owns iptables.

So, wondering if there should be a sync.Once per controller, and only call if it iptables is enabled. I see arrangeUserFilterRule is already (largely) idempotent (as in: it stops adding the return/jump rules if the chain already existed).

Is iptables enabled/disabled a global option, or one per controller (wondering) 🤔

This fix assumes there shall be only one instance of controller per docker engine (instantiated moby/moby/daemon/daemon_unix.go). I'd think (please correct me) many things will be broken if multiple libnetwork controllers are created within the same engine.

in another word, iptables enabled/disabled is a global option, it is executed based on singleton controller configuration. Hope this make sense.

I realise having multiple controllers is not a realistic scenario; it's not what the code reflects though; there's a New(), which "creates a new instance of network controller", so it's not a singleton.

If:

iptables enable/disabled is not something that can be changed during the lifetime of a controller (wether that be a single one, or multiple)

arrangeUserFilterRule is idempotent (even if multiple controllers were instantiated, they would not trip over each other, because they'd only mutate iptables if the DOCKER-USER chain doesn't exist)

Then, why not setup the DOCKER-USER chain when instantiating the controller? (I don't think a sync.once would even be needed in that case)

sorry I misunderstood u. I have moved invocation of setupArrangeUserFilterRule to controller.go:New(), and do away with sync.once.
Thx, Su

suwang48404 · 2019-11-07T19:06:24Z

@thaJeztah I have taken ur firewall_test.go, it is much cleaner, thanks alot.
Beyond that, let me know if there is anything else I should address.

thx, Su

selansen · 2019-11-10T03:55:06Z

@suwang48404 , changes look good to me. If I remember correctly there were few places I mentioned #2339 that this code path fix didnt cover. Can you pls take a look and let me know if that is ok. coping from #2339 PR comment.

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.```

selansen

LGTM

suwang48404 · 2019-11-12T15:45:14Z

@suwang48404 , changes look good to me. If I remember correctly there were few places I mentioned #2339 that this code path fix didnt cover. Can you pls take a look and let me know if that is ok. coping from #2339 PR comment.
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.```

@selansen The fix is pointed fix for docekr-user chain only. The iptable usage is pervasive in libnetwork, we do not have a good understanding and/or test metrics to confidently disable all iptable programming when iptable=false. IMHO, the risk is to0 high for such general fix.

suwang48404 · 2019-11-12T16:01:18Z

@euanh @arkodg can u guys help reviewing?

controller.go

firewall_linux.go

This fix addresses https://docker.atlassian.net/browse/ENGCORE-1115 Expected behaviors upon docker engine restarts: 1. IPTableEnable=true, DOCKER-USER chain present -- no change to DOCKER-USER chain 2. IPTableEnable=true, DOCKER-USER chain not present -- DOCKER-USER chain created and inserted top of FORWARD chain. 3. IPTableEnable=false, DOCKER-USER chain present -- no change to DOCKER-USER chain the rational is that DOCKER-USER is populated and may be used by end-user for purpose other than filtering docker container traffic. Thus even if IPTableEnable=false, docker engine does not touch pre-existing DOCKER-USER chain. 4. IPTableEnable=false, DOCKER-USER chain not present -- DOCKER-USER chain is not created. Signed-off-by: Su Wang <su.wang@docker.com>

selansen · 2019-11-19T16:23:24Z

@thaJeztah PTAL ? it looks good to me . wanted to make sure your review comments are updated and you approve this one.

arkodg · 2020-03-30T16:28:33Z

PTAL @selansen @thaJeztah

selansen

LGTM

full diff: moby/libnetwork@ef149a9...1a17fb3 - moby/libnetwork#2538 produce an error with invalid address pool - addresses moby#40388 dockerd ignores the --default-address-pool option - moby/libnetwork#2471 DOCKER-USER chain not created when IPTableEnable=false - moby/libnetwork#2544 Fix NPE due to null value returned by ep.Iface() - carries moby/libnetwork#2239 Prevent NPE in addServiceInfoToCluster() - addresses moby#37506 Error initializing docker.server while starting daemon by systemd Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

full diff: moby/libnetwork@ef149a9...1a17fb3 - moby/libnetwork#2538 produce an error with invalid address pool - addresses moby/moby#40388 dockerd ignores the --default-address-pool option - moby/libnetwork#2471 DOCKER-USER chain not created when IPTableEnable=false - moby/libnetwork#2544 Fix NPE due to null value returned by ep.Iface() - carries moby/libnetwork#2239 Prevent NPE in addServiceInfoToCluster() - addresses moby/moby#37506 Error initializing docker.server while starting daemon by systemd Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Upstream-commit: c3808634e7a44fc5b4fb8caacd9d079f7e0a0fee Component: engine

arrangeUserFilterRule uses the package-level [`ctrl` variable][1], which holds a reference to a controller instance. This variable is set by [`setupArrangeUserFilterRule()`][2], which is called when initialization a controller ([`libnetwork.New`][3]). In normal circumstances, there would only be one controller, created during daemon startup, and the instance of the controller would be the same as the controller that `NewNetwork` is called from, but there's no protection for the `ctrl` variable, and various integration tests create their own controller instance. The global `ctrl` var was introduced in [54e7900fb89b1aeeb188d935f29cf05514fd419b][4], with the assumption that [only one controller could ever exist][5]. This patch tries to reduce uses of the `ctrl` variable, and as we're calling this code from inside a method on a specific controller, we inline the code and use that specific controller instead. [1]: https://github.com/moby/moby/blob/37b908aa628ccf8f1e10182d2fc049423707422d/libnetwork/firewall_linux.go#L12 [2]: https://github.com/moby/moby/blob/37b908aa628ccf8f1e10182d2fc049423707422d/libnetwork/firewall_linux.go#L14-L17 [3]: https://github.com/moby/moby/blob/37b908aa628ccf8f1e10182d2fc049423707422d/libnetwork/controller.go#L163 [4]: moby/libnetwork@54e7900 [5]: moby/libnetwork#2471 (comment) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

GordonTheTurtle added the dco/no label Oct 15, 2019

suwang48404 force-pushed the master branch from ea6a359 to 651eabc Compare October 15, 2019 04:25

GordonTheTurtle removed the dco/no label Oct 15, 2019

suwang48404 force-pushed the master branch 2 times, most recently from f148a99 to 55c5699 Compare October 15, 2019 04:51

suwang48404 closed this Oct 16, 2019

suwang48404 reopened this Oct 16, 2019

thaJeztah requested changes Nov 7, 2019

View reviewed changes

suwang48404 force-pushed the master branch from 55c5699 to f69597c Compare November 7, 2019 18:58

suwang48404 requested a review from thaJeztah November 7, 2019 19:02

suwang48404 force-pushed the master branch 2 times, most recently from 3806be4 to f137635 Compare November 8, 2019 20:00

selansen approved these changes Nov 12, 2019

View reviewed changes

suwang48404 requested a review from selansen November 12, 2019 15:45

euanh reviewed Nov 12, 2019

View reviewed changes

controller.go Outdated Show resolved Hide resolved

euanh reviewed Nov 12, 2019

View reviewed changes

firewall_linux.go Outdated Show resolved Hide resolved

euanh approved these changes Nov 12, 2019

View reviewed changes

suwang48404 force-pushed the master branch from f137635 to 54e7900 Compare November 12, 2019 16:32

suwang48404 requested a review from euanh November 12, 2019 17:08

euanh approved these changes Nov 12, 2019

View reviewed changes

selansen approved these changes Mar 30, 2020

View reviewed changes

arkodg merged commit 1a17fb3 into moby:master Apr 9, 2020

thaJeztah mentioned this pull request Apr 10, 2020

vendor: update libnetwork 1a17fb36132631a95fe6bb055b91e24a516ad81d moby/moby#40808

Merged

thaJeztah mentioned this pull request Jul 21, 2023

libnetwork/iptables: cleaning up: "there's more where that came from" moby/moby#46043

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

suwang48404 commented Oct 15, 2019

GordonTheTurtle commented Oct 15, 2019

suwang48404 commented Oct 15, 2019

suwang48404 commented Oct 15, 2019

arkodg commented Oct 15, 2019

suwang48404 commented Oct 16, 2019 •

edited

Loading

thaJeztah commented Oct 16, 2019

suwang48404 commented Oct 16, 2019

suwang48404 commented Nov 5, 2019

thaJeztah left a comment

thaJeztah Nov 6, 2019

thaJeztah Nov 6, 2019

thaJeztah Nov 6, 2019

suwang48404 Nov 7, 2019

thaJeztah Nov 7, 2019

suwang48404 Nov 7, 2019

thaJeztah Nov 8, 2019

thaJeztah Nov 7, 2019

suwang48404 Nov 7, 2019

thaJeztah Nov 8, 2019

suwang48404 Nov 8, 2019

suwang48404 Nov 8, 2019

thaJeztah Nov 8, 2019

suwang48404 Nov 8, 2019

suwang48404 commented Nov 7, 2019

selansen commented Nov 10, 2019

selansen left a comment

suwang48404 commented Nov 12, 2019

suwang48404 commented Nov 12, 2019

selansen commented Nov 19, 2019

arkodg commented Mar 30, 2020

selansen left a comment

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

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

Conversation

suwang48404 commented Oct 15, 2019

GordonTheTurtle commented Oct 15, 2019

suwang48404 commented Oct 15, 2019

suwang48404 commented Oct 15, 2019

arkodg commented Oct 15, 2019

suwang48404 commented Oct 16, 2019 • edited Loading

thaJeztah commented Oct 16, 2019

suwang48404 commented Oct 16, 2019

suwang48404 commented Nov 5, 2019

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suwang48404 commented Nov 7, 2019

selansen commented Nov 10, 2019

selansen left a comment

Choose a reason for hiding this comment

suwang48404 commented Nov 12, 2019

suwang48404 commented Nov 12, 2019

selansen commented Nov 19, 2019

arkodg commented Mar 30, 2020

selansen left a comment

Choose a reason for hiding this comment

suwang48404 commented Oct 16, 2019 •

edited

Loading