tuning: revert values on delete (#540)

Values changed by Tuning plugin should be changed only for pod, therefore should be reverted when NIC is being moved from pod back to host.

Fixes: #493

Signed-off-by: Patryk Strusiewicz-Surmacki <patrykx.strusiewicz-surmacki@intel.com>

plugins/meta/tuning/tuning.go

@@ -22,6 +22,8 @@ import (
 	"fmt"
 	"io/ioutil"
 	"net"
+	"os"
+	"path"
 	"path/filepath"
 	"strings"
 
@@ -37,9 +39,12 @@ import (
 	bv "github.com/containernetworking/plugins/pkg/utils/buildversion"
 )
 
+const defaultDataDir = "/run/cni/tuning"
+
 // TuningConf represents the network tuning configuration.
 type TuningConf struct {
 	types.NetConf
+	DataDir string            `json:"dataDir,omitempty"`
 	SysCtl  map[string]string `json:"sysctl"`
 	Mac     string            `json:"mac,omitempty"`
 	Promisc bool              `json:"promisc,omitempty"`
@@ -60,6 +65,13 @@ type IPAMArgs struct {
 	Mtu     *int               `json:"mtu,omitempty"`
 }
 
+// configToRestore will contain interface attributes that should be restored on cmdDel
+type configToRestore struct {
+	Mac     string `json:"mac,omitempty"`
+	Promisc *bool  `json:"promisc,omitempty"`
+	Mtu     int    `json:"mtu,omitempty"`
+}
+
 // MacEnvArgs represents CNI_ARG
 type MacEnvArgs struct {
 	types.CommonArgs
@@ -72,6 +84,10 @@ func parseConf(data []byte, envArgs string) (*TuningConf, error) {
 		return nil, fmt.Errorf("failed to load netconf: %v", err)
 	}
 
+	if conf.DataDir == "" {
+		conf.DataDir = defaultDataDir
+	}
+
 	// Parse custom Mac from both env args
 	if envArgs != "" {
 		e := MacEnvArgs{}
@@ -165,6 +181,95 @@ func changeMtu(ifName string, mtu int) error {
 	return netlink.LinkSetMTU(link, mtu)
 }
 
+func createBackup(ifName, containerID, backupPath string, tuningConf *TuningConf) error {
+	config := configToRestore{}
+	link, err := netlink.LinkByName(ifName)
+	if err != nil {
+		return fmt.Errorf("failed to get %q: %v", ifName, err)
+	}
+	if tuningConf.Mac != "" {
+		config.Mac = link.Attrs().HardwareAddr.String()
+	}
+	if tuningConf.Promisc {
+		config.Promisc = new(bool)
+		*config.Promisc = (link.Attrs().Promisc != 0)
+	}
+	if tuningConf.Mtu != 0 {
+		config.Mtu = link.Attrs().MTU
+	}
+
+	if _, err := os.Stat(backupPath); os.IsNotExist(err) {
+		if err = os.MkdirAll(backupPath, 0600); err != nil {
+			return fmt.Errorf("failed to create backup directory: %v", err)
+		}
+	}
+
+	data, err := json.MarshalIndent(config, "", " ")
+	if err != nil {
+		return fmt.Errorf("failed to marshall data for %q: %v", ifName, err)
+	}
+	if err = ioutil.WriteFile(path.Join(backupPath, containerID+"_"+ifName+".json"), data, 0600); err != nil {
+		return fmt.Errorf("failed to save file %s.json: %v", ifName, err)
+	}
+
+	return nil
+}
+
+func restoreBackup(ifName, containerID, backupPath string) error {
+	filePath := path.Join(backupPath, containerID+"_"+ifName+".json")
+
+	if _, err := os.Stat(filePath); os.IsNotExist(err) {
+		// No backup file - nothing to revert
+		return nil
+	}
+
+	file, err := ioutil.ReadFile(filePath)
+	if err != nil {
+		return fmt.Errorf("failed to open file %q: %v", filePath, err)
+	}
+
+	config := configToRestore{}
+	if err = json.Unmarshal([]byte(file), &config); err != nil {
+		return nil
+	}
+
+	var errStr []string
+
+	_, err = netlink.LinkByName(ifName)
+	if err != nil {
+		return nil
+	}
+
+	if config.Mtu != 0 {
+		if err = changeMtu(ifName, config.Mtu); err != nil {
+			err = fmt.Errorf("failed to restore MTU: %v", err)
+			errStr = append(errStr, err.Error())
+		}
+	}
+	if config.Mac != "" {
+		if err = changeMacAddr(ifName, config.Mac); err != nil {
+			err = fmt.Errorf("failed to restore MAC address: %v", err)
+			errStr = append(errStr, err.Error())
+		}
+	}
+	if config.Promisc != nil {
+		if err = changePromisc(ifName, *config.Promisc); err != nil {
+			err = fmt.Errorf("failed to restore promiscuous mode: %v", err)
+			errStr = append(errStr, err.Error())
+		}
+	}
+
+	if len(errStr) > 0 {
+		return fmt.Errorf(strings.Join(errStr, "; "))
+	}
+
+	if err = os.Remove(filePath); err != nil {
+		return fmt.Errorf("failed to remove file %v: %v", filePath, err)
+	}
+
+	return nil
+}
+
 func cmdAdd(args *skel.CmdArgs) error {
 	tuningConf, err := parseConf(args.StdinData, args.Args)
 	if err != nil {
@@ -205,6 +310,12 @@ func cmdAdd(args *skel.CmdArgs) error {
 			}
 		}
 
+		if tuningConf.Mac != "" || tuningConf.Mtu != 0 || tuningConf.Promisc {
+			if err = createBackup(args.IfName, args.ContainerID, tuningConf.DataDir, tuningConf); err != nil {
+				return err
+			}
+		}
+
 		if tuningConf.Mac != "" {
 			if err = changeMacAddr(args.IfName, tuningConf.Mac); err != nil {
 				return err
@@ -239,10 +350,17 @@ func cmdAdd(args *skel.CmdArgs) error {
 	return types.PrintResult(tuningConf.PrevResult, tuningConf.CNIVersion)
 }
 
+// cmdDel will restore NIC attributes to the original ones when called
 func cmdDel(args *skel.CmdArgs) error {
-	// TODO: the settings are not reverted to the previous values. Reverting the
+	tuningConf, err := parseConf(args.StdinData, args.Args)
-	// settings is not useful when the whole container goes away but it could be
+	if err != nil {
-	// useful in scenarios where plugins are added and removed at runtime.
+		return err
+	}
+
+	ns.WithNetNSPath(args.Netns, func(_ ns.NetNS) error {
+		// MAC address, MTU and promiscuous mode settings will be restored
+		return restoreBackup(args.IfName, args.ContainerID, tuningConf.DataDir)
+	})
 	return nil
 }
 

plugins/meta/tuning/tuning_test.go

@@ -17,13 +17,13 @@ package main
 import (
 	"encoding/json"
 	"fmt"
+	"net"
 
 	"github.com/containernetworking/cni/pkg/skel"
 	"github.com/containernetworking/cni/pkg/types"
 	"github.com/containernetworking/cni/pkg/types/current"
 	"github.com/containernetworking/plugins/pkg/ns"
 	"github.com/containernetworking/plugins/pkg/testutils"
-	"net"
 
 	"github.com/vishvananda/netlink"
 
@@ -77,6 +77,7 @@ func buildOneConfig(name, cniVersion string, orig *TuningConf, prevResult types.
 var _ = Describe("tuning plugin", func() {
 	var originalNS ns.NetNS
 	const IFNAME string = "dummy0"
+	var beforeConf configToRestore
 
 	BeforeEach(func() {
 		// Create a new NetNS so we don't modify the host
@@ -93,8 +94,13 @@ var _ = Describe("tuning plugin", func() {
 				},
 			})
 			Expect(err).NotTo(HaveOccurred())
-			_, err = netlink.LinkByName(IFNAME)
+			link, err := netlink.LinkByName(IFNAME)
 			Expect(err).NotTo(HaveOccurred())
+
+			beforeConf.Mac = link.Attrs().HardwareAddr.String()
+			beforeConf.Mtu = link.Attrs().MTU
+			beforeConf.Promisc = new(bool)
+			*beforeConf.Promisc = (link.Attrs().Promisc != 0)
 			return nil
 		})
 		Expect(err).NotTo(HaveOccurred())
@@ -138,6 +144,8 @@ var _ = Describe("tuning plugin", func() {
 			StdinData:   conf,
 		}
 
+		beforeConf = configToRestore{}
+
 		err = originalNS.Do(func(ns.NetNS) error {
 			defer GinkgoRecover()
 
@@ -153,6 +161,13 @@ var _ = Describe("tuning plugin", func() {
 			Expect(result.Interfaces[0].Name).To(Equal(IFNAME))
 			Expect(len(result.IPs)).To(Equal(1))
 			Expect(result.IPs[0].Address.String()).To(Equal("10.0.0.2/24"))
+
+			Expect("/tmp/tuning-test/dummy_dummy0.json").ShouldNot(BeAnExistingFile())
+
+			err = testutils.CmdDel(originalNS.Path(),
+				args.ContainerID, "", func() error { return cmdDel(args) })
+			Expect(err).NotTo(HaveOccurred())
+
 			return nil
 		})
 		Expect(err).NotTo(HaveOccurred())
@@ -210,6 +225,10 @@ var _ = Describe("tuning plugin", func() {
 				args.ContainerID, "", func() error { return cmdDel(args) })
 			Expect(err).NotTo(HaveOccurred())
 
+			link, err = netlink.LinkByName(IFNAME)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(link.Attrs().Promisc != 0).To(Equal(*beforeConf.Promisc))
+
 			return nil
 		})
 		Expect(err).NotTo(HaveOccurred())
@@ -271,6 +290,10 @@ var _ = Describe("tuning plugin", func() {
 				args.ContainerID, "", func() error { return cmdDel(args) })
 			Expect(err).NotTo(HaveOccurred())
 
+			link, err = netlink.LinkByName(IFNAME)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(link.Attrs().Promisc != 0).To(Equal(*beforeConf.Promisc))
+
 			return nil
 		})
 		Expect(err).NotTo(HaveOccurred())
@@ -328,6 +351,10 @@ var _ = Describe("tuning plugin", func() {
 				args.ContainerID, "", func() error { return cmdDel(args) })
 			Expect(err).NotTo(HaveOccurred())
 
+			link, err = netlink.LinkByName(IFNAME)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(link.Attrs().MTU).To(Equal(beforeConf.Mtu))
+
 			return nil
 		})
 		Expect(err).NotTo(HaveOccurred())
@@ -389,6 +416,10 @@ var _ = Describe("tuning plugin", func() {
 				args.ContainerID, "", func() error { return cmdDel(args) })
 			Expect(err).NotTo(HaveOccurred())
 
+			link, err = netlink.LinkByName(IFNAME)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(link.Attrs().MTU).To(Equal(beforeConf.Mtu))
+
 			return nil
 		})
 		Expect(err).NotTo(HaveOccurred())
@@ -448,6 +479,10 @@ var _ = Describe("tuning plugin", func() {
 				args.ContainerID, "", func() error { return cmdDel(args) })
 			Expect(err).NotTo(HaveOccurred())
 
+			link, err = netlink.LinkByName(IFNAME)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(link.Attrs().HardwareAddr.String()).To(Equal(beforeConf.Mac))
+
 			return nil
 		})
 		Expect(err).NotTo(HaveOccurred())
@@ -511,6 +546,10 @@ var _ = Describe("tuning plugin", func() {
 				args.ContainerID, "", func() error { return cmdDel(args) })
 			Expect(err).NotTo(HaveOccurred())
 
+			link, err = netlink.LinkByName(IFNAME)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(link.Attrs().HardwareAddr.String()).To(Equal(beforeConf.Mac))
+
 			return nil
 		})
 		Expect(err).NotTo(HaveOccurred())
@@ -571,6 +610,10 @@ var _ = Describe("tuning plugin", func() {
 				args.ContainerID, "", func() error { return cmdDel(args) })
 			Expect(err).NotTo(HaveOccurred())
 
+			link, err = netlink.LinkByName(IFNAME)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(link.Attrs().HardwareAddr.String()).To(Equal(beforeConf.Mac))
+
 			return nil
 		})
 		Expect(err).NotTo(HaveOccurred())
@@ -643,6 +686,10 @@ var _ = Describe("tuning plugin", func() {
 				args.ContainerID, "", func() error { return cmdDel(args) })
 			Expect(err).NotTo(HaveOccurred())
 
+			link, err = netlink.LinkByName(IFNAME)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(link.Attrs().Promisc != 0).To(Equal(*beforeConf.Promisc))
+
 			return nil
 		})
 		Expect(err).NotTo(HaveOccurred())
@@ -715,6 +762,10 @@ var _ = Describe("tuning plugin", func() {
 				args.ContainerID, "", func() error { return cmdDel(args) })
 			Expect(err).NotTo(HaveOccurred())
 
+			link, err = netlink.LinkByName(IFNAME)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(link.Attrs().MTU).To(Equal(beforeConf.Mtu))
+
 			return nil
 		})
 		Expect(err).NotTo(HaveOccurred())
@@ -789,6 +840,10 @@ var _ = Describe("tuning plugin", func() {
 				args.ContainerID, "", func() error { return cmdDel(args) })
 			Expect(err).NotTo(HaveOccurred())
 
+			link, err = netlink.LinkByName(IFNAME)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(link.Attrs().HardwareAddr.String()).To(Equal(beforeConf.Mac))
+
 			return nil
 		})
 		Expect(err).NotTo(HaveOccurred())
@@ -863,6 +918,10 @@ var _ = Describe("tuning plugin", func() {
 				args.ContainerID, "", func() error { return cmdDel(args) })
 			Expect(err).NotTo(HaveOccurred())
 
+			link, err = netlink.LinkByName(IFNAME)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(link.Attrs().HardwareAddr.String()).To(Equal(beforeConf.Mac))
+
 			return nil
 		})
 		Expect(err).NotTo(HaveOccurred())
@@ -925,6 +984,97 @@ var _ = Describe("tuning plugin", func() {
 				args.ContainerID, "", func() error { return cmdDel(args) })
 			Expect(err).NotTo(HaveOccurred())
 
+			link, err = netlink.LinkByName(IFNAME)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(link.Attrs().HardwareAddr.String()).To(Equal(beforeConf.Mac))
+
+			return nil
+		})
+		Expect(err).NotTo(HaveOccurred())
+	})
+
+	It("configures and deconfigures mac address, promisc mode and MTU (from conf file) with custom dataDir", func() {
+		conf := []byte(`{
+	"name": "test",
+	"type": "iplink",
+	"cniVersion": "0.4.0",
+	"mac": "c2:11:22:33:44:77",
+	"promisc": true,
+	"mtu": 4000,
+	"dataDir": "/tmp/tuning-test",
+	"prevResult": {
+		"interfaces": [
+			{"name": "dummy0", "sandbox":"netns"}
+		],
+		"ips": [
+			{
+				"version": "4",
+				"address": "10.0.0.2/24",
+				"gateway": "10.0.0.1",
+				"interface": 0
+			}
+		]
+	}
+}`)
+
+		args := &skel.CmdArgs{
+			ContainerID: "dummy",
+			Netns:       originalNS.Path(),
+			IfName:      IFNAME,
+			StdinData:   conf,
+		}
+
+		err := originalNS.Do(func(ns.NetNS) error {
+			defer GinkgoRecover()
+
+			r, _, err := testutils.CmdAddWithArgs(args, func() error {
+				return cmdAdd(args)
+			})
+			Expect(err).NotTo(HaveOccurred())
+
+			result, err := current.GetResult(r)
+			Expect(err).NotTo(HaveOccurred())
+
+			Expect(len(result.Interfaces)).To(Equal(1))
+			Expect(result.Interfaces[0].Name).To(Equal(IFNAME))
+			Expect(len(result.IPs)).To(Equal(1))
+			Expect(result.IPs[0].Address.String()).To(Equal("10.0.0.2/24"))
+
+			link, err := netlink.LinkByName(IFNAME)
+			Expect(err).NotTo(HaveOccurred())
+			hw, err := net.ParseMAC("c2:11:22:33:44:77")
+			Expect(err).NotTo(HaveOccurred())
+			Expect(link.Attrs().HardwareAddr).To(Equal(hw))
+			Expect(link.Attrs().Promisc).To(Equal(1))
+			Expect(link.Attrs().MTU).To(Equal(4000))
+
+			Expect("/tmp/tuning-test/dummy_dummy0.json").Should(BeAnExistingFile())
+
+			n := &TuningConf{}
+			err = json.Unmarshal([]byte(conf), &n)
+			Expect(err).NotTo(HaveOccurred())
+
+			cniVersion := "0.4.0"
+			_, confString, err := buildOneConfig("testConfig", cniVersion, n, r)
+			Expect(err).NotTo(HaveOccurred())
+
+			args.StdinData = confString
+
+			err = testutils.CmdCheckWithArgs(args, func() error {
+				return cmdCheck(args)
+			})
+			Expect(err).NotTo(HaveOccurred())
+
+			err = testutils.CmdDel(originalNS.Path(),
+				args.ContainerID, "", func() error { return cmdDel(args) })
+			Expect(err).NotTo(HaveOccurred())
+
+			link, err = netlink.LinkByName(IFNAME)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(link.Attrs().HardwareAddr.String()).To(Equal(beforeConf.Mac))
+			Expect(link.Attrs().MTU).To(Equal(beforeConf.Mtu))
+			Expect(link.Attrs().Promisc != 0).To(Equal(*beforeConf.Promisc))
+
 			return nil
 		})
 		Expect(err).NotTo(HaveOccurred())