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,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 := subnet.main
-		copy.Tags = m.getTags(ec2.ResourceTypeSubnet, *subnet.SubnetId)
+		copy.Tags = m.getTags(ec2.ResourceTypeSubnet, id)
 		subnets = append(subnets, &copy)
 	}
 

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 shared {
-		if name != "" {
+			// If the resource is shared, we don't try to set the Name - we presume that is managed externally
-			tags["Name"] = name
+			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
+// IsKubernetesGTE checks if the kubernetes version is at least version, ignoring prereleases / patches
-func VersionGTE(version semver.Version, major uint64, minor uint64) bool {
+func (c *KopsModelContext) IsKubernetesGTE(version string) bool {
-	if version.Major > major {
+	return util.IsKubernetesGTE(version, c.KubernetesVersion())
-		return true
-	}
-	if version.Major == major && version.Minor >= minor {
-		return true
-	}
-	return false
 }
 
 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
 	}
 

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

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
+	return t.AddAWSTags(*e.ID, e.Tags)
-	if shared {
-		// Don't tag shared resources
-		tags = nil
-	}
-	return t.AddAWSTags(*e.ID, 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
 	}