From e7949480d7a2b8fcc4e85e085892fc76e9c52d64 Mon Sep 17 00:00:00 2001 From: James Pogran Date: Wed, 24 Jul 2024 12:08:25 -0400 Subject: [PATCH 1/2] Enable early validation for Terraform Stacks This change enables early validation for Terraform Stacks in tfstack. hcl and tfdeploy.hcl files. Available validations: - Missing required attributes - Deprecated attributes - Deprecated blocks - Unexpected attributes - Unexpected blocks - Maximum number of blocks - Minimum number of blocks - Block labels length This adds a new job to validate the schema of the these files. The job is enqueued when the `EnableEnhancedValidation` option is set to true. --- .../ENHANCEMENTS-20240724-122601.yaml | 6 + internal/features/stacks/ast/stacks.go | 8 ++ .../features/stacks/decoder/path_reader.go | 2 + .../validations/missing_required_attribute.go | 72 +++++++++++ .../features/stacks/decoder/validators.go | 20 +++ internal/features/stacks/events.go | 21 ++++ internal/features/stacks/jobs/validation.go | 118 ++++++++++++++++++ .../module/operation/op_type_string.go | 17 +-- .../terraform/module/operation/operation.go | 1 + 9 files changed, 257 insertions(+), 8 deletions(-) create mode 100644 .changes/unreleased/ENHANCEMENTS-20240724-122601.yaml create mode 100644 internal/features/stacks/decoder/validations/missing_required_attribute.go create mode 100644 internal/features/stacks/decoder/validators.go create mode 100644 internal/features/stacks/jobs/validation.go diff --git a/.changes/unreleased/ENHANCEMENTS-20240724-122601.yaml b/.changes/unreleased/ENHANCEMENTS-20240724-122601.yaml new file mode 100644 index 000000000..ba1f1b7aa --- /dev/null +++ b/.changes/unreleased/ENHANCEMENTS-20240724-122601.yaml @@ -0,0 +1,6 @@ +kind: ENHANCEMENTS +body: Enable early validation for Terraform Stack files +time: 2024-07-24T12:26:01.677192-04:00 +custom: + Issue: "1776" + Repository: terraform-ls diff --git a/internal/features/stacks/ast/stacks.go b/internal/features/stacks/ast/stacks.go index fd1fb9618..34886c6bb 100644 --- a/internal/features/stacks/ast/stacks.go +++ b/internal/features/stacks/ast/stacks.go @@ -124,6 +124,14 @@ func (sd Diagnostics) Count() int { return count } +func DiagnosticsFromMap(m map[string]hcl.Diagnostics) Diagnostics { + mf := make(Diagnostics, len(m)) + for name, file := range m { + mf[FilenameFromName(name)] = file + } + return mf +} + type SourceDiagnostics map[globalAst.DiagnosticSource]Diagnostics func (svd SourceDiagnostics) Count() int { diff --git a/internal/features/stacks/decoder/path_reader.go b/internal/features/stacks/decoder/path_reader.go index 91846a4e1..04c5db295 100644 --- a/internal/features/stacks/decoder/path_reader.go +++ b/internal/features/stacks/decoder/path_reader.go @@ -102,6 +102,7 @@ func stackPathContext(record *state.StackRecord, stateReader CombinedReader) (*d ReferenceTargets: make(reference.Targets, 0), Files: make(map[string]*hcl.File, 0), Functions: mustFunctionsForVersion(version), + Validators: stackValidators, } // TODO: Add reference origins and targets if needed @@ -148,6 +149,7 @@ func deployPathContext(record *state.StackRecord) (*decoder.PathContext, error) ReferenceOrigins: make(reference.Origins, 0), ReferenceTargets: make(reference.Targets, 0), Files: make(map[string]*hcl.File, 0), + Validators: stackValidators, } // TODO: Add reference origins and targets if needed diff --git a/internal/features/stacks/decoder/validations/missing_required_attribute.go b/internal/features/stacks/decoder/validations/missing_required_attribute.go new file mode 100644 index 000000000..4aa4544d1 --- /dev/null +++ b/internal/features/stacks/decoder/validations/missing_required_attribute.go @@ -0,0 +1,72 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package validations + +import ( + "context" + "fmt" + + "github.com/hashicorp/hcl-lang/schema" + "github.com/hashicorp/hcl-lang/schemacontext" + "github.com/hashicorp/hcl/v2" + "github.com/hashicorp/hcl/v2/hclsyntax" +) + +type MissingRequiredAttribute struct{} + +func (mra MissingRequiredAttribute) Visit(ctx context.Context, node hclsyntax.Node, nodeSchema schema.Schema) (context.Context, hcl.Diagnostics) { + var diags hcl.Diagnostics + if HasUnknownRequiredAttributes(ctx) { + return ctx, diags + } + + switch nodeType := node.(type) { + case *hclsyntax.Block: + // Providers were excluded from Terraform validation due to https://github.com/hashicorp/vscode-terraform/issues/1616 + // TODO: See if we can remove this exclusion for Terraform Stacks + // We would need to check if the provider can be configured via environment variables in Stacks. Since you can + // have multiple configurations for the same provider, this may be challenging. + // If it is possible, we may need to update this code to reflect the nested config { } block. But we're not sure right now. + nestingLvl, nestingOk := schemacontext.BlockNestingLevel(ctx) + if nodeType.Type == "provider" && (nestingOk && nestingLvl == 0) { + ctx = WithUnknownRequiredAttributes(ctx) + } + case *hclsyntax.Body: + if nodeSchema == nil { + return ctx, diags + } + + bodySchema := nodeSchema.(*schema.BodySchema) + if bodySchema.Attributes == nil { + return ctx, diags + } + + for name, attr := range bodySchema.Attributes { + if attr.IsRequired { + _, ok := nodeType.Attributes[name] + if !ok { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: fmt.Sprintf("Required attribute %q not specified", name), + Detail: fmt.Sprintf("An attribute named %q is required here", name), + Subject: nodeType.SrcRange.Ptr(), + }) + } + } + } + } + + return ctx, diags +} + +type unknownRequiredAttrsCtxKey struct{} + +func HasUnknownRequiredAttributes(ctx context.Context) bool { + _, ok := ctx.Value(unknownRequiredAttrsCtxKey{}).(bool) + return ok +} + +func WithUnknownRequiredAttributes(ctx context.Context) context.Context { + return context.WithValue(ctx, unknownRequiredAttrsCtxKey{}, true) +} diff --git a/internal/features/stacks/decoder/validators.go b/internal/features/stacks/decoder/validators.go new file mode 100644 index 000000000..9573bbbe9 --- /dev/null +++ b/internal/features/stacks/decoder/validators.go @@ -0,0 +1,20 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package decoder + +import ( + "github.com/hashicorp/hcl-lang/validator" + "github.com/hashicorp/terraform-ls/internal/features/stacks/decoder/validations" +) + +var stackValidators = []validator.Validator{ + validator.BlockLabelsLength{}, + validator.DeprecatedAttribute{}, + validator.DeprecatedBlock{}, + validator.MaxBlocks{}, + validator.MinBlocks{}, + validations.MissingRequiredAttribute{}, + validator.UnexpectedAttribute{}, + validator.UnexpectedBlock{}, +} diff --git a/internal/features/stacks/events.go b/internal/features/stacks/events.go index b7c955652..0e34bc395 100644 --- a/internal/features/stacks/events.go +++ b/internal/features/stacks/events.go @@ -8,6 +8,7 @@ import ( "os" "path/filepath" + lsctx "github.com/hashicorp/terraform-ls/internal/context" "github.com/hashicorp/terraform-ls/internal/document" "github.com/hashicorp/terraform-ls/internal/eventbus" "github.com/hashicorp/terraform-ls/internal/features/stacks/ast" @@ -191,6 +192,10 @@ func (f *StacksFeature) decodeStack(ctx context.Context, dir document.DirHandle, } ids = append(ids, parseId) + // this needs to be here because the setting context + // is not available in the validate job + validationOptions, _ := lsctx.ValidationOptions(ctx) + metaId, err := f.stateStore.JobStore.EnqueueJob(ctx, job.Job{ Dir: dir, Func: func(ctx context.Context) error { @@ -233,6 +238,22 @@ func (f *StacksFeature) decodeStack(ctx context.Context, dir document.DirHandle, } deferIds = append(deferIds, eSchemaId) + if validationOptions.EnableEnhancedValidation { + validationId, err := f.stateStore.JobStore.EnqueueJob(ctx, job.Job{ + Dir: dir, + Func: func(ctx context.Context) error { + return jobs.SchemaStackValidation(ctx, f.store, f.moduleFeature, dir.Path()) + }, + Type: operation.OpTypeSchemaStackValidation.String(), + DependsOn: deferIds, + IgnoreState: ignoreState, + }) + if err != nil { + return deferIds, err + } + deferIds = append(deferIds, validationId) + } + return deferIds, nil }, }) diff --git a/internal/features/stacks/jobs/validation.go b/internal/features/stacks/jobs/validation.go new file mode 100644 index 000000000..975226273 --- /dev/null +++ b/internal/features/stacks/jobs/validation.go @@ -0,0 +1,118 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package jobs + +import ( + "context" + "path" + + "github.com/hashicorp/hcl-lang/decoder" + "github.com/hashicorp/hcl-lang/lang" + "github.com/hashicorp/hcl/v2" + lsctx "github.com/hashicorp/terraform-ls/internal/context" + idecoder "github.com/hashicorp/terraform-ls/internal/decoder" + "github.com/hashicorp/terraform-ls/internal/document" + "github.com/hashicorp/terraform-ls/internal/features/stacks/ast" + stackDecoder "github.com/hashicorp/terraform-ls/internal/features/stacks/decoder" + "github.com/hashicorp/terraform-ls/internal/features/stacks/state" + "github.com/hashicorp/terraform-ls/internal/job" + ilsp "github.com/hashicorp/terraform-ls/internal/lsp" + globalAst "github.com/hashicorp/terraform-ls/internal/terraform/ast" + "github.com/hashicorp/terraform-ls/internal/terraform/module/operation" +) + +func SchemaStackValidation(ctx context.Context, stackStore *state.StackStore, moduleFeature stackDecoder.ModuleReader, stackPath string) error { + rpcContext := lsctx.DocumentContext(ctx) + + record, err := stackStore.StackRecordByPath(stackPath) + if err != nil { + return err + } + + // Avoid validation if it is already in progress or already finished + if record.DiagnosticsState[globalAst.SchemaValidationSource] != operation.OpStateUnknown && !job.IgnoreState(ctx) { + return job.StateNotChangedErr{Dir: document.DirHandleFromPath(stackPath)} + } + + err = stackStore.SetDiagnosticsState(stackPath, globalAst.SchemaValidationSource, operation.OpStateLoading) + if err != nil { + return err + } + + d := decoder.NewDecoder(&stackDecoder.PathReader{ + StateReader: stackStore, + ModuleReader: moduleFeature, + }) + d.SetContext(idecoder.DecoderContext(ctx)) + + var rErr error + if rpcContext.Method == "textDocument/didChange" { + // We validate only the file that has changed + // This means only creating a decoder for the file type that has changed + decoder, err := d.Path(lang.Path{ + Path: stackPath, + LanguageID: rpcContext.LanguageID, + }) + if err != nil { + return err + } + + filename := path.Base(rpcContext.URI) + + var fileDiags hcl.Diagnostics + fileDiags, rErr = decoder.ValidateFile(ctx, filename) + + diags, ok := record.Diagnostics[globalAst.SchemaValidationSource] + if !ok { + diags = make(ast.Diagnostics) + } + diags[ast.FilenameFromName(filename)] = fileDiags + + sErr := stackStore.UpdateDiagnostics(stackPath, globalAst.SchemaValidationSource, diags) + if sErr != nil { + return sErr + } + } else { + // We validate the whole stack, and so need to create decoders for + // all the file types in the stack + stackDecoder, err := d.Path(lang.Path{ + Path: stackPath, + LanguageID: ilsp.Stacks.String(), + }) + if err != nil { + return err + } + deployDecoder, err := d.Path(lang.Path{ + Path: stackPath, + LanguageID: ilsp.Deploy.String(), + }) + if err != nil { + return err + } + + diags := make(lang.DiagnosticsMap) + + stacksDiags, err := stackDecoder.Validate(ctx) + if err != nil { + // TODO: Should we really return here or continue with the other decoders? + // Is this really a complete fail case? Shouldn't a failure in a stack file + // not prevent the deploy file from being validated? + return err + } + diags = diags.Extend(stacksDiags) + + deployDiags, err := deployDecoder.Validate(ctx) + if err != nil { + return err + } + diags = diags.Extend(deployDiags) + + sErr := stackStore.UpdateDiagnostics(stackPath, globalAst.SchemaValidationSource, ast.DiagnosticsFromMap(diags)) + if sErr != nil { + return sErr + } + } + + return rErr +} diff --git a/internal/terraform/module/operation/op_type_string.go b/internal/terraform/module/operation/op_type_string.go index 705f0a961..b116ecb6d 100644 --- a/internal/terraform/module/operation/op_type_string.go +++ b/internal/terraform/module/operation/op_type_string.go @@ -24,17 +24,18 @@ func _() { _ = x[OpTypePreloadEmbeddedSchema-13] _ = x[OpTypeStacksPreloadEmbeddedSchema-14] _ = x[OpTypeSchemaModuleValidation-15] - _ = x[OpTypeSchemaVarsValidation-16] - _ = x[OpTypeReferenceValidation-17] - _ = x[OpTypeTerraformValidate-18] - _ = x[OpTypeParseStackConfiguration-19] - _ = x[OpTypeLoadStackMetadata-20] - _ = x[OpTypeLoadStackRequiredTerraformVersion-21] + _ = x[OpTypeSchemaStackValidation-16] + _ = x[OpTypeSchemaVarsValidation-17] + _ = x[OpTypeReferenceValidation-18] + _ = x[OpTypeTerraformValidate-19] + _ = x[OpTypeParseStackConfiguration-20] + _ = x[OpTypeLoadStackMetadata-21] + _ = x[OpTypeLoadStackRequiredTerraformVersion-22] } -const _OpType_name = "OpTypeUnknownOpTypeGetTerraformVersionOpTypeGetInstalledTerraformVersionOpTypeObtainSchemaOpTypeParseModuleConfigurationOpTypeParseVariablesOpTypeParseModuleManifestOpTypeLoadModuleMetadataOpTypeDecodeReferenceTargetsOpTypeDecodeReferenceOriginsOpTypeDecodeVarsReferencesOpTypeGetModuleDataFromRegistryOpTypeParseProviderVersionsOpTypePreloadEmbeddedSchemaOpTypeStacksPreloadEmbeddedSchemaOpTypeSchemaModuleValidationOpTypeSchemaVarsValidationOpTypeReferenceValidationOpTypeTerraformValidateOpTypeParseStackConfigurationOpTypeLoadStackMetadataOpTypeLoadStackRequiredTerraformVersion" +const _OpType_name = "OpTypeUnknownOpTypeGetTerraformVersionOpTypeGetInstalledTerraformVersionOpTypeObtainSchemaOpTypeParseModuleConfigurationOpTypeParseVariablesOpTypeParseModuleManifestOpTypeLoadModuleMetadataOpTypeDecodeReferenceTargetsOpTypeDecodeReferenceOriginsOpTypeDecodeVarsReferencesOpTypeGetModuleDataFromRegistryOpTypeParseProviderVersionsOpTypePreloadEmbeddedSchemaOpTypeStacksPreloadEmbeddedSchemaOpTypeSchemaModuleValidationOpTypeSchemaStackValidationOpTypeSchemaVarsValidationOpTypeReferenceValidationOpTypeTerraformValidateOpTypeParseStackConfigurationOpTypeLoadStackMetadataOpTypeLoadStackRequiredTerraformVersion" -var _OpType_index = [...]uint16{0, 13, 38, 72, 90, 120, 140, 165, 189, 217, 245, 271, 302, 329, 356, 389, 417, 443, 468, 491, 520, 543, 582} +var _OpType_index = [...]uint16{0, 13, 38, 72, 90, 120, 140, 165, 189, 217, 245, 271, 302, 329, 356, 389, 417, 444, 470, 495, 518, 547, 570, 609} func (i OpType) String() string { if i >= OpType(len(_OpType_index)-1) { diff --git a/internal/terraform/module/operation/operation.go b/internal/terraform/module/operation/operation.go index 1043cdc93..a4f1ed81b 100644 --- a/internal/terraform/module/operation/operation.go +++ b/internal/terraform/module/operation/operation.go @@ -33,6 +33,7 @@ const ( OpTypePreloadEmbeddedSchema OpTypeStacksPreloadEmbeddedSchema OpTypeSchemaModuleValidation + OpTypeSchemaStackValidation OpTypeSchemaVarsValidation OpTypeReferenceValidation OpTypeTerraformValidate From d43801465c6d7a9f9beb9d870a91b008fb226daa Mon Sep 17 00:00:00 2001 From: James Pogran Date: Fri, 2 Aug 2024 14:21:58 -0400 Subject: [PATCH 2/2] Add validation for stack block names TODO: Revist checking Stack concepts in the static validators Other validators are more generic and can be applied to any block/attribute/etc, but this one is specific to the stack block types. I'm sure this could be done in a more generic way, but I'm not sure if it's worth it at the moment. The names of these blocks are used as identifiers in the stack and are used in the UI, so they have to be valid Terraform identifiers or else the stack will not be able to be created. This is a very basic check to ensure that the names are valid until we can do more advanced checks or use the cloud api. I tried adding this to the earlydecoder, but we do not seem to report the diagnostics there at all currently, so nothing was being reported. I'm not sure if that's a bug or intended. --- .../stacks/decoder/validations/valid_name.go | 71 +++++++++++++++++++ .../features/stacks/decoder/validators.go | 3 +- 2 files changed, 73 insertions(+), 1 deletion(-) create mode 100644 internal/features/stacks/decoder/validations/valid_name.go diff --git a/internal/features/stacks/decoder/validations/valid_name.go b/internal/features/stacks/decoder/validations/valid_name.go new file mode 100644 index 000000000..97fe31d40 --- /dev/null +++ b/internal/features/stacks/decoder/validations/valid_name.go @@ -0,0 +1,71 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package validations + +import ( + "context" + "fmt" + + "github.com/hashicorp/hcl-lang/schema" + "github.com/hashicorp/hcl/v2" + "github.com/hashicorp/hcl/v2/hclsyntax" +) + +type StackBlockValidName struct{} + +func (sb StackBlockValidName) Visit(ctx context.Context, node hclsyntax.Node, nodeSchema schema.Schema) (context.Context, hcl.Diagnostics) { + var diags hcl.Diagnostics + + block, ok := node.(*hclsyntax.Block) + if !ok { + return ctx, diags + } + + if nodeSchema == nil { + return ctx, diags + } + + // TODO: Revist checking Stack concepts in the static validators + // Other validators are more generic and can be applied to any block/attribute/etc, but + // this one is specific to the stack block types. I'm sure this could be done in a more generic way, + // but I'm not sure if it's worth it at the moment. The names of these blocks are used as identifiers + // in the stack and are used in the UI, so they have to be valid Terraform identifiers or else the stack + // will not be able to be created. This is a very basic check to ensure that the names are valid until we can + // do more advanced checks or use the cloud api. + // I tried adding this to the earlydecoder, but we do not seem to report the diagnostics there at all currently, + // so nothing was being reported. I'm not sure if that's a bug or intended. + + switch block.Type { + // stack + case "component": + diags = hasValidNameLabel(block, diags) + // case "provider": + // case "required_providers": + case "variable": + diags = hasValidNameLabel(block, diags) + case "output": + diags = hasValidNameLabel(block, diags) + // deployment + case "deployment": + diags = hasValidNameLabel(block, diags) + case "identity_token": + diags = hasValidNameLabel(block, diags) + case "orchestrate": + diags = hasValidNameLabel(block, diags) + } + + return ctx, diags +} + +func hasValidNameLabel(block *hclsyntax.Block, diags hcl.Diagnostics) hcl.Diagnostics { + if !hclsyntax.ValidIdentifier(block.Labels[0]) { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: fmt.Sprintf("Invalid %q name %q", block.Type, block.Labels[0]), + Detail: "Names must be valid identifiers: beginning with a letter or underscore, followed by zero or more letters, digits, or underscores.", + Subject: block.LabelRanges[0].Ptr(), + }) + } + return diags +} diff --git a/internal/features/stacks/decoder/validators.go b/internal/features/stacks/decoder/validators.go index 9573bbbe9..413647581 100644 --- a/internal/features/stacks/decoder/validators.go +++ b/internal/features/stacks/decoder/validators.go @@ -14,7 +14,8 @@ var stackValidators = []validator.Validator{ validator.DeprecatedBlock{}, validator.MaxBlocks{}, validator.MinBlocks{}, - validations.MissingRequiredAttribute{}, validator.UnexpectedAttribute{}, validator.UnexpectedBlock{}, + validations.MissingRequiredAttribute{}, + validations.StackBlockValidName{}, }