diff --git a/pkg/multus/multus.go b/pkg/multus/multus.go index 1dedf8575..f85092760 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,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 } } } 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"`