Merge pull request #16293 from justinsb/deletion_processing_mode

refactor: Introduce DeletionProcessingMode

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)
 	}

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)
 	}

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)
 	}

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
 }

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
 }

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
 }

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
 }

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)
 	}

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
 }

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
 }

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
 }

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
 }

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] {

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() {
+	if producesDeletions, ok := e.(ProducesDeletions[T]); ok && c.deletionProcessingMode != DeletionProcessingModeIgnore {
-		var deletions []Deletion[T]
+		deletions, err := producesDeletions.FindDeletions(c)
-		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)
+				if err := c.Target.(*DryRunTarget[T]).RecordDeletion(deletion); err != nil {
-			} else if _, ok := c.Target.(*DryRunTarget[T]); ok {
+					return err
-				err = c.Target.(*DryRunTarget[T]).Delete(deletion)
+				}
 			} else {
-				err = deletion.Delete(c.Target)
+				if err := deletion.Delete(c.Target); err != nil {
-			}
+					return err
-			if err != nil {
+				}
-				return err
 			}
 		}
 	}

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)
 }

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()
 

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
 }

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
 }

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