From a55b0b39de62a1827bfbfbe477c231d0ccc47537 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 25 Jan 2024 11:51:38 -0800 Subject: [PATCH] core: Placeholder values for local values in partial-expanded modules Since local value evaluation is largely identical between the fully- and partially-expanded cases, this factors out that handling into a shared function and makes the two graph node Execute functions just thin wrappers around it. The error-handling behavior is intentionally marginally different: it now always registers the local value result in the named values state, even on error, but uses cty.DynamicVal as a placeholder when an error prevents returning anything more precise than that. Nothing depends on that detail because downstream nodes don't get to execute when an upstream returns an error, but this approach reduces the number of possible outcomes and thus makes this marginally easier to follow. This also includes some modernization of the unit tests for TestNodeLocalExecute, so that it no longer relies on shimming functions from the Terraform v0.12 transition. --- internal/terraform/node_local.go | 97 ++++++++++++++++++--------- internal/terraform/node_local_test.go | 32 ++++----- 2 files changed, 80 insertions(+), 49 deletions(-) diff --git a/internal/terraform/node_local.go b/internal/terraform/node_local.go index 067878e534..895ca28c74 100644 --- a/internal/terraform/node_local.go +++ b/internal/terraform/node_local.go @@ -145,35 +145,9 @@ func (n *NodeLocal) References() []*addrs.Reference { // expression for a local value and writes it into a transient part of // the state. func (n *NodeLocal) Execute(ctx EvalContext, op walkOperation) (diags tfdiags.Diagnostics) { - expr := n.Config.Expr - addr := n.Addr.LocalValue - - // We ignore diags here because any problems we might find will be found - // again in EvaluateExpr below. - refs, _ := lang.ReferencesInExpr(addrs.ParseRef, expr) - for _, ref := range refs { - if ref.Subject == addr { - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Self-referencing local value", - Detail: fmt.Sprintf("Local value %s cannot use its own result as part of its expression.", addr), - Subject: ref.SourceRange.ToHCL().Ptr(), - Context: expr.Range().Ptr(), - }) - } - } - if diags.HasErrors() { - return diags - } - - val, moreDiags := ctx.EvaluateExpr(expr, cty.DynamicPseudoType, nil) - diags = diags.Append(moreDiags) - if moreDiags.HasErrors() { - return diags - } - - ctx.NamedValues().SetLocalValue(addr.Absolute(ctx.Path()), val) - + namedVals := ctx.NamedValues() + val, diags := evaluateLocalValue(n.Config, n.Addr.LocalValue, n.Addr.String(), ctx) + namedVals.SetLocalValue(n.Addr, val) return diags } @@ -201,8 +175,71 @@ type nodeLocalInPartialModule struct { Config *configs.Local } +// Path implements [GraphNodePartialExpandedModule], meaning that the +// Execute method receives an [EvalContext] that's set up for partial-expanded +// evaluation instead of full evaluation. func (n *nodeLocalInPartialModule) Path() addrs.PartialExpandedModule { return n.Addr.Module } -// TODO: Implement nodeLocalUnexpandedPlaceholder.Execute +func (n *nodeLocalInPartialModule) Execute(ctx EvalContext, op walkOperation) tfdiags.Diagnostics { + // Our job here is to make sure that the local value definition is + // valid for all instances of this local value across all of the possible + // module instances under our partially-expanded prefix, and to record + // a placeholder value that captures as precisely as possible what all + // of those results have in common. In the worst case where they have + // absolutely nothing in common cty.DynamicVal is the ultimate fallback, + // but we should try to do better when possible to give operators earlier + // feedback about any problems they would definitely encounter on a + // subsequent plan where the local values get evaluated concretely. + + namedVals := ctx.NamedValues() + val, diags := evaluateLocalValue(n.Config, n.Addr.Local, n.Addr.String(), ctx) + namedVals.SetLocalValuePlaceholder(n.Addr, val) + return diags +} + +// evaluateLocalValue is the common evaluation logic shared between +// [NodeLocal] and [nodeLocalInPartialModule]. +// +// The overall validation and evaluation process is the same for each, with +// the differences encapsulated inside the given [EvalContext], which is +// configured in a different way when doing partial-expanded evaluation. +// +// the addrStr argument should be the canonical string representation of the +// anbsolute address of the object being evaluated, which should either be an +// [addrs.AbsLocalValue] or an [addrs.InPartialEvaluatedModule[addrs.LocalValue]] +// depending on which of the two callers are calling this function. +// +// localAddr should match the local portion of the address that was stringified +// for addrStr, describing the local value relative to the module it's declared +// inside. +func evaluateLocalValue(config *configs.Local, localAddr addrs.LocalValue, addrStr string, ctx EvalContext) (cty.Value, tfdiags.Diagnostics) { + var diags tfdiags.Diagnostics + expr := config.Expr + + // We ignore diags here because any problems we might find will be found + // again in EvaluateExpr below. + refs, _ := lang.ReferencesInExpr(addrs.ParseRef, expr) + for _, ref := range refs { + if ref.Subject == localAddr { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Self-referencing local value", + Detail: fmt.Sprintf("Local value %s cannot use its own result as part of its expression.", addrStr), + Subject: ref.SourceRange.ToHCL().Ptr(), + Context: expr.Range().Ptr(), + }) + } + } + if diags.HasErrors() { + return cty.DynamicVal, diags + } + + val, moreDiags := ctx.EvaluateExpr(expr, cty.DynamicPseudoType, nil) + diags = diags.Append(moreDiags) + if val == cty.NilVal { + val = cty.DynamicVal + } + return val, diags +} diff --git a/internal/terraform/node_local_test.go b/internal/terraform/node_local_test.go index ba28c118e9..b3ed692cde 100644 --- a/internal/terraform/node_local_test.go +++ b/internal/terraform/node_local_test.go @@ -8,10 +8,10 @@ import ( "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" ) @@ -19,22 +19,22 @@ import ( func TestNodeLocalExecute(t *testing.T) { tests := []struct { Value string - Want interface{} + Want cty.Value Err bool }{ { "hello!", - "hello!", + cty.StringVal("hello!"), false, }, { "", - "", + cty.StringVal(""), false, }, { "Hello, ${local.foo}", - nil, + cty.DynamicVal, true, // self-referencing }, } @@ -57,7 +57,7 @@ func TestNodeLocalExecute(t *testing.T) { StateState: states.NewState().SyncWrapper(), NamedValuesState: namedvals.NewState(), - EvaluateExprResult: hcl2shim.HCL2ValueFromConfigValue(test.Want), + EvaluateExprResult: test.Want, } err := n.Execute(ctx, walkApply) @@ -69,19 +69,13 @@ func TestNodeLocalExecute(t *testing.T) { } } - 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) - } + if !ctx.NamedValues().HasLocalValue(localAddr) { + t.Fatalf("no value for %s", localAddr) + } + got := ctx.NamedValues().GetLocalValue(localAddr) + want := test.Want + if !want.RawEquals(got) { + t.Errorf("wrong value for %s\ngot: %#v\nwant: %#v", localAddr, got, want) } }) }