Fix duplicate iptables rules by jessfraz · Pull Request #6950 · moby/moby

jessfraz · 2014-07-10T05:42:30Z

If iptables version is < 1.4.11, try to delete the rule vs. checking if it exists. Fixes #6831.

Docker-DCO-1.1-Signed-off-by: Jessica Frazelle jfrazelle@users.noreply.github.com (github: jfrazelle)

jessfraz · 2014-07-10T13:48:11Z

I'm not sure if this is the best plan of attack, but of the options I could think of it was the least bad.

I'll go over what those were.

For verifying that iptables has the -C,--check option, it would be awesome if we could do the same as --wait in a one line command that checks if the exit status is 0. But to do this with -C we need to know a rule already exists. To do this we would need to create a rule just to check for it and then delete said rule. To avoid the possibility of any overlap of a user created rule we should create a new user-defined chain, create a rule in that chain, check for the rule, then flush all rules from the chain, and delete the chain. That gets a bit messy IMO. So that would be why I took the route of parsing for the iptables version. According to their changelog, -C,--check was added in v1.4.11.

That leaves the Exists function. We can keep the same functionality if supportsCheck is true. But if it's false we have the option of grepping -L for a consistent portion of args or trying to delete the rule only to recreate it anyways. I went with delete.

Of course there is always the option of changing the iptables version in the docs (re hack/PACKAGERS.md & docs/sources/installation/binaries.md) from 1.4 to 1.4.11.

vbatts · 2014-07-10T15:40:36Z

I'll take a look into this. Though I'm not keen on bumping the iptables requirement to 1.4.11, because that would constrain some distributions to have to exclude or carry silly patches to provide docker.

vieux · 2014-07-10T17:36:58Z

@jfrazelle something we use somewhere else in the code in the try with the new option -C if it's unknown it print the usage and return an != 0 return code. Then retry without the -C

jessfraz · 2014-07-10T23:35:27Z

ok I think I simplified things

vbatts · 2014-07-11T15:57:12Z

LGTM

Docker daemon log

[22bfbecd] +job init_networkdriver()
[debug] /sbin/iptables, [-C POSTROUTING -t nat -s 172.17.42.1/16 ! -o docker0 -j MASQUERADE]
[debug] /sbin/iptables, [-D POSTROUTING -t nat -s 172.17.42.1/16 ! -o docker0 -j MASQUERADE]
[debug] /sbin/iptables, [-I POSTROUTING -t nat -s 172.17.42.1/16 ! -o docker0 -j MASQUERADE]
[debug] /sbin/iptables, [-D FORWARD -i docker0 -o docker0 -j DROP]
[debug] /sbin/iptables, [-C FORWARD -i docker0 -o docker0 -j ACCEPT]
[debug] /sbin/iptables, [-D FORWARD -i docker0 -o docker0 -j ACCEPT]
[debug] driver.go:210 Enable inter-container communication
[debug] /sbin/iptables, [-I FORWARD -i docker0 -o docker0 -j ACCEPT]
[debug] /sbin/iptables, [-C FORWARD -i docker0 ! -o docker0 -j ACCEPT]
[debug] /sbin/iptables, [-D FORWARD -i docker0 ! -o docker0 -j ACCEPT]
[debug] /sbin/iptables, [-I FORWARD -i docker0 ! -o docker0 -j ACCEPT]
[debug] /sbin/iptables, [-C FORWARD -o docker0 -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT]
[debug] /sbin/iptables, [-D FORWARD -o docker0 -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT]
[debug] /sbin/iptables, [-I FORWARD -o docker0 -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT]
[debug] /sbin/iptables, [-t nat -D PREROUTING -m addrtype --dst-type LOCAL -j DOCKER]
[debug] /sbin/iptables, [-t nat -D OUTPUT -m addrtype --dst-type LOCAL ! --dst 127.0.0.0/8 -j DOCKER]
[debug] /sbin/iptables, [-t nat -D OUTPUT -m addrtype --dst-type LOCAL -j DOCKER]
[debug] /sbin/iptables, [-t nat -D PREROUTING -j DOCKER]
[debug] /sbin/iptables, [-t nat -D OUTPUT -j DOCKER]
[debug] /sbin/iptables, [-t nat -F DOCKER]
[debug] /sbin/iptables, [-t nat -X DOCKER]
[debug] /sbin/iptables, [-t nat -N DOCKER]
[debug] /sbin/iptables, [-t nat -A PREROUTING -m addrtype --dst-type LOCAL -j DOCKER]
[debug] /sbin/iptables, [-t nat -A OUTPUT -m addrtype --dst-type LOCAL ! --dst 127.0.0.0/8 -j DOCKER]
[22bfbecd] -job init_networkdriver() = OK (0)
[22bfbecd.initserver()] Creating pidfile
[22bfbecd.initserver()] Setting up signal traps
[22bfbecd] -job initserver() = OK (0)
[22bfbecd] +job acceptconnections()
[22bfbecd] -job acceptconnections() = OK (0)

