From cd747a534bb076614a05bb58325d624ebcefb32a Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Mon, 27 Sep 2021 16:59:43 -0700 Subject: [PATCH] core: New lint-checking mode for Validate Here we establish a new mode for Context.Validate which enables some extra checks which can produce warnings that don't necessarily need to be fixed in order for a Terraform module to be accepted, but that which are indicative of common mistakes that might lead to unintended behavior or of non-idiomatic style. The idea of this mode is that users could potentially enable it if they are seeing some unexpected behavior, to potentially get some additional hints as to what might be causing it. Since linting might sometimes generate false negatives, this mode probably won't be suitable for use as a pre-merge automated check. For this first round, the lint checks are focused on the depends_on arguments within "resource" and "data" blocks, catching situations where entries in depends_on are redundant with other declarations or refer to individual resource instances when Terraform only considers whole resources. This new mode is currently not exposed in the UI anywhere, so this is all just dead code for the moment. Perhaps in future it will be exposed either as a new mode for "terraform validate" or as a new command "terraform lint", but we'll save that decision for later work. --- internal/terraform/context_validate.go | 18 ++- .../terraform/context_validate_lint_test.go | 131 +++++++++++++++ internal/terraform/graph_builder_validate.go | 5 + internal/terraform/linter.go | 91 +++++++++++ internal/terraform/node_resource_validate.go | 151 ++++++++++++++++++ .../redundant-resource-depends.tf | 45 ++++++ 6 files changed, 438 insertions(+), 3 deletions(-) create mode 100644 internal/terraform/context_validate_lint_test.go create mode 100644 internal/terraform/linter.go create mode 100644 internal/terraform/testdata/validate-lint-redundant-resource-depends/redundant-resource-depends.tf diff --git a/internal/terraform/context_validate.go b/internal/terraform/context_validate.go index 1e230815ac8c..2d4ec707dbaf 100644 --- a/internal/terraform/context_validate.go +++ b/internal/terraform/context_validate.go @@ -13,13 +13,25 @@ import ( // ValidateOpts are the various options that affect the details of how Terraform // will validate a configuration. type ValidateOpts struct { - // There are currently no special options for Validate. This struct is - // just here to allow for less-disruptive addition of new options later. + // LintChecks, if set to true, enables additional warnings that describe + // ways to possibly improve a Terraform configuration even though the + // current configuration is valid as written. + // + // The additional warnings produced in "lint" mode are more subjective and + // so module authors can evaluate each one and choose to evaluate warnings + // that don't apply in some particular situations. There might be additional + // lint warnings in later releases, thus making a previously-lint-free + // configuration potentially lint-y again, and so considering lint warnings + // should typically be a development task in its own right, rather than a + // blocker for completing other development tasks. + LintChecks bool } // DefaultValidateOpts is a reasonable default set of validate options to use // in common cases without any special needs. -var DefaultValidateOpts = &ValidateOpts{} +var DefaultValidateOpts = &ValidateOpts{ + LintChecks: false, +} // Validate performs semantic validation of a configuration, and returns // any warnings or errors. diff --git a/internal/terraform/context_validate_lint_test.go b/internal/terraform/context_validate_lint_test.go new file mode 100644 index 000000000000..8b4a87491f3d --- /dev/null +++ b/internal/terraform/context_validate_lint_test.go @@ -0,0 +1,131 @@ +package terraform + +import ( + "testing" + + "github.com/hashicorp/hcl/v2" + "github.com/hashicorp/terraform/internal/addrs" + "github.com/hashicorp/terraform/internal/configs/configschema" + "github.com/hashicorp/terraform/internal/providers" + "github.com/hashicorp/terraform/internal/tfdiags" + "github.com/zclconf/go-cty/cty" +) + +// The tests in this file are for the extra warnings we produce when running +// validation in linting mode. All of the tests in here should be calling +// validate with LintChecks: true at some point, or else they'd be better +// placed in context_validate_test.go instead. + +func TestContextValidateLint_redundantResourceDepends(t *testing.T) { + p := testProvider("test") + p.GetProviderSchemaResponse = getProviderSchemaResponseFromProviderSchema(&ProviderSchema{ + ResourceTypes: map[string]*configschema.Block{ + "test_thing": { + Attributes: map[string]*configschema.Attribute{ + "name": { + Type: cty.String, + Optional: true, + }, + }, + BlockTypes: map[string]*configschema.NestedBlock{ + "single_block": { + Nesting: configschema.NestingSingle, + Block: configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "id": { + Type: cty.String, + Optional: true, + }, + }, + }, + }, + "map_block": { + Nesting: configschema.NestingMap, + Block: configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "doodad": { + Type: cty.String, + Optional: true, + }, + }, + }, + }, + }, + }, + }, + }) + + m := testModule(t, "validate-lint-redundant-resource-depends") + c := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + gotDiags := c.Validate(m, &ValidateOpts{ + LintChecks: true, + }) + var wantDiags tfdiags.Diagnostics + wantDiags = wantDiags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagWarning, + Summary: "Redundant explicit dependency", + Detail: "There is already an implied dependency for test_thing.a at testdata/validate-lint-redundant-resource-depends/redundant-resource-depends.tf:23,10, so this declaration is redundant.", + Subject: &hcl.Range{ + Filename: "testdata/validate-lint-redundant-resource-depends/redundant-resource-depends.tf", + Start: hcl.Pos{Line: 34, Column: 5, Byte: 404}, + End: hcl.Pos{Line: 34, Column: 17, Byte: 416}, + }, + }) + wantDiags = wantDiags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagWarning, + Summary: "Redundant explicit dependency", + Detail: "There is already an explicit dependency for test_thing.b at testdata/validate-lint-redundant-resource-depends/redundant-resource-depends.tf:35,5, so this declaration is redundant.", + Subject: &hcl.Range{ + Filename: "testdata/validate-lint-redundant-resource-depends/redundant-resource-depends.tf", + Start: hcl.Pos{Line: 36, Column: 5, Byte: 440}, + End: hcl.Pos{Line: 36, Column: 17, Byte: 452}, + }, + }) + wantDiags = wantDiags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagWarning, + Summary: "Redundant explicit dependency", + Detail: "There is already an implied dependency for test_thing.c at testdata/validate-lint-redundant-resource-depends/redundant-resource-depends.tf:26,10, so this declaration is redundant.", + Subject: &hcl.Range{ + Filename: "testdata/validate-lint-redundant-resource-depends/redundant-resource-depends.tf", + Start: hcl.Pos{Line: 37, Column: 5, Byte: 458}, + End: hcl.Pos{Line: 37, Column: 17, Byte: 470}, + }, + }) + wantDiags = wantDiags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagWarning, + Summary: "Redundant explicit dependency", + Detail: "There is already an implied dependency for test_thing.d at testdata/validate-lint-redundant-resource-depends/redundant-resource-depends.tf:30,14, so this declaration is redundant.", + Subject: &hcl.Range{ + Filename: "testdata/validate-lint-redundant-resource-depends/redundant-resource-depends.tf", + Start: hcl.Pos{Line: 38, Column: 5, Byte: 476}, + End: hcl.Pos{Line: 38, Column: 17, Byte: 488}, + }, + }) + wantDiags = wantDiags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagWarning, + Summary: "Over-specified explicit dependency", + Detail: "Terraform references are between resources as a whole, and don't consider individual resource instances. The [0] instance key in this address has no effect.", + Subject: &hcl.Range{ + Filename: "testdata/validate-lint-redundant-resource-depends/redundant-resource-depends.tf", + Start: hcl.Pos{Line: 39, Column: 5, Byte: 494}, + End: hcl.Pos{Line: 39, Column: 20, Byte: 509}, + }, + }) + wantDiags = wantDiags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagWarning, + Summary: "Redundant explicit dependency", + Detail: "There is already an explicit dependency for test_thing.f at testdata/validate-lint-redundant-resource-depends/redundant-resource-depends.tf:39,5, so this declaration is redundant.", + Subject: &hcl.Range{ + Filename: "testdata/validate-lint-redundant-resource-depends/redundant-resource-depends.tf", + Start: hcl.Pos{Line: 40, Column: 5, Byte: 515}, + End: hcl.Pos{Line: 40, Column: 17, Byte: 527}, + }, + }) + assertDiagnosticsMatch(t, gotDiags, wantDiags) + +} diff --git a/internal/terraform/graph_builder_validate.go b/internal/terraform/graph_builder_validate.go index 80ea90febd87..2642873b80c0 100644 --- a/internal/terraform/graph_builder_validate.go +++ b/internal/terraform/graph_builder_validate.go @@ -11,6 +11,10 @@ import ( // PlanGraphBuilder given will be modified so it shouldn't be used for anything // else after calling this function. func validateGraphBuilder(p *PlanGraphBuilder, opts *ValidateOpts) GraphBuilder { + if opts == nil { + opts = DefaultValidateOpts + } + // We're going to customize the concrete functions p.CustomConcrete = true @@ -24,6 +28,7 @@ func validateGraphBuilder(p *PlanGraphBuilder, opts *ValidateOpts) GraphBuilder p.ConcreteResource = func(a *NodeAbstractResource) dag.Vertex { return &NodeValidatableResource{ NodeAbstractResource: a, + LintChecks: opts.LintChecks, } } diff --git a/internal/terraform/linter.go b/internal/terraform/linter.go new file mode 100644 index 000000000000..38b43df3d882 --- /dev/null +++ b/internal/terraform/linter.go @@ -0,0 +1,91 @@ +package terraform + +import ( + "github.com/hashicorp/hcl/v2" + "github.com/hashicorp/terraform/internal/configs/configschema" + "github.com/hashicorp/terraform/internal/tfdiags" +) + +// lintExpressionGeneric runs a generic set of linting rules on the given +// expression, which we assume is just a normal expression that'll be evaluated +// for its value in a lang.Scope, but with no specific assumptions about +// where in the configuration it is placed. +// +// It's safe to pass a nil expression to this function, in which case this +// function will never produce any lint warnings. +func lintExpressionGeneric(expr hcl.Expression) tfdiags.Diagnostics { + if expr == nil { + return nil + } + + // No individual expression rules yet + return nil +} + +// lintExpressionsInBodyGeneric runs lintExpressionGeneric against each of +// the expressions detected inside the given body, which it detects using +// the given schema. +// +// If the body doesn't fully conform to the schema then +// lintExpressionsInBodyGeneric will skip over some or all of the body contents +// in order to still produce a partial result where possible. +// +// It's safe to pass a nil body or schema to this function, in which case this +// function will never produce any lint warnings. +func lintExpressionsInBodyGeneric(body hcl.Body, schema *configschema.Block) tfdiags.Diagnostics { + if body == nil || schema == nil { + return nil + } + + return visitExpressionsInBody(body, schema, lintExpressionGeneric) +} + +// visitExpressionsInBody is a helper for writting linter-like logic which +// applies to all expressions declared in a particular HCL body, which should +// conform to the given schema. +// +// If the body doesn't fully conform to the schema then visitExpressionsInBody +// will skip over some or all of the body contents in order to still produce +// a partial result. Therefore this expression isn't suitable for implementing +// blocking validation, because it only makes a best effort to find expressions. +func visitExpressionsInBody(body hcl.Body, schema *configschema.Block, cb func(hcl.Expression) tfdiags.Diagnostics) tfdiags.Diagnostics { + var diags tfdiags.Diagnostics + + var hclSchema hcl.BodySchema + for name := range schema.Attributes { + hclSchema.Attributes = append(hclSchema.Attributes, hcl.AttributeSchema{ + Name: name, + // We intentionally don't set "Required" here because we still + // want to scan all given expressions, even if the author omitted + // some arguments. + }) + } + for typeName, blockS := range schema.BlockTypes { + var labelNames []string + if blockS.Nesting == configschema.NestingMap { + labelNames = []string{"key"} + } + + hclSchema.Blocks = append(hclSchema.Blocks, hcl.BlockHeaderSchema{ + Type: typeName, + LabelNames: labelNames, + }) + } + + content, _, _ := body.PartialContent(&hclSchema) + + for _, attr := range content.Attributes { + diags = diags.Append(cb(attr.Expr)) + } + + for _, block := range content.Blocks { + typeName := block.Type + blockS, ok := schema.BlockTypes[typeName] + if !ok { + continue // Weird, but we'll tolerate it to be robust + } + diags = diags.Append(visitExpressionsInBody(block.Body, &blockS.Block, cb)) + } + + return diags +} diff --git a/internal/terraform/node_resource_validate.go b/internal/terraform/node_resource_validate.go index cd4ec91fe80b..13f6a4b044b5 100644 --- a/internal/terraform/node_resource_validate.go +++ b/internal/terraform/node_resource_validate.go @@ -8,6 +8,7 @@ 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/lang" "github.com/hashicorp/terraform/internal/providers" "github.com/hashicorp/terraform/internal/provisioners" "github.com/hashicorp/terraform/internal/tfdiags" @@ -18,6 +19,7 @@ import ( // only. type NodeValidatableResource struct { *NodeAbstractResource + LintChecks bool } var ( @@ -59,6 +61,7 @@ func (n *NodeValidatableResource) Execute(ctx EvalContext, op walkOperation) (di } } } + return diags } @@ -442,6 +445,154 @@ func (n *NodeValidatableResource) validateResource(ctx EvalContext) tfdiags.Diag diags = diags.Append(resp.Diagnostics.InConfigBody(n.Config.Config, n.Addr.String())) } + if n.LintChecks { + diags = diags.Append(n.lintResource(ctx)) + } + + return diags +} + +func (n *NodeValidatableResource) lintResource(ctx EvalContext) tfdiags.Diagnostics { + // NOTE: Lint functions are only allowed to produce _warning_ diagnostics + var diags tfdiags.Diagnostics + + if n.Config == nil { + // We can't do anything without a configuration. + return diags + } + + // Generic expression linting first, before we do our resource-specific lints. + diags = diags.Append(lintExpressionGeneric(n.Config.Count)) + diags = diags.Append(lintExpressionGeneric(n.Config.ForEach)) + if schema := n.Schema; schema != nil { + // NOTE: This doesn't take into account references inside dynamic + // blocks, so we won't catch explicit dependencies being redundant + // with those for now. + // This also doesn't consider "attributes as blocks mode", and so + // we won't consider references that appear inside a block that's + // pretending to be an element of a list/set attribute. + diags = diags.Append(lintExpressionsInBodyGeneric(n.Config.Config, schema)) + } + + // We're intentionally ignoring n.forceDependsOn here because we're + // linting the configuration as written, not the dynamic effect of any + // other processing we might do during graph construction. + dependsOn := n.Config.DependsOn + + explicitRefs := make(map[addrs.UniqueKey]*addrs.Reference) + for _, traversal := range dependsOn { + ref, refDiags := addrs.ParseRef(traversal) + if refDiags.HasErrors() { + // We'll ignore invalid references altogether + continue + } + + if addr, ok := ref.Subject.(addrs.ResourceInstance); ok && addr.Key != addrs.NoKey { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagWarning, + Summary: "Over-specified explicit dependency", + Detail: fmt.Sprintf( + "Terraform references are between resources as a whole, and don't consider individual resource instances. The %s instance key in this address has no effect.", + addr.Key, + ), + Subject: ref.SourceRange.ToHCL().Ptr(), + }) + + // For the rest of our work here we'll pretend that this was a + // whole-resource reference, and thus we can potentially detect + // when there are redundant references to the same resource. + fakeRemain := make(hcl.Traversal, 0, len(ref.Remaining)+1) + switch key := addr.Key.(type) { + case addrs.IntKey: + fakeRemain = append(fakeRemain, hcl.TraverseIndex{ + Key: cty.NumberIntVal(int64(key)), + SrcRange: ref.SourceRange.ToHCL(), + }) + case addrs.StringKey: + fakeRemain = append(fakeRemain, hcl.TraverseIndex{ + Key: cty.StringVal(string(key)), + SrcRange: ref.SourceRange.ToHCL(), + }) + default: + // We're not really going to use the "remain" part for + // anything interesting here anyway, so we'll just stub + // it out to make the data structure rational. + fakeRemain = append(fakeRemain, hcl.TraverseIndex{ + Key: cty.DynamicVal, + SrcRange: ref.SourceRange.ToHCL(), + }) + } + fakeRemain = append(fakeRemain, ref.Remaining...) + + // A synthetic ref that we'll use for the rest of our linting work. + ref = &addrs.Reference{ + Subject: addr.ContainingResource(), + SourceRange: ref.SourceRange, + Remaining: fakeRemain, + } + } + + refKey := ref.Subject.UniqueKey() + if existing, exists := explicitRefs[refKey]; exists { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagWarning, + Summary: "Redundant explicit dependency", + Detail: fmt.Sprintf( + "There is already an explicit dependency for %s at %s, so this declaration is redundant.", + ref.Subject, existing.SourceRange.StartString(), + ), + Subject: ref.SourceRange.ToHCL().Ptr(), + }) + } else { + explicitRefs[ref.Subject.UniqueKey()] = ref + } + } + + var impliedRefsRaw []*addrs.Reference + moreRefs, _ := lang.ReferencesInExpr(n.Config.Count) + impliedRefsRaw = append(impliedRefsRaw, moreRefs...) + moreRefs, _ = lang.ReferencesInExpr(n.Config.ForEach) + impliedRefsRaw = append(impliedRefsRaw, moreRefs...) + if schema := n.Schema; schema != nil { + // NOTE: This doesn't take into account references inside dynamic + // blocks, so we won't catch explicit dependencies being redundant + // with those for now. + // This also doesn't consider "attributes as blocks mode", and so + // we won't consider references that appear inside a block that's + // pretending to be an element of a list/set attribute. + moreRefs, _ = lang.ReferencesInBlock(n.Config.Config, schema) + impliedRefsRaw = append(impliedRefsRaw, moreRefs...) + } + impliedRefs := make(map[addrs.UniqueKey]*addrs.Reference) + for _, ref := range impliedRefsRaw { + addrForKey := ref.Subject + if addr, ok := addrForKey.(addrs.ResourceInstance); ok { + // For graph building we only consider whole-resource instances, + // so we'll discard any instance key portion. + addrForKey = addr.ContainingResource() + } + impliedRefs[addrForKey.UniqueKey()] = ref + } + + for explicitKey, explicitRef := range explicitRefs { + if implicitRef, exists := impliedRefs[explicitKey]; exists { + // If we have both an implied and an explicit dependency then + // we flag the explicit one as the suspect one, because the implied + // one is presumably also contributing to a configuration value and + // thus couldn't be so easily removed in order to to quiet this + // lint. + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagWarning, + Summary: "Redundant explicit dependency", + Detail: fmt.Sprintf( + "There is already an implied dependency for %s at %s, so this declaration is redundant.", + explicitRef.Subject, implicitRef.SourceRange.StartString(), + ), + Subject: explicitRef.SourceRange.ToHCL().Ptr(), + }) + } + } + return diags } diff --git a/internal/terraform/testdata/validate-lint-redundant-resource-depends/redundant-resource-depends.tf b/internal/terraform/testdata/validate-lint-redundant-resource-depends/redundant-resource-depends.tf new file mode 100644 index 000000000000..d580cc95cebb --- /dev/null +++ b/internal/terraform/testdata/validate-lint-redundant-resource-depends/redundant-resource-depends.tf @@ -0,0 +1,45 @@ + +resource "test_thing" "a" { + count = 1 +} + +resource "test_thing" "b" { +} + +resource "test_thing" "c" { +} + +resource "test_thing" "d" { +} + +resource "test_thing" "e" { +} + +resource "test_thing" "f" { + count = 1 +} + +resource "test_thing" "depender" { + name = test_thing.a[0].name + + single_block { + id = test_thing.c.name + } + + map_block "boop" { + doodad = test_thing.d.name + } + + depends_on = [ + test_thing.a, + test_thing.b, + test_thing.b, + test_thing.c, + test_thing.d, + test_thing.f[0], + test_thing.f, + + # This one is okay because this is the only reference to it. + test_thing.e, + ] +}