Cluster management lib: allow custom cluster name (#723)

* Cluster management lib: allow custom cluster name

* Apply suggestions from code review

Co-Authored-By: Chi Zhang <chizhg@google.com>

* Apply suggestions from code review

Co-Authored-By: Chi Zhang <chizhg@google.com>

* revert unit64 change

* Apply suggestions from code review

Co-Authored-By: Adriano Cunha <35786489+adrcunha@users.noreply.github.com>

testutils/clustermanager/example_test.go

@@ -26,14 +26,21 @@ func Example() {
 	var (
 		minNodes int64 = 1
 		maxNodes int64 = 3
-		nodeType       = "n1-standard-8"
+		nodeType        = "n1-standard-8"
-		region         = "us-east1"
+		region          = "us-east1"
-		zone           = "a"
+		zone            = "a"
-		project        = "myGKEproject"
+		project         = "myGKEproject"
-		addons         = []string{"istio"}
+		addons          = []string{"istio"}
 	)
 	gkeClient := GKEClient{}
-	clusterOps := gkeClient.Setup(&minNodes, &maxNodes, &nodeType, &region, &zone, &project, addons)
+	clusterOps := gkeClient.Setup(GKERequest{
+		MinNodes: minNodes,
+		MaxNodes: maxNodes,
+		NodeType: nodeType,
+		Region:   region,
+		Zone:     zone,
+		Project:  project,
+		Addons:   addons})
 	// Cast to GKEOperation
 	gkeOps := clusterOps.(*GKECluster)
 	if err := gkeOps.Initialize(); err != nil {

testutils/clustermanager/gke.go

@@ -57,13 +57,35 @@ type GKEClient struct {
 
 // GKERequest contains all requests collected for cluster creation
 type GKERequest struct {
-	MinNodes      int64
+	// Project: GKE project, no default. Fall back to get project from kubeconfig
-	MaxNodes      int64
+	// then gcloud config
-	NodeType      string
+	Project string
-	Region        string
+
-	Zone          string
+	// ClusterName: custom cluster name to use. Fall back to cluster set by
+	// kubeconfig, else composed as k[REPO]-cls-e2e-[BUILD_ID]
+	ClusterName string
+
+	// MinNodes: default to 1 if not provided
+	MinNodes int64
+
+	// MaxNodes: default to max(3, MinNodes) if not provided
+	MaxNodes int64
+
+	// NodeType: default to n1-standard-4 if not provided
+	NodeType string
+
+	// Region: default to regional cluster if not provided, and use default backup regions
+	Region string
+
+	// Zone: default is none, must be provided together with region
+	Zone string
+
+	// BackupRegions: fall back regions to try out in case of cluster creation
+	// failure due to regional issue(s)
 	BackupRegions []string
-	Addons        []string
+
+	// Addons: cluster addons to be added to cluster, such as istio
+	Addons []string
 }
 
 // GKECluster implements ClusterOperations
@@ -123,55 +145,50 @@ func (gsc *GKESDKClient) setAutoscaling(project, clusterName, location, nodepool
 	return gsc.Service.Projects.Locations.Clusters.NodePools.SetAutoscaling(parent, rb).Do()
 }
 
-// Setup sets up a GKECluster client.
+// Setup sets up a GKECluster client, takes GEKRequest as parameter and applies
-// minNodes: default to 1 if not provided
+// all defaults if not defined.
-// maxNodes: default to 3 if not provided
+func (gs *GKEClient) Setup(r GKERequest) ClusterOperations {
-// nodeType: default to n1-standard-4 if not provided
+	gc := &GKECluster{}
-// region: default to regional cluster if not provided, and use default backup regions
-// zone: default is none, must be provided together with region
-// project: no default
-// addons: cluster addons to be added to cluster
-func (gs *GKEClient) Setup(minNodes *int64, maxNodes *int64, nodeType *string, region *string, zone *string, project *string, addons []string) ClusterOperations {
-	gc := &GKECluster{
-		Request: &GKERequest{
-			MinNodes:      DefaultGKEMinNodes,
-			MaxNodes:      DefaultGKEMaxNodes,
-			NodeType:      DefaultGKENodeType,
-			Region:        DefaultGKERegion,
-			Zone:          DefaultGKEZone,
-			BackupRegions: DefaultGKEBackupRegions,
-			Addons:        addons,
-		},
-	}
 
-	if project != nil { // use provided project and create cluster
+	if r.Project != "" { // use provided project and create cluster
-		gc.Project = project
+		gc.Project = &r.Project
 		gc.NeedCleanup = true
 	}
 
-	if minNodes != nil {
+	if r.MinNodes == 0 {
-		gc.Request.MinNodes = *minNodes
+		r.MinNodes = DefaultGKEMinNodes
 	}
-	if maxNodes != nil {
+	if r.MaxNodes == 0 {
-		gc.Request.MaxNodes = *maxNodes
+		r.MaxNodes = DefaultGKEMaxNodes
+		// We don't want MaxNodes < MinNodes
+		if r.MinNodes > r.MaxNodes {
+			r.MaxNodes = r.MinNodes
+		}
 	}
-	if nodeType != nil {
+	if r.NodeType == "" {
-		gc.Request.NodeType = *nodeType
+		r.NodeType = DefaultGKENodeType
 	}
-	if region != nil {
+	// Only use default backup regions if region is not provided
-		gc.Request.Region = *region
+	if len(r.BackupRegions) == 0 && r.Region == "" {
+		r.BackupRegions = DefaultGKEBackupRegions
+		if common.GetOSEnv(backupRegionEnv) != "" {
+			r.BackupRegions = strings.Split(common.GetOSEnv(backupRegionEnv), " ")
+		}
 	}
-	if common.GetOSEnv(regionEnv) != "" {
+	if r.Region == "" {
-		gc.Request.Region = common.GetOSEnv(regionEnv)
+		r.Region = DefaultGKERegion
+		if common.GetOSEnv(regionEnv) != "" {
+			r.Region = common.GetOSEnv(regionEnv)
+		}
 	}
-	if common.GetOSEnv(backupRegionEnv) != "" {
+	if r.Zone == "" {
-		gc.Request.BackupRegions = strings.Split(common.GetOSEnv(backupRegionEnv), " ")
+		r.Zone = DefaultGKEZone
-	}
+	} else { // No backupregions if zone is provided
-	if zone != nil {
+		r.BackupRegions = make([]string, 0)
-		gc.Request.Zone = *zone
-		gc.Request.BackupRegions = make([]string, 0)
 	}
 
+	gc.Request = &r
+
 	ctx := context.Background()
 	c, err := google.DefaultClient(ctx, container.CloudPlatformScope)
 	if err != nil {
@@ -189,8 +206,8 @@ func (gs *GKEClient) Setup(minNodes *int64, maxNodes *int64, nodeType *string, r
 	return gc
 }
 
-// Initialize sets up GKE SDK client, checks environment for cluster and
+// Initialize checks environment for cluster and projects to decide whether using
-// projects to decide whether use existing cluster/project or creating new ones.
+// existing cluster/project or creating new ones.
 func (gc *GKECluster) Initialize() error {
 	// Try obtain project name via `kubectl`, `gcloud`
 	if gc.Project == nil {
@@ -229,15 +246,20 @@ func (gc *GKECluster) Provider() string {
 // Region or Zone is provided then there is no retries
 func (gc *GKECluster) Acquire() error {
 	gc.ensureProtected()
+	var clusterName string
 	var err error
 	// Check if using existing cluster
 	if gc.Cluster != nil {
 		return nil
 	}
 	// Perform GKE specific cluster creation logics
-	clusterName, err := getResourceName(ClusterResource)
+	if gc.Request.ClusterName == "" {
-	if err != nil {
+		clusterName, err = getResourceName(ClusterResource)
-		return fmt.Errorf("failed getting cluster name: '%v'", err)
+		if err != nil {
+			return fmt.Errorf("failed getting cluster name: '%v'", err)
+		}
+	} else {
+		clusterName = gc.Request.ClusterName
 	}
 
 	regions := []string{gc.Request.Region}

testutils/clustermanager/gke_test.go

@@ -178,16 +178,14 @@ func TestSetup(t *testing.T) {
 	zoneOverride := "foozone"
 	fakeAddons := "fake-addon"
 	datas := []struct {
-		minNodes                        *int64
+		r                          GKERequest
-		maxNodes                        *int64
+		regionEnv, backupRegionEnv string
-		nodeType, region, zone, project *string
+		expClusterOperations       *GKECluster
-		addons                          []string
-		regionEnv, backupRegionEnv      string
-		expClusterOperations            *GKECluster
 	}{
 		{
 			// Defaults
-			nil, nil, nil, nil, nil, nil, []string{}, "", "",
+			GKERequest{},
+			"", "",
 			&GKECluster{
 				Request: &GKERequest{
 					MinNodes:      1,
@@ -196,28 +194,57 @@ func TestSetup(t *testing.T) {
 					Region:        "us-central1",
 					Zone:          "",
 					BackupRegions: []string{"us-west1", "us-east1"},
-					Addons:        []string{},
+					Addons:        nil,
 				},
 			},
 		}, {
 			// Project provided
-			nil, nil, nil, nil, nil, &fakeProj, []string{}, "", "",
+			GKERequest{
+				Project: fakeProj,
+			},
+			"", "",
 			&GKECluster{
 				Request: &GKERequest{
+					Project:       "b",
 					MinNodes:      1,
 					MaxNodes:      3,
 					NodeType:      "n1-standard-4",
 					Region:        "us-central1",
 					Zone:          "",
 					BackupRegions: []string{"us-west1", "us-east1"},
-					Addons:        []string{},
+					Addons:        nil,
 				},
 				Project:     &fakeProj,
 				NeedCleanup: true,
 			},
+		}, {
+			// Cluster name provided
+			GKERequest{
+				ClusterName: "predefined-cluster-name",
+			},
+			"", "",
+			&GKECluster{
+				Request: &GKERequest{
+					ClusterName:   "predefined-cluster-name",
+					MinNodes:      1,
+					MaxNodes:      3,
+					NodeType:      "n1-standard-4",
+					Region:        "us-central1",
+					Zone:          "",
+					BackupRegions: []string{"us-west1", "us-east1"},
+					Addons:        nil,
+				},
+			},
 		}, {
 			// Override other parts
-			&minNodesOverride, &maxNodesOverride, &nodeTypeOverride, &regionOverride, &zoneOverride, nil, []string{}, "", "",
+			GKERequest{
+				MinNodes: minNodesOverride,
+				MaxNodes: maxNodesOverride,
+				NodeType: nodeTypeOverride,
+				Region:   regionOverride,
+				Zone:     zoneOverride,
+			},
+			"", "",
 			&GKECluster{
 				Request: &GKERequest{
 					MinNodes:      2,
@@ -226,12 +253,18 @@ func TestSetup(t *testing.T) {
 					Region:        "fooregion",
 					Zone:          "foozone",
 					BackupRegions: []string{},
-					Addons:        []string{},
+					Addons:        nil,
 				},
 			},
 		}, {
 			// Override other parts but not zone
-			&minNodesOverride, &maxNodesOverride, &nodeTypeOverride, &regionOverride, nil, nil, []string{}, "", "",
+			GKERequest{
+				MinNodes: minNodesOverride,
+				MaxNodes: maxNodesOverride,
+				NodeType: nodeTypeOverride,
+				Region:   regionOverride,
+			},
+			"", "",
 			&GKECluster{
 				Request: &GKERequest{
 					MinNodes:      2,
@@ -239,13 +272,14 @@ func TestSetup(t *testing.T) {
 					NodeType:      "foonode",
 					Region:        "fooregion",
 					Zone:          "",
-					BackupRegions: []string{"us-west1", "us-east1"},
+					BackupRegions: nil,
-					Addons:        []string{},
+					Addons:        nil,
 				},
 			},
 		}, {
 			// Set env Region
-			nil, nil, nil, nil, nil, nil, []string{}, "customregion", "",
+			GKERequest{},
+			"customregion", "",
 			&GKECluster{
 				Request: &GKERequest{
 					MinNodes:      1,
@@ -254,12 +288,13 @@ func TestSetup(t *testing.T) {
 					Region:        "customregion",
 					Zone:          "",
 					BackupRegions: []string{"us-west1", "us-east1"},
-					Addons:        []string{},
+					Addons:        nil,
 				},
 			},
 		}, {
 			// Set env backupzone
-			nil, nil, nil, nil, nil, nil, []string{}, "", "backupregion1 backupregion2",
+			GKERequest{},
+			"", "backupregion1 backupregion2",
 			&GKECluster{
 				Request: &GKERequest{
 					MinNodes:      1,
@@ -268,12 +303,15 @@ func TestSetup(t *testing.T) {
 					Region:        "us-central1",
 					Zone:          "",
 					BackupRegions: []string{"backupregion1", "backupregion2"},
-					Addons:        []string{},
+					Addons:        nil,
 				},
 			},
 		}, {
 			// Set addons
-			nil, nil, nil, nil, nil, nil, []string{fakeAddons}, "", "",
+			GKERequest{
+				Addons: []string{fakeAddons},
+			},
+			"", "",
 			&GKECluster{
 				Request: &GKERequest{
 					MinNodes:      1,
@@ -331,15 +369,15 @@ func TestSetup(t *testing.T) {
 			return oldEnvFunc(s)
 		}
 		c := GKEClient{}
-		co := c.Setup(data.minNodes, data.maxNodes, data.nodeType, data.region, data.zone, data.project, data.addons)
+		co := c.Setup(data.r)
-		errMsg := fmt.Sprintf("testing setup with:\n\tminNodes: %v\n\tnodeType: %v\n\tregion: %v\n\tzone: %v\n\tproject: %v\n\taddons: %v\n\tregionEnv: %v\n\tbackupRegionEnv: %v",
+		errMsg := fmt.Sprintf("testing setup with:\n\t%+v\n\tregionEnv: %v\n\tbackupRegionEnv: %v",
-			data.minNodes, data.nodeType, data.region, data.zone, data.project, data.addons, data.regionEnv, data.backupRegionEnv)
+			data.r, data.regionEnv, data.backupRegionEnv)
 		gotCo := co.(*GKECluster)
 		// mock for easier comparison
 		gotCo.operations = nil
 		gotCo.boskosOps = nil
-		if !reflect.DeepEqual(co, data.expClusterOperations) {
+		if dif := cmp.Diff(gotCo.Request, data.expClusterOperations.Request); dif != "" {
-			t.Fatalf("%s\nwant GKECluster:\n'%v'\ngot GKECluster:\n'%v'", errMsg, data.expClusterOperations, co)
+			t.Errorf("%s\nRequest got(+) is different from wanted(-)\n%v", errMsg, dif)
 		}
 	}
 }
@@ -575,23 +613,41 @@ func TestGKECheckEnvironment(t *testing.T) {
 }
 
 func TestAcquire(t *testing.T) {
+	predefinedClusterName := "predefined-cluster-name"
 	fakeClusterName := "kpkg-e2e-cls-1234"
 	fakeBuildID := "1234"
 	datas := []struct {
-		existCluster  *container.Cluster
+		existCluster          *container.Cluster
-		kubeconfigSet bool
+		predefinedClusterName string
-		addons        []string
+		kubeconfigSet         bool
-		nextOpStatus  []string
+		addons                []string
-		expCluster    *container.Cluster
+		nextOpStatus          []string
-		expErr        error
+		expCluster            *container.Cluster
-		expPanic      bool
+		expErr                error
+		expPanic              bool
 	}{
 		{
 			// cluster already found
 			&container.Cluster{
 				Name:     "customcluster",
 				Location: "us-central1",
-			}, true, []string{}, []string{}, &container.Cluster{
+			}, "", true, []string{}, []string{}, &container.Cluster{
+				Name:         "customcluster",
+				Location:     "us-central1",
+				Status:       "RUNNING",
+				AddonsConfig: &container.AddonsConfig{},
+				NodePools: []*container.NodePool{
+					{
+						Name: "default-pool",
+					},
+				},
+			}, nil, false,
+		}, {
+			// cluster already found and clustername predefined
+			&container.Cluster{
+				Name:     "customcluster",
+				Location: "us-central1",
+			}, predefinedClusterName, true, []string{}, []string{}, &container.Cluster{
 				Name:         "customcluster",
 				Location:     "us-central1",
 				Status:       "RUNNING",
@@ -608,7 +664,7 @@ func TestAcquire(t *testing.T) {
 			&container.Cluster{
 				Name:     fakeClusterName,
 				Location: "us-central1",
-			}, false, []string{}, []string{}, &container.Cluster{
+			}, "", false, []string{}, []string{}, &container.Cluster{
 				Name:         fakeClusterName,
 				Location:     "us-central1",
 				Status:       "RUNNING",
@@ -629,7 +685,7 @@ func TestAcquire(t *testing.T) {
 			&container.Cluster{
 				Name:     fakeClusterName,
 				Location: "us-central1",
-			}, false, []string{}, []string{"BAD"}, &container.Cluster{
+			}, "", false, []string{}, []string{"BAD"}, &container.Cluster{
 				Name:         fakeClusterName,
 				Location:     "us-west1",
 				Status:       "RUNNING",
@@ -644,9 +700,46 @@ func TestAcquire(t *testing.T) {
 					Username: "admin",
 				},
 			}, nil, false,
+		}, {
+			// cluster exists but not set in kubeconfig, clusterName defined
+			&container.Cluster{
+				Name:     fakeClusterName,
+				Location: "us-central1",
+			}, predefinedClusterName, false, []string{}, []string{}, &container.Cluster{
+				Name:         predefinedClusterName,
+				Location:     "us-central1",
+				Status:       "RUNNING",
+				AddonsConfig: &container.AddonsConfig{},
+				NodePools: []*container.NodePool{
+					{
+						Name:        "default-pool",
+						Autoscaling: &container.NodePoolAutoscaling{Enabled: true, MaxNodeCount: 3, MinNodeCount: 1},
+					},
+				},
+				MasterAuth: &container.MasterAuth{
+					Username: "admin",
+				},
+			}, nil, false,
+		}, {
+			// cluster not exist, but clustername defined
+			nil, predefinedClusterName, false, []string{}, []string{}, &container.Cluster{
+				Name:         predefinedClusterName,
+				Location:     "us-central1",
+				Status:       "RUNNING",
+				AddonsConfig: &container.AddonsConfig{},
+				NodePools: []*container.NodePool{
+					{
+						Name:        "default-pool",
+						Autoscaling: &container.NodePoolAutoscaling{Enabled: true, MaxNodeCount: 3, MinNodeCount: 1},
+					},
+				},
+				MasterAuth: &container.MasterAuth{
+					Username: "admin",
+				},
+			}, nil, false,
 		}, {
 			// cluster creation succeeded
-			nil, false, []string{}, []string{}, &container.Cluster{
+			nil, "", false, []string{}, []string{}, &container.Cluster{
 				Name:         fakeClusterName,
 				Location:     "us-central1",
 				Status:       "RUNNING",
@@ -663,7 +756,7 @@ func TestAcquire(t *testing.T) {
 			}, nil, false,
 		}, {
 			// cluster creation succeeded with addon
-			nil, false, []string{"istio"}, []string{}, &container.Cluster{
+			nil, "", false, []string{"istio"}, []string{}, &container.Cluster{
 				Name:     fakeClusterName,
 				Location: "us-central1",
 				Status:   "RUNNING",
@@ -682,7 +775,7 @@ func TestAcquire(t *testing.T) {
 			}, nil, false,
 		}, {
 			// cluster creation succeeded retry
-			nil, false, []string{}, []string{"PENDING"}, &container.Cluster{
+			nil, "", false, []string{}, []string{"PENDING"}, &container.Cluster{
 				Name:         fakeClusterName,
 				Location:     "us-west1",
 				Status:       "RUNNING",
@@ -699,7 +792,7 @@ func TestAcquire(t *testing.T) {
 			}, nil, false,
 		}, {
 			// cluster creation failed set addon, but succeeded retry
-			nil, false, []string{}, []string{"DONE", "PENDING"}, &container.Cluster{
+			nil, "", false, []string{}, []string{"DONE", "PENDING"}, &container.Cluster{
 				Name:         fakeClusterName,
 				Location:     "us-west1",
 				Status:       "RUNNING",
@@ -716,13 +809,13 @@ func TestAcquire(t *testing.T) {
 			}, nil, false,
 		}, {
 			// cluster creation failed all retry
-			nil, false, []string{}, []string{"PENDING", "PENDING", "PENDING"}, nil, fmt.Errorf("timed out waiting"), false,
+			nil, "", false, []string{}, []string{"PENDING", "PENDING", "PENDING"}, nil, fmt.Errorf("timed out waiting"), false,
 		}, {
 			// cluster creation went bad state
-			nil, false, []string{}, []string{"BAD", "BAD", "BAD"}, nil, fmt.Errorf("unexpected operation status: %q", "BAD"), false,
+			nil, "", false, []string{}, []string{"BAD", "BAD", "BAD"}, nil, fmt.Errorf("unexpected operation status: %q", "BAD"), false,
 		}, {
 			// bad addon, should get a panic
-			nil, false, []string{"bad_addon"}, []string{}, nil, nil, true,
+			nil, "", false, []string{"bad_addon"}, []string{}, nil, nil, true,
 		},
 	}
 
@@ -783,6 +876,7 @@ func TestAcquire(t *testing.T) {
 		}
 
 		fgc.Request = &GKERequest{
+			ClusterName:   data.predefinedClusterName,
 			MinNodes:      DefaultGKEMinNodes,
 			MaxNodes:      DefaultGKEMaxNodes,
 			NodeType:      DefaultGKENodeType,