Fix inconsistent plan error (#218)

This is a fix for the inconsistent plan error.

### Background

To expose outputs from a module, we need to account for the secretness of the outputs
If you try and output a secret value without setting `sensitive: true` in the output
The deployment will fail. We also need to be able to handle this dynamically since we won't know
the secret status until after the operation, and we need to write the output definition when we create the file (before the operation).

### Previous Solution

In #132 we added support for module outputs by using a `terraform_data` resource as a proxy for the output. This allowed us to account for the dynamic nature of the secret since we could determine the secret status of the value using the same method that we use for all other resource properties.

This ran into an issue #198 where `terraform_data` resources cannot be used if the value may be "inconsistent". A very common case that is used in most modules is to define an output like this:

```hcl
output "default_security_group_id" {
  description = "The ID of the security group created by default on VPC creation"
  value       = try(aws_vpc.this[0].default_security_group_id, null)
}
```

`try` is used because most modules will conditionally create resources and need to handle the case where the resource was not created and the value doesn't exist. This is a common case, but there may be other cases of this "inconsistent" data.

### New Solution

This PR tries a new solution of using output values with a combination of the [nonsensitive](https://developer.hashicorp.com/terraform/language/functions/nonsensitive) and [issensitive](https://developer.hashicorp.com/terraform/language/functions/issensitive) functions. We can use the `issensitive` function to dynamically determine whether the value is sensitive or not. Unfortunately it is not possible to use functions in the `output.sensitive` field (e.g. `sensitive: "${issensitive(module.source_module.output_name1)}"` won't work).

To work around this we use two outputs for each output value:
- The first output is the actual value that we mark as non-sensitive to avoid the error
- The second output is a boolean that indicates whether the value is a secret or not.

```json
"output": {
  "name1": {
      "value": "${nonsensitive(module.source_module.output_name1)}"
  },
  "internal_output_is_secret_name1": {
      "value": "${issensitive(module.source_module.output_name1)}"
  },
  ...
}
```

fixes #198

Author: corymhall

pkg/modprovider/module_component.go

@@ -183,12 +183,6 @@ func NewModuleComponentResource(
 		var errs []error
 
 		plan.VisitResources(func(rp *tfsandbox.ResourcePlan) {
-			if rp.IsInternalOutputResource() {
-				// skip internal output resources which we created
-				// so that we propagate outputs from module
-				return
-			}
-
 			resourceOptions := []pulumi.ResourceOption{
 				pulumi.Parent(&component),
 			}
@@ -233,12 +227,6 @@ func NewModuleComponentResource(
 
 		var errs []error
 		tfState.VisitResources(func(rp *tfsandbox.ResourceState) {
-			if rp.IsInternalOutputResource() {
-				// skip internal output resources which we created
-				// so that we propagate outputs from module
-				return
-			}
-
 			resourceOptions := []pulumi.ResourceOption{
 				pulumi.Parent(&component),
 			}

pkg/tfsandbox/details.go

@@ -15,6 +15,7 @@
 package tfsandbox
 
 import (
+	"fmt"
 	"strings"
 
 	tfjson "github.com/hashicorp/terraform-json"
@@ -188,43 +189,46 @@ func unknown() resource.PropertyValue {
 	})
 }
 
-// secretUnknown returns true if the value is a secret and the secret value is unknown.
-func secretUnknown(value resource.PropertyValue) bool {
-	if value.IsSecret() {
-		secret := value.SecretValue()
-		if secret != nil && secret.Element.IsComputed() {
-			return true
+// isInternalOutputResource returns true if the resource is an internal is_secret output
+// which is used to keep track of the secretness of the output.
+func isInternalOutputResource(name string) bool {
+	return strings.HasPrefix(name, terraformIsSecretOutputPrefix)
+}
+
+// outputIsSecret returns true if the output is a secret based on the value of the
+// corresponding is_secret output.
+func (p *Plan) outputIsSecret(outputName string) bool {
+	isSecretKey := fmt.Sprintf("%s%s", terraformIsSecretOutputPrefix, outputName)
+	if isSecretVal, ok := p.rawPlan.OutputChanges[isSecretKey]; ok {
+		// If the value is unknown, just return false because we don't know the value
+		// so secretness doesn't matter yet
+		if afterUnknown, ok := isSecretVal.AfterUnknown.(bool); ok && afterUnknown {
+			return false
 		}
+		return isSecretVal.After.(bool)
 	}
-
+	contract.Failf("isSecret key %q not found in output changes", isSecretKey)
 	return false
 }
 
-// IsInternalOutputResource returns true if the resource is an internal output resource.
-// which is used to expose outputs from the source module in a way that maintains unknown-ness
-// and secretness of the outputs.
-func (p *ResourcePlan) IsInternalOutputResource() bool {
-	return strings.HasPrefix(p.Name(), terraformDataResourcePrefix)
-}
-
 // Outputs returns the outputs of a terraform plan as a Pulumi property map.
 func (p *Plan) Outputs() resource.PropertyMap {
 	outputs := resource.PropertyMap{}
-	p.Resources.VisitResources(func(res *ResourcePlan) {
-		if res.IsInternalOutputResource() {
-			withoutPrefix := strings.TrimPrefix(res.Name(), terraformDataResourcePrefix)
-			outputKey := resource.PropertyKey(withoutPrefix)
-			plannedValues := res.PlannedValues()
-			if v, ok := plannedValues[resource.PropertyKey("input")]; ok {
-				if secretUnknown(v) {
-					// collapse secret unknowns into just unknown
-					outputs[outputKey] = unknown()
-				} else {
-					outputs[outputKey] = v
-				}
+	for outputKey, output := range p.rawPlan.OutputChanges {
+		if isInternalOutputResource(outputKey) {
+			continue
+		}
+		key := resource.PropertyKey(outputKey)
+		if afterUnknown, ok := output.AfterUnknown.(bool); ok && afterUnknown {
+			outputs[key] = unknown()
+		} else {
+			val := resource.NewPropertyValueRepl(output.After, nil, replaceJSONNumberValue)
+			if p.outputIsSecret(outputKey) {
+				val = resource.MakeSecret(val)
 			}
+			outputs[key] = val
 		}
-	})
+	}
 	return outputs
 }
 
@@ -260,26 +264,31 @@ func newState(rawState *tfjson.State) (*State, error) {
 	}, nil
 }
 
-// IsInternalOutputResource returns true if the resource is an internal output resource.
-// which is used to expose outputs from the source module in a way that maintains unknown-ness
-// and secretness of the outputs.
-func (s *ResourceState) IsInternalOutputResource() bool {
-	return strings.HasPrefix(s.Name(), terraformDataResourcePrefix)
+// outputIsSecret returns true if the output is a secret based on the value of the
+// corresponding is_secret output.
+func (s *State) outputIsSecret(outputName string) bool {
+	isSecretKey := fmt.Sprintf("%s%s", terraformIsSecretOutputPrefix, outputName)
+	if isSecretVal, ok := s.rawState.Values.Outputs[isSecretKey]; ok {
+		return isSecretVal.Value.(bool)
+	}
+	contract.Failf("isSecret key %q not found in output changes", isSecretKey)
+	return false
 }
 
 // Outputs returns the outputs of a terraform module state as a Pulumi property map.
 func (s *State) Outputs() resource.PropertyMap {
 	outputs := resource.PropertyMap{}
-	s.Resources.VisitResources(func(res *ResourceState) {
-		if res.IsInternalOutputResource() {
-			withoutPrefix := strings.TrimPrefix(res.Name(), terraformDataResourcePrefix)
-			outputKey := resource.PropertyKey(withoutPrefix)
-			attributeValues := res.AttributeValues()
-			if v, ok := attributeValues[resource.PropertyKey("input")]; ok {
-				outputs[outputKey] = v
-			}
+	for outputKey, output := range s.rawState.Values.Outputs {
+		if isInternalOutputResource(outputKey) {
+			continue
 		}
-	})
+		key := resource.PropertyKey(outputKey)
+		val := resource.NewPropertyValueRepl(output.Value, nil, replaceJSONNumberValue)
+		if s.outputIsSecret(outputKey) {
+			val = resource.MakeSecret(val)
+		}
+		outputs[key] = val
+	}
 
 	return outputs
 }

pkg/tfsandbox/state_test.go

@@ -18,12 +18,14 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
+	"io"
 	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/require"
 
 	"github.com/pulumi/pulumi/sdk/v3/go/common/resource"
+	"github.com/pulumi/pulumi/sdk/v3/go/common/util/contract"
 )
 
 func TestState(t *testing.T) {
@@ -178,3 +180,61 @@ func TestStateMatchesPlan(t *testing.T) {
 		})
 	}
 }
+
+type testLogger struct {
+	r io.Writer
+}
+
+func (t *testLogger) Log(_ LogLevel, input string) {
+	_, err := t.r.Write([]byte(input))
+	contract.AssertNoErrorf(err, "test logger failed to write")
+}
+
+func TestSecretOutputs(t *testing.T) {
+	t.Run("nested secrets", func(t *testing.T) {
+		ctx := context.Background()
+
+		tofu, err := NewTofu(ctx, nil)
+		require.NoError(t, err, "error initializing tofu")
+		var buffer bytes.Buffer
+		logger := &testLogger{r: &buffer}
+
+		outputs := []TFOutputSpec{
+			{Name: "nested_sensitive_output"},
+		}
+		ms := TFModuleSource(filepath.Join(getCwd(t), "testdata", "modules", "test_module"))
+		inputs := map[string]any{
+			"inputVar":        "test",
+			"anotherInputVar": resource.NewSecretProperty(&resource.Secret{Element: resource.NewStringProperty("somevalue")}),
+		}
+		emptyProviders := map[string]resource.PropertyMap{}
+		err = CreateTFFile("test", ms, "", tofu.WorkingDir(),
+			resource.NewPropertyMapFromMap(inputs), outputs, emptyProviders)
+		require.NoError(t, err, "error creating tf file")
+
+		err = tofu.Init(ctx, logger)
+		require.NoErrorf(t, err, "error running tofu init: %s", buffer.String())
+		initialPlan, err := tofu.Plan(ctx, logger)
+		require.NoErrorf(t, err, "error running tofu plan (before apply): %s", buffer.String())
+		require.NotNil(t, initialPlan, "expected a non-nil plan")
+
+		plannedOutputs := initialPlan.Outputs()
+		require.Equal(t, resource.PropertyMap{
+			"nested_sensitive_output": resource.MakeComputed(resource.NewStringProperty("")),
+		}, plannedOutputs)
+
+		state, err := tofu.Apply(ctx, logger)
+		require.NoErrorf(t, err, "error running tofu apply: %s", buffer.String())
+		moduleOutputs := state.Outputs()
+		// output value is the same as the input
+		require.Equal(t, resource.PropertyMap{
+			resource.PropertyKey("nested_sensitive_output"): resource.MakeSecret(
+				resource.NewObjectProperty(
+					resource.NewPropertyMapFromMap(map[string]any{
+						"A": resource.NewStringProperty("test"),
+						"B": resource.NewStringProperty("somevalue"),
+						"C": resource.NewStringProperty("test"),
+					}))),
+		}, moduleOutputs)
+	})
+}

pkg/tfsandbox/testdata/modules/test_module/outputs.tf

@@ -14,3 +14,12 @@ output "statically_known" {
 output "number_output" {
   value = var.input_number_var
 }
+
+output "nested_sensitive_output" {
+  value = {
+    A = var.input_var
+    B = var.another_input_var
+    # This will be unknown during plan
+    C = terraform_data.example.output
+  }
+}

pkg/tfsandbox/testdata/modules/test_module/variables.tf

@@ -9,3 +9,8 @@ variable "input_number_var" {
   // we can't pass in a big value
   default = 4222222222222222222
 }
+
+variable "another_input_var" {
+  type    = string
+  default = "default"
+}

pkg/tfsandbox/tf_file.go

@@ -17,9 +17,9 @@ type TFOutputSpec struct {
 }
 
 const (
-	terraformDataResourceType   = "terraform_data"
-	terraformDataResourceName   = "unknown_proxy"
-	terraformDataResourcePrefix = "internal_output_"
+	terraformDataResourceType     = "terraform_data"
+	terraformDataResourceName     = "unknown_proxy"
+	terraformIsSecretOutputPrefix = "internal_output_is_secret_"
 )
 
 func writeTerraformFilesToDirectory() (string, bool) {
@@ -145,6 +145,7 @@ func CreateTFFile(
 	containsUnknowns := inputs.ContainsUnknowns()
 
 	resources := map[string]map[string]interface{}{}
+	mOutputs := map[string]map[string]interface{}{}
 	providers := map[string]interface{}{}
 
 	// NOTE: this should only happen at plan time. At apply time all computed values
@@ -190,34 +191,41 @@ func CreateTFFile(
 		moduleProps["providers"] = providersField
 	}
 
-	// To expose outputs from a module, we create proxy resources of type "terraform_data"
-	// the inputs to those resources are the outputs of the source module
-	// the resources are named "internal_output_<output_name>" and have the following format
-	// "resource": {
-	//    "terraform_data": {
-	//       "internal_output_output_name1": {
-	//           "input": "${module.source_module.output_name1}"
-	//       },
-	//       "internal_output_output_name2": {
-	//           "input": "${module.source_module.output_name2}"
-	//       },
-	//       ...
-	//     }
+	// To expose outputs from a module, we need to account for the secretness of the outputs
+	// If you try and output a secret value without setting `sensitive: true` in the output
+	// The deployment will fail. We need to be able to handle this dynamically since we won't know
+	// the secret status until after the operation.
+	//
+	// To handle this, we use two outputs for each output value:
+	// - The first output is the actual value that we mark as non-sensitive to avoid the error
+	// - The second output is a boolean that indicates whether the value is a secret or not.
+	//
+	// "output": {
+	//    "name1": {
+	//       "value": "${nonsensitive(module.source_module.output_name1)}"
+	//    },
+	//    "internal_output_is_secret_name1": {
+	//       "value": "${issensitive(module.source_module.output_name1)}"
+	//    },
+	//    ...
 	// }
-	// the reason we are using terraform_data resource is because
-	// they maintain unknown-ness and secretness of the outputs of the modules
-	// regardless of whether the outputs were marked as sensitive or not
+	//
+	// NOTE: terraform only allows plain booleans in the output.sensitive field.
+	// i.e. `sensitive: "${issensitive(module.source_module.output_name1)}"` won't work
 	for _, output := range outputs {
-		if _, ok := resources[terraformDataResourceType]; !ok {
-			resources[terraformDataResourceType] = map[string]interface{}{}
+		mOutputs[output.Name] = map[string]any{
+			// wrapping in jsondecode/jsonencode to workaround an issue where nonsensitive/issensitive is not recursive
+			"value": fmt.Sprintf("${jsondecode(nonsensitive(jsonencode(module.%s.%s)))}", name, output.Name),
 		}
-
-		resourceName := fmt.Sprintf("%s%s", terraformDataResourcePrefix, output.Name)
-		resources[terraformDataResourceType][resourceName] = map[string]interface{}{
-			"input": fmt.Sprintf("${module.%s.%s}", name, output.Name),
+		mOutputs[fmt.Sprintf("%s%s", terraformIsSecretOutputPrefix, output.Name)] = map[string]any{
+			"value": fmt.Sprintf("${jsondecode(issensitive(jsonencode(module.%s.%s)))}", name, output.Name),
 		}
 	}
 
+	if len(mOutputs) > 0 {
+		tfFile["output"] = mOutputs
+	}
+
 	if len(resources) > 0 {
 		tfFile["resource"] = resources
 	}