From fef58cc562ad065c7f3569120b135df37f527c45 Mon Sep 17 00:00:00 2001 From: Philip Lombardi <893096+plombardi89@users.noreply.github.com> Date: Wed, 26 Aug 2026 16:38:10 -0400 Subject: [PATCH] net: delete the superseded node-CIDR controller internal/net/controller/controller.go is a 542-line Kubernetes node controller that allocates pod CIDRs per node. Nothing constructs it. Its package is imported by cmd/unbounded-net-controller, which is what kept it out of every previous sweep, but the symbols that import reaches are NewSiteController, NewGatewayPoolController, NewPeeringAggregationController, NewManagedKubeProxyController, ManagedKubeProxyOptions, NodeSiteLabel, TunnelMTUAnnotation and WireGuardPubKeyAnnotation. Not Controller. Without -test, `make deadcode` reports every one of its methods unreachable: InformerSynced, NewController, enqueueNode, matchesNodeFilter, Run, runWorker, processNextWorkItem, syncHandler, allocateCIDRsForNode, patchNodeCIDRs, InitializeAllocator and DryRun. With -test, four remain unreachable, and the only thing holding the rest up is controller_helpers_test.go, whose five tests exist for no other purpose. Site-based pod CIDR assignment superseded this; the live path is SiteController.buildAssignmentAllocator. The file and its test go together. Keeping the tests would have kept twelve dead methods alive and moved them from one section of the report to the other. This is the one deletion in the series where intent is inferred from the call graph rather than read from a document. If there is a plan to revive this controller, this is the commit to push back on. The cascade: - allocator.ContainsCIDR had no caller at all. - allocator.ParseCIDRs had no caller in production. Both live callers of NewAllocator - site_controller.go and webhook/validation.go - build their pools with splitCIDRBlocks. It was exported API kept alive entirely by allocator_test.go, so it moves into that file as an unexported helper and the tests are unchanged apart from the name. - computeReachableRoutes was a two-line wrapper around computeReachable called only from peering_aggregation_controller_test.go. The wrapper goes and the test calls computeReachable directly, which is what it was actually exercising. - SiteController.GetSiteForNode had no reference anywhere. The three unobserved promauto metric vars in controller/metrics.go are deliberately left alone; they are tracked in #672. Refs #670 --- internal/net/allocator/allocator.go | 46 -- internal/net/allocator/allocator_test.go | 55 +- internal/net/controller/controller.go | 542 ------------------ .../net/controller/controller_helpers_test.go | 120 ---- .../peering_aggregation_controller.go | 5 - .../peering_aggregation_controller_test.go | 4 +- internal/net/controller/site_controller.go | 9 - 7 files changed, 42 insertions(+), 739 deletions(-) delete mode 100644 internal/net/controller/controller.go delete mode 100644 internal/net/controller/controller_helpers_test.go diff --git a/internal/net/allocator/allocator.go b/internal/net/allocator/allocator.go index 2348660cd..4a434a128 100644 --- a/internal/net/allocator/allocator.go +++ b/internal/net/allocator/allocator.go @@ -146,29 +146,6 @@ func (a *Allocator) IsAllocated(cidr string) bool { return allocated } -// ContainsCIDR returns true if the given CIDR falls within any of the allocator's pools. -func (a *Allocator) ContainsCIDR(cidr string) bool { - _, ipNet, err := net.ParseCIDR(cidr) - if err != nil { - return false - } - - ip := ipNet.IP - for _, pool := range a.ipv4Pools { - if pool.Contains(ip) { - return true - } - } - - for _, pool := range a.ipv6Pools { - if pool.Contains(ip) { - return true - } - } - - return false -} - // AllocateIPv4 allocates the next available IPv4 CIDR from the pools. // Returns ErrPoolExhausted if no CIDRs are available. func (a *Allocator) AllocateIPv4() (string, error) { @@ -406,26 +383,3 @@ func (a *Allocator) DebugState() AllocatorDebugState { return state } - -// ParseCIDRs parses a slice of CIDR strings into net.IPNet objects. -func ParseCIDRs(cidrs []string) ([]*net.IPNet, error) { - klog.V(3).Infof("ParseCIDRs: parsing %d CIDR strings", len(cidrs)) - - result := make([]*net.IPNet, 0, len(cidrs)) - for i, cidr := range cidrs { - klog.V(4).Infof("ParseCIDRs: parsing CIDR[%d]: %q", i, cidr) - - _, ipNet, err := net.ParseCIDR(cidr) - if err != nil { - klog.Errorf("ParseCIDRs: failed to parse CIDR[%d] %q: %v", i, cidr, err) - return nil, fmt.Errorf("invalid CIDR %q: %w", cidr, err) - } - - klog.V(4).Infof("ParseCIDRs: successfully parsed CIDR[%d]: %s", i, ipNet.String()) - result = append(result, ipNet) - } - - klog.V(3).Infof("ParseCIDRs: successfully parsed %d CIDRs", len(result)) - - return result, nil -} diff --git a/internal/net/allocator/allocator_test.go b/internal/net/allocator/allocator_test.go index 090707b59..5d91c701b 100644 --- a/internal/net/allocator/allocator_test.go +++ b/internal/net/allocator/allocator_test.go @@ -3,7 +3,32 @@ package allocator -import "testing" +import ( + "fmt" + "net" + "testing" +) + +// parseCIDRs turns CIDR strings into the net.IPNet slices NewAllocator takes. +// +// It used to be an exported helper on the allocator itself. Nothing in +// production ever called it - the live callers in site_controller.go and the +// webhook build their pools with splitCIDRBlocks - so it moved here, where its +// only callers already were. +func parseCIDRs(cidrs []string) ([]*net.IPNet, error) { + result := make([]*net.IPNet, 0, len(cidrs)) + + for _, cidr := range cidrs { + _, ipNet, err := net.ParseCIDR(cidr) + if err != nil { + return nil, fmt.Errorf("invalid CIDR %q: %w", cidr, err) + } + + result = append(result, ipNet) + } + + return result, nil +} // TestNewAllocator tests new allocator. func TestNewAllocator(t *testing.T) { @@ -63,8 +88,8 @@ func TestNewAllocator(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ipv4Pools, _ := ParseCIDRs(tt.ipv4Pools) - ipv6Pools, _ := ParseCIDRs(tt.ipv6Pools) + ipv4Pools, _ := parseCIDRs(tt.ipv4Pools) + ipv6Pools, _ := parseCIDRs(tt.ipv6Pools) _, err := NewAllocator(ipv4Pools, ipv6Pools, tt.ipv4MaskSize, tt.ipv6MaskSize) if (err != nil) != tt.wantErr { @@ -77,7 +102,7 @@ func TestNewAllocator(t *testing.T) { // TestAllocateIPv4 tests allocate ipv4. func TestAllocateIPv4(t *testing.T) { // Test with /24 from /22 (4 possible /24s) - ipv4Pools, _ := ParseCIDRs([]string{"10.0.0.0/22"}) + ipv4Pools, _ := parseCIDRs([]string{"10.0.0.0/22"}) alloc, err := NewAllocator(ipv4Pools, nil, 24, 0) if err != nil { @@ -114,7 +139,7 @@ func TestAllocateIPv4(t *testing.T) { // TestAllocateIPv6 tests allocate ipv6. func TestAllocateIPv6(t *testing.T) { // Test with /64 from /62 (4 possible /64s) - ipv6Pools, _ := ParseCIDRs([]string{"fd00::/62"}) + ipv6Pools, _ := parseCIDRs([]string{"fd00::/62"}) alloc, err := NewAllocator(nil, ipv6Pools, 0, 64) if err != nil { @@ -150,7 +175,7 @@ func TestAllocateIPv6(t *testing.T) { // TestMarkAllocated tests mark allocated. func TestMarkAllocated(t *testing.T) { - ipv4Pools, _ := ParseCIDRs([]string{"10.0.0.0/22"}) + ipv4Pools, _ := parseCIDRs([]string{"10.0.0.0/22"}) alloc, err := NewAllocator(ipv4Pools, nil, 24, 0) if err != nil { @@ -175,7 +200,7 @@ func TestMarkAllocated(t *testing.T) { // TestMultiplePools tests multiple pools. func TestMultiplePools(t *testing.T) { // Test with multiple pools - ipv4Pools, _ := ParseCIDRs([]string{"10.0.0.0/24", "10.1.0.0/24"}) + ipv4Pools, _ := parseCIDRs([]string{"10.0.0.0/24", "10.1.0.0/24"}) alloc, err := NewAllocator(ipv4Pools, nil, 26, 0) if err != nil { @@ -224,7 +249,7 @@ func TestMultiplePools(t *testing.T) { } } -// TestParseCIDRs tests parse cidrs. +// TestParseCIDRs tests the local CIDR parsing helper. func TestParseCIDRs(t *testing.T) { tests := []struct { name string @@ -255,9 +280,9 @@ func TestParseCIDRs(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - _, err := ParseCIDRs(tt.cidrs) + _, err := parseCIDRs(tt.cidrs) if (err != nil) != tt.wantErr { - t.Errorf("ParseCIDRs() error = %v, wantErr %v", err, tt.wantErr) + t.Errorf("parseCIDRs() error = %v, wantErr %v", err, tt.wantErr) } }) } @@ -265,7 +290,7 @@ func TestParseCIDRs(t *testing.T) { // TestIsAllocated tests is allocated. func TestIsAllocated(t *testing.T) { - ipv4Pools, _ := ParseCIDRs([]string{"10.0.0.0/16"}) + ipv4Pools, _ := parseCIDRs([]string{"10.0.0.0/16"}) alloc, _ := NewAllocator(ipv4Pools, nil, 24, 0) cidr := "10.0.5.0/24" @@ -287,7 +312,7 @@ func TestIsAllocated(t *testing.T) { // TestHasPools tests has pools. func TestHasPools(t *testing.T) { t.Run("IPv4 only", func(t *testing.T) { - ipv4Pools, _ := ParseCIDRs([]string{"10.0.0.0/16"}) + ipv4Pools, _ := parseCIDRs([]string{"10.0.0.0/16"}) alloc, _ := NewAllocator(ipv4Pools, nil, 24, 0) if !alloc.HasIPv4Pools() { @@ -300,7 +325,7 @@ func TestHasPools(t *testing.T) { }) t.Run("IPv6 only", func(t *testing.T) { - ipv6Pools, _ := ParseCIDRs([]string{"fd00::/48"}) + ipv6Pools, _ := parseCIDRs([]string{"fd00::/48"}) alloc, _ := NewAllocator(nil, ipv6Pools, 0, 64) if alloc.HasIPv4Pools() { @@ -313,8 +338,8 @@ func TestHasPools(t *testing.T) { }) t.Run("dual-stack", func(t *testing.T) { - ipv4Pools, _ := ParseCIDRs([]string{"10.0.0.0/16"}) - ipv6Pools, _ := ParseCIDRs([]string{"fd00::/48"}) + ipv4Pools, _ := parseCIDRs([]string{"10.0.0.0/16"}) + ipv6Pools, _ := parseCIDRs([]string{"fd00::/48"}) alloc, _ := NewAllocator(ipv4Pools, ipv6Pools, 24, 64) if !alloc.HasIPv4Pools() { diff --git a/internal/net/controller/controller.go b/internal/net/controller/controller.go deleted file mode 100644 index 61006b0ed..000000000 --- a/internal/net/controller/controller.go +++ /dev/null @@ -1,542 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. - -// Package controller implements the Kubernetes node controller for CIDR allocation. -package controller - -import ( - "context" - "fmt" - "regexp" - "sync" - "time" - - corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" - "k8s.io/apimachinery/pkg/util/wait" - "k8s.io/client-go/informers" - "k8s.io/client-go/kubernetes" - corev1listers "k8s.io/client-go/listers/core/v1" - "k8s.io/client-go/tools/cache" - "k8s.io/client-go/tools/record" - "k8s.io/client-go/util/workqueue" - "k8s.io/klog/v2" - - "github.com/Azure/unbounded/internal/net/allocator" -) - -const ( - // maxRetries is the maximum number of retries for processing a node. - maxRetries = 5 -) - -// Controller manages CIDR allocation for Kubernetes nodes. -type Controller struct { - clientset kubernetes.Interface - nodeLister corev1listers.NodeLister - nodeSynced cache.InformerSynced - workqueue workqueue.TypedRateLimitingInterface[string] - allocator *allocator.Allocator - nodeRegex *regexp.Regexp - recorder record.EventRecorder - - // pendingReleases stores CIDRs that need to be released when a deleted - // node is processed via the workqueue, preventing a race with direct - // release in the delete event handler. - pendingReleases map[string][]string - pendingReleasesLock sync.Mutex -} - -// InformerSynced returns true if the informer cache has been synced. -func (c *Controller) InformerSynced() bool { - return c.nodeSynced() -} - -// NewController creates a new node CIDR controller. -// If nodeRegexPattern is non-empty, only nodes matching the regex will be processed. -func NewController( - clientset kubernetes.Interface, - informerFactory informers.SharedInformerFactory, - alloc *allocator.Allocator, - nodeRegexPattern string, - recorder record.EventRecorder, -) (*Controller, error) { - nodeInformer := informerFactory.Core().V1().Nodes() - - var ( - nodeRegex *regexp.Regexp - err error - ) - - if nodeRegexPattern != "" { - nodeRegex, err = regexp.Compile(nodeRegexPattern) - if err != nil { - return nil, fmt.Errorf("invalid node regex pattern %q: %w", nodeRegexPattern, err) - } - - klog.V(2).Infof("Node filter regex: %s", nodeRegexPattern) - } - - c := &Controller{ - clientset: clientset, - nodeLister: nodeInformer.Lister(), - nodeSynced: nodeInformer.Informer().HasSynced, - workqueue: workqueue.NewTypedRateLimitingQueueWithConfig(workqueue.DefaultTypedControllerRateLimiter[string](), workqueue.TypedRateLimitingQueueConfig[string]{Name: "Nodes"}), - allocator: alloc, - nodeRegex: nodeRegex, - recorder: recorder, - pendingReleases: make(map[string][]string), - } - - // Set up event handlers - if _, err := nodeInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ - AddFunc: func(obj interface{}) { - node := obj.(*corev1.Node) //nolint:errcheck - klog.Infof("Node added: %s", node.Name) - c.enqueueNode(obj) - }, - UpdateFunc: func(old, new interface{}) { - c.enqueueNode(new) - }, - DeleteFunc: func(obj interface{}) { - // Handle deleted nodes - defer CIDR release to the workqueue - var node *corev1.Node - - switch t := obj.(type) { - case *corev1.Node: - node = t - case cache.DeletedFinalStateUnknown: - var ok bool - - node, ok = t.Obj.(*corev1.Node) - if !ok { - klog.Errorf("DeletedFinalStateUnknown contained non-Node object: %#v", t.Obj) - return - } - default: - klog.Errorf("Delete event contained non-Node object: %#v", obj) - return - } - - klog.Infof("Node deleted: %s", node.Name) - - // Store CIDRs for deferred release via the workqueue - var cidrs []string - if node.Spec.PodCIDR != "" { - cidrs = append(cidrs, node.Spec.PodCIDR) - } - - cidrs = append(cidrs, node.Spec.PodCIDRs...) - if len(cidrs) > 0 { - c.pendingReleasesLock.Lock() - c.pendingReleases[node.Name] = cidrs - c.pendingReleasesLock.Unlock() - } - - c.enqueueNode(obj) - }, - }); err != nil { - return nil, fmt.Errorf("add node informer event handler: %w", err) - } - - return c, nil -} - -// enqueueNode adds a node to the workqueue. -func (c *Controller) enqueueNode(obj interface{}) { - var ( - key string - err error - ) - - if key, err = cache.MetaNamespaceKeyFunc(obj); err != nil { - klog.Errorf("Error getting key for object: %v", err) - return - } - - c.workqueue.Add(key) -} - -// matchesNodeFilter returns true if the node name matches the configured regex filter. -// If no filter is configured, all nodes match. -func (c *Controller) matchesNodeFilter(nodeName string) bool { - if c.nodeRegex == nil { - return true - } - - matches := c.nodeRegex.MatchString(nodeName) - if !matches { - klog.V(4).Infof("Node %s does not match filter regex, skipping", nodeName) - } - - return matches -} - -// Run starts the controller. -func (c *Controller) Run(ctx context.Context, workers int) error { - defer c.workqueue.ShutDown() - - klog.Info("Starting node CIDR controller") - - // Wait for caches to sync - klog.Info("Waiting for informer caches to sync") - - if ok := cache.WaitForCacheSync(ctx.Done(), c.nodeSynced); !ok { - return fmt.Errorf("failed to wait for caches to sync") - } - - klog.Info("Starting workers") - - for i := 0; i < workers; i++ { - go wait.UntilWithContext(ctx, c.runWorker, time.Second) - } - - klog.Info("Controller started") - <-ctx.Done() - klog.Info("Shutting down controller") - - return nil -} - -// runWorker processes items from the workqueue. -func (c *Controller) runWorker(ctx context.Context) { - for c.processNextWorkItem(ctx) { - } -} - -// processNextWorkItem processes a single item from the workqueue. -func (c *Controller) processNextWorkItem(ctx context.Context) bool { - key, shutdown := c.workqueue.Get() - if shutdown { - return false - } - defer c.workqueue.Done(key) - - start := time.Now() - - err := c.syncHandler(ctx, key) - if err != nil { - if c.workqueue.NumRequeues(key) < maxRetries { - c.workqueue.AddRateLimited(key) - workqueueRetries.WithLabelValues("Nodes").Inc() - - err = fmt.Errorf("error syncing '%s': %s, requeuing", key, err.Error()) - } else { - c.workqueue.Forget(key) - klog.Errorf("Dropping node %q out of the queue after %d retries: %v", key, maxRetries, err) - } - } else { - c.workqueue.Forget(key) - } - - duration := time.Since(start).Seconds() - reconciliationDuration.WithLabelValues("Nodes").Observe(duration) - - if err != nil { - reconciliationErrors.WithLabelValues("Nodes").Inc() - reconciliationTotal.WithLabelValues("Nodes", "error").Inc() - klog.Error(err) - } else { - reconciliationTotal.WithLabelValues("Nodes", "success").Inc() - } - - return true -} - -// syncHandler processes a node and allocates CIDRs if needed. -func (c *Controller) syncHandler(ctx context.Context, key string) error { - node, err := c.nodeLister.Get(key) - if err != nil { - if errors.IsNotFound(err) { - klog.V(4).Infof("Node %s has been deleted", key) - // Release any pending CIDRs for this deleted node - c.pendingReleasesLock.Lock() - - cidrs, exists := c.pendingReleases[key] - if exists { - delete(c.pendingReleases, key) - } - c.pendingReleasesLock.Unlock() - - if exists { - for _, cidr := range cidrs { - c.allocator.Release(cidr) - PodCIDRReleases.Inc() - } - - klog.Infof("Released CIDRs %v for deleted node %s via workqueue", cidrs, key) - } - - return nil - } - - return err - } - - // Check if node matches the filter - if !c.matchesNodeFilter(node.Name) { - return nil - } - - // Check if node already has podCIDRs assigned (from cache - fast path) - if len(node.Spec.PodCIDRs) > 0 || node.Spec.PodCIDR != "" { - // Mark existing CIDRs as allocated (idempotent) - if node.Spec.PodCIDR != "" { - c.allocator.MarkAllocated(node.Spec.PodCIDR) - } - - for _, cidr := range node.Spec.PodCIDRs { - c.allocator.MarkAllocated(cidr) - } - - klog.V(4).Infof("Node %s already has podCIDRs assigned: %v", node.Name, node.Spec.PodCIDRs) - - return nil - } - - // Node needs CIDRs - do a direct API fetch to get the latest state - // This prevents double-allocation when the cache is stale - freshNode, err := c.clientset.CoreV1().Nodes().Get(ctx, key, metav1.GetOptions{}) - if err != nil { - if errors.IsNotFound(err) { - klog.V(4).Infof("Node %s has been deleted", key) - return nil - } - - return err - } - - // Re-check with fresh data - if len(freshNode.Spec.PodCIDRs) > 0 || freshNode.Spec.PodCIDR != "" { - // Node was updated between cache read and API fetch - mark as allocated - if freshNode.Spec.PodCIDR != "" { - c.allocator.MarkAllocated(freshNode.Spec.PodCIDR) - } - - for _, cidr := range freshNode.Spec.PodCIDRs { - c.allocator.MarkAllocated(cidr) - } - - klog.V(2).Infof("Node %s already has podCIDRs assigned (confirmed from API): %v", freshNode.Name, freshNode.Spec.PodCIDRs) - - return nil - } - - // Allocate CIDRs for this node - return c.allocateCIDRsForNode(ctx, freshNode) -} - -// allocateCIDRsForNode allocates and assigns CIDRs to a node. -func (c *Controller) allocateCIDRsForNode(ctx context.Context, node *corev1.Node) error { - var ( - podCIDR string - podCIDRs []string - ) - - hasIPv4 := c.allocator.HasIPv4Pools() - hasIPv6 := c.allocator.HasIPv6Pools() - - if hasIPv4 { - ipv4CIDR, err := c.allocator.AllocateIPv4() - if err != nil { - PodCIDRExhaustion.Inc() - klog.Errorf("Failed to allocate IPv4 CIDR for node %s: %v -- CIDR pool exhausted", node.Name, err) - - if c.recorder != nil { - c.recorder.Eventf(node, corev1.EventTypeWarning, "CIDRExhausted", "Failed to allocate IPv4 CIDR: %v", err) - } - - return fmt.Errorf("failed to allocate IPv4 CIDR for node %s: %w", node.Name, err) - } - - podCIDR = ipv4CIDR - podCIDRs = append(podCIDRs, ipv4CIDR) - - PodCIDRAllocations.Inc() - klog.Infof("Allocated IPv4 CIDR %s for node %s", ipv4CIDR, node.Name) - } - - if hasIPv6 { - ipv6CIDR, err := c.allocator.AllocateIPv6() - if err != nil { - PodCIDRExhaustion.Inc() - klog.Errorf("Failed to allocate IPv6 CIDR for node %s: %v -- CIDR pool exhausted", node.Name, err) - - if c.recorder != nil { - c.recorder.Eventf(node, corev1.EventTypeWarning, "CIDRExhausted", "Failed to allocate IPv6 CIDR: %v", err) - } - - return fmt.Errorf("failed to allocate IPv6 CIDR for node %s: %w", node.Name, err) - } - - if podCIDR == "" { - podCIDR = ipv6CIDR - } - - podCIDRs = append(podCIDRs, ipv6CIDR) - - PodCIDRAllocations.Inc() - klog.Infof("Allocated IPv6 CIDR %s for node %s", ipv6CIDR, node.Name) - } - - // Patch the node with the allocated CIDRs - if err := c.patchNodeCIDRs(ctx, node.Name, podCIDR, podCIDRs); err != nil { - return err - } - - // Emit success event - if c.recorder != nil { - c.recorder.Eventf(node, corev1.EventTypeNormal, "CIDRAssigned", "Assigned podCIDRs %v", podCIDRs) - } - - return nil -} - -// patchNodeCIDRs patches a node's spec with the allocated CIDRs. -func (c *Controller) patchNodeCIDRs(ctx context.Context, nodeName, podCIDR string, podCIDRs []string) error { - // Build the patch - podCIDRsJSON := "[" - - for i, cidr := range podCIDRs { - if i > 0 { - podCIDRsJSON += "," - } - - podCIDRsJSON += fmt.Sprintf("%q", cidr) - } - - podCIDRsJSON += "]" - - patch := fmt.Sprintf(`{"spec":{"podCIDR":%q,"podCIDRs":%s}}`, podCIDR, podCIDRsJSON) - - _, err := c.clientset.CoreV1().Nodes().Patch( - ctx, - nodeName, - types.StrategicMergePatchType, - []byte(patch), - metav1.PatchOptions{}, - ) - if err != nil { - return fmt.Errorf("failed to patch node %s: %w", nodeName, err) - } - - klog.Infof("Successfully assigned podCIDR=%s, podCIDRs=%v to node %s", podCIDR, podCIDRs, nodeName) - - return nil -} - -// InitializeAllocator scans all existing nodes and marks their CIDRs as allocated. -// Note: This marks CIDRs from ALL nodes as allocated, regardless of the node filter, -// to avoid allocating CIDRs that are already in use by nodes outside the filter. -func (c *Controller) InitializeAllocator(ctx context.Context) error { - nodes, err := c.clientset.CoreV1().Nodes().List(ctx, metav1.ListOptions{}) - if err != nil { - return fmt.Errorf("failed to list nodes: %w", err) - } - - matchingNodes := 0 - - for _, node := range nodes.Items { - if node.Spec.PodCIDR != "" { - c.allocator.MarkAllocated(node.Spec.PodCIDR) - klog.V(2).Infof("Marked existing CIDR %s as allocated (node %s)", node.Spec.PodCIDR, node.Name) - } - - for _, cidr := range node.Spec.PodCIDRs { - c.allocator.MarkAllocated(cidr) - klog.V(2).Infof("Marked existing CIDR %s as allocated (node %s)", cidr, node.Name) - } - - if c.matchesNodeFilter(node.Name) { - matchingNodes++ - } - } - - if c.nodeRegex != nil { - klog.Infof("Initialized allocator with %d existing nodes (%d matching filter)", len(nodes.Items), matchingNodes) - } else { - klog.Infof("Initialized allocator with %d existing nodes", len(nodes.Items)) - } - - return nil -} - -// DryRun performs a single evaluation pass and prints proposed changes without applying them. -func (c *Controller) DryRun(ctx context.Context) error { - nodes, err := c.clientset.CoreV1().Nodes().List(ctx, metav1.ListOptions{}) - if err != nil { - return fmt.Errorf("failed to list nodes: %w", err) - } - - // First pass: mark existing allocations from ALL nodes (regardless of filter) - for _, node := range nodes.Items { - if node.Spec.PodCIDR != "" { - c.allocator.MarkAllocated(node.Spec.PodCIDR) - } - - for _, cidr := range node.Spec.PodCIDRs { - c.allocator.MarkAllocated(cidr) - } - } - - // Second pass: calculate proposed allocations (only for matching nodes) - hasChanges := false - - for _, node := range nodes.Items { - // Check if node matches the filter - if !c.matchesNodeFilter(node.Name) { - klog.V(3).Infof("[DRY-RUN] Node %s: skipped (does not match filter)", node.Name) - continue - } - - if len(node.Spec.PodCIDRs) > 0 || node.Spec.PodCIDR != "" { - klog.Infof("[DRY-RUN] Node %s: already has podCIDR=%s, podCIDRs=%v (no changes)", node.Name, node.Spec.PodCIDR, node.Spec.PodCIDRs) - continue - } - - var ( - podCIDR string - podCIDRs []string - ) - - hasIPv4 := c.allocator.HasIPv4Pools() - hasIPv6 := c.allocator.HasIPv6Pools() - - if hasIPv4 { - ipv4CIDR, err := c.allocator.AllocateIPv4() - if err != nil { - klog.Errorf("[DRY-RUN] Node %s: failed to allocate IPv4 CIDR: %v", node.Name, err) - return fmt.Errorf("IPv4 CIDR pool exhausted") - } - - podCIDR = ipv4CIDR - podCIDRs = append(podCIDRs, ipv4CIDR) - } - - if hasIPv6 { - ipv6CIDR, err := c.allocator.AllocateIPv6() - if err != nil { - klog.Errorf("[DRY-RUN] Node %s: failed to allocate IPv6 CIDR: %v", node.Name, err) - return fmt.Errorf("IPv6 CIDR pool exhausted") - } - - if podCIDR == "" { - podCIDR = ipv6CIDR - } - - podCIDRs = append(podCIDRs, ipv6CIDR) - } - - fmt.Printf("[DRY-RUN] Node %s: would assign podCIDR=%s, podCIDRs=%v\n", node.Name, podCIDR, podCIDRs) - - hasChanges = true - } - - if !hasChanges { - fmt.Println("[DRY-RUN] No changes needed - all matching nodes already have podCIDRs assigned") - } - - return nil -} diff --git a/internal/net/controller/controller_helpers_test.go b/internal/net/controller/controller_helpers_test.go deleted file mode 100644 index 7be301abc..000000000 --- a/internal/net/controller/controller_helpers_test.go +++ /dev/null @@ -1,120 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. - -package controller - -import ( - "context" - "regexp" - "testing" - - "k8s.io/client-go/informers" - "k8s.io/client-go/kubernetes/fake" - "k8s.io/client-go/tools/cache" - "k8s.io/client-go/util/workqueue" - - "github.com/Azure/unbounded/internal/net/allocator" -) - -// TestControllerInformerSynced tests controller informer synced. -func TestControllerInformerSynced(t *testing.T) { - c := &Controller{nodeSynced: func() bool { return true }} - if !c.InformerSynced() { - t.Fatalf("expected informer synced to be true") - } - - c.nodeSynced = func() bool { return false } - if c.InformerSynced() { - t.Fatalf("expected informer synced to be false") - } -} - -// TestControllerMatchesNodeFilter tests controller matches node filter. -func TestControllerMatchesNodeFilter(t *testing.T) { - c := &Controller{} - if !c.matchesNodeFilter("any-node") { - t.Fatalf("expected match when no regex configured") - } - - c.nodeRegex = regexp.MustCompile(`^node-[0-9]+$`) - if !c.matchesNodeFilter("node-12") { - t.Fatalf("expected regex-matching node to pass filter") - } - - if c.matchesNodeFilter("worker-a") { - t.Fatalf("expected non-matching node to fail filter") - } -} - -// TestControllerEnqueueNode tests controller enqueue node. -func TestControllerEnqueueNode(t *testing.T) { - q := workqueue.NewTypedRateLimitingQueueWithConfig(workqueue.DefaultTypedControllerRateLimiter[string](), workqueue.TypedRateLimitingQueueConfig[string]{Name: "test"}) - defer q.ShutDown() - - c := &Controller{workqueue: q} - c.enqueueNode(cache.ExplicitKey("node-a")) - - if got := q.Len(); got != 1 { - t.Fatalf("expected queue length 1 after enqueue, got %d", got) - } - - obj, shutdown := q.Get() - if shutdown { - t.Fatalf("expected queue item, got shutdown") - } - - q.Done(obj) - - if obj != "node-a" { - t.Fatalf("unexpected queued key: %q", obj) - } -} - -// TestControllerProcessNextWorkItemNotFoundNode verifies that processNextWorkItem -// gracefully handles a node key not found in the lister (the node was deleted). -func TestControllerProcessNextWorkItemNotFoundNode(t *testing.T) { - q := workqueue.NewTypedRateLimitingQueueWithConfig(workqueue.DefaultTypedControllerRateLimiter[string](), workqueue.TypedRateLimitingQueueConfig[string]{Name: "test"}) - defer q.ShutDown() - - client := fake.NewClientset() - factory := informers.NewSharedInformerFactory(client, 0) - nodeInformer := factory.Core().V1().Nodes() - - alloc, err := allocator.NewAllocator(nil, nil, 24, 64) - if err != nil { - t.Fatalf("failed to create allocator: %v", err) - } - - c := &Controller{ - workqueue: q, - nodeLister: nodeInformer.Lister(), - nodeSynced: nodeInformer.Informer().HasSynced, - allocator: alloc, - pendingReleases: make(map[string][]string), - } - - q.Add("nonexistent-node") - - if ok := c.processNextWorkItem(context.Background()); !ok { - t.Fatalf("expected worker loop to continue after not-found node") - } - - if q.Len() != 0 { - t.Fatalf("expected queue to be drained") - } -} - -// TestNewControllerRejectsInvalidRegex tests new controller rejects invalid regex. -func TestNewControllerRejectsInvalidRegex(t *testing.T) { - client := fake.NewClientset() - factory := informers.NewSharedInformerFactory(client, 0) - - alloc, err := allocator.NewAllocator(nil, nil, 24, 64) - if err != nil { - t.Fatalf("failed to create allocator: %v", err) - } - - if _, err := NewController(client, factory, alloc, "[", nil); err == nil { - t.Fatalf("expected invalid regex error") - } -} diff --git a/internal/net/controller/peering_aggregation_controller.go b/internal/net/controller/peering_aggregation_controller.go index 6f02fd1b3..bbd224ccb 100644 --- a/internal/net/controller/peering_aggregation_controller.go +++ b/internal/net/controller/peering_aggregation_controller.go @@ -543,11 +543,6 @@ func (pc *PeeringAggregationController) reconcileSitePeeringStatuses(ctx context return nil } -func computeReachableRoutes(startPool string, pools map[string]*unboundednetv1alpha1.GatewayPool, sites map[string]*unboundedv1alpha3.Site, connectedSites, adjacency map[string]map[string]struct{}) []unboundednetv1alpha1.GatewayPoolRoute { - _, routes := computeReachable(startPool, pools, sites, connectedSites, adjacency) - return routes -} - type reachableSiteSets struct { connected []string reachable []string diff --git a/internal/net/controller/peering_aggregation_controller_test.go b/internal/net/controller/peering_aggregation_controller_test.go index 93c29e363..d6f11ca19 100644 --- a/internal/net/controller/peering_aggregation_controller_test.go +++ b/internal/net/controller/peering_aggregation_controller_test.go @@ -107,9 +107,9 @@ func TestComputeReachableRoutesAndRouteSelectionPriority(t *testing.T) { "pool-b": {"pool-a": {}}, } - routes := computeReachableRoutes("pool-a", pools, sites, connectedSites, adjacency) + _, routes := computeReachable("pool-a", pools, sites, connectedSites, adjacency) if len(routes) == 0 { - t.Fatalf("expected non-empty routes from computeReachableRoutes") + t.Fatalf("expected non-empty routes from computeReachable") } // Check direct-vs-transitive weighting by looking up site-a/site-b node CIDRs. diff --git a/internal/net/controller/site_controller.go b/internal/net/controller/site_controller.go index deabeba98..6540bd75b 100644 --- a/internal/net/controller/site_controller.go +++ b/internal/net/controller/site_controller.go @@ -2303,15 +2303,6 @@ func (sc *SiteController) findSiteForNode(node *corev1.Node, sites []unboundedv1 return "" } -// GetSiteForNode looks up which site a node belongs to using the cached sites. -// This is a faster lookup for use by other components. -func (sc *SiteController) GetSiteForNode(node *corev1.Node) string { - sc.sitesCacheLock.RLock() - defer sc.sitesCacheLock.RUnlock() - - return sc.findSiteForNode(node, sc.sitesCache) -} - // Helper functions // siteLabelKeys are the node site-membership label keys in priority order: