Skip to content
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

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

Open
wants to merge 1 commit into
base: master
from

Conversation

@suwang48404
Copy link
Contributor

suwang48404 commented Oct 15, 2019

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.
@GordonTheTurtle

This comment has been minimized.

Copy link

GordonTheTurtle commented Oct 15, 2019

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 suwang48404 force-pushed the suwang48404:master branch from ea6a359 to 651eabc Oct 15, 2019
@GordonTheTurtle GordonTheTurtle removed the dco/no label Oct 15, 2019
@suwang48404 suwang48404 force-pushed the suwang48404:master branch 2 times, most recently from f148a99 to 55c5699 Oct 15, 2019
@suwang48404

This comment has been minimized.

Copy link
Contributor Author

suwang48404 commented Oct 15, 2019

@suwang48404

This comment has been minimized.

Copy link
Contributor Author

suwang48404 commented Oct 15, 2019

@arkodg

This comment has been minimized.

Copy link
Contributor

arkodg commented Oct 15, 2019

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

This comment has been minimized.

Copy link
Contributor Author

suwang48404 commented Oct 16, 2019

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

@suwang48404 suwang48404 reopened this Oct 16, 2019
@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Oct 16, 2019

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

This comment has been minimized.

Copy link
Contributor Author

suwang48404 commented Oct 16, 2019

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

This comment has been minimized.

Copy link
Contributor Author

suwang48404 commented Nov 5, 2019

@thaJeztah

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

Copy link
Member

thaJeztah left a comment

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 Outdated Show resolved Hide resolved
{enable: true, insert: true},
}

for _, i := range tests {

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Nov 7, 2019

Member

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

firewall_test.go Outdated Show resolved Hide resolved

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

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Nov 7, 2019

Member

Looks like there's an existing const for this in the actual (non-testing) code;

const userChain = "DOCKER-USER"

firewall_test.go Outdated Show resolved Hide resolved
}
rules := strings.Split(string(output), "\n")
if !enabled {
if len(rules)-1 != 1 {

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Nov 7, 2019

Member

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 🤔

This comment has been minimized.

Copy link
@suwang48404

suwang48404 Nov 7, 2019

Author Contributor

take ur firewall_test.go instead

controller.go Show resolved Hide resolved
t.Fatal(err)
}
if insert {
_, err = iptables.Raw("-A", fwdChainName, "-j", "DROP")

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Nov 7, 2019

Member

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

This comment has been minimized.

Copy link
@suwang48404

suwang48404 Nov 7, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Nov 8, 2019

Member

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 Outdated Show resolved Hide resolved
iptables.OnReloaded(func() {
var (
ctrl *controller = nil
userChainOnce sync.Once

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Nov 7, 2019

Member

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?

This comment has been minimized.

Copy link
@suwang48404

suwang48404 Nov 7, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Nov 8, 2019

Member

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 comment has been minimized.

Copy link
@suwang48404

suwang48404 Nov 8, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@suwang48404

suwang48404 Nov 8, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Nov 8, 2019

Member

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)

This comment has been minimized.

Copy link
@suwang48404

suwang48404 Nov 8, 2019

Author Contributor

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

@suwang48404 suwang48404 force-pushed the suwang48404:master branch from 55c5699 to f69597c Nov 7, 2019
@suwang48404 suwang48404 requested a review from thaJeztah Nov 7, 2019
@suwang48404

This comment has been minimized.

Copy link
Contributor Author

suwang48404 commented Nov 7, 2019

@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

@suwang48404 suwang48404 force-pushed the suwang48404:master branch 2 times, most recently from 3806be4 to f137635 Nov 7, 2019
@selansen

This comment has been minimized.

Copy link
Contributor

selansen commented Nov 10, 2019

@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.```
Copy link
Contributor

selansen left a comment

LGTM

@suwang48404

This comment has been minimized.

Copy link
Contributor Author

suwang48404 commented Nov 12, 2019

@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 suwang48404 requested a review from selansen Nov 12, 2019
@suwang48404

This comment has been minimized.

Copy link
Contributor Author

suwang48404 commented Nov 12, 2019

@euanh @arkodg can u guys help reviewing?

controller.go Outdated Show resolved Hide resolved
firewall_linux.go Outdated Show resolved Hide resolved
@euanh
euanh approved these changes Nov 12, 2019
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>
@suwang48404 suwang48404 force-pushed the suwang48404:master branch from f137635 to 54e7900 Nov 12, 2019
@suwang48404 suwang48404 requested a review from euanh Nov 12, 2019
@euanh
euanh approved these changes Nov 12, 2019
@selansen

This comment has been minimized.

Copy link
Contributor

selansen commented Nov 19, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.