diff --git a/cloudmock/aws/mockautoscaling/attach.go b/cloudmock/aws/mockautoscaling/attach.go index 98642989b5422..9b2d4b654cd8f 100644 --- 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 index 322364a5460ad..1579da6e27596 100644 --- 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 index b2ca8a3a11a99..6b74512693ca1 100644 --- a/docs/cli/kops_update_cluster.md +++ b/docs/cli/kops_update_cluster.md @@ -33,6 +33,7 @@ kops update cluster [CLUSTER] [flags] --lifecycle-overrides strings comma separated list of phase overrides, example: SecurityGroups=Ignore,InternetGateway=ExistsAndWarnIfChanges --out string Path to write any local output --phase string Subset of tasks to run: cluster, network, security + --prune Delete old revisions of cloud resources that were needed during an upgrade --ssh-public-key string SSH public key to use (deprecated: use kops create secret instead) --target string Target - direct, terraform (default "direct") --user string Existing user in kubeconfig file to use. Implies --create-kube-config diff --git a/pkg/instancegroups/instancegroups.go b/pkg/instancegroups/instancegroups.go index 5ce6f925bce01..72d15f7edbe3e 100644 --- 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 index 0207ebe4ae2fa..33b66b9d88e2a 100644 --- 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 index 604d3644489e8..cae98deea8753 100644 --- 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 index 750b5938bb3cd..e616db32e2cfe 100644 --- 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 index 894a28e421e1a..4d6b004b1cf91 100644 --- 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 index eeb6c4f44b0b0..2e688225f9959 100644 --- 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 index a3db3a1a3d978..2b9b123bab8c7 100644 --- 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 index 02b6a87983d10..1cff71a99685b 100644 --- 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 index 9551a5174751c..0d043d4b75b5a 100644 --- 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) diff --git a/upup/pkg/fi/cloudup/awstasks/launchtemplate_target_api.go b/upup/pkg/fi/cloudup/awstasks/launchtemplate_target_api.go index 6c0cb5923be53..4073341c3647c 100644 --- a/upup/pkg/fi/cloudup/awstasks/launchtemplate_target_api.go +++ b/upup/pkg/fi/cloudup/awstasks/launchtemplate_target_api.go @@ -402,7 +402,7 @@ func (t *LaunchTemplate) findLatestLaunchTemplateVersion(c *fi.CloudupContext) ( } // deleteLaunchTemplate tracks a LaunchConfiguration that we're going to delete -// It implements fi.Deletion +// It implements fi.CloudupDeletion type deleteLaunchTemplate struct { lc *ec2.LaunchTemplate } @@ -438,3 +438,7 @@ func (d *deleteLaunchTemplate) Delete(t fi.CloudupTarget) error { func (d *deleteLaunchTemplate) String() string { return d.TaskName() + "-" + d.Item() } + +func (d *deleteLaunchTemplate) DeferDeletion() bool { + return false // TODO: Should we defer deletion? +} diff --git a/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go b/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go index b1f0ddfe0162d..f1a847ebe8aa5 100644 --- a/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go +++ b/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go @@ -22,13 +22,14 @@ import ( "sort" "strconv" "strings" + "time" "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/service/elb" "github.com/aws/aws-sdk-go/service/elbv2" "github.com/aws/aws-sdk-go/service/route53" "k8s.io/klog/v2" + "k8s.io/kops/pkg/truncate" "k8s.io/kops/pkg/wellknownservices" "k8s.io/kops/upup/pkg/fi" "k8s.io/kops/upup/pkg/fi/cloudup/awsup" @@ -46,11 +47,14 @@ type NetworkLoadBalancer struct { Name *string Lifecycle fi.Lifecycle - // LoadBalancerName is the name in NLB, possibly different from our name + // LoadBalancerBaseName is the base name to use when naming load balancers in NLB. + // The full, stable name will be in the Name tag. // (NLB is restricted as to names, so we have limited choices!) - // We use the Name tag to find the existing NLB. - LoadBalancerName *string - CLBName *string + LoadBalancerBaseName *string + + // CLBName is the name of a ClassicLoadBalancer to delete, if found. + // This enables migration from CLB -> NLB + CLBName *string DNSName *string HostedZoneId *string @@ -80,6 +84,12 @@ type NetworkLoadBalancer struct { // After this is found/created, we store the ARN loadBalancerArn string + + // After this is found/created, we store the revision + revision string + + // deletions is a list of previous versions of this object, that we should delete when asked to clean up. + deletions []fi.CloudupDeletion } func (e *NetworkLoadBalancer) SetWaitForLoadBalancerReady(v bool) { @@ -94,43 +104,9 @@ func (e *NetworkLoadBalancer) CompareWithID() *string { return e.Name } -// The load balancer name 'api.renamenlbcluster.k8s.local' can only contain characters that are alphanumeric characters and hyphens(-)\n\tstatus code: 400, -func findNetworkLoadBalancerByLoadBalancerName(cloud awsup.AWSCloud, loadBalancerName string) (*elbv2.LoadBalancer, error) { - request := &elbv2.DescribeLoadBalancersInput{ - Names: []*string{&loadBalancerName}, - } - found, err := describeNetworkLoadBalancers(cloud, request, func(lb *elbv2.LoadBalancer) bool { - // TODO: Filter by cluster? - - if aws.StringValue(lb.LoadBalancerName) == loadBalancerName { - return true - } - - klog.Warningf("Got NLB with unexpected name: %q", aws.StringValue(lb.LoadBalancerName)) - return false - }) - if err != nil { - if awsError, ok := err.(awserr.Error); ok { - if awsError.Code() == "LoadBalancerNotFound" { - return nil, nil - } - } - - return nil, fmt.Errorf("error listing NLBs: %v", err) - } - - if len(found) == 0 { - return nil, nil - } - - if len(found) != 1 { - return nil, fmt.Errorf("Found multiple NLBs with name %q", loadBalancerName) - } - - return found[0], nil -} - func findNetworkLoadBalancerByAlias(cloud awsup.AWSCloud, alias *route53.AliasTarget) (*elbv2.LoadBalancer, error) { + ctx := context.TODO() + // TODO: Any way to avoid listing all NLBs? request := &elbv2.DescribeLoadBalancersInput{} @@ -142,9 +118,7 @@ func findNetworkLoadBalancerByAlias(cloud awsup.AWSCloud, alias *route53.AliasTa matchHostedZoneId := aws.StringValue(alias.HostedZoneId) - found, err := describeNetworkLoadBalancers(cloud, request, func(lb *elbv2.LoadBalancer) bool { - // TODO: Filter by cluster? - + found, err := describeNetworkLoadBalancers(ctx, cloud, request, func(lb *elbv2.LoadBalancer) bool { if matchHostedZoneId != aws.StringValue(lb.CanonicalHostedZoneId) { return false } @@ -168,9 +142,9 @@ func findNetworkLoadBalancerByAlias(cloud awsup.AWSCloud, alias *route53.AliasTa return found[0], nil } -func describeNetworkLoadBalancers(cloud awsup.AWSCloud, request *elbv2.DescribeLoadBalancersInput, filter func(*elbv2.LoadBalancer) bool) ([]*elbv2.LoadBalancer, error) { +func describeNetworkLoadBalancers(ctx context.Context, cloud awsup.AWSCloud, request *elbv2.DescribeLoadBalancersInput, filter func(*elbv2.LoadBalancer) bool) ([]*elbv2.LoadBalancer, error) { var found []*elbv2.LoadBalancer - err := cloud.ELBV2().DescribeLoadBalancersPages(request, func(p *elbv2.DescribeLoadBalancersOutput, lastPage bool) (shouldContinue bool) { + err := cloud.ELBV2().DescribeLoadBalancersPagesWithContext(ctx, request, func(p *elbv2.DescribeLoadBalancersOutput, lastPage bool) (shouldContinue bool) { for _, lb := range p.LoadBalancers { if filter(lb) { found = append(found, lb) @@ -195,22 +169,44 @@ func (e *NetworkLoadBalancer) getHostedZoneId() *string { } func (e *NetworkLoadBalancer) Find(c *fi.CloudupContext) (*NetworkLoadBalancer, error) { + ctx := c.Context() cloud := c.T.Cloud.(awsup.AWSCloud) - lb, err := cloud.FindELBV2ByNameTag(e.Tags["Name"]) + allLoadBalancers, err := awsup.ListELBV2LoadBalancers(ctx, cloud) if err != nil { return nil, err } - if lb == nil { + + latest := awsup.FindLatestELBV2ByNameTag(allLoadBalancers, fi.ValueOf(e.Name)) + if err != nil { + return nil, err + } + + // Stash deletions for later + for _, lb := range allLoadBalancers { + if lb.NameTag() != fi.ValueOf(e.Name) { + continue + } + if latest != nil && latest.ARN() == lb.ARN() { + continue + } + + e.deletions = append(e.deletions, &deleteNLB{ + obj: lb, + }) + } + + if latest == nil { return nil, nil } - loadBalancerArn := lb.LoadBalancerArn + lb := latest.LoadBalancer + + loadBalancerArn := latest.ARN() actual := &NetworkLoadBalancer{} actual.Name = e.Name actual.CLBName = e.CLBName - actual.LoadBalancerName = lb.LoadBalancerName actual.DNSName = lb.DNSName actual.HostedZoneId = lb.CanonicalHostedZoneId // CanonicalHostedZoneNameID actual.Scheme = lb.Scheme @@ -218,16 +214,16 @@ func (e *NetworkLoadBalancer) Find(c *fi.CloudupContext) (*NetworkLoadBalancer, actual.Type = lb.Type actual.IpAddressType = lb.IpAddressType - tagMap, err := cloud.DescribeELBV2Tags([]string{*loadBalancerArn}) - if err != nil { - return nil, err - } actual.Tags = make(map[string]string) - for _, tag := range tagMap[*loadBalancerArn] { - if strings.HasPrefix(aws.StringValue(tag.Key), "aws:cloudformation:") { + for _, tag := range latest.Tags { + k := aws.StringValue(tag.Key) + if strings.HasPrefix(k, "aws:cloudformation:") { + continue + } + if k == awsup.KopsResourceRevisionTag { continue } - actual.Tags[aws.StringValue(tag.Key)] = aws.StringValue(tag.Value) + actual.Tags[k] = aws.StringValue(tag.Value) } for _, az := range lb.AvailabilityZones { @@ -256,7 +252,7 @@ func (e *NetworkLoadBalancer) Find(c *fi.CloudupContext) (*NetworkLoadBalancer, } { - lbAttributes, err := findNetworkLoadBalancerAttributes(cloud, aws.StringValue(loadBalancerArn)) + lbAttributes, err := findNetworkLoadBalancerAttributes(cloud, loadBalancerArn) if err != nil { return nil, err } @@ -312,31 +308,35 @@ func (e *NetworkLoadBalancer) Find(c *fi.CloudupContext) (*NetworkLoadBalancer, if e.HostedZoneId == nil { e.HostedZoneId = actual.HostedZoneId } - if e.LoadBalancerName == nil { - e.LoadBalancerName = actual.LoadBalancerName - } // An existing internal NLB can't be updated to dualstack. if fi.ValueOf(actual.Scheme) == elbv2.LoadBalancerSchemeEnumInternal && fi.ValueOf(actual.IpAddressType) == elbv2.IpAddressTypeIpv4 { e.IpAddressType = actual.IpAddressType } - // We allow for the LoadBalancerName to be wrong: - // 1. We don't want to force a rename of the NLB, because that is a destructive operation - if fi.ValueOf(e.LoadBalancerName) != fi.ValueOf(actual.LoadBalancerName) { - klog.V(2).Infof("Reusing existing load balancer with name: %q", aws.StringValue(actual.LoadBalancerName)) - e.LoadBalancerName = actual.LoadBalancerName - } - _ = actual.Normalize(c) actual.WellKnownServices = e.WellKnownServices actual.Lifecycle = e.Lifecycle + actual.LoadBalancerBaseName = e.LoadBalancerBaseName - // Store for other tasks + // Store state for other tasks e.loadBalancerArn = aws.StringValue(lb.LoadBalancerArn) + actual.loadBalancerArn = e.loadBalancerArn + e.revision, _ = latest.GetTag(awsup.KopsResourceRevisionTag) + actual.revision = e.revision klog.V(4).Infof("Found NLB %+v", actual) + // AWS does not allow us to add security groups to an ELB that was initially created without them. + // This forces a new revision (currently, the only operation that forces a new revision) + if len(actual.SecurityGroups) == 0 && len(e.SecurityGroups) > 0 { + klog.Warningf("setting securityGroups on an existing NLB created without securityGroups; will force a new NLB") + t := time.Now() + revision := strconv.FormatInt(t.Unix(), 10) + actual = nil + e.revision = revision + } + return actual, nil } @@ -348,30 +348,37 @@ func (e *NetworkLoadBalancer) GetWellKnownServices() []wellknownservices.WellKno return e.WellKnownServices } -func (e *NetworkLoadBalancer) FindAddresses(context *fi.CloudupContext) ([]string, error) { +func (e *NetworkLoadBalancer) FindAddresses(c *fi.CloudupContext) ([]string, error) { + ctx := c.Context() + var addresses []string - cloud := context.T.Cloud.(awsup.AWSCloud) - cluster := context.T.Cluster + cloud := c.T.Cloud.(awsup.AWSCloud) + cluster := c.T.Cluster { - lb, err := cloud.FindELBV2ByNameTag(e.Tags["Name"]) + allLoadBalancers, err := awsup.ListELBV2LoadBalancers(ctx, cloud) if err != nil { - return nil, fmt.Errorf("failed to find load balancer matching %q: %w", e.Tags["Name"], err) - } - if lb != nil && fi.ValueOf(lb.DNSName) != "" { - addresses = append(addresses, fi.ValueOf(lb.DNSName)) + return nil, err } - } - if cluster.UsesNoneDNS() { - nis, err := cloud.FindELBV2NetworkInterfacesByName(fi.ValueOf(e.VPC.ID), fi.ValueOf(e.LoadBalancerName)) - if err != nil { - return nil, fmt.Errorf("failed to find network interfaces matching %q: %w", fi.ValueOf(e.LoadBalancerName), err) - } - for _, ni := range nis { - if fi.ValueOf(ni.PrivateIpAddress) != "" { - addresses = append(addresses, fi.ValueOf(ni.PrivateIpAddress)) + lb := awsup.FindLatestELBV2ByNameTag(allLoadBalancers, fi.ValueOf(e.Name)) + + if lb != nil { + if fi.ValueOf(lb.LoadBalancer.DNSName) != "" { + addresses = append(addresses, fi.ValueOf(lb.LoadBalancer.DNSName)) + } + + if cluster.UsesNoneDNS() { + nis, err := cloud.FindELBV2NetworkInterfacesByName(fi.ValueOf(e.VPC.ID), aws.StringValue(lb.LoadBalancer.LoadBalancerName)) + if err != nil { + return nil, fmt.Errorf("failed to find network interfaces matching %q: %w", aws.StringValue(lb.LoadBalancer.LoadBalancerName), err) + } + for _, ni := range nis { + if fi.ValueOf(ni.PrivateIpAddress) != "" { + addresses = append(addresses, fi.ValueOf(ni.PrivateIpAddress)) + } + } } } } @@ -456,23 +463,42 @@ func (*NetworkLoadBalancer) CheckChanges(a, e, changes *NetworkLoadBalancer) err func (_ *NetworkLoadBalancer) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *NetworkLoadBalancer) error { ctx := context.TODO() - var loadBalancerName string - var loadBalancerArn string + loadBalancerArn := "" + + revision := e.revision + + // TODO: Use maps.Clone when we are >= go1.21 on supported branches + tags := make(map[string]string) + for k, v := range e.Tags { + tags[k] = v + } + + // We removed revision for the diff/plan, but we want to set it + if revision != "" { + tags[awsup.KopsResourceRevisionTag] = revision + } if a == nil { - if e.LoadBalancerName == nil { - return fi.RequiredField("LoadBalancerName") + loadBalancerName := fi.ValueOf(e.LoadBalancerBaseName) + if revision != "" { + s := fi.ValueOf(e.LoadBalancerBaseName) + "-" + revision + + // We always compute the hash and add it, lest we trick users into assuming that we never do this + opt := truncate.TruncateStringOptions{ + MaxLength: 32, + AlwaysAddHash: true, + HashLength: 6, + } + loadBalancerName = truncate.TruncateString(s, opt) } - loadBalancerName = *e.LoadBalancerName - { request := &elbv2.CreateLoadBalancerInput{} - request.Name = e.LoadBalancerName + request.Name = &loadBalancerName request.Scheme = e.Scheme request.Type = e.Type request.IpAddressType = e.IpAddressType - request.Tags = awsup.ELBv2Tags(e.Tags) + request.Tags = awsup.ELBv2Tags(tags) for _, subnetMapping := range e.SubnetMappings { request.SubnetMappings = append(request.SubnetMappings, &elbv2.SubnetMapping{ @@ -502,6 +528,7 @@ func (_ *NetworkLoadBalancer) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *Ne e.VPC = &VPC{ID: lb.VpcId} loadBalancerArn = aws.StringValue(lb.LoadBalancerArn) e.loadBalancerArn = loadBalancerArn + e.revision = revision } if e.waitForLoadBalancerReady { @@ -517,19 +544,12 @@ func (_ *NetworkLoadBalancer) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *Ne } } else { - loadBalancerName = fi.ValueOf(a.LoadBalancerName) - - lb, err := findNetworkLoadBalancerByLoadBalancerName(t.Cloud, loadBalancerName) - if err != nil { - return fmt.Errorf("error getting load balancer by name: %v", err) - } - - loadBalancerArn = fi.ValueOf(lb.LoadBalancerArn) + loadBalancerArn = a.loadBalancerArn if changes.IpAddressType != nil { request := &elbv2.SetIpAddressTypeInput{ IpAddressType: e.IpAddressType, - LoadBalancerArn: lb.LoadBalancerArn, + LoadBalancerArn: &loadBalancerArn, } if _, err := t.Cloud.ELBV2().SetIpAddressType(request); err != nil { return fmt.Errorf("error setting the IP addresses type: %v", err) @@ -576,7 +596,7 @@ func (_ *NetworkLoadBalancer) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *Ne if changes.SecurityGroups != nil { request := &elbv2.SetSecurityGroupsInput{ - LoadBalancerArn: lb.LoadBalancerArn, + LoadBalancerArn: &loadBalancerArn, } for _, sg := range e.SecurityGroups { request.SecurityGroups = append(request.SecurityGroups, sg.ID) @@ -588,11 +608,11 @@ func (_ *NetworkLoadBalancer) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *Ne } } - if err := t.AddELBV2Tags(loadBalancerArn, e.Tags); err != nil { + if err := t.AddELBV2Tags(loadBalancerArn, tags); err != nil { return err } - if err := t.RemoveELBV2Tags(loadBalancerArn, e.Tags); err != nil { + if err := t.RemoveELBV2Tags(loadBalancerArn, tags); err != nil { return err } } @@ -625,7 +645,7 @@ type terraformNetworkLoadBalancerSubnetMapping struct { func (_ *NetworkLoadBalancer) RenderTerraform(t *terraform.TerraformTarget, a, e, changes *NetworkLoadBalancer) error { nlbTF := &terraformNetworkLoadBalancer{ - Name: *e.LoadBalancerName, + Name: *e.LoadBalancerBaseName, Internal: fi.ValueOf(e.Scheme) == elbv2.LoadBalancerSchemeEnumInternal, Type: elbv2.LoadBalancerTypeEnumNetwork, Tags: e.Tags, @@ -679,33 +699,25 @@ func (e *NetworkLoadBalancer) TerraformLink(params ...string) *terraformWriter.L // FindDeletions schedules deletion of the corresponding legacy classic load balancer when it no longer has targets. func (e *NetworkLoadBalancer) FindDeletions(context *fi.CloudupContext) ([]fi.CloudupDeletion, error) { - if e.CLBName == nil { - return nil, nil - } + var deletions []fi.CloudupDeletion - cloud := context.T.Cloud.(awsup.AWSCloud) + deletions = append(deletions, e.deletions...) - lb, err := cloud.FindELBByNameTag(fi.ValueOf(e.CLBName)) - if err != nil { - return nil, err - } - if lb == nil { - return nil, nil - } - - // Testing shows that the instances are deregistered immediately after the apply_cluster. - // TODO: Figure out how to delay deregistration until instances are terminated. - //if len(lb.Instances) > 0 { - // klog.V(2).Infof("CLB %s has targets; not scheduling deletion", *lb.LoadBalancerName) - // return nil, nil - //} + if e.CLBName != nil { + cloud := context.T.Cloud.(awsup.AWSCloud) - actual := &deleteClassicLoadBalancer{} - actual.LoadBalancerName = lb.LoadBalancerName + lb, err := cloud.FindELBByNameTag(fi.ValueOf(e.CLBName)) + if err != nil { + return nil, err + } - klog.V(4).Infof("Found CLB %+v", actual) + if lb != nil { + klog.V(4).Infof("Found CLB %v", aws.StringValue(lb.LoadBalancerName)) + deletions = append(deletions, &deleteClassicLoadBalancer{LoadBalancerName: e.CLBName}) + } + } - return []fi.CloudupDeletion{actual}, nil + return deletions, nil } type deleteClassicLoadBalancer struct { @@ -714,6 +726,18 @@ type deleteClassicLoadBalancer struct { LoadBalancerName *string } +func (d deleteClassicLoadBalancer) TaskName() string { + return "ClassicLoadBalancer" +} + +func (d deleteClassicLoadBalancer) Item() string { + return *d.LoadBalancerName +} + +func (d deleteClassicLoadBalancer) DeferDeletion() bool { + return true +} + func (d deleteClassicLoadBalancer) Delete(t fi.CloudupTarget) error { awsTarget, ok := t.(*awsup.AWSAPITarget) if !ok { @@ -730,10 +754,54 @@ func (d deleteClassicLoadBalancer) Delete(t fi.CloudupTarget) error { return nil } -func (d deleteClassicLoadBalancer) TaskName() string { - return "ClassicLoadBalancer" +// deleteNLB tracks a NLB that we're going to delete +// It implements fi.CloudupDeletion +type deleteNLB struct { + obj *awsup.LoadBalancerInfo } -func (d deleteClassicLoadBalancer) Item() string { - return *d.LoadBalancerName +func buildDeleteNLB(obj *awsup.LoadBalancerInfo) *deleteNLB { + d := &deleteNLB{} + d.obj = obj + return d +} + +var _ fi.CloudupDeletion = &deleteNLB{} + +func (d *deleteNLB) 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) + } + + arn := d.obj.ARN() + klog.V(2).Infof("deleting load balancer %q", arn) + if _, err := awsTarget.Cloud.ELBV2().DeleteLoadBalancerWithContext(ctx, &elbv2.DeleteLoadBalancerInput{ + LoadBalancerArn: &arn, + }); err != nil { + return fmt.Errorf("error deleting ELB LoadBalancer %q: %w", arn, err) + } + + return nil +} + +// String returns a string representation of the task +func (d *deleteNLB) String() string { + return d.TaskName() + "-" + d.Item() +} + +// TaskName returns the task name +func (d *deleteNLB) TaskName() string { + return "network-load-balancer" +} + +// Item returns the launch template name +func (d *deleteNLB) Item() string { + return d.obj.ARN() +} + +func (d *deleteNLB) DeferDeletion() bool { + return true } diff --git a/upup/pkg/fi/cloudup/awstasks/networkloadbalancer_attributes.go b/upup/pkg/fi/cloudup/awstasks/networkloadbalancer_attributes.go index d670da6ce475d..08fb732983705 100644 --- a/upup/pkg/fi/cloudup/awstasks/networkloadbalancer_attributes.go +++ b/upup/pkg/fi/cloudup/awstasks/networkloadbalancer_attributes.go @@ -72,8 +72,6 @@ func (_ *NetworkLoadBalancer) modifyLoadBalancerAttributes(t *awsup.AWSAPITarget return nil } - loadBalancerName := fi.ValueOf(e.LoadBalancerName) - request := &elbv2.ModifyLoadBalancerAttributesInput{ LoadBalancerArn: aws.String(loadBalancerArn), } @@ -113,14 +111,14 @@ func (_ *NetworkLoadBalancer) modifyLoadBalancerAttributes(t *awsup.AWSAPITarget request.Attributes = attributes - klog.V(2).Infof("Configuring NLB attributes for NLB %q", loadBalancerName) + klog.V(2).Infof("Configuring NLB attributes for NLB %q", loadBalancerArn) response, err := t.Cloud.ELBV2().ModifyLoadBalancerAttributes(request) if err != nil { - return fmt.Errorf("error configuring NLB attributes for NLB %q: %v", loadBalancerName, err) + return fmt.Errorf("error configuring NLB attributes for NLB %q: %v", loadBalancerArn, err) } - klog.V(4).Infof("modified NLB attributes for NLB %q, response %+v", loadBalancerName, response) + klog.V(4).Infof("modified NLB attributes for NLB %q, response %+v", loadBalancerArn, response) return nil } diff --git a/upup/pkg/fi/cloudup/awstasks/networkloadbalancerlistener.go b/upup/pkg/fi/cloudup/awstasks/networkloadbalancerlistener.go index 4fea0a218effe..79b366fc2f36b 100644 --- a/upup/pkg/fi/cloudup/awstasks/networkloadbalancerlistener.go +++ b/upup/pkg/fi/cloudup/awstasks/networkloadbalancerlistener.go @@ -31,6 +31,8 @@ import ( // +kops:fitask type NetworkLoadBalancerListener struct { + // We use the Name tag to find the existing NLB, because we are (more or less) unrestricted when + // it comes to tag values, but the LoadBalancerName is length limited Name *string Lifecycle fi.Lifecycle @@ -203,8 +205,6 @@ func (*NetworkLoadBalancerListener) RenderAWS(t *awsup.AWSAPITarget, a, e, chang } } - // TODO: Tags on the listener? - return nil } @@ -226,7 +226,6 @@ func (_ *NetworkLoadBalancerListener) RenderTerraform(t *terraform.TerraformTarg if e.TargetGroup == nil { return fi.RequiredField("TargetGroup") } - listenerTF := &terraformNetworkLoadBalancerListener{ LoadBalancer: e.NetworkLoadBalancer.TerraformLink(), Port: int64(e.Port), diff --git a/upup/pkg/fi/cloudup/awstasks/securitygroup.go b/upup/pkg/fi/cloudup/awstasks/securitygroup.go index fc7a0894732b9..ca2b51aef8743 100644 --- a/upup/pkg/fi/cloudup/awstasks/securitygroup.go +++ b/upup/pkg/fi/cloudup/awstasks/securitygroup.go @@ -222,10 +222,18 @@ func (e *SecurityGroup) TerraformLink() *terraformWriter.Literal { return terraformWriter.LiteralProperty("aws_security_group", *e.Name, "id") } +// deleteSecurityGroupRule tracks a securitygrouprule that we're going to delete +// It implements fi.CloudupDeletion type deleteSecurityGroupRule struct { rule *ec2.SecurityGroupRule } +func buildDeleteSecurityGroupRule(rule *ec2.SecurityGroupRule) *deleteSecurityGroupRule { + d := &deleteSecurityGroupRule{} + d.rule = rule + return d +} + var _ fi.CloudupDeletion = &deleteSecurityGroupRule{} func (d *deleteSecurityGroupRule) Delete(t fi.CloudupTarget) error { @@ -236,7 +244,7 @@ func (d *deleteSecurityGroupRule) Delete(t fi.CloudupTarget) error { return fmt.Errorf("unexpected target type for deletion: %T", t) } - if fi.ValueOf(d.rule.IsEgress) { + if aws.BoolValue(d.rule.IsEgress) { request := &ec2.RevokeSecurityGroupEgressInput{ GroupId: d.rule.GroupId, SecurityGroupRuleIds: []*string{d.rule.SecurityGroupRuleId}, @@ -290,6 +298,10 @@ func (d *deleteSecurityGroupRule) Item() string { return s } +func (d *deleteSecurityGroupRule) DeferDeletion() bool { + return true +} + func (e *SecurityGroup) FindDeletions(c *fi.CloudupContext) ([]fi.CloudupDeletion, error) { var removals []fi.CloudupDeletion @@ -360,9 +372,7 @@ func (e *SecurityGroup) FindDeletions(c *fi.CloudupContext) ([]fi.CloudupDeletio } } if !found { - removals = append(removals, &deleteSecurityGroupRule{ - rule: permission, - }) + removals = append(removals, buildDeleteSecurityGroupRule(permission)) } } diff --git a/upup/pkg/fi/cloudup/awstasks/subnet.go b/upup/pkg/fi/cloudup/awstasks/subnet.go index fd9654a683ef6..907f2d094afa9 100644 --- a/upup/pkg/fi/cloudup/awstasks/subnet.go +++ b/upup/pkg/fi/cloudup/awstasks/subnet.go @@ -541,6 +541,10 @@ func (d *deleteSubnetIPv6CIDRBlock) Item() string { return fmt.Sprintf("%v: ipv6cidr=%v", *d.vpcID, *d.ipv6CidrBlock) } +func (d *deleteSubnetIPv6CIDRBlock) DeferDeletion() bool { + return false // TODO: should we defer this? +} + func calculateSubnetCIDR(vpcCIDR, subnetCIDR *string) (*string, error) { if vpcCIDR == nil { return nil, fmt.Errorf("expecting VPC CIDR to not be ") diff --git a/upup/pkg/fi/cloudup/awstasks/targetgroup.go b/upup/pkg/fi/cloudup/awstasks/targetgroup.go index 125c36ce9d65a..d7b6ab1c353e5 100644 --- a/upup/pkg/fi/cloudup/awstasks/targetgroup.go +++ b/upup/pkg/fi/cloudup/awstasks/targetgroup.go @@ -19,11 +19,13 @@ package awstasks import ( "context" "fmt" + "strconv" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/service/elbv2" "k8s.io/klog/v2" + "k8s.io/kops/pkg/truncate" "k8s.io/kops/upup/pkg/fi" "k8s.io/kops/upup/pkg/fi/cloudup/awsup" "k8s.io/kops/upup/pkg/fi/cloudup/terraform" @@ -50,6 +52,9 @@ type TargetGroup struct { Port *int64 Protocol *string + // networkLoadBalancer, if set, will create a new Target Group for each revision of the Network Load Balancer + networkLoadBalancer *NetworkLoadBalancer + // ARN is the Amazon Resource Name for the Target Group ARN *string @@ -61,15 +66,41 @@ type TargetGroup struct { Interval *int64 HealthyThreshold *int64 UnhealthyThreshold *int64 + + info *awsup.TargetGroupInfo + revision string + + // deletions is a list of previous versions of this object, that we should delete when asked to clean up. + deletions []fi.CloudupDeletion +} + +// CreateNewRevisionsWith will create new revisions of the TargetGroup when the given networkLoadBalancer has a new revision. +// This works around the fact that TargetGroups can only be attached to a single NetworkLoadBalancer. +func (e *TargetGroup) CreateNewRevisionsWith(nlb *NetworkLoadBalancer) { + e.networkLoadBalancer = nlb +} + +var _ fi.CloudupHasDependencies = &TargetGroup{} + +// GetDependencies returns the dependencies of the TargetGroup task +// We need to do this because we hide the networkLoadBalancer field +func (e *TargetGroup) GetDependencies(tasks map[string]fi.CloudupTask) []fi.CloudupTask { + var deps []fi.CloudupTask + deps = append(deps, e.VPC) + deps = append(deps, e.networkLoadBalancer) + return deps } var _ fi.CompareWithID = &TargetGroup{} func (e *TargetGroup) CompareWithID() *string { - return e.ARN + if e.ARN != nil { + return e.ARN + } + return e.Name } -func (e *TargetGroup) findTargetGroupByName(ctx context.Context, cloud awsup.AWSCloud) (*awsup.TargetGroupInfo, error) { +func (e *TargetGroup) findLatestTargetGroupByName(ctx context.Context, cloud awsup.AWSCloud) (*awsup.TargetGroupInfo, error) { name := fi.ValueOf(e.Name) targetGroups, err := awsup.ListELBV2TargetGroups(ctx, cloud) @@ -78,15 +109,56 @@ func (e *TargetGroup) findTargetGroupByName(ctx context.Context, cloud awsup.AWS } var latest *awsup.TargetGroupInfo + var latestRevision int for _, targetGroup := range targetGroups { // We accept the name tag _or_ the TargetGroupName itself, to allow matching groups that might predate tagging. if aws.StringValue(targetGroup.TargetGroup.TargetGroupName) != name && targetGroup.NameTag() != name { continue } - if latest != nil { - return nil, fmt.Errorf("found multiple TargetGroups with name %q, expected 1", fi.ValueOf(e.Name)) + revisionTag, _ := targetGroup.GetTag(awsup.KopsResourceRevisionTag) + + revision := -1 + if revisionTag == "" { + revision = 0 + } else { + n, err := strconv.Atoi(revisionTag) + if err != nil { + klog.Warningf("ignoring target group %q with revision %q", targetGroup.ARN, revision) + continue + } + revision = n + } + + if latest == nil || revision > latestRevision { + latestRevision = revision + latest = targetGroup } - latest = targetGroup + } + + if latest != nil && e.networkLoadBalancer != nil { + matchRevision := e.networkLoadBalancer.revision + arn := e.networkLoadBalancer.loadBalancerArn + if arn == "" { + return nil, fmt.Errorf("load balancer not ready (no ARN)") + } + revisionTag, _ := latest.GetTag(awsup.KopsResourceRevisionTag) + + if revisionTag != matchRevision { + klog.Warningf("found target group but revision %q does not match load balancer revision %q; will create a new target group", revisionTag, matchRevision) + latest = nil + } + } + + // Record deletions for later + for _, targetGroup := range targetGroups { + if aws.StringValue(targetGroup.TargetGroup.TargetGroupName) != name && targetGroup.NameTag() != name { + continue + } + if latest != nil && latest.ARN == targetGroup.ARN { + continue + } + + e.deletions = append(e.deletions, buildDeleteTargetGroup(targetGroup)) } return latest, nil @@ -124,6 +196,7 @@ func (e *TargetGroup) findTargetGroupByARN(ctx context.Context, cloud awsup.AWSC info := &awsup.TargetGroupInfo{ TargetGroup: tg, + ARN: aws.StringValue(tg.TargetGroupArn), } for _, t := range tagResponse.TagDescriptions { @@ -140,7 +213,7 @@ func (e *TargetGroup) Find(c *fi.CloudupContext) (*TargetGroup, error) { var targetGroupInfo *awsup.TargetGroupInfo if e.ARN == nil { - tgi, err := e.findTargetGroupByName(ctx, cloud) + tgi, err := e.findLatestTargetGroupByName(ctx, cloud) if err != nil { return nil, err } @@ -169,15 +242,23 @@ func (e *TargetGroup) Find(c *fi.CloudupContext) (*TargetGroup, error) { UnhealthyThreshold: tg.UnhealthyThresholdCount, VPC: &VPC{ID: tg.VpcId}, } + actual.info = targetGroupInfo + e.info = targetGroupInfo + actual.revision, _ = targetGroupInfo.GetTag(awsup.KopsResourceRevisionTag) + e.revision = actual.revision + // Interval cannot be changed after TargetGroup creation e.Interval = actual.Interval e.ARN = tg.TargetGroupArn - tags := make(map[string]string) for _, tag := range targetGroupInfo.Tags { k := fi.ValueOf(tag.Key) v := fi.ValueOf(tag.Value) + if k == awsup.KopsResourceRevisionTag { + actual.revision = v + continue + } tags[k] = v } actual.Tags = tags @@ -230,26 +311,58 @@ func (_ *TargetGroup) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *TargetGrou return nil } + tags := make(map[string]string) + for k, v := range e.Tags { + tags[k] = v + } + if a != nil { + if a.revision != "" { + tags[awsup.KopsResourceRevisionTag] = a.revision + } + } + + if e.networkLoadBalancer != nil { + if e.networkLoadBalancer.loadBalancerArn == "" { + return fmt.Errorf("load balancer not yet ready (arn is empty)") + } + nlbRevision := e.networkLoadBalancer.revision + if nlbRevision != "" { + tags[awsup.KopsResourceRevisionTag] = nlbRevision + } + } + // You register targets for your Network Load Balancer with a target group. By default, the load balancer sends requests // to registered targets using the port and protocol that you specified for the target group. You can override this port // when you register each target with the target group. if a == nil { + createTargetGroupName := *e.Name + if tags[awsup.KopsResourceRevisionTag] != "" { + s := *e.Name + tags[awsup.KopsResourceRevisionTag] + // We always compute the hash and add it, lest we trick users into assuming that we never do this + opt := truncate.TruncateStringOptions{ + MaxLength: 32, + AlwaysAddHash: true, + HashLength: 6, + } + createTargetGroupName = truncate.TruncateString(s, opt) + } + request := &elbv2.CreateTargetGroupInput{ - Name: e.Name, + Name: &createTargetGroupName, Port: e.Port, Protocol: e.Protocol, VpcId: e.VPC.ID, HealthCheckIntervalSeconds: e.Interval, HealthyThresholdCount: e.HealthyThreshold, UnhealthyThresholdCount: e.UnhealthyThreshold, - Tags: awsup.ELBv2Tags(e.Tags), + Tags: awsup.ELBv2Tags(tags), } klog.V(2).Infof("Creating Target Group for NLB") response, err := t.Cloud.ELBV2().CreateTargetGroup(request) if err != nil { - return fmt.Errorf("Error creating target group for NLB : %v", err) + return fmt.Errorf("creating NLB target group: %w", err) } if err := ModifyTargetGroupAttributes(t.Cloud, response.TargetGroups[0].TargetGroupArn, e.Attributes); err != nil { @@ -259,6 +372,7 @@ func (_ *TargetGroup) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *TargetGrou // Avoid spurious changes e.ARN = response.TargetGroups[0].TargetGroupArn + // TODO: Set revision or info? } else { if a.ARN != nil { if err := t.AddELBV2Tags(fi.ValueOf(a.ARN), e.Tags); err != nil { @@ -364,3 +478,62 @@ func (e *TargetGroup) TerraformLink() *terraformWriter.Literal { } return terraformWriter.LiteralProperty("aws_lb_target_group", *e.Name, "id") } + +var _ fi.CloudupProducesDeletions = &TargetGroup{} + +// FindDeletions is responsible for finding launch templates which can be deleted +func (e *TargetGroup) FindDeletions(c *fi.CloudupContext) ([]fi.CloudupDeletion, error) { + var removals []fi.CloudupDeletion + removals = append(removals, e.deletions...) + return removals, nil +} + +// deleteTargetGroup tracks a TargetGroup that we're going to delete +// It implements fi.CloudupDeletion +type deleteTargetGroup struct { + obj *awsup.TargetGroupInfo +} + +func buildDeleteTargetGroup(obj *awsup.TargetGroupInfo) *deleteTargetGroup { + d := &deleteTargetGroup{} + d.obj = obj + return d +} + +var _ fi.CloudupDeletion = &deleteTargetGroup{} + +func (d *deleteTargetGroup) 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) + } + + arn := d.obj.ARN + klog.V(2).Infof("deleting target group %q", arn) + if _, err := awsTarget.Cloud.ELBV2().DeleteTargetGroupWithContext(ctx, &elbv2.DeleteTargetGroupInput{ + TargetGroupArn: &arn, + }); err != nil { + return fmt.Errorf("error deleting ELB TargetGroup %q: %w", arn, err) + } + + return nil +} + +// String returns a string representation of the task +func (d *deleteTargetGroup) String() string { + return d.TaskName() + "-" + d.Item() +} + +func (d *deleteTargetGroup) TaskName() string { + return "target-group" +} + +func (d *deleteTargetGroup) Item() string { + return d.obj.ARN +} + +func (d *deleteTargetGroup) DeferDeletion() bool { + return true +} diff --git a/upup/pkg/fi/cloudup/awstasks/vpc.go b/upup/pkg/fi/cloudup/awstasks/vpc.go index 83d595cfe8656..3f47f17ff9eff 100644 --- a/upup/pkg/fi/cloudup/awstasks/vpc.go +++ b/upup/pkg/fi/cloudup/awstasks/vpc.go @@ -391,3 +391,7 @@ func (d *deleteVPCCIDRBlock) TaskName() string { func (d *deleteVPCCIDRBlock) Item() string { return fmt.Sprintf("%v: cidr=%v", *d.vpcID, *d.cidrBlock) } + +func (d *deleteVPCCIDRBlock) DeferDeletion() bool { + return false // TODO: should we defer this? +} diff --git a/upup/pkg/fi/cloudup/awsup/aws_cloud.go b/upup/pkg/fi/cloudup/awsup/aws_cloud.go index 300d3200b4735..49e0f4cbbf642 100644 --- a/upup/pkg/fi/cloudup/awsup/aws_cloud.go +++ b/upup/pkg/fi/cloudup/awsup/aws_cloud.go @@ -164,8 +164,6 @@ type AWSCloud interface { FindELBByNameTag(findNameTag string) (*elb.LoadBalancerDescription, error) DescribeELBTags(loadBalancerNames []string) (map[string][]*elb.Tag, error) // TODO: Remove, replace with awsup.ListELBV2LoadBalancers - FindELBV2ByNameTag(findNameTag string) (*elbv2.LoadBalancer, error) - // TODO: Remove, replace with awsup.ListELBV2LoadBalancers DescribeELBV2Tags(loadBalancerNames []string) (map[string][]*elbv2.Tag, error) FindELBV2NetworkInterfacesByName(vpcID string, loadBalancerName string) ([]*ec2.NetworkInterface, error) @@ -1880,68 +1878,34 @@ func describeELBTags(c AWSCloud, loadBalancerNames []string) (map[string][]*elb. return tagMap, nil } -func (c *awsCloudImplementation) FindELBV2ByNameTag(findNameTag string) (*elbv2.LoadBalancer, error) { - return findELBV2ByNameTag(c, findNameTag) -} - -func findELBV2ByNameTag(c AWSCloud, findNameTag string) (*elbv2.LoadBalancer, error) { - // TODO: Any way around this? - klog.V(2).Infof("Listing all NLBs for findNetworkLoadBalancerByNameTag") - - request := &elbv2.DescribeLoadBalancersInput{} - // ELB DescribeTags has a limit of 20 names, so we set the page size here to 20 also - request.PageSize = aws.Int64(20) - - var found []*elbv2.LoadBalancer - - var innerError error - err := c.ELBV2().DescribeLoadBalancersPages(request, func(p *elbv2.DescribeLoadBalancersOutput, lastPage bool) bool { - if len(p.LoadBalancers) == 0 { - return true - } - - // TODO: Filter by cluster? - - var arns []string - arnToELB := make(map[string]*elbv2.LoadBalancer) - for _, elb := range p.LoadBalancers { - arn := aws.StringValue(elb.LoadBalancerArn) - arnToELB[arn] = elb - arns = append(arns, arn) - } - - tagMap, err := c.DescribeELBV2Tags(arns) - if err != nil { - innerError = err - return false +func FindLatestELBV2ByNameTag(loadBalancers []*LoadBalancerInfo, findNameTag string) *LoadBalancerInfo { + var latest *LoadBalancerInfo + var latestRevision int + for _, lb := range loadBalancers { + if lb.NameTag() != findNameTag { + continue } + revisionTag, _ := lb.GetTag(KopsResourceRevisionTag) - for loadBalancerArn, tags := range tagMap { - name, foundNameTag := FindELBV2Tag(tags, "Name") - if !foundNameTag || name != findNameTag { + revision := -1 + if revisionTag == "" { + revision = 0 + } else { + n, err := strconv.Atoi(revisionTag) + if err != nil { + klog.Warningf("ignoring load balancer %q with revision %q", aws.StringValue(lb.LoadBalancer.LoadBalancerArn), revision) continue } - elb := arnToELB[loadBalancerArn] - found = append(found, elb) + revision = n } - return true - }) - if err != nil { - return nil, fmt.Errorf("error describing LoadBalancers: %v", err) - } - if innerError != nil { - return nil, fmt.Errorf("error describing LoadBalancers: %v", innerError) - } - if len(found) == 0 { - return nil, nil - } - - if len(found) != 1 { - return nil, fmt.Errorf("Found multiple NLBs with Name %q", findNameTag) + if latest == nil || revision > latestRevision { + latestRevision = revision + latest = lb + } } - return found[0], nil + return latest } func (c *awsCloudImplementation) FindELBV2NetworkInterfacesByName(vpcID string, loadBalancerName string) ([]*ec2.NetworkInterface, error) { @@ -2333,22 +2297,28 @@ func getApiIngressStatus(c AWSCloud, cluster *kops.Cluster) ([]fi.ApiIngressStat return ingresses, nil } -func findDNSName(c AWSCloud, cluster *kops.Cluster) (string, error) { +func findDNSName(cloud AWSCloud, cluster *kops.Cluster) (string, error) { + ctx := context.TODO() + name := "api." + cluster.Name if cluster.Spec.API.LoadBalancer == nil { return "", nil } if cluster.Spec.API.LoadBalancer.Class == kops.LoadBalancerClassClassic { - if lb, err := c.FindELBByNameTag(name); err != nil { + if lb, err := cloud.FindELBByNameTag(name); err != nil { return "", fmt.Errorf("error looking for AWS ELB: %v", err) } else if lb != nil { return aws.StringValue(lb.DNSName), nil } } else if cluster.Spec.API.LoadBalancer.Class == kops.LoadBalancerClassNetwork { - if lb, err := c.FindELBV2ByNameTag(name); err != nil { - return "", fmt.Errorf("error looking for AWS NLB: %v", err) - } else if lb != nil { - return aws.StringValue(lb.DNSName), nil + allLoadBalancers, err := ListELBV2LoadBalancers(ctx, cloud) + if err != nil { + return "", fmt.Errorf("looking for AWS NLB: %w", err) + } + + latest := FindLatestELBV2ByNameTag(allLoadBalancers, name) + if latest != nil { + return aws.StringValue(latest.LoadBalancer.DNSName), nil } } return "", nil diff --git a/upup/pkg/fi/cloudup/awsup/aws_utils.go b/upup/pkg/fi/cloudup/awsup/aws_utils.go index e1d935cdadd2d..37c6bb0af7a43 100644 --- a/upup/pkg/fi/cloudup/awsup/aws_utils.go +++ b/upup/pkg/fi/cloudup/awsup/aws_utils.go @@ -241,8 +241,8 @@ func GetResourceName32(cluster string, prefix string) string { return truncate.TruncateString(s, opt) } -// GetTargetGroupNameFromARN will attempt to parse a target group ARN and return its name -func GetTargetGroupNameFromARN(targetGroupARN string) (string, error) { +// NameForExternalTargetGroup will attempt to calculate a meaningful name for a target group given an ARN. +func NameForExternalTargetGroup(targetGroupARN string) (string, error) { parsed, err := arn.Parse(targetGroupARN) if err != nil { return "", fmt.Errorf("error parsing target group ARN: %v", err) diff --git a/upup/pkg/fi/cloudup/awsup/elbv2_targetgroups.go b/upup/pkg/fi/cloudup/awsup/elbv2_targetgroups.go index 0417b201bb9eb..89b43c18bdf57 100644 --- a/upup/pkg/fi/cloudup/awsup/elbv2_targetgroups.go +++ b/upup/pkg/fi/cloudup/awsup/elbv2_targetgroups.go @@ -29,12 +29,9 @@ import ( type TargetGroupInfo struct { TargetGroup *elbv2.TargetGroup Tags []*elbv2.Tag - arn string -} -// ARN returns the ARN of the load balancer. -func (i *TargetGroupInfo) ARN() string { - return i.arn + // ARN holds the arn (amazon id) of the target group. + ARN string } // NameTag returns the value of the tag with the key "Name". @@ -72,7 +69,7 @@ func ListELBV2TargetGroups(ctx context.Context, cloud AWSCloud) ([]*TargetGroupI for _, tg := range p.TargetGroups { arn := aws.StringValue(tg.TargetGroupArn) - byARN[arn] = &TargetGroupInfo{TargetGroup: tg, arn: arn} + byARN[arn] = &TargetGroupInfo{TargetGroup: tg, ARN: arn} tagRequest.ResourceArns = append(tagRequest.ResourceArns, tg.TargetGroupArn) } diff --git a/upup/pkg/fi/cloudup/awsup/mock_aws_cloud.go b/upup/pkg/fi/cloudup/awsup/mock_aws_cloud.go index 7a9d75f91155c..9511782e48e88 100644 --- a/upup/pkg/fi/cloudup/awsup/mock_aws_cloud.go +++ b/upup/pkg/fi/cloudup/awsup/mock_aws_cloud.go @@ -211,10 +211,6 @@ func (c *MockAWSCloud) DescribeELBTags(loadBalancerNames []string) (map[string][ return describeELBTags(c, loadBalancerNames) } -func (c *MockAWSCloud) FindELBV2ByNameTag(findNameTag string) (*elbv2.LoadBalancer, error) { - return findELBV2ByNameTag(c, findNameTag) -} - func (c *MockAWSCloud) DescribeELBV2Tags(loadBalancerArns []string) (map[string][]*elbv2.Tag, error) { return describeELBV2Tags(c, loadBalancerArns) } diff --git a/upup/pkg/fi/cloudup/awsup/tags.go b/upup/pkg/fi/cloudup/awsup/tags.go new file mode 100644 index 0000000000000..f41fd804dfdb7 --- /dev/null +++ b/upup/pkg/fi/cloudup/awsup/tags.go @@ -0,0 +1,24 @@ +/* +Copyright 2024 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 awsup + +// KopsResourceRevisionTag is the tag used to store the revision timestamp, +// when we are forced to create a new version of a resource because we cannot modify it in-place. +// This happens when the resource field is immutable; +// it also happens for ELBs, when we cannot have two ELBs pointing at the same target group +// and thus must create a second. +const KopsResourceRevisionTag = "kops.k8s.io/revision" diff --git a/upup/pkg/fi/cloudup/openstacktasks/securitygroup.go b/upup/pkg/fi/cloudup/openstacktasks/securitygroup.go index df3e7618b1cf6..d46e464dad143 100644 --- a/upup/pkg/fi/cloudup/openstacktasks/securitygroup.go +++ b/upup/pkg/fi/cloudup/openstacktasks/securitygroup.go @@ -259,6 +259,10 @@ func (d *deleteSecurityGroup) Item() string { return s } +func (d *deleteSecurityGroup) DeferDeletion() bool { + return false // TODO: Should we defer deletion? +} + type deleteSecurityGroupRule struct { rule sgr.SecGroupRule securityGroup *SecurityGroup @@ -298,6 +302,10 @@ func (d *deleteSecurityGroupRule) Item() string { return s } +func (d *deleteSecurityGroupRule) DeferDeletion() bool { + return false // TODO: Should we defer deletion? +} + // RemovalRule is a rule that filters the permissions we should remove type RemovalRule interface { Matches(sgr.SecGroupRule) bool diff --git a/upup/pkg/fi/default_methods.go b/upup/pkg/fi/default_methods.go index 65ee14047e8cc..8dc0aa7c773e0 100644 --- a/upup/pkg/fi/default_methods.go +++ b/upup/pkg/fi/default_methods.go @@ -20,6 +20,7 @@ import ( "fmt" "reflect" + "k8s.io/klog/v2" "k8s.io/kops/util/pkg/reflectutils" ) @@ -115,6 +116,17 @@ func defaultDeltaRunMethod[T SubContext](e Task[T], c *Context[T]) error { return err } } else { + if deletion.DeferDeletion() { + switch c.deletionProcessingMode { + case DeletionProcessingModeDeleteIfNotDeferrred: + klog.Infof("not deleting %s/%s because it is marked for deferred-deletion", deletion.TaskName(), deletion.Item()) + continue + case DeletionProcessingModeDeleteIncludingDeferred: + klog.V(2).Infof("processing deferred deletion of %s/%s", deletion.TaskName(), deletion.Item()) + default: + klog.Fatalf("unhandled deletionProcessingMode %v", c.deletionProcessingMode) + } + } if err := deletion.Delete(c.Target); err != nil { return err } diff --git a/upup/pkg/fi/deletions.go b/upup/pkg/fi/deletions.go index bc13c6d2ae389..7b2f2558bc55a 100644 --- a/upup/pkg/fi/deletions.go +++ b/upup/pkg/fi/deletions.go @@ -20,11 +20,13 @@ type DeletionProcessingMode string const ( // DeletionProcessingModeIgnore will ignore all deletion tasks. + // This is typically used when the target implements pruning directly (e.g. terraform) DeletionProcessingModeIgnore DeletionProcessingMode = "Ignore" - // TODO: implement deferred-deletion in the tasks! // DeletionProcessingModeDeleteIfNotDeferrred will delete resources only if they are not marked for deferred-deletion. + // This corresponds to a cluster update with --prune=false. DeletionProcessingModeDeleteIfNotDeferrred DeletionProcessingMode = "IfNotDeferred" // DeletionProcessingModeDeleteIncludingDeferrred will delete resources including those marked for deferred-deletion. + // This corresponds to a cluster update with --prune=true. DeletionProcessingModeDeleteIncludingDeferred DeletionProcessingMode = "DeleteIncludingDeferred" ) @@ -38,6 +40,7 @@ type Deletion[T SubContext] interface { Delete(target Target[T]) error TaskName() string Item() string + DeferDeletion() bool } type CloudupDeletion = Deletion[CloudupSubContext] diff --git a/upup/pkg/fi/dryrun_target.go b/upup/pkg/fi/dryrun_target.go index d14ab26a01c17..0f69bbca76ce4 100644 --- a/upup/pkg/fi/dryrun_target.go +++ b/upup/pkg/fi/dryrun_target.go @@ -256,9 +256,30 @@ func (t *DryRunTarget[T]) PrintReport(taskMap map[string]Task[T], out io.Writer) // Give everything a consistent ordering sort.Sort(DeletionByTaskName[T](t.deletions)) - fmt.Fprintf(b, "Will delete items:\n") + var deferred []Deletion[T] + var immediate []Deletion[T] + for _, d := range t.deletions { - fmt.Fprintf(b, " %-20s %s\n", d.TaskName(), d.Item()) + if d.DeferDeletion() { + deferred = append(deferred, d) + } else { + immediate = append(immediate, d) + } + } + + if len(deferred) != 0 { + fmt.Fprintf(b, "Items will be deleted only when the --prune flag is specified:\n") + for _, d := range deferred { + fmt.Fprintf(b, " %-20s %s\n", d.TaskName(), d.Item()) + } + fmt.Fprintf(b, "\n") + } + if len(immediate) != 0 { + fmt.Fprintf(b, "Items will be deleted during update:\n") + for _, d := range immediate { + fmt.Fprintf(b, " %-20s %s\n", d.TaskName(), d.Item()) + } + fmt.Fprintf(b, "\n") } } diff --git a/upup/pkg/fi/topological_sort.go b/upup/pkg/fi/topological_sort.go index 983fc88d33d12..9f1382e5f5fa8 100644 --- a/upup/pkg/fi/topological_sort.go +++ b/upup/pkg/fi/topological_sort.go @@ -66,9 +66,13 @@ func FindTaskDependencies[T SubContext](tasks map[string]Task[T]) map[string][]s var dependencyKeys []string for _, dep := range dependencies { + // Skip nils, including interface nils + if dep == nil || reflect.ValueOf(dep).IsNil() { + continue + } dependencyKey, found := taskToId[dep] if !found { - klog.Fatalf("dependency not found: %v", dep) + klog.Fatalf("dependency for task %T:%q not found: %v", t, k, dep) } dependencyKeys = append(dependencyKeys, dependencyKey) }