From c33c8b013f556e09233cc24b1d32f561cfb074e5 Mon Sep 17 00:00:00 2001 From: Brandon Croft Date: Wed, 16 Mar 2022 22:47:06 -0600 Subject: [PATCH 1/7] fix: have `terraform output` adhere to authorization w/ cloud 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. --- internal/backend/local/backend_local_test.go | 4 + internal/cloud/backend.go | 3 +- internal/cloud/backend_state_test.go | 2 +- internal/cloud/state.go | 110 +++++++++++++++++++ internal/cloud/state_test.go | 82 ++++++++++++++ internal/cloud/testing.go | 66 ++++++++++- internal/cloud/tfe_client_mock.go | 45 ++++++++ internal/command/output.go | 13 +-- internal/states/remote/state.go | 13 +++ internal/states/remote/state_test.go | 28 +++++ internal/states/statemgr/filesystem.go | 14 +++ internal/states/statemgr/filesystem_test.go | 14 +++ internal/states/statemgr/lock.go | 4 + internal/states/statemgr/persistent.go | 11 ++ internal/states/statemgr/statemgr_fake.go | 8 ++ internal/states/statemgr/testing.go | 4 + 16 files changed, 408 insertions(+), 13 deletions(-) create mode 100644 internal/cloud/state.go create mode 100644 internal/cloud/state_test.go diff --git a/internal/backend/local/backend_local_test.go b/internal/backend/local/backend_local_test.go index 32675e0949e4..00af0a36324d 100644 --- a/internal/backend/local/backend_local_test.go +++ b/internal/backend/local/backend_local_test.go @@ -220,6 +220,10 @@ func (s *stateStorageThatFailsRefresh) State() *states.State { return nil } +func (s *stateStorageThatFailsRefresh) GetRootOutputValues() (map[string]*states.OutputValue, error) { + return nil, fmt.Errorf("unimplemented") +} + func (s *stateStorageThatFailsRefresh) WriteState(*states.State) error { return fmt.Errorf("unimplemented") } diff --git a/internal/cloud/backend.go b/internal/cloud/backend.go index 989fcc26eb69..fa3ed98f85d9 100644 --- a/internal/cloud/backend.go +++ b/internal/cloud/backend.go @@ -19,7 +19,6 @@ import ( "github.com/hashicorp/terraform/internal/backend" "github.com/hashicorp/terraform/internal/configs/configschema" "github.com/hashicorp/terraform/internal/plans" - "github.com/hashicorp/terraform/internal/states/remote" "github.com/hashicorp/terraform/internal/states/statemgr" "github.com/hashicorp/terraform/internal/terraform" "github.com/hashicorp/terraform/internal/tfdiags" @@ -628,7 +627,7 @@ func (b *Cloud) StateMgr(name string) (statemgr.Full, error) { runID: os.Getenv("TFE_RUN_ID"), } - return &remote.State{Client: client}, nil + return NewState(client), nil } // Operation implements backend.Enhanced. diff --git a/internal/cloud/backend_state_test.go b/internal/cloud/backend_state_test.go index d28d4d65b850..14142740005a 100644 --- a/internal/cloud/backend_state_test.go +++ b/internal/cloud/backend_state_test.go @@ -78,7 +78,7 @@ func TestRemoteClient_TestRemoteLocks(t *testing.T) { t.Fatalf("expected no error, got %v", err) } - remote.TestRemoteLocks(t, s1.(*remote.State).Client, s2.(*remote.State).Client) + remote.TestRemoteLocks(t, s1.(*State).Client, s2.(*State).Client) } func TestRemoteClient_withRunID(t *testing.T) { diff --git a/internal/cloud/state.go b/internal/cloud/state.go new file mode 100644 index 000000000000..739d422c5935 --- /dev/null +++ b/internal/cloud/state.go @@ -0,0 +1,110 @@ +package cloud + +import ( + "context" + "encoding/json" + "fmt" + + "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" + "github.com/zclconf/go-cty/cty/gocty" +) + +// 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 { + if output.Sensitive { + // Since this is a sensitive value, the output must be requested explicitly in order to + // read its value, which is assumed to be present by callers + sensitiveOutput, err := s.Client.client.StateVersionOutputs.Read(ctx, output.ID) + if err != nil { + return nil, fmt.Errorf("could not read state version output %s: %w", output.ID, err) + } + output.Value = sensitiveOutput.Value + } + + bufType, err := json.Marshal(output.DetailedType) + if err != nil { + return nil, fmt.Errorf("could not marshal output %s type: %w", output.ID, err) + } + + var ctype cty.Type + err = ctype.UnmarshalJSON(bufType) + if err != nil { + return nil, fmt.Errorf("could not interpret output %s type: %w", output.ID, err) + } + + cval, err := gocty.ToCtyValue(output.Value, ctype) + if err != nil { + return nil, fmt.Errorf("could not interpret value %v as type %s for output %s: %w", cval, ctype.FriendlyName(), output.ID, err) + } + + result[output.Name] = &states.OutputValue{ + Value: cval, + Sensitive: output.Sensitive, + } + } + + return result, nil +} diff --git a/internal/cloud/state_test.go b/internal/cloud/state_test.go new file mode 100644 index 000000000000..331daff6611a --- /dev/null +++ b/internal/cloud/state_test.go @@ -0,0 +1,82 @@ +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, + }, + { + Name: "object_output", + Sensitive: false, + IsNull: false, + }, + { + Name: "list_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) + } + } +} diff --git a/internal/cloud/testing.go b/internal/cloud/testing.go index 4297540fca8a..c800178e99c0 100644 --- a/internal/cloud/testing.go +++ b/internal/cloud/testing.go @@ -2,6 +2,7 @@ package cloud import ( "context" + "encoding/json" "fmt" "io" "net/http" @@ -117,7 +118,69 @@ func testRemoteClient(t *testing.T) remote.Client { t.Fatalf("error: %v", err) } - return raw.(*remote.State).Client + return raw.(*State).Client +} + +func testBackendWithOutputs(t *testing.T) (*Cloud, func()) { + b, cleanup := testBackendWithName(t) + + // Get a new mock client to use for adding outputs + mc := NewMockClient() + + mc.StateVersionOutputs.create("svo-abcd", &tfe.StateVersionOutput{ + ID: "svo-abcd", + Value: "foobar", + Sensitive: true, + Type: "string", + Name: "sensitive_output", + DetailedType: "string", + }) + + mc.StateVersionOutputs.create("svo-zyxw", &tfe.StateVersionOutput{ + ID: "svo-zyxw", + Value: "bazqux", + Type: "string", + Name: "nonsensitive_output", + DetailedType: "string", + }) + + var dt interface{} + var val interface{} + err := json.Unmarshal([]byte(`["object", {"foo":"string"}]`), &dt) + if err != nil { + t.Fatalf("could not unmarshal detailed type: %s", err) + } + err = json.Unmarshal([]byte(`{"foo":"bar"}`), &val) + if err != nil { + t.Fatalf("could not unmarshal value: %s", err) + } + mc.StateVersionOutputs.create("svo-efgh", &tfe.StateVersionOutput{ + ID: "svo-efgh", + Value: val, + Type: "object", + Name: "object_output", + DetailedType: dt, + }) + + err = json.Unmarshal([]byte(`["list", "bool"]`), &dt) + if err != nil { + t.Fatalf("could not unmarshal detailed type: %s", err) + } + err = json.Unmarshal([]byte(`[true, false, true, true]`), &val) + if err != nil { + t.Fatalf("could not unmarshal value: %s", err) + } + mc.StateVersionOutputs.create("svo-ijkl", &tfe.StateVersionOutput{ + ID: "svo-ijkl", + Value: val, + Type: "array", + Name: "list_output", + DetailedType: dt, + }) + + b.client.StateVersionOutputs = mc.StateVersionOutputs + + return b, cleanup } func testBackend(t *testing.T, obj cty.Value) (*Cloud, func()) { @@ -149,6 +212,7 @@ func testBackend(t *testing.T, obj cty.Value) (*Cloud, func()) { b.client.PolicyChecks = mc.PolicyChecks b.client.Runs = mc.Runs b.client.StateVersions = mc.StateVersions + b.client.StateVersionOutputs = mc.StateVersionOutputs b.client.Variables = mc.Variables b.client.Workspaces = mc.Workspaces diff --git a/internal/cloud/tfe_client_mock.go b/internal/cloud/tfe_client_mock.go index cd27af8f400f..2ac259a2eba8 100644 --- a/internal/cloud/tfe_client_mock.go +++ b/internal/cloud/tfe_client_mock.go @@ -30,6 +30,7 @@ type MockClient struct { PolicyChecks *MockPolicyChecks Runs *MockRuns StateVersions *MockStateVersions + StateVersionOutputs *MockStateVersionOutputs Variables *MockVariables Workspaces *MockWorkspaces } @@ -44,6 +45,7 @@ func NewMockClient() *MockClient { c.PolicyChecks = newMockPolicyChecks(c) c.Runs = newMockRuns(c) c.StateVersions = newMockStateVersions(c) + c.StateVersionOutputs = newMockStateVersionOutputs(c) c.Variables = newMockVariables(c) c.Workspaces = newMockWorkspaces(c) return c @@ -1029,6 +1031,49 @@ func (m *MockStateVersions) ListOutputs(ctx context.Context, svID string, option panic("not implemented") } +type MockStateVersionOutputs struct { + client *MockClient + outputs map[string]*tfe.StateVersionOutput +} + +func newMockStateVersionOutputs(client *MockClient) *MockStateVersionOutputs { + return &MockStateVersionOutputs{ + client: client, + outputs: make(map[string]*tfe.StateVersionOutput), + } +} + +// This is a helper function in order to create mocks to be read later +func (m *MockStateVersionOutputs) create(id string, svo *tfe.StateVersionOutput) { + m.outputs[id] = svo +} + +func (m *MockStateVersionOutputs) Read(ctx context.Context, outputID string) (*tfe.StateVersionOutput, error) { + result, ok := m.outputs[outputID] + if !ok { + return nil, tfe.ErrResourceNotFound + } + + return result, nil +} + +func (m *MockStateVersionOutputs) ReadCurrent(ctx context.Context, workspaceID string) (*tfe.StateVersionOutputsList, error) { + svl := &tfe.StateVersionOutputsList{} + for _, sv := range m.outputs { + svl.Items = append(svl.Items, sv) + } + + svl.Pagination = &tfe.Pagination{ + CurrentPage: 1, + NextPage: 1, + PreviousPage: 1, + TotalPages: 1, + TotalCount: len(svl.Items), + } + + return svl, nil +} + type MockVariables struct { client *MockClient workspaces map[string]*tfe.VariableList diff --git a/internal/command/output.go b/internal/command/output.go index f53f6e206ac1..0bcde54e8fae 100644 --- a/internal/command/output.go +++ b/internal/command/output.go @@ -82,17 +82,12 @@ func (c *OutputCommand) Outputs(statePath string) (map[string]*states.OutputValu return nil, diags } - if err := stateStore.RefreshState(); err != nil { - diags = diags.Append(fmt.Errorf("Failed to load state: %s", err)) - return nil, diags - } - - state := stateStore.State() - if state == nil { - state = states.NewState() + output, err := stateStore.GetRootOutputValues() + if err != nil { + return nil, diags.Append(err) } - return state.RootModule().OutputValues, nil + return output, diags } func (c *OutputCommand) Help() string { diff --git a/internal/states/remote/state.go b/internal/states/remote/state.go index ca939a96a312..5348abb8f031 100644 --- a/internal/states/remote/state.go +++ b/internal/states/remote/state.go @@ -46,6 +46,19 @@ func (s *State) State() *states.State { return s.state.DeepCopy() } +func (s *State) GetRootOutputValues() (map[string]*states.OutputValue, error) { + if err := s.RefreshState(); err != nil { + return nil, fmt.Errorf("Failed to load state: %s", err) + } + + state := s.State() + if state == nil { + state = states.NewState() + } + + return state.RootModule().OutputValues, nil +} + // StateForMigration is part of our implementation of statemgr.Migrator. func (s *State) StateForMigration() *statefile.File { s.mu.Lock() diff --git a/internal/states/remote/state_test.go b/internal/states/remote/state_test.go index 9687d11f1b5a..1089ba1aaeed 100644 --- a/internal/states/remote/state_test.go +++ b/internal/states/remote/state_test.go @@ -19,6 +19,7 @@ func TestState_impl(t *testing.T) { 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) } @@ -276,6 +277,33 @@ func TestStatePersist(t *testing.T) { } } +func TestState_GetRootOutputValues(t *testing.T) { + // Initial setup of state with outputs already defined + mgr := &State{ + Client: &mockClient{ + current: []byte(` + { + "version": 4, + "lineage": "mock-lineage", + "serial": 1, + "terraform_version":"0.0.0", + "outputs": {"foo": {"value":"bar", "type": "string"}}, + "resources": [] + } + `), + }, + } + + outputs, err := mgr.GetRootOutputValues() + if err != nil { + t.Errorf("Expected GetRootOutputValues to not return an error, but it returned %v", err) + } + + if len(outputs) != 1 { + t.Errorf("Expected %d outputs, but received %d", 1, len(outputs)) + } +} + type migrationTestCase struct { name string // A function to generate a statefile diff --git a/internal/states/statemgr/filesystem.go b/internal/states/statemgr/filesystem.go index 7cd19e8b0b52..1406f046563b 100644 --- a/internal/states/statemgr/filesystem.go +++ b/internal/states/statemgr/filesystem.go @@ -233,6 +233,20 @@ func (s *Filesystem) RefreshState() error { return s.refreshState() } +func (s *Filesystem) GetRootOutputValues() (map[string]*states.OutputValue, error) { + err := s.RefreshState() + if err != nil { + return nil, err + } + + state := s.State() + if state == nil { + state = states.NewState() + } + + return state.RootModule().OutputValues, nil +} + func (s *Filesystem) refreshState() error { var reader io.Reader diff --git a/internal/states/statemgr/filesystem_test.go b/internal/states/statemgr/filesystem_test.go index 1d319920f536..2f6285fbd1ee 100644 --- a/internal/states/statemgr/filesystem_test.go +++ b/internal/states/statemgr/filesystem_test.go @@ -336,6 +336,7 @@ func TestFilesystem_impl(t *testing.T) { var _ Writer = new(Filesystem) var _ Persister = new(Filesystem) var _ Refresher = new(Filesystem) + var _ OutputReader = new(Filesystem) var _ Locker = new(Filesystem) } @@ -410,6 +411,19 @@ func TestFilesystem_refreshWhileLocked(t *testing.T) { } } +func TestFilesystem_GetRootOutputValues(t *testing.T) { + fs := testFilesystem(t) + + outputs, err := fs.GetRootOutputValues() + if err != nil { + t.Errorf("Expected GetRootOutputValues to not return an error, but it returned %v", err) + } + + if len(outputs) != 2 { + t.Errorf("Expected %d outputs, but received %d", 2, len(outputs)) + } +} + func testOverrideVersion(t *testing.T, v string) func() { oldVersionStr := tfversion.Version oldPrereleaseStr := tfversion.Prerelease diff --git a/internal/states/statemgr/lock.go b/internal/states/statemgr/lock.go index 190e06ea7de8..79c149fe736e 100644 --- a/internal/states/statemgr/lock.go +++ b/internal/states/statemgr/lock.go @@ -15,6 +15,10 @@ func (s *LockDisabled) State() *states.State { return s.Inner.State() } +func (s *LockDisabled) GetRootOutputValues() (map[string]*states.OutputValue, error) { + return s.Inner.GetRootOutputValues() +} + func (s *LockDisabled) WriteState(v *states.State) error { return s.Inner.WriteState(v) } diff --git a/internal/states/statemgr/persistent.go b/internal/states/statemgr/persistent.go index c15e84af2dc3..73b2124fc000 100644 --- a/internal/states/statemgr/persistent.go +++ b/internal/states/statemgr/persistent.go @@ -2,6 +2,7 @@ package statemgr import ( version "github.com/hashicorp/go-version" + "github.com/hashicorp/terraform/internal/states" ) // Persistent is a union of the Refresher and Persistent interfaces, for types @@ -16,6 +17,16 @@ import ( type Persistent interface { Refresher Persister + OutputReader +} + +// OutputReader is the interface for managers that fetches output values from state +// or another source. This is a refinement of fetching the entire state and digging +// the output values from it because enhanced backends can apply special permissions +// to differentiate reading the state and reading the outputs within the state. +type OutputReader interface { + // GetRootOutputValues fetches the root module output values from state or another source + GetRootOutputValues() (map[string]*states.OutputValue, error) } // Refresher is the interface for managers that can read snapshots from diff --git a/internal/states/statemgr/statemgr_fake.go b/internal/states/statemgr/statemgr_fake.go index a547c1bc7c4f..8d88e4d24e7e 100644 --- a/internal/states/statemgr/statemgr_fake.go +++ b/internal/states/statemgr/statemgr_fake.go @@ -65,6 +65,10 @@ func (m *fakeFull) PersistState() error { return m.fakeP.WriteState(m.t.State()) } +func (m *fakeFull) GetRootOutputValues() (map[string]*states.OutputValue, error) { + return m.State().RootModule().OutputValues, nil +} + func (m *fakeFull) Lock(info *LockInfo) (string, error) { m.lockLock.Lock() defer m.lockLock.Unlock() @@ -111,6 +115,10 @@ func (m *fakeErrorFull) State() *states.State { return nil } +func (m *fakeErrorFull) GetRootOutputValues() (map[string]*states.OutputValue, error) { + return nil, errors.New("fake state manager error") +} + func (m *fakeErrorFull) WriteState(s *states.State) error { return errors.New("fake state manager error") } diff --git a/internal/states/statemgr/testing.go b/internal/states/statemgr/testing.go index 82cecc0de55e..171b21ad2ea0 100644 --- a/internal/states/statemgr/testing.go +++ b/internal/states/statemgr/testing.go @@ -155,5 +155,9 @@ func TestFullInitialState() *states.State { Module: addrs.RootModule, } childMod.SetResourceProvider(rAddr, providerAddr) + + state.RootModule().SetOutputValue("sensitive_output", cty.StringVal("it's a secret"), true) + state.RootModule().SetOutputValue("nonsensitive_output", cty.StringVal("hello, world!"), false) + return state } From e794efc31e732ce8aff5a5aa524dda55602eb97f Mon Sep 17 00:00:00 2001 From: Brandon Croft Date: Mon, 4 Apr 2022 13:39:27 -0600 Subject: [PATCH 2/7] better errors from terraform output when cloud is configured --- internal/cloud/state.go | 2 +- internal/states/state.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/cloud/state.go b/internal/cloud/state.go index 739d422c5935..14e381256e61 100644 --- a/internal/cloud/state.go +++ b/internal/cloud/state.go @@ -68,7 +68,7 @@ func (s *State) GetRootOutputValues() (map[string]*states.OutputValue, error) { 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) + return nil, fmt.Errorf("could not read state version outputs: %w", err) } result := make(map[string]*states.OutputValue) diff --git a/internal/states/state.go b/internal/states/state.go index 28d962786d0c..90c73c11582e 100644 --- a/internal/states/state.go +++ b/internal/states/state.go @@ -19,7 +19,7 @@ import ( // so when accessing a State object concurrently it is the caller's // responsibility to ensure that only one write is in progress at a time // and that reads only occur when no write is in progress. The most common -// way to acheive this is to wrap the State in a SyncState and use the +// way to achieve this is to wrap the State in a SyncState and use the // higher-level atomic operations supported by that type. type State struct { // Modules contains the state for each module. The keys in this map are @@ -412,7 +412,7 @@ func (s *State) MoveAbsResource(src, dst addrs.AbsResource) { // MaybeMoveAbsResource moves the given src AbsResource's current state to the // new dst address. This function will succeed if both the src address does not // exist in state and the dst address does; the return value indicates whether -// or not the move occured. This function will panic if either the src does not +// or not the move occurred. This function will panic if either the src does not // exist or the dst does exist (but not both). func (s *State) MaybeMoveAbsResource(src, dst addrs.AbsResource) bool { // Get the source and destinatation addresses from state. From 15b938a0c84878fb1eef092d45cf11aa5850ef82 Mon Sep 17 00:00:00 2001 From: Brandon Croft Date: Mon, 25 Jul 2022 13:50:57 -0600 Subject: [PATCH 3/7] Update to go-tfe v1.6.0 --- go.mod | 4 ++-- go.sum | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/go.mod b/go.mod index f6283047a0b5..a8ff0c7f8564 100644 --- a/go.mod +++ b/go.mod @@ -40,7 +40,7 @@ require ( github.com/hashicorp/go-multierror v1.1.1 github.com/hashicorp/go-plugin v1.4.3 github.com/hashicorp/go-retryablehttp v0.7.1 - github.com/hashicorp/go-tfe v1.5.0 + github.com/hashicorp/go-tfe v1.6.0 github.com/hashicorp/go-uuid v1.0.3 github.com/hashicorp/go-version v1.6.0 github.com/hashicorp/hcl v0.0.0-20170504190234-a4b07c25de5f @@ -177,7 +177,7 @@ require ( go.opencensus.io v0.23.0 // indirect golang.org/x/exp/typeparams v0.0.0-20220218215828-6cf2b201936e // indirect golang.org/x/lint v0.0.0-20210508222113-6edffad5e616 // indirect - golang.org/x/time v0.0.0-20220224211638-0e9765cccd65 // indirect + golang.org/x/time v0.0.0-20220722155302-e5dcc9cfc0b9 // indirect google.golang.org/appengine v1.6.7 // indirect google.golang.org/genproto v0.0.0-20210602131652-f16073e35f0c // indirect gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c // indirect diff --git a/go.sum b/go.sum index 275b0d2c00b8..128b5ab61175 100644 --- a/go.sum +++ b/go.sum @@ -375,8 +375,8 @@ github.com/hashicorp/go-slug v0.9.1/go.mod h1:Ib+IWBYfEfJGI1ZyXMGNbu2BU+aa3Dzu41 github.com/hashicorp/go-sockaddr v1.0.0 h1:GeH6tui99pF4NJgfnhp+L6+FfobzVW3Ah46sLo0ICXs= github.com/hashicorp/go-sockaddr v1.0.0/go.mod h1:7Xibr9yA9JjQq1JpNB2Vw7kxv8xerXegt+ozgdvDeDU= github.com/hashicorp/go-syslog v1.0.0/go.mod h1:qPfqrKkXGihmCqbJM2mZgkZGvKG1dFdvsLplgctolz4= -github.com/hashicorp/go-tfe v1.5.0 h1:MtABkqH2s6lRFl8HaGt0qESLGAyrmMAFfecsEm+13K8= -github.com/hashicorp/go-tfe v1.5.0/go.mod h1:E8a90lC4kjU5Lc2c0D+SnWhUuyuoCIVm4Ewzv3jCD3A= +github.com/hashicorp/go-tfe v1.6.0 h1:lRfyTVLBP1njo2wShE9FimALzVZBfOqMGNuBdsor38w= +github.com/hashicorp/go-tfe v1.6.0/go.mod h1:E8a90lC4kjU5Lc2c0D+SnWhUuyuoCIVm4Ewzv3jCD3A= github.com/hashicorp/go-uuid v1.0.0/go.mod h1:6SBZvOh/SIDV7/2o3Jml5SYk/TvGqwFJ/bN7x4byOro= github.com/hashicorp/go-uuid v1.0.1/go.mod h1:6SBZvOh/SIDV7/2o3Jml5SYk/TvGqwFJ/bN7x4byOro= github.com/hashicorp/go-uuid v1.0.2/go.mod h1:6SBZvOh/SIDV7/2o3Jml5SYk/TvGqwFJ/bN7x4byOro= @@ -858,8 +858,8 @@ golang.org/x/time v0.0.0-20181108054448-85acf8d2951c/go.mod h1:tRJNPiyCQ0inRvYxb golang.org/x/time v0.0.0-20190308202827-9d24e82272b4/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ= golang.org/x/time v0.0.0-20191024005414-555d28b269f0/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ= golang.org/x/time v0.0.0-20210723032227-1f47c861a9ac/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ= -golang.org/x/time v0.0.0-20220224211638-0e9765cccd65 h1:M73Iuj3xbbb9Uk1DYhzydthsj6oOd6l9bpuFcNoUvTs= -golang.org/x/time v0.0.0-20220224211638-0e9765cccd65/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ= +golang.org/x/time v0.0.0-20220722155302-e5dcc9cfc0b9 h1:ftMN5LMiBFjbzleLqtoBZk7KdJwhuybIU+FckUHgoyQ= +golang.org/x/time v0.0.0-20220722155302-e5dcc9cfc0b9/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ= golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= golang.org/x/tools v0.0.0-20190114222345-bf090417da8b/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= golang.org/x/tools v0.0.0-20190226205152-f727befe758c/go.mod h1:9Yl7xja0Znq3iFh3HoIrodX9oNMXvdceNzlUR8zjMvY= From 166d21b20bbef081ad1d11a871f0e4762cf9b281 Mon Sep 17 00:00:00 2001 From: Brandon Croft Date: Mon, 25 Jul 2022 14:48:32 -0600 Subject: [PATCH 4/7] refactor: extract cloud state ctyValue conversion function --- internal/cloud/backend_state_test.go | 2 +- internal/cloud/state.go | 50 ++++++++++++++++++++-------- 2 files changed, 38 insertions(+), 14 deletions(-) diff --git a/internal/cloud/backend_state_test.go b/internal/cloud/backend_state_test.go index 14142740005a..3b9833c38e5e 100644 --- a/internal/cloud/backend_state_test.go +++ b/internal/cloud/backend_state_test.go @@ -30,7 +30,7 @@ func TestRemoteClient_stateVersionCreated(t *testing.T) { t.Fatalf("error: %v", err) } - client := raw.(*remote.State).Client + client := raw.(*State).Client err = client.Put(([]byte)(` { diff --git a/internal/cloud/state.go b/internal/cloud/state.go index 14e381256e61..bb4db138a1f1 100644 --- a/internal/cloud/state.go +++ b/internal/cloud/state.go @@ -3,8 +3,11 @@ package cloud import ( "context" "encoding/json" + "errors" "fmt" + "strings" + "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" @@ -21,6 +24,11 @@ type State struct { delegate remote.State } +const ErrStateVersionOutputUpgrade = ` +The remote state version was created by a version of terraform older than +1.3.0 and must be updated before outputs can be read by terraform. +` + // Proof that cloud State is a statemgr.Persistent interface var _ statemgr.Persistent = (*State)(nil) @@ -74,6 +82,10 @@ func (s *State) GetRootOutputValues() (map[string]*states.OutputValue, error) { result := make(map[string]*states.OutputValue) for _, output := range so.Items { + if output.DetailedType == nil { + return nil, errors.New(strings.TrimSpace(ErrStateVersionOutputUpgrade)) + } + if output.Sensitive { // Since this is a sensitive value, the output must be requested explicitly in order to // read its value, which is assumed to be present by callers @@ -84,20 +96,9 @@ func (s *State) GetRootOutputValues() (map[string]*states.OutputValue, error) { output.Value = sensitiveOutput.Value } - bufType, err := json.Marshal(output.DetailedType) - if err != nil { - return nil, fmt.Errorf("could not marshal output %s type: %w", output.ID, err) - } - - var ctype cty.Type - err = ctype.UnmarshalJSON(bufType) + cval, err := tfeOutputToCtyValue(*output) if err != nil { - return nil, fmt.Errorf("could not interpret output %s type: %w", output.ID, err) - } - - cval, err := gocty.ToCtyValue(output.Value, ctype) - if err != nil { - return nil, fmt.Errorf("could not interpret value %v as type %s for output %s: %w", cval, ctype.FriendlyName(), output.ID, err) + return nil, fmt.Errorf("could not decode output %s (ID %s)", output.Name, output.ID) } result[output.Name] = &states.OutputValue{ @@ -108,3 +109,26 @@ func (s *State) GetRootOutputValues() (map[string]*states.OutputValue, error) { return result, nil } + +// tfeOutputToCtyValue decodes a combination of TFE output value and detailed-type to create a +// cty value that is suitable for use in terraform. +func tfeOutputToCtyValue(output tfe.StateVersionOutput) (cty.Value, error) { + var result cty.Value + bufType, err := json.Marshal(output.DetailedType) + if err != nil { + return result, fmt.Errorf("could not marshal output %s type: %w", output.ID, err) + } + + var ctype cty.Type + err = ctype.UnmarshalJSON(bufType) + if err != nil { + return result, fmt.Errorf("could not interpret output %s type: %w", output.ID, err) + } + + result, err = gocty.ToCtyValue(output.Value, ctype) + if err != nil { + return result, fmt.Errorf("could not interpret value %v as type %s for output %s: %w", result, ctype.FriendlyName(), output.ID, err) + } + + return result, nil +} From e1fa690879711dcdef3a7bc12560de7b317c8b01 Mon Sep 17 00:00:00 2001 From: Brandon Croft Date: Mon, 25 Jul 2022 14:49:31 -0600 Subject: [PATCH 5/7] style: goimports fixes --- internal/backend/local/backend_local_test.go | 3 ++- internal/cloud/backend.go | 9 +++++---- internal/cloud/state.go | 5 +++-- internal/cloud/state_test.go | 1 + internal/cloud/testing.go | 5 +++-- internal/states/remote/state.go | 1 + internal/states/statemgr/persistent.go | 1 + 7 files changed, 16 insertions(+), 9 deletions(-) diff --git a/internal/backend/local/backend_local_test.go b/internal/backend/local/backend_local_test.go index 00af0a36324d..05573f0f9d97 100644 --- a/internal/backend/local/backend_local_test.go +++ b/internal/backend/local/backend_local_test.go @@ -6,6 +6,8 @@ import ( "path/filepath" "testing" + "github.com/zclconf/go-cty/cty" + "github.com/hashicorp/terraform/internal/backend" "github.com/hashicorp/terraform/internal/command/arguments" "github.com/hashicorp/terraform/internal/command/clistate" @@ -20,7 +22,6 @@ import ( "github.com/hashicorp/terraform/internal/states/statemgr" "github.com/hashicorp/terraform/internal/terminal" "github.com/hashicorp/terraform/internal/tfdiags" - "github.com/zclconf/go-cty/cty" ) func TestLocalRun(t *testing.T) { diff --git a/internal/cloud/backend.go b/internal/cloud/backend.go index fa3ed98f85d9..d08754d230a5 100644 --- a/internal/cloud/backend.go +++ b/internal/cloud/backend.go @@ -16,6 +16,11 @@ import ( version "github.com/hashicorp/go-version" svchost "github.com/hashicorp/terraform-svchost" "github.com/hashicorp/terraform-svchost/disco" + "github.com/mitchellh/cli" + "github.com/mitchellh/colorstring" + "github.com/zclconf/go-cty/cty" + "github.com/zclconf/go-cty/cty/gocty" + "github.com/hashicorp/terraform/internal/backend" "github.com/hashicorp/terraform/internal/configs/configschema" "github.com/hashicorp/terraform/internal/plans" @@ -23,10 +28,6 @@ import ( "github.com/hashicorp/terraform/internal/terraform" "github.com/hashicorp/terraform/internal/tfdiags" tfversion "github.com/hashicorp/terraform/version" - "github.com/mitchellh/cli" - "github.com/mitchellh/colorstring" - "github.com/zclconf/go-cty/cty" - "github.com/zclconf/go-cty/cty/gocty" backendLocal "github.com/hashicorp/terraform/internal/backend/local" ) diff --git a/internal/cloud/state.go b/internal/cloud/state.go index bb4db138a1f1..fabc1b8b5d93 100644 --- a/internal/cloud/state.go +++ b/internal/cloud/state.go @@ -8,11 +8,12 @@ import ( "strings" "github.com/hashicorp/go-tfe" + "github.com/zclconf/go-cty/cty" + "github.com/zclconf/go-cty/cty/gocty" + "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" - "github.com/zclconf/go-cty/cty/gocty" ) // State is similar to remote State and delegates to it, except in the case of output values, diff --git a/internal/cloud/state_test.go b/internal/cloud/state_test.go index 331daff6611a..738ae721a44d 100644 --- a/internal/cloud/state_test.go +++ b/internal/cloud/state_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/hashicorp/go-tfe" + "github.com/hashicorp/terraform/internal/states/statemgr" ) diff --git a/internal/cloud/testing.go b/internal/cloud/testing.go index c800178e99c0..cfb49cf9b352 100644 --- a/internal/cloud/testing.go +++ b/internal/cloud/testing.go @@ -15,6 +15,9 @@ import ( svchost "github.com/hashicorp/terraform-svchost" "github.com/hashicorp/terraform-svchost/auth" "github.com/hashicorp/terraform-svchost/disco" + "github.com/mitchellh/cli" + "github.com/zclconf/go-cty/cty" + "github.com/hashicorp/terraform/internal/backend" "github.com/hashicorp/terraform/internal/configs" "github.com/hashicorp/terraform/internal/configs/configschema" @@ -24,8 +27,6 @@ import ( "github.com/hashicorp/terraform/internal/terraform" "github.com/hashicorp/terraform/internal/tfdiags" "github.com/hashicorp/terraform/version" - "github.com/mitchellh/cli" - "github.com/zclconf/go-cty/cty" backendLocal "github.com/hashicorp/terraform/internal/backend/local" ) diff --git a/internal/states/remote/state.go b/internal/states/remote/state.go index 5348abb8f031..ae123f8e3647 100644 --- a/internal/states/remote/state.go +++ b/internal/states/remote/state.go @@ -6,6 +6,7 @@ import ( "sync" uuid "github.com/hashicorp/go-uuid" + "github.com/hashicorp/terraform/internal/states" "github.com/hashicorp/terraform/internal/states/statefile" "github.com/hashicorp/terraform/internal/states/statemgr" diff --git a/internal/states/statemgr/persistent.go b/internal/states/statemgr/persistent.go index 73b2124fc000..fde4a7f0f8ac 100644 --- a/internal/states/statemgr/persistent.go +++ b/internal/states/statemgr/persistent.go @@ -2,6 +2,7 @@ package statemgr import ( version "github.com/hashicorp/go-version" + "github.com/hashicorp/terraform/internal/states" ) From 0139e75a1a71647c4dd7ac0e4435d0c32fe38d1d Mon Sep 17 00:00:00 2001 From: Brandon Croft Date: Mon, 25 Jul 2022 17:17:24 -0600 Subject: [PATCH 6/7] feature: fall back to reading entire state when detailed-type is not available --- internal/cloud/state.go | 33 ++++++++++++++++++++++++++++----- 1 file changed, 28 insertions(+), 5 deletions(-) diff --git a/internal/cloud/state.go b/internal/cloud/state.go index fabc1b8b5d93..1fe7fa3161ec 100644 --- a/internal/cloud/state.go +++ b/internal/cloud/state.go @@ -25,10 +25,13 @@ type State struct { delegate remote.State } -const ErrStateVersionOutputUpgrade = ` -The remote state version was created by a version of terraform older than -1.3.0 and must be updated before outputs can be read by terraform. -` +var ErrNotEnoughOutputsTypeInformation = errors.New("not enough type information to read outputs") +var ErrStateVersionOutputsUpgradeState = errors.New(strings.TrimSpace(` +You are not authorized to read the full state version containing outputs. +State versions created by terraform v1.3.0 and newer do not require this level +of authorization and therefore this error can be fixed by upgrading the remote +state version. +`)) // Proof that cloud State is a statemgr.Persistent interface var _ statemgr.Persistent = (*State)(nil) @@ -70,6 +73,22 @@ func (s *State) WriteState(state *states.State) error { return s.delegate.WriteState(state) } +func (s *State) fallbackReadOutputsFromFullState() (map[string]*states.OutputValue, error) { + if err := s.RefreshState(); err != nil { + if strings.HasSuffix(err.Error(), "failed to retrieve state: forbidden") { + return nil, ErrStateVersionOutputsUpgradeState + } + return nil, fmt.Errorf("failed to load state: %w", err) + } + + state := s.State() + if state == nil { + state = states.NewState() + } + + return state.RootModule().OutputValues, nil +} + // GetRootOutputValues fetches output values from Terraform Cloud func (s *State) GetRootOutputValues() (map[string]*states.OutputValue, error) { ctx := context.Background() @@ -84,7 +103,11 @@ func (s *State) GetRootOutputValues() (map[string]*states.OutputValue, error) { for _, output := range so.Items { if output.DetailedType == nil { - return nil, errors.New(strings.TrimSpace(ErrStateVersionOutputUpgrade)) + // If there is no detailed type information available, this state was probably created + // with a version of terraform < 1.3.0. In this case, we'll eject completely from this + // function and fall back to the old behavior of reading the entire state file, which + // requires a higher level of authorization. + return s.fallbackReadOutputsFromFullState() } if output.Sensitive { From 50d48c635eac7cdf81672ef77663100ecd3c741e Mon Sep 17 00:00:00 2001 From: Brandon Croft Date: Thu, 28 Jul 2022 11:44:16 -0600 Subject: [PATCH 7/7] remove unused error and fix output authorization detection --- internal/cloud/state.go | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/internal/cloud/state.go b/internal/cloud/state.go index 1fe7fa3161ec..73bea8ba5d9c 100644 --- a/internal/cloud/state.go +++ b/internal/cloud/state.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + "log" "strings" "github.com/hashicorp/go-tfe" @@ -25,12 +26,11 @@ type State struct { delegate remote.State } -var ErrNotEnoughOutputsTypeInformation = errors.New("not enough type information to read outputs") -var ErrStateVersionOutputsUpgradeState = errors.New(strings.TrimSpace(` +var ErrStateVersionUnauthorizedUpgradeState = errors.New(strings.TrimSpace(` You are not authorized to read the full state version containing outputs. State versions created by terraform v1.3.0 and newer do not require this level -of authorization and therefore this error can be fixed by upgrading the remote -state version. +of authorization and therefore this error can usually be fixed by upgrading the +remote state version. `)) // Proof that cloud State is a statemgr.Persistent interface @@ -74,16 +74,18 @@ func (s *State) WriteState(state *states.State) error { } func (s *State) fallbackReadOutputsFromFullState() (map[string]*states.OutputValue, error) { + log.Printf("[DEBUG] falling back to reading full state") + if err := s.RefreshState(); err != nil { - if strings.HasSuffix(err.Error(), "failed to retrieve state: forbidden") { - return nil, ErrStateVersionOutputsUpgradeState - } return nil, fmt.Errorf("failed to load state: %w", err) } state := s.State() if state == nil { - state = states.NewState() + // We know that there is supposed to be state (and this is not simply a new workspace + // without state) because the fallback is only invoked when outputs are present but + // detailed types are not available. + return nil, ErrStateVersionUnauthorizedUpgradeState } return state.RootModule().OutputValues, nil