From 2d40d5e8e8d0ee63bdfff45bbd7f3dd276453ea3 Mon Sep 17 00:00:00 2001 From: Tom Pantelis Date: Mon, 15 Apr 2024 12:37:23 -0400 Subject: [PATCH 1/3] Change ipset to use admiral's command wrapper ...instead of the K8s exec wrapper as the former provides a fake implementation that is simpler and easer to use. Signed-off-by: Tom Pantelis --- pkg/ipset/ipset.go | 30 ++++++++++----------------- pkg/packetfilter/iptables/iptables.go | 3 +-- 2 files changed, 12 insertions(+), 21 deletions(-) diff --git a/pkg/ipset/ipset.go b/pkg/ipset/ipset.go index 8d9026845..01bdcfcd8 100644 --- a/pkg/ipset/ipset.go +++ b/pkg/ipset/ipset.go @@ -53,13 +53,14 @@ import ( "bytes" "fmt" "net" + "os/exec" "regexp" "strconv" "strings" "github.com/pkg/errors" + "github.com/submariner-io/admiral/pkg/command" "github.com/submariner-io/admiral/pkg/log" - utilexec "k8s.io/utils/exec" logf "sigs.k8s.io/controller-runtime/pkg/log" ) @@ -331,27 +332,17 @@ func (e *Entry) String() string { return "" } -type runner struct { - exec utilexec.Interface -} - -var NewFunc func() Interface +type runner struct{} // New returns a new Interface which will exec ipset. -func New(exec utilexec.Interface) Interface { - if NewFunc != nil { - return NewFunc() - } - - return &runner{ - exec: exec, - } +func New() Interface { + return &runner{} } func (runner *runner) runWithOutput(args []string, errFormat string, a ...interface{}) (string, error) { logger.V(log.DEBUG).Infof("Running ipset %v", args) - out, err := runner.exec.Command(IPSetCmd, args...).CombinedOutput() + out, err := command.New(exec.Command(IPSetCmd, args...)).CombinedOutput() if err != nil { return "", fmt.Errorf("%s: %w (%s)", fmt.Sprintf(errFormat, a...), err, out) } @@ -545,13 +536,14 @@ func (runner *runner) ListAllSetInfo() (string, error) { // GetVersion returns the version string. func (runner *runner) GetVersion() (string, error) { - return getIPSetVersionString(runner.exec) + return getIPSetVersionString() } // getIPSetVersionString runs "ipset --version" to get the version string in the form of "X.Y", i.e "6.19". -func getIPSetVersionString(exec utilexec.Interface) (string, error) { - cmd := exec.Command(IPSetCmd, "--version") - cmd.SetStdin(bytes.NewReader([]byte{})) +func getIPSetVersionString() (string, error) { + osCmd := exec.Command(IPSetCmd, "--version") + osCmd.Stdin = bytes.NewReader([]byte{}) + cmd := command.New(osCmd) cmdBytes, err := cmd.CombinedOutput() if err != nil { diff --git a/pkg/packetfilter/iptables/iptables.go b/pkg/packetfilter/iptables/iptables.go index 38de56417..a8ccb0761 100644 --- a/pkg/packetfilter/iptables/iptables.go +++ b/pkg/packetfilter/iptables/iptables.go @@ -26,7 +26,6 @@ import ( "github.com/submariner-io/admiral/pkg/log" "github.com/submariner-io/submariner/pkg/ipset" "github.com/submariner-io/submariner/pkg/packetfilter" - utilexec "k8s.io/utils/exec" logf "sigs.k8s.io/controller-runtime/pkg/log" ) @@ -79,7 +78,7 @@ func New() (packetfilter.Driver, error) { return nil, errors.Wrap(err, "error creating IP tables") } - ipSetIface := ipset.New(utilexec.New()) + ipSetIface := ipset.New() return &packetFilter{ ipt: ipt, From ffb33065c400450be11a577924c90e5139783b5f Mon Sep 17 00:00:00 2001 From: Tom Pantelis Date: Mon, 15 Apr 2024 17:05:49 -0400 Subject: [PATCH 2/3] Retry ipset DestroySet/DestroyAllSets if error indicates in use This handles the case where a set is destroyed right after associated IP table rule(s) are deleted where the latter may not be complete yet. Retry up to 2 seconds. Fixes https://github.com/submariner-io/submariner/issues/1746 Signed-off-by: Tom Pantelis --- pkg/ipset/ipset.go | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/pkg/ipset/ipset.go b/pkg/ipset/ipset.go index 01bdcfcd8..618185ab2 100644 --- a/pkg/ipset/ipset.go +++ b/pkg/ipset/ipset.go @@ -57,10 +57,13 @@ import ( "regexp" "strconv" "strings" + "time" "github.com/pkg/errors" "github.com/submariner-io/admiral/pkg/command" "github.com/submariner-io/admiral/pkg/log" + "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/util/retry" logf "sigs.k8s.io/controller-runtime/pkg/log" ) @@ -480,9 +483,24 @@ func (runner *runner) FlushSet(set string) error { return err } +func (runner *runner) retryIfInUse(args []string, errFormat string, a ...interface{}) error { + backoff := wait.Backoff{ + Duration: 100 * time.Millisecond, + Factor: 1.0, + Steps: 20, + } + + //nolint:wrapcheck // No need to wrap. + return retry.OnError(backoff, func(err error) bool { + return strings.Contains(err.Error(), "is in use") + }, func() error { + return runner.run(args, errFormat, a...) + }) +} + // DestroySet is used to destroy a named set. func (runner *runner) DestroySet(set string) error { - err := runner.run([]string{"destroy", set}, "error destroying set %q", set) + err := runner.retryIfInUse([]string{"destroy", set}, "error destroying set %q", set) if IsNotFoundError(err) { return nil } @@ -492,7 +510,7 @@ func (runner *runner) DestroySet(set string) error { // DestroyAllSets is used to destroy all sets. func (runner *runner) DestroyAllSets() error { - return runner.run([]string{"destroy"}, "error destroying all sets") + return runner.retryIfInUse([]string{"destroy"}, "error destroying all sets") } // ListSets list all set names from kernel. From d170b9145aceefb700dcc5c8d6ed91e6dae782cb Mon Sep 17 00:00:00 2001 From: Tom Pantelis Date: Mon, 15 Apr 2024 17:01:57 -0400 Subject: [PATCH 3/3] Add unit tests for ipset DestroySet/DestroyAllSets functions Signed-off-by: Tom Pantelis --- go.mod | 4 +- go.sum | 12 ++--- pkg/ipset/ipset_test.go | 101 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 109 insertions(+), 8 deletions(-) create mode 100644 pkg/ipset/ipset_test.go diff --git a/go.mod b/go.mod index db428a9c2..26265313f 100644 --- a/go.mod +++ b/go.mod @@ -15,7 +15,7 @@ require ( github.com/projectcalico/api v0.0.0-20230602153125-fb7148692637 github.com/prometheus-community/pro-bing v0.4.0 github.com/prometheus/client_golang v1.19.0 - github.com/submariner-io/admiral v0.18.0-m2 + github.com/submariner-io/admiral v0.18.0-m2.0.20240417062804-5cfcef6bc3ba github.com/submariner-io/shipyard v0.18.0-m2 github.com/vishvananda/netlink v1.2.1-beta.2 golang.org/x/net v0.24.0 @@ -27,7 +27,7 @@ require ( k8s.io/client-go v0.29.3 k8s.io/component-helpers v0.29.3 k8s.io/utils v0.0.0-20230726121419-3b25d923346b - sigs.k8s.io/controller-runtime v0.17.2 + sigs.k8s.io/controller-runtime v0.17.3 sigs.k8s.io/knftables v0.0.16 sigs.k8s.io/mcs-api v0.1.0 sigs.k8s.io/structured-merge-diff/v4 v4.4.1 diff --git a/go.sum b/go.sum index 1492fc54c..6f9a5bc20 100644 --- a/go.sum +++ b/go.sum @@ -503,8 +503,8 @@ github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO github.com/stretchr/testify v1.8.1/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4= github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk= github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo= -github.com/submariner-io/admiral v0.18.0-m2 h1:SGW+azCnbCAe/K1pAC1NeKJ3kR+poY1d99Gq8ZGAMsY= -github.com/submariner-io/admiral v0.18.0-m2/go.mod h1:yaPz+wX65Ew3iVulB3T7r/JqYUwcT6RxTS/oGnZaNTo= +github.com/submariner-io/admiral v0.18.0-m2.0.20240417062804-5cfcef6bc3ba h1:2ERBixskBiw1BsTtPt2XKKtAiUg/GofmxCwPqNNnL98= +github.com/submariner-io/admiral v0.18.0-m2.0.20240417062804-5cfcef6bc3ba/go.mod h1:GR4J3O4aLA5VXQyWcFgvfqn4ahWbfh93a2TIISPO4k8= github.com/submariner-io/shipyard v0.18.0-m2 h1:9jnyioyAvotl6GMb/6gHAFoEcOFmjR0ixUl4AqzJGiA= github.com/submariner-io/shipyard v0.18.0-m2/go.mod h1:gqN3jA+kUYv1CDS95Wopfj8vXCfBJmL/NupebzB8KBc= github.com/syndtr/gocapability v0.0.0-20200815063812-42c35b437635/go.mod h1:hkRG7XYTFWNJGYcbNJQlaLq0fg1yr4J4t/NcTQtrfww= @@ -783,8 +783,8 @@ k8s.io/api v0.29.3 h1:2ORfZ7+bGC3YJqGpV0KSDDEVf8hdGQ6A03/50vj8pmw= k8s.io/api v0.29.3/go.mod h1:y2yg2NTyHUUkIoTC+phinTnEa3KFM6RZ3szxt014a80= k8s.io/apiextensions-apiserver v0.18.2/go.mod h1:q3faSnRGmYimiocj6cHQ1I3WpLqmDgJFlKL37fC4ZvY= k8s.io/apiextensions-apiserver v0.18.4/go.mod h1:NYeyeYq4SIpFlPxSAB6jHPIdvu3hL0pc36wuRChybio= -k8s.io/apiextensions-apiserver v0.29.0 h1:0VuspFG7Hj+SxyF/Z/2T0uFbI5gb5LRgEyUVE3Q4lV0= -k8s.io/apiextensions-apiserver v0.29.0/go.mod h1:TKmpy3bTS0mr9pylH0nOt/QzQRrW7/h7yLdRForMZwc= +k8s.io/apiextensions-apiserver v0.29.2 h1:UK3xB5lOWSnhaCk0RFZ0LUacPZz9RY4wi/yt2Iu+btg= +k8s.io/apiextensions-apiserver v0.29.2/go.mod h1:aLfYjpA5p3OwtqNXQFkhJ56TB+spV8Gc4wfMhUA3/b8= k8s.io/apimachinery v0.18.2/go.mod h1:9SnR/e11v5IbyPCGbvJViimtJ0SwHG4nfZFjU77ftcA= k8s.io/apimachinery v0.18.4/go.mod h1:OaXp26zu/5J7p0f92ASynJa1pZo06YlV9fG7BoWbCko= k8s.io/apimachinery v0.29.3 h1:2tbx+5L7RNvqJjn7RIuIKu9XTsIZ9Z5wX2G22XAa5EU= @@ -819,8 +819,8 @@ k8s.io/utils v0.0.0-20230726121419-3b25d923346b h1:sgn3ZU783SCgtaSJjpcVVlRqd6GSn k8s.io/utils v0.0.0-20230726121419-3b25d923346b/go.mod h1:OLgZIPagt7ERELqWJFomSt595RzquPNLL48iOWgYOg0= sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.0.7/go.mod h1:PHgbrJT7lCHcxMU+mDHEm+nx46H4zuuHZkDP6icnhu0= sigs.k8s.io/controller-runtime v0.6.1/go.mod h1:XRYBPdbf5XJu9kpS84VJiZ7h/u1hF3gEORz0efEja7A= -sigs.k8s.io/controller-runtime v0.17.2 h1:FwHwD1CTUemg0pW2otk7/U5/i5m2ymzvOXdbeGOUvw0= -sigs.k8s.io/controller-runtime v0.17.2/go.mod h1:+MngTvIQQQhfXtwfdGw/UOQ/aIaqsYywfCINOtwMO/s= +sigs.k8s.io/controller-runtime v0.17.3 h1:65QmN7r3FWgTxDMz9fvGnO1kbf2nu+acg9p2R9oYYYk= +sigs.k8s.io/controller-runtime v0.17.3/go.mod h1:N0jpP5Lo7lMTF9aL56Z/B2oWBJjey6StQM0jRbKQXtY= sigs.k8s.io/controller-tools v0.3.0/go.mod h1:enhtKGfxZD1GFEoMgP8Fdbu+uKQ/cq1/WGJhdVChfvI= sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd h1:EDPBXCAspyGV4jQlpZSudPeMmr1bNJefnuqLsRAsHZo= sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd/go.mod h1:B8JuhiUyNFVKdsE8h686QcCxMaH6HrOAZj4vswFpcB0= diff --git a/pkg/ipset/ipset_test.go b/pkg/ipset/ipset_test.go new file mode 100644 index 000000000..3cfe6a7a9 --- /dev/null +++ b/pkg/ipset/ipset_test.go @@ -0,0 +1,101 @@ +/* +SPDX-License-Identifier: Apache-2.0 + +Copyright Contributors to the Submariner project. + +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 ipset_test + +import ( + "errors" + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + fakecommand "github.com/submariner-io/admiral/pkg/command/fake" + "github.com/submariner-io/submariner/pkg/ipset" +) + +const testSet = "test-set" + +var IPSetPathMatcher = HaveSuffix(ipset.IPSetCmd) + +func TestIPSet(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "IP Set Suite") +} + +var _ = Describe("DestroySet", func() { + t := newTestDriver() + + It("should invoke the correct command", func() { + err := t.ipSet.DestroySet(testSet) + Expect(err).To(Succeed()) + t.cmdExecutor.AwaitCommand(IPSetPathMatcher, "destroy", testSet) + }) + + When("the set does not exist", func() { + It("should succeed", func() { + t.cmdExecutor.SetupCommandOutputWithError("The set with the given name does not exist", + errors.New("exit status 1"), IPSetPathMatcher, "destroy") + err := t.ipSet.DestroySet(testSet) + Expect(err).To(Succeed()) + }) + }) + + When("the set is initially in use", func() { + It("should retry and succeed", func() { + t.cmdExecutor.SetupCommandOutputWithError("Set cannot be destroyed: it is in use by a kernel component", + errors.New("exit status 1"), IPSetPathMatcher, "destroy") + err := t.ipSet.DestroySet(testSet) + Expect(err).To(Succeed()) + }) + }) +}) + +var _ = Describe("DestroyAllSets", func() { + t := newTestDriver() + + It("should invoke the correct command", func() { + err := t.ipSet.DestroyAllSets() + Expect(err).To(Succeed()) + t.cmdExecutor.AwaitCommand(IPSetPathMatcher, "destroy") + }) + + When("a set is initially in use", func() { + It("should retry and succeed", func() { + t.cmdExecutor.SetupCommandOutputWithError("Set cannot be destroyed: it is in use by a kernel component", + errors.New("exit status 1"), IPSetPathMatcher, "destroy") + err := t.ipSet.DestroyAllSets() + Expect(err).To(Succeed()) + }) + }) +}) + +type testDriver struct { + cmdExecutor *fakecommand.Executor + ipSet ipset.Interface +} + +func newTestDriver() *testDriver { + t := &testDriver{} + + BeforeEach(func() { + t.cmdExecutor = fakecommand.New() + t.ipSet = ipset.New() + }) + + return t +}