`iptables -t nat -L

Chain PREROUTING (policy ACCEPT)
target     prot opt source               destination
DOCKER     all  --  anywhere             anywhere            ADDRTYPE match dst-type LOCAL

Chain POSTROUTING (policy ACCEPT)
target     prot opt source               destination
MASQUERADE  all  --  172.17.0.0/16        anywhere
MASQUERADE  all  --  172.17.0.0/16       !172.17.0.0/16
MASQUERADE  tcp  --  192.168.122.0/24    !192.168.122.0/24    masq ports: 1024-65535
MASQUERADE  udp  --  192.168.122.0/24    !192.168.122.0/24    masq ports: 1024-65535
MASQUERADE  all  --  192.168.122.0/24    !192.168.122.0/24

Chain OUTPUT (policy ACCEPT)
target     prot opt source               destination
DOCKER     all  --  anywhere            !loopback/8          ADDRTYPE match dst-type LOCAL

Chain DOCKER (2 references)
target     prot opt source               destination

jpetazzo · 2014-07-11T19:24:01Z

I have mixed feelings about this! For two reasons.

The name of the function is Exists() but if we are using the old version of iptables, it will delete an existing rule. It's not a big deal, since currently the function is only used in the context of "check if the rule exists, and add it otherwise"; but if it gets reused later, someone might legitimately believe that it just checks for existence, and doesn't have side effects.
It will remove and re-add the rule, which might change the position of the rule in the chain (if people have customized their ruleset).

"But what should we do then?"

I would suggest to parse the output of iptables-save. Calling iptables-save is a bit more taxing on resources, but since it is done only when the daemon starts, I believe it's an acceptable trade-off. I can give more details if needed.

crosbymichael · 2014-07-11T22:22:51Z

pkg/iptables/iptables.go

I would probably move this delete out of the Exist function and have the caller determine if they want to delete the rule or not. I think this is two separate issues, the -C on the check is not always available and then the deletion of duplication rules

crosbymichael · 2014-07-22T00:38:23Z

@jpetazzo I'm not to worried about removing and re-adding the rules so that we know they are correct but I agree that iptables-save sounds like the best solution if you are running a version of iptables that does not have --check.

vieux · 2014-09-03T01:21:50Z

@jfrazelle @crosbymichael @jpetazzo what's the status of this ?

jessfraz · 2014-09-03T01:23:41Z

I'll fix what I have to use iptables-save.
On Sep 2, 2014 6:22 PM, "Victor Vieux" notifications@github.com wrote:

@jfrazelle https://github.com/jfrazelle @crosbymichael
https://github.com/crosbymichael @jpetazzo https://github.com/jpetazzo
what's the status of this ?

—
Reply to this email directly or view it on GitHub
#6950 (comment).

jessfraz · 2014-09-03T16:29:20Z

@crosbymichael @jpetazzo let me know if this is what you had in mind

vbatts · 2014-09-03T17:54:41Z

@jfrazelle this works on RHEL6 (iptables-1.4.7-11) and preserves order. Only thing I'm noticing that is still duplicated on docker daemon restart is the rule -A POSTROUTING -s 172.17.0.0/16 ! -o docker0 -j MASQUERADE

jessfraz · 2014-09-03T18:59:10Z

oh right I see the problem, will fix

jessfraz · 2014-09-03T19:38:06Z

fixed, problem with the ip passed not matching exactly the one given
I will admin regexes are my kryptonite, so if there is a better way to do that I am very open to changing it

thaJeztah · 2014-09-03T22:23:56Z

pkg/iptables/iptables.go

Perhaps [0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3}\\/[0-9]{1,2} (Net mask is 1 or 2 digits?)

A bit more restrictive on the numer of digits, but will allow 999.999.999.999 so not 100% correct

/+ before the net mask in your Regex seems wrong (I think that allows multiple /, but haven't tested)

That works and I swapped it out, I was also thinking along the lines of if there was a way to check if the string contains the substring via regex all in one swoop. Because this feels a bit nasty.

erikh · 2014-09-04T00:29:39Z

pkg/iptables/iptables.go

probably not necessary, but [1-9][0-9]{0,2} might be more appropriate for octets

jessfraz · 2014-09-04T15:51:14Z

ping @vbatts

vbatts · 2014-09-04T18:45:16Z

The duplication of that rule is gone now.
LGTM

jessfraz · 2014-09-04T18:47:07Z

\o/

On Thu, Sep 4, 2014 at 11:45 AM, Vincent Batts notifications@github.com
wrote:

The duplication of that rule is gone now.
LGTM

—
Reply to this email directly or view it on GitHub
#6950 (comment).

vieux · 2014-09-04T19:12:35Z

LGTM ping @jpetazzo

If iptables version is < 1.4.11, try to delete the rule vs. checking if it exists. Fixes moby#6831. Docker-DCO-1.1-Signed-off-by: Jessica Frazelle <jfrazelle@users.noreply.github.com> (github: jfrazelle)

jessfraz · 2014-09-07T22:13:41Z

i just rebased, I think we just need an ok from @jpetazzo

jessfraz · 2014-09-15T22:06:10Z

ping @jpetazzo who is now back from out of town, sorry to be such a nag, this one has just been around awhile

jpetazzo · 2014-09-19T00:30:57Z

:boratveryverynice:

(Ack, that emoji doesn't exist, shh!)

LGTM

jessfraz · 2014-09-19T00:32:21Z

yayyy!

crosbymichael · 2014-09-19T00:50:55Z

LGTM

vbatts · 2014-09-19T00:51:02Z

:squirrel:

Fix duplicate iptables rules

vbatts mentioned this pull request Jul 11, 2014

perhaps docker daemon should check for pid _first_ #6973

Closed

crosbymichael reviewed Jul 11, 2014
View reviewed changes

erikh added the ICC label Jul 17, 2014

jessfraz force-pushed the 6831-check-flag-centos6.5 branch 2 times, most recently from ebf5179 to cd2f653 Compare September 3, 2014 16:28

jessfraz force-pushed the 6831-check-flag-centos6.5 branch from cd2f653 to 4f50943 Compare September 3, 2014 19:36

thaJeztah reviewed Sep 3, 2014
View reviewed changes

jessfraz force-pushed the 6831-check-flag-centos6.5 branch from 4f50943 to 812fb7a Compare September 4, 2014 00:11

erikh reviewed Sep 4, 2014
View reviewed changes

jessfraz force-pushed the 6831-check-flag-centos6.5 branch 2 times, most recently from 216a2ea to a3d8291 Compare September 4, 2014 00:54

Fix duplicate iptables rules

f3a68ff

If iptables version is < 1.4.11, try to delete the rule vs. checking if it exists. Fixes moby#6831. Docker-DCO-1.1-Signed-off-by: Jessica Frazelle <jfrazelle@users.noreply.github.com> (github: jfrazelle)

jessfraz force-pushed the 6831-check-flag-centos6.5 branch from a3d8291 to f3a68ff Compare September 7, 2014 22:12

crosbymichael added a commit that referenced this pull request Sep 19, 2014

Merge pull request #6950 from jfrazelle/6831-check-flag-centos6.5

d5537e0

Fix duplicate iptables rules

crosbymichael merged commit d5537e0 into moby:master Sep 19, 2014

jessfraz deleted the 6831-check-flag-centos6.5 branch October 10, 2014 03:15

tyrannasaurusbanks mentioned this pull request Nov 3, 2014

Proxy (iptables v1.4.7) is failing over on CentOS 6.5 kubernetes/kubernetes#2134

Closed

thaJeztah mentioned this pull request Apr 16, 2015

Removal of the regex to replace ips #12437

Merged

thaJeztah mentioned this pull request Jun 10, 2015

[1.7.0-rc2] Connection reset from published ports #13847

Closed

thaJeztah mentioned this pull request Nov 10, 2022

Comment docker-managed iptables rules #43896

Open

Conversation

jessfraz commented Jul 10, 2014

Uh oh!

jessfraz commented Jul 10, 2014

Uh oh!

vbatts commented Jul 10, 2014

Uh oh!

vieux commented Jul 10, 2014

Uh oh!

jessfraz commented Jul 10, 2014

Uh oh!

vbatts commented Jul 11, 2014

Uh oh!

jpetazzo commented Jul 11, 2014

Uh oh!

crosbymichael Jul 11, 2014

Choose a reason for hiding this comment

Uh oh!

crosbymichael commented Jul 22, 2014

Uh oh!

vieux commented Sep 3, 2014

Uh oh!

jessfraz commented Sep 3, 2014

Uh oh!

jessfraz commented Sep 3, 2014

Uh oh!

vbatts commented Sep 3, 2014

Uh oh!

jessfraz commented Sep 3, 2014

Uh oh!

jessfraz commented Sep 3, 2014

Uh oh!

thaJeztah Sep 3, 2014

Choose a reason for hiding this comment

Uh oh!

jessfraz Sep 4, 2014

Choose a reason for hiding this comment

Uh oh!

erikh Sep 4, 2014

Choose a reason for hiding this comment

Uh oh!

erikh Sep 4, 2014

Choose a reason for hiding this comment

Uh oh!

jessfraz commented Sep 4, 2014

Uh oh!

vbatts commented Sep 4, 2014

Uh oh!

jessfraz commented Sep 4, 2014

Uh oh!

vieux commented Sep 4, 2014

Uh oh!

jessfraz commented Sep 7, 2014

Uh oh!

jessfraz commented Sep 15, 2014

Uh oh!

jpetazzo commented Sep 19, 2014

Uh oh!

jessfraz commented Sep 19, 2014

Uh oh!

crosbymichael commented Sep 19, 2014

Uh oh!

vbatts commented Sep 19, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants