From a4385e47875d753b26bcc04aea78f76351f4c5d4 Mon Sep 17 00:00:00 2001
From: Petr Horacek <phoracek@redhat.com>
Date: Wed, 4 Jan 2023 18:34:54 +0100
Subject: [PATCH] Cleanup ports after IPAM failure (#259)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* Test cleanup after ADD failure

Signed-off-by: Petr Horáček <phoracek@redhat.com>

* Cleanup ports after IPAM failure

While most of the configuration gets cleaned up on ADD failure through
the removal of containers netns, OVS ports do not.

In case port attachment succeeded but the following IPAM configuration
failed, we end up with the port left behind.

With this patch, the port gets explicitly deleted on IPAM failure.

Signed-off-by: Petr Horáček <phoracek@redhat.com>

Signed-off-by: Petr Horáček <phoracek@redhat.com>
---
 pkg/plugin/plugin.go      | 21 +++++++++++--
 pkg/plugin/plugin_test.go | 62 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 81 insertions(+), 2 deletions(-)

diff --git a/pkg/plugin/plugin.go b/pkg/plugin/plugin.go
index f1717383..5d173a71 100644
--- a/pkg/plugin/plugin.go
+++ b/pkg/plugin/plugin.go
@@ -308,6 +308,21 @@ func CmdAdd(args *skel.CmdArgs) error {
 	if err = attachIfaceToBridge(ovsDriver, hostIface.Name, contIface.Name, netconf.OfportRequest, vlanTagNum, trunks, portType, netconf.InterfaceType, args.Netns, ovnPort); err != nil {
 		return err
 	}
+	defer func() {
+		if err != nil {
+			// Unlike veth pair, OVS port will not be automatically removed
+			// if the following IPAM configuration fails and netns gets removed.
+			portName, portFound, err := getOvsPortForContIface(ovsDriver, args.IfName, args.Netns)
+			if err != nil {
+				log.Printf("Failed best-effort cleanup: %v", err)
+			}
+			if portFound {
+				if err := removeOvsPort(ovsDriver, portName); err != nil {
+					log.Printf("Failed best-effort cleanup: %v", err)
+				}
+			}
+		}
+	}()
 
 	result := &current.Result{
 		Interfaces: []*current.Interface{hostIface, contIface},
@@ -315,7 +330,8 @@ func CmdAdd(args *skel.CmdArgs) error {
 
 	// run the IPAM plugin
 	if netconf.IPAM.Type != "" {
-		r, err := ipam.ExecAdd(netconf.IPAM.Type, args.StdinData)
+		var r cnitypes.Result
+		r, err = ipam.ExecAdd(netconf.IPAM.Type, args.StdinData)
 		defer func() {
 			if err != nil {
 				if err := ipam.ExecDel(netconf.IPAM.Type, args.StdinData); err != nil {
@@ -328,7 +344,8 @@ func CmdAdd(args *skel.CmdArgs) error {
 		}
 
 		// Convert the IPAM result into the current Result type
-		newResult, err := current.NewResultFromResult(r)
+		var newResult *current.Result
+		newResult, err = current.NewResultFromResult(r)
 		if err != nil {
 			return err
 		}
diff --git a/pkg/plugin/plugin_test.go b/pkg/plugin/plugin_test.go
index e24a4047..3cf85e85 100644
--- a/pkg/plugin/plugin_test.go
+++ b/pkg/plugin/plugin_test.go
@@ -430,6 +430,40 @@ var testFunc = func(version string) {
 		return hostIface.Name, r
 	}
 
+	testInvalidAdd := func(conf string) {
+		By("Creating temporary target namespace to simulate a container")
+		targetNs, err := testutils.NewNS()
+		Expect(err).NotTo(HaveOccurred())
+		defer func() {
+			Expect(targetNs.Close()).To(Succeed())
+			Expect(testutils.UnmountNS(targetNs)).To(Succeed())
+		}()
+
+		args := &skel.CmdArgs{
+			ContainerID: "dummy",
+			Netns:       targetNs.Path(),
+			IfName:      IFNAME,
+			StdinData:   []byte(conf),
+		}
+		netconf, err := config.LoadConf(args.StdinData)
+		Expect(err).NotTo(HaveOccurred())
+
+		By("Snapshotting original configuration")
+		originalBrPorts, err := listBridgePorts(netconf.BrName)
+		Expect(err).NotTo(HaveOccurred())
+
+		By("Calling ADD command")
+		_, _, err = cmdAddWithArgs(args, func() error {
+			return CmdAdd(args)
+		})
+		Expect(err).To(HaveOccurred())
+
+		By("Checking that new port is not present on the bridge")
+		brPorts, err := listBridgePorts(netconf.BrName)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(brPorts).To(Equal(originalBrPorts))
+	}
+
 	Context("connecting container to a bridge", func() {
 		Context("with VLAN ID set on port", func() {
 			conf := fmt.Sprintf(`{
@@ -534,6 +568,34 @@ var testFunc = func(version string) {
 				testIPAM(conf, true, "10.1.2", "3ffe:ffff:0:1ff")
 			})
 		})
+		Context("with invalid VLAN configuration", func() {
+			conf := fmt.Sprintf(`{
+				"cniVersion": "%s",
+				"name": "mynet",
+				"type": "ovs",
+				"bridge": "%s",
+				"vlan": 9999
+			}`, version, bridgeName)
+			It("should fail and leave no leftovers", func() {
+				testInvalidAdd(conf)
+			})
+		})
+		Context("with invalid IPAM configuration", func() {
+			conf := fmt.Sprintf(`{
+				"cniVersion": "%s",
+				"name": "mynet",
+				"type": "ovs",
+				"bridge": "%s",
+				"ipam": {
+					"type": "host-local",
+					"ranges": [[ {"subnet": "invalid", "gateway": "10.1.2.1"} ]],
+					"dataDir": "/tmp/ovs-cni/conf"
+				}
+			}`, version, bridgeName)
+			It("should fail and leave no leftovers", func() {
+				testInvalidAdd(conf)
+			})
+		})
 		Context("with MTU set on port", func() {
 			conf := fmt.Sprintf(`{
 				"cniVersion": "%s",