Fix panic on empty/sign-only --kubernetes-feature-gates

upup/pkg/fi/cloudup/awsup/aws_utils.go

@@ -92,14 +92,15 @@ func ValidateRegion(ctx context.Context, region string) error {
 func FindRegion(cluster *kops.Cluster) (string, error) {
 	region := ""
 
-	nodeZones := make(map[string]bool)
 	for _, subnet := range cluster.Spec.Networking.Subnets {
+		if subnet.Zone == "" {
+			continue
+		}
+
 		if len(subnet.Zone) <= 2 {
 			return "", fmt.Errorf("invalid AWS zone: %q in subnet %q", subnet.Zone, subnet.Name)
 		}
 
-		nodeZones[subnet.Zone] = true
-
 		zoneRegion := subnet.Zone[:len(subnet.Zone)-1]
 		if region != "" && zoneRegion != region {
 			return "", fmt.Errorf("error Clusters cannot span multiple regions (found zone %q, but region is %q)", subnet.Zone, region)
@@ -108,6 +109,10 @@ func FindRegion(cluster *kops.Cluster) (string, error) {
 		region = zoneRegion
 	}
 
+	if region == "" {
+		return "", fmt.Errorf("no AWS subnet zones were specified")
+	}
+
 	return region, nil
 }
 

upup/pkg/fi/cloudup/awsup/aws_utils_test.go

@@ -66,6 +66,35 @@ func TestFindRegion(t *testing.T) {
 			t.Fatalf("unexpected region for zone: %q vs %q", expected, region)
 		}
 	}
+
+	t.Run("ignores missing zones when other subnets specify a region", func(t *testing.T) {
+		c := &kops.Cluster{}
+		c.Spec.Networking.Subnets = []kops.ClusterSubnetSpec{
+			{Name: "subnet-a", ID: "subnet-12345678"},
+			{Name: "subnet-b", Zone: "us-east-2b"},
+		}
+
+		region, err := FindRegion(c)
+		if err != nil {
+			t.Fatalf("unexpected error finding region: %v", err)
+		}
+
+		if region != "us-east-2" {
+			t.Fatalf("unexpected region: %q", region)
+		}
+	})
+
+	t.Run("errors when no subnet zones are provided", func(t *testing.T) {
+		c := &kops.Cluster{}
+		c.Spec.Networking.Subnets = []kops.ClusterSubnetSpec{
+			{Name: "subnet-a", ID: "subnet-12345678"},
+		}
+
+		region, err := FindRegion(c)
+		if err == nil {
+			t.Fatalf("expected error finding region, got %q", region)
+		}
+	})
 }
 
 func TestEC2TagSpecification(t *testing.T) {

upup/pkg/fi/cloudup/new_cluster.go

@@ -202,6 +202,29 @@ type NewClusterResult struct {
 	Channel *api.Channel
 }
 
+// parseFeatureGates parses feature gates in the form of "+GateName" or "-GateName".
+// Returns a map of gate name to enabled bool, or error if parsing fails.
+func parseFeatureGates(gates []string) (map[string]bool, error) {
+	result := make(map[string]bool)
+	for _, featureGate := range gates {
+		if len(featureGate) == 0 {
+			return nil, fmt.Errorf("--kubernetes-feature-gates: empty feature gate name")
+		}
+		enabled := true
+		if featureGate[0] == '+' {
+			featureGate = featureGate[1:]
+		} else if featureGate[0] == '-' {
+			enabled = false
+			featureGate = featureGate[1:]
+		}
+		if len(featureGate) == 0 {
+			return nil, fmt.Errorf("--kubernetes-feature-gates: feature gate name is empty after stripping sign prefix")
+		}
+		result[featureGate] = enabled
+	}
+	return result, nil
+}
+
 // NewCluster initializes cluster and instance groups specifications as
 // intended for newly created clusters.
 // It is the responsibility of the caller to call cloudup.PerformAssignments() on
@@ -284,20 +307,16 @@ func NewCluster(opt *NewClusterOptions, clientset simple.Clientset) (*NewCluster
 			FeatureGates: make(map[string]string),
 		}
 
-		for _, featureGate := range opt.KubernetesFeatureGates {
-			enabled := true
-			if featureGate[0] == '+' {
-				featureGate = featureGate[1:]
-			}
-			if featureGate[0] == '-' {
-				enabled = false
-				featureGate = featureGate[1:]
-			}
-			cluster.Spec.Kubelet.FeatureGates[featureGate] = strconv.FormatBool(enabled)
-			cluster.Spec.KubeAPIServer.FeatureGates[featureGate] = strconv.FormatBool(enabled)
-			cluster.Spec.KubeControllerManager.FeatureGates[featureGate] = strconv.FormatBool(enabled)
-			cluster.Spec.KubeProxy.FeatureGates[featureGate] = strconv.FormatBool(enabled)
-			cluster.Spec.KubeScheduler.FeatureGates[featureGate] = strconv.FormatBool(enabled)
+		gates, err := parseFeatureGates(opt.KubernetesFeatureGates)
+		if err != nil {
+			return nil, err
+		}
+		for name, enabled := range gates {
+			cluster.Spec.Kubelet.FeatureGates[name] = strconv.FormatBool(enabled)
+			cluster.Spec.KubeAPIServer.FeatureGates[name] = strconv.FormatBool(enabled)
+			cluster.Spec.KubeControllerManager.FeatureGates[name] = strconv.FormatBool(enabled)
+			cluster.Spec.KubeProxy.FeatureGates[name] = strconv.FormatBool(enabled)
+			cluster.Spec.KubeScheduler.FeatureGates[name] = strconv.FormatBool(enabled)
 		}
 	}
 

upup/pkg/fi/cloudup/new_cluster_test.go

@@ -602,3 +602,73 @@ func TestSetupZonesLinodeSingleRegionOnly(t *testing.T) {
 		t.Fatalf("unexpected error: %v", err)
 	}
 }
+
+func TestParseFeatureGates(t *testing.T) {
+	tests := []struct {
+		name        string
+		gates       []string
+		shouldError bool
+		expected    map[string]bool
+	}{
+		{
+			name:        "empty string",
+			gates:       []string{""},
+			shouldError: true,
+		},
+		{
+			name:        "sign only plus",
+			gates:       []string{"+"},
+			shouldError: true,
+		},
+		{
+			name:        "sign only minus",
+			gates:       []string{"-"},
+			shouldError: true,
+		},
+		{
+			name:        "simple feature gate",
+			gates:       []string{"Foo"},
+			shouldError: false,
+			expected:    map[string]bool{"Foo": true},
+		},
+		{
+			name:        "feature gate with plus",
+			gates:       []string{"+Bar"},
+			shouldError: false,
+			expected:    map[string]bool{"Bar": true},
+		},
+		{
+			name:        "feature gate with minus",
+			gates:       []string{"-Baz"},
+			shouldError: false,
+			expected:    map[string]bool{"Baz": false},
+		},
+		{
+			name:        "multiple gates",
+			gates:       []string{"Foo", "+Bar", "-Baz"},
+			shouldError: false,
+			expected:    map[string]bool{"Foo": true, "Bar": true, "Baz": false},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			result, err := parseFeatureGates(test.gates)
+
+			if test.shouldError {
+				if err == nil {
+					t.Errorf("expected error but got none")
+				}
+				return
+			}
+
+			if err != nil {
+				t.Errorf("unexpected error: %v", err)
+			}
+
+			if !reflect.DeepEqual(result, test.expected) {
+				t.Errorf("expected %v, got %v", test.expected, result)
+			}
+		})
+	}
+}

upup/pkg/fi/cloudup/subnets.go

@@ -82,7 +82,7 @@ func assignCIDRsToSubnets(c *kops.Cluster, cloud fi.Cloud) error {
 					return fmt.Errorf("Subnet %q has configured CIDR %q, but the actual CIDR found was %q", subnet.ID, subnet.CIDR, cloudSubnet.CIDR)
 				}
 
-				if subnet.Zone != cloudSubnet.Zone {
+				if subnet.Zone != "" && subnet.Zone != cloudSubnet.Zone {
 					return fmt.Errorf("Subnet %q has configured Zone %q, but the actual Zone found was %q", subnet.ID, subnet.Zone, cloudSubnet.Zone)
 				}
 

upup/pkg/fi/cloudup/utils.go

@@ -93,7 +93,9 @@ func BuildCloud(cluster *kops.Cluster) (fi.Cloud, error) {
 
 			var zoneNames []string
 			for _, subnet := range cluster.Spec.Networking.Subnets {
-				zoneNames = append(zoneNames, subnet.Zone)
+				if subnet.Zone != "" {
+					zoneNames = append(zoneNames, subnet.Zone)
+				}
 			}
 			err = awsup.ValidateZones(zoneNames, awsCloud)
 			if err != nil {