From f069e36ea5b4499fba8a0ba1007980f924c4fac7 Mon Sep 17 00:00:00 2001 From: Vasilis Remmas Date: Tue, 6 Aug 2024 08:21:31 +0200 Subject: [PATCH] Adjust CIDRPool to use int32 instead of uint fields Signed-off-by: Vasilis Remmas --- api/v1alpha1/cidrpool_test.go | 49 +++++++++---------- api/v1alpha1/cidrpool_type.go | 4 +- api/v1alpha1/cidrpool_validate.go | 15 +++++- api/v1alpha1/helpers.go | 2 +- api/v1alpha1/helpers_test.go | 2 +- api/v1alpha1/zz_generated.deepcopy.go | 2 +- cmd/ipam-controller/app/app_test.go | 10 ++-- cmd/ipam-node/app/app_test.go | 4 +- deploy/crds/nv-ipam.nvidia.com_cidrpools.yaml | 2 + pkg/ip/cidr.go | 4 +- 10 files changed, 52 insertions(+), 42 deletions(-) diff --git a/api/v1alpha1/cidrpool_test.go b/api/v1alpha1/cidrpool_test.go index 14aeeb4..a80fecc 100644 --- a/api/v1alpha1/cidrpool_test.go +++ b/api/v1alpha1/cidrpool_test.go @@ -20,6 +20,7 @@ import ( gomegaTypes "github.com/onsi/gomega/types" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" "github.com/Mellanox/nvidia-k8s-ipam/api/v1alpha1" ) @@ -39,10 +40,6 @@ func validatePoolAndCheckErr(pool *v1alpha1.CIDRPool, isValid bool, errMatcher . } } -func getUintPtr(i uint) *uint { - return &i -} - var _ = Describe("CIDRPool", func() { It("Valid IPv4 pool", func() { cidrPool := v1alpha1.CIDRPool{ @@ -50,7 +47,7 @@ var _ = Describe("CIDRPool", func() { Spec: v1alpha1.CIDRPoolSpec{ CIDR: "192.168.0.0/16", PerNodeNetworkPrefix: 24, - GatewayIndex: getUintPtr(100), + GatewayIndex: ptr.To[int32](100), Exclusions: []v1alpha1.ExcludeRange{ {StartIP: "192.168.0.10", EndIP: "192.168.0.20"}, {StartIP: "192.168.0.25", EndIP: "192.168.0.25"}, @@ -78,7 +75,7 @@ var _ = Describe("CIDRPool", func() { Spec: v1alpha1.CIDRPoolSpec{ CIDR: "fdf8:6aef:d1fe::/48", PerNodeNetworkPrefix: 120, - GatewayIndex: getUintPtr(5), + GatewayIndex: ptr.To[int32](5), Exclusions: []v1alpha1.ExcludeRange{ {StartIP: "fdf8:6aef:d1fe::5", EndIP: "fdf8:6aef:d1fe::5"}, }, @@ -98,7 +95,7 @@ var _ = Describe("CIDRPool", func() { validatePoolAndCheckErr(&cidrPool, true) }) DescribeTable("CIDR", - func(cidr string, prefix uint, isValid bool) { + func(cidr string, prefix int32, isValid bool) { cidrPool := v1alpha1.CIDRPool{ ObjectMeta: metav1.ObjectMeta{Name: "test"}, Spec: v1alpha1.CIDRPoolSpec{ @@ -107,15 +104,15 @@ var _ = Describe("CIDRPool", func() { }} validatePoolAndCheckErr(&cidrPool, isValid, ContainSubstring("spec.cidr")) }, - Entry("empty", "", uint(30), false), - Entry("invalid value", "aaaa", uint(30), false), - Entry("/32", "192.168.1.1/32", uint(32), false), - Entry("/128", "2001:db8:3333:4444::0/128", uint(128), false), - Entry("valid ipv4", "192.168.1.0/24", uint(30), true), - Entry("valid ipv6", "2001:db8:3333:4444::0/64", uint(120), true), + Entry("empty", "", int32(30), false), + Entry("invalid value", "aaaa", int32(30), false), + Entry("/32", "192.168.1.1/32", int32(32), false), + Entry("/128", "2001:db8:3333:4444::0/128", int32(128), false), + Entry("valid ipv4", "192.168.1.0/24", int32(30), true), + Entry("valid ipv6", "2001:db8:3333:4444::0/64", int32(120), true), ) DescribeTable("PerNodeNetworkPrefix", - func(cidr string, prefix uint, isValid bool) { + func(cidr string, prefix int32, isValid bool) { cidrPool := v1alpha1.CIDRPool{ ObjectMeta: metav1.ObjectMeta{Name: "test"}, Spec: v1alpha1.CIDRPoolSpec{ @@ -124,12 +121,13 @@ var _ = Describe("CIDRPool", func() { }} validatePoolAndCheckErr(&cidrPool, isValid, ContainSubstring("spec.perNodeNetworkPrefix")) }, - Entry("not set", "192.168.0.0/16", uint(0), false), - Entry("larger than CIDR", "192.168.0.0/16", uint(8), false), - Entry("smaller than 31 for IPv4 pool", "192.168.0.0/16", uint(32), false), - Entry("smaller than 127 for IPv6 pool", "2001:db8:3333:4444::0/64", uint(128), false), - Entry("match CIDR prefix size - ipv4", "192.168.0.0/16", uint(16), true), - Entry("match CIDR prefix size - ipv6", "2001:db8:3333:4444::0/64", uint(64), true), + Entry("not set", "192.168.0.0/16", int32(0), false), + Entry("negative", "192.168.0.0/16", int32(-10), false), + Entry("larger than CIDR", "192.168.0.0/16", int32(8), false), + Entry("smaller than 31 for IPv4 pool", "192.168.0.0/16", int32(32), false), + Entry("smaller than 127 for IPv6 pool", "2001:db8:3333:4444::0/64", int32(128), false), + Entry("match CIDR prefix size - ipv4", "192.168.0.0/16", int32(16), true), + Entry("match CIDR prefix size - ipv6", "2001:db8:3333:4444::0/64", int32(64), true), ) DescribeTable("NodeSelector", func(nodeSelector *corev1.NodeSelector, isValid bool, errMatcher ...gomegaTypes.GomegaMatcher) { @@ -156,7 +154,7 @@ var _ = Describe("CIDRPool", func() { }, false, ContainSubstring("spec.nodeSelectorTerms[0].matchExpressions[0].operator")), ) DescribeTable("GatewayIndex", - func(gatewayIndex uint, isValid bool, errMatcher ...gomegaTypes.GomegaMatcher) { + func(gatewayIndex int32, isValid bool, errMatcher ...gomegaTypes.GomegaMatcher) { cidrPool := v1alpha1.CIDRPool{ ObjectMeta: metav1.ObjectMeta{Name: "test"}, Spec: v1alpha1.CIDRPoolSpec{ @@ -167,9 +165,10 @@ var _ = Describe("CIDRPool", func() { } validatePoolAndCheckErr(&cidrPool, isValid, ContainSubstring("spec.gatewayIndex")) }, - Entry("too large", uint(255), false), - Entry("index 1 is valid for point to point", uint(1), true), - Entry("index 2 is valid for point to point", uint(2), true), + Entry("negative", int32(-10), false), + Entry("too large", int32(255), false), + Entry("index 1 is valid for point to point", int32(1), true), + Entry("index 2 is valid for point to point", int32(2), true), ) DescribeTable("Exclusions", func(exclusions []v1alpha1.ExcludeRange, isValid bool, errMatcher ...gomegaTypes.GomegaMatcher) { @@ -273,7 +272,7 @@ var _ = Describe("CIDRPoolAllocation", func() { Spec: v1alpha1.CIDRPoolSpec{ CIDR: "192.168.0.0/16", PerNodeNetworkPrefix: 24, - GatewayIndex: getUintPtr(100), + GatewayIndex: ptr.To[int32](100), StaticAllocations: []v1alpha1.CIDRPoolStaticAllocation{ {NodeName: "node1", Prefix: "192.168.1.0/24", Gateway: "192.168.1.10"}, {NodeName: "node2", Prefix: "192.168.2.0/24"}, diff --git a/api/v1alpha1/cidrpool_type.go b/api/v1alpha1/cidrpool_type.go index 53c7557..84abebc 100644 --- a/api/v1alpha1/cidrpool_type.go +++ b/api/v1alpha1/cidrpool_type.go @@ -38,10 +38,10 @@ type CIDRPoolSpec struct { // and distributed between matching nodes CIDR string `json:"cidr"` // use IP with this index from the host prefix as a gateway, skip gateway configuration if the value not set - GatewayIndex *uint `json:"gatewayIndex,omitempty"` + GatewayIndex *int32 `json:"gatewayIndex,omitempty"` // size of the network prefix for each host, the network defined in "cidr" field will be split to multiple networks // with this size. - PerNodeNetworkPrefix uint `json:"perNodeNetworkPrefix"` + PerNodeNetworkPrefix int32 `json:"perNodeNetworkPrefix"` // contains reserved IP addresses that should not be allocated by nv-ipam Exclusions []ExcludeRange `json:"exclusions,omitempty"` // static allocations for the pool diff --git a/api/v1alpha1/cidrpool_validate.go b/api/v1alpha1/cidrpool_validate.go index 4e9348f..27143c4 100644 --- a/api/v1alpha1/cidrpool_validate.go +++ b/api/v1alpha1/cidrpool_validate.go @@ -51,9 +51,20 @@ func (r *CIDRPool) validateCIDR() field.ErrorList { return field.ErrorList{field.Invalid( field.NewPath("spec", "cidr"), r.Spec.CIDR, "single IP prefixes are not supported")} } + + if r.Spec.GatewayIndex != nil && *r.Spec.GatewayIndex < 0 { + return field.ErrorList{field.Invalid( + field.NewPath("spec", "gatewayIndex"), r.Spec.GatewayIndex, "must not be negative")} + } + + if r.Spec.PerNodeNetworkPrefix < 0 { + return field.ErrorList{field.Invalid( + field.NewPath("spec", "perNodeNetworkPrefix"), r.Spec.PerNodeNetworkPrefix, "must not be negative")} + } + if r.Spec.PerNodeNetworkPrefix == 0 || - r.Spec.PerNodeNetworkPrefix >= uint(bitsTotal) || - r.Spec.PerNodeNetworkPrefix < uint(setBits) { + r.Spec.PerNodeNetworkPrefix >= int32(bitsTotal) || + r.Spec.PerNodeNetworkPrefix < int32(setBits) { return field.ErrorList{field.Invalid( field.NewPath("spec", "perNodeNetworkPrefix"), r.Spec.PerNodeNetworkPrefix, "must be less or equal than network prefix size in the \"cidr\" field")} diff --git a/api/v1alpha1/helpers.go b/api/v1alpha1/helpers.go index 86b65eb..43fa3bd 100644 --- a/api/v1alpha1/helpers.go +++ b/api/v1alpha1/helpers.go @@ -27,7 +27,7 @@ import ( // - subnet 192.168.0.0/24, index 10, gateway = 102.168.1.10 // - subnet 192.168.1.2/31, index 0, gateway = 192.168.1.2 (point to point network can use network IP as a gateway) // - subnet 192.168.33.0/24, index 900, gateway = "" (invalid config, index too large) -func GetGatewayForSubnet(subnet *net.IPNet, index uint) string { +func GetGatewayForSubnet(subnet *net.IPNet, index int32) string { setBits, bitsTotal := subnet.Mask.Size() maxIndex := GetPossibleIPCount(subnet) diff --git a/api/v1alpha1/helpers_test.go b/api/v1alpha1/helpers_test.go index a28df9a..5a6c4dc 100644 --- a/api/v1alpha1/helpers_test.go +++ b/api/v1alpha1/helpers_test.go @@ -27,7 +27,7 @@ var _ = Describe("Helpers", func() { func(subnet string, index int, gw string) { _, network, err := net.ParseCIDR(subnet) Expect(err).NotTo(HaveOccurred()) - ret := v1alpha1.GetGatewayForSubnet(network, uint(index)) + ret := v1alpha1.GetGatewayForSubnet(network, int32(index)) Expect(ret).To(Equal(gw)) }, Entry("ipv4 - start range", "192.168.1.0/24", 1, "192.168.1.1"), diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 3927558..1e15c94 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -116,7 +116,7 @@ func (in *CIDRPoolSpec) DeepCopyInto(out *CIDRPoolSpec) { *out = *in if in.GatewayIndex != nil { in, out := &in.GatewayIndex, &out.GatewayIndex - *out = new(uint) + *out = new(int32) **out = **in } if in.Exclusions != nil { diff --git a/cmd/ipam-controller/app/app_test.go b/cmd/ipam-controller/app/app_test.go index 6237cf9..96a6cb5 100644 --- a/cmd/ipam-controller/app/app_test.go +++ b/cmd/ipam-controller/app/app_test.go @@ -27,6 +27,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/klog/v2" + "k8s.io/utils/ptr" ipamv1alpha1 "github.com/Mellanox/nvidia-k8s-ipam/api/v1alpha1" "github.com/Mellanox/nvidia-k8s-ipam/cmd/ipam-controller/app" @@ -306,7 +307,6 @@ var _ = Describe("App", func() { It("CIDRPool Basic tests", func(ctx SpecContext) { By("Create valid pools") - pool1gatewayIndex := uint(1) pool1 := &ipamv1alpha1.CIDRPool{ ObjectMeta: metav1.ObjectMeta{ Name: pool1Name, @@ -314,7 +314,7 @@ var _ = Describe("App", func() { }, Spec: ipamv1alpha1.CIDRPoolSpec{ CIDR: "10.10.0.0/16", - GatewayIndex: &pool1gatewayIndex, + GatewayIndex: ptr.To[int32](1), PerNodeNetworkPrefix: 24, Exclusions: []ipamv1alpha1.ExcludeRange{ {StartIP: "10.10.1.22", EndIP: "10.10.1.30"}, @@ -329,7 +329,6 @@ var _ = Describe("App", func() { } Expect(k8sClient.Create(ctx, pool1)).NotTo(HaveOccurred()) - pool2gatewayIndex := uint(1) pool2 := &ipamv1alpha1.CIDRPool{ ObjectMeta: metav1.ObjectMeta{ Name: pool2Name, @@ -337,7 +336,7 @@ var _ = Describe("App", func() { }, Spec: ipamv1alpha1.CIDRPoolSpec{ CIDR: "192.168.0.0/16", - GatewayIndex: &pool2gatewayIndex, + GatewayIndex: ptr.To[int32](1), PerNodeNetworkPrefix: 31, }, } @@ -415,7 +414,6 @@ var _ = Describe("App", func() { By("Create Pool3, with selector which ignores all nodes") // ranges for two nodes only can be allocated - pool3gatewayIndex := uint(1) pool3 := &ipamv1alpha1.CIDRPool{ ObjectMeta: metav1.ObjectMeta{ Name: pool3Name, @@ -423,7 +421,7 @@ var _ = Describe("App", func() { }, Spec: ipamv1alpha1.CIDRPoolSpec{ CIDR: "10.10.0.0/24", - GatewayIndex: &pool3gatewayIndex, + GatewayIndex: ptr.To[int32](1), PerNodeNetworkPrefix: 25, NodeSelector: &corev1.NodeSelector{ NodeSelectorTerms: []corev1.NodeSelectorTerm{ diff --git a/cmd/ipam-node/app/app_test.go b/cmd/ipam-node/app/app_test.go index 50ecadc..490086e 100644 --- a/cmd/ipam-node/app/app_test.go +++ b/cmd/ipam-node/app/app_test.go @@ -28,6 +28,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/klog/v2" + "k8s.io/utils/ptr" nodev1 "github.com/Mellanox/nvidia-k8s-ipam/api/grpc/nvidia/ipam/node/v1" ipamv1alpha1 "github.com/Mellanox/nvidia-k8s-ipam/api/v1alpha1" @@ -84,12 +85,11 @@ func createTestIPPools() { } func createTestCIDRPools() { - pool1GatewayIndex := uint(1) pool1 := &ipamv1alpha1.CIDRPool{ ObjectMeta: metav1.ObjectMeta{Name: testPoolName1, Namespace: testNamespace}, Spec: ipamv1alpha1.CIDRPoolSpec{ CIDR: "192.100.0.0/16", - GatewayIndex: &pool1GatewayIndex, + GatewayIndex: ptr.To[int32](1), PerNodeNetworkPrefix: 24, Exclusions: []ipamv1alpha1.ExcludeRange{ {StartIP: "192.100.0.1", EndIP: "192.100.0.10"}, diff --git a/deploy/crds/nv-ipam.nvidia.com_cidrpools.yaml b/deploy/crds/nv-ipam.nvidia.com_cidrpools.yaml index 665b068..5cdd79d 100644 --- a/deploy/crds/nv-ipam.nvidia.com_cidrpools.yaml +++ b/deploy/crds/nv-ipam.nvidia.com_cidrpools.yaml @@ -68,6 +68,7 @@ spec: gatewayIndex: description: use IP with this index from the host prefix as a gateway, skip gateway configuration if the value not set + format: int32 type: integer nodeSelector: description: selector for nodes, if empty match all nodes @@ -157,6 +158,7 @@ spec: description: size of the network prefix for each host, the network defined in "cidr" field will be split to multiple networks with this size. + format: int32 type: integer staticAllocations: description: static allocations for the pool diff --git a/pkg/ip/cidr.go b/pkg/ip/cidr.go index 8bbbf59..1da054c 100644 --- a/pkg/ip/cidr.go +++ b/pkg/ip/cidr.go @@ -178,9 +178,9 @@ func LastIP(network *net.IPNet) net.IP { // println(gen().String()) // 192.168.1.0/25 // println(gen().String()) // 192.168.1.128/25 // println(gen().String()) // - no more ranges available -func GetSubnetGen(network *net.IPNet, prefixSize uint) func() *net.IPNet { +func GetSubnetGen(network *net.IPNet, prefixSize int32) func() *net.IPNet { networkOnes, netBitsTotal := network.Mask.Size() - if prefixSize < uint(networkOnes) || prefixSize > uint(netBitsTotal) { + if prefixSize < int32(networkOnes) || prefixSize > int32(netBitsTotal) { return func() *net.IPNet { return nil } } isIPv6 := false