From 1b27ab4c737d057e73fc5efecb8ba17a0fb227a3 Mon Sep 17 00:00:00 2001 From: Chee Hau Lim Date: Wed, 16 Mar 2022 04:54:51 +0800 Subject: [PATCH 1/4] libnetwork/iptables: Fix test panic when execute only one test - use local variables for chains instead of sharing global variables - make createNewChain a t.Helper Signed-off-by: Chee Hau Lim (cherry picked from commit a2cea992c224b5de5f82a0ff24e93b5b09a63987) Signed-off-by: Sebastiaan van Stijn --- libnetwork/iptables/iptables_test.go | 53 +++++++++++++++------------- 1 file changed, 28 insertions(+), 25 deletions(-) diff --git a/libnetwork/iptables/iptables_test.go b/libnetwork/iptables/iptables_test.go index d46e5153cc..1791a7acfb 100644 --- a/libnetwork/iptables/iptables_test.go +++ b/libnetwork/iptables/iptables_test.go @@ -13,19 +13,16 @@ import ( "golang.org/x/sync/errgroup" ) -const chainName = "DOCKEREST" - -var natChain *ChainInfo -var filterChain *ChainInfo -var bridgeName string - -func TestNewChain(t *testing.T) { - var err error - +const ( + chainName = "DOCKEREST" bridgeName = "lo" +) + +func createNewChain(t *testing.T) (*IPTable, *ChainInfo, *ChainInfo) { + t.Helper() iptable := GetIptable(IPv4) - natChain, err = iptable.NewChain(chainName, Nat, false) + natChain, err := iptable.NewChain(chainName, Nat, false) if err != nil { t.Fatal(err) } @@ -34,7 +31,7 @@ func TestNewChain(t *testing.T) { t.Fatal(err) } - filterChain, err = iptable.NewChain(chainName, Filter, false) + filterChain, err := iptable.NewChain(chainName, Filter, false) if err != nil { t.Fatal(err) } @@ -42,18 +39,23 @@ func TestNewChain(t *testing.T) { if err != nil { t.Fatal(err) } + + return iptable, natChain, filterChain +} + +func TestNewChain(t *testing.T) { + createNewChain(t) } func TestForward(t *testing.T) { + iptable, natChain, filterChain := createNewChain(t) + ip := net.ParseIP("192.168.1.1") port := 1234 dstAddr := "172.17.0.1" dstPort := 4321 proto := "tcp" - bridgeName := "lo" - iptable := GetIptable(IPv4) - err := natChain.Forward(Insert, ip, port, proto, dstAddr, dstPort, bridgeName) if err != nil { t.Fatal(err) @@ -99,16 +101,13 @@ func TestForward(t *testing.T) { } func TestLink(t *testing.T) { - var err error - - bridgeName := "lo" - iptable := GetIptable(IPv4) + iptable, _, filterChain := createNewChain(t) ip1 := net.ParseIP("192.168.1.1") ip2 := net.ParseIP("192.168.1.2") port := 1234 proto := "tcp" - err = filterChain.Link(Append, ip1, ip2, port, proto, bridgeName) + err := filterChain.Link(Append, ip1, ip2, port, proto, bridgeName) if err != nil { t.Fatal(err) } @@ -141,10 +140,11 @@ func TestLink(t *testing.T) { } func TestPrerouting(t *testing.T) { + iptable, natChain, _ := createNewChain(t) + args := []string{ "-i", "lo", "-d", "192.168.1.1"} - iptable := GetIptable(IPv4) err := natChain.Prerouting(Insert, args...) if err != nil { @@ -162,10 +162,11 @@ func TestPrerouting(t *testing.T) { } func TestOutput(t *testing.T) { + iptable, natChain, _ := createNewChain(t) + args := []string{ "-o", "lo", "-d", "192.168.1.1"} - iptable := GetIptable(IPv4) err := natChain.Output(Insert, args...) if err != nil { @@ -196,6 +197,8 @@ func TestConcurrencyNoWait(t *testing.T) { // Note that if iptables does not support the xtable lock on this // system, then allowXlock has no effect -- it will always be off. func RunConcurrencyTest(t *testing.T, allowXlock bool) { + _, natChain, _ := createNewChain(t) + if !allowXlock && supportsXlock { supportsXlock = false defer func() { supportsXlock = true }() @@ -219,7 +222,8 @@ func RunConcurrencyTest(t *testing.T, allowXlock bool) { } func TestCleanup(t *testing.T) { - var err error + iptable, _, filterChain := createNewChain(t) + var rules []byte // Cleanup filter/FORWARD first otherwise output of iptables-save is dirty @@ -227,14 +231,13 @@ func TestCleanup(t *testing.T) { string(Delete), "FORWARD", "-o", bridgeName, "-j", filterChain.Name} - iptable := GetIptable(IPv4) - if _, err = iptable.Raw(link...); err != nil { + if _, err := iptable.Raw(link...); err != nil { t.Fatal(err) } filterChain.Remove() - err = iptable.RemoveExistingChain(chainName, Nat) + err := iptable.RemoveExistingChain(chainName, Nat) if err != nil { t.Fatal(err) } From 2bf66f725c8bd474a9510f2ec805db0b28c25058 Mon Sep 17 00:00:00 2001 From: Albin Kerouanton Date: Mon, 6 Dec 2021 01:03:14 +0100 Subject: [PATCH 2/4] Check ipt options before looking for ip6t iptables package has a function `detectIptables()` called to initialize some local variables. Since v20.10.0, it first looks for iptables bin, then ip6tables and finally it checks what iptables flags are available (including -C). It early exits when ip6tables isn't available, and doesn't execute the last check. To remove port mappings (eg. when a container stops/dies), Docker first checks if those NAT rules exist and then deletes them. However, in the particular case where there's no ip6tables bin available, iptables `-C` flag is considered unavailable and thus it looks for NAT rules by using some substring matching. This substring matching then fails because `iptables -t nat -S POSTROUTING` dumps rules in a slighly format than what's expected. For instance, here's what `iptables -t nat -S POSTROUTING` dumps: ``` -A POSTROUTING -s 172.18.0.2/32 -d 172.18.0.2/32 -p tcp -m tcp --dport 9999 -j MASQUERADE ``` And here's what Docker looks for: ``` POSTROUTING -p tcp -s 172.18.0.2 -d 172.18.0.2 --dport 9999 -j MASQUERADE ``` Because of that, those rules are considered non-existant by Docker and thus never deleted. To fix that, this change reorders the code in `detectIptables()`. Fixes #42127. Signed-off-by: Albin Kerouanton (cherry picked from commit af7236f85a3477b37518aed7eb8dbc8e2b595d59) Signed-off-by: Sebastiaan van Stijn --- libnetwork/iptables/iptables.go | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/libnetwork/iptables/iptables.go b/libnetwork/iptables/iptables.go index 3fc70c9f6c..03c10745fd 100644 --- a/libnetwork/iptables/iptables.go +++ b/libnetwork/iptables/iptables.go @@ -123,11 +123,7 @@ func detectIptables() { return } iptablesPath = path - path, err = exec.LookPath("ip6tables") - if err != nil { - return - } - ip6tablesPath = path + supportsXlock = exec.Command(iptablesPath, "--wait", "-L", "-n").Run() == nil mj, mn, mc, err := GetVersion() if err != nil { @@ -135,6 +131,13 @@ func detectIptables() { return } supportsCOpt = supportsCOption(mj, mn, mc) + + path, err = exec.LookPath("ip6tables") + if err != nil { + return + } else { + ip6tablesPath = path + } } func initDependencies() { From 91f2d963c68d0292f15d683aae95b09315f3b28d Mon Sep 17 00:00:00 2001 From: Albin Kerouanton Date: Mon, 6 Dec 2021 01:32:42 +0100 Subject: [PATCH 3/4] Merge iptables.probe() into iptables.detectIptables() The former was doing some checks and logging warnings, whereas the latter was doing the same checks but to set some internal variables. As both are called only once and from the same place, there're now merged together. Signed-off-by: Albin Kerouanton (cherry picked from commit 205e5278c6c17aa306dd8d565b29b8263005958b) Signed-off-by: Sebastiaan van Stijn --- libnetwork/iptables/iptables.go | 56 ++++++++++++++------------------- 1 file changed, 24 insertions(+), 32 deletions(-) diff --git a/libnetwork/iptables/iptables.go b/libnetwork/iptables/iptables.go index 03c10745fd..a74ac572fb 100644 --- a/libnetwork/iptables/iptables.go +++ b/libnetwork/iptables/iptables.go @@ -89,19 +89,32 @@ func (e ChainError) Error() string { return fmt.Sprintf("Error iptables %s: %s", e.Chain, string(e.Output)) } -func probe() { +func detectIptables() { path, err := exec.LookPath("iptables") if err != nil { - logrus.Warnf("Failed to find iptables: %v", err) + logrus.WithError(err).Warnf("failed to find iptables") return } - if out, err := exec.Command(path, "--wait", "-t", "nat", "-L", "-n").CombinedOutput(); err != nil { - logrus.Warnf("Running iptables --wait -t nat -L -n failed with message: `%s`, error: %v", strings.TrimSpace(string(out)), err) + iptablesPath = path + + if out, err := exec.Command(path, "--wait", "-L", "-n").CombinedOutput(); err != nil { + logrus.WithError(err).Infof("unable to detect if iptables supports xlock: 'iptables --wait -L -n': `%s`", strings.TrimSpace(string(out))) + } else { + supportsXlock = true } - _, err = exec.LookPath("ip6tables") + + mj, mn, mc, err := GetVersion() if err != nil { - logrus.Warnf("Failed to find ip6tables: %v", err) - return + logrus.Warnf("Failed to read iptables version: %v", err) + } else { + supportsCOpt = supportsCOption(mj, mn, mc) + } + + path, err = exec.LookPath("ip6tables") + if err != nil { + logrus.WithError(err).Warnf("unable to find ip6tables") + } else { + ip6tablesPath = path } } @@ -113,35 +126,11 @@ func initFirewalld() { return } if err := FirewalldInit(); err != nil { - logrus.Debugf("Fail to initialize firewalld: %v, using raw iptables instead", err) - } -} - -func detectIptables() { - path, err := exec.LookPath("iptables") - if err != nil { - return - } - iptablesPath = path - - supportsXlock = exec.Command(iptablesPath, "--wait", "-L", "-n").Run() == nil - mj, mn, mc, err := GetVersion() - if err != nil { - logrus.Warnf("Failed to read iptables version: %v", err) - return - } - supportsCOpt = supportsCOption(mj, mn, mc) - - path, err = exec.LookPath("ip6tables") - if err != nil { - return - } else { - ip6tablesPath = path + logrus.WithError(err).Debugf("unable to initialize firewalld; using raw iptables instead") } } func initDependencies() { - probe() initFirewalld() detectIptables() } @@ -554,6 +543,9 @@ func (iptable IPTable) raw(args ...string) ([]byte, error) { path := iptablesPath commandName := "iptables" if iptable.Version == IPv6 { + if ip6tablesPath == "" { + return nil, fmt.Errorf("ip6tables is missing") + } path = ip6tablesPath commandName = "ip6tables" } From 6a0a2c4f79fb1813ccdbd39fc69d788835615033 Mon Sep 17 00:00:00 2001 From: Albin Kerouanton Date: Mon, 6 Dec 2021 02:23:42 +0100 Subject: [PATCH 4/4] Always use iptables -C to look for rules iptables -C flag was introduced in v1.4.11, which was released ten years ago. Thus, there're no more Linux distributions supported by Docker using this version. As such, this commit removes the old way of checking if an iptables rule exists (by using substring matching). Signed-off-by: Albin Kerouanton (cherry picked from commit 799cc143c929ebc32a3ac0018c6b615801d16e0a) Signed-off-by: Sebastiaan van Stijn --- libnetwork/iptables/iptables.go | 56 +++------------------------- libnetwork/iptables/iptables_test.go | 33 +--------------- 2 files changed, 7 insertions(+), 82 deletions(-) diff --git a/libnetwork/iptables/iptables.go b/libnetwork/iptables/iptables.go index a74ac572fb..1eb15e18c1 100644 --- a/libnetwork/iptables/iptables.go +++ b/libnetwork/iptables/iptables.go @@ -8,7 +8,6 @@ import ( "fmt" "net" "os/exec" - "regexp" "strconv" "strings" "sync" @@ -57,7 +56,6 @@ var ( iptablesPath string ip6tablesPath string supportsXlock = false - supportsCOpt = false xLockWaitMsg = "Another app is currently holding the xtables lock" // used to lock iptables commands if xtables lock is not supported bestEffortLock sync.Mutex @@ -97,19 +95,14 @@ func detectIptables() { } iptablesPath = path + // The --wait flag was added in iptables v1.6.0. + // TODO remove this check once we drop support for CentOS/RHEL 7, which uses an older version of iptables if out, err := exec.Command(path, "--wait", "-L", "-n").CombinedOutput(); err != nil { logrus.WithError(err).Infof("unable to detect if iptables supports xlock: 'iptables --wait -L -n': `%s`", strings.TrimSpace(string(out))) } else { supportsXlock = true } - mj, mn, mc, err := GetVersion() - if err != nil { - logrus.Warnf("Failed to read iptables version: %v", err) - } else { - supportsCOpt = supportsCOption(mj, mn, mc) - } - path, err = exec.LookPath("ip6tables") if err != nil { logrus.WithError(err).Warnf("unable to find ip6tables") @@ -470,26 +463,9 @@ func (iptable IPTable) exists(native bool, table Table, chain string, rule ...st return false } - if supportsCOpt { - // if exit status is 0 then return true, the rule exists - _, err := f(append([]string{"-t", string(table), "-C", chain}, rule...)...) - return err == nil - } - - // parse "iptables -S" for the rule (it checks rules in a specific chain - // in a specific table and it is very unreliable) - return iptable.existsRaw(table, chain, rule...) -} - -func (iptable IPTable) existsRaw(table Table, chain string, rule ...string) bool { - path := iptablesPath - if iptable.Version == IPv6 { - path = ip6tablesPath - } - ruleString := fmt.Sprintf("%s %s\n", chain, strings.Join(rule, " ")) - existingRules, _ := exec.Command(path, "-t", string(table), "-S", chain).Output() - - return strings.Contains(string(existingRules), ruleString) + // if exit status is 0 then return true, the rule exists + _, err := f(append([]string{"-t", string(table), "-C", chain}, rule...)...) + return err == nil } // Maximum duration that an iptables operation can take @@ -587,15 +563,6 @@ func (iptable IPTable) ExistChain(chain string, table Table) bool { return false } -// GetVersion reads the iptables version numbers during initialization -func GetVersion() (major, minor, micro int, err error) { - out, err := exec.Command(iptablesPath, "--version").CombinedOutput() - if err == nil { - major, minor, micro = parseVersionNumbers(string(out)) - } - return -} - // SetDefaultPolicy sets the passed default policy for the table/chain func (iptable IPTable) SetDefaultPolicy(table Table, chain string, policy Policy) error { if err := iptable.RawCombinedOutput("-t", string(table), "-P", chain, string(policy)); err != nil { @@ -604,19 +571,6 @@ func (iptable IPTable) SetDefaultPolicy(table Table, chain string, policy Policy return nil } -func parseVersionNumbers(input string) (major, minor, micro int) { - re := regexp.MustCompile(`v\d*.\d*.\d*`) - line := re.FindString(input) - fmt.Sscanf(line, "v%d.%d.%d", &major, &minor, µ) - return -} - -// iptables -C, --check option was added in v.1.4.11 -// http://ftp.netfilter.org/pub/iptables/changes-iptables-1.4.11.txt -func supportsCOption(mj, mn, mc int) bool { - return mj > 1 || (mj == 1 && (mn > 4 || (mn == 4 && mc >= 11))) -} - // AddReturnRule adds a return rule for the chain in the filter table func (iptable IPTable) AddReturnRule(chain string) error { var ( diff --git a/libnetwork/iptables/iptables_test.go b/libnetwork/iptables/iptables_test.go index 1791a7acfb..decf948033 100644 --- a/libnetwork/iptables/iptables_test.go +++ b/libnetwork/iptables/iptables_test.go @@ -287,44 +287,15 @@ func TestExistsRaw(t *testing.T) { if err != nil { t.Fatalf("i=%d, err: %v", i, err) } - if !iptable.existsRaw(Filter, testChain1, r.rule...) { + if !iptable.exists(true, Filter, testChain1, r.rule...) { t.Fatalf("Failed to detect rule. i=%d", i) } // Truncate the rule trg := r.rule[len(r.rule)-1] trg = trg[:len(trg)-2] r.rule[len(r.rule)-1] = trg - if iptable.existsRaw(Filter, testChain1, r.rule...) { + if iptable.exists(true, Filter, testChain1, r.rule...) { t.Fatalf("Invalid detection. i=%d", i) } } } - -func TestGetVersion(t *testing.T) { - mj, mn, mc := parseVersionNumbers("iptables v1.4.19.1-alpha") - if mj != 1 || mn != 4 || mc != 19 { - t.Fatal("Failed to parse version numbers") - } -} - -func TestSupportsCOption(t *testing.T) { - input := []struct { - mj int - mn int - mc int - ok bool - }{ - {1, 4, 11, true}, - {1, 4, 12, true}, - {1, 5, 0, true}, - {0, 4, 11, false}, - {0, 5, 12, false}, - {1, 3, 12, false}, - {1, 4, 10, false}, - } - for ind, inp := range input { - if inp.ok != supportsCOption(inp.mj, inp.mn, inp.mc) { - t.Fatalf("Incorrect check: %d", ind) - } - } -}