From 7c678d104fe4fbc63260f02c1ffdc0809da630cd Mon Sep 17 00:00:00 2001 From: Thayne McCombs Date: Thu, 25 Jul 2019 09:51:55 -0600 Subject: [PATCH] Add support for for_each for data blocks. This also fixes a few things with resource for_each: It makes validation more like validation for count. It makes sure the index is stored in the state properly. --- configs/parser_config_test.go | 5 ++ configs/resource.go | 16 +++--- .../invalid-files/data-count-and-for-each.tf | 4 ++ terraform/eval_for_each.go | 52 +++++++++++++------ terraform/eval_state.go | 14 +++-- terraform/eval_validate.go | 17 +++++- terraform/node_data_refresh.go | 12 +++++ 7 files changed, 91 insertions(+), 29 deletions(-) create mode 100644 configs/testdata/invalid-files/data-count-and-for-each.tf diff --git a/configs/parser_config_test.go b/configs/parser_config_test.go index 9c8b70ade9..7b0192353d 100644 --- a/configs/parser_config_test.go +++ b/configs/parser_config_test.go @@ -114,6 +114,11 @@ func TestParserLoadConfigFileFailureMessages(t *testing.T) { hcl.DiagError, `Invalid combination of "count" and "for_each"`, }, + { + "invalid-files/data-count-and-for_each.tf", + hcl.DiagError, + `Invalid combination of "count" and "for_each"`, + }, { "invalid-files/resource-lifecycle-badbool.tf", hcl.DiagError, diff --git a/configs/resource.go b/configs/resource.go index 2528a5583a..edf822c1bb 100644 --- a/configs/resource.go +++ b/configs/resource.go @@ -302,13 +302,15 @@ func decodeDataBlock(block *hcl.Block) (*Resource, hcl.Diagnostics) { if attr, exists := content.Attributes["for_each"]; exists { r.ForEach = attr.Expr - // We currently parse this, but don't yet do anything with it. - diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Reserved argument name in module block", - Detail: fmt.Sprintf("The name %q is reserved for use in a future version of Terraform.", attr.Name), - Subject: &attr.NameRange, - }) + // Cannot have count and for_each on the same data block + if r.Count != nil { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: `Invalid combination of "count" and "for_each"`, + Detail: `The "count" and "for_each" meta-arguments are mutually-exclusive, only one should be used to be explicit about the number of resources to be created.`, + Subject: &attr.NameRange, + }) + } } if attr, exists := content.Attributes["provider"]; exists { diff --git a/configs/testdata/invalid-files/data-count-and-for-each.tf b/configs/testdata/invalid-files/data-count-and-for-each.tf new file mode 100644 index 0000000000..7cc4763250 --- /dev/null +++ b/configs/testdata/invalid-files/data-count-and-for-each.tf @@ -0,0 +1,4 @@ +data "test" "foo" { + count = 2 + for_each = ["a"] +} diff --git a/terraform/eval_for_each.go b/terraform/eval_for_each.go index f9b2e35cce..a49d0686eb 100644 --- a/terraform/eval_for_each.go +++ b/terraform/eval_for_each.go @@ -14,29 +14,50 @@ import ( // the expression is nil, and is used to distinguish between an unset for_each and an // empty map func evaluateResourceForEachExpression(expr hcl.Expression, ctx EvalContext) (forEach map[string]cty.Value, diags tfdiags.Diagnostics) { + forEachMap, known, diags := evaluateResourceForEachExpressionKnown(expr, ctx) + if !known { + // 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. + // FIXME: In future, implement a built-in mechanism for deferring changes that + // can't yet be predicted, and use it to guide the user through several + // plan/apply steps until the desired configuration is eventually reached. + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid forEach argument", + Detail: `The "for_each" value depends on resource attributes that cannot be determined until apply, so Terraform cannot predict how many instances will be created. To work around this, use the -target argument to first apply only the resources that the for_each depends on.`, + }) + panic("uh-oh") + } + return forEachMap, diags +} + +// evaluateResourceForEachExpressionKnown is like evaluateResourceForEachExpression +// except that it handles an unknown result by returning an empty map and +// a known = false, rather than by reporting the unknown value as an error +// diagnostic. +func evaluateResourceForEachExpressionKnown(expr hcl.Expression, ctx EvalContext) (forEach map[string]cty.Value, known bool, diags tfdiags.Diagnostics) { if expr == nil { - return nil, nil + return nil, true, nil } forEachVal, forEachDiags := ctx.EvaluateExpr(expr, cty.DynamicPseudoType, nil) diags = diags.Append(forEachDiags) if diags.HasErrors() { - return nil, diags - } - - // No-op for dynamic types, so that these pass validation, but are then populated at apply - if forEachVal.Type() == cty.DynamicPseudoType { - return nil, diags + return nil, true, diags } - if forEachVal.IsNull() { + switch { + case forEachVal.IsNull(): diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, Summary: "Invalid for_each argument", Detail: `The given "for_each" argument value is unsuitable: the given "for_each" argument value is null. A map, or set of strings is allowed.`, Subject: expr.Range().Ptr(), }) - return nil, diags + return nil, true, diags + case !forEachVal.IsKnown(): + return map[string]cty.Value{}, false, diags } if !forEachVal.CanIterateElements() || forEachVal.Type().IsListType() { @@ -46,7 +67,7 @@ func evaluateResourceForEachExpression(expr hcl.Expression, ctx EvalContext) (fo Detail: fmt.Sprintf(`The given "for_each" argument value is unsuitable: the "for_each" argument must be a map, or set of strings, and you have provided a value of type %s.`, forEachVal.Type().FriendlyName()), Subject: expr.Range().Ptr(), }) - return nil, diags + return nil, true, diags } if forEachVal.Type().IsSetType() { @@ -57,17 +78,14 @@ func evaluateResourceForEachExpression(expr hcl.Expression, ctx EvalContext) (fo Detail: fmt.Sprintf(`The given "for_each" argument value is unsuitable: "for_each" supports maps and sets of strings, but you have provided a set containing type %s.`, forEachVal.Type().ElementType().FriendlyName()), Subject: expr.Range().Ptr(), }) - return nil, diags + return nil, true, diags } } // If the map is empty ({}), return an empty map, because cty will return nil when representing {} AsValueMap - // Also return an empty map if the value is not known -- as this function - // is used to check if the for_each value is valid as well as to apply it, the empty - // map will later be filled in. - if !forEachVal.IsKnown() || forEachVal.LengthInt() == 0 { - return map[string]cty.Value{}, diags + if forEachVal.LengthInt() == 0 { + return map[string]cty.Value{}, true, diags } - return forEachVal.AsValueMap(), nil + return forEachVal.AsValueMap(), true, nil } diff --git a/terraform/eval_state.go b/terraform/eval_state.go index d506ce3fe1..b611113e3c 100644 --- a/terraform/eval_state.go +++ b/terraform/eval_state.go @@ -424,15 +424,21 @@ func (n *EvalWriteResourceState) Eval(ctx EvalContext) (interface{}, error) { return nil, diags.Err() } - // Currently we ony support NoEach and EachList, because for_each support - // is not fully wired up across Terraform. Once for_each support is added, - // we'll need to handle that here too, setting states.EachMap if the - // assigned expression is a map. eachMode := states.NoEach if count >= 0 { // -1 signals "count not set" eachMode = states.EachList } + forEach, forEachDiags := evaluateResourceForEachExpression(n.Config.ForEach, ctx) + diags = diags.Append(forEachDiags) + if forEachDiags.HasErrors() { + return nil, diags.Err() + } + + if forEach != nil { + eachMode = states.EachMap + } + // This method takes care of all of the business logic of updating this // while ensuring that any existing instances are preserved, etc. state.SetResourceMeta(absAddr, eachMode, n.ProviderAddr) diff --git a/terraform/eval_validate.go b/terraform/eval_validate.go index 581a91b0e2..9331143c51 100644 --- a/terraform/eval_validate.go +++ b/terraform/eval_validate.go @@ -377,7 +377,7 @@ func (n *EvalValidateResource) Eval(ctx EvalContext) (interface{}, error) { } // Evaluate the for_each expression here so we can expose the diagnostics - _, forEachDiags := evaluateResourceForEachExpression(n.Config.ForEach, ctx) + forEachDiags := n.validateForEach(ctx, n.Config.ForEach) diags = diags.Append(forEachDiags) } @@ -553,3 +553,18 @@ func (n *EvalValidateResource) validateCount(ctx EvalContext, expr hcl.Expressio return diags } + +func (n *EvalValidateResource) validateForEach(ctx EvalContext, expr hcl.Expression) (diags tfdiags.Diagnostics) { + _, known, forEachDiags := evaluateResourceForEachExpressionKnown(expr, ctx) + // If the value isn't known then that's the best we can do for now, but + // we'll check more thoroughly during the plan walk + if !known { + return diags + } + + if forEachDiags.HasErrors() { + diags = diags.Append(forEachDiags) + } + + return diags +} diff --git a/terraform/node_data_refresh.go b/terraform/node_data_refresh.go index ab82163412..dd9286648b 100644 --- a/terraform/node_data_refresh.go +++ b/terraform/node_data_refresh.go @@ -38,6 +38,16 @@ func (n *NodeRefreshableDataResource) DynamicExpand(ctx EvalContext) (*Graph, er return nil, nil } + forEachMap, forEachKnown, forEachDiags := evaluateResourceForEachExpressionKnown(n.Config.ForEach, ctx) + if forEachDiags.HasErrors() { + return nil, diags.Err() + } + if !forEachKnown { + // If the for_each isn't known yet, we'll skip refreshing and try expansion + // again during the plan walk. + return nil, nil + } + // Next we need to potentially rename an instance address in the state // if we're transitioning whether "count" is set at all. fixResourceCountSetTransition(ctx, n.ResourceAddr(), count != -1) @@ -77,6 +87,7 @@ func (n *NodeRefreshableDataResource) DynamicExpand(ctx EvalContext) (*Graph, er Concrete: concreteResource, Schema: n.Schema, Count: count, + ForEach: forEachMap, Addr: n.ResourceAddr(), }, @@ -85,6 +96,7 @@ func (n *NodeRefreshableDataResource) DynamicExpand(ctx EvalContext) (*Graph, er &OrphanResourceCountTransformer{ Concrete: concreteResourceDestroyable, Count: count, + ForEach: forEachMap, Addr: n.ResourceAddr(), State: state, },