From 12311af6499dce39d3132ce5ff16b99737ffd8cf Mon Sep 17 00:00:00 2001 From: phuhung273 Date: Sat, 26 Oct 2024 16:34:00 +0700 Subject: [PATCH 1/5] implementation --- go.mod | 1 + go.sum | 2 + pkg/iptableswrapper/iptables.go | 92 ++++++++++++++++++++++++++++----- pkg/networkutils/network.go | 2 +- 4 files changed, 82 insertions(+), 15 deletions(-) diff --git a/go.mod b/go.mod index 0201b99b3f..c0c14d36eb 100644 --- a/go.mod +++ b/go.mod @@ -7,6 +7,7 @@ require ( github.com/aws/amazon-vpc-cni-k8s/test/agent v0.0.0-20231212223725-21c4bd73015b github.com/aws/amazon-vpc-resource-controller-k8s v1.5.0 github.com/aws/aws-sdk-go v1.55.5 + github.com/cenkalti/backoff/v4 v4.3.0 github.com/containernetworking/cni v1.2.3 github.com/containernetworking/plugins v1.5.1 github.com/coreos/go-iptables v0.8.0 diff --git a/go.sum b/go.sum index fc1fb23151..c46494e1f5 100644 --- a/go.sum +++ b/go.sum @@ -51,6 +51,8 @@ github.com/bugsnag/osext v0.0.0-20130617224835-0dd3f918b21b h1:otBG+dV+YK+Soembj github.com/bugsnag/osext v0.0.0-20130617224835-0dd3f918b21b/go.mod h1:obH5gd0BsqsP2LwDJ9aOkm/6J86V6lyAXCoQWGw3K50= github.com/bugsnag/panicwrap v0.0.0-20151223152923-e2c28503fcd0 h1:nvj0OLI3YqYXer/kZD8Ri1aaunCxIEsOst1BVJswV0o= github.com/bugsnag/panicwrap v0.0.0-20151223152923-e2c28503fcd0/go.mod h1:D/8v3kj0zr8ZAKg1AQ6crr+5VwKN5eIywRkfhyM/+dE= +github.com/cenkalti/backoff/v4 v4.3.0 h1:MyRJ/UdXutAwSAT+s3wNd7MfTIcy71VQueUuFK343L8= +github.com/cenkalti/backoff/v4 v4.3.0/go.mod h1:Y3VNntkOUPxTVeUxJ/G5vcM//AlwfmyYozVcomhLiZE= github.com/census-instrumentation/opencensus-proto v0.2.1/go.mod h1:f6KPmirojxKA12rnyqOA5BBL4O983OfeGPqjHWSTneU= github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UFvs= github.com/cespare/xxhash/v2 v2.3.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs= diff --git a/pkg/iptableswrapper/iptables.go b/pkg/iptableswrapper/iptables.go index ce1ab24343..91046586d0 100644 --- a/pkg/iptableswrapper/iptables.go +++ b/pkg/iptableswrapper/iptables.go @@ -14,7 +14,14 @@ // Package iptableswrapper is a wrapper interface for the iptables package package iptableswrapper -import "github.com/coreos/go-iptables/iptables" +import ( + "time" + + log "github.com/sirupsen/logrus" + + "github.com/cenkalti/backoff/v4" + "github.com/coreos/go-iptables/iptables" +) // IPTablesIface is an interface created to make code unit testable. // Both the iptables package version and mocked version implement the same interface @@ -35,76 +42,133 @@ type IPTablesIface interface { // ipTables is a struct that implements IPTablesIface using iptables package. type ipTables struct { - ipt *iptables.IPTables + ipt *iptables.IPTables + backoff *backoff.ExponentialBackOff } // NewIPTables return a ipTables struct that implements IPTablesIface func NewIPTables(protocol iptables.Protocol) (IPTablesIface, error) { - ipt, err := iptables.NewWithProtocol(protocol) + ipt, err := iptables.New(iptables.IPFamily(protocol), iptables.Timeout(1)) if err != nil { return nil, err } return &ipTables{ ipt: ipt, + backoff: &backoff.ExponentialBackOff{ + MaxElapsedTime: 0, // Never stop retrying + }, }, nil } // Exists implements IPTablesIface interface by calling iptables package func (i ipTables) Exists(table, chain string, rulespec ...string) (bool, error) { - return i.ipt.Exists(table, chain, rulespec...) + operation := func() (bool, error) { + return i.ipt.Exists(table, chain, rulespec...) + } + result, err := backoff.RetryNotifyWithData(operation, i.backoff, logRetryError) + if err != nil { + return true, err + } + return result, nil } // Insert implements IPTablesIface interface by calling iptables package func (i ipTables) Insert(table, chain string, pos int, rulespec ...string) error { - return i.ipt.Insert(table, chain, pos, rulespec...) + operation := func() error { + return i.ipt.Insert(table, chain, pos, rulespec...) + } + return backoff.RetryNotify(operation, i.backoff, logRetryError) } // Append implements IPTablesIface interface by calling iptables package func (i ipTables) Append(table, chain string, rulespec ...string) error { - return i.ipt.Append(table, chain, rulespec...) + operation := func() error { + return i.ipt.Append(table, chain, rulespec...) + } + return backoff.RetryNotify(operation, i.backoff, logRetryError) } // AppendUnique implements IPTablesIface interface by calling iptables package func (i ipTables) AppendUnique(table, chain string, rulespec ...string) error { - return i.ipt.AppendUnique(table, chain, rulespec...) + operation := func() error { + return i.ipt.AppendUnique(table, chain, rulespec...) + } + return backoff.RetryNotify(operation, i.backoff, logRetryError) } // Delete implements IPTablesIface interface by calling iptables package func (i ipTables) Delete(table, chain string, rulespec ...string) error { - return i.ipt.Delete(table, chain, rulespec...) + operation := func() error { + return i.ipt.Delete(table, chain, rulespec...) + } + return backoff.RetryNotify(operation, i.backoff, logRetryError) } // List implements IPTablesIface interface by calling iptables package func (i ipTables) List(table, chain string) ([]string, error) { - return i.ipt.List(table, chain) + operation := func() ([]string, error) { + return i.ipt.List(table, chain) + } + result, err := backoff.RetryNotifyWithData(operation, i.backoff, logRetryError) + if err != nil { + return nil, err + } + return result, nil } // NewChain implements IPTablesIface interface by calling iptables package func (i ipTables) NewChain(table, chain string) error { - return i.ipt.NewChain(table, chain) + operation := func() error { + return i.ipt.NewChain(table, chain) + } + return backoff.RetryNotify(operation, i.backoff, logRetryError) } // ClearChain implements IPTablesIface interface by calling iptables package func (i ipTables) ClearChain(table, chain string) error { - return i.ipt.ClearChain(table, chain) + operation := func() error { + return i.ipt.ClearChain(table, chain) + } + return backoff.RetryNotify(operation, i.backoff, logRetryError) } // DeleteChain implements IPTablesIface interface by calling iptables package func (i ipTables) DeleteChain(table, chain string) error { - return i.ipt.DeleteChain(table, chain) + operation := func() error { + return i.ipt.DeleteChain(table, chain) + } + return backoff.RetryNotify(operation, i.backoff, logRetryError) } // ListChains implements IPTablesIface interface by calling iptables package func (i ipTables) ListChains(table string) ([]string, error) { - return i.ipt.ListChains(table) + operation := func() ([]string, error) { + return i.ipt.ListChains(table) + } + result, err := backoff.RetryNotifyWithData(operation, i.backoff, logRetryError) + if err != nil { + return nil, err + } + return result, nil } // ChainExists implements IPTablesIface interface by calling iptables package func (i ipTables) ChainExists(table, chain string) (bool, error) { - return i.ipt.ChainExists(table, chain) + operation := func() (bool, error) { + return i.ipt.ChainExists(table, chain) + } + result, err := backoff.RetryNotifyWithData(operation, i.backoff, logRetryError) + if err != nil { + return true, err + } + return result, nil } // HasRandomFully implements IPTablesIface interface by calling iptables package func (i ipTables) HasRandomFully() bool { return i.ipt.HasRandomFully() } + +func logRetryError(err error, t time.Duration) { + log.WithError(err).Errorf("Another app is currently holding the xtables lock. Retrying in %f seconds", t.Seconds()) +} diff --git a/pkg/networkutils/network.go b/pkg/networkutils/network.go index c7a7f0a110..6c04c8a8e1 100644 --- a/pkg/networkutils/network.go +++ b/pkg/networkutils/network.go @@ -204,7 +204,7 @@ func New() NetworkAPIs { netLink: netlinkwrapper.NewNetLink(), ns: nswrapper.NewNS(), newIptables: func(IPProtocol iptables.Protocol) (iptableswrapper.IPTablesIface, error) { - ipt, err := iptables.NewWithProtocol(IPProtocol) + ipt, err := iptableswrapper.NewIPTables(IPProtocol) return ipt, err }, } From 2aaa3c89b2a14145770cf04f5db978934c71aec2 Mon Sep 17 00:00:00 2001 From: phuhung273 Date: Sat, 26 Oct 2024 18:54:02 +0700 Subject: [PATCH 2/5] fix syntax --- pkg/iptableswrapper/iptables.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/pkg/iptableswrapper/iptables.go b/pkg/iptableswrapper/iptables.go index 91046586d0..3117f6d24d 100644 --- a/pkg/iptableswrapper/iptables.go +++ b/pkg/iptableswrapper/iptables.go @@ -53,10 +53,8 @@ func NewIPTables(protocol iptables.Protocol) (IPTablesIface, error) { return nil, err } return &ipTables{ - ipt: ipt, - backoff: &backoff.ExponentialBackOff{ - MaxElapsedTime: 0, // Never stop retrying - }, + ipt: ipt, + backoff: backoff.NewExponentialBackOff(backoff.WithMaxElapsedTime(0)), // Never stop retrying as backward compatibility }, nil } From d393ca102be5b32f165a305758bfef746de64110 Mon Sep 17 00:00:00 2001 From: phuhung273 Date: Sun, 27 Oct 2024 09:40:55 +0700 Subject: [PATCH 3/5] unit test --- cmd/egress-cni-plugin/egressContext.go | 5 +- pkg/networkutils/iptables.go | 141 +++++++++++++++ pkg/networkutils/iptables_test.go | 228 +++++++++++++++++++++++++ pkg/networkutils/network.go | 2 +- 4 files changed, 373 insertions(+), 3 deletions(-) create mode 100644 pkg/networkutils/iptables.go create mode 100644 pkg/networkutils/iptables_test.go diff --git a/cmd/egress-cni-plugin/egressContext.go b/cmd/egress-cni-plugin/egressContext.go index fce7d3480e..97411bf1da 100644 --- a/cmd/egress-cni-plugin/egressContext.go +++ b/cmd/egress-cni-plugin/egressContext.go @@ -29,6 +29,7 @@ import ( "github.com/aws/amazon-vpc-cni-k8s/pkg/hostipamwrapper" "github.com/aws/amazon-vpc-cni-k8s/pkg/iptableswrapper" "github.com/aws/amazon-vpc-cni-k8s/pkg/netlinkwrapper" + "github.com/aws/amazon-vpc-cni-k8s/pkg/networkutils" "github.com/aws/amazon-vpc-cni-k8s/pkg/nswrapper" "github.com/aws/amazon-vpc-cni-k8s/pkg/procsyswrapper" "github.com/aws/amazon-vpc-cni-k8s/pkg/utils/cniutils" @@ -82,7 +83,7 @@ func NewEgressAddContext(nsPath, ifName string) egressContext { ArgsIfName: ifName, Veth: vethwrapper.NewSetupVeth(), IptCreator: func(protocol iptables.Protocol) (iptableswrapper.IPTablesIface, error) { - return iptableswrapper.NewIPTables(protocol) + return networkutils.NewIPTables(protocol) }, } } @@ -95,7 +96,7 @@ func NewEgressDelContext(nsPath string) egressContext { Ns: nswrapper.NewNS(), NsPath: nsPath, IptCreator: func(protocol iptables.Protocol) (iptableswrapper.IPTablesIface, error) { - return iptableswrapper.NewIPTables(protocol) + return networkutils.NewIPTables(protocol) }, } } diff --git a/pkg/networkutils/iptables.go b/pkg/networkutils/iptables.go new file mode 100644 index 0000000000..2fde1c02f8 --- /dev/null +++ b/pkg/networkutils/iptables.go @@ -0,0 +1,141 @@ +// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"). You may +// not use this file except in compliance with the License. A copy of the +// License is located at +// +// http://aws.amazon.com/apache2.0/ +// +// or in the "license" file accompanying this file. This file 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 networkutils is a collection of iptables and netlink functions +package networkutils + +import ( + "time" + + "github.com/aws/amazon-vpc-cni-k8s/pkg/iptableswrapper" + "github.com/cenkalti/backoff/v4" + "github.com/coreos/go-iptables/iptables" +) + +type ipTables struct { + ipt iptableswrapper.IPTablesIface + backoff *backoff.ExponentialBackOff +} + +// NewIPTables return a ipTables struct that implements IPTablesIface +func NewIPTables(protocol iptables.Protocol) (iptableswrapper.IPTablesIface, error) { + ipt, err := iptables.New(iptables.IPFamily(protocol), iptables.Timeout(1)) + if err != nil { + return nil, err + } + return &ipTables{ + ipt: ipt, + backoff: backoff.NewExponentialBackOff(backoff.WithMaxElapsedTime(0)), // Never stop retrying as backward compatibility + }, nil +} + +func (i ipTables) Exists(table, chain string, rulespec ...string) (bool, error) { + operation := func() (bool, error) { + return i.ipt.Exists(table, chain, rulespec...) + } + result, err := backoff.RetryNotifyWithData(operation, i.backoff, logRetryError) + if err != nil { + return true, err + } + return result, nil +} + +func (i ipTables) Insert(table, chain string, pos int, rulespec ...string) error { + operation := func() error { + return i.ipt.Insert(table, chain, pos, rulespec...) + } + return backoff.RetryNotify(operation, i.backoff, logRetryError) +} + +func (i ipTables) Append(table, chain string, rulespec ...string) error { + operation := func() error { + return i.ipt.Append(table, chain, rulespec...) + } + return backoff.RetryNotify(operation, i.backoff, logRetryError) +} + +func (i ipTables) AppendUnique(table, chain string, rulespec ...string) error { + operation := func() error { + return i.ipt.AppendUnique(table, chain, rulespec...) + } + return backoff.RetryNotify(operation, i.backoff, logRetryError) +} + +func (i ipTables) Delete(table, chain string, rulespec ...string) error { + operation := func() error { + return i.ipt.Delete(table, chain, rulespec...) + } + return backoff.RetryNotify(operation, i.backoff, logRetryError) +} + +func (i ipTables) List(table, chain string) ([]string, error) { + operation := func() ([]string, error) { + return i.ipt.List(table, chain) + } + result, err := backoff.RetryNotifyWithData(operation, i.backoff, logRetryError) + if err != nil { + return nil, err + } + return result, nil +} + +func (i ipTables) NewChain(table, chain string) error { + operation := func() error { + return i.ipt.NewChain(table, chain) + } + return backoff.RetryNotify(operation, i.backoff, logRetryError) +} + +func (i ipTables) ClearChain(table, chain string) error { + operation := func() error { + return i.ipt.ClearChain(table, chain) + } + return backoff.RetryNotify(operation, i.backoff, logRetryError) +} + +func (i ipTables) DeleteChain(table, chain string) error { + operation := func() error { + return i.ipt.DeleteChain(table, chain) + } + return backoff.RetryNotify(operation, i.backoff, logRetryError) +} + +func (i ipTables) ListChains(table string) ([]string, error) { + operation := func() ([]string, error) { + return i.ipt.ListChains(table) + } + result, err := backoff.RetryNotifyWithData(operation, i.backoff, logRetryError) + if err != nil { + return nil, err + } + return result, nil +} + +func (i ipTables) ChainExists(table, chain string) (bool, error) { + operation := func() (bool, error) { + return i.ipt.ChainExists(table, chain) + } + result, err := backoff.RetryNotifyWithData(operation, i.backoff, logRetryError) + if err != nil { + return true, err + } + return result, nil +} + +func (i ipTables) HasRandomFully() bool { + return i.ipt.HasRandomFully() +} + +func logRetryError(err error, t time.Duration) { + log.Errorf("Another app is currently holding the xtables lock. Retrying in %f seconds", t.Seconds()) +} diff --git a/pkg/networkutils/iptables_test.go b/pkg/networkutils/iptables_test.go new file mode 100644 index 0000000000..baa99d1a8f --- /dev/null +++ b/pkg/networkutils/iptables_test.go @@ -0,0 +1,228 @@ +// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"). You may +// not use this file except in compliance with the License. A copy of the +// License is located at +// +// http://aws.amazon.com/apache2.0/ +// +// or in the "license" file accompanying this file. This file 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 networkutils is a collection of iptables and netlink functions +package networkutils + +import ( + "crypto/rand" + "math/big" + "testing" + + mock_iptables "github.com/aws/amazon-vpc-cni-k8s/pkg/iptableswrapper/mocks" + "github.com/cenkalti/backoff/v4" + "github.com/golang/mock/gomock" + "github.com/stretchr/testify/assert" +) + +const ( + address1 = "203.0.113.1/32" + address2 = "203.0.113.2/32" + subnet1 = "192.0.2.0/24" +) + +func randChain(t *testing.T) string { + n, err := rand.Int(rand.Reader, big.NewInt(1000000)) + if err != nil { + t.Fatalf("Failed to generate random chain name: %v", err) + } + + return "TEST-" + n.String() +} + +func setupIPTTest(t *testing.T) (*gomock.Controller, + string, + string, + *backoff.ExponentialBackOff) { + return gomock.NewController(t), + "filter", + randChain(t), + backoff.NewExponentialBackOff(backoff.WithMaxElapsedTime(0)) +} + +func TestExists(t *testing.T) { + ctrl, table, chain, expBackoff := setupIPTTest(t) + mockIptable := mock_iptables.NewMockIPTablesIface(ctrl) + + mockIptable.EXPECT().Exists(table, chain, "-s", subnet1, "-d", address2, "-j", "ACCEPT").Return(false, nil) + + ipt := &ipTables{ + ipt: mockIptable, + backoff: expBackoff, + } + + exists, _ := ipt.Exists(table, chain, "-s", subnet1, "-d", address2, "-j", "ACCEPT") + assert.Equal(t, exists, false) +} + +func TestInsert(t *testing.T) { + ctrl, table, chain, expBackoff := setupIPTTest(t) + mockIptable := mock_iptables.NewMockIPTablesIface(ctrl) + + mockIptable.EXPECT().Insert(table, chain, 2, "-s", subnet1, "-d", address2, "-j", "ACCEPT").Return(nil) + + ipt := &ipTables{ + ipt: mockIptable, + backoff: expBackoff, + } + + err := ipt.Insert(table, chain, 2, "-s", subnet1, "-d", address2, "-j", "ACCEPT") + assert.NoError(t, err) +} + +func TestAppend(t *testing.T) { + ctrl, table, chain, expBackoff := setupIPTTest(t) + mockIptable := mock_iptables.NewMockIPTablesIface(ctrl) + + mockIptable.EXPECT().Append(table, chain, "-s", subnet1, "-d", address2, "-j", "ACCEPT").Return(nil) + + ipt := &ipTables{ + ipt: mockIptable, + backoff: expBackoff, + } + + err := ipt.Append(table, chain, "-s", subnet1, "-d", address2, "-j", "ACCEPT") + assert.NoError(t, err) +} + +func TestAppendUnique(t *testing.T) { + ctrl, table, chain, expBackoff := setupIPTTest(t) + mockIptable := mock_iptables.NewMockIPTablesIface(ctrl) + + mockIptable.EXPECT().AppendUnique(table, chain, "-s", subnet1, "-d", address2, "-j", "ACCEPT").Return(nil) + + ipt := &ipTables{ + ipt: mockIptable, + backoff: expBackoff, + } + + err := ipt.AppendUnique(table, chain, "-s", subnet1, "-d", address2, "-j", "ACCEPT") + assert.NoError(t, err) +} + +func TestDelete(t *testing.T) { + ctrl, table, chain, expBackoff := setupIPTTest(t) + mockIptable := mock_iptables.NewMockIPTablesIface(ctrl) + + mockIptable.EXPECT().Delete(table, chain, "-s", subnet1, "-d", address2, "-j", "ACCEPT").Return(nil) + + ipt := &ipTables{ + ipt: mockIptable, + backoff: expBackoff, + } + + err := ipt.Delete(table, chain, "-s", subnet1, "-d", address2, "-j", "ACCEPT") + assert.NoError(t, err) +} + +func TestList(t *testing.T) { + ctrl, table, chain, expBackoff := setupIPTTest(t) + mockIptable := mock_iptables.NewMockIPTablesIface(ctrl) + + expected := []string{ + "-N " + chain, + "-A " + chain + " -s " + address1 + " -d " + subnet1 + " -j ACCEPT", + "-A " + chain + " -s " + address2 + " -d " + subnet1 + " -j ACCEPT", + } + + mockIptable.EXPECT().List(table, chain).Return(expected, nil) + + ipt := &ipTables{ + ipt: mockIptable, + backoff: expBackoff, + } + + result, _ := ipt.List(table, chain) + assert.Equal(t, result, expected) +} + +func TestNewChain(t *testing.T) { + ctrl, table, chain, expBackoff := setupIPTTest(t) + mockIptable := mock_iptables.NewMockIPTablesIface(ctrl) + + mockIptable.EXPECT().NewChain(table, chain).Return(nil) + + ipt := &ipTables{ + ipt: mockIptable, + backoff: expBackoff, + } + + err := ipt.NewChain(table, chain) + assert.NoError(t, err) +} + +func TestClearChain(t *testing.T) { + ctrl, table, chain, expBackoff := setupIPTTest(t) + mockIptable := mock_iptables.NewMockIPTablesIface(ctrl) + + mockIptable.EXPECT().ClearChain(table, chain).Return(nil) + + ipt := &ipTables{ + ipt: mockIptable, + backoff: expBackoff, + } + + err := ipt.ClearChain(table, chain) + assert.NoError(t, err) +} + +func TestDeleteChain(t *testing.T) { + ctrl, table, chain, expBackoff := setupIPTTest(t) + mockIptable := mock_iptables.NewMockIPTablesIface(ctrl) + + mockIptable.EXPECT().DeleteChain(table, chain).Return(nil) + + ipt := &ipTables{ + ipt: mockIptable, + backoff: expBackoff, + } + + err := ipt.DeleteChain(table, chain) + assert.NoError(t, err) +} + +func TestListChains(t *testing.T) { + ctrl, table, _, expBackoff := setupIPTTest(t) + mockIptable := mock_iptables.NewMockIPTablesIface(ctrl) + + expected := []string{ + "filter", + "input", + } + + mockIptable.EXPECT().ListChains(table).Return(expected, nil) + + ipt := &ipTables{ + ipt: mockIptable, + backoff: expBackoff, + } + + result, _ := ipt.ListChains(table) + assert.Equal(t, expected, result) +} + +func TestChainExists(t *testing.T) { + ctrl, table, chain, expBackoff := setupIPTTest(t) + + mockIptable := mock_iptables.NewMockIPTablesIface(ctrl) + + mockIptable.EXPECT().ChainExists(table, chain).Return(true, nil) + + ipt := &ipTables{ + ipt: mockIptable, + backoff: expBackoff, + } + + exists, _ := ipt.ChainExists(table, chain) + assert.Equal(t, exists, true) +} diff --git a/pkg/networkutils/network.go b/pkg/networkutils/network.go index 6c04c8a8e1..6ccf1198c9 100644 --- a/pkg/networkutils/network.go +++ b/pkg/networkutils/network.go @@ -204,7 +204,7 @@ func New() NetworkAPIs { netLink: netlinkwrapper.NewNetLink(), ns: nswrapper.NewNS(), newIptables: func(IPProtocol iptables.Protocol) (iptableswrapper.IPTablesIface, error) { - ipt, err := iptableswrapper.NewIPTables(IPProtocol) + ipt, err := NewIPTables(IPProtocol) return ipt, err }, } From 6652e1f6d1d62bbaed5994081408840bdf86beea Mon Sep 17 00:00:00 2001 From: phuhung273 Date: Sun, 27 Oct 2024 09:45:15 +0700 Subject: [PATCH 4/5] restore iptableswrapper --- pkg/iptableswrapper/iptables.go | 92 ++++++--------------------------- 1 file changed, 15 insertions(+), 77 deletions(-) diff --git a/pkg/iptableswrapper/iptables.go b/pkg/iptableswrapper/iptables.go index 3117f6d24d..ce1ab24343 100644 --- a/pkg/iptableswrapper/iptables.go +++ b/pkg/iptableswrapper/iptables.go @@ -14,14 +14,7 @@ // Package iptableswrapper is a wrapper interface for the iptables package package iptableswrapper -import ( - "time" - - log "github.com/sirupsen/logrus" - - "github.com/cenkalti/backoff/v4" - "github.com/coreos/go-iptables/iptables" -) +import "github.com/coreos/go-iptables/iptables" // IPTablesIface is an interface created to make code unit testable. // Both the iptables package version and mocked version implement the same interface @@ -42,131 +35,76 @@ type IPTablesIface interface { // ipTables is a struct that implements IPTablesIface using iptables package. type ipTables struct { - ipt *iptables.IPTables - backoff *backoff.ExponentialBackOff + ipt *iptables.IPTables } // NewIPTables return a ipTables struct that implements IPTablesIface func NewIPTables(protocol iptables.Protocol) (IPTablesIface, error) { - ipt, err := iptables.New(iptables.IPFamily(protocol), iptables.Timeout(1)) + ipt, err := iptables.NewWithProtocol(protocol) if err != nil { return nil, err } return &ipTables{ - ipt: ipt, - backoff: backoff.NewExponentialBackOff(backoff.WithMaxElapsedTime(0)), // Never stop retrying as backward compatibility + ipt: ipt, }, nil } // Exists implements IPTablesIface interface by calling iptables package func (i ipTables) Exists(table, chain string, rulespec ...string) (bool, error) { - operation := func() (bool, error) { - return i.ipt.Exists(table, chain, rulespec...) - } - result, err := backoff.RetryNotifyWithData(operation, i.backoff, logRetryError) - if err != nil { - return true, err - } - return result, nil + return i.ipt.Exists(table, chain, rulespec...) } // Insert implements IPTablesIface interface by calling iptables package func (i ipTables) Insert(table, chain string, pos int, rulespec ...string) error { - operation := func() error { - return i.ipt.Insert(table, chain, pos, rulespec...) - } - return backoff.RetryNotify(operation, i.backoff, logRetryError) + return i.ipt.Insert(table, chain, pos, rulespec...) } // Append implements IPTablesIface interface by calling iptables package func (i ipTables) Append(table, chain string, rulespec ...string) error { - operation := func() error { - return i.ipt.Append(table, chain, rulespec...) - } - return backoff.RetryNotify(operation, i.backoff, logRetryError) + return i.ipt.Append(table, chain, rulespec...) } // AppendUnique implements IPTablesIface interface by calling iptables package func (i ipTables) AppendUnique(table, chain string, rulespec ...string) error { - operation := func() error { - return i.ipt.AppendUnique(table, chain, rulespec...) - } - return backoff.RetryNotify(operation, i.backoff, logRetryError) + return i.ipt.AppendUnique(table, chain, rulespec...) } // Delete implements IPTablesIface interface by calling iptables package func (i ipTables) Delete(table, chain string, rulespec ...string) error { - operation := func() error { - return i.ipt.Delete(table, chain, rulespec...) - } - return backoff.RetryNotify(operation, i.backoff, logRetryError) + return i.ipt.Delete(table, chain, rulespec...) } // List implements IPTablesIface interface by calling iptables package func (i ipTables) List(table, chain string) ([]string, error) { - operation := func() ([]string, error) { - return i.ipt.List(table, chain) - } - result, err := backoff.RetryNotifyWithData(operation, i.backoff, logRetryError) - if err != nil { - return nil, err - } - return result, nil + return i.ipt.List(table, chain) } // NewChain implements IPTablesIface interface by calling iptables package func (i ipTables) NewChain(table, chain string) error { - operation := func() error { - return i.ipt.NewChain(table, chain) - } - return backoff.RetryNotify(operation, i.backoff, logRetryError) + return i.ipt.NewChain(table, chain) } // ClearChain implements IPTablesIface interface by calling iptables package func (i ipTables) ClearChain(table, chain string) error { - operation := func() error { - return i.ipt.ClearChain(table, chain) - } - return backoff.RetryNotify(operation, i.backoff, logRetryError) + return i.ipt.ClearChain(table, chain) } // DeleteChain implements IPTablesIface interface by calling iptables package func (i ipTables) DeleteChain(table, chain string) error { - operation := func() error { - return i.ipt.DeleteChain(table, chain) - } - return backoff.RetryNotify(operation, i.backoff, logRetryError) + return i.ipt.DeleteChain(table, chain) } // ListChains implements IPTablesIface interface by calling iptables package func (i ipTables) ListChains(table string) ([]string, error) { - operation := func() ([]string, error) { - return i.ipt.ListChains(table) - } - result, err := backoff.RetryNotifyWithData(operation, i.backoff, logRetryError) - if err != nil { - return nil, err - } - return result, nil + return i.ipt.ListChains(table) } // ChainExists implements IPTablesIface interface by calling iptables package func (i ipTables) ChainExists(table, chain string) (bool, error) { - operation := func() (bool, error) { - return i.ipt.ChainExists(table, chain) - } - result, err := backoff.RetryNotifyWithData(operation, i.backoff, logRetryError) - if err != nil { - return true, err - } - return result, nil + return i.ipt.ChainExists(table, chain) } // HasRandomFully implements IPTablesIface interface by calling iptables package func (i ipTables) HasRandomFully() bool { return i.ipt.HasRandomFully() } - -func logRetryError(err error, t time.Duration) { - log.WithError(err).Errorf("Another app is currently holding the xtables lock. Retrying in %f seconds", t.Seconds()) -} From 4878431fe0dc4ff414058cdf25157fbc34b1c391 Mon Sep 17 00:00:00 2001 From: phuhung273 Date: Sun, 27 Oct 2024 11:43:32 +0700 Subject: [PATCH 5/5] backoff test --- pkg/networkutils/iptables_test.go | 71 ++++++++++++++++++++++++++----- 1 file changed, 60 insertions(+), 11 deletions(-) diff --git a/pkg/networkutils/iptables_test.go b/pkg/networkutils/iptables_test.go index baa99d1a8f..46a08d6996 100644 --- a/pkg/networkutils/iptables_test.go +++ b/pkg/networkutils/iptables_test.go @@ -22,6 +22,7 @@ import ( mock_iptables "github.com/aws/amazon-vpc-cni-k8s/pkg/iptableswrapper/mocks" "github.com/cenkalti/backoff/v4" "github.com/golang/mock/gomock" + "github.com/pkg/errors" "github.com/stretchr/testify/assert" ) @@ -31,6 +32,11 @@ const ( subnet1 = "192.0.2.0/24" ) +var ( + timeoutError = errors.New("timeout") + permissionDeniedError = errors.New("permission denied") +) + func randChain(t *testing.T) string { n, err := rand.Int(rand.Reader, big.NewInt(1000000)) if err != nil { @@ -54,7 +60,10 @@ func TestExists(t *testing.T) { ctrl, table, chain, expBackoff := setupIPTTest(t) mockIptable := mock_iptables.NewMockIPTablesIface(ctrl) - mockIptable.EXPECT().Exists(table, chain, "-s", subnet1, "-d", address2, "-j", "ACCEPT").Return(false, nil) + gomock.InOrder( + mockIptable.EXPECT().Exists(table, chain, "-s", subnet1, "-d", address2, "-j", "ACCEPT").Return(false, timeoutError), + mockIptable.EXPECT().Exists(table, chain, "-s", subnet1, "-d", address2, "-j", "ACCEPT").Return(false, nil), + ) ipt := &ipTables{ ipt: mockIptable, @@ -69,7 +78,12 @@ func TestInsert(t *testing.T) { ctrl, table, chain, expBackoff := setupIPTTest(t) mockIptable := mock_iptables.NewMockIPTablesIface(ctrl) - mockIptable.EXPECT().Insert(table, chain, 2, "-s", subnet1, "-d", address2, "-j", "ACCEPT").Return(nil) + gomock.InOrder( + mockIptable.EXPECT().Insert(table, chain, 2, "-s", subnet1, "-d", address2, "-j", "ACCEPT").Return(timeoutError), + mockIptable.EXPECT().Insert(table, chain, 2, "-s", subnet1, "-d", address2, "-j", "ACCEPT").Return(permissionDeniedError), + mockIptable.EXPECT().Insert(table, chain, 2, "-s", subnet1, "-d", address2, "-j", "ACCEPT").Return(timeoutError), + mockIptable.EXPECT().Insert(table, chain, 2, "-s", subnet1, "-d", address2, "-j", "ACCEPT").Return(nil), + ) ipt := &ipTables{ ipt: mockIptable, @@ -84,7 +98,12 @@ func TestAppend(t *testing.T) { ctrl, table, chain, expBackoff := setupIPTTest(t) mockIptable := mock_iptables.NewMockIPTablesIface(ctrl) - mockIptable.EXPECT().Append(table, chain, "-s", subnet1, "-d", address2, "-j", "ACCEPT").Return(nil) + gomock.InOrder( + mockIptable.EXPECT().Append(table, chain, "-s", subnet1, "-d", address2, "-j", "ACCEPT").Return(timeoutError), + mockIptable.EXPECT().Append(table, chain, "-s", subnet1, "-d", address2, "-j", "ACCEPT").Return(permissionDeniedError), + mockIptable.EXPECT().Append(table, chain, "-s", subnet1, "-d", address2, "-j", "ACCEPT").Return(timeoutError), + mockIptable.EXPECT().Append(table, chain, "-s", subnet1, "-d", address2, "-j", "ACCEPT").Return(nil), + ) ipt := &ipTables{ ipt: mockIptable, @@ -99,7 +118,10 @@ func TestAppendUnique(t *testing.T) { ctrl, table, chain, expBackoff := setupIPTTest(t) mockIptable := mock_iptables.NewMockIPTablesIface(ctrl) - mockIptable.EXPECT().AppendUnique(table, chain, "-s", subnet1, "-d", address2, "-j", "ACCEPT").Return(nil) + gomock.InOrder( + mockIptable.EXPECT().AppendUnique(table, chain, "-s", subnet1, "-d", address2, "-j", "ACCEPT").Return(timeoutError), + mockIptable.EXPECT().AppendUnique(table, chain, "-s", subnet1, "-d", address2, "-j", "ACCEPT").Return(nil), + ) ipt := &ipTables{ ipt: mockIptable, @@ -114,7 +136,10 @@ func TestDelete(t *testing.T) { ctrl, table, chain, expBackoff := setupIPTTest(t) mockIptable := mock_iptables.NewMockIPTablesIface(ctrl) - mockIptable.EXPECT().Delete(table, chain, "-s", subnet1, "-d", address2, "-j", "ACCEPT").Return(nil) + gomock.InOrder( + mockIptable.EXPECT().Delete(table, chain, "-s", subnet1, "-d", address2, "-j", "ACCEPT").Return(permissionDeniedError), + mockIptable.EXPECT().Delete(table, chain, "-s", subnet1, "-d", address2, "-j", "ACCEPT").Return(nil), + ) ipt := &ipTables{ ipt: mockIptable, @@ -135,7 +160,11 @@ func TestList(t *testing.T) { "-A " + chain + " -s " + address2 + " -d " + subnet1 + " -j ACCEPT", } - mockIptable.EXPECT().List(table, chain).Return(expected, nil) + gomock.InOrder( + mockIptable.EXPECT().List(table, chain).Return(nil, timeoutError), + mockIptable.EXPECT().List(table, chain).Return(nil, timeoutError), + mockIptable.EXPECT().List(table, chain).Return(expected, nil), + ) ipt := &ipTables{ ipt: mockIptable, @@ -150,7 +179,11 @@ func TestNewChain(t *testing.T) { ctrl, table, chain, expBackoff := setupIPTTest(t) mockIptable := mock_iptables.NewMockIPTablesIface(ctrl) - mockIptable.EXPECT().NewChain(table, chain).Return(nil) + gomock.InOrder( + mockIptable.EXPECT().NewChain(table, chain).Return(permissionDeniedError), + mockIptable.EXPECT().NewChain(table, chain).Return(timeoutError), + mockIptable.EXPECT().NewChain(table, chain).Return(nil), + ) ipt := &ipTables{ ipt: mockIptable, @@ -165,7 +198,11 @@ func TestClearChain(t *testing.T) { ctrl, table, chain, expBackoff := setupIPTTest(t) mockIptable := mock_iptables.NewMockIPTablesIface(ctrl) - mockIptable.EXPECT().ClearChain(table, chain).Return(nil) + gomock.InOrder( + mockIptable.EXPECT().ClearChain(table, chain).Return(permissionDeniedError), + mockIptable.EXPECT().ClearChain(table, chain).Return(timeoutError), + mockIptable.EXPECT().ClearChain(table, chain).Return(nil), + ) ipt := &ipTables{ ipt: mockIptable, @@ -180,7 +217,12 @@ func TestDeleteChain(t *testing.T) { ctrl, table, chain, expBackoff := setupIPTTest(t) mockIptable := mock_iptables.NewMockIPTablesIface(ctrl) - mockIptable.EXPECT().DeleteChain(table, chain).Return(nil) + gomock.InOrder( + mockIptable.EXPECT().DeleteChain(table, chain).Return(permissionDeniedError), + mockIptable.EXPECT().DeleteChain(table, chain).Return(timeoutError), + mockIptable.EXPECT().DeleteChain(table, chain).Return(permissionDeniedError), + mockIptable.EXPECT().DeleteChain(table, chain).Return(nil), + ) ipt := &ipTables{ ipt: mockIptable, @@ -200,7 +242,10 @@ func TestListChains(t *testing.T) { "input", } - mockIptable.EXPECT().ListChains(table).Return(expected, nil) + gomock.InOrder( + mockIptable.EXPECT().ListChains(table).Return(nil, timeoutError), + mockIptable.EXPECT().ListChains(table).Return(expected, nil), + ) ipt := &ipTables{ ipt: mockIptable, @@ -216,7 +261,11 @@ func TestChainExists(t *testing.T) { mockIptable := mock_iptables.NewMockIPTablesIface(ctrl) - mockIptable.EXPECT().ChainExists(table, chain).Return(true, nil) + gomock.InOrder( + mockIptable.EXPECT().ChainExists(table, chain).Return(false, timeoutError), + mockIptable.EXPECT().ChainExists(table, chain).Return(false, timeoutError), + mockIptable.EXPECT().ChainExists(table, chain).Return(true, nil), + ) ipt := &ipTables{ ipt: mockIptable,