Merge pull request #1095 from s1061123/fix/plugin-without-interface

Fix multus to support CNI plugin which does not create interface

.github/workflows/image-build.yml

@@ -64,11 +64,14 @@ jobs:
         uses: docker/setup-buildx-action@v2
 
       - name: Download OKD Builder Dockerfile
-        run: curl https://raw.githubusercontent.com/okd-project/images/main/okd-builder.Dockerfile -o images/okd-builder.Dockerfile
+        run: curl https://raw.githubusercontent.com/okd-project/images/main/builder/Dockerfile -o images/okd-builder.Dockerfile
 
       - name: Patch OKD Builder Dockerfile to workaround error
         run: sed -i -e "s/yum install -y yum-utils/rpm --import \/etc\/pki\/rpm-gpg\/*;yum install -y yum-utils/" images/okd-builder.Dockerfile
 
+      - name: Create root for builder
+        run: mkdir root
+
       - name: Organically build golang builder image
         run: docker build -t local/okdbuilder:latest -f images/okd-builder.Dockerfile .
 

README.md

@@ -62,3 +62,5 @@ In addition to the [quick-start guide](docs/quickstart.md), you may:
 ## Contact Us
 
 For any questions about Multus CNI, open up a GitHub issue or feel free to ask a question in #general in the [NPWG Slack](https://npwg-team.slack.com/).
