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

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

pkg/model/context.go

@@ -214,9 +214,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.KubernetesGTE(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 {
@@ -283,24 +301,24 @@ 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
+// VersionGTE is a simplified semver comparison; ignoring prereleases / patches
 func VersionGTE(version semver.Version, major uint64, minor uint64) bool {
 	if version.Major > major {
 		return true
@@ -311,6 +329,13 @@ func VersionGTE(version semver.Version, major uint64, minor uint64) bool {
 	return false
 }
 
+// KubernetesGTE checks if the kubernetes version is at least major.minor, according to the VersionGTE check
+// i.e. we ignore prereleases / patches
+func (c *KopsModelContext) KubernetesGTE(major, minor uint64) bool {
+	kubernetesVersion := c.KubernetesVersion()
+	return VersionGTE(kubernetesVersion, major, minor)
+}
+
 func (c *KopsModelContext) WellKnownServiceIP(id int) (net.IP, error) {
 	return components.WellKnownServiceIP(&c.Cluster.Spec, id)
 }

pkg/model/network.go

@@ -35,11 +35,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()
 
@@ -55,20 +50,21 @@ func (b *NetworkModelBuilder) Build(c *fi.ModelBuilderContext) error {
 			Tags:             tags,
 		}
 
-		if sharedVPC && VersionGTE(kubernetesVersion, 1, 5) {
+		if sharedVPC && b.KubernetesGTE(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
 
 			t.EnableDNSHostnames = fi.Bool(true)
 		}
 
-		if b.Cluster.Spec.NetworkID != "" {
+		if sharedVPC {
 			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,7 @@ 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
 		return t.AddOutputVariableArray("subnet_ids", terraform.LiteralFromStringValue(*e.ID))
 	}
 
@@ -248,6 +248,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
 	}