From 17f420102fdb54568a03b7022dbd641953a589e9 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 9 Mar 2023 14:36:22 -0800 Subject: [PATCH] states: Local values no longer live in state Back when we added local values (a long time ago now!) we put their results in state mainly just because it was the only suitable shared data structure to keep them in. They are a bit ideosyncratic there because we intentionally discard them when serializing state to a snapshot, and that's just fine because they never need to be retained between runs anyway. We now have namedvals.State for all of our named value result storage needs, so we can remove the local-value-related fields of states.Module and use the relevant map inside the local value state instead. As of this commit, states.State now tracks only the data that we actually persist between runs in state snapshots, which will hopefully avoid future bugs resulting from the former difference in fidelity between a freshly-created in-memory state vs. one loaded from a snapshot. --- internal/command/jsonformat/state_test.go | 1 - internal/states/module.go | 23 +------ internal/states/state.go | 13 +--- internal/states/state_deepcopy.go | 10 +-- internal/states/state_test.go | 20 ++---- internal/states/sync.go | 38 ------------ internal/terraform/context_apply2_test.go | 62 ++++++++++++++----- internal/terraform/context_plan2_test.go | 14 +++-- internal/terraform/evaluate.go | 7 +-- internal/terraform/node_external_reference.go | 18 +++++- internal/terraform/node_local.go | 8 +-- internal/terraform/node_local_test.go | 35 ++++++----- 12 files changed, 104 insertions(+), 145 deletions(-) diff --git a/internal/command/jsonformat/state_test.go b/internal/command/jsonformat/state_test.go index 69030a1e48..aeb3cf8dd2 100644 --- a/internal/command/jsonformat/state_test.go +++ b/internal/command/jsonformat/state_test.go @@ -252,7 +252,6 @@ func basicState(t *testing.T) *states.State { t.Errorf("root module is nil; want valid object") } - rootModule.SetLocalValue("foo", cty.StringVal("foo value")) state.SetOutputValue( addrs.OutputValue{Name: "bar"}.Absolute(addrs.RootModuleInstance), cty.StringVal("bar value"), false, diff --git a/internal/states/module.go b/internal/states/module.go index e9f6355b38..2c22cea8d0 100644 --- a/internal/states/module.go +++ b/internal/states/module.go @@ -5,7 +5,6 @@ package states import ( "github.com/hashicorp/terraform/internal/addrs" - "github.com/zclconf/go-cty/cty" ) // Module is a container for the states of objects within a particular module. @@ -15,18 +14,13 @@ type Module struct { // Resources contains the state for each resource. The keys in this map are // an implementation detail and must not be used by outside callers. Resources map[string]*Resource - - // LocalValues contains the value for each named output value. The keys - // in this map are local value names. - LocalValues map[string]cty.Value } // NewModule constructs an empty module state for the given module address. func NewModule(addr addrs.ModuleInstance) *Module { return &Module{ - Addr: addr, - Resources: map[string]*Resource{}, - LocalValues: map[string]cty.Value{}, + Addr: addr, + Resources: map[string]*Resource{}, } } @@ -247,19 +241,6 @@ func (ms *Module) maybeRestoreResourceInstanceDeposed(addr addrs.ResourceInstanc return true } -// SetLocalValue writes a local value into the state, overwriting any -// existing value of the same name. -func (ms *Module) SetLocalValue(name string, value cty.Value) { - ms.LocalValues[name] = value -} - -// RemoveLocalValue removes the local value of the given name from the state, -// if it exists. This method is a no-op if there is no value of the given -// name. -func (ms *Module) RemoveLocalValue(name string) { - delete(ms.LocalValues, name) -} - // PruneResourceHusks is a specialized method that will remove any Resource // objects that do not contain any instances, even if they have an EachMode. // diff --git a/internal/states/state.go b/internal/states/state.go index 151155ed0c..4766fd99b0 100644 --- a/internal/states/state.go +++ b/internal/states/state.go @@ -7,10 +7,9 @@ import ( "fmt" "sort" - "github.com/zclconf/go-cty/cty" - "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/getproviders" + "github.com/zclconf/go-cty/cty" ) // State is the top-level type of a Terraform state. @@ -323,16 +322,6 @@ func (s *State) RemoveOutputValue(addr addrs.AbsOutputValue) { delete(s.RootOutputValues, addr.OutputValue.Name) } -// LocalValue returns the value of the named local value with the given address, -// or cty.NilVal if no such value is tracked in the state. -func (s *State) LocalValue(addr addrs.AbsLocalValue) cty.Value { - ms := s.Module(addr.Module) - if ms == nil { - return cty.NilVal - } - return ms.LocalValues[addr.LocalValue.Name] -} - // ProviderAddrs returns a list of all of the provider configuration addresses // referenced throughout the receiving state. // diff --git a/internal/states/state_deepcopy.go b/internal/states/state_deepcopy.go index c1984834d9..5bd88f7777 100644 --- a/internal/states/state_deepcopy.go +++ b/internal/states/state_deepcopy.go @@ -59,16 +59,10 @@ func (ms *Module) DeepCopy() *Module { for k, r := range ms.Resources { resources[k] = r.DeepCopy() } - localValues := make(map[string]cty.Value, len(ms.LocalValues)) - for k, v := range ms.LocalValues { - // cty.Value is immutable, so we don't need to copy these. - localValues[k] = v - } return &Module{ - Addr: ms.Addr, // technically mutable, but immutable by convention - Resources: resources, - LocalValues: localValues, + Addr: ms.Addr, // technically mutable, but immutable by convention + Resources: resources, } } diff --git a/internal/states/state_test.go b/internal/states/state_test.go index 73ca8b83a6..ef49bb95f2 100644 --- a/internal/states/state_test.go +++ b/internal/states/state_test.go @@ -28,7 +28,6 @@ func TestState(t *testing.T) { t.Errorf("root module is nil; want valid object") } - rootModule.SetLocalValue("foo", cty.StringVal("foo value")) state.SetOutputValue( addrs.OutputValue{Name: "bar"}.Absolute(addrs.RootModuleInstance), cty.StringVal("bar value"), false, @@ -91,9 +90,6 @@ func TestState(t *testing.T) { Modules: map[string]*Module{ "": { Addr: addrs.RootModuleInstance, - LocalValues: map[string]cty.Value{ - "foo": cty.StringVal("foo value"), - }, Resources: map[string]*Resource{ "test_thing.baz": { Addr: addrs.Resource{ @@ -120,19 +116,16 @@ func TestState(t *testing.T) { }, }, "module.child": { - Addr: addrs.RootModuleInstance.Child("child", addrs.NoKey), - LocalValues: map[string]cty.Value{}, - Resources: map[string]*Resource{}, + Addr: addrs.RootModuleInstance.Child("child", addrs.NoKey), + Resources: map[string]*Resource{}, }, `module.multi["a"]`: { - Addr: addrs.RootModuleInstance.Child("multi", addrs.StringKey("a")), - LocalValues: map[string]cty.Value{}, - Resources: map[string]*Resource{}, + Addr: addrs.RootModuleInstance.Child("multi", addrs.StringKey("a")), + Resources: map[string]*Resource{}, }, `module.multi["b"]`: { - Addr: addrs.RootModuleInstance.Child("multi", addrs.StringKey("b")), - LocalValues: map[string]cty.Value{}, - Resources: map[string]*Resource{}, + Addr: addrs.RootModuleInstance.Child("multi", addrs.StringKey("b")), + Resources: map[string]*Resource{}, }, }, } @@ -189,7 +182,6 @@ func TestStateDeepCopy(t *testing.T) { t.Fatalf("root module is nil; want valid object") } - rootModule.SetLocalValue("foo", cty.StringVal("foo value")) state.SetOutputValue( addrs.OutputValue{Name: "bar"}.Absolute(rootModule.Addr), cty.StringVal("bar value"), false, diff --git a/internal/states/sync.go b/internal/states/sync.go index 84839f420c..c3c480a4ee 100644 --- a/internal/states/sync.go +++ b/internal/states/sync.go @@ -103,44 +103,6 @@ func (s *SyncState) RemoveOutputValue(addr addrs.AbsOutputValue) { s.state.RemoveOutputValue(addr) } -// LocalValue returns the current value associated with the given local value -// address. -func (s *SyncState) LocalValue(addr addrs.AbsLocalValue) cty.Value { - s.lock.RLock() - // cty.Value is immutable, so we don't need any extra copying here. - ret := s.state.LocalValue(addr) - s.lock.RUnlock() - return ret -} - -// SetLocalValue writes a given output value into the state, overwriting -// any existing value of the same name. -// -// If the module containing the local value is not yet tracked in state then it -// will be added as a side-effect. -func (s *SyncState) SetLocalValue(addr addrs.AbsLocalValue, value cty.Value) { - defer s.beginWrite()() - - ms := s.state.EnsureModule(addr.Module) - ms.SetLocalValue(addr.LocalValue.Name, value) -} - -// RemoveLocalValue removes the stored value for the local value with the -// given address. -// -// If this results in its containing module being empty, the module will be -// pruned from the state as a side-effect. -func (s *SyncState) RemoveLocalValue(addr addrs.AbsLocalValue) { - defer s.beginWrite()() - - ms := s.state.Module(addr.Module) - if ms == nil { - return - } - ms.RemoveLocalValue(addr.LocalValue.Name) - s.maybePruneModule(addr.Module) -} - // Resource returns a snapshot of the state of the resource with the given // address, or nil if no such resource is tracked. // diff --git a/internal/terraform/context_apply2_test.go b/internal/terraform/context_apply2_test.go index 0f257189b0..2f522abef2 100644 --- a/internal/terraform/context_apply2_test.go +++ b/internal/terraform/context_apply2_test.go @@ -2272,16 +2272,29 @@ locals { t.Errorf("expected no errors, but got %s", diags) } - state, diags := ctx.Apply(plan, m, nil) - if diags.HasErrors() { - t.Errorf("expected no errors, but got %s", diags) + g, _, diags := ctx.applyGraph(plan, m, &ApplyOpts{}, true) + assertNoDiagnostics(t, diags) + + // The local value should've been pruned from the graph because nothing + // refers to it. + gotGraph := g.String() + wantGraph := `provider["registry.terraform.io/hashicorp/test"] +provider["registry.terraform.io/hashicorp/test"] (close) + test_object.a +root + provider["registry.terraform.io/hashicorp/test"] (close) +test_object.a + test_object.a (expand) +test_object.a (expand) + provider["registry.terraform.io/hashicorp/test"] +` + if diff := cmp.Diff(wantGraph, gotGraph); diff != "" { + t.Errorf("wrong apply graph\n%s", diff) } - // We didn't specify any external references, so the unreferenced local - // value should have been tidied up and never made it into the state. - module := state.RootModule() - if len(module.LocalValues) > 0 { - t.Errorf("expected no local values in the state but found %d", len(module.LocalValues)) + _, diags = ctx.Apply(plan, m, nil) + if diags.HasErrors() { + t.Errorf("expected no errors, but got %s", diags) } } @@ -2315,17 +2328,36 @@ locals { t.Errorf("expected no errors, but got %s", diags) } - state, diags := ctx.Apply(plan, m, nil) + g, _, diags := ctx.applyGraph(plan, m, &ApplyOpts{}, true) if diags.HasErrors() { t.Errorf("expected no errors, but got %s", diags) } - // We did specify the local value in the external references, so it should - // have been preserved even though it is not referenced by anything directly - // in the config. - module := state.RootModule() - if module.LocalValues["local_value"].AsString() != "foo" { - t.Errorf("expected local value to be \"foo\" but was \"%s\"", module.LocalValues["local_value"].AsString()) + // The local value should remain in the graph because the external + // reference uses it. + gotGraph := g.String() + wantGraph := ` + local.local_value (expand) +local.local_value (expand) + test_object.a +provider["registry.terraform.io/hashicorp/test"] +provider["registry.terraform.io/hashicorp/test"] (close) + test_object.a +root + + provider["registry.terraform.io/hashicorp/test"] (close) +test_object.a + test_object.a (expand) +test_object.a (expand) + provider["registry.terraform.io/hashicorp/test"] +` + if diff := cmp.Diff(wantGraph, gotGraph); diff != "" { + t.Errorf("wrong graph\n%s", diff) + } + + _, diags = ctx.Apply(plan, m, nil) + if diags.HasErrors() { + t.Errorf("expected no errors, but got %s", diags) } } diff --git a/internal/terraform/context_plan2_test.go b/internal/terraform/context_plan2_test.go index cf2c5876ba..787e8d34cd 100644 --- a/internal/terraform/context_plan2_test.go +++ b/internal/terraform/context_plan2_test.go @@ -4234,6 +4234,10 @@ resource "test_object" "a" { locals { local_value = test_object.a.test_string } + +output "from_local_value" { + value = local.local_value +} `, }) @@ -4253,20 +4257,20 @@ locals { module := state.RootModule() // So, the original state shouldn't have been updated at all. - if len(module.LocalValues) > 0 { - t.Errorf("expected no local values in the state but found %d", len(module.LocalValues)) + if len(state.RootOutputValues) > 0 { + t.Errorf("expected no root output values in the state but found %d", len(state.RootOutputValues)) } if len(module.Resources) > 0 { - t.Errorf("expected no resources in the state but found %d", len(module.LocalValues)) + t.Errorf("expected no resources in the state but found %d", len(module.Resources)) } // But, this makes it hard for the testing framework to valid things about // the returned plan. So, the plan contains the planned state: module = plan.PlannedState.RootModule() - if module.LocalValues["local_value"].AsString() != "foo" { - t.Errorf("expected local value to be \"foo\" but was \"%s\"", module.LocalValues["local_value"].AsString()) + if got, want := plan.PlannedState.RootOutputValues["from_local_value"].Value.AsString(), "foo"; got != want { + t.Errorf("expected local value to be %q but was %q", want, got) } if module.ResourceInstance(addr.Resource).Current.Status != states.ObjectPlanned { diff --git a/internal/terraform/evaluate.go b/internal/terraform/evaluate.go index 077c41aec3..ddad69ae5d 100644 --- a/internal/terraform/evaluate.go +++ b/internal/terraform/evaluate.go @@ -303,12 +303,7 @@ func (d *evaluationStateData) GetLocalValue(addr addrs.LocalValue, rng tfdiags.S return cty.DynamicVal, diags } - val := d.Evaluator.State.LocalValue(addr.Absolute(d.ModulePath)) - if val == cty.NilVal { - // Not evaluated yet? - val = cty.DynamicVal - } - + val := d.Evaluator.NamedValues.GetLocalValue(addr.Absolute(d.ModulePath)) return val, diags } diff --git a/internal/terraform/node_external_reference.go b/internal/terraform/node_external_reference.go index 59ac112a11..8050d15e3f 100644 --- a/internal/terraform/node_external_reference.go +++ b/internal/terraform/node_external_reference.go @@ -3,7 +3,13 @@ package terraform -import "github.com/hashicorp/terraform/internal/addrs" +import ( + "fmt" + "sort" + "strings" + + "github.com/hashicorp/terraform/internal/addrs" +) // nodeExternalReference allows external callers (such as the testing framework) // to provide the list of references they are making into the graph. This @@ -32,3 +38,13 @@ func (n *nodeExternalReference) ModulePath() addrs.Module { func (n *nodeExternalReference) References() []*addrs.Reference { return n.ExternalReferences } + +// Name implements dag.NamedVertex +func (n *nodeExternalReference) Name() string { + names := make([]string, len(n.ExternalReferences)) + for i, ref := range n.ExternalReferences { + names[i] = ref.DisplayString() + } + sort.Strings(names) + return fmt.Sprintf("", strings.Join(names, ", ")) +} diff --git a/internal/terraform/node_local.go b/internal/terraform/node_local.go index 7d955e514f..ad5943cab9 100644 --- a/internal/terraform/node_local.go +++ b/internal/terraform/node_local.go @@ -161,13 +161,7 @@ func (n *NodeLocal) Execute(ctx EvalContext, op walkOperation) (diags tfdiags.Di return diags } - state := ctx.State() - if state == nil { - diags = diags.Append(fmt.Errorf("cannot write local value to nil state")) - return diags - } - - state.SetLocalValue(addr.Absolute(ctx.Path()), val) + ctx.NamedValues().SetLocalValue(addr.Absolute(ctx.Path()), val) return diags } diff --git a/internal/terraform/node_local_test.go b/internal/terraform/node_local_test.go index 6e11d74f7d..ba28c118e9 100644 --- a/internal/terraform/node_local_test.go +++ b/internal/terraform/node_local_test.go @@ -4,17 +4,15 @@ package terraform import ( - "reflect" "testing" - "github.com/davecgh/go-spew/spew" "github.com/hashicorp/hcl/v2" "github.com/hashicorp/hcl/v2/hclsyntax" - "github.com/zclconf/go-cty/cty" "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/configs" "github.com/hashicorp/terraform/internal/configs/hcl2shim" + "github.com/hashicorp/terraform/internal/namedvals" "github.com/hashicorp/terraform/internal/states" ) @@ -48,14 +46,16 @@ func TestNodeLocalExecute(t *testing.T) { t.Fatal(diags.Error()) } + localAddr := addrs.LocalValue{Name: "foo"}.Absolute(addrs.RootModuleInstance) n := &NodeLocal{ - Addr: addrs.LocalValue{Name: "foo"}.Absolute(addrs.RootModuleInstance), + Addr: localAddr, Config: &configs.Local{ Expr: expr, }, } ctx := &MockEvalContext{ - StateState: states.NewState().SyncWrapper(), + StateState: states.NewState().SyncWrapper(), + NamedValuesState: namedvals.NewState(), EvaluateExprResult: hcl2shim.HCL2ValueFromConfigValue(test.Want), } @@ -69,18 +69,19 @@ func TestNodeLocalExecute(t *testing.T) { } } - ms := ctx.StateState.Module(addrs.RootModuleInstance) - gotLocals := ms.LocalValues - wantLocals := map[string]cty.Value{} - if test.Want != nil { - wantLocals["foo"] = hcl2shim.HCL2ValueFromConfigValue(test.Want) - } - - if !reflect.DeepEqual(gotLocals, wantLocals) { - t.Errorf( - "wrong locals after Eval\ngot: %swant: %s", - spew.Sdump(gotLocals), spew.Sdump(wantLocals), - ) + if test.Err { + if ctx.NamedValues().HasLocalValue(localAddr) { + t.Errorf("have value for %s, but wanted none", localAddr) + } + } else { + if !ctx.NamedValues().HasLocalValue(localAddr) { + t.Fatalf("no value for %s", localAddr) + } + got := ctx.NamedValues().GetLocalValue(localAddr) + want := hcl2shim.HCL2ValueFromConfigValue(test.Want) + if !want.RawEquals(got) { + t.Errorf("wrong value for %s\ngot: %#v\nwant: %#v", localAddr, got, want) + } } }) }