fix(cloud): terraform output should adhere to read state outputs authorization
#30769
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,118 @@ | ||
| package cloud | ||
|
|
||
| import ( | ||
| "context" | ||
| "encoding/json" | ||
| "fmt" | ||
|
|
||
| tfe "github.com/hashicorp/go-tfe" | ||
| "github.com/hashicorp/terraform/internal/states" | ||
| "github.com/hashicorp/terraform/internal/states/remote" | ||
| "github.com/hashicorp/terraform/internal/states/statemgr" | ||
| "github.com/zclconf/go-cty/cty" | ||
| ctyjson "github.com/zclconf/go-cty/cty/json" | ||
| ) | ||
|
|
||
| // State is similar to remote State and delegates to it, except in the case of output values, | ||
| // which use a separate methodology that ensures the caller is authorized to read cloud | ||
| // workspace outputs. | ||
| type State struct { | ||
| Client *remoteClient | ||
|
|
||
| delegate remote.State | ||
| } | ||
|
|
||
| // Proof that cloud State is a statemgr.Persistent interface | ||
| var _ statemgr.Persistent = (*State)(nil) | ||
|
|
||
| func NewState(client *remoteClient) *State { | ||
| return &State{ | ||
| Client: client, | ||
| delegate: remote.State{Client: client}, | ||
| } | ||
| } | ||
|
|
||
| // State delegates calls to read State to the remote State | ||
| func (s *State) State() *states.State { | ||
| return s.delegate.State() | ||
| } | ||
|
|
||
| // Lock delegates calls to lock state to the remote State | ||
| func (s *State) Lock(info *statemgr.LockInfo) (string, error) { | ||
| return s.delegate.Lock(info) | ||
| } | ||
|
|
||
| // Unlock delegates calls to unlock state to the remote State | ||
| func (s *State) Unlock(id string) error { | ||
| return s.delegate.Unlock(id) | ||
| } | ||
|
|
||
| // RefreshState delegates calls to refresh State to the remote State | ||
| func (s *State) RefreshState() error { | ||
| return s.delegate.RefreshState() | ||
| } | ||
|
|
||
| // RefreshState delegates calls to refresh State to the remote State | ||
| func (s *State) PersistState() error { | ||
| return s.delegate.PersistState() | ||
| } | ||
|
|
||
| // WriteState delegates calls to write State to the remote State | ||
| func (s *State) WriteState(state *states.State) error { | ||
| return s.delegate.WriteState(state) | ||
| } | ||
|
|
||
| // GetRootOutputValues fetches output values from Terraform Cloud | ||
| func (s *State) GetRootOutputValues() (map[string]*states.OutputValue, error) { | ||
| ctx := context.Background() | ||
|
|
||
| so, err := s.Client.client.StateVersionOutputs.ReadCurrent(ctx, s.Client.workspace.ID) | ||
|
|
||
| if err != nil { | ||
| return nil, fmt.Errorf("Could not read state version outputs: %w", err) | ||
| } | ||
|
|
||
| result := make(map[string]*states.OutputValue) | ||
|
|
||
| for _, output := range so.Items { | ||
| value, err := s.cloudOutputToCtyValue(ctx, output) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("Could not interpret output value as simple json: %w", err) | ||
| } | ||
|
|
||
| result[output.Name] = &states.OutputValue{ | ||
| Value: *value, | ||
| Sensitive: output.Sensitive, | ||
| } | ||
| } | ||
|
|
||
| return result, nil | ||
| } | ||
|
|
||
| func (s *State) cloudOutputToCtyValue(ctx context.Context, output *tfe.StateVersionOutput) (*cty.Value, error) { | ||
| value := output.Value | ||
| // If an output is sensitive, the API requires that we fetch this output individually to get the value | ||
| // Terraform will decline to reveal the value under some circumstances, but we must provide it to callers | ||
| if output.Sensitive { | ||
| svo, err := s.Client.client.StateVersionOutputs.Read(ctx, output.ID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("Could not read sensitive output %s: %w", output.ID, err) | ||
| } | ||
|
|
||
| value = svo.Value | ||
| } | ||
|
|
||
| buf, err := json.Marshal(value) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("Could not interpret output value: %w", err) | ||
| } | ||
|
|
||
| v := ctyjson.SimpleJSONValue{} | ||
| err = v.UnmarshalJSON(buf) | ||
|
|
||
| if err != nil { | ||
| return nil, fmt.Errorf("Could not interpret output value as simple json: %w", err) | ||
|
There was a problem hiding this comment. 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. There was a problem hiding this comment. Adding to that: 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 In particular, that means that this will lose information compared to what would happen if using the values directly from For example, given a root module output value declared like this: output "example" {
type = toset("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([])
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. There was a problem hiding this comment. 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. 😣 There was a problem hiding this comment. 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 There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. 😁 |
||
| } | ||
|
|
||
| return &v.Value, err | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| package cloud | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/hashicorp/go-tfe" | ||
| "github.com/hashicorp/terraform/internal/states/statemgr" | ||
| ) | ||
|
|
||
| func TestState_impl(t *testing.T) { | ||
| var _ statemgr.Reader = new(State) | ||
| var _ statemgr.Writer = new(State) | ||
| var _ statemgr.Persister = new(State) | ||
| var _ statemgr.Refresher = new(State) | ||
| var _ statemgr.OutputReader = new(State) | ||
| var _ statemgr.Locker = new(State) | ||
| } | ||
|
|
||
| type ExpectedOutput struct { | ||
| Name string | ||
| Sensitive bool | ||
| IsNull bool | ||
| } | ||
|
|
||
| func TestState_GetRootOutputValues(t *testing.T) { | ||
| b, bCleanup := testBackendWithOutputs(t) | ||
| defer bCleanup() | ||
|
|
||
| client := &remoteClient{ | ||
| client: b.client, | ||
| workspace: &tfe.Workspace{ | ||
| ID: "ws-abcd", | ||
| }, | ||
| } | ||
|
|
||
| state := NewState(client) | ||
| outputs, err := state.GetRootOutputValues() | ||
|
|
||
| if err != nil { | ||
| t.Fatalf("error returned from GetRootOutputValues: %s", err) | ||
| } | ||
|
|
||
| cases := []ExpectedOutput{ | ||
| { | ||
| Name: "sensitive_output", | ||
| Sensitive: true, | ||
| IsNull: false, | ||
| }, | ||
| { | ||
| Name: "nonsensitive_output", | ||
| Sensitive: false, | ||
| IsNull: false, | ||
| }, | ||
| } | ||
|
|
||
| if len(outputs) != len(cases) { | ||
| t.Errorf("Expected %d item but %d were returned", len(cases), len(outputs)) | ||
| } | ||
|
|
||
| for _, testCase := range cases { | ||
| so, ok := outputs[testCase.Name] | ||
| if !ok { | ||
| t.Fatalf("Expected key %s but it was not found", testCase.Name) | ||
| } | ||
| if so.Value.IsNull() != testCase.IsNull { | ||
| t.Errorf("Key %s does not match null expectation %v", testCase.Name, testCase.IsNull) | ||
| } | ||
| if so.Sensitive != testCase.Sensitive { | ||
| t.Errorf("Key %s does not match sensitive expectation %v", testCase.Name, testCase.Sensitive) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.