Merge pull request #438 from tnqn/vlan-multi-del

Fix vlan plugin returning error when device is already removed

pkg/utils/sysctl/sysctl_linux.go

@@ -35,7 +35,7 @@ func Sysctl(name string, params ...string) (string, error) {
 }
 
 func getSysctl(name string) (string, error) {
-	fullName := filepath.Join("/proc/sys", strings.Replace(name, ".", "/", -1))
+	fullName := filepath.Join("/proc/sys", toNormalName(name))
 	fullName = filepath.Clean(fullName)
 	data, err := ioutil.ReadFile(fullName)
 	if err != nil {
@@ -46,7 +46,7 @@ func getSysctl(name string) (string, error) {
 }
 
 func setSysctl(name, value string) (string, error) {
-	fullName := filepath.Join("/proc/sys", strings.Replace(name, ".", "/", -1))
+	fullName := filepath.Join("/proc/sys", toNormalName(name))
 	fullName = filepath.Clean(fullName)
 	if err := ioutil.WriteFile(fullName, []byte(value), 0644); err != nil {
 		return "", err
@@ -54,3 +54,27 @@ func setSysctl(name, value string) (string, error) {
 
 	return getSysctl(name)
 }
+
+// Normalize names by using slash as separator
+// Sysctl names can use dots or slashes as separator:
+// - if dots are used, dots and slashes are interchanged.
+// - if slashes are used, slashes and dots are left intact.
+// Separator in use is determined by firt ocurrence.
+func toNormalName(name string) string {
+	interchange := false
+	for _, c := range name {
+		if c == '.' {
+			interchange = true
+			break
+		}
+		if c == '/' {
+			break
+		}
+	}
+
+	if interchange {
+		r := strings.NewReplacer(".", "/", "/", ".")
+		return r.Replace(name)
+	}
+	return name
+}

pkg/utils/sysctl/sysctl_linux_test.go

@@ -0,0 +1,113 @@
+// Copyright 2017-2020 CNI authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package sysctl
+
+import (
+	"fmt"
+	"math/rand"
+	"runtime"
+	"strings"
+
+	"github.com/containernetworking/plugins/pkg/ns"
+	"github.com/containernetworking/plugins/pkg/testutils"
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+	"github.com/vishvananda/netlink"
+)
+
+const (
+	sysctlDotKeyTemplate   = "net.ipv4.conf.%s.proxy_arp"
+	sysctlSlashKeyTemplate = "net/ipv4/conf/%s/proxy_arp"
+)
+
+var _ = Describe("Sysctl tests", func() {
+	var testIfaceName string
+	var cleanup func()
+
+	BeforeEach(func() {
+
+		// Save a reference to the original namespace,
+		// Add a new NS
+		currNs, err := ns.GetCurrentNS()
+		Expect(err).NotTo(HaveOccurred())
+
+		testNs, err := testutils.NewNS()
+		Expect(err).NotTo(HaveOccurred())
+
+		testIfaceName = fmt.Sprintf("cnitest.%d", rand.Intn(100000))
+		testIface := &netlink.Dummy{
+			LinkAttrs: netlink.LinkAttrs{
+				Name:      testIfaceName,
+				Namespace: netlink.NsFd(int(testNs.Fd())),
+			},
+		}
+
+		err = netlink.LinkAdd(testIface)
+		Expect(err).NotTo(HaveOccurred())
+
+		runtime.LockOSThread()
+		err = testNs.Set()
+		Expect(err).NotTo(HaveOccurred())
+
+		cleanup = func() {
+			netlink.LinkDel(testIface)
+			currNs.Set()
+		}
+
+	})
+
+	AfterEach(func() {
+		cleanup()
+	})
+
+	Describe("Sysctl", func() {
+		It("reads keys with dot separators", func() {
+			sysctlIfaceName := strings.Replace(testIfaceName, ".", "/", -1)
+			sysctlKey := fmt.Sprintf(sysctlDotKeyTemplate, sysctlIfaceName)
+
+			_, err := Sysctl(sysctlKey)
+			Expect(err).NotTo(HaveOccurred())
+		})
+	})
+
+	Describe("Sysctl", func() {
+		It("reads keys with slash separators", func() {
+			sysctlKey := fmt.Sprintf(sysctlSlashKeyTemplate, testIfaceName)
+
+			_, err := Sysctl(sysctlKey)
+			Expect(err).NotTo(HaveOccurred())
+		})
+	})
+
+	Describe("Sysctl", func() {
+		It("writes keys with dot separators", func() {
+			sysctlIfaceName := strings.Replace(testIfaceName, ".", "/", -1)
+			sysctlKey := fmt.Sprintf(sysctlDotKeyTemplate, sysctlIfaceName)
+
+			_, err := Sysctl(sysctlKey, "1")
+			Expect(err).NotTo(HaveOccurred())
+		})
+	})
+
+	Describe("Sysctl", func() {
+		It("writes keys with slash separators", func() {
+			sysctlKey := fmt.Sprintf(sysctlSlashKeyTemplate, testIfaceName)
+
+			_, err := Sysctl(sysctlKey, "1")
+			Expect(err).NotTo(HaveOccurred())
+		})
+	})
+
+})

pkg/utils/sysctl/sysctl_suite_test.go

@@ -0,0 +1,27 @@
+// Copyright 2017-2020 CNI authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package sysctl_test
+
+import (
+	"testing"
+
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+)
+
+func TestSysctl(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "Sysctl Suite")
+}

plugins/main/bridge/bridge.go

@@ -224,7 +224,9 @@ func ensureBridge(brName string, mtu int, promiscMode, vlanFiltering bool) (*net
 			// default packet limit
 			TxQLen: -1,
 		},
-		VlanFiltering: &vlanFiltering,
+	}
+	if vlanFiltering {
+		br.VlanFiltering = &vlanFiltering
 	}
 
 	err := netlink.LinkAdd(br)

plugins/main/vlan/vlan.go

@@ -207,7 +207,7 @@ func cmdDel(args *skel.CmdArgs) error {
 
 	err = ns.WithNetNSPath(args.Netns, func(_ ns.NetNS) error {
 		err = ip.DelLinkByName(args.IfName)
-		if err != nil && err != ip.ErrLinkNotFound {
+		if err != nil && err == ip.ErrLinkNotFound {
 			return nil
 		}
 		return err

plugins/main/vlan/vlan_test.go

@@ -292,6 +292,19 @@ var _ = Describe("vlan Operations", func() {
 			return nil
 		})
 		Expect(err).NotTo(HaveOccurred())
+
+		// DEL can be called multiple times, make sure no error is returned
+		// if the device is already removed.
+		err = originalNS.Do(func(ns.NetNS) error {
+			defer GinkgoRecover()
+
+			err = testutils.CmdDelWithArgs(args, func() error {
+				return cmdDel(args)
+			})
+			Expect(err).NotTo(HaveOccurred())
+			return nil
+		})
+		Expect(err).NotTo(HaveOccurred())
 	})
 
 	It("configures and deconfigures an CNI V4 vlan link with ADD/CHECK/DEL", func() {