Repository: crossplane-contrib/provider-terraform
Pull request #6 (2021-07-07 01:14:24)

Use `terraform plan` to determine whether the workspace is up-to-date 
<!--
Thank you for helping to improve Crossplane!

Please read through https://git.io/fj2m9 if this is your first time opening a
Crossplane pull request. Find us in https://slack.crossplane.io/messages/dev if
you need any help contributing.
-->

### Description of your changes

<!--
Briefly describe what this pull request does. Be sure to direct your reviewers'
attention to anything that needs special consideration.

We love pull requests that resolve an open Crossplane issue. If yours does, you
can uncomment the below line to indicate which issue your PR fixes, for example
"Fixes #500":

-->
Previously we assumed the workspace was not up-to-date during the observe phase and went straight to invoking terraform apply. The rationale was we might as well go straight to trying to apply, because terraform will just abort without making any changes if none are needed. The downside of this approach is that the resource would act as if it were constantly out-of-date, emitting a spurious update event on every reconcile.

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

<!--
Before reviewers can be confident in the correctness of this pull request, it
needs to tested and shown to be correct. Briefly describe the testing that has
already been done or which is planned for this change.
-->

[contribution process]: https://git.io/fj2m9

I've run `terraform_harness_test.go` which is effectively an integration test. I've also tested this out using `examples/workspace-inline.yaml` and confirmed that the provider now only updates workspaces when I actually change them.


Patch:
diff --git a/internal/controller/workspace/workspace.go b/internal/controller/workspace/workspace.go
index a78d2fd..6b1d75c 100644
--- a/internal/controller/workspace/workspace.go
+++ b/internal/controller/workspace/workspace.go
@@ -56,6 +56,7 @@ const (
 	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"
@@ -76,6 +77,7 @@ type tfclient interface {
 	Workspace(ctx context.Context, name string) error
 	Outputs(ctx context.Context) ([]terraform.Output, error)
 	Resources(ctx context.Context) ([]string, error)
+	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
 }
@@ -175,28 +177,38 @@ type external struct {
 	kube client.Reader
 }
 
-func (c *external) Observe(ctx context.Context, _ resource.Managed) (managed.ExternalObservation, error) {
+func (c *external) Observe(ctx context.Context, mg resource.Managed) (managed.ExternalObservation, error) {
+	cr, ok := mg.(*v1alpha1.Workspace)
+	if !ok {
+		return managed.ExternalObservation{}, errors.New(errNotWorkspace)
+	}
+
+	o, err := c.options(ctx, cr.Spec.ForProvider)
+	if err != nil {
+		return managed.ExternalObservation{}, errors.Wrap(err, errOptions)
+	}
+
+	differs, err := c.tf.Diff(ctx, o...)
+	if err != nil {
+		return managed.ExternalObservation{}, errors.Wrap(err, errDiff)
+	}
+
 	r, err := c.tf.Resources(ctx)
 	if err != nil {
 		return managed.ExternalObservation{}, errors.Wrap(err, errResources)
 	}
 
 	// TODO(negz): Include any non-sensitive outputs in our status?
-	o, err := c.tf.Outputs(ctx)
+	op, err := c.tf.Outputs(ctx)
 	if err != nil {
 		return managed.ExternalObservation{}, errors.Wrap(err, errOutputs)
 	}
 
-	// TODO(negz): Is there any value in running terraform plan to determine
-	// whether the workspace is up-to-date? Presumably running a no-op apply
-	// is about the same as running a plan. One downside of the current
-	// approach is that we'll emit an event stating that we're updating the
-	// workspace on every reconcile, even if the update is a no-op.
 	return managed.ExternalObservation{
 		ResourceExists:          len(r) > 0,
-		ResourceUpToDate:        false,
+		ResourceUpToDate:        !differs,
 		ResourceLateInitialized: false,
-		ConnectionDetails:       op2cd(o),
+		ConnectionDetails:       op2cd(op),
 	}, nil
 }
 
diff --git a/internal/terraform/terraform.go b/internal/terraform/terraform.go
index c3de7de..ab374b9 100644
--- a/internal/terraform/terraform.go
+++ b/internal/terraform/terraform.go
@@ -358,6 +358,36 @@ func WithVarFile(data []byte, f FileFormat) Option {
 	}
 }
 
+// Diff invokes 'terraform plan' to determine whether there is a diff between
+// the desired and the actual state of the configuration. It returns true if
+// there is a diff.
+func (h Harness) Diff(ctx context.Context, o ...Option) (bool, error) {
+	ao := &options{}
+	for _, fn := range o {
+		fn(ao)
+	}
+
+	for _, vf := range ao.varFiles {
+		if err := ioutil.WriteFile(filepath.Join(h.Dir, vf.filename), vf.data, 0600); err != nil {
+			return false, errors.Wrap(err, errWriteVarFile)
+		}
+	}
+
+	args := append([]string{"plan", "-no-color", "-input=false", "-detailed-exitcode"}, ao.args...)
+	cmd := exec.CommandContext(ctx, h.Path, args...) //nolint:gosec
+	cmd.Dir = h.Dir
+
+	// The -detailed-exitcode flag will make terraform plan return:
+	// 0 - Succeeded, diff is empty (no changes)
+	// 1 - Errored
+	// 2 - Succeeded, there is a diff
+	_, err := cmd.Output()
+	if cmd.ProcessState.ExitCode() == 2 {
+		return true, nil
+	}
+	return false, Classify(err)
+}
+
 // Apply a Terraform configuration.
 func (h Harness) Apply(ctx context.Context, o ...Option) error {
 	ao := &options{}
@@ -371,7 +401,7 @@ func (h Harness) Apply(ctx context.Context, o ...Option) error {
 		}
 	}
 
-	args := append([]string{"apply", "-no-color", "-auto-approve"}, ao.args...)
+	args := append([]string{"apply", "-no-color", "-auto-approve", "-input=false"}, ao.args...)
 	cmd := exec.CommandContext(ctx, h.Path, args...) //nolint:gosec
 	cmd.Dir = h.Dir
 
@@ -392,7 +422,7 @@ func (h Harness) Destroy(ctx context.Context, o ...Option) error {
 		}
 	}
 
