From 2a9343a1685ce942c6e7068d5ba66bcff366efe1 Mon Sep 17 00:00:00 2001 From: justinsb Date: Sun, 28 Jan 2024 13:31:01 -0500 Subject: [PATCH] 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 --- cloudmock/aws/mockautoscaling/attach.go | 11 +- cmd/kops/update_cluster.go | 16 + docs/cli/kops_update_cluster.md | 1 + pkg/instancegroups/instancegroups.go | 2 + pkg/model/awsmodel/api_loadbalancer.go | 15 +- pkg/model/awsmodel/autoscalinggroup.go | 2 +- pkg/model/awsmodel/bastion.go | 10 +- pkg/model/awsmodel/spotinst.go | 2 +- pkg/resources/aws/aws.go | 2 +- upup/pkg/fi/cloudup/apply_cluster.go | 5 +- .../fi/cloudup/awstasks/autoscalinggroup.go | 90 +++-- upup/pkg/fi/cloudup/awstasks/dnsname.go | 3 +- .../awstasks/launchtemplate_target_api.go | 6 +- .../cloudup/awstasks/network_load_balancer.go | 334 +++++++++++------- .../networkloadbalancer_attributes.go | 8 +- .../awstasks/networkloadbalancerlistener.go | 5 +- upup/pkg/fi/cloudup/awstasks/securitygroup.go | 18 +- upup/pkg/fi/cloudup/awstasks/subnet.go | 4 + upup/pkg/fi/cloudup/awstasks/targetgroup.go | 193 +++++++++- upup/pkg/fi/cloudup/awstasks/vpc.go | 4 + upup/pkg/fi/cloudup/awsup/aws_cloud.go | 94 ++--- upup/pkg/fi/cloudup/awsup/aws_utils.go | 4 +- .../fi/cloudup/awsup/elbv2_targetgroups.go | 9 +- upup/pkg/fi/cloudup/awsup/mock_aws_cloud.go | 4 - upup/pkg/fi/cloudup/awsup/tags.go | 24 ++ .../cloudup/openstacktasks/securitygroup.go | 8 + upup/pkg/fi/default_methods.go | 12 + upup/pkg/fi/deletions.go | 5 +- upup/pkg/fi/dryrun_target.go | 25 +- upup/pkg/fi/topological_sort.go | 6 +- 30 files changed, 644 insertions(+), 278 deletions(-) create mode 100644 upup/pkg/fi/cloudup/awsup/tags.go 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) }