libnetwork: Remove iptables nat rule when hairpin is disabled #44803
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
akerouanton
force-pushed
the
fix-44721
branch
from
c61e5d2
to
3a70e5a
Compare
When userland-proxy is turned off and on again, the iptables nat rule doing hairpinning isn't properly removed. This fix makes sure this nat rule is removed whenever the bridge is torn down or hairpinning is disabled (through setting userland-proxy to true). Unlike for ip masquerading and ICC, the `programChainRule()` call setting up the "MASQ LOCAL HOST" rule has to be called unconditionally because the hairpin parameter isn't restored from the driver store, but always comes from the driver config. For the "SKIP DNAT" rule, things are a bit different: this rule is always deleted by `removeIPChains()` when the bridge driver is initialized. Fixes moby#44721. Signed-off-by: Albin Kerouanton <albinker@gmail.com>
akerouanton
force-pushed
the
fix-44721
branch
from
3a70e5a
to
0f872b7
Compare
neersighted
requested changes
Jan 11, 2023
There was a problem hiding this comment.
Overall LGTM. This is definitely messy, but not as bad as I feared in #44721 (comment), and the one-line change makes it a lot more palatable.
Overall I think this is the lesser evil until the rules logic gets cleaned up in its entirety.
neersighted
added
status/2-code-review
area/networking
kind/bugfix
|
Eyeing this for 23.0.y; I think it's fine to omit from 20.10. |
akerouanton
force-pushed
the
fix-44721
branch
from
0f872b7
to
566a2e4
Compare
neersighted
approved these changes
Jan 12, 2023
thaJeztah
approved these changes
Jan 12, 2023
corhere
approved these changes
Jan 12, 2023
|
Test failure is known flake/unrelated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When userland-proxy is turned off and on again, the iptables nat rule
doing hairpinning isn't properly removed. This fix makes sure this nat
rule is removed whenever the bridge is torn down or hairpinning is
disabled (through setting userland-proxy to true).
Unlike for ip masquerading and ICC, the
programChainRule()callsetting up the "MASQ LOCAL HOST" rule has to be called unconditionally
because the hairpin parameter isn't restored from the driver store, but
always comes from the driver config.
For the "SKIP DNAT" rule, things are a bit different: this rule is
always deleted by
removeIPChains()when the bridge driver isinitialized.
Fixes #44721.
Side note: I'm not super happy with this fix. It does its job but there're now 3 ways iptables rules created by
setupIPTablesInternalare torn down. There might be a better way to do that, but this probably won't be possible until libnetwork got cleaned up and iptables rules are managed in a better way.- How to verify it
Turning
userland-proxyoff and on again, and then runningiptables -t nat -nvL. There should be no rule looking like-A POSTROUTING -o docker0 -m addrtype --src-type LOCAL -j MASQUERADE.