Fix inconsistent plan error

[corymhall]

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:

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 and 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.

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

fixes #198

[corymhall]

This change is part of the following stack:

• Fix inconsistent plan error #218 ◀

_{Change managed by git-spice.}

[t0yv0]

Can you reference what try does? Ah https://developer.hashicorp.com/terraform/language/functions/try

So in case of an error this will swallow data which is not great.

We need some creative thinking here actually, and this got me thinking about other functions we can use:

https://developer.hashicorp.com/terraform/language/functions/nonsensitive
https://developer.hashicorp.com/terraform/language/functions/issensitive

These look intriguing. What if we abandoned wrapping module outputs in terraform_data but instead directly exported them as we did before the workaround from the top-level module, but this time instead of propagating the module M's output o we would propagate nonsensitive(o) and a separate boolean for issensitive(o) to reconstruct this on Pulumi side?

Will this work? ONe interesting case if sensitive bits are not at the top-level of o.

[corymhall]

These look intriguing. What if we abandoned wrapping module outputs in terraform_data but instead directly exported them as we did before the workaround from the top-level module, but this time instead of propagating the module M's output o we would propagate nonsensitive(o) and a separate boolean for issensitive(o) to reconstruct this on Pulumi side?

This is interesting, let me give it a shot.

[t0yv0]

This is great! Can we pin it with a simple test?

Also with regard to testing I was curious if nonsensitive() and issensitive() take care of nested secrets or that's something left for later.

Apparently it is not recursive so we have this:
hashicorp/terraform#32868

Example:

module "m" {
  source = "./m"
  inputvar = sensitive(10)
}

output "exposed" {
  value = nonsensitive(module.m.outputvar)
}

With this module:

inputs.tf:

variable "inputvar" {
  type    = number
  default = 16
}

infra.tf:

resource "random_string" "random" {
  length           = 16
  special          = true
  override_special = "/@£$"
}

outputs.tf:

output "outputvar" {
  value = {
    "X" = var.inputvar,
    "A" = "B"
  }
}

I get the dreaded:

Planning failed. Terraform encountered an error while generating this plan.

╷
│ Error: Output refers to sensitive values
│ 
│   on infra.tf line 6:
│    6: output "exposed" {
│ 
│ To reduce the risk of accidentally exporting sensitive data that was intended to be only internal, Terraform requires that any root module output containing sensitive data be explicitly marked as sensitive, to confirm your
│ intent.
│ 
│ If you do intend to export this data, annotate the output value as sensitive by adding the following argument:
│     sensitive = true
╵

[t0yv0]

I've tried this but similarly issensitive is non-recursive so it does not detect the presence of nested secrets 🤔

module "webservers" {
  source   = "./webservers"
  inputvar = sensitive(10)
}

output "exposed" {
  value     = module.webservers.outputvar
  sensitive = true
}

output "exposed_is_sensitive" {
  value = issensitive(module.webservers.outputvar)
}

[t0yv0]

I think this solution doesn't work generally. Because of the non-recursive issue in these functions. I'm sorry. If we want to support full generality we still need to write our own TF provider it seems like, and perhaps the tweaked terraform_data is the lowest lift.

If we are looking for workarounds that do not address secret handling in full generality, we can consider not supporting first-class secrets or supporting them in an over-approximating manner.

1. do not support fc secrets - fail if any are passed as arguments to the module, asking the user to unsecret them and take care of it
2. over-approximation - if any fc secrets are passed as inputs to the module, mark every single output as sensitive=true

[t0yv0]

Interesting, seems to work!

One limitation I think we still have is secret floating, if any of the innards of an output are secret, the entire thing is secret. That is acceptable for starters, we can document this.

[t0yv0]

Should this just be :

issensitive(jsonencode(module.%s.%s))

Do we need outer jsondecode? issensitive returns bool right?