From bdef106a8b85dfc0f1c099462a33f082ee5da631 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 30 Sep 2020 10:45:55 -0400 Subject: [PATCH 1/7] WithPath should only modify the copy --- terraform/eval_context_builtin.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/terraform/eval_context_builtin.go b/terraform/eval_context_builtin.go index 17e84de6e..67264948e 100644 --- a/terraform/eval_context_builtin.go +++ b/terraform/eval_context_builtin.go @@ -79,8 +79,8 @@ type BuiltinEvalContext struct { var _ EvalContext = (*BuiltinEvalContext)(nil) func (ctx *BuiltinEvalContext) WithPath(path addrs.ModuleInstance) EvalContext { - ctx.pathSet = true newCtx := *ctx + newCtx.pathSet = true newCtx.PathValue = path return &newCtx } From 8c699fbe32e8d69548f3655152adc1231f01f8e9 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 30 Sep 2020 10:59:57 -0400 Subject: [PATCH 2/7] Unsynchronized maps in test --- terraform/context_apply_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 2d3cb8752..4596ae09d 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -10135,9 +10135,15 @@ func TestContext2Apply_ProviderMeta_apply_set(t *testing.T) { }, }, } + + var pmMu sync.Mutex arcPMs := map[string]cty.Value{} + p.ApplyResourceChangeFn = func(req providers.ApplyResourceChangeRequest) providers.ApplyResourceChangeResponse { + pmMu.Lock() + defer pmMu.Unlock() arcPMs[req.TypeName] = req.ProviderMeta + s := req.PlannedState.AsValueMap() s["id"] = cty.StringVal("ID") return providers.ApplyResourceChangeResponse{ @@ -10209,9 +10215,13 @@ func TestContext2Apply_ProviderMeta_apply_unset(t *testing.T) { }, }, } + var pmMu sync.Mutex arcPMs := map[string]cty.Value{} p.ApplyResourceChangeFn = func(req providers.ApplyResourceChangeRequest) providers.ApplyResourceChangeResponse { + pmMu.Lock() + defer pmMu.Unlock() arcPMs[req.TypeName] = req.ProviderMeta + s := req.PlannedState.AsValueMap() s["id"] = cty.StringVal("ID") return providers.ApplyResourceChangeResponse{ From 8dc6f518c22a8e6d8a4abbc327c63a89f1f25834 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 30 Sep 2020 11:49:04 -0400 Subject: [PATCH 3/7] don't use same state in Validate for refreshState --- terraform/context.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/terraform/context.go b/terraform/context.go index 4dbb18012..ccb8941bb 100644 --- a/terraform/context.go +++ b/terraform/context.go @@ -743,12 +743,11 @@ func (c *Context) graphWalker(operation walkOperation) *ContextGraphWalker { switch operation { case walkValidate: // validate should not use any state - s := states.NewState() - state = s.SyncWrapper() + state = states.NewState().SyncWrapper() // validate currently uses the plan graph, so we have to populate the // refreshState. - refreshState = s.SyncWrapper() + refreshState = states.NewState().SyncWrapper() case walkPlan: state = c.state.SyncWrapper() From b464bcbc5d98c3294f9d23f5bcfca4ff2ffe2a33 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 30 Sep 2020 12:13:22 -0400 Subject: [PATCH 4/7] copy the refreshed state at the end of plan Otherwise we may end up with the same state value for the state and refreshState when re-using the context. --- terraform/context.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/terraform/context.go b/terraform/context.go index ccb8941bb..e3f26ef59 100644 --- a/terraform/context.go +++ b/terraform/context.go @@ -562,11 +562,13 @@ The -target option is not for routine use, and is provided only for exceptional p.Changes = c.changes c.refreshState.SyncWrapper().RemovePlannedResourceInstanceObjects() - p.State = c.refreshState.DeepCopy() + + refreshedState := c.refreshState.DeepCopy() + p.State = refreshedState // replace the working state with the updated state, so that immediate calls // to Apply work as expected. - c.state = c.refreshState + c.state = refreshedState return p, diags } From ab6d6f99aeb3c37ea7dbe15093322d5b9a4113d7 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 30 Sep 2020 13:36:04 -0400 Subject: [PATCH 5/7] fix races in remote backend mock --- backend/remote/backend_mock.go | 36 +++++++++++++++++++++++++++++++--- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/backend/remote/backend_mock.go b/backend/remote/backend_mock.go index 9b9b51126..eaa81cbfc 100644 --- a/backend/remote/backend_mock.go +++ b/backend/remote/backend_mock.go @@ -12,10 +12,12 @@ import ( "os" "path/filepath" "strings" + "sync" "time" tfe "github.com/hashicorp/go-tfe" "github.com/hashicorp/terraform/terraform" + "github.com/mitchellh/copystructure" ) type mockClient struct { @@ -693,6 +695,8 @@ func (m *mockPolicyChecks) Logs(ctx context.Context, policyCheckID string) (io.R } type mockRuns struct { + sync.Mutex + client *mockClient runs map[string]*tfe.Run workspaces map[string][]*tfe.Run @@ -712,13 +716,21 @@ func newMockRuns(client *mockClient) *mockRuns { } func (m *mockRuns) List(ctx context.Context, workspaceID string, options tfe.RunListOptions) (*tfe.RunList, error) { + m.Lock() + defer m.Unlock() + w, ok := m.client.Workspaces.workspaceIDs[workspaceID] if !ok { return nil, tfe.ErrResourceNotFound } - rl := &tfe.RunList{ - Items: m.workspaces[w.ID], + rl := &tfe.RunList{} + for _, run := range m.workspaces[w.ID] { + rc, err := copystructure.Copy(run) + if err != nil { + panic(err) + } + rl.Items = append(rl.Items, rc.(*tfe.Run)) } rl.Pagination = &tfe.Pagination{ @@ -733,6 +745,9 @@ func (m *mockRuns) List(ctx context.Context, workspaceID string, options tfe.Run } func (m *mockRuns) Create(ctx context.Context, options tfe.RunCreateOptions) (*tfe.Run, error) { + m.Lock() + defer m.Unlock() + a, err := m.client.Applies.create(options.ConfigurationVersion.ID, options.Workspace.ID) if err != nil { return nil, err @@ -798,6 +813,9 @@ func (m *mockRuns) Create(ctx context.Context, options tfe.RunCreateOptions) (*t } func (m *mockRuns) Read(ctx context.Context, runID string) (*tfe.Run, error) { + m.Lock() + defer m.Unlock() + r, ok := m.runs[runID] if !ok { return nil, tfe.ErrResourceNotFound @@ -833,10 +851,19 @@ func (m *mockRuns) Read(ctx context.Context, runID string) (*tfe.Run, error) { } } - return r, nil + // we must return a copy for the client + rc, err := copystructure.Copy(r) + if err != nil { + panic(err) + } + + return rc.(*tfe.Run), nil } func (m *mockRuns) Apply(ctx context.Context, runID string, options tfe.RunApplyOptions) error { + m.Lock() + defer m.Unlock() + r, ok := m.runs[runID] if !ok { return tfe.ErrResourceNotFound @@ -859,6 +886,9 @@ func (m *mockRuns) ForceCancel(ctx context.Context, runID string, options tfe.Ru } func (m *mockRuns) Discard(ctx context.Context, runID string, options tfe.RunDiscardOptions) error { + m.Lock() + defer m.Unlock() + r, ok := m.runs[runID] if !ok { return tfe.ErrResourceNotFound From a3c9b33b7b91dc36b193defeef703632f67f8323 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 30 Sep 2020 13:44:07 -0400 Subject: [PATCH 6/7] output buffer must be synchronized --- builtin/provisioners/puppet/resource_provisioner.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/builtin/provisioners/puppet/resource_provisioner.go b/builtin/provisioners/puppet/resource_provisioner.go index 767b352af..70a99cdd2 100644 --- a/builtin/provisioners/puppet/resource_provisioner.go +++ b/builtin/provisioners/puppet/resource_provisioner.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "strings" + "sync" "time" "github.com/hashicorp/terraform/builtin/provisioners/puppet/bolt" @@ -39,6 +40,8 @@ type provisioner struct { instanceState *terraform.InstanceState output terraform.UIOutput comm communicator.Communicator + + outputWG sync.WaitGroup } type csrAttributes struct { @@ -297,8 +300,11 @@ func (p *provisioner) runCommand(command string) (stdout string, err error) { outR, outW := io.Pipe() errR, errW := io.Pipe() outTee := io.TeeReader(outR, &stdoutBuffer) + + p.outputWG.Add(2) go p.copyToOutput(outTee) go p.copyToOutput(errR) + defer outW.Close() defer errW.Close() @@ -315,12 +321,19 @@ func (p *provisioner) runCommand(command string) (stdout string, err error) { } err = cmd.Wait() + + outW.Close() + errW.Close() + p.outputWG.Wait() + stdout = strings.TrimSpace(stdoutBuffer.String()) return stdout, err } func (p *provisioner) copyToOutput(reader io.Reader) { + defer p.outputWG.Done() + lr := linereader.New(reader) for line := range lr.Ch { p.output.Output(line) From 59110a2ca548d058326278e85f9fc48602f86a73 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 30 Sep 2020 14:28:02 -0400 Subject: [PATCH 7/7] e2etest server was unsynchronized --- command/e2etest/unmanaged_test.go | 42 ++++++++++++++++++++++++++++--- 1 file changed, 38 insertions(+), 4 deletions(-) diff --git a/command/e2etest/unmanaged_test.go b/command/e2etest/unmanaged_test.go index a4e67a471..ab8e19aa1 100644 --- a/command/e2etest/unmanaged_test.go +++ b/command/e2etest/unmanaged_test.go @@ -6,6 +6,7 @@ import ( "io/ioutil" "path/filepath" "strings" + "sync" "testing" "github.com/hashicorp/go-hclog" @@ -40,21 +41,54 @@ type reattachConfigAddr struct { } type providerServer struct { + sync.Mutex *grpcplugin.GRPCProviderServer planResourceChangeCalled bool applyResourceChangeCalled bool } func (p *providerServer) PlanResourceChange(ctx context.Context, req *proto.PlanResourceChange_Request) (*proto.PlanResourceChange_Response, error) { + p.Lock() + defer p.Unlock() + p.planResourceChangeCalled = true return p.GRPCProviderServer.PlanResourceChange(ctx, req) } func (p *providerServer) ApplyResourceChange(ctx context.Context, req *proto.ApplyResourceChange_Request) (*proto.ApplyResourceChange_Response, error) { + p.Lock() + defer p.Unlock() + p.applyResourceChangeCalled = true return p.GRPCProviderServer.ApplyResourceChange(ctx, req) } +func (p *providerServer) PlanResourceChangeCalled() bool { + p.Lock() + defer p.Unlock() + + return p.planResourceChangeCalled +} +func (p *providerServer) ResetPlanResourceChangeCalled() { + p.Lock() + defer p.Unlock() + + p.planResourceChangeCalled = false +} + +func (p *providerServer) ApplyResourceChangeCalled() bool { + p.Lock() + defer p.Unlock() + + return p.applyResourceChangeCalled +} +func (p *providerServer) ResetApplyResourceChangeCalled() { + p.Lock() + defer p.Unlock() + + p.applyResourceChangeCalled = false +} + func TestUnmanagedSeparatePlan(t *testing.T) { t.Parallel() @@ -129,7 +163,7 @@ func TestUnmanagedSeparatePlan(t *testing.T) { t.Fatalf("unexpected plan error: %s\nstderr:\n%s", err, stderr) } - if !provider.planResourceChangeCalled { + if !provider.PlanResourceChangeCalled() { t.Error("PlanResourceChange not called on in-process provider") } @@ -139,10 +173,10 @@ func TestUnmanagedSeparatePlan(t *testing.T) { t.Fatalf("unexpected apply error: %s\nstderr:\n%s", err, stderr) } - if !provider.applyResourceChangeCalled { + if !provider.ApplyResourceChangeCalled() { t.Error("ApplyResourceChange not called on in-process provider") } - provider.applyResourceChangeCalled = false + provider.ResetApplyResourceChangeCalled() //// DESTROY _, stderr, err = tf.Run("destroy", "-auto-approve") @@ -150,7 +184,7 @@ func TestUnmanagedSeparatePlan(t *testing.T) { t.Fatalf("unexpected destroy error: %s\nstderr:\n%s", err, stderr) } - if !provider.applyResourceChangeCalled { + if !provider.ApplyResourceChangeCalled() { t.Error("ApplyResourceChange (destroy) not called on in-process provider") } cancel()