iptables: simplify version handling

Author: danwinship

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 {

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",
         ],

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<seconds> 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)
 	}
-	return match[1], nil
-}
-
-// Checks if iptables-restore has a "wait" flag
-func getIPTablesRestoreWaitFlag(vstring string) []string {
-	version, err := utilversion.ParseGeneric(vstring)
+	version, err := utilversion.ParseGeneric(match[1])
 	if err != nil {
-		klog.Errorf("vstring (%s) is not a valid version string: %v", vstring, err)
-		return nil
+		return nil, fmt.Errorf("iptables version %q is not a valid version string: %v", match[1], err)
 	}
 
-	minVersion, err := utilversion.ParseGeneric(WaitRestoreMinVersion)
-	if err != nil {
-		klog.Errorf("WaitRestoreMinVersion (%s) is not a valid version string: %v", WaitRestoreMinVersion, 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
 	}
-	if version.LessThan(minVersion) {
+}
+
+// Checks if iptables-restore has a "wait" flag
+func getIPTablesRestoreWaitFlag(version *utilversion.Version) []string {
+	if version.AtLeast(WaitRestoreMinVersion) {
+		return []string{WaitString, WaitSecondsValue}
+	} else {
 		return nil
 	}
-	return []string{WaitString, WaitSecondsValue}
 }
 
 // goroutine to listen for D-Bus signals

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)
 		}

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
 }