@LK4D4 maybe this is what you were seeing?

@crosbymichael Nope :) But this stuff is bad too.

I was going to LGTM this earlier as it makes sense to me, but I'm curious if anyone with more background than me can explain why the replacement with "?" was there in the first place? The comment is too cryptic to fully "get", for me at least. @LK4D4

@estesp Have no idea. Always thought it is stupid.

Was added to close #6831 and added in #6950

I think the problem was that when setting the rules, the existing rules didn't fully match the format, therefore weren't detected, resulting in duplicate rules

The regexp replaces the IP-address (which is easier to find) with a random character ("?"), to check if the rule exists. Basically, it's a "fuzzy" matching

So, not sure this PR is safe; it may cause a regression and re-introduce #6831

/cc @jfrazelle to confirm if my analysis of the original change was correct 😄 (it's quite a while ago)

@thaJeztah yeah, just read through the issue and PR--thanks for doing the legwork. I guess it depends if the initial issue is resolved with distros that have moved up to more recent iptables? Or, given that this PR is fixing a bad problem (containers can't get "outside" if docker0 IP changes), is there a better way to solve the original problem?

@estesp thinking...

What I don't understand in this PR, is;

Because the func Exists(table Table, chain string, rule ...string) bool _return true_. This is caused by that ip is replaced by "?" .

I would expect that because of this, no new rule will be added?

(This is still a problem because the existing rule doesn't match the new IP address)

A possible solution will require knowing the IP-address before and after changing, so that the IP-address in the existing rule can be replaced (using the same regexp)

I would expect that because of this, no new rule will be added?

Correct, that's is what is fixed by this PR.. in current form, the code will say "yes, this rule exists", even though it doesn't exist for the current IP of docker0.

I'm not sure how we can access "the old IP" other than assuming that no one else but Docker daemon is inserting rules for MASQUERADE on docker0 and that you would only need one per bridge that matches the current IP.

wow yeah that was my first PR ever btw. but I do believe if CentOS still ships with that version of iptables it does still fix their issue. So we still need something like that. I do not think this is the fix we need

also if i am reading this correctly this only occurs if you mess with the bridge after docker has set it up, which really isnt recommended

why not use --bridge-ip

@jfrazelle No, you can run docker on one bridge, then stop, then run it on other bridge - rule will be wrong. And -bip won't work too I think.

I'm not sure how we can access "the old IP"

possible ideas (two hacky ones);

• Remove the rule if the daemon shuts down, and re-create when starting (tricky)
• Replace the IP with a placeholder on shutdown (e.g. 123.456.789.012) and propagate on start (dirty)
• Store the IP in a config file (Migrate .dockercfg to .docker/config.json and support for HTTP Headers #12009) (cleanest)

I'm not sure why we should care about old IP. We should try to remove it on shutdown, if it's failed - well, not worst thing in the world.

ah ok gotcha

we should probably cleanup the rules on shutdown

But this check with ? is bad too. If rule wasn't deleted - it'll prevent your docker from working until you delete it by hands.

I don't think the "?" trick on itself is bad, it depends what you do with it;

If, on startup, an existing rule is found; remove it and add the new rule? (Or won't that work?)

I don't understand what ? doing at all. Yes it will work, but not with ?.

Thinking of this, why not add a comment (--comment) to the rules added by docker? Comments can be used to identify which rules where added by docker, so should make it easy to find and/or replace them?

@thaJeztah Yup, if it's supported by all versions of iptables. Then I personally think that we should recreate all rules on start.

if it's supported by all versions of iptables.

Good point; looks like the comment extension was added in version 1.3; http://git.netfilter.org/iptables/commit/?id=514b1b488eaf07d66e209681f4f34246d7db2f60

Not sure if there are distros that don't ship all extensions?

So, not sure this PR is safe; it may cause a regression and re-introduce #6831

This PR does cause multiple MASQUERADE rules. But, these rules have different IPs. It doesn't cause completely duplicate rules. The func Exist just check whether the rules already exist, It does not care whether there are rules useless，i think.

Indeed, we should cleanup the rules on shutdown. However, we still need to check whether the rules already exist before Docker create them.

I think the correct solution is that - If here exist a MASQUERADE rule at the Docker bridge (docker0 or other), We extract the IP as OldIP, if the OldIP range include the new bridge IP, it return true, else false.
This is same as the iptables -C

OldIP: 172.17.0.0/16    include    NewIP: 172.17.100.0/24          return true.   

@fmzhen it would be awesome if we had some tests that cover the bad behavior and how this PR solves it. That would help other maintainers to understand the intention of the change.

@fmzhen Would you mind to add test? There is some similar tests already, which playing with docker0 and stuff.
TestDaemonBridgeFixedCidr for example

@LK4D4 How about the test TestDaemonRestartWithBridgeIPChange, which i added in the integration-cli/docker_cli_daemon_test.go. Is that what you want? If it needs to be changed, please tell me . I am a novice.

@LK4D4

Ok, you should use c.Assert next time in tests.
LGTM
ping @docker/core-maintainers

This is going to need backporting to libnetwork (cc @mrjana)

LGTM

I think this PR results in duplicate POSTROUTING MASQUERADE rules, see moby/libnetwork#375