From a5148724c985e27ac9bdd76fd9d6b37ff3847989 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 25 Jan 2024 11:47:49 -0800 Subject: [PATCH 1/3] core: Placeholder values for output values in partial-expanded modules --- internal/terraform/node_output.go | 38 ++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/internal/terraform/node_output.go b/internal/terraform/node_output.go index 3be1fceeee9e..0875f16e1499 100644 --- a/internal/terraform/node_output.go +++ b/internal/terraform/node_output.go @@ -132,6 +132,10 @@ func (n *nodeExpandOutput) DynamicExpand(ctx EvalContext) (*Graph, tfdiags.Diagn Config: n.Config, RefreshOnly: n.RefreshOnly, } + // We don't need to handle the module.IsRoot() && n.Destroying case + // seen in the fully-expanded case above, because the root module + // instance is always "fully expanded" (it's always a singleton) + // and so we can't get here for output values in the root module. log.Printf("[TRACE] Expanding output: adding placeholder for all %s as %T", absAddr.String(), node) g.Add(node) }, @@ -464,11 +468,43 @@ type nodeOutputInPartialModule struct { RefreshOnly bool } +// 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 *nodeOutputInPartialModule) Path() addrs.PartialExpandedModule { return n.Addr.Module } -// TODO: Implement nodeOutputInPartialModule.Execute +func (n *nodeOutputInPartialModule) Execute(ctx EvalContext, op walkOperation) tfdiags.Diagnostics { + // Our job here is to make sure that the output value definition is + // valid for all instances of this output 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 output values get evaluated concretely. + + namedVals := ctx.NamedValues() + + // this "ctx" is preconfigured to evaluate in terms of other placeholder + // values generated in the same unexpanded module prefix, rather than + // from the active state/plan, so this result is likely to be derived + // from unknown value placeholders itself. + val, diags := ctx.EvaluateExpr(n.Config.Expr, cty.DynamicPseudoType, nil) + if val == cty.NilVal { + val = cty.DynamicVal + } + + // We'll also check that the depends_on argument is valid, since that's + // a static concern anyway and so cannot vary between instances of the + // same module. + diags = diags.Append(validateDependsOn(ctx, n.Config.DependsOn)) + + namedVals.SetOutputValuePlaceholder(n.Addr, val) + return diags +} // NodeDestroyableOutput represents an output that is "destroyable": // its application will remove the output from the state. From 674d1098668ba7ec15b82dd41788bc9bfe5a1440 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 25 Jan 2024 11:51:38 -0800 Subject: [PATCH 2/3] 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 067878e534a6..895ca28c74e1 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 ba28c118e99d..b3ed692cde86 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) } }) } From d0c22a085269fea2db38b8386acb254e325288be Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 25 Jan 2024 17:14:10 -0800 Subject: [PATCH 3/3] core: Placeholder values for input variables in partial-expanded modules Since input variables are defined in the parent of the module where they are declared, this particular case ends up straddling over a module boundary: the EvalContext belongs to the parent while the node's own absolute address belongs to the child. The DynamicExpand implementation of the expander node already knows how to wire that up (the same way as for the fully-expanded equivalent) and so we don't need to do anything particularly special in this function, but it does mean that we need to provide some placeholder repetition data to use in case the definition expression refers to each.key, each.value, or count.index. For now we're using the "totally-unknown" repetition data that just sets all three symbols to unknown values. In future it would be nice to improve the precision here so that e.g. each.value can be treated as invalid when used in a call that doesn't set for_each, but that would require propagating the call configuration into this graph node and we don't yet have the needed information in the code that generates this node, so we'll save the more disruptive rework to allow that for a later change. --- internal/terraform/node_module_variable.go | 51 +++++++++++++++++----- 1 file changed, 39 insertions(+), 12 deletions(-) diff --git a/internal/terraform/node_module_variable.go b/internal/terraform/node_module_variable.go index 11a053e77224..ea2a59d1ee6c 100644 --- a/internal/terraform/node_module_variable.go +++ b/internal/terraform/node_module_variable.go @@ -160,6 +160,11 @@ type nodeModuleVariable struct { // ModuleCallArguments, ex. so count.index and each.key can resolve ModuleInstance addrs.ModuleInstance + // ModuleCallConfig is the module call that the expression in field Expr + // came from, which helps decide what [instances.RepetitionData] we should + // use when evaluating Expr. + ModuleCallConfig *configs.ModuleCall + // DestroyApply must be set to true when applying a destroy operation and // false otherwise. DestroyApply bool @@ -241,12 +246,6 @@ func (n *nodeModuleVariable) DotNode(name string, opts *dag.DotOpts) *dag.DotNod // evalModuleVariable produces the value for a particular variable as will // be used by a child module instance. // -// The result is written into a map, with its key set to the local name of the -// variable, disregarding the module instance address. A map is returned instead -// of a single value as a result of trying to be convenient for use with -// EvalContext.SetModuleCallArguments, which expects a map to merge in with any -// existing arguments. -// // validateOnly indicates that this evaluation is only for config // validation, and we will not have any expansion module instance // repetition data. @@ -261,11 +260,10 @@ func (n *nodeModuleVariable) evalModuleVariable(ctx EvalContext, validateOnly bo case validateOnly: // the instance expander does not track unknown expansion values, so we // have to assume all RepetitionData is unknown. - moduleInstanceRepetitionData = instances.RepetitionData{ - CountIndex: cty.UnknownVal(cty.Number), - EachKey: cty.UnknownVal(cty.String), - EachValue: cty.DynamicVal, - } + // TODO: Ideally we should vary the placeholder we use here based + // on how the module call repetition was configured, but we don't + // have enough information here to decide that. + moduleInstanceRepetitionData = instances.TotallyUnknownRepetitionData default: // Get the repetition data for this module instance, @@ -327,4 +325,33 @@ func (n *nodeModuleVariableInPartialModule) Path() addrs.PartialExpandedModule { return n.Addr.Module } -// TODO: Implement nodeModuleVariableInPartialModule.Execute +func (n *nodeModuleVariableInPartialModule) Execute(ctx EvalContext, op walkOperation) tfdiags.Diagnostics { + // Our job here is to make sure that the input variable definition is + // valid for all instances of this input variable 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 input variables get evaluated concretely. + + namedVals := ctx.NamedValues() + + // TODO: Ideally we should vary the placeholder we use here based + // on how the module call repetition was configured, but we don't + // have enough information here to decide that. + moduleInstanceRepetitionData := instances.TotallyUnknownRepetitionData + + // NOTE WELL: Input variables are a little strange in that they announce + // themselves as belonging to the caller of the module they are declared + // in, because that's where their definition expressions get evaluated. + // Therefore this [EvalContext] is in the scope of the parent module, + // while n.Addr describes an object in the child module (where the + // variable declaration appeared). + scope := ctx.EvaluationScope(nil, nil, moduleInstanceRepetitionData) + val, diags := scope.EvalExpr(n.Expr, cty.DynamicPseudoType) + + namedVals.SetInputVariablePlaceholder(n.Addr, val) + return diags +}