diff --git a/upup/pkg/fi/cloudup/awstasks/autoscalinggroup.go b/upup/pkg/fi/cloudup/awstasks/autoscalinggroup.go index a816bc2f54380..30b09148fd834 100644 --- a/upup/pkg/fi/cloudup/awstasks/autoscalinggroup.go +++ b/upup/pkg/fi/cloudup/awstasks/autoscalinggroup.go @@ -222,10 +222,12 @@ func (_ *AutoscalingGroup) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *Autos changes.Subnets = nil } - var tagsRequest *autoscaling.CreateOrUpdateTagsInput + var updateTagsRequest *autoscaling.CreateOrUpdateTagsInput + var deleteTagsRequest *autoscaling.DeleteTagsInput if changes.Tags != nil { - tagsRequest = &autoscaling.CreateOrUpdateTagsInput{} - tagsRequest.Tags = tags + updateTagsRequest = &autoscaling.CreateOrUpdateTagsInput{Tags: tags} + deleteTagsRequest = &autoscaling.DeleteTagsInput{} + deleteTagsRequest.Tags = e.getASGTagsToDelete(a.Tags) changes.Tags = nil } @@ -236,17 +238,21 @@ func (_ *AutoscalingGroup) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *Autos glog.V(2).Infof("Updating autoscaling group %s", *e.Name) - _, err := t.Cloud.Autoscaling().UpdateAutoScalingGroup(request) - if err != nil { + if _, err := t.Cloud.Autoscaling().UpdateAutoScalingGroup(request); err != nil { return fmt.Errorf("error updating AutoscalingGroup: %v", err) } - if tagsRequest != nil { - _, err := t.Cloud.Autoscaling().CreateOrUpdateTags(tagsRequest) - if err != nil { + if updateTagsRequest != nil { + if _, err := t.Cloud.Autoscaling().CreateOrUpdateTags(updateTagsRequest); err != nil { return fmt.Errorf("error updating AutoscalingGroup tags: %v", err) } } + + if deleteTagsRequest != nil && len(deleteTagsRequest.Tags) > 0 { + if _, err := t.Cloud.Autoscaling().DeleteTags(deleteTagsRequest); err != nil { + return fmt.Errorf("error deleting old AutoscalingGroup tags: %v", err) + } + } } // TODO: Use PropagateAtLaunch = false for tagging? @@ -254,6 +260,24 @@ func (_ *AutoscalingGroup) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *Autos return nil // We have } +// getASGTagsToDelete loops through the currently set tags and builds a list of +// tags to be deleted from the Autoscaling Group +func (e *AutoscalingGroup) getASGTagsToDelete(currentTags map[string]string) []*autoscaling.Tag { + tagsToDelete := []*autoscaling.Tag{} + + for k, v := range currentTags { + if _, ok := e.Tags[k]; !ok { + tagsToDelete = append(tagsToDelete, &autoscaling.Tag{ + Key: aws.String(k), + Value: aws.String(v), + ResourceId: e.Name, + ResourceType: aws.String("auto-scaling-group"), + }) + } + } + return tagsToDelete +} + type terraformASGTag struct { Key *string `json:"key"` Value *string `json:"value"` diff --git a/upup/pkg/fi/cloudup/awstasks/autoscalinggroup_test.go b/upup/pkg/fi/cloudup/awstasks/autoscalinggroup_test.go new file mode 100644 index 0000000000000..9ac01b1bde1fb --- /dev/null +++ b/upup/pkg/fi/cloudup/awstasks/autoscalinggroup_test.go @@ -0,0 +1,129 @@ +/* +Copyright 2016 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package awstasks + +import ( + "sort" + "testing" + + "k8s.io/kops/pkg/diff" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/autoscaling" + "github.com/ghodss/yaml" +) + +func TestGetASGTagsToDelete(t *testing.T) { + asg := &AutoscalingGroup{ + Name: aws.String("MyASGName"), + Tags: map[string]string{ + "KubernetesCluster": "MyCluster", + "Name": "nodes.cluster.k8s.local", + }, + } + + cases := []struct { + CurrentTags map[string]string + ExpectedTagsToDelete []*autoscaling.Tag + }{ + { + CurrentTags: map[string]string{ + "KubernetesCluster": "MyCluster", + "Name": "nodes.cluster.k8s.local", + }, + ExpectedTagsToDelete: []*autoscaling.Tag{}, + }, + { + CurrentTags: map[string]string{ + "KubernetesCluster": "MyCluster", + "Name": "nodes.cluster.k8s.locall", + }, + ExpectedTagsToDelete: []*autoscaling.Tag{}, + }, + { + CurrentTags: map[string]string{ + "KubernetesCluster": "MyCluster", + }, + ExpectedTagsToDelete: []*autoscaling.Tag{}, + }, + { + CurrentTags: map[string]string{ + "KubernetesCluster": "MyCluster", + "Name": "nodes.cluster.k8s.local", + "OldTag": "OldValue", + }, + ExpectedTagsToDelete: []*autoscaling.Tag{ + { + Key: aws.String("OldTag"), + Value: aws.String("OldValue"), + ResourceId: asg.Name, + ResourceType: aws.String("auto-scaling-group"), + }, + }, + }, + { + CurrentTags: map[string]string{ + "KubernetesCluster": "MyCluster", + "Name": "nodes.cluster.k8s.local", + "MyCustomTag": "MyCustomValue", + "k8s.io/cluster-autoscaler/node-template/taint/sometaint": "somevalue:NoSchedule", + }, + ExpectedTagsToDelete: []*autoscaling.Tag{ + { + Key: aws.String("MyCustomTag"), + Value: aws.String("MyCustomValue"), + ResourceId: asg.Name, + ResourceType: aws.String("auto-scaling-group"), + }, + { + Key: aws.String("k8s.io/cluster-autoscaler/node-template/taint/sometaint"), + Value: aws.String("somevalue:NoSchedule"), + ResourceId: asg.Name, + ResourceType: aws.String("auto-scaling-group"), + }, + }, + }, + } + + for i, x := range cases { + tagsToDelete := asg.getASGTagsToDelete(x.CurrentTags) + + // Sort both lists to ensure comparisons don't show a false negative + sort.Slice(tagsToDelete, func(i, j int) bool { + return *tagsToDelete[i].Key < *tagsToDelete[j].Key + }) + sort.Slice(x.ExpectedTagsToDelete, func(i, j int) bool { + return *x.ExpectedTagsToDelete[i].Key < *x.ExpectedTagsToDelete[j].Key + }) + + expected, err := yaml.Marshal(x.ExpectedTagsToDelete) + if err != nil { + t.Errorf("case %d, unexpected error converting expected tags to yaml: %v", i, err) + } + + actual, err := yaml.Marshal(tagsToDelete) + if err != nil { + t.Errorf("case %d, unexpected error converting actual tags to yaml: %v", i, err) + } + + if string(expected) != string(actual) { + diffString := diff.FormatDiff(string(expected), string(actual)) + t.Errorf("case %d failed, actual output differed from expected.", i) + t.Logf("diff:\n%s\n", diffString) + } + } +} diff --git a/upup/pkg/fi/cloudup/awstasks/ebsvolume.go b/upup/pkg/fi/cloudup/awstasks/ebsvolume.go index 470005fdbfa86..2eb310e7f2516 100644 --- a/upup/pkg/fi/cloudup/awstasks/ebsvolume.go +++ b/upup/pkg/fi/cloudup/awstasks/ebsvolume.go @@ -149,7 +149,29 @@ func (_ *EBSVolume) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *EBSVolume) e e.ID = response.VolumeId } - return t.AddAWSTags(*e.ID, e.Tags) + if err := t.AddAWSTags(*e.ID, e.Tags); err != nil { + return fmt.Errorf("error adding AWS Tags to EBS Volume: %v", err) + } + + tagsToDelete := e.getEBSVolumeTagsToDelete(a.Tags) + if len(tagsToDelete) > 0 { + return t.DeleteTags(*e.ID, tagsToDelete) + } + + return nil +} + +// getEBSVolumeTagsToDelete loops through the currently set tags and builds +// a list of tags to be deleted from the EBS Volume +func (e *EBSVolume) getEBSVolumeTagsToDelete(currentTags map[string]string) map[string]string { + tagsToDelete := map[string]string{} + for k, v := range currentTags { + if _, ok := e.Tags[k]; !ok { + tagsToDelete[k] = v + } + } + + return tagsToDelete } type terraformVolume struct { diff --git a/upup/pkg/fi/cloudup/awstasks/ebsvolume_test.go b/upup/pkg/fi/cloudup/awstasks/ebsvolume_test.go new file mode 100644 index 0000000000000..24bb4a247b2da --- /dev/null +++ b/upup/pkg/fi/cloudup/awstasks/ebsvolume_test.go @@ -0,0 +1,104 @@ +/* +Copyright 2016 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package awstasks + +import ( + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/ghodss/yaml" + + "k8s.io/kops/pkg/diff" +) + +func TestGetEBSVolumeTagsToDelete(t *testing.T) { + ebsv := &EBSVolume{ + ID: aws.String("ebs-1234567"), + Tags: map[string]string{ + "KubernetesCluster": "MyCluster", + "Name": "nodes.cluster.k8s.local", + }, + } + + cases := []struct { + CurrentTags map[string]string + ExpectedTagsToDelete map[string]string + }{ + { + CurrentTags: map[string]string{ + "KubernetesCluster": "MyCluster", + "Name": "nodes.cluster.k8s.local", + }, + ExpectedTagsToDelete: map[string]string{}, + }, + { + CurrentTags: map[string]string{ + "KubernetesCluster": "MyCluster", + "Name": "nodes.cluster.k8s.locall", + }, + ExpectedTagsToDelete: map[string]string{}, + }, + { + CurrentTags: map[string]string{ + "KubernetesCluster": "MyCluster", + }, + ExpectedTagsToDelete: map[string]string{}, + }, + { + CurrentTags: map[string]string{ + "KubernetesCluster": "MyCluster", + "Name": "nodes.cluster.k8s.local", + "OldTag": "OldValue", + }, + ExpectedTagsToDelete: map[string]string{ + "OldTag": "OldValue", + }, + }, + { + CurrentTags: map[string]string{ + "KubernetesCluster": "MyCluster", + "Name": "nodes.cluster.k8s.local", + "MyCustomTag": "MyCustomValue", + "k8s.io/cluster-autoscaler/node-template/taint/sometaint": "somevalue:NoSchedule", + }, + ExpectedTagsToDelete: map[string]string{ + "MyCustomTag": "MyCustomValue", + "k8s.io/cluster-autoscaler/node-template/taint/sometaint": "somevalue:NoSchedule", + }, + }, + } + + for i, x := range cases { + tagsToDelete := ebsv.getEBSVolumeTagsToDelete(x.CurrentTags) + + expected, err := yaml.Marshal(x.ExpectedTagsToDelete) + if err != nil { + t.Errorf("case %d, unexpected error converting expected tags to yaml: %v", i, err) + } + + actual, err := yaml.Marshal(tagsToDelete) + if err != nil { + t.Errorf("case %d, unexpected error converting actual tags to yaml: %v", i, err) + } + + if string(expected) != string(actual) { + diffString := diff.FormatDiff(string(expected), string(actual)) + t.Errorf("case %d failed, actual output differed from expected.", i) + t.Logf("diff:\n%s\n", diffString) + } + } +} diff --git a/upup/pkg/fi/cloudup/awsup/aws_apitarget.go b/upup/pkg/fi/cloudup/awsup/aws_apitarget.go index fcd5a7de3582c..4f3af36bb226a 100644 --- a/upup/pkg/fi/cloudup/awsup/aws_apitarget.go +++ b/upup/pkg/fi/cloudup/awsup/aws_apitarget.go @@ -18,11 +18,12 @@ package awsup import ( "fmt" + "time" "github.com/aws/aws-sdk-go/aws" "github.com/golang/glog" + "k8s.io/kops/upup/pkg/fi" - "time" ) type AWSAPITarget struct { @@ -49,6 +50,10 @@ func (t *AWSAPITarget) AddAWSTags(id string, expected map[string]string) error { return t.Cloud.AddAWSTags(id, expected) } +func (t *AWSAPITarget) DeleteTags(id string, tags map[string]string) error { + return t.Cloud.DeleteTags(id, tags) +} + func (t *AWSAPITarget) AddELBTags(loadBalancerName string, expected map[string]string) error { actual, err := t.Cloud.GetELBTags(loadBalancerName) if err != nil { diff --git a/upup/pkg/fi/cloudup/awsup/aws_cloud.go b/upup/pkg/fi/cloudup/awsup/aws_cloud.go index 96a222efcdbd5..ef7618e7b8146 100644 --- a/upup/pkg/fi/cloudup/awsup/aws_cloud.go +++ b/upup/pkg/fi/cloudup/awsup/aws_cloud.go @@ -18,6 +18,9 @@ package awsup import ( "fmt" + "strings" + "time" + "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/aws/session" @@ -31,13 +34,12 @@ import ( "github.com/aws/aws-sdk-go/service/route53" "github.com/aws/aws-sdk-go/service/route53/route53iface" "github.com/golang/glog" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/kops/pkg/apis/kops" "k8s.io/kops/upup/pkg/fi" "k8s.io/kubernetes/federation/pkg/dnsprovider" dnsproviderroute53 "k8s.io/kubernetes/federation/pkg/dnsprovider/providers/aws/route53" - "strings" - "time" ) // By default, aws-sdk-go only retries 3 times, which doesn't give @@ -100,6 +102,9 @@ type AWSCloud interface { // CreateELBTags will add tags to the specified loadBalancer, retrying up to MaxCreateTagsAttempts times if it hits an eventual-consistency type error CreateELBTags(loadBalancerName string, tags map[string]string) error + // DeleteTags will delete tags from the specified resource, retrying up to MaxCreateTagsAttempts times if it hits an eventual-consistency type error + DeleteTags(id string, tags map[string]string) error + // DescribeInstance is a helper that queries for the specified instance by id DescribeInstance(instanceID string) (*ec2.Instance, error) @@ -343,6 +348,10 @@ func createTags(c AWSCloud, resourceId string, tags map[string]string) error { // DeleteTags will remove tags from the specified resource, retrying up to MaxCreateTagsAttempts times if it hits an eventual-consistency type error func (c *awsCloudImplementation) DeleteTags(resourceId string, tags map[string]string) error { + return deleteTags(c, resourceId, tags) +} + +func deleteTags(c AWSCloud, resourceId string, tags map[string]string) error { if len(tags) == 0 { return nil } diff --git a/upup/pkg/fi/cloudup/awsup/mock_aws_cloud.go b/upup/pkg/fi/cloudup/awsup/mock_aws_cloud.go index ff3957a5748a6..fc047e60ec4b6 100644 --- a/upup/pkg/fi/cloudup/awsup/mock_aws_cloud.go +++ b/upup/pkg/fi/cloudup/awsup/mock_aws_cloud.go @@ -109,6 +109,10 @@ func (c *MockAWSCloud) AddAWSTags(id string, expected map[string]string) error { return addAWSTags(c, id, expected) } +func (c *MockAWSCloud) DeleteTags(id string, tags map[string]string) error { + return deleteTags(c, id, tags) +} + func (c *MockAWSCloud) BuildTags(name *string) map[string]string { return buildTags(c.tags, name) }