From 8de14b526e96ad6bca781e83fecedba082f141e5 Mon Sep 17 00:00:00 2001 From: Daman Arora Date: Sun, 13 Jul 2025 23:31:04 +0530 Subject: [PATCH 1/2] service-ip-alloc: make repair loop consumer clock interface The private constructor for the repair loop now consumes clock.Clock interface allowing predictable unit testing. Signed-off-by: Daman Arora --- .../ipallocator/controller/repairip.go | 14 +- .../ipallocator/controller/repairip_test.go | 217 +++++++++++++----- 2 files changed, 168 insertions(+), 63 deletions(-) diff --git a/pkg/registry/core/service/ipallocator/controller/repairip.go b/pkg/registry/core/service/ipallocator/controller/repairip.go index c7669210384..bb7c2d877ec 100644 --- a/pkg/registry/core/service/ipallocator/controller/repairip.go +++ b/pkg/registry/core/service/ipallocator/controller/repairip.go @@ -108,13 +108,23 @@ type RepairIPAddress struct { clock clock.Clock } -// NewRepair creates a controller that periodically ensures that all clusterIPs are uniquely allocated across the cluster +// NewRepairIPAddress creates a controller that periodically ensures that all clusterIPs are uniquely allocated across the cluster // and generates informational warnings for a cluster that is not in sync. func NewRepairIPAddress(interval time.Duration, client kubernetes.Interface, serviceInformer coreinformers.ServiceInformer, serviceCIDRInformer networkinginformers.ServiceCIDRInformer, ipAddressInformer networkinginformers.IPAddressInformer) *RepairIPAddress { + return newRepairIPAddress(interval, client, serviceInformer, serviceCIDRInformer, ipAddressInformer, clock.RealClock{}) +} + +// newRepairIPAddress implements NewRepairIPAddress by additionally consuming clock.Clock. +func newRepairIPAddress(interval time.Duration, + client kubernetes.Interface, + serviceInformer coreinformers.ServiceInformer, + serviceCIDRInformer networkinginformers.ServiceCIDRInformer, + ipAddressInformer networkinginformers.IPAddressInformer, + c clock.Clock) *RepairIPAddress { eventBroadcaster := events.NewBroadcaster(&events.EventSinkImpl{Interface: client.EventsV1()}) recorder := eventBroadcaster.NewRecorder(legacyscheme.Scheme, "ipallocator-repair-controller") @@ -138,7 +148,7 @@ func NewRepairIPAddress(interval time.Duration, workerLoopPeriod: time.Second, broadcaster: eventBroadcaster, recorder: recorder, - clock: clock.RealClock{}, + clock: c, } _, _ = serviceInformer.Informer().AddEventHandlerWithResyncPeriod(cache.ResourceEventHandlerFuncs{ diff --git a/pkg/registry/core/service/ipallocator/controller/repairip_test.go b/pkg/registry/core/service/ipallocator/controller/repairip_test.go index ccee3666c5d..f716b6035c1 100644 --- a/pkg/registry/core/service/ipallocator/controller/repairip_test.go +++ b/pkg/registry/core/service/ipallocator/controller/repairip_test.go @@ -33,11 +33,15 @@ import ( "k8s.io/client-go/tools/cache" "k8s.io/client-go/tools/events" "k8s.io/kubernetes/pkg/registry/core/service/ipallocator" + testclock "k8s.io/utils/clock/testing" ) var ( serviceCIDRv4 = "10.0.0.0/16" serviceCIDRv6 = "2001:db8::/64" + + // testTimeNow is used to lock the fake clocks view of the current time. + testTimeNow = time.Date(2012, 1, 1, 0, 0, 0, 0, time.UTC) ) type fakeRepair struct { @@ -47,7 +51,7 @@ type fakeRepair struct { serviceCIDRStore cache.Store } -func newFakeRepair() (*fake.Clientset, *fakeRepair) { +func newFakeRepair(testTime time.Time) (*fake.Clientset, *fakeRepair) { fakeClient := fake.NewSimpleClientset() informerFactory := informers.NewSharedInformerFactory(fakeClient, 0*time.Second) @@ -74,12 +78,13 @@ func newFakeRepair() (*fake.Clientset, *fakeRepair) { err := ipIndexer.Delete(ip) return false, &networkingv1.IPAddress{}, err })) - - r := NewRepairIPAddress(0*time.Second, + fakeClock := testclock.NewFakeClock(testTime) + r := newRepairIPAddress(0*time.Second, fakeClient, serviceInformer, serviceCIDRInformer, ipInformer, + fakeClock, ) return fakeClient, &fakeRepair{r, serviceIndexer, ipIndexer, serviceCIDRIndexer} } @@ -90,48 +95,58 @@ func TestRepairServiceIP(t *testing.T) { svcs []*v1.Service ipAddresses []*networkingv1.IPAddress cidrs []*networkingv1.ServiceCIDR + testTime time.Time expectedIPs []string actions [][]string // verb and resource events []string }{ { name: "no changes needed single stack", - svcs: []*v1.Service{newService("test-svc", []string{"10.0.1.1"})}, + svcs: []*v1.Service{ + newServiceWithCreationTimestamp("test-svc", []string{"10.0.1.1"}, testTimeNow.Add(1*time.Second)), + }, ipAddresses: []*networkingv1.IPAddress{ - newIPAddress("10.0.1.1", newService("test-svc", []string{"10.0.1.1"})), + newIPAddressWithCreationTimestamp("10.0.1.1", newService("test-svc", []string{"10.0.1.1"}), testTimeNow), }, cidrs: []*networkingv1.ServiceCIDR{ newServiceCIDR("kubernetes", serviceCIDRv4, serviceCIDRv6), }, + testTime: testTimeNow, expectedIPs: []string{"10.0.1.1"}, actions: [][]string{}, events: []string{}, }, { name: "no changes needed dual stack", - svcs: []*v1.Service{newService("test-svc", []string{"10.0.1.1", "2001:db8::10"})}, + svcs: []*v1.Service{ + newServiceWithCreationTimestamp("test-svc", []string{"10.0.1.1", "2001:db8::10"}, testTimeNow.Add(1*time.Second)), + }, ipAddresses: []*networkingv1.IPAddress{ - newIPAddress("10.0.1.1", newService("test-svc", []string{"10.0.1.1"})), - newIPAddress("2001:db8::10", newService("test-svc", []string{"2001:db8::10"})), + newIPAddressWithCreationTimestamp("10.0.1.1", newService("test-svc", []string{"10.0.1.1"}), testTimeNow), + newIPAddressWithCreationTimestamp("2001:db8::10", newService("test-svc", []string{"2001:db8::10"}), testTimeNow), }, cidrs: []*networkingv1.ServiceCIDR{ newServiceCIDR("kubernetes", serviceCIDRv4, serviceCIDRv6), }, + testTime: testTimeNow, expectedIPs: []string{"10.0.1.1", "2001:db8::10"}, actions: [][]string{}, events: []string{}, }, { name: "no changes needed dual stack multiple cidrs", - svcs: []*v1.Service{newService("test-svc", []string{"192.168.0.1", "2001:db8:a:b::10"})}, + svcs: []*v1.Service{ + newServiceWithCreationTimestamp("test-svc", []string{"192.168.0.1", "2001:db8:a:b::10"}, testTimeNow.Add(1*time.Second)), + }, ipAddresses: []*networkingv1.IPAddress{ - newIPAddress("192.168.0.1", newService("test-svc", []string{"192.168.0.1"})), - newIPAddress("2001:db8:a:b::10", newService("test-svc", []string{"2001:db8:a:b::10"})), + newIPAddressWithCreationTimestamp("192.168.0.1", newService("test-svc", []string{"192.168.0.1"}), testTimeNow), + newIPAddressWithCreationTimestamp("2001:db8:a:b::10", newService("test-svc", []string{"2001:db8:a:b::10"}), testTimeNow), }, cidrs: []*networkingv1.ServiceCIDR{ newServiceCIDR("kubernetes", serviceCIDRv4, serviceCIDRv6), newServiceCIDR("custom", "192.168.0.0/24", "2001:db8:a:b::/64"), }, + testTime: testTimeNow, expectedIPs: []string{"192.168.0.1", "2001:db8:a:b::10"}, actions: [][]string{}, events: []string{}, @@ -139,20 +154,26 @@ func TestRepairServiceIP(t *testing.T) { // these two cases simulate migrating from bitmaps to IPAddress objects { name: "create IPAddress single stack", - svcs: []*v1.Service{newService("test-svc", []string{"10.0.1.1"})}, + svcs: []*v1.Service{ + newServiceWithCreationTimestamp("test-svc", []string{"10.0.1.1"}, testTimeNow.Add(1*time.Second)), + }, cidrs: []*networkingv1.ServiceCIDR{ newServiceCIDR("kubernetes", serviceCIDRv4, serviceCIDRv6), }, + testTime: testTimeNow, expectedIPs: []string{"10.0.1.1"}, actions: [][]string{{"create", "ipaddresses"}}, events: []string{"Warning ClusterIPNotAllocated Cluster IP [IPv4]: 10.0.1.1 is not allocated; repairing"}, }, { name: "create IPAddresses dual stack", - svcs: []*v1.Service{newService("test-svc", []string{"10.0.1.1", "2001:db8::10"})}, + svcs: []*v1.Service{ + newServiceWithCreationTimestamp("test-svc", []string{"10.0.1.1", "2001:db8::10"}, testTimeNow.Add(1*time.Second)), + }, cidrs: []*networkingv1.ServiceCIDR{ newServiceCIDR("kubernetes", serviceCIDRv4, serviceCIDRv6), }, + testTime: testTimeNow, expectedIPs: []string{"10.0.1.1", "2001:db8::10"}, actions: [][]string{{"create", "ipaddresses"}, {"create", "ipaddresses"}}, events: []string{ @@ -162,38 +183,46 @@ func TestRepairServiceIP(t *testing.T) { }, { name: "create IPAddress single stack from secondary", - svcs: []*v1.Service{newService("test-svc", []string{"192.168.1.1"})}, + svcs: []*v1.Service{ + newServiceWithCreationTimestamp("test-svc", []string{"192.168.1.1"}, testTimeNow.Add(1*time.Second)), + }, cidrs: []*networkingv1.ServiceCIDR{ newServiceCIDR("kubernetes", serviceCIDRv4, serviceCIDRv6), newServiceCIDR("custom", "192.168.1.0/24", ""), }, + testTime: testTimeNow, expectedIPs: []string{"192.168.1.1"}, actions: [][]string{{"create", "ipaddresses"}}, events: []string{"Warning ClusterIPNotAllocated Cluster IP [IPv4]: 192.168.1.1 is not allocated; repairing"}, }, { name: "reconcile IPAddress single stack wrong reference", - svcs: []*v1.Service{newService("test-svc", []string{"10.0.1.1"})}, + svcs: []*v1.Service{ + newServiceWithCreationTimestamp("test-svc", []string{"10.0.1.1"}, testTimeNow.Add(1*time.Second)), + }, ipAddresses: []*networkingv1.IPAddress{ - newIPAddress("10.0.1.1", newService("test-svc2", []string{"10.0.1.1"})), + newIPAddressWithCreationTimestamp("10.0.1.1", newService("test-svc2", []string{"10.0.1.1"}), testTimeNow), }, cidrs: []*networkingv1.ServiceCIDR{ newServiceCIDR("kubernetes", serviceCIDRv4, serviceCIDRv6), }, + testTime: testTimeNow, expectedIPs: []string{"10.0.1.1"}, actions: [][]string{{"delete", "ipaddresses"}, {"create", "ipaddresses"}}, events: []string{"Warning ClusterIPNotAllocated the ClusterIP [IPv4]: 10.0.1.1 for Service bar/test-svc has a wrong reference; repairing"}, }, { name: "reconcile IPAddresses dual stack", - svcs: []*v1.Service{newService("test-svc", []string{"10.0.1.1", "2001:db8::10"})}, + svcs: []*v1.Service{ + newServiceWithCreationTimestamp("test-svc", []string{"10.0.1.1", "2001:db8::10"}, testTimeNow.Add(-1*time.Second))}, ipAddresses: []*networkingv1.IPAddress{ - newIPAddress("10.0.1.1", newService("test-svc2", []string{"10.0.1.1"})), - newIPAddress("2001:db8::10", newService("test-svc2", []string{"2001:db8::10"})), + newIPAddressWithCreationTimestamp("10.0.1.1", newService("test-svc2", []string{"10.0.1.1"}), testTimeNow), + newIPAddressWithCreationTimestamp("2001:db8::10", newService("test-svc2", []string{"2001:db8::10"}), testTimeNow), }, cidrs: []*networkingv1.ServiceCIDR{ newServiceCIDR("kubernetes", serviceCIDRv4, serviceCIDRv6), }, + testTime: testTimeNow, expectedIPs: []string{"10.0.1.1", "2001:db8::10"}, actions: [][]string{{"delete", "ipaddresses"}, {"create", "ipaddresses"}, {"delete", "ipaddresses"}, {"create", "ipaddresses"}}, events: []string{ @@ -203,123 +232,167 @@ func TestRepairServiceIP(t *testing.T) { }, { name: "one IP out of range", - svcs: []*v1.Service{newService("test-svc", []string{"192.168.1.1", "2001:db8::10"})}, + svcs: []*v1.Service{ + newServiceWithCreationTimestamp("test-svc", []string{"192.168.1.1", "2001:db8::10"}, testTimeNow.Add(1*time.Second)), + }, ipAddresses: []*networkingv1.IPAddress{ - newIPAddress("192.168.1.1", newService("test-svc", []string{"192.168.1.1"})), - newIPAddress("2001:db8::10", newService("test-svc", []string{"2001:db8::10"})), + newIPAddressWithCreationTimestamp("192.168.1.1", newService("test-svc", []string{"192.168.1.1"}), testTimeNow), + newIPAddressWithCreationTimestamp("2001:db8::10", newService("test-svc", []string{"2001:db8::10"}), testTimeNow), }, cidrs: []*networkingv1.ServiceCIDR{ newServiceCIDR("kubernetes", serviceCIDRv4, serviceCIDRv6), }, + testTime: testTimeNow, expectedIPs: []string{"2001:db8::10"}, actions: [][]string{}, events: []string{"Warning ClusterIPOutOfRange Cluster IP [IPv4]: 192.168.1.1 is not within any configured Service CIDR; please recreate service"}, }, { - name: "one IP orphan", + name: "one IP orphan within grace period", ipAddresses: []*networkingv1.IPAddress{ - newIPAddress("10.0.1.1", newService("test-svc", []string{"10.0.1.1"})), + newIPAddressWithCreationTimestamp("10.0.1.1", newService("test-svc", []string{"10.0.1.1"}), testTimeNow), }, cidrs: []*networkingv1.ServiceCIDR{ newServiceCIDR("kubernetes", serviceCIDRv4, serviceCIDRv6), }, - actions: [][]string{{"delete", "ipaddresses"}}, - events: []string{"Warning IPAddressNotAllocated IPAddress: 10.0.1.1 for Service bar/test-svc appears to have leaked: cleaning up"}, + }, + { + name: "one IP orphan after grace period", + ipAddresses: []*networkingv1.IPAddress{ + newIPAddressWithCreationTimestamp("10.0.1.1", newService("test-svc", []string{"10.0.1.1"}), testTimeNow), + }, + cidrs: []*networkingv1.ServiceCIDR{ + newServiceCIDR("kubernetes", serviceCIDRv4, serviceCIDRv6), + }, + // we update the fake clock here simulating the IPAddress was created more than 70 seconds ago, + // this simulates grace period that prevents ip address cleanup is over. + testTime: testTimeNow.Add(70 * time.Second), + actions: [][]string{{"delete", "ipaddresses"}}, + events: []string{"Warning IPAddressNotAllocated IPAddress: 10.0.1.1 for Service bar/test-svc appears to have leaked: cleaning up"}, }, { name: "one IP out of range matching the network address", - svcs: []*v1.Service{newService("test-svc", []string{"10.0.0.0"})}, + svcs: []*v1.Service{ + newServiceWithCreationTimestamp("test-svc", []string{"10.0.0.0"}, testTimeNow.Add(1*time.Second)), + }, ipAddresses: []*networkingv1.IPAddress{ - newIPAddress("10.0.0.0", newService("test-svc", []string{"10.0.0.0"})), + newIPAddressWithCreationTimestamp("10.0.0.0", newService("test-svc", []string{"10.0.0.0"}), testTimeNow), }, cidrs: []*networkingv1.ServiceCIDR{ newServiceCIDR("kubernetes", serviceCIDRv4, serviceCIDRv6), }, + testTime: testTimeNow, expectedIPs: []string{"10.0.0.0"}, actions: [][]string{}, events: []string{"Warning ClusterIPOutOfRange Cluster IP [IPv4]: 10.0.0.0 is not within any configured Service CIDR; please recreate service"}, }, { name: "one IP out of range matching the broadcast address", - svcs: []*v1.Service{newService("test-svc", []string{"10.0.255.255"})}, + svcs: []*v1.Service{ + newServiceWithCreationTimestamp("test-svc", []string{"10.0.255.255"}, testTimeNow.Add(1*time.Second)), + }, ipAddresses: []*networkingv1.IPAddress{ - newIPAddress("10.0.255.255", newService("test-svc", []string{"10.0.255.255"})), + newIPAddressWithCreationTimestamp("10.0.255.255", newService("test-svc", []string{"10.0.255.255"}), testTimeNow), }, cidrs: []*networkingv1.ServiceCIDR{ newServiceCIDR("kubernetes", serviceCIDRv4, serviceCIDRv6), }, + testTime: testTimeNow, expectedIPs: []string{"10.0.255.255"}, actions: [][]string{}, events: []string{"Warning ClusterIPOutOfRange Cluster IP [IPv4]: 10.0.255.255 is not within any configured Service CIDR; please recreate service"}, }, { name: "one IPv6 out of range matching the subnet address", - svcs: []*v1.Service{newService("test-svc", []string{"2001:db8::"})}, + svcs: []*v1.Service{ + newServiceWithCreationTimestamp("test-svc", []string{"2001:db8::"}, testTimeNow.Add(1*time.Second)), + }, ipAddresses: []*networkingv1.IPAddress{ - newIPAddress("2001:db8::", newService("test-svc", []string{"2001:db8::"})), + newIPAddressWithCreationTimestamp("2001:db8::", newService("test-svc", []string{"2001:db8::"}), testTimeNow), }, cidrs: []*networkingv1.ServiceCIDR{ newServiceCIDR("kubernetes", serviceCIDRv4, serviceCIDRv6), }, + testTime: testTimeNow, expectedIPs: []string{"2001:db8::"}, actions: [][]string{}, events: []string{"Warning ClusterIPOutOfRange Cluster IP [IPv6]: 2001:db8:: is not within any configured Service CIDR; please recreate service"}, }, { name: "one IPv6 matching the broadcast address", - svcs: []*v1.Service{newService("test-svc", []string{"2001:db8::ffff:ffff:ffff:ffff"})}, + svcs: []*v1.Service{ + newServiceWithCreationTimestamp("test-svc", []string{"2001:db8::ffff:ffff:ffff:ffff"}, testTimeNow.Add(1*time.Second)), + }, ipAddresses: []*networkingv1.IPAddress{ - newIPAddress("2001:db8::ffff:ffff:ffff:ffff", newService("test-svc", []string{"2001:db8::ffff:ffff:ffff:ffff"})), + newIPAddressWithCreationTimestamp("2001:db8::ffff:ffff:ffff:ffff", newService("test-svc", []string{"2001:db8::ffff:ffff:ffff:ffff"}), testTimeNow), }, cidrs: []*networkingv1.ServiceCIDR{ newServiceCIDR("kubernetes", serviceCIDRv4, serviceCIDRv6), }, + testTime: testTimeNow, expectedIPs: []string{"2001:db8::ffff:ffff:ffff:ffff"}, }, { - name: "one IP orphan matching the broadcast address", + name: "one IP orphan matching the broadcast address within grace period", ipAddresses: []*networkingv1.IPAddress{ - newIPAddress("10.0.255.255", newService("test-svc", []string{"10.0.255.255"})), + newIPAddressWithCreationTimestamp("10.0.255.255", newService("test-svc", []string{"10.0.255.255"}), testTimeNow), }, cidrs: []*networkingv1.ServiceCIDR{ newServiceCIDR("kubernetes", serviceCIDRv4, serviceCIDRv6), }, - actions: [][]string{{"delete", "ipaddresses"}}, - events: []string{"Warning IPAddressNotAllocated IPAddress: 10.0.255.255 for Service bar/test-svc appears to have leaked: cleaning up"}, + testTime: testTimeNow, + }, + { + name: "one IP orphan matching the broadcast address after grace period", + ipAddresses: []*networkingv1.IPAddress{ + // we update the fake clock here simulating the IPAddress was created more than 90 seconds ago, + // this simulates grace period that prevents ip address cleanup is over. + newIPAddressWithCreationTimestamp("10.0.255.255", newService("test-svc", []string{"10.0.255.255"}), testTimeNow), + }, + cidrs: []*networkingv1.ServiceCIDR{ + newServiceCIDR("kubernetes", serviceCIDRv4, serviceCIDRv6), + }, + testTime: testTimeNow.Add(90 * time.Second), + actions: [][]string{{"delete", "ipaddresses"}}, + events: []string{"Warning IPAddressNotAllocated IPAddress: 10.0.255.255 for Service bar/test-svc appears to have leaked: cleaning up"}, }, { name: "Two IPAddresses referencing the same service", - svcs: []*v1.Service{newService("test-svc", []string{"10.0.1.1"})}, + svcs: []*v1.Service{ + newServiceWithCreationTimestamp("test-svc", []string{"10.0.1.1"}, testTimeNow.Add(1*time.Second)), + }, ipAddresses: []*networkingv1.IPAddress{ - newIPAddress("10.0.1.1", newService("test-svc", []string{"10.0.1.1"})), - newIPAddress("10.0.1.2", newService("test-svc", []string{"10.0.1.1"})), + newIPAddressWithCreationTimestamp("10.0.1.1", newService("test-svc", []string{"10.0.1.1"}), testTimeNow), + newIPAddressWithCreationTimestamp("10.0.1.2", newService("test-svc", []string{"10.0.1.1"}), testTimeNow), }, cidrs: []*networkingv1.ServiceCIDR{ newServiceCIDR("kubernetes", serviceCIDRv4, serviceCIDRv6), }, - actions: [][]string{{"delete", "ipaddresses"}}, - events: []string{"Warning IPAddressWrongReference IPAddress: 10.0.1.2 for Service bar/test-svc has a wrong reference; cleaning up"}, + testTime: testTimeNow, + actions: [][]string{{"delete", "ipaddresses"}}, + events: []string{"Warning IPAddressWrongReference IPAddress: 10.0.1.2 for Service bar/test-svc has a wrong reference; cleaning up"}, }, { name: "Two Services with same ClusterIP", svcs: []*v1.Service{ - newService("test-svc", []string{"10.0.1.1"}), - newService("test-svc2", []string{"10.0.1.1"}), + newServiceWithCreationTimestamp("test-svc", []string{"10.0.1.1"}, testTimeNow.Add(1*time.Second)), + newServiceWithCreationTimestamp("test-svc2", []string{"10.0.1.1"}, testTimeNow.Add(1*time.Second)), }, ipAddresses: []*networkingv1.IPAddress{ - newIPAddress("10.0.1.1", newService("test-svc2", []string{"10.0.1.1"})), + newIPAddressWithCreationTimestamp("10.0.1.1", newService("test-svc2", []string{"10.0.1.1"}), testTimeNow), }, cidrs: []*networkingv1.ServiceCIDR{ newServiceCIDR("kubernetes", serviceCIDRv4, serviceCIDRv6), }, - events: []string{"Warning ClusterIPAlreadyAllocated Cluster IP [IPv4]:10.0.1.1 was assigned to multiple services; please recreate service"}, + testTime: testTimeNow, + events: []string{"Warning ClusterIPAlreadyAllocated Cluster IP [IPv4]:10.0.1.1 was assigned to multiple services; please recreate service"}, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - c, r := newFakeRepair() + c, r := newFakeRepair(test.testTime) // add cidrs for _, cidr := range test.cidrs { err := r.serviceCIDRStore.Add(cidr) @@ -342,7 +415,6 @@ func TestRepairServiceIP(t *testing.T) { } for _, ip := range test.ipAddresses { - ip.CreationTimestamp = metav1.Date(2012, 1, 1, 0, 0, 0, 0, time.UTC) err := r.ipAddressStore.Add(ip) if err != nil { t.Errorf("Unexpected error trying to add IPAddress %s object: %v", ip, err) @@ -369,10 +441,11 @@ func TestRepairServiceIP(t *testing.T) { func TestRepairIPAddress_syncIPAddress(t *testing.T) { tests := []struct { - name string - ip *networkingv1.IPAddress - actions [][]string // verb and resource - wantErr bool + name string + ip *networkingv1.IPAddress + testTime time.Time + actions [][]string // verb and resource + wantErr bool }{ { name: "correct ipv4 address", @@ -383,7 +456,7 @@ func TestRepairIPAddress_syncIPAddress(t *testing.T) { networkingv1.LabelIPAddressFamily: string(v1.IPv4Protocol), networkingv1.LabelManagedBy: ipallocator.ControllerName, }, - CreationTimestamp: metav1.Date(2012, 1, 1, 0, 0, 0, 0, time.UTC), + CreationTimestamp: metav1.Time{Time: testTimeNow}, }, Spec: networkingv1.IPAddressSpec{ ParentRef: &networkingv1.ParentReference{ @@ -394,6 +467,7 @@ func TestRepairIPAddress_syncIPAddress(t *testing.T) { }, }, }, + testTime: testTimeNow, }, { name: "correct ipv6 address", @@ -404,7 +478,7 @@ func TestRepairIPAddress_syncIPAddress(t *testing.T) { networkingv1.LabelIPAddressFamily: string(v1.IPv6Protocol), networkingv1.LabelManagedBy: ipallocator.ControllerName, }, - CreationTimestamp: metav1.Date(2012, 1, 1, 0, 0, 0, 0, time.UTC), + CreationTimestamp: metav1.Time{Time: testTimeNow}, }, Spec: networkingv1.IPAddressSpec{ ParentRef: &networkingv1.ParentReference{ @@ -415,6 +489,7 @@ func TestRepairIPAddress_syncIPAddress(t *testing.T) { }, }, }, + testTime: testTimeNow, }, { name: "not managed by this controller", @@ -425,7 +500,7 @@ func TestRepairIPAddress_syncIPAddress(t *testing.T) { networkingv1.LabelIPAddressFamily: string(v1.IPv6Protocol), networkingv1.LabelManagedBy: "controller-foo", }, - CreationTimestamp: metav1.Date(2012, 1, 1, 0, 0, 0, 0, time.UTC), + CreationTimestamp: metav1.Time{Time: testTimeNow}, }, Spec: networkingv1.IPAddressSpec{ ParentRef: &networkingv1.ParentReference{ @@ -436,6 +511,7 @@ func TestRepairIPAddress_syncIPAddress(t *testing.T) { }, }, }, + testTime: testTimeNow, }, { name: "out of range", @@ -446,7 +522,7 @@ func TestRepairIPAddress_syncIPAddress(t *testing.T) { networkingv1.LabelIPAddressFamily: string(v1.IPv6Protocol), networkingv1.LabelManagedBy: ipallocator.ControllerName, }, - CreationTimestamp: metav1.Date(2012, 1, 1, 0, 0, 0, 0, time.UTC), + CreationTimestamp: metav1.Time{Time: testTimeNow}, }, Spec: networkingv1.IPAddressSpec{ ParentRef: &networkingv1.ParentReference{ @@ -457,6 +533,7 @@ func TestRepairIPAddress_syncIPAddress(t *testing.T) { }, }, }, + testTime: testTimeNow, }, { name: "leaked ip", @@ -467,7 +544,7 @@ func TestRepairIPAddress_syncIPAddress(t *testing.T) { networkingv1.LabelIPAddressFamily: string(v1.IPv6Protocol), networkingv1.LabelManagedBy: ipallocator.ControllerName, }, - CreationTimestamp: metav1.Date(2012, 1, 1, 0, 0, 0, 0, time.UTC), + CreationTimestamp: metav1.Time{Time: testTimeNow}, }, Spec: networkingv1.IPAddressSpec{ ParentRef: &networkingv1.ParentReference{ @@ -478,12 +555,15 @@ func TestRepairIPAddress_syncIPAddress(t *testing.T) { }, }, }, - actions: [][]string{{"delete", "ipaddresses"}}, + // we update the fake clock here simulating the IPAddress was created more than 90 seconds ago, + // this simulates grace period that prevents ip address cleanup is over. + testTime: testTimeNow.Add(90 * time.Second), + actions: [][]string{{"delete", "ipaddresses"}}, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - c, r := newFakeRepair() + c, r := newFakeRepair(tt.testTime) err := r.ipAddressStore.Add(tt.ip) if err != nil { t.Fatal(err) @@ -507,12 +587,27 @@ func TestRepairIPAddress_syncIPAddress(t *testing.T) { } } +func newIPAddressWithCreationTimestamp(name string, svc *v1.Service, timestamp time.Time) *networkingv1.IPAddress { + ipAddress := newIPAddress(name, svc) + ipAddress.CreationTimestamp = metav1.Time{Time: timestamp} + return ipAddress +} + +func newServiceWithCreationTimestamp(name string, ips []string, timestamp time.Time) *v1.Service { + service := newService(name, ips) + service.CreationTimestamp = metav1.Time{Time: timestamp} + return service +} + func newService(name string, ips []string) *v1.Service { if len(ips) == 0 { return nil } svc := &v1.Service{ - ObjectMeta: metav1.ObjectMeta{Namespace: "bar", Name: name}, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "bar", + Name: name, + CreationTimestamp: metav1.Time{Time: time.Now()}}, Spec: v1.ServiceSpec{ ClusterIP: ips[0], ClusterIPs: ips, From fcb595a2df7be94abd0d6a63c1b4c9bd16c163f8 Mon Sep 17 00:00:00 2001 From: Daman Arora Date: Sun, 13 Jul 2025 23:59:30 +0530 Subject: [PATCH 2/2] service-ip-alloc: delay ip processing on service recreate Service storage implements transactions. It creates an IPAddress object first and then creates the Service object, and if the Service object already exists the complete transaction is reverted. There can be race conditions when the repair loop picks up the new IPAddress object for reconciliation before the transaction is reverted. This leads to spurious IPAddressWrongReference warnings, to suppress these warnings we delay the processing of the new IPAddress object by 5 seconds. The service allocation creates the IPAddress object before creating the Service object, we easily identify this scenario when the IPAddress object creation timestamp is after the Service creation timestamp. We do this only when the IPAddress object is created recently in order to avoid indefinitely requeue/delay in IPAddress cleanup if for some reason the service transaction revert fails. Signed-off-by: Daman Arora --- .../ipallocator/controller/repairip.go | 17 ++++++++++ .../ipallocator/controller/repairip_test.go | 34 +++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/pkg/registry/core/service/ipallocator/controller/repairip.go b/pkg/registry/core/service/ipallocator/controller/repairip.go index bb7c2d877ec..feb7fce3076 100644 --- a/pkg/registry/core/service/ipallocator/controller/repairip.go +++ b/pkg/registry/core/service/ipallocator/controller/repairip.go @@ -530,6 +530,23 @@ func (r *RepairIPAddress) syncIPAddress(key string) error { return nil } } + + // Service storage implements transactions. It creates an IPAddress object first and then creates + // the Service object, and if the Service object already exists the complete transaction is + // reverted. There can be race conditions when the repair loop picks up the new IPAddress object + // for reconciliation before the transaction is reverted. This leads to spurious + // IPAddressWrongReference warnings, to suppress these warnings we delay the processing of the new + // IPAddress object by 5 seconds. The service allocation creates the IPAddress object before creating + // the Service object, we easily identify this scenario when the IPAddress object creation timestamp + // is after the Service creation timestamp. We do this only when the IPAddress object is created + // recently in order to avoid indefinitely requeue/delay in IPAddress cleanup if for some reason + // the service transaction revert fails. + if ipAddress.CreationTimestamp.After(svc.CreationTimestamp.Time) && + r.clock.Now().Sub(ipAddress.CreationTimestamp.Time) < 5*time.Second { + // requeue after the grace period + r.ipQueue.AddAfter(key, 5*time.Second) + return nil + } runtime.HandleError(fmt.Errorf("the IPAddress: %s for Service %s/%s has a wrong reference %#v; cleaning up", ipAddress.Name, svc.Name, svc.Namespace, ipAddress.Spec.ParentRef)) r.recorder.Eventf(ipAddress, nil, v1.EventTypeWarning, "IPAddressWrongReference", "IPAddressAllocation", "IPAddress: %s for Service %s/%s has a wrong reference; cleaning up", ipAddress.Name, svc.Namespace, svc.Name) err = r.client.NetworkingV1().IPAddresses().Delete(context.Background(), ipAddress.Name, metav1.DeleteOptions{}) diff --git a/pkg/registry/core/service/ipallocator/controller/repairip_test.go b/pkg/registry/core/service/ipallocator/controller/repairip_test.go index f716b6035c1..1e44687f025 100644 --- a/pkg/registry/core/service/ipallocator/controller/repairip_test.go +++ b/pkg/registry/core/service/ipallocator/controller/repairip_test.go @@ -372,6 +372,40 @@ func TestRepairServiceIP(t *testing.T) { actions: [][]string{{"delete", "ipaddresses"}}, events: []string{"Warning IPAddressWrongReference IPAddress: 10.0.1.2 for Service bar/test-svc has a wrong reference; cleaning up"}, }, + { + name: "Two IPAddresses referencing the same service when service create is called again and ip address is in grace period", + svcs: []*v1.Service{ + newServiceWithCreationTimestamp("test-svc", []string{"10.0.1.1"}, testTimeNow.Add(1*time.Second)), + }, + ipAddresses: []*networkingv1.IPAddress{ + newIPAddressWithCreationTimestamp("10.0.1.1", newService("test-svc", []string{"10.0.1.1"}), testTimeNow), + // ensure the second IPAddress object has a creation timestamp greater than service creation timestamp + newIPAddressWithCreationTimestamp("10.0.1.2", newService("test-svc", []string{"10.0.1.1"}), testTimeNow.Add(2*time.Second)), + }, + cidrs: []*networkingv1.ServiceCIDR{ + newServiceCIDR("kubernetes", serviceCIDRv4, serviceCIDRv6), + }, + // update the test time, ensuring 5 seconds have not passed after creation of the second IPAddress Object + testTime: testTimeNow.Add(3 * time.Second), + }, + { + name: "Two IPAddresses referencing the same service when service create is called again and ip address is not in grace period", + svcs: []*v1.Service{ + newServiceWithCreationTimestamp("test-svc", []string{"10.0.1.1"}, testTimeNow.Add(1*time.Second)), + }, + ipAddresses: []*networkingv1.IPAddress{ + newIPAddressWithCreationTimestamp("10.0.1.1", newService("test-svc", []string{"10.0.1.1"}), testTimeNow), + // ensure the second IPAddress object has a creation timestamp greater than service creation timestamp + newIPAddressWithCreationTimestamp("10.0.1.2", newService("test-svc", []string{"10.0.1.1"}), testTimeNow.Add(2*time.Second)), + }, + cidrs: []*networkingv1.ServiceCIDR{ + newServiceCIDR("kubernetes", serviceCIDRv4, serviceCIDRv6), + }, + // update the test time, ensuring 5 seconds have passed after creation of the second IPAddress Object + testTime: testTimeNow.Add(10 * time.Second), + actions: [][]string{{"delete", "ipaddresses"}}, + events: []string{"Warning IPAddressWrongReference IPAddress: 10.0.1.2 for Service bar/test-svc has a wrong reference; cleaning up"}, + }, { name: "Two Services with same ClusterIP", svcs: []*v1.Service{