Merge pull request #16356 from justinsb/revisions_and_pruning

Generate revisions of NLB objects, and introduce cleanup phase

cloudmock/aws/mockautoscaling/attach.go

@@ -17,6 +17,7 @@ limitations under the License.
 package mockautoscaling
 
 import (
+	"context"
 	"fmt"
 
 	"github.com/aws/aws-sdk-go/aws"
@@ -52,7 +53,7 @@ func (m *MockAutoscaling) AttachLoadBalancersRequest(*autoscaling.AttachLoadBala
 	return nil, nil
 }
 
-func (m *MockAutoscaling) AttachLoadBalancerTargetGroups(request *autoscaling.AttachLoadBalancerTargetGroupsInput) (*autoscaling.AttachLoadBalancerTargetGroupsOutput, error) {
+func (m *MockAutoscaling) AttachLoadBalancerTargetGroupsWithContext(ctx aws.Context, request *autoscaling.AttachLoadBalancerTargetGroupsInput, opts ...request.Option) (*autoscaling.AttachLoadBalancerTargetGroupsOutput, error) {
 	m.mutex.Lock()
 	defer m.mutex.Unlock()
 
@@ -62,9 +63,13 @@ func (m *MockAutoscaling) AttachLoadBalancerTargetGroups(request *autoscaling.At
 
 	asg := m.Groups[name]
 	if asg == nil {
-		return nil, fmt.Errorf("Group %q not found", name)
+		return nil, fmt.Errorf("group %q not found", name)
 	}
 
-	asg.TargetGroupARNs = request.TargetGroupARNs
+	asg.TargetGroupARNs = append(asg.TargetGroupARNs, request.TargetGroupARNs...)
 	return &autoscaling.AttachLoadBalancerTargetGroupsOutput{}, nil
 }
+
+func (m *MockAutoscaling) AttachLoadBalancerTargetGroups(request *autoscaling.AttachLoadBalancerTargetGroupsInput) (*autoscaling.AttachLoadBalancerTargetGroupsOutput, error) {
+	return m.AttachLoadBalancerTargetGroupsWithContext(context.TODO(), request)
+}

cmd/kops/update_cluster.go

@@ -80,6 +80,12 @@ type UpdateClusterOptions struct {
 	// LifecycleOverrides is a slice of taskName=lifecycle name values.  This slice is used
 	// to populate the LifecycleOverrides struct member in ApplyClusterCmd struct.
 	LifecycleOverrides []string
+
+	// Prune is true if we should clean up any old revisions of objects.
+	// Typically this is done in after we have rolling-updated the cluster.
+	// The goal is that the cluster can keep running even during more disruptive
+	// infrastructure changes.
+	Prune bool
 }
 
 func (o *UpdateClusterOptions) InitDefaults() {
@@ -91,6 +97,8 @@ func (o *UpdateClusterOptions) InitDefaults() {
 	// By default we export a kubecfg, but it doesn't have a static/eternal credential in it any more.
 	o.CreateKubecfg = true
 
+	o.Prune = false
+
 	o.RunTasksOptions.InitDefaults()
 }
 
@@ -133,6 +141,8 @@ func NewCmdUpdateCluster(f *util.Factory, out io.Writer) *cobra.Command {
 	viper.BindEnv("lifecycle-overrides", "KOPS_LIFECYCLE_OVERRIDES")
 	cmd.RegisterFlagCompletionFunc("lifecycle-overrides", completeLifecycleOverrides)
 
+	cmd.Flags().BoolVar(&options.Prune, "prune", options.Prune, "Delete old revisions of cloud resources that were needed during an upgrade")
+
 	return cmd
 }
 
@@ -251,6 +261,11 @@ func RunUpdateCluster(ctx context.Context, f *util.Factory, out io.Writer, c *Up
 		}
 	}
 
+	deletionProcessing := fi.DeletionProcessingModeDeleteIfNotDeferrred
+	if c.Prune {
+		deletionProcessing = fi.DeletionProcessingModeDeleteIncludingDeferred
+	}
+
 	lifecycleOverrideMap := make(map[string]fi.Lifecycle)
 
 	for _, override := range c.LifecycleOverrides {
@@ -287,6 +302,7 @@ func RunUpdateCluster(ctx context.Context, f *util.Factory, out io.Writer, c *Up
 		TargetName:         targetName,
 		LifecycleOverrides: lifecycleOverrideMap,
 		GetAssets:          c.GetAssets,
+		DeletionProcessing: deletionProcessing,
 	}
 
 	if err := applyCmd.Run(ctx); err != nil {

pkg/instancegroups/instancegroups.go

@@ -492,6 +492,8 @@ func (c *RollingUpdateCluster) reconcileInstanceGroup() error {
 		Phase:              "",
 		TargetName:         "direct",
 		LifecycleOverrides: map[string]fi.Lifecycle{},
+
+		DeletionProcessing: fi.DeletionProcessingModeDeleteIfNotDeferrred,
 	}
 
 	return applyCmd.Run(c.Ctx)

pkg/model/awsmodel/api_loadbalancer.go

@@ -124,8 +124,6 @@ func (b *APILoadBalancerBuilder) Build(c *fi.CloudupModelBuilderContext) error {
 	var clb *awstasks.ClassicLoadBalancer
 	var nlb *awstasks.NetworkLoadBalancer
 	{
-		loadBalancerName := b.LBName32("api")
-
 		idleTimeout := LoadBalancerDefaultIdleTimeout
 		if lbSpec.IdleTimeoutSeconds != nil {
 			idleTimeout = time.Second * time.Duration(*lbSpec.IdleTimeoutSeconds)
@@ -196,13 +194,12 @@ func (b *APILoadBalancerBuilder) Build(c *fi.CloudupModelBuilderContext) error {
 			Name:      fi.PtrTo(b.NLBName("api")),
 			Lifecycle: b.Lifecycle,
 
-			LoadBalancerName: fi.PtrTo(loadBalancerName),
-			CLBName:          fi.PtrTo("api." + b.ClusterName()),
+			LoadBalancerBaseName: fi.PtrTo(b.LBName32("api")),
+			CLBName:              fi.PtrTo("api." + b.ClusterName()),
 			SecurityGroups: []*awstasks.SecurityGroup{
 				b.LinkToELBSecurityGroup("api"),
 			},
-			SubnetMappings: nlbSubnetMappings,
-
+			SubnetMappings:    nlbSubnetMappings,
 			Tags:              tags,
 			WellKnownServices: []wellknownservices.WellKnownService{wellknownservices.KubeAPIServer},
 			VPC:               b.LinkToVPC(),
@@ -219,7 +216,7 @@ func (b *APILoadBalancerBuilder) Build(c *fi.CloudupModelBuilderContext) error {
 			Name:      fi.PtrTo("api." + b.ClusterName()),
 			Lifecycle: b.Lifecycle,
 
-			LoadBalancerName: fi.PtrTo(loadBalancerName),
+			LoadBalancerName: fi.PtrTo(b.LBName32("api")),
 			SecurityGroups: []*awstasks.SecurityGroup{
 				b.LinkToELBSecurityGroup("api"),
 			},
@@ -320,7 +317,7 @@ func (b *APILoadBalancerBuilder) Build(c *fi.CloudupModelBuilderContext) error {
 					UnhealthyThreshold: fi.PtrTo(int64(2)),
 					Shared:             fi.PtrTo(false),
 				}
-
+				tg.CreateNewRevisionsWith(nlb)
 				c.AddTask(tg)
 			}
 
@@ -344,6 +341,7 @@ func (b *APILoadBalancerBuilder) Build(c *fi.CloudupModelBuilderContext) error {
 					UnhealthyThreshold: fi.PtrTo(int64(2)),
 					Shared:             fi.PtrTo(false),
 				}
+				tg.CreateNewRevisionsWith(nlb)
 
 				c.AddTask(tg)
 			}
@@ -367,6 +365,7 @@ func (b *APILoadBalancerBuilder) Build(c *fi.CloudupModelBuilderContext) error {
 					UnhealthyThreshold: fi.PtrTo(int64(2)),
 					Shared:             fi.PtrTo(false),
 				}
+				secondaryTG.CreateNewRevisionsWith(nlb)
 				c.AddTask(secondaryTG)
 			}
 			for _, nlbListener := range nlbListeners {

pkg/model/awsmodel/autoscalinggroup.go

@@ -492,7 +492,7 @@ func (b *AutoscalingGroupModelBuilder) buildAutoScalingGroupTask(c *fi.CloudupMo
 		}
 
 		if extLB.TargetGroupARN != nil {
-			targetGroupName, err := awsup.GetTargetGroupNameFromARN(fi.ValueOf(extLB.TargetGroupARN))
+			targetGroupName, err := awsup.NameForExternalTargetGroup(fi.ValueOf(extLB.TargetGroupARN))
 			if err != nil {
 				return nil, err
 			}

pkg/model/awsmodel/bastion.go

@@ -319,8 +319,6 @@ func (b *BastionModelBuilder) Build(c *fi.CloudupModelBuilderContext) error {
 	// Create NLB itself
 	var nlb *awstasks.NetworkLoadBalancer
 	{
-		loadBalancerName := b.LBName32("bastion")
-
 		tags := b.CloudTags("", false)
 		for k, v := range b.Cluster.Spec.CloudLabels {
 			tags[k] = v
@@ -341,13 +339,12 @@ func (b *BastionModelBuilder) Build(c *fi.CloudupModelBuilderContext) error {
 			Name:      fi.PtrTo(b.NLBName("bastion")),
 			Lifecycle: b.Lifecycle,
 
-			LoadBalancerName: fi.PtrTo(loadBalancerName),
-			CLBName:          fi.PtrTo("bastion." + b.ClusterName()),
-			SubnetMappings:   nlbSubnetMappings,
+			LoadBalancerBaseName: fi.PtrTo(b.LBName32("bastion")),
+			CLBName:              fi.PtrTo("bastion." + b.ClusterName()),
+			SubnetMappings:       nlbSubnetMappings,
 			SecurityGroups: []*awstasks.SecurityGroup{
 				b.LinkToELBSecurityGroup("bastion"),
 			},
-
 			Tags:          tags,
 			VPC:           b.LinkToVPC(),
 			Type:          fi.PtrTo("network"),
@@ -390,6 +387,7 @@ func (b *BastionModelBuilder) Build(c *fi.CloudupModelBuilderContext) error {
 			UnhealthyThreshold: fi.PtrTo(int64(2)),
 			Shared:             fi.PtrTo(false),
 		}
+		tg.CreateNewRevisionsWith(nlb)
 
 		c.AddTask(tg)
 

pkg/model/awsmodel/spotinst.go

@@ -851,7 +851,7 @@ func (b *SpotInstanceGroupModelBuilder) buildLoadBalancers(c *fi.CloudupModelBui
 			c.EnsureTask(lb)
 		}
 		if extLB.TargetGroupARN != nil {
-			targetGroupName, err := awsup.GetTargetGroupNameFromARN(fi.ValueOf(extLB.TargetGroupARN))
+			targetGroupName, err := awsup.NameForExternalTargetGroup(fi.ValueOf(extLB.TargetGroupARN))
 			if err != nil {
 				return nil, nil, err
 			}

pkg/resources/aws/aws.go

@@ -1624,7 +1624,7 @@ func ListTargetGroups(cloud fi.Cloud, vpcID, clusterName string) ([]*resources.R
 		id := aws.StringValue(tg.TargetGroupName)
 		resourceTracker := &resources.Resource{
 			Name:    id,
-			ID:      targetGroup.ARN(),
+			ID:      targetGroup.ARN,
 			Type:    TypeTargetGroup,
 			Deleter: DeleteTargetGroup,
 			Dumper:  DumpTargetGroup,

upup/pkg/fi/cloudup/apply_cluster.go

@@ -155,6 +155,9 @@ type ApplyClusterCmd struct {
 
 	// AdditionalObjects holds cluster-asssociated configuration objects, other than the Cluster and InstanceGroups.
 	AdditionalObjects kubemanifest.ObjectList
+
+	// DeletionProcessing controls whether we process deletions.
+	DeletionProcessing fi.DeletionProcessingMode
 }
 
 func (c *ApplyClusterCmd) Run(ctx context.Context) error {
@@ -714,7 +717,7 @@ func (c *ApplyClusterCmd) Run(ctx context.Context) error {
 	var target fi.CloudupTarget
 	shouldPrecreateDNS := true
 
-	deletionProcessingMode := fi.DeletionProcessingModeDeleteIfNotDeferrred
+	deletionProcessingMode := c.DeletionProcessing
 	switch c.TargetName {
 	case TargetDirect:
 		switch cluster.Spec.GetCloudProvider() {

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

@@ -101,8 +101,11 @@ type AutoscalingGroup struct {
 	CapacityRebalance *bool
 	// WarmPool is the WarmPool config for the ASG
 	WarmPool *WarmPool
+
+	deletions []fi.CloudupDeletion
 }
 
+var _ fi.CloudupProducesDeletions = &AutoscalingGroup{}
 var _ fi.CompareWithID = &AutoscalingGroup{}
 var _ fi.CloudupTaskNormalize = &AutoscalingGroup{}
 
@@ -212,13 +215,21 @@ func (e *AutoscalingGroup) Find(c *fi.CloudupContext) (*AutoscalingGroup, error)
 	sort.Stable(OrderLoadBalancersByName(actual.LoadBalancers))
 
 	actual.TargetGroups = []*TargetGroup{}
-	if len(g.TargetGroupARNs) > 0 {
-		for _, tg := range g.TargetGroupARNs {
-			targetGroupName, err := awsup.GetTargetGroupNameFromARN(fi.ValueOf(tg))
-			if err != nil {
-				return nil, err
+	{
+		byARN := make(map[string]*TargetGroup)
+		for _, tg := range e.TargetGroups {
+			if tg.info != nil {
+				byARN[tg.info.ARN] = tg
 			}
-			actual.TargetGroups = append(actual.TargetGroups, &TargetGroup{ARN: aws.String(*tg), Name: aws.String(targetGroupName)})
+		}
+		for _, arn := range g.TargetGroupARNs {
+			tg := byARN[aws.StringValue(arn)]
+			if tg != nil {
+				actual.TargetGroups = append(actual.TargetGroups, tg)
+				continue
+			}
+			actual.TargetGroups = append(actual.TargetGroups, &TargetGroup{ARN: arn})
+			e.deletions = append(e.deletions, buildDeleteAutoscalingTargetGroupAttachment(aws.StringValue(g.AutoScalingGroupName), aws.StringValue(arn)))
 		}
 	}
 	sort.Stable(OrderTargetGroupsByName(actual.TargetGroups))
@@ -612,7 +623,6 @@ func (v *AutoscalingGroup) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *Autos
 		}
 
 		var attachTGRequests []*autoscaling.AttachLoadBalancerTargetGroupsInput
-		var detachTGRequests []*autoscaling.DetachLoadBalancerTargetGroupsInput
 		if changes.TargetGroups != nil {
 			if e != nil && len(e.TargetGroups) > 0 {
 				for _, tgsChunkToAttach := range sliceChunks(e.AutoscalingTargetGroups(), attachLoadBalancerTargetGroupsMaxItems) {
@@ -623,14 +633,8 @@ func (v *AutoscalingGroup) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *Autos
 				}
 			}
 
-			if a != nil && len(a.TargetGroups) > 0 {
-				for _, tgsChunkToDetach := range sliceChunks(e.getTGsToDetach(a.TargetGroups), detachLoadBalancerTargetGroupsMaxItems) {
-					detachTGRequests = append(detachTGRequests, &autoscaling.DetachLoadBalancerTargetGroupsInput{
-						AutoScalingGroupName: e.Name,
-						TargetGroupARNs:      tgsChunkToDetach,
-					})
-				}
-			}
+			// Detaching is done in a deletion task
+
 			changes.TargetGroups = nil
 		}
 
@@ -719,13 +723,6 @@ func (v *AutoscalingGroup) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *Autos
 				return fmt.Errorf("error attaching LoadBalancers: %v", err)
 			}
 		}
-		if len(detachTGRequests) > 0 {
-			for _, detachTGRequest := range detachTGRequests {
-				if _, err := t.Cloud.Autoscaling().DetachLoadBalancerTargetGroupsWithContext(ctx, detachTGRequest); err != nil {
-					return fmt.Errorf("failed to detach target groups: %v", err)
-				}
-			}
-		}
 		if len(attachTGRequests) > 0 {
 			for _, attachTGRequest := range attachTGRequests {
 				if _, err := t.Cloud.Autoscaling().AttachLoadBalancerTargetGroupsWithContext(ctx, attachTGRequest); err != nil {
@@ -1121,3 +1118,52 @@ func (_ *AutoscalingGroup) RenderTerraform(t *terraform.TerraformTarget, a, e, c
 func (e *AutoscalingGroup) TerraformLink() *terraformWriter.Literal {
 	return terraformWriter.LiteralProperty("aws_autoscaling_group", fi.ValueOf(e.Name), "id")
 }
+
+func (e *AutoscalingGroup) FindDeletions(context *fi.CloudupContext) ([]fi.CloudupDeletion, error) {
+	return e.deletions, nil
+}
+
+type deleteAutoscalingTargetGroupAttachment struct {
+	autoScalingGroupName string
+	targetGroupARN       string
+}
+
+var _ fi.CloudupDeletion = &deleteAutoscalingTargetGroupAttachment{}
+
+func buildDeleteAutoscalingTargetGroupAttachment(autoScalingGroupName string, targetGroupARN string) *deleteAutoscalingTargetGroupAttachment {
+	d := &deleteAutoscalingTargetGroupAttachment{}
+	d.autoScalingGroupName = autoScalingGroupName
+	d.targetGroupARN = targetGroupARN
+	return d
+}
+
+func (d *deleteAutoscalingTargetGroupAttachment) Delete(t fi.CloudupTarget) error {
+	ctx := context.TODO()
+
+	awsTarget, ok := t.(*awsup.AWSAPITarget)
+	if !ok {
+		return fmt.Errorf("unexpected target type for deletion: %T", t)
+	}
+
+	req := &autoscaling.DetachLoadBalancerTargetGroupsInput{
+		AutoScalingGroupName: aws.String(d.autoScalingGroupName),
+		TargetGroupARNs:      aws.StringSlice([]string{d.targetGroupARN}),
+	}
+	if _, err := awsTarget.Cloud.Autoscaling().DetachLoadBalancerTargetGroupsWithContext(ctx, req); err != nil {
+		return fmt.Errorf("failed to detach target groups from autoscaling group: %v", err)
+	}
+
+	return nil
+}
+
+func (d *deleteAutoscalingTargetGroupAttachment) TaskName() string {
+	return "autoscaling-elb-attachment"
+
+}
+func (d *deleteAutoscalingTargetGroupAttachment) Item() string {
+	return d.autoScalingGroupName + ":" + d.targetGroupARN
+}
+
+func (d *deleteAutoscalingTargetGroupAttachment) DeferDeletion() bool {
+	return true
+}

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

@@ -50,6 +50,7 @@ type DNSTarget interface {
 }
 
 func (e *DNSName) Find(c *fi.CloudupContext) (*DNSName, error) {
+	ctx := c.Context()
 	cloud := c.T.Cloud.(awsup.AWSCloud)
 
 	if e.Zone == nil || e.Zone.ZoneID == nil {
@@ -75,7 +76,7 @@ func (e *DNSName) Find(c *fi.CloudupContext) (*DNSName, error) {
 
 	var found *route53.ResourceRecordSet
 
-	err := cloud.Route53().ListResourceRecordSetsPages(request, func(p *route53.ListResourceRecordSetsOutput, lastPage bool) (shouldContinue bool) {
+	err := cloud.Route53().ListResourceRecordSetsPagesWithContext(ctx, request, func(p *route53.ListResourceRecordSetsOutput, lastPage bool) (shouldContinue bool) {
 		for _, rr := range p.ResourceRecordSets {
 			resourceType := aws.StringValue(rr.Type)
 			name := aws.StringValue(rr.Name)