Merge pull request #607 from amorenoz/bug/runtime

conf: do not modify global runtimeconfig when merging
k8snetworkplumbingwg · Feb 16, 2021 · cd9efbf · cd9efbf
2 parents 63734fc + 91e4efc
commit cd9efbf
Show file tree

Hide file tree

Showing 2 changed files with 33 additions and 21 deletions.
diff --git a/pkg/types/conf.go b/pkg/types/conf.go
@@ -145,52 +145,58 @@ func LoadDelegateNetConf(bytes []byte, net *NetworkSelectionElement, deviceID st
 // mergeCNIRuntimeConfig creates CNI runtimeconfig from delegate
 func mergeCNIRuntimeConfig(runtimeConfig *RuntimeConfig, delegate *DelegateNetConf) *RuntimeConfig {
 	logging.Debugf("mergeCNIRuntimeConfig: %v %v", runtimeConfig, delegate)
+	var mergedRuntimeConfig RuntimeConfig
+
 	if runtimeConfig == nil {
-		runtimeConfig = &RuntimeConfig{}
+		mergedRuntimeConfig = RuntimeConfig{}
+	} else {
+		mergedRuntimeConfig = *runtimeConfig
 	}
 
 	// multus inject RuntimeConfig only in case of non MasterPlugin.
 	if delegate.MasterPlugin != true {
-		logging.Debugf("mergeCNIRuntimeConfig: add runtimeConfig for net-attach-def: %v", runtimeConfig)
+		logging.Debugf("mergeCNIRuntimeConfig: add runtimeConfig for net-attach-def: %v", mergedRuntimeConfig)
 		if delegate.PortMappingsRequest != nil {
-			runtimeConfig.PortMaps = delegate.PortMappingsRequest
+			mergedRuntimeConfig.PortMaps = delegate.PortMappingsRequest
 		}
 		if delegate.BandwidthRequest != nil {
-			runtimeConfig.Bandwidth = delegate.BandwidthRequest
+			mergedRuntimeConfig.Bandwidth = delegate.BandwidthRequest
 		}
 		if delegate.IPRequest != nil {
-			runtimeConfig.IPs = delegate.IPRequest
+			mergedRuntimeConfig.IPs = delegate.IPRequest
 		}
 		if delegate.MacRequest != "" {
-			runtimeConfig.Mac = delegate.MacRequest
+			mergedRuntimeConfig.Mac = delegate.MacRequest
 		}
 		if delegate.InfinibandGUIDRequest != "" {
-			runtimeConfig.InfinibandGUID = delegate.InfinibandGUIDRequest
+			mergedRuntimeConfig.InfinibandGUID = delegate.InfinibandGUIDRequest
 		}
 		if delegate.DeviceID != "" {
-			runtimeConfig.DeviceID = delegate.DeviceID
+			mergedRuntimeConfig.DeviceID = delegate.DeviceID
 		}
-		logging.Debugf("mergeCNIRuntimeConfig: add runtimeConfig for net-attach-def: %v", runtimeConfig)
+		logging.Debugf("mergeCNIRuntimeConfig: add runtimeConfig for net-attach-def: %v", mergedRuntimeConfig)
 	}
-
-	return runtimeConfig
+	return &mergedRuntimeConfig
 }
 
 // CreateCNIRuntimeConf create CNI RuntimeConf for a delegate. If delegate configuration
 // exists, merge data with the runtime config.
 func CreateCNIRuntimeConf(args *skel.CmdArgs, k8sArgs *K8sArgs, ifName string, rc *RuntimeConfig, delegate *DelegateNetConf) (*libcni.RuntimeConf, string) {
 	logging.Debugf("LoadCNIRuntimeConf: %v, %v, %s, %v %v", args, k8sArgs, ifName, rc, delegate)
 	var cniDeviceInfoFile string
+	var delegateRc *RuntimeConfig
 
-	delegateRc := rc
 	if delegate != nil {
-		delegateRc = mergeCNIRuntimeConfig(delegateRc, delegate)
-		if delegateRc.CNIDeviceInfoFile == "" && delegate.Name != "" {
-			autoDeviceInfo := fmt.Sprintf("%s-%s_%s", delegate.Name, args.ContainerID, ifName)
-			delegateRc.CNIDeviceInfoFile = nadutils.GetCNIDeviceInfoPath(autoDeviceInfo)
-			cniDeviceInfoFile = delegateRc.CNIDeviceInfoFile
-			logging.Debugf("Adding auto-generated CNIDeviceInfoFile: %s", delegateRc.CNIDeviceInfoFile)
+		delegateRc = mergeCNIRuntimeConfig(rc, delegate)
+		if delegateRc.CNIDeviceInfoFile != "" {
+			logging.Debugf("Warning: Existing value of CNIDeviceInfoFile will be overwritten %s", delegateRc.CNIDeviceInfoFile)
 		}
+		autoDeviceInfo := fmt.Sprintf("%s-%s_%s", delegate.Name, args.ContainerID, ifName)
+		delegateRc.CNIDeviceInfoFile = nadutils.GetCNIDeviceInfoPath(autoDeviceInfo)
+		cniDeviceInfoFile = delegateRc.CNIDeviceInfoFile
+		logging.Debugf("Adding auto-generated CNIDeviceInfoFile: %s", delegateRc.CNIDeviceInfoFile)
+	} else {
+		delegateRc = rc
 	}
 
 	// In part, adapted from K8s pkg/kubelet/dockershim/network/cni/cni.go#buildCNIRuntimeConf

diff --git a/pkg/types/conf_test.go b/pkg/types/conf_test.go
@@ -26,8 +26,8 @@ import (
 	types020 "github.com/containernetworking/cni/pkg/types/020"
 	"github.com/containernetworking/plugins/pkg/ns"
 	"github.com/containernetworking/plugins/pkg/testutils"
-	testhelpers "gopkg.in/intel/multus-cni.v3/pkg/testing"
 	netutils "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/utils"
+	testhelpers "gopkg.in/intel/multus-cni.v3/pkg/testing"
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
@@ -757,11 +757,14 @@ var _ = Describe("config operations", func() {
 		}
 		delegate, err := LoadDelegateNetConf([]byte(conf), networkSelection, "", "")
 		delegate.MasterPlugin = true
+		origRuntimeConfig := RuntimeConfig{}
 		Expect(err).NotTo(HaveOccurred())
-		runtimeConf := mergeCNIRuntimeConfig(&RuntimeConfig{}, delegate)
+		runtimeConf := mergeCNIRuntimeConfig(&origRuntimeConfig, delegate)
 		Expect(runtimeConf.PortMaps).To(BeNil())
 		Expect(runtimeConf.Bandwidth).To(BeNil())
 		Expect(runtimeConf.InfinibandGUID).To(Equal(""))
+		// The original RuntimeConfig must have not been overwritten
+		Expect(origRuntimeConfig).To(Equal(RuntimeConfig{}))
 	})
 
 	It("test mergeCNIRuntimeConfig with delegate plugin", func() {
@@ -792,15 +795,18 @@ var _ = Describe("config operations", func() {
 			BandwidthRequest:      bandwidthEntry1,
 			PortMappingsRequest:   []*PortMapEntry{portMapEntry1},
 		}
+		origRuntimeConfig := RuntimeConfig{}
 		delegate, err := LoadDelegateNetConf([]byte(conf), networkSelection, "", "")
 		Expect(err).NotTo(HaveOccurred())
-		runtimeConf := mergeCNIRuntimeConfig(&RuntimeConfig{}, delegate)
+		runtimeConf := mergeCNIRuntimeConfig(&origRuntimeConfig, delegate)
 		Expect(runtimeConf.PortMaps).NotTo(BeNil())
 		Expect(len(runtimeConf.PortMaps)).To(BeEquivalentTo(1))
 		Expect(runtimeConf.PortMaps[0]).To(Equal(portMapEntry1))
 		Expect(runtimeConf.Bandwidth).To(Equal(bandwidthEntry1))
 		Expect(len(runtimeConf.IPs)).To(BeEquivalentTo(1))
 		Expect(runtimeConf.Mac).To(Equal("c2:11:22:33:44:66"))
 		Expect(runtimeConf.InfinibandGUID).To(Equal("24:8a:07:03:00:8d:ae:2e"))
+		// The original RuntimeConfig must have not been overwritten
+		Expect(origRuntimeConfig).To(Equal(RuntimeConfig{}))
 	})
 })