From 81b0c4b28d41bee8c95300f7f6e9f4962f392431 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Tue, 2 May 2017 15:42:34 -0700 Subject: [PATCH] config: generate errors for unnamed blocks of various sources We've been incorrectly validating (or not validating at all) the requirement that certain blocks be followed by a name string, to prohibit e.g. this: variable {} and: variable = "" Before this change we were catching this for most constructs only if there were no _valid_ blocks of the same name in the same file. For modules in particular, we were not catching this at all. Now we detect this for all kinds of block (resources had a pre-existing check, so aren't touched here) and produce a different error message depending on which of the above incorrect forms are used. This fixes #13575. --- config/loader_hcl.go | 55 ++++++++++++++++++++---- config/loader_test.go | 16 ++++++- config/test-fixtures/module-unnamed.tf | 7 +++ config/test-fixtures/output-unnamed.tf | 4 ++ config/test-fixtures/variable-no-name.tf | 3 ++ 5 files changed, 75 insertions(+), 10 deletions(-) create mode 100644 config/test-fixtures/module-unnamed.tf diff --git a/config/loader_hcl.go b/config/loader_hcl.go index a40ad5ba7..9abb1960f 100644 --- a/config/loader_hcl.go +++ b/config/loader_hcl.go @@ -327,6 +327,10 @@ func loadAtlasHcl(list *ast.ObjectList) (*AtlasConfig, error) { // represents exactly one module definition in the HCL configuration. // We leave it up to another pass to merge them together. func loadModulesHcl(list *ast.ObjectList) ([]*Module, error) { + if err := assertAllBlocksHaveNames("module", list); err != nil { + return nil, err + } + list = list.Children() if len(list.Items) == 0 { return nil, nil @@ -391,12 +395,12 @@ func loadModulesHcl(list *ast.ObjectList) ([]*Module, error) { // LoadOutputsHcl recurses into the given HCL object and turns // it into a mapping of outputs. func loadOutputsHcl(list *ast.ObjectList) ([]*Output, error) { - list = list.Children() - if len(list.Items) == 0 { - return nil, fmt.Errorf( - "'output' must be followed by exactly one string: a name") + if err := assertAllBlocksHaveNames("output", list); err != nil { + return nil, err } + list = list.Children() + // Go through each object and turn it into an actual result. result := make([]*Output, 0, len(list.Items)) for _, item := range list.Items { @@ -450,12 +454,12 @@ func loadOutputsHcl(list *ast.ObjectList) ([]*Output, error) { // LoadVariablesHcl recurses into the given HCL object and turns // it into a list of variables. func loadVariablesHcl(list *ast.ObjectList) ([]*Variable, error) { - list = list.Children() - if len(list.Items) == 0 { - return nil, fmt.Errorf( - "'variable' must be followed by exactly one strings: a name") + if err := assertAllBlocksHaveNames("variable", list); err != nil { + return nil, err } + list = list.Children() + // hclVariable is the structure each variable is decoded into type hclVariable struct { DeclaredType string `hcl:"type"` @@ -531,6 +535,10 @@ func loadVariablesHcl(list *ast.ObjectList) ([]*Variable, error) { // LoadProvidersHcl recurses into the given HCL object and turns // it into a mapping of provider configs. func loadProvidersHcl(list *ast.ObjectList) ([]*ProviderConfig, error) { + if err := assertAllBlocksHaveNames("provider", list); err != nil { + return nil, err + } + list = list.Children() if len(list.Items) == 0 { return nil, nil @@ -592,6 +600,10 @@ func loadProvidersHcl(list *ast.ObjectList) ([]*ProviderConfig, error) { // represents exactly one data definition in the HCL configuration. // We leave it up to another pass to merge them together. func loadDataResourcesHcl(list *ast.ObjectList) ([]*Resource, error) { + if err := assertAllBlocksHaveNames("data", list); err != nil { + return nil, err + } + list = list.Children() if len(list.Items) == 0 { return nil, nil @@ -901,6 +913,10 @@ func loadManagedResourcesHcl(list *ast.ObjectList) ([]*Resource, error) { } func loadProvisionersHcl(list *ast.ObjectList, connInfo map[string]interface{}) ([]*Provisioner, error) { + if err := assertAllBlocksHaveNames("provisioner", list); err != nil { + return nil, err + } + list = list.Children() if len(list.Items) == 0 { return nil, nil @@ -1023,6 +1039,29 @@ func hclObjectMap(os *hclobj.Object) map[string]ast.ListNode { } */ +// assertAllBlocksHaveNames returns an error if any of the items in +// the given object list are blocks without keys (like "module {}") +// or simple assignments (like "module = 1"). It returns nil if +// neither of these things are true. +// +// The given name is used in any generated error messages, and should +// be the name of the block we're dealing with. The given list should +// be the result of calling .Filter on an object list with that same +// name. +func assertAllBlocksHaveNames(name string, list *ast.ObjectList) error { + if elem := list.Elem(); len(elem.Items) != 0 { + switch et := elem.Items[0].Val.(type) { + case *ast.ObjectType: + pos := et.Lbrace + return fmt.Errorf("%s: %q must be followed by a name", pos, name) + default: + pos := elem.Items[0].Val.Pos() + return fmt.Errorf("%s: %q must be a configuration block", pos, name) + } + } + return nil +} + func checkHCLKeys(node ast.Node, valid []string) error { var list *ast.ObjectList switch n := node.(type) { diff --git a/config/loader_test.go b/config/loader_test.go index a2a2929f2..a3aeb7321 100644 --- a/config/loader_test.go +++ b/config/loader_test.go @@ -314,6 +314,18 @@ func TestLoadFileBasic_modules(t *testing.T) { } } +func TestLoadFile_unnamedModule(t *testing.T) { + _, err := LoadFile(filepath.Join(fixtureDir, "module-unnamed.tf")) + if err == nil { + t.Fatalf("bad: expected error") + } + + errorStr := err.Error() + if !strings.Contains(errorStr, `"module" must be followed`) { + t.Fatalf("bad: expected error has wrong text: %s", errorStr) + } +} + func TestLoadFile_outputDependsOn(t *testing.T) { c, err := LoadFile(filepath.Join(fixtureDir, "output-depends-on.tf")) if err != nil { @@ -696,7 +708,7 @@ func TestLoadFile_variableNoName(t *testing.T) { } errorStr := err.Error() - if !strings.Contains(errorStr, "'variable' must be followed") { + if !strings.Contains(errorStr, `"variable" must be followed`) { t.Fatalf("bad: expected error has wrong text: %s", errorStr) } } @@ -740,7 +752,7 @@ func TestLoadFile_unnamedOutput(t *testing.T) { } errorStr := err.Error() - if !strings.Contains(errorStr, "'output' must be followed") { + if !strings.Contains(errorStr, `"output" must be followed`) { t.Fatalf("bad: expected error has wrong text: %s", errorStr) } } diff --git a/config/test-fixtures/module-unnamed.tf b/config/test-fixtures/module-unnamed.tf new file mode 100644 index 000000000..e285519bf --- /dev/null +++ b/config/test-fixtures/module-unnamed.tf @@ -0,0 +1,7 @@ +module "okay" { + source = "./okay" +} + +module { + source = "./not-okay" +} diff --git a/config/test-fixtures/output-unnamed.tf b/config/test-fixtures/output-unnamed.tf index 7e7529153..7ef8ebe1b 100644 --- a/config/test-fixtures/output-unnamed.tf +++ b/config/test-fixtures/output-unnamed.tf @@ -1,3 +1,7 @@ +output "okay" { + value = "bar" +} + output { value = "foo" } diff --git a/config/test-fixtures/variable-no-name.tf b/config/test-fixtures/variable-no-name.tf index f3856886f..7f09d1e64 100644 --- a/config/test-fixtures/variable-no-name.tf +++ b/config/test-fixtures/variable-no-name.tf @@ -1,3 +1,6 @@ +variable "okay" { +} + variable { name = "test" default = "test_value"