From 8bced9b130342cba640b267625fbf9e6b2763f2a Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Tue, 16 Jul 2019 17:28:27 -0400 Subject: [PATCH 1/3] iptables: don't do feature detection on the iptables-restore binary The iptables code was doing version detection on the iptables binary but feature detection on the iptables-restore binary, to try to support the version of iptables in RHEL 7, which claims to be 1.4.21 but has certain features from iptables 1.6. The problem is that this particular set of versions and checks resulted in the code passing "-w" ("wait forever for the lock") to iptables, but "-w 5" ("wait at most 5 seconds for the lock") to iptables-restore. On systems with very very many iptables rules, this could result in the kubelet periodic resyncs (which use "iptables") blocking kube-proxy (which uses "iptables-restore") and causing it to time out. We already have code to grab the lock file by hand when using a version of iptables-restore that doesn't support "-w", and it works fine. So just use that instead, and only pass "-w 5" to iptables-restore when iptables reports a version that actually supports it. --- pkg/util/iptables/iptables.go | 52 ++--- pkg/util/iptables/iptables_test.go | 299 +++++++++++------------------ 2 files changed, 122 insertions(+), 229 deletions(-) diff --git a/pkg/util/iptables/iptables.go b/pkg/util/iptables/iptables.go index 761e55c94a1..3f2e460934c 100644 --- a/pkg/util/iptables/iptables.go +++ b/pkg/util/iptables/iptables.go @@ -126,6 +126,7 @@ const MinCheckVersion = "1.4.11" // Minimum iptables versions supporting the -w and -w flags const WaitMinVersion = "1.4.20" const WaitSecondsMinVersion = "1.4.22" +const WaitRestoreMinVersion = "1.6.2" const WaitString = "-w" const WaitSecondsValue = "5" @@ -167,7 +168,7 @@ func newInternal(exec utilexec.Interface, dbus utildbus.Interface, protocol Prot hasCheck: getIPTablesHasCheckCommand(vstring), hasListener: false, waitFlag: getIPTablesWaitFlag(vstring), - restoreWaitFlag: getIPTablesRestoreWaitFlag(exec, protocol), + restoreWaitFlag: getIPTablesRestoreWaitFlag(vstring), lockfilePath: lockfilePath, } return runner @@ -580,7 +581,6 @@ func getIPTablesWaitFlag(vstring string) []string { return []string{WaitString} } return []string{WaitString, WaitSecondsValue} - } // getIPTablesVersionString runs "iptables --version" to get the version string @@ -601,44 +601,22 @@ func getIPTablesVersionString(exec utilexec.Interface, protocol Protocol) (strin } // Checks if iptables-restore has a "wait" flag -// --wait support landed in v1.6.1+ right before --version support, so -// any version of iptables-restore that supports --version will also -// support --wait -func getIPTablesRestoreWaitFlag(exec utilexec.Interface, protocol Protocol) []string { - vstring, err := getIPTablesRestoreVersionString(exec, protocol) - if err != nil || vstring == "" { - klog.V(3).Infof("couldn't get iptables-restore version; assuming it doesn't support --wait") - return nil - } - if _, err := utilversion.ParseGeneric(vstring); err != nil { - klog.V(3).Infof("couldn't parse iptables-restore version; assuming it doesn't support --wait") - return nil - } - - return []string{WaitString, WaitSecondsValue} -} - -// getIPTablesRestoreVersionString runs "iptables-restore --version" to get the version string -// in the form "X.X.X" -func getIPTablesRestoreVersionString(exec utilexec.Interface, protocol Protocol) (string, error) { - // this doesn't access mutable state so we don't need to use the interface / runner - - // iptables-restore hasn't always had --version, and worse complains - // about unrecognized commands but doesn't exit when it gets them. - // Work around that by setting stdin to nothing so it exits immediately. - iptablesRestoreCmd := iptablesRestoreCommand(protocol) - cmd := exec.Command(iptablesRestoreCmd, "--version") - cmd.SetStdin(bytes.NewReader([]byte{})) - bytes, err := cmd.CombinedOutput() +func getIPTablesRestoreWaitFlag(vstring string) []string { + version, err := utilversion.ParseGeneric(vstring) if err != nil { - return "", err + klog.Errorf("vstring (%s) is not a valid version string: %v", vstring, err) + return nil } - versionMatcher := regexp.MustCompile("v([0-9]+(\\.[0-9]+)+)") - match := versionMatcher.FindStringSubmatch(string(bytes)) - if match == nil { - return "", fmt.Errorf("no iptables version found in string: %s", bytes) + + minVersion, err := utilversion.ParseGeneric(WaitRestoreMinVersion) + if err != nil { + klog.Errorf("WaitRestoreMinVersion (%s) is not a valid version string: %v", WaitRestoreMinVersion, err) + return nil } - return match[1], nil + if version.LessThan(minVersion) { + return nil + } + return []string{WaitString, WaitSecondsValue} } // goroutine to listen for D-Bus signals diff --git a/pkg/util/iptables/iptables_test.go b/pkg/util/iptables/iptables_test.go index dfc58095381..361adce72c0 100644 --- a/pkg/util/iptables/iptables_test.go +++ b/pkg/util/iptables/iptables_test.go @@ -46,15 +46,12 @@ func protocolStr(protocol Protocol) string { func testIPTablesVersionCmds(t *testing.T, protocol Protocol) { version := " v1.9.22" iptablesCmd := iptablesCommand(protocol) - iptablesRestoreCmd := iptablesRestoreCommand(protocol) protoStr := protocolStr(protocol) fcmd := fakeexec.FakeCmd{ CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{ // iptables version response (for runner instantiation) func() ([]byte, error) { return []byte(iptablesCmd + version), nil }, - // iptables-restore version response (for runner instantiation) - func() ([]byte, error) { return []byte(iptablesRestoreCmd + version), nil }, // iptables version response (for call to runner.GetVersion()) func() ([]byte, error) { return []byte(iptablesCmd + version), nil }, }, @@ -63,7 +60,6 @@ func testIPTablesVersionCmds(t *testing.T, protocol Protocol) { CommandScript: []fakeexec.FakeCommandAction{ func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } runner := New(&fexec, dbus.NewFake(nil, nil), protocol) @@ -74,18 +70,13 @@ func testIPTablesVersionCmds(t *testing.T, protocol Protocol) { t.Errorf("%s runner instantiate: Expected cmd '%s --version', Got '%s'", protoStr, iptablesCmd, fcmd.CombinedOutputLog[0]) } - // Check that proper iptables restore version command was used during runner instantiation - if !sets.NewString(fcmd.CombinedOutputLog[1]...).HasAll(iptablesRestoreCmd, "--version") { - t.Errorf("%s runner instantiate: Expected cmd '%s --version', Got '%s'", protoStr, iptablesRestoreCmd, fcmd.CombinedOutputLog[1]) - } - _, err := runner.GetVersion() if err != nil { t.Errorf("%s GetVersion: Expected success, got %v", protoStr, err) } // Check that proper iptables version command was used for runner.GetVersion - if !sets.NewString(fcmd.CombinedOutputLog[2]...).HasAll(iptablesCmd, "--version") { + if !sets.NewString(fcmd.CombinedOutputLog[1]...).HasAll(iptablesCmd, "--version") { t.Errorf("%s GetVersion: Expected cmd '%s --version', Got '%s'", protoStr, iptablesCmd, fcmd.CombinedOutputLog[2]) } } @@ -105,8 +96,6 @@ func testEnsureChain(t *testing.T, protocol Protocol) { CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{ // iptables version check func() ([]byte, error) { return []byte("iptables v1.9.22"), nil }, - // iptables-restore version check - func() ([]byte, error) { return []byte("iptables-restore v1.9.22"), nil }, // Success. func() ([]byte, error) { return []byte{}, nil }, // Exists. @@ -121,7 +110,6 @@ func testEnsureChain(t *testing.T, protocol Protocol) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } runner := New(&fexec, dbus.NewFake(nil, nil), protocol) @@ -134,11 +122,11 @@ func testEnsureChain(t *testing.T, protocol Protocol) { if exists { t.Errorf("%s new chain: Expected exists = false", protoStr) } - if fcmd.CombinedOutputCalls != 3 { - t.Errorf("%s new chain: Expected 3 CombinedOutput() calls, got %d", protoStr, fcmd.CombinedOutputCalls) + if fcmd.CombinedOutputCalls != 2 { + t.Errorf("%s new chain: Expected 2 CombinedOutput() calls, got %d", protoStr, fcmd.CombinedOutputCalls) } cmd := iptablesCommand(protocol) - if !sets.NewString(fcmd.CombinedOutputLog[2]...).HasAll(cmd, "-t", "nat", "-N", "FOOBAR") { + if !sets.NewString(fcmd.CombinedOutputLog[1]...).HasAll(cmd, "-t", "nat", "-N", "FOOBAR") { t.Errorf("%s new chain: Expected cmd containing '%s -t nat -N FOOBAR', got %s", protoStr, cmd, fcmd.CombinedOutputLog[2]) } // Exists. @@ -169,8 +157,6 @@ func TestFlushChain(t *testing.T) { CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{ // iptables version check func() ([]byte, error) { return []byte("iptables v1.9.22"), nil }, - // iptables-restore version check - func() ([]byte, error) { return []byte("iptables-restore v1.9.22"), nil }, // Success. func() ([]byte, error) { return []byte{}, nil }, // Failure. @@ -182,7 +168,6 @@ func TestFlushChain(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } runner := New(&fexec, dbus.NewFake(nil, nil), ProtocolIpv4) @@ -192,10 +177,10 @@ func TestFlushChain(t *testing.T) { if err != nil { t.Errorf("expected success, got %v", err) } - if fcmd.CombinedOutputCalls != 3 { - t.Errorf("expected 3 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) + if fcmd.CombinedOutputCalls != 2 { + t.Errorf("expected 2 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) } - if !sets.NewString(fcmd.CombinedOutputLog[2]...).HasAll("iptables", "-t", "nat", "-F", "FOOBAR") { + if !sets.NewString(fcmd.CombinedOutputLog[1]...).HasAll("iptables", "-t", "nat", "-F", "FOOBAR") { t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[2]) } // Failure. @@ -210,8 +195,6 @@ func TestDeleteChain(t *testing.T) { CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{ // iptables version check func() ([]byte, error) { return []byte("iptables v1.9.22"), nil }, - // iptables-restore version check - func() ([]byte, error) { return []byte("iptables-restore v1.9.22"), nil }, // Success. func() ([]byte, error) { return []byte{}, nil }, // Failure. @@ -223,7 +206,6 @@ func TestDeleteChain(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } runner := New(&fexec, dbus.NewFake(nil, nil), ProtocolIpv4) @@ -233,10 +215,10 @@ func TestDeleteChain(t *testing.T) { if err != nil { t.Errorf("expected success, got %v", err) } - if fcmd.CombinedOutputCalls != 3 { - t.Errorf("expected 3 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) + if fcmd.CombinedOutputCalls != 2 { + t.Errorf("expected 2 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) } - if !sets.NewString(fcmd.CombinedOutputLog[2]...).HasAll("iptables", "-t", "nat", "-X", "FOOBAR") { + if !sets.NewString(fcmd.CombinedOutputLog[1]...).HasAll("iptables", "-t", "nat", "-X", "FOOBAR") { t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[2]) } // Failure. @@ -251,8 +233,6 @@ func TestEnsureRuleAlreadyExists(t *testing.T) { CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{ // iptables version check func() ([]byte, error) { return []byte("iptables v1.9.22"), nil }, - // iptables-restore version check - func() ([]byte, error) { return []byte("iptables-restore v1.9.22"), nil }, // Success. func() ([]byte, error) { return []byte{}, nil }, }, @@ -261,8 +241,6 @@ func TestEnsureRuleAlreadyExists(t *testing.T) { CommandScript: []fakeexec.FakeCommandAction{ // iptables version check func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - // iptables-restore version check - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, // The second Command() call is checking the rule. Success of that exec means "done". func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, @@ -276,10 +254,10 @@ func TestEnsureRuleAlreadyExists(t *testing.T) { if !exists { t.Errorf("expected exists = true") } - if fcmd.CombinedOutputCalls != 3 { - t.Errorf("expected 3 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) + if fcmd.CombinedOutputCalls != 2 { + t.Errorf("expected 2 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) } - if !sets.NewString(fcmd.CombinedOutputLog[2]...).HasAll("iptables", "-t", "nat", "-C", "OUTPUT", "abc", "123") { + if !sets.NewString(fcmd.CombinedOutputLog[1]...).HasAll("iptables", "-t", "nat", "-C", "OUTPUT", "abc", "123") { t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[2]) } } @@ -289,8 +267,6 @@ func TestEnsureRuleNew(t *testing.T) { CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{ // iptables version check func() ([]byte, error) { return []byte("iptables v1.9.22"), nil }, - // iptables-restore version check - func() ([]byte, error) { return []byte("iptables-restore v1.9.22"), nil }, // Status 1 on the first call. func() ([]byte, error) { return nil, &fakeexec.FakeExitError{Status: 1} }, // Success on the second call. @@ -301,8 +277,6 @@ func TestEnsureRuleNew(t *testing.T) { CommandScript: []fakeexec.FakeCommandAction{ // iptables version check func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - // iptables-restore version check - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, // The second Command() call is checking the rule. Failure of that means create it. func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, @@ -317,10 +291,10 @@ func TestEnsureRuleNew(t *testing.T) { if exists { t.Errorf("expected exists = false") } - if fcmd.CombinedOutputCalls != 4 { - t.Errorf("expected 4 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) + if fcmd.CombinedOutputCalls != 3 { + t.Errorf("expected 3 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) } - if !sets.NewString(fcmd.CombinedOutputLog[3]...).HasAll("iptables", "-t", "nat", "-A", "OUTPUT", "abc", "123") { + if !sets.NewString(fcmd.CombinedOutputLog[2]...).HasAll("iptables", "-t", "nat", "-A", "OUTPUT", "abc", "123") { t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[3]) } } @@ -330,8 +304,6 @@ func TestEnsureRuleErrorChecking(t *testing.T) { CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{ // iptables version check func() ([]byte, error) { return []byte("iptables v1.9.22"), nil }, - // iptables-restore version check - func() ([]byte, error) { return []byte("iptables-restore v1.9.22"), nil }, // Status 2 on the first call. func() ([]byte, error) { return nil, &fakeexec.FakeExitError{Status: 2} }, }, @@ -340,10 +312,39 @@ func TestEnsureRuleErrorChecking(t *testing.T) { CommandScript: []fakeexec.FakeCommandAction{ // iptables version check func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - // iptables-restore version check + // The second Command() call is checking the rule. Failure of that means create it. + func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, + }, + } + runner := New(&fexec, dbus.NewFake(nil, nil), ProtocolIpv4) + defer runner.Destroy() + _, err := runner.EnsureRule(Append, TableNAT, ChainOutput, "abc", "123") + if err == nil { + t.Errorf("expected failure") + } + if fcmd.CombinedOutputCalls != 2 { + t.Errorf("expected 2 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) + } +} + +func TestEnsureRuleErrorCreating(t *testing.T) { + fcmd := fakeexec.FakeCmd{ + CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{ + // iptables version check + func() ([]byte, error) { return []byte("iptables v1.9.22"), nil }, + // Status 1 on the first call. + func() ([]byte, error) { return nil, &fakeexec.FakeExitError{Status: 1} }, + // Status 1 on the second call. + func() ([]byte, error) { return nil, &fakeexec.FakeExitError{Status: 1} }, + }, + } + fexec := fakeexec.FakeExec{ + CommandScript: []fakeexec.FakeCommandAction{ + // iptables version check func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, // The second Command() call is checking the rule. Failure of that means create it. func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, + func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } runner := New(&fexec, dbus.NewFake(nil, nil), ProtocolIpv4) @@ -357,48 +358,11 @@ func TestEnsureRuleErrorChecking(t *testing.T) { } } -func TestEnsureRuleErrorCreating(t *testing.T) { - fcmd := fakeexec.FakeCmd{ - CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{ - // iptables version check - func() ([]byte, error) { return []byte("iptables v1.9.22"), nil }, - // iptables-restore version check - func() ([]byte, error) { return []byte("iptables-restore v1.9.22"), nil }, - // Status 1 on the first call. - func() ([]byte, error) { return nil, &fakeexec.FakeExitError{Status: 1} }, - // Status 1 on the second call. - func() ([]byte, error) { return nil, &fakeexec.FakeExitError{Status: 1} }, - }, - } - fexec := fakeexec.FakeExec{ - CommandScript: []fakeexec.FakeCommandAction{ - // iptables version check - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - // iptables-restore version check - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - // The second Command() call is checking the rule. Failure of that means create it. - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - }, - } - runner := New(&fexec, dbus.NewFake(nil, nil), ProtocolIpv4) - defer runner.Destroy() - _, err := runner.EnsureRule(Append, TableNAT, ChainOutput, "abc", "123") - if err == nil { - t.Errorf("expected failure") - } - if fcmd.CombinedOutputCalls != 4 { - t.Errorf("expected 4 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) - } -} - func TestDeleteRuleDoesNotExist(t *testing.T) { fcmd := fakeexec.FakeCmd{ CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{ // iptables version check func() ([]byte, error) { return []byte("iptables v1.9.22"), nil }, - // iptables-restore version check - func() ([]byte, error) { return []byte("iptables-restore v1.9.22"), nil }, // Status 1 on the first call. func() ([]byte, error) { return nil, &fakeexec.FakeExitError{Status: 1} }, }, @@ -407,8 +371,6 @@ func TestDeleteRuleDoesNotExist(t *testing.T) { CommandScript: []fakeexec.FakeCommandAction{ // iptables version check func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - // iptables-restore version check - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, // The second Command() call is checking the rule. Failure of that exec means "does not exist". func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, @@ -419,10 +381,10 @@ func TestDeleteRuleDoesNotExist(t *testing.T) { if err != nil { t.Errorf("expected success, got %v", err) } - if fcmd.CombinedOutputCalls != 3 { - t.Errorf("expected 3 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) + if fcmd.CombinedOutputCalls != 2 { + t.Errorf("expected 2 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) } - if !sets.NewString(fcmd.CombinedOutputLog[2]...).HasAll("iptables", "-t", "nat", "-C", "OUTPUT", "abc", "123") { + if !sets.NewString(fcmd.CombinedOutputLog[1]...).HasAll("iptables", "-t", "nat", "-C", "OUTPUT", "abc", "123") { t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[2]) } } @@ -432,8 +394,6 @@ func TestDeleteRuleExists(t *testing.T) { CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{ // iptables version check func() ([]byte, error) { return []byte("iptables v1.9.22"), nil }, - // iptables-restore version check - func() ([]byte, error) { return []byte("iptables-restore v1.9.22"), nil }, // Success on the first call. func() ([]byte, error) { return []byte{}, nil }, // Success on the second call. @@ -444,8 +404,6 @@ func TestDeleteRuleExists(t *testing.T) { CommandScript: []fakeexec.FakeCommandAction{ // iptables version check func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - // iptables-restore version check - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, // The second Command() call is checking the rule. Success of that means delete it. func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, @@ -457,10 +415,10 @@ func TestDeleteRuleExists(t *testing.T) { if err != nil { t.Errorf("expected success, got %v", err) } - if fcmd.CombinedOutputCalls != 4 { - t.Errorf("expected 4 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) + if fcmd.CombinedOutputCalls != 3 { + t.Errorf("expected 3 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) } - if !sets.NewString(fcmd.CombinedOutputLog[3]...).HasAll("iptables", "-t", "nat", "-D", "OUTPUT", "abc", "123") { + if !sets.NewString(fcmd.CombinedOutputLog[2]...).HasAll("iptables", "-t", "nat", "-D", "OUTPUT", "abc", "123") { t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[3]) } } @@ -470,8 +428,6 @@ func TestDeleteRuleErrorChecking(t *testing.T) { CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{ // iptables version check func() ([]byte, error) { return []byte("iptables v1.9.22"), nil }, - // iptables-restore version check - func() ([]byte, error) { return []byte("iptables-restore v1.9.22"), nil }, // Status 2 on the first call. func() ([]byte, error) { return nil, &fakeexec.FakeExitError{Status: 2} }, }, @@ -480,8 +436,6 @@ func TestDeleteRuleErrorChecking(t *testing.T) { CommandScript: []fakeexec.FakeCommandAction{ // iptables version check func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - // iptables-restore version check - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, // The second Command() call is checking the rule. Failure of that means create it. func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, @@ -492,8 +446,8 @@ func TestDeleteRuleErrorChecking(t *testing.T) { if err == nil { t.Errorf("expected failure") } - if fcmd.CombinedOutputCalls != 3 { - t.Errorf("expected 3 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) + if fcmd.CombinedOutputCalls != 2 { + t.Errorf("expected 2 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) } } @@ -502,8 +456,6 @@ func TestDeleteRuleErrorDeleting(t *testing.T) { CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{ // iptables version check func() ([]byte, error) { return []byte("iptables v1.9.22"), nil }, - // iptables-restore version check - func() ([]byte, error) { return []byte("iptables-restore v1.9.22"), nil }, // Success on the first call. func() ([]byte, error) { return []byte{}, nil }, // Status 1 on the second call. @@ -514,8 +466,6 @@ func TestDeleteRuleErrorDeleting(t *testing.T) { CommandScript: []fakeexec.FakeCommandAction{ // iptables version check func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - // iptables-restore version check - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, // The second Command() call is checking the rule. Success of that means delete it. func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, @@ -527,8 +477,8 @@ func TestDeleteRuleErrorDeleting(t *testing.T) { if err == nil { t.Errorf("expected failure") } - if fcmd.CombinedOutputCalls != 4 { - t.Errorf("expected 4 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) + if fcmd.CombinedOutputCalls != 3 { + t.Errorf("expected 3 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) } } @@ -707,8 +657,6 @@ func TestWaitFlagUnavailable(t *testing.T) { CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{ // iptables version check func() ([]byte, error) { return []byte("iptables v1.4.19"), nil }, - // iptables-restore version check - func() ([]byte, error) { return []byte("iptables-restore v1.9.22"), nil }, // Success. func() ([]byte, error) { return []byte{}, nil }, }, @@ -717,8 +665,6 @@ func TestWaitFlagUnavailable(t *testing.T) { CommandScript: []fakeexec.FakeCommandAction{ // iptables version check func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - // iptables-restore version check - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } @@ -728,10 +674,10 @@ func TestWaitFlagUnavailable(t *testing.T) { if err != nil { t.Errorf("expected success, got %v", err) } - if fcmd.CombinedOutputCalls != 3 { - t.Errorf("expected 3 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) + if fcmd.CombinedOutputCalls != 2 { + t.Errorf("expected 2 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) } - if sets.NewString(fcmd.CombinedOutputLog[2]...).Has(WaitString) { + if sets.NewString(fcmd.CombinedOutputLog[1]...).Has(WaitString) { t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[2]) } } @@ -741,8 +687,6 @@ func TestWaitFlagOld(t *testing.T) { CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{ // iptables version check func() ([]byte, error) { return []byte("iptables v1.4.20"), nil }, - // iptables-restore version check - func() ([]byte, error) { return []byte("iptables-restore v1.9.22"), nil }, // Success. func() ([]byte, error) { return []byte{}, nil }, }, @@ -751,7 +695,6 @@ func TestWaitFlagOld(t *testing.T) { CommandScript: []fakeexec.FakeCommandAction{ func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } runner := New(&fexec, dbus.NewFake(nil, nil), ProtocolIpv4) @@ -760,14 +703,14 @@ func TestWaitFlagOld(t *testing.T) { if err != nil { t.Errorf("expected success, got %v", err) } - if fcmd.CombinedOutputCalls != 3 { - t.Errorf("expected 3 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) + if fcmd.CombinedOutputCalls != 2 { + t.Errorf("expected 2 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) } - if !sets.NewString(fcmd.CombinedOutputLog[2]...).HasAll("iptables", WaitString) { - t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[2]) + if !sets.NewString(fcmd.CombinedOutputLog[1]...).HasAll("iptables", WaitString) { + t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[1]) } - if sets.NewString(fcmd.CombinedOutputLog[2]...).Has(WaitSecondsValue) { - t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[2]) + if sets.NewString(fcmd.CombinedOutputLog[1]...).Has(WaitSecondsValue) { + t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[1]) } } @@ -776,8 +719,6 @@ func TestWaitFlagNew(t *testing.T) { CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{ // iptables version check func() ([]byte, error) { return []byte("iptables v1.4.22"), nil }, - // iptables-restore version check - func() ([]byte, error) { return []byte("iptables-restore v1.9.22"), nil }, // Success. func() ([]byte, error) { return []byte{}, nil }, }, @@ -786,7 +727,6 @@ func TestWaitFlagNew(t *testing.T) { CommandScript: []fakeexec.FakeCommandAction{ func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } runner := New(&fexec, dbus.NewFake(nil, nil), ProtocolIpv4) @@ -795,11 +735,11 @@ func TestWaitFlagNew(t *testing.T) { if err != nil { t.Errorf("expected success, got %v", err) } - if fcmd.CombinedOutputCalls != 3 { - t.Errorf("expected 3 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) + if fcmd.CombinedOutputCalls != 2 { + t.Errorf("expected 2 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) } - if !sets.NewString(fcmd.CombinedOutputLog[2]...).HasAll("iptables", WaitString, WaitSecondsValue) { - t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[2]) + if !sets.NewString(fcmd.CombinedOutputLog[1]...).HasAll("iptables", WaitString, WaitSecondsValue) { + t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[1]) } } @@ -815,8 +755,6 @@ func TestReload(t *testing.T) { CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{ // iptables version check func() ([]byte, error) { return []byte("iptables v1.4.22"), nil }, - // iptables-restore version check - func() ([]byte, error) { return []byte("iptables-restore v1.9.22"), nil }, // first reload // EnsureChain @@ -844,7 +782,6 @@ func TestReload(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } @@ -877,16 +814,16 @@ func TestReload(t *testing.T) { <-reloaded <-reloaded - if fcmd.CombinedOutputCalls != 5 { - t.Errorf("expected 5 CombinedOutput() calls total, got %d", fcmd.CombinedOutputCalls) + if fcmd.CombinedOutputCalls != 4 { + t.Errorf("expected 4 CombinedOutput() calls total, got %d", fcmd.CombinedOutputCalls) } - if !sets.NewString(fcmd.CombinedOutputLog[2]...).HasAll("iptables", "-t", "nat", "-N", "FOOBAR") { + if !sets.NewString(fcmd.CombinedOutputLog[1]...).HasAll("iptables", "-t", "nat", "-N", "FOOBAR") { t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[2]) } - if !sets.NewString(fcmd.CombinedOutputLog[3]...).HasAll("iptables", "-t", "nat", "-C", "OUTPUT", "abc", "123") { + if !sets.NewString(fcmd.CombinedOutputLog[2]...).HasAll("iptables", "-t", "nat", "-C", "OUTPUT", "abc", "123") { t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[3]) } - if !sets.NewString(fcmd.CombinedOutputLog[4]...).HasAll("iptables", "-t", "nat", "-A", "OUTPUT", "abc", "123") { + if !sets.NewString(fcmd.CombinedOutputLog[3]...).HasAll("iptables", "-t", "nat", "-A", "OUTPUT", "abc", "123") { t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[4]) } @@ -895,7 +832,7 @@ func TestReload(t *testing.T) { dbusConn.EmitSignal("org.freedesktop.DBus", "/org/freedesktop/DBus", "org.freedesktop.DBus", "NameOwnerChanged", "io.k8s.Something", "", ":1.1") <-reloaded - if fcmd.CombinedOutputCalls != 5 { + if fcmd.CombinedOutputCalls != 4 { t.Errorf("Incorrect signal caused a reload") } @@ -903,16 +840,16 @@ func TestReload(t *testing.T) { <-reloaded <-reloaded - if fcmd.CombinedOutputCalls != 8 { - t.Errorf("expected 8 CombinedOutput() calls total, got %d", fcmd.CombinedOutputCalls) + if fcmd.CombinedOutputCalls != 7 { + t.Errorf("expected 7 CombinedOutput() calls total, got %d", fcmd.CombinedOutputCalls) } - if !sets.NewString(fcmd.CombinedOutputLog[5]...).HasAll("iptables", "-t", "nat", "-N", "FOOBAR") { + if !sets.NewString(fcmd.CombinedOutputLog[4]...).HasAll("iptables", "-t", "nat", "-N", "FOOBAR") { t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[5]) } - if !sets.NewString(fcmd.CombinedOutputLog[6]...).HasAll("iptables", "-t", "nat", "-C", "OUTPUT", "abc", "123") { + if !sets.NewString(fcmd.CombinedOutputLog[5]...).HasAll("iptables", "-t", "nat", "-C", "OUTPUT", "abc", "123") { t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[6]) } - if !sets.NewString(fcmd.CombinedOutputLog[7]...).HasAll("iptables", "-t", "nat", "-A", "OUTPUT", "abc", "123") { + if !sets.NewString(fcmd.CombinedOutputLog[6]...).HasAll("iptables", "-t", "nat", "-A", "OUTPUT", "abc", "123") { t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[7]) } } @@ -921,7 +858,6 @@ func testSaveInto(t *testing.T, protocol Protocol) { version := " v1.9.22" iptablesCmd := iptablesCommand(protocol) iptablesSaveCmd := iptablesSaveCommand(protocol) - iptablesRestoreCmd := iptablesRestoreCommand(protocol) protoStr := protocolStr(protocol) output := fmt.Sprintf(`# Generated by %s on Thu Jan 19 11:38:09 2017 @@ -938,8 +874,6 @@ COMMIT CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{ // iptables version check func() ([]byte, error) { return []byte(iptablesCmd + version), nil }, - // iptables-restore version check - func() ([]byte, error) { return []byte(iptablesRestoreCmd + version), nil }, }, RunScript: []fakeexec.FakeRunAction{ func() ([]byte, []byte, error) { return []byte(output), []byte(stderrOutput), nil }, @@ -951,7 +885,6 @@ COMMIT func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } runner := New(&fexec, dbus.NewFake(nil, nil), protocol) @@ -968,8 +901,8 @@ COMMIT t.Errorf("%s: Expected output '%s', got '%v'", protoStr, output, string(buffer.Bytes())) } - if fcmd.CombinedOutputCalls != 2 { - t.Errorf("%s: Expected 2 CombinedOutput() calls, got %d", protoStr, fcmd.CombinedOutputCalls) + if fcmd.CombinedOutputCalls != 1 { + t.Errorf("%s: Expected 1 CombinedOutput() calls, got %d", protoStr, fcmd.CombinedOutputCalls) } if fcmd.RunCalls != 1 { t.Errorf("%s: Expected 1 Run() call, got %d", protoStr, fcmd.RunCalls) @@ -1007,8 +940,6 @@ func testRestore(t *testing.T, protocol Protocol) { CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{ // iptables version check func() ([]byte, error) { return []byte(iptablesCmd + version), nil }, - // iptables-restore version check - func() ([]byte, error) { return []byte(iptablesRestoreCmd + version), nil }, func() ([]byte, error) { return []byte{}, nil }, func() ([]byte, error) { return []byte{}, nil }, func() ([]byte, error) { return []byte{}, nil }, @@ -1024,7 +955,6 @@ func testRestore(t *testing.T, protocol Protocol) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } runner := New(&fexec, dbus.NewFake(nil, nil), protocol) @@ -1036,9 +966,9 @@ func testRestore(t *testing.T, protocol Protocol) { t.Errorf("%s flush,restore: Expected success, got %v", protoStr, err) } - commandSet := sets.NewString(fcmd.CombinedOutputLog[2]...) + commandSet := sets.NewString(fcmd.CombinedOutputLog[1]...) if !commandSet.HasAll(iptablesRestoreCmd, "-T", string(TableNAT), "--counters") || commandSet.HasAny("--noflush") { - t.Errorf("%s flush, restore: Expected cmd containing '%s -T %s --counters', got '%s'", protoStr, iptablesRestoreCmd, string(TableNAT), fcmd.CombinedOutputLog[2]) + t.Errorf("%s flush, restore: Expected cmd containing '%s -T %s --counters', got '%s'", protoStr, iptablesRestoreCmd, string(TableNAT), fcmd.CombinedOutputLog[1]) } // FlushTables, NoRestoreCounters @@ -1047,9 +977,9 @@ func testRestore(t *testing.T, protocol Protocol) { t.Errorf("%s flush, no restore: Expected success, got %v", protoStr, err) } - commandSet = sets.NewString(fcmd.CombinedOutputLog[3]...) + commandSet = sets.NewString(fcmd.CombinedOutputLog[2]...) if !commandSet.HasAll(iptablesRestoreCmd, "-T", string(TableNAT)) || commandSet.HasAny("--noflush", "--counters") { - t.Errorf("%s flush, no restore: Expected cmd containing '--noflush' or '--counters', got '%s'", protoStr, fcmd.CombinedOutputLog[3]) + t.Errorf("%s flush, no restore: Expected cmd containing '--noflush' or '--counters', got '%s'", protoStr, fcmd.CombinedOutputLog[2]) } // NoFlushTables, RestoreCounters @@ -1058,9 +988,9 @@ func testRestore(t *testing.T, protocol Protocol) { t.Errorf("%s no flush, restore: Expected success, got %v", protoStr, err) } - commandSet = sets.NewString(fcmd.CombinedOutputLog[4]...) + commandSet = sets.NewString(fcmd.CombinedOutputLog[3]...) if !commandSet.HasAll(iptablesRestoreCmd, "-T", string(TableNAT), "--noflush", "--counters") { - t.Errorf("%s no flush, restore: Expected cmd containing '--noflush' and '--counters', got '%s'", protoStr, fcmd.CombinedOutputLog[4]) + t.Errorf("%s no flush, restore: Expected cmd containing '--noflush' and '--counters', got '%s'", protoStr, fcmd.CombinedOutputLog[3]) } // NoFlushTables, NoRestoreCounters @@ -1069,13 +999,13 @@ func testRestore(t *testing.T, protocol Protocol) { t.Errorf("%s no flush, no restore: Expected success, got %v", protoStr, err) } - commandSet = sets.NewString(fcmd.CombinedOutputLog[5]...) + commandSet = sets.NewString(fcmd.CombinedOutputLog[4]...) if !commandSet.HasAll(iptablesRestoreCmd, "-T", string(TableNAT), "--noflush") || commandSet.HasAny("--counters") { - t.Errorf("%s no flush, no restore: Expected cmd containing '%s -T %s --noflush', got '%s'", protoStr, iptablesRestoreCmd, string(TableNAT), fcmd.CombinedOutputLog[5]) + t.Errorf("%s no flush, no restore: Expected cmd containing '%s -T %s --noflush', got '%s'", protoStr, iptablesRestoreCmd, string(TableNAT), fcmd.CombinedOutputLog[4]) } - if fcmd.CombinedOutputCalls != 6 { - t.Errorf("%s: Expected 6 total CombinedOutput() calls, got %d", protoStr, fcmd.CombinedOutputCalls) + if fcmd.CombinedOutputCalls != 5 { + t.Errorf("%s: Expected 5 total CombinedOutput() calls, got %d", protoStr, fcmd.CombinedOutputCalls) } // Failure. @@ -1099,8 +1029,6 @@ func TestRestoreAll(t *testing.T) { CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{ // iptables version check func() ([]byte, error) { return []byte("iptables v1.9.22"), nil }, - // iptables-restore version check - func() ([]byte, error) { return []byte("iptables-restore v1.9.22"), nil }, func() ([]byte, error) { return []byte{}, nil }, func() ([]byte, error) { return nil, &fakeexec.FakeExitError{Status: 1} }, }, @@ -1110,7 +1038,6 @@ func TestRestoreAll(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } runner := newInternal(&fexec, dbus.NewFake(nil, nil), ProtocolIpv4, TestLockfilePath) @@ -1122,13 +1049,13 @@ func TestRestoreAll(t *testing.T) { t.Fatalf("expected success, got %v", err) } - commandSet := sets.NewString(fcmd.CombinedOutputLog[2]...) + commandSet := sets.NewString(fcmd.CombinedOutputLog[1]...) if !commandSet.HasAll("iptables-restore", "--counters", "--noflush") { t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[2]) } - if fcmd.CombinedOutputCalls != 3 { - t.Errorf("expected 3 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) + if fcmd.CombinedOutputCalls != 2 { + t.Errorf("expected 2 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) } // Failure. @@ -1144,8 +1071,6 @@ func TestRestoreAllWait(t *testing.T) { CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{ // iptables version check func() ([]byte, error) { return []byte("iptables v1.9.22"), nil }, - // iptables-restore version check - func() ([]byte, error) { return []byte("iptables-restore v1.9.22"), nil }, func() ([]byte, error) { return []byte{}, nil }, func() ([]byte, error) { return nil, &fakeexec.FakeExitError{Status: 1} }, }, @@ -1155,7 +1080,6 @@ func TestRestoreAllWait(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } runner := newInternal(&fexec, dbus.NewFake(nil, nil), ProtocolIpv4, TestLockfilePath) @@ -1167,13 +1091,13 @@ func TestRestoreAllWait(t *testing.T) { t.Fatalf("expected success, got %v", err) } - commandSet := sets.NewString(fcmd.CombinedOutputLog[2]...) + commandSet := sets.NewString(fcmd.CombinedOutputLog[1]...) if !commandSet.HasAll("iptables-restore", WaitString, WaitSecondsValue, "--counters", "--noflush") { - t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[2]) + t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[1]) } - if fcmd.CombinedOutputCalls != 3 { - t.Errorf("expected 3 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) + if fcmd.CombinedOutputCalls != 2 { + t.Errorf("expected 2 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) } // Failure. @@ -1184,14 +1108,12 @@ func TestRestoreAllWait(t *testing.T) { } // TestRestoreAllWaitOldIptablesRestore tests that the "wait" flag is not passed -// to a in-compatible iptables-restore +// to an old iptables-restore func TestRestoreAllWaitOldIptablesRestore(t *testing.T) { fcmd := fakeexec.FakeCmd{ CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{ // iptables version check - func() ([]byte, error) { return []byte("iptables v1.9.22"), nil }, - // iptables-restore version check - func() ([]byte, error) { return []byte("unrecognized option: --version"), nil }, + func() ([]byte, error) { return []byte("iptables v1.4.22"), nil }, func() ([]byte, error) { return []byte{}, nil }, func() ([]byte, error) { return nil, &fakeexec.FakeExitError{Status: 1} }, }, @@ -1201,7 +1123,6 @@ func TestRestoreAllWaitOldIptablesRestore(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } runner := newInternal(&fexec, dbus.NewFake(nil, nil), ProtocolIpv4, TestLockfilePath) @@ -1213,16 +1134,16 @@ func TestRestoreAllWaitOldIptablesRestore(t *testing.T) { t.Fatalf("expected success, got %v", err) } - commandSet := sets.NewString(fcmd.CombinedOutputLog[2]...) + commandSet := sets.NewString(fcmd.CombinedOutputLog[1]...) if !commandSet.HasAll("iptables-restore", "--counters", "--noflush") { t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[2]) } if commandSet.HasAll(WaitString, WaitSecondsValue) { - t.Errorf("wrong CombinedOutput() log (unexpected %s option), got %s", WaitString, fcmd.CombinedOutputLog[2]) + t.Errorf("wrong CombinedOutput() log (unexpected %s option), got %s", WaitString, fcmd.CombinedOutputLog[1]) } - if fcmd.CombinedOutputCalls != 3 { - t.Errorf("expected 3 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) + if fcmd.CombinedOutputCalls != 2 { + t.Errorf("expected 2 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) } // Failure. @@ -1239,15 +1160,12 @@ func TestRestoreAllGrabNewLock(t *testing.T) { fcmd := fakeexec.FakeCmd{ CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{ // iptables version check - func() ([]byte, error) { return []byte("iptables v1.9.22"), nil }, - // iptables-restore version check - func() ([]byte, error) { return []byte("unrecognized option: --version"), nil }, + func() ([]byte, error) { return []byte("iptables v1.4.22"), nil }, }, } fexec := fakeexec.FakeExec{ CommandScript: []fakeexec.FakeCommandAction{ func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } @@ -1282,15 +1200,12 @@ func TestRestoreAllGrabOldLock(t *testing.T) { fcmd := fakeexec.FakeCmd{ CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{ // iptables version check - func() ([]byte, error) { return []byte("iptables v1.9.22"), nil }, - // iptables-restore version check - func() ([]byte, error) { return []byte("unrecognized option: --version"), nil }, + func() ([]byte, error) { return []byte("iptables v1.4.22"), nil }, }, } fexec := fakeexec.FakeExec{ CommandScript: []fakeexec.FakeCommandAction{ func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } From a735c97356847e063e66180a4b0a549e861aee5c Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Fri, 19 Jul 2019 12:08:20 -0400 Subject: [PATCH 2/3] kube-proxy: drop iptables version check Kube-proxy's iptables mode used to care whether utiliptables's EnsureRule was able to use "iptables -C" or if it had to implement it hackily using "iptables-save". But that became irrelevant when kube-proxy was reimplemented using "iptables-restore", and no one ever noticed. So remove that check. --- cmd/kube-proxy/app/BUILD | 10 -- cmd/kube-proxy/app/server_others.go | 18 +-- cmd/kube-proxy/app/server_others_test.go | 176 ++++++++++------------- cmd/kube-proxy/app/server_test.go | 47 ------ pkg/proxy/iptables/BUILD | 1 - pkg/proxy/iptables/proxier.go | 41 +----- 6 files changed, 87 insertions(+), 206 deletions(-) diff --git a/cmd/kube-proxy/app/BUILD b/cmd/kube-proxy/app/BUILD index 55a67682916..087e752cf86 100644 --- a/cmd/kube-proxy/app/BUILD +++ b/cmd/kube-proxy/app/BUILD @@ -172,43 +172,33 @@ go_test( ] + select({ "@io_bazel_rules_go//go/platform:android": [ "//pkg/proxy/ipvs:go_default_library", - "//pkg/util/iptables:go_default_library", ], "@io_bazel_rules_go//go/platform:darwin": [ "//pkg/proxy/ipvs:go_default_library", - "//pkg/util/iptables:go_default_library", ], "@io_bazel_rules_go//go/platform:dragonfly": [ "//pkg/proxy/ipvs:go_default_library", - "//pkg/util/iptables:go_default_library", ], "@io_bazel_rules_go//go/platform:freebsd": [ "//pkg/proxy/ipvs:go_default_library", - "//pkg/util/iptables:go_default_library", ], "@io_bazel_rules_go//go/platform:linux": [ "//pkg/proxy/ipvs:go_default_library", - "//pkg/util/iptables:go_default_library", ], "@io_bazel_rules_go//go/platform:nacl": [ "//pkg/proxy/ipvs:go_default_library", - "//pkg/util/iptables:go_default_library", ], "@io_bazel_rules_go//go/platform:netbsd": [ "//pkg/proxy/ipvs:go_default_library", - "//pkg/util/iptables:go_default_library", ], "@io_bazel_rules_go//go/platform:openbsd": [ "//pkg/proxy/ipvs:go_default_library", - "//pkg/util/iptables:go_default_library", ], "@io_bazel_rules_go//go/platform:plan9": [ "//pkg/proxy/ipvs:go_default_library", - "//pkg/util/iptables:go_default_library", ], "@io_bazel_rules_go//go/platform:solaris": [ "//pkg/proxy/ipvs:go_default_library", - "//pkg/util/iptables:go_default_library", ], "//conditions:default": [], }), diff --git a/cmd/kube-proxy/app/server_others.go b/cmd/kube-proxy/app/server_others.go index a86a9593cf5..7f84c455954 100644 --- a/cmd/kube-proxy/app/server_others.go +++ b/cmd/kube-proxy/app/server_others.go @@ -134,7 +134,7 @@ func newProxyServer( var proxier proxy.ProxyProvider - proxyMode := getProxyMode(string(config.Mode), iptInterface, kernelHandler, ipsetInterface, iptables.LinuxKernelCompatTester{}) + proxyMode := getProxyMode(string(config.Mode), kernelHandler, ipsetInterface, iptables.LinuxKernelCompatTester{}) nodeIP := net.ParseIP(config.BindAddress) if nodeIP.IsUnspecified() { nodeIP = utilnode.GetNodeIP(client, hostname) @@ -236,20 +236,20 @@ func newProxyServer( }, nil } -func getProxyMode(proxyMode string, iptver iptables.Versioner, khandle ipvs.KernelHandler, ipsetver ipvs.IPSetVersioner, kcompat iptables.KernelCompatTester) string { +func getProxyMode(proxyMode string, khandle ipvs.KernelHandler, ipsetver ipvs.IPSetVersioner, kcompat iptables.KernelCompatTester) string { switch proxyMode { case proxyModeUserspace: return proxyModeUserspace case proxyModeIPTables: - return tryIPTablesProxy(iptver, kcompat) + return tryIPTablesProxy(kcompat) case proxyModeIPVS: - return tryIPVSProxy(iptver, khandle, ipsetver, kcompat) + return tryIPVSProxy(khandle, ipsetver, kcompat) } klog.Warningf("Flag proxy-mode=%q unknown, assuming iptables proxy", proxyMode) - return tryIPTablesProxy(iptver, kcompat) + return tryIPTablesProxy(kcompat) } -func tryIPVSProxy(iptver iptables.Versioner, khandle ipvs.KernelHandler, ipsetver ipvs.IPSetVersioner, kcompat iptables.KernelCompatTester) string { +func tryIPVSProxy(khandle ipvs.KernelHandler, ipsetver ipvs.IPSetVersioner, kcompat iptables.KernelCompatTester) string { // guaranteed false on error, error only necessary for debugging // IPVS Proxier relies on ip_vs_* kernel modules and ipset useIPVSProxy, err := ipvs.CanUseIPVSProxier(khandle, ipsetver) @@ -263,12 +263,12 @@ func tryIPVSProxy(iptver iptables.Versioner, khandle ipvs.KernelHandler, ipsetve // Try to fallback to iptables before falling back to userspace klog.V(1).Infof("Can't use ipvs proxier, trying iptables proxier") - return tryIPTablesProxy(iptver, kcompat) + return tryIPTablesProxy(kcompat) } -func tryIPTablesProxy(iptver iptables.Versioner, kcompat iptables.KernelCompatTester) string { +func tryIPTablesProxy(kcompat iptables.KernelCompatTester) string { // guaranteed false on error, error only necessary for debugging - useIPTablesProxy, err := iptables.CanUseIPTablesProxier(iptver, kcompat) + useIPTablesProxy, err := iptables.CanUseIPTablesProxier(kcompat) if err != nil { utilruntime.HandleError(fmt.Errorf("can't determine whether to use iptables proxy, using userspace proxier: %v", err)) return proxyModeUserspace diff --git a/cmd/kube-proxy/app/server_others_test.go b/cmd/kube-proxy/app/server_others_test.go index a7b7f216bbf..eb6b086142a 100644 --- a/cmd/kube-proxy/app/server_others_test.go +++ b/cmd/kube-proxy/app/server_others_test.go @@ -23,68 +23,75 @@ import ( "testing" "k8s.io/kubernetes/pkg/proxy/ipvs" - "k8s.io/kubernetes/pkg/util/iptables" ) +type fakeIPSetVersioner struct { + version string // what to return + err error // what to return +} + +func (fake *fakeIPSetVersioner) GetVersion() (string, error) { + return fake.version, fake.err +} + +type fakeKernelCompatTester struct { + ok bool +} + +func (fake *fakeKernelCompatTester) IsCompatible() error { + if !fake.ok { + return fmt.Errorf("error") + } + return nil +} + +// fakeKernelHandler implements KernelHandler. +type fakeKernelHandler struct { + modules []string + kernelVersion string +} + +func (fake *fakeKernelHandler) GetModules() ([]string, error) { + return fake.modules, nil +} + +func (fake *fakeKernelHandler) GetKernelVersion() (string, error) { + return fake.kernelVersion, nil +} + func Test_getProxyMode(t *testing.T) { var cases = []struct { - flag string - iptablesVersion string - ipsetVersion string - kmods []string - kernelVersion string - kernelCompat bool - iptablesError error - ipsetError error - expected string + flag string + ipsetVersion string + kmods []string + kernelVersion string + kernelCompat bool + ipsetError error + expected string }{ { // flag says userspace flag: "userspace", expected: proxyModeUserspace, }, - { // flag says iptables, error detecting version - flag: "iptables", - iptablesError: fmt.Errorf("flag says iptables, error detecting version"), - expected: proxyModeUserspace, + { // flag says iptables, kernel not compatible + flag: "iptables", + kernelCompat: false, + expected: proxyModeUserspace, }, - { // flag says iptables, version too low - flag: "iptables", - iptablesVersion: "0.0.0", - expected: proxyModeUserspace, + { // flag says iptables, kernel is compatible + flag: "iptables", + kernelCompat: true, + expected: proxyModeIPTables, }, - { // flag says iptables, version ok, kernel not compatible - flag: "iptables", - iptablesVersion: iptables.MinCheckVersion, - kernelCompat: false, - expected: proxyModeUserspace, + { // detect, kernel not compatible + flag: "", + kernelCompat: false, + expected: proxyModeUserspace, }, - { // flag says iptables, version ok, kernel is compatible - flag: "iptables", - iptablesVersion: iptables.MinCheckVersion, - kernelCompat: true, - expected: proxyModeIPTables, - }, - { // detect, error - flag: "", - iptablesError: fmt.Errorf("oops"), - expected: proxyModeUserspace, - }, - { // detect, version too low - flag: "", - iptablesVersion: "0.0.0", - expected: proxyModeUserspace, - }, - { // detect, version ok, kernel not compatible - flag: "", - iptablesVersion: iptables.MinCheckVersion, - kernelCompat: false, - expected: proxyModeUserspace, - }, - { // detect, version ok, kernel is compatible - flag: "", - iptablesVersion: iptables.MinCheckVersion, - kernelCompat: true, - expected: proxyModeIPTables, + { // detect, kernel is compatible + flag: "", + kernelCompat: true, + expected: proxyModeIPTables, }, { // flag says ipvs, ipset version ok, kernel modules installed for linux kernel before 4.19 flag: "ipvs", @@ -101,69 +108,38 @@ func Test_getProxyMode(t *testing.T) { expected: proxyModeIPVS, }, { // flag says ipvs, ipset version too low, fallback on iptables mode - flag: "ipvs", - kmods: []string{"ip_vs", "ip_vs_rr", "ip_vs_wrr", "ip_vs_sh", "nf_conntrack"}, - kernelVersion: "4.19", - ipsetVersion: "0.0", - iptablesVersion: iptables.MinCheckVersion, - kernelCompat: true, - expected: proxyModeIPTables, + flag: "ipvs", + kmods: []string{"ip_vs", "ip_vs_rr", "ip_vs_wrr", "ip_vs_sh", "nf_conntrack"}, + kernelVersion: "4.19", + ipsetVersion: "0.0", + kernelCompat: true, + expected: proxyModeIPTables, }, { // flag says ipvs, bad ipset version, fallback on iptables mode - flag: "ipvs", - kmods: []string{"ip_vs", "ip_vs_rr", "ip_vs_wrr", "ip_vs_sh", "nf_conntrack"}, - kernelVersion: "4.19", - ipsetVersion: "a.b.c", - iptablesVersion: iptables.MinCheckVersion, - kernelCompat: true, - expected: proxyModeIPTables, + flag: "ipvs", + kmods: []string{"ip_vs", "ip_vs_rr", "ip_vs_wrr", "ip_vs_sh", "nf_conntrack"}, + kernelVersion: "4.19", + ipsetVersion: "a.b.c", + kernelCompat: true, + expected: proxyModeIPTables, }, { // flag says ipvs, required kernel modules are not installed, fallback on iptables mode - flag: "ipvs", - kmods: []string{"foo", "bar", "baz"}, - kernelVersion: "4.19", - ipsetVersion: ipvs.MinIPSetCheckVersion, - iptablesVersion: iptables.MinCheckVersion, - kernelCompat: true, - expected: proxyModeIPTables, - }, - { // flag says ipvs, required kernel modules are not installed, iptables version too old, fallback on userspace mode - flag: "ipvs", - kmods: []string{"foo", "bar", "baz"}, - kernelVersion: "4.19", - ipsetVersion: ipvs.MinIPSetCheckVersion, - iptablesVersion: "0.0.0", - kernelCompat: true, - expected: proxyModeUserspace, - }, - { // flag says ipvs, required kernel modules are not installed, iptables version too old, fallback on userspace mode - flag: "ipvs", - kmods: []string{"foo", "bar", "baz"}, - kernelVersion: "4.19", - ipsetVersion: ipvs.MinIPSetCheckVersion, - iptablesVersion: "0.0.0", - kernelCompat: true, - expected: proxyModeUserspace, - }, - { // flag says ipvs, ipset version too low, iptables version too old, kernel not compatible, fallback on userspace mode - flag: "ipvs", - kmods: []string{"ip_vs", "ip_vs_rr", "ip_vs_wrr", "ip_vs_sh", "nf_conntrack"}, - kernelVersion: "4.19", - ipsetVersion: "0.0", - iptablesVersion: iptables.MinCheckVersion, - kernelCompat: false, - expected: proxyModeUserspace, + flag: "ipvs", + kmods: []string{"foo", "bar", "baz"}, + kernelVersion: "4.19", + ipsetVersion: ipvs.MinIPSetCheckVersion, + kernelCompat: true, + expected: proxyModeIPTables, }, } for i, c := range cases { - versioner := &fakeIPTablesVersioner{c.iptablesVersion, c.iptablesError} kcompater := &fakeKernelCompatTester{c.kernelCompat} ipsetver := &fakeIPSetVersioner{c.ipsetVersion, c.ipsetError} khandler := &fakeKernelHandler{ modules: c.kmods, kernelVersion: c.kernelVersion, } - r := getProxyMode(c.flag, versioner, khandler, ipsetver, kcompater) + r := getProxyMode(c.flag, khandler, ipsetver, kcompater) if r != c.expected { t.Errorf("Case[%d] Expected %q, got %q", i, c.expected, r) } diff --git a/cmd/kube-proxy/app/server_test.go b/cmd/kube-proxy/app/server_test.go index 5b7671c3e13..615bdebe3a8 100644 --- a/cmd/kube-proxy/app/server_test.go +++ b/cmd/kube-proxy/app/server_test.go @@ -38,53 +38,6 @@ import ( utilpointer "k8s.io/utils/pointer" ) -type fakeIPTablesVersioner struct { - version string // what to return - err error // what to return -} - -func (fake *fakeIPTablesVersioner) GetVersion() (string, error) { - return fake.version, fake.err -} - -func (fake *fakeIPTablesVersioner) IsCompatible() error { - return fake.err -} - -type fakeIPSetVersioner struct { - version string // what to return - err error // what to return -} - -func (fake *fakeIPSetVersioner) GetVersion() (string, error) { - return fake.version, fake.err -} - -type fakeKernelCompatTester struct { - ok bool -} - -func (fake *fakeKernelCompatTester) IsCompatible() error { - if !fake.ok { - return fmt.Errorf("error") - } - return nil -} - -// fakeKernelHandler implements KernelHandler. -type fakeKernelHandler struct { - modules []string - kernelVersion string -} - -func (fake *fakeKernelHandler) GetModules() ([]string, error) { - return fake.modules, nil -} - -func (fake *fakeKernelHandler) GetKernelVersion() (string, error) { - return fake.kernelVersion, nil -} - // This test verifies that NewProxyServer does not crash when CleanupAndExit is true. func TestProxyServerWithCleanupAndExit(t *testing.T) { // Each bind address below is a separate test case diff --git a/pkg/proxy/iptables/BUILD b/pkg/proxy/iptables/BUILD index 3c0ef8f1334..f7e0d409c6b 100644 --- a/pkg/proxy/iptables/BUILD +++ b/pkg/proxy/iptables/BUILD @@ -21,7 +21,6 @@ go_library( "//pkg/util/sysctl:go_default_library", "//staging/src/k8s.io/api/core/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library", - "//staging/src/k8s.io/apimachinery/pkg/util/version:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/wait:go_default_library", "//staging/src/k8s.io/client-go/tools/record:go_default_library", "//vendor/k8s.io/klog:go_default_library", diff --git a/pkg/proxy/iptables/proxier.go b/pkg/proxy/iptables/proxier.go index 7f2abc42ec6..274d57ab1fb 100644 --- a/pkg/proxy/iptables/proxier.go +++ b/pkg/proxy/iptables/proxier.go @@ -36,7 +36,6 @@ import ( v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" - utilversion "k8s.io/apimachinery/pkg/util/version" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/tools/record" "k8s.io/kubernetes/pkg/proxy" @@ -52,15 +51,6 @@ import ( ) const ( - // iptablesMinVersion is the minimum version of iptables for which we will use the Proxier - // from this package instead of the userspace Proxier. While most of the - // features we need were available earlier, the '-C' flag was added more - // recently. We use that indirectly in Ensure* functions, and if we don't - // have it, we have to be extra careful about the exact args we feed in being - // the same as the args we read back (iptables itself normalizes some args). - // This is the "new" Proxier, so we require "new" versions of tools. - iptablesMinVersion = utiliptables.MinCheckVersion - // the services chain kubeServicesChain utiliptables.Chain = "KUBE-SERVICES" @@ -83,12 +73,6 @@ const ( kubeForwardChain utiliptables.Chain = "KUBE-FORWARD" ) -// Versioner can query the current iptables version. -type Versioner interface { - // returns "X.Y.Z" - GetVersion() (string, error) -} - // KernelCompatTester tests whether the required kernel capabilities are // present to run the iptables proxier. type KernelCompatTester interface { @@ -96,28 +80,8 @@ type KernelCompatTester interface { } // CanUseIPTablesProxier returns true if we should use the iptables Proxier -// instead of the "classic" userspace Proxier. This is determined by checking -// the iptables version and for the existence of kernel features. It may return -// an error if it fails to get the iptables version without error, in which -// case it will also return false. -func CanUseIPTablesProxier(iptver Versioner, kcompat KernelCompatTester) (bool, error) { - minVersion, err := utilversion.ParseGeneric(iptablesMinVersion) - if err != nil { - return false, err - } - versionString, err := iptver.GetVersion() - if err != nil { - return false, err - } - version, err := utilversion.ParseGeneric(versionString) - if err != nil { - return false, err - } - if version.LessThan(minVersion) { - return false, nil - } - - // Check that the kernel supports what we need. +// instead of the "classic" userspace Proxier. +func CanUseIPTablesProxier(kcompat KernelCompatTester) (bool, error) { if err := kcompat.IsCompatible(); err != nil { return false, err } @@ -131,7 +95,6 @@ type LinuxKernelCompatTester struct{} // that it exists. If this Proxier is chosen, we'll initialize it as we // need. func (lkct LinuxKernelCompatTester) IsCompatible() error { - _, err := utilsysctl.New().GetSysctl(sysctlRouteLocalnet) return err } From 81cd27a51e0a414f80a275b2cb156c923a7f9ef4 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Tue, 16 Jul 2019 17:11:02 -0400 Subject: [PATCH 3/3] iptables: simplify version handling --- .../network/hostport/fake_iptables.go | 4 - pkg/util/iptables/BUILD | 1 + pkg/util/iptables/iptables.go | 115 ++++++------------ pkg/util/iptables/iptables_test.go | 40 ++---- pkg/util/iptables/testing/fake.go | 4 - 5 files changed, 48 insertions(+), 116 deletions(-) diff --git a/pkg/kubelet/dockershim/network/hostport/fake_iptables.go b/pkg/kubelet/dockershim/network/hostport/fake_iptables.go index ab28a1d8747..83ec5ba7994 100644 --- a/pkg/kubelet/dockershim/network/hostport/fake_iptables.go +++ b/pkg/kubelet/dockershim/network/hostport/fake_iptables.go @@ -52,10 +52,6 @@ func NewFakeIPTables() *fakeIPTables { } } -func (f *fakeIPTables) GetVersion() (string, error) { - return "1.4.21", nil -} - func (f *fakeIPTables) getTable(tableName utiliptables.Table) (*fakeTable, error) { table, ok := f.tables[string(tableName)] if !ok { diff --git a/pkg/util/iptables/BUILD b/pkg/util/iptables/BUILD index 6e62709b6da..a0d32ac7f78 100644 --- a/pkg/util/iptables/BUILD +++ b/pkg/util/iptables/BUILD @@ -45,6 +45,7 @@ go_test( "@io_bazel_rules_go//go/platform:linux": [ "//pkg/util/dbus:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/util/version:go_default_library", "//vendor/k8s.io/utils/exec:go_default_library", "//vendor/k8s.io/utils/exec/testing:go_default_library", ], diff --git a/pkg/util/iptables/iptables.go b/pkg/util/iptables/iptables.go index 3f2e460934c..2f1faed3e07 100644 --- a/pkg/util/iptables/iptables.go +++ b/pkg/util/iptables/iptables.go @@ -43,8 +43,6 @@ const ( // An injectable interface for running iptables commands. Implementations must be goroutine-safe. type Interface interface { - // GetVersion returns the "X.Y.Z" version string for iptables. - GetVersion() (string, error) // EnsureChain checks if the specified chain exists and, if not, creates it. If the chain existed, return true. EnsureChain(table Table, chain Chain) (bool, error) // FlushChain clears the specified chain. If the chain did not exist, return error. @@ -121,12 +119,13 @@ const NoFlushTables FlushFlag = false // Versions of iptables less than this do not support the -C / --check flag // (test whether a rule exists). -const MinCheckVersion = "1.4.11" +var MinCheckVersion = utilversion.MustParseGeneric("1.4.11") // Minimum iptables versions supporting the -w and -w flags -const WaitMinVersion = "1.4.20" -const WaitSecondsMinVersion = "1.4.22" -const WaitRestoreMinVersion = "1.6.2" +var WaitMinVersion = utilversion.MustParseGeneric("1.4.20") +var WaitSecondsMinVersion = utilversion.MustParseGeneric("1.4.22") +var WaitRestoreMinVersion = utilversion.MustParseGeneric("1.6.2") + const WaitString = "-w" const WaitSecondsValue = "5" @@ -151,10 +150,10 @@ type runner struct { // newInternal returns a new Interface which will exec iptables, and allows the // caller to change the iptables-restore lockfile path func newInternal(exec utilexec.Interface, dbus utildbus.Interface, protocol Protocol, lockfilePath string) Interface { - vstring, err := getIPTablesVersionString(exec, protocol) + version, err := getIPTablesVersion(exec, protocol) if err != nil { klog.Warningf("Error checking iptables version, assuming version at least %s: %v", MinCheckVersion, err) - vstring = MinCheckVersion + version = MinCheckVersion } if lockfilePath == "" { @@ -165,10 +164,10 @@ func newInternal(exec utilexec.Interface, dbus utildbus.Interface, protocol Prot exec: exec, dbus: dbus, protocol: protocol, - hasCheck: getIPTablesHasCheckCommand(vstring), + hasCheck: version.AtLeast(MinCheckVersion), hasListener: false, - waitFlag: getIPTablesWaitFlag(vstring), - restoreWaitFlag: getIPTablesRestoreWaitFlag(vstring), + waitFlag: getIPTablesWaitFlag(version), + restoreWaitFlag: getIPTablesRestoreWaitFlag(version), lockfilePath: lockfilePath, } return runner @@ -215,11 +214,6 @@ func (runner *runner) connectToFirewallD() { go runner.dbusSignalHandler(bus) } -// GetVersion returns the version string. -func (runner *runner) GetVersion() (string, error) { - return getIPTablesVersionString(runner.exec, runner.protocol) -} - // EnsureChain is part of Interface. func (runner *runner) EnsureChain(table Table, chain Chain) (bool, error) { fullArgs := makeFullArgs(table, chain) @@ -540,83 +534,46 @@ func makeFullArgs(table Table, chain Chain, args ...string) []string { return append([]string{string(chain), "-t", string(table)}, args...) } -// Checks if iptables has the "-C" flag -func getIPTablesHasCheckCommand(vstring string) bool { - minVersion, err := utilversion.ParseGeneric(MinCheckVersion) - if err != nil { - klog.Errorf("MinCheckVersion (%s) is not a valid version string: %v", MinCheckVersion, err) - return true - } - version, err := utilversion.ParseGeneric(vstring) - if err != nil { - klog.Errorf("vstring (%s) is not a valid version string: %v", vstring, err) - return true - } - return version.AtLeast(minVersion) -} - -// Checks if iptables version has a "wait" flag -func getIPTablesWaitFlag(vstring string) []string { - version, err := utilversion.ParseGeneric(vstring) - if err != nil { - klog.Errorf("vstring (%s) is not a valid version string: %v", vstring, err) - return nil - } - - minVersion, err := utilversion.ParseGeneric(WaitMinVersion) - if err != nil { - klog.Errorf("WaitMinVersion (%s) is not a valid version string: %v", WaitMinVersion, err) - return nil - } - if version.LessThan(minVersion) { - return nil - } - - minVersion, err = utilversion.ParseGeneric(WaitSecondsMinVersion) - if err != nil { - klog.Errorf("WaitSecondsMinVersion (%s) is not a valid version string: %v", WaitSecondsMinVersion, err) - return nil - } - if version.LessThan(minVersion) { - return []string{WaitString} - } - return []string{WaitString, WaitSecondsValue} -} - -// getIPTablesVersionString runs "iptables --version" to get the version string -// in the form "X.X.X" -func getIPTablesVersionString(exec utilexec.Interface, protocol Protocol) (string, error) { +// getIPTablesVersion runs "iptables --version" and parses the returned version +func getIPTablesVersion(exec utilexec.Interface, protocol Protocol) (*utilversion.Version, error) { // this doesn't access mutable state so we don't need to use the interface / runner iptablesCmd := iptablesCommand(protocol) bytes, err := exec.Command(iptablesCmd, "--version").CombinedOutput() if err != nil { - return "", err + return nil, err } versionMatcher := regexp.MustCompile("v([0-9]+(\\.[0-9]+)+)") match := versionMatcher.FindStringSubmatch(string(bytes)) if match == nil { - return "", fmt.Errorf("no iptables version found in string: %s", bytes) + return nil, fmt.Errorf("no iptables version found in string: %s", bytes) + } + version, err := utilversion.ParseGeneric(match[1]) + if err != nil { + return nil, fmt.Errorf("iptables version %q is not a valid version string: %v", match[1], err) + } + + return version, nil +} + +// Checks if iptables version has a "wait" flag +func getIPTablesWaitFlag(version *utilversion.Version) []string { + switch { + case version.AtLeast(WaitSecondsMinVersion): + return []string{WaitString, WaitSecondsValue} + case version.AtLeast(WaitMinVersion): + return []string{WaitString} + default: + return nil } - return match[1], nil } // Checks if iptables-restore has a "wait" flag -func getIPTablesRestoreWaitFlag(vstring string) []string { - version, err := utilversion.ParseGeneric(vstring) - if err != nil { - klog.Errorf("vstring (%s) is not a valid version string: %v", vstring, err) +func getIPTablesRestoreWaitFlag(version *utilversion.Version) []string { + if version.AtLeast(WaitRestoreMinVersion) { + return []string{WaitString, WaitSecondsValue} + } else { return nil } - - minVersion, err := utilversion.ParseGeneric(WaitRestoreMinVersion) - if err != nil { - klog.Errorf("WaitRestoreMinVersion (%s) is not a valid version string: %v", WaitRestoreMinVersion, err) - return nil - } - if version.LessThan(minVersion) { - return nil - } - return []string{WaitString, WaitSecondsValue} } // goroutine to listen for D-Bus signals diff --git a/pkg/util/iptables/iptables_test.go b/pkg/util/iptables/iptables_test.go index 361adce72c0..f263c1d2eef 100644 --- a/pkg/util/iptables/iptables_test.go +++ b/pkg/util/iptables/iptables_test.go @@ -29,6 +29,7 @@ import ( "time" "k8s.io/apimachinery/pkg/util/sets" + utilversion "k8s.io/apimachinery/pkg/util/version" "k8s.io/kubernetes/pkg/util/dbus" "k8s.io/utils/exec" fakeexec "k8s.io/utils/exec/testing" @@ -52,14 +53,11 @@ func testIPTablesVersionCmds(t *testing.T, protocol Protocol) { CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{ // iptables version response (for runner instantiation) func() ([]byte, error) { return []byte(iptablesCmd + version), nil }, - // iptables version response (for call to runner.GetVersion()) - func() ([]byte, error) { return []byte(iptablesCmd + version), nil }, }, } fexec := fakeexec.FakeExec{ CommandScript: []fakeexec.FakeCommandAction{ func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } runner := New(&fexec, dbus.NewFake(nil, nil), protocol) @@ -69,16 +67,6 @@ func testIPTablesVersionCmds(t *testing.T, protocol Protocol) { if !sets.NewString(fcmd.CombinedOutputLog[0]...).HasAll(iptablesCmd, "--version") { t.Errorf("%s runner instantiate: Expected cmd '%s --version', Got '%s'", protoStr, iptablesCmd, fcmd.CombinedOutputLog[0]) } - - _, err := runner.GetVersion() - if err != nil { - t.Errorf("%s GetVersion: Expected success, got %v", protoStr, err) - } - - // Check that proper iptables version command was used for runner.GetVersion - if !sets.NewString(fcmd.CombinedOutputLog[1]...).HasAll(iptablesCmd, "--version") { - t.Errorf("%s GetVersion: Expected cmd '%s --version', Got '%s'", protoStr, iptablesCmd, fcmd.CombinedOutputLog[2]) - } } func TestIPTablesVersionCmdsIPv4(t *testing.T) { @@ -485,14 +473,13 @@ func TestDeleteRuleErrorDeleting(t *testing.T) { func TestGetIPTablesHasCheckCommand(t *testing.T) { testCases := []struct { Version string - Err bool Expected bool }{ - {"iptables v1.4.7", false, false}, - {"iptables v1.4.11", false, true}, - {"iptables v1.4.19.1", false, true}, - {"iptables v2.0.0", false, true}, - {"total junk", true, false}, + {"iptables v1.4.7", false}, + {"iptables v1.4.11", true}, + {"iptables v1.4.19.1", true}, + {"iptables v2.0.0", true}, + {"total junk", true}, } for _, testCase := range testCases { @@ -506,15 +493,10 @@ func TestGetIPTablesHasCheckCommand(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - version, err := getIPTablesVersionString(&fexec, ProtocolIpv4) - if (err != nil) != testCase.Err { - t.Errorf("Expected error: %v, Got error: %v", testCase.Err, err) - } - if err == nil { - check := getIPTablesHasCheckCommand(version) - if testCase.Expected != check { - t.Errorf("Expected result: %v, Got result: %v", testCase.Expected, check) - } + ipt := New(&fexec, nil, ProtocolIpv4) + runner := ipt.(*runner) + if testCase.Expected != runner.hasCheck { + t.Errorf("Expected result: %v, Got result: %v", testCase.Expected, runner.hasCheck) } } } @@ -645,7 +627,7 @@ func TestIPTablesWaitFlag(t *testing.T) { } for _, testCase := range testCases { - result := getIPTablesWaitFlag(testCase.Version) + result := getIPTablesWaitFlag(utilversion.MustParseGeneric(testCase.Version)) if !reflect.DeepEqual(result, testCase.Result) { t.Errorf("For %s expected %v got %v", testCase.Version, testCase.Result, result) } diff --git a/pkg/util/iptables/testing/fake.go b/pkg/util/iptables/testing/fake.go index 66adc1a275e..6470eb86289 100644 --- a/pkg/util/iptables/testing/fake.go +++ b/pkg/util/iptables/testing/fake.go @@ -48,10 +48,6 @@ func NewFake() *FakeIPTables { return &FakeIPTables{} } -func (*FakeIPTables) GetVersion() (string, error) { - return "0.0.0", nil -} - func (*FakeIPTables) EnsureChain(table iptables.Table, chain iptables.Chain) (bool, error) { return true, nil }