From b25b6a66bab94c6d4a5d981c5fa729dc071baeeb Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Fri, 22 May 2026 10:47:38 -0400 Subject: [PATCH 1/8] Improve comments on exec setup in utiltiptables tests --- pkg/util/iptables/iptables_test.go | 40 +++++++++--------------------- 1 file changed, 12 insertions(+), 28 deletions(-) diff --git a/pkg/util/iptables/iptables_test.go b/pkg/util/iptables/iptables_test.go index 0e89fd4fd19..213906adcf1 100644 --- a/pkg/util/iptables/iptables_test.go +++ b/pkg/util/iptables/iptables_test.go @@ -482,15 +482,13 @@ func TestEnsureRuleAlreadyExists(t *testing.T) { CombinedOutputScript: []fakeexec.FakeAction{ // iptables version check func() ([]byte, []byte, error) { return []byte("iptables v1.9.22"), nil, nil }, - // Success. + // Success on the -C call, meaning the rule exists. func() ([]byte, []byte, error) { return []byte{}, nil, nil }, }, } 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. Success of that exec means "done". func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } @@ -515,17 +513,15 @@ func TestEnsureRuleNew(t *testing.T) { CombinedOutputScript: []fakeexec.FakeAction{ // iptables version check func() ([]byte, []byte, error) { return []byte("iptables v1.9.22"), nil, nil }, - // Status 1 on the first call. + // Status 1 on the -C call, meaning the rule doesn't exist func() ([]byte, []byte, error) { return nil, nil, &fakeexec.FakeExitError{Status: 1} }, - // Success on the second call. + // Success on the -A call. func() ([]byte, []byte, error) { return []byte{}, nil, nil }, }, } 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...) }, }, @@ -551,15 +547,13 @@ func TestEnsureRuleErrorChecking(t *testing.T) { CombinedOutputScript: []fakeexec.FakeAction{ // iptables version check func() ([]byte, []byte, error) { return []byte("iptables v1.9.22"), nil, nil }, - // Status 2 on the first call. + // Status 2 on the -C call, meaning something went wrong while checking. func() ([]byte, []byte, error) { return nil, nil, &fakeexec.FakeExitError{Status: 2} }, }, } 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...) }, }, } @@ -578,17 +572,15 @@ func TestEnsureRuleErrorCreating(t *testing.T) { CombinedOutputScript: []fakeexec.FakeAction{ // iptables version check func() ([]byte, []byte, error) { return []byte("iptables v1.9.22"), nil, nil }, - // Status 1 on the first call. + // Status 1 on the -C call, meaning the rule doesn't exist. func() ([]byte, []byte, error) { return nil, nil, &fakeexec.FakeExitError{Status: 1} }, - // Status 1 on the second call. + // Status 1 on the -A call, meaning failure adding it. func() ([]byte, []byte, error) { return nil, 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...) }, }, @@ -608,15 +600,13 @@ func TestDeleteRuleDoesNotExist(t *testing.T) { CombinedOutputScript: []fakeexec.FakeAction{ // iptables version check func() ([]byte, []byte, error) { return []byte("iptables v1.9.22"), nil, nil }, - // Status 1 on the first call. + // Status 1 on the -C call, meaning the rule doesn't exist. func() ([]byte, []byte, error) { return nil, 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 exec means "does not exist". func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } @@ -638,17 +628,15 @@ func TestDeleteRuleExists(t *testing.T) { CombinedOutputScript: []fakeexec.FakeAction{ // iptables version check func() ([]byte, []byte, error) { return []byte("iptables v1.9.22"), nil, nil }, - // Success on the first call. + // Success on the -C call, meaning the rule exists. func() ([]byte, []byte, error) { return []byte{}, nil, nil }, - // Success on the second call. + // Success on the -D call. func() ([]byte, []byte, error) { return []byte{}, nil, nil }, }, } 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. 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...) }, }, @@ -671,15 +659,13 @@ func TestDeleteRuleErrorChecking(t *testing.T) { CombinedOutputScript: []fakeexec.FakeAction{ // iptables version check func() ([]byte, []byte, error) { return []byte("iptables v1.9.22"), nil, nil }, - // Status 2 on the first call. + // Status 2 on the -C call, meaning something went wrong while checking. func() ([]byte, []byte, error) { return nil, nil, &fakeexec.FakeExitError{Status: 2} }, }, } 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...) }, }, } @@ -698,17 +684,15 @@ func TestDeleteRuleErrorDeleting(t *testing.T) { CombinedOutputScript: []fakeexec.FakeAction{ // iptables version check func() ([]byte, []byte, error) { return []byte("iptables v1.9.22"), nil, nil }, - // Success on the first call. + // Success on the -C call, meaning the rule exists. func() ([]byte, []byte, error) { return []byte{}, nil, nil }, - // Status 1 on the second call. + // Status 1 on the -D call, meaning failure to delete it. func() ([]byte, []byte, error) { return nil, 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. 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...) }, }, From ef208e79187d0a22f6280b8fb3e0012ffedaae11 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Tue, 12 May 2026 09:00:03 -0400 Subject: [PATCH 2/8] Assume iptables has -C itables 1.4.11 added the -C flag in 2011, but this didn't reach Debian stable until a few months before k8s 1.0 was released, so at the time it was reasonable to want to support older versions as well. It is no longer reasonable. --- pkg/util/iptables/iptables.go | 78 +----------------- pkg/util/iptables/iptables_test.go | 122 ----------------------------- 2 files changed, 3 insertions(+), 197 deletions(-) diff --git a/pkg/util/iptables/iptables.go b/pkg/util/iptables/iptables.go index 52b81f1ec82..ddda230224e 100644 --- a/pkg/util/iptables/iptables.go +++ b/pkg/util/iptables/iptables.go @@ -30,7 +30,6 @@ import ( "time" v1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/util/sets" utilversion "k8s.io/apimachinery/pkg/util/version" utilwait "k8s.io/apimachinery/pkg/util/wait" "k8s.io/klog/v2" @@ -164,11 +163,6 @@ const FlushTables FlushFlag = true // NoFlushTables a boolean false constant for option flag FlushFlag const NoFlushTables FlushFlag = false -// MinCheckVersion minimum version to be checked -// Versions of iptables less than this do not support the -C / --check flag -// (test whether a rule exists). -var MinCheckVersion = utilversion.MustParseGeneric("1.4.11") - // RandomFullyMinVersion is the minimum version from which the --random-fully flag is supported, // used for port mapping to be fully randomized var RandomFullyMinVersion = utilversion.MustParseGeneric("1.6.2") @@ -199,7 +193,6 @@ type runner struct { mu sync.Mutex exec utilexec.Interface protocol Protocol - hasCheck bool hasRandomFully bool waitFlag []string restoreWaitFlag []string @@ -232,7 +225,6 @@ func newInternal(exec utilexec.Interface, protocol Protocol, lockfilePath14x, lo return runner } - runner.hasCheck = version.AtLeast(MinCheckVersion) runner.hasRandomFully = version.AtLeast(RandomFullyMinVersion) runner.waitFlag = getIPTablesWaitFlag(version) runner.restoreWaitFlag = getIPTablesRestoreWaitFlag(version, exec, protocol) @@ -322,7 +314,7 @@ func (runner *runner) EnsureRule(position RulePosition, table Table, chain Chain runner.mu.Lock() defer runner.mu.Unlock() - exists, err := runner.checkRule(table, chain, args...) + exists, err := runner.checkRule(fullArgs) if err != nil { return false, err } @@ -343,7 +335,7 @@ func (runner *runner) DeleteRule(table Table, chain Chain, args ...string) error runner.mu.Lock() defer runner.mu.Unlock() - exists, err := runner.checkRule(table, chain, args...) + exists, err := runner.checkRule(fullArgs) if err != nil { return err } @@ -494,71 +486,7 @@ func (runner *runner) runContext(ctx context.Context, op operation, args []strin // Returns (bool, nil) if it was able to check the existence of the rule, or // (, error) if the process of checking failed. -func (runner *runner) checkRule(table Table, chain Chain, args ...string) (bool, error) { - if runner.hasCheck { - return runner.checkRuleUsingCheck(makeFullArgs(table, chain, args...)) - } - return runner.checkRuleWithoutCheck(table, chain, args...) -} - -var hexnumRE = regexp.MustCompile("0x0+([0-9])") - -func trimhex(s string) string { - return hexnumRE.ReplaceAllString(s, "0x$1") -} - -// Executes the rule check without using the "-C" flag, instead parsing iptables-save. -// Present for compatibility with <1.4.11 versions of iptables. This is full -// of hack and half-measures. We should nix this ASAP. -func (runner *runner) checkRuleWithoutCheck(table Table, chain Chain, args ...string) (bool, error) { - iptablesSaveCmd := iptablesSaveCommand(runner.protocol) - klog.V(1).InfoS("Running", "command", iptablesSaveCmd, "table", string(table)) - out, err := runner.exec.Command(iptablesSaveCmd, "-t", string(table)).CombinedOutput() - if err != nil { - return false, fmt.Errorf("error checking rule: %v", err) - } - - // Sadly, iptables has inconsistent quoting rules for comments. Just remove all quotes. - // Also, quoted multi-word comments (which are counted as a single arg) - // will be unpacked into multiple args, - // in order to compare against iptables-save output (which will be split at whitespace boundary) - // e.g. a single arg('"this must be before the NodePort rules"') will be unquoted and unpacked into 7 args. - var argsCopy []string - for i := range args { - tmpField := strings.Trim(args[i], "\"") - tmpField = trimhex(tmpField) - argsCopy = append(argsCopy, strings.Fields(tmpField)...) - } - argset := sets.New(argsCopy...) - - for _, line := range strings.Split(string(out), "\n") { - fields := strings.Fields(line) - - // Check that this is a rule for the correct chain, and that it has - // the correct number of argument (+2 for "-A ") - if !strings.HasPrefix(line, fmt.Sprintf("-A %s", string(chain))) || len(fields) != len(argsCopy)+2 { - continue - } - - // Sadly, iptables has inconsistent quoting rules for comments. - // Just remove all quotes. - for i := range fields { - fields[i] = strings.Trim(fields[i], "\"") - fields[i] = trimhex(fields[i]) - } - - // TODO: This misses reorderings e.g. "-x foo ! -y bar" will match "! -x foo -y bar" - if sets.New(fields...).IsSuperset(argset) { - return true, nil - } - klog.V(5).InfoS("DBG: fields is not a superset of args", "fields", fields, "arguments", args) - } - - return false, nil -} - -// Executes the rule check using the "-C" flag -func (runner *runner) checkRuleUsingCheck(args []string) (bool, error) { +func (runner *runner) checkRule(args []string) (bool, error) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) defer cancel() diff --git a/pkg/util/iptables/iptables_test.go b/pkg/util/iptables/iptables_test.go index 213906adcf1..03220f9b4c1 100644 --- a/pkg/util/iptables/iptables_test.go +++ b/pkg/util/iptables/iptables_test.go @@ -115,7 +115,6 @@ func TestNew(t *testing.T) { }, }, expected: &runner{ - hasCheck: false, hasRandomFully: false, waitFlag: nil, restoreWaitFlag: nil, @@ -134,7 +133,6 @@ func TestNew(t *testing.T) { }, }, expected: &runner{ - hasCheck: true, hasRandomFully: false, waitFlag: []string{"-w"}, restoreWaitFlag: []string{"-w"}, @@ -149,7 +147,6 @@ func TestNew(t *testing.T) { }, }, expected: &runner{ - hasCheck: true, hasRandomFully: true, waitFlag: []string{"-w", "5"}, restoreWaitFlag: []string{"-w", "5"}, @@ -164,7 +161,6 @@ func TestNew(t *testing.T) { }, }, expected: &runner{ - hasCheck: true, hasRandomFully: true, waitFlag: []string{"-w", "5"}, restoreWaitFlag: []string{"-w", "5"}, @@ -179,7 +175,6 @@ func TestNew(t *testing.T) { }, }, expected: &runner{ - hasCheck: false, hasRandomFully: false, waitFlag: nil, restoreWaitFlag: nil, @@ -192,9 +187,6 @@ func TestNew(t *testing.T) { fexec := fakeExecForCommands(tc.commands) runner := newInternal(fexec, ProtocolIPv4, "", "").(*runner) - if runner.hasCheck != tc.expected.hasCheck { - t.Errorf("Expected hasCheck=%v, got %v", tc.expected.hasCheck, runner.hasCheck) - } if runner.hasRandomFully != tc.expected.hasRandomFully { t.Errorf("Expected hasRandomFully=%v, got %v", tc.expected.hasRandomFully, runner.hasRandomFully) } @@ -707,39 +699,6 @@ func TestDeleteRuleErrorDeleting(t *testing.T) { } } -func TestGetIPTablesHasCheckCommand(t *testing.T) { - testCases := []struct { - Version string - Expected bool - }{ - {"iptables v1.4.7", false}, - {"iptables v1.4.11", true}, - {"iptables v1.4.19.1", true}, - {"iptables v2.0.0", true}, - {"total junk", false}, - } - - for _, testCase := range testCases { - fcmd := fakeexec.FakeCmd{ - CombinedOutputScript: []fakeexec.FakeAction{ - func() ([]byte, []byte, error) { return []byte(testCase.Version), nil, nil }, - func() ([]byte, []byte, error) { return []byte(testCase.Version), nil, 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...) }, - }, - } - ipt := newInternal(fexec, ProtocolIPv4, "", "") - runner := ipt.(*runner) - if testCase.Expected != runner.hasCheck { - t.Errorf("Expected result: %v, Got result: %v", testCase.Expected, runner.hasCheck) - } - } -} - func TestIPTablesCommands(t *testing.T) { testCases := []struct { funcName string @@ -769,49 +728,6 @@ func TestIPTablesCommands(t *testing.T) { } } -func TestCheckRuleWithoutCheckPresent(t *testing.T) { - iptablesSaveOutput := `# Generated by iptables-save v1.4.7 on Wed Oct 29 14:56:01 2014 -*nat -:PREROUTING ACCEPT [2136997:197881818] -:POSTROUTING ACCEPT [4284525:258542680] -:OUTPUT ACCEPT [5901660:357267963] --A PREROUTING -m addrtype --dst-type LOCAL -m mark --mark 0x00004000/0x00004000 -j DOCKER -COMMIT -# Completed on Wed Oct 29 14:56:01 2014` - - fcmd := fakeexec.FakeCmd{ - CombinedOutputScript: []fakeexec.FakeAction{ - // Success. - func() ([]byte, []byte, error) { return []byte(iptablesSaveOutput), nil, nil }, - }, - } - fexec := &fakeexec.FakeExec{ - CommandScript: []fakeexec.FakeCommandAction{ - // The first 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...) }, - }, - } - runner := &runner{exec: fexec} - exists, err := runner.checkRuleWithoutCheck( - TableNAT, ChainPrerouting, - "-m", "addrtype", - "-m", "mark", "--mark", "0x4000/0x4000", - "-j", "DOCKER", - "--dst-type", "LOCAL") - if err != nil { - t.Errorf("expected success, got %v", err) - } - if !exists { - t.Errorf("expected exists = true") - } - if fcmd.CombinedOutputCalls != 1 { - t.Errorf("expected 1 CombinedOutput() call, got %d", fcmd.CombinedOutputCalls) - } - if !sets.New(fcmd.CombinedOutputLog[0]...).HasAll("iptables-save", "-t", "nat") { - t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[0]) - } -} - func TestGetIPTablesRestoreWaitFlag(t *testing.T) { testCases := []struct { name string @@ -919,44 +835,6 @@ func TestGetIPTablesRestoreWaitFlag(t *testing.T) { } } -func TestCheckRuleWithoutCheckAbsent(t *testing.T) { - iptablesSaveOutput := `# Generated by iptables-save v1.4.7 on Wed Oct 29 14:56:01 2014 -*nat -:PREROUTING ACCEPT [2136997:197881818] -:POSTROUTING ACCEPT [4284525:258542680] -:OUTPUT ACCEPT [5901660:357267963] --A PREROUTING -m addrtype --dst-type LOCAL -j DOCKER -COMMIT -# Completed on Wed Oct 29 14:56:01 2014` - - fcmd := fakeexec.FakeCmd{ - CombinedOutputScript: []fakeexec.FakeAction{ - // Success. - func() ([]byte, []byte, error) { return []byte(iptablesSaveOutput), nil, nil }, - }, - } - fexec := &fakeexec.FakeExec{ - CommandScript: []fakeexec.FakeCommandAction{ - // The first 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...) }, - }, - } - runner := &runner{exec: fexec} - exists, err := runner.checkRuleWithoutCheck(TableNAT, ChainPrerouting, "-m", "addrtype", "-j", "DOCKER") - if err != nil { - t.Errorf("expected success, got %v", err) - } - if exists { - t.Errorf("expected exists = false") - } - if fcmd.CombinedOutputCalls != 1 { - t.Errorf("expected 1 CombinedOutput() call, got %d", fcmd.CombinedOutputCalls) - } - if !sets.New(fcmd.CombinedOutputLog[0]...).HasAll("iptables-save", "-t", "nat") { - t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[0]) - } -} - func TestIPTablesWaitFlag(t *testing.T) { testCases := []struct { Version string From 0fbc6c3acb3aaa67d17f60fd259e2456b3ca4e3a Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Tue, 12 May 2026 09:08:38 -0400 Subject: [PATCH 3/8] Assume iptables and iptables-restore have -w iptables got -w in 1.6.0, in 2015. iptables-restore got it in 1.6.2, in 2018. If someone wants to run k8s on an 8-year-old distro, they can just build their kube-proxy image on something newer. --- pkg/util/iptables/iptables.go | 89 +---- pkg/util/iptables/iptables_test.go | 502 +---------------------------- 2 files changed, 10 insertions(+), 581 deletions(-) diff --git a/pkg/util/iptables/iptables.go b/pkg/util/iptables/iptables.go index ddda230224e..fae17db0048 100644 --- a/pkg/util/iptables/iptables.go +++ b/pkg/util/iptables/iptables.go @@ -167,15 +167,6 @@ const NoFlushTables FlushFlag = false // used for port mapping to be fully randomized var RandomFullyMinVersion = utilversion.MustParseGeneric("1.6.2") -// WaitMinVersion a minimum iptables versions supporting the -w and -w flags -var WaitMinVersion = utilversion.MustParseGeneric("1.4.20") - -// WaitSecondsMinVersion a minimum iptables versions supporting the wait seconds -var WaitSecondsMinVersion = utilversion.MustParseGeneric("1.4.22") - -// WaitRestoreMinVersion a minimum iptables versions supporting the wait restore seconds -var WaitRestoreMinVersion = utilversion.MustParseGeneric("1.6.2") - // WaitString a constant for specifying the wait flag const WaitString = "-w" @@ -194,8 +185,6 @@ type runner struct { exec utilexec.Interface protocol Protocol hasRandomFully bool - waitFlag []string - restoreWaitFlag []string lockfilePath14x string lockfilePath16x string } @@ -226,8 +215,6 @@ func newInternal(exec utilexec.Interface, protocol Protocol, lockfilePath14x, lo } runner.hasRandomFully = version.AtLeast(RandomFullyMinVersion) - runner.waitFlag = getIPTablesWaitFlag(version) - runner.restoreWaitFlag = getIPTablesRestoreWaitFlag(version, exec, protocol) return runner } @@ -414,24 +401,8 @@ func (runner *runner) restoreInternal(args []string, data []byte, flush FlushFla args = append(args, "--counters") } - // Grab the iptables lock to prevent iptables-restore and iptables - // from stepping on each other. iptables-restore 1.6.2 will have - // a --wait option like iptables itself, but that's not widely deployed. - if len(runner.restoreWaitFlag) == 0 { - locker, err := grabIptablesLocks(runner.lockfilePath14x, runner.lockfilePath16x) - if err != nil { - return err - } - trace.Step("Locks grabbed") - defer func(locker iptablesLocker) { - if err := locker.Close(); err != nil { - klog.ErrorS(err, "Failed to close iptables locks") - } - }(locker) - } - // run the command and return the output or an error including the output and error - fullArgs := append(runner.restoreWaitFlag, args...) + fullArgs := append([]string{WaitString, WaitSecondsValue}, args...) iptablesRestoreCmd := iptablesRestoreCommand(runner.protocol) klog.V(4).InfoS("Running", "command", iptablesRestoreCmd, "arguments", fullArgs) cmd := runner.exec.Command(iptablesRestoreCmd, fullArgs...) @@ -474,8 +445,7 @@ func (runner *runner) run(op operation, args []string) ([]byte, error) { func (runner *runner) runContext(ctx context.Context, op operation, args []string) ([]byte, error) { iptablesCmd := iptablesCommand(runner.protocol) - fullArgs := append(runner.waitFlag, string(op)) - fullArgs = append(fullArgs, args...) + fullArgs := append([]string{WaitString, WaitSecondsValue, string(op)}, args...) klog.V(5).InfoS("Running", "command", iptablesCmd, "arguments", fullArgs) if ctx == nil { return runner.exec.Command(iptablesCmd, fullArgs...).CombinedOutput() @@ -619,61 +589,6 @@ func getIPTablesVersion(exec utilexec.Interface, protocol Protocol) (*utilversio 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 - } -} - -// Checks if iptables-restore has a "wait" flag -func getIPTablesRestoreWaitFlag(version *utilversion.Version, exec utilexec.Interface, protocol Protocol) []string { - if version.AtLeast(WaitRestoreMinVersion) { - return []string{WaitString, WaitSecondsValue} - } - - // Older versions may have backported features; if iptables-restore supports - // --version, assume it also supports --wait - vstring, err := getIPTablesRestoreVersionString(exec, protocol) - if err != nil || vstring == "" { - klog.V(3).InfoS("Couldn't get iptables-restore version; assuming it doesn't support --wait") - return nil - } - if _, err := utilversion.ParseGeneric(vstring); err != nil { - klog.V(3).InfoS("Couldn't parse iptables-restore version; assuming it doesn't support --wait") - return nil - } - return []string{WaitString} -} - -// 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() - if err != nil { - return "", err - } - versionMatcher := regexp.MustCompile(iptablesVersionPattern) - match := versionMatcher.FindStringSubmatch(string(bytes)) - if match == nil { - return "", fmt.Errorf("no iptables version found in string: %s", bytes) - } - return match[1], nil -} - func (runner *runner) HasRandomFully() bool { return runner.hasRandomFully } diff --git a/pkg/util/iptables/iptables_test.go b/pkg/util/iptables/iptables_test.go index 03220f9b4c1..93b0c5ee4e9 100644 --- a/pkg/util/iptables/iptables_test.go +++ b/pkg/util/iptables/iptables_test.go @@ -21,8 +21,6 @@ package iptables import ( "bytes" "fmt" - "net" - "os" "reflect" "strings" "testing" @@ -30,8 +28,6 @@ import ( v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/sets" - utilversion "k8s.io/apimachinery/pkg/util/version" - "k8s.io/apimachinery/pkg/util/wait" "k8s.io/utils/exec" fakeexec "k8s.io/utils/exec/testing" ) @@ -115,9 +111,7 @@ func TestNew(t *testing.T) { }, }, expected: &runner{ - hasRandomFully: false, - waitFlag: nil, - restoreWaitFlag: nil, + hasRandomFully: false, }, }, { @@ -133,9 +127,7 @@ func TestNew(t *testing.T) { }, }, expected: &runner{ - hasRandomFully: false, - waitFlag: []string{"-w"}, - restoreWaitFlag: []string{"-w"}, + hasRandomFully: false, }, }, { @@ -147,9 +139,7 @@ func TestNew(t *testing.T) { }, }, expected: &runner{ - hasRandomFully: true, - waitFlag: []string{"-w", "5"}, - restoreWaitFlag: []string{"-w", "5"}, + hasRandomFully: true, }, }, { @@ -161,9 +151,7 @@ func TestNew(t *testing.T) { }, }, expected: &runner{ - hasRandomFully: true, - waitFlag: []string{"-w", "5"}, - restoreWaitFlag: []string{"-w", "5"}, + hasRandomFully: true, }, }, { @@ -175,9 +163,7 @@ func TestNew(t *testing.T) { }, }, expected: &runner{ - hasRandomFully: false, - waitFlag: nil, - restoreWaitFlag: nil, + hasRandomFully: false, }, }, } @@ -190,12 +176,6 @@ func TestNew(t *testing.T) { if runner.hasRandomFully != tc.expected.hasRandomFully { t.Errorf("Expected hasRandomFully=%v, got %v", tc.expected.hasRandomFully, runner.hasRandomFully) } - if !reflect.DeepEqual(runner.waitFlag, tc.expected.waitFlag) { - t.Errorf("Expected waitFlag=%v, got %v", tc.expected.waitFlag, runner.waitFlag) - } - if !reflect.DeepEqual(runner.restoreWaitFlag, tc.expected.restoreWaitFlag) { - t.Errorf("Expected restoreWaitFlag=%v, got %v", tc.expected.restoreWaitFlag, runner.restoreWaitFlag) - } }) } } @@ -254,7 +234,7 @@ func TestNewDualStack(t *testing.T) { }, { // ipv6 Present() - command: "ip6tables -S POSTROUTING -t nat", + command: "ip6tables -w 5 -S POSTROUTING -t nat", action: func() ([]byte, []byte, error) { return nil, nil, fmt.Errorf("no such file or directory") }, }, }, @@ -298,7 +278,7 @@ func TestNewDualStack(t *testing.T) { }, { // ipv4 Present() - command: "iptables -S POSTROUTING -t nat", + command: "iptables -w 5 -S POSTROUTING -t nat", action: func() ([]byte, []byte, error) { return nil, nil, fmt.Errorf("no such file or directory") }, }, { @@ -308,7 +288,7 @@ func TestNewDualStack(t *testing.T) { }, { // ipv6 Present() - command: "ip6tables -S POSTROUTING -t nat", + command: "ip6tables -w 5 -S POSTROUTING -t nat", action: func() ([]byte, []byte, error) { return nil, nil, fmt.Errorf("no such file or directory") }, }, }, @@ -728,236 +708,6 @@ func TestIPTablesCommands(t *testing.T) { } } -func TestGetIPTablesRestoreWaitFlag(t *testing.T) { - testCases := []struct { - name string - version string - restoreVersionOutput string - protocol string - expected []string - }{ - { - name: "version >= WaitRestoreMinVersion (1.6.2)", - version: "1.6.2", - protocol: "ipv4", - expected: []string{WaitString, WaitSecondsValue}, - }, - { - name: "version < WaitRestoreMinVersion (1.6.2) with valid restore version", - version: "1.4.22", - restoreVersionOutput: "iptables v1.4.22", - protocol: "ipv4", - expected: []string{WaitString}, - }, - { - name: "version < WaitRestoreMinVersion (1.6.2) with empty restore version", - version: "1.4.21", - restoreVersionOutput: "", - protocol: "ipv4", - expected: nil, - }, - { - name: "version < WaitRestoreMinVersion (1.6.2) with unparseable restore version", - version: "1.4.21", - restoreVersionOutput: "invalid-version", - protocol: "ipv4", - expected: nil, - }, - { - name: "version < WaitRestoreMinVersion with restore command error", - version: "1.4.21", - restoreVersionOutput: "error", - protocol: "ipv4", - expected: nil, - }, - { - name: "IPv6 protocol test with valid restore version", - version: "1.4.22", - restoreVersionOutput: "iptables v1.4.22", - protocol: "ipv6", - expected: []string{WaitString}, - }, - { - name: "Very old version (1.0.0)", - version: "1.0.0", - restoreVersionOutput: "iptables v1.0.0", - protocol: "ipv4", - expected: []string{WaitString}, - }, - { - name: "Version just below WaitRestoreMinVersion (1.6.1)", - version: "1.6.1", - restoreVersionOutput: "iptables v1.6.1", - protocol: "ipv4", - expected: []string{WaitString}, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - fcmd := fakeexec.FakeCmd{ - CombinedOutputScript: []fakeexec.FakeAction{ - func() ([]byte, []byte, error) { - if tc.restoreVersionOutput == "error" { - return nil, nil, fmt.Errorf("error getting version") - } - return []byte(tc.restoreVersionOutput), nil, nil - }, - }, - } - fexec := &fakeexec.FakeExec{ - CommandScript: []fakeexec.FakeCommandAction{ - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - }, - } - - version := utilversion.MustParseGeneric(tc.version) - - var protocol Protocol - if tc.protocol == "ipv6" { - protocol = ProtocolIPv6 - } else { - protocol = ProtocolIPv4 - } - - result := getIPTablesRestoreWaitFlag(version, fexec, protocol) - - if !reflect.DeepEqual(result, tc.expected) { - t.Errorf("Expected %v, got %v", tc.expected, result) - } - - if version.LessThan(WaitRestoreMinVersion) && tc.restoreVersionOutput != "" && tc.restoreVersionOutput != "error" { - if fcmd.CombinedOutputCalls != 1 { - t.Errorf("Expected iptables-restore --version to be called once, got %d calls", fcmd.CombinedOutputCalls) - } - } - }) - } -} - -func TestIPTablesWaitFlag(t *testing.T) { - testCases := []struct { - Version string - Result []string - }{ - {"0.55.55", nil}, - {"1.0.55", nil}, - {"1.4.19", nil}, - {"1.4.20", []string{WaitString}}, - {"1.4.21", []string{WaitString}}, - {"1.4.22", []string{WaitString, WaitSecondsValue}}, - {"1.5.0", []string{WaitString, WaitSecondsValue}}, - {"1.8.7", []string{WaitString, WaitSecondsValue}}, - {"1.8.8", []string{WaitString, WaitSecondsValue}}, - {"2.0.0", []string{WaitString, WaitSecondsValue}}, - } - - for _, testCase := range testCases { - 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) - } - } -} - -func TestWaitFlagUnavailable(t *testing.T) { - fcmd := fakeexec.FakeCmd{ - CombinedOutputScript: []fakeexec.FakeAction{ - // iptables version check - func() ([]byte, []byte, error) { return []byte("iptables v1.4.19"), nil, nil }, - // iptables-restore version check - func() ([]byte, []byte, error) { return []byte{}, nil, nil }, - // Success. - func() ([]byte, []byte, error) { return []byte{}, nil, nil }, - }, - } - 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...) }, - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - }, - } - runner := newInternal(fexec, ProtocolIPv4, "", "") - err := runner.DeleteChain(TableNAT, Chain("FOOBAR")) - 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 sets.New(fcmd.CombinedOutputLog[2]...).Has(WaitString) { - t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[2]) - } -} - -func TestWaitFlagOld(t *testing.T) { - fcmd := fakeexec.FakeCmd{ - CombinedOutputScript: []fakeexec.FakeAction{ - // iptables version check - func() ([]byte, []byte, error) { return []byte("iptables v1.4.20"), nil, nil }, - // iptables-restore version check - func() ([]byte, []byte, error) { return []byte{}, nil, nil }, - // Success. - func() ([]byte, []byte, error) { return []byte{}, nil, 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...) }, - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - }, - } - runner := newInternal(fexec, ProtocolIPv4, "", "") - err := runner.DeleteChain(TableNAT, Chain("FOOBAR")) - 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 !sets.New(fcmd.CombinedOutputLog[2]...).HasAll("iptables", WaitString) { - t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[2]) - } - if sets.New(fcmd.CombinedOutputLog[2]...).Has(WaitSecondsValue) { - t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[2]) - } -} - -func TestWaitFlagNew(t *testing.T) { - fcmd := fakeexec.FakeCmd{ - CombinedOutputScript: []fakeexec.FakeAction{ - // iptables version check - func() ([]byte, []byte, error) { return []byte("iptables v1.4.22"), nil, nil }, - // iptables-restore version check - func() ([]byte, []byte, error) { return []byte{}, nil, nil }, - // Success. - func() ([]byte, []byte, error) { return []byte{}, nil, 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...) }, - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - }, - } - runner := newInternal(fexec, ProtocolIPv4, "", "") - err := runner.DeleteChain(TableNAT, Chain("FOOBAR")) - 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 !sets.New(fcmd.CombinedOutputLog[2]...).HasAll("iptables", WaitString, WaitSecondsValue) { - t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[2]) - } -} - func testSaveInto(t *testing.T, protocol Protocol) { version := " v1.9.22" iptablesCmd := iptablesCommand(protocol) @@ -1164,242 +914,6 @@ func TestRestoreAll(t *testing.T) { } } -// TestRestoreAllWait tests that the "wait" flag is passed to a compatible iptables-restore -func TestRestoreAllWait(t *testing.T) { - fcmd := fakeexec.FakeCmd{ - CombinedOutputScript: []fakeexec.FakeAction{ - // iptables version check - func() ([]byte, []byte, error) { return []byte("iptables v1.9.22"), nil, nil }, - func() ([]byte, []byte, error) { return []byte{}, nil, nil }, - func() ([]byte, []byte, error) { return nil, nil, &fakeexec.FakeExitError{Status: 1} }, - }, - } - 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...) }, - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - }, - } - lockPath14x, lockPath16x := getLockPaths() - runner := newInternal(fexec, ProtocolIPv4, lockPath14x, lockPath16x) - - err := runner.RestoreAll([]byte{}, NoFlushTables, RestoreCounters) - if err != nil { - t.Fatalf("expected success, got %v", err) - } - - commandSet := sets.New(fcmd.CombinedOutputLog[1]...) - if !commandSet.HasAll("iptables-restore", WaitString, WaitSecondsValue, "--counters", "--noflush") { - t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[1]) - } - - if fcmd.CombinedOutputCalls != 2 { - t.Errorf("expected 2 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) - } - - // Failure. - err = runner.Restore(TableNAT, []byte{}, FlushTables, RestoreCounters) - if err == nil { - t.Errorf("expected failure") - } -} - -// TestRestoreAllWaitOldIptablesRestore tests that the "wait" flag is not passed -// to an old iptables-restore -func TestRestoreAllWaitOldIptablesRestore(t *testing.T) { - fcmd := fakeexec.FakeCmd{ - CombinedOutputScript: []fakeexec.FakeAction{ - // iptables version check - func() ([]byte, []byte, error) { return []byte("iptables v1.4.22"), nil, nil }, - // iptables-restore version check - func() ([]byte, []byte, error) { return []byte{}, nil, nil }, - func() ([]byte, []byte, error) { return []byte{}, nil, nil }, - func() ([]byte, []byte, error) { return nil, nil, &fakeexec.FakeExitError{Status: 1} }, - }, - } - 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...) }, - 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...) }, - }, - } - lockPath14x, lockPath16x := getLockPaths() - // the lockPath14x is a UNIX socket which is cleaned up automatically on close, but the - // lockPath16x is a plain file which is not cleaned up. - defer os.Remove(lockPath16x) - runner := newInternal(fexec, ProtocolIPv4, lockPath14x, lockPath16x) - - err := runner.RestoreAll([]byte{}, NoFlushTables, RestoreCounters) - if err != nil { - t.Fatalf("expected success, got %v", err) - } - - commandSet := sets.New(fcmd.CombinedOutputLog[2]...) - if !commandSet.HasAll("iptables-restore", "--counters", "--noflush") { - t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[2]) - } - if commandSet.HasAll(WaitString) { - 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) - } - - // Failure. - err = runner.Restore(TableNAT, []byte{}, FlushTables, RestoreCounters) - if err == nil { - t.Errorf("expected failure") - } -} - -// TestRestoreAllGrabNewLock tests that the iptables code will grab the -// iptables /run lock when using an iptables-restore version that does not -// support the --wait argument -func TestRestoreAllGrabNewLock(t *testing.T) { - fcmd := fakeexec.FakeCmd{ - CombinedOutputScript: []fakeexec.FakeAction{ - // iptables version check - func() ([]byte, []byte, error) { return []byte("iptables v1.4.22"), nil, nil }, - // iptables-restore version check - func() ([]byte, []byte, error) { return []byte{}, nil, 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...) }, - }, - } - lockPath14x, lockPath16x := getLockPaths() - runner := newInternal(fexec, ProtocolIPv4, lockPath14x, lockPath16x) - - // Grab the /run lock and ensure the RestoreAll fails - runLock, err := os.OpenFile(lockPath16x, os.O_CREATE, 0600) - if err != nil { - t.Fatalf("expected to open %s, got %v", lockPath16x, err) - } - defer func() { - runLock.Close() - os.Remove(lockPath16x) - }() - - if err := grabIptablesFileLock(runLock); err != nil { - t.Errorf("expected to lock %s, got %v", lockPath16x, err) - } - - err = runner.RestoreAll([]byte{}, NoFlushTables, RestoreCounters) - if err == nil { - t.Fatal("expected failure, got success instead") - } - if !strings.Contains(err.Error(), "failed to acquire new iptables lock: timed out waiting for the condition") { - t.Errorf("expected timeout error, got %v", err) - } -} - -// TestRestoreAllGrabOldLock tests that the iptables code will grab the -// iptables @xtables abstract unix socket lock when using an iptables-restore -// version that does not support the --wait argument -func TestRestoreAllGrabOldLock(t *testing.T) { - fcmd := fakeexec.FakeCmd{ - CombinedOutputScript: []fakeexec.FakeAction{ - // iptables version check - func() ([]byte, []byte, error) { return []byte("iptables v1.4.22"), nil, nil }, - // iptables-restore version check - func() ([]byte, []byte, error) { return []byte{}, nil, 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...) }, - }, - } - lockPath14x, lockPath16x := getLockPaths() - // the lockPath14x is a UNIX socket which is cleaned up automatically on close, but the - // lockPath16x is a plain file which is not cleaned up. - defer os.Remove(lockPath16x) - runner := newInternal(fexec, ProtocolIPv4, lockPath14x, lockPath16x) - - var runLock *net.UnixListener - // Grab the abstract @xtables socket, will retry if the socket exists - err := wait.PollImmediate(time.Second, wait.ForeverTestTimeout, func() (done bool, err error) { - runLock, err = net.ListenUnix("unix", &net.UnixAddr{Name: lockPath14x, Net: "unix"}) - if err != nil { - t.Logf("Failed to lock %s: %v, will retry.", lockPath14x, err) - return false, nil - } - return true, nil - }) - if err != nil { - t.Fatalf("Timed out locking %s", lockPath14x) - } - if runLock == nil { - t.Fatal("Unexpected nil runLock") - } - - defer runLock.Close() - - err = runner.RestoreAll([]byte{}, NoFlushTables, RestoreCounters) - if err == nil { - t.Fatal("expected failure, got success instead") - } - if !strings.Contains(err.Error(), "failed to acquire old iptables lock: timed out waiting for the condition") { - t.Errorf("expected timeout error, got %v", err) - } -} - -// TestRestoreAllWaitBackportedIptablesRestore tests that the "wait" flag is passed -// to a seemingly-old-but-actually-new iptables-restore -func TestRestoreAllWaitBackportedIptablesRestore(t *testing.T) { - fcmd := fakeexec.FakeCmd{ - CombinedOutputScript: []fakeexec.FakeAction{ - // iptables version check - func() ([]byte, []byte, error) { return []byte("iptables v1.4.22"), nil, nil }, - // iptables-restore version check - func() ([]byte, []byte, error) { return []byte("iptables v1.4.22"), nil, nil }, - func() ([]byte, []byte, error) { return []byte{}, nil, nil }, - func() ([]byte, []byte, error) { return nil, nil, &fakeexec.FakeExitError{Status: 1} }, - }, - } - 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...) }, - 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...) }, - }, - } - lockPath14x, lockPath16x := getLockPaths() - runner := newInternal(fexec, ProtocolIPv4, lockPath14x, lockPath16x) - - err := runner.RestoreAll([]byte{}, NoFlushTables, RestoreCounters) - if err != nil { - t.Fatalf("expected success, got %v", err) - } - - commandSet := sets.New(fcmd.CombinedOutputLog[2]...) - if !commandSet.HasAll("iptables-restore", "--counters", "--noflush") { - t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[2]) - } - if !commandSet.HasAll(WaitString) { - t.Errorf("wrong CombinedOutput() log (expected %s option), got %s", WaitString, fcmd.CombinedOutputLog[1]) - } - - if fcmd.CombinedOutputCalls != 3 { - t.Errorf("expected 3 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) - } - - // Failure. - err = runner.Restore(TableNAT, []byte{}, FlushTables, RestoreCounters) - if err == nil { - t.Errorf("expected failure") - } -} - // TestExtractLines tests that func TestExtractLines(t *testing.T) { mkLines := func(lines ...LineData) []LineData { From 7544d50ece9a6c5548dde3f4580f483a9bfb7347 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Tue, 12 May 2026 09:15:38 -0400 Subject: [PATCH 4/8] Remove manual iptables locking support This was only needed to support versions of iptables-restore that didn't handle locking by themselves. --- pkg/util/iptables/iptables.go | 44 +++--------- pkg/util/iptables/iptables_linux.go | 100 ---------------------------- pkg/util/iptables/iptables_test.go | 38 +++++------ pkg/util/iptables/monitor_test.go | 2 +- 4 files changed, 27 insertions(+), 157 deletions(-) delete mode 100644 pkg/util/iptables/iptables_linux.go diff --git a/pkg/util/iptables/iptables.go b/pkg/util/iptables/iptables.go index fae17db0048..8620c84e2bd 100644 --- a/pkg/util/iptables/iptables.go +++ b/pkg/util/iptables/iptables.go @@ -173,37 +173,19 @@ const WaitString = "-w" // WaitSecondsValue a constant for specifying the default wait seconds const WaitSecondsValue = "5" -// LockfilePath16x is the iptables 1.6.x lock file acquired by any process that's making any change in the iptable rule -const LockfilePath16x = "/run/xtables.lock" - -// LockfilePath14x is the iptables 1.4.x lock file acquired by any process that's making any change in the iptable rule -const LockfilePath14x = "@xtables" - // runner implements Interface in terms of exec("iptables"). type runner struct { - mu sync.Mutex - exec utilexec.Interface - protocol Protocol - hasRandomFully bool - lockfilePath14x string - lockfilePath16x string + mu sync.Mutex + exec utilexec.Interface + protocol Protocol + hasRandomFully bool } -// 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, protocol Protocol, lockfilePath14x, lockfilePath16x string) Interface { - if lockfilePath16x == "" { - lockfilePath16x = LockfilePath16x - } - if lockfilePath14x == "" { - lockfilePath14x = LockfilePath14x - } - +// newInternal returns a new Interface which will exec iptables +func newInternal(exec utilexec.Interface, protocol Protocol) Interface { runner := &runner{ - exec: exec, - protocol: protocol, - lockfilePath14x: lockfilePath14x, - lockfilePath16x: lockfilePath16x, + exec: exec, + protocol: protocol, } version, err := getIPTablesVersion(exec, protocol) @@ -222,16 +204,16 @@ func newInternal(exec utilexec.Interface, protocol Protocol, lockfilePath14x, lo // Note that this function will return a single iptables Interface *and* an error, if only // a single family is supported. func New(protocol Protocol) Interface { - return newInternal(utilexec.New(), protocol, "", "") + return newInternal(utilexec.New(), protocol) } func newDualStackInternal(exec utilexec.Interface) map[v1.IPFamily]Interface { interfaces := map[v1.IPFamily]Interface{} - iptv4 := newInternal(exec, ProtocolIPv4, "", "") + iptv4 := newInternal(exec, ProtocolIPv4) if presentErr := iptv4.Present(); presentErr == nil { interfaces[v1.IPv4Protocol] = iptv4 } - iptv6 := newInternal(exec, ProtocolIPv6, "", "") + iptv6 := newInternal(exec, ProtocolIPv6) if presentErr := iptv6.Present(); presentErr == nil { interfaces[v1.IPv6Protocol] = iptv6 } @@ -382,10 +364,6 @@ func (runner *runner) RestoreAll(data []byte, flush FlushFlag, counters RestoreC return runner.restoreInternal(args, data, flush, counters) } -type iptablesLocker interface { - Close() error -} - // restoreInternal is the shared part of Restore/RestoreAll func (runner *runner) restoreInternal(args []string, data []byte, flush FlushFlag, counters RestoreCountersFlag) error { runner.mu.Lock() diff --git a/pkg/util/iptables/iptables_linux.go b/pkg/util/iptables/iptables_linux.go deleted file mode 100644 index b038791ff62..00000000000 --- a/pkg/util/iptables/iptables_linux.go +++ /dev/null @@ -1,100 +0,0 @@ -//go:build linux - -/* -Copyright 2017 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package iptables - -import ( - "fmt" - "net" - "os" - "time" - - "golang.org/x/sys/unix" - utilerrors "k8s.io/apimachinery/pkg/util/errors" - "k8s.io/apimachinery/pkg/util/wait" -) - -type locker struct { - lock16 *os.File - lock14 *net.UnixListener -} - -func (l *locker) Close() error { - errList := []error{} - if l.lock16 != nil { - if err := l.lock16.Close(); err != nil { - errList = append(errList, err) - } - } - if l.lock14 != nil { - if err := l.lock14.Close(); err != nil { - errList = append(errList, err) - } - } - return utilerrors.NewAggregate(errList) -} - -func grabIptablesLocks(lockfilePath14x, lockfilePath16x string) (iptablesLocker, error) { - var err error - var success bool - - l := &locker{} - defer func(l *locker) { - // Clean up immediately on failure - if !success { - l.Close() - } - }(l) - - // Grab both 1.6.x and 1.4.x-style locks; we don't know what the - // iptables-restore version is if it doesn't support --wait, so we - // can't assume which lock method it'll use. - - // Roughly duplicate iptables 1.6.x xtables_lock() function. - l.lock16, err = os.OpenFile(lockfilePath16x, os.O_CREATE, 0600) - if err != nil { - return nil, fmt.Errorf("failed to open iptables lock %s: %v", lockfilePath16x, err) - } - - if err := wait.PollImmediate(200*time.Millisecond, 2*time.Second, func() (bool, error) { - if err := grabIptablesFileLock(l.lock16); err != nil { - return false, nil - } - return true, nil - }); err != nil { - return nil, fmt.Errorf("failed to acquire new iptables lock: %v", err) - } - - // Roughly duplicate iptables 1.4.x xtables_lock() function. - if err := wait.PollImmediate(200*time.Millisecond, 2*time.Second, func() (bool, error) { - l.lock14, err = net.ListenUnix("unix", &net.UnixAddr{Name: lockfilePath14x, Net: "unix"}) - if err != nil { - return false, nil - } - return true, nil - }); err != nil { - return nil, fmt.Errorf("failed to acquire old iptables lock: %v", err) - } - - success = true - return l, nil -} - -func grabIptablesFileLock(f *os.File) error { - return unix.Flock(int(f.Fd()), unix.LOCK_EX|unix.LOCK_NB) -} diff --git a/pkg/util/iptables/iptables_test.go b/pkg/util/iptables/iptables_test.go index 93b0c5ee4e9..7c03950637e 100644 --- a/pkg/util/iptables/iptables_test.go +++ b/pkg/util/iptables/iptables_test.go @@ -24,7 +24,6 @@ import ( "reflect" "strings" "testing" - "time" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/sets" @@ -32,12 +31,6 @@ import ( fakeexec "k8s.io/utils/exec/testing" ) -func getLockPaths() (string, string) { - lock14x := fmt.Sprintf("@xtables-%d", time.Now().Nanosecond()) - lock16x := fmt.Sprintf("xtables-%d.lock", time.Now().Nanosecond()) - return lock14x, lock16x -} - type testCommand struct { command string action fakeexec.FakeAction @@ -171,7 +164,7 @@ func TestNew(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { fexec := fakeExecForCommands(tc.commands) - runner := newInternal(fexec, ProtocolIPv4, "", "").(*runner) + runner := newInternal(fexec, ProtocolIPv4).(*runner) if runner.hasRandomFully != tc.expected.hasRandomFully { t.Errorf("Expected hasRandomFully=%v, got %v", tc.expected.hasRandomFully, runner.hasRandomFully) @@ -336,7 +329,7 @@ func testEnsureChain(t *testing.T, protocol Protocol) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := newInternal(fexec, protocol, "", "") + runner := newInternal(fexec, protocol) // Success. exists, err := runner.EnsureChain(TableNAT, Chain("FOOBAR")) if err != nil { @@ -393,7 +386,7 @@ func TestFlushChain(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := newInternal(fexec, ProtocolIPv4, "", "") + runner := newInternal(fexec, ProtocolIPv4) // Success. err := runner.FlushChain(TableNAT, Chain("FOOBAR")) if err != nil { @@ -430,7 +423,7 @@ func TestDeleteChain(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := newInternal(fexec, ProtocolIPv4, "", "") + runner := newInternal(fexec, ProtocolIPv4) // Success. err := runner.DeleteChain(TableNAT, Chain("FOOBAR")) if err != nil { @@ -464,7 +457,7 @@ func TestEnsureRuleAlreadyExists(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := newInternal(fexec, ProtocolIPv4, "", "") + runner := newInternal(fexec, ProtocolIPv4) exists, err := runner.EnsureRule(Append, TableNAT, ChainOutput, "abc", "123") if err != nil { t.Errorf("expected success, got %v", err) @@ -498,7 +491,7 @@ func TestEnsureRuleNew(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := newInternal(fexec, ProtocolIPv4, "", "") + runner := newInternal(fexec, ProtocolIPv4) exists, err := runner.EnsureRule(Append, TableNAT, ChainOutput, "abc", "123") if err != nil { t.Errorf("expected success, got %v", err) @@ -529,7 +522,7 @@ func TestEnsureRuleErrorChecking(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := newInternal(fexec, ProtocolIPv4, "", "") + runner := newInternal(fexec, ProtocolIPv4) _, err := runner.EnsureRule(Append, TableNAT, ChainOutput, "abc", "123") if err == nil { t.Errorf("expected failure") @@ -557,7 +550,7 @@ func TestEnsureRuleErrorCreating(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := newInternal(fexec, ProtocolIPv4, "", "") + runner := newInternal(fexec, ProtocolIPv4) _, err := runner.EnsureRule(Append, TableNAT, ChainOutput, "abc", "123") if err == nil { t.Errorf("expected failure") @@ -582,7 +575,7 @@ func TestDeleteRuleDoesNotExist(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := newInternal(fexec, ProtocolIPv4, "", "") + runner := newInternal(fexec, ProtocolIPv4) err := runner.DeleteRule(TableNAT, ChainOutput, "abc", "123") if err != nil { t.Errorf("expected success, got %v", err) @@ -613,7 +606,7 @@ func TestDeleteRuleExists(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := newInternal(fexec, ProtocolIPv4, "", "") + runner := newInternal(fexec, ProtocolIPv4) err := runner.DeleteRule(TableNAT, ChainOutput, "abc", "123") if err != nil { t.Errorf("expected success, got %v", err) @@ -641,7 +634,7 @@ func TestDeleteRuleErrorChecking(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := newInternal(fexec, ProtocolIPv4, "", "") + runner := newInternal(fexec, ProtocolIPv4) err := runner.DeleteRule(TableNAT, ChainOutput, "abc", "123") if err == nil { t.Errorf("expected failure") @@ -669,7 +662,7 @@ func TestDeleteRuleErrorDeleting(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := newInternal(fexec, ProtocolIPv4, "", "") + runner := newInternal(fexec, ProtocolIPv4) err := runner.DeleteRule(TableNAT, ChainOutput, "abc", "123") if err == nil { t.Errorf("expected failure") @@ -740,7 +733,7 @@ COMMIT func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := newInternal(fexec, protocol, "", "") + runner := newInternal(fexec, protocol) buffer := bytes.NewBuffer(nil) // Success. @@ -808,7 +801,7 @@ func testRestore(t *testing.T, protocol Protocol) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := newInternal(fexec, protocol, "", "") + runner := newInternal(fexec, protocol) // both flags true err := runner.Restore(TableNAT, []byte{}, FlushTables, RestoreCounters) @@ -890,8 +883,7 @@ func TestRestoreAll(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - lockPath14x, lockPath16x := getLockPaths() - runner := newInternal(fexec, ProtocolIPv4, lockPath14x, lockPath16x) + runner := newInternal(fexec, ProtocolIPv4) err := runner.RestoreAll([]byte{}, NoFlushTables, RestoreCounters) if err != nil { diff --git a/pkg/util/iptables/monitor_test.go b/pkg/util/iptables/monitor_test.go index 52fc1e5fea5..8de2631fd4b 100644 --- a/pkg/util/iptables/monitor_test.go +++ b/pkg/util/iptables/monitor_test.go @@ -190,7 +190,7 @@ func (mfc *monitorFakeCmd) Stop() { func TestIPTablesMonitor(t *testing.T) { mfe := newMonitorFakeExec() - ipt := newInternal(mfe, ProtocolIPv4, "", "") + ipt := newInternal(mfe, ProtocolIPv4) var reloads uint32 stopCh := make(chan struct{}) From 6ee779e962a6b4b0de19fe5bec499b9a0c7051bb Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Tue, 12 May 2026 09:36:51 -0400 Subject: [PATCH 5/8] Assume iptables has --random-fully (This became available in the same release as iptables-restore -w, so we're already assuming it.) --- pkg/proxy/iptables/proxier.go | 15 +---- pkg/proxy/iptables/proxier_test.go | 55 ++++++------------ pkg/proxy/ipvs/proxier.go | 9 +-- pkg/proxy/ipvs/proxier_test.go | 35 ------------ pkg/util/iptables/iptables.go | 24 ++------ pkg/util/iptables/iptables_test.go | 89 ------------------------------ pkg/util/iptables/testing/fake.go | 14 +---- 7 files changed, 27 insertions(+), 214 deletions(-) diff --git a/pkg/proxy/iptables/proxier.go b/pkg/proxy/iptables/proxier.go index a7e963cf44f..e403c570323 100644 --- a/pkg/proxy/iptables/proxier.go +++ b/pkg/proxy/iptables/proxier.go @@ -307,12 +307,6 @@ func NewProxier(ctx context.Context, go ipt.Monitor(kubeProxyCanaryChain, []utiliptables.Table{utiliptables.TableMangle, utiliptables.TableNAT, utiliptables.TableFilter}, proxier.forceSyncProxyRules, syncPeriod, wait.NeverStop) - if ipt.HasRandomFully() { - logger.V(2).Info("Iptables supports --random-fully") - } else { - logger.V(2).Info("Iptables does not support --random-fully") - } - return proxier, nil } @@ -754,15 +748,12 @@ func (proxier *Proxier) syncProxyRules() (retryError error) { "-A", string(kubePostroutingChain), "-j", "MARK", "--xor-mark", proxier.masqueradeMark, ) - masqRule := []string{ + proxier.natRules.Write( "-A", string(kubePostroutingChain), "-m", "comment", "--comment", `"kubernetes service traffic requiring SNAT"`, "-j", "MASQUERADE", - } - if proxier.iptables.HasRandomFully() { - masqRule = append(masqRule, "--random-fully") - } - proxier.natRules.Write(masqRule) + "--random-fully", + ) // Install the kubernetes-specific masquerade mark rule. We use a whole chain for // this so that it is easier to flush and change, for example if the mark diff --git a/pkg/proxy/iptables/proxier_test.go b/pkg/proxy/iptables/proxier_test.go index f0020c10d9b..88da00a55a5 100644 --- a/pkg/proxy/iptables/proxier_test.go +++ b/pkg/proxy/iptables/proxier_test.go @@ -204,7 +204,7 @@ func TestParseIPTablesData(t *testing.T) { :KUBE-SEP-SXIVWICOYRO3J4NJ - [0:0] -A KUBE-POSTROUTING -m mark ! --mark 0x4000/0x4000 -j RETURN -A KUBE-POSTROUTING -j MARK --xor-mark 0x4000 - -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE + -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE --random-fully -A KUBE-MARK-MASQ -j MARK --or-mark 0x4000 -A KUBE-SERVICES -m comment --comment "ns1/svc1:p80 cluster IP" -m tcp -p tcp -d 10.20.30.41 --dport 80 -j KUBE-SVC-XPGD46QRK7WJZT7O -A KUBE-SVC-XPGD46QRK7WJZT7O -m comment --comment "ns1/svc1:p80 cluster IP" -m tcp -p tcp -d 10.20.30.41 --dport 80 ! -s 10.0.0.0/24 -j KUBE-MARK-MASQ @@ -237,7 +237,7 @@ func TestParseIPTablesData(t *testing.T) { `:KUBE-SEP-SXIVWICOYRO3J4NJ - [0:0]`, `-A KUBE-POSTROUTING -m mark ! --mark 0x4000/0x4000 -j RETURN`, `-A KUBE-POSTROUTING -j MARK --xor-mark 0x4000`, - `-A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE`, + `-A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE --random-fully`, `-A KUBE-MARK-MASQ -j MARK --or-mark 0x4000`, `-A KUBE-SERVICES -m comment --comment "ns1/svc1:p80 cluster IP" -m tcp -p tcp -d 10.20.30.41 --dport 80 -j KUBE-SVC-XPGD46QRK7WJZT7O`, `-A KUBE-SVC-XPGD46QRK7WJZT7O -m comment --comment "ns1/svc1:p80 cluster IP" -m tcp -p tcp -d 10.20.30.41 --dport 80 ! -s 10.0.0.0/24 -j KUBE-MARK-MASQ`, @@ -780,7 +780,7 @@ func TestSortIPTablesRules(t *testing.T) { :KUBE-SEP-C6EBXVWJJZMIWKLZ - [0:0] -A KUBE-POSTROUTING -m mark ! --mark 0x4000/0x4000 -j RETURN -A KUBE-POSTROUTING -j MARK --xor-mark 0x4000 - -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE + -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE --random-fully -A KUBE-MARK-MASQ -j MARK --or-mark 0x4000 -A KUBE-SERVICES -m comment --comment "ns1/svc1:p80 cluster IP" -m tcp -p tcp -d 172.30.0.41 --dport 80 -j KUBE-SVC-XPGD46QRK7WJZT7O -A KUBE-SVC-XPGD46QRK7WJZT7O -m comment --comment "ns1/svc1:p80 cluster IP" -m tcp -p tcp -d 172.30.0.41 --dport 80 ! -s 10.0.0.0/8 -j KUBE-MARK-MASQ @@ -876,7 +876,7 @@ func TestSortIPTablesRules(t *testing.T) { -A KUBE-MARK-MASQ -j MARK --or-mark 0x4000 -A KUBE-POSTROUTING -m mark ! --mark 0x4000/0x4000 -j RETURN -A KUBE-POSTROUTING -j MARK --xor-mark 0x4000 - -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE + -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE --random-fully -A KUBE-SEP-C6EBXVWJJZMIWKLZ -m comment --comment ns4/svc4:p80 -s 10.180.0.5 -j KUBE-MARK-MASQ -A KUBE-SEP-C6EBXVWJJZMIWKLZ -m comment --comment ns4/svc4:p80 -m tcp -p tcp -j DNAT --to-destination 10.180.0.5:80 -A KUBE-SEP-OYPFS5VJICHGATKP -m comment --comment ns3/svc3:p80 -s 10.180.0.3 -j KUBE-MARK-MASQ @@ -1427,7 +1427,7 @@ func TestTracePacket(t *testing.T) { -A POSTROUTING -m comment --comment kubernetes postrouting rules -j KUBE-POSTROUTING -A KUBE-POSTROUTING -m mark ! --mark 0x4000/0x4000 -j RETURN -A KUBE-POSTROUTING -j MARK --xor-mark 0x4000 - -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE + -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE --random-fully -A KUBE-MARK-MASQ -j MARK --or-mark 0x4000 -A KUBE-NODEPORTS -m comment --comment ns2/svc2:p80 -m tcp -p tcp --dport 3001 -j KUBE-EXT-GNZBNJ2PO5MGZ6GT -A KUBE-NODEPORTS -m comment --comment ns3/svc3:p80 -m tcp -p tcp --dport 3003 -j KUBE-EXT-X27LE4BHSL4DOUIK @@ -1775,7 +1775,7 @@ func TestOverallIPTablesRules(t *testing.T) { -A KUBE-MARK-MASQ -j MARK --or-mark 0x4000 -A KUBE-POSTROUTING -m mark ! --mark 0x4000/0x4000 -j RETURN -A KUBE-POSTROUTING -j MARK --xor-mark 0x4000 - -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE + -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE --random-fully -A KUBE-SEP-C6EBXVWJJZMIWKLZ -m comment --comment ns4/svc4:p80 -s 10.180.0.5 -j KUBE-MARK-MASQ -A KUBE-SEP-C6EBXVWJJZMIWKLZ -m comment --comment ns4/svc4:p80 -m tcp -p tcp -j DNAT --to-destination 10.180.0.5:80 -A KUBE-SEP-I77PXRDZVX7PMWMN -m comment --comment ns5/svc5:p80 -s 10.180.0.3 -j KUBE-MARK-MASQ @@ -2595,29 +2595,6 @@ func TestDropInvalidRule(t *testing.T) { } } -func TestMasqueradeRule(t *testing.T) { - for _, randomFully := range []bool{false, true} { - t.Run(fmt.Sprintf("randomFully %t", randomFully), func(t *testing.T) { - ipt := iptablestest.NewFake().SetHasRandomFully(randomFully) - fp := NewFakeProxier(ipt) - fp.syncProxyRules() - - expectedFmt := dedent.Dedent(` - -A KUBE-POSTROUTING -m mark ! --mark 0x4000/0x4000 -j RETURN - -A KUBE-POSTROUTING -j MARK --xor-mark 0x4000 - -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE%s - `) - var expected string - if randomFully { - expected = fmt.Sprintf(expectedFmt, " --random-fully") - } else { - expected = fmt.Sprintf(expectedFmt, "") - } - assertIPTablesChainEqual(t, getLine(), utiliptables.TableNAT, kubePostroutingChain, expected, fp.iptablesData.String()) - }) - } -} - // TestExternalTrafficPolicyLocal tests that traffic to externally-facing IPs does not get // masqueraded when using Local traffic policy. For traffic from external sources, that // means it can also only be routed to local endpoints, but for traffic from internal @@ -5823,7 +5800,7 @@ func TestSyncProxyRulesRepeated(t *testing.T) { -A KUBE-MARK-MASQ -j MARK --or-mark 0x4000 -A KUBE-POSTROUTING -m mark ! --mark 0x4000/0x4000 -j RETURN -A KUBE-POSTROUTING -j MARK --xor-mark 0x4000 - -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE + -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE --random-fully -A KUBE-SEP-SNQ3ZNILQDEJNDQO -m comment --comment ns1/svc1:p80 -s 10.0.1.1 -j KUBE-MARK-MASQ -A KUBE-SEP-SNQ3ZNILQDEJNDQO -m comment --comment ns1/svc1:p80 -m tcp -p tcp -j DNAT --to-destination 10.0.1.1:80 -A KUBE-SEP-UHEGFW77JX3KXTOV -m comment --comment ns2/svc2:p8080 -s 10.0.2.1 -j KUBE-MARK-MASQ @@ -5905,7 +5882,7 @@ func TestSyncProxyRulesRepeated(t *testing.T) { -A KUBE-MARK-MASQ -j MARK --or-mark 0x4000 -A KUBE-POSTROUTING -m mark ! --mark 0x4000/0x4000 -j RETURN -A KUBE-POSTROUTING -j MARK --xor-mark 0x4000 - -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE + -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE --random-fully -A KUBE-SEP-BSWRHOQ77KEXZLNL -m comment --comment ns3/svc3:p80 -s 10.0.3.1 -j KUBE-MARK-MASQ -A KUBE-SEP-BSWRHOQ77KEXZLNL -m comment --comment ns3/svc3:p80 -m tcp -p tcp -j DNAT --to-destination 10.0.3.1:80 -A KUBE-SVC-X27LE4BHSL4DOUIK -m comment --comment "ns3/svc3:p80 cluster IP" -m tcp -p tcp -d 172.30.0.43 --dport 80 ! -s 10.0.0.0/8 -j KUBE-MARK-MASQ @@ -5958,7 +5935,7 @@ func TestSyncProxyRulesRepeated(t *testing.T) { -A KUBE-MARK-MASQ -j MARK --or-mark 0x4000 -A KUBE-POSTROUTING -m mark ! --mark 0x4000/0x4000 -j RETURN -A KUBE-POSTROUTING -j MARK --xor-mark 0x4000 - -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE + -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE --random-fully -X KUBE-SEP-UHEGFW77JX3KXTOV -X KUBE-SVC-2VJB64SDSIJUP5T6 COMMIT @@ -6020,7 +5997,7 @@ func TestSyncProxyRulesRepeated(t *testing.T) { -A KUBE-MARK-MASQ -j MARK --or-mark 0x4000 -A KUBE-POSTROUTING -m mark ! --mark 0x4000/0x4000 -j RETURN -A KUBE-POSTROUTING -j MARK --xor-mark 0x4000 - -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE + -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE --random-fully COMMIT `) assertIPTablesRulesEqual(t, getLine(), false, expected, fp.iptablesData.String()) @@ -6079,7 +6056,7 @@ func TestSyncProxyRulesRepeated(t *testing.T) { -A KUBE-MARK-MASQ -j MARK --or-mark 0x4000 -A KUBE-POSTROUTING -m mark ! --mark 0x4000/0x4000 -j RETURN -A KUBE-POSTROUTING -j MARK --xor-mark 0x4000 - -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE + -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE --random-fully -A KUBE-SEP-AYCN5HPXMIRJNJXU -m comment --comment ns4/svc4:p80 -s 10.0.4.1 -j KUBE-MARK-MASQ -A KUBE-SEP-AYCN5HPXMIRJNJXU -m comment --comment ns4/svc4:p80 -m tcp -p tcp -j DNAT --to-destination 10.0.4.1:80 -A KUBE-SVC-4SW47YFZTEDKD3PK -m comment --comment "ns4/svc4:p80 cluster IP" -m tcp -p tcp -d 172.30.0.44 --dport 80 ! -s 10.0.0.0/8 -j KUBE-MARK-MASQ @@ -6137,7 +6114,7 @@ func TestSyncProxyRulesRepeated(t *testing.T) { -A KUBE-MARK-MASQ -j MARK --or-mark 0x4000 -A KUBE-POSTROUTING -m mark ! --mark 0x4000/0x4000 -j RETURN -A KUBE-POSTROUTING -j MARK --xor-mark 0x4000 - -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE + -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE --random-fully -A KUBE-SEP-DKCFIS26GWF2WLWC -m comment --comment ns3/svc3:p80 -s 10.0.3.2 -j KUBE-MARK-MASQ -A KUBE-SEP-DKCFIS26GWF2WLWC -m comment --comment ns3/svc3:p80 -m tcp -p tcp -j DNAT --to-destination 10.0.3.2:80 -A KUBE-SVC-X27LE4BHSL4DOUIK -m comment --comment "ns3/svc3:p80 cluster IP" -m tcp -p tcp -d 172.30.0.43 --dport 80 ! -s 10.0.0.0/8 -j KUBE-MARK-MASQ @@ -6193,7 +6170,7 @@ func TestSyncProxyRulesRepeated(t *testing.T) { -A KUBE-MARK-MASQ -j MARK --or-mark 0x4000 -A KUBE-POSTROUTING -m mark ! --mark 0x4000/0x4000 -j RETURN -A KUBE-POSTROUTING -j MARK --xor-mark 0x4000 - -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE + -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE --random-fully -A KUBE-SEP-DKCFIS26GWF2WLWC -m comment --comment ns3/svc3:p80 -s 10.0.3.2 -j KUBE-MARK-MASQ -A KUBE-SEP-DKCFIS26GWF2WLWC -m comment --comment ns3/svc3:p80 -m tcp -p tcp -j DNAT --to-destination 10.0.3.2:80 -A KUBE-SEP-JVVZVJ7BSEPPRNBS -m comment --comment ns3/svc3:p80 -s 10.0.3.3 -j KUBE-MARK-MASQ @@ -6248,7 +6225,7 @@ func TestSyncProxyRulesRepeated(t *testing.T) { -A KUBE-MARK-MASQ -j MARK --or-mark 0x4000 -A KUBE-POSTROUTING -m mark ! --mark 0x4000/0x4000 -j RETURN -A KUBE-POSTROUTING -j MARK --xor-mark 0x4000 - -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE + -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE --random-fully COMMIT `) assertIPTablesRulesEqual(t, getLine(), false, expected, fp.iptablesData.String()) @@ -6345,7 +6322,7 @@ func TestSyncProxyRulesRepeated(t *testing.T) { -A KUBE-MARK-MASQ -j MARK --or-mark 0x4000 -A KUBE-POSTROUTING -m mark ! --mark 0x4000/0x4000 -j RETURN -A KUBE-POSTROUTING -j MARK --xor-mark 0x4000 - -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE + -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE --random-fully -A KUBE-SEP-DKCFIS26GWF2WLWC -m comment --comment ns3/svc3:p80 -s 10.0.3.2 -j KUBE-MARK-MASQ -A KUBE-SEP-DKCFIS26GWF2WLWC -m comment --comment ns3/svc3:p80 -m tcp -p tcp -j DNAT --to-destination 10.0.3.2:80 -A KUBE-SEP-JVVZVJ7BSEPPRNBS -m comment --comment ns3/svc3:p80 -s 10.0.3.3 -j KUBE-MARK-MASQ @@ -6701,7 +6678,7 @@ func TestBadIPs(t *testing.T) { -A KUBE-MARK-MASQ -j MARK --or-mark 0x4000 -A KUBE-POSTROUTING -m mark ! --mark 0x4000/0x4000 -j RETURN -A KUBE-POSTROUTING -j MARK --xor-mark 0x4000 - -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE + -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE --random-fully -A KUBE-SEP-SXIVWICOYRO3J4NJ -m comment --comment ns1/svc1:p80 -s 10.180.0.1 -j KUBE-MARK-MASQ -A KUBE-SEP-SXIVWICOYRO3J4NJ -m comment --comment ns1/svc1:p80 -m tcp -p tcp -j DNAT --to-destination 10.180.0.1:80 -A KUBE-SVC-XPGD46QRK7WJZT7O -m comment --comment "ns1/svc1:p80 cluster IP" -m tcp -p tcp -d 172.30.0.41 --dport 80 ! -s 10.0.0.0/8 -j KUBE-MARK-MASQ diff --git a/pkg/proxy/ipvs/proxier.go b/pkg/proxy/ipvs/proxier.go index 04bf838ea01..6d746b7d463 100644 --- a/pkg/proxy/ipvs/proxier.go +++ b/pkg/proxy/ipvs/proxier.go @@ -1468,15 +1468,12 @@ func (proxier *Proxier) writeIptablesRules() { // XOR proxier.masqueradeMark to unset it "-j", "MARK", "--xor-mark", proxier.masqueradeMark, ) - masqRule := []string{ + proxier.natRules.Write( "-A", string(kubePostroutingChain), "-m", "comment", "--comment", `"kubernetes service traffic requiring SNAT"`, "-j", "MASQUERADE", - } - if proxier.iptables.HasRandomFully() { - masqRule = append(masqRule, "--random-fully") - } - proxier.natRules.Write(masqRule) + "--random-fully", + ) // Install the kubernetes-specific masquerade mark rule. We use a whole chain for // this so that it is easier to flush and change, for example if the mark diff --git a/pkg/proxy/ipvs/proxier_test.go b/pkg/proxy/ipvs/proxier_test.go index a27bf1f89b0..f2154065c99 100644 --- a/pkg/proxy/ipvs/proxier_test.go +++ b/pkg/proxy/ipvs/proxier_test.go @@ -1563,41 +1563,6 @@ func TestIPv6Proxier(t *testing.T) { } } -func TestMasqueradeRule(t *testing.T) { - for _, testcase := range []bool{false, true} { - _, ctx := ktesting.NewTestContext(t) - ipt := iptablestest.NewFake().SetHasRandomFully(testcase) - ipvs := ipvstest.NewFake() - ipset := ipsettest.NewFake(testIPSetVersion) - fp := NewFakeProxier(ctx, ipt, ipvs, ipset, nil, nil, v1.IPv4Protocol) - makeServiceMap(fp) - fp.syncProxyRules() - - buf := bytes.NewBuffer(nil) - _ = ipt.SaveInto(utiliptables.TableNAT, buf) - natRules := strings.Split(buf.String(), "\n") - var hasMasqueradeJump, hasMasqRandomFully bool - for _, line := range natRules { - rule, _ := iptablestest.ParseRule(line, false) - if rule != nil && rule.Chain == kubePostroutingChain && rule.Jump != nil && rule.Jump.Value == "MASQUERADE" { - hasMasqueradeJump = true - if rule.RandomFully != nil { - hasMasqRandomFully = true - } - break - } - } - - if !hasMasqueradeJump { - t.Errorf("Failed to find -j MASQUERADE in %s chain", kubePostroutingChain) - } - if hasMasqRandomFully != testcase { - probs := map[bool]string{false: "found", true: "did not find"} - t.Errorf("%s --random-fully in -j MASQUERADE rule in %s chain for HasRandomFully()=%v", probs[testcase], kubePostroutingChain, testcase) - } - } -} - func TestExternalIPsNoEndpoint(t *testing.T) { _, ctx := ktesting.NewTestContext(t) ipt := iptablestest.NewFake() diff --git a/pkg/util/iptables/iptables.go b/pkg/util/iptables/iptables.go index 8620c84e2bd..7ca335dad15 100644 --- a/pkg/util/iptables/iptables.go +++ b/pkg/util/iptables/iptables.go @@ -87,12 +87,6 @@ type Interface interface { // a reload) it will log an error and stop monitoring. // (This function should be called from a goroutine.) Monitor(canary Chain, tables []Table, reloadFunc func(), interval time.Duration, stopCh <-chan struct{}) - // HasRandomFully reveals whether `-j MASQUERADE` takes the - // `--random-fully` option. This is helpful to work around a - // Linux kernel bug that sometimes causes multiple flows to get - // mapped to the same IP:PORT and consequently some suffer packet - // drops. - HasRandomFully() bool // Present checks if the kernel supports the iptable interface Present() error @@ -163,10 +157,6 @@ const FlushTables FlushFlag = true // NoFlushTables a boolean false constant for option flag FlushFlag const NoFlushTables FlushFlag = false -// RandomFullyMinVersion is the minimum version from which the --random-fully flag is supported, -// used for port mapping to be fully randomized -var RandomFullyMinVersion = utilversion.MustParseGeneric("1.6.2") - // WaitString a constant for specifying the wait flag const WaitString = "-w" @@ -175,10 +165,9 @@ const WaitSecondsValue = "5" // runner implements Interface in terms of exec("iptables"). type runner struct { - mu sync.Mutex - exec utilexec.Interface - protocol Protocol - hasRandomFully bool + mu sync.Mutex + exec utilexec.Interface + protocol Protocol } // newInternal returns a new Interface which will exec iptables @@ -188,7 +177,7 @@ func newInternal(exec utilexec.Interface, protocol Protocol) Interface { protocol: protocol, } - version, err := getIPTablesVersion(exec, protocol) + _, err := getIPTablesVersion(exec, protocol) if err != nil { // The only likely error is "no such file or directory", in which case any // further commands will fail the same way, so we don't need to do @@ -196,7 +185,6 @@ func newInternal(exec utilexec.Interface, protocol Protocol) Interface { return runner } - runner.hasRandomFully = version.AtLeast(RandomFullyMinVersion) return runner } @@ -567,10 +555,6 @@ func getIPTablesVersion(exec utilexec.Interface, protocol Protocol) (*utilversio return version, nil } -func (runner *runner) HasRandomFully() bool { - return runner.hasRandomFully -} - // Present tests if iptable is supported on current kernel by checking the existence // of default table and chain func (runner *runner) Present() error { diff --git a/pkg/util/iptables/iptables_test.go b/pkg/util/iptables/iptables_test.go index 7c03950637e..be659472419 100644 --- a/pkg/util/iptables/iptables_test.go +++ b/pkg/util/iptables/iptables_test.go @@ -84,95 +84,6 @@ func TestFakeExecForCommands(t *testing.T) { } } -func TestNew(t *testing.T) { - testCases := []struct { - name string - commands []testCommand - expected *runner - }{ - { - name: "ancient", - commands: []testCommand{ - { - command: "iptables --version", - action: func() ([]byte, []byte, error) { return []byte("iptables v1.4.0"), nil, nil }, - }, - { - // iptables-restore version check: ignores --version and just no-ops - command: "iptables-restore --version", - action: func() ([]byte, []byte, error) { return nil, nil, nil }, - }, - }, - expected: &runner{ - hasRandomFully: false, - }, - }, - { - name: "RHEL/CentOS 7", - commands: []testCommand{ - { - command: "iptables --version", - action: func() ([]byte, []byte, error) { return []byte("iptables v1.4.21"), nil, nil }, - }, - { - command: "iptables-restore --version", - action: func() ([]byte, []byte, error) { return []byte("iptables-restore v1.4.21"), nil, nil }, - }, - }, - expected: &runner{ - hasRandomFully: false, - }, - }, - { - name: "1.6", - commands: []testCommand{ - { - command: "iptables --version", - action: func() ([]byte, []byte, error) { return []byte("iptables v1.6.2"), nil, nil }, - }, - }, - expected: &runner{ - hasRandomFully: true, - }, - }, - { - name: "1.8", - commands: []testCommand{ - { - command: "iptables --version", - action: func() ([]byte, []byte, error) { return []byte("iptables v1.8.11"), nil, nil }, - }, - }, - expected: &runner{ - hasRandomFully: true, - }, - }, - { - name: "no iptables", - commands: []testCommand{ - { - command: "iptables --version", - action: func() ([]byte, []byte, error) { return nil, nil, fmt.Errorf("no such file or directory") }, - }, - }, - expected: &runner{ - hasRandomFully: false, - }, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - fexec := fakeExecForCommands(tc.commands) - runner := newInternal(fexec, ProtocolIPv4).(*runner) - - if runner.hasRandomFully != tc.expected.hasRandomFully { - t.Errorf("Expected hasRandomFully=%v, got %v", tc.expected.hasRandomFully, runner.hasRandomFully) - } - }) - } -} - func TestNewDualStack(t *testing.T) { testCases := []struct { name string diff --git a/pkg/util/iptables/testing/fake.go b/pkg/util/iptables/testing/fake.go index 78c0c2070ec..685ca228aa0 100644 --- a/pkg/util/iptables/testing/fake.go +++ b/pkg/util/iptables/testing/fake.go @@ -30,8 +30,7 @@ import ( // FakeIPTables is no-op implementation of iptables Interface. type FakeIPTables struct { - hasRandomFully bool - protocol iptables.Protocol + protocol iptables.Protocol Dump *IPTablesDump } @@ -77,12 +76,6 @@ func NewIPv6Fake() *FakeIPTables { return f } -// SetHasRandomFully sets f's return value for HasRandomFully() -func (f *FakeIPTables) SetHasRandomFully(can bool) *FakeIPTables { - f.hasRandomFully = can - return f -} - // EnsureChain is part of iptables.Interface func (f *FakeIPTables) EnsureChain(table iptables.Table, chain iptables.Chain) (bool, error) { t, err := f.Dump.GetTable(table) @@ -321,11 +314,6 @@ func (f *FakeIPTables) RestoreAll(data []byte, flush iptables.FlushFlag, counter func (f *FakeIPTables) Monitor(canary iptables.Chain, tables []iptables.Table, reloadFunc func(), interval time.Duration, stopCh <-chan struct{}) { } -// HasRandomFully is part of iptables.Interface -func (f *FakeIPTables) HasRandomFully() bool { - return f.hasRandomFully -} - func (f *FakeIPTables) Present() error { return nil } From 2f4b6ecdca0318d9b70b02a968aca4949ab3350d Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Thu, 21 May 2026 16:58:10 -0400 Subject: [PATCH 6/8] Normalize/fix output checking in utiliptables unit tests Use a `numCalls` variable in each rather than repeatedly hardcoding constants. (A few of the tests here had an off-by-one in the error message part before, meaning if the output didn't match the expected output, it would have panicked with an out-of-bounds access rather than failing with a proper error message.) --- pkg/util/iptables/iptables_test.go | 133 ++++++++++++++++------------- 1 file changed, 76 insertions(+), 57 deletions(-) diff --git a/pkg/util/iptables/iptables_test.go b/pkg/util/iptables/iptables_test.go index be659472419..c0d46a7595f 100644 --- a/pkg/util/iptables/iptables_test.go +++ b/pkg/util/iptables/iptables_test.go @@ -249,12 +249,13 @@ func testEnsureChain(t *testing.T, protocol Protocol) { if exists { t.Errorf("%s new chain: Expected exists = false", protocol) } - if fcmd.CombinedOutputCalls != 2 { - t.Errorf("%s new chain: Expected 2 CombinedOutput() calls, got %d", protocol, fcmd.CombinedOutputCalls) + numCalls := len(fcmd.CombinedOutputScript) - 2 // we haven't used all of fcmd.CombinedOutputScript yet + if fcmd.CombinedOutputCalls != numCalls { + t.Errorf("%s new chain: Expected %d CombinedOutput() calls, got %d", protocol, numCalls, fcmd.CombinedOutputCalls) } cmd := iptablesCommand(protocol) - if !sets.New(fcmd.CombinedOutputLog[1]...).HasAll(cmd, "-t", "nat", "-N", "FOOBAR") { - t.Errorf("%s new chain: Expected cmd containing '%s -t nat -N FOOBAR', got %s", protocol, cmd, fcmd.CombinedOutputLog[2]) + if !sets.New(fcmd.CombinedOutputLog[numCalls-1]...).HasAll(cmd, "-t", "nat", "-N", "FOOBAR") { + t.Errorf("%s new chain: Expected cmd containing '%s -t nat -N FOOBAR', got %s", protocol, cmd, fcmd.CombinedOutputLog[numCalls-1]) } // Exists. exists, err = runner.EnsureChain(TableNAT, Chain("FOOBAR")) @@ -303,11 +304,12 @@ func TestFlushChain(t *testing.T) { if err != nil { t.Errorf("expected success, got %v", err) } - if fcmd.CombinedOutputCalls != 2 { - t.Errorf("expected 2 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) + numCalls := len(fcmd.CombinedOutputScript) - 1 // we haven't used all of fcmd.CombinedOutputScript yet + if fcmd.CombinedOutputCalls != numCalls { + t.Errorf("expected %d CombinedOutput() calls, got %d", numCalls, fcmd.CombinedOutputCalls) } - if !sets.New(fcmd.CombinedOutputLog[1]...).HasAll("iptables", "-t", "nat", "-F", "FOOBAR") { - t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[2]) + if !sets.New(fcmd.CombinedOutputLog[numCalls-1]...).HasAll("iptables", "-t", "nat", "-F", "FOOBAR") { + t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[numCalls-1]) } // Failure. err = runner.FlushChain(TableNAT, Chain("FOOBAR")) @@ -340,11 +342,12 @@ func TestDeleteChain(t *testing.T) { if err != nil { t.Errorf("expected success, got %v", err) } - if fcmd.CombinedOutputCalls != 2 { - t.Errorf("expected 2 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) + numCalls := len(fcmd.CombinedOutputScript) - 1 // we haven't used all of fcmd.CombinedOutputScript yet + if fcmd.CombinedOutputCalls != numCalls { + t.Errorf("expected %d CombinedOutput() calls, got %d", numCalls, fcmd.CombinedOutputCalls) } - if !sets.New(fcmd.CombinedOutputLog[1]...).HasAll("iptables", "-t", "nat", "-X", "FOOBAR") { - t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[2]) + if !sets.New(fcmd.CombinedOutputLog[numCalls-1]...).HasAll("iptables", "-t", "nat", "-X", "FOOBAR") { + t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[numCalls-1]) } // Failure. err = runner.DeleteChain(TableNAT, Chain("FOOBAR")) @@ -376,11 +379,12 @@ func TestEnsureRuleAlreadyExists(t *testing.T) { if !exists { t.Errorf("expected exists = true") } - if fcmd.CombinedOutputCalls != 2 { - t.Errorf("expected 2 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) + numCalls := len(fcmd.CombinedOutputScript) + if fcmd.CombinedOutputCalls != numCalls { + t.Errorf("expected %d CombinedOutput() calls, got %d", numCalls, fcmd.CombinedOutputCalls) } - if !sets.New(fcmd.CombinedOutputLog[1]...).HasAll("iptables", "-t", "nat", "-C", "OUTPUT", "abc", "123") { - t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[2]) + if !sets.New(fcmd.CombinedOutputLog[numCalls-1]...).HasAll("iptables", "-t", "nat", "-C", "OUTPUT", "abc", "123") { + t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[numCalls-1]) } } @@ -410,11 +414,12 @@ func TestEnsureRuleNew(t *testing.T) { if exists { t.Errorf("expected exists = false") } - if fcmd.CombinedOutputCalls != 3 { - t.Errorf("expected 3 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) + numCalls := len(fcmd.CombinedOutputScript) + if fcmd.CombinedOutputCalls != numCalls { + t.Errorf("expected %d CombinedOutput() calls, got %d", numCalls, fcmd.CombinedOutputCalls) } - if !sets.New(fcmd.CombinedOutputLog[2]...).HasAll("iptables", "-t", "nat", "-A", "OUTPUT", "abc", "123") { - t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[3]) + if !sets.New(fcmd.CombinedOutputLog[numCalls-1]...).HasAll("iptables", "-t", "nat", "-A", "OUTPUT", "abc", "123") { + t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[numCalls-1]) } } @@ -438,8 +443,9 @@ func TestEnsureRuleErrorChecking(t *testing.T) { if err == nil { t.Errorf("expected failure") } - if fcmd.CombinedOutputCalls != 2 { - t.Errorf("expected 2 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) + numCalls := len(fcmd.CombinedOutputScript) + if fcmd.CombinedOutputCalls != numCalls { + t.Errorf("expected %d CombinedOutput() calls, got %d", numCalls, fcmd.CombinedOutputCalls) } } @@ -466,8 +472,9 @@ func TestEnsureRuleErrorCreating(t *testing.T) { if err == nil { t.Errorf("expected failure") } - if fcmd.CombinedOutputCalls != 3 { - t.Errorf("expected 3 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) + numCalls := len(fcmd.CombinedOutputScript) + if fcmd.CombinedOutputCalls != numCalls { + t.Errorf("expected %d CombinedOutput() calls, got %d", numCalls, fcmd.CombinedOutputCalls) } } @@ -491,11 +498,12 @@ func TestDeleteRuleDoesNotExist(t *testing.T) { if err != nil { t.Errorf("expected success, got %v", err) } - if fcmd.CombinedOutputCalls != 2 { - t.Errorf("expected 2 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) + numCalls := len(fcmd.CombinedOutputScript) + if fcmd.CombinedOutputCalls != numCalls { + t.Errorf("expected %d CombinedOutput() calls, got %d", numCalls, fcmd.CombinedOutputCalls) } - if !sets.New(fcmd.CombinedOutputLog[1]...).HasAll("iptables", "-t", "nat", "-C", "OUTPUT", "abc", "123") { - t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[2]) + if !sets.New(fcmd.CombinedOutputLog[numCalls-1]...).HasAll("iptables", "-t", "nat", "-C", "OUTPUT", "abc", "123") { + t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[numCalls-1]) } } @@ -522,11 +530,12 @@ func TestDeleteRuleExists(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) + numCalls := len(fcmd.CombinedOutputScript) + if fcmd.CombinedOutputCalls != numCalls { + t.Errorf("expected %d CombinedOutput() calls, got %d", numCalls, fcmd.CombinedOutputCalls) } - if !sets.New(fcmd.CombinedOutputLog[2]...).HasAll("iptables", "-t", "nat", "-D", "OUTPUT", "abc", "123") { - t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[3]) + if !sets.New(fcmd.CombinedOutputLog[numCalls-1]...).HasAll("iptables", "-t", "nat", "-D", "OUTPUT", "abc", "123") { + t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[numCalls-1]) } } @@ -550,8 +559,9 @@ func TestDeleteRuleErrorChecking(t *testing.T) { if err == nil { t.Errorf("expected failure") } - if fcmd.CombinedOutputCalls != 2 { - t.Errorf("expected 2 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) + numCalls := len(fcmd.CombinedOutputScript) + if fcmd.CombinedOutputCalls != numCalls { + t.Errorf("expected %d CombinedOutput() calls, got %d", numCalls, fcmd.CombinedOutputCalls) } } @@ -578,8 +588,9 @@ func TestDeleteRuleErrorDeleting(t *testing.T) { if err == nil { t.Errorf("expected failure") } - if fcmd.CombinedOutputCalls != 3 { - t.Errorf("expected 3 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) + numCalls := len(fcmd.CombinedOutputScript) + if fcmd.CombinedOutputCalls != numCalls { + t.Errorf("expected %d CombinedOutput() calls, got %d", numCalls, fcmd.CombinedOutputCalls) } } @@ -657,14 +668,16 @@ COMMIT t.Errorf("%s: Expected output '%s', got '%v'", protocol, output, buffer.String()) } - if fcmd.CombinedOutputCalls != 1 { - t.Errorf("%s: Expected 1 CombinedOutput() calls, got %d", protocol, fcmd.CombinedOutputCalls) + numCombinedOutputCalls := 1 + if fcmd.CombinedOutputCalls != numCombinedOutputCalls { + t.Errorf("%s: Expected %d CombinedOutput() calls, got %d", protocol, numCombinedOutputCalls, fcmd.CombinedOutputCalls) } - if fcmd.RunCalls != 1 { - t.Errorf("%s: Expected 1 Run() call, got %d", protocol, fcmd.RunCalls) + numRunCalls := 1 + if fcmd.RunCalls != numRunCalls { + t.Errorf("%s: Expected %d Run() calls, got %d", protocol, numRunCalls, fcmd.RunCalls) } - if !sets.New(fcmd.RunLog[0]...).HasAll(iptablesSaveCmd, "-t", "nat") { - t.Errorf("%s: Expected cmd containing '%s -t nat', got '%s'", protocol, iptablesSaveCmd, fcmd.RunLog[0]) + if !sets.New(fcmd.RunLog[numRunCalls-1]...).HasAll(iptablesSaveCmd, "-t", "nat") { + t.Errorf("%s: Expected cmd containing '%s -t nat', got '%s'", protocol, iptablesSaveCmd, fcmd.RunLog[numRunCalls-1]) } // Failure. @@ -713,6 +726,7 @@ func testRestore(t *testing.T, protocol Protocol) { }, } runner := newInternal(fexec, protocol) + numCalls := 1 // both flags true err := runner.Restore(TableNAT, []byte{}, FlushTables, RestoreCounters) @@ -720,9 +734,10 @@ func testRestore(t *testing.T, protocol Protocol) { t.Errorf("%s flush,restore: Expected success, got %v", protocol, err) } - commandSet := sets.New(fcmd.CombinedOutputLog[1]...) + numCalls++ + commandSet := sets.New(fcmd.CombinedOutputLog[numCalls-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'", protocol, iptablesRestoreCmd, string(TableNAT), fcmd.CombinedOutputLog[1]) + t.Errorf("%s flush, restore: Expected cmd containing '%s -T %s --counters', got '%s'", protocol, iptablesRestoreCmd, string(TableNAT), fcmd.CombinedOutputLog[numCalls-1]) } // FlushTables, NoRestoreCounters @@ -730,10 +745,11 @@ func testRestore(t *testing.T, protocol Protocol) { if err != nil { t.Errorf("%s flush, no restore: Expected success, got %v", protocol, err) } + numCalls++ - commandSet = sets.New(fcmd.CombinedOutputLog[2]...) + commandSet = sets.New(fcmd.CombinedOutputLog[numCalls-1]...) 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'", protocol, fcmd.CombinedOutputLog[2]) + t.Errorf("%s flush, no restore: Expected cmd containing '--noflush' or '--counters', got '%s'", protocol, fcmd.CombinedOutputLog[numCalls-1]) } // NoFlushTables, RestoreCounters @@ -741,10 +757,11 @@ func testRestore(t *testing.T, protocol Protocol) { if err != nil { t.Errorf("%s no flush, restore: Expected success, got %v", protocol, err) } + numCalls++ - commandSet = sets.New(fcmd.CombinedOutputLog[3]...) + commandSet = sets.New(fcmd.CombinedOutputLog[numCalls-1]...) if !commandSet.HasAll(iptablesRestoreCmd, "-T", string(TableNAT), "--noflush", "--counters") { - t.Errorf("%s no flush, restore: Expected cmd containing '--noflush' and '--counters', got '%s'", protocol, fcmd.CombinedOutputLog[3]) + t.Errorf("%s no flush, restore: Expected cmd containing '--noflush' and '--counters', got '%s'", protocol, fcmd.CombinedOutputLog[numCalls-1]) } // NoFlushTables, NoRestoreCounters @@ -752,14 +769,15 @@ func testRestore(t *testing.T, protocol Protocol) { if err != nil { t.Errorf("%s no flush, no restore: Expected success, got %v", protocol, err) } + numCalls++ - commandSet = sets.New(fcmd.CombinedOutputLog[4]...) + commandSet = sets.New(fcmd.CombinedOutputLog[numCalls-1]...) 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'", protocol, iptablesRestoreCmd, string(TableNAT), fcmd.CombinedOutputLog[4]) + t.Errorf("%s no flush, no restore: Expected cmd containing '%s -T %s --noflush', got '%s'", protocol, iptablesRestoreCmd, string(TableNAT), fcmd.CombinedOutputLog[numCalls-1]) } - if fcmd.CombinedOutputCalls != 5 { - t.Errorf("%s: Expected 5 total CombinedOutput() calls, got %d", protocol, fcmd.CombinedOutputCalls) + if fcmd.CombinedOutputCalls != numCalls { + t.Errorf("%s: Expected %d total CombinedOutput() calls, got %d", protocol, numCalls, fcmd.CombinedOutputCalls) } // Failure. @@ -801,13 +819,14 @@ func TestRestoreAll(t *testing.T) { t.Fatalf("expected success, got %v", err) } - commandSet := sets.New(fcmd.CombinedOutputLog[1]...) - if !commandSet.HasAll("iptables-restore", "--counters", "--noflush") { - t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[2]) + numCalls := len(fcmd.CombinedOutputScript) - 1 + if fcmd.CombinedOutputCalls != numCalls { + t.Errorf("expected %d CombinedOutput() calls, got %d", numCalls, fcmd.CombinedOutputCalls) } - if fcmd.CombinedOutputCalls != 2 { - t.Errorf("expected 2 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) + commandSet := sets.New(fcmd.CombinedOutputLog[numCalls-1]...) + if !commandSet.HasAll("iptables-restore", "--counters", "--noflush") { + t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[numCalls-1]) } // Failure. From 04d51ea41b74f854f3f5afb2c6cae6970f9b0876 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Tue, 12 May 2026 09:36:51 -0400 Subject: [PATCH 7/8] Remove construct-time iptables version checks --- pkg/util/iptables/iptables.go | 32 ----------- pkg/util/iptables/iptables_test.go | 90 +----------------------------- 2 files changed, 2 insertions(+), 120 deletions(-) diff --git a/pkg/util/iptables/iptables.go b/pkg/util/iptables/iptables.go index 7ca335dad15..ab009ca5c1d 100644 --- a/pkg/util/iptables/iptables.go +++ b/pkg/util/iptables/iptables.go @@ -30,7 +30,6 @@ import ( "time" v1 "k8s.io/api/core/v1" - utilversion "k8s.io/apimachinery/pkg/util/version" utilwait "k8s.io/apimachinery/pkg/util/wait" "k8s.io/klog/v2" utilexec "k8s.io/utils/exec" @@ -177,14 +176,6 @@ func newInternal(exec utilexec.Interface, protocol Protocol) Interface { protocol: protocol, } - _, err := getIPTablesVersion(exec, protocol) - if err != nil { - // The only likely error is "no such file or directory", in which case any - // further commands will fail the same way, so we don't need to do - // anything special here. - return runner - } - return runner } @@ -532,29 +523,6 @@ func makeFullArgs(table Table, chain Chain, args ...string) []string { return append([]string{string(chain), "-t", string(table)}, args...) } -const iptablesVersionPattern = `v([0-9]+(\.[0-9]+)+)` - -// 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 nil, err - } - versionMatcher := regexp.MustCompile(iptablesVersionPattern) - match := versionMatcher.FindStringSubmatch(string(bytes)) - if match == nil { - 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 -} - // Present tests if iptable is supported on current kernel by checking the existence // of default table and chain func (runner *runner) Present() error { diff --git a/pkg/util/iptables/iptables_test.go b/pkg/util/iptables/iptables_test.go index c0d46a7595f..8bdfbdba4f5 100644 --- a/pkg/util/iptables/iptables_test.go +++ b/pkg/util/iptables/iptables_test.go @@ -94,21 +94,11 @@ func TestNewDualStack(t *testing.T) { { name: "both available", commands: []testCommand{ - { - // ipv4 creation - command: "iptables --version", - action: func() ([]byte, []byte, error) { return []byte("iptables v1.8.0"), nil, nil }, - }, { // ipv4 Present() command: "iptables -w 5 -S POSTROUTING -t nat", action: func() ([]byte, []byte, error) { return nil, nil, nil }, }, - { - // ipv6 creation - command: "ip6tables --version", - action: func() ([]byte, []byte, error) { return []byte("iptables v1.8.0"), nil, nil }, - }, { // ipv6 Present() command: "ip6tables -w 5 -S POSTROUTING -t nat", @@ -121,21 +111,11 @@ func TestNewDualStack(t *testing.T) { { name: "ipv4 available, ipv6 not installed", commands: []testCommand{ - { - // ipv4 creation - command: "iptables --version", - action: func() ([]byte, []byte, error) { return []byte("iptables v1.8.0"), nil, nil }, - }, { // ipv4 Present() command: "iptables -w 5 -S POSTROUTING -t nat", action: func() ([]byte, []byte, error) { return nil, nil, nil }, }, - { - // ipv6 creation - command: "ip6tables --version", - action: func() ([]byte, []byte, error) { return nil, nil, fmt.Errorf("no such file or directory") }, - }, { // ipv6 Present() command: "ip6tables -w 5 -S POSTROUTING -t nat", @@ -148,21 +128,11 @@ func TestNewDualStack(t *testing.T) { { name: "ipv4 available, ipv6 disabled", commands: []testCommand{ - { - // ipv4 creation - command: "iptables --version", - action: func() ([]byte, []byte, error) { return []byte("iptables v1.8.0"), nil, nil }, - }, { // ipv4 Present() command: "iptables -w 5 -S POSTROUTING -t nat", action: func() ([]byte, []byte, error) { return nil, nil, nil }, }, - { - // ipv6 creation - command: "ip6tables --version", - action: func() ([]byte, []byte, error) { return []byte("iptables v1.8.0"), nil, nil }, - }, { // ipv6 Present() command: "ip6tables -w 5 -S POSTROUTING -t nat", @@ -175,21 +145,11 @@ func TestNewDualStack(t *testing.T) { { name: "no iptables support", commands: []testCommand{ - { - // ipv4 creation - command: "iptables --version", - action: func() ([]byte, []byte, error) { return nil, nil, fmt.Errorf("no such file or directory") }, - }, { // ipv4 Present() command: "iptables -w 5 -S POSTROUTING -t nat", action: func() ([]byte, []byte, error) { return nil, nil, fmt.Errorf("no such file or directory") }, }, - { - // ipv6 creation - command: "ip6tables --version", - action: func() ([]byte, []byte, error) { return nil, nil, fmt.Errorf("no such file or directory") }, - }, { // ipv6 Present() command: "ip6tables -w 5 -S POSTROUTING -t nat", @@ -222,8 +182,6 @@ func TestNewDualStack(t *testing.T) { func testEnsureChain(t *testing.T, protocol Protocol) { fcmd := fakeexec.FakeCmd{ CombinedOutputScript: []fakeexec.FakeAction{ - // iptables version check - func() ([]byte, []byte, error) { return []byte("iptables v1.9.22"), nil, nil }, // Success. func() ([]byte, []byte, error) { return []byte{}, nil, nil }, // Exists. @@ -237,7 +195,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 := newInternal(fexec, protocol) @@ -283,8 +240,6 @@ func TestEnsureChainIPv6(t *testing.T) { func TestFlushChain(t *testing.T) { fcmd := fakeexec.FakeCmd{ CombinedOutputScript: []fakeexec.FakeAction{ - // iptables version check - func() ([]byte, []byte, error) { return []byte("iptables v1.9.22"), nil, nil }, // Success. func() ([]byte, []byte, error) { return []byte{}, nil, nil }, // Failure. @@ -295,7 +250,6 @@ func TestFlushChain(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 := newInternal(fexec, ProtocolIPv4) @@ -321,8 +275,6 @@ func TestFlushChain(t *testing.T) { func TestDeleteChain(t *testing.T) { fcmd := fakeexec.FakeCmd{ CombinedOutputScript: []fakeexec.FakeAction{ - // iptables version check - func() ([]byte, []byte, error) { return []byte("iptables v1.9.22"), nil, nil }, // Success. func() ([]byte, []byte, error) { return []byte{}, nil, nil }, // Failure. @@ -333,7 +285,6 @@ func TestDeleteChain(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 := newInternal(fexec, ProtocolIPv4) @@ -359,8 +310,6 @@ func TestDeleteChain(t *testing.T) { func TestEnsureRuleAlreadyExists(t *testing.T) { fcmd := fakeexec.FakeCmd{ CombinedOutputScript: []fakeexec.FakeAction{ - // iptables version check - func() ([]byte, []byte, error) { return []byte("iptables v1.9.22"), nil, nil }, // Success on the -C call, meaning the rule exists. func() ([]byte, []byte, error) { return []byte{}, nil, nil }, }, @@ -368,7 +317,6 @@ func TestEnsureRuleAlreadyExists(t *testing.T) { 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 := newInternal(fexec, ProtocolIPv4) @@ -391,8 +339,6 @@ func TestEnsureRuleAlreadyExists(t *testing.T) { func TestEnsureRuleNew(t *testing.T) { fcmd := fakeexec.FakeCmd{ CombinedOutputScript: []fakeexec.FakeAction{ - // iptables version check - func() ([]byte, []byte, error) { return []byte("iptables v1.9.22"), nil, nil }, // Status 1 on the -C call, meaning the rule doesn't exist func() ([]byte, []byte, error) { return nil, nil, &fakeexec.FakeExitError{Status: 1} }, // Success on the -A call. @@ -403,7 +349,6 @@ func TestEnsureRuleNew(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 := newInternal(fexec, ProtocolIPv4) @@ -426,8 +371,6 @@ func TestEnsureRuleNew(t *testing.T) { func TestEnsureRuleErrorChecking(t *testing.T) { fcmd := fakeexec.FakeCmd{ CombinedOutputScript: []fakeexec.FakeAction{ - // iptables version check - func() ([]byte, []byte, error) { return []byte("iptables v1.9.22"), nil, nil }, // Status 2 on the -C call, meaning something went wrong while checking. func() ([]byte, []byte, error) { return nil, nil, &fakeexec.FakeExitError{Status: 2} }, }, @@ -435,7 +378,6 @@ func TestEnsureRuleErrorChecking(t *testing.T) { 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 := newInternal(fexec, ProtocolIPv4) @@ -452,8 +394,6 @@ func TestEnsureRuleErrorChecking(t *testing.T) { func TestEnsureRuleErrorCreating(t *testing.T) { fcmd := fakeexec.FakeCmd{ CombinedOutputScript: []fakeexec.FakeAction{ - // iptables version check - func() ([]byte, []byte, error) { return []byte("iptables v1.9.22"), nil, nil }, // Status 1 on the -C call, meaning the rule doesn't exist. func() ([]byte, []byte, error) { return nil, nil, &fakeexec.FakeExitError{Status: 1} }, // Status 1 on the -A call, meaning failure adding it. @@ -464,7 +404,6 @@ func TestEnsureRuleErrorCreating(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 := newInternal(fexec, ProtocolIPv4) @@ -481,8 +420,6 @@ func TestEnsureRuleErrorCreating(t *testing.T) { func TestDeleteRuleDoesNotExist(t *testing.T) { fcmd := fakeexec.FakeCmd{ CombinedOutputScript: []fakeexec.FakeAction{ - // iptables version check - func() ([]byte, []byte, error) { return []byte("iptables v1.9.22"), nil, nil }, // Status 1 on the -C call, meaning the rule doesn't exist. func() ([]byte, []byte, error) { return nil, nil, &fakeexec.FakeExitError{Status: 1} }, }, @@ -490,7 +427,6 @@ func TestDeleteRuleDoesNotExist(t *testing.T) { 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 := newInternal(fexec, ProtocolIPv4) @@ -510,8 +446,6 @@ func TestDeleteRuleDoesNotExist(t *testing.T) { func TestDeleteRuleExists(t *testing.T) { fcmd := fakeexec.FakeCmd{ CombinedOutputScript: []fakeexec.FakeAction{ - // iptables version check - func() ([]byte, []byte, error) { return []byte("iptables v1.9.22"), nil, nil }, // Success on the -C call, meaning the rule exists. func() ([]byte, []byte, error) { return []byte{}, nil, nil }, // Success on the -D call. @@ -522,7 +456,6 @@ func TestDeleteRuleExists(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 := newInternal(fexec, ProtocolIPv4) @@ -542,8 +475,6 @@ func TestDeleteRuleExists(t *testing.T) { func TestDeleteRuleErrorChecking(t *testing.T) { fcmd := fakeexec.FakeCmd{ CombinedOutputScript: []fakeexec.FakeAction{ - // iptables version check - func() ([]byte, []byte, error) { return []byte("iptables v1.9.22"), nil, nil }, // Status 2 on the -C call, meaning something went wrong while checking. func() ([]byte, []byte, error) { return nil, nil, &fakeexec.FakeExitError{Status: 2} }, }, @@ -568,8 +499,6 @@ func TestDeleteRuleErrorChecking(t *testing.T) { func TestDeleteRuleErrorDeleting(t *testing.T) { fcmd := fakeexec.FakeCmd{ CombinedOutputScript: []fakeexec.FakeAction{ - // iptables version check - func() ([]byte, []byte, error) { return []byte("iptables v1.9.22"), nil, nil }, // Success on the -C call, meaning the rule exists. func() ([]byte, []byte, error) { return []byte{}, nil, nil }, // Status 1 on the -D call, meaning failure to delete it. @@ -580,7 +509,6 @@ func TestDeleteRuleErrorDeleting(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 := newInternal(fexec, ProtocolIPv4) @@ -625,7 +553,6 @@ func TestIPTablesCommands(t *testing.T) { func testSaveInto(t *testing.T, protocol Protocol) { version := " v1.9.22" - iptablesCmd := iptablesCommand(protocol) iptablesSaveCmd := iptablesSaveCommand(protocol) output := fmt.Sprintf(`# Generated by %s on Thu Jan 19 11:38:09 2017 @@ -639,10 +566,6 @@ COMMIT stderrOutput := "#STDERR OUTPUT" // SaveInto() should should NOT capture stderr into the buffer fcmd := fakeexec.FakeCmd{ - CombinedOutputScript: []fakeexec.FakeAction{ - // iptables version check - func() ([]byte, []byte, error) { return []byte(iptablesCmd + version), nil, nil }, - }, RunScript: []fakeexec.FakeAction{ func() ([]byte, []byte, error) { return []byte(output), []byte(stderrOutput), nil }, func() ([]byte, []byte, error) { return nil, []byte(stderrOutput), &fakeexec.FakeExitError{Status: 1} }, @@ -652,7 +575,6 @@ COMMIT 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 := newInternal(fexec, protocol) @@ -668,7 +590,7 @@ COMMIT t.Errorf("%s: Expected output '%s', got '%v'", protocol, output, buffer.String()) } - numCombinedOutputCalls := 1 + numCombinedOutputCalls := 0 if fcmd.CombinedOutputCalls != numCombinedOutputCalls { t.Errorf("%s: Expected %d CombinedOutput() calls, got %d", protocol, numCombinedOutputCalls, fcmd.CombinedOutputCalls) } @@ -700,14 +622,10 @@ func TestSaveIntoIPv6(t *testing.T) { } func testRestore(t *testing.T, protocol Protocol) { - version := " v1.9.22" - iptablesCmd := iptablesCommand(protocol) iptablesRestoreCmd := iptablesRestoreCommand(protocol) fcmd := fakeexec.FakeCmd{ CombinedOutputScript: []fakeexec.FakeAction{ - // iptables version check - func() ([]byte, []byte, error) { return []byte(iptablesCmd + version), nil, nil }, func() ([]byte, []byte, error) { return []byte{}, nil, nil }, func() ([]byte, []byte, error) { return []byte{}, nil, nil }, func() ([]byte, []byte, error) { return []byte{}, nil, nil }, @@ -722,11 +640,10 @@ 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 := newInternal(fexec, protocol) - numCalls := 1 + numCalls := 0 // both flags true err := runner.Restore(TableNAT, []byte{}, FlushTables, RestoreCounters) @@ -799,8 +716,6 @@ func TestRestoreIPv6(t *testing.T) { func TestRestoreAll(t *testing.T) { fcmd := fakeexec.FakeCmd{ CombinedOutputScript: []fakeexec.FakeAction{ - // iptables version check - func() ([]byte, []byte, error) { return []byte("iptables v1.9.22"), nil, nil }, func() ([]byte, []byte, error) { return []byte{}, nil, nil }, func() ([]byte, []byte, error) { return nil, nil, &fakeexec.FakeExitError{Status: 1} }, }, @@ -809,7 +724,6 @@ func TestRestoreAll(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 := newInternal(fexec, ProtocolIPv4) From 9d8f8d59a5fc0f4dd983c504a872c753d421b095 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Tue, 12 May 2026 10:55:08 -0400 Subject: [PATCH 8/8] Remove mutex from iptables runner This dates back to before the iptables binary itself had lock support. It serves no purpose now. --- pkg/util/iptables/iptables.go | 26 -------------------------- 1 file changed, 26 deletions(-) diff --git a/pkg/util/iptables/iptables.go b/pkg/util/iptables/iptables.go index ab009ca5c1d..3aed0dcc623 100644 --- a/pkg/util/iptables/iptables.go +++ b/pkg/util/iptables/iptables.go @@ -26,7 +26,6 @@ import ( "regexp" "strconv" "strings" - "sync" "time" v1 "k8s.io/api/core/v1" @@ -164,7 +163,6 @@ const WaitSecondsValue = "5" // runner implements Interface in terms of exec("iptables"). type runner struct { - mu sync.Mutex exec utilexec.Interface protocol Protocol } @@ -212,9 +210,6 @@ func NewBestEffort() map[v1.IPFamily]Interface { func (runner *runner) EnsureChain(table Table, chain Chain) (bool, error) { fullArgs := makeFullArgs(table, chain) - runner.mu.Lock() - defer runner.mu.Unlock() - out, err := runner.run(opCreateChain, fullArgs) if err != nil { if ee, ok := err.(utilexec.ExitError); ok { @@ -231,9 +226,6 @@ func (runner *runner) EnsureChain(table Table, chain Chain) (bool, error) { func (runner *runner) FlushChain(table Table, chain Chain) error { fullArgs := makeFullArgs(table, chain) - runner.mu.Lock() - defer runner.mu.Unlock() - out, err := runner.run(opFlushChain, fullArgs) if err != nil { return fmt.Errorf("error flushing chain %q: %v: %s", chain, err, out) @@ -245,9 +237,6 @@ func (runner *runner) FlushChain(table Table, chain Chain) error { func (runner *runner) DeleteChain(table Table, chain Chain) error { fullArgs := makeFullArgs(table, chain) - runner.mu.Lock() - defer runner.mu.Unlock() - out, err := runner.run(opDeleteChain, fullArgs) if err != nil { return fmt.Errorf("error deleting chain %q: %v: %s", chain, err, out) @@ -259,9 +248,6 @@ func (runner *runner) DeleteChain(table Table, chain Chain) error { func (runner *runner) EnsureRule(position RulePosition, table Table, chain Chain, args ...string) (bool, error) { fullArgs := makeFullArgs(table, chain, args...) - runner.mu.Lock() - defer runner.mu.Unlock() - exists, err := runner.checkRule(fullArgs) if err != nil { return false, err @@ -280,9 +266,6 @@ func (runner *runner) EnsureRule(position RulePosition, table Table, chain Chain func (runner *runner) DeleteRule(table Table, chain Chain, args ...string) error { fullArgs := makeFullArgs(table, chain, args...) - runner.mu.Lock() - defer runner.mu.Unlock() - exists, err := runner.checkRule(fullArgs) if err != nil { return err @@ -307,9 +290,6 @@ func (runner *runner) Protocol() Protocol { // SaveInto is part of Interface. func (runner *runner) SaveInto(table Table, buffer *bytes.Buffer) error { - runner.mu.Lock() - defer runner.mu.Unlock() - trace := utiltrace.New("iptables save") defer trace.LogIfLong(2 * time.Second) @@ -345,9 +325,6 @@ func (runner *runner) RestoreAll(data []byte, flush FlushFlag, counters RestoreC // restoreInternal is the shared part of Restore/RestoreAll func (runner *runner) restoreInternal(args []string, data []byte, flush FlushFlag, counters RestoreCountersFlag) error { - runner.mu.Lock() - defer runner.mu.Unlock() - trace := utiltrace.New("iptables restore") defer trace.LogIfLong(2 * time.Second) @@ -495,9 +472,6 @@ func (runner *runner) Monitor(canary Chain, tables []Table, reloadFunc func(), i func (runner *runner) ChainExists(table Table, chain Chain) (bool, error) { fullArgs := makeFullArgs(table, chain) - runner.mu.Lock() - defer runner.mu.Unlock() - trace := utiltrace.New("iptables ChainExists") defer trace.LogIfLong(2 * time.Second)