libnetwork: fix resolver restore w/ chatty 'iptables -C'#45657
Merged
corhere merged 1 commit intomoby:masterfrom May 30, 2023
Merged
libnetwork: fix resolver restore w/ chatty 'iptables -C'#45657corhere merged 1 commit intomoby:masterfrom
corhere merged 1 commit intomoby:masterfrom
Conversation
Resolver.setupIPTable() checks whether it needs to flush or create the user chains used for NATing container DNS requests by testing for the existence of the rules which jump to said user chains. Unfortunately it does so using the IPTable.RawCombinedOutputNative() method, which returns a non-nil error if the iptables command returns any output even if the command exits with a zero status code. While that is fine with iptables-legacy as it prints no output if the rule exists, iptables-nft v1.8.7 prints some information about the rule. Consequently, Resolver.setupIPTable() would incorrectly think that the rule does not exist during container restore and attempt to create it. This happened work work by coincidence before 8f5a9a7 because the failure to create the already-existing table would be ignored and the new NAT rules would be inserted before the stale rules left in the table from when the container was last started/restored. Now that failing to create the table is treated as a fatal error, the incompatibility with iptables-nft is no longer hidden. Switch to using IPTable.ExistsNative() to test for the existence of the jump rules as it correctly only checks the iptables command's exit status without regard for whether it outputs anything. Signed-off-by: Cory Snider <csnider@mirantis.com>
neersighted
approved these changes
May 30, 2023
Member
neersighted
left a comment
There was a problem hiding this comment.
LGTM; I wonder if there are other instances of this (anti-)pattern we need to look out for?
Contributor
Author
I already checked. Aside from the implementation of $ rg -F '"-C"' libnetwork
libnetwork/iptables/iptables.go
465: _, err := f(append([]string{"-t", string(table), "-C", chain}, rule...)...)
libnetwork/resolver_unix.go
40: err := iptable.RawCombinedOutputNative("-t", "nat", "-C", "OUTPUT", "-d", resolverIP, "-j", outputChain)
57: err = iptable.RawCombinedOutputNative("-t", "nat", "-C", "POSTROUTING", "-d", resolverIP, "-j", postroutingChain) |
akerouanton
approved these changes
May 30, 2023
Member
akerouanton
left a comment
There was a problem hiding this comment.
This reminds me that these *Native methods are another potential foot-gun that we need to get rid of.
LGTM
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Resolver.setupIPTable()checks whether it needs to flush or create the user chains used for NATing container DNS requests by testing for the existence of the rules which jump to said user chains. Unfortunately it does so using theIPTable.RawCombinedOutputNative()method, which returns a non-nil error if the iptables command returns any output even if the command exits with a zero status code. While that is fine with iptables-legacy as it prints no output if the rule exists, iptables-nft v1.8.7 prints some information about the rule. Consequently,Resolver.setupIPTable()would incorrectly think that the rule does not exist during container restore and attempt to create it. This happened work work by coincidence before 8f5a9a7 because the failure to create the already-existing table would be ignored and the new NAT rules would be inserted before the stale rules left in the table from when the container was last started/restored. Now that failing to create the table is treated as a fatal error, the incompatibility with iptables-nft is no longer hidden.Switch to using
IPTable.ExistsNative()to test for the existence of the jump rules as it correctly only checks the iptables command's exit status without regard for whether it outputs anything.- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)