From 52b3daadbe65782c6bb1dbe09e3cba1782a8dd1e Mon Sep 17 00:00:00 2001 From: adrianc Date: Wed, 25 Sep 2024 16:38:18 +0300 Subject: [PATCH] Add option to force specifying pool name in some cases it is desired to make specifying at least one pool in poolName or pools mandatory. To achieve this we add a new IPAM CNI parameter which can be defined in nv-ipam config file. this will prevent CNI from using network name as pool name in case poolName was not specified. Signed-off-by: adrianc --- Dockerfile | 2 +- README.md | 4 + cmd/ipam-node/app/app.go | 6 +- cmd/ipam-node/app/app_test.go | 162 ++++++++++++++++++--------- cmd/ipam-node/app/options/options.go | 4 + pkg/cni/types/types.go | 19 +++- pkg/cni/types/types_test.go | 25 +++++ 7 files changed, 163 insertions(+), 59 deletions(-) diff --git a/Dockerfile b/Dockerfile index e4dfd3f..9e2efef 100644 --- a/Dockerfile +++ b/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 diff --git a/README.md b/README.md index 0ff9211..f05b52e 100644 --- a/README.md +++ b/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 diff --git a/cmd/ipam-node/app/app.go b/cmd/ipam-node/app/app.go index bc2dfdc..9cf14df 100644 --- a/cmd/ipam-node/app/app.go +++ b/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 { diff --git a/cmd/ipam-node/app/app_test.go b/cmd/ipam-node/app/app_test.go index 490086e..4da4913 100644 --- a/cmd/ipam-node/app/app_test.go +++ b/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)) }) }) diff --git a/cmd/ipam-node/app/options/options.go b/cmd/ipam-node/app/options/options.go index 8194262..76e62e4 100644 --- a/cmd/ipam-node/app/options/options.go +++ b/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 diff --git a/pkg/cni/types/types.go b/pkg/cni/types/types.go index 668b022..6e900ab 100644 --- a/pkg/cni/types/types.go +++ b/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 + } } diff --git a/pkg/cni/types/types_test.go b/pkg/cni/types/types_test.go index 21b3a46..b1efcb8 100644 --- a/pkg/cni/types/types_test.go +++ b/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"}`