From 90764004363ccd80ed1ef775bf85253caab2d0b9 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Fri, 20 Nov 2020 14:56:21 -0800 Subject: [PATCH] configs: Decode preconditions and postconditions This allows precondition and postcondition checks to be declared for resources and output values as long as the preconditions_postconditions experiment is enabled. Terraform Core doesn't currently know anything about these features, so as of this commit declaring them does nothing at all. --- internal/configs/checks.go | 11 +++ internal/configs/experiments.go | 49 ++++++++++ internal/configs/named_values.go | 26 +++++ internal/configs/parser_config.go | 4 +- internal/configs/parser_config_test.go | 2 +- internal/configs/resource.go | 94 ++++++++++++++++--- .../precondition-postcondition-constant.tf | 34 +++++++ .../invalid-files/data-reserved-lifecycle.tf | 3 - .../invalid-files/data-resource-lifecycle.tf | 4 +- ...preconditions-postconditions-experiment.tf | 34 +++++++ ...preconditions-postconditions-experiment.tf | 38 ++++++++ 11 files changed, 279 insertions(+), 20 deletions(-) create mode 100644 internal/configs/testdata/error-files/precondition-postcondition-constant.tf delete mode 100644 internal/configs/testdata/invalid-files/data-reserved-lifecycle.tf create mode 100644 internal/configs/testdata/invalid-modules/preconditions-postconditions-experiment/preconditions-postconditions-experiment.tf create mode 100644 internal/configs/testdata/warning-files/preconditions-postconditions-experiment.tf diff --git a/internal/configs/checks.go b/internal/configs/checks.go index 31af8ae2f..8339640cc 100644 --- a/internal/configs/checks.go +++ b/internal/configs/checks.go @@ -64,6 +64,17 @@ func decodeCheckRuleBlock(block *hcl.Block, override bool) (*CheckRule, hcl.Diag if attr, exists := content.Attributes["condition"]; exists { cr.Condition = attr.Expr + + if len(cr.Condition.Variables()) == 0 { + // A condition expression that doesn't refer to any variable is + // pointless, because its result would always be a constant. + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: fmt.Sprintf("Invalid %s expression", block.Type), + Detail: "The condition expression must refer to at least one object from elsewhere in the configuration, or else its result would not be checking anything.", + Subject: cr.Condition.Range().Ptr(), + }) + } } if attr, exists := content.Attributes["error_message"]; exists { diff --git a/internal/configs/experiments.go b/internal/configs/experiments.go index 2ebf2d700..9175fd01f 100644 --- a/internal/configs/experiments.go +++ b/internal/configs/experiments.go @@ -209,6 +209,55 @@ func checkModuleExperiments(m *Module) hcl.Diagnostics { } } + if !m.ActiveExperiments.Has(experiments.PreconditionsPostconditions) { + for _, r := range m.ManagedResources { + for _, c := range r.Preconditions { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Preconditions are experimental", + Detail: "The resource preconditions feature is currently an opt-in experiment, subject to change in future releases based on feedback.\n\nActivate the feature for this module by adding preconditions_postconditions to the list of active experiments.", + Subject: c.DeclRange.Ptr(), + }) + } + for _, c := range r.Postconditions { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Postconditions are experimental", + Detail: "The resource preconditions feature is currently an opt-in experiment, subject to change in future releases based on feedback.\n\nActivate the feature for this module by adding preconditions_postconditions to the list of active experiments.", + Subject: c.DeclRange.Ptr(), + }) + } + } + for _, r := range m.DataResources { + for _, c := range r.Preconditions { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Preconditions are experimental", + Detail: "The resource preconditions feature is currently an opt-in experiment, subject to change in future releases based on feedback.\n\nActivate the feature for this module by adding preconditions_postconditions to the list of active experiments.", + Subject: c.DeclRange.Ptr(), + }) + } + for _, c := range r.Postconditions { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Postconditions are experimental", + Detail: "The resource preconditions feature is currently an opt-in experiment, subject to change in future releases based on feedback.\n\nActivate the feature for this module by adding preconditions_postconditions to the list of active experiments.", + Subject: c.DeclRange.Ptr(), + }) + } + } + for _, o := range m.Outputs { + for _, c := range o.Preconditions { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Preconditions are experimental", + Detail: "The output value preconditions feature is currently an opt-in experiment, subject to change in future releases based on feedback.\n\nActivate the feature for this module by adding preconditions_postconditions to the list of active experiments.", + Subject: c.DeclRange.Ptr(), + }) + } + } + } + return diags } diff --git a/internal/configs/named_values.go b/internal/configs/named_values.go index 021339885..970656511 100644 --- a/internal/configs/named_values.go +++ b/internal/configs/named_values.go @@ -356,6 +356,8 @@ type Output struct { DependsOn []hcl.Traversal Sensitive bool + Preconditions []*CheckRule + DescriptionSet bool SensitiveSet bool @@ -409,6 +411,26 @@ func decodeOutputBlock(block *hcl.Block, override bool) (*Output, hcl.Diagnostic o.DependsOn = append(o.DependsOn, deps...) } + for _, block := range content.Blocks { + switch block.Type { + case "precondition": + cr, moreDiags := decodeCheckRuleBlock(block, override) + diags = append(diags, moreDiags...) + o.Preconditions = append(o.Preconditions, cr) + case "postcondition": + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Postconditions are not allowed", + Detail: "Output values can only have preconditions, not postconditions.", + Subject: block.TypeRange.Ptr(), + }) + default: + // The cases above should be exhaustive for all block types + // defined in the block type schema, so this shouldn't happen. + panic(fmt.Sprintf("unexpected lifecycle sub-block type %q", block.Type)) + } + } + return o, diags } @@ -497,4 +519,8 @@ var outputBlockSchema = &hcl.BodySchema{ Name: "sensitive", }, }, + Blocks: []hcl.BlockHeaderSchema{ + {Type: "precondition"}, + {Type: "postcondition"}, + }, } diff --git a/internal/configs/parser_config.go b/internal/configs/parser_config.go index caebb8911..0281339c6 100644 --- a/internal/configs/parser_config.go +++ b/internal/configs/parser_config.go @@ -142,14 +142,14 @@ func (p *Parser) loadConfigFile(path string, override bool) (*File, hcl.Diagnost } case "resource": - cfg, cfgDiags := decodeResourceBlock(block) + cfg, cfgDiags := decodeResourceBlock(block, override) diags = append(diags, cfgDiags...) if cfg != nil { file.ManagedResources = append(file.ManagedResources, cfg) } case "data": - cfg, cfgDiags := decodeDataBlock(block) + cfg, cfgDiags := decodeDataBlock(block, override) diags = append(diags, cfgDiags...) if cfg != nil { file.DataResources = append(file.DataResources, cfg) diff --git a/internal/configs/parser_config_test.go b/internal/configs/parser_config_test.go index 7832914c9..e1244ade1 100644 --- a/internal/configs/parser_config_test.go +++ b/internal/configs/parser_config_test.go @@ -97,7 +97,7 @@ func TestParserLoadConfigFileFailureMessages(t *testing.T) { { "invalid-files/data-resource-lifecycle.tf", hcl.DiagError, - "Unsupported lifecycle block", + "Invalid data resource lifecycle argument", }, { "invalid-files/variable-type-unknown.tf", diff --git a/internal/configs/resource.go b/internal/configs/resource.go index ec38cf5ca..0928f701e 100644 --- a/internal/configs/resource.go +++ b/internal/configs/resource.go @@ -22,6 +22,9 @@ type Resource struct { ProviderConfigRef *ProviderConfigRef Provider addrs.Provider + Preconditions []*CheckRule + Postconditions []*CheckRule + DependsOn []hcl.Traversal // Managed is populated only for Mode = addrs.ManagedResourceMode, @@ -81,7 +84,7 @@ func (r *Resource) ProviderConfigAddr() addrs.LocalProviderConfig { } } -func decodeResourceBlock(block *hcl.Block) (*Resource, hcl.Diagnostics) { +func decodeResourceBlock(block *hcl.Block, override bool) (*Resource, hcl.Diagnostics) { var diags hcl.Diagnostics r := &Resource{ Mode: addrs.ManagedResourceMode, @@ -237,6 +240,24 @@ func decodeResourceBlock(block *hcl.Block) (*Resource, hcl.Diagnostics) { } + for _, block := range lcContent.Blocks { + switch block.Type { + case "precondition", "postcondition": + cr, moreDiags := decodeCheckRuleBlock(block, override) + diags = append(diags, moreDiags...) + switch block.Type { + case "precondition": + r.Preconditions = append(r.Preconditions, cr) + case "postcondition": + r.Postconditions = append(r.Postconditions, cr) + } + default: + // The cases above should be exhaustive for all block types + // defined in the lifecycle schema, so this shouldn't happen. + panic(fmt.Sprintf("unexpected lifecycle sub-block type %q", block.Type)) + } + } + case "connection": if seenConnection != nil { diags = append(diags, &hcl.Diagnostic{ @@ -307,7 +328,7 @@ func decodeResourceBlock(block *hcl.Block) (*Resource, hcl.Diagnostics) { return r, diags } -func decodeDataBlock(block *hcl.Block) (*Resource, hcl.Diagnostics) { +func decodeDataBlock(block *hcl.Block, override bool) (*Resource, hcl.Diagnostics) { var diags hcl.Diagnostics r := &Resource{ Mode: addrs.DataResourceMode, @@ -368,6 +389,7 @@ func decodeDataBlock(block *hcl.Block) (*Resource, hcl.Diagnostics) { } var seenEscapeBlock *hcl.Block + var seenLifecycle *hcl.Block for _, block := range content.Blocks { switch block.Type { @@ -391,21 +413,59 @@ func decodeDataBlock(block *hcl.Block) (*Resource, hcl.Diagnostics) { // will see a blend of both. r.Config = hcl.MergeBodies([]hcl.Body{r.Config, block.Body}) - // The rest of these are just here to reserve block type names for future use. case "lifecycle": - diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Unsupported lifecycle block", - Detail: "Data resources do not have lifecycle settings, so a lifecycle block is not allowed.", - Subject: &block.DefRange, - }) + if seenLifecycle != nil { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Duplicate lifecycle block", + Detail: fmt.Sprintf("This resource already has a lifecycle block at %s.", seenLifecycle.DefRange), + Subject: block.DefRange.Ptr(), + }) + continue + } + seenLifecycle = block + + lcContent, lcDiags := block.Body.Content(resourceLifecycleBlockSchema) + diags = append(diags, lcDiags...) + + // All of the attributes defined for resource lifecycle are for + // managed resources only, so we can emit a common error message + // for any given attributes that HCL accepted. + for name, attr := range lcContent.Attributes { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid data resource lifecycle argument", + Detail: fmt.Sprintf("The lifecycle argument %q is defined only for managed resources (\"resource\" blocks), and is not valid for data resources.", name), + Subject: attr.NameRange.Ptr(), + }) + } + + for _, block := range lcContent.Blocks { + switch block.Type { + case "precondition", "postcondition": + cr, moreDiags := decodeCheckRuleBlock(block, override) + diags = append(diags, moreDiags...) + switch block.Type { + case "precondition": + r.Preconditions = append(r.Preconditions, cr) + case "postcondition": + r.Postconditions = append(r.Postconditions, cr) + } + default: + // The cases above should be exhaustive for all block types + // defined in the lifecycle schema, so this shouldn't happen. + panic(fmt.Sprintf("unexpected lifecycle sub-block type %q", block.Type)) + } + } default: + // Any other block types are ones we're reserving for future use, + // but don't have any defined meaning today. diags = append(diags, &hcl.Diagnostic{ Severity: hcl.DiagError, Summary: "Reserved block type name in data block", Detail: fmt.Sprintf("The block type name %q is reserved for use by Terraform in a future version.", block.Type), - Subject: &block.TypeRange, + Subject: block.TypeRange.Ptr(), }) } } @@ -551,13 +611,17 @@ var resourceBlockSchema = &hcl.BodySchema{ var dataBlockSchema = &hcl.BodySchema{ Attributes: commonResourceAttributes, Blocks: []hcl.BlockHeaderSchema{ - {Type: "lifecycle"}, // reserved for future use - {Type: "locals"}, // reserved for future use - {Type: "_"}, // meta-argument escaping block + {Type: "lifecycle"}, + {Type: "locals"}, // reserved for future use + {Type: "_"}, // meta-argument escaping block }, } var resourceLifecycleBlockSchema = &hcl.BodySchema{ + // We tell HCL that these elements are all valid for both "resource" + // and "data" lifecycle blocks, but the rules are actually more restrictive + // than that. We deal with that after decoding so that we can return + // more specific error messages than HCL would typically return itself. Attributes: []hcl.AttributeSchema{ { Name: "create_before_destroy", @@ -569,4 +633,8 @@ var resourceLifecycleBlockSchema = &hcl.BodySchema{ Name: "ignore_changes", }, }, + Blocks: []hcl.BlockHeaderSchema{ + {Type: "precondition"}, + {Type: "postcondition"}, + }, } diff --git a/internal/configs/testdata/error-files/precondition-postcondition-constant.tf b/internal/configs/testdata/error-files/precondition-postcondition-constant.tf new file mode 100644 index 000000000..30f44313e --- /dev/null +++ b/internal/configs/testdata/error-files/precondition-postcondition-constant.tf @@ -0,0 +1,34 @@ +resource "test" "test" { + lifecycle { + precondition { + condition = true # ERROR: Invalid precondition expression + error_message = "Must be true." + } + postcondition { + condition = true # ERROR: Invalid postcondition expression + error_message = "Must be true." + } + } +} + +data "test" "test" { + lifecycle { + precondition { + condition = true # ERROR: Invalid precondition expression + error_message = "Must be true." + } + postcondition { + condition = true # ERROR: Invalid postcondition expression + error_message = "Must be true." + } + } +} + +output "test" { + value = "" + + precondition { + condition = true # ERROR: Invalid precondition expression + error_message = "Must be true." + } +} diff --git a/internal/configs/testdata/invalid-files/data-reserved-lifecycle.tf b/internal/configs/testdata/invalid-files/data-reserved-lifecycle.tf deleted file mode 100644 index a1e1a1e6e..000000000 --- a/internal/configs/testdata/invalid-files/data-reserved-lifecycle.tf +++ /dev/null @@ -1,3 +0,0 @@ -data "test" "foo" { - lifecycle {} -} diff --git a/internal/configs/testdata/invalid-files/data-resource-lifecycle.tf b/internal/configs/testdata/invalid-files/data-resource-lifecycle.tf index b0c34d463..7c1aebe5a 100644 --- a/internal/configs/testdata/invalid-files/data-resource-lifecycle.tf +++ b/internal/configs/testdata/invalid-files/data-resource-lifecycle.tf @@ -1,5 +1,7 @@ data "example" "example" { lifecycle { - # This block intentionally left blank + # The lifecycle arguments are not valid for data resources: + # only the precondition and postcondition blocks are allowed. + ignore_changes = [] } } diff --git a/internal/configs/testdata/invalid-modules/preconditions-postconditions-experiment/preconditions-postconditions-experiment.tf b/internal/configs/testdata/invalid-modules/preconditions-postconditions-experiment/preconditions-postconditions-experiment.tf new file mode 100644 index 000000000..8b63e77f3 --- /dev/null +++ b/internal/configs/testdata/invalid-modules/preconditions-postconditions-experiment/preconditions-postconditions-experiment.tf @@ -0,0 +1,34 @@ +resource "test" "test" { + lifecycle { + precondition { # ERROR: Preconditions are experimental + condition = path.module != "" + error_message = "Must be true." + } + postcondition { # ERROR: Postconditions are experimental + condition = path.module != "" + error_message = "Must be true." + } + } +} + +data "test" "test" { + lifecycle { + precondition { # ERROR: Preconditions are experimental + condition = path.module != "" + error_message = "Must be true." + } + postcondition { # ERROR: Postconditions are experimental + condition = path.module != "" + error_message = "Must be true." + } + } +} + +output "test" { + value = "" + + precondition { # ERROR: Preconditions are experimental + condition = path.module != "" + error_message = "Must be true." + } +} diff --git a/internal/configs/testdata/warning-files/preconditions-postconditions-experiment.tf b/internal/configs/testdata/warning-files/preconditions-postconditions-experiment.tf new file mode 100644 index 000000000..35dac3472 --- /dev/null +++ b/internal/configs/testdata/warning-files/preconditions-postconditions-experiment.tf @@ -0,0 +1,38 @@ +terraform { + experiments = [preconditions_postconditions] # WARNING: Experimental feature "preconditions_postconditions" is active +} + +resource "test" "test" { + lifecycle { + precondition { + condition = path.module != "" + error_message = "Must be true." + } + postcondition { + condition = path.module != "" + error_message = "Must be true." + } + } +} + +data "test" "test" { + lifecycle { + precondition { + condition = path.module != "" + error_message = "Must be true." + } + postcondition { + condition = path.module != "" + error_message = "Must be true." + } + } +} + +output "test" { + value = "" + + precondition { + condition = path.module != "" + error_message = "Must be true." + } +}