Merge pull request #9017 from olemarkus/change-zone

Allow changing AZ of masters

cmd/kops/create_ig.go

@@ -227,7 +227,7 @@ func RunCreateInstanceGroup(ctx context.Context, f *util.Factory, cmd *cobra.Com
 			return fmt.Errorf("unexpected object type: %T", obj)
 		}
 
-		err = validation.ValidateInstanceGroup(group).ToAggregate()
+		err = validation.CrossValidateInstanceGroup(group, cluster).ToAggregate()
 		if err != nil {
 			return err
 		}

cmd/kops/edit_instancegroup.go

@@ -177,7 +177,7 @@ func RunEditInstanceGroup(ctx context.Context, f *util.Factory, cmd *cobra.Comma
 		return err
 	}
 
-	err = validation.CrossValidateInstanceGroup(fullGroup, fullCluster, true).ToAggregate()
+	err = validation.CrossValidateInstanceGroup(fullGroup, fullCluster).ToAggregate()
 	if err != nil {
 		return err
 	}

pkg/apis/kops/validation/cluster.go

@@ -92,13 +92,6 @@ func validateEtcdClusterUpdate(fp *field.Path, obj *kops.EtcdClusterSpec, status
 				allErrs = append(allErrs, validateEtcdMemberUpdate(fp, newMember, etcdClusterStatus, oldMember)...)
 			}
 		}
-		for k := range oldMembers {
-			newCluster := newMembers[k]
-			if newCluster == nil {
-				fp := fp.Child("etcdMembers").Key(k)
-				allErrs = append(allErrs, field.Forbidden(fp, "EtcdCluster members cannot be removed"))
-			}
-		}
 	}
 
 	return allErrs

pkg/apis/kops/validation/cluster_test.go

