Repository: crossplane-contrib/provider-terraform (go)

Issue:
Fix for Secret/Lease deletion needs to be reworked
### What happened?
The fix for #40 needs to be reworked since it deletes the Secret/Lease before the reconciliation loop has finished with the Managed Resource.  That causes terraform init errors and leaves the Workspace objects hung.

### What environment did it happen in?
Crossplane version: 1.7


Pull request:
Refactor delete handling to remove the workspace
Signed-off-by: Bob Haddleton <bob.haddleton@nokia.com>

### Description of your changes
Refactor the delete handling code to use "terraform workspace delete" to remove the workspace cleanly.

Fixes #55 

I have:

- [X] Read and followed Crossplane's [contribution process].
- [X] Run `make reviewable test` to ensure this PR is ready for review.

### How has this code been tested
The code ran in my local environment for a week with no issues.

[contribution process]: https://git.io/fj2m9


Patch:
diff --git a/internal/controller/workspace/workspace.go b/internal/controller/workspace/workspace.go
index 713b171..b86bdff 100644
--- a/internal/controller/workspace/workspace.go
+++ b/internal/controller/workspace/workspace.go
@@ -24,7 +24,6 @@ import (
 
 	"github.com/pkg/errors"
 	"github.com/spf13/afero"
-	coordv1 "k8s.io/api/coordination/v1"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/util/workqueue"
@@ -53,25 +52,22 @@ const (
 	errGetPC        = "cannot get ProviderConfig"
 	errGetCreds     = "cannot get credentials"
 
-	errMkdir         = "cannot make Terraform configuration directory"
-	errRemoteModule  = "cannot get remote Terraform module"
-	errWriteCreds    = "cannot write Terraform credentials"
-	errWriteGitCreds = "cannot write .git-credentials to /tmp dir"
-	errWriteConfig   = "cannot write Terraform configuration " + tfConfig
-	errWriteMain     = "cannot write Terraform configuration " + tfMain
-	errInit          = "cannot initialize Terraform configuration"
-	errWorkspace     = "cannot select Terraform workspace"
-	errResources     = "cannot list Terraform resources"
-	errDiff          = "cannot diff (i.e. plan) Terraform configuration"
-	errOutputs       = "cannot list Terraform outputs"
-	errOptions       = "cannot determine Terraform options"
-	errApply         = "cannot apply Terraform configuration"
-	errDestroy       = "cannot destroy Terraform configuration"
-	errVarFile       = "cannot get tfvars"
-	errListLeases    = "cannot get list of Lease objects"
-	errListSecrets   = "cannot get list of Secret objects"
-	errDeleteSecret  = "cannot delete Secret for Workspace"
-	errDeleteLease   = "cannot delete Least for Workspace"
+	errMkdir           = "cannot make Terraform configuration directory"
+	errRemoteModule    = "cannot get remote Terraform module"
+	errWriteCreds      = "cannot write Terraform credentials"
+	errWriteGitCreds   = "cannot write .git-credentials to /tmp dir"
+	errWriteConfig     = "cannot write Terraform configuration " + tfConfig
+	errWriteMain       = "cannot write Terraform configuration " + tfMain
+	errInit            = "cannot initialize Terraform configuration"
+	errWorkspace       = "cannot select Terraform workspace"
+	errResources       = "cannot list Terraform resources"
+	errDiff            = "cannot diff (i.e. plan) Terraform configuration"
+	errOutputs         = "cannot list Terraform outputs"
+	errOptions         = "cannot determine Terraform options"
+	errApply           = "cannot apply Terraform configuration"
+	errDestroy         = "cannot destroy Terraform configuration"
+	errVarFile         = "cannot get tfvars"
+	errDeleteWorkspace = "cannot delete Terraform workspace"
 
 	gitCredentialsFilename = ".git-credentials"
 )
@@ -92,6 +88,7 @@ type tfclient interface {
 	Diff(ctx context.Context, o ...terraform.Option) (bool, error)
 	Apply(ctx context.Context, o ...terraform.Option) error
 	Destroy(ctx context.Context, o ...terraform.Option) error
+	DeleteCurrentWorkspace(ctx context.Context) error
 }
 
 // Setup adds a controller that reconciles Workspace managed resources.
@@ -263,20 +260,23 @@ func (c *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex
 	o = append(o, terraform.WithArgs(cr.Spec.ForProvider.PlanArgs))
 	differs, err := c.tf.Diff(ctx, o...)
 	if err != nil {
-		if meta.WasDeleted(mg) {
-			if derr := c.Delete(ctx, mg); derr != nil {
-				return managed.ExternalObservation{}, errors.Wrap(derr, errDiff)
-			}
-			return managed.ExternalObservation{ResourceExists: false}, nil
+		if !meta.WasDeleted(mg) {
+			return managed.ExternalObservation{}, errors.Wrap(err, errDiff)
 		}
-		return managed.ExternalObservation{}, errors.Wrap(err, errDiff)
+		// terraform plan can fail on deleted resources, so let the reconciliation loop
+		// call Delete() if there are still resources in the tfstate file
+		differs = false
 	}
-
 	r, err := c.tf.Resources(ctx)
 	if err != nil {
 		return managed.ExternalObservation{}, errors.Wrap(err, errResources)
 	}
-
+	if meta.WasDeleted(cr) && len(r) == 0 {
+		// The CR was deleted and there are no more terraform resources so the workspace can be deleted
+		if err = c.tf.DeleteCurrentWorkspace(ctx); err != nil {
+			return managed.ExternalObservation{}, errors.Wrap(err, errDeleteWorkspace)
+		}
+	}
 	// Include any non-sensitive outputs in our status
 	op, err := c.tf.Outputs(ctx)
 
@@ -339,31 +339,7 @@ func (c *external) Delete(ctx context.Context, mg resource.Managed) error {
 	}
 
 	o = append(o, terraform.WithArgs(cr.Spec.ForProvider.DestroyArgs))
-	if err := c.tf.Destroy(ctx, o...); err != nil {
-		return errors.Wrap(err, errDestroy)
-	}
-	labels := map[string]string{"tfstate": "true", "tfstateWorkspace": cr.Name}
-	sl := &corev1.SecretList{}
-	if err := c.kube.List(ctx, sl, client.MatchingLabels(labels)); err != nil {
-		return errors.Wrap(err, errListSecrets)
-	}
-	for s := range sl.Items {
-		sec := sl.Items[s]
-		if err := c.kube.Delete(ctx, &sec); err != nil {
-			return errors.Wrap(err, errDeleteSecret)
-		}
-	}
-	ll := &coordv1.LeaseList{}
-	if err := c.kube.List(ctx, ll, client.MatchingLabels(labels)); err != nil {
-		return errors.Wrap(err, errListLeases)
-	}
-	for l := range ll.Items {
-		ls := ll.Items[l]
-		if err := c.kube.Delete(ctx, &ls); err != nil {
-			return errors.Wrap(err, errDeleteLease)
-		}
-	}
-	return nil
+	return errors.Wrap(c.tf.Destroy(ctx, o...), errDestroy)
 }
 
 func (c *external) options(ctx context.Context, p v1alpha1.WorkspaceParameters) ([]terraform.Option, error) {
diff --git a/internal/terraform/terraform.go b/internal/terraform/terraform.go
index fe2a1eb..dfca627 100644
--- a/internal/terraform/terraform.go
+++ b/internal/terraform/terraform.go
@@ -39,6 +39,8 @@ const (
 	errWriteVarFile = "cannot write tfvars file"
 
 	errFmtInvalidConfig = "invalid Terraform configuration: found %d errors"
+
+	tfDefault = "default"
 )
 
 const varFilePrefix = "crossplane-provider-terraform-"
@@ -185,6 +187,37 @@ func (h Harness) Workspace(ctx context.Context, name string) error {
 	return Classify(err)
 }
 
+// DeleteCurrentWorkspace deletes the current Terraform workspace if it is not the default.
+func (h Harness) DeleteCurrentWorkspace(ctx context.Context) error {
+	cmd := exec.CommandContext(ctx, h.Path, "workspace", "show", "-no-color") //nolint:gosec
+	cmd.Dir = h.Dir
+
+	n, err := cmd.Output()
+	if err != nil {
+		return Classify(err)
+	}
+	name := strings.TrimSuffix(string(n), "\n")
+	if name == tfDefault {
+		return nil
+	}
+
+	// Switch to the default workspace
+	err = h.Workspace(ctx, tfDefault)
+	if err != nil {
+		return Classify(err)
+	}
+	cmd = exec.CommandContext(ctx, h.Path, "workspace", "delete", "-no-color", name) //nolint:gosec
+	cmd.Dir = h.Dir
+
+	_, err = cmd.Output()
+	if err == nil {
+		// We successfully deleted the workspace; we're done.
+		return nil
+	}
+	// TODO(bobh66) The working directory could be deleted here instead of waiting for GC to clean it up
+	return Classify(err)
+}
+
 // An OutputType of Terraform.
 type OutputType int
 


Test patch:
diff --git a/internal/controller/workspace/workspace_test.go b/internal/controller/workspace/workspace_test.go
index 7fea723..9788046 100644
--- a/internal/controller/workspace/workspace_test.go
+++ b/internal/controller/workspace/workspace_test.go
@@ -61,13 +61,14 @@ func (e *ErrFs) OpenFile(name string, flag int, perm os.FileMode) (afero.File, e
 }
 
 type MockTf struct {
-	MockInit      func(ctx context.Context, o ...terraform.InitOption) error
-	MockWorkspace func(ctx context.Context, name string) error
-	MockOutputs   func(ctx context.Context) ([]terraform.Output, error)
-	MockResources func(ctx context.Context) ([]string, error)
-	MockDiff      func(ctx context.Context, o ...terraform.Option) (bool, error)
-	MockApply     func(ctx context.Context, o ...terraform.Option) error
-	MockDestroy   func(ctx context.Context, o ...terraform.Option) error
+	MockInit                   func(ctx context.Context, o ...terraform.InitOption) error
+	MockWorkspace              func(ctx context.Context, name string) error
+	MockOutputs                func(ctx context.Context) ([]terraform.Output, error)
+	MockResources              func(ctx context.Context) ([]string, error)
+	MockDiff                   func(ctx context.Context, o ...terraform.Option) (bool, error)
+	MockApply                  func(ctx context.Context, o ...terraform.Option) error
+	MockDestroy                func(ctx context.Context, o ...terraform.Option) error
+	MockDeleteCurrentWorkspace func(ctx context.Context) error
 }
 
 func (tf *MockTf) Init(ctx context.Context, o ...terraform.InitOption) error {
@@ -98,6 +99,10 @@ func (tf *MockTf) Destroy(ctx context.Context, o ...terraform.Option) error {
 	return tf.MockDestroy(ctx, o...)
 }
 
+func (tf *MockTf) DeleteCurrentWorkspace(ctx context.Context) error {
+	return tf.MockDeleteCurrentWorkspace(ctx)
+}
+
 func TestConnect(t *testing.T) {
 	errBoom := errors.New("boom")
 	uid := types.UID("no-you-id")
@@ -477,7 +482,6 @@ func TestConnect(t *testing.T) {
 func TestObserve(t *testing.T) {
 	errBoom := errors.New("boom")
 	now := metav1.Now()
-
 	type fields struct {
 		tf   tfclient
 		kube client.Client
@@ -583,17 +587,43 @@ func TestObserve(t *testing.T) {
 				err: errors.Wrap(errBoom, errDiff),
 			},
 		},
-		"DiffErrorDeleted": {
-			reason: "We should call the Delete function when an error is encountered while diffing the Terraform configuration on a deleted Workspace",
+		"DiffErrorDeletedWithExistingResources": {
+			reason: "We should return ResourceUpToDate true when resource is deleted and there are existing resources but terraform plan fails",
 			fields: fields{
 				tf: &MockTf{
-					MockDestroy: func(_ context.Context, _ ...terraform.Option) error { return nil },
 					MockDiff:    func(ctx context.Context, o ...terraform.Option) (bool, error) { return false, errBoom },
+					MockOutputs: func(ctx context.Context) ([]terraform.Output, error) { return nil, nil },
+					MockResources: func(ctx context.Context) ([]string, error) {
+						return []string{"cool_resource.very"}, nil
+					},
 				},
-				kube: &test.MockClient{
-					MockDelete: test.NewMockDeleteFn(nil),
-					MockGet:    test.NewMockGetFn(nil),
-					MockList:   test.NewMockListFn(nil),
+			},
+			args: args{
+				mg: &v1alpha1.Workspace{
+					ObjectMeta: metav1.ObjectMeta{
+						DeletionTimestamp: &now,
+					},
+				},
+			},
+			want: want{
+				o: managed.ExternalObservation{
+					ResourceExists:    true,
+					ResourceUpToDate:  true,
+					ConnectionDetails: managed.ConnectionDetails{},
+				},
+				wo: v1alpha1.WorkspaceObservation{
+					Outputs: map[string]string{},
+				},
+			},
+		},
+		"DiffErrorDeletedWithoutExistingResources": {
+			reason: "We should return ResourceUpToDate true when resource is deleted and there are no existing resources and terraform plan fails",
+			fields: fields{
+				tf: &MockTf{
+					MockDiff:                   func(ctx context.Context, o ...terraform.Option) (bool, error) { return false, errBoom },
+					MockOutputs:                func(ctx context.Context) ([]terraform.Output, error) { return nil, nil },
+					MockResources:              func(ctx context.Context) ([]string, error) { return nil, nil },
+					MockDeleteCurrentWorkspace: func(ctx context.Context) error { return nil },
 				},
 			},
 			args: args{
@@ -604,15 +634,23 @@ func TestObserve(t *testing.T) {
 				},
 			},
 			want: want{
-				o: managed.ExternalObservation{ResourceExists: false},
+				o: managed.ExternalObservation{
+					ResourceExists:    false,
+					ResourceUpToDate:  true,
+					ConnectionDetails: managed.ConnectionDetails{},
+				},
+				wo: v1alpha1.WorkspaceObservation{
+					Outputs: map[string]string{},
+				},
 			},
 		},
-		"DiffErrorDeletedDestroyError": {
-			reason: "We should raise an error when the Delete function fails after Diff fails on a deleted resource",
+		"DiffErrorDeletedWithoutExistingResourcesWorkspaceDeleteError": {
+			reason: "We should return ResourceUpToDate true when resource is deleted and there are no existing resources and terraform plan fails",
 			fields: fields{
 				tf: &MockTf{
-					MockDestroy: func(_ context.Context, _ ...terraform.Option) error { return errBoom },
-					MockDiff:    func(ctx context.Context, o ...terraform.Option) (bool, error) { return false, errBoom },
+					MockDiff:                   func(ctx context.Context, o ...terraform.Option) (bool, error) { return false, errBoom },
+					MockResources:              func(ctx context.Context) ([]string, error) { return nil, nil },
+					MockDeleteCurrentWorkspace: func(ctx context.Context) error { return errBoom },
 				},
 			},
 			args: args{
@@ -623,8 +661,7 @@ func TestObserve(t *testing.T) {
 				},
 			},
 			want: want{
-				o:   managed.ExternalObservation{},
-				err: errors.Wrap(errors.Wrap(errBoom, errDestroy), errDiff),
+				err: errors.Wrap(errBoom, errDeleteWorkspace),
 			},
 		},
 		"ResourcesError": {
@@ -1039,9 +1076,7 @@ func TestDelete(t *testing.T) {
 					MockDestroy: func(_ context.Context, _ ...terraform.Option) error { return nil },
 				},
 				kube: &test.MockClient{
-					MockDelete: test.NewMockDeleteFn(nil),
-					MockGet:    test.NewMockGetFn(nil),
-					MockList:   test.NewMockListFn(nil),
+					MockGet: test.NewMockGetFn(nil),
 				},
 			},
 			args: args{
diff --git a/internal/terraform/terraform_harness_test.go b/internal/terraform/terraform_harness_test.go
index cc25b87..71ab495 100644
--- a/internal/terraform/terraform_harness_test.go
+++ b/internal/terraform/terraform_harness_test.go
@@ -1,3 +1,4 @@
+//go:build invoke_terraform
 // +build invoke_terraform
 
 /*
@@ -130,6 +131,46 @@ func TestWorkspace(t *testing.T) {
 	}
 }
 
+func TestDeleteWorkspace(t *testing.T) {
+	type args struct {
+		ctx  context.Context
+		name string
+	}
+
+	cases := map[string]struct {
+		reason string
+		args   args
+		want   error
+	}{
+		"SuccessfulDelete": {
+			reason: "It should be possible to delete an existing workspace.",
+			args: args{
+				ctx:  context.Background(),
+				name: "cool",
+			},
+			want: nil,
+		},
+	}
+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			dir, err := ioutil.TempDir("", "provider-terraform-test")
+			if err != nil {
+				t.Fatalf("Cannot create temporary directory: %v", err)
+			}
+			defer os.RemoveAll(dir)
+
+			tf := Harness{Path: tfBinaryPath, Dir: dir}
+			ws := tf.Workspace(tc.args.ctx, tc.args.name)
+			got := tf.DeleteCurrentWorkspace(tc.args.ctx)
+
+			if diff := cmp.Diff(tc.want, got, test.EquateErrors()); diff != "" {
+				t.Errorf("\n%s\ntf.Workspace(...): -want, +got:\n%s", tc.reason, diff)
+			}
+		})
+	}
+}
+
 func TestOutputs(t *testing.T) {
 	type want struct {
 		outputs []Output