Kubetest2 - refactor how `kops create cluster` arguments are set

This allows kubetest's --create-args to take precedence over arguments we were otherwise hardcoding.
Specifically this will be used to hardcode the list of zones for some prow jobs with `--create-args="--zones us-east-1a"`.

tests/e2e/kubetest2-kops/deployer/up.go

@@ -84,52 +84,33 @@ func (d *deployer) createCluster(zones []string, adminAccess string) error {
 	args := []string{
 		d.KopsBinaryPath, "create", "cluster",
 		"--name", d.ClusterName,
-		"--admin-access", adminAccess,
 		"--cloud", d.CloudProvider,
 		"--kubernetes-version", d.KubernetesVersion,
-		"--master-count", "1",
-		"--master-volume-size", "48",
-		"--node-count", "4",
-		"--node-volume-size", "48",
-		"--override", "cluster.spec.nodePortAccess=0.0.0.0/0",
 		"--ssh-public-key", d.SSHPublicKeyPath,
+		"--override", "cluster.spec.nodePortAccess=0.0.0.0/0",
 		"--yes",
 	}
 
-	if d.CloudProvider == "aws" {
-		zones, err := aws.RandomZones(1)
-		if err != nil {
-			return err
-		}
-		args = append(args, "--zones", strings.Join(zones, ","))
-
-		args = append(args, "--master-size", "c5.large")
-	}
-
-	if d.CloudProvider == "gce" {
-		zones, err := gce.RandomZones(1)
-		if err != nil {
-			return err
-		}
-		args = append(args, "--zones", strings.Join(zones, ","))
-
-		args = append(args, "--master-size", "e2-standard-2")
-	}
-
-	if d.CloudProvider == "digitalocean" {
-		zones, err := do.RandomZones(1)
-		if err != nil {
-			return err
-		}
-		args = append(args, "--zones", strings.Join(zones, ","))
-		args = append(args, "--master-size", "s-8vcpu-16gb")
-	}
-
 	if d.CreateArgs != "" {
-		// TODO: we should only set the above flags if they're not in CreateArgs
-		// allowing any flag to be overridden
 		args = append(args, strings.Split(d.CreateArgs, " ")...)
 	}
+	args = appendIfUnset(args, "--admin-access", adminAccess)
+	args = appendIfUnset(args, "--master-count", "1")
+	args = appendIfUnset(args, "--master-volume-size", "48")
+	args = appendIfUnset(args, "--node-count", "4")
+	args = appendIfUnset(args, "--node-volume-size", "48")
+	args = appendIfUnset(args, "--override", adminAccess)
+	args = appendIfUnset(args, "--admin-access", adminAccess)
+	args = appendIfUnset(args, "--zones", strings.Join(zones, ","))
+
+	switch d.CloudProvider {
+	case "aws":
+		args = appendIfUnset(args, "--master-size", "c5.large")
+	case "gce":
+		args = appendIfUnset(args, "--master-size", "e2-standard-2")
+	case "digitalocean":
+		args = appendIfUnset(args, "--master-size", "s-8vcpu-16gb")
+	}
 
 	klog.Info(strings.Join(args, " "))
 	cmd := exec.Command(args[0], args[1:]...)
@@ -184,6 +165,20 @@ func (d *deployer) zones() ([]string, error) {
 		return aws.RandomZones(1)
 	case "gce":
 		return gce.RandomZones(1)
+	case "digitalocean":
+		return do.RandomZones(1)
 	}
 	return nil, fmt.Errorf("unsupported CloudProvider: %v", d.CloudProvider)
 }
+
+// appendIfUnset will append an argument and its value to args if the arg is not already present
+// This shouldn't be used for arguments that can be specified multiple times like --override
+func appendIfUnset(args []string, arg, value string) []string {
+	for _, existingArg := range args {
+		if existingArg == arg {
+			return args
+		}
+	}
+	args = append(args, arg, value)
+	return args
+}

tests/e2e/kubetest2-kops/deployer/up_test.go

@@ -0,0 +1,77 @@
+/*
+Copyright 2021 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 deployer
+
+import (
+	"reflect"
+	"testing"
+)
+
+func TestAppendIfUnset(t *testing.T) {
+	cases := []struct {
+		name     string
+		args     []string
+		arg      string
+		val      string
+		expected []string
+	}{
+		{
+			"empty",
+			[]string{},
+			"--foo",
+			"bar",
+			[]string{"--foo", "bar"},
+		},
+		{
+			"unset",
+			[]string{"--baz"},
+			"--foo",
+			"bar",
+			[]string{"--baz", "--foo", "bar"},
+		},
+		{
+			"set without value",
+			[]string{"--foo"},
+			"--foo",
+			"bar",
+			[]string{"--foo"},
+		},
+		{
+			"set with different value",
+			[]string{"--foo", "123"},
+			"--foo",
+			"bar",
+			[]string{"--foo", "123"},
+		},
+		{
+			"set with same value",
+			[]string{"--foo", "bar"},
+			"--foo",
+			"bar",
+			[]string{"--foo", "bar"},
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			actual := appendIfUnset(tc.args, tc.arg, tc.val)
+			if !reflect.DeepEqual(actual, tc.expected) {
+				t.Errorf("arguments didnt match: %v vs %v", actual, tc.expected)
+			}
+		})
+	}
+}