From 6597fa65f546b05317e890990d338818e7662b27 Mon Sep 17 00:00:00 2001 From: Chen Tang Date: Mon, 29 Jun 2026 18:30:16 +0800 Subject: [PATCH 1/2] fix(types): preserve conflist delegate config so DEL does not leak IPs A clusterNetwork loaded from a .conflist path was reconstructed via the lossy cnitypes.PluginConf, dropping CNI-specific fields (calico's kubeconfig/datastore_type/policy, etc). ADD survived by using the complete CNINetworkConfigList, but DEL used the stripped Bytes, so calico could not build its datastore client, never released the IP, and leaked one address per teardown. - LoadDelegateNetConfFromConfList: rebuild Bytes losslessly from the libcni raw bytes (per-plugin, covering subdirectory-chain plugins) instead of marshaling the lossy NetConfList; assign Bytes once after deviceID/cni-args injection (fixes deviceID-without-cni-args drop) - collapse the redundant CNINetworkConfigList field; conflistAdd now uses Bytes uniformly, removing the dual source of truth and the TODO - CmdDel: inject cniVersion onto raw Bytes via InjectCNIVersionInConfList instead of re-marshaling the lossy ConfList (second stripping point) - add regression tests for field preservation, subdirectory-chain plugins, and cniVersion injection Signed-off-by: Chen Tang --- pkg/multus/multus.go | 27 ++++------ pkg/types/conf.go | 61 ++++++++++++++++++---- pkg/types/conf_test.go | 111 +++++++++++++++++++++++++++++++++++++++++ pkg/types/types.go | 2 - 4 files changed, 173 insertions(+), 28 deletions(-) diff --git a/pkg/multus/multus.go b/pkg/multus/multus.go index a54854ba4..e851955ea 100644 --- a/pkg/multus/multus.go +++ b/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,13 @@ 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) + // 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. + v.Bytes, err = types.InjectCNIVersionInConfList(v.Bytes, in.CNIVersion) if err != nil { // error happen but continue to delete - logging.Errorf("Multus: failed to marshal delegate %q config: %v", v.Name, err) + logging.Errorf("Multus: failed to inject cniVersion into delegate %q config: %v", v.Name, err) } } } diff --git a/pkg/types/conf.go b/pkg/types/conf.go index bfd7662ef..04757b4e6 100644 --- a/pkg/types/conf.go +++ b/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 diff --git a/pkg/types/conf_test.go b/pkg/types/conf_test.go index a0ac3a666..348a2e4ec 100644 --- a/pkg/types/conf_test.go +++ b/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")) + }) + }) diff --git a/pkg/types/types.go b/pkg/types/types.go index a306ea55d..2caff0edb 100644 --- a/pkg/types/types.go +++ b/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"` From 16881445687c9a445cb97ff6450f0719393fd10b Mon Sep 17 00:00:00 2001 From: Chen Tang Date: Tue, 30 Jun 2026 13:26:05 +0800 Subject: [PATCH 2/2] fix(multus): preserve delegate bytes when cniVersion injection fails CmdDel: do not clobber delegate Bytes when cniVersion injection fails. Assign to a temporary and only overwrite on success, so DEL keeps the original config instead of running with nil input, which would risk leaking the IP. Signed-off-by: Chen Tang --- pkg/multus/multus.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/pkg/multus/multus.go b/pkg/multus/multus.go index e851955ea..12f8e73c8 100644 --- a/pkg/multus/multus.go +++ b/pkg/multus/multus.go @@ -1086,10 +1086,14 @@ func CmdDel(args *skel.CmdArgs, exec invoke.Exec, kubeClient *k8s.ClientInfo) er // 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. - v.Bytes, err = types.InjectCNIVersionInConfList(v.Bytes, in.CNIVersion) - if err != nil { - // error happen but continue to delete - logging.Errorf("Multus: failed to inject cniVersion into delegate %q config: %v", v.Name, err) + 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 } } }