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

Issue:
Publish non-sensitive outputs in atProvider
### What problem are you facing?

We have some functionality that we need to implement in Terraform because the crossplane aws provider does not yet support the required resources.   The Terraform is creating objects that are dependencies for other resources in the composition, so we need a way to enforce the dependency chain to prevent the other resources from failing to create because the Terraform hasn't completed yet.

### How could Crossplane help solve your problem?

If the Terraform provider could publish it's non-sensitive outputs in the atProvider section, they could be used to create dependencies with other resources in the composition, so that those resources would not attempt to execute until the Terraform had completed.

Sensitive outputs can still be published in the connection secret, although there may be use cases where it would be useful to have access to sensitive outputs in the composition.

Pull request description:
Add Terraform outputs to status.atProvider
Signed-off-by: Bob Haddleton <bob.haddleton@nokia.com>

### Description of your changes

Add the non-sensitive Terraform module outputs to the status.atProvider.outputs section of the Workspace object

Fixes #36 

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

I tested these changes locally in a kind cluster using a mock Terraform module that outputs all 5 value types:

```
Spec:
  Deletion Policy:  Delete
  For Provider:
    Module:  // Modules _must_ use remote state. The provider does not persist state.
terraform {
  backend "kubernetes" {
    secret_suffix     = "xp"
    namespace         = "default"
    config_path = "/home/bobh/.kube/config"
    config_context = "kind-provider-terraform-dev"
  }
}
resource "random_integer" "foo" {
        min = 1
        max = 10
}
output "string" {
  value = "bar"
  sensitive = false
}

output "number" {
  value = 1.9
  sensitive = false
}

output "object" {
  value = random_integer.foo
  sensitive = false
}

output "tuple" {
  value = ["foo", "bar"]
  sensitive = false
}

output "bool" {
  value = false
  sensitive = false
}

output "sensitive" {
  value = "SENSITIVE"
  sensitive = true
}
    Source:  Inline
  Provider Config Ref:
    Name:  default
Status:
  At Provider:
    Outputs:
      Bool:    false
      Number:  1.9
      Object:  {"id":"9","keepers":null,"max":10,"min":1,"result":9,"seed":null}
      String:  bar
      Tuple:   ["foo","bar"]

```

[contribution process]: https://git.io/fj2m9


Patch:
diff --git a/README.md b/README.md
index 8cb5d9f..ac98de5 100644
--- a/README.md
+++ b/README.md
@@ -23,7 +23,8 @@ spec:
     # main.tf as opaque, inline HCL.
     source: Inline
     module: |
-      // Outputs are written to the connection secret.
+      // All outputs are written to the connection secret.  Non-sensitive outputs
+      // are stored as string values in the status.atProvider.outputs object.
       output "url" {
         value       = google_storage_bucket.example.self_link
       }
@@ -102,6 +103,62 @@ spec:
 Standard `.git-credentials` filename is important to keep so provider-terraform
 controller will be able to automatically pick it up.
 
+
+## Terraform Output support
+
+Non-sensitive outputs are mapped to the status.atProvider.outputs section
+as strings so they can be referenced by the Composition.
+Strings, numbers and booleans can be referenced directly in Compositions
+and can be used in the _convert_ transform if type conversion is needed.
+Tuple and object outputs will be available in the corresponding JSON form.
+This is required because undefined object attributes are not specified in the Workspace
+CRD and so will be sanitized before the status is stored in the database.
+
+That means that any output values required for use in the Composition must be published
+explicitly and individually, and they cannot be referenced inside a tuple or object.
+
+For example, the following terraform outputs:
+```yaml
+      output "string" {
+        value = "bar"
+        sensitive = false
+      }
+      output "number" {
+        value = 1.9
+        sensitive = false
+      }
+      output "object" {
+        // This will be a JSON string - the key/value pairs are not accessible
+        value = {"a": 3, "b": 2}
+        sensitive = false
+      }
+      output "tuple" {
+        // This will be a JSON string - the elements will not be accessible
+        value = ["foo", "bar"]
+        sensitive = false
+      }
+      output "bool" {
+        value = false
+        sensitive = false
+      }
+      output "sensitive" {
+        value = "SENSITIVE"
+        sensitive = true
+      }
+```
+Appear in the corresponding outputs section as:
+```yaml
+  status:
+    atProvider:
+      outputs:
+        bool: "false"
+        number: "1.9"
+        object: '{"a":3,"b":2}'
+        string: bar
+        tuple: '["foo", "bar"]'
+```
+Note that the "sensitive" output is not included in status.atProvider.outputs
+
 ## Known limitations:
 
 * You must either use remote state or ensure the provider container's `/tf`
