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 }