Correctly delete cloud labels that are no longer present in the expec…

…ted resources (cloudLabels, labels, taints).
kubernetes · Aug 16, 2017 · 8261767 · 8261767
1 parent 74d0e21
commit 8261767
Show file tree

Hide file tree

Showing 7 changed files with 309 additions and 12 deletions.
diff --git 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,24 +238,46 @@ 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?
 
 	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
@@ -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
@@ -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
@@ -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
@@ -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 {