Merge pull request #59490 from agau4779/gce-unittest

Automatic merge from submit-queue (batch tested with PRs 58437, 59490, 55684). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

[GCE] Unit test ExternalLoadBalancer

**What this PR does / why we need it**:

- Unit tests for `pkg/cloudprovider/providers/gce/gce_loadbalancer_external.go`
- Tests creating, updating, and deleting an external LoadBalancer

**Future Improvements**
In order to further test `gce_loadbalancer_external.go`, we should add tests for the following cases:

- Network Tiers - when the current/desired network tier doesn't match, existing resources with the wrong tier should be torn down
- Expect an error when the TargetPool does not exist
- Expect an error when the LoadBalancer Firewall does not exist
- Case when TargetPool needs to be recreated 
- Case when IP needs to be released (calls gce.DeleteRegionAddress)
```release-note
NONE
```

pkg/cloudprovider/providers/gce/BUILD

@@ -106,6 +106,8 @@ go_test(
     importpath = "k8s.io/kubernetes/pkg/cloudprovider/providers/gce",
     deps = [
         "//pkg/cloudprovider:go_default_library",
+        "//pkg/cloudprovider/providers/gce/cloud: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",
         "//vendor/github.com/stretchr/testify/require:go_default_library",

pkg/cloudprovider/providers/gce/cloud/BUILD

@@ -57,6 +57,7 @@ filegroup(
         "//pkg/cloudprovider/providers/gce/cloud/filter:all-srcs",
         "//pkg/cloudprovider/providers/gce/cloud/gen:all-srcs",
         "//pkg/cloudprovider/providers/gce/cloud/meta:all-srcs",
+        "//pkg/cloudprovider/providers/gce/cloud/mock:all-srcs",
     ],
     tags = ["automanaged"],
     visibility = ["//visibility:public"],

pkg/cloudprovider/providers/gce/cloud/gen/main.go

@@ -352,27 +352,27 @@ type {{.MockWrapType}} struct {
 	// execution flow of the mock. Return (false, nil, nil) to continue with
 	// normal mock behavior/ after the hook function executes.
 	{{- if .GenerateGet}}
-	GetHook    func(m *{{.MockWrapType}}, ctx context.Context, key *meta.Key) (bool, *{{.FQObjectType}}, error)
+	GetHook    func(ctx context.Context, key *meta.Key, m *{{.MockWrapType}}) (bool, *{{.FQObjectType}}, error)
 	{{- end -}}
 	{{- if .GenerateList}}
 	{{- if .KeyIsGlobal}}
-	ListHook   func(m *{{.MockWrapType}}, ctx context.Context, fl *filter.F) (bool, []*{{.FQObjectType}}, error)
+	ListHook   func(ctx context.Context, fl *filter.F, m *{{.MockWrapType}}) (bool, []*{{.FQObjectType}}, error)
 	{{- end -}}
 	{{- if .KeyIsRegional}}
-	ListHook   func(m *{{.MockWrapType}}, ctx context.Context, region string, fl *filter.F) (bool, []*{{.FQObjectType}}, error)
+	ListHook   func(ctx context.Context, region string, fl *filter.F, m *{{.MockWrapType}}) (bool, []*{{.FQObjectType}}, error)
 	{{- end -}}
 	{{- if .KeyIsZonal}}
-	ListHook   func(m *{{.MockWrapType}}, ctx context.Context, zone string, fl *filter.F) (bool, []*{{.FQObjectType}}, error)
+	ListHook   func(ctx context.Context, zone string, fl *filter.F, m *{{.MockWrapType}}) (bool, []*{{.FQObjectType}}, error)
 	{{- end}}
 	{{- end -}}
 	{{- if .GenerateInsert}}
-	InsertHook func(m *{{.MockWrapType}}, ctx context.Context, key *meta.Key, obj *{{.FQObjectType}}) (bool, error)
+	InsertHook func(ctx context.Context, key *meta.Key, obj *{{.FQObjectType}}, m *{{.MockWrapType}}) (bool, error)
 	{{- end -}}
 	{{- if .GenerateDelete}}
-	DeleteHook func(m *{{.MockWrapType}}, ctx context.Context, key *meta.Key) (bool, error)
+	DeleteHook func(ctx context.Context, key *meta.Key, m *{{.MockWrapType}}) (bool, error)
 	{{- end -}}
 	{{- if .AggregatedList}}
-	AggregatedListHook func(m *{{.MockWrapType}}, ctx context.Context, fl *filter.F) (bool, map[string][]*{{.FQObjectType}}, error)
+	AggregatedListHook func(ctx context.Context, fl *filter.F, m *{{.MockWrapType}}) (bool, map[string][]*{{.FQObjectType}}, error)
 	{{- end}}
 
 {{- with .Methods -}}
@@ -390,7 +390,7 @@ type {{.MockWrapType}} struct {
 // Get returns the object from the mock.
 func (m *{{.MockWrapType}}) Get(ctx context.Context, key *meta.Key) (*{{.FQObjectType}}, error) {
 	if m.GetHook != nil {
-		if intercept, obj, err := m.GetHook(m, ctx, key);  intercept {
+		if intercept, obj, err := m.GetHook(ctx, key, m);  intercept {
 			glog.V(5).Infof("{{.MockWrapType}}.Get(%v, %s) = %+v, %v", ctx, key, obj ,err)
 			return obj, err
 		}
@@ -436,15 +436,15 @@ func (m *{{.MockWrapType}}) List(ctx context.Context, zone string, fl *filter.F)
 {{- end}}
 	if m.ListHook != nil {
 		{{if .KeyIsGlobal -}}
-		if intercept, objs, err := m.ListHook(m, ctx, fl);  intercept {
+		if intercept, objs, err := m.ListHook(ctx, fl, m);  intercept {
 			glog.V(5).Infof("{{.MockWrapType}}.List(%v, %v) = [%v items], %v", ctx, fl, len(objs), err)
 		{{- end -}}
 		{{- if .KeyIsRegional -}}
-		if intercept, objs, err := m.ListHook(m, ctx, region, fl);  intercept {
+		if intercept, objs, err := m.ListHook(ctx, region, fl, m);  intercept {
 			glog.V(5).Infof("{{.MockWrapType}}.List(%v, %q, %v) = [%v items], %v", ctx, region, fl, len(objs), err)
 		{{- end -}}
 		{{- if .KeyIsZonal -}}
-		if intercept, objs, err := m.ListHook(m, ctx, zone, fl);  intercept {
+		if intercept, objs, err := m.ListHook(ctx, zone, fl, m);  intercept {
 			glog.V(5).Infof("{{.MockWrapType}}.List(%v, %q, %v) = [%v items], %v", ctx, zone, fl, len(objs), err)
 		{{- end}}
 			return objs, err
@@ -508,7 +508,7 @@ func (m *{{.MockWrapType}}) List(ctx context.Context, zone string, fl *filter.F)
 // Insert is a mock for inserting/creating a new object.
 func (m *{{.MockWrapType}}) Insert(ctx context.Context, key *meta.Key, obj *{{.FQObjectType}}) error {
 	if m.InsertHook != nil {
-		if intercept, err := m.InsertHook(m, ctx, key, obj);  intercept {
+		if intercept, err := m.InsertHook(ctx, key, obj, m);  intercept {
 			glog.V(5).Infof("{{.MockWrapType}}.Insert(%v, %v, %+v) = %v", ctx, key, obj, err)
 			return err
 		}
@@ -548,7 +548,7 @@ func (m *{{.MockWrapType}}) Insert(ctx context.Context, key *meta.Key, obj *{{.F
 // Delete is a mock for deleting the object.
 func (m *{{.MockWrapType}}) Delete(ctx context.Context, key *meta.Key) error {
 	if m.DeleteHook != nil {
-		if intercept, err := m.DeleteHook(m, ctx, key);  intercept {
+		if intercept, err := m.DeleteHook(ctx, key, m);  intercept {
 			glog.V(5).Infof("{{.MockWrapType}}.Delete(%v, %v) = %v", ctx, key, err)
 			return err
 		}
@@ -583,7 +583,7 @@ func (m *{{.MockWrapType}}) Delete(ctx context.Context, key *meta.Key) error {
 // AggregatedList is a mock for AggregatedList.
 func (m *{{.MockWrapType}}) AggregatedList(ctx context.Context, fl *filter.F) (map[string][]*{{.FQObjectType}}, error) {
 	if m.AggregatedListHook != nil {
-		if intercept, objs, err := m.AggregatedListHook(m, ctx, fl); intercept {
+		if intercept, objs, err := m.AggregatedListHook(ctx, fl, m); intercept {
 			glog.V(5).Infof("{{.MockWrapType}}.AggregatedList(%v, %v) = [%v items], %v", ctx, fl, len(objs), err)
 			return objs, err
 		}
@@ -632,17 +632,17 @@ func (m *{{.MockWrapType}}) Obj(o *{{.FQObjectType}}) *Mock{{.Service}}Obj {
 func (m *{{.MockWrapType}}) {{.FcnArgs}} {
 {{- if .IsOperation }}
 	if m.{{.MockHookName}} != nil {
-		return m.{{.MockHookName}}(m, ctx, key {{.CallArgs}})
+		return m.{{.MockHookName}}(ctx, key {{.CallArgs}}, m)
 	}
 	return nil
 {{- else if .IsGet}}
 	if m.{{.MockHookName}} != nil {
-		return m.{{.MockHookName}}(m, ctx, key {{.CallArgs}})
+		return m.{{.MockHookName}}(ctx, key {{.CallArgs}}, m)
 	}
 	return nil, fmt.Errorf("{{.MockHookName}} must be set")
 {{- else if .IsPaged}}
 	if m.{{.MockHookName}} != nil {
-		return m.{{.MockHookName}}(m, ctx, key {{.CallArgs}}, fl)
+		return m.{{.MockHookName}}(ctx, key {{.CallArgs}}, fl, m)
 	}
 	return nil, nil
 {{- end}}

pkg/cloudprovider/providers/gce/cloud/meta/method.go

@@ -270,7 +270,6 @@ func (m *Method) MockHookName() string {
 // MockHook is the definition of the hook function.
 func (m *Method) MockHook() string {
 	args := m.args(m.argsSkip(), false, []string{
-		fmt.Sprintf("*%s", m.MockWrapType()),
 		"context.Context",
 		"*meta.Key",
 	})
@@ -278,6 +277,8 @@ func (m *Method) MockHook() string {
 		args = append(args, "*filter.F")
 	}
 
+	args = append(args, fmt.Sprintf("*%s", m.MockWrapType()))
+
 	switch m.kind {
 	case MethodOperation:
 		return fmt.Sprintf("%v func(%v) error", m.MockHookName(), strings.Join(args, ", "))

pkg/cloudprovider/providers/gce/cloud/mock/BUILD

@@ -0,0 +1,28 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library")
+
+go_library(
+    name = "go_default_library",
+    srcs = ["mock.go"],
+    importpath = "k8s.io/kubernetes/pkg/cloudprovider/providers/gce/cloud/mock",
+    visibility = ["//visibility:public"],
+    deps = [
+        "//pkg/cloudprovider/providers/gce/cloud:go_default_library",
+        "//pkg/cloudprovider/providers/gce/cloud/meta:go_default_library",
+        "//vendor/google.golang.org/api/compute/v1:go_default_library",
+        "//vendor/google.golang.org/api/googleapi:go_default_library",
+    ],
+)
+
+filegroup(
+    name = "package-srcs",
+    srcs = glob(["**"]),
+    tags = ["automanaged"],
+    visibility = ["//visibility:private"],
+)
+
+filegroup(
+    name = "all-srcs",
+    srcs = [":package-srcs"],
+    tags = ["automanaged"],
+    visibility = ["//visibility:public"],
+)

pkg/cloudprovider/providers/gce/cloud/mock/mock.go

@@ -0,0 +1,76 @@
+/*
+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 mock encapsulates mocks for testing GCE provider functionality.
+// These methods are used to override the mock objects' methods in order to
+// intercept the standard processing and to add custom logic for test purposes.
+//
+//  // Example usage:
+// cloud := cloud.NewMockGCE()
+// cloud.MockTargetPools.AddInstanceHook = mock.AddInstanceHook
+package mock
+
+import (
+	"context"
+	"fmt"
+	"net/http"
+
+	ga "google.golang.org/api/compute/v1"
+	"google.golang.org/api/googleapi"
+	cloud "k8s.io/kubernetes/pkg/cloudprovider/providers/gce/cloud"
+	"k8s.io/kubernetes/pkg/cloudprovider/providers/gce/cloud/meta"
+)
+
+// AddInstanceHook mocks adding a Instance to MockTargetPools
+func AddInstanceHook(ctx context.Context, key *meta.Key, req *ga.TargetPoolsAddInstanceRequest, m *cloud.MockTargetPools) error {
+	pool, err := m.Get(ctx, key)
+	if err != nil {
+		return &googleapi.Error{
+			Code:    http.StatusNotFound,
+			Message: fmt.Sprintf("Key: %s was not found in TargetPools", key.String()),
+		}
+	}
+
+	for _, instance := range req.Instances {
+		pool.Instances = append(pool.Instances, instance.Instance)
+	}
+
+	return nil
+}
+
+// RemoveInstanceHook mocks removing a Instance from MockTargetPools
+func RemoveInstanceHook(ctx context.Context, key *meta.Key, req *ga.TargetPoolsRemoveInstanceRequest, m *cloud.MockTargetPools) error {
+	pool, err := m.Get(ctx, key)
+	if err != nil {
+		return &googleapi.Error{
+			Code:    http.StatusNotFound,
+			Message: fmt.Sprintf("Key: %s was not found in TargetPools", key.String()),
+		}
+	}
+
+	for _, instanceToRemove := range req.Instances {
+		for i, instance := range pool.Instances {
+			if instanceToRemove.Instance == instance {
+				// Delete instance from pool.Instances without preserving order
+				pool.Instances[i] = pool.Instances[len(pool.Instances)-1]
+				pool.Instances = pool.Instances[:len(pool.Instances)-1]
+				break
+			}
+		}
+	}
+
+	return nil
+}

pkg/cloudprovider/providers/gce/gce_loadbalancer_external_test.go

@@ -23,8 +23,14 @@ import (
 	"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/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"
 )
 
 func TestEnsureStaticIP(t *testing.T) {
@@ -237,3 +243,259 @@ func TestDeleteAddressWithWrongTier(t *testing.T) {
 		})
 	}
 }
+
+const (
+	gceProjectId = "test-project"
+	gceRegion    = "us-central1"
+	zoneName     = "us-central1-b"
+	nodeName     = "test-node-1"
+	clusterName  = "Test Cluster Name"
+	clusterID    = "test-cluster-id"
+)
+
+var apiService = &v1.Service{
+	Spec: v1.ServiceSpec{
+		SessionAffinity: v1.ServiceAffinityClientIP,
+		Type:            v1.ServiceTypeClusterIP,
+		Ports:           []v1.ServicePort{{Protocol: v1.ProtocolTCP, Port: int32(123)}},
+	},
+}
+
+func fakeGCECloud() (*GCECloud, error) {
+	client, err := newOauthClient(nil)
+	if err != nil {
+		return nil, err
+	}
+
+	service, err := compute.New(client)
+	if err != nil {
+		return nil, err
+	}
+
+	// Used in disk unit tests
+	fakeManager := newFakeManager(gceProjectId, gceRegion)
+	zonesWithNodes := createNodeZones([]string{zoneName})
+
+	alphaFeatureGate, err := NewAlphaFeatureGate([]string{})
+	if err != nil {
+		return nil, err
+	}
+
+	cloud := cloud.NewMockGCE()
+	cloud.MockTargetPools.AddInstanceHook = mock.AddInstanceHook
+	cloud.MockTargetPools.RemoveInstanceHook = mock.RemoveInstanceHook
+
+	gce := GCECloud{
+		region:             gceRegion,
+		service:            service,
+		manager:            fakeManager,
+		managedZones:       []string{zoneName},
+		projectID:          gceProjectId,
+		AlphaFeatureGate:   alphaFeatureGate,
+		nodeZones:          zonesWithNodes,
+		nodeInformerSynced: func() bool { return true },
+		c:                  cloud,
+	}
+
+	return &gce, nil
+}
+
+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(
+				gceProjectId,
+				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})
+	if err != nil {
+		return nil, err
+	}
+
+	return gce.ensureExternalLoadBalancer(
+		clusterName,
+		clusterID,
+		apiService,
+		nil,
+		nodes,
+	)
+}
+
+func TestEnsureExternalLoadBalancer(t *testing.T) {
+	gce, err := fakeGCECloud()
+	require.NoError(t, err)
+
+	status, err := createExternalLoadBalancer(gce)
+	assert.NoError(t, err)
+	assert.NotEmpty(t, status.Ingress)
+
+	lbName := cloudprovider.GetLoadBalancerName(apiService)
+	hcName := MakeNodesHealthCheckName(clusterID)
+
+	// Check that Firewalls are created for the LoadBalancer and the HealthCheck
+	fwNames := []string{
+		MakeFirewallName(lbName),
+		MakeHealthCheckFirewallName(clusterID, hcName, 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 TargetPool is Created
+	pool, err := gce.GetTargetPool(lbName, gceRegion)
+	require.NoError(t, err)
+	assert.Equal(t, lbName, pool.Name)
+	assert.NotEmpty(t, pool.HealthChecks)
+	assert.Equal(t, 1, len(pool.Instances))
+
+	// Check that HealthCheck is created
+	healthcheck, err := gce.GetHttpHealthCheck(hcName)
+	require.NoError(t, err)
+	assert.Equal(t, hcName, healthcheck.Name)
+
+	// Check that ForwardingRule is created
+	fwdRule, err := gce.GetRegionForwardingRule(lbName, gceRegion)
+	require.NoError(t, err)
+	assert.Equal(t, lbName, fwdRule.Name)
+	assert.Equal(t, "TCP", fwdRule.IPProtocol)
+	assert.Equal(t, "123-123", fwdRule.PortRange)
+}
+
+func TestUpdateExternalLoadBalancer(t *testing.T) {
+	gce, err := fakeGCECloud()
+	require.NoError(t, err)
+
+	_, err = createExternalLoadBalancer(gce)
+	assert.NoError(t, err)
+
+	newNodeName := "test-node-2"
+	newNodes, err := createAndInsertNodes(gce, []string{nodeName, newNodeName})
+	assert.NoError(t, err)
+
+	// Add the new node, then check that it is properly added to the TargetPool
+	err = gce.updateExternalLoadBalancer(clusterName, apiService, newNodes)
+	assert.NoError(t, err)
+
+	lbName := cloudprovider.GetLoadBalancerName(apiService)
+
+	pool, err := gce.GetTargetPool(lbName, gceRegion)
+	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),
+	)
+
+	assert.Contains(
+		t,
+		pool.Instances,
+		fmt.Sprintf("/zones/%s/instances/%s", zoneName, newNodeName),
+	)
+
+	newNodes, err = createAndInsertNodes(gce, []string{nodeName})
+	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)
+	assert.NoError(t, err)
+
+	pool, err = gce.GetTargetPool(lbName, gceRegion)
+	require.NoError(t, err)
+
+	assert.Equal(
+		t,
+		[]string{fmt.Sprintf("/zones/%s/instances/%s", zoneName, nodeName)},
+		pool.Instances,
+	)
+}
+
+func TestEnsureExternalLoadBalancerDeleted(t *testing.T) {
+	gce, err := fakeGCECloud()
+	require.NoError(t, err)
+
+	_, err = createExternalLoadBalancer(gce)
+	assert.NoError(t, err)
+
+	err = gce.ensureExternalLoadBalancerDeleted(clusterName, clusterID, apiService)
+	assert.NoError(t, err)
+
+	lbName := cloudprovider.GetLoadBalancerName(apiService)
+	hcName := MakeNodesHealthCheckName(clusterID)
+
+	// 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 TargetPool is deleted
+	pool, err := gce.GetTargetPool(lbName, gceRegion)
+	require.Error(t, err)
+	assert.Nil(t, pool)
+
+	// Check that HealthCheck is deleted
+	healthcheck, err := gce.GetHttpHealthCheck(hcName)
+	require.Error(t, err)
+	assert.Nil(t, healthcheck)
+
+	// Check forwarding rule is deleted
+	fwdRule, err := gce.GetRegionForwardingRule(lbName, gceRegion)
+	require.Error(t, err)
+	assert.Nil(t, fwdRule)
+}