diff --git a/cloudmock/aws/mockec2/api.go b/cloudmock/aws/mockec2/api.go index 8c21835ccd..9758564cc7 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 735541e774..2fdcfc46fc 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 ed64a59ef0..ecc3be3cef 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 3576017c2a..4076c492d6 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 d60212db0f..28f9d7c57f 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 3b853a4920..aeaab57204 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 bf17eb6b75..441b2faa50 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 }