From 9c98d29795356a16e4ecda2af38ec4f1cfa4f3f2 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Fri, 24 Jan 2025 08:34:44 -0500 Subject: [PATCH 1/4] Remove exec arg from utiliptables.New It was there so you could mock the results via a FakeExec, but these days any unit tests outside of pkg/util/iptables that want to mock iptables results use a FakeIPTables instead of a real utiliptables.Interface with a FakeExec. --- cmd/kube-proxy/app/server_linux.go | 6 ++--- pkg/kubelet/kubelet_network_linux.go | 6 ++--- pkg/util/iptables/iptables.go | 4 +-- pkg/util/iptables/iptables_test.go | 38 ++++++++++++++-------------- pkg/util/iptables/monitor_test.go | 2 +- 5 files changed, 26 insertions(+), 30 deletions(-) diff --git a/cmd/kube-proxy/app/server_linux.go b/cmd/kube-proxy/app/server_linux.go index 19e5170d339..9cf782c1a3f 100644 --- a/cmd/kube-proxy/app/server_linux.go +++ b/cmd/kube-proxy/app/server_linux.go @@ -109,12 +109,10 @@ func isIPTablesBased(mode proxyconfigapi.ProxyMode) bool { // is not v1.IPFamilyUnknown then it will also separately return the interface for just // that family. func getIPTables(primaryFamily v1.IPFamily) ([2]utiliptables.Interface, utiliptables.Interface) { - execer := exec.New() - // Create iptables handlers for both families. Always ordered as IPv4, IPv6 ipt := [2]utiliptables.Interface{ - utiliptables.New(execer, utiliptables.ProtocolIPv4), - utiliptables.New(execer, utiliptables.ProtocolIPv6), + utiliptables.New(utiliptables.ProtocolIPv4), + utiliptables.New(utiliptables.ProtocolIPv6), } var iptInterface utiliptables.Interface diff --git a/pkg/kubelet/kubelet_network_linux.go b/pkg/kubelet/kubelet_network_linux.go index fce57689061..b814e9b829d 100644 --- a/pkg/kubelet/kubelet_network_linux.go +++ b/pkg/kubelet/kubelet_network_linux.go @@ -25,7 +25,6 @@ import ( "k8s.io/apimachinery/pkg/util/wait" "k8s.io/klog/v2" utiliptables "k8s.io/kubernetes/pkg/util/iptables" - utilexec "k8s.io/utils/exec" ) const ( @@ -38,10 +37,9 @@ const ( ) func (kl *Kubelet) initNetworkUtil() { - exec := utilexec.New() iptClients := []utiliptables.Interface{ - utiliptables.New(exec, utiliptables.ProtocolIPv4), - utiliptables.New(exec, utiliptables.ProtocolIPv6), + utiliptables.New(utiliptables.ProtocolIPv4), + utiliptables.New(utiliptables.ProtocolIPv6), } for i := range iptClients { diff --git a/pkg/util/iptables/iptables.go b/pkg/util/iptables/iptables.go index c6c64e45f73..3181f3eb21b 100644 --- a/pkg/util/iptables/iptables.go +++ b/pkg/util/iptables/iptables.go @@ -246,8 +246,8 @@ func newInternal(exec utilexec.Interface, protocol Protocol, lockfilePath14x, lo } // New returns a new Interface which will exec iptables. -func New(exec utilexec.Interface, protocol Protocol) Interface { - return newInternal(exec, protocol, "", "") +func New(protocol Protocol) Interface { + return newInternal(utilexec.New(), protocol, "", "") } // EnsureChain is part of Interface. diff --git a/pkg/util/iptables/iptables_test.go b/pkg/util/iptables/iptables_test.go index 652b7786431..480ae98e827 100644 --- a/pkg/util/iptables/iptables_test.go +++ b/pkg/util/iptables/iptables_test.go @@ -61,7 +61,7 @@ func testIPTablesVersionCmds(t *testing.T, protocol Protocol) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - _ = New(fexec, protocol) + _ = newInternal(fexec, protocol, "", "") // Check that proper iptables version command was used during runner instantiation if !sets.New(fcmd.CombinedOutputLog[0]...).HasAll(iptablesCmd, "--version") { @@ -103,7 +103,7 @@ func testEnsureChain(t *testing.T, protocol Protocol) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := New(fexec, protocol) + runner := newInternal(fexec, protocol, "", "") // Success. exists, err := runner.EnsureChain(TableNAT, Chain("FOOBAR")) if err != nil { @@ -160,7 +160,7 @@ func TestFlushChain(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := New(fexec, ProtocolIPv4) + runner := newInternal(fexec, ProtocolIPv4, "", "") // Success. err := runner.FlushChain(TableNAT, Chain("FOOBAR")) if err != nil { @@ -197,7 +197,7 @@ func TestDeleteChain(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := New(fexec, ProtocolIPv4) + runner := newInternal(fexec, ProtocolIPv4, "", "") // Success. err := runner.DeleteChain(TableNAT, Chain("FOOBAR")) if err != nil { @@ -233,7 +233,7 @@ func TestEnsureRuleAlreadyExists(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := New(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) @@ -269,7 +269,7 @@ func TestEnsureRuleNew(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := New(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) @@ -302,7 +302,7 @@ func TestEnsureRuleErrorChecking(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := New(fexec, ProtocolIPv4) + runner := newInternal(fexec, ProtocolIPv4, "", "") _, err := runner.EnsureRule(Append, TableNAT, ChainOutput, "abc", "123") if err == nil { t.Errorf("expected failure") @@ -332,7 +332,7 @@ func TestEnsureRuleErrorCreating(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := New(fexec, ProtocolIPv4) + runner := newInternal(fexec, ProtocolIPv4, "", "") _, err := runner.EnsureRule(Append, TableNAT, ChainOutput, "abc", "123") if err == nil { t.Errorf("expected failure") @@ -359,7 +359,7 @@ func TestDeleteRuleDoesNotExist(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := New(fexec, ProtocolIPv4) + runner := newInternal(fexec, ProtocolIPv4, "", "") err := runner.DeleteRule(TableNAT, ChainOutput, "abc", "123") if err != nil { t.Errorf("expected success, got %v", err) @@ -392,7 +392,7 @@ func TestDeleteRuleExists(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := New(fexec, ProtocolIPv4) + runner := newInternal(fexec, ProtocolIPv4, "", "") err := runner.DeleteRule(TableNAT, ChainOutput, "abc", "123") if err != nil { t.Errorf("expected success, got %v", err) @@ -422,7 +422,7 @@ func TestDeleteRuleErrorChecking(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := New(fexec, ProtocolIPv4) + runner := newInternal(fexec, ProtocolIPv4, "", "") err := runner.DeleteRule(TableNAT, ChainOutput, "abc", "123") if err == nil { t.Errorf("expected failure") @@ -452,7 +452,7 @@ func TestDeleteRuleErrorDeleting(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := New(fexec, ProtocolIPv4) + runner := newInternal(fexec, ProtocolIPv4, "", "") err := runner.DeleteRule(TableNAT, ChainOutput, "abc", "123") if err == nil { t.Errorf("expected failure") @@ -487,7 +487,7 @@ func TestGetIPTablesHasCheckCommand(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - ipt := New(fexec, ProtocolIPv4) + ipt := newInternal(fexec, ProtocolIPv4, "", "") runner := ipt.(*runner) if testCase.Expected != runner.hasCheck { t.Errorf("Expected result: %v, Got result: %v", testCase.Expected, runner.hasCheck) @@ -648,7 +648,7 @@ func TestWaitFlagUnavailable(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := New(fexec, ProtocolIPv4) + runner := newInternal(fexec, ProtocolIPv4, "", "") err := runner.DeleteChain(TableNAT, Chain("FOOBAR")) if err != nil { t.Errorf("expected success, got %v", err) @@ -679,7 +679,7 @@ func TestWaitFlagOld(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := New(fexec, ProtocolIPv4) + runner := newInternal(fexec, ProtocolIPv4, "", "") err := runner.DeleteChain(TableNAT, Chain("FOOBAR")) if err != nil { t.Errorf("expected success, got %v", err) @@ -713,7 +713,7 @@ func TestWaitFlagNew(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := New(fexec, ProtocolIPv4) + runner := newInternal(fexec, ProtocolIPv4, "", "") err := runner.DeleteChain(TableNAT, Chain("FOOBAR")) if err != nil { t.Errorf("expected success, got %v", err) @@ -744,7 +744,7 @@ func TestWaitIntervalFlagNew(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := New(fexec, ProtocolIPv4) + runner := newInternal(fexec, ProtocolIPv4, "", "") err := runner.DeleteChain(TableNAT, Chain("FOOBAR")) if err != nil { t.Errorf("expected success, got %v", err) @@ -789,7 +789,7 @@ COMMIT func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := New(fexec, protocol) + runner := newInternal(fexec, protocol, "", "") buffer := bytes.NewBuffer(nil) // Success. @@ -857,7 +857,7 @@ func testRestore(t *testing.T, protocol Protocol) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := New(fexec, protocol) + runner := newInternal(fexec, protocol, "", "") // both flags true err := runner.Restore(TableNAT, []byte{}, FlushTables, RestoreCounters) diff --git a/pkg/util/iptables/monitor_test.go b/pkg/util/iptables/monitor_test.go index de522c59ef6..223fbaa04e0 100644 --- a/pkg/util/iptables/monitor_test.go +++ b/pkg/util/iptables/monitor_test.go @@ -191,7 +191,7 @@ func (mfc *monitorFakeCmd) Stop() { func TestIPTablesMonitor(t *testing.T) { mfe := newMonitorFakeExec() - ipt := New(mfe, ProtocolIPv4) + ipt := newInternal(mfe, ProtocolIPv4, "", "") var reloads uint32 stopCh := make(chan struct{}) From f1d0eb4fe47f80e999847f7dfe716f1aa597abc1 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Sat, 25 Jan 2025 12:06:36 -0500 Subject: [PATCH 2/4] Add a unit test for utiliptables.New() --- pkg/util/iptables/iptables_test.go | 194 ++++++++++++++++++++++++----- 1 file changed, 162 insertions(+), 32 deletions(-) diff --git a/pkg/util/iptables/iptables_test.go b/pkg/util/iptables/iptables_test.go index 480ae98e827..5b59a044306 100644 --- a/pkg/util/iptables/iptables_test.go +++ b/pkg/util/iptables/iptables_test.go @@ -42,44 +42,174 @@ func getLockPaths() (string, string) { return lock14x, lock16x } -func testIPTablesVersionCmds(t *testing.T, protocol Protocol) { - version := " v1.4.22" - iptablesCmd := iptablesCommand(protocol) - iptablesRestoreCmd := iptablesRestoreCommand(protocol) +type testCommand struct { + command string + action fakeexec.FakeAction +} - fcmd := fakeexec.FakeCmd{ - CombinedOutputScript: []fakeexec.FakeAction{ - // iptables version response (for runner instantiation) - func() ([]byte, []byte, error) { return []byte(iptablesCmd + version), nil, nil }, - // iptables-restore version response (for runner instantiation) - func() ([]byte, []byte, error) { return []byte(iptablesRestoreCmd + version), nil, nil }, - }, - } +// Creates a FakeExec that expects exactly commands to be run (and will fail otherwise). +func fakeExecForCommands(commands []testCommand) *fakeexec.FakeExec { 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...) }, + CommandScript: make([]fakeexec.FakeCommandAction, len(commands)), + ExactOrder: true, + } + for i := range commands { + fcmd := fakeexec.FakeCmd{ + CombinedOutputScript: []fakeexec.FakeAction{commands[i].action}, + } + argv := strings.Fields(commands[i].command) + fexec.CommandScript[i] = func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, argv[0], argv[1:]...) } + } + return fexec +} + +func TestFakeExecForCommands(t *testing.T) { + var panicresult interface{} + defer func() { + panicresult = recover() + }() + + fake1 := fakeExecForCommands([]testCommand{{ + command: "foo bar baz", + action: func() ([]byte, []byte, error) { return []byte("output"), nil, nil }, + }}) + cmd := fake1.Command("foo", "bar", "baz") + out, err := cmd.CombinedOutput() + if string(out) != "output" { + t.Errorf("fake1: wrong output: expected %q, got %q", "output", out) + } + if err != nil { + t.Errorf("fake1: expected no error, got %v", err) + } + if panicresult != nil { + t.Errorf("fake1: expected no panic, got %q", panicresult) + } + + fake2 := fakeExecForCommands([]testCommand{{ + command: "foo bar baz", + action: func() ([]byte, []byte, error) { return []byte("output"), nil, nil }, + }}) + _ = fake2.Command("foo", "baz") + if panicresult == nil { + t.Errorf("fake2: expected panic from FakeExec, got none") + } +} + +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{ + hasCheck: false, + hasRandomFully: false, + waitFlag: nil, + restoreWaitFlag: nil, + }, + }, + { + 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{ + hasCheck: true, + hasRandomFully: false, + waitFlag: []string{"-w"}, + restoreWaitFlag: []string{"-w"}, + }, + }, + { + name: "1.6", + commands: []testCommand{ + { + command: "iptables --version", + action: func() ([]byte, []byte, error) { return []byte("iptables v1.6.2"), nil, nil }, + }, + }, + expected: &runner{ + hasCheck: true, + hasRandomFully: true, + waitFlag: []string{"-w", "5", "-W", "100000"}, + restoreWaitFlag: []string{"-w", "5", "-W", "100000"}, + }, + }, + { + name: "1.8", + commands: []testCommand{ + { + command: "iptables --version", + action: func() ([]byte, []byte, error) { return []byte("iptables v1.8.11"), nil, nil }, + }, + }, + expected: &runner{ + hasCheck: true, + hasRandomFully: true, + waitFlag: []string{"-w", "5", "-W", "100000"}, + restoreWaitFlag: []string{"-w", "5", "-W", "100000"}, + }, + }, + { + name: "no iptables", + commands: []testCommand{ + { + command: "iptables --version", + action: func() ([]byte, []byte, error) { return nil, nil, fmt.Errorf("no such file or directory") }, + }, + { + command: "iptables-restore --version", + action: func() ([]byte, []byte, error) { return nil, nil, fmt.Errorf("no such file or directory") }, + }, + }, + expected: &runner{ + hasCheck: true, + hasRandomFully: false, + waitFlag: nil, + restoreWaitFlag: nil, + }, }, } - _ = newInternal(fexec, protocol, "", "") - // Check that proper iptables version command was used during runner instantiation - if !sets.New(fcmd.CombinedOutputLog[0]...).HasAll(iptablesCmd, "--version") { - t.Errorf("%s runner instantiate: Expected cmd '%s --version', Got '%s'", protocol, iptablesCmd, fcmd.CombinedOutputLog[0]) + for _, tc := range testCases { + t.Run(tc.name, func(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) + } + 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) + } + }) } - - // Check that proper iptables restore version command was used during runner instantiation - if !sets.New(fcmd.CombinedOutputLog[1]...).HasAll(iptablesRestoreCmd, "--version") { - t.Errorf("%s runner instantiate: Expected cmd '%s --version', Got '%s'", protocol, iptablesRestoreCmd, fcmd.CombinedOutputLog[1]) - } -} - -func TestIPTablesVersionCmdsIPv4(t *testing.T) { - testIPTablesVersionCmds(t, ProtocolIPv4) -} - -func TestIPTablesVersionCmdsIPv6(t *testing.T) { - testIPTablesVersionCmds(t, ProtocolIPv6) } func testEnsureChain(t *testing.T, protocol Protocol) { From b03125896923d59a2d12a17e16d6289be9fce6e8 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Sat, 25 Jan 2025 10:48:27 -0500 Subject: [PATCH 3/4] Improve utiliptables error handling when there's no iptables binary If `iptables --version` failed, utiliptables.New() would log a warning and assume that the problem was that you had an implausibly ancient version of iptables installed. Change it to instead assume that the problem is that you don't have iptables installed at all (and don't log anything; the caller will discover this later). --- pkg/util/iptables/iptables.go | 23 +++++++++++++---------- pkg/util/iptables/iptables_test.go | 8 ++------ 2 files changed, 15 insertions(+), 16 deletions(-) diff --git a/pkg/util/iptables/iptables.go b/pkg/util/iptables/iptables.go index 3181f3eb21b..a41f0ea7bb8 100644 --- a/pkg/util/iptables/iptables.go +++ b/pkg/util/iptables/iptables.go @@ -219,12 +219,6 @@ type runner struct { // newInternal returns a new Interface which will exec iptables, and allows the // caller to change the iptables-restore lockfile path func newInternal(exec utilexec.Interface, protocol Protocol, lockfilePath14x, lockfilePath16x string) Interface { - version, err := getIPTablesVersion(exec, protocol) - if err != nil { - klog.InfoS("Error checking iptables version, assuming version at least", "version", MinCheckVersion, "err", err) - version = MinCheckVersion - } - if lockfilePath16x == "" { lockfilePath16x = LockfilePath16x } @@ -235,13 +229,22 @@ func newInternal(exec utilexec.Interface, protocol Protocol, lockfilePath14x, lo runner := &runner{ exec: exec, protocol: protocol, - hasCheck: version.AtLeast(MinCheckVersion), - hasRandomFully: version.AtLeast(RandomFullyMinVersion), - waitFlag: getIPTablesWaitFlag(version), - restoreWaitFlag: getIPTablesRestoreWaitFlag(version, exec, protocol), lockfilePath14x: lockfilePath14x, lockfilePath16x: lockfilePath16x, } + + version, 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 + } + + runner.hasCheck = version.AtLeast(MinCheckVersion) + runner.hasRandomFully = version.AtLeast(RandomFullyMinVersion) + runner.waitFlag = getIPTablesWaitFlag(version) + runner.restoreWaitFlag = getIPTablesRestoreWaitFlag(version, exec, protocol) return runner } diff --git a/pkg/util/iptables/iptables_test.go b/pkg/util/iptables/iptables_test.go index 5b59a044306..21fa1d0e067 100644 --- a/pkg/util/iptables/iptables_test.go +++ b/pkg/util/iptables/iptables_test.go @@ -177,13 +177,9 @@ func TestNew(t *testing.T) { command: "iptables --version", action: func() ([]byte, []byte, error) { return nil, nil, fmt.Errorf("no such file or directory") }, }, - { - command: "iptables-restore --version", - action: func() ([]byte, []byte, error) { return nil, nil, fmt.Errorf("no such file or directory") }, - }, }, expected: &runner{ - hasCheck: true, + hasCheck: false, hasRandomFully: false, waitFlag: nil, restoreWaitFlag: nil, @@ -601,7 +597,7 @@ func TestGetIPTablesHasCheckCommand(t *testing.T) { {"iptables v1.4.11", true}, {"iptables v1.4.19.1", true}, {"iptables v2.0.0", true}, - {"total junk", true}, + {"total junk", false}, } for _, testCase := range testCases { From 8c98dee1edbb900a88952ce295788dd93402f885 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Sat, 11 Jan 2025 07:44:39 -0500 Subject: [PATCH 4/4] Add utiliptables.NewDualStack Basically all callers want dual-stack-if-possible, so simplify that. Also, tweak the startup-time checking in kubelet to treat "no iptables support" as interesting but not an error. --- cmd/kube-proxy/app/server_linux.go | 44 ++------- pkg/kubelet/kubelet_network_linux.go | 11 ++- pkg/proxy/iptables/proxier.go | 6 +- pkg/proxy/ipvs/proxier.go | 6 +- pkg/util/iptables/iptables.go | 23 +++++ pkg/util/iptables/iptables_test.go | 136 +++++++++++++++++++++++++++ 6 files changed, 181 insertions(+), 45 deletions(-) diff --git a/cmd/kube-proxy/app/server_linux.go b/cmd/kube-proxy/app/server_linux.go index 9cf782c1a3f..a4769d3817b 100644 --- a/cmd/kube-proxy/app/server_linux.go +++ b/cmd/kube-proxy/app/server_linux.go @@ -50,7 +50,6 @@ import ( "k8s.io/kubernetes/pkg/proxy/nftables" proxyutil "k8s.io/kubernetes/pkg/proxy/util" utiliptables "k8s.io/kubernetes/pkg/util/iptables" - "k8s.io/utils/exec" ) // timeoutForNodePodCIDR is the time to wait for allocators to assign a PodCIDR to the @@ -105,35 +104,15 @@ func isIPTablesBased(mode proxyconfigapi.ProxyMode) bool { return mode == proxyconfigapi.ProxyModeIPTables || mode == proxyconfigapi.ProxyModeIPVS } -// getIPTables returns an array of [IPv4, IPv6] utiliptables.Interfaces. If primaryFamily -// is not v1.IPFamilyUnknown then it will also separately return the interface for just -// that family. -func getIPTables(primaryFamily v1.IPFamily) ([2]utiliptables.Interface, utiliptables.Interface) { - // Create iptables handlers for both families. Always ordered as IPv4, IPv6 - ipt := [2]utiliptables.Interface{ - utiliptables.New(utiliptables.ProtocolIPv4), - utiliptables.New(utiliptables.ProtocolIPv6), - } - - var iptInterface utiliptables.Interface - if primaryFamily == v1.IPv4Protocol { - iptInterface = ipt[0] - } else if primaryFamily == v1.IPv6Protocol { - iptInterface = ipt[1] - } - - return ipt, iptInterface -} - // platformCheckSupported is called immediately before creating the Proxier, to check // what IP families are supported (and whether the configuration is usable at all). func (s *ProxyServer) platformCheckSupported(ctx context.Context) (ipv4Supported, ipv6Supported, dualStackSupported bool, err error) { logger := klog.FromContext(ctx) if isIPTablesBased(s.Config.Mode) { - ipt, _ := getIPTables(v1.IPFamilyUnknown) - ipv4Supported = ipt[0].Present() - ipv6Supported = ipt[1].Present() + ipts := utiliptables.NewDualStack() + ipv4Supported = ipts[v1.IPv4Protocol] != nil + ipv6Supported = ipts[v1.IPv6Protocol] != nil if !ipv4Supported && !ipv6Supported { err = fmt.Errorf("iptables is not available on this host") @@ -164,14 +143,13 @@ func (s *ProxyServer) createProxier(ctx context.Context, config *proxyconfigapi. if config.Mode == proxyconfigapi.ProxyModeIPTables { logger.Info("Using iptables Proxier") + ipts := utiliptables.NewDualStack() if dualStack { - ipt, _ := getIPTables(s.PrimaryIPFamily) - // TODO this has side effects that should only happen when Run() is invoked. proxier, err = iptables.NewDualStackProxier( ctx, - ipt, + ipts, utilsysctl.New(), config.SyncPeriod.Duration, config.MinSyncPeriod.Duration, @@ -188,13 +166,12 @@ func (s *ProxyServer) createProxier(ctx context.Context, config *proxyconfigapi. ) } else { // Create a single-stack proxier if and only if the node does not support dual-stack (i.e, no iptables support). - _, iptInterface := getIPTables(s.PrimaryIPFamily) // TODO this has side effects that should only happen when Run() is invoked. proxier, err = iptables.NewProxier( ctx, s.PrimaryIPFamily, - iptInterface, + ipts[s.PrimaryIPFamily], utilsysctl.New(), config.SyncPeriod.Duration, config.MinSyncPeriod.Duration, @@ -220,13 +197,13 @@ func (s *ProxyServer) createProxier(ctx context.Context, config *proxyconfigapi. if err := ipvs.CanUseIPVSProxier(ctx, ipvsInterface, ipsetInterface, config.IPVS.Scheduler); err != nil { return nil, fmt.Errorf("can't use the IPVS proxier: %v", err) } + ipts := utiliptables.NewDualStack() logger.Info("Using ipvs Proxier") if dualStack { - ipt, _ := getIPTables(s.PrimaryIPFamily) proxier, err = ipvs.NewDualStackProxier( ctx, - ipt, + ipts, ipvsInterface, ipsetInterface, utilsysctl.New(), @@ -249,11 +226,10 @@ func (s *ProxyServer) createProxier(ctx context.Context, config *proxyconfigapi. initOnly, ) } else { - _, iptInterface := getIPTables(s.PrimaryIPFamily) proxier, err = ipvs.NewProxier( ctx, s.PrimaryIPFamily, - iptInterface, + ipts[s.PrimaryIPFamily], ipvsInterface, ipsetInterface, utilsysctl.New(), @@ -507,7 +483,7 @@ func platformCleanup(ctx context.Context, mode proxyconfigapi.ProxyMode, cleanup // Clean up iptables and ipvs rules if switching to nftables, or if cleanupAndExit if !isIPTablesBased(mode) || cleanupAndExit { - ipts, _ := getIPTables(v1.IPFamilyUnknown) + ipts := utiliptables.NewDualStack() ipsetInterface := utilipset.New() ipvsInterface := utilipvs.New() diff --git a/pkg/kubelet/kubelet_network_linux.go b/pkg/kubelet/kubelet_network_linux.go index b814e9b829d..32ed9eb2429 100644 --- a/pkg/kubelet/kubelet_network_linux.go +++ b/pkg/kubelet/kubelet_network_linux.go @@ -37,13 +37,14 @@ const ( ) func (kl *Kubelet) initNetworkUtil() { - iptClients := []utiliptables.Interface{ - utiliptables.New(utiliptables.ProtocolIPv4), - utiliptables.New(utiliptables.ProtocolIPv6), + iptClients := utiliptables.NewDualStack() + if len(iptClients) == 0 { + klog.InfoS("No iptables support on this system; not creating the KUBE-IPTABLES-HINT chain") + return } - for i := range iptClients { - iptClient := iptClients[i] + for family := range iptClients { + iptClient := iptClients[family] if kl.syncIPTablesRules(iptClient) { klog.InfoS("Initialized iptables rules.", "protocol", iptClient.Protocol()) go iptClient.Monitor( diff --git a/pkg/proxy/iptables/proxier.go b/pkg/proxy/iptables/proxier.go index 80c00cd53a5..85a6adb2e5a 100644 --- a/pkg/proxy/iptables/proxier.go +++ b/pkg/proxy/iptables/proxier.go @@ -94,7 +94,7 @@ const sysctlNFConntrackTCPBeLiberal = "net/netfilter/nf_conntrack_tcp_be_liberal // NewDualStackProxier creates a MetaProxier instance, with IPv4 and IPv6 proxies. func NewDualStackProxier( ctx context.Context, - ipt [2]utiliptables.Interface, + ipts map[v1.IPFamily]utiliptables.Interface, sysctl utilsysctl.Interface, syncPeriod time.Duration, minSyncPeriod time.Duration, @@ -110,7 +110,7 @@ func NewDualStackProxier( initOnly bool, ) (proxy.Provider, error) { // Create an ipv4 instance of the single-stack proxier - ipv4Proxier, err := NewProxier(ctx, v1.IPv4Protocol, ipt[0], sysctl, + ipv4Proxier, err := NewProxier(ctx, v1.IPv4Protocol, ipts[v1.IPv4Protocol], sysctl, syncPeriod, minSyncPeriod, masqueradeAll, localhostNodePorts, masqueradeBit, localDetectors[v1.IPv4Protocol], hostname, nodeIPs[v1.IPv4Protocol], recorder, healthzServer, nodePortAddresses, initOnly) @@ -118,7 +118,7 @@ func NewDualStackProxier( return nil, fmt.Errorf("unable to create ipv4 proxier: %v", err) } - ipv6Proxier, err := NewProxier(ctx, v1.IPv6Protocol, ipt[1], sysctl, + ipv6Proxier, err := NewProxier(ctx, v1.IPv6Protocol, ipts[v1.IPv6Protocol], sysctl, syncPeriod, minSyncPeriod, masqueradeAll, false, masqueradeBit, localDetectors[v1.IPv6Protocol], hostname, nodeIPs[v1.IPv6Protocol], recorder, healthzServer, nodePortAddresses, initOnly) diff --git a/pkg/proxy/ipvs/proxier.go b/pkg/proxy/ipvs/proxier.go index c83a3cad5ee..9d70be2ee7e 100644 --- a/pkg/proxy/ipvs/proxier.go +++ b/pkg/proxy/ipvs/proxier.go @@ -110,7 +110,7 @@ const ( // NewDualStackProxier returns a new Proxier for dual-stack operation func NewDualStackProxier( ctx context.Context, - ipt [2]utiliptables.Interface, + ipts map[v1.IPFamily]utiliptables.Interface, ipvs utilipvs.Interface, ipset utilipset.Interface, sysctl utilsysctl.Interface, @@ -133,7 +133,7 @@ func NewDualStackProxier( initOnly bool, ) (proxy.Provider, error) { // Create an ipv4 instance of the single-stack proxier - ipv4Proxier, err := NewProxier(ctx, v1.IPv4Protocol, ipt[0], ipvs, ipset, sysctl, + ipv4Proxier, err := NewProxier(ctx, v1.IPv4Protocol, ipts[v1.IPv4Protocol], ipvs, ipset, sysctl, syncPeriod, minSyncPeriod, filterCIDRs(false, excludeCIDRs), strictARP, tcpTimeout, tcpFinTimeout, udpTimeout, masqueradeAll, masqueradeBit, localDetectors[v1.IPv4Protocol], hostname, nodeIPs[v1.IPv4Protocol], recorder, @@ -142,7 +142,7 @@ func NewDualStackProxier( return nil, fmt.Errorf("unable to create ipv4 proxier: %v", err) } - ipv6Proxier, err := NewProxier(ctx, v1.IPv6Protocol, ipt[1], ipvs, ipset, sysctl, + ipv6Proxier, err := NewProxier(ctx, v1.IPv6Protocol, ipts[v1.IPv6Protocol], ipvs, ipset, sysctl, syncPeriod, minSyncPeriod, filterCIDRs(true, excludeCIDRs), strictARP, tcpTimeout, tcpFinTimeout, udpTimeout, masqueradeAll, masqueradeBit, localDetectors[v1.IPv6Protocol], hostname, nodeIPs[v1.IPv6Protocol], recorder, diff --git a/pkg/util/iptables/iptables.go b/pkg/util/iptables/iptables.go index a41f0ea7bb8..d38ab0f9d0e 100644 --- a/pkg/util/iptables/iptables.go +++ b/pkg/util/iptables/iptables.go @@ -30,6 +30,7 @@ import ( "sync" "time" + "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" @@ -253,6 +254,28 @@ func New(protocol Protocol) Interface { return newInternal(utilexec.New(), protocol, "", "") } +func newDualStackInternal(exec utilexec.Interface) map[v1.IPFamily]Interface { + interfaces := map[v1.IPFamily]Interface{} + + iptv4 := newInternal(exec, ProtocolIPv4, "", "") + if iptv4.Present() { + interfaces[v1.IPv4Protocol] = iptv4 + } + iptv6 := newInternal(exec, ProtocolIPv6, "", "") + if iptv6.Present() { + interfaces[v1.IPv6Protocol] = iptv6 + } + + return interfaces +} + +// NewDualStack returns a map containing an IPv4 Interface (if IPv4 iptables is supported) +// and an IPv6 Interface (if IPv6 iptables is supported). If either family is not +// supported, no Interface will be returned for that family. +func NewDualStack() map[v1.IPFamily]Interface { + return newDualStackInternal(utilexec.New()) +} + // EnsureChain is part of Interface. func (runner *runner) EnsureChain(table Table, chain Chain) (bool, error) { fullArgs := makeFullArgs(table, chain) diff --git a/pkg/util/iptables/iptables_test.go b/pkg/util/iptables/iptables_test.go index 21fa1d0e067..26ab3a48e41 100644 --- a/pkg/util/iptables/iptables_test.go +++ b/pkg/util/iptables/iptables_test.go @@ -29,6 +29,7 @@ import ( "testing" "time" + 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" @@ -208,6 +209,141 @@ func TestNew(t *testing.T) { } } +func TestNewDualStack(t *testing.T) { + testCases := []struct { + name string + commands []testCommand + ipv4 bool + ipv6 bool + }{ + { + 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 -W 100000 -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 -W 100000 -S POSTROUTING -t nat", + action: func() ([]byte, []byte, error) { return nil, nil, nil }, + }, + }, + ipv4: true, + ipv6: true, + }, + { + 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 -W 100000 -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 -S POSTROUTING -t nat", + action: func() ([]byte, []byte, error) { return nil, nil, fmt.Errorf("no such file or directory") }, + }, + }, + ipv4: true, + ipv6: false, + }, + { + 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 -W 100000 -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 -W 100000 -S POSTROUTING -t nat", + action: func() ([]byte, []byte, error) { return nil, nil, fmt.Errorf("ipv6 is broken") }, + }, + }, + ipv4: true, + ipv6: false, + }, + { + 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 -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 -S POSTROUTING -t nat", + action: func() ([]byte, []byte, error) { return nil, nil, fmt.Errorf("no such file or directory") }, + }, + }, + ipv4: false, + ipv6: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + fexec := fakeExecForCommands(tc.commands) + runners := newDualStackInternal(fexec) + + if tc.ipv4 && runners[v1.IPv4Protocol] == nil { + t.Errorf("Expected ipv4 runner, got nil") + } else if !tc.ipv4 && runners[v1.IPv4Protocol] != nil { + t.Errorf("Expected no ipv4 runner, got one") + } + if tc.ipv6 && runners[v1.IPv6Protocol] == nil { + t.Errorf("Expected ipv6 runner, got nil") + } else if !tc.ipv6 && runners[v1.IPv6Protocol] != nil { + t.Errorf("Expected no ipv6 runner, got one") + } + }) + } +} func testEnsureChain(t *testing.T, protocol Protocol) { fcmd := fakeexec.FakeCmd{ CombinedOutputScript: []fakeexec.FakeAction{