From ae2a64a3ad4bbb095b48ae02a69f69d16fdfe316 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Tue, 23 Jan 2024 15:52:39 -0800 Subject: [PATCH] terraform: count and for_each evaluation permitting unknown values This is currently an opt-in argument, with no callers (except some tests) using it. Therefore this should not change any externally-visible behavior yet. Future commits will gradually incorporate support for modules and resources whose expansion isn't yet known, which will then cause planning of anything downstream of them to be deferred to a future run. --- internal/terraform/eval_count.go | 15 +++++-- internal/terraform/eval_count_test.go | 36 +++++++++++++++- internal/terraform/eval_for_each.go | 39 +++++++++++------- internal/terraform/eval_for_each_test.go | 41 +++++++++++++++++-- internal/terraform/node_module_expand.go | 6 +-- internal/terraform/node_resource_abstract.go | 4 +- .../node_resource_abstract_instance.go | 8 ++-- internal/terraform/node_resource_plan.go | 2 +- .../terraform/node_resource_plan_instance.go | 4 +- internal/terraform/node_resource_validate.go | 2 +- 10 files changed, 122 insertions(+), 35 deletions(-) diff --git a/internal/terraform/eval_count.go b/internal/terraform/eval_count.go index 9e2144d9e0..3d1f091bf4 100644 --- a/internal/terraform/eval_count.go +++ b/internal/terraform/eval_count.go @@ -6,10 +6,11 @@ package terraform import ( "fmt" - "github.com/hashicorp/hcl/v2" - "github.com/hashicorp/terraform/internal/tfdiags" "github.com/zclconf/go-cty/cty" "github.com/zclconf/go-cty/cty/gocty" + + "github.com/hashicorp/hcl/v2" + "github.com/hashicorp/terraform/internal/tfdiags" ) // evaluateCountExpression is our standard mechanism for interpreting an @@ -20,9 +21,15 @@ import ( // evaluateCountExpression differs from evaluateCountExpressionValue by // returning an error if the count value is not known, and converting the // cty.Value to an integer. -func evaluateCountExpression(expr hcl.Expression, ctx EvalContext) (int, tfdiags.Diagnostics) { +// +// If allowUnknown is false then this function will return error diagnostics +// whenever the expression returns an unknown value. Setting allowUnknown to +// true instead permits unknown values, indicating them by returning the +// placeholder value -1. Callers can assume that a return value of -1 without +// any error diagnostics represents a valid unknown value. +func evaluateCountExpression(expr hcl.Expression, ctx EvalContext, allowUnknown bool) (int, tfdiags.Diagnostics) { countVal, diags := evaluateCountExpressionValue(expr, ctx) - if !countVal.IsKnown() { + if !allowUnknown && !countVal.IsKnown() { // Currently this is a rather bad outcome from a UX standpoint, since we have // no real mechanism to deal with this situation and all we can do is produce // an error message. diff --git a/internal/terraform/eval_count_test.go b/internal/terraform/eval_count_test.go index 74bcfbdf72..d4d785c37d 100644 --- a/internal/terraform/eval_count_test.go +++ b/internal/terraform/eval_count_test.go @@ -32,7 +32,7 @@ func TestEvaluateCountExpression(t *testing.T) { t.Run(name, func(t *testing.T) { ctx := &MockEvalContext{} ctx.installSimpleEval() - countVal, diags := evaluateCountExpression(test.Expr, ctx) + countVal, diags := evaluateCountExpression(test.Expr, ctx, false) if len(diags) != 0 { t.Errorf("unexpected diagnostics %s", spew.Sdump(diags)) @@ -47,3 +47,37 @@ func TestEvaluateCountExpression(t *testing.T) { }) } } + +func TestEvaluateCountExpression_allowUnknown(t *testing.T) { + tests := map[string]struct { + Expr hcl.Expression + Count int + }{ + "unknown number": { + hcltest.MockExprLiteral(cty.UnknownVal(cty.Number)), + -1, + }, + "dynamicval": { + hcltest.MockExprLiteral(cty.DynamicVal), + -1, + }, + } + for name, test := range tests { + t.Run(name, func(t *testing.T) { + ctx := &MockEvalContext{} + ctx.installSimpleEval() + countVal, diags := evaluateCountExpression(test.Expr, ctx, true) + + if len(diags) != 0 { + t.Errorf("unexpected diagnostics %s", spew.Sdump(diags)) + } + + if !reflect.DeepEqual(countVal, test.Count) { + t.Errorf( + "wrong result\ngot: %#v\nwant: %#v", + countVal, test.Count, + ) + } + }) + } +} diff --git a/internal/terraform/eval_for_each.go b/internal/terraform/eval_for_each.go index 3782234e9b..c567a54397 100644 --- a/internal/terraform/eval_for_each.go +++ b/internal/terraform/eval_for_each.go @@ -19,20 +19,21 @@ import ( // evaluateForEachExpression differs from evaluateForEachExpressionValue by // returning an error if the count value is not known, and converting the // cty.Value to a map[string]cty.Value for compatibility with other calls. -func evaluateForEachExpression(expr hcl.Expression, ctx EvalContext) (forEach map[string]cty.Value, diags tfdiags.Diagnostics) { - return newForEachEvaluator(expr, ctx).ResourceValue() +func evaluateForEachExpression(expr hcl.Expression, ctx EvalContext, allowUnknown bool) (forEach map[string]cty.Value, known bool, diags tfdiags.Diagnostics) { + return newForEachEvaluator(expr, ctx, allowUnknown).ResourceValue() } // forEachEvaluator is the standard mechanism for interpreting an expression // given for a "for_each" argument on a resource, module, or import. -func newForEachEvaluator(expr hcl.Expression, ctx EvalContext) *forEachEvaluator { +func newForEachEvaluator(expr hcl.Expression, ctx EvalContext, allowUnknown bool) *forEachEvaluator { if ctx == nil { panic("nil EvalContext") } return &forEachEvaluator{ - ctx: ctx, - expr: expr, + ctx: ctx, + expr: expr, + allowUnknown: allowUnknown, } } @@ -48,44 +49,54 @@ type forEachEvaluator struct { ctx EvalContext expr hcl.Expression + // TEMP: If allowUnknown is set then we skip the usual restriction that + // unknown values are not allowed in for_each. A caller that sets this + // must therefore be ready to deal with the result being unknown. + // This will eventually become the default behavior, once we've updated + // the rest of this package to handle that situation in a reasonable way. + allowUnknown bool + // internal hclCtx *hcl.EvalContext } // ResourceForEachValue returns a known for_each map[string]cty.Value // appropriate for use within resource expansion. -func (ev *forEachEvaluator) ResourceValue() (map[string]cty.Value, tfdiags.Diagnostics) { +func (ev *forEachEvaluator) ResourceValue() (map[string]cty.Value, bool, tfdiags.Diagnostics) { res := map[string]cty.Value{} // no expression always results in an empty map if ev.expr == nil { - return res, nil + return res, true, nil } forEachVal, diags := ev.Value() if diags.HasErrors() { - return res, diags + return res, false, diags } // ensure our value is known for use in resource expansion - diags = diags.Append(ev.ensureKnownForResource(forEachVal)) - if diags.HasErrors() { - return res, diags + unknownDiags := ev.ensureKnownForResource(forEachVal) + if unknownDiags.HasErrors() { + if !ev.allowUnknown { + diags = diags.Append(unknownDiags) + } + return res, false, diags } // validate the for_each value for use in resource expansion diags = diags.Append(ev.validateResource(forEachVal)) if diags.HasErrors() { - return res, diags + return res, false, diags } if forEachVal.IsNull() || !forEachVal.IsKnown() || markSafeLengthInt(forEachVal) == 0 { // we check length, because an empty set returns a nil map which will panic below - return res, diags + return res, true, diags } res = forEachVal.AsValueMap() - return res, diags + return res, true, diags } // ImportValue returns the for_each map for use within an import block, diff --git a/internal/terraform/eval_for_each_test.go b/internal/terraform/eval_for_each_test.go index 3f7717d090..6f4885b1d4 100644 --- a/internal/terraform/eval_for_each_test.go +++ b/internal/terraform/eval_for_each_test.go @@ -72,7 +72,7 @@ func TestEvaluateForEachExpression_valid(t *testing.T) { t.Run(name, func(t *testing.T) { ctx := &MockEvalContext{} ctx.installSimpleEval() - forEachMap, diags := evaluateForEachExpression(test.Expr, ctx) + forEachMap, _, diags := evaluateForEachExpression(test.Expr, ctx, false) if len(diags) != 0 { t.Errorf("unexpected diagnostics %s", spew.Sdump(diags)) @@ -176,7 +176,7 @@ func TestEvaluateForEachExpression_errors(t *testing.T) { t.Run(name, func(t *testing.T) { ctx := &MockEvalContext{} ctx.installSimpleEval() - _, diags := evaluateForEachExpression(test.Expr, ctx) + _, _, diags := evaluateForEachExpression(test.Expr, ctx, false) if len(diags) != 1 { t.Fatalf("got %d diagnostics; want 1", len(diags)) @@ -211,6 +211,41 @@ func TestEvaluateForEachExpression_errors(t *testing.T) { } } +func TestEvaluateForEachExpression_allowUnknown(t *testing.T) { + tests := map[string]struct { + Expr hcl.Expression + }{ + "unknown string set": { + hcltest.MockExprLiteral(cty.UnknownVal(cty.Set(cty.String))), + }, + "unknown map": { + hcltest.MockExprLiteral(cty.UnknownVal(cty.Map(cty.Bool))), + }, + "set containing unknown value": { + hcltest.MockExprLiteral(cty.SetVal([]cty.Value{cty.UnknownVal(cty.String)})), + }, + "set containing dynamic unknown value": { + hcltest.MockExprLiteral(cty.SetVal([]cty.Value{cty.UnknownVal(cty.DynamicPseudoType)})), + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + ctx := &MockEvalContext{} + ctx.installSimpleEval() + _, known, diags := evaluateForEachExpression(test.Expr, ctx, true) + + // With allowUnknown set, all of these expressions should be treated + // as valid for_each values. + assertNoDiagnostics(t, diags) + + if known { + t.Errorf("result is known; want unknown") + } + }) + } +} + func TestEvaluateForEachExpressionKnown(t *testing.T) { tests := map[string]hcl.Expression{ "unknown string set": hcltest.MockExprLiteral(cty.UnknownVal(cty.Set(cty.String))), @@ -221,7 +256,7 @@ func TestEvaluateForEachExpressionKnown(t *testing.T) { t.Run(name, func(t *testing.T) { ctx := &MockEvalContext{} ctx.installSimpleEval() - diags := newForEachEvaluator(expr, ctx).ValidateResourceValue() + diags := newForEachEvaluator(expr, ctx, false).ValidateResourceValue() if len(diags) != 0 { t.Errorf("unexpected diagnostics %s", spew.Sdump(diags)) diff --git a/internal/terraform/node_module_expand.go b/internal/terraform/node_module_expand.go index cb6f3a88bc..529dd48dba 100644 --- a/internal/terraform/node_module_expand.go +++ b/internal/terraform/node_module_expand.go @@ -113,7 +113,7 @@ func (n *nodeExpandModule) Execute(ctx EvalContext, op walkOperation) (diags tfd ctx = ctx.WithPath(module) switch { case n.ModuleCall.Count != nil: - count, ctDiags := evaluateCountExpression(n.ModuleCall.Count, ctx) + count, ctDiags := evaluateCountExpression(n.ModuleCall.Count, ctx, false) diags = diags.Append(ctDiags) if diags.HasErrors() { return diags @@ -121,7 +121,7 @@ func (n *nodeExpandModule) Execute(ctx EvalContext, op walkOperation) (diags tfd expander.SetModuleCount(module, call, count) case n.ModuleCall.ForEach != nil: - forEach, feDiags := evaluateForEachExpression(n.ModuleCall.ForEach, ctx) + forEach, _, feDiags := evaluateForEachExpression(n.ModuleCall.ForEach, ctx, false) diags = diags.Append(feDiags) if diags.HasErrors() { return diags @@ -256,7 +256,7 @@ func (n *nodeValidateModule) Execute(ctx EvalContext, op walkOperation) (diags t diags = diags.Append(countDiags) case n.ModuleCall.ForEach != nil: - forEachDiags := newForEachEvaluator(n.ModuleCall.ForEach, ctx).ValidateResourceValue() + forEachDiags := newForEachEvaluator(n.ModuleCall.ForEach, ctx, false).ValidateResourceValue() diags = diags.Append(forEachDiags) } diff --git a/internal/terraform/node_resource_abstract.go b/internal/terraform/node_resource_abstract.go index cbf4f1d16e..b1708ce66b 100644 --- a/internal/terraform/node_resource_abstract.go +++ b/internal/terraform/node_resource_abstract.go @@ -404,7 +404,7 @@ func (n *NodeAbstractResource) writeResourceState(ctx EvalContext, addr addrs.Ab switch { case n.Config != nil && n.Config.Count != nil: - count, countDiags := evaluateCountExpression(n.Config.Count, ctx) + count, countDiags := evaluateCountExpression(n.Config.Count, ctx, false) diags = diags.Append(countDiags) if countDiags.HasErrors() { return diags @@ -414,7 +414,7 @@ func (n *NodeAbstractResource) writeResourceState(ctx EvalContext, addr addrs.Ab expander.SetResourceCount(addr.Module, n.Addr.Resource, count) case n.Config != nil && n.Config.ForEach != nil: - forEach, forEachDiags := evaluateForEachExpression(n.Config.ForEach, ctx) + forEach, _, forEachDiags := evaluateForEachExpression(n.Config.ForEach, ctx, false) diags = diags.Append(forEachDiags) if forEachDiags.HasErrors() { return diags diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go index 696645a91b..b7d44b2c29 100644 --- a/internal/terraform/node_resource_abstract_instance.go +++ b/internal/terraform/node_resource_abstract_instance.go @@ -777,7 +777,7 @@ func (n *NodeAbstractResourceInstance) plan( } // Evaluate the configuration - forEach, _ := evaluateForEachExpression(n.Config.ForEach, ctx) + forEach, _, _ := evaluateForEachExpression(n.Config.ForEach, ctx, false) keyData = EvalDataForInstanceKey(n.ResourceInstanceAddr().Resource.Key, forEach) @@ -1709,7 +1709,7 @@ func (n *NodeAbstractResourceInstance) planDataSource(ctx EvalContext, checkRule objTy := schema.ImpliedType() priorVal := cty.NullVal(objTy) - forEach, _ := evaluateForEachExpression(config.ForEach, ctx) + forEach, _, _ := evaluateForEachExpression(config.ForEach, ctx, false) keyData = EvalDataForInstanceKey(n.ResourceInstanceAddr().Resource.Key, forEach) checkDiags := evalCheckRules( @@ -1989,7 +1989,7 @@ func (n *NodeAbstractResourceInstance) applyDataSource(ctx EvalContext, planned return nil, keyData, diags } - forEach, _ := evaluateForEachExpression(config.ForEach, ctx) + forEach, _, _ := evaluateForEachExpression(config.ForEach, ctx, false) keyData = EvalDataForInstanceKey(n.Addr.Resource.Key, forEach) checkDiags := evalCheckRules( @@ -2293,7 +2293,7 @@ func (n *NodeAbstractResourceInstance) applyProvisioners(ctx EvalContext, state func (n *NodeAbstractResourceInstance) evalProvisionerConfig(ctx EvalContext, body hcl.Body, self cty.Value, schema *configschema.Block) (cty.Value, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics - forEach, forEachDiags := evaluateForEachExpression(n.Config.ForEach, ctx) + forEach, _, forEachDiags := evaluateForEachExpression(n.Config.ForEach, ctx, false) diags = diags.Append(forEachDiags) keyData := EvalDataForInstanceKey(n.ResourceInstanceAddr().Resource.Key, forEach) diff --git a/internal/terraform/node_resource_plan.go b/internal/terraform/node_resource_plan.go index 041d414293..4ab00e9f72 100644 --- a/internal/terraform/node_resource_plan.go +++ b/internal/terraform/node_resource_plan.go @@ -378,7 +378,7 @@ func (n nodeExpandPlannableResource) expandResourceImports(ctx EvalContext, addr continue } - forEachData, forEachDiags := newForEachEvaluator(imp.Config.ForEach, ctx).ImportValues() + forEachData, forEachDiags := newForEachEvaluator(imp.Config.ForEach, ctx, false).ImportValues() diags = diags.Append(forEachDiags) if forEachDiags.HasErrors() { return imports, diags diff --git a/internal/terraform/node_resource_plan_instance.go b/internal/terraform/node_resource_plan_instance.go index 6322097d5a..d00b9f9bb9 100644 --- a/internal/terraform/node_resource_plan_instance.go +++ b/internal/terraform/node_resource_plan_instance.go @@ -355,7 +355,7 @@ func (n *NodePlannableResourceInstance) managedResourceExecute(ctx EvalContext) // values, which could result in a post-condition check relying on that // value being inaccurate. Unless we decide to store the value of the // for-each expression in state, this is unavoidable. - forEach, _ := evaluateForEachExpression(n.Config.ForEach, ctx) + forEach, _, _ := evaluateForEachExpression(n.Config.ForEach, ctx, false) repeatData := EvalDataForInstanceKey(n.ResourceInstanceAddr().Resource.Key, forEach) checkDiags := evalCheckRules( @@ -463,7 +463,7 @@ func (n *NodePlannableResourceInstance) importState(ctx EvalContext, addr addrs. return nil, diags } - forEach, _ := evaluateForEachExpression(n.Config.ForEach, ctx) + forEach, _, _ := evaluateForEachExpression(n.Config.ForEach, ctx, false) keyData := EvalDataForInstanceKey(n.ResourceInstanceAddr().Resource.Key, forEach) configVal, _, configDiags := ctx.EvaluateBlock(n.Config.Config, schema, nil, keyData) if configDiags.HasErrors() { diff --git a/internal/terraform/node_resource_validate.go b/internal/terraform/node_resource_validate.go index 253bcb0097..9d6df70945 100644 --- a/internal/terraform/node_resource_validate.go +++ b/internal/terraform/node_resource_validate.go @@ -306,7 +306,7 @@ func (n *NodeValidatableResource) validateResource(ctx EvalContext) tfdiags.Diag } // Evaluate the for_each expression here so we can expose the diagnostics - forEachDiags := newForEachEvaluator(n.Config.ForEach, ctx).ValidateResourceValue() + forEachDiags := newForEachEvaluator(n.Config.ForEach, ctx, false).ValidateResourceValue() diags = diags.Append(forEachDiags) }