From ffd52cac4081ac0ad244d8572a1df8698fddc86b Mon Sep 17 00:00:00 2001 From: justinsb Date: Mon, 29 Jan 2024 08:35:31 -0500 Subject: [PATCH] refactor: Introduce DeletionProcessingMode Deletion processing is not entirely a factor of the target, it is more a factor of our mode of execution (dry-run vs pre-rolling-update vs post-rolling-update). We want to introduce that post-rolling-update phase, so introduce the DeletionProcessingMode enum and move it from the target to the context. --- upup/pkg/fi/cloudup/apply_cluster.go | 6 +++++- .../egressonlyinternetgateway_test.go | 2 +- .../fi/cloudup/awstasks/elastic_ip_test.go | 2 +- upup/pkg/fi/cloudup/awsup/aws_apitarget.go | 4 ---- upup/pkg/fi/cloudup/azure/azure_apitarget.go | 5 ----- upup/pkg/fi/cloudup/do/api_target.go | 4 ---- upup/pkg/fi/cloudup/gce/gce_apitarget.go | 4 ---- .../cloudup/gcetasks/serviceaccount_test.go | 4 ++-- upup/pkg/fi/cloudup/hetzner/api_target.go | 4 ---- upup/pkg/fi/cloudup/openstack/apitarget.go | 4 ---- upup/pkg/fi/cloudup/scaleway/api_target.go | 4 ---- upup/pkg/fi/cloudup/terraform/target.go | 5 ----- upup/pkg/fi/context.go | 21 ++++++++++++++----- upup/pkg/fi/default_methods.go | 18 +++++++--------- upup/pkg/fi/deletions.go | 12 +++++++++++ upup/pkg/fi/dryrun_target.go | 7 +------ upup/pkg/fi/nodeup/install/install_target.go | 5 ----- upup/pkg/fi/nodeup/local/local_target.go | 5 ----- upup/pkg/fi/target.go | 4 ---- 19 files changed, 46 insertions(+), 74 deletions(-) diff --git a/upup/pkg/fi/cloudup/apply_cluster.go b/upup/pkg/fi/cloudup/apply_cluster.go index 0918894b1c361..a3db3a1a3d978 100644 --- a/upup/pkg/fi/cloudup/apply_cluster.go +++ b/upup/pkg/fi/cloudup/apply_cluster.go @@ -714,6 +714,7 @@ func (c *ApplyClusterCmd) Run(ctx context.Context) error { var target fi.CloudupTarget shouldPrecreateDNS := true + deletionProcessingMode := fi.DeletionProcessingModeDeleteIfNotDeferrred switch c.TargetName { case TargetDirect: switch cluster.Spec.GetCloudProvider() { @@ -764,6 +765,9 @@ func (c *ApplyClusterCmd) Run(ctx context.Context) error { // Can cause conflicts with terraform management shouldPrecreateDNS = false + // Terraform tracks & performs deletions itself + deletionProcessingMode = fi.DeletionProcessingModeIgnore + case TargetDryRun: var out io.Writer = os.Stdout if c.GetAssets { @@ -786,7 +790,7 @@ func (c *ApplyClusterCmd) Run(ctx context.Context) error { } } - context, err := fi.NewCloudupContext(ctx, target, cluster, cloud, keyStore, secretStore, configBase, c.TaskMap) + context, err := fi.NewCloudupContext(ctx, deletionProcessingMode, target, cluster, cloud, keyStore, secretStore, configBase, c.TaskMap) if err != nil { return fmt.Errorf("error building context: %v", err) } diff --git a/upup/pkg/fi/cloudup/awstasks/egressonlyinternetgateway_test.go b/upup/pkg/fi/cloudup/awstasks/egressonlyinternetgateway_test.go index 65230e2835e05..bd800c24fc963 100644 --- a/upup/pkg/fi/cloudup/awstasks/egressonlyinternetgateway_test.go +++ b/upup/pkg/fi/cloudup/awstasks/egressonlyinternetgateway_test.go @@ -142,7 +142,7 @@ func runTasks(t *testing.T, cloud awsup.AWSCloud, allTasks map[string]fi.Cloudup Cloud: cloud, } - context, err := fi.NewCloudupContext(ctx, target, nil, cloud, nil, nil, nil, allTasks) + context, err := fi.NewCloudupContext(ctx, fi.DeletionProcessingModeDeleteIncludingDeferred, target, nil, cloud, nil, nil, nil, allTasks) if err != nil { t.Fatalf("error building context: %v", err) } diff --git a/upup/pkg/fi/cloudup/awstasks/elastic_ip_test.go b/upup/pkg/fi/cloudup/awstasks/elastic_ip_test.go index 61d8b6f0f45b2..84a7c7b8d4ffb 100644 --- a/upup/pkg/fi/cloudup/awstasks/elastic_ip_test.go +++ b/upup/pkg/fi/cloudup/awstasks/elastic_ip_test.go @@ -119,7 +119,7 @@ func checkNoChanges(t *testing.T, ctx context.Context, cloud fi.Cloud, allTasks } assetBuilder := assets.NewAssetBuilder(vfs.Context, cluster.Spec.Assets, cluster.Spec.KubernetesVersion, false) target := fi.NewCloudupDryRunTarget(assetBuilder, os.Stderr) - context, err := fi.NewCloudupContext(ctx, target, nil, cloud, nil, nil, nil, allTasks) + context, err := fi.NewCloudupContext(ctx, fi.DeletionProcessingModeDeleteIncludingDeferred, target, nil, cloud, nil, nil, nil, allTasks) if err != nil { t.Fatalf("error building context: %v", err) } diff --git a/upup/pkg/fi/cloudup/awsup/aws_apitarget.go b/upup/pkg/fi/cloudup/awsup/aws_apitarget.go index 473003d247798..c1b108f2b66fa 100644 --- a/upup/pkg/fi/cloudup/awsup/aws_apitarget.go +++ b/upup/pkg/fi/cloudup/awsup/aws_apitarget.go @@ -38,10 +38,6 @@ func NewAWSAPITarget(cloud AWSCloud) *AWSAPITarget { } } -func (t *AWSAPITarget) ProcessDeletions() bool { - return true -} - func (t *AWSAPITarget) DefaultCheckExisting() bool { return true } diff --git a/upup/pkg/fi/cloudup/azure/azure_apitarget.go b/upup/pkg/fi/cloudup/azure/azure_apitarget.go index 2bec93524156a..e3ea02c42d07c 100644 --- a/upup/pkg/fi/cloudup/azure/azure_apitarget.go +++ b/upup/pkg/fi/cloudup/azure/azure_apitarget.go @@ -40,11 +40,6 @@ func (t *AzureAPITarget) Finish(taskMap map[string]fi.CloudupTask) error { return nil } -// ProcessDeletions returns true if we should delete resources. -func (t *AzureAPITarget) ProcessDeletions() bool { - return true -} - func (t *AzureAPITarget) DefaultCheckExisting() bool { return true } diff --git a/upup/pkg/fi/cloudup/do/api_target.go b/upup/pkg/fi/cloudup/do/api_target.go index ce289cab673dc..cc63b7a5edcca 100644 --- a/upup/pkg/fi/cloudup/do/api_target.go +++ b/upup/pkg/fi/cloudup/do/api_target.go @@ -36,10 +36,6 @@ func (t *DOAPITarget) Finish(taskMap map[string]fi.CloudupTask) error { return nil } -func (t *DOAPITarget) ProcessDeletions() bool { - return true -} - func (t *DOAPITarget) DefaultCheckExisting() bool { return true } diff --git a/upup/pkg/fi/cloudup/gce/gce_apitarget.go b/upup/pkg/fi/cloudup/gce/gce_apitarget.go index 3475bf6a4d494..65511cf1581e0 100644 --- a/upup/pkg/fi/cloudup/gce/gce_apitarget.go +++ b/upup/pkg/fi/cloudup/gce/gce_apitarget.go @@ -36,10 +36,6 @@ func (t *GCEAPITarget) Finish(taskMap map[string]fi.CloudupTask) error { return nil } -func (t *GCEAPITarget) ProcessDeletions() bool { - return true -} - func (t *GCEAPITarget) DefaultCheckExisting() bool { return true } diff --git a/upup/pkg/fi/cloudup/gcetasks/serviceaccount_test.go b/upup/pkg/fi/cloudup/gcetasks/serviceaccount_test.go index 98e4ad47b8574..ca1e4e5131ece 100644 --- a/upup/pkg/fi/cloudup/gcetasks/serviceaccount_test.go +++ b/upup/pkg/fi/cloudup/gcetasks/serviceaccount_test.go @@ -101,7 +101,7 @@ func checkHasChanges(t *testing.T, ctx context.Context, cloud fi.Cloud, allTasks func runTasks(t *testing.T, ctx context.Context, cloud gce.GCECloud, allTasks map[string]fi.CloudupTask) { target := gce.NewGCEAPITarget(cloud) - context, err := fi.NewCloudupContext(ctx, target, nil, cloud, nil, nil, nil, allTasks) + context, err := fi.NewCloudupContext(ctx, fi.DeletionProcessingModeDeleteIncludingDeferred, target, nil, cloud, nil, nil, nil, allTasks) if err != nil { t.Fatalf("error building context: %v", err) } @@ -120,7 +120,7 @@ func doDryRun(t *testing.T, ctx context.Context, cloud fi.Cloud, allTasks map[st } assetBuilder := assets.NewAssetBuilder(vfs.Context, cluster.Spec.Assets, cluster.Spec.KubernetesVersion, false) target := fi.NewCloudupDryRunTarget(assetBuilder, os.Stderr) - context, err := fi.NewCloudupContext(ctx, target, nil, cloud, nil, nil, nil, allTasks) + context, err := fi.NewCloudupContext(ctx, fi.DeletionProcessingModeDeleteIncludingDeferred, target, nil, cloud, nil, nil, nil, allTasks) if err != nil { t.Fatalf("error building context: %v", err) } diff --git a/upup/pkg/fi/cloudup/hetzner/api_target.go b/upup/pkg/fi/cloudup/hetzner/api_target.go index 1668ebdee6805..235bdac769078 100644 --- a/upup/pkg/fi/cloudup/hetzner/api_target.go +++ b/upup/pkg/fi/cloudup/hetzner/api_target.go @@ -36,10 +36,6 @@ func (t *HetznerAPITarget) Finish(taskMap map[string]fi.CloudupTask) error { return nil } -func (t *HetznerAPITarget) ProcessDeletions() bool { - return true -} - func (t *HetznerAPITarget) DefaultCheckExisting() bool { return true } diff --git a/upup/pkg/fi/cloudup/openstack/apitarget.go b/upup/pkg/fi/cloudup/openstack/apitarget.go index 732ca4c06a55b..58a8f528f6f99 100644 --- a/upup/pkg/fi/cloudup/openstack/apitarget.go +++ b/upup/pkg/fi/cloudup/openstack/apitarget.go @@ -36,10 +36,6 @@ func (t *OpenstackAPITarget) Finish(taskMap map[string]fi.CloudupTask) error { return nil } -func (t *OpenstackAPITarget) ProcessDeletions() bool { - return true -} - func (t *OpenstackAPITarget) DefaultCheckExisting() bool { return true } diff --git a/upup/pkg/fi/cloudup/scaleway/api_target.go b/upup/pkg/fi/cloudup/scaleway/api_target.go index 269003698030c..f98a5702cd112 100644 --- a/upup/pkg/fi/cloudup/scaleway/api_target.go +++ b/upup/pkg/fi/cloudup/scaleway/api_target.go @@ -34,10 +34,6 @@ func (s ScwAPITarget) Finish(taskMap map[string]fi.CloudupTask) error { return nil } -func (s ScwAPITarget) ProcessDeletions() bool { - return true -} - func (t *ScwAPITarget) DefaultCheckExisting() bool { return true } diff --git a/upup/pkg/fi/cloudup/terraform/target.go b/upup/pkg/fi/cloudup/terraform/target.go index 2d861b79e5356..292b6b7934e99 100644 --- a/upup/pkg/fi/cloudup/terraform/target.go +++ b/upup/pkg/fi/cloudup/terraform/target.go @@ -63,11 +63,6 @@ func (t *TerraformTarget) AddFileResource(resourceType string, resourceName stri return t.AddFileBytes(resourceType, resourceName, key, d, base64) } -func (t *TerraformTarget) ProcessDeletions() bool { - // Terraform tracks & performs deletions itself - return false -} - func (t *TerraformTarget) DefaultCheckExisting() bool { return false } diff --git a/upup/pkg/fi/context.go b/upup/pkg/fi/context.go index 4c50c4908dc63..03e81c2f0050c 100644 --- a/upup/pkg/fi/context.go +++ b/upup/pkg/fi/context.go @@ -38,6 +38,8 @@ type Context[T SubContext] struct { tasks map[string]Task[T] warnings []*Warning[T] + deletionProcessingMode DeletionProcessingMode + T T } @@ -74,31 +76,40 @@ type Warning[T SubContext] struct { Message string } -func newContext[T SubContext](ctx context.Context, target Target[T], sub T, tasks map[string]Task[T]) (*Context[T], error) { +func newContext[T SubContext](ctx context.Context, deletionProcessingMode DeletionProcessingMode, target Target[T], sub T, tasks map[string]Task[T]) (*Context[T], error) { c := &Context[T]{ ctx: ctx, Target: target, tasks: tasks, T: sub, + + deletionProcessingMode: deletionProcessingMode, } return c, nil } func NewInstallContext(ctx context.Context, target InstallTarget, tasks map[string]InstallTask) (*InstallContext, error) { + // We don't expect deletions, but we would be the one to handle them. + deletionProcessingMode := DeletionProcessingModeDeleteIncludingDeferred + sub := InstallSubContext{} - return newContext[InstallSubContext](ctx, target, sub, tasks) + return newContext[InstallSubContext](ctx, deletionProcessingMode, target, sub, tasks) } + func NewNodeupContext(ctx context.Context, target NodeupTarget, keystore KeystoreReader, bootConfig *nodeup.BootConfig, nodeupConfig *nodeup.Config, tasks map[string]NodeupTask) (*NodeupContext, error) { + // We don't expect deletions, but we would be the one to handle them. + deletionProcessingMode := DeletionProcessingModeDeleteIncludingDeferred + sub := NodeupSubContext{ BootConfig: bootConfig, NodeupConfig: nodeupConfig, Keystore: keystore, } - return newContext[NodeupSubContext](ctx, target, sub, tasks) + return newContext[NodeupSubContext](ctx, deletionProcessingMode, target, sub, tasks) } -func NewCloudupContext(ctx context.Context, target CloudupTarget, cluster *kops.Cluster, cloud Cloud, keystore Keystore, secretStore SecretStore, clusterConfigBase vfs.Path, tasks map[string]CloudupTask) (*CloudupContext, error) { +func NewCloudupContext(ctx context.Context, deletionProcessingMode DeletionProcessingMode, target CloudupTarget, cluster *kops.Cluster, cloud Cloud, keystore Keystore, secretStore SecretStore, clusterConfigBase vfs.Path, tasks map[string]CloudupTask) (*CloudupContext, error) { sub := CloudupSubContext{ Cloud: cloud, Cluster: cluster, @@ -106,7 +117,7 @@ func NewCloudupContext(ctx context.Context, target CloudupTarget, cluster *kops. Keystore: keystore, SecretStore: secretStore, } - return newContext[CloudupSubContext](ctx, target, sub, tasks) + return newContext[CloudupSubContext](ctx, deletionProcessingMode, target, sub, tasks) } func (c *Context[T]) AllTasks() map[string]Task[T] { diff --git a/upup/pkg/fi/default_methods.go b/upup/pkg/fi/default_methods.go index 8fc142ab4b370..65ee14047e8cc 100644 --- a/upup/pkg/fi/default_methods.go +++ b/upup/pkg/fi/default_methods.go @@ -104,22 +104,20 @@ func defaultDeltaRunMethod[T SubContext](e Task[T], c *Context[T]) error { } } - if producesDeletions, ok := e.(ProducesDeletions[T]); ok && c.Target.ProcessDeletions() { - var deletions []Deletion[T] - deletions, err = producesDeletions.FindDeletions(c) + if producesDeletions, ok := e.(ProducesDeletions[T]); ok && c.deletionProcessingMode != DeletionProcessingModeIgnore { + deletions, err := producesDeletions.FindDeletions(c) if err != nil { return err } for _, deletion := range deletions { if _, ok := c.Target.(*DryRunTarget[T]); ok { - err = c.Target.(*DryRunTarget[T]).Delete(deletion) - } else if _, ok := c.Target.(*DryRunTarget[T]); ok { - err = c.Target.(*DryRunTarget[T]).Delete(deletion) + if err := c.Target.(*DryRunTarget[T]).RecordDeletion(deletion); err != nil { + return err + } } else { - err = deletion.Delete(c.Target) - } - if err != nil { - return err + 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 4e2a3e38512d5..bc13c6d2ae389 100644 --- a/upup/pkg/fi/deletions.go +++ b/upup/pkg/fi/deletions.go @@ -16,6 +16,18 @@ limitations under the License. package fi +type DeletionProcessingMode string + +const ( + // DeletionProcessingModeIgnore will ignore all deletion tasks. + DeletionProcessingModeIgnore DeletionProcessingMode = "Ignore" + // TODO: implement deferred-deletion in the tasks! + // DeletionProcessingModeDeleteIfNotDeferrred will delete resources only if they are not marked for deferred-deletion. + DeletionProcessingModeDeleteIfNotDeferrred DeletionProcessingMode = "IfNotDeferred" + // DeletionProcessingModeDeleteIncludingDeferrred will delete resources including those marked for deferred-deletion. + DeletionProcessingModeDeleteIncludingDeferred DeletionProcessingMode = "DeleteIncludingDeferred" +) + type ProducesDeletions[T SubContext] interface { FindDeletions(*Context[T]) ([]Deletion[T], error) } diff --git a/upup/pkg/fi/dryrun_target.go b/upup/pkg/fi/dryrun_target.go index e2be535b43371..d14ab26a01c17 100644 --- a/upup/pkg/fi/dryrun_target.go +++ b/upup/pkg/fi/dryrun_target.go @@ -92,11 +92,6 @@ func NewNodeupDryRunTarget(assetBuilder *assets.AssetBuilder, out io.Writer) *No return newDryRunTarget[NodeupSubContext](assetBuilder, out) } -func (t *DryRunTarget[T]) ProcessDeletions() bool { - // We display deletions - return true -} - func (t *DryRunTarget[T]) DefaultCheckExisting() bool { return true } @@ -117,7 +112,7 @@ func (t *DryRunTarget[T]) Render(a, e, changes Task[T]) error { return nil } -func (t *DryRunTarget[T]) Delete(deletion Deletion[T]) error { +func (t *DryRunTarget[T]) RecordDeletion(deletion Deletion[T]) error { t.mutex.Lock() defer t.mutex.Unlock() diff --git a/upup/pkg/fi/nodeup/install/install_target.go b/upup/pkg/fi/nodeup/install/install_target.go index 08de4ac4f94e0..2fc966e77d9da 100644 --- a/upup/pkg/fi/nodeup/install/install_target.go +++ b/upup/pkg/fi/nodeup/install/install_target.go @@ -31,11 +31,6 @@ func (t *InstallTarget) Finish(taskMap map[string]fi.InstallTask) error { return nil } -func (t *InstallTarget) ProcessDeletions() bool { - // We don't expect any, but it would be our job to process them - return true -} - func (t *InstallTarget) DefaultCheckExisting() bool { return true } diff --git a/upup/pkg/fi/nodeup/local/local_target.go b/upup/pkg/fi/nodeup/local/local_target.go index d9efabb023aa2..dcf296bef8301 100644 --- a/upup/pkg/fi/nodeup/local/local_target.go +++ b/upup/pkg/fi/nodeup/local/local_target.go @@ -33,11 +33,6 @@ func (t *LocalTarget) Finish(taskMap map[string]fi.NodeupTask) error { return nil } -func (t *LocalTarget) ProcessDeletions() bool { - // We don't expect any, but it would be our job to process them - return true -} - func (t *LocalTarget) DefaultCheckExisting() bool { return true } diff --git a/upup/pkg/fi/target.go b/upup/pkg/fi/target.go index 66929fc5650e0..68d2365ce85af 100644 --- a/upup/pkg/fi/target.go +++ b/upup/pkg/fi/target.go @@ -20,10 +20,6 @@ type Target[T SubContext] interface { // Lifecycle methods, called by the driver Finish(taskMap map[string]Task[T]) error - // ProcessDeletions returns true if we should delete resources - // Some providers (e.g. Terraform) actively keep state, and will delete resources automatically - ProcessDeletions() bool - // DefaultCheckExisting returns true if DefaultDeltaRun tasks which aren't HasCheckExisting // should invoke Find() when running against this Target. DefaultCheckExisting() bool