From dbbfae5a1cbd5a1a7bd5526b8ce06fece9ec9381 Mon Sep 17 00:00:00 2001 From: Brandon Croft Date: Wed, 6 Oct 2021 16:10:25 -0600 Subject: [PATCH] refactor ParseVariableValues into separate operations 1. ParseDeclaredValues: parses unparsed variables into terraform.InputValues 2. ProbeUndeclaredVariableValues: compares variable declarations with unparsed values to warn/error about undeclared variables --- internal/backend/unparsed_value.go | 193 +++++++++++++--------- internal/backend/unparsed_value_test.go | 206 +++++++++++++++++------- 2 files changed, 271 insertions(+), 128 deletions(-) diff --git a/internal/backend/unparsed_value.go b/internal/backend/unparsed_value.go index abd16ef9e..91c982582 100644 --- a/internal/backend/unparsed_value.go +++ b/internal/backend/unparsed_value.go @@ -25,6 +25,121 @@ type UnparsedVariableValue interface { ParseVariableValue(mode configs.VariableParsingMode) (*terraform.InputValue, tfdiags.Diagnostics) } +// ParseUndeclaredVariableValues processes a map of unparsed variable values +// and returns an input values map of the ones not declared in the specified +// declaration map along with detailed diagnostics about values of undeclared +// variables being present, depending on the source of these values. If more +// than two undeclared values are present in file form (config, auto, -var-file) +// the remaining errors are summarized to avoid a massive list of errors. +func ParseUndeclaredVariableValues(vv map[string]UnparsedVariableValue, decls map[string]*configs.Variable) (terraform.InputValues, tfdiags.Diagnostics) { + var diags tfdiags.Diagnostics + ret := make(terraform.InputValues, len(vv)) + seenUndeclaredInFile := 0 + + for name, rv := range vv { + if _, declared := decls[name]; declared { + // Only interested in parsing undeclared variables + continue + } + + val, valDiags := rv.ParseVariableValue(configs.VariableParseLiteral) + if valDiags.HasErrors() { + continue + } + + ret[name] = val + + switch val.SourceType { + case terraform.ValueFromConfig, terraform.ValueFromAutoFile, terraform.ValueFromNamedFile: + // We allow undeclared names for variable values from files and warn in case + // users have forgotten a variable {} declaration or have a typo in their var name. + // Some users will actively ignore this warning because they use a .tfvars file + // across multiple configurations. + if seenUndeclaredInFile < 2 { + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Warning, + "Value for undeclared variable", + fmt.Sprintf("The root module does not declare a variable named %q but a value was found in file %q. If you meant to use this value, add a \"variable\" block to the configuration.\n\nTo silence these warnings, use TF_VAR_... environment variables to provide certain \"global\" settings to all configurations in your organization. To reduce the verbosity of these warnings, use the -compact-warnings option.", name, val.SourceRange.Filename), + )) + } + seenUndeclaredInFile++ + + case terraform.ValueFromEnvVar: + // We allow and ignore undeclared names for environment + // variables, because users will often set these globally + // when they are used across many (but not necessarily all) + // configurations. + case terraform.ValueFromCLIArg: + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Value for undeclared variable", + fmt.Sprintf("A variable named %q was assigned on the command line, but the root module does not declare a variable of that name. To use this value, add a \"variable\" block to the configuration.", name), + )) + default: + // For all other source types we are more vague, but other situations + // don't generally crop up at this layer in practice. + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Value for undeclared variable", + fmt.Sprintf("A variable named %q was assigned a value, but the root module does not declare a variable of that name. To use this value, add a \"variable\" block to the configuration.", name), + )) + } + } + + if seenUndeclaredInFile > 2 { + extras := seenUndeclaredInFile - 2 + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagWarning, + Summary: "Values for undeclared variables", + Detail: fmt.Sprintf("In addition to the other similar warnings shown, %d other variable(s) defined without being declared.", extras), + }) + } + + return ret, diags +} + +// ParseDeclaredVariableValues processes a map of unparsed variable values +// and returns an input values map of the ones declared in the specified +// variable declaration mapping. Diagnostics will be populating with +// any variable parsing errors encountered within this collection. +func ParseDeclaredVariableValues(vv map[string]UnparsedVariableValue, decls map[string]*configs.Variable) (terraform.InputValues, tfdiags.Diagnostics) { + var diags tfdiags.Diagnostics + ret := make(terraform.InputValues, len(vv)) + + for name, rv := range vv { + var mode configs.VariableParsingMode + config, declared := decls[name] + + if declared { + mode = config.ParsingMode + } else { + // Only interested in parsing declared variables + continue + } + + val, valDiags := rv.ParseVariableValue(mode) + diags = diags.Append(valDiags) + if valDiags.HasErrors() { + continue + } + + ret[name] = val + } + + return ret, diags +} + +// Checks all given terraform.InputValues variable maps for the existance of +// a named variable +func isDefinedAny(name string, maps ...terraform.InputValues) bool { + for _, m := range maps { + if _, defined := m[name]; defined { + return true + } + } + return false +} + // ParseVariableValues processes a map of unparsed variable values by // correlating each one with the given variable declarations which should // be from a root module. @@ -42,87 +157,17 @@ type UnparsedVariableValue interface { // that were successfully processed, allowing for careful analysis of the // partial result. func ParseVariableValues(vv map[string]UnparsedVariableValue, decls map[string]*configs.Variable) (terraform.InputValues, tfdiags.Diagnostics) { - var diags tfdiags.Diagnostics - ret := make(terraform.InputValues, len(vv)) + ret, diags := ParseDeclaredVariableValues(vv, decls) + undeclared, diagsUndeclared := ParseUndeclaredVariableValues(vv, decls) - // Currently we're generating only warnings for undeclared variables - // defined in files (see below) but we only want to generate a few warnings - // at a time because existing deployments may have lots of these and - // the result can therefore be overwhelming. - seenUndeclaredInFile := 0 - - for name, rv := range vv { - var mode configs.VariableParsingMode - config, declared := decls[name] - if declared { - mode = config.ParsingMode - } else { - mode = configs.VariableParseLiteral - } - - val, valDiags := rv.ParseVariableValue(mode) - diags = diags.Append(valDiags) - if valDiags.HasErrors() { - continue - } - - if !declared { - switch val.SourceType { - case terraform.ValueFromConfig, terraform.ValueFromAutoFile, terraform.ValueFromNamedFile: - // We allow undeclared names for variable values from files and warn in case - // users have forgotten a variable {} declaration or have a typo in their var name. - // Some users will actively ignore this warning because they use a .tfvars file - // across multiple configurations. - if seenUndeclaredInFile < 2 { - diags = diags.Append(tfdiags.Sourceless( - tfdiags.Warning, - "Value for undeclared variable", - fmt.Sprintf("The root module does not declare a variable named %q but a value was found in file %q. If you meant to use this value, add a \"variable\" block to the configuration.\n\nTo silence these warnings, use TF_VAR_... environment variables to provide certain \"global\" settings to all configurations in your organization. To reduce the verbosity of these warnings, use the -compact-warnings option.", name, val.SourceRange.Filename), - )) - } - seenUndeclaredInFile++ - - case terraform.ValueFromEnvVar: - // We allow and ignore undeclared names for environment - // variables, because users will often set these globally - // when they are used across many (but not necessarily all) - // configurations. - case terraform.ValueFromCLIArg: - diags = diags.Append(tfdiags.Sourceless( - tfdiags.Error, - "Value for undeclared variable", - fmt.Sprintf("A variable named %q was assigned on the command line, but the root module does not declare a variable of that name. To use this value, add a \"variable\" block to the configuration.", name), - )) - default: - // For all other source types we are more vague, but other situations - // don't generally crop up at this layer in practice. - diags = diags.Append(tfdiags.Sourceless( - tfdiags.Error, - "Value for undeclared variable", - fmt.Sprintf("A variable named %q was assigned a value, but the root module does not declare a variable of that name. To use this value, add a \"variable\" block to the configuration.", name), - )) - } - continue - } - - ret[name] = val - } - - if seenUndeclaredInFile > 2 { - extras := seenUndeclaredInFile - 2 - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagWarning, - Summary: "Values for undeclared variables", - Detail: fmt.Sprintf("In addition to the other similar warnings shown, %d other variable(s) defined without being declared.", extras), - }) - } + diags = diags.Append(diagsUndeclared) // By this point we should've gathered all of the required root module // variables from one of the many possible sources. We'll now populate // any we haven't gathered as their defaults and fail if any of the // missing ones are required. for name, vc := range decls { - if _, defined := ret[name]; defined { + if isDefinedAny(name, ret, undeclared) { continue } diff --git a/internal/backend/unparsed_value_test.go b/internal/backend/unparsed_value_test.go index 6df7c226a..981c84a43 100644 --- a/internal/backend/unparsed_value_test.go +++ b/internal/backend/unparsed_value_test.go @@ -13,7 +13,7 @@ import ( "github.com/hashicorp/terraform/internal/tfdiags" ) -func TestParseVariableValuesUndeclared(t *testing.T) { +func TestUnparsedValue(t *testing.T) { vv := map[string]UnparsedVariableValue{ "undeclared0": testUnparsedVariableValue("0"), "undeclared1": testUnparsedVariableValue("1"), @@ -59,66 +59,164 @@ func TestParseVariableValuesUndeclared(t *testing.T) { }, } - gotVals, diags := ParseVariableValues(vv, decls) - for _, diag := range diags { - t.Logf("%s: %s", diag.Description().Summary, diag.Description().Detail) - } - if got, want := len(diags), 4; got != want { - t.Fatalf("wrong number of diagnostics %d; want %d", got, want) - } - const undeclSingular = `Value for undeclared variable` const undeclPlural = `Values for undeclared variables` - const missingRequired = `No value for required variable` - if got, want := diags[0].Description().Summary, undeclSingular; got != want { - t.Errorf("wrong summary for diagnostic 0\ngot: %s\nwant: %s", got, want) - } - if got, want := diags[1].Description().Summary, undeclSingular; got != want { - t.Errorf("wrong summary for diagnostic 1\ngot: %s\nwant: %s", got, want) - } - if got, want := diags[2].Description().Summary, undeclPlural; got != want { - t.Errorf("wrong summary for diagnostic 2\ngot: %s\nwant: %s", got, want) - } - if got, want := diags[2].Description().Detail, "3 other variable(s)"; !strings.Contains(got, want) { - t.Errorf("wrong detail for diagnostic 2\ngot: %s\nmust contain: %s", got, want) - } - if got, want := diags[3].Description().Summary, missingRequired; got != want { - t.Errorf("wrong summary for diagnostic 3\ngot: %s\nwant: %s", got, want) - } + t.Run("ParseDeclaredVariableValues", func(t *testing.T) { + gotVals, diags := ParseDeclaredVariableValues(vv, decls) - wantVals := terraform.InputValues{ - "declared1": { - Value: cty.StringVal("5"), - SourceType: terraform.ValueFromNamedFile, - SourceRange: tfdiags.SourceRange{ - Filename: "fake.tfvars", - Start: tfdiags.SourcePos{Line: 1, Column: 1, Byte: 0}, - End: tfdiags.SourcePos{Line: 1, Column: 1, Byte: 0}, + if got, want := len(diags), 0; got != want { + t.Fatalf("wrong number of diagnostics %d; want %d", got, want) + } + + wantVals := terraform.InputValues{ + "declared1": { + Value: cty.StringVal("5"), + SourceType: terraform.ValueFromNamedFile, + SourceRange: tfdiags.SourceRange{ + Filename: "fake.tfvars", + Start: tfdiags.SourcePos{Line: 1, Column: 1, Byte: 0}, + End: tfdiags.SourcePos{Line: 1, Column: 1, Byte: 0}, + }, }, - }, - "missing1": { - Value: cty.DynamicVal, - SourceType: terraform.ValueFromConfig, - SourceRange: tfdiags.SourceRange{ - Filename: "fake.tf", - Start: tfdiags.SourcePos{Line: 3, Column: 1, Byte: 0}, - End: tfdiags.SourcePos{Line: 3, Column: 1, Byte: 0}, + } + + if diff := cmp.Diff(wantVals, gotVals, cmp.Comparer(cty.Value.RawEquals)); diff != "" { + t.Errorf("wrong result\n%s", diff) + } + }) + + t.Run("ParseUndeclaredVariableValues", func(t *testing.T) { + gotVals, diags := ParseUndeclaredVariableValues(vv, decls) + + if got, want := len(diags), 3; got != want { + t.Fatalf("wrong number of diagnostics %d; want %d", got, want) + } + + if got, want := diags[0].Description().Summary, undeclSingular; got != want { + t.Errorf("wrong summary for diagnostic 0\ngot: %s\nwant: %s", got, want) + } + + if got, want := diags[1].Description().Summary, undeclSingular; got != want { + t.Errorf("wrong summary for diagnostic 1\ngot: %s\nwant: %s", got, want) + } + + if got, want := diags[2].Description().Summary, undeclPlural; got != want { + t.Errorf("wrong summary for diagnostic 2\ngot: %s\nwant: %s", got, want) + } + + wantVals := terraform.InputValues{ + "undeclared0": { + Value: cty.StringVal("0"), + SourceType: terraform.ValueFromNamedFile, + SourceRange: tfdiags.SourceRange{ + Filename: "fake.tfvars", + Start: tfdiags.SourcePos{Line: 1, Column: 1}, + End: tfdiags.SourcePos{Line: 1, Column: 1}, + }, }, - }, - "missing2": { - Value: cty.StringVal("default for missing2"), - SourceType: terraform.ValueFromConfig, - SourceRange: tfdiags.SourceRange{ - Filename: "fake.tf", - Start: tfdiags.SourcePos{Line: 4, Column: 1, Byte: 0}, - End: tfdiags.SourcePos{Line: 4, Column: 1, Byte: 0}, + "undeclared1": { + Value: cty.StringVal("1"), + SourceType: terraform.ValueFromNamedFile, + SourceRange: tfdiags.SourceRange{ + Filename: "fake.tfvars", + Start: tfdiags.SourcePos{Line: 1, Column: 1}, + End: tfdiags.SourcePos{Line: 1, Column: 1}, + }, }, - }, - } - if diff := cmp.Diff(wantVals, gotVals, cmp.Comparer(cty.Value.RawEquals)); diff != "" { - t.Errorf("wrong result\n%s", diff) - } + "undeclared2": { + Value: cty.StringVal("2"), + SourceType: terraform.ValueFromNamedFile, + SourceRange: tfdiags.SourceRange{ + Filename: "fake.tfvars", + Start: tfdiags.SourcePos{Line: 1, Column: 1}, + End: tfdiags.SourcePos{Line: 1, Column: 1}, + }, + }, + "undeclared3": { + Value: cty.StringVal("3"), + SourceType: terraform.ValueFromNamedFile, + SourceRange: tfdiags.SourceRange{ + Filename: "fake.tfvars", + Start: tfdiags.SourcePos{Line: 1, Column: 1}, + End: tfdiags.SourcePos{Line: 1, Column: 1}, + }, + }, + "undeclared4": { + Value: cty.StringVal("4"), + SourceType: terraform.ValueFromNamedFile, + SourceRange: tfdiags.SourceRange{ + Filename: "fake.tfvars", + Start: tfdiags.SourcePos{Line: 1, Column: 1}, + End: tfdiags.SourcePos{Line: 1, Column: 1}, + }, + }, + } + if diff := cmp.Diff(wantVals, gotVals, cmp.Comparer(cty.Value.RawEquals)); diff != "" { + t.Errorf("wrong result\n%s", diff) + } + }) + + t.Run("ParseVariableValues", func(t *testing.T) { + gotVals, diags := ParseVariableValues(vv, decls) + for _, diag := range diags { + t.Logf("%s: %s", diag.Description().Summary, diag.Description().Detail) + } + if got, want := len(diags), 4; got != want { + t.Fatalf("wrong number of diagnostics %d; want %d", got, want) + } + + const missingRequired = `No value for required variable` + + if got, want := diags[0].Description().Summary, undeclSingular; got != want { + t.Errorf("wrong summary for diagnostic 0\ngot: %s\nwant: %s", got, want) + } + if got, want := diags[1].Description().Summary, undeclSingular; got != want { + t.Errorf("wrong summary for diagnostic 1\ngot: %s\nwant: %s", got, want) + } + if got, want := diags[2].Description().Summary, undeclPlural; got != want { + t.Errorf("wrong summary for diagnostic 2\ngot: %s\nwant: %s", got, want) + } + if got, want := diags[2].Description().Detail, "3 other variable(s)"; !strings.Contains(got, want) { + t.Errorf("wrong detail for diagnostic 2\ngot: %s\nmust contain: %s", got, want) + } + if got, want := diags[3].Description().Summary, missingRequired; got != want { + t.Errorf("wrong summary for diagnostic 3\ngot: %s\nwant: %s", got, want) + } + + wantVals := terraform.InputValues{ + "declared1": { + Value: cty.StringVal("5"), + SourceType: terraform.ValueFromNamedFile, + SourceRange: tfdiags.SourceRange{ + Filename: "fake.tfvars", + Start: tfdiags.SourcePos{Line: 1, Column: 1, Byte: 0}, + End: tfdiags.SourcePos{Line: 1, Column: 1, Byte: 0}, + }, + }, + "missing1": { + Value: cty.DynamicVal, + SourceType: terraform.ValueFromConfig, + SourceRange: tfdiags.SourceRange{ + Filename: "fake.tf", + Start: tfdiags.SourcePos{Line: 3, Column: 1, Byte: 0}, + End: tfdiags.SourcePos{Line: 3, Column: 1, Byte: 0}, + }, + }, + "missing2": { + Value: cty.StringVal("default for missing2"), + SourceType: terraform.ValueFromConfig, + SourceRange: tfdiags.SourceRange{ + Filename: "fake.tf", + Start: tfdiags.SourcePos{Line: 4, Column: 1, Byte: 0}, + End: tfdiags.SourcePos{Line: 4, Column: 1, Byte: 0}, + }, + }, + } + if diff := cmp.Diff(wantVals, gotVals, cmp.Comparer(cty.Value.RawEquals)); diff != "" { + t.Errorf("wrong result\n%s", diff) + } + }) } type testUnparsedVariableValue string