From 54b6a416fba22f193dafccac16011ce9fed967b7 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Thu, 1 Jul 2021 23:01:36 -0700 Subject: [PATCH] Service REST test: better IP and port alloc checks --- pkg/api/service/testing/make.go | 1 + .../core/service/ipallocator/allocator.go | 2 - .../core/service/portallocator/allocator.go | 2 - .../core/service/storage/rest_test.go | 70 ++++++++++++------- 4 files changed, 46 insertions(+), 29 deletions(-) diff --git a/pkg/api/service/testing/make.go b/pkg/api/service/testing/make.go index a3e2b7b3883..bb4d2b8c761 100644 --- a/pkg/api/service/testing/make.go +++ b/pkg/api/service/testing/make.go @@ -62,6 +62,7 @@ func SetTypeClusterIP(svc *api.Service) { svc.Spec.Ports[i].NodePort = 0 } svc.Spec.ExternalName = "" + svc.Spec.ExternalTrafficPolicy = "" } // SetTypeNodePort sets the service type to NodePort and clears other fields. diff --git a/pkg/registry/core/service/ipallocator/allocator.go b/pkg/registry/core/service/ipallocator/allocator.go index e42b3401ca4..7e57dc23d5b 100644 --- a/pkg/registry/core/service/ipallocator/allocator.go +++ b/pkg/registry/core/service/ipallocator/allocator.go @@ -36,8 +36,6 @@ type Interface interface { ForEach(func(net.IP)) CIDR() net.IPNet IPFamily() api.IPFamily - - // For testing Has(ip net.IP) bool } diff --git a/pkg/registry/core/service/portallocator/allocator.go b/pkg/registry/core/service/portallocator/allocator.go index 13eea2d275e..d344aa894da 100644 --- a/pkg/registry/core/service/portallocator/allocator.go +++ b/pkg/registry/core/service/portallocator/allocator.go @@ -34,8 +34,6 @@ type Interface interface { AllocateNext() (int, error) Release(int) error ForEach(func(int)) - - // For testing Has(int) bool } diff --git a/pkg/registry/core/service/storage/rest_test.go b/pkg/registry/core/service/storage/rest_test.go index 1db155effbb..71f91222be6 100644 --- a/pkg/registry/core/service/storage/rest_test.go +++ b/pkg/registry/core/service/storage/rest_test.go @@ -259,7 +259,27 @@ func makeIPNet6(t *testing.T) *net.IPNet { return net } +func ipIsAllocated(t *testing.T, alloc ipallocator.Interface, ipstr string) bool { + t.Helper() + ip := net.ParseIP(ipstr) + if ip == nil { + t.Errorf("error parsing IP %q", ipstr) + return false + } + return alloc.Has(ip) +} + +func portIsAllocated(t *testing.T, alloc portallocator.Interface, port int32) bool { + t.Helper() + if port == 0 { + t.Errorf("port is 0") + return false + } + return alloc.Has(int(port)) +} + func releaseServiceNodePorts(t *testing.T, ctx context.Context, svcName string, rest *REST) { + t.Helper() obj, err := rest.Get(ctx, svcName, &metav1.GetOptions{}) if err != nil { t.Fatalf("Unexpected error: %v", err) @@ -399,7 +419,7 @@ func TestServiceRegistryCreateDryRun(t *testing.T) { for i, family := range tc.svc.Spec.IPFamilies { alloc := storage.serviceIPAllocatorsByFamily[family] - if alloc.Has(net.ParseIP(tc.svc.Spec.ClusterIPs[i])) { + if ipIsAllocated(t, alloc, tc.svc.Spec.ClusterIPs[i]) { t.Errorf("unexpected side effect: ip allocated %v", tc.svc.Spec.ClusterIPs[i]) } } @@ -428,7 +448,7 @@ func TestDryRunNodePort(t *testing.T) { if createdSvc.Spec.Ports[0].NodePort == 0 { t.Errorf("expected NodePort value assigned") } - if storage.serviceNodePorts.Has(int(createdSvc.Spec.Ports[0].NodePort)) { + if portIsAllocated(t, storage.serviceNodePorts, createdSvc.Spec.Ports[0].NodePort) { t.Errorf("unexpected side effect: NodePort allocated") } _, err = getService(storage, ctx, svc.Name, &metav1.GetOptions{}) @@ -455,7 +475,7 @@ func TestDryRunNodePort(t *testing.T) { t.Errorf("Expected %v, but got %v", expectNodePorts, actualNodePorts) } for i := range svc.Spec.Ports { - if storage.serviceNodePorts.Has(int(svc.Spec.Ports[i].NodePort)) { + if portIsAllocated(t, storage.serviceNodePorts, svc.Spec.Ports[i].NodePort) { t.Errorf("unexpected side effect: NodePort allocated") } } @@ -635,43 +655,45 @@ func TestServiceRegistryUpdateDryRun(t *testing.T) { // Test dry run update request external name to node port new1 := svc.DeepCopy() svctest.SetTypeNodePort(new1) - updatedSvc, created, err := storage.Update(ctx, svc.Name, rest.DefaultUpdatedObjectInfo(new1), + svctest.SetNodePorts(30001)(new1) // DryRun does not set port values yet + obj, created, err := storage.Update(ctx, new1.Name, rest.DefaultUpdatedObjectInfo(new1), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{DryRun: []string{metav1.DryRunAll}}) if err != nil { t.Fatalf("Expected no error: %v", err) } - if updatedSvc == nil { + if obj == nil { t.Errorf("Expected non-nil object") } if created { t.Errorf("expected not created") } - if storage.serviceNodePorts.Has(int(svc.Spec.Ports[0].NodePort)) { + if portIsAllocated(t, storage.serviceNodePorts, new1.Spec.Ports[0].NodePort) { t.Errorf("unexpected side effect: NodePort allocated") } // Test dry run update request external name to cluster ip new2 := svc.DeepCopy() svctest.SetTypeClusterIP(new2) + svctest.SetClusterIPs("1.2.3.4")(new2) // DryRun does not set IP values yet _, _, err = storage.Update(ctx, svc.Name, rest.DefaultUpdatedObjectInfo(new2), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{DryRun: []string{metav1.DryRunAll}}) if err != nil { t.Fatalf("Expected no error: %v", err) } - if storage.serviceIPAllocatorsByFamily[storage.defaultServiceIPFamily].Has(net.ParseIP(svc.Spec.ClusterIP)) { + if ipIsAllocated(t, storage.serviceIPAllocatorsByFamily[storage.defaultServiceIPFamily], new2.Spec.ClusterIP) { t.Errorf("unexpected side effect: ip allocated") } // Test dry run update request remove node port - obj, err = storage.Create(ctx, svctest.MakeService("foo2", svctest.SetTypeNodePort), rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) + obj, err = storage.Create(ctx, svctest.MakeService("foo2", svctest.SetTypeNodePort, svctest.SetNodePorts(30001)), rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) if err != nil { t.Fatalf("Expected no error: %v", err) } svc = obj.(*api.Service) - if !storage.serviceIPAllocatorsByFamily[storage.defaultServiceIPFamily].Has(net.ParseIP(svc.Spec.ClusterIP)) { + if !ipIsAllocated(t, storage.serviceIPAllocatorsByFamily[storage.defaultServiceIPFamily], svc.Spec.ClusterIP) { t.Errorf("expected IP to be allocated") } - if !storage.serviceNodePorts.Has(int(svc.Spec.Ports[0].NodePort)) { + if !portIsAllocated(t, storage.serviceNodePorts, svc.Spec.Ports[0].NodePort) { t.Errorf("expected NodePort to be allocated") } @@ -682,12 +704,12 @@ func TestServiceRegistryUpdateDryRun(t *testing.T) { if err != nil { t.Fatalf("Expected no error: %v", err) } - if !storage.serviceNodePorts.Has(int(svc.Spec.Ports[0].NodePort)) { + if !portIsAllocated(t, storage.serviceNodePorts, svc.Spec.Ports[0].NodePort) { t.Errorf("unexpected side effect: NodePort unallocated") } // Test dry run update request remove cluster ip - obj, err = storage.Create(ctx, svctest.MakeService("foo3"), rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) + obj, err = storage.Create(ctx, svctest.MakeService("foo3", svctest.SetClusterIPs("1.2.3.4")), rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) if err != nil { t.Fatalf("expected no error: %v", err) } @@ -700,7 +722,7 @@ func TestServiceRegistryUpdateDryRun(t *testing.T) { if err != nil { t.Fatalf("expected no error: %v", err) } - if !storage.serviceIPAllocatorsByFamily[storage.defaultServiceIPFamily].Has(net.ParseIP(svc.Spec.ClusterIP)) { + if !ipIsAllocated(t, storage.serviceIPAllocatorsByFamily[storage.defaultServiceIPFamily], svc.Spec.ClusterIP) { t.Errorf("unexpected side effect: ip unallocated") } } @@ -862,14 +884,14 @@ func TestServiceRegistryDeleteDryRun(t *testing.T) { if createdSvc.Spec.ClusterIP == "" { t.Fatalf("expected ClusterIP to be set") } - if !storage.serviceIPAllocatorsByFamily[storage.defaultServiceIPFamily].Has(net.ParseIP(createdSvc.Spec.ClusterIP)) { + if !ipIsAllocated(t, storage.serviceIPAllocatorsByFamily[storage.defaultServiceIPFamily], createdSvc.Spec.ClusterIP) { t.Errorf("expected ClusterIP to be allocated") } _, _, err = storage.Delete(ctx, svc.Name, rest.ValidateAllObjectFunc, &metav1.DeleteOptions{DryRun: []string{metav1.DryRunAll}}) if err != nil { t.Fatalf("Expected no error: %v", err) } - if !storage.serviceIPAllocatorsByFamily[storage.defaultServiceIPFamily].Has(net.ParseIP(createdSvc.Spec.ClusterIP)) { + if !ipIsAllocated(t, storage.serviceIPAllocatorsByFamily[storage.defaultServiceIPFamily], createdSvc.Spec.ClusterIP) { t.Errorf("unexpected side effect: ip unallocated") } @@ -883,7 +905,7 @@ func TestServiceRegistryDeleteDryRun(t *testing.T) { if createdSvc.Spec.Ports[0].NodePort == 0 { t.Fatalf("expected NodePort to be set") } - if !storage.serviceNodePorts.Has(int(createdSvc.Spec.Ports[0].NodePort)) { + if !portIsAllocated(t, storage.serviceNodePorts, createdSvc.Spec.Ports[0].NodePort) { t.Errorf("expected NodePort to be allocated") } @@ -893,7 +915,7 @@ func TestServiceRegistryDeleteDryRun(t *testing.T) { if err != nil { t.Fatalf("Expected no error: %v", err) } - if !storage.serviceNodePorts.Has(int(createdSvc.Spec.Ports[0].NodePort)) { + if !portIsAllocated(t, storage.serviceNodePorts, createdSvc.Spec.Ports[0].NodePort) { t.Errorf("unexpected side effect: NodePort unallocated") } } @@ -921,7 +943,7 @@ func TestDualStackServiceRegistryDeleteDryRun(t *testing.T) { t.Fatalf("Expected no error: %v", err) } for i, family := range dualstack_svc.Spec.IPFamilies { - if !dualstack_storage.serviceIPAllocatorsByFamily[family].Has(net.ParseIP(dualstack_svc.Spec.ClusterIPs[i])) { + if !ipIsAllocated(t, dualstack_storage.serviceIPAllocatorsByFamily[family], dualstack_svc.Spec.ClusterIPs[i]) { t.Errorf("unexpected side effect: ip unallocated %v", dualstack_svc.Spec.ClusterIPs[i]) } } @@ -1225,7 +1247,7 @@ func TestServiceRegistryIPAllocation(t *testing.T) { testIPs := []string{"1.2.3.93", "1.2.3.94", "1.2.3.95", "1.2.3.96"} testIP := "not-an-ip" for _, ip := range testIPs { - if !storage.serviceIPAllocatorsByFamily[storage.defaultServiceIPFamily].(*ipallocator.Range).Has(net.ParseIP(ip)) { + if !ipIsAllocated(t, storage.serviceIPAllocatorsByFamily[storage.defaultServiceIPFamily].(*ipallocator.Range), ip) { testIP = ip break } @@ -1314,7 +1336,7 @@ func TestServiceRegistryIPUpdate(t *testing.T) { testIPs := []string{"1.2.3.93", "1.2.3.94", "1.2.3.95", "1.2.3.96"} testIP := "" for _, ip := range testIPs { - if !storage.serviceIPAllocatorsByFamily[storage.defaultServiceIPFamily].(*ipallocator.Range).Has(net.ParseIP(ip)) { + if !ipIsAllocated(t, storage.serviceIPAllocatorsByFamily[storage.defaultServiceIPFamily].(*ipallocator.Range), ip) { testIP = ip break } @@ -1660,8 +1682,7 @@ func TestInitClusterIP(t *testing.T) { family = api.IPv6Protocol } allocator := storage.serviceIPAllocatorsByFamily[family] - // has retruns true if it was allocated *sigh*.. - if !allocator.Has(net.ParseIP(ip)) { + if !ipIsAllocated(t, allocator, ip) { t.Fatalf("expected ip:%v to be allocated by %v allocator. it was not", ip, family) } } @@ -2107,8 +2128,7 @@ func TestServiceUpgrade(t *testing.T) { for i, family := range updatedSvc.Spec.IPFamilies { ip := updatedSvc.Spec.ClusterIPs[i] allocator := storage.serviceIPAllocatorsByFamily[family] - // has retruns true if it was allocated *sigh*.. - if !allocator.Has(net.ParseIP(ip)) { + if !ipIsAllocated(t, allocator, ip) { t.Fatalf("expected ip:%v to be allocated by %v allocator. it was not", ip, family) } } @@ -2241,7 +2261,7 @@ func TestServiceDowngrade(t *testing.T) { releasedIPFamily := copySvc.Spec.IPFamilies[1] allocator := storage.serviceIPAllocatorsByFamily[releasedIPFamily] - if allocator.Has(net.ParseIP(releasedIP)) { + if ipIsAllocated(t, allocator, releasedIP) { t.Fatalf("expected ip:%v to be released by %v allocator. it was not", releasedIP, releasedIPFamily) } }