-	args := append([]string{"destroy", "-no-color", "-auto-approve"}, do.args...)
+	args := append([]string{"destroy", "-no-color", "-auto-approve", "-input=false"}, do.args...)
 	cmd := exec.CommandContext(ctx, h.Path, args...) //nolint:gosec
 	cmd.Dir = h.Dir
 


Test patch:
diff --git a/internal/controller/workspace/workspace_test.go b/internal/controller/workspace/workspace_test.go
index fe5400d..bdcf7d2 100644
--- a/internal/controller/workspace/workspace_test.go
+++ b/internal/controller/workspace/workspace_test.go
@@ -65,6 +65,7 @@ type MockTf struct {
 	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
 }
@@ -85,6 +86,10 @@ func (tf *MockTf) Resources(ctx context.Context) ([]string, error) {
 	return tf.MockResources(ctx)
 }
 
+func (tf *MockTf) Diff(ctx context.Context, o ...terraform.Option) (bool, error) {
+	return tf.MockDiff(ctx, o...)
+}
+
 func (tf *MockTf) Apply(ctx context.Context, o ...terraform.Option) error {
 	return tf.MockApply(ctx, o...)
 }
@@ -405,13 +410,100 @@ func TestObserve(t *testing.T) {
 		args   args
 		want   want
 	}{
+		"NotAWorkspaceError": {
+			reason: "We should return an error if the supplied managed resource is not a Workspace",
+			args: args{
+				mg: nil,
+			},
+			want: want{
+				err: errors.New(errNotWorkspace),
+			},
+		},
+		"GetConfigMapError": {
+			reason: "We should return any error we encounter getting tfvars from a ConfigMap",
+			fields: fields{
+				kube: &test.MockClient{
+					MockGet: test.NewMockGetFn(nil, func(obj client.Object) error {
+						if _, ok := obj.(*corev1.ConfigMap); ok {
+							return errBoom
+						}
+						return nil
+					}),
+				},
+			},
+			args: args{
+				mg: &v1alpha1.Workspace{
+					Spec: v1alpha1.WorkspaceSpec{
+						ForProvider: v1alpha1.WorkspaceParameters{
+							VarFiles: []v1alpha1.VarFile{
+								{
+									Source:                v1alpha1.VarFileSourceConfigMapKey,
+									ConfigMapKeyReference: &v1alpha1.KeyReference{},
+								},
+							},
+						},
+					},
+				},
+			},
+			want: want{
+				err: errors.Wrap(errors.Wrap(errBoom, errVarFile), errOptions),
+			},
+		},
+		"GetSecretError": {
+			reason: "We should return any error we encounter getting tfvars from a Secret",
+			fields: fields{
+				kube: &test.MockClient{
+					MockGet: test.NewMockGetFn(nil, func(obj client.Object) error {
+						if _, ok := obj.(*corev1.Secret); ok {
+							return errBoom
+						}
+						return nil
+					}),
+				},
+			},
+			args: args{
+				mg: &v1alpha1.Workspace{
+					Spec: v1alpha1.WorkspaceSpec{
+						ForProvider: v1alpha1.WorkspaceParameters{
+							VarFiles: []v1alpha1.VarFile{
+								{
+									Source:             v1alpha1.VarFileSourceSecretKey,
+									SecretKeyReference: &v1alpha1.KeyReference{},
+								},
+							},
+						},
+					},
+				},
+			},
+			want: want{
+				err: errors.Wrap(errors.Wrap(errBoom, errVarFile), errOptions),
+			},
+		},
+		"DiffError": {
+			reason: "We should return any error encountered while diffing the Terraform configuration",
+			fields: fields{
+				tf: &MockTf{
+					MockDiff: func(ctx context.Context, o ...terraform.Option) (bool, error) { return false, errBoom },
+				},
+			},
+			args: args{
+				mg: &v1alpha1.Workspace{},
+			},
+			want: want{
+				err: errors.Wrap(errBoom, errDiff),
+			},
+		},
 		"ResourcesError": {
 			reason: "We should return any error encountered while listing extant Terraform resources",
 			fields: fields{
 				tf: &MockTf{
+					MockDiff:      func(ctx context.Context, o ...terraform.Option) (bool, error) { return false, nil },
 					MockResources: func(ctx context.Context) ([]string, error) { return nil, errBoom },
 				},
 			},
+			args: args{
+				mg: &v1alpha1.Workspace{},
+			},
 			want: want{
 				err: errors.Wrap(errBoom, errResources),
 			},
@@ -420,10 +512,14 @@ func TestObserve(t *testing.T) {
 			reason: "We should return any error encountered while listing Terraform outputs",
 			fields: fields{
 				tf: &MockTf{
+					MockDiff:      func(ctx context.Context, o ...terraform.Option) (bool, error) { return false, nil },
 					MockResources: func(ctx context.Context) ([]string, error) { return nil, nil },
 					MockOutputs:   func(ctx context.Context) ([]terraform.Output, error) { return nil, errBoom },
 				},
 			},
+			args: args{
+				mg: &v1alpha1.Workspace{},
+			},
 			want: want{
 				err: errors.Wrap(errBoom, errOutputs),
 			},
@@ -432,13 +528,18 @@ func TestObserve(t *testing.T) {
 			reason: "A workspace with zero resources should be considered to be non-existent",
 			fields: fields{
 				tf: &MockTf{
+					MockDiff:      func(ctx context.Context, o ...terraform.Option) (bool, error) { return false, nil },
 					MockResources: func(ctx context.Context) ([]string, error) { return []string{}, nil },
 					MockOutputs:   func(ctx context.Context) ([]terraform.Output, error) { return nil, nil },
 				},
 			},
+			args: args{
+				mg: &v1alpha1.Workspace{},
+			},
 			want: want{
 				o: managed.ExternalObservation{
 					ResourceExists:    false,
+					ResourceUpToDate:  true,
 					ConnectionDetails: managed.ConnectionDetails{},
 				},
 			},
@@ -447,6 +548,7 @@ func TestObserve(t *testing.T) {
 			reason: "A workspace with resources should return its outputs as connection details",
 			fields: fields{
 				tf: &MockTf{
+					MockDiff: func(ctx context.Context, o ...terraform.Option) (bool, error) { return false, nil },
 					MockResources: func(ctx context.Context) ([]string, error) {
 						return []string{"cool_resource.very"}, nil
 					},
@@ -458,10 +560,13 @@ func TestObserve(t *testing.T) {
 					},
 				},
 			},
+			args: args{
+				mg: &v1alpha1.Workspace{},
+			},
 			want: want{
 				o: managed.ExternalObservation{
 					ResourceExists:   true,
-					ResourceUpToDate: false,
+					ResourceUpToDate: true,
 					ConnectionDetails: managed.ConnectionDetails{
 						"string": {},
 						"object": []byte("null"), // Because we JSON decode the the value, which is interface{}{}
diff --git a/internal/terraform/terraform_harness_test.go b/internal/terraform/terraform_harness_test.go
index d36a8ba..cc25b87 100644
--- a/internal/terraform/terraform_harness_test.go
+++ b/internal/terraform/terraform_harness_test.go
@@ -229,7 +229,7 @@ func TestResources(t *testing.T) {
 	}
 }
 
-func TestInitApplyDestroy(t *testing.T) {
+func TestInitDiffApplyDestroy(t *testing.T) {
 	type initArgs struct {
 		ctx context.Context
 		o   []InitOption
@@ -240,13 +240,18 @@ func TestInitApplyDestroy(t *testing.T) {
 	}
 	type want struct {
 		init    error
+		diff    error
 		apply   error
 		destroy error
+
+		differsBeforeApply bool
+		differsAfterApply  bool
 	}
 
 	cases := map[string]struct {
 		reason      string
 		initArgs    initArgs
+		diffArgs    args
 		applyArgs   args
 		destroyArgs args
 		want        want
@@ -260,9 +265,15 @@ func TestInitApplyDestroy(t *testing.T) {
 			applyArgs: args{
 				ctx: context.Background(),
 			},
+			diffArgs: args{
+				ctx: context.Background(),
+			},
 			destroyArgs: args{
 				ctx: context.Background(),
 			},
+			want: want{
+				differsBeforeApply: false,
+			},
 		},
 		"WithVar": {
 			reason: "It should be possible to initialize a simple Terraform module, then apply and destroy it with a supplied variable",
@@ -274,10 +285,17 @@ func TestInitApplyDestroy(t *testing.T) {
 				ctx: context.Background(),
 				o:   []Option{WithVar("coolness", "extreme")},
 			},
+			diffArgs: args{
+				ctx: context.Background(),
+				o:   []Option{WithVar("coolness", "extreme")},
+			},
 			destroyArgs: args{
 				ctx: context.Background(),
 				o:   []Option{WithVar("coolness", "extreme")},
 			},
+			want: want{
+				differsBeforeApply: true,
+			},
 		},
 		"WithHCLVarFile": {
 			reason: "It should be possible to initialize a simple Terraform module, then apply and destroy it with a supplied HCL file of variables",
@@ -285,6 +303,10 @@ func TestInitApplyDestroy(t *testing.T) {
 				ctx: context.Background(),
 				o:   []InitOption{FromModule(filepath.Join(tfTestDataPath(), "nullmodule"))},
 			},
+			diffArgs: args{
+				ctx: context.Background(),
+				o:   []Option{WithVarFile([]byte(`coolness = "extreme!"`), HCL)},
+			},
 			applyArgs: args{
 				ctx: context.Background(),
 				o:   []Option{WithVarFile([]byte(`coolness = "extreme!"`), HCL)},
@@ -293,6 +315,9 @@ func TestInitApplyDestroy(t *testing.T) {
 				ctx: context.Background(),
 				o:   []Option{WithVarFile([]byte(`coolness = "extreme!"`), HCL)},
 			},
+			want: want{
+				differsBeforeApply: true,
+			},
 		},
 		"WithJSONVarFile": {
 			reason: "It should be possible to initialize a simple Terraform module, then apply and destroy it with a supplied JSON file of variables",
@@ -300,6 +325,10 @@ func TestInitApplyDestroy(t *testing.T) {
 				ctx: context.Background(),
 				o:   []InitOption{FromModule(filepath.Join(tfTestDataPath(), "nullmodule"))},
 			},
+			diffArgs: args{
+				ctx: context.Background(),
+				o:   []Option{WithVarFile([]byte(`{"coolness":"extreme!"}`), JSON)},
+			},
 			applyArgs: args{
 				ctx: context.Background(),
 				o:   []Option{WithVarFile([]byte(`{"coolness":"extreme!"}`), JSON)},
@@ -308,6 +337,9 @@ func TestInitApplyDestroy(t *testing.T) {
 				ctx: context.Background(),
 				o:   []Option{WithVarFile([]byte(`{"coolness":"extreme!"}`), JSON)},
 			},
+			want: want{
+				differsBeforeApply: true,
+			},
 		},
 		// NOTE(negz): The goal of these error case tests is to validate that
 		// any kind of error classification is happening. We don't want to test
@@ -319,6 +351,9 @@ func TestInitApplyDestroy(t *testing.T) {
 				ctx: context.Background(),
 				o:   []InitOption{FromModule("./nonexistent")},
 			},
+			diffArgs: args{
+				ctx: context.Background(),
+			},
 			applyArgs: args{
 				ctx: context.Background(),
 			},
@@ -327,6 +362,7 @@ func TestInitApplyDestroy(t *testing.T) {
 			},
 			want: want{
 				init:  errors.New("module not found"),
+				diff:  errors.New("no configuration files"),
 				apply: errors.New("no configuration files"),
 				// Apparently destroy 'works' in this situation ¯\_(ツ)_/¯
 			},
@@ -337,6 +373,9 @@ func TestInitApplyDestroy(t *testing.T) {
 				ctx: context.Background(),
 				o:   []InitOption{FromModule(filepath.Join(tfTestDataPath(), "nullmodule"))},
 			},
+			diffArgs: args{
+				ctx: context.Background(),
+			},
 			applyArgs: args{
 				ctx: context.Background(),
 			},
@@ -352,7 +391,7 @@ func TestInitApplyDestroy(t *testing.T) {
 
 	for name, tc := range cases {
 		t.Run(name, func(t *testing.T) {
-			t.Parallel()
+			// t.Parallel()
 
 			dir, err := ioutil.TempDir("", "provider-terraform-test")
 			if err != nil {
@@ -362,19 +401,36 @@ func TestInitApplyDestroy(t *testing.T) {
 
 			tf := Harness{Path: tfBinaryPath, Dir: dir}
 
-			got := tf.Init(tc.initArgs.ctx, tc.initArgs.o...)
-			if diff := cmp.Diff(tc.want.init, got, test.EquateErrors()); diff != "" {
-				t.Errorf("\n%s\ntf.Init(...): -want, +got:\n%s", tc.reason, diff)
+			err = tf.Init(tc.initArgs.ctx, tc.initArgs.o...)
+			if diff := cmp.Diff(tc.want.init, err, test.EquateErrors()); diff != "" {
+				t.Errorf("\n%s\ntf.Init(...): -want error, +got error:\n%s", tc.reason, diff)
+			}
+
+			differs, err := tf.Diff(tc.diffArgs.ctx, tc.diffArgs.o...)
+			t.Logf("Want %t, got %t", tc.want.differsBeforeApply, differs)
+			if diff := cmp.Diff(tc.want.diff, err, test.EquateErrors()); diff != "" {
+				t.Errorf("\n%s\ntf.Diff(...): -want error, +got error (before apply):\n%s", tc.reason, diff)
+			}
+			if diff := cmp.Diff(tc.want.differsBeforeApply, differs); diff != "" {
+				t.Errorf("\n%s\ntf.Diff(...): -want, +got (before apply):\n%s", tc.reason, diff)
 			}
 
-			got = tf.Apply(tc.applyArgs.ctx, tc.applyArgs.o...)
-			if diff := cmp.Diff(tc.want.apply, got, test.EquateErrors()); diff != "" {
-				t.Errorf("\n%s\ntf.Apply(...): -want, +got:\n%s", tc.reason, diff)
+			err = tf.Apply(tc.applyArgs.ctx, tc.applyArgs.o...)
+			if diff := cmp.Diff(tc.want.apply, err, test.EquateErrors()); diff != "" {
+				t.Errorf("\n%s\ntf.Apply(...): -want error, +got error:\n%s", tc.reason, diff)
+			}
+
+			differs, err = tf.Diff(tc.diffArgs.ctx, tc.diffArgs.o...)
+			if diff := cmp.Diff(tc.want.diff, err, test.EquateErrors()); diff != "" {
+				t.Errorf("\n%s\ntf.Diff(...): -want error, +got error (after apply):\n%s", tc.reason, diff)
+			}
+			if diff := cmp.Diff(tc.want.differsAfterApply, differs); diff != "" {
+				t.Errorf("\n%s\ntf.Diff(...): -want, +got (after apply):\n%s", tc.reason, diff)
 			}
 
-			got = tf.Destroy(tc.destroyArgs.ctx, tc.destroyArgs.o...)
-			if diff := cmp.Diff(tc.want.destroy, got, test.EquateErrors()); diff != "" {
-				t.Errorf("\n%s\ntf.Destroy(...): -want, +got:\n%s", tc.reason, diff)
+			err = tf.Destroy(tc.destroyArgs.ctx, tc.destroyArgs.o...)
+			if diff := cmp.Diff(tc.want.destroy, err, test.EquateErrors()); diff != "" {
+				t.Errorf("\n%s\ntf.Destroy(...): -want error, +got error:\n%s", tc.reason, diff)
 			}
 		})
 	}