@@ -31,6 +31,54 @@ func TestValidEtcdChanges(t *testing.T) {
 		Status  *kops.ClusterStatus
 		Details string
 	}{
+		{
+			OldSpec: &kops.EtcdClusterSpec{
+				Name: "main",
+				Members: []*kops.EtcdMemberSpec{
+					{
+						Name:          "a",
+						InstanceGroup: fi.String("eu-central-1a"),
+					},
+					{
+						Name:          "b",
+						InstanceGroup: fi.String("eu-central-1b"),
+					},
+					{
+						Name:          "c",
+						InstanceGroup: fi.String("eu-central-1c"),
+					},
+				},
+			},
+
+			NewSpec: &kops.EtcdClusterSpec{
+				Name: "main",
+				Members: []*kops.EtcdMemberSpec{
+					{
+						Name:          "a",
+						InstanceGroup: fi.String("eu-central-1a"),
+					},
+					{
+						Name:          "b",
+						InstanceGroup: fi.String("eu-central-1b"),
+					},
+					{
+						Name:          "d",
+						InstanceGroup: fi.String("eu-central-1d"),
+					},
+				},
+			},
+
+			Status: &kops.ClusterStatus{
+				EtcdClusters: []kops.EtcdClusterStatus{
+					{
+						Name: "main",
+					},
+				},
+			},
+
+			Details: "Could not move master from one zone to another",
+		},
+
 		{
 			OldSpec: &kops.EtcdClusterSpec{
 				Name: "main",
@@ -107,7 +155,7 @@ func TestValidEtcdChanges(t *testing.T) {
 	for _, g := range grid {
 		errorList := validateEtcdClusterUpdate(nil, g.NewSpec, g.Status, g.OldSpec)
 		if len(errorList) != 0 {
-			t.Error(g.Details)
+			t.Errorf("%v: %v", g.Details, errorList)
 		}
 	}
 }

pkg/apis/kops/validation/instancegroup.go

@@ -17,6 +17,7 @@ limitations under the License.
 package validation
 
 import (
+	"fmt"
 	"strings"
 
 	"github.com/aws/aws-sdk-go/aws/arn"
@@ -189,9 +190,13 @@ func validateVolumeMountSpec(path *field.Path, spec *kops.VolumeMountSpec) field
 
 // CrossValidateInstanceGroup performs validation of the instance group, including that it is consistent with the Cluster
 // It calls ValidateInstanceGroup, so all that validation is included.
-func CrossValidateInstanceGroup(g *kops.InstanceGroup, cluster *kops.Cluster, strict bool) field.ErrorList {
+func CrossValidateInstanceGroup(g *kops.InstanceGroup, cluster *kops.Cluster) field.ErrorList {
 	allErrs := ValidateInstanceGroup(g)
 
+	if g.Spec.Role == kops.InstanceGroupRoleMaster {
+		allErrs = append(allErrs, ValidateMasterInstanceGroup(g, cluster)...)
+	}
+
 	// Check that instance groups are defined in subnets that are defined in the cluster
 	{
 		clusterSubnets := make(map[string]*kops.ClusterSubnetSpec)
@@ -210,6 +215,23 @@ func CrossValidateInstanceGroup(g *kops.InstanceGroup, cluster *kops.Cluster, st
 	return allErrs
 }
 
+func ValidateMasterInstanceGroup(g *kops.InstanceGroup, cluster *kops.Cluster) field.ErrorList {
+	allErrs := field.ErrorList{}
+	for _, etcd := range cluster.Spec.EtcdClusters {
+		hasEtcd := false
+		for _, m := range etcd.Members {
+			if fi.StringValue(m.InstanceGroup) == g.ObjectMeta.Name {
+				hasEtcd = true
+				break
+			}
+		}
+		if !hasEtcd {
+			allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "metadata", "name"), fmt.Sprintf("InstanceGroup \"%s\" with role Master must have a member in etcd cluster \"%s\"", g.ObjectMeta.Name, etcd.Name)))
+		}
+	}
+	return allErrs
+}
+
 var validUserDataTypes = []string{
 	"text/x-include-once-url",
 	"text/x-include-url",

pkg/apis/kops/validation/instancegroup_test.go

@@ -19,6 +19,7 @@ package validation
 import (
 	"testing"
 
+	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/kops/pkg/apis/kops"
 	"k8s.io/kops/upup/pkg/fi"
@@ -90,3 +91,91 @@ func TestValidateInstanceProfile(t *testing.T) {
 		}
 	}
 }
+
+func TestValidMasterInstanceGroup(t *testing.T) {
+	grid := []struct {
+		Cluster        *kops.Cluster
+		IG             *kops.InstanceGroup
+		ExpectedErrors int
+		Description    string
+	}{
+		{
+			Cluster: &kops.Cluster{
+				Spec: kops.ClusterSpec{
+					EtcdClusters: []*kops.EtcdClusterSpec{
+						{
+							Name: "main",
+							Members: []*kops.EtcdMemberSpec{
+								{
+									Name:          "a",
+									InstanceGroup: fi.String("eu-central-1a"),
+								},
+								{
+									Name:          "b",
+									InstanceGroup: fi.String("eu-central-1b"),
+								},
+								{
+									Name:          "c",
+									InstanceGroup: fi.String("eu-central-1c"),
+								},
+							},
+						},
+					},
+				},
+			},
+			IG: &kops.InstanceGroup{
+				ObjectMeta: v1.ObjectMeta{
+					Name: "eu-central-1a",
+				},
+				Spec: kops.InstanceGroupSpec{
+					Role: kops.InstanceGroupRoleMaster,
+				},
+			},
+			ExpectedErrors: 0,
+			Description:    "Valid instance group failed to validate",
+		},
+		{
+			Cluster: &kops.Cluster{
+				Spec: kops.ClusterSpec{
+					EtcdClusters: []*kops.EtcdClusterSpec{
+						{
+							Name: "main",
+							Members: []*kops.EtcdMemberSpec{
+								{
+									Name:          "a",
+									InstanceGroup: fi.String("eu-central-1a"),
+								},
+								{
+									Name:          "b",
+									InstanceGroup: fi.String("eu-central-1b"),
+								},
+								{
+									Name:          "c",
+									InstanceGroup: fi.String("eu-central-1c"),
+								},
+							},
+						},
+					},
+				},
+			},
+			IG: &kops.InstanceGroup{
+				ObjectMeta: v1.ObjectMeta{
+					Name: "eu-central-1d",
+				},
+				Spec: kops.InstanceGroupSpec{
+					Role: kops.InstanceGroupRoleMaster,
+				},
+			},
+			ExpectedErrors: 1,
+			Description:    "Master IG without etcd member validated",
+		},
+	}
+
+	for _, g := range grid {
+		errList := ValidateMasterInstanceGroup(g.IG, g.Cluster)
+		if len(errList) != g.ExpectedErrors {
+			t.Error(g.Description)
+		}
+	}
+
+}

pkg/apis/kops/validation/legacy.go

@@ -654,7 +654,7 @@ func DeepValidate(c *kops.Cluster, groups []*kops.InstanceGroup, strict bool) er
 	}
 
 	for _, g := range groups {
-		errs := CrossValidateInstanceGroup(g, c, strict)
+		errs := CrossValidateInstanceGroup(g, c)
 
 		// Additional cloud-specific validation rules,
 		// such as making sure that identifiers match the expected formats for the given cloud

tests/integration/update_cluster/existing_iam/data/aws_launch_configuration_master-us-test-1a.masters.existing-iam.example.com_user_data

@@ -153,7 +153,7 @@ etcdClusters:
 kubeAPIServer:
   allowPrivileged: true
   anonymousAuth: false
-  apiServerCount: 1
+  apiServerCount: 3
   authorizationMode: AlwaysAllow
   bindAddress: 0.0.0.0
   cloudProvider: aws

tests/integration/update_cluster/existing_iam/data/aws_launch_configuration_master-us-test-1b.masters.existing-iam.example.com_user_data

@@ -153,7 +153,7 @@ etcdClusters:
 kubeAPIServer:
   allowPrivileged: true
   anonymousAuth: false
-  apiServerCount: 1
+  apiServerCount: 3
   authorizationMode: AlwaysAllow
   bindAddress: 0.0.0.0
   cloudProvider: aws

tests/integration/update_cluster/existing_iam/data/aws_launch_configuration_master-us-test-1c.masters.existing-iam.example.com_user_data

@@ -153,7 +153,7 @@ etcdClusters:
 kubeAPIServer:
   allowPrivileged: true
   anonymousAuth: false
-  apiServerCount: 1
+  apiServerCount: 3
   authorizationMode: AlwaysAllow
   bindAddress: 0.0.0.0
   cloudProvider: aws

tests/integration/update_cluster/existing_iam/in-v1alpha2.yaml

@@ -13,10 +13,18 @@ spec:
   - etcdMembers:
     - instanceGroup: master-us-test-1a
       name: a
+    - instanceGroup: master-us-test-1b
+      name: b
+    - instanceGroup: master-us-test-1c
+      name: c
     name: main
   - etcdMembers:
     - instanceGroup: master-us-test-1a
       name: a
+    - instanceGroup: master-us-test-1b
+      name: b
+    - instanceGroup: master-us-test-1c
+      name: c
     name: events
   kubelet:
     anonymousAuth: false

tests/integration/update_cluster/existing_iam/kubernetes.tf

@@ -217,7 +217,7 @@ resource "aws_ebs_volume" "a-etcd-events-existing-iam-example-com" {
   tags = {
     "KubernetesCluster"                              = "existing-iam.example.com"
     "Name"                                           = "a.etcd-events.existing-iam.example.com"
-    "k8s.io/etcd/events"                             = "a/a"
+    "k8s.io/etcd/events"                             = "a/a,b,c"
     "k8s.io/role/master"                             = "1"
     "kubernetes.io/cluster/existing-iam.example.com" = "owned"
   }
@@ -231,7 +231,63 @@ resource "aws_ebs_volume" "a-etcd-main-existing-iam-example-com" {
   tags = {
     "KubernetesCluster"                              = "existing-iam.example.com"
     "Name"                                           = "a.etcd-main.existing-iam.example.com"
-    "k8s.io/etcd/main"                               = "a/a"
+    "k8s.io/etcd/main"                               = "a/a,b,c"
+    "k8s.io/role/master"                             = "1"
+    "kubernetes.io/cluster/existing-iam.example.com" = "owned"
+  }
+  type = "gp2"
+}
+
+resource "aws_ebs_volume" "b-etcd-events-existing-iam-example-com" {
+  availability_zone = "us-test-1b"
+  encrypted         = false
+  size              = 20
+  tags = {
+    "KubernetesCluster"                              = "existing-iam.example.com"
+    "Name"                                           = "b.etcd-events.existing-iam.example.com"
+    "k8s.io/etcd/events"                             = "b/a,b,c"
+    "k8s.io/role/master"                             = "1"
+    "kubernetes.io/cluster/existing-iam.example.com" = "owned"
+  }
+  type = "gp2"
+}
+
+resource "aws_ebs_volume" "b-etcd-main-existing-iam-example-com" {
+  availability_zone = "us-test-1b"
+  encrypted         = false
+  size              = 20
+  tags = {
+    "KubernetesCluster"                              = "existing-iam.example.com"
+    "Name"                                           = "b.etcd-main.existing-iam.example.com"
+    "k8s.io/etcd/main"                               = "b/a,b,c"
+    "k8s.io/role/master"                             = "1"
+    "kubernetes.io/cluster/existing-iam.example.com" = "owned"
+  }
+  type = "gp2"
+}
+
+resource "aws_ebs_volume" "c-etcd-events-existing-iam-example-com" {
+  availability_zone = "us-test-1c"
+  encrypted         = false
+  size              = 20
+  tags = {
+    "KubernetesCluster"                              = "existing-iam.example.com"
+    "Name"                                           = "c.etcd-events.existing-iam.example.com"
+    "k8s.io/etcd/events"                             = "c/a,b,c"
+    "k8s.io/role/master"                             = "1"
+    "kubernetes.io/cluster/existing-iam.example.com" = "owned"
+  }
+  type = "gp2"
+}
+
+resource "aws_ebs_volume" "c-etcd-main-existing-iam-example-com" {
+  availability_zone = "us-test-1c"
+  encrypted         = false
+  size              = 20
+  tags = {
+    "KubernetesCluster"                              = "existing-iam.example.com"
+    "Name"                                           = "c.etcd-main.existing-iam.example.com"
+    "k8s.io/etcd/main"                               = "c/a,b,c"
     "k8s.io/role/master"                             = "1"
     "kubernetes.io/cluster/existing-iam.example.com" = "owned"
   }

upup/pkg/fi/cloudup/deepvalidate_test.go

@@ -118,10 +118,12 @@ func TestDeepValidate_ExtraMasterZone(t *testing.T) {
 		{Name: "mock1b", Zone: "us-mock-1b", CIDR: "172.20.2.0/24"},
 	}
 	var groups []*kopsapi.InstanceGroup
-	groups = append(groups, buildMinimalMasterInstanceGroup("subnet-us-mock-1a", "subnet-us-mock-1b", "subnet-us-mock-1c"))
+	groups = append(groups, buildMinimalMasterInstanceGroup("subnet-us-mock-1a"))
+	groups = append(groups, buildMinimalMasterInstanceGroup("subnet-us-mock-1b"))
+	groups = append(groups, buildMinimalMasterInstanceGroup("subnet-us-mock-1c"))
 	groups = append(groups, buildMinimalNodeInstanceGroup("subnet-us-mock-1a", "subnet-us-mock-1b"))
 
-	expectErrorFromDeepValidate(t, c, groups, "[spec.subnets[0]: Not found: \"subnet-us-mock-1a\", spec.subnets[1]: Not found: \"subnet-us-mock-1b\", spec.subnets[2]: Not found: \"subnet-us-mock-1c\"]")
+	expectErrorFromDeepValidate(t, c, groups, "spec.subnets[0]: Not found: \"subnet-us-mock-1a\"")
 }
 
 func TestDeepValidate_EvenEtcdClusterSize(t *testing.T) {
@@ -137,12 +139,38 @@ func TestDeepValidate_EvenEtcdClusterSize(t *testing.T) {
 	}
 
 	var groups []*kopsapi.InstanceGroup
-	groups = append(groups, buildMinimalMasterInstanceGroup("subnet-us-mock-1a", "subnet-us-mock-1b", "subnet-us-mock-1c", "subnet-us-mock-1d"))
+	groups = append(groups, buildMinimalMasterInstanceGroup("subnet-us-mock-1a"))
+	groups = append(groups, buildMinimalMasterInstanceGroup("subnet-us-mock-1b"))
+	groups = append(groups, buildMinimalMasterInstanceGroup("subnet-us-mock-1c"))
+	groups = append(groups, buildMinimalMasterInstanceGroup("subnet-us-mock-1d"))
 	groups = append(groups, buildMinimalNodeInstanceGroup("subnet-us-mock-1a"))
 
 	expectErrorFromDeepValidate(t, c, groups, "Should be an odd number of master-zones for quorum. Use --zones and --master-zones to declare node zones and master zones separately")
 }
 
+func TestDeepValidate_MissingEtcdMember(t *testing.T) {
+	c := buildDefaultCluster(t)
+	c.Spec.EtcdClusters = []*kopsapi.EtcdClusterSpec{
+		{
+			Name: "main",
+			Members: []*kopsapi.EtcdMemberSpec{
+				{Name: "us-mock-1a", InstanceGroup: fi.String("us-mock-1a")},
+				{Name: "us-mock-1b", InstanceGroup: fi.String("us-mock-1b")},
+				{Name: "us-mock-1c", InstanceGroup: fi.String("us-mock-1c")},
+			},
+		},
+	}
+
+	var groups []*kopsapi.InstanceGroup
+	groups = append(groups, buildMinimalMasterInstanceGroup("subnet-us-mock-1a"))
+	groups = append(groups, buildMinimalMasterInstanceGroup("subnet-us-mock-1b"))
+	groups = append(groups, buildMinimalMasterInstanceGroup("subnet-us-mock-1c"))
+	groups = append(groups, buildMinimalMasterInstanceGroup("subnet-us-mock-1d"))
+	groups = append(groups, buildMinimalNodeInstanceGroup("subnet-us-mock-1a"))
+
+	expectErrorFromDeepValidate(t, c, groups, "spec.metadata.name: Forbidden: InstanceGroup \"master-subnet-us-mock-1a\" with role Master must have a member in etcd cluster \"main\"")
+}
+
 func expectErrorFromDeepValidate(t *testing.T, c *kopsapi.Cluster, groups []*kopsapi.InstanceGroup, message string) {
 	err := validation.DeepValidate(c, groups, true)
 	if err == nil {

upup/pkg/fi/cloudup/populateinstancegroup_test.go

@@ -33,11 +33,11 @@ func buildMinimalNodeInstanceGroup(subnets ...string) *kopsapi.InstanceGroup {
 	return g
 }
 
-func buildMinimalMasterInstanceGroup(subnets ...string) *kopsapi.InstanceGroup {
+func buildMinimalMasterInstanceGroup(subnet string) *kopsapi.InstanceGroup {
 	g := &kopsapi.InstanceGroup{}
-	g.ObjectMeta.Name = "master"
+	g.ObjectMeta.Name = "master-" + subnet
 	g.Spec.Role = kopsapi.InstanceGroupRoleMaster
-	g.Spec.Subnets = subnets
+	g.Spec.Subnets = []string{subnet}
 
 	return g
 }

upup/pkg/fi/cloudup/validation_test.go

@@ -21,7 +21,6 @@ import (
 	"strings"
 	"testing"
 
-	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/klog"
 	api "k8s.io/kops/pkg/apis/kops"
 	"k8s.io/kops/pkg/apis/kops/validation"
@@ -39,23 +38,20 @@ func buildDefaultCluster(t *testing.T) *api.Cluster {
 	}
 
 	if len(c.Spec.EtcdClusters) == 0 {
-		zones := sets.NewString()
-		for _, z := range c.Spec.Subnets {
-			zones.Insert(z.Zone)
-		}
-		etcdZones := zones.List()
 
 		for _, etcdCluster := range EtcdClusters {
+
 			etcd := &api.EtcdClusterSpec{}
 			etcd.Name = etcdCluster
-			for _, zone := range etcdZones {
+			for _, subnet := range c.Spec.Subnets {
 				m := &api.EtcdMemberSpec{}
-				m.Name = zone
+				m.Name = subnet.Zone
-				m.InstanceGroup = fi.String(zone)
+				m.InstanceGroup = fi.String("master-" + subnet.Name)
 				etcd.Members = append(etcd.Members, m)
 			}
 			c.Spec.EtcdClusters = append(c.Spec.EtcdClusters, etcd)
 		}
+
 	}
 
 	fullSpec, err := mockedPopulateClusterSpec(c)