From b25af8e3c9946ac9875e416fb270472ff3519a94 Mon Sep 17 00:00:00 2001 From: knight42 Date: Wed, 9 Sep 2020 18:12:16 +0800 Subject: [PATCH 1/2] feat(iptables): be able to override iptables-1.4-compatible lock path --- pkg/util/iptables/iptables.go | 24 +++++++++++++++-------- pkg/util/iptables/iptables_linux.go | 8 ++++---- pkg/util/iptables/iptables_unsupported.go | 2 +- 3 files changed, 21 insertions(+), 13 deletions(-) diff --git a/pkg/util/iptables/iptables.go b/pkg/util/iptables/iptables.go index 728d5b30c09..69c262bb345 100644 --- a/pkg/util/iptables/iptables.go +++ b/pkg/util/iptables/iptables.go @@ -186,9 +186,12 @@ const WaitIntervalString = "-W" // WaitIntervalUsecondsValue a constant for specifying the default wait interval useconds const WaitIntervalUsecondsValue = "100000" -// LockfilePath16x is the iptables lock file acquired by any process that's making any change in the iptable rule +// 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 @@ -198,20 +201,24 @@ type runner struct { hasRandomFully bool waitFlag []string restoreWaitFlag []string - lockfilePath string + lockfilePath14x string + lockfilePath16x string } // 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, lockfilePath string) Interface { +func newInternal(exec utilexec.Interface, protocol Protocol, lockfilePath14x, lockfilePath16x string) Interface { version, err := getIPTablesVersion(exec, protocol) if err != nil { klog.Warningf("Error checking iptables version, assuming version at least %s: %v", MinCheckVersion, err) version = MinCheckVersion } - if lockfilePath == "" { - lockfilePath = LockfilePath16x + if lockfilePath16x == "" { + lockfilePath16x = LockfilePath16x + } + if lockfilePath14x == "" { + lockfilePath14x = LockfilePath14x } runner := &runner{ @@ -221,14 +228,15 @@ func newInternal(exec utilexec.Interface, protocol Protocol, lockfilePath string hasRandomFully: version.AtLeast(RandomFullyMinVersion), waitFlag: getIPTablesWaitFlag(version), restoreWaitFlag: getIPTablesRestoreWaitFlag(version, exec, protocol), - lockfilePath: lockfilePath, + lockfilePath14x: lockfilePath14x, + lockfilePath16x: lockfilePath16x, } return runner } // New returns a new Interface which will exec iptables. func New(exec utilexec.Interface, protocol Protocol) Interface { - return newInternal(exec, protocol, "") + return newInternal(exec, protocol, "", "") } // EnsureChain is part of Interface. @@ -390,7 +398,7 @@ func (runner *runner) restoreInternal(args []string, data []byte, flush FlushFla // 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.lockfilePath) + locker, err := grabIptablesLocks(runner.lockfilePath14x, runner.lockfilePath16x) if err != nil { return err } diff --git a/pkg/util/iptables/iptables_linux.go b/pkg/util/iptables/iptables_linux.go index c28fd62dda8..6ee6260305a 100644 --- a/pkg/util/iptables/iptables_linux.go +++ b/pkg/util/iptables/iptables_linux.go @@ -49,7 +49,7 @@ func (l *locker) Close() error { return utilerrors.NewAggregate(errList) } -func grabIptablesLocks(lockfilePath string) (iptablesLocker, error) { +func grabIptablesLocks(lockfilePath14x, lockfilePath16x string) (iptablesLocker, error) { var err error var success bool @@ -66,9 +66,9 @@ func grabIptablesLocks(lockfilePath string) (iptablesLocker, error) { // can't assume which lock method it'll use. // Roughly duplicate iptables 1.6.x xtables_lock() function. - l.lock16, err = os.OpenFile(lockfilePath, os.O_CREATE, 0600) + l.lock16, err = os.OpenFile(lockfilePath16x, os.O_CREATE, 0600) if err != nil { - return nil, fmt.Errorf("failed to open iptables lock %s: %v", lockfilePath, err) + 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) { @@ -82,7 +82,7 @@ func grabIptablesLocks(lockfilePath string) (iptablesLocker, error) { // 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: "@xtables", Net: "unix"}) + l.lock14, err = net.ListenUnix("unix", &net.UnixAddr{Name: lockfilePath14x, Net: "unix"}) if err != nil { return false, nil } diff --git a/pkg/util/iptables/iptables_unsupported.go b/pkg/util/iptables/iptables_unsupported.go index c6a5f0d7dc6..17e61d762eb 100644 --- a/pkg/util/iptables/iptables_unsupported.go +++ b/pkg/util/iptables/iptables_unsupported.go @@ -23,7 +23,7 @@ import ( "os" ) -func grabIptablesLocks(lockfilePath string) (iptablesLocker, error) { +func grabIptablesLocks(lock14filePath, lock16filePath string) (iptablesLocker, error) { return nil, fmt.Errorf("iptables unsupported on this platform") } From ce0a423ef7a8a8293a55b9f9a8b6c2435972310a Mon Sep 17 00:00:00 2001 From: knight42 Date: Wed, 9 Sep 2020 18:13:31 +0800 Subject: [PATCH 2/2] test(iptables): deflake TestRestoreAllWaitOldIptablesRestore Signed-off-by: knight42 --- pkg/util/iptables/iptables_test.go | 48 ++++++++++++++++-------------- 1 file changed, 25 insertions(+), 23 deletions(-) diff --git a/pkg/util/iptables/iptables_test.go b/pkg/util/iptables/iptables_test.go index ebe96747c3e..c2ff33773ec 100644 --- a/pkg/util/iptables/iptables_test.go +++ b/pkg/util/iptables/iptables_test.go @@ -35,7 +35,11 @@ import ( fakeexec "k8s.io/utils/exec/testing" ) -const TestLockfilePath = "xtables.lock" +func getLockPaths() (string, string) { + lock14x := fmt.Sprintf("@xtables-%d", time.Now().Nanosecond()) + lock16x := fmt.Sprintf("xtables-%d.lock", time.Now().Nanosecond()) + return lock14x, lock16x +} func testIPTablesVersionCmds(t *testing.T, protocol Protocol) { version := " v1.4.22" @@ -934,8 +938,8 @@ func TestRestoreAll(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := newInternal(&fexec, ProtocolIPv4, TestLockfilePath) - defer os.Remove(TestLockfilePath) + lockPath14x, lockPath16x := getLockPaths() + runner := newInternal(&fexec, ProtocolIPv4, lockPath14x, lockPath16x) err := runner.RestoreAll([]byte{}, NoFlushTables, RestoreCounters) if err != nil { @@ -975,8 +979,8 @@ func TestRestoreAllWait(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := newInternal(&fexec, ProtocolIPv4, TestLockfilePath) - defer os.Remove(TestLockfilePath) + lockPath14x, lockPath16x := getLockPaths() + runner := newInternal(&fexec, ProtocolIPv4, lockPath14x, lockPath16x) err := runner.RestoreAll([]byte{}, NoFlushTables, RestoreCounters) if err != nil { @@ -1020,8 +1024,8 @@ func TestRestoreAllWaitOldIptablesRestore(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := newInternal(&fexec, ProtocolIPv4, TestLockfilePath) - defer os.Remove(TestLockfilePath) + lockPath14x, lockPath16x := getLockPaths() + runner := newInternal(&fexec, ProtocolIPv4, lockPath14x, lockPath16x) err := runner.RestoreAll([]byte{}, NoFlushTables, RestoreCounters) if err != nil { @@ -1065,24 +1069,23 @@ func TestRestoreAllGrabNewLock(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - - runner := newInternal(&fexec, ProtocolIPv4, TestLockfilePath) - defer os.Remove(TestLockfilePath) + lockPath14x, lockPath16x := getLockPaths() + runner := newInternal(&fexec, ProtocolIPv4, lockPath14x, lockPath16x) // Grab the /run lock and ensure the RestoreAll fails - runLock, err := os.OpenFile(TestLockfilePath, os.O_CREATE, 0600) + runLock, err := os.OpenFile(lockPath16x, os.O_CREATE, 0600) if err != nil { - t.Fatalf("expected to open %s, got %v", TestLockfilePath, err) + t.Fatalf("expected to open %s, got %v", lockPath16x, err) } defer runLock.Close() if err := grabIptablesFileLock(runLock); err != nil { - t.Errorf("expected to lock %s, got %v", TestLockfilePath, err) + t.Errorf("expected to lock %s, got %v", lockPath16x, err) } err = runner.RestoreAll([]byte{}, NoFlushTables, RestoreCounters) if err == nil { - t.Errorf("expected failure, got success instead") + 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) @@ -1107,22 +1110,21 @@ func TestRestoreAllGrabOldLock(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - - runner := newInternal(&fexec, ProtocolIPv4, TestLockfilePath) - defer os.Remove(TestLockfilePath) + lockPath14x, lockPath16x := getLockPaths() + 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: "@xtables", Net: "unix"}) + runLock, err = net.ListenUnix("unix", &net.UnixAddr{Name: lockPath14x, Net: "unix"}) if err != nil { - t.Logf("Failed to lock @xtables: %v, will retry.", err) + t.Logf("Failed to lock %s: %v, will retry.", lockPath14x, err) return false, nil } return true, nil }) if err != nil { - t.Fatal("Timed out locking @xtables") + t.Fatalf("Timed out locking %s", lockPath14x) } if runLock == nil { t.Fatal("Unexpected nil runLock") @@ -1132,7 +1134,7 @@ func TestRestoreAllGrabOldLock(t *testing.T) { err = runner.RestoreAll([]byte{}, NoFlushTables, RestoreCounters) if err == nil { - t.Errorf("expected failure, got success instead") + 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) @@ -1160,8 +1162,8 @@ func TestRestoreAllWaitBackportedIptablesRestore(t *testing.T) { func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, }, } - runner := newInternal(&fexec, ProtocolIPv4, TestLockfilePath) - defer os.Remove(TestLockfilePath) + lockPath14x, lockPath16x := getLockPaths() + runner := newInternal(&fexec, ProtocolIPv4, lockPath14x, lockPath16x) err := runner.RestoreAll([]byte{}, NoFlushTables, RestoreCounters) if err != nil {