Cleanup ports after IPAM failure (#259)

* Test cleanup after ADD failure Signed-off-by: Petr Horáček <[email protected]> * 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 <[email protected]> Signed-off-by: Petr Horáček <[email protected]>
k8snetworkplumbingwg · Jan 4, 2023 · a4385e4 · a4385e4
1 parent 9b91227
commit a4385e4
Show file tree

Hide file tree

Showing 2 changed files with 81 additions and 2 deletions.
diff --git a/pkg/plugin/plugin.go b/pkg/plugin/plugin.go
@@ -308,14 +308,30 @@ 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},
 	}
 
 	// 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
@@ -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",