From a7f82a638035a028fbf6589af1dfcdaaeafb2fde Mon Sep 17 00:00:00 2001 From: Justin Santa Barbara Date: Wed, 9 Aug 2017 19:45:33 -0400 Subject: [PATCH 1/2] Fix shared subnet/vpc tags * Stop setting the Name tag on a shared subnet/vpc * Stop setting the legacy KubernetesCluster tag on a shared subnet/vpc that is new enough (>=1.6); we rely on the shared tags instead * Set tags on shared subnets; i.e. we _do_ set the shared tag on a shared subnet; that is important for ELBs * Set tags on shared VPCs; i.e. we _do_ set the shared tag on a shared VPC; that is not used but consistent with subnets. * Add tests for shared subnet --- cloudmock/aws/mockec2/api.go | 2 +- cloudmock/aws/mockec2/subnets.go | 40 +++- pkg/model/context.go | 44 +++-- pkg/model/network.go | 10 +- upup/pkg/fi/cloudup/awstasks/subnet.go | 8 +- upup/pkg/fi/cloudup/awstasks/subnet_test.go | 197 ++++++++++++++++++++ upup/pkg/fi/cloudup/awstasks/vpc.go | 13 +- 7 files changed, 274 insertions(+), 40 deletions(-) diff --git a/cloudmock/aws/mockec2/api.go b/cloudmock/aws/mockec2/api.go index 8c21835ccddad..9758564cc73e8 100644 --- a/cloudmock/aws/mockec2/api.go +++ b/cloudmock/aws/mockec2/api.go @@ -33,7 +33,7 @@ type MockEC2 struct { SecurityGroups []*ec2.SecurityGroup subnetNumber int - Subnets []*ec2.Subnet + subnets map[string]*subnetInfo volumeNumber int Volumes []*ec2.Volume diff --git a/cloudmock/aws/mockec2/subnets.go b/cloudmock/aws/mockec2/subnets.go index 735541e774ef5..2fdcfc46fc82e 100644 --- a/cloudmock/aws/mockec2/subnets.go +++ b/cloudmock/aws/mockec2/subnets.go @@ -24,6 +24,29 @@ import ( "strings" ) +type subnetInfo struct { + main ec2.Subnet +} + +func (m *MockEC2) FindSubnet(id string) *ec2.Subnet { + subnet := m.subnets[id] + if subnet == nil { + return nil + } + + copy := subnet.main + copy.Tags = m.getTags(ec2.ResourceTypeSubnet, id) + return © +} + +func (m *MockEC2) SubnetIds() []string { + var ids []string + for id := range m.subnets { + ids = append(ids, id) + } + return ids +} + func (m *MockEC2) CreateSubnetRequest(*ec2.CreateSubnetInput) (*request.Request, *ec2.CreateSubnetOutput) { panic("Not implemented") return nil, nil @@ -40,7 +63,14 @@ func (m *MockEC2) CreateSubnet(request *ec2.CreateSubnetInput) (*ec2.CreateSubne VpcId: request.VpcId, CidrBlock: request.CidrBlock, } - m.Subnets = append(m.Subnets, subnet) + + if m.subnets == nil { + m.subnets = make(map[string]*subnetInfo) + } + m.subnets[*subnet.SubnetId] = &subnetInfo{ + main: *subnet, + } + response := &ec2.CreateSubnetOutput{ Subnet: subnet, } @@ -57,7 +87,7 @@ func (m *MockEC2) DescribeSubnets(request *ec2.DescribeSubnetsInput) (*ec2.Descr var subnets []*ec2.Subnet - for _, subnet := range m.Subnets { + for id, subnet := range m.subnets { allFiltersMatch := true for _, filter := range request.Filters { match := false @@ -65,7 +95,7 @@ func (m *MockEC2) DescribeSubnets(request *ec2.DescribeSubnetsInput) (*ec2.Descr default: if strings.HasPrefix(*filter.Name, "tag:") { - match = m.hasTag(ec2.ResourceTypeSubnet, *subnet.SubnetId, filter) + match = m.hasTag(ec2.ResourceTypeSubnet, *subnet.main.SubnetId, filter) } else { return nil, fmt.Errorf("unknown filter name: %q", *filter.Name) } @@ -81,8 +111,8 @@ func (m *MockEC2) DescribeSubnets(request *ec2.DescribeSubnetsInput) (*ec2.Descr continue } - copy := *subnet - copy.Tags = m.getTags(ec2.ResourceTypeSubnet, *subnet.SubnetId) + copy := subnet.main + copy.Tags = m.getTags(ec2.ResourceTypeSubnet, id) subnets = append(subnets, ©) } diff --git a/pkg/model/context.go b/pkg/model/context.go index ed64a59ef0fdb..ecc3be3cef159 100644 --- a/pkg/model/context.go +++ b/pkg/model/context.go @@ -215,9 +215,27 @@ func (m *KopsModelContext) CloudTags(name string, shared bool) map[string]string switch kops.CloudProviderID(m.Cluster.Spec.CloudProvider) { case kops.CloudProviderAWS: - tags[awsup.TagClusterName] = m.Cluster.ObjectMeta.Name - if name != "" { - tags["Name"] = name + if shared { + // If the resource is shared, we don't try to set the Name - we presume that is managed externally + glog.V(4).Infof("Skipping Name tag for shared resource") + } else { + if name != "" { + tags["Name"] = name + } + } + + // Kubernetes 1.6 introduced the shared ownership tag; that replaces TagClusterName + setLegacyTag := true + if m.IsKubernetesGTE("1.6") { + // For the moment, we only skip the legacy tag for shared resources + // (other people may be using it) + if shared { + glog.V(4).Infof("Skipping %q tag for shared resource", awsup.TagClusterName) + setLegacyTag = false + } + } + if setLegacyTag { + tags[awsup.TagClusterName] = m.Cluster.ObjectMeta.Name } if shared { @@ -284,32 +302,26 @@ func (c *KopsModelContext) UseEtcdTLS() bool { } // KubernetesVersion parses the semver version of kubernetes, from the cluster spec -func (c *KopsModelContext) KubernetesVersion() (semver.Version, error) { +func (c *KopsModelContext) KubernetesVersion() semver.Version { // TODO: Remove copy-pasting c.f. https://github.com/kubernetes/kops/blob/master/pkg/model/components/context.go#L32 kubernetesVersion := c.Cluster.Spec.KubernetesVersion if kubernetesVersion == "" { - return semver.Version{}, fmt.Errorf("KubernetesVersion is required") + glog.Fatalf("KubernetesVersion is required") } sv, err := util.ParseKubernetesVersion(kubernetesVersion) if err != nil { - return semver.Version{}, fmt.Errorf("unable to determine kubernetes version from %q", kubernetesVersion) + glog.Fatalf("unable to determine kubernetes version from %q", kubernetesVersion) } - return *sv, nil + return *sv } -// VersionGTE is a simplified semver comparison -func VersionGTE(version semver.Version, major uint64, minor uint64) bool { - if version.Major > major { - return true - } - if version.Major == major && version.Minor >= minor { - return true - } - return false +// IsKubernetesGTE checks if the kubernetes version is at least version, ignoring prereleases / patches +func (c *KopsModelContext) IsKubernetesGTE(version string) bool { + return util.IsKubernetesGTE(version, c.KubernetesVersion()) } func (c *KopsModelContext) WellKnownServiceIP(id int) (net.IP, error) { diff --git a/pkg/model/network.go b/pkg/model/network.go index 3576017c2a00d..4076c492d6580 100644 --- a/pkg/model/network.go +++ b/pkg/model/network.go @@ -37,11 +37,6 @@ type NetworkModelBuilder struct { var _ fi.ModelBuilder = &NetworkModelBuilder{} func (b *NetworkModelBuilder) Build(c *fi.ModelBuilderContext) error { - kubernetesVersion, err := b.KubernetesVersion() - if err != nil { - return err - } - sharedVPC := b.Cluster.SharedVPC() vpcName := b.ClusterName() @@ -57,10 +52,10 @@ func (b *NetworkModelBuilder) Build(c *fi.ModelBuilderContext) error { Tags: tags, } - if sharedVPC && VersionGTE(kubernetesVersion, 1, 5) { + if sharedVPC && b.IsKubernetesGTE("1.5") { // If we're running k8s 1.5, and we have e.g. --kubelet-preferred-address-types=InternalIP,Hostname,ExternalIP,LegacyHostIP // then we don't need EnableDNSHostnames any more - glog.V(4).Infof("Kubernetes version %q; skipping EnableDNSHostnames requirement on VPC", kubernetesVersion) + glog.V(4).Infof("Kubernetes version %q; skipping EnableDNSHostnames requirement on VPC", b.KubernetesVersion()) } else { // In theory we don't need to enable it for >= 1.5, // but seems safer to stick with existing behaviour @@ -71,6 +66,7 @@ func (b *NetworkModelBuilder) Build(c *fi.ModelBuilderContext) error { if b.Cluster.Spec.NetworkID != "" { t.ID = s(b.Cluster.Spec.NetworkID) } + if b.Cluster.Spec.NetworkCIDR != "" { t.CIDR = s(b.Cluster.Spec.NetworkCIDR) } diff --git a/upup/pkg/fi/cloudup/awstasks/subnet.go b/upup/pkg/fi/cloudup/awstasks/subnet.go index d60212db0f955..28f9d7c57f0e8 100644 --- a/upup/pkg/fi/cloudup/awstasks/subnet.go +++ b/upup/pkg/fi/cloudup/awstasks/subnet.go @@ -82,7 +82,8 @@ func (e *Subnet) Find(c *fi.Context) (*Subnet, error) { e.ID = actual.ID // Prevent spurious changes - actual.Lifecycle = e.Lifecycle + actual.Lifecycle = e.Lifecycle // Lifecycle is not materialized in AWS + actual.Name = e.Name // Name is part of Tags return actual, nil } @@ -159,8 +160,6 @@ func (_ *Subnet) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *Subnet) error { if a == nil { return fmt.Errorf("Subnet with id %q not found", fi.StringValue(e.ID)) } - - return nil } if a == nil { @@ -210,6 +209,8 @@ func (_ *Subnet) RenderTerraform(t *terraform.TerraformTarget, a, e, changes *Su shared := fi.BoolValue(e.Shared) if shared { // Not terraform owned / managed + // We won't apply changes, but our validation (kops update) will still warn + // // We probably shouldn't output subnet_ids only in this case - we normally output them by role, // but removing it now might break people. We could always output subnet_ids though, if we // ever get a request for that. @@ -251,6 +252,7 @@ func (_ *Subnet) RenderCloudformation(t *cloudformation.CloudformationTarget, a, shared := fi.BoolValue(e.Shared) if shared { // Not cloudformation owned / managed + // We won't apply changes, but our validation (kops update) will still warn return nil } diff --git a/upup/pkg/fi/cloudup/awstasks/subnet_test.go b/upup/pkg/fi/cloudup/awstasks/subnet_test.go index 3b853a492087f..aeaab57204869 100644 --- a/upup/pkg/fi/cloudup/awstasks/subnet_test.go +++ b/upup/pkg/fi/cloudup/awstasks/subnet_test.go @@ -18,7 +18,12 @@ package awstasks import ( "fmt" + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "k8s.io/kops/cloudmock/aws/mockec2" "k8s.io/kops/upup/pkg/fi" + "k8s.io/kops/upup/pkg/fi/cloudup/awsup" + "reflect" "testing" ) @@ -56,3 +61,195 @@ func Test_Subnet_CannotChangeSubnet(t *testing.T) { t.Errorf("unexpected error: %v", err) } } + +func TestSubnetCreate(t *testing.T) { + cloud := awsup.BuildMockAWSCloud("us-east-1", "abc") + c := &mockec2.MockEC2{} + cloud.MockEC2 = c + + // We define a function so we can rebuild the tasks, because we modify in-place when running + buildTasks := func() map[string]fi.Task { + vpc1 := &VPC{ + Name: s("vpc1"), + CIDR: s("172.20.0.0/16"), + Tags: map[string]string{"Name": "vpc1"}, + } + subnet1 := &Subnet{ + Name: s("subnet1"), + VPC: vpc1, + CIDR: s("172.20.1.0/24"), + Tags: map[string]string{"Name": "subnet1"}, + } + + return map[string]fi.Task{ + "subnet1": subnet1, + "vpc1": vpc1, + } + } + + { + allTasks := buildTasks() + subnet1 := allTasks["subnet1"].(*Subnet) + + target := &awsup.AWSAPITarget{ + Cloud: cloud, + } + + context, err := fi.NewContext(target, cloud, nil, nil, nil, true, allTasks) + if err != nil { + t.Fatalf("error building context: %v", err) + } + + if err := context.RunTasks(defaultDeadline); err != nil { + t.Fatalf("unexpected error during Run: %v", err) + } + + if fi.StringValue(subnet1.ID) == "" { + t.Fatalf("ID not set after create") + } + + if len(c.SubnetIds()) != 1 { + t.Fatalf("Expected exactly one Subnet; found %v", c.SubnetIds()) + } + + expected := &ec2.Subnet{ + CidrBlock: aws.String("172.20.1.0/24"), + SubnetId: aws.String("subnet-1"), + VpcId: aws.String("vpc-1"), + Tags: buildTags(map[string]string{ + "Name": "subnet1", + }), + } + actual := c.FindSubnet(*subnet1.ID) + if actual == nil { + t.Fatalf("Subnet created but then not found") + } + if !reflect.DeepEqual(actual, expected) { + t.Fatalf("Unexpected Subnet: expected=%v actual=%v", expected, actual) + } + } + + { + allTasks := buildTasks() + checkNoChanges(t, cloud, allTasks) + } +} + +func TestSharedSubnetCreateDoesNotCreateNew(t *testing.T) { + cloud := awsup.BuildMockAWSCloud("us-east-1", "abc") + c := &mockec2.MockEC2{} + cloud.MockEC2 = c + + // Pre-create the vpc / subnet + vpc, err := c.CreateVpc(&ec2.CreateVpcInput{ + CidrBlock: aws.String("172.20.0.0/16"), + }) + if err != nil { + t.Fatalf("error creating test VPC: %v", err) + } + _, err = c.CreateTags(&ec2.CreateTagsInput{ + Resources: []*string{vpc.Vpc.VpcId}, + Tags: []*ec2.Tag{ + { + Key: aws.String("Name"), + Value: aws.String("ExistingVPC"), + }, + }, + }) + if err != nil { + t.Fatalf("error tagging test vpc: %v", err) + } + + subnet, err := c.CreateSubnet(&ec2.CreateSubnetInput{ + VpcId: vpc.Vpc.VpcId, + CidrBlock: aws.String("172.20.1.0/24"), + }) + if err != nil { + t.Fatalf("error creating test subnet: %v", err) + } + + _, err = c.CreateTags(&ec2.CreateTagsInput{ + Resources: []*string{subnet.Subnet.SubnetId}, + Tags: []*ec2.Tag{ + { + Key: aws.String("Name"), + Value: aws.String("ExistingSubnet"), + }, + }, + }) + if err != nil { + t.Fatalf("error tagging test subnet: %v", err) + } + + // We define a function so we can rebuild the tasks, because we modify in-place when running + buildTasks := func() map[string]fi.Task { + vpc1 := &VPC{ + Name: s("vpc1"), + CIDR: s("172.20.0.0/16"), + Tags: map[string]string{"kubernetes.io/cluster/cluster.example.com": "shared"}, + Shared: fi.Bool(true), + ID: vpc.Vpc.VpcId, + } + subnet1 := &Subnet{ + Name: s("subnet1"), + VPC: vpc1, + CIDR: s("172.20.1.0/24"), + Tags: map[string]string{"kubernetes.io/cluster/cluster.example.com": "shared"}, + Shared: fi.Bool(true), + ID: subnet.Subnet.SubnetId, + } + + return map[string]fi.Task{ + "subnet1": subnet1, + "vpc1": vpc1, + } + } + + { + allTasks := buildTasks() + subnet1 := allTasks["subnet1"].(*Subnet) + + target := &awsup.AWSAPITarget{ + Cloud: cloud, + } + + context, err := fi.NewContext(target, cloud, nil, nil, nil, true, allTasks) + if err != nil { + t.Fatalf("error building context: %v", err) + } + + if err := context.RunTasks(defaultDeadline); err != nil { + t.Fatalf("unexpected error during Run: %v", err) + } + + if fi.StringValue(subnet1.ID) == "" { + t.Fatalf("ID not set after create") + } + + if len(c.SubnetIds()) != 1 { + t.Fatalf("Expected exactly one Subnet; found %v", c.SubnetIds()) + } + + actual := c.FindSubnet(*subnet.Subnet.SubnetId) + if actual == nil { + t.Fatalf("Subnet created but then not found") + } + expected := &ec2.Subnet{ + CidrBlock: aws.String("172.20.1.0/24"), + SubnetId: aws.String("subnet-1"), + VpcId: aws.String("vpc-1"), + Tags: buildTags(map[string]string{ + "Name": "ExistingSubnet", + "kubernetes.io/cluster/cluster.example.com": "shared", + }), + } + if !reflect.DeepEqual(actual, expected) { + t.Fatalf("Unexpected Subnet: expected=%v actual=%v", expected, actual) + } + } + + { + allTasks := buildTasks() + checkNoChanges(t, cloud, allTasks) + } +} diff --git a/upup/pkg/fi/cloudup/awstasks/vpc.go b/upup/pkg/fi/cloudup/awstasks/vpc.go index bf17eb6b75f25..441b2faa50cca 100644 --- a/upup/pkg/fi/cloudup/awstasks/vpc.go +++ b/upup/pkg/fi/cloudup/awstasks/vpc.go @@ -107,6 +107,7 @@ func (e *VPC) Find(c *fi.Context) (*VPC, error) { e.ID = actual.ID } actual.Lifecycle = e.Lifecycle + actual.Name = e.Name // Name is part of Tags return actual, nil } @@ -143,11 +144,10 @@ func (_ *VPC) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *VPC) error { if featureflag.VPCSkipEnableDNSSupport.Enabled() { glog.Warningf("VPC did not have EnableDNSSupport=true, but ignoring because of VPCSkipEnableDNSSupport feature-flag") } else { + // TODO: We could easily just allow kops to fix this... return fmt.Errorf("VPC with id %q was set to be shared, but did not have EnableDNSSupport=true.", fi.StringValue(e.ID)) } } - - return nil } if a == nil { @@ -189,12 +189,7 @@ func (_ *VPC) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *VPC) error { } } - tags := e.Tags - if shared { - // Don't tag shared resources - tags = nil - } - return t.AddAWSTags(*e.ID, tags) + return t.AddAWSTags(*e.ID, e.Tags) } type terraformVPC struct { @@ -212,6 +207,7 @@ func (_ *VPC) RenderTerraform(t *terraform.TerraformTarget, a, e, changes *VPC) shared := fi.BoolValue(e.Shared) if shared { // Not terraform owned / managed + // We won't apply changes, but our validation (kops update) will still warn return nil } @@ -250,6 +246,7 @@ func (_ *VPC) RenderCloudformation(t *cloudformation.CloudformationTarget, a, e, shared := fi.BoolValue(e.Shared) if shared { // Not cloudformation owned / managed + // We won't apply changes, but our validation (kops update) will still warn return nil } From 9cf22aeeef444aa652f250fa8e33180ad99fcc48 Mon Sep 17 00:00:00 2001 From: Justin Santa Barbara Date: Mon, 23 Oct 2017 11:39:18 -0400 Subject: [PATCH 2/2] Sort Tags consistently to avoid test flakes --- cloudmock/aws/mockec2/tags.go | 16 +++++++++++++++- upup/pkg/fi/cloudup/awstasks/subnet_test.go | 4 ++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/cloudmock/aws/mockec2/tags.go b/cloudmock/aws/mockec2/tags.go index 6e0a464f55325..d914e19c2b8c2 100644 --- a/cloudmock/aws/mockec2/tags.go +++ b/cloudmock/aws/mockec2/tags.go @@ -18,10 +18,13 @@ package mockec2 import ( "fmt" + "sort" + "strings" + + "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/request" "github.com/aws/aws-sdk-go/service/ec2" "github.com/golang/glog" - "strings" ) func (m *MockEC2) CreateTagsRequest(*ec2.CreateTagsInput) (*request.Request, *ec2.CreateTagsOutput) { @@ -165,3 +168,14 @@ func (m *MockEC2) DescribeTagsPages(*ec2.DescribeTagsInput, func(*ec2.DescribeTa panic("Not implemented") return nil } + +// SortTags sorts the slice of tags by Key +func SortTags(tags []*ec2.Tag) { + keys := make([]string, len(tags)) + for i := range tags { + if tags[i] != nil { + keys[i] = aws.StringValue(tags[i].Key) + } + } + sort.SliceStable(tags, func(i, j int) bool { return keys[i] < keys[j] }) +} diff --git a/upup/pkg/fi/cloudup/awstasks/subnet_test.go b/upup/pkg/fi/cloudup/awstasks/subnet_test.go index aeaab57204869..afb5606f2fe7c 100644 --- a/upup/pkg/fi/cloudup/awstasks/subnet_test.go +++ b/upup/pkg/fi/cloudup/awstasks/subnet_test.go @@ -243,6 +243,10 @@ func TestSharedSubnetCreateDoesNotCreateNew(t *testing.T) { "kubernetes.io/cluster/cluster.example.com": "shared", }), } + + mockec2.SortTags(expected.Tags) + mockec2.SortTags(actual.Tags) + if !reflect.DeepEqual(actual, expected) { t.Fatalf("Unexpected Subnet: expected=%v actual=%v", expected, actual) }