From ad1f2063298e8f0913812dccd6bd6b6219fbb850 Mon Sep 17 00:00:00 2001 From: Ashley Gau Date: Mon, 26 Feb 2018 14:16:26 -0800 Subject: [PATCH 01/11] isolate logic to be shared with internal lb tests into separate file --- .../gce/gce_loadbalancer_external_test.go | 216 ++++++------------ .../gce/gce_loadbalancer_test_utils.go | 144 ++++++++++++ 2 files changed, 214 insertions(+), 146 deletions(-) create mode 100644 pkg/cloudprovider/providers/gce/gce_loadbalancer_test_utils.go diff --git a/pkg/cloudprovider/providers/gce/gce_loadbalancer_external_test.go b/pkg/cloudprovider/providers/gce/gce_loadbalancer_external_test.go index 587de352404..07ec73a7888 100644 --- a/pkg/cloudprovider/providers/gce/gce_loadbalancer_external_test.go +++ b/pkg/cloudprovider/providers/gce/gce_loadbalancer_external_test.go @@ -18,90 +18,26 @@ package gce import ( "fmt" - "net/http" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" computealpha "google.golang.org/api/compute/v0.alpha" - compute "google.golang.org/api/compute/v1" "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/kubernetes/pkg/cloudprovider" "k8s.io/kubernetes/pkg/cloudprovider/providers/gce/cloud" - "k8s.io/kubernetes/pkg/cloudprovider/providers/gce/cloud/mock" - kubeletapis "k8s.io/kubernetes/pkg/kubelet/apis" ) -const ( - projectID = "test-project" - region = "us-central1" - zoneName = "us-central1-b" - nodeName = "test-node-1" - clusterName = "Test Cluster Name" - clusterID = "test-cluster-id" - serviceName = "" -) - -var apiService = &v1.Service{ - Spec: v1.ServiceSpec{ - SessionAffinity: v1.ServiceAffinityClientIP, - Type: v1.ServiceTypeClusterIP, - Ports: []v1.ServicePort{{Protocol: v1.ProtocolTCP, Port: int32(123)}}, - }, -} - -type fakeRoundTripper struct{} - -func (*fakeRoundTripper) RoundTrip(*http.Request) (*http.Response, error) { - return nil, fmt.Errorf("err: test used fake http client") -} - -func fakeGCECloud() (*GCECloud, error) { - c := &http.Client{Transport: &fakeRoundTripper{}} - - service, err := compute.New(c) - if err != nil { - return nil, err - } - - // Used in disk unit tests - fakeManager := newFakeManager(projectID, region) - zonesWithNodes := createNodeZones([]string{zoneName}) - - alphaFeatureGate, err := NewAlphaFeatureGate([]string{}) - if err != nil { - return nil, err - } - - gce := &GCECloud{ - region: region, - service: service, - manager: fakeManager, - managedZones: []string{zoneName}, - projectID: projectID, - networkProjectID: projectID, - AlphaFeatureGate: alphaFeatureGate, - nodeZones: zonesWithNodes, - nodeInformerSynced: func() bool { return true }, - } - - cloud := cloud.NewMockGCE(&gceProjectRouter{gce}) - cloud.MockTargetPools.AddInstanceHook = mock.AddInstanceHook - cloud.MockTargetPools.RemoveInstanceHook = mock.RemoveInstanceHook - cloud.MockForwardingRules.InsertHook = mock.InsertFwdRuleHook - cloud.MockAddresses.InsertHook = mock.InsertAddressHook - cloud.MockAlphaAddresses.InsertHook = mock.InsertAlphaAddressHook - - gce.c = cloud - - return gce, nil -} +const serviceName = "" func TestEnsureStaticIP(t *testing.T) { - gce, err := fakeGCECloud() + projectID := "test-project" + region := "us-central1" + zoneName := "us-central1-b" + + gce, err := fakeGCECloud(projectID, region, zoneName) require.NoError(t, err) ipName := "some-static-ip" @@ -121,7 +57,11 @@ func TestEnsureStaticIP(t *testing.T) { } func TestEnsureStaticIPWithTier(t *testing.T) { - s, err := fakeGCECloud() + projectID := "test-project" + region := "us-central1" + zoneName := "us-central1-b" + + s, err := fakeGCECloud(projectID, region, zoneName) require.NoError(t, err) for desc, tc := range map[string]struct { @@ -156,6 +96,7 @@ func TestEnsureStaticIPWithTier(t *testing.T) { func TestVerifyRequestedIP(t *testing.T) { lbRef := "test-lb" + region := "us-central1" for desc, tc := range map[string]struct { requestedIP string @@ -196,7 +137,7 @@ func TestVerifyRequestedIP(t *testing.T) { }, } { t.Run(desc, func(t *testing.T) { - s, err := fakeGCECloud() + s, err := fakeGCECloud("test-project", region, "us-central1a") require.NoError(t, err) for _, addr := range tc.addrList { @@ -214,6 +155,9 @@ func TestCreateForwardingRuleWithTier(t *testing.T) { ports := []v1.ServicePort{{Name: "foo", Protocol: v1.ProtocolTCP, Port: int32(123)}} target := "test-target-pool" svcName := "foo-svc" + projectID := "test-project" + region := "us-central1" + baseLinkUrl := "https://www.googleapis.com/compute/%v/projects/%v/regions/%v/forwardingRules/%v" for desc, tc := range map[string]struct { @@ -248,7 +192,7 @@ func TestCreateForwardingRuleWithTier(t *testing.T) { }, } { t.Run(desc, func(t *testing.T) { - s, err := fakeGCECloud() + s, err := fakeGCECloud(projectID, region, "us-central1a") require.NoError(t, err) lbName := tc.expectedRule.Name @@ -266,8 +210,9 @@ func TestCreateForwardingRuleWithTier(t *testing.T) { func TestDeleteAddressWithWrongTier(t *testing.T) { lbRef := "test-lb" + region := "us-central1" - s, err := fakeGCECloud() + s, err := fakeGCECloud("test-region", region, "us-central1a") require.NoError(t, err) // Enable the cloud.NetworkTiers feature @@ -324,56 +269,8 @@ func TestDeleteAddressWithWrongTier(t *testing.T) { } } -func createAndInsertNodes(gce *GCECloud, nodeNames []string) ([]*v1.Node, error) { - nodes := []*v1.Node{} - - for _, name := range nodeNames { - // Inserting the same node name twice causes an error - here we check if - // the instance exists already before insertion. - // TestUpdateExternalLoadBalancer inserts a new node, and relies on an older - // node to already have been inserted. - instance, _ := gce.getInstanceByName(name) - - if instance == nil { - err := gce.InsertInstance( - projectID, - zoneName, - &compute.Instance{ - Name: name, - Tags: &compute.Tags{ - Items: []string{name}, - }, - }, - ) - if err != nil { - return nodes, err - } - } - - nodes = append( - nodes, - &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: name, - Labels: map[string]string{ - kubeletapis.LabelHostname: name, - }, - }, - Status: v1.NodeStatus{ - NodeInfo: v1.NodeSystemInfo{ - KubeProxyVersion: "v1.7.2", - }, - }, - }, - ) - - } - - return nodes, nil -} - -func createExternalLoadBalancer(gce *GCECloud) (*v1.LoadBalancerStatus, error) { - nodes, err := createAndInsertNodes(gce, []string{nodeName}) +func createExternalLoadBalancer(gce *GCECloud, nodeNames []string, clusterName, clusterID, zoneName string) (*v1.LoadBalancerStatus, error) { + nodes, err := createAndInsertNodes(gce, nodeNames, zoneName) if err != nil { return nil, err } @@ -381,21 +278,28 @@ func createExternalLoadBalancer(gce *GCECloud) (*v1.LoadBalancerStatus, error) { return gce.ensureExternalLoadBalancer( clusterName, clusterID, - apiService, + fakeApiService, nil, nodes, ) } func TestEnsureExternalLoadBalancer(t *testing.T) { - gce, err := fakeGCECloud() + projectID := "test-project" + region := "us-central1" + zoneName := "us-central1-b" + clusterName := "Test Cluster Name" + clusterID := "test-cluster-id" + nodeName := "test-node-1" + + gce, err := fakeGCECloud(projectID, region, zoneName) require.NoError(t, err) - status, err := createExternalLoadBalancer(gce) + status, err := createExternalLoadBalancer(gce, []string{nodeName}, clusterName, clusterID, zoneName) assert.NoError(t, err) assert.NotEmpty(t, status.Ingress) - lbName := cloudprovider.GetLoadBalancerName(apiService) + lbName := cloudprovider.GetLoadBalancerName(fakeApiService) hcName := MakeNodesHealthCheckName(clusterID) // Check that Firewalls are created for the LoadBalancer and the HealthCheck @@ -432,21 +336,29 @@ func TestEnsureExternalLoadBalancer(t *testing.T) { } func TestUpdateExternalLoadBalancer(t *testing.T) { - gce, err := fakeGCECloud() + projectID := "test-project" + region := "us-central1" + zoneName := "us-central1-b" + clusterName := "Test Cluster Name" + clusterID := "test-cluster-1" + + nodeName := "test-node-1" + + gce, err := fakeGCECloud(projectID, region, zoneName) require.NoError(t, err) - _, err = createExternalLoadBalancer(gce) + _, err = createExternalLoadBalancer(gce, []string{nodeName}, clusterName, clusterID, zoneName) assert.NoError(t, err) newNodeName := "test-node-2" - newNodes, err := createAndInsertNodes(gce, []string{nodeName, newNodeName}) + newNodes, err := createAndInsertNodes(gce, []string{nodeName, newNodeName}, zoneName) assert.NoError(t, err) // Add the new node, then check that it is properly added to the TargetPool - err = gce.updateExternalLoadBalancer(clusterName, apiService, newNodes) + err = gce.updateExternalLoadBalancer("", fakeApiService, newNodes) assert.NoError(t, err) - lbName := cloudprovider.GetLoadBalancerName(apiService) + lbName := cloudprovider.GetLoadBalancerName(fakeApiService) pool, err := gce.GetTargetPool(lbName, region) require.NoError(t, err) @@ -464,13 +376,13 @@ func TestUpdateExternalLoadBalancer(t *testing.T) { fmt.Sprintf("/zones/%s/instances/%s", zoneName, newNodeName), ) - newNodes, err = createAndInsertNodes(gce, []string{nodeName}) + newNodes, err = createAndInsertNodes(gce, []string{nodeName}, zoneName) assert.NoError(t, err) // Remove the new node by calling updateExternalLoadBalancer with a list // only containing the old node, and test that the TargetPool no longer // contains the new node. - err = gce.updateExternalLoadBalancer(clusterName, apiService, newNodes) + err = gce.updateExternalLoadBalancer(clusterName, fakeApiService, newNodes) assert.NoError(t, err) pool, err = gce.GetTargetPool(lbName, region) @@ -484,16 +396,22 @@ func TestUpdateExternalLoadBalancer(t *testing.T) { } func TestEnsureExternalLoadBalancerDeleted(t *testing.T) { - gce, err := fakeGCECloud() + projectID := "test-project" + region := "us-central1" + zoneName := "us-central1-b" + clusterName := "Test Cluster Name" + clusterID := "test-cluster-id" + + gce, err := fakeGCECloud(projectID, region, zoneName) require.NoError(t, err) - _, err = createExternalLoadBalancer(gce) + _, err = createExternalLoadBalancer(gce, []string{"test-node-1"}, clusterName, clusterID, zoneName) assert.NoError(t, err) - err = gce.ensureExternalLoadBalancerDeleted(clusterName, clusterID, apiService) + err = gce.ensureExternalLoadBalancerDeleted(clusterName, clusterID, fakeApiService) assert.NoError(t, err) - lbName := cloudprovider.GetLoadBalancerName(apiService) + lbName := cloudprovider.GetLoadBalancerName(fakeApiService) hcName := MakeNodesHealthCheckName(clusterID) // Check that Firewalls are deleted for the LoadBalancer and the HealthCheck @@ -525,20 +443,26 @@ func TestEnsureExternalLoadBalancerDeleted(t *testing.T) { } func TestLoadBalancerWrongTierResourceDeletion(t *testing.T) { - gce, err := fakeGCECloud() + projectID := "test-project" + region := "us-central1" + zoneName := "us-central1-b" + clusterName := "Test Cluster Name" + clusterID := "test-cluster-id" + + gce, err := fakeGCECloud(projectID, region, zoneName) require.NoError(t, err) // Enable the cloud.NetworkTiers feature gce.AlphaFeatureGate.features[AlphaFeatureNetworkTiers] = true - apiService.Annotations = map[string]string{NetworkTierAnnotationKey: "Premium"} + fakeApiService.Annotations = map[string]string{NetworkTierAnnotationKey: "Premium"} // cloud.NetworkTier defaults to Premium - desiredTier, err := gce.getServiceNetworkTier(apiService) + desiredTier, err := gce.getServiceNetworkTier(fakeApiService) require.NoError(t, err) assert.Equal(t, cloud.NetworkTierPremium, desiredTier) - lbName := cloudprovider.GetLoadBalancerName(apiService) - serviceName := types.NamespacedName{Namespace: apiService.Namespace, Name: apiService.Name} + lbName := cloudprovider.GetLoadBalancerName(fakeApiService) + serviceName := types.NamespacedName{Namespace: fakeApiService.Namespace, Name: fakeApiService.Name} // create ForwardingRule and Address with the wrong tier err = createForwardingRule( @@ -548,7 +472,7 @@ func TestLoadBalancerWrongTierResourceDeletion(t *testing.T) { region, "", gce.targetPoolURL(lbName), - apiService.Spec.Ports, + fakeApiService.Spec.Ports, cloud.NetworkTierStandard, ) require.NoError(t, err) @@ -562,7 +486,7 @@ func TestLoadBalancerWrongTierResourceDeletion(t *testing.T) { err = gce.ReserveAlphaRegionAddress(addressObj, region) require.NoError(t, err) - _, err = createExternalLoadBalancer(gce) + _, err = createExternalLoadBalancer(gce, []string{"test-node-1"}, clusterName, clusterID, zoneName) require.NoError(t, err) // Expect forwarding rule tier to not be Standard diff --git a/pkg/cloudprovider/providers/gce/gce_loadbalancer_test_utils.go b/pkg/cloudprovider/providers/gce/gce_loadbalancer_test_utils.go new file mode 100644 index 00000000000..8e57e35ad12 --- /dev/null +++ b/pkg/cloudprovider/providers/gce/gce_loadbalancer_test_utils.go @@ -0,0 +1,144 @@ +/* +Copyright 2017 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. + +This file contains shared functions and variables to set up for tests for +ExternalLoadBalancer and InternalLoadBalancers. It currently cannot live in a +separate package from GCE because then it would cause a circular import. +*/ + +package gce + +import ( + "fmt" + "net/http" + + compute "google.golang.org/api/compute/v1" + + "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/kubernetes/pkg/cloudprovider/providers/gce/cloud" + "k8s.io/kubernetes/pkg/cloudprovider/providers/gce/cloud/meta" + "k8s.io/kubernetes/pkg/cloudprovider/providers/gce/cloud/mock" + kubeletapis "k8s.io/kubernetes/pkg/kubelet/apis" +) + +var fakeApiService = &v1.Service{ + Spec: v1.ServiceSpec{ + SessionAffinity: v1.ServiceAffinityClientIP, + Type: v1.ServiceTypeClusterIP, + Ports: []v1.ServicePort{{Protocol: v1.ProtocolTCP, Port: int32(123)}}, + }, +} + +type fakeRoundTripper struct{} + +func (*fakeRoundTripper) RoundTrip(*http.Request) (*http.Response, error) { + return nil, fmt.Errorf("err: test used fake http client") +} + +func fakeGCECloud(projectID, region, zoneName string) (*GCECloud, error) { + client := &http.Client{Transport: &fakeRoundTripper{}} + + service, err := compute.New(client) + if err != nil { + return nil, err + } + + // Used in disk unit tests + fakeManager := newFakeManager(projectID, region) + zonesWithNodes := createNodeZones([]string{zoneName}) + + alphaFeatureGate, err := NewAlphaFeatureGate([]string{}) + if err != nil { + return nil, err + } + + gce := &GCECloud{ + region: region, + service: service, + manager: fakeManager, + managedZones: []string{zoneName}, + projectID: projectID, + networkProjectID: projectID, + AlphaFeatureGate: alphaFeatureGate, + nodeZones: zonesWithNodes, + nodeInformerSynced: func() bool { return true }, + } + + c := cloud.NewMockGCE(&gceProjectRouter{gce}) + c.MockTargetPools.AddInstanceHook = mock.AddInstanceHook + c.MockTargetPools.RemoveInstanceHook = mock.RemoveInstanceHook + c.MockForwardingRules.InsertHook = mock.InsertFwdRuleHook + c.MockAddresses.InsertHook = mock.InsertAddressHook + c.MockAlphaAddresses.InsertHook = mock.InsertAlphaAddressHook + + keyGA := meta.GlobalKey("key-ga") + c.MockZones.Objects[*keyGA] = &cloud.MockZonesObj{ + Obj: &compute.Zone{Name: zoneName, Region: gce.getRegionLink(region)}, + } + + gce.c = c + + return gce, nil +} + +func createAndInsertNodes(gce *GCECloud, nodeNames []string, zoneName string) ([]*v1.Node, error) { + nodes := []*v1.Node{} + + for _, name := range nodeNames { + // Inserting the same node name twice causes an error - here we check if + // the instance exists already before insertion. + // TestUpdateExternalLoadBalancer inserts a new node, and relies on an older + // node to already have been inserted. + instance, _ := gce.getInstanceByName(name) + + if instance == nil { + err := gce.InsertInstance( + gce.ProjectID(), + zoneName, + &compute.Instance{ + Name: name, + Tags: &compute.Tags{ + Items: []string{name}, + }, + }, + ) + if err != nil { + return nodes, err + } + } + + nodes = append( + nodes, + &v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Labels: map[string]string{ + kubeletapis.LabelHostname: name, + kubeletapis.LabelZoneFailureDomain: zoneName, + }, + }, + Status: v1.NodeStatus{ + NodeInfo: v1.NodeSystemInfo{ + KubeProxyVersion: "v1.7.2", + }, + }, + }, + ) + + } + + return nodes, nil +} From f625b4a2d9be9b48ef11ff27db497f8a80fcd181 Mon Sep 17 00:00:00 2001 From: Ashley Gau Date: Mon, 26 Feb 2018 14:18:11 -0800 Subject: [PATCH 02/11] add hooks to add, remove, insert instances from instancegroups --- .../providers/gce/cloud/mock/mock.go | 107 +++++++++++++++++- .../gce/gce_loadbalancer_test_utils.go | 4 + 2 files changed, 110 insertions(+), 1 deletion(-) diff --git a/pkg/cloudprovider/providers/gce/cloud/mock/mock.go b/pkg/cloudprovider/providers/gce/cloud/mock/mock.go index 06ff480d47b..8d9b7382efd 100644 --- a/pkg/cloudprovider/providers/gce/cloud/mock/mock.go +++ b/pkg/cloudprovider/providers/gce/cloud/mock/mock.go @@ -33,7 +33,8 @@ import ( beta "google.golang.org/api/compute/v0.beta" ga "google.golang.org/api/compute/v1" "google.golang.org/api/googleapi" - "k8s.io/kubernetes/pkg/cloudprovider/providers/gce/cloud" + cloud "k8s.io/kubernetes/pkg/cloudprovider/providers/gce/cloud" + "k8s.io/kubernetes/pkg/cloudprovider/providers/gce/cloud/filter" "k8s.io/kubernetes/pkg/cloudprovider/providers/gce/cloud/meta" ) @@ -226,3 +227,107 @@ func InsertAlphaAddressHook(ctx context.Context, key *meta.Key, obj *alpha.Addre projectID := m.ProjectRouter.ProjectID(ctx, meta.VersionBeta, "addresses") return convertAndInsertAlphaAddress(key, obj, m.Objects, meta.VersionAlpha, projectID) } + +// Map from InstanceGroup key to list of Instances +type instanceGroupAttributes struct { + instanceMap map[meta.Key][]*ga.InstanceWithNamedPorts +} + +func AddInstancesHook(ctx context.Context, key *meta.Key, req *ga.InstanceGroupsAddInstancesRequest, m *cloud.MockInstanceGroups) error { + _, err := m.Get(ctx, key) + if err != nil { + return &googleapi.Error{ + Code: http.StatusNotFound, + Message: fmt.Sprintf("Key: %s was not found in InstanceGroups", key.String()), + } + } + + if m.X == nil { + m.X = instanceGroupAttributes{ + instanceMap: make(map[meta.Key][]*ga.InstanceWithNamedPorts), + } + } + + var attrs instanceGroupAttributes + attrs = m.X.(instanceGroupAttributes) + + instances, ok := attrs.instanceMap[*key] + if !ok { + instances = []*ga.InstanceWithNamedPorts{} + } + + for _, instance := range req.Instances { + iWithPort := &ga.InstanceWithNamedPorts{ + Instance: instance.Instance, + } + + instances = append(instances, iWithPort) + } + + attrs.instanceMap[*key] = instances + m.X = attrs + return nil +} + +func ListInstancesHook(ctx context.Context, key *meta.Key, req *ga.InstanceGroupsListInstancesRequest, filter *filter.F, m *cloud.MockInstanceGroups) ([]*ga.InstanceWithNamedPorts, error) { + _, err := m.Get(ctx, key) + if err != nil { + return nil, &googleapi.Error{ + Code: http.StatusNotFound, + Message: fmt.Sprintf("Key: %s was not found in InstanceGroups", key.String()), + } + } + + if m.X == nil { + m.X = instanceGroupAttributes{instanceMap: make(map[meta.Key][]*ga.InstanceWithNamedPorts)} + } + + var attrs instanceGroupAttributes + attrs = m.X.(instanceGroupAttributes) + + instances, ok := attrs.instanceMap[*key] + if !ok { + instances = []*ga.InstanceWithNamedPorts{} + } + + return instances, nil +} + +func RemoveInstancesHook(ctx context.Context, key *meta.Key, req *ga.InstanceGroupsRemoveInstancesRequest, m *cloud.MockInstanceGroups) error { + _, err := m.Get(ctx, key) + if err != nil { + return &googleapi.Error{ + Code: http.StatusNotFound, + Message: fmt.Sprintf("Key: %s was not found in InstanceGroups", key.String()), + } + } + + if m.X == nil { + m.X = instanceGroupAttributes{ + instanceMap: make(map[meta.Key][]*ga.InstanceWithNamedPorts), + } + } + + var attrs instanceGroupAttributes + attrs = m.X.(instanceGroupAttributes) + + instances, ok := attrs.instanceMap[*key] + if !ok { + instances = []*ga.InstanceWithNamedPorts{} + } + + for _, instanceToRemove := range req.Instances { + for i, instance := range instances { + if instanceToRemove.Instance == instance.Instance { + // Delete instance from instances without preserving order + instances[i] = instances[len(instances)-1] + instances = instances[:len(instances)-1] + break + } + } + } + + attrs.instanceMap[*key] = instances + m.X = attrs + return nil +} diff --git a/pkg/cloudprovider/providers/gce/gce_loadbalancer_test_utils.go b/pkg/cloudprovider/providers/gce/gce_loadbalancer_test_utils.go index 8e57e35ad12..67ee161350f 100644 --- a/pkg/cloudprovider/providers/gce/gce_loadbalancer_test_utils.go +++ b/pkg/cloudprovider/providers/gce/gce_loadbalancer_test_utils.go @@ -84,6 +84,10 @@ func fakeGCECloud(projectID, region, zoneName string) (*GCECloud, error) { c.MockAddresses.InsertHook = mock.InsertAddressHook c.MockAlphaAddresses.InsertHook = mock.InsertAlphaAddressHook + c.MockInstanceGroups.AddInstancesHook = mock.AddInstancesHook + c.MockInstanceGroups.RemoveInstancesHook = mock.RemoveInstancesHook + c.MockInstanceGroups.ListInstancesHook = mock.ListInstancesHook + keyGA := meta.GlobalKey("key-ga") c.MockZones.Objects[*keyGA] = &cloud.MockZonesObj{ Obj: &compute.Zone{Name: zoneName, Region: gce.getRegionLink(region)}, From ebd54ea5e303c36cd917dcad6d9b5dddb61fd6c9 Mon Sep 17 00:00:00 2001 From: Ashley Gau Date: Mon, 26 Feb 2018 14:19:44 -0800 Subject: [PATCH 03/11] test ensureInternalLoadBalancer and ensureInternalLoadBalancerDeleted --- .../gce/gce_loadbalancer_internal_test.go | 159 ++++++++++++++++++ 1 file changed, 159 insertions(+) create mode 100644 pkg/cloudprovider/providers/gce/gce_loadbalancer_internal_test.go diff --git a/pkg/cloudprovider/providers/gce/gce_loadbalancer_internal_test.go b/pkg/cloudprovider/providers/gce/gce_loadbalancer_internal_test.go new file mode 100644 index 00000000000..32c83cb94a1 --- /dev/null +++ b/pkg/cloudprovider/providers/gce/gce_loadbalancer_internal_test.go @@ -0,0 +1,159 @@ +/* +Copyright 2017 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package gce + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + compute "google.golang.org/api/compute/v1" + "k8s.io/api/core/v1" + v1_service "k8s.io/kubernetes/pkg/api/v1/service" + "k8s.io/kubernetes/pkg/cloudprovider" + "k8s.io/kubernetes/pkg/cloudprovider/providers/gce/cloud" +) + +func createInternalLoadBalancer(gce *GCECloud, existingFwdRule *compute.ForwardingRule, nodeNames []string, clusterName, clusterID, zoneName string) (*v1.LoadBalancerStatus, error) { + nodes, err := createAndInsertNodes(gce, nodeNames, zoneName) + if err != nil { + return nil, err + } + + return gce.ensureInternalLoadBalancer( + clusterName, + clusterID, + fakeApiService, + existingFwdRule, + nodes, + ) +} + +func TestEnsureInternalLoadBalancer(t *testing.T) { + projectID := "test-project" + region := "us-central1" + zoneName := "us-central1-b" + clusterName := "Test Cluster" + clusterID := "test-cluster-id" +nodeName: + + gce, err := fakeGCECloud(projectID, region, zoneName) + require.NoError(t, err) + + status, err := createInternalLoadBalancer(gce, nil, []string{nodeName}, clusterName, clusterID, zoneName) + assert.NoError(t, err) + assert.NotEmpty(t, status.Ingress) + + lbName := cloudprovider.GetLoadBalancerName(fakeApiService) + + // Check that Instance Group is created + igName := makeInstanceGroupName(clusterID) + ig, err := gce.GetInstanceGroup(igName, zoneName) + assert.NoError(t, err) + assert.Equal(t, igName, ig.Name) + + // Check that Firewalls are created for the LoadBalancer and the HealthCheck + fwNames := []string{ + lbName, + makeHealthCheckFirewallName(lbName, clusterID, true), + } + + for _, fwName := range fwNames { + firewall, err := gce.GetFirewall(fwName) + require.NoError(t, err) + assert.Equal(t, []string{nodeName}, firewall.TargetTags) + assert.NotEmpty(t, firewall.SourceRanges) + } + + // Check that HealthCheck is created + sharedHealthCheck := !v1_service.RequestsOnlyLocalTraffic(fakeApiService) + hcName := makeHealthCheckName(lbName, clusterID, sharedHealthCheck) + healthcheck, err := gce.GetHealthCheck(hcName) + require.NoError(t, err) + assert.Equal(t, hcName, healthcheck.Name) + + // Check that BackendService exists + sharedBackend := shareBackendService(fakeApiService) + backendServiceName := makeBackendServiceName(lbName, clusterID, sharedBackend, cloud.SchemeInternal, "TCP", fakeApiService.Spec.SessionAffinity) + backendServiceLink := gce.getBackendServiceLink(backendServiceName) + + bs, err := gce.GetRegionBackendService(backendServiceName, gce.region) + require.NoError(t, err) + assert.Equal(t, "TCP", bs.Protocol) + assert.Equal( + t, + []string{healthcheck.SelfLink}, + bs.HealthChecks, + ) + + // Check that ForwardingRule is created + fwdRule, err := gce.GetRegionForwardingRule(lbName, region) + require.NoError(t, err) + assert.Equal(t, lbName, fwdRule.Name) + assert.Equal(t, "TCP", fwdRule.IPProtocol) + assert.Equal(t, backendServiceLink, fwdRule.BackendService) +} + +func TestEnsureInternalLoadBalancerDeleted(t *testing.T) { + projectID := "test-project" + region := "us-central1" + zoneName := "us-central1-b" + clusterName := "Test Cluster Name" + clusterID := "test-cluster-id" + + gce, err := fakeGCECloud(projectID, region, zoneName) + require.NoError(t, err) + + _, err = createInternalLoadBalancer(gce, nil, []string{"test-node-1"}, clusterName, clusterID, zoneName) + assert.NoError(t, err) + + err = gce.ensureInternalLoadBalancerDeleted(clusterName, clusterID, fakeApiService) + assert.NoError(t, err) + + lbName := cloudprovider.GetLoadBalancerName(fakeApiService) + sharedHealthCheck := !v1_service.RequestsOnlyLocalTraffic(fakeApiService) + hcName := makeHealthCheckName(lbName, clusterID, sharedHealthCheck) + + // Check that Firewalls are deleted for the LoadBalancer and the HealthCheck + fwNames := []string{ + MakeFirewallName(lbName), + MakeHealthCheckFirewallName(clusterID, hcName, true), + } + + for _, fwName := range fwNames { + firewall, err := gce.GetFirewall(fwName) + require.Error(t, err) + assert.Nil(t, firewall) + } + + // Check that Instance Group is deleted + igName := makeInstanceGroupName(clusterID) + ig, err := gce.GetInstanceGroup(igName, zoneName) + assert.Error(t, err) + assert.Nil(t, ig) + + // Check that HealthCheck is deleted + healthcheck, err := gce.GetHealthCheck(hcName) + require.Error(t, err) + assert.Nil(t, healthcheck) + + // Check forwarding rule is deleted + fwdRule, err := gce.GetRegionForwardingRule(lbName, region) + require.Error(t, err) + assert.Nil(t, fwdRule) +} From fd2bf37d2884d80ac5f47294e9ce4aeefe705d3c Mon Sep 17 00:00:00 2001 From: Ashley Gau Date: Mon, 26 Feb 2018 14:20:32 -0800 Subject: [PATCH 04/11] hooks for updating healthchecks, firewalls, regional backendservices --- .../providers/gce/cloud/mock/mock.go | 58 +++++++++++++++++++ .../gce/gce_loadbalancer_test_utils.go | 4 ++ 2 files changed, 62 insertions(+) diff --git a/pkg/cloudprovider/providers/gce/cloud/mock/mock.go b/pkg/cloudprovider/providers/gce/cloud/mock/mock.go index 8d9b7382efd..3938aa068a0 100644 --- a/pkg/cloudprovider/providers/gce/cloud/mock/mock.go +++ b/pkg/cloudprovider/providers/gce/cloud/mock/mock.go @@ -331,3 +331,61 @@ func RemoveInstancesHook(ctx context.Context, key *meta.Key, req *ga.InstanceGro m.X = attrs return nil } + +// UpdateFirewallHook defines the hook for updating a Firewall. It replaces the +// object with the same key in the mock with the updated object. +func UpdateFirewallHook(ctx context.Context, key *meta.Key, obj *ga.Firewall, m *cloud.MockFirewalls) error { + _, err := m.Get(ctx, key) + if err != nil { + return &googleapi.Error{ + Code: http.StatusNotFound, + Message: fmt.Sprintf("Key: %s was not found in Firewalls", key.String()), + } + } + + obj.Name = key.Name + projectID := m.ProjectRouter.ProjectID(ctx, "ga", "firewalls") + obj.SelfLink = cloud.SelfLink(meta.VersionGA, projectID, "firewalls", key) + + m.Objects[*key] = &cloud.MockFirewallsObj{obj} + return nil +} + +// UpdateHealthCheckHook defines the hook for updating a HealthCheck. It +// replaces the object with the same key in the mock with the updated object. +func UpdateHealthCheckHook(ctx context.Context, key *meta.Key, obj *ga.HealthCheck, m *cloud.MockHealthChecks) error { + _, err := m.Get(ctx, key) + if err != nil { + return &googleapi.Error{ + Code: http.StatusNotFound, + Message: fmt.Sprintf("Key: %s was not found in HealthChecks", key.String()), + } + } + + obj.Name = key.Name + projectID := m.ProjectRouter.ProjectID(ctx, "ga", "healthChecks") + obj.SelfLink = cloud.SelfLink(meta.VersionGA, projectID, "healthChecks", key) + + m.Objects[*key] = &cloud.MockHealthChecksObj{obj} + return nil +} + +// UpdateRegionBackendServiceHook defines the hook for updating a Region +// BackendsService. It replaces the object with the same key in the mock with +// the updated object. +func UpdateRegionBackendServiceHook(ctx context.Context, key *meta.Key, obj *ga.BackendService, m *cloud.MockRegionBackendServices) error { + _, err := m.Get(ctx, key) + if err != nil { + return &googleapi.Error{ + Code: http.StatusNotFound, + Message: fmt.Sprintf("Key: %s was not found in RegionBackendServices", key.String()), + } + } + + obj.Name = key.Name + projectID := m.ProjectRouter.ProjectID(ctx, "ga", "backendServices") + obj.SelfLink = cloud.SelfLink(meta.VersionGA, projectID, "backendServices", key) + + m.Objects[*key] = &cloud.MockRegionBackendServicesObj{obj} + return nil +} diff --git a/pkg/cloudprovider/providers/gce/gce_loadbalancer_test_utils.go b/pkg/cloudprovider/providers/gce/gce_loadbalancer_test_utils.go index 67ee161350f..c960abdb04f 100644 --- a/pkg/cloudprovider/providers/gce/gce_loadbalancer_test_utils.go +++ b/pkg/cloudprovider/providers/gce/gce_loadbalancer_test_utils.go @@ -88,6 +88,10 @@ func fakeGCECloud(projectID, region, zoneName string) (*GCECloud, error) { c.MockInstanceGroups.RemoveInstancesHook = mock.RemoveInstancesHook c.MockInstanceGroups.ListInstancesHook = mock.ListInstancesHook + c.MockRegionBackendServices.UpdateHook = mock.UpdateRegionBackendServiceHook + c.MockHealthChecks.UpdateHook = mock.UpdateHealthCheckHook + c.MockFirewalls.UpdateHook = mock.UpdateFirewallHook + keyGA := meta.GlobalKey("key-ga") c.MockZones.Objects[*keyGA] = &cloud.MockZonesObj{ Obj: &compute.Zone{Name: zoneName, Region: gce.getRegionLink(region)}, From 7648696c880f0a09be07f96bfe6eedaf0613461e Mon Sep 17 00:00:00 2001 From: Ashley Gau Date: Mon, 26 Feb 2018 14:21:20 -0800 Subject: [PATCH 05/11] test updateInternalLoadBalancer --- .../gce/gce_loadbalancer_internal_test.go | 162 +++++++++++++++++- 1 file changed, 161 insertions(+), 1 deletion(-) diff --git a/pkg/cloudprovider/providers/gce/gce_loadbalancer_internal_test.go b/pkg/cloudprovider/providers/gce/gce_loadbalancer_internal_test.go index 32c83cb94a1..8c3eaaf9d03 100644 --- a/pkg/cloudprovider/providers/gce/gce_loadbalancer_internal_test.go +++ b/pkg/cloudprovider/providers/gce/gce_loadbalancer_internal_test.go @@ -24,6 +24,7 @@ import ( compute "google.golang.org/api/compute/v1" "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" v1_service "k8s.io/kubernetes/pkg/api/v1/service" "k8s.io/kubernetes/pkg/cloudprovider" "k8s.io/kubernetes/pkg/cloudprovider/providers/gce/cloud" @@ -50,7 +51,7 @@ func TestEnsureInternalLoadBalancer(t *testing.T) { zoneName := "us-central1-b" clusterName := "Test Cluster" clusterID := "test-cluster-id" -nodeName: + nodeName := "test-node-1" gce, err := fakeGCECloud(projectID, region, zoneName) require.NoError(t, err) @@ -109,6 +110,165 @@ nodeName: assert.Equal(t, backendServiceLink, fwdRule.BackendService) } +func TestEnsureInternalLoadBalancerDeleteWrongResources(t *testing.T) { + projectID := "test-project" + region := "us-central1" + zoneName := "us-central1-b" + clusterName := "Test Cluster" + clusterID := "test-cluster-id" + + gce, err := fakeGCECloud(projectID, region, zoneName) + require.NoError(t, err) + + lbName := cloudprovider.GetLoadBalancerName(fakeApiService) + + // Create a ForwardingRule that's missing an IP address and BackendService + existingFwdRule := &compute.ForwardingRule{ + Name: lbName, + IPAddress: "", + Ports: []string{"123"}, + IPProtocol: "TCP", + LoadBalancingScheme: string(cloud.SchemeInternal), + } + gce.CreateRegionForwardingRule(existingFwdRule, gce.region) + + // Create a Firewall that's missing a Description + existingFirewall := &compute.Firewall{ + Name: lbName, + Network: gce.networkURL, + Allowed: []*compute.FirewallAllowed{ + { + IPProtocol: "tcp", + Ports: []string{"123"}, + }, + }, + } + gce.CreateFirewall(existingFirewall) + + sharedHealthCheck := !v1_service.RequestsOnlyLocalTraffic(fakeApiService) + hcName := makeHealthCheckName(lbName, clusterID, sharedHealthCheck) + hcPath, hcPort := GetNodesHealthCheckPath(), GetNodesHealthCheckPort() + nm := types.NamespacedName{Name: fakeApiService.Name, Namespace: fakeApiService.Namespace} + + // Create a healthcheck with an incorrect threshold + existingHC := newInternalLBHealthCheck(hcName, nm, sharedHealthCheck, hcPath, hcPort) + existingHC.HealthyThreshold = gceHcHealthyThreshold * 10 + gce.CreateHealthCheck(existingHC) + + // Create a backend Service that's missing Description and Backends + sharedBackend := shareBackendService(fakeApiService) + backendServiceName := makeBackendServiceName(lbName, clusterID, sharedBackend, cloud.SchemeInternal, "TCP", fakeApiService.Spec.SessionAffinity) + existingBS := &compute.BackendService{ + Name: backendServiceName, + Protocol: "TCP", + HealthChecks: []string{existingHC.SelfLink}, + SessionAffinity: translateAffinityType(fakeApiService.Spec.SessionAffinity), + LoadBalancingScheme: string(cloud.SchemeInternal), + } + + gce.CreateRegionBackendService(existingBS, gce.region) + + _, err = createInternalLoadBalancer(gce, existingFwdRule, []string{"test-node-1"}, clusterName, clusterID, zoneName) + assert.NoError(t, err) + + // Expect new resources with the correct attributes to be created + rule, _ := gce.GetRegionForwardingRule(lbName, gce.region) + assert.NotEqual(t, existingFwdRule, rule) + + firewall, err := gce.GetFirewall(lbName) + require.NoError(t, err) + assert.NotEqual(t, firewall, existingFirewall) + + healthcheck, err := gce.GetHealthCheck(hcName) + require.NoError(t, err) + assert.NotEqual(t, healthcheck, existingHC) + + bs, err := gce.GetRegionBackendService(backendServiceName, gce.region) + require.NoError(t, err) + assert.NotEqual(t, bs, existingBS) +} + +func TestUpdateInternalLoadBalancerBackendServices(t *testing.T) { + projectID := "test-project" + region := "us-central1" + zoneName := "us-central1-b" + clusterName := "Test Cluster Name" + clusterID := "test-cluster-id" + + nodeName := "test-node-1" + + gce, err := fakeGCECloud(projectID, region, zoneName) + require.NoError(t, err) + + _, err = createInternalLoadBalancer(gce, nil, []string{"test-node-1"}, clusterName, clusterID, zoneName) + assert.NoError(t, err) + + // BackendService exists prior to updateInternalLoadBalancer call, but has + // incorrect (missing) attributes. + // ensureInternalBackendServiceGroups is called and creates the correct + // BackendService + lbName := cloudprovider.GetLoadBalancerName(fakeApiService) + sharedBackend := shareBackendService(fakeApiService) + backendServiceName := makeBackendServiceName(lbName, clusterID, sharedBackend, cloud.SchemeInternal, "TCP", fakeApiService.Spec.SessionAffinity) + existingBS := &compute.BackendService{ + Name: backendServiceName, + Protocol: "TCP", + SessionAffinity: translateAffinityType(fakeApiService.Spec.SessionAffinity), + LoadBalancingScheme: string(cloud.SchemeInternal), + } + + gce.CreateRegionBackendService(existingBS, gce.region) + + nodes, err := createAndInsertNodes(gce, []string{nodeName}, zoneName) + require.NoError(t, err) + + err = gce.updateInternalLoadBalancer(clusterName, clusterID, fakeApiService, nodes) + assert.NoError(t, err) + + bs, err := gce.GetRegionBackendService(backendServiceName, gce.region) + require.NoError(t, err) + + // Check that the new BackendService has the correct attributes + assert.NotEqual(t, existingBS, bs) + assert.NotEmpty(t, bs.SelfLink) + assert.NotEmpty(t, bs.Description) + assert.NotEmpty(t, bs.HealthChecks) +} + +func TestUpdateInternalLoadBalancerNodes(t *testing.T) { + projectID := "test-project" + region := "us-central1" + zoneName := "us-central1-b" + clusterName := "Test Cluster Name" + clusterID := "test-cluster-id" + + gce, err := fakeGCECloud(projectID, region, zoneName) + require.NoError(t, err) + + _, err = createInternalLoadBalancer(gce, nil, []string{"test-node-1"}, clusterName, clusterID, zoneName) + assert.NoError(t, err) + + // Remove the old Node and insert a new Node. + newNodeName := "test-node-2" + newNodes, err := createAndInsertNodes(gce, []string{newNodeName}, zoneName) + require.NoError(t, err) + + err = gce.updateInternalLoadBalancer(clusterName, clusterID, fakeApiService, newNodes) + assert.NoError(t, err) + + // Expect node 1 to be deleted and node 2 to still exist + igName := makeInstanceGroupName(clusterID) + instances, err := gce.ListInstancesInInstanceGroup(igName, zoneName, "ALL") + require.NoError(t, err) + + assert.Equal(t, 1, len(instances)) + assert.Equal( + t, + "https://www.googleapis.com/compute/v1/projects/test-project/zones/us-central1-b/instances/test-node-2", + instances[0].Instance, + ) +} + func TestEnsureInternalLoadBalancerDeleted(t *testing.T) { projectID := "test-project" region := "us-central1" From 42c5bca0c0634a0ce1947045166b6a04b2f2f5c0 Mon Sep 17 00:00:00 2001 From: Ashley Gau Date: Mon, 26 Feb 2018 15:24:18 -0800 Subject: [PATCH 06/11] rename to _test.go, update-bazel, comments --- pkg/cloudprovider/providers/gce/BUILD | 4 ++++ pkg/cloudprovider/providers/gce/cloud/mock/BUILD | 1 + pkg/cloudprovider/providers/gce/cloud/mock/mock.go | 9 ++++++--- ...ncer_test_utils.go => gce_loadbalancer_utils_test.go} | 8 ++++---- 4 files changed, 15 insertions(+), 7 deletions(-) rename pkg/cloudprovider/providers/gce/{gce_loadbalancer_test_utils.go => gce_loadbalancer_utils_test.go} (94%) diff --git a/pkg/cloudprovider/providers/gce/BUILD b/pkg/cloudprovider/providers/gce/BUILD index 5c04c174800..769974b7b91 100644 --- a/pkg/cloudprovider/providers/gce/BUILD +++ b/pkg/cloudprovider/providers/gce/BUILD @@ -101,14 +101,18 @@ go_test( "gce_disks_test.go", "gce_healthchecks_test.go", "gce_loadbalancer_external_test.go", + "gce_loadbalancer_internal_test.go", + "gce_loadbalancer_utils_test.go", "gce_test.go", "gce_util_test.go", "metrics_test.go", ], embed = [":go_default_library"], deps = [ + "//pkg/api/v1/service:go_default_library", "//pkg/cloudprovider:go_default_library", "//pkg/cloudprovider/providers/gce/cloud:go_default_library", + "//pkg/cloudprovider/providers/gce/cloud/meta:go_default_library", "//pkg/cloudprovider/providers/gce/cloud/mock:go_default_library", "//pkg/kubelet/apis:go_default_library", "//vendor/github.com/stretchr/testify/assert:go_default_library", diff --git a/pkg/cloudprovider/providers/gce/cloud/mock/BUILD b/pkg/cloudprovider/providers/gce/cloud/mock/BUILD index bceb2e84739..e6e359278c4 100644 --- a/pkg/cloudprovider/providers/gce/cloud/mock/BUILD +++ b/pkg/cloudprovider/providers/gce/cloud/mock/BUILD @@ -7,6 +7,7 @@ go_library( visibility = ["//visibility:public"], deps = [ "//pkg/cloudprovider/providers/gce/cloud:go_default_library", + "//pkg/cloudprovider/providers/gce/cloud/filter:go_default_library", "//pkg/cloudprovider/providers/gce/cloud/meta:go_default_library", "//vendor/google.golang.org/api/compute/v0.alpha:go_default_library", "//vendor/google.golang.org/api/compute/v0.beta:go_default_library", diff --git a/pkg/cloudprovider/providers/gce/cloud/mock/mock.go b/pkg/cloudprovider/providers/gce/cloud/mock/mock.go index 3938aa068a0..c2c772d1431 100644 --- a/pkg/cloudprovider/providers/gce/cloud/mock/mock.go +++ b/pkg/cloudprovider/providers/gce/cloud/mock/mock.go @@ -233,6 +233,7 @@ type instanceGroupAttributes struct { instanceMap map[meta.Key][]*ga.InstanceWithNamedPorts } +// AddInstancesHook mocks adding instances from an InstanceGroup func AddInstancesHook(ctx context.Context, key *meta.Key, req *ga.InstanceGroupsAddInstancesRequest, m *cloud.MockInstanceGroups) error { _, err := m.Get(ctx, key) if err != nil { @@ -269,6 +270,7 @@ func AddInstancesHook(ctx context.Context, key *meta.Key, req *ga.InstanceGroups return nil } +// ListInstancesHook mocks listing instances from an InstanceGroup func ListInstancesHook(ctx context.Context, key *meta.Key, req *ga.InstanceGroupsListInstancesRequest, filter *filter.F, m *cloud.MockInstanceGroups) ([]*ga.InstanceWithNamedPorts, error) { _, err := m.Get(ctx, key) if err != nil { @@ -293,6 +295,7 @@ func ListInstancesHook(ctx context.Context, key *meta.Key, req *ga.InstanceGroup return instances, nil } +// RemoveInstancesHook mocks removing instances from an InstanceGroup func RemoveInstancesHook(ctx context.Context, key *meta.Key, req *ga.InstanceGroupsRemoveInstancesRequest, m *cloud.MockInstanceGroups) error { _, err := m.Get(ctx, key) if err != nil { @@ -347,7 +350,7 @@ func UpdateFirewallHook(ctx context.Context, key *meta.Key, obj *ga.Firewall, m projectID := m.ProjectRouter.ProjectID(ctx, "ga", "firewalls") obj.SelfLink = cloud.SelfLink(meta.VersionGA, projectID, "firewalls", key) - m.Objects[*key] = &cloud.MockFirewallsObj{obj} + m.Objects[*key] = &cloud.MockFirewallsObj{Obj: obj} return nil } @@ -366,7 +369,7 @@ func UpdateHealthCheckHook(ctx context.Context, key *meta.Key, obj *ga.HealthChe projectID := m.ProjectRouter.ProjectID(ctx, "ga", "healthChecks") obj.SelfLink = cloud.SelfLink(meta.VersionGA, projectID, "healthChecks", key) - m.Objects[*key] = &cloud.MockHealthChecksObj{obj} + m.Objects[*key] = &cloud.MockHealthChecksObj{Obj: obj} return nil } @@ -386,6 +389,6 @@ func UpdateRegionBackendServiceHook(ctx context.Context, key *meta.Key, obj *ga. projectID := m.ProjectRouter.ProjectID(ctx, "ga", "backendServices") obj.SelfLink = cloud.SelfLink(meta.VersionGA, projectID, "backendServices", key) - m.Objects[*key] = &cloud.MockRegionBackendServicesObj{obj} + m.Objects[*key] = &cloud.MockRegionBackendServicesObj{Obj: obj} return nil } diff --git a/pkg/cloudprovider/providers/gce/gce_loadbalancer_test_utils.go b/pkg/cloudprovider/providers/gce/gce_loadbalancer_utils_test.go similarity index 94% rename from pkg/cloudprovider/providers/gce/gce_loadbalancer_test_utils.go rename to pkg/cloudprovider/providers/gce/gce_loadbalancer_utils_test.go index c960abdb04f..65b7900163a 100644 --- a/pkg/cloudprovider/providers/gce/gce_loadbalancer_test_utils.go +++ b/pkg/cloudprovider/providers/gce/gce_loadbalancer_utils_test.go @@ -12,12 +12,12 @@ distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. - -This file contains shared functions and variables to set up for tests for -ExternalLoadBalancer and InternalLoadBalancers. It currently cannot live in a -separate package from GCE because then it would cause a circular import. */ +// This file contains shared functions and variables to set up for tests for +// ExternalLoadBalancer and InternalLoadBalancers. It currently cannot live in a +// separate package from GCE because then it would cause a circular import. + package gce import ( From 8855702ed2ff22979e9dc53113c05deba03e4a83 Mon Sep 17 00:00:00 2001 From: Ashley Gau Date: Tue, 27 Feb 2018 13:11:57 -0800 Subject: [PATCH 07/11] test that deleting twice does not throw error --- .../gce/gce_loadbalancer_internal_test.go | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/pkg/cloudprovider/providers/gce/gce_loadbalancer_internal_test.go b/pkg/cloudprovider/providers/gce/gce_loadbalancer_internal_test.go index 8c3eaaf9d03..4fa4eeda19c 100644 --- a/pkg/cloudprovider/providers/gce/gce_loadbalancer_internal_test.go +++ b/pkg/cloudprovider/providers/gce/gce_loadbalancer_internal_test.go @@ -317,3 +317,24 @@ func TestEnsureInternalLoadBalancerDeleted(t *testing.T) { require.Error(t, err) assert.Nil(t, fwdRule) } + +func TestEnsureInternalLoadBalancerDeletedTwiceDoesNotError(t *testing.T) { + projectID := "test-project" + region := "us-central1" + zoneName := "us-central1-b" + clusterName := "Test Cluster Name" + clusterID := "test-cluster-id" + + gce, err := fakeGCECloud(projectID, region, zoneName) + require.NoError(t, err) + + _, err = createInternalLoadBalancer(gce, nil, []string{"test-node-1"}, clusterName, clusterID, zoneName) + assert.NoError(t, err) + + err = gce.ensureInternalLoadBalancerDeleted(clusterName, clusterID, fakeApiService) + assert.NoError(t, err) + + // Deleting the loadbalancer and resources again should not cause an error. + err = gce.ensureInternalLoadBalancerDeleted(clusterName, clusterID, fakeApiService) + assert.NoError(t, err) +} From 26c0a0d11fb10309c1ad68f39a291fe25704bf28 Mon Sep 17 00:00:00 2001 From: Ashley Gau Date: Tue, 27 Feb 2018 14:14:34 -0800 Subject: [PATCH 08/11] expect no error when correct resources already exist. DeleteWrongResources -> ClearPreviousResources --- .../gce/gce_loadbalancer_internal_test.go | 46 +++++++++++++++++-- 1 file changed, 42 insertions(+), 4 deletions(-) diff --git a/pkg/cloudprovider/providers/gce/gce_loadbalancer_internal_test.go b/pkg/cloudprovider/providers/gce/gce_loadbalancer_internal_test.go index 4fa4eeda19c..f3fb30990ee 100644 --- a/pkg/cloudprovider/providers/gce/gce_loadbalancer_internal_test.go +++ b/pkg/cloudprovider/providers/gce/gce_loadbalancer_internal_test.go @@ -110,7 +110,44 @@ func TestEnsureInternalLoadBalancer(t *testing.T) { assert.Equal(t, backendServiceLink, fwdRule.BackendService) } -func TestEnsureInternalLoadBalancerDeleteWrongResources(t *testing.T) { +func TestEnsureInternalLoadBalancerWithExistingResources(t *testing.T) { + projectID := "test-project" + region := "us-central1" + zoneName := "us-central1-b" + clusterName := "Test Cluster" + clusterID := "test-cluster-id" + nodeNames := []string{"test-node-1"} + + gce, err := fakeGCECloud(projectID, region, zoneName) + require.NoError(t, err) + + // Create the expected resources necessary for an Internal Load Balancer + nm := types.NamespacedName{Name: fakeApiService.Name, Namespace: fakeApiService.Namespace} + lbName := cloudprovider.GetLoadBalancerName(fakeApiService) + + sharedHealthCheck := !v1_service.RequestsOnlyLocalTraffic(fakeApiService) + hcName := makeHealthCheckName(lbName, clusterID, sharedHealthCheck) + hcPath, hcPort := GetNodesHealthCheckPath(), GetNodesHealthCheckPort() + existingHC := newInternalLBHealthCheck(hcName, nm, sharedHealthCheck, hcPath, hcPort) + err = gce.CreateHealthCheck(existingHC) + require.NoError(t, err) + + nodes, err := createAndInsertNodes(gce, nodeNames, zoneName) + igName := makeInstanceGroupName(clusterID) + igLinks, err := gce.ensureInternalInstanceGroups(igName, nodes) + require.NoError(t, err) + + sharedBackend := shareBackendService(fakeApiService) + bsDescription := makeBackendServiceDescription(nm, sharedBackend) + bsName := makeBackendServiceName(lbName, clusterID, sharedBackend, cloud.SchemeInternal, "TCP", fakeApiService.Spec.SessionAffinity) + err = gce.ensureInternalBackendService(bsName, bsDescription, fakeApiService.Spec.SessionAffinity, cloud.SchemeInternal, "TCP", igLinks, existingHC.SelfLink) + require.NoError(t, err) + + _, err = createInternalLoadBalancer(gce, nil, nodeNames, clusterName, clusterID, zoneName) + assert.NoError(t, err) +} + +func TestEnsureInternalLoadBalancerClearPreviousResources(t *testing.T) { projectID := "test-project" region := "us-central1" zoneName := "us-central1-b" @@ -122,9 +159,9 @@ func TestEnsureInternalLoadBalancerDeleteWrongResources(t *testing.T) { lbName := cloudprovider.GetLoadBalancerName(fakeApiService) - // Create a ForwardingRule that's missing an IP address and BackendService + // Create a ForwardingRule that's missing an IP address existingFwdRule := &compute.ForwardingRule{ - Name: lbName, + Name: "Bad Name", IPAddress: "", Ports: []string{"123"}, IPProtocol: "TCP", @@ -159,7 +196,7 @@ func TestEnsureInternalLoadBalancerDeleteWrongResources(t *testing.T) { sharedBackend := shareBackendService(fakeApiService) backendServiceName := makeBackendServiceName(lbName, clusterID, sharedBackend, cloud.SchemeInternal, "TCP", fakeApiService.Spec.SessionAffinity) existingBS := &compute.BackendService{ - Name: backendServiceName, + Name: "Bad Name", Protocol: "TCP", HealthChecks: []string{existingHC.SelfLink}, SessionAffinity: translateAffinityType(fakeApiService.Spec.SessionAffinity), @@ -167,6 +204,7 @@ func TestEnsureInternalLoadBalancerDeleteWrongResources(t *testing.T) { } gce.CreateRegionBackendService(existingBS, gce.region) + existingFwdRule.BackendService = existingBS.Name _, err = createInternalLoadBalancer(gce, existingFwdRule, []string{"test-node-1"}, clusterName, clusterID, zoneName) assert.NoError(t, err) From ac6ff68e209bab662169538f963351400b94455f Mon Sep 17 00:00:00 2001 From: Ashley Gau Date: Tue, 27 Feb 2018 14:57:07 -0800 Subject: [PATCH 09/11] test ensureInternalBackendService, ensureInternalBackendServiceGroups --- .../gce/gce_loadbalancer_internal_test.go | 91 +++++++++++++++++-- 1 file changed, 84 insertions(+), 7 deletions(-) diff --git a/pkg/cloudprovider/providers/gce/gce_loadbalancer_internal_test.go b/pkg/cloudprovider/providers/gce/gce_loadbalancer_internal_test.go index f3fb30990ee..443e780abbd 100644 --- a/pkg/cloudprovider/providers/gce/gce_loadbalancer_internal_test.go +++ b/pkg/cloudprovider/providers/gce/gce_loadbalancer_internal_test.go @@ -17,6 +17,8 @@ limitations under the License. package gce import ( + "fmt" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -45,6 +47,69 @@ func createInternalLoadBalancer(gce *GCECloud, existingFwdRule *compute.Forwardi ) } +func TestEnsureInternalBackendServiceUpdates(t *testing.T) { + vals := DefaultTestClusterValues() + nodeNames := []string{"test-node-1"} + + gce, err := fakeGCECloud(projectID, region, zoneName) + require.NoError(t, err) + + lbName := cloudprovider.GetLoadBalancerName(fakeApiService) + nodes, err := createAndInsertNodes(gce, nodeNames, zoneName) + igName := makeInstanceGroupName(clusterID) + igLinks, err := gce.ensureInternalInstanceGroups(igName, nodes) + require.NoError(t, err) + + sharedBackend := shareBackendService(fakeApiService) + bsName := makeBackendServiceName(lbName, clusterID, sharedBackend, cloud.SchemeInternal, "TCP", fakeApiService.Spec.SessionAffinity) + err = gce.ensureInternalBackendService(bsName, "description", fakeApiService.Spec.SessionAffinity, cloud.SchemeInternal, "TCP", igLinks, "") + require.NoError(t, err) + + // Update the Internal Backend Service with a new ServiceAffinity + err = gce.ensureInternalBackendService(bsName, "description", v1.ServiceAffinityNone, cloud.SchemeInternal, "TCP", igLinks, "") + require.NoError(t, err) + + bs, err := gce.GetRegionBackendService(bsName, gce.region) + assert.NoError(t, err) + assert.Equal(t, bs.SessionAffinity, strings.ToUpper(string(v1.ServiceAffinityNone))) +} + +func TestEnsureInternalBackendServiceGroups(t *testing.T) { + projectID := "test-project" + region := "us-central1" + zoneName := "us-central1-b" + clusterID := "test-cluster-id" + nodeNames := []string{"test-node-1"} + + gce, err := fakeGCECloud(projectID, region, zoneName) + require.NoError(t, err) + + lbName := cloudprovider.GetLoadBalancerName(fakeApiService) + nodes, err := createAndInsertNodes(gce, nodeNames, zoneName) + igName := makeInstanceGroupName(clusterID) + igLinks, err := gce.ensureInternalInstanceGroups(igName, nodes) + require.NoError(t, err) + + sharedBackend := shareBackendService(fakeApiService) + bsName := makeBackendServiceName(lbName, clusterID, sharedBackend, cloud.SchemeInternal, "TCP", fakeApiService.Spec.SessionAffinity) + err = gce.ensureInternalBackendService(bsName, "description", fakeApiService.Spec.SessionAffinity, cloud.SchemeInternal, "TCP", igLinks, "") + require.NoError(t, err) + + // Update the BackendService with new Instances + newNodeNames := []string{"new-test-node-1", "new-test-node-2"} + err = gce.ensureInternalBackendServiceGroups(bsName, newNodeNames) + assert.NoError(t, err) + + bs, err := gce.GetRegionBackendService(bsName, gce.region) + assert.NoError(t, err) + + // Check that the instances are updated + newNodes, err := createAndInsertNodes(gce, newNodeNames, zoneName) + newIgLinks, err := gce.ensureInternalInstanceGroups(igName, newNodes) + backends := backendsFromGroupLinks(newIgLinks) + assert.Equal(t, bs.Backends, backends) +} + func TestEnsureInternalLoadBalancer(t *testing.T) { projectID := "test-project" region := "us-central1" @@ -108,6 +173,8 @@ func TestEnsureInternalLoadBalancer(t *testing.T) { assert.Equal(t, lbName, fwdRule.Name) assert.Equal(t, "TCP", fwdRule.IPProtocol) assert.Equal(t, backendServiceLink, fwdRule.BackendService) + // if no Subnetwork specified, defaults to the GCE NetworkURL + assert.Equal(t, gce.NetworkURL(), fwdRule.Subnetwork) } func TestEnsureInternalLoadBalancerWithExistingResources(t *testing.T) { @@ -161,7 +228,7 @@ func TestEnsureInternalLoadBalancerClearPreviousResources(t *testing.T) { // Create a ForwardingRule that's missing an IP address existingFwdRule := &compute.ForwardingRule{ - Name: "Bad Name", + Name: lbName, IPAddress: "", Ports: []string{"123"}, IPProtocol: "TCP", @@ -196,7 +263,7 @@ func TestEnsureInternalLoadBalancerClearPreviousResources(t *testing.T) { sharedBackend := shareBackendService(fakeApiService) backendServiceName := makeBackendServiceName(lbName, clusterID, sharedBackend, cloud.SchemeInternal, "TCP", fakeApiService.Spec.SessionAffinity) existingBS := &compute.BackendService{ - Name: "Bad Name", + Name: lbName, Protocol: "TCP", HealthChecks: []string{existingHC.SelfLink}, SessionAffinity: translateAffinityType(fakeApiService.Spec.SessionAffinity), @@ -267,10 +334,20 @@ func TestUpdateInternalLoadBalancerBackendServices(t *testing.T) { require.NoError(t, err) // Check that the new BackendService has the correct attributes + url_base := fmt.Sprintf("https://www.googleapis.com/compute/v1/projects/%s", vals.ProjectID) + assert.NotEqual(t, existingBS, bs) - assert.NotEmpty(t, bs.SelfLink) - assert.NotEmpty(t, bs.Description) - assert.NotEmpty(t, bs.HealthChecks) + assert.Equal( + t, + bs.SelfLink, + fmt.Sprintf("%s/regions/%s/backendServices/%s", url_base, vals.Region, bs.Name), + ) + assert.Equal(t, bs.Description, `{"kubernetes.io/service-name":"/"}`) + assert.Equal( + t, + bs.HealthChecks, + []string{fmt.Sprintf("%s/healthChecks/k8s-%s-node", url_base, vals.ClusterID)}, + ) } func TestUpdateInternalLoadBalancerNodes(t *testing.T) { @@ -300,10 +377,10 @@ func TestUpdateInternalLoadBalancerNodes(t *testing.T) { require.NoError(t, err) assert.Equal(t, 1, len(instances)) - assert.Equal( + assert.Contains( t, - "https://www.googleapis.com/compute/v1/projects/test-project/zones/us-central1-b/instances/test-node-2", instances[0].Instance, + fmt.Sprintf("projects/%s/zones/%s/instances/%s", projectID, zoneName, newNodeName), ) } From 7fff54cfdd373f8f58c1fa119740099d5c25734d Mon Sep 17 00:00:00 2001 From: Ashley Gau Date: Mon, 12 Mar 2018 14:00:57 -0700 Subject: [PATCH 10/11] move shared test cluster vars into method + type --- .../gce/gce_loadbalancer_external_test.go | 147 +++++++---------- .../gce/gce_loadbalancer_internal_test.go | 154 +++++++----------- .../gce/gce_loadbalancer_utils_test.go | 34 +++- 3 files changed, 144 insertions(+), 191 deletions(-) diff --git a/pkg/cloudprovider/providers/gce/gce_loadbalancer_external_test.go b/pkg/cloudprovider/providers/gce/gce_loadbalancer_external_test.go index 07ec73a7888..608a68d0792 100644 --- a/pkg/cloudprovider/providers/gce/gce_loadbalancer_external_test.go +++ b/pkg/cloudprovider/providers/gce/gce_loadbalancer_external_test.go @@ -30,40 +30,33 @@ import ( "k8s.io/kubernetes/pkg/cloudprovider/providers/gce/cloud" ) -const serviceName = "" - func TestEnsureStaticIP(t *testing.T) { - projectID := "test-project" - region := "us-central1" - zoneName := "us-central1-b" - - gce, err := fakeGCECloud(projectID, region, zoneName) + gce, err := fakeGCECloud(DefaultTestClusterValues()) require.NoError(t, err) ipName := "some-static-ip" + serviceName := "some-service" // First ensure call - ip, existed, err := ensureStaticIP(gce, ipName, serviceName, region, "", cloud.NetworkTierDefault) + ip, existed, err := ensureStaticIP(gce, ipName, serviceName, gce.region, "", cloud.NetworkTierDefault) if err != nil || existed { - t.Fatalf(`ensureStaticIP(%v, %v, %v, %v, "") = %v, %v, %v; want valid ip, false, nil`, gce, ipName, serviceName, region, ip, existed, err) + t.Fatalf(`ensureStaticIP(%v, %v, %v, %v, "") = %v, %v, %v; want valid ip, false, nil`, gce, ipName, serviceName, gce.region, ip, existed, err) } // Second ensure call var ipPrime string - ipPrime, existed, err = ensureStaticIP(gce, ipName, serviceName, region, ip, cloud.NetworkTierDefault) + ipPrime, existed, err = ensureStaticIP(gce, ipName, serviceName, gce.region, ip, cloud.NetworkTierDefault) if err != nil || !existed || ip != ipPrime { - t.Fatalf(`ensureStaticIP(%v, %v, %v, %v, %v) = %v, %v, %v; want %v, true, nil`, gce, ipName, serviceName, region, ip, ipPrime, existed, err, ip) + t.Fatalf(`ensureStaticIP(%v, %v, %v, %v, %v) = %v, %v, %v; want %v, true, nil`, gce, ipName, serviceName, gce.region, ip, ipPrime, existed, err, ip) } } func TestEnsureStaticIPWithTier(t *testing.T) { - projectID := "test-project" - region := "us-central1" - zoneName := "us-central1-b" - - s, err := fakeGCECloud(projectID, region, zoneName) + s, err := fakeGCECloud(DefaultTestClusterValues()) require.NoError(t, err) + serviceName := "some-service" + for desc, tc := range map[string]struct { name string netTier cloud.NetworkTier @@ -81,13 +74,13 @@ func TestEnsureStaticIPWithTier(t *testing.T) { }, } { t.Run(desc, func(t *testing.T) { - ip, existed, err := ensureStaticIP(s, tc.name, serviceName, region, "", tc.netTier) + ip, existed, err := ensureStaticIP(s, tc.name, serviceName, s.region, "", tc.netTier) assert.NoError(t, err) assert.False(t, existed) assert.NotEqual(t, ip, "") // Get the Address from the fake address service and verify that the tier // is set correctly. - alphaAddr, err := s.GetAlphaRegionAddress(tc.name, region) + alphaAddr, err := s.GetAlphaRegionAddress(tc.name, s.region) require.NoError(t, err) assert.Equal(t, tc.expected, alphaAddr.NetworkTier) }) @@ -96,7 +89,6 @@ func TestEnsureStaticIPWithTier(t *testing.T) { func TestVerifyRequestedIP(t *testing.T) { lbRef := "test-lb" - region := "us-central1" for desc, tc := range map[string]struct { requestedIP string @@ -137,13 +129,13 @@ func TestVerifyRequestedIP(t *testing.T) { }, } { t.Run(desc, func(t *testing.T) { - s, err := fakeGCECloud("test-project", region, "us-central1a") + s, err := fakeGCECloud(DefaultTestClusterValues()) require.NoError(t, err) for _, addr := range tc.addrList { - s.ReserveAlphaRegionAddress(addr, region) + s.ReserveAlphaRegionAddress(addr, s.region) } - isUserOwnedIP, err := verifyUserRequestedIP(s, region, tc.requestedIP, tc.fwdRuleIP, lbRef, tc.netTier) + isUserOwnedIP, err := verifyUserRequestedIP(s, s.region, tc.requestedIP, tc.fwdRuleIP, lbRef, tc.netTier) assert.Equal(t, tc.expectErr, err != nil, fmt.Sprintf("err: %v", err)) assert.Equal(t, tc.expectUserOwned, isUserOwnedIP) }) @@ -154,9 +146,8 @@ func TestCreateForwardingRuleWithTier(t *testing.T) { // Common variables among the tests. ports := []v1.ServicePort{{Name: "foo", Protocol: v1.ProtocolTCP, Port: int32(123)}} target := "test-target-pool" - svcName := "foo-svc" - projectID := "test-project" - region := "us-central1" + vals := DefaultTestClusterValues() + serviceName := "foo-svc" baseLinkUrl := "https://www.googleapis.com/compute/%v/projects/%v/regions/%v/forwardingRules/%v" @@ -174,7 +165,7 @@ func TestCreateForwardingRuleWithTier(t *testing.T) { PortRange: "123-123", Target: target, NetworkTier: "PREMIUM", - SelfLink: fmt.Sprintf(baseLinkUrl, "v1", projectID, region, "lb-1"), + SelfLink: fmt.Sprintf(baseLinkUrl, "v1", vals.ProjectID, vals.Region, "lb-1"), }, }, "Standard tier": { @@ -187,21 +178,21 @@ func TestCreateForwardingRuleWithTier(t *testing.T) { PortRange: "123-123", Target: target, NetworkTier: "STANDARD", - SelfLink: fmt.Sprintf(baseLinkUrl, "alpha", projectID, region, "lb-2"), + SelfLink: fmt.Sprintf(baseLinkUrl, "alpha", vals.ProjectID, vals.Region, "lb-2"), }, }, } { t.Run(desc, func(t *testing.T) { - s, err := fakeGCECloud(projectID, region, "us-central1a") + s, err := fakeGCECloud(vals) require.NoError(t, err) lbName := tc.expectedRule.Name ipAddr := tc.expectedRule.IPAddress - err = createForwardingRule(s, lbName, svcName, region, ipAddr, target, ports, tc.netTier) + err = createForwardingRule(s, lbName, serviceName, s.region, ipAddr, target, ports, tc.netTier) assert.NoError(t, err) - alphaRule, err := s.GetAlphaRegionForwardingRule(lbName, region) + alphaRule, err := s.GetAlphaRegionForwardingRule(lbName, s.region) assert.NoError(t, err) assert.Equal(t, tc.expectedRule, alphaRule) }) @@ -210,9 +201,8 @@ func TestCreateForwardingRuleWithTier(t *testing.T) { func TestDeleteAddressWithWrongTier(t *testing.T) { lbRef := "test-lb" - region := "us-central1" - s, err := fakeGCECloud("test-region", region, "us-central1a") + s, err := fakeGCECloud(DefaultTestClusterValues()) require.NoError(t, err) // Enable the cloud.NetworkTiers feature @@ -249,17 +239,17 @@ func TestDeleteAddressWithWrongTier(t *testing.T) { } { t.Run(desc, func(t *testing.T) { for _, addr := range tc.addrList { - s.ReserveAlphaRegionAddress(addr, region) + s.ReserveAlphaRegionAddress(addr, s.region) } // Sanity check to ensure we inject the right address. - _, err = s.GetRegionAddress(tc.addrName, region) + _, err = s.GetRegionAddress(tc.addrName, s.region) require.NoError(t, err) - err = deleteAddressWithWrongTier(s, region, tc.addrName, lbRef, tc.netTier) + err = deleteAddressWithWrongTier(s, s.region, tc.addrName, lbRef, tc.netTier) assert.NoError(t, err) // Check whether the address still exists. - _, err = s.GetRegionAddress(tc.addrName, region) + _, err = s.GetRegionAddress(tc.addrName, s.region) if tc.expectDelete { assert.True(t, isNotFound(err)) } else { @@ -285,27 +275,23 @@ func createExternalLoadBalancer(gce *GCECloud, nodeNames []string, clusterName, } func TestEnsureExternalLoadBalancer(t *testing.T) { - projectID := "test-project" - region := "us-central1" - zoneName := "us-central1-b" - clusterName := "Test Cluster Name" - clusterID := "test-cluster-id" + vals := DefaultTestClusterValues() nodeName := "test-node-1" - gce, err := fakeGCECloud(projectID, region, zoneName) + gce, err := fakeGCECloud(vals) require.NoError(t, err) - status, err := createExternalLoadBalancer(gce, []string{nodeName}, clusterName, clusterID, zoneName) + status, err := createExternalLoadBalancer(gce, []string{nodeName}, vals.ClusterName, vals.ClusterID, vals.ZoneName) assert.NoError(t, err) assert.NotEmpty(t, status.Ingress) lbName := cloudprovider.GetLoadBalancerName(fakeApiService) - hcName := MakeNodesHealthCheckName(clusterID) + hcName := MakeNodesHealthCheckName(vals.ClusterID) // Check that Firewalls are created for the LoadBalancer and the HealthCheck fwNames := []string{ MakeFirewallName(lbName), - MakeHealthCheckFirewallName(clusterID, hcName, true), + MakeHealthCheckFirewallName(vals.ClusterID, hcName, true), } for _, fwName := range fwNames { @@ -316,7 +302,7 @@ func TestEnsureExternalLoadBalancer(t *testing.T) { } // Check that TargetPool is Created - pool, err := gce.GetTargetPool(lbName, region) + pool, err := gce.GetTargetPool(lbName, gce.region) require.NoError(t, err) assert.Equal(t, lbName, pool.Name) assert.NotEmpty(t, pool.HealthChecks) @@ -328,7 +314,7 @@ func TestEnsureExternalLoadBalancer(t *testing.T) { assert.Equal(t, hcName, healthcheck.Name) // Check that ForwardingRule is created - fwdRule, err := gce.GetRegionForwardingRule(lbName, region) + fwdRule, err := gce.GetRegionForwardingRule(lbName, gce.region) require.NoError(t, err) assert.Equal(t, lbName, fwdRule.Name) assert.Equal(t, "TCP", fwdRule.IPProtocol) @@ -336,22 +322,17 @@ func TestEnsureExternalLoadBalancer(t *testing.T) { } func TestUpdateExternalLoadBalancer(t *testing.T) { - projectID := "test-project" - region := "us-central1" - zoneName := "us-central1-b" - clusterName := "Test Cluster Name" - clusterID := "test-cluster-1" - + vals := DefaultTestClusterValues() nodeName := "test-node-1" - gce, err := fakeGCECloud(projectID, region, zoneName) + gce, err := fakeGCECloud((DefaultTestClusterValues())) require.NoError(t, err) - _, err = createExternalLoadBalancer(gce, []string{nodeName}, clusterName, clusterID, zoneName) + _, err = createExternalLoadBalancer(gce, []string{nodeName}, vals.ClusterName, vals.ClusterID, vals.ZoneName) assert.NoError(t, err) newNodeName := "test-node-2" - newNodes, err := createAndInsertNodes(gce, []string{nodeName, newNodeName}, zoneName) + newNodes, err := createAndInsertNodes(gce, []string{nodeName, newNodeName}, vals.ZoneName) assert.NoError(t, err) // Add the new node, then check that it is properly added to the TargetPool @@ -360,64 +341,59 @@ func TestUpdateExternalLoadBalancer(t *testing.T) { lbName := cloudprovider.GetLoadBalancerName(fakeApiService) - pool, err := gce.GetTargetPool(lbName, region) + pool, err := gce.GetTargetPool(lbName, gce.region) require.NoError(t, err) // TODO: when testify is updated to v1.2.0+, use ElementsMatch instead assert.Contains( t, pool.Instances, - fmt.Sprintf("/zones/%s/instances/%s", zoneName, nodeName), + fmt.Sprintf("/zones/%s/instances/%s", vals.ZoneName, nodeName), ) assert.Contains( t, pool.Instances, - fmt.Sprintf("/zones/%s/instances/%s", zoneName, newNodeName), + fmt.Sprintf("/zones/%s/instances/%s", vals.ZoneName, newNodeName), ) - newNodes, err = createAndInsertNodes(gce, []string{nodeName}, zoneName) + newNodes, err = createAndInsertNodes(gce, []string{nodeName}, vals.ZoneName) assert.NoError(t, err) // Remove the new node by calling updateExternalLoadBalancer with a list // only containing the old node, and test that the TargetPool no longer // contains the new node. - err = gce.updateExternalLoadBalancer(clusterName, fakeApiService, newNodes) + err = gce.updateExternalLoadBalancer(vals.ClusterName, fakeApiService, newNodes) assert.NoError(t, err) - pool, err = gce.GetTargetPool(lbName, region) + pool, err = gce.GetTargetPool(lbName, gce.region) require.NoError(t, err) assert.Equal( t, - []string{fmt.Sprintf("/zones/%s/instances/%s", zoneName, nodeName)}, + []string{fmt.Sprintf("/zones/%s/instances/%s", vals.ZoneName, nodeName)}, pool.Instances, ) } func TestEnsureExternalLoadBalancerDeleted(t *testing.T) { - projectID := "test-project" - region := "us-central1" - zoneName := "us-central1-b" - clusterName := "Test Cluster Name" - clusterID := "test-cluster-id" - - gce, err := fakeGCECloud(projectID, region, zoneName) + vals := DefaultTestClusterValues() + gce, err := fakeGCECloud(vals) require.NoError(t, err) - _, err = createExternalLoadBalancer(gce, []string{"test-node-1"}, clusterName, clusterID, zoneName) + _, err = createExternalLoadBalancer(gce, []string{"test-node-1"}, vals.ClusterName, vals.ClusterID, vals.ZoneName) assert.NoError(t, err) - err = gce.ensureExternalLoadBalancerDeleted(clusterName, clusterID, fakeApiService) + err = gce.ensureExternalLoadBalancerDeleted(vals.ClusterName, vals.ClusterID, fakeApiService) assert.NoError(t, err) lbName := cloudprovider.GetLoadBalancerName(fakeApiService) - hcName := MakeNodesHealthCheckName(clusterID) + hcName := MakeNodesHealthCheckName(vals.ClusterID) // Check that Firewalls are deleted for the LoadBalancer and the HealthCheck fwNames := []string{ MakeFirewallName(lbName), - MakeHealthCheckFirewallName(clusterID, hcName, true), + MakeHealthCheckFirewallName(vals.ClusterID, hcName, true), } for _, fwName := range fwNames { @@ -427,7 +403,7 @@ func TestEnsureExternalLoadBalancerDeleted(t *testing.T) { } // Check that TargetPool is deleted - pool, err := gce.GetTargetPool(lbName, region) + pool, err := gce.GetTargetPool(lbName, gce.region) require.Error(t, err) assert.Nil(t, pool) @@ -437,19 +413,14 @@ func TestEnsureExternalLoadBalancerDeleted(t *testing.T) { assert.Nil(t, healthcheck) // Check forwarding rule is deleted - fwdRule, err := gce.GetRegionForwardingRule(lbName, region) + fwdRule, err := gce.GetRegionForwardingRule(lbName, gce.region) require.Error(t, err) assert.Nil(t, fwdRule) } func TestLoadBalancerWrongTierResourceDeletion(t *testing.T) { - projectID := "test-project" - region := "us-central1" - zoneName := "us-central1-b" - clusterName := "Test Cluster Name" - clusterID := "test-cluster-id" - - gce, err := fakeGCECloud(projectID, region, zoneName) + vals := DefaultTestClusterValues() + gce, err := fakeGCECloud(vals) require.NoError(t, err) // Enable the cloud.NetworkTiers feature @@ -469,7 +440,7 @@ func TestLoadBalancerWrongTierResourceDeletion(t *testing.T) { gce, lbName, serviceName.String(), - region, + gce.region, "", gce.targetPoolURL(lbName), fakeApiService.Spec.Ports, @@ -483,18 +454,18 @@ func TestLoadBalancerWrongTierResourceDeletion(t *testing.T) { NetworkTier: cloud.NetworkTierStandard.ToGCEValue(), } - err = gce.ReserveAlphaRegionAddress(addressObj, region) + err = gce.ReserveAlphaRegionAddress(addressObj, gce.region) require.NoError(t, err) - _, err = createExternalLoadBalancer(gce, []string{"test-node-1"}, clusterName, clusterID, zoneName) + _, err = createExternalLoadBalancer(gce, []string{"test-node-1"}, vals.ClusterName, vals.ClusterID, vals.ZoneName) require.NoError(t, err) // Expect forwarding rule tier to not be Standard - tier, err := gce.getNetworkTierFromForwardingRule(lbName, region) + tier, err := gce.getNetworkTierFromForwardingRule(lbName, gce.region) assert.NoError(t, err) assert.Equal(t, cloud.NetworkTierDefault.ToGCEValue(), tier) // Expect address to be deleted - _, err = gce.GetRegionAddress(lbName, region) + _, err = gce.GetRegionAddress(lbName, gce.region) assert.True(t, isNotFound(err)) } diff --git a/pkg/cloudprovider/providers/gce/gce_loadbalancer_internal_test.go b/pkg/cloudprovider/providers/gce/gce_loadbalancer_internal_test.go index 443e780abbd..90cefc90190 100644 --- a/pkg/cloudprovider/providers/gce/gce_loadbalancer_internal_test.go +++ b/pkg/cloudprovider/providers/gce/gce_loadbalancer_internal_test.go @@ -51,17 +51,17 @@ func TestEnsureInternalBackendServiceUpdates(t *testing.T) { vals := DefaultTestClusterValues() nodeNames := []string{"test-node-1"} - gce, err := fakeGCECloud(projectID, region, zoneName) + gce, err := fakeGCECloud(vals) require.NoError(t, err) lbName := cloudprovider.GetLoadBalancerName(fakeApiService) - nodes, err := createAndInsertNodes(gce, nodeNames, zoneName) - igName := makeInstanceGroupName(clusterID) + nodes, err := createAndInsertNodes(gce, nodeNames, vals.ZoneName) + igName := makeInstanceGroupName(vals.ClusterID) igLinks, err := gce.ensureInternalInstanceGroups(igName, nodes) require.NoError(t, err) sharedBackend := shareBackendService(fakeApiService) - bsName := makeBackendServiceName(lbName, clusterID, sharedBackend, cloud.SchemeInternal, "TCP", fakeApiService.Spec.SessionAffinity) + bsName := makeBackendServiceName(lbName, vals.ClusterID, sharedBackend, cloud.SchemeInternal, "TCP", fakeApiService.Spec.SessionAffinity) err = gce.ensureInternalBackendService(bsName, "description", fakeApiService.Spec.SessionAffinity, cloud.SchemeInternal, "TCP", igLinks, "") require.NoError(t, err) @@ -75,23 +75,20 @@ func TestEnsureInternalBackendServiceUpdates(t *testing.T) { } func TestEnsureInternalBackendServiceGroups(t *testing.T) { - projectID := "test-project" - region := "us-central1" - zoneName := "us-central1-b" - clusterID := "test-cluster-id" + vals := DefaultTestClusterValues() nodeNames := []string{"test-node-1"} - gce, err := fakeGCECloud(projectID, region, zoneName) + gce, err := fakeGCECloud(vals) require.NoError(t, err) lbName := cloudprovider.GetLoadBalancerName(fakeApiService) - nodes, err := createAndInsertNodes(gce, nodeNames, zoneName) - igName := makeInstanceGroupName(clusterID) + nodes, err := createAndInsertNodes(gce, nodeNames, vals.ZoneName) + igName := makeInstanceGroupName(vals.ClusterID) igLinks, err := gce.ensureInternalInstanceGroups(igName, nodes) require.NoError(t, err) sharedBackend := shareBackendService(fakeApiService) - bsName := makeBackendServiceName(lbName, clusterID, sharedBackend, cloud.SchemeInternal, "TCP", fakeApiService.Spec.SessionAffinity) + bsName := makeBackendServiceName(lbName, vals.ClusterID, sharedBackend, cloud.SchemeInternal, "TCP", fakeApiService.Spec.SessionAffinity) err = gce.ensureInternalBackendService(bsName, "description", fakeApiService.Spec.SessionAffinity, cloud.SchemeInternal, "TCP", igLinks, "") require.NoError(t, err) @@ -104,39 +101,35 @@ func TestEnsureInternalBackendServiceGroups(t *testing.T) { assert.NoError(t, err) // Check that the instances are updated - newNodes, err := createAndInsertNodes(gce, newNodeNames, zoneName) + newNodes, err := createAndInsertNodes(gce, newNodeNames, vals.ZoneName) newIgLinks, err := gce.ensureInternalInstanceGroups(igName, newNodes) backends := backendsFromGroupLinks(newIgLinks) assert.Equal(t, bs.Backends, backends) } func TestEnsureInternalLoadBalancer(t *testing.T) { - projectID := "test-project" - region := "us-central1" - zoneName := "us-central1-b" - clusterName := "Test Cluster" - clusterID := "test-cluster-id" + vals := DefaultTestClusterValues() nodeName := "test-node-1" - gce, err := fakeGCECloud(projectID, region, zoneName) + gce, err := fakeGCECloud(vals) require.NoError(t, err) - status, err := createInternalLoadBalancer(gce, nil, []string{nodeName}, clusterName, clusterID, zoneName) + status, err := createInternalLoadBalancer(gce, nil, []string{nodeName}, vals.ClusterName, vals.ClusterID, vals.ZoneName) assert.NoError(t, err) assert.NotEmpty(t, status.Ingress) lbName := cloudprovider.GetLoadBalancerName(fakeApiService) // Check that Instance Group is created - igName := makeInstanceGroupName(clusterID) - ig, err := gce.GetInstanceGroup(igName, zoneName) + igName := makeInstanceGroupName(vals.ClusterID) + ig, err := gce.GetInstanceGroup(igName, vals.ZoneName) assert.NoError(t, err) assert.Equal(t, igName, ig.Name) // Check that Firewalls are created for the LoadBalancer and the HealthCheck fwNames := []string{ lbName, - makeHealthCheckFirewallName(lbName, clusterID, true), + makeHealthCheckFirewallName(lbName, vals.ClusterID, true), } for _, fwName := range fwNames { @@ -148,14 +141,14 @@ func TestEnsureInternalLoadBalancer(t *testing.T) { // Check that HealthCheck is created sharedHealthCheck := !v1_service.RequestsOnlyLocalTraffic(fakeApiService) - hcName := makeHealthCheckName(lbName, clusterID, sharedHealthCheck) + hcName := makeHealthCheckName(lbName, vals.ClusterID, sharedHealthCheck) healthcheck, err := gce.GetHealthCheck(hcName) require.NoError(t, err) assert.Equal(t, hcName, healthcheck.Name) // Check that BackendService exists sharedBackend := shareBackendService(fakeApiService) - backendServiceName := makeBackendServiceName(lbName, clusterID, sharedBackend, cloud.SchemeInternal, "TCP", fakeApiService.Spec.SessionAffinity) + backendServiceName := makeBackendServiceName(lbName, vals.ClusterID, sharedBackend, cloud.SchemeInternal, "TCP", fakeApiService.Spec.SessionAffinity) backendServiceLink := gce.getBackendServiceLink(backendServiceName) bs, err := gce.GetRegionBackendService(backendServiceName, gce.region) @@ -168,7 +161,7 @@ func TestEnsureInternalLoadBalancer(t *testing.T) { ) // Check that ForwardingRule is created - fwdRule, err := gce.GetRegionForwardingRule(lbName, region) + fwdRule, err := gce.GetRegionForwardingRule(lbName, gce.region) require.NoError(t, err) assert.Equal(t, lbName, fwdRule.Name) assert.Equal(t, "TCP", fwdRule.IPProtocol) @@ -178,14 +171,10 @@ func TestEnsureInternalLoadBalancer(t *testing.T) { } func TestEnsureInternalLoadBalancerWithExistingResources(t *testing.T) { - projectID := "test-project" - region := "us-central1" - zoneName := "us-central1-b" - clusterName := "Test Cluster" - clusterID := "test-cluster-id" + vals := DefaultTestClusterValues() nodeNames := []string{"test-node-1"} - gce, err := fakeGCECloud(projectID, region, zoneName) + gce, err := fakeGCECloud(vals) require.NoError(t, err) // Create the expected resources necessary for an Internal Load Balancer @@ -193,35 +182,30 @@ func TestEnsureInternalLoadBalancerWithExistingResources(t *testing.T) { lbName := cloudprovider.GetLoadBalancerName(fakeApiService) sharedHealthCheck := !v1_service.RequestsOnlyLocalTraffic(fakeApiService) - hcName := makeHealthCheckName(lbName, clusterID, sharedHealthCheck) + hcName := makeHealthCheckName(lbName, vals.ClusterID, sharedHealthCheck) hcPath, hcPort := GetNodesHealthCheckPath(), GetNodesHealthCheckPort() existingHC := newInternalLBHealthCheck(hcName, nm, sharedHealthCheck, hcPath, hcPort) err = gce.CreateHealthCheck(existingHC) require.NoError(t, err) - nodes, err := createAndInsertNodes(gce, nodeNames, zoneName) - igName := makeInstanceGroupName(clusterID) + nodes, err := createAndInsertNodes(gce, nodeNames, vals.ZoneName) + igName := makeInstanceGroupName(vals.ClusterID) igLinks, err := gce.ensureInternalInstanceGroups(igName, nodes) require.NoError(t, err) sharedBackend := shareBackendService(fakeApiService) bsDescription := makeBackendServiceDescription(nm, sharedBackend) - bsName := makeBackendServiceName(lbName, clusterID, sharedBackend, cloud.SchemeInternal, "TCP", fakeApiService.Spec.SessionAffinity) + bsName := makeBackendServiceName(lbName, vals.ClusterID, sharedBackend, cloud.SchemeInternal, "TCP", fakeApiService.Spec.SessionAffinity) err = gce.ensureInternalBackendService(bsName, bsDescription, fakeApiService.Spec.SessionAffinity, cloud.SchemeInternal, "TCP", igLinks, existingHC.SelfLink) require.NoError(t, err) - _, err = createInternalLoadBalancer(gce, nil, nodeNames, clusterName, clusterID, zoneName) + _, err = createInternalLoadBalancer(gce, nil, nodeNames, vals.ClusterName, vals.ClusterID, vals.ZoneName) assert.NoError(t, err) } func TestEnsureInternalLoadBalancerClearPreviousResources(t *testing.T) { - projectID := "test-project" - region := "us-central1" - zoneName := "us-central1-b" - clusterName := "Test Cluster" - clusterID := "test-cluster-id" - - gce, err := fakeGCECloud(projectID, region, zoneName) + vals := DefaultTestClusterValues() + gce, err := fakeGCECloud(vals) require.NoError(t, err) lbName := cloudprovider.GetLoadBalancerName(fakeApiService) @@ -250,7 +234,7 @@ func TestEnsureInternalLoadBalancerClearPreviousResources(t *testing.T) { gce.CreateFirewall(existingFirewall) sharedHealthCheck := !v1_service.RequestsOnlyLocalTraffic(fakeApiService) - hcName := makeHealthCheckName(lbName, clusterID, sharedHealthCheck) + hcName := makeHealthCheckName(lbName, vals.ClusterID, sharedHealthCheck) hcPath, hcPort := GetNodesHealthCheckPath(), GetNodesHealthCheckPort() nm := types.NamespacedName{Name: fakeApiService.Name, Namespace: fakeApiService.Namespace} @@ -261,7 +245,7 @@ func TestEnsureInternalLoadBalancerClearPreviousResources(t *testing.T) { // Create a backend Service that's missing Description and Backends sharedBackend := shareBackendService(fakeApiService) - backendServiceName := makeBackendServiceName(lbName, clusterID, sharedBackend, cloud.SchemeInternal, "TCP", fakeApiService.Spec.SessionAffinity) + backendServiceName := makeBackendServiceName(lbName, vals.ClusterID, sharedBackend, cloud.SchemeInternal, "TCP", fakeApiService.Spec.SessionAffinity) existingBS := &compute.BackendService{ Name: lbName, Protocol: "TCP", @@ -273,7 +257,7 @@ func TestEnsureInternalLoadBalancerClearPreviousResources(t *testing.T) { gce.CreateRegionBackendService(existingBS, gce.region) existingFwdRule.BackendService = existingBS.Name - _, err = createInternalLoadBalancer(gce, existingFwdRule, []string{"test-node-1"}, clusterName, clusterID, zoneName) + _, err = createInternalLoadBalancer(gce, existingFwdRule, []string{"test-node-1"}, vals.ClusterName, vals.ClusterID, vals.ZoneName) assert.NoError(t, err) // Expect new resources with the correct attributes to be created @@ -294,18 +278,13 @@ func TestEnsureInternalLoadBalancerClearPreviousResources(t *testing.T) { } func TestUpdateInternalLoadBalancerBackendServices(t *testing.T) { - projectID := "test-project" - region := "us-central1" - zoneName := "us-central1-b" - clusterName := "Test Cluster Name" - clusterID := "test-cluster-id" - + vals := DefaultTestClusterValues() nodeName := "test-node-1" - gce, err := fakeGCECloud(projectID, region, zoneName) + gce, err := fakeGCECloud(vals) require.NoError(t, err) - _, err = createInternalLoadBalancer(gce, nil, []string{"test-node-1"}, clusterName, clusterID, zoneName) + _, err = createInternalLoadBalancer(gce, nil, []string{"test-node-1"}, vals.ClusterName, vals.ClusterID, vals.ZoneName) assert.NoError(t, err) // BackendService exists prior to updateInternalLoadBalancer call, but has @@ -314,7 +293,7 @@ func TestUpdateInternalLoadBalancerBackendServices(t *testing.T) { // BackendService lbName := cloudprovider.GetLoadBalancerName(fakeApiService) sharedBackend := shareBackendService(fakeApiService) - backendServiceName := makeBackendServiceName(lbName, clusterID, sharedBackend, cloud.SchemeInternal, "TCP", fakeApiService.Spec.SessionAffinity) + backendServiceName := makeBackendServiceName(lbName, vals.ClusterID, sharedBackend, cloud.SchemeInternal, "TCP", fakeApiService.Spec.SessionAffinity) existingBS := &compute.BackendService{ Name: backendServiceName, Protocol: "TCP", @@ -324,10 +303,10 @@ func TestUpdateInternalLoadBalancerBackendServices(t *testing.T) { gce.CreateRegionBackendService(existingBS, gce.region) - nodes, err := createAndInsertNodes(gce, []string{nodeName}, zoneName) + nodes, err := createAndInsertNodes(gce, []string{nodeName}, vals.ZoneName) require.NoError(t, err) - err = gce.updateInternalLoadBalancer(clusterName, clusterID, fakeApiService, nodes) + err = gce.updateInternalLoadBalancer(vals.ClusterName, vals.ClusterID, fakeApiService, nodes) assert.NoError(t, err) bs, err := gce.GetRegionBackendService(backendServiceName, gce.region) @@ -351,63 +330,53 @@ func TestUpdateInternalLoadBalancerBackendServices(t *testing.T) { } func TestUpdateInternalLoadBalancerNodes(t *testing.T) { - projectID := "test-project" - region := "us-central1" - zoneName := "us-central1-b" - clusterName := "Test Cluster Name" - clusterID := "test-cluster-id" - - gce, err := fakeGCECloud(projectID, region, zoneName) + vals := DefaultTestClusterValues() + gce, err := fakeGCECloud(vals) require.NoError(t, err) - _, err = createInternalLoadBalancer(gce, nil, []string{"test-node-1"}, clusterName, clusterID, zoneName) + _, err = createInternalLoadBalancer(gce, nil, []string{"test-node-1"}, vals.ClusterName, vals.ClusterID, vals.ZoneName) assert.NoError(t, err) // Remove the old Node and insert a new Node. newNodeName := "test-node-2" - newNodes, err := createAndInsertNodes(gce, []string{newNodeName}, zoneName) + newNodes, err := createAndInsertNodes(gce, []string{newNodeName}, vals.ZoneName) require.NoError(t, err) - err = gce.updateInternalLoadBalancer(clusterName, clusterID, fakeApiService, newNodes) + err = gce.updateInternalLoadBalancer(vals.ClusterName, vals.ClusterID, fakeApiService, newNodes) assert.NoError(t, err) // Expect node 1 to be deleted and node 2 to still exist - igName := makeInstanceGroupName(clusterID) - instances, err := gce.ListInstancesInInstanceGroup(igName, zoneName, "ALL") + igName := makeInstanceGroupName(vals.ClusterID) + instances, err := gce.ListInstancesInInstanceGroup(igName, vals.ZoneName, "ALL") require.NoError(t, err) assert.Equal(t, 1, len(instances)) assert.Contains( t, instances[0].Instance, - fmt.Sprintf("projects/%s/zones/%s/instances/%s", projectID, zoneName, newNodeName), + fmt.Sprintf("projects/%s/zones/%s/instances/%s", vals.ProjectID, vals.ZoneName, newNodeName), ) } func TestEnsureInternalLoadBalancerDeleted(t *testing.T) { - projectID := "test-project" - region := "us-central1" - zoneName := "us-central1-b" - clusterName := "Test Cluster Name" - clusterID := "test-cluster-id" - - gce, err := fakeGCECloud(projectID, region, zoneName) + vals := DefaultTestClusterValues() + gce, err := fakeGCECloud(vals) require.NoError(t, err) - _, err = createInternalLoadBalancer(gce, nil, []string{"test-node-1"}, clusterName, clusterID, zoneName) + _, err = createInternalLoadBalancer(gce, nil, []string{"test-node-1"}, vals.ClusterName, vals.ClusterID, vals.ZoneName) assert.NoError(t, err) - err = gce.ensureInternalLoadBalancerDeleted(clusterName, clusterID, fakeApiService) + err = gce.ensureInternalLoadBalancerDeleted(vals.ClusterName, vals.ClusterID, fakeApiService) assert.NoError(t, err) lbName := cloudprovider.GetLoadBalancerName(fakeApiService) sharedHealthCheck := !v1_service.RequestsOnlyLocalTraffic(fakeApiService) - hcName := makeHealthCheckName(lbName, clusterID, sharedHealthCheck) + hcName := makeHealthCheckName(lbName, vals.ClusterID, sharedHealthCheck) // Check that Firewalls are deleted for the LoadBalancer and the HealthCheck fwNames := []string{ MakeFirewallName(lbName), - MakeHealthCheckFirewallName(clusterID, hcName, true), + MakeHealthCheckFirewallName(vals.ClusterID, hcName, true), } for _, fwName := range fwNames { @@ -417,8 +386,8 @@ func TestEnsureInternalLoadBalancerDeleted(t *testing.T) { } // Check that Instance Group is deleted - igName := makeInstanceGroupName(clusterID) - ig, err := gce.GetInstanceGroup(igName, zoneName) + igName := makeInstanceGroupName(vals.ClusterID) + ig, err := gce.GetInstanceGroup(igName, vals.ZoneName) assert.Error(t, err) assert.Nil(t, ig) @@ -428,28 +397,23 @@ func TestEnsureInternalLoadBalancerDeleted(t *testing.T) { assert.Nil(t, healthcheck) // Check forwarding rule is deleted - fwdRule, err := gce.GetRegionForwardingRule(lbName, region) + fwdRule, err := gce.GetRegionForwardingRule(lbName, gce.region) require.Error(t, err) assert.Nil(t, fwdRule) } func TestEnsureInternalLoadBalancerDeletedTwiceDoesNotError(t *testing.T) { - projectID := "test-project" - region := "us-central1" - zoneName := "us-central1-b" - clusterName := "Test Cluster Name" - clusterID := "test-cluster-id" - - gce, err := fakeGCECloud(projectID, region, zoneName) + vals := DefaultTestClusterValues() + gce, err := fakeGCECloud(vals) require.NoError(t, err) - _, err = createInternalLoadBalancer(gce, nil, []string{"test-node-1"}, clusterName, clusterID, zoneName) + _, err = createInternalLoadBalancer(gce, nil, []string{"test-node-1"}, vals.ClusterName, vals.ClusterID, vals.ZoneName) assert.NoError(t, err) - err = gce.ensureInternalLoadBalancerDeleted(clusterName, clusterID, fakeApiService) + err = gce.ensureInternalLoadBalancerDeleted(vals.ClusterName, vals.ClusterID, fakeApiService) assert.NoError(t, err) // Deleting the loadbalancer and resources again should not cause an error. - err = gce.ensureInternalLoadBalancerDeleted(clusterName, clusterID, fakeApiService) + err = gce.ensureInternalLoadBalancerDeleted(vals.ClusterName, vals.ClusterID, fakeApiService) assert.NoError(t, err) } diff --git a/pkg/cloudprovider/providers/gce/gce_loadbalancer_utils_test.go b/pkg/cloudprovider/providers/gce/gce_loadbalancer_utils_test.go index 65b7900163a..4813cbedb69 100644 --- a/pkg/cloudprovider/providers/gce/gce_loadbalancer_utils_test.go +++ b/pkg/cloudprovider/providers/gce/gce_loadbalancer_utils_test.go @@ -34,6 +34,24 @@ import ( kubeletapis "k8s.io/kubernetes/pkg/kubelet/apis" ) +type TestClusterValues struct { + ProjectID string + Region string + ZoneName string + ClusterID string + ClusterName string +} + +func DefaultTestClusterValues() TestClusterValues { + return TestClusterValues{ + ProjectID: "test-project", + Region: "us-central1", + ZoneName: "us-central1-b", + ClusterID: "test-cluster-id", + ClusterName: "Test Cluster Name", + } +} + var fakeApiService = &v1.Service{ Spec: v1.ServiceSpec{ SessionAffinity: v1.ServiceAffinityClientIP, @@ -48,7 +66,7 @@ func (*fakeRoundTripper) RoundTrip(*http.Request) (*http.Response, error) { return nil, fmt.Errorf("err: test used fake http client") } -func fakeGCECloud(projectID, region, zoneName string) (*GCECloud, error) { +func fakeGCECloud(vals TestClusterValues) (*GCECloud, error) { client := &http.Client{Transport: &fakeRoundTripper{}} service, err := compute.New(client) @@ -57,8 +75,8 @@ func fakeGCECloud(projectID, region, zoneName string) (*GCECloud, error) { } // Used in disk unit tests - fakeManager := newFakeManager(projectID, region) - zonesWithNodes := createNodeZones([]string{zoneName}) + fakeManager := newFakeManager(vals.ProjectID, vals.Region) + zonesWithNodes := createNodeZones([]string{vals.ZoneName}) alphaFeatureGate, err := NewAlphaFeatureGate([]string{}) if err != nil { @@ -66,12 +84,12 @@ func fakeGCECloud(projectID, region, zoneName string) (*GCECloud, error) { } gce := &GCECloud{ - region: region, + region: vals.Region, service: service, manager: fakeManager, - managedZones: []string{zoneName}, - projectID: projectID, - networkProjectID: projectID, + managedZones: []string{vals.ZoneName}, + projectID: vals.ProjectID, + networkProjectID: vals.ProjectID, AlphaFeatureGate: alphaFeatureGate, nodeZones: zonesWithNodes, nodeInformerSynced: func() bool { return true }, @@ -94,7 +112,7 @@ func fakeGCECloud(projectID, region, zoneName string) (*GCECloud, error) { keyGA := meta.GlobalKey("key-ga") c.MockZones.Objects[*keyGA] = &cloud.MockZonesObj{ - Obj: &compute.Zone{Name: zoneName, Region: gce.getRegionLink(region)}, + Obj: &compute.Zone{Name: vals.ZoneName, Region: gce.getRegionLink(vals.Region)}, } gce.c = c From e07a944c49622284a67cf44a98b83818ee464fdb Mon Sep 17 00:00:00 2001 From: Ashley Gau Date: Mon, 12 Mar 2018 13:20:33 -0700 Subject: [PATCH 11/11] add Get/Set methods, mutex on instanceGroupAttrs. --- .../providers/gce/cloud/mock/mock.go | 142 ++++++++++-------- .../gce/gce_loadbalancer_utils_test.go | 5 + 2 files changed, 84 insertions(+), 63 deletions(-) diff --git a/pkg/cloudprovider/providers/gce/cloud/mock/mock.go b/pkg/cloudprovider/providers/gce/cloud/mock/mock.go index c2c772d1431..acb1fb62ef8 100644 --- a/pkg/cloudprovider/providers/gce/cloud/mock/mock.go +++ b/pkg/cloudprovider/providers/gce/cloud/mock/mock.go @@ -28,6 +28,7 @@ import ( "encoding/json" "fmt" "net/http" + "sync" alpha "google.golang.org/api/compute/v0.alpha" beta "google.golang.org/api/compute/v0.beta" @@ -228,9 +229,75 @@ func InsertAlphaAddressHook(ctx context.Context, key *meta.Key, obj *alpha.Addre return convertAndInsertAlphaAddress(key, obj, m.Objects, meta.VersionAlpha, projectID) } -// Map from InstanceGroup key to list of Instances -type instanceGroupAttributes struct { - instanceMap map[meta.Key][]*ga.InstanceWithNamedPorts +// InstanceGroupAttributes maps from InstanceGroup key to a map of Instances +type InstanceGroupAttributes struct { + InstanceMap map[meta.Key]map[string]*ga.InstanceWithNamedPorts + Lock *sync.Mutex +} + +// AddInstances adds a list of Instances passed by InstanceReference +func (igAttrs *InstanceGroupAttributes) AddInstances(key *meta.Key, instanceRefs []*ga.InstanceReference) error { + igAttrs.Lock.Lock() + defer igAttrs.Lock.Unlock() + + instancesWithNamedPorts, ok := igAttrs.InstanceMap[*key] + if !ok { + instancesWithNamedPorts = make(map[string]*ga.InstanceWithNamedPorts) + } + + for _, instance := range instanceRefs { + iWithPort := &ga.InstanceWithNamedPorts{ + Instance: instance.Instance, + } + + instancesWithNamedPorts[instance.Instance] = iWithPort + } + + igAttrs.InstanceMap[*key] = instancesWithNamedPorts + return nil +} + +// RemoveInstances removes a list of Instances passed by InstanceReference +func (igAttrs *InstanceGroupAttributes) RemoveInstances(key *meta.Key, instanceRefs []*ga.InstanceReference) error { + igAttrs.Lock.Lock() + defer igAttrs.Lock.Unlock() + + instancesWithNamedPorts, ok := igAttrs.InstanceMap[*key] + if !ok { + instancesWithNamedPorts = make(map[string]*ga.InstanceWithNamedPorts) + } + + for _, instanceToRemove := range instanceRefs { + if _, ok := instancesWithNamedPorts[instanceToRemove.Instance]; ok { + delete(instancesWithNamedPorts, instanceToRemove.Instance) + } else { + return &googleapi.Error{ + Code: http.StatusBadRequest, + Message: fmt.Sprintf("%s is not a member of %s", instanceToRemove.Instance, key.String()), + } + } + } + + igAttrs.InstanceMap[*key] = instancesWithNamedPorts + return nil +} + +// List gets a list of InstanceWithNamedPorts +func (igAttrs *InstanceGroupAttributes) List(key *meta.Key) []*ga.InstanceWithNamedPorts { + igAttrs.Lock.Lock() + defer igAttrs.Lock.Unlock() + + instancesWithNamedPorts, ok := igAttrs.InstanceMap[*key] + if !ok { + instancesWithNamedPorts = make(map[string]*ga.InstanceWithNamedPorts) + } + + var instanceList []*ga.InstanceWithNamedPorts + for _, val := range instancesWithNamedPorts { + instanceList = append(instanceList, val) + } + + return instanceList } // AddInstancesHook mocks adding instances from an InstanceGroup @@ -243,29 +310,9 @@ func AddInstancesHook(ctx context.Context, key *meta.Key, req *ga.InstanceGroups } } - if m.X == nil { - m.X = instanceGroupAttributes{ - instanceMap: make(map[meta.Key][]*ga.InstanceWithNamedPorts), - } - } - - var attrs instanceGroupAttributes - attrs = m.X.(instanceGroupAttributes) - - instances, ok := attrs.instanceMap[*key] - if !ok { - instances = []*ga.InstanceWithNamedPorts{} - } - - for _, instance := range req.Instances { - iWithPort := &ga.InstanceWithNamedPorts{ - Instance: instance.Instance, - } - - instances = append(instances, iWithPort) - } - - attrs.instanceMap[*key] = instances + var attrs InstanceGroupAttributes + attrs = m.X.(InstanceGroupAttributes) + attrs.AddInstances(key, req.Instances) m.X = attrs return nil } @@ -280,17 +327,9 @@ func ListInstancesHook(ctx context.Context, key *meta.Key, req *ga.InstanceGroup } } - if m.X == nil { - m.X = instanceGroupAttributes{instanceMap: make(map[meta.Key][]*ga.InstanceWithNamedPorts)} - } - - var attrs instanceGroupAttributes - attrs = m.X.(instanceGroupAttributes) - - instances, ok := attrs.instanceMap[*key] - if !ok { - instances = []*ga.InstanceWithNamedPorts{} - } + var attrs InstanceGroupAttributes + attrs = m.X.(InstanceGroupAttributes) + instances := attrs.List(key) return instances, nil } @@ -305,32 +344,9 @@ func RemoveInstancesHook(ctx context.Context, key *meta.Key, req *ga.InstanceGro } } - if m.X == nil { - m.X = instanceGroupAttributes{ - instanceMap: make(map[meta.Key][]*ga.InstanceWithNamedPorts), - } - } - - var attrs instanceGroupAttributes - attrs = m.X.(instanceGroupAttributes) - - instances, ok := attrs.instanceMap[*key] - if !ok { - instances = []*ga.InstanceWithNamedPorts{} - } - - for _, instanceToRemove := range req.Instances { - for i, instance := range instances { - if instanceToRemove.Instance == instance.Instance { - // Delete instance from instances without preserving order - instances[i] = instances[len(instances)-1] - instances = instances[:len(instances)-1] - break - } - } - } - - attrs.instanceMap[*key] = instances + var attrs InstanceGroupAttributes + attrs = m.X.(InstanceGroupAttributes) + attrs.RemoveInstances(key, req.Instances) m.X = attrs return nil } diff --git a/pkg/cloudprovider/providers/gce/gce_loadbalancer_utils_test.go b/pkg/cloudprovider/providers/gce/gce_loadbalancer_utils_test.go index 4813cbedb69..1dcdb19beea 100644 --- a/pkg/cloudprovider/providers/gce/gce_loadbalancer_utils_test.go +++ b/pkg/cloudprovider/providers/gce/gce_loadbalancer_utils_test.go @@ -23,6 +23,7 @@ package gce import ( "fmt" "net/http" + "sync" compute "google.golang.org/api/compute/v1" @@ -102,6 +103,10 @@ func fakeGCECloud(vals TestClusterValues) (*GCECloud, error) { c.MockAddresses.InsertHook = mock.InsertAddressHook c.MockAlphaAddresses.InsertHook = mock.InsertAlphaAddressHook + c.MockInstanceGroups.X = mock.InstanceGroupAttributes{ + InstanceMap: make(map[meta.Key]map[string]*compute.InstanceWithNamedPorts), + Lock: &sync.Mutex{}, + } c.MockInstanceGroups.AddInstancesHook = mock.AddInstancesHook c.MockInstanceGroups.RemoveInstancesHook = mock.RemoveInstancesHook c.MockInstanceGroups.ListInstancesHook = mock.ListInstancesHook