AWS: Delete old LaunchConfigurations

We delete old AWS LaunchConfigurations when we see that we have more
than 3.  We add a feature flag KeepLaunchConfigurations to disable this
functionality, for backwards compatability.

Fixes kubernetes#329

cloudmock/aws/mockautoscaling/launchconfigurations.go

@@ -95,6 +95,9 @@ func (m *MockAutoscaling) CreateLaunchConfiguration(request *autoscaling.CreateL
 	if m.LaunchConfigurations == nil {
 		m.LaunchConfigurations = make(map[string]*autoscaling.LaunchConfiguration)
 	}
+	if m.LaunchConfigurations[*lc.LaunchConfigurationName] != nil {
+		return nil, fmt.Errorf("duplicate LaunchConfigurationName %s", *lc.LaunchConfigurationName)
+	}
 	m.LaunchConfigurations[*lc.LaunchConfigurationName] = lc
 
 	return &autoscaling.CreateLaunchConfigurationOutput{}, nil

pkg/featureflag/featureflag.go

@@ -36,6 +36,9 @@ func Bool(b bool) *bool {
 	return &b
 }
 
+// KeepLaunchConfigurations can be set to prevent garbage collection of old launch configurations
+var KeepLaunchConfigurations = New("KeepLaunchConfigurations", Bool(false))
+
 // DNSPreCreate controls whether we pre-create DNS records.
 var DNSPreCreate = New("DNSPreCreate", Bool(true))
 

upup/pkg/fi/cloudup/awstasks/BUILD.bazel

@@ -96,12 +96,14 @@ go_test(
         "ebsvolume_test.go",
         "elastic_ip_test.go",
         "internetgateway_test.go",
+        "launchconfiguration_test.go",
         "securitygroup_test.go",
         "subnet_test.go",
         "vpc_test.go",
     ],
     embed = [":go_default_library"],
     deps = [
+        "//cloudmock/aws/mockautoscaling:go_default_library",
         "//cloudmock/aws/mockec2:go_default_library",
         "//pkg/apis/kops:go_default_library",
         "//pkg/assets:go_default_library",

upup/pkg/fi/cloudup/awstasks/launchconfiguration.go

@@ -19,6 +19,7 @@ package awstasks
 import (
 	"encoding/base64"
 	"fmt"
+	"math"
 	"sort"
 	"strings"
 	"time"
@@ -27,12 +28,27 @@ import (
 	"github.com/aws/aws-sdk-go/service/autoscaling"
 	"github.com/golang/glog"
 	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/kops/pkg/featureflag"
 	"k8s.io/kops/upup/pkg/fi"
 	"k8s.io/kops/upup/pkg/fi/cloudup/awsup"
 	"k8s.io/kops/upup/pkg/fi/cloudup/cloudformation"
 	"k8s.io/kops/upup/pkg/fi/cloudup/terraform"
 )
 
+// defaultRetainLaunchConfigurationCount is the number of launch configurations (matching the name prefix) that we should
+// keep, we delete older ones
+var defaultRetainLaunchConfigurationCount = 3
+
+// TODO: Release note , including featureflag
+
+// RetainLaunchConfigurationCount returns the number of launch configurations to keep
+func RetainLaunchConfigurationCount() int {
+	if featureflag.KeepLaunchConfigurations.Enabled() {
+		return math.MaxInt32
+	}
+	return defaultRetainLaunchConfigurationCount
+}
+
 //go:generate fitask -type=LaunchConfiguration
 type LaunchConfiguration struct {
 	Name      *string
@@ -68,24 +84,26 @@ type LaunchConfiguration struct {
 
 var _ fi.CompareWithID = &LaunchConfiguration{}
 
+var _ fi.ProducesDeletions = &LaunchConfiguration{}
+
 func (e *LaunchConfiguration) CompareWithID() *string {
 	return e.ID
 }
 
-func (e *LaunchConfiguration) Find(c *fi.Context) (*LaunchConfiguration, error) {
+// findLaunchConfigurations returns matching LaunchConfigurations, sorted by CreatedTime (ascending)
+func (e *LaunchConfiguration) findLaunchConfigurations(c *fi.Context) ([]*autoscaling.LaunchConfiguration, error) {
 	cloud := c.Cloud.(awsup.AWSCloud)
 
 	request := &autoscaling.DescribeLaunchConfigurationsInput{}
 
 	prefix := *e.Name + "-"
 
-	configurations := map[string]*autoscaling.LaunchConfiguration{}
+	var configurations []*autoscaling.LaunchConfiguration
 	err := cloud.Autoscaling().DescribeLaunchConfigurationsPages(request, func(page *autoscaling.DescribeLaunchConfigurationsOutput, lastPage bool) bool {
 		for _, l := range page.LaunchConfigurations {
 			name := aws.StringValue(l.LaunchConfigurationName)
 			if strings.HasPrefix(name, prefix) {
-				suffix := name[len(prefix):]
-				configurations[suffix] = l
+				configurations = append(configurations, l)
 			}
 		}
 		return true
@@ -94,21 +112,36 @@ func (e *LaunchConfiguration) Find(c *fi.Context) (*LaunchConfiguration, error)
 		return nil, fmt.Errorf("error listing AutoscalingLaunchConfigurations: %v", err)
 	}
 
-	if len(configurations) == 0 {
-		return nil, nil
+	sort.Slice(configurations, func(i, j int) bool {
+		ti := configurations[i].CreatedTime
+		tj := configurations[j].CreatedTime
+		if tj == nil {
+			return true
+		}
+		if ti == nil {
+			return false
+		}
+		return ti.UnixNano() < tj.UnixNano()
+	})
+
+	return configurations, nil
+}
+
+func (e *LaunchConfiguration) Find(c *fi.Context) (*LaunchConfiguration, error) {
+	cloud := c.Cloud.(awsup.AWSCloud)
+
+	configurations, err := e.findLaunchConfigurations(c)
+	if err != nil {
+		return nil, err
 	}
 
-	var newest *autoscaling.LaunchConfiguration
-	var newestTime int64
-	for _, lc := range configurations {
-		t := lc.CreatedTime.UnixNano()
-		if t > newestTime {
-			newestTime = t
-			newest = lc
-		}
+	if len(configurations) == 0 {
+		return nil, nil
 	}
 
-	lc := newest
+	// We pick up the latest launch configuration
+	// (TODO: this might not actually be attached to the AutoScalingGroup, if something went wrong previously)
+	lc := configurations[len(configurations)-1]
 
 	glog.V(2).Infof("found existing AutoscalingLaunchConfiguration: %q", *lc.LaunchConfigurationName)
 
@@ -117,15 +150,21 @@ func (e *LaunchConfiguration) Find(c *fi.Context) (*LaunchConfiguration, error)
 		ID:                     lc.LaunchConfigurationName,
 		ImageID:                lc.ImageId,
 		InstanceType:           lc.InstanceType,
-		SSHKey:                 &SSHKey{Name: lc.KeyName},
 		AssociatePublicIP:      lc.AssociatePublicIpAddress,
-		IAMInstanceProfile:     &IAMInstanceProfile{Name: lc.IamInstanceProfile},
 		InstanceMonitoring:     lc.InstanceMonitoring.Enabled,
 		SpotPrice:              aws.StringValue(lc.SpotPrice),
 		Tenancy:                lc.PlacementTenancy,
 		RootVolumeOptimization: lc.EbsOptimized,
 	}
 
+	if lc.KeyName != nil {
+		actual.SSHKey = &SSHKey{Name: lc.KeyName}
+	}
+
+	if lc.IamInstanceProfile != nil {
+		actual.IAMInstanceProfile = &IAMInstanceProfile{Name: lc.IamInstanceProfile}
+	}
+
 	securityGroups := []*SecurityGroup{}
 	for _, sgID := range lc.SecurityGroups {
 		securityGroups = append(securityGroups, &SecurityGroup{ID: sgID})
@@ -145,11 +184,13 @@ func (e *LaunchConfiguration) Find(c *fi.Context) (*LaunchConfiguration, error)
 		actual.RootVolumeIops = b.Ebs.Iops
 	}
 
-	userData, err := base64.StdEncoding.DecodeString(aws.StringValue(lc.UserData))
-	if err != nil {
-		return nil, fmt.Errorf("error decoding UserData: %v", err)
+	if lc.UserData != nil {
+		userData, err := base64.StdEncoding.DecodeString(aws.StringValue(lc.UserData))
+		if err != nil {
+			return nil, fmt.Errorf("error decoding UserData: %v", err)
+		}
+		actual.UserData = fi.WrapResource(fi.NewStringResource(string(userData)))
 	}
-	actual.UserData = fi.WrapResource(fi.NewStringResource(string(userData)))
 
 	// Avoid spurious changes on ImageId
 	if e.ImageID != nil && actual.ImageID != nil && *actual.ImageID != *e.ImageID {
@@ -617,3 +658,68 @@ func (_ *LaunchConfiguration) RenderCloudformation(t *cloudformation.Cloudformat
 func (e *LaunchConfiguration) CloudformationLink() *cloudformation.Literal {
 	return cloudformation.Ref("AWS::AutoScaling::LaunchConfiguration", *e.Name)
 }
+
+// deleteLaunchConfiguration tracks a LaunchConfiguration that we're going to delete
+// It implements fi.Deletion
+type deleteLaunchConfiguration struct {
+	lc *autoscaling.LaunchConfiguration
+}
+
+var _ fi.Deletion = &deleteLaunchConfiguration{}
+
+func (d *deleteLaunchConfiguration) TaskName() string {
+	return "LaunchConfiguration"
+}
+
+func (d *deleteLaunchConfiguration) Item() string {
+	return aws.StringValue(d.lc.LaunchConfigurationName)
+}
+
+func (d *deleteLaunchConfiguration) Delete(t fi.Target) error {
+	glog.V(2).Infof("deleting launch configuration %v", d)
+
+	awsTarget, ok := t.(*awsup.AWSAPITarget)
+	if !ok {
+		return fmt.Errorf("unexpected target type for deletion: %T", t)
+	}
+
+	request := &autoscaling.DeleteLaunchConfigurationInput{
+		LaunchConfigurationName: d.lc.LaunchConfigurationName,
+	}
+
+	name := aws.StringValue(request.LaunchConfigurationName)
+	glog.V(2).Infof("Calling autoscaling DeleteLaunchConfiguration for %s", name)
+	_, err := awsTarget.Cloud.Autoscaling().DeleteLaunchConfiguration(request)
+	if err != nil {
+		return fmt.Errorf("error deleting autoscaling LaunchConfiguration %s: %v", name, err)
+	}
+
+	return nil
+}
+
+func (d *deleteLaunchConfiguration) String() string {
+	return d.TaskName() + "-" + d.Item()
+}
+
+func (e *LaunchConfiguration) FindDeletions(c *fi.Context) ([]fi.Deletion, error) {
+	var removals []fi.Deletion
+
+	configurations, err := e.findLaunchConfigurations(c)
+	if err != nil {
+		return nil, err
+	}
+
+	if len(configurations) <= RetainLaunchConfigurationCount() {
+		return nil, nil
+	}
+
+	configurations = configurations[:len(configurations)-RetainLaunchConfigurationCount()]
+
+	for _, configuration := range configurations {
+		removals = append(removals, &deleteLaunchConfiguration{lc: configuration})
+	}
+
+	glog.V(2).Infof("will delete launch configurations: %v", removals)
+
+	return removals, nil
+}

upup/pkg/fi/cloudup/awstasks/launchconfiguration_test.go

@@ -0,0 +1,112 @@
+/*
+Copyright 2018 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 (
+	"strconv"
+	"testing"
+
+	"time"
+
+	"github.com/aws/aws-sdk-go/aws"
+	"github.com/aws/aws-sdk-go/service/ec2"
+	"k8s.io/kops/cloudmock/aws/mockautoscaling"
+	"k8s.io/kops/cloudmock/aws/mockec2"
+	"k8s.io/kops/upup/pkg/fi"
+	"k8s.io/kops/upup/pkg/fi/cloudup/awsup"
+)
+
+func TestLaunchConfigurationGarbageCollection(t *testing.T) {
+	cloud := awsup.BuildMockAWSCloud("us-east-1", "abc")
+	mockEC2 := &mockec2.MockEC2{}
+	cloud.MockEC2 = mockEC2
+	as := &mockautoscaling.MockAutoscaling{}
+	cloud.MockAutoscaling = as
+
+	mockEC2.Images = append(mockEC2.Images, &ec2.Image{
+		CreationDate:   aws.String("2016-10-21T20:07:19.000Z"),
+		ImageId:        aws.String("ami-12345678"),
+		Name:           aws.String("k8s-1.4-debian-jessie-amd64-hvm-ebs-2016-10-21"),
+		OwnerId:        aws.String(awsup.WellKnownAccountKopeio),
+		RootDeviceName: aws.String("/dev/xvda"),
+	})
+
+	// We define a function so we can rebuild the tasks, because we modify in-place when running
+	buildTasks := func(spotPrice string) map[string]fi.Task {
+		lc := &LaunchConfiguration{
+			Name:           s("lc1"),
+			SpotPrice:      spotPrice,
+			ImageID:        s("ami-12345678"),
+			InstanceType:   s("m3.medium"),
+			SecurityGroups: []*SecurityGroup{},
+		}
+
+		return map[string]fi.Task{
+			"lc1": lc,
+		}
+	}
+
+	// We change the launch configuration 5 times, verifying that new launch configurations are created,
+	// and that older ones are eventually GCed
+	for i := 0; i < 5; i++ {
+		spotPrice := strconv.Itoa(i + 1)
+		{
+			allTasks := buildTasks(spotPrice)
+			lc1 := allTasks["lc1"].(*LaunchConfiguration)
+
+			target := &awsup.AWSAPITarget{
+				Cloud: cloud,
+			}
+
+			context, err := fi.NewContext(target, nil, cloud, nil, nil, nil, true, allTasks)
+			if err != nil {
+				t.Fatalf("error building context: %v", err)
+			}
+
+			time.Sleep(time.Second)
+			// TODO: Remove sleep, find out why we don't retry
+
+			if err := context.RunTasks(defaultDeadline); err != nil {
+				t.Fatalf("unexpected error during Run: %v", err)
+			}
+
+			if fi.StringValue(lc1.ID) == "" {
+				t.Fatalf("ID not set after create")
+			}
+
+			expectedCount := i + 1
+			if expectedCount > RetainLaunchConfigurationCount() {
+				expectedCount = RetainLaunchConfigurationCount()
+			}
+			if len(as.LaunchConfigurations) != expectedCount {
+				t.Fatalf("Expected exactly %d LaunchConfigurations; found %v", expectedCount, as.LaunchConfigurations)
+			}
+
+			// TODO: verify that we retained the N latest
+
+			actual := as.LaunchConfigurations[*lc1.ID]
+			if aws.StringValue(actual.SpotPrice) != spotPrice {
+				t.Fatalf("Unexpected spotPrice: expected=%v actual=%v", spotPrice, aws.StringValue(actual.SpotPrice))
+			}
+		}
+
+		{
+			allTasks := buildTasks(spotPrice)
+			checkNoChanges(t, cloud, allTasks)
+		}
+	}
+}