diff --git a/apis/v1alpha1/workspace_types.go b/apis/v1alpha1/workspace_types.go
index 32528e5..6a875c9 100644
--- a/apis/v1alpha1/workspace_types.go
+++ b/apis/v1alpha1/workspace_types.go
@@ -17,9 +17,8 @@ limitations under the License.
 package v1alpha1
 
 import (
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-
 	xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 // A Var represents a Terraform configuration variable.
@@ -113,8 +112,7 @@ type WorkspaceParameters struct {
 
 // WorkspaceObservation are the observable fields of a Workspace.
 type WorkspaceObservation struct {
-	// TODO(negz): Should we include outputs here? Or only in connection
-	// details.
+	Outputs map[string]string `json:"outputs,omitempty"`
 }
 
 // A WorkspaceSpec defines the desired state of a Workspace.
diff --git a/apis/v1alpha1/zz_generated.deepcopy.go b/apis/v1alpha1/zz_generated.deepcopy.go
index d241712..8ebf1d6 100644
--- a/apis/v1alpha1/zz_generated.deepcopy.go
+++ b/apis/v1alpha1/zz_generated.deepcopy.go
@@ -323,6 +323,13 @@ func (in *WorkspaceList) DeepCopyObject() runtime.Object {
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *WorkspaceObservation) DeepCopyInto(out *WorkspaceObservation) {
 	*out = *in
+	if in.Outputs != nil {
+		in, out := &in.Outputs, &out.Outputs
+		*out = make(map[string]string, len(*in))
+		for key, val := range *in {
+			(*out)[key] = val
+		}
+	}
 }
 
 // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new WorkspaceObservation.
@@ -383,7 +390,7 @@ func (in *WorkspaceSpec) DeepCopy() *WorkspaceSpec {
 func (in *WorkspaceStatus) DeepCopyInto(out *WorkspaceStatus) {
 	*out = *in
 	in.ResourceStatus.DeepCopyInto(&out.ResourceStatus)
-	out.AtProvider = in.AtProvider
+	in.AtProvider.DeepCopyInto(&out.AtProvider)
 }
 
 // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new WorkspaceStatus.
diff --git a/internal/controller/workspace/workspace.go b/internal/controller/workspace/workspace.go
index adc540d..518880a 100644
--- a/internal/controller/workspace/workspace.go
+++ b/internal/controller/workspace/workspace.go
@@ -262,11 +262,13 @@ func (c *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex
 		return managed.ExternalObservation{}, errors.Wrap(err, errResources)
 	}
 
-	// TODO(negz): Include any non-sensitive outputs in our status?
+	// Include any non-sensitive outputs in our status
 	op, err := c.tf.Outputs(ctx)
+
 	if err != nil {
 		return managed.ExternalObservation{}, errors.Wrap(err, errOutputs)
 	}
+	cr.Status.AtProvider = generateWorkspaceObservation(op)
 
 	return managed.ExternalObservation{
 		ResourceExists:          len(r) > 0,
@@ -301,7 +303,7 @@ func (c *external) Update(ctx context.Context, mg resource.Managed) (managed.Ext
 	if err != nil {
 		return managed.ExternalUpdate{}, errors.Wrap(err, errOutputs)
 	}
-
+	cr.Status.AtProvider = generateWorkspaceObservation(op)
 	// TODO(negz): Allow Workspaces to optionally derive their readiness from an
 	// output - similar to the logic XRs use to derive readiness from a field of
 	// a composed resource.
@@ -373,3 +375,21 @@ func op2cd(o []terraform.Output) managed.ConnectionDetails {
 	}
 	return cd
 }
+
+// generateWorkspaceObservation is used to produce v1alpha1.WorkspaceObservation from
+// workspace_type.Workspace.
+func generateWorkspaceObservation(op []terraform.Output) v1alpha1.WorkspaceObservation {
+	wo := v1alpha1.WorkspaceObservation{
+		Outputs: make(map[string]string, len(op)),
+	}
+	for _, o := range op {
+		if !o.Sensitive {
+			if o.Type == terraform.OutputTypeString {
+				wo.Outputs[o.Name] = o.StringValue()
+			} else if j, err := o.JSONValue(); err == nil {
+				wo.Outputs[o.Name] = string(j)
+			}
+		}
+	}
+	return wo
+}
diff --git a/internal/terraform/terraform.go b/internal/terraform/terraform.go
index ab374b9..8133437 100644
--- a/internal/terraform/terraform.go
+++ b/internal/terraform/terraform.go
@@ -217,6 +217,11 @@ type Output struct {
 	value interface{}
 }
 
+// Value returns the output's actual value.
+func (o Output) Value() interface{} {
+	return o.value
+}
+
 // StringValue returns the output's value as a string. It should only be used
 // for outputs of type 'string'.
 func (o Output) StringValue() string {
diff --git a/package/crds/tf.crossplane.io_workspaces.yaml b/package/crds/tf.crossplane.io_workspaces.yaml
index 98840b6..2183e5f 100644
--- a/package/crds/tf.crossplane.io_workspaces.yaml
+++ b/package/crds/tf.crossplane.io_workspaces.yaml
@@ -168,6 +168,11 @@ spec:
             properties:
               atProvider:
                 description: WorkspaceObservation are the observable fields of a Workspace.
+                properties:
+                  outputs:
+                    additionalProperties:
+                      type: string
+                    type: object
                 type: object
               conditions:
                 description: Conditions of the resource.


Test patch:
diff --git a/internal/controller/workspace/workspace_test.go b/internal/controller/workspace/workspace_test.go
index 625f01c..6029038 100644
--- a/internal/controller/workspace/workspace_test.go
+++ b/internal/controller/workspace/workspace_test.go
@@ -486,6 +486,7 @@ func TestObserve(t *testing.T) {
 
 	type want struct {
 		o   managed.ExternalObservation
+		wo  v1alpha1.WorkspaceObservation
 		err error
 	}
 
@@ -627,6 +628,9 @@ func TestObserve(t *testing.T) {
 					ResourceUpToDate:  true,
 					ConnectionDetails: managed.ConnectionDetails{},
 				},
+				wo: v1alpha1.WorkspaceObservation{
+					Outputs: map[string]string{},
+				},
 			},
 		},
 		"WorkspaceExists": {
@@ -639,8 +643,8 @@ func TestObserve(t *testing.T) {
 					},
 					MockOutputs: func(ctx context.Context) ([]terraform.Output, error) {
 						return []terraform.Output{
-							{Name: "string", Type: terraform.OutputTypeString},
-							{Name: "object", Type: terraform.OutputTypeObject},
+							{Name: "string", Type: terraform.OutputTypeString, Sensitive: false},
+							{Name: "object", Type: terraform.OutputTypeObject, Sensitive: true},
 						}, nil
 					},
 				},
@@ -657,6 +661,11 @@ func TestObserve(t *testing.T) {
 						"object": []byte("null"), // Because we JSON decode the the value, which is interface{}{}
 					},
 				},
+				wo: v1alpha1.WorkspaceObservation{
+					Outputs: map[string]string{
+						"string": "",
+					},
+				},
 			},
 		},
 	}
