Merge pull request #3184 from justinsb/test_does_not_change_tags

Automatic merge from submit-queue.

Fix shared subnet/vpc tags

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

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 &copy
+}
+
+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,15 +87,15 @@ 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
 			switch *filter.Name {
 
 			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, &copy)
 	}
 

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] })
+}

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) {

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)
 		}

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
 	}