Merge pull request #1521 from chent1996/fix/conflist-del-ip-leak-fork

fix(types): preserve conflist delegate config so DEL does not leak IPs

pkg/multus/multus.go

@@ -296,25 +296,16 @@ func confStatus(rt *libcni.RuntimeConf, rawNetconf []byte, multusNetconf *types.
 	return err
 }
 
-func conflistAdd(rt *libcni.RuntimeConf, rawnetconflist []byte, cniConfList *libcni.NetworkConfigList, multusNetconf *types.NetConf, exec invoke.Exec) (cnitypes.Result, error) {
+func conflistAdd(rt *libcni.RuntimeConf, rawnetconflist []byte, multusNetconf *types.NetConf, exec invoke.Exec) (cnitypes.Result, error) {
 	logging.Debugf("conflistAdd: %v, %s", rt, string(rawnetconflist))
 	// In part, adapted from K8s pkg/kubelet/dockershim/network/cni/cni.go
 	binDirs := filepath.SplitList(os.Getenv("CNI_PATH"))
 	binDirs = append([]string{multusNetconf.BinDir}, binDirs...)
 	cniNet := libcni.NewCNIConfigWithCacheDir(binDirs, multusNetconf.CNIDir, exec)
 
-	var confList *libcni.NetworkConfigList
-	var err error
-
-	// This may wind up being set during parsing the default network config.
-	// In this case -- we'll use it as passed. Otherwise, we'll recalculate it.
-	if len(cniConfList.Plugins) > 0 {
-		confList = cniConfList
-	} else {
-		confList, err = libcni.NetworkConfFromBytes(rawnetconflist)
-		if err != nil {
-			return nil, logging.Errorf("conflistAdd: error converting the raw bytes into a conflist: %v", err)
-		}
+	confList, err := libcni.NetworkConfFromBytes(rawnetconflist)
+	if err != nil {
+		return nil, logging.Errorf("conflistAdd: error converting the raw bytes into a conflist: %v", err)
 	}
 
 	result, err := cniNet.AddNetworkList(context.Background(), confList, rt)
@@ -436,8 +427,7 @@ func DelegateAdd(exec invoke.Exec, kubeClient *k8s.ClientInfo, pod *v1.Pod, dele
 	var result cnitypes.Result
 	var err error
 	if delegate.ConfListPlugin {
-		// TODO: why are we passing bytes here? don't we have a better representation of it?
-		result, err = conflistAdd(rt, delegate.Bytes, &delegate.CNINetworkConfigList, multusNetconf, exec)
+		result, err = conflistAdd(rt, delegate.Bytes, multusNetconf, exec)
 		if err != nil {
 			return nil, err
 		}
@@ -1093,10 +1083,17 @@ func CmdDel(args *skel.CmdArgs, exec invoke.Exec, kubeClient *k8s.ClientInfo) er
 	for _, v := range in.Delegates {
 		if v.ConfListPlugin && v.ConfList.CNIVersion == "" && in.CNIVersion != "" {
 			v.ConfList.CNIVersion = in.CNIVersion
-			v.Bytes, err = json.Marshal(v.ConfList)
-			if err != nil {
-				// error happen but continue to delete
-				logging.Errorf("Multus: failed to marshal delegate %q config: %v", v.Name, err)
+			// Inject cniVersion onto the raw bytes losslessly. Marshaling the
+			// structured ConfList (types.NetConfList) here would strip
+			// CNI-specific fields (e.g. calico's kubeconfig) and break DEL.
+			updatedBytes, injectErr := types.InjectCNIVersionInConfList(v.Bytes, in.CNIVersion)
+			if injectErr != nil {
+				// error happen but continue to delete; keep the original bytes
+				// rather than clobbering them with nil, which would feed DEL
+				// worse input than before and risk leaking the IP.
+				logging.Errorf("Multus: failed to inject cniVersion into delegate %q config: %v", v.Name, injectErr)
+			} else {
+				v.Bytes = updatedBytes
 			}
 		}
 	}

pkg/types/conf.go

@@ -83,6 +83,45 @@ func ConvertNetworkConfigListToNetConfList(ncList *libcni.NetworkConfigList) (*t
 	return netConfList, nil
 }
 
+// rawConfListBytes rebuilds the complete conflist JSON from a libcni
+// NetworkConfigList without losing any field. It takes the list-level keys
+// from the original bytes and rebuilds the "plugins" array from each plugin's
+// raw bytes. Per-plugin raw bytes are used (instead of confList.Bytes as a
+// whole) because libcni appends plugins loaded from a subdirectory chain into
+// confList.Plugins without updating confList.Bytes; relying on confList.Bytes
+// alone would drop those appended plugins.
+func rawConfListBytes(confList *libcni.NetworkConfigList) ([]byte, error) {
+	var rawList map[string]interface{}
+	if err := json.Unmarshal(confList.Bytes, &rawList); err != nil {
+		return nil, logging.Errorf("rawConfListBytes: failed to unmarshal conflist bytes: %v", err)
+	}
+
+	plugins := make([]interface{}, 0, len(confList.Plugins))
+	for idx, plugin := range confList.Plugins {
+		var rawPlugin map[string]interface{}
+		if err := json.Unmarshal(plugin.Bytes, &rawPlugin); err != nil {
+			return nil, logging.Errorf("rawConfListBytes: failed to unmarshal plugin #%d: %v", idx, err)
+		}
+		plugins = append(plugins, rawPlugin)
+	}
+	rawList["plugins"] = plugins
+
+	return json.Marshal(rawList)
+}
+
+// InjectCNIVersionInConfList sets the cniVersion field on a conflist JSON
+// without losing any other field. It is used on the DEL path to backfill the
+// cniVersion onto the raw bytes; marshaling the structured NetConfList instead
+// would strip CNI-specific fields and break the plugin's DEL.
+func InjectCNIVersionInConfList(inBytes []byte, cniVersion string) ([]byte, error) {
+	var rawConfig map[string]interface{}
+	if err := json.Unmarshal(inBytes, &rawConfig); err != nil {
+		return nil, logging.Errorf("InjectCNIVersionInConfList: failed to unmarshal inBytes: %v", err)
+	}
+	rawConfig["cniVersion"] = cniVersion
+	return json.Marshal(rawConfig)
+}
+
 // LoadDelegateNetConfFromConfList converts a libcni.NetworkConfigList into a DelegateNetConf structure
 func LoadDelegateNetConfFromConfList(confList *libcni.NetworkConfigList, netElement *NetworkSelectionElement, deviceID string, resourceName string) (*DelegateNetConf, error) {
 	var err error
@@ -95,18 +134,21 @@ func LoadDelegateNetConfFromConfList(confList *libcni.NetworkConfigList, netElem
 	}
 
 	delegateConf := &DelegateNetConf{
-		Name:                 netConfList.Name,
-		ConfList:             *netConfList,
-		CNINetworkConfigList: *confList,
-		ConfListPlugin:       true,
+		Name:           netConfList.Name,
+		ConfList:       *netConfList,
+		ConfListPlugin: true,
 	}
 
-	// Convert the plugins back to bytes for consistency
-	pluginsBytes, err := json.Marshal(netConfList)
+	// Preserve the original conflist bytes losslessly. The structured
+	// NetConfList (via cnitypes.PluginConf) drops any field outside
+	// cniVersion/name/type/capabilities/ipam.type/dns, which would strip
+	// CNI-specific fields such as calico's kubeconfig/datastore_type and
+	// break the plugin's DEL (e.g. failing to release IPs). Rebuild from the
+	// libcni raw bytes instead so DEL receives the same complete config ADD did.
+	pluginsBytes, err := rawConfListBytes(confList)
 	if err != nil {
-		return nil, logging.Errorf("LoadDelegateNetConfFromConfList: error marshaling netConfList: %v", err)
+		return nil, logging.Errorf("LoadDelegateNetConfFromConfList: error rebuilding conflist bytes: %v", err)
 	}
-	delegateConf.Bytes = pluginsBytes
 
 	if deviceID != "" {
 		pluginsBytes, err = addDeviceIDInConfList(pluginsBytes, deviceID)
@@ -122,9 +164,10 @@ func LoadDelegateNetConfFromConfList(confList *libcni.NetworkConfigList, netElem
 		if err != nil {
 			return nil, logging.Errorf("LoadDelegateNetConfFromConfList: failed to add cni-args in NetConfList bytes: %v", err)
 		}
-		delegateConf.Bytes = pluginsBytes
 	}
 
+	delegateConf.Bytes = pluginsBytes
+
 	if netElement != nil {
 		if netElement.Name != "" {
 			// Overwrite CNI config name with net-attach-def name

pkg/types/conf_test.go

@@ -23,6 +23,7 @@ import (
 	"os"
 	"testing"
 
+	"github.com/containernetworking/cni/libcni"
 	"github.com/containernetworking/cni/pkg/skel"
 	types020 "github.com/containernetworking/cni/pkg/types/020"
 	"github.com/containernetworking/plugins/pkg/ns"
@@ -1021,4 +1022,114 @@ var _ = Describe("config operations", func() {
 		Expect(netconf.IsFilterV6Gateway).To(BeFalse())
 	})
 
+	It("LoadDelegateNetConfFromConfList preserves CNI-specific fields so DEL does not leak", func() {
+		// A calico-style conflist carries fields (kubeconfig, datastore_type,
+		// policy, ...) that the structured NetConfList would drop. If they are
+		// stripped, calico's DEL fails and the IP is never released.
+		conflist := `{
+			"name": "k8s-pod-network",
+			"cniVersion": "0.3.1",
+			"plugins": [
+				{
+					"type": "calico",
+					"datastore_type": "kubernetes",
+					"nodename": "node-1",
+					"mtu": 1500,
+					"policy": {"type": "k8s"},
+					"kubernetes": {"kubeconfig": "/etc/cni/net.d/calico-kubeconfig"},
+					"ipam": {"type": "calico-ipam"}
+				},
+				{
+					"type": "portmap",
+					"capabilities": {"portMappings": true}
+				}
+			]
+		}`
+
+		confList, err := libcni.NetworkConfFromBytes([]byte(conflist))
+		Expect(err).NotTo(HaveOccurred())
+
+		delegate, err := LoadDelegateNetConfFromConfList(confList, nil, "", "")
+		Expect(err).NotTo(HaveOccurred())
+
+		var parsed map[string]interface{}
+		Expect(json.Unmarshal(delegate.Bytes, &parsed)).To(Succeed())
+
+		plugins, ok := parsed["plugins"].([]interface{})
+		Expect(ok).To(BeTrue())
+		Expect(plugins).To(HaveLen(2))
+
+		calico, ok := plugins[0].(map[string]interface{})
+		Expect(ok).To(BeTrue())
+		Expect(calico["datastore_type"]).To(Equal("kubernetes"))
+		Expect(calico["nodename"]).To(Equal("node-1"))
+		Expect(calico["mtu"]).To(BeEquivalentTo(1500))
+		Expect(calico["policy"]).NotTo(BeNil())
+
+		k8s, ok := calico["kubernetes"].(map[string]interface{})
+		Expect(ok).To(BeTrue())
+		Expect(k8s["kubeconfig"]).To(Equal("/etc/cni/net.d/calico-kubeconfig"))
+
+		ipam, ok := calico["ipam"].(map[string]interface{})
+		Expect(ok).To(BeTrue())
+		Expect(ipam["type"]).To(Equal("calico-ipam"))
+	})
+
+	It("LoadDelegateNetConfFromConfList keeps plugins appended from a subdirectory chain", func() {
+		// libcni appends subdirectory-chain plugins into confList.Plugins but
+		// does NOT update confList.Bytes. Rebuilding from confList.Bytes alone
+		// would drop them, so rawConfListBytes must use per-plugin Bytes.
+		base := `{
+			"name": "k8s-pod-network",
+			"cniVersion": "0.3.1",
+			"plugins": [
+				{"type": "calico", "kubernetes": {"kubeconfig": "/etc/cni/net.d/calico-kubeconfig"}}
+			]
+		}`
+
+		confList, err := libcni.NetworkConfFromBytes([]byte(base))
+		Expect(err).NotTo(HaveOccurred())
+		Expect(confList.Plugins).To(HaveLen(1))
+
+		// Simulate a plugin appended from the subdirectory chain: present in
+		// Plugins, absent from the list-level Bytes.
+		appended, err := libcni.NetworkPluginConfFromBytes([]byte(`{"type": "bandwidth", "capabilities": {"bandwidth": true}}`))
+		Expect(err).NotTo(HaveOccurred())
+		confList.Plugins = append(confList.Plugins, appended)
+
+		delegate, err := LoadDelegateNetConfFromConfList(confList, nil, "", "")
+		Expect(err).NotTo(HaveOccurred())
+
+		var parsed map[string]interface{}
+		Expect(json.Unmarshal(delegate.Bytes, &parsed)).To(Succeed())
+		plugins, ok := parsed["plugins"].([]interface{})
+		Expect(ok).To(BeTrue())
+		Expect(plugins).To(HaveLen(2))
+		Expect(plugins[1].(map[string]interface{})["type"]).To(Equal("bandwidth"))
+	})
+
+	It("InjectCNIVersionInConfList sets cniVersion without dropping fields", func() {
+		conflist := `{
+			"name": "k8s-pod-network",
+			"plugins": [
+				{"type": "calico", "kubernetes": {"kubeconfig": "/etc/cni/net.d/calico-kubeconfig"}}
+			]
+		}`
+
+		out, err := InjectCNIVersionInConfList([]byte(conflist), "0.3.1")
+		Expect(err).NotTo(HaveOccurred())
+
+		var parsed map[string]interface{}
+		Expect(json.Unmarshal(out, &parsed)).To(Succeed())
+		Expect(parsed["cniVersion"]).To(Equal("0.3.1"))
+
+		plugins, ok := parsed["plugins"].([]interface{})
+		Expect(ok).To(BeTrue())
+		calico, ok := plugins[0].(map[string]interface{})
+		Expect(ok).To(BeTrue())
+		k8s, ok := calico["kubernetes"].(map[string]interface{})
+		Expect(ok).To(BeTrue())
+		Expect(k8s["kubeconfig"]).To(Equal("/etc/cni/net.d/calico-kubeconfig"))
+	})
+
 })

pkg/types/types.go

@@ -19,7 +19,6 @@ import (
 	"net"
 	"sort"
 
-	"github.com/containernetworking/cni/libcni"
 	"github.com/containernetworking/cni/pkg/types"
 	cni100 "github.com/containernetworking/cni/pkg/types/100"
 	"gopkg.in/k8snetworkplumbingwg/multus-cni.v4/pkg/logging"
@@ -101,7 +100,6 @@ type BandwidthEntry struct {
 type DelegateNetConf struct {
 	Conf                  types.NetConf
 	ConfList              types.NetConfList
-	CNINetworkConfigList  libcni.NetworkConfigList
 	Name                  string
 	IfnameRequest         string          `json:"ifnameRequest,omitempty"`
 	MacRequest            string          `json:"macRequest,omitempty"`