Merge pull request kubernetes-sigs#73 from chizhg/fix-private-network…

…-args-for-multicluster

Fix --private-cluster-master-ip-range arg for multicluster

kubetest2-gke/deployer/commandutils.go

@@ -56,7 +56,7 @@ func (d *deployer) prepareGcpIfNeeded(projectID string) error {
 	}
 
 	if err := runWithOutput(exec.RawCommand("gcloud config set project " + projectID)); err != nil {
-		return fmt.Errorf("failed to set project %s : err %v", projectID, err)
+		return fmt.Errorf("failed to set project %s: %w", projectID, err)
 	}
 
 	// gcloud creds may have changed

kubetest2-gke/deployer/common.go

@@ -44,15 +44,15 @@ func (d *deployer) init() error {
 func (d *deployer) initialize() error {
 	if d.commonOptions.ShouldUp() {
 		if err := d.verifyUpFlags(); err != nil {
-			return fmt.Errorf("init failed to verify flags for up: %s", err)
+			return fmt.Errorf("init failed to verify flags for up: %w", err)
 		}
 
 		if len(d.projects) == 0 {
 			klog.V(1).Infof("No GCP projects provided, acquiring from Boskos %d project/s", d.boskosProjectsRequested)
 
 			boskosClient, err := boskos.NewClient(d.boskosLocation)
 			if err != nil {
-				return fmt.Errorf("failed to make boskos client: %s", err)
+				return fmt.Errorf("failed to make boskos client: %w", err)
 			}
 			d.boskos = boskosClient
 
@@ -65,7 +65,7 @@ func (d *deployer) initialize() error {
 				)
 
 				if err != nil {
-					return fmt.Errorf("init failed to get project from boskos: %s", err)
+					return fmt.Errorf("init failed to get project from boskos: %w", err)
 				}
 				d.projects = append(d.projects, resource.Name)
 				klog.V(1).Infof("Got project %s from boskos", resource.Name)
@@ -74,29 +74,33 @@ func (d *deployer) initialize() error {
 
 		// Multi-cluster name adjustment
 		numProjects := len(d.projects)
-		d.projectClustersLayout = make(map[string][]string, numProjects)
+		d.projectClustersLayout = make(map[string][]cluster, numProjects)
 		if numProjects > 1 {
 			if err := buildProjectClustersLayout(d.projects, d.clusters, d.projectClustersLayout); err != nil {
 				return fmt.Errorf("failed to build the project clusters layout: %v", err)
 			}
 		} else {
 			// Backwards compatible construction
-			d.projectClustersLayout[d.projects[0]] = d.clusters
+			clusters := make([]cluster, len(d.clusters))
+			for i, clusterName := range d.clusters {
+				clusters[i] = cluster{i, clusterName}
+			}
+			d.projectClustersLayout[d.projects[0]] = clusters
 		}
 	}
 
 	if d.commonOptions.ShouldDown() {
 		if err := d.verifyDownFlags(); err != nil {
-			return fmt.Errorf("init failed to verify flags for down: %s", err)
+			return fmt.Errorf("init failed to verify flags for down: %w", err)
 		}
 	}
 
 	return nil
 }
 
 // buildProjectClustersLayout builds the projects and real cluster names mapping based on the provided --cluster-name flag.
-func buildProjectClustersLayout(projects, clusters []string, projectClustersLayout map[string][]string) error {
-	for _, clusterName := range clusters {
+func buildProjectClustersLayout(projects, clusters []string, projectClustersLayout map[string][]cluster) error {
+	for i, clusterName := range clusters {
 		parts := strings.Split(clusterName, ":")
 		if len(parts) != 2 {
 			return fmt.Errorf("cluster name does not follow expected format (name:projectIndex): %s", clusterName)
@@ -108,7 +112,7 @@ func buildProjectClustersLayout(projects, clusters []string, projectClustersLayo
 		if projectIndex >= len(projects) {
 			return fmt.Errorf("project index %d specified in the cluster name should be smaller than the number of projects %d", projectIndex, len(projects))
 		}
-		projectClustersLayout[projects[projectIndex]] = append(projectClustersLayout[projects[projectIndex]], parts[0])
+		projectClustersLayout[projects[projectIndex]] = append(projectClustersLayout[projects[projectIndex]], cluster{i, parts[0]})
 	}
 	return nil
 }

kubetest2-gke/deployer/deployer.go

@@ -79,6 +79,12 @@ type ig struct {
 	uniq string
 }
 
+type cluster struct {
+	// index is the index of the cluster in the list provided via the --cluster-name flag
+	index int
+	name  string
+}
+
 type deployer struct {
 	// generic parts
 	commonOptions types.Options
@@ -98,7 +104,7 @@ type deployer struct {
 	region   string
 	clusters []string
 	// only used for multi-project multi-cluster profile to save the project-clusters mapping
-	projectClustersLayout map[string][]string
+	projectClustersLayout map[string][]cluster
 	nodes                 int
 	machineType           string
 	network               string
@@ -124,8 +130,8 @@ type deployer struct {
 
 	// Private cluster access level, must be one of "no", "limited" and "unrestricted".
 	// See the details in https://cloud.google.com/kubernetes-engine/docs/how-to/private-clusters
-	privateClusterAccessLevel   string
-	privateClusterMasterIPRange string
+	privateClusterAccessLevel    string
+	privateClusterMasterIPRanges []string
 
 	boskosLocation              string
 	boskosResourceType          string
@@ -219,7 +225,7 @@ func bindFlags(d *deployer) *pflag.FlagSet {
 	flags.BoolVar(&d.gcpSSHKeyIgnored, "ignore-gcp-ssh-key", false, "Whether the GCP SSH key should be ignored or not for bringing up the cluster.")
 	flags.BoolVar(&d.workloadIdentityEnabled, "enable-workload-identity", false, "Whether enable workload identity for the cluster or not.")
 	flags.StringVar(&d.privateClusterAccessLevel, "private-cluster-access-level", "", "Private cluster access level, if not empty, must be one of 'no', 'limited' or 'unrestricted'")
-	flags.StringVar(&d.privateClusterMasterIPRange, "private-cluster-master-ip-range", "172.16.0.32/28", "Private cluster master IP range. It should be an IPv4 CIDR, and must not be empty if private cluster is requested.")
+	flags.StringSliceVar(&d.privateClusterMasterIPRanges, "private-cluster-master-ip-range", []string{"172.16.0.32/28"}, "Private cluster master IP ranges. It should be IPv4 CIDR(s), and its length must be the same as the number of clusters if private cluster is requested.")
 	flags.StringVar(&d.boskosLocation, "boskos-location", defaultBoskosLocation, "If set, manually specifies the location of the Boskos server")
 	flags.StringVar(&d.boskosResourceType, "boskos-resource-type", defaultGKEProjectResourceType, "If set, manually specifies the resource type of GCP projects to acquire from Boskos")
 	flags.IntVar(&d.boskosAcquireTimeoutSeconds, "boskos-acquire-timeout-seconds", 300, "How long (in seconds) to hang on a request to Boskos to acquire a resource before erroring")

kubetest2-gke/deployer/down.go

@@ -46,7 +46,7 @@ func (d *deployer) Down() error {
 					defer wg.Done()
 					// We best-effort try all of these and report errors as appropriate.
 					if err := runWithOutput(exec.Command(
-						"gcloud", containerArgs("clusters", "delete", "-q", cluster,
+						"gcloud", containerArgs("clusters", "delete", "-q", cluster.name,
 							"--project="+project,
 							loc)...)); err != nil {
 						klog.Errorf("Error deleting cluster: %v", err)

kubetest2-gke/deployer/dumplogs.go

@@ -66,7 +66,7 @@ export KUBE_NODE_OS_DISTRIBUTION='%[3]s'
 			if err := d.getInstanceGroups(); err != nil {
 				return err
 			}
-			for _, ig := range d.instanceGroups[project][cluster] {
+			for _, ig := range d.instanceGroups[project][cluster.name] {
 				filters = append(filters, fmt.Sprintf("(metadata.created-by:*%s)", ig.path))
 			}
 		}

kubetest2-gke/deployer/firewall.go

@@ -42,10 +42,11 @@ func (d *deployer) ensureFirewallRules() error {
 }
 
 // Ensure firewall rules for e2e testing for all clusters in one single project.
-func ensureFirewallRulesForSingleProject(project, network string, clusters []string, instanceGroups map[string]map[string][]*ig) error {
+func ensureFirewallRulesForSingleProject(project, network string, clusters []cluster, instanceGroups map[string]map[string][]*ig) error {
 	for _, cluster := range clusters {
-		klog.V(1).Infof("Ensuring firewall rules for cluster %s in %s", cluster, project)
-		firewall := getClusterFirewall(project, cluster, instanceGroups)
+		clusterName := cluster.name
+		klog.V(1).Infof("Ensuring firewall rules for cluster %s in %s", clusterName, project)
+		firewall := getClusterFirewall(project, clusterName, instanceGroups)
 		if runWithNoOutput(exec.Command("gcloud", "compute", "firewall-rules", "describe", firewall,
 			"--project="+project,
 			"--format=value(name)")) == nil {
@@ -56,7 +57,7 @@ func ensureFirewallRulesForSingleProject(project, network string, clusters []str
 
 		tagOut, err := exec.Output(exec.Command("gcloud", "compute", "instances", "list",
 			"--project="+project,
-			"--filter=metadata.created-by:*"+instanceGroups[project][cluster][0].path,
+			"--filter=metadata.created-by:*"+instanceGroups[project][clusterName][0].path,
 			"--limit=1",
 			"--format=get(tags.items)"))
 		if err != nil {
@@ -156,7 +157,9 @@ func (d *deployer) getInstanceGroups() error {
 		d.instanceGroups[project] = map[string][]*ig{}
 
 		for _, cluster := range d.projectClustersLayout[project] {
-			igs, err := exec.Output(exec.Command("gcloud", containerArgs("clusters", "describe", cluster,
+			clusterName := cluster.name
+
+			igs, err := exec.Output(exec.Command("gcloud", containerArgs("clusters", "describe", clusterName,
 				"--format=value(instanceGroupUrls)",
 				"--project="+project,
 				location)...))
@@ -170,14 +173,14 @@ func (d *deployer) getInstanceGroups() error {
 			sort.Strings(igURLs)
 
 			// Initialize cluster instance groups
-			d.instanceGroups[project][cluster] = make([]*ig, 0)
+			d.instanceGroups[project][clusterName] = make([]*ig, 0)
 
 			for _, igURL := range igURLs {
 				m := poolRe.FindStringSubmatch(igURL)
 				if len(m) == 0 {
 					return fmt.Errorf("instanceGroupUrl %q did not match regex %v", igURL, poolRe)
 				}
-				d.instanceGroups[project][cluster] = append(d.instanceGroups[project][cluster], &ig{path: m[0], zone: m[1], name: m[2], uniq: m[3]})
+				d.instanceGroups[project][clusterName] = append(d.instanceGroups[project][clusterName], &ig{path: m[0], zone: m[1], name: m[2], uniq: m[3]})
 			}
 		}
 	}

kubetest2-gke/deployer/network.go

@@ -38,7 +38,16 @@ etag: %s
 `
 
 func (d *deployer) verifyNetworkFlags() error {
-	// For single project, no verification is needed.
+	// Verify private cluster args.
+	if d.privateClusterAccessLevel != "" && d.privateClusterAccessLevel != string(no) &&
+		d.privateClusterAccessLevel != string(limited) && d.privateClusterAccessLevel != string(unrestricted) {
+		return fmt.Errorf("--private-cluster-access-level must be one of %v", []string{"", string(no), string(limited), string(unrestricted)})
+	}
+	if d.privateClusterAccessLevel != "" && len(d.clusters) != len(d.privateClusterMasterIPRanges) {
+		return fmt.Errorf("--private-cluster-master-ip-range must have the same length as the number of clusters when requesting private cluster(s)")
+	}
+
+	// For single project, no other verification is needed.
 	numProjects := len(d.projects)
 	if numProjects == 0 {
 		numProjects = d.boskosProjectsRequested
@@ -64,14 +73,6 @@ func (d *deployer) verifyNetworkFlags() error {
 		}
 	}
 
-	if d.privateClusterAccessLevel != "" && d.privateClusterAccessLevel != string(no) &&
-		d.privateClusterAccessLevel != string(limited) && d.privateClusterAccessLevel != string(unrestricted) {
-		return fmt.Errorf("--private-cluster-access-level must be one of %v", []string{"", string(no), string(limited), string(unrestricted)})
-	}
-	if d.privateClusterAccessLevel != "" && d.privateClusterMasterIPRange == "" {
-		return fmt.Errorf("--private-cluster-master-ip-range must not be empty when requesting a private cluster")
-	}
-
 	return nil
 }
 

kubetest2-gke/deployer/up.go

@@ -70,7 +70,7 @@ func (d *deployer) Up() error {
 		subNetworkArgs := subNetworkArgs(d.projects, d.region, d.network, i)
 		for j := range d.projectClustersLayout[project] {
 			cluster := d.projectClustersLayout[project][j]
-			privateClusterArgs := privateClusterArgs(d.network, cluster, d.privateClusterAccessLevel, d.privateClusterMasterIPRange)
+			privateClusterArgs := privateClusterArgs(d.network, cluster.name, d.privateClusterAccessLevel, d.privateClusterMasterIPRanges[cluster.index])
 			eg.Go(func() error {
 				// Create the cluster
 				args := make([]string, len(d.createCommand()))
@@ -104,7 +104,7 @@ func (d *deployer) Up() error {
 				}
 				args = append(args, subNetworkArgs...)
 				args = append(args, privateClusterArgs...)
-				args = append(args, cluster)
+				args = append(args, cluster.name)
 				klog.V(1).Infof("Gcloud command: gcloud %+v\n", args)
 				if err := runWithOutput(exec.CommandContext(ctx, "gcloud", args...)); err != nil {
 					// Cancel the context to kill other cluster creation processes if any error happens.
@@ -138,7 +138,7 @@ func (d *deployer) IsUp() (up bool, err error) {
 
 	for _, project := range d.projects {
 		for _, cluster := range d.projectClustersLayout[project] {
-			if err := getClusterCredentials(project, location(d.region, d.zone), cluster); err != nil {
+			if err := getClusterCredentials(project, location(d.region, d.zone), cluster.name); err != nil {
 				return false, err
 			}
 
@@ -197,11 +197,11 @@ func (d *deployer) Kubeconfig() (string, error) {
 	kubecfgFiles := make([]string, 0)
 	for _, project := range d.projects {
 		for _, cluster := range d.projectClustersLayout[project] {
-			filename := filepath.Join(tmpdir, fmt.Sprintf("kubecfg-%s-%s", project, cluster))
+			filename := filepath.Join(tmpdir, fmt.Sprintf("kubecfg-%s-%s", project, cluster.name))
 			if err := os.Setenv("KUBECONFIG", filename); err != nil {
 				return "", err
 			}
-			if err := getClusterCredentials(project, location(d.region, d.zone), cluster); err != nil {
+			if err := getClusterCredentials(project, location(d.region, d.zone), cluster.name); err != nil {
 				return "", err
 			}
 			kubecfgFiles = append(kubecfgFiles, filename)
@@ -217,9 +217,7 @@ func (d *deployer) verifyUpFlags() error {
 	if len(d.projects) == 0 && d.boskosProjectsRequested <= 0 {
 		return fmt.Errorf("either --project or --projects-requested with a value larger than 0 must be set for GKE deployment")
 	}
-	if err := d.verifyNetworkFlags(); err != nil {
-		return err
-	}
+
 	if len(d.clusters) == 0 {
 		if len(d.projects) > 1 || d.boskosProjectsRequested > 1 {
 			return fmt.Errorf("explicit --cluster-name must be set for multi-project profile")
@@ -231,6 +229,9 @@ func (d *deployer) verifyUpFlags() error {
 	} else {
 		klog.V(0).Infof("explicit --cluster-name specified, ignoring --num-clusters")
 	}
+	if err := d.verifyNetworkFlags(); err != nil {
+		return err
+	}
 	if err := d.verifyLocationFlags(); err != nil {
 		return err
 	}