Merge pull request #53 from adrianchiris/force-pool-name

Add option to force specifying pool name

Dockerfile

@@ -1,5 +1,5 @@
 # Build the image
-FROM golang:1.21 as builder
+FROM golang:1.21 AS builder
 
 WORKDIR /workspace
 # Copy the Go Modules manifests

README.md

@@ -559,6 +559,8 @@ Shim CNI Configuration flags:
                 shim CNI config: timeout for IPAM daemon calls (default 5)
       --cni-daemon-socket string                                                                                                                                                                     
                 shim CNI config: IPAM daemon socket path (default "unix:///var/lib/cni/nv-ipam/daemon.sock")
+      --cni-force-pool-name string
+                shim CNI config: force specifying pool name in CNI configuration
       --cni-log-file string                                                                                                                                                                          
                 shim CNI config: path to log file for shim CNI (default "/var/log/nv-ipam-cni.log")
       --cni-log-level string                                                                                                                                                                         
@@ -579,6 +581,7 @@ nv-ipam accepts the following CNI configuration:
 ```json
 {
     "type": "nv-ipam",
+    "forcePoolName" : false,
     "poolName": "pool1,pool2",
     "poolType": "ippool",
     "daemonSocket": "unix:///var/lib/cni/nv-ipam/daemon.sock",
@@ -590,6 +593,7 @@ nv-ipam accepts the following CNI configuration:
 ```
 
 * `type` (string, required): CNI plugin name, MUST be `"nv-ipam"`
+* `forcePoolName` (bool, optional): force specifying pool name in CNI configuration.
 * `poolName` (string, optional): name of the Pool to be used for IP allocation.
 It is possible to allocate two IPs for the interface from different pools by specifying pool names separated by coma,
 e.g. `"my-ipv4-pool,my-ipv6-pool"`. The primary intent to support multiple pools is a dual-stack use-case when an 

cmd/ipam-node/app/app.go

