Generate revisions of NLB objects, and introduce cleanup phase

This lets us safely make changes to otherwise immutable fields, in particular for adding security groups to NLBs created without them. We detect the older versions, and create deletion tasks to remove them. These tasks can be deferred, and we expect them to be deferred to a "prune" phase that runs after cluster apply. Co-authored-by: Ciprian Hacman <[email protected]>
kubernetes · Feb 17, 2024 · 2a9343a · 2a9343a
1 parent 1cbd622
commit 2a9343a
Show file tree

Hide file tree

Showing 30 changed files with 644 additions and 278 deletions.
diff --git a/cloudmock/aws/mockautoscaling/attach.go b/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)
+}
diff --git a/cmd/kops/update_cluster.go b/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 {

diff --git a/docs/cli/kops_update_cluster.md b/docs/cli/kops_update_cluster.md
diff --git a/pkg/instancegroups/instancegroups.go b/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)

diff --git a/pkg/model/awsmodel/api_loadbalancer.go b/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 {

diff --git a/pkg/model/awsmodel/autoscalinggroup.go b/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
 			}

diff --git a/pkg/model/awsmodel/bastion.go b/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)
 

diff --git a/pkg/model/awsmodel/spotinst.go b/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
 			}

diff --git a/pkg/resources/aws/aws.go b/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,

diff --git a/upup/pkg/fi/cloudup/apply_cluster.go b/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() {

diff --git a/upup/pkg/fi/cloudup/awstasks/autoscalinggroup.go b/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
+}
diff --git a/upup/pkg/fi/cloudup/awstasks/dnsname.go b/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)