From a957f63ec591426a1786c41d11507251c85dea20 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Sat, 10 Jul 2021 13:59:59 -0700 Subject: [PATCH] Svc REST: HealthCheckNodePort tests This commit ports the ExternalTrafficPolicy and HealthCheckNodePort tests from rest_test to storage_test. It's not a direct port, though. I have added more cases (much more exhaustive) and more assertions. --- .../core/service/storage/rest_test.go | 105 --------- .../core/service/storage/storage_test.go | 219 ++++++++++++++++++ 2 files changed, 219 insertions(+), 105 deletions(-) diff --git a/pkg/registry/core/service/storage/rest_test.go b/pkg/registry/core/service/storage/rest_test.go index 37fc6327cae..d3e14bb9a3e 100644 --- a/pkg/registry/core/service/storage/rest_test.go +++ b/pkg/registry/core/service/storage/rest_test.go @@ -41,7 +41,6 @@ import ( utilfeature "k8s.io/apiserver/pkg/util/feature" featuregatetesting "k8s.io/component-base/featuregate/testing" epstest "k8s.io/kubernetes/pkg/api/endpoints/testing" - "k8s.io/kubernetes/pkg/api/service" svctest "k8s.io/kubernetes/pkg/api/service/testing" api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/features" @@ -1185,110 +1184,6 @@ func TestServiceRegistryIPUpdate(t *testing.T) { } } -// Validate allocation of a nodePort when ExternalTrafficPolicy is set to Local -// and type is LoadBalancer. -func TestServiceRegistryCreateExternalTrafficHealthCheckNodePortAllocation(t *testing.T) { - ctx := genericapirequest.NewDefaultContext() - storage, server := NewTestREST(t, []api.IPFamily{api.IPv4Protocol}) - defer server.Terminate(t) - svc := svctest.MakeService("external-lb-esipp", - svctest.SetTypeLoadBalancer, - func(s *api.Service) { - s.Spec.ExternalTrafficPolicy = api.ServiceExternalTrafficPolicyTypeLocal - }, - ) - obj, err := storage.Create(ctx, svc, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) - if obj == nil || err != nil { - t.Errorf("Unexpected failure creating service %v", err) - } - - createdSvc := obj.(*api.Service) - if !service.NeedsHealthCheck(createdSvc) { - t.Errorf("Expecting health check needed, returned health check not needed instead") - } - port := createdSvc.Spec.HealthCheckNodePort - if port == 0 { - t.Errorf("Failed to allocate health check node port and set the HealthCheckNodePort") - } -} - -// Validate using the user specified nodePort when ExternalTrafficPolicy is set to Local -// and type is LoadBalancer. -func TestServiceRegistryExternalTrafficHealthCheckNodePortUserAllocation(t *testing.T) { - ctx := genericapirequest.NewDefaultContext() - storage, server := NewTestREST(t, []api.IPFamily{api.IPv4Protocol}) - defer server.Terminate(t) - svc := svctest.MakeService("external-lb-esipp", - svctest.SetTypeLoadBalancer, - func(s *api.Service) { - // hard-code NodePort to make sure it doesn't conflict with the healthport. - // TODO: remove this once http://issue.k8s.io/93922 fixes auto-allocation conflicting with user-specified health check ports - s.Spec.Ports[0].NodePort = 30500 - s.Spec.ExternalTrafficPolicy = api.ServiceExternalTrafficPolicyTypeLocal - s.Spec.HealthCheckNodePort = 30501 - }, - ) - obj, err := storage.Create(ctx, svc, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) - if obj == nil || err != nil { - t.Fatalf("Unexpected failure creating service :%v", err) - } - - createdSvc := obj.(*api.Service) - if !service.NeedsHealthCheck(createdSvc) { - t.Errorf("Expecting health check needed, returned health check not needed instead") - } - port := createdSvc.Spec.HealthCheckNodePort - if port == 0 { - t.Errorf("Failed to allocate health check node port and set the HealthCheckNodePort") - } - if port != 30501 { - t.Errorf("Failed to allocate requested nodePort expected %d, got %d", 30501, port) - } -} - -// Validate that the service creation fails when the requested port number is -1. -func TestServiceRegistryCreateExternalTrafficHealthCheckNodePortNegative(t *testing.T) { - ctx := genericapirequest.NewDefaultContext() - storage, server := NewTestREST(t, []api.IPFamily{api.IPv4Protocol}) - defer server.Terminate(t) - svc := svctest.MakeService("external-lb-esipp", svctest.SetTypeLoadBalancer, func(s *api.Service) { - s.Spec.ExternalTrafficPolicy = api.ServiceExternalTrafficPolicyTypeLocal - s.Spec.HealthCheckNodePort = int32(-1) - }) - obj, err := storage.Create(ctx, svc, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) - if obj == nil || err != nil { - return - } - t.Errorf("Unexpected creation of service with invalid HealthCheckNodePort specified") -} - -// Validate that the health check nodePort is not allocated when ExternalTrafficPolicy is set to Global. -func TestServiceRegistryCreateExternalTrafficGlobal(t *testing.T) { - ctx := genericapirequest.NewDefaultContext() - storage, server := NewTestREST(t, []api.IPFamily{api.IPv4Protocol}) - defer server.Terminate(t) - svc := svctest.MakeService("external-lb-esipp", - svctest.SetTypeLoadBalancer, - func(s *api.Service) { - s.Spec.ExternalTrafficPolicy = api.ServiceExternalTrafficPolicyTypeCluster - }, - ) - obj, err := storage.Create(ctx, svc, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) - if obj == nil || err != nil { - t.Errorf("Unexpected failure creating service %v", err) - } - - createdSvc := obj.(*api.Service) - if service.NeedsHealthCheck(createdSvc) { - t.Errorf("Expecting health check not needed, returned health check needed instead") - } - // Make sure the service does not have the health check node port allocated - port := createdSvc.Spec.HealthCheckNodePort - if port != 0 { - t.Errorf("Unexpected allocation of health check node port: %v", port) - } -} - // Validate the internalTrafficPolicy field when set to "Cluster" then updated to "Local" func TestServiceRegistryInternalTrafficPolicyClusterThenLocal(t *testing.T) { ctx := genericapirequest.NewDefaultContext() diff --git a/pkg/registry/core/service/storage/storage_test.go b/pkg/registry/core/service/storage/storage_test.go index 52cfc42e210..c5933c30d7e 100644 --- a/pkg/registry/core/service/storage/storage_test.go +++ b/pkg/registry/core/service/storage/storage_test.go @@ -5126,6 +5126,225 @@ func TestCreateInitNodePorts(t *testing.T) { } } +func TestCreateExternalTrafficPolicy(t *testing.T) { + testCases := []struct { + name string + svc *api.Service + expectError bool + expectHCNP bool + }{{ + name: "ExternalName_policy:none_hcnp:none", + svc: svctest.MakeService("foo", + svctest.SetTypeExternalName, + svctest.SetExternalTrafficPolicy("")), + expectHCNP: false, + }, { + name: "ExternalName_policy:none_hcnp:specified", + svc: svctest.MakeService("foo", + svctest.SetTypeExternalName, + svctest.SetExternalTrafficPolicy(""), + svctest.SetHealthCheckNodePort(30000)), + expectError: true, + }, { + name: "ExternalName_policy:Cluster_hcnp:none", + svc: svctest.MakeService("foo", + svctest.SetTypeExternalName, + svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeCluster)), + expectError: true, + }, { + name: "ExternalName_policy:Cluster_hcnp:specified", + svc: svctest.MakeService("foo", + svctest.SetTypeExternalName, + svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeCluster), + svctest.SetHealthCheckNodePort(30000)), + expectError: true, + }, { + name: "ExternalName_policy:Local_hcnp:none", + svc: svctest.MakeService("foo", + svctest.SetTypeExternalName, + svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal)), + expectError: true, + }, { + name: "ExternalName_policy:Local_hcnp:specified", + svc: svctest.MakeService("foo", + svctest.SetTypeExternalName, + svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal), + svctest.SetHealthCheckNodePort(30000)), + expectError: true, + }, { + name: "ClusterIP_policy:none_hcnp:none", + svc: svctest.MakeService("foo", + svctest.SetTypeClusterIP, + svctest.SetExternalTrafficPolicy("")), + expectHCNP: false, + }, { + name: "ClusterIP_policy:none_hcnp:specified", + svc: svctest.MakeService("foo", + svctest.SetTypeClusterIP, + svctest.SetExternalTrafficPolicy(""), + svctest.SetHealthCheckNodePort(30000)), + expectError: true, + }, { + name: "ClusterIP_policy:Cluster_hcnp:none", + svc: svctest.MakeService("foo", + svctest.SetTypeClusterIP, + svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeCluster)), + expectError: true, + }, { + name: "ClusterIP_policy:Cluster_hcnp:specified", + svc: svctest.MakeService("foo", + svctest.SetTypeClusterIP, + svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeCluster), + svctest.SetHealthCheckNodePort(30000)), + expectError: true, + }, { + name: "ClusterIP_policy:Local_hcnp:none", + svc: svctest.MakeService("foo", + svctest.SetTypeClusterIP, + svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal)), + expectError: true, + }, { + name: "ClusterIP_policy:Local_hcnp:specified", + svc: svctest.MakeService("foo", + svctest.SetTypeClusterIP, + svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal), + svctest.SetHealthCheckNodePort(30000)), + expectError: true, + }, { + name: "NodePort_policy:none_hcnp:none", + svc: svctest.MakeService("foo", + svctest.SetTypeNodePort, + svctest.SetExternalTrafficPolicy("")), + expectHCNP: false, + }, { + name: "NodePort_policy:none_hcnp:specified", + svc: svctest.MakeService("foo", + svctest.SetTypeNodePort, + svctest.SetExternalTrafficPolicy(""), + svctest.SetHealthCheckNodePort(30000)), + expectError: true, + }, { + name: "NodePort_policy:Cluster:none", + svc: svctest.MakeService("foo", + svctest.SetTypeNodePort, + svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeCluster)), + expectHCNP: false, + }, { + name: "NodePort_policy:Cluster:specified", + svc: svctest.MakeService("foo", + svctest.SetTypeNodePort, + svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeCluster), + svctest.SetHealthCheckNodePort(30000)), + expectError: true, + }, { + name: "NodePort_policy:Local_hcnp:none", + svc: svctest.MakeService("foo", + svctest.SetTypeNodePort, + svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal)), + expectHCNP: false, + }, { + name: "NodePort_policy:Local_hcnp:specified", + svc: svctest.MakeService("foo", + svctest.SetTypeNodePort, + svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal), + svctest.SetHealthCheckNodePort(30000)), + expectError: true, + }, { + name: "LoadBalancer_policy:none_hcnp:none", + svc: svctest.MakeService("foo", + svctest.SetTypeLoadBalancer, + svctest.SetExternalTrafficPolicy("")), + expectHCNP: false, + }, { + name: "LoadBalancer_policy:none_hcnp:specified", + svc: svctest.MakeService("foo", + svctest.SetTypeLoadBalancer, + svctest.SetExternalTrafficPolicy(""), + svctest.SetHealthCheckNodePort(30000)), + expectError: true, + }, { + name: "LoadBalancer_policy:Cluster_hcnp:none", + svc: svctest.MakeService("foo", + svctest.SetTypeLoadBalancer, + svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeCluster)), + expectHCNP: false, + }, { + name: "LoadBalancer_policy:Cluster_hcnp:specified", + svc: svctest.MakeService("foo", + svctest.SetTypeLoadBalancer, + svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeCluster), + svctest.SetHealthCheckNodePort(30000)), + expectError: true, + }, { + name: "LoadBalancer_policy:Local_hcnp:none", + svc: svctest.MakeService("foo", + svctest.SetTypeLoadBalancer, + svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal)), + expectHCNP: true, + }, { + name: "LoadBalancer_policy:Local_hcnp:specified", + svc: svctest.MakeService("foo", + svctest.SetTypeLoadBalancer, + svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal), + svctest.SetHealthCheckNodePort(30000)), + expectHCNP: true, + }, { + name: "LoadBalancer_policy:Local_hcnp:negative", + svc: svctest.MakeService("foo", + svctest.SetTypeLoadBalancer, + svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal), + svctest.SetHealthCheckNodePort(-1)), + expectError: true, + }} + + // Do this in the outer scope for performance. + storage, _, server := newStorage(t, []api.IPFamily{api.IPv4Protocol}) + defer server.Terminate(t) + defer storage.Store.DestroyFunc() + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + ctx := genericapirequest.NewDefaultContext() + createdObj, err := storage.Create(ctx, tc.svc, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) + if tc.expectError && err != nil { + return + } + if err != nil { + t.Fatalf("unexpected error creating service: %v", err) + } + defer storage.Delete(ctx, tc.svc.Name, rest.ValidateAllObjectFunc, &metav1.DeleteOptions{}) + if tc.expectError && err == nil { + t.Fatalf("unexpected success creating service") + } + createdSvc := createdObj.(*api.Service) + + if !tc.expectHCNP { + if createdSvc.Spec.HealthCheckNodePort != 0 { + t.Fatalf("expected no HealthCheckNodePort, got %d", createdSvc.Spec.HealthCheckNodePort) + } + return + } + + if createdSvc.Spec.HealthCheckNodePort == 0 { + t.Fatalf("expected a HealthCheckNodePort") + } + if !portIsAllocated(t, storage.alloc.serviceNodePorts, createdSvc.Spec.HealthCheckNodePort) { + t.Errorf("expected HealthCheckNodePort to be allocated: %v", createdSvc.Spec.HealthCheckNodePort) + } + if tc.svc.Spec.HealthCheckNodePort != 0 { + if want, got := tc.svc.Spec.HealthCheckNodePort, createdSvc.Spec.HealthCheckNodePort; want != got { + t.Errorf("wrong HealthCheckNodePort value: wanted %d, got %d", want, got) + } + } + for i, p := range createdSvc.Spec.Ports { + if p.NodePort == createdSvc.Spec.HealthCheckNodePort { + t.Errorf("HealthCheckNodePort overlaps NodePort[%d]", i) + } + } + }) + } +} + // Prove that a dry-run create doesn't actually allocate IPs or ports. func TestCreateDryRun(t *testing.T) { testCases := []struct {