@@ -671,6 +680,11 @@ func TestObserve(t *testing.T) {
 			if diff := cmp.Diff(tc.want.o, got); diff != "" {
 				t.Errorf("\n%s\ne.Observe(...): -want, +got:\n%s\n", tc.reason, diff)
 			}
+			if tc.args.mg != nil {
+				if diff := cmp.Diff(tc.want.wo, tc.args.mg.(*v1alpha1.Workspace).Status.AtProvider); diff != "" {
+					t.Errorf("\n%s\ne.Observe(...): -want, +got:\n%s\n", tc.reason, diff)
+				}
+			}
 		})
 	}
 }
@@ -690,6 +704,7 @@ func TestCreate(t *testing.T) {
 
 	type want struct {
 		c   managed.ExternalCreation
+		wo  v1alpha1.WorkspaceObservation
 		err error
 	}
 
@@ -804,8 +819,8 @@ func TestCreate(t *testing.T) {
 					MockApply: func(_ context.Context, _ ...terraform.Option) error { return nil },
 					MockOutputs: func(ctx context.Context) ([]terraform.Output, error) {
 						return []terraform.Output{
-							{Name: "string", Type: terraform.OutputTypeString},
-							{Name: "object", Type: terraform.OutputTypeObject},
+							{Name: "string", Type: terraform.OutputTypeString, Sensitive: true},
+							{Name: "object", Type: terraform.OutputTypeObject, Sensitive: false},
 						}, nil
 					},
 				},
@@ -837,7 +852,12 @@ func TestCreate(t *testing.T) {
 				c: managed.ExternalCreation{
 					ConnectionDetails: managed.ConnectionDetails{
 						"string": {},
-						"object": []byte("null"), // Because we JSON decode the the value, which is interface{}{}
+						"object": []byte("null"), // Because we JSON decode the value, which is interface{}{}
+					},
+				},
+				wo: v1alpha1.WorkspaceObservation{
+					Outputs: map[string]string{
+						"object": "null",
 					},
 				},
 			},
@@ -854,6 +874,11 @@ func TestCreate(t *testing.T) {
 			if diff := cmp.Diff(tc.want.c, got); diff != "" {
 				t.Errorf("\n%s\ne.Create(...): -want, +got:\n%s\n", tc.reason, diff)
 			}
+			if tc.args.mg != nil {
+				if diff := cmp.Diff(tc.want.wo, tc.args.mg.(*v1alpha1.Workspace).Status.AtProvider); diff != "" {
+					t.Errorf("\n%s\ne.Observe(...): -want, +got:\n%s\n", tc.reason, diff)
+				}
+			}
 		})
 	}
 }