diff --git a/pkg/registry/core/service/ipallocator/controller/repairip.go b/pkg/registry/core/service/ipallocator/controller/repairip.go index c7669210384..feb7fce3076 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{ @@ -520,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 ccee3666c5d..1e44687f025 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,201 @@ 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 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{ - 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 +449,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 +475,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 +490,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 +501,7 @@ func TestRepairIPAddress_syncIPAddress(t *testing.T) { }, }, }, + testTime: testTimeNow, }, { name: "correct ipv6 address", @@ -404,7 +512,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 +523,7 @@ func TestRepairIPAddress_syncIPAddress(t *testing.T) { }, }, }, + testTime: testTimeNow, }, { name: "not managed by this controller", @@ -425,7 +534,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 +545,7 @@ func TestRepairIPAddress_syncIPAddress(t *testing.T) { }, }, }, + testTime: testTimeNow, }, { name: "out of range", @@ -446,7 +556,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 +567,7 @@ func TestRepairIPAddress_syncIPAddress(t *testing.T) { }, }, }, + testTime: testTimeNow, }, { name: "leaked ip", @@ -467,7 +578,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 +589,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 +621,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,