fix(cloud): terraform output should adhere to read state outputs authorization

[brandonc]

Normally, terraform output refreshes and reads the entire state in the command package before pulling output values out of it. This doesn't give Terraform Cloud the opportunity to apply the read state outputs org permission and instead applies the read state versions permission.

I decided to expand the state manager interface to provide a separate GetRootOutputValues function in order to give the cloud backend a more nuanced opportunity to fetch just the outputs. This required moving state Refresh/Read code that was previously in the command into the shared backend state as well as the filesystem state packages, and implement an alternative version for cloud integration, while all other statemgr methods are delegated to the previous remote state manager.

Testing

When smoke testing, I tested terraform output and terraform output <name> with a sensitive value on the local backend, the cloud backend, and the s3 backend.

Notes

Terraform doesn't reveal sensitive values unless <name> is specified, but GetRootOutputValues isn't aware of this config and retrieves all sensitive values individually from the API. By requesting just the outputs instead of the entire state version, the appropriate permission is applied to the command.

Important

This should not be merged until hashicorp/go-tfe#370 is released and the go-tfe module is bumped to the next version

[alisdair]

This seems like a good, pragmatic approach to me. Some minor comments/questions inline.

I have a slight worry that there's nothing forcing callers to use the GetRootOutputValues method, and they could instead rely on the existing approach of calling Refresh then reading outputs directly. This might lead to new code paths which use root module outputs not using this method, and therefore not allowing Cloud users to access outputs. I don't have any proposal for a better way forward, but I wondered if it's something you had any thoughts on.

[alisdair]

We might consider hiding some of the detail of what's happening here. A message like "Could not parse output value" doesn't get so far into the weeds, as cty's "simple json" is not something that I'd expect a Terraform user to understand.

[apparentlymart]

Adding to that: cty's error messages for JSON parsing are intentionally written with the author of the JSON object as the intended audience, so it is possible that an error message here would use some JSON-specific terminology anyway, but I do agree with Alisdair that 'simple JSON" is very much a cty API convenience feature and not anything an end-user should ever see.

Presumably if we ever get into this error case it reflects a bug in Terraform Cloud rather than something the end-user could address, so I imagine it doesn't really make a lot of difference in the end. In situations like this elsewhere we have included statements like "This is a bug in Terraform; please report it!" but I think we're too deep in the stack for it to make sense to do that here, since we'd typically add that annotation as part of converting the error to a diagnostic, rather than directly in the error object.

---

One thing to watch out for here is that the SimpleJSONValue helper is a wrapper around using ImpliedType and then decoding using that implied type.

In particular, that means that this will lose information compared to what would happen if using the values directly from states.OutputValue.

For example, given a root module output value declared like this:

output "example" {
  type = toset("hello", "world")
}

states.OutputValue.Value would preserve the exact type information and appear as cty.SetVal([]string{"hello", "world"}), but because JSON doesn't have any set type the naive ImpliedType/SimpleJSONValue approach would instead yield cty.TupleVal([]string{"hello", "world"}).

For many purposes that distinction isn't super important, but if these values ever get into a context where users can evaluate HCL expressions against them then this could appear as a breaking change for any expression that is relying on the differences between set, tuple, and list types.

For example:

data.terraform_remote_state.example.outputs.foo == toset([])

== can return true only if both operands are of the same type, so the above could potentially return true for an output value which returns an empty set when accessing the state directly, but would always return false if we silently convert the value to be an empty tuple value instead.

I expect there are other situtions where behavior would be slightly different but not obviously wrong if we silently convert a set to a tuple.

If we do let this leak into the language then it could create subtly incorrect behavior (without an explicit error message explaining why) for someone who started off using some other backend and then switched to Terraform Cloud later.

[brandonc]

This made total sense and I appreciate that you explained it. I became concerned that we were losing this type information somewhere in Terraform Cloud, because all my set values were described as array by the API. Sure enough, I found that the terraform-state-parser service within TFE interprets all values using ImpliedType. I can send you a link if you are interested.

Even if I do the right thing it will still be broken and set type outputs will be displayed as lists (and maps as objects!). I think the right thing to do is to modify cloudOutputToCtyValue so that it can express all terraform types, and then modify the API to do the same... but that is a slightly heavier lift and won't be backwards compatible. It doesn't seem like these output types are very well documented in the API and I've got some concerns about suddenly changing "array" to "set", everything else being equal. 😣

[apparentlymart]

It does unfortunately seem like that API is at the "wrong" level of abstraction for this use-case, indeed -- I guess it was written with UI concerns in mind, where the server is encapsulating certain logic to simplify the UI code, rather than to be used directly as data inside Terraform Core.

I don't really know what to suggest here other than to add to the table the idea that maybe it's warranted to have a separate API for the raw data underneath the abstraction; literally a copy of the verbatim outputs object from the underlying state, perhaps, so that additions to the format in future will just pass through without needing to alter the intermediaries and we'd thus be guaranteed that fetching the output state and fetching the whole state would always yield identical results?

[brandonc]

It may make sense to just add the underlying type information (with json state version info!?) to the current API, under a separate key. If I had the type information and the value expressed as json, I think I can use ctyjson.UnmarshalType and ctyjson.Unmarshal to get the correct values.

[apparentlymart]

As long as the API layer isn't doing any other transforms to the value I think you're right that having a true JSON representation of the type as recorded in the state would be sufficient to address the concern I originally raised here.

It does still mean that if we were to add anything else inside the output state in future we'd need to modify the Terraform Cloud API to pass it through and Terraform Enterprise users who have upgraded Terraform CLI but not yet updated Terraform Cloud would potentially see that information seem to get quietly "lost" in the API layer, so something which passes it through verbatim would be a more robust answer to allow us to evolve these things more gracefully in future (by avoiding tight coupling) but I will concede that it's a separate concern than just the type information being incorrect. 😁

[alisdair]

I'm curious about why this method returns a pointer to a value, instead of a value directly. It's more common to return a cty.Value, and I can't find any other cases where we use a pointer.

[brandonc]

@alisdair

I have a slight worry that there's nothing forcing callers to use the GetRootOutputValues method, and they could instead rely on the existing approach of calling Refresh then reading outputs directly. This might lead to new code paths which use root module outputs not using this method, and therefore not allowing Cloud users to access outputs. I don't have any proposal for a better way forward, but I wondered if it's something you had any thoughts on.

I don't think fetching the entire state is much of a bypass because it requires a higher level of authorization than reading the outputs. In other words, terraform output was failing for cloud users that don't have access to read the entire state. I think it's still valid to read the output using the entire state if you have that access. To fully disallow callers from this possibility, it seems like you'd have to decompose all the parts of the state into similar methods but I don't think that's a good change. Let me know if I misunderstood you

[brandonc]

Putting this on hold until I have more time to dedicate to expanding the amount output type data within terraform cloud

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.