+
+To be invited, use [this slack invite link](https://join.slack.com/t/npwg-team/shared_invite/zt-1u2vmsn2b-tKdOokdPY73zn9B32JoAOg).

pkg/multus/multus.go

@@ -369,11 +369,14 @@ func DelegateAdd(exec invoke.Exec, kubeClient *k8s.ClientInfo, pod *v1.Pod, dele
 	}
 
 	if pod != nil {
-		// send kubernetes events
-		if delegate.Name != "" {
-			kubeClient.Eventf(pod, v1.EventTypeNormal, "AddedInterface", "Add %s %v from %s", rt.IfName, ips, delegate.Name)
-		} else {
-			kubeClient.Eventf(pod, v1.EventTypeNormal, "AddedInterface", "Add %s %v", rt.IfName, ips)
+		// check Interfaces and IPs because some CNI plugin just return empty result
+		if res.Interfaces != nil || res.IPs != nil {
+			// send kubernetes events
+			if delegate.Name != "" {
+				kubeClient.Eventf(pod, v1.EventTypeNormal, "AddedInterface", "Add %s %v from %s", rt.IfName, ips, delegate.Name)
+			} else {
+				kubeClient.Eventf(pod, v1.EventTypeNormal, "AddedInterface", "Add %s %v", rt.IfName, ips)
+			}
 		}
 	} else {
 		// for further debug https://github.com/k8snetworkplumbingwg/multus-cni/issues/481
@@ -636,71 +639,80 @@ func CmdAdd(args *skel.CmdArgs, exec invoke.Exec, kubeClient *k8s.ClientInfo) (c
 			return nil, cmdPluginErr(k8sArgs, netName, "error adding container to network %q: %v", netName, err)
 		}
 
-		// Remove gateway from routing table if the gateway is not used
-		deleteV4gateway := false
-		deleteV6gateway := false
-		adddefaultgateway := false
-		if delegate.IsFilterV4Gateway {
-			deleteV4gateway = true
-			logging.Debugf("Marked interface %v for v4 gateway deletion", ifName)
-		} else {
-			// Otherwise, determine if this interface now gets our default route.
-			// According to
-			// https://docs.google.com/document/d/1Ny03h6IDVy_e_vmElOqR7UdTPAG_RNydhVE1Kx54kFQ (4.1.2.1.9)
-			// the list can be empty; if it is, we'll assume the CNI's config for the default gateway holds,
-			// else we'll update the defaultgateway to the one specified.
-			if delegate.GatewayRequest != nil && len(*delegate.GatewayRequest) != 0 {
-				deleteV4gateway = true
-				adddefaultgateway = true
-				logging.Debugf("Detected gateway override on interface %v to %v", ifName, delegate.GatewayRequest)
-			}
-		}
-
-		if delegate.IsFilterV6Gateway {
-			deleteV6gateway = true
-			logging.Debugf("Marked interface %v for v6 gateway deletion", ifName)
-		} else {
-			// Otherwise, determine if this interface now gets our default route.
-			// According to
-			// https://docs.google.com/document/d/1Ny03h6IDVy_e_vmElOqR7UdTPAG_RNydhVE1Kx54kFQ (4.1.2.1.9)
-			// the list can be empty; if it is, we'll assume the CNI's config for the default gateway holds,
-			// else we'll update the defaultgateway to the one specified.
-			if delegate.GatewayRequest != nil && len(*delegate.GatewayRequest) != 0 {
-				deleteV6gateway = true
-				adddefaultgateway = true
-				logging.Debugf("Detected gateway override on interface %v to %v", ifName, delegate.GatewayRequest)
-			}
-		}
-
-		// Remove gateway if `default-route` network selection is specified
-		if deleteV4gateway || deleteV6gateway {
-			err = netutils.DeleteDefaultGW(args.Netns, ifName)
-			if err != nil {
-				return nil, cmdErr(k8sArgs, "error deleting default gateway: %v", err)
-			}
-			err = netutils.DeleteDefaultGWCache(n.CNIDir, rt, netName, ifName, deleteV4gateway, deleteV6gateway)
-			if err != nil {
-				return nil, cmdErr(k8sArgs, "error deleting default gateway in cache: %v", err)
-			}
-		}
-
-		// Here we'll set the default gateway which specified in `default-route` network selection
-		if adddefaultgateway {
-			err = netutils.SetDefaultGW(args.Netns, ifName, *delegate.GatewayRequest)
-			if err != nil {
-				return nil, cmdErr(k8sArgs, "error setting default gateway: %v", err)
-			}
-			err = netutils.AddDefaultGWCache(n.CNIDir, rt, netName, ifName, *delegate.GatewayRequest)
-			if err != nil {
-				return nil, cmdErr(k8sArgs, "error setting default gateway in cache: %v", err)
-			}
-		}
-
 		// Master plugin result is always used if present
 		if delegate.MasterPlugin || result == nil {
 			result = tmpResult
 		}
 
+		res, err := cni100.NewResultFromResult(tmpResult)
+		if err != nil {
+			logging.Errorf("CmdAdd: failed to read result: %v, but proceed", err)
+		}
+
+		// check Interfaces and IPs because some CNI plugin does not create any interface
+		// and just returns empty result
+		if res != nil &&  (res.Interfaces != nil || res.IPs != nil) {
+			// Remove gateway from routing table if the gateway is not used
+			deleteV4gateway := false
+			deleteV6gateway := false
+			adddefaultgateway := false
+			if delegate.IsFilterV4Gateway {
+				deleteV4gateway = true
+				logging.Debugf("Marked interface %v for v4 gateway deletion", ifName)
+			} else {
+				// Otherwise, determine if this interface now gets our default route.
+				// According to
+				// https://docs.google.com/document/d/1Ny03h6IDVy_e_vmElOqR7UdTPAG_RNydhVE1Kx54kFQ (4.1.2.1.9)
+				// the list can be empty; if it is, we'll assume the CNI's config for the default gateway holds,
+				// else we'll update the defaultgateway to the one specified.
+				if delegate.GatewayRequest != nil && len(*delegate.GatewayRequest) != 0 {
+					deleteV4gateway = true
+					adddefaultgateway = true
+					logging.Debugf("Detected gateway override on interface %v to %v", ifName, delegate.GatewayRequest)
+				}
+			}
+
+			if delegate.IsFilterV6Gateway {
+				deleteV6gateway = true
+				logging.Debugf("Marked interface %v for v6 gateway deletion", ifName)
+			} else {
+				// Otherwise, determine if this interface now gets our default route.
+				// According to
+				// https://docs.google.com/document/d/1Ny03h6IDVy_e_vmElOqR7UdTPAG_RNydhVE1Kx54kFQ (4.1.2.1.9)
+				// the list can be empty; if it is, we'll assume the CNI's config for the default gateway holds,
+				// else we'll update the defaultgateway to the one specified.
+				if delegate.GatewayRequest != nil && len(*delegate.GatewayRequest) != 0 {
+					deleteV6gateway = true
+					adddefaultgateway = true
+					logging.Debugf("Detected gateway override on interface %v to %v", ifName, delegate.GatewayRequest)
+				}
+			}
+
+			// Remove gateway if `default-route` network selection is specified
+			if deleteV4gateway || deleteV6gateway {
+				err = netutils.DeleteDefaultGW(args.Netns, ifName)
+				if err != nil {
+					return nil, cmdErr(k8sArgs, "error deleting default gateway: %v", err)
+				}
+				err = netutils.DeleteDefaultGWCache(n.CNIDir, rt, netName, ifName, deleteV4gateway, deleteV6gateway)
+				if err != nil {
+					return nil, cmdErr(k8sArgs, "error deleting default gateway in cache: %v", err)
+				}
+			}
+
+			// Here we'll set the default gateway which specified in `default-route` network selection
+			if adddefaultgateway {
+				err = netutils.SetDefaultGW(args.Netns, ifName, *delegate.GatewayRequest)
+				if err != nil {
+					return nil, cmdErr(k8sArgs, "error setting default gateway: %v", err)
+				}
+				err = netutils.AddDefaultGWCache(n.CNIDir, rt, netName, ifName, *delegate.GatewayRequest)
+				if err != nil {
+					return nil, cmdErr(k8sArgs, "error setting default gateway in cache: %v", err)
+				}
+			}
+		}
+
 		// Read devInfo from CNIDeviceInfoFile if it exists so
 		// it can be copied to the NetworkStatus.
 		devinfo, err := getDelegateDeviceInfo(delegate, rt)

pkg/multus/multus_cni020_test.go

@@ -154,6 +154,67 @@ var _ = Describe("multus operations cniVersion 0.2.0 config", func() {
 		Expect(fExec.delIndex).To(Equal(len(fExec.plugins)))
 	})
 
+	It("executes delegates (plugin without interface)", func() {
+		args := &skel.CmdArgs{
+			ContainerID: "123456789",
+			Netns:       testNS.Path(),
+			IfName:      "eth0",
+			StdinData: []byte(`{
+	    "name": "node-cni-network",
+	    "type": "multus",
+	    "defaultnetworkfile": "/tmp/foo.multus.conf",
+	    "defaultnetworkwaitseconds": 3,
+	    "delegates": [{
+	        "name": "weave1",
+	        "cniVersion": "0.2.0",
+	        "type": "weave-net"
+	    },{
+	        "name": "other1",
+	        "cniVersion": "0.2.0",
+	        "type": "other-plugin"
+	    }]
+	}`),
+		}
+
+		logging.SetLogLevel("verbose")
+
+		fExec := newFakeExec()
+		expectedResult1 := &types020.Result{
+			CNIVersion: "0.2.0",
+			IP4: &types020.IPConfig{
+				IP: *testhelpers.EnsureCIDR("1.1.1.2/24"),
+			},
+		}
+		expectedConf1 := `{
+	    "name": "weave1",
+	    "cniVersion": "0.2.0",
+	    "type": "weave-net"
+	}`
+		fExec.addPlugin020(nil, "eth0", expectedConf1, expectedResult1, nil)
+
+		// other1 just returns empty result
+		expectedResult2 := &types020.Result{
+			CNIVersion: "0.2.0",
+		}
+		expectedConf2 := `{
+	    "name": "other1",
+	    "cniVersion": "0.2.0",
+	    "type": "other-plugin"
+	}`
+		fExec.addPlugin020(nil, "net1", expectedConf2, expectedResult2, nil)
+
+		result, err := CmdAdd(args, fExec, nil)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(fExec.addIndex).To(Equal(len(fExec.plugins)))
+		r := result.(*types020.Result)
+		// plugin 1 is the masterplugin
+		Expect(reflect.DeepEqual(r, expectedResult1)).To(BeTrue())
+
+		err = CmdDel(args, fExec, nil)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(fExec.delIndex).To(Equal(len(fExec.plugins)))
+	})
+
 	It("executes delegates given faulty namespace", func() {
 		args := &skel.CmdArgs{
 			ContainerID: "123456789",

pkg/multus/multus_cni040_test.go

@@ -247,6 +247,67 @@ var _ = Describe("multus operations cniVersion 0.3.1 config", func() {
 		Expect(reflect.DeepEqual(r, expectedResult1)).To(BeTrue())
 	})
 
+	It("executes delegates (plugin without interface)", func() {
+		args := &skel.CmdArgs{
+			ContainerID: "123456789",
+			Netns:       testNS.Path(),
+			IfName:      "eth0",
+			StdinData: []byte(`{
+	    "name": "node-cni-network",
+	    "type": "multus",
+	    "defaultnetworkfile": "/tmp/foo.multus.conf",
+	    "defaultnetworkwaitseconds": 3,
+	    "delegates": [{
+	        "name": "weave1",
+	        "cniVersion": "0.3.1",
+	        "type": "weave-net"
+	    },{
+	        "name": "other1",
+	        "cniVersion": "0.3.1",
+	        "type": "other-plugin"
+	    }]
+	}`),
+		}
+
+		logging.SetLogLevel("verbose")
+
+		fExec := newFakeExec()
+		expectedResult1 := &cni040.Result{
+			CNIVersion: "0.3.1",
+			IPs: []*cni040.IPConfig{{
+				Address: *testhelpers.EnsureCIDR("1.1.1.2/24"),
+			}},
+		}
+		expectedConf1 := `{
+	    "name": "weave1",
+	    "cniVersion": "0.3.1",
+	    "type": "weave-net"
+	}`
+		fExec.addPlugin040(nil, "eth0", expectedConf1, expectedResult1, nil)
+
+		// other1 just returns empty result
+		expectedResult2 := &cni040.Result{
+			CNIVersion: "0.3.1",
+		}
+		expectedConf2 := `{
+	    "name": "other1",
+	    "cniVersion": "0.3.1",
+	    "type": "other-plugin"
+	}`
+		fExec.addPlugin040(nil, "net1", expectedConf2, expectedResult2, nil)
+
+		result, err := CmdAdd(args, fExec, nil)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(fExec.addIndex).To(Equal(len(fExec.plugins)))
+		r := result.(*cni040.Result)
+		// plugin 1 is the masterplugin
+		Expect(reflect.DeepEqual(r, expectedResult1)).To(BeTrue())
+
+		err = CmdDel(args, fExec, nil)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(fExec.delIndex).To(Equal(len(fExec.plugins)))
+	})
+
 	It("fails when pod UID is provided and does not match Kube API pod UID", func() {
 		fakePod := testhelpers.NewFakePod("testpod", "net1", "")
 		net1 := `{

pkg/multus/multus_cni100_test.go

@@ -192,6 +192,71 @@ var _ = Describe("multus operations cniVersion 1.0.0 config", func() {
 		Expect(err).To(MatchError("[//:weave1]: error adding container to network \"weave1\": DelegateAdd: cannot set \"weave-net\" interface name to \"eth0\": validateIfName: no net namespace fsdadfad found: failed to Statfs \"fsdadfad\": no such file or directory"))
 	})
 
+	It("executes delegates (plugin without interface)", func() {
+		args := &skel.CmdArgs{
+			ContainerID: "123456789",
+			Netns:       testNS.Path(),
+			IfName:      "eth0",
+			StdinData: []byte(`{
+	    "name": "node-cni-network",
+	    "type": "multus",
+	    "defaultnetworkfile": "/tmp/foo.multus.conf",
+	    "defaultnetworkwaitseconds": 3,
+	    "delegates": [{
+	        "name": "weave1",
+	        "cniVersion": "1.0.0",
+	        "type": "weave-net"
+	    },{
+	        "name": "other1",
+	        "cniVersion": "1.0.0",
+	        "type": "other-plugin"
+	    }]
+	}`),
+		}
+
+		logging.SetLogLevel("verbose")
+
+		fExec := newFakeExec()
+		expectedResult1 := &cni100.Result{
+			CNIVersion: "1.0.0",
+			IPs: []*cni100.IPConfig{{
+				Address: *testhelpers.EnsureCIDR("1.1.1.2/24"),
+			},
+			},
+		}
+		expectedConf1 := `{
+	    "name": "weave1",
+	    "cniVersion": "1.0.0",
+	    "type": "weave-net"
+	}`
+		fExec.addPlugin100(nil, "eth0", expectedConf1, expectedResult1, nil)
+
+		// other1 just returns empty result
+		expectedResult2 := &cni100.Result{
+			CNIVersion: "0.4.0",
+		}
+		expectedConf2 := `{
+	    "name": "other1",
+	    "cniVersion": "1.0.0",
+	    "type": "other-plugin"
+	}`
+		fExec.addPlugin100(nil, "net1", expectedConf2, expectedResult2, nil)
+
+		result, err := CmdAdd(args, fExec, nil)
+		Expect(err).NotTo(HaveOccurred())
+
+		Expect(fExec.addIndex).To(Equal(len(fExec.plugins)))
+		// plugin 1 is the masterplugin
+		Expect(reflect.DeepEqual(result, expectedResult1)).To(BeTrue())
+
+		err = CmdCheck(args, fExec, nil)
+		Expect(err).NotTo(HaveOccurred())
+
+		err = CmdDel(args, fExec, nil)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(fExec.delIndex).To(Equal(len(fExec.plugins)))
+	})
+
 	It("returns the previous result using CmdCheck", func() {
 		args := &skel.CmdArgs{
 			ContainerID: "123456789",

pkg/server/api/shim.go

@@ -65,7 +65,8 @@ func CmdCheck(args *skel.CmdArgs) error {
 func CmdDel(args *skel.CmdArgs) error {
 	_, _, err := postRequest(args)
 	if err != nil {
-		return logging.Errorf("CmdDel (shim): %v", err)
+		// No error in DEL (as of CNI spec)
+		logging.Errorf("CmdCheck (shim): %v", err)
 	}
 	return nil
 }