diff --git a/pkg/api/servicecidr/servicecidr.go b/pkg/api/servicecidr/servicecidr.go index a3af14b633fed..c6d47732df655 100644 --- a/pkg/api/servicecidr/servicecidr.go +++ b/pkg/api/servicecidr/servicecidr.go @@ -26,6 +26,46 @@ import ( networkinglisters "k8s.io/client-go/listers/networking/v1alpha1" ) +// OverlapsPrefix return the list of ServiceCIDR that overlaps with the prefix passed as argument +func OverlapsPrefix(serviceCIDRLister networkinglisters.ServiceCIDRLister, prefix netip.Prefix) []*networkingv1alpha1.ServiceCIDR { + result := []*networkingv1alpha1.ServiceCIDR{} + serviceCIDRList, err := serviceCIDRLister.List(labels.Everything()) + if err != nil { + return result + } + + for _, serviceCIDR := range serviceCIDRList { + for _, cidr := range serviceCIDR.Spec.CIDRs { + if p, err := netip.ParsePrefix(cidr); err == nil { // it can not fail since is already validated + if p.Overlaps(prefix) { + result = append(result, serviceCIDR) + } + } + } + } + return result +} + +// ContainsPrefix return the list of ServiceCIDR that contains the prefix passed as argument +func ContainsPrefix(serviceCIDRLister networkinglisters.ServiceCIDRLister, prefix netip.Prefix) []*networkingv1alpha1.ServiceCIDR { + result := []*networkingv1alpha1.ServiceCIDR{} + serviceCIDRList, err := serviceCIDRLister.List(labels.Everything()) + if err != nil { + return result + } + + for _, serviceCIDR := range serviceCIDRList { + for _, cidr := range serviceCIDR.Spec.CIDRs { + if p, err := netip.ParsePrefix(cidr); err == nil { // it can not fail since is already validated + if p.Overlaps(prefix) && p.Bits() <= prefix.Bits() { + result = append(result, serviceCIDR) + } + } + } + } + return result +} + // ContainsIP return the list of ServiceCIDR that contains the IP address passed as argument func ContainsIP(serviceCIDRLister networkinglisters.ServiceCIDRLister, ip net.IP) []*networkingv1alpha1.ServiceCIDR { address := IPToAddr(ip) diff --git a/pkg/api/servicecidr/servicecidr_test.go b/pkg/api/servicecidr/servicecidr_test.go index 73140376cdf1f..ac7e169aa0852 100644 --- a/pkg/api/servicecidr/servicecidr_test.go +++ b/pkg/api/servicecidr/servicecidr_test.go @@ -43,6 +43,372 @@ func newServiceCIDR(name, primary, secondary string) *networkingv1alpha1.Service return serviceCIDR } +func TestOverlapsPrefix(t *testing.T) { + tests := []struct { + name string + serviceCIDRs []*networkingv1alpha1.ServiceCIDR + prefix netip.Prefix + want []string + }{ + { + name: "only one ServiceCIDR and IPv4 prefix contained", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + }, + prefix: netip.MustParsePrefix("10.0.0.0/26"), + want: []string{"kubernetes"}, + }, + { + name: "only one ServiceCIDR and same IPv4 prefix", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + }, + prefix: netip.MustParsePrefix("10.0.0.0/24"), + want: []string{"kubernetes"}, + }, + { + name: "only one ServiceCIDR and larger IPv4 prefix", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + }, + prefix: netip.MustParsePrefix("10.0.0.0/16"), + want: []string{"kubernetes"}, + }, + { + name: "only one ServiceCIDR and non contained IPv4 prefix", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + }, + prefix: netip.MustParsePrefix("192.168.0.0/24"), + want: []string{}, + }, + { + name: "only one ServiceCIDR and IPv6 prefix contained", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + }, + prefix: netip.MustParsePrefix("2001:db8::/112"), + want: []string{"kubernetes"}, + }, + { + name: "only one ServiceCIDR and same IPv6 prefix", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + }, + prefix: netip.MustParsePrefix("2001:db8::/96"), + want: []string{"kubernetes"}, + }, + { + name: "only one ServiceCIDR and IPv6 larger", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + }, + prefix: netip.MustParsePrefix("2001:db8::/64"), + want: []string{"kubernetes"}, + }, + { + name: "only one ServiceCIDR and IPv6 prefix out of range", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + }, + prefix: netip.MustParsePrefix("2001:db2::/112"), + want: []string{}, + }, + { + name: "two ServiceCIDR and IPv4 prefix contained", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + newServiceCIDR("secondary", "10.0.0.0/16", "2001:db8::/64"), + }, + prefix: netip.MustParsePrefix("10.0.0.0/24"), + want: []string{"kubernetes", "secondary"}, + }, + { + name: "two overlapping ServiceCIDR and IPv4 prefix only contained in one", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + newServiceCIDR("secondary", "10.0.0.0/16", "2001:db8::/64"), + }, + prefix: netip.MustParsePrefix("10.0.0.0/18"), + want: []string{"kubernetes", "secondary"}, + }, + { + name: "two ServiceCIDR and IPv4 larger", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + newServiceCIDR("secondary", "10.0.0.0/16", "2001:db8::/64"), + }, + prefix: netip.MustParsePrefix("10.0.0.0/8"), + want: []string{"kubernetes", "secondary"}, + }, + { + name: "two ServiceCIDR and IPv4 prefix not contained", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + newServiceCIDR("secondary", "10.0.0.0/16", "2001:db8::/64"), + }, + prefix: netip.MustParsePrefix("192.168.0.0/24"), + want: []string{}, + }, + { + name: "two ServiceCIDR and IPv6 prefix contained", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + newServiceCIDR("secondary", "10.0.0.0/16", "2001:db8::/64"), + }, + prefix: netip.MustParsePrefix("2001:db8::/96"), + want: []string{"kubernetes", "secondary"}, + }, + { + name: "two ServiceCIDR and IPv6 prefix contained in one", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + newServiceCIDR("secondary", "10.0.0.0/16", "2001:db8::/64"), + }, + prefix: netip.MustParsePrefix("2001:db8::/72"), + want: []string{"kubernetes", "secondary"}, + }, + { + name: "two ServiceCIDR and aprefix larger", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + newServiceCIDR("secondary", "10.0.0.0/16", "2001:db8::/64"), + }, + prefix: netip.MustParsePrefix("2001:db8::/52"), + want: []string{"kubernetes", "secondary"}, + }, + { + name: "two ServiceCIDR and prefix out of range", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + newServiceCIDR("secondary", "10.0.0.0/16", "2001:db8::/64"), + }, + prefix: netip.MustParsePrefix("2001:db2::/64"), + want: []string{}, + }, + { + name: "multiple ServiceCIDR match with overlap contained", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + newServiceCIDR("kubernetes2", "10.0.0.0/24", "2001:db8::/96"), + newServiceCIDR("secondary", "10.0.0.0/16", "2001:db8::/64"), + }, + prefix: netip.MustParsePrefix("10.0.0.0/28"), + want: []string{"kubernetes", "kubernetes2", "secondary"}, + }, + { + name: "multiple ServiceCIDR match with overlap contains", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + newServiceCIDR("kubernetes2", "10.0.0.0/24", "2001:db8::/96"), + newServiceCIDR("secondary", "10.0.0.0/16", "2001:db8::/64"), + }, + prefix: netip.MustParsePrefix("10.0.0.0/8"), + want: []string{"kubernetes", "kubernetes2", "secondary"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}) + for _, serviceCIDR := range tt.serviceCIDRs { + err := indexer.Add(serviceCIDR) + if err != nil { + t.Fatalf("unexpected error %v", err) + } + } + lister := networkinglisters.NewServiceCIDRLister(indexer) + got := []string{} + for _, serviceCIDR := range OverlapsPrefix(lister, tt.prefix) { + got = append(got, serviceCIDR.Name) + } + // sort slices to make the order predictable and avoid flakiness + sort.Strings(got) + sort.Strings(tt.want) + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("OverlapsAddress() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestContainsPrefix(t *testing.T) { + tests := []struct { + name string + serviceCIDRs []*networkingv1alpha1.ServiceCIDR + prefix netip.Prefix + want []string + }{ + { + name: "only one ServiceCIDR and IPv4 prefix contained", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + }, + prefix: netip.MustParsePrefix("10.0.0.0/26"), + want: []string{"kubernetes"}, + }, + { + name: "only one ServiceCIDR and same IPv4 prefix", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + }, + prefix: netip.MustParsePrefix("10.0.0.0/24"), + want: []string{"kubernetes"}, + }, + { + name: "only one ServiceCIDR and larger IPv4 prefix", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + }, + prefix: netip.MustParsePrefix("10.0.0.0/16"), + want: []string{}, + }, + { + name: "only one ServiceCIDR and non containerd IPv4 prefix", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + }, + prefix: netip.MustParsePrefix("192.168.0.0/24"), + want: []string{}, + }, + { + name: "only one ServiceCIDR and IPv6 prefix contained", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + }, + prefix: netip.MustParsePrefix("2001:db8::/112"), + want: []string{"kubernetes"}, + }, + { + name: "only one ServiceCIDR and same IPv6 prefix", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + }, + prefix: netip.MustParsePrefix("2001:db8::/96"), + want: []string{"kubernetes"}, + }, + { + name: "only one ServiceCIDR and IPv6 larger", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + }, + prefix: netip.MustParsePrefix("2001:db8::/64"), + want: []string{}, + }, + { + name: "only one ServiceCIDR and IPv6 prefix out of range", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + }, + prefix: netip.MustParsePrefix("2001:db2::/112"), + want: []string{}, + }, + { + name: "two ServiceCIDR and IPv4 prefix contained", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + newServiceCIDR("secondary", "10.0.0.0/16", "2001:db8::/64"), + }, + prefix: netip.MustParsePrefix("10.0.0.0/24"), + want: []string{"kubernetes", "secondary"}, + }, + { + name: "two ServiceCIDR and IPv4 prefix only contained in one", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + newServiceCIDR("secondary", "10.0.0.0/16", "2001:db8::/64"), + }, + prefix: netip.MustParsePrefix("10.0.0.0/18"), + want: []string{"secondary"}, + }, + { + name: "two ServiceCIDR and IPv4 larger", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + newServiceCIDR("secondary", "10.0.0.0/16", "2001:db8::/64"), + }, + prefix: netip.MustParsePrefix("10.0.0.0/8"), + want: []string{}, + }, + { + name: "two ServiceCIDR and IPv4 prefix not contained", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + newServiceCIDR("secondary", "10.0.0.0/16", "2001:db8::/64"), + }, + prefix: netip.MustParsePrefix("192.168.0.0/24"), + want: []string{}, + }, + { + name: "two ServiceCIDR and IPv6 prefix contained", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + newServiceCIDR("secondary", "10.0.0.0/16", "2001:db8::/64"), + }, + prefix: netip.MustParsePrefix("2001:db8::/96"), + want: []string{"kubernetes", "secondary"}, + }, + { + name: "two ServiceCIDR and IPv6 prefix contained in one", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + newServiceCIDR("secondary", "10.0.0.0/16", "2001:db8::/64"), + }, + prefix: netip.MustParsePrefix("2001:db8::/72"), + want: []string{"secondary"}, + }, + { + name: "two ServiceCIDR and aprefix larger", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + newServiceCIDR("secondary", "10.0.0.0/16", "2001:db8::/64"), + }, + prefix: netip.MustParsePrefix("2001:db8::/52"), + want: []string{}, + }, + { + name: "two ServiceCIDR and prefix out of range", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + newServiceCIDR("secondary", "10.0.0.0/16", "2001:db8::/64"), + }, + prefix: netip.MustParsePrefix("2001:db2::/64"), + want: []string{}, + }, + { + name: "multiple ServiceCIDR match with overlap", + serviceCIDRs: []*networkingv1alpha1.ServiceCIDR{ + newServiceCIDR("kubernetes", "10.0.0.0/24", "2001:db8::/96"), + newServiceCIDR("kubernetes2", "10.0.0.0/24", "2001:db8::/96"), + newServiceCIDR("secondary", "10.0.0.0/16", "2001:db8::/64"), + }, + prefix: netip.MustParsePrefix("10.0.0.0/28"), + want: []string{"kubernetes", "kubernetes2", "secondary"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}) + for _, serviceCIDR := range tt.serviceCIDRs { + err := indexer.Add(serviceCIDR) + if err != nil { + t.Fatalf("unexpected error %v", err) + } + } + lister := networkinglisters.NewServiceCIDRLister(indexer) + got := []string{} + for _, serviceCIDR := range ContainsPrefix(lister, tt.prefix) { + got = append(got, serviceCIDR.Name) + } + // sort slices to make the order predictable and avoid flakiness + sort.Strings(got) + sort.Strings(tt.want) + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("ContainsAddress() = %v, want %v", got, tt.want) + } + }) + } +} + func TestContainsAddress(t *testing.T) { tests := []struct { name string diff --git a/pkg/controller/servicecidrs/servicecidrs_controller.go b/pkg/controller/servicecidrs/servicecidrs_controller.go index ee40847f0b605..b31a0b0339623 100644 --- a/pkg/controller/servicecidrs/servicecidrs_controller.go +++ b/pkg/controller/servicecidrs/servicecidrs_controller.go @@ -20,7 +20,6 @@ import ( "context" "encoding/json" "net/netip" - "sync" "time" v1 "k8s.io/api/core/v1" @@ -43,8 +42,8 @@ import ( "k8s.io/client-go/tools/record" "k8s.io/client-go/util/workqueue" "k8s.io/klog/v2" + "k8s.io/kubernetes/pkg/api/servicecidr" "k8s.io/kubernetes/pkg/registry/core/service/ipallocator" - "k8s.io/kubernetes/pkg/util/iptree" netutils "k8s.io/utils/net" ) @@ -80,7 +79,6 @@ func NewController( workqueue.DefaultTypedControllerRateLimiter[string](), workqueue.TypedRateLimitingQueueConfig[string]{Name: "ipaddresses"}, ), - tree: iptree.New[sets.Set[string]](), workerLoopPeriod: time.Second, } @@ -122,10 +120,6 @@ type Controller struct { // workerLoopPeriod is the time between worker runs. The workers process the queue of service and ipRange changes. workerLoopPeriod time.Duration - - // tree store the ServiceCIDRs names associated to each - muTree sync.Mutex - tree *iptree.Tree[sets.Set[string]] } // Run will not return until stopCh is closed. @@ -213,24 +207,17 @@ func (c *Controller) deleteIPAddress(obj interface{}) { // this is required because adding or removing a CIDR will require to recompute the // state of each ServiceCIDR to check if can be unblocked on deletion. func (c *Controller) overlappingServiceCIDRs(serviceCIDR *networkingapiv1alpha1.ServiceCIDR) []string { - c.muTree.Lock() - defer c.muTree.Unlock() - - serviceCIDRs := sets.New[string]() + result := sets.New[string]() for _, cidr := range serviceCIDR.Spec.CIDRs { if prefix, err := netip.ParsePrefix(cidr); err == nil { // if is empty err will not be nil - c.tree.WalkPath(prefix, func(k netip.Prefix, v sets.Set[string]) bool { - serviceCIDRs.Insert(v.UnsortedList()...) - return false - }) - c.tree.WalkPrefix(prefix, func(k netip.Prefix, v sets.Set[string]) bool { - serviceCIDRs.Insert(v.UnsortedList()...) - return false - }) + serviceCIDRs := servicecidr.OverlapsPrefix(c.serviceCIDRLister, prefix) + for _, v := range serviceCIDRs { + result.Insert(v.Name) + } } } - return serviceCIDRs.UnsortedList() + return result.UnsortedList() } // containingServiceCIDRs, given an IPAddress return the ServiceCIDRs that contains the IP, @@ -249,16 +236,13 @@ func (c *Controller) containingServiceCIDRs(ip *networkingapiv1alpha1.IPAddress) return []string{} } - c.muTree.Lock() - defer c.muTree.Unlock() - serviceCIDRs := []string{} - // walk the tree to get all the ServiceCIDRs that contain this IP address - prefixes := c.tree.GetHostIPPrefixMatches(address) - for _, v := range prefixes { - serviceCIDRs = append(serviceCIDRs, v.UnsortedList()...) + result := sets.New[string]() + serviceCIDRs := servicecidr.ContainsAddress(c.serviceCIDRLister, address) + for _, v := range serviceCIDRs { + result.Insert(v.Name) } - return serviceCIDRs + return result.UnsortedList() } func (c *Controller) worker(ctx context.Context) { @@ -290,38 +274,6 @@ func (c *Controller) processNext(ctx context.Context) bool { return true } -// syncCIDRs rebuilds the radix tree based from the informers cache -func (c *Controller) syncCIDRs() error { - serviceCIDRList, err := c.serviceCIDRLister.List(labels.Everything()) - if err != nil { - return err - } - - // track the names of the different ServiceCIDRs, there - // can be multiple ServiceCIDRs sharing the same prefixes - // and this is important to determine if a ServiceCIDR can - // be deleted. - tree := iptree.New[sets.Set[string]]() - for _, serviceCIDR := range serviceCIDRList { - for _, cidr := range serviceCIDR.Spec.CIDRs { - if prefix, err := netip.ParsePrefix(cidr); err == nil { // if is empty err will not be nil - // if the prefix already exist append the new ServiceCIDR name - v, ok := tree.GetPrefix(prefix) - if !ok { - v = sets.Set[string]{} - } - v.Insert(serviceCIDR.Name) - tree.InsertPrefix(prefix, v) - } - } - } - - c.muTree.Lock() - defer c.muTree.Unlock() - c.tree = tree - return nil -} - func (c *Controller) sync(ctx context.Context, key string) error { logger := klog.FromContext(ctx) startTime := time.Now() @@ -329,13 +281,6 @@ func (c *Controller) sync(ctx context.Context, key string) error { logger.V(4).Info("Finished syncing ServiceCIDR)", "ServiceCIDR", key, "elapsed", time.Since(startTime)) }() - // TODO(aojea) verify if this present a performance problem - // restore the radix tree from the current state - err := c.syncCIDRs() - if err != nil { - return err - } - logger.V(4).Info("syncing ServiceCIDR", "ServiceCIDR", key) cidr, err := c.serviceCIDRLister.Get(key) if err != nil { @@ -406,23 +351,16 @@ func (c *Controller) sync(ctx context.Context, key string) error { // canDeleteCIDR checks that the ServiceCIDR can be safely deleted and not leave orphan IPAddresses func (c *Controller) canDeleteCIDR(ctx context.Context, serviceCIDR *networkingapiv1alpha1.ServiceCIDR) (bool, error) { - // TODO(aojea) Revisit the lock usage and if we need to keep it only for the tree operations - // to avoid holding it during the whole operation. - c.muTree.Lock() - defer c.muTree.Unlock() logger := klog.FromContext(ctx) // Check if there is a subnet that already contains the ServiceCIDR that is going to be deleted. hasParent := true for _, cidr := range serviceCIDR.Spec.CIDRs { - // Walk the tree to find if there is a larger subnet that contains the existing one, + // Find if there is a larger subnet that contains the existing one, // or there is another ServiceCIDR with the same subnet. if prefix, err := netip.ParsePrefix(cidr); err == nil { - serviceCIDRs := sets.New[string]() - c.tree.WalkPath(prefix, func(k netip.Prefix, v sets.Set[string]) bool { - serviceCIDRs.Insert(v.UnsortedList()...) - return false - }) - if serviceCIDRs.Len() == 1 && serviceCIDRs.Has(serviceCIDR.Name) { + serviceCIDRs := servicecidr.ContainsPrefix(c.serviceCIDRLister, prefix) + if len(serviceCIDRs) == 0 || + len(serviceCIDRs) == 1 && serviceCIDRs[0].Name == serviceCIDR.Name { hasParent = false } } @@ -431,7 +369,7 @@ func (c *Controller) canDeleteCIDR(ctx context.Context, serviceCIDR *networkinga // All the existing IP addresses will be contained on the parent ServiceCIDRs, // it is safe to delete, remove the finalizer. if hasParent { - logger.V(2).Info("Removing finalizer for ServiceCIDR", "ServiceCIDR", serviceCIDR.String()) + logger.Info("Deleting ServiceCIDR contained in other ServiceCIDR", "ServiceCIDR", serviceCIDR.String()) return true, nil } @@ -458,22 +396,18 @@ func (c *Controller) canDeleteCIDR(ctx context.Context, serviceCIDR *networkinga logger.Info("[SHOULD NOT HAPPEN] unexpected error parsing IPAddress", "IPAddress", ip.Name, "error", err) continue } - // walk the tree to find all ServiceCIDRs containing this IP - prefixes := c.tree.GetHostIPPrefixMatches(address) - if len(prefixes) != 1 { - continue - } - for _, v := range prefixes { - if v.Len() == 1 && v.Has(serviceCIDR.Name) { - return false, nil - } + // find all ServiceCIDRs containing this IP + serviceCIDRs := servicecidr.ContainsAddress(c.serviceCIDRLister, address) + if len(serviceCIDRs) == 1 && serviceCIDRs[0].Name == serviceCIDR.Name { + logger.Info("Deleting ServiceCIDR blocked by IP address", "IPAddress", address.String()) + return false, nil } } } // There are no IPAddresses that depend on the existing ServiceCIDR, so // it is safe to delete, remove finalizer. - logger.Info("ServiceCIDR no longer have orphan IPs", "ServiceCDIR", serviceCIDR.String()) + logger.Info("Deleting ServiceCIDR no longer have orphan IPs", "ServiceCIDR", serviceCIDR.String()) return true, nil } diff --git a/pkg/controller/servicecidrs/servicecidrs_controller_test.go b/pkg/controller/servicecidrs/servicecidrs_controller_test.go index c5ef708598de1..251fb2f019c69 100644 --- a/pkg/controller/servicecidrs/servicecidrs_controller_test.go +++ b/pkg/controller/servicecidrs/servicecidrs_controller_test.go @@ -428,11 +428,6 @@ func TestController_canDeleteCIDR(t *testing.T) { t.Run(tc.name, func(t *testing.T) { tCtx := ktesting.Init(t) _, controller := newController(tCtx, t, tc.cidrs, tc.ips) - err := controller.syncCIDRs() - if err != nil { - t.Fatal(err) - } - got, err := controller.canDeleteCIDR(tCtx, tc.cidrSynced) if err != nil { t.Fatal(err) @@ -534,10 +529,6 @@ func TestController_ipToCidrs(t *testing.T) { t.Run(tt.name, func(t *testing.T) { tCtx := ktesting.Init(t) _, controller := newController(tCtx, t, tt.cidrs, nil) - err := controller.syncCIDRs() - if err != nil { - t.Fatal(err) - } if got := controller.containingServiceCIDRs(tt.ip); !cmp.Equal(got, tt.want, cmpopts.SortSlices(func(a, b string) bool { return a < b })) { t.Errorf("Controller.ipToCidrs() = %v, want %v", got, tt.want) } @@ -591,10 +582,6 @@ func TestController_cidrToCidrs(t *testing.T) { t.Run(tt.name, func(t *testing.T) { tCtx := ktesting.Init(t) _, controller := newController(tCtx, t, tt.cidrs, nil) - err := controller.syncCIDRs() - if err != nil { - t.Fatal(err) - } if got := controller.overlappingServiceCIDRs(tt.cidr); !cmp.Equal(got, tt.want, cmpopts.SortSlices(func(a, b string) bool { return a < b })) { t.Errorf("Controller.cidrToCidrs() = %v, want %v", got, tt.want) }