diff --git a/cmd/kops/create_ig.go b/cmd/kops/create_ig.go index 3d7a5fe91e..415fdb36fe 100644 --- a/cmd/kops/create_ig.go +++ b/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 } diff --git a/cmd/kops/edit_instancegroup.go b/cmd/kops/edit_instancegroup.go index 0d446d786b..4cdea053c1 100644 --- a/cmd/kops/edit_instancegroup.go +++ b/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 } diff --git a/pkg/apis/kops/validation/cluster.go b/pkg/apis/kops/validation/cluster.go index d8e6ab4441..362f97d512 100644 --- a/pkg/apis/kops/validation/cluster.go +++ b/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 diff --git a/pkg/apis/kops/validation/cluster_test.go b/pkg/apis/kops/validation/cluster_test.go index 98b16d4a2d..dcaaf64d83 100644 --- a/pkg/apis/kops/validation/cluster_test.go +++ b/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) } } } diff --git a/pkg/apis/kops/validation/instancegroup.go b/pkg/apis/kops/validation/instancegroup.go index 48af151629..50a51a641c 100644 --- a/pkg/apis/kops/validation/instancegroup.go +++ b/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", diff --git a/pkg/apis/kops/validation/instancegroup_test.go b/pkg/apis/kops/validation/instancegroup_test.go index 9ecb2e0a07..947d7d51c5 100644 --- a/pkg/apis/kops/validation/instancegroup_test.go +++ b/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) + } + } + +} diff --git a/pkg/apis/kops/validation/legacy.go b/pkg/apis/kops/validation/legacy.go index ba82b10c5a..dc13b0841d 100644 --- a/pkg/apis/kops/validation/legacy.go +++ b/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 diff --git a/tests/integration/update_cluster/existing_iam/data/aws_launch_configuration_master-us-test-1a.masters.existing-iam.example.com_user_data b/tests/integration/update_cluster/existing_iam/data/aws_launch_configuration_master-us-test-1a.masters.existing-iam.example.com_user_data index 97eed3c18e..535827c7b8 100644 --- a/tests/integration/update_cluster/existing_iam/data/aws_launch_configuration_master-us-test-1a.masters.existing-iam.example.com_user_data +++ b/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 diff --git a/tests/integration/update_cluster/existing_iam/data/aws_launch_configuration_master-us-test-1b.masters.existing-iam.example.com_user_data b/tests/integration/update_cluster/existing_iam/data/aws_launch_configuration_master-us-test-1b.masters.existing-iam.example.com_user_data index 7ece69c523..628fe1a66d 100644 --- a/tests/integration/update_cluster/existing_iam/data/aws_launch_configuration_master-us-test-1b.masters.existing-iam.example.com_user_data +++ b/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 diff --git a/tests/integration/update_cluster/existing_iam/data/aws_launch_configuration_master-us-test-1c.masters.existing-iam.example.com_user_data b/tests/integration/update_cluster/existing_iam/data/aws_launch_configuration_master-us-test-1c.masters.existing-iam.example.com_user_data index 41c01b84fa..1e0d47f35d 100644 --- a/tests/integration/update_cluster/existing_iam/data/aws_launch_configuration_master-us-test-1c.masters.existing-iam.example.com_user_data +++ b/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 diff --git a/tests/integration/update_cluster/existing_iam/in-v1alpha2.yaml b/tests/integration/update_cluster/existing_iam/in-v1alpha2.yaml index dd4860ab2d..a57f3e6e4a 100644 --- a/tests/integration/update_cluster/existing_iam/in-v1alpha2.yaml +++ b/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 diff --git a/tests/integration/update_cluster/existing_iam/kubernetes.tf b/tests/integration/update_cluster/existing_iam/kubernetes.tf index 62ec92de28..6d0a7eb71e 100644 --- a/tests/integration/update_cluster/existing_iam/kubernetes.tf +++ b/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" } diff --git a/upup/pkg/fi/cloudup/deepvalidate_test.go b/upup/pkg/fi/cloudup/deepvalidate_test.go index f479a7e972..3b0314b562 100644 --- a/upup/pkg/fi/cloudup/deepvalidate_test.go +++ b/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 { diff --git a/upup/pkg/fi/cloudup/populateinstancegroup_test.go b/upup/pkg/fi/cloudup/populateinstancegroup_test.go index a28e11e040..079df7a40b 100644 --- a/upup/pkg/fi/cloudup/populateinstancegroup_test.go +++ b/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 } diff --git a/upup/pkg/fi/cloudup/validation_test.go b/upup/pkg/fi/cloudup/validation_test.go index 0e17d5cb0e..ae82872bcc 100644 --- a/upup/pkg/fi/cloudup/validation_test.go +++ b/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.InstanceGroup = fi.String(zone) + m.Name = subnet.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)