From 38418f256ff2aff7a4fac14accc4f4b08906cdc4 Mon Sep 17 00:00:00 2001 From: Khaja Omer Date: Wed, 6 May 2026 13:15:38 -0500 Subject: [PATCH 1/3] [improvement] Reserve final PodCIDR block in node IPAM --- cloud/nodeipam/ipam/cloud_allocator.go | 10 ++++- cloud/nodeipam/ipam/cloud_allocator_test.go | 46 +++++++++++++++++++++ 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/cloud/nodeipam/ipam/cloud_allocator.go b/cloud/nodeipam/ipam/cloud_allocator.go index 2d556767..5a460116 100644 --- a/cloud/nodeipam/ipam/cloud_allocator.go +++ b/cloud/nodeipam/ipam/cloud_allocator.go @@ -101,6 +101,15 @@ func NewLinodeCIDRAllocator(ctx context.Context, linodeClient linode.Client, cli return nil, err } + // Reserve the last block in the cluster range by occupying its last IP. + lastIP, err := netutils.GetIndexedIP(allocatorParams.ClusterCIDRs[0], int(netutils.RangeSize(allocatorParams.ClusterCIDRs[0]))-1) + if err != nil { + return nil, err + } + if err := cidrSet.Occupy(&net.IPNet{IP: lastIP.To4(), Mask: net.CIDRMask(32, 32)}); err != nil { + return nil, err + } + ca := &cloudAllocator{ client: client, linodeClient: linodeClient, @@ -184,7 +193,6 @@ func NewLinodeCIDRAllocator(ctx context.Context, linodeClient linode.Client, cli return ca, nil } - func (c *cloudAllocator) Run(ctx context.Context) { defer utilruntime.HandleCrash() diff --git a/cloud/nodeipam/ipam/cloud_allocator_test.go b/cloud/nodeipam/ipam/cloud_allocator_test.go index 083cdc3c..6974f3ea 100644 --- a/cloud/nodeipam/ipam/cloud_allocator_test.go +++ b/cloud/nodeipam/ipam/cloud_allocator_test.go @@ -17,6 +17,7 @@ limitations under the License. package ipam import ( + "errors" "fmt" "net" "testing" @@ -28,6 +29,7 @@ import ( v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes/fake" + "k8s.io/kubernetes/pkg/controller/nodeipam/ipam/cidrset" "k8s.io/kubernetes/pkg/controller/nodeipam/ipam/test" "k8s.io/kubernetes/pkg/controller/testutil" "k8s.io/kubernetes/test/utils/ktesting" @@ -96,6 +98,50 @@ func TestComputeStableIPv6PodCIDR(t *testing.T) { } } +func TestNewLinodeCIDRAllocatorReservesFinalCIDR(t *testing.T) { + _, clusterCIDR, err := netutils.ParseCIDRSloppy("10.0.0.0/28") + if err != nil { + t.Fatalf("parse cluster cidr: %v", err) + } + + _, tCtx := ktesting.NewTestContext(t) + fakeNodeHandler := &testutil.FakeNodeHandler{Clientset: fake.NewClientset()} + allocatorParams := CIDRAllocatorParams{ + ClusterCIDRs: []*net.IPNet{clusterCIDR}, + ServiceCIDR: nil, + NodeCIDRMaskSizes: []int{30, 112}, + } + + allocator, err := NewLinodeCIDRAllocator(tCtx, nil, fakeNodeHandler, test.FakeNodeInformer(fakeNodeHandler), allocatorParams, nil) + if err != nil { + t.Fatalf("failed to create allocator: %v", err) + } + + cloudAllocator, ok := allocator.(*cloudAllocator) + if !ok { + t.Fatalf("found non-default implementation of CIDRAllocator") + } + + reserved := "10.0.0.12/30" + + for i := 0; i < 3; i++ { + cidr, err := cloudAllocator.cidrSet.AllocateNext() + if err != nil { + t.Fatalf("unexpected error allocating cidr %d: %v", i, err) + } + if cidr.String() != fmt.Sprintf("10.0.0.%d/30", i*4) { + t.Fatalf("unexpected cidr %d: got %s", i, cidr.String()) + } + if cidr.String() == reserved { + t.Fatalf("allocated reserved cidr %s", cidr.String()) + } + } + + if _, err := cloudAllocator.cidrSet.AllocateNext(); !errors.Is(err, cidrset.ErrCIDRRangeNoCIDRsRemaining) { + t.Fatalf("expected cidr exhaustion after reserving final block, got %v", err) + } +} + func TestGetIPv6RangeFromLinodeInterface(t *testing.T) { for _, tc := range []struct { iface linodego.LinodeInterface From 8816f7ae4636907a1b8d52917117f8a8bd17c38d Mon Sep 17 00:00:00 2001 From: Khaja Omer Date: Fri, 8 May 2026 12:32:39 -0500 Subject: [PATCH 2/3] [improvement] Dynamically reserve the final PodCIDR block Compare the cluster CIDR end IP with the selected Linode subnet end IP so node IPAM only skips the final PodCIDR block when the Linode VPC subnet actually overlaps that boundary. --- cloud/nodeipam/ipam/cloud_allocator.go | 55 ++++++- cloud/nodeipam/ipam/cloud_allocator_test.go | 166 ++++++++++++++++---- 2 files changed, 189 insertions(+), 32 deletions(-) diff --git a/cloud/nodeipam/ipam/cloud_allocator.go b/cloud/nodeipam/ipam/cloud_allocator.go index 5a460116..a65712ba 100644 --- a/cloud/nodeipam/ipam/cloud_allocator.go +++ b/cloud/nodeipam/ipam/cloud_allocator.go @@ -45,6 +45,8 @@ import ( netutils "k8s.io/utils/net" linode "github.com/linode/linode-cloud-controller-manager/cloud/linode/client" + "github.com/linode/linode-cloud-controller-manager/cloud/linode/options" + "github.com/linode/linode-cloud-controller-manager/cloud/linode/services" ) type cloudAllocator struct { @@ -101,13 +103,19 @@ func NewLinodeCIDRAllocator(ctx context.Context, linodeClient linode.Client, cli return nil, err } - // Reserve the last block in the cluster range by occupying its last IP. - lastIP, err := netutils.GetIndexedIP(allocatorParams.ClusterCIDRs[0], int(netutils.RangeSize(allocatorParams.ClusterCIDRs[0]))-1) + reserveFinalIPv4Block, err := shouldReserveFinalIPv4Block(ctx, linodeClient, allocatorParams.ClusterCIDRs[0]) if err != nil { return nil, err } - if err := cidrSet.Occupy(&net.IPNet{IP: lastIP.To4(), Mask: net.CIDRMask(32, 32)}); err != nil { - return nil, err + if reserveFinalIPv4Block { + // Reserve the last block in the cluster range by occupying its last IP. + lastIP, err := lastIPForCIDR(allocatorParams.ClusterCIDRs[0]) + if err != nil { + return nil, err + } + if err := cidrSet.Occupy(&net.IPNet{IP: lastIP.To4(), Mask: net.CIDRMask(32, 32)}); err != nil { + return nil, err + } } ca := &cloudAllocator{ @@ -193,6 +201,45 @@ func NewLinodeCIDRAllocator(ctx context.Context, linodeClient linode.Client, cli return ca, nil } + +func shouldReserveFinalIPv4Block(ctx context.Context, client linode.Client, clusterCIDR *net.IPNet) (bool, error) { + if clusterCIDR == nil || len(options.Options.VPCNames) == 0 || len(options.Options.SubnetNames) == 0 { + return false, nil + } + + vpcID, err := services.GetVPCID(ctx, client, options.Options.VPCNames[0]) + if err != nil { + return false, err + } + subnetID, err := services.GetSubnetID(ctx, client, vpcID, options.Options.SubnetNames[0]) + if err != nil { + return false, err + } + subnet, err := client.GetVPCSubnet(ctx, vpcID, subnetID) + if err != nil { + return false, err + } + _, subnetCIDR, err := net.ParseCIDR(subnet.IPv4) + if err != nil { + return false, fmt.Errorf("failed to parse subnet cidr %s: %w", subnet.IPv4, err) + } + + clusterLastIP, err := lastIPForCIDR(clusterCIDR) + if err != nil { + return false, err + } + subnetLastIP, err := lastIPForCIDR(subnetCIDR) + if err != nil { + return false, err + } + + return clusterLastIP.Equal(subnetLastIP), nil +} + +func lastIPForCIDR(cidr *net.IPNet) (net.IP, error) { + return netutils.GetIndexedIP(cidr, int(netutils.RangeSize(cidr))-1) +} + func (c *cloudAllocator) Run(ctx context.Context) { defer utilruntime.HandleCrash() diff --git a/cloud/nodeipam/ipam/cloud_allocator_test.go b/cloud/nodeipam/ipam/cloud_allocator_test.go index 6974f3ea..39a4de19 100644 --- a/cloud/nodeipam/ipam/cloud_allocator_test.go +++ b/cloud/nodeipam/ipam/cloud_allocator_test.go @@ -17,6 +17,7 @@ limitations under the License. package ipam import ( + "context" "errors" "fmt" "net" @@ -37,6 +38,8 @@ import ( "k8s.io/utils/ptr" "github.com/linode/linode-cloud-controller-manager/cloud/linode/client/mocks" + "github.com/linode/linode-cloud-controller-manager/cloud/linode/options" + "github.com/linode/linode-cloud-controller-manager/cloud/linode/services" ) type testCase struct { @@ -98,48 +101,155 @@ func TestComputeStableIPv6PodCIDR(t *testing.T) { } } -func TestNewLinodeCIDRAllocatorReservesFinalCIDR(t *testing.T) { +func TestNewLinodeCIDRAllocatorFinalCIDRReservation(t *testing.T) { _, clusterCIDR, err := netutils.ParseCIDRSloppy("10.0.0.0/28") if err != nil { t.Fatalf("parse cluster cidr: %v", err) } - _, tCtx := ktesting.NewTestContext(t) - fakeNodeHandler := &testutil.FakeNodeHandler{Clientset: fake.NewClientset()} - allocatorParams := CIDRAllocatorParams{ - ClusterCIDRs: []*net.IPNet{clusterCIDR}, - ServiceCIDR: nil, - NodeCIDRMaskSizes: []int{30, 112}, + for _, tc := range []struct { + name string + vpcNames []string + subnetNames []string + subnetCIDR string + wantCIDRs []string + }{ + { + name: "reserve final cidr when subnet end matches", + vpcNames: []string{"test-vpc"}, + subnetNames: []string{"default"}, + subnetCIDR: "10.0.0.0/28", + wantCIDRs: []string{"10.0.0.0/30", "10.0.0.4/30", "10.0.0.8/30"}, + }, + { + name: "do not reserve final cidr without subnet target", + wantCIDRs: []string{"10.0.0.0/30", "10.0.0.4/30", "10.0.0.8/30", "10.0.0.12/30"}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + setNodeIPAMSubnetOptionsForTest(t, tc.vpcNames, tc.subnetNames) + + var client *mocks.MockClient + if tc.subnetCIDR != "" { + ctrl := gomock.NewController(t) + client = mocks.NewMockClient(ctrl) + expectNodeIPAMSubnetLookup(client, tc.subnetCIDR) + } + + _, tCtx := ktesting.NewTestContext(t) + fakeNodeHandler := &testutil.FakeNodeHandler{Clientset: fake.NewClientset()} + allocatorParams := CIDRAllocatorParams{ + ClusterCIDRs: []*net.IPNet{clusterCIDR}, + ServiceCIDR: nil, + NodeCIDRMaskSizes: []int{30, 112}, + } + + allocator, err := NewLinodeCIDRAllocator(tCtx, client, fakeNodeHandler, test.FakeNodeInformer(fakeNodeHandler), allocatorParams, nil) + if err != nil { + t.Fatalf("failed to create allocator: %v", err) + } + + cloudAllocator, ok := allocator.(*cloudAllocator) + if !ok { + t.Fatalf("found non-default implementation of CIDRAllocator") + } + + for i, wantCIDR := range tc.wantCIDRs { + cidr, err := cloudAllocator.cidrSet.AllocateNext() + if err != nil { + t.Fatalf("unexpected error allocating cidr %d: %v", i, err) + } + if cidr.String() != wantCIDR { + t.Fatalf("unexpected cidr %d: got %s want %s", i, cidr.String(), wantCIDR) + } + } + + if _, err := cloudAllocator.cidrSet.AllocateNext(); !errors.Is(err, cidrset.ErrCIDRRangeNoCIDRsRemaining) { + t.Fatalf("expected cidr exhaustion after allocations, got %v", err) + } + }) } +} - allocator, err := NewLinodeCIDRAllocator(tCtx, nil, fakeNodeHandler, test.FakeNodeInformer(fakeNodeHandler), allocatorParams, nil) +func TestShouldReserveFinalIPv4Block(t *testing.T) { + _, clusterCIDR, err := net.ParseCIDR("10.0.4.0/22") if err != nil { - t.Fatalf("failed to create allocator: %v", err) + t.Fatalf("parse cluster cidr: %v", err) } - cloudAllocator, ok := allocator.(*cloudAllocator) - if !ok { - t.Fatalf("found non-default implementation of CIDRAllocator") - } + for _, tc := range []struct { + name string + vpcNames []string + subnetNames []string + subnetCIDR string + want bool + }{ + { + name: "cluster and subnet share end ip so reserve", + vpcNames: []string{"test-vpc"}, + subnetNames: []string{"default"}, + subnetCIDR: "10.0.0.0/21", + want: true, + }, + { + name: "cluster and subnet end ip differ so no reserve", + vpcNames: []string{"test-vpc"}, + subnetNames: []string{"default"}, + subnetCIDR: "10.0.0.0/20", + want: false, + }, + { + name: "missing subnet target does not reserve", + vpcNames: []string{"test-vpc"}, + want: false, + }, + } { + t.Run(tc.name, func(t *testing.T) { + setNodeIPAMSubnetOptionsForTest(t, tc.vpcNames, tc.subnetNames) - reserved := "10.0.0.12/30" + var client *mocks.MockClient + if tc.subnetCIDR != "" { + ctrl := gomock.NewController(t) + client = mocks.NewMockClient(ctrl) + expectNodeIPAMSubnetLookup(client, tc.subnetCIDR) + } - for i := 0; i < 3; i++ { - cidr, err := cloudAllocator.cidrSet.AllocateNext() - if err != nil { - t.Fatalf("unexpected error allocating cidr %d: %v", i, err) - } - if cidr.String() != fmt.Sprintf("10.0.0.%d/30", i*4) { - t.Fatalf("unexpected cidr %d: got %s", i, cidr.String()) - } - if cidr.String() == reserved { - t.Fatalf("allocated reserved cidr %s", cidr.String()) - } + reserve, err := shouldReserveFinalIPv4Block(context.Background(), client, clusterCIDR) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if reserve != tc.want { + t.Fatalf("unexpected reserve result: got %t want %t", reserve, tc.want) + } + }) } +} - if _, err := cloudAllocator.cidrSet.AllocateNext(); !errors.Is(err, cidrset.ErrCIDRRangeNoCIDRsRemaining) { - t.Fatalf("expected cidr exhaustion after reserving final block, got %v", err) - } +func expectNodeIPAMSubnetLookup(client *mocks.MockClient, subnetCIDR string) { + client.EXPECT().ListVPCs(gomock.Any(), gomock.Any()).Return([]linodego.VPC{{ID: 1, Label: "test-vpc"}}, nil) + client.EXPECT().ListVPCSubnets(gomock.Any(), 1, gomock.Any()).Return([]linodego.VPCSubnet{{ID: 2, Label: "default"}}, nil) + client.EXPECT().GetVPCSubnet(gomock.Any(), 1, 2).Return(&linodego.VPCSubnet{ID: 2, Label: "default", IPv4: subnetCIDR}, nil) +} + +func setNodeIPAMSubnetOptionsForTest(t *testing.T, vpcNames, subnetNames []string) { + t.Helper() + + prevVPCNames := options.Options.VPCNames + prevSubnetNames := options.Options.SubnetNames + prevVpcIDs := services.VpcIDs + prevSubnetIDs := services.SubnetIDs + + options.Options.VPCNames = vpcNames + options.Options.SubnetNames = subnetNames + services.VpcIDs = map[string]int{} + services.SubnetIDs = map[string]int{} + + t.Cleanup(func() { + options.Options.VPCNames = prevVPCNames + options.Options.SubnetNames = prevSubnetNames + services.VpcIDs = prevVpcIDs + services.SubnetIDs = prevSubnetIDs + }) } func TestGetIPv6RangeFromLinodeInterface(t *testing.T) { From f56e373316d07e6a184b6d12a86bfaab6773f023 Mon Sep 17 00:00:00 2001 From: Khaja Omer Date: Fri, 8 May 2026 12:52:01 -0500 Subject: [PATCH 3/3] add clarification comment --- cloud/nodeipam/ipam/cloud_allocator.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cloud/nodeipam/ipam/cloud_allocator.go b/cloud/nodeipam/ipam/cloud_allocator.go index a65712ba..1b88a9a4 100644 --- a/cloud/nodeipam/ipam/cloud_allocator.go +++ b/cloud/nodeipam/ipam/cloud_allocator.go @@ -103,6 +103,9 @@ func NewLinodeCIDRAllocator(ctx context.Context, linodeClient linode.Client, cli return nil, err } + // Using Linode API, check if we need to reserve the final block in the cluster CIDR. + // Reserve when cluster CIDR last IP is the same as the VPC subnet last IP. + // We cannot reserve that block since the last IP is a reserved IP for VPC functionality. reserveFinalIPv4Block, err := shouldReserveFinalIPv4Block(ctx, linodeClient, allocatorParams.ClusterCIDRs[0]) if err != nil { return nil, err