@@ -359,9 +359,9 @@ func createNVIPAMConfig(log logr.Logger, opts *options.Options) error {
   "daemonSocket":    "%s",
   "daemonCallTimeoutSeconds":    %d,
   "logFile":   "%s",
-  "logLevel": "%s"
-}
-`, opts.CNIDaemonSocket, opts.CNIDaemonCallTimeoutSeconds, opts.CNILogFile, opts.CNILogLevel)
+  "logLevel": "%s",
+  "forcePoolName": %v
+}`, opts.CNIDaemonSocket, opts.CNIDaemonCallTimeoutSeconds, opts.CNILogFile, opts.CNILogLevel, opts.CNIForcePoolName)
 
 	err := renameio.WriteFile(filepath.Join(opts.CNIConfDir, cniTypes.ConfFileName), []byte(cfg), 0664)
 	if err != nil {

cmd/ipam-node/app/app_test.go

@@ -14,8 +14,11 @@
 package app_test
 
 import (
+	"context"
+	"encoding/json"
 	"os"
 	"path/filepath"
+	"sync"
 	"time"
 
 	"github.com/go-logr/logr"
@@ -34,6 +37,7 @@ import (
 	ipamv1alpha1 "github.com/Mellanox/nvidia-k8s-ipam/api/v1alpha1"
 	"github.com/Mellanox/nvidia-k8s-ipam/cmd/ipam-node/app"
 	"github.com/Mellanox/nvidia-k8s-ipam/cmd/ipam-node/app/options"
+	"github.com/Mellanox/nvidia-k8s-ipam/pkg/cni/types"
 )
 
 const (
@@ -158,6 +162,7 @@ func getOptions(testDir string) *options.Options {
 	opts.CNIConfDir = cniConfDir
 	opts.CNIDaemonSocket = daemonSocket
 	opts.PoolsNamespace = testNamespace
+	opts.CNIForcePoolName = true
 	return opts
 }
 
@@ -175,68 +180,117 @@ func getValidReqParams(uid, name, namespace string) *nodev1.IPAMParameters {
 	}
 }
 
+func pathExists(path string) error {
+	_, err := os.Stat(path)
+	return err
+}
+
 var _ = Describe("IPAM Node daemon", func() {
-	It("Validate main flows", func() {
-		done := make(chan interface{})
+	var (
+		wg          sync.WaitGroup
+		testDir     string
+		opts        *options.Options
+		cFuncDaemon context.CancelFunc
+		daemonCtx   context.Context
+	)
+
+	BeforeEach(func() {
+		wg = sync.WaitGroup{}
+		testDir = GinkgoT().TempDir()
+		opts = getOptions(testDir)
+
+		daemonCtx, cFuncDaemon = context.WithCancel(ctx)
+		wg.Add(1)
 		go func() {
+			defer wg.Done()
 			defer GinkgoRecover()
-			defer close(done)
-			testDir := GinkgoT().TempDir()
-			opts := getOptions(testDir)
+			Expect(app.RunNodeDaemon(logr.NewContext(daemonCtx, klog.NewKlogr()), cfg, opts)).NotTo(HaveOccurred())
+		}()
+	})
+
+	AfterEach(func() {
+		cFuncDaemon()
+		wg.Wait()
+	})
+
+	It("Validate main flows", func() {
+		createTestIPPools()
+		createTestCIDRPools()
+		pod := createTestPod()
 
-			createTestIPPools()
-			createTestCIDRPools()
-			pod := createTestPod()
+		conn, err := grpc.DialContext(ctx, opts.CNIDaemonSocket,
+			grpc.WithTransportCredentials(insecure.NewCredentials()),
+			grpc.WithBlock())
+		Expect(err).NotTo(HaveOccurred())
 
-			ctx = logr.NewContext(ctx, klog.NewKlogr())
+		grpcClient := nodev1.NewIPAMServiceClient(conn)
 
-			go func() {
-				Expect(app.RunNodeDaemon(ctx, cfg, opts)).NotTo(HaveOccurred())
-			}()
+		cidrPoolParams := getValidReqParams(string(pod.UID), pod.Name, pod.Namespace)
+		cidrPoolParams.PoolType = nodev1.PoolType_POOL_TYPE_CIDRPOOL
+		ipPoolParams := getValidReqParams(string(pod.UID), pod.Name, pod.Namespace)
 
-			conn, err := grpc.DialContext(ctx, opts.CNIDaemonSocket,
-				grpc.WithTransportCredentials(insecure.NewCredentials()),
-				grpc.WithBlock())
+		for _, params := range []*nodev1.IPAMParameters{ipPoolParams, cidrPoolParams} {
+			// no allocation yet
+			_, err = grpcClient.IsAllocated(ctx,
+				&nodev1.IsAllocatedRequest{Parameters: params})
+			Expect(status.Code(err) == codes.NotFound).To(BeTrue())
+
+			// allocate
+			resp, err := grpcClient.Allocate(ctx, &nodev1.AllocateRequest{Parameters: params})
 			Expect(err).NotTo(HaveOccurred())
+			Expect(resp.Allocations).To(HaveLen(2))
+			Expect(resp.Allocations[0].Pool).NotTo(BeEmpty())
+			Expect(resp.Allocations[0].Gateway).NotTo(BeEmpty())
+			Expect(resp.Allocations[0].Ip).NotTo(BeEmpty())
 
-			grpcClient := nodev1.NewIPAMServiceClient(conn)
-
-			cidrPoolParams := getValidReqParams(string(pod.UID), pod.Name, pod.Namespace)
-			cidrPoolParams.PoolType = nodev1.PoolType_POOL_TYPE_CIDRPOOL
-			ipPoolParams := getValidReqParams(string(pod.UID), pod.Name, pod.Namespace)
-
-			for _, params := range []*nodev1.IPAMParameters{ipPoolParams, cidrPoolParams} {
-				// no allocation yet
-				_, err = grpcClient.IsAllocated(ctx,
-					&nodev1.IsAllocatedRequest{Parameters: params})
-				Expect(status.Code(err) == codes.NotFound).To(BeTrue())
-
-				// allocate
-				resp, err := grpcClient.Allocate(ctx, &nodev1.AllocateRequest{Parameters: params})
-				Expect(err).NotTo(HaveOccurred())
-				Expect(resp.Allocations).To(HaveLen(2))
-				Expect(resp.Allocations[0].Pool).NotTo(BeEmpty())
-				Expect(resp.Allocations[0].Gateway).NotTo(BeEmpty())
-				Expect(resp.Allocations[0].Ip).NotTo(BeEmpty())
-
-				_, err = grpcClient.IsAllocated(ctx,
-					&nodev1.IsAllocatedRequest{Parameters: params})
-				Expect(err).NotTo(HaveOccurred())
-
-				// deallocate
-				_, err = grpcClient.Deallocate(ctx, &nodev1.DeallocateRequest{Parameters: params})
-				Expect(err).NotTo(HaveOccurred())
-
-				// deallocate should be idempotent
-				_, err = grpcClient.Deallocate(ctx, &nodev1.DeallocateRequest{Parameters: params})
-				Expect(err).NotTo(HaveOccurred())
-
-				// check should fail
-				_, err = grpcClient.IsAllocated(ctx,
-					&nodev1.IsAllocatedRequest{Parameters: params})
-				Expect(status.Code(err) == codes.NotFound).To(BeTrue())
-			}
-		}()
-		Eventually(done, 5*time.Minute).Should(BeClosed())
+			_, err = grpcClient.IsAllocated(ctx,
+				&nodev1.IsAllocatedRequest{Parameters: params})
+			Expect(err).NotTo(HaveOccurred())
+
+			// deallocate
+			_, err = grpcClient.Deallocate(ctx, &nodev1.DeallocateRequest{Parameters: params})
+			Expect(err).NotTo(HaveOccurred())
+
+			// deallocate should be idempotent
+			_, err = grpcClient.Deallocate(ctx, &nodev1.DeallocateRequest{Parameters: params})
+			Expect(err).NotTo(HaveOccurred())
+
+			// check should fail
+			_, err = grpcClient.IsAllocated(ctx,
+				&nodev1.IsAllocatedRequest{Parameters: params})
+			Expect(status.Code(err) == codes.NotFound).To(BeTrue())
+		}
+	})
+
+	It("deployShimCNI works as expected", func() {
+		// cni binary copied
+		Eventually(func() error {
+			return pathExists(filepath.Join(testDir, "cnibin", "nv-ipam"))
+		}).
+			WithTimeout(2 * time.Second).
+			ShouldNot(HaveOccurred())
+		// conf file copied
+		Eventually(func() error {
+			return pathExists(filepath.Join(testDir, "cniconf", "nv-ipam.conf"))
+		}).
+			WithTimeout(2 * time.Second).
+			ShouldNot(HaveOccurred())
+		// store dir created
+		Eventually(func() error {
+			return pathExists(filepath.Join(testDir, "store"))
+		}).
+			WithTimeout(2 * time.Second).
+			ShouldNot(HaveOccurred())
+		// conf file contains expected results
+		data, err := os.ReadFile(filepath.Join(testDir, "cniconf", "nv-ipam.conf"))
+		Expect(err).ToNot(HaveOccurred())
+		ipamConf := types.IPAMConf{}
+		Expect(json.Unmarshal(data, &ipamConf)).ToNot(HaveOccurred())
+		Expect(ipamConf.DaemonSocket).To(Equal(opts.CNIDaemonSocket))
+		Expect(ipamConf.DaemonCallTimeoutSeconds).To(Equal(opts.CNIDaemonCallTimeoutSeconds))
+		Expect(ipamConf.LogFile).To(Equal(opts.CNILogFile))
+		Expect(ipamConf.LogLevel).To(Equal(opts.CNILogLevel))
+		Expect(ipamConf.ForcePoolName).ToNot(BeNil())
+		Expect(*ipamConf.ForcePoolName).To(Equal(opts.CNIForcePoolName))
 	})
 })

cmd/ipam-node/app/options/options.go

@@ -53,6 +53,7 @@ func New() *Options {
 		CNIConfDir:                  cniTypes.DefaultConfDir,
 		CNILogLevel:                 cniTypes.DefaultLogLevel,
 		CNILogFile:                  cniTypes.DefaultLogFile,
+		CNIForcePoolName:            false,
 	}
 }
 
@@ -75,6 +76,7 @@ type Options struct {
 	CNIDaemonCallTimeoutSeconds int
 	CNILogFile                  string
 	CNILogLevel                 string
+	CNIForcePoolName            bool
 }
 
 // AddNamedFlagSets register flags for common options in NamedFlagSets
@@ -119,6 +121,8 @@ func (o *Options) AddNamedFlagSets(sharedFS *cliflag.NamedFlagSets) {
 		"shim CNI config: path to log file for shim CNI")
 	cniFS.StringVar(&o.CNILogLevel, "cni-log-level", o.CNILogLevel,
 		"shim CNI config: log level for shim CNI")
+	cniFS.BoolVar(&o.CNIForcePoolName, "cni-force-pool-name", o.CNIForcePoolName,
+		"shim CNI config: force specifying pool name in CNI configuration")
 }
 
 // Validate registered options

pkg/cni/types/types.go

@@ -23,6 +23,7 @@ import (
 
 	"github.com/containernetworking/cni/pkg/skel"
 	"github.com/containernetworking/cni/pkg/types"
+	"k8s.io/utils/ptr"
 
 	"github.com/Mellanox/nvidia-k8s-ipam/pkg/common"
 )
@@ -54,6 +55,9 @@ type ConfLoader interface {
 type IPAMConf struct {
 	types.IPAM
 
+	// ForcePoolName if set, specifying PoolName in CNI call is mandatory and will
+	// not be derrived from the network name.
+	ForcePoolName *bool `json:"forcePoolName,omitempty"`
 	// PoolName is the name of the pool to be used to allocate IP
 	PoolName string `json:"poolName,omitempty"`
 	// PoolType is the type of the pool which is referred by the PoolName,
@@ -150,7 +154,7 @@ func (cl *confLoader) LoadConf(args *skel.CmdArgs) (*NetConf, error) {
 	// overlay config with defaults
 	defaultConf := &IPAMConf{
 		// use network name as pool name by default
-		PoolName:                 n.Name,
+		ForcePoolName:            ptr.To(false),
 		PoolType:                 common.PoolTypeIPPool,
 		ConfDir:                  DefaultConfDir,
 		LogFile:                  DefaultLogFile,
@@ -160,6 +164,11 @@ func (cl *confLoader) LoadConf(args *skel.CmdArgs) (*NetConf, error) {
 	}
 	cl.overlayConf(defaultConf, n.IPAM)
 
+	if !*n.IPAM.ForcePoolName && n.IPAM.PoolName == "" {
+		// set poolName as network name
+		n.IPAM.PoolName = n.Name
+	}
+
 	// static IP address priority:
 	// stdin runtimeConfig["ips"] > stdin args["cni"]["ips"] > IP argument from CNI_ARGS env variable
 	requestedIPs := n.RuntimeConfig.IPs
@@ -194,6 +203,7 @@ func (cl *confLoader) LoadConf(args *skel.CmdArgs) (*NetConf, error) {
 	if err != nil {
 		return nil, err
 	}
+
 	n.IPAM.PoolType, err = getPoolType(n)
 	if err != nil {
 		return nil, err
@@ -243,6 +253,9 @@ func getPools(n *NetConf) ([]string, error) {
 	if n.Args != nil && len(n.Args.ArgsCNI.PoolNames) > 0 {
 		pools = n.Args.ArgsCNI.PoolNames
 	} else {
+		if n.IPAM.PoolName == "" {
+			return nil, fmt.Errorf("no pool provided")
+		}
 		pools = strings.Split(n.IPAM.PoolName, ",")
 	}
 	if len(pools) > 2 {
@@ -319,4 +332,8 @@ func (cl *confLoader) overlayConf(from, to *IPAMConf) {
 	if to.DaemonCallTimeoutSeconds == 0 {
 		to.DaemonCallTimeoutSeconds = from.DaemonCallTimeoutSeconds
 	}
+
+	if to.ForcePoolName == nil {
+		to.ForcePoolName = from.ForcePoolName
+	}
 }

pkg/cni/types/types_test.go

@@ -109,6 +109,31 @@ var _ = Describe("Types Tests", func() {
 			Expect(err).To(HaveOccurred())
 		})
 
+		It("Fails if CNI stdin data does not contain pool name and forcePoolName is set in config file", func() {
+			// write config file
+			confData := `{"forcePoolName": true}`
+			err := os.WriteFile(path.Join(testConfDir, cniTypes.ConfFileName), []byte(confData), 0o644)
+			Expect(err).ToNot(HaveOccurred())
+
+			// Load CNI config
+			testConf := fmt.Sprintf(`{"name": "my-net", "ipam": {"confDir": %q}}`, testConfDir)
+			_, err = cniTypes.NewConfLoader().LoadConf(&skel.CmdArgs{
+				StdinData: []byte(testConf), Args: testArgs})
+			Expect(err).To(HaveOccurred())
+		})
+
+		It("Fails if CNI stdin data does not contain pool name and forcePoolName is set", func() {
+			// write empty config file
+			err := os.WriteFile(path.Join(testConfDir, cniTypes.ConfFileName), []byte("{}"), 0o644)
+			Expect(err).ToNot(HaveOccurred())
+
+			// Load CNI config
+			testConf := fmt.Sprintf(`{"name": "my-net", "ipam": {"confDir": %q, "forcePoolName": true}}`, testConfDir)
+			_, err = cniTypes.NewConfLoader().LoadConf(&skel.CmdArgs{
+				StdinData: []byte(testConf), Args: testArgs})
+			Expect(err).To(HaveOccurred())
+		})
+
 		It("Missing metadata arguments", func() {
 			// write config file
 			confData := `{"logLevel": "debug", "logFile": "some/path.log"}`