From ad995322e1f36ba148a67de16082b0edbbdeeb11 Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Thu, 10 Mar 2022 13:46:40 -0500 Subject: [PATCH 1/2] core: Fix expanded condition block validation The previous precondition/postcondition block validation implementation failed if the enclosing resource was expanded. This commit fixes this by generating appropriate placeholder instance data for the resource, depending on whether `count` or `for_each` is used. --- internal/terraform/context_validate_test.go | 94 ++++++++++++++++++++ internal/terraform/node_resource_validate.go | 77 ++++++++++++---- 2 files changed, 156 insertions(+), 15 deletions(-) diff --git a/internal/terraform/context_validate_test.go b/internal/terraform/context_validate_test.go index c049b17ce..5b6b98da8 100644 --- a/internal/terraform/context_validate_test.go +++ b/internal/terraform/context_validate_test.go @@ -2385,3 +2385,97 @@ resource "aws_instance" "test" { t.Errorf("unexpected error.\ngot: %s\nshould contain: %q", got, want) } } + +func TestContext2Validate_precondition_count(t *testing.T) { + p := testProvider("aws") + p.GetProviderSchemaResponse = getProviderSchemaResponseFromProviderSchema(&ProviderSchema{ + ResourceTypes: map[string]*configschema.Block{ + "aws_instance": { + Attributes: map[string]*configschema.Attribute{ + "foo": {Type: cty.String, Optional: true}, + }, + }, + }, + }) + m := testModuleInline(t, map[string]string{ + "main.tf": ` +terraform { + experiments = [preconditions_postconditions] +} + +locals { + foos = ["bar", "baz"] +} + +resource "aws_instance" "test" { + count = 3 + foo = local.foos[count.index] + + lifecycle { + precondition { + condition = count.index < length(local.foos) + error_message = "Insufficient foos." + } + } +} + `, + }) + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("aws"): testProviderFuncFixed(p), + }, + }) + + diags := ctx.Validate(m) + if diags.HasErrors() { + t.Fatal(diags.ErrWithWarnings()) + } +} + +func TestContext2Validate_postcondition_forEach(t *testing.T) { + p := testProvider("aws") + p.GetProviderSchemaResponse = getProviderSchemaResponseFromProviderSchema(&ProviderSchema{ + ResourceTypes: map[string]*configschema.Block{ + "aws_instance": { + Attributes: map[string]*configschema.Attribute{ + "foo": {Type: cty.String, Optional: true}, + }, + }, + }, + }) + m := testModuleInline(t, map[string]string{ + "main.tf": ` +terraform { + experiments = [preconditions_postconditions] +} + +locals { + foos = toset(["bar", "baz", "boop"]) +} + +resource "aws_instance" "test" { + for_each = local.foos + foo = "foo" + + lifecycle { + postcondition { + condition = length(each.value) == 3 + error_message = "Short foo required, not \"${each.key}\"." + } + } +} + `, + }) + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("aws"): testProviderFuncFixed(p), + }, + }) + + diags := ctx.Validate(m) + if diags.HasErrors() { + t.Fatal(diags.ErrWithWarnings()) + } +} diff --git a/internal/terraform/node_resource_validate.go b/internal/terraform/node_resource_validate.go index 4f316906d..41c1231ca 100644 --- a/internal/terraform/node_resource_validate.go +++ b/internal/terraform/node_resource_validate.go @@ -9,6 +9,8 @@ import ( "github.com/hashicorp/terraform/internal/configs" "github.com/hashicorp/terraform/internal/configs/configschema" "github.com/hashicorp/terraform/internal/didyoumean" + "github.com/hashicorp/terraform/internal/instances" + "github.com/hashicorp/terraform/internal/lang" "github.com/hashicorp/terraform/internal/providers" "github.com/hashicorp/terraform/internal/provisioners" "github.com/hashicorp/terraform/internal/tfdiags" @@ -41,17 +43,7 @@ func (n *NodeValidatableResource) Path() addrs.ModuleInstance { func (n *NodeValidatableResource) Execute(ctx EvalContext, op walkOperation) (diags tfdiags.Diagnostics) { diags = diags.Append(n.validateResource(ctx)) - var self addrs.Referenceable - switch { - case n.Config.Count != nil: - self = n.Addr.Resource.Instance(addrs.IntKey(0)) - case n.Config.ForEach != nil: - self = n.Addr.Resource.Instance(addrs.StringKey("")) - default: - self = n.Addr.Resource.Instance(addrs.NoKey) - } - diags = diags.Append(validateCheckRules(ctx, n.Config.Preconditions, nil)) - diags = diags.Append(validateCheckRules(ctx, n.Config.Postconditions, self)) + diags = diags.Append(n.validateCheckRules(ctx, n.Config)) if managed := n.Config.Managed; managed != nil { hasCount := n.Config.Count != nil @@ -478,14 +470,69 @@ func (n *NodeValidatableResource) validateResource(ctx EvalContext) tfdiags.Diag return diags } -func validateCheckRules(ctx EvalContext, crs []*configs.CheckRule, self addrs.Referenceable) tfdiags.Diagnostics { +func (n *NodeValidatableResource) evaluateExpr(ctx EvalContext, expr hcl.Expression, wantTy cty.Type, self addrs.Referenceable, keyData instances.RepetitionData) (cty.Value, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics - for _, cr := range crs { - _, conditionDiags := ctx.EvaluateExpr(cr.Condition, cty.Bool, self) + refs, refDiags := lang.ReferencesInExpr(expr) + diags = diags.Append(refDiags) + + scope := ctx.EvaluationScope(self, keyData) + + hclCtx, moreDiags := scope.EvalContext(refs) + diags = diags.Append(moreDiags) + + result, hclDiags := expr.Value(hclCtx) + diags = diags.Append(hclDiags) + + return result, diags +} + +func (n *NodeValidatableResource) validateCheckRules(ctx EvalContext, config *configs.Resource) tfdiags.Diagnostics { + var diags tfdiags.Diagnostics + + keyData := EvalDataForNoInstanceKey + selfAddr := n.ResourceAddr().Resource.Instance(addrs.NoKey) + + if n.Config.Count != nil { + // For a resource that has count, we allow count.index but don't + // know at this stage what it will return. + keyData = InstanceKeyEvalData{ + CountIndex: cty.UnknownVal(cty.Number), + } + + // "self" can't point to an unknown key, but we'll force it to be + // key 0 here, which should return an unknown value of the + // expected type since none of these elements are known at this + // point anyway. + selfAddr = n.ResourceAddr().Resource.Instance(addrs.IntKey(0)) + } else if n.Config.ForEach != nil { + // For a resource that has for_each, we allow each.value and each.key + // but don't know at this stage what it will return. + keyData = InstanceKeyEvalData{ + EachKey: cty.UnknownVal(cty.String), + EachValue: cty.DynamicVal, + } + + // "self" can't point to an unknown key, but we'll force it to be + // key "" here, which should return an unknown value of the + // expected type since none of these elements are known at + // this point anyway. + selfAddr = n.ResourceAddr().Resource.Instance(addrs.StringKey("")) + } + + for _, cr := range config.Preconditions { + _, conditionDiags := n.evaluateExpr(ctx, cr.Condition, cty.Bool, nil, keyData) diags = diags.Append(conditionDiags) - _, errorMessageDiags := ctx.EvaluateExpr(cr.ErrorMessage, cty.String, self) + _, errorMessageDiags := n.evaluateExpr(ctx, cr.ErrorMessage, cty.Bool, nil, keyData) + diags = diags.Append(errorMessageDiags) + } + + for _, cr := range config.Postconditions { + _, conditionDiags := n.evaluateExpr(ctx, cr.Condition, cty.Bool, selfAddr, keyData) + diags = diags.Append(conditionDiags) + + _, errorMessageDiags := n.evaluateExpr(ctx, cr.ErrorMessage, cty.Bool, selfAddr, keyData) diags = diags.Append(errorMessageDiags) } From ef0d859af761f2b25140fc051d067a461db733f4 Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Thu, 10 Mar 2022 13:52:03 -0500 Subject: [PATCH 2/2] core: Refactor stub repetition data generation --- internal/terraform/node_resource_validate.go | 55 +++++-------------- .../terraform/node_resource_validate_test.go | 6 +- 2 files changed, 18 insertions(+), 43 deletions(-) diff --git a/internal/terraform/node_resource_validate.go b/internal/terraform/node_resource_validate.go index 41c1231ca..867cfc257 100644 --- a/internal/terraform/node_resource_validate.go +++ b/internal/terraform/node_resource_validate.go @@ -46,9 +46,6 @@ func (n *NodeValidatableResource) Execute(ctx EvalContext, op walkOperation) (di diags = diags.Append(n.validateCheckRules(ctx, n.Config)) if managed := n.Config.Managed; managed != nil { - hasCount := n.Config.Count != nil - hasForEach := n.Config.ForEach != nil - // Validate all the provisioners for _, p := range managed.Provisioners { if p.Connection == nil { @@ -58,7 +55,7 @@ func (n *NodeValidatableResource) Execute(ctx EvalContext, op walkOperation) (di } // Validate Provisioner Config - diags = diags.Append(n.validateProvisioner(ctx, p, hasCount, hasForEach)) + diags = diags.Append(n.validateProvisioner(ctx, p)) if diags.HasErrors() { return diags } @@ -70,7 +67,7 @@ func (n *NodeValidatableResource) Execute(ctx EvalContext, op walkOperation) (di // validateProvisioner validates the configuration of a provisioner belonging to // a resource. The provisioner config is expected to contain the merged // connection configurations. -func (n *NodeValidatableResource) validateProvisioner(ctx EvalContext, p *configs.Provisioner, hasCount, hasForEach bool) tfdiags.Diagnostics { +func (n *NodeValidatableResource) validateProvisioner(ctx EvalContext, p *configs.Provisioner) tfdiags.Diagnostics { var diags tfdiags.Diagnostics provisioner, err := ctx.Provisioner(p.Type) @@ -91,7 +88,7 @@ func (n *NodeValidatableResource) validateProvisioner(ctx EvalContext, p *config } // Validate the provisioner's own config first - configVal, _, configDiags := n.evaluateBlock(ctx, p.Config, provisionerSchema, hasCount, hasForEach) + configVal, _, configDiags := n.evaluateBlock(ctx, p.Config, provisionerSchema) diags = diags.Append(configDiags) if configVal == cty.NilVal { @@ -115,42 +112,14 @@ func (n *NodeValidatableResource) validateProvisioner(ctx EvalContext, p *config // configuration keys that are not valid for *any* communicator, catching // typos early rather than waiting until we actually try to run one of // the resource's provisioners. - _, _, connDiags := n.evaluateBlock(ctx, p.Connection.Config, connectionBlockSupersetSchema, hasCount, hasForEach) + _, _, connDiags := n.evaluateBlock(ctx, p.Connection.Config, connectionBlockSupersetSchema) diags = diags.Append(connDiags) } return diags } -func (n *NodeValidatableResource) evaluateBlock(ctx EvalContext, body hcl.Body, schema *configschema.Block, hasCount, hasForEach bool) (cty.Value, hcl.Body, tfdiags.Diagnostics) { - keyData := EvalDataForNoInstanceKey - selfAddr := n.ResourceAddr().Resource.Instance(addrs.NoKey) - - if hasCount { - // For a resource that has count, we allow count.index but don't - // know at this stage what it will return. - keyData = InstanceKeyEvalData{ - CountIndex: cty.UnknownVal(cty.Number), - } - - // "self" can't point to an unknown key, but we'll force it to be - // key 0 here, which should return an unknown value of the - // expected type since none of these elements are known at this - // point anyway. - selfAddr = n.ResourceAddr().Resource.Instance(addrs.IntKey(0)) - } else if hasForEach { - // For a resource that has for_each, we allow each.value and each.key - // but don't know at this stage what it will return. - keyData = InstanceKeyEvalData{ - EachKey: cty.UnknownVal(cty.String), - EachValue: cty.DynamicVal, - } - - // "self" can't point to an unknown key, but we'll force it to be - // key "" here, which should return an unknown value of the - // expected type since none of these elements are known at - // this point anyway. - selfAddr = n.ResourceAddr().Resource.Instance(addrs.StringKey("")) - } +func (n *NodeValidatableResource) evaluateBlock(ctx EvalContext, body hcl.Body, schema *configschema.Block) (cty.Value, hcl.Body, tfdiags.Diagnostics) { + keyData, selfAddr := n.stubRepetitionData(n.Config.Count != nil, n.Config.ForEach != nil) return ctx.EvaluateBlock(body, schema, selfAddr, keyData) } @@ -487,9 +456,7 @@ func (n *NodeValidatableResource) evaluateExpr(ctx EvalContext, expr hcl.Express return result, diags } -func (n *NodeValidatableResource) validateCheckRules(ctx EvalContext, config *configs.Resource) tfdiags.Diagnostics { - var diags tfdiags.Diagnostics - +func (n *NodeValidatableResource) stubRepetitionData(hasCount, hasForEach bool) (instances.RepetitionData, addrs.Referenceable) { keyData := EvalDataForNoInstanceKey selfAddr := n.ResourceAddr().Resource.Instance(addrs.NoKey) @@ -520,6 +487,14 @@ func (n *NodeValidatableResource) validateCheckRules(ctx EvalContext, config *co selfAddr = n.ResourceAddr().Resource.Instance(addrs.StringKey("")) } + return keyData, selfAddr +} + +func (n *NodeValidatableResource) validateCheckRules(ctx EvalContext, config *configs.Resource) tfdiags.Diagnostics { + var diags tfdiags.Diagnostics + + keyData, selfAddr := n.stubRepetitionData(n.Config.Count != nil, n.Config.ForEach != nil) + for _, cr := range config.Preconditions { _, conditionDiags := n.evaluateExpr(ctx, cr.Condition, cty.Bool, nil, keyData) diags = diags.Append(conditionDiags) diff --git a/internal/terraform/node_resource_validate_test.go b/internal/terraform/node_resource_validate_test.go index d0e08c730..b5b2af74c 100644 --- a/internal/terraform/node_resource_validate_test.go +++ b/internal/terraform/node_resource_validate_test.go @@ -51,7 +51,7 @@ func TestNodeValidatableResource_ValidateProvisioner_valid(t *testing.T) { }, } - diags := node.validateProvisioner(ctx, pc, false, false) + diags := node.validateProvisioner(ctx, pc) if diags.HasErrors() { t.Fatalf("node.Eval failed: %s", diags.Err()) } @@ -96,7 +96,7 @@ func TestNodeValidatableResource_ValidateProvisioner__warning(t *testing.T) { } } - diags := node.validateProvisioner(ctx, pc, false, false) + diags := node.validateProvisioner(ctx, pc) if len(diags) != 1 { t.Fatalf("wrong number of diagnostics in %s; want one warning", diags.ErrWithWarnings()) } @@ -141,7 +141,7 @@ func TestNodeValidatableResource_ValidateProvisioner__connectionInvalid(t *testi }, } - diags := node.validateProvisioner(ctx, pc, false, false) + diags := node.validateProvisioner(ctx, pc) if !diags.HasErrors() { t.Fatalf("node.Eval succeeded; want error") }