From 0681935df5abbb18cc0a3b58293770f0e1b9c514 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Tue, 20 Nov 2018 11:53:45 -0800 Subject: [PATCH] configs: Reserve various names for future use We want the forthcoming v0.12.0 release to be the last significant breaking change to our main configuration constructs for a long time, but not everything could be implemented in that release. As a compromise then, we reserve various names we have some intent of using in a future release so that such future uses will not be a further breaking change later. Some of these names are associated with specific short-term plans, while others are reserved conservatively for possible later work and may be "un-reserved" in a later release if we don't end up using them. The ones that we expect to use in the near future were already being handled, so we'll continue to decode them at the config layer but also produce an error so that we don't get weird behavior downstream where the corresponding features don't work yet. --- configs/module_call.go | 40 ++++++++++ configs/module_call_test.go | 14 ++-- configs/named_values.go | 10 +++ configs/parser_test.go | 30 ++++++++ configs/provider.go | 33 ++++++++ configs/provider_test.go | 26 +++++++ configs/provisioner.go | 23 +++--- configs/resource.go | 76 ++++++++++++------- .../invalid-files/data-reserved-for_each.tf | 3 + .../invalid-files/data-reserved-lifecycle.tf | 3 + .../invalid-files/data-reserved-locals.tf | 3 + .../module-calls.tf | 0 .../invalid-files/provider-reserved.tf | 16 ++++ .../resource-reserved-for_each.tf | 3 + .../invalid-files/resource-reserved-locals.tf | 3 + 15 files changed, 235 insertions(+), 48 deletions(-) create mode 100644 configs/provider_test.go create mode 100644 configs/test-fixtures/invalid-files/data-reserved-for_each.tf create mode 100644 configs/test-fixtures/invalid-files/data-reserved-lifecycle.tf create mode 100644 configs/test-fixtures/invalid-files/data-reserved-locals.tf rename configs/test-fixtures/{valid-files => invalid-files}/module-calls.tf (100%) create mode 100644 configs/test-fixtures/invalid-files/provider-reserved.tf create mode 100644 configs/test-fixtures/invalid-files/resource-reserved-for_each.tf create mode 100644 configs/test-fixtures/invalid-files/resource-reserved-locals.tf diff --git a/configs/module_call.go b/configs/module_call.go index 94bd1181e1..8c3ba67ce6 100644 --- a/configs/module_call.go +++ b/configs/module_call.go @@ -68,16 +68,40 @@ func decodeModuleBlock(block *hcl.Block, override bool) (*ModuleCall, hcl.Diagno if attr, exists := content.Attributes["count"]; exists { mc.Count = attr.Expr + + // We currently parse this, but don't yet do anything with it. + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Reserved argument name in module block", + Detail: fmt.Sprintf("The name %q is reserved for use in a future version of Terraform.", attr.Name), + Subject: &attr.NameRange, + }) } if attr, exists := content.Attributes["for_each"]; exists { mc.ForEach = attr.Expr + + // We currently parse this, but don't yet do anything with it. + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Reserved argument name in module block", + Detail: fmt.Sprintf("The name %q is reserved for use in a future version of Terraform.", attr.Name), + Subject: &attr.NameRange, + }) } if attr, exists := content.Attributes["depends_on"]; exists { deps, depsDiags := decodeDependsOn(attr) diags = append(diags, depsDiags...) mc.DependsOn = append(mc.DependsOn, deps...) + + // We currently parse this, but don't yet do anything with it. + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Reserved argument name in module block", + Detail: fmt.Sprintf("The name %q is reserved for use in a future version of Terraform.", attr.Name), + Subject: &attr.NameRange, + }) } if attr, exists := content.Attributes["providers"]; exists { @@ -113,6 +137,16 @@ func decodeModuleBlock(block *hcl.Block, override bool) (*ModuleCall, hcl.Diagno } } + // Reserved block types (all of them) + for _, block := range content.Blocks { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Reserved block type name in module block", + Detail: fmt.Sprintf("The block type name %q is reserved for use by Terraform in a future version.", block.Type), + Subject: &block.TypeRange, + }) + } + return mc, diags } @@ -145,4 +179,10 @@ var moduleBlockSchema = &hcl.BodySchema{ Name: "providers", }, }, + Blocks: []hcl.BlockHeaderSchema{ + // These are all reserved for future use. + {Type: "lifecycle"}, + {Type: "locals"}, + {Type: "provider", LabelNames: []string{"type"}}, + }, } diff --git a/configs/module_call_test.go b/configs/module_call_test.go index d86ddcc068..2bc425ad95 100644 --- a/configs/module_call_test.go +++ b/configs/module_call_test.go @@ -9,7 +9,7 @@ import ( ) func TestLoadModuleCall(t *testing.T) { - src, err := ioutil.ReadFile("test-fixtures/valid-files/module-calls.tf") + src, err := ioutil.ReadFile("test-fixtures/invalid-files/module-calls.tf") if err != nil { t.Fatal(err) } @@ -19,13 +19,11 @@ func TestLoadModuleCall(t *testing.T) { }) file, diags := parser.LoadConfigFile("module-calls.tf") - if len(diags) != 0 { - t.Errorf("Wrong number of diagnostics %d; want 0", len(diags)) - for _, diag := range diags { - t.Logf("- %s", diag) - } - return - } + assertExactDiagnostics(t, diags, []string{ + `module-calls.tf:19,3-8: Reserved argument name in module block; The name "count" is reserved for use in a future version of Terraform.`, + `module-calls.tf:20,3-11: Reserved argument name in module block; The name "for_each" is reserved for use in a future version of Terraform.`, + `module-calls.tf:22,3-13: Reserved argument name in module block; The name "depends_on" is reserved for use in a future version of Terraform.`, + }) gotModules := file.ModuleCalls wantModules := []*ModuleCall{ diff --git a/configs/named_values.go b/configs/named_values.go index 0454a98ee2..6f6b469fd7 100644 --- a/configs/named_values.go +++ b/configs/named_values.go @@ -68,6 +68,16 @@ func decodeVariableBlock(block *hcl.Block, override bool) (*Variable, hcl.Diagno }) } } + for _, blockS := range moduleBlockSchema.Blocks { + if blockS.Type == v.Name { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid variable name", + Detail: fmt.Sprintf("The variable name %q is reserved due to its special meaning inside module blocks.", blockS.Type), + Subject: &block.LabelRanges[0], + }) + } + } if attr, exists := content.Attributes["description"]; exists { valDiags := gohcl.DecodeExpression(attr.Expr, nil, &v.Description) diff --git a/configs/parser_test.go b/configs/parser_test.go index 93939fd88b..42249f4a60 100644 --- a/configs/parser_test.go +++ b/configs/parser_test.go @@ -85,6 +85,36 @@ func assertDiagnosticSummary(t *testing.T, diags hcl.Diagnostics, want string) b return true } +func assertExactDiagnostics(t *testing.T, diags hcl.Diagnostics, want []string) bool { + t.Helper() + + gotDiags := map[string]bool{} + wantDiags := map[string]bool{} + + for _, diag := range diags { + gotDiags[diag.Error()] = true + } + for _, msg := range want { + wantDiags[msg] = true + } + + bad := false + for got := range gotDiags { + if _, exists := wantDiags[got]; !exists { + t.Errorf("unexpected diagnostic: %s", got) + bad = true + } + } + for want := range wantDiags { + if _, exists := gotDiags[want]; !exists { + t.Errorf("missing expected diagnostic: %s", want) + bad = true + } + } + + return bad +} + func assertResultDeepEqual(t *testing.T, got, want interface{}) bool { t.Helper() if !reflect.DeepEqual(got, want) { diff --git a/configs/provider.go b/configs/provider.go index b12a95f49d..d01d5cf2e5 100644 --- a/configs/provider.go +++ b/configs/provider.go @@ -56,6 +56,28 @@ func decodeProviderBlock(block *hcl.Block) (*Provider, hcl.Diagnostics) { diags = append(diags, versionDiags...) } + // Reserved attribute names + for _, name := range []string{"count", "depends_on", "for_each", "source"} { + if attr, exists := content.Attributes[name]; exists { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Reserved argument name in provider block", + Detail: fmt.Sprintf("The provider argument name %q is reserved for use by Terraform in a future version.", name), + Subject: &attr.NameRange, + }) + } + } + + // Reserved block types (all of them) + for _, block := range content.Blocks { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Reserved block type name in provider block", + Detail: fmt.Sprintf("The block type name %q is reserved for use by Terraform in a future version.", block.Type), + Subject: &block.TypeRange, + }) + } + return provider, diags } @@ -107,5 +129,16 @@ var providerBlockSchema = &hcl.BodySchema{ { Name: "version", }, + + // Attribute names reserved for future expansion. + {Name: "count"}, + {Name: "depends_on"}, + {Name: "for_each"}, + {Name: "source"}, + }, + Blocks: []hcl.BlockHeaderSchema{ + // _All_ of these are reserved for future expansion. + {Type: "lifecycle"}, + {Type: "locals"}, }, } diff --git a/configs/provider_test.go b/configs/provider_test.go new file mode 100644 index 0000000000..526f2342e1 --- /dev/null +++ b/configs/provider_test.go @@ -0,0 +1,26 @@ +package configs + +import ( + "io/ioutil" + "testing" +) + +func TestProviderReservedNames(t *testing.T) { + src, err := ioutil.ReadFile("test-fixtures/invalid-files/provider-reserved.tf") + if err != nil { + t.Fatal(err) + } + parser := testParser(map[string]string{ + "config.tf": string(src), + }) + _, diags := parser.LoadConfigFile("config.tf") + + assertExactDiagnostics(t, diags, []string{ + `config.tf:10,3-8: Reserved argument name in provider block; The provider argument name "count" is reserved for use by Terraform in a future version.`, + `config.tf:11,3-13: Reserved argument name in provider block; The provider argument name "depends_on" is reserved for use by Terraform in a future version.`, + `config.tf:12,3-11: Reserved argument name in provider block; The provider argument name "for_each" is reserved for use by Terraform in a future version.`, + `config.tf:14,3-12: Reserved block type name in provider block; The block type name "lifecycle" is reserved for use by Terraform in a future version.`, + `config.tf:15,3-9: Reserved block type name in provider block; The block type name "locals" is reserved for use by Terraform in a future version.`, + `config.tf:13,3-9: Reserved argument name in provider block; The provider argument name "source" is reserved for use by Terraform in a future version.`, + }) +} diff --git a/configs/provisioner.go b/configs/provisioner.go index 9090449025..b031dd0b0a 100644 --- a/configs/provisioner.go +++ b/configs/provisioner.go @@ -93,8 +93,14 @@ func decodeProvisionerBlock(block *hcl.Block) (*Provisioner, hcl.Diagnostics) { } default: - // Should never happen because there are no other block types - // declared in our schema. + // Any other block types are ones we've reserved for future use, + // so they get a generic message. + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Reserved block type name in provisioner block", + Detail: fmt.Sprintf("The block type name %q is reserved for use by Terraform in a future version.", block.Type), + Subject: &block.TypeRange, + }) } } @@ -134,16 +140,11 @@ const ( var provisionerBlockSchema = &hcl.BodySchema{ Attributes: []hcl.AttributeSchema{ - { - Name: "when", - }, - { - Name: "on_failure", - }, + {Name: "when"}, + {Name: "on_failure"}, }, Blocks: []hcl.BlockHeaderSchema{ - { - Type: "connection", - }, + {Type: "connection"}, + {Type: "lifecycle"}, // reserved for future use }, } diff --git a/configs/resource.go b/configs/resource.go index 1a39fe7cc2..de1a3434a4 100644 --- a/configs/resource.go +++ b/configs/resource.go @@ -110,7 +110,14 @@ func decodeResourceBlock(block *hcl.Block) (*Resource, hcl.Diagnostics) { } if attr, exists := content.Attributes["for_each"]; exists { - r.Count = attr.Expr + r.ForEach = attr.Expr + // We currently parse this, but don't yet do anything with it. + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Reserved argument name in resource block", + Detail: fmt.Sprintf("The name %q is reserved for use in a future version of Terraform.", attr.Name), + Subject: &attr.NameRange, + }) } if attr, exists := content.Attributes["provider"]; exists { @@ -244,9 +251,14 @@ func decodeResourceBlock(block *hcl.Block) (*Resource, hcl.Diagnostics) { } default: - // Should never happen, because the above cases should always be - // exhaustive for all the types specified in our schema. - continue + // Any other block types are ones we've reserved for future use, + // so they get a generic message. + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Reserved block type name in resource block", + Detail: fmt.Sprintf("The block type name %q is reserved for use by Terraform in a future version.", block.Type), + Subject: &block.TypeRange, + }) } } @@ -287,7 +299,14 @@ func decodeDataBlock(block *hcl.Block) (*Resource, hcl.Diagnostics) { } if attr, exists := content.Attributes["for_each"]; exists { - r.Count = attr.Expr + r.ForEach = attr.Expr + // We currently parse this, but don't yet do anything with it. + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Reserved argument name in module block", + Detail: fmt.Sprintf("The name %q is reserved for use in a future version of Terraform.", attr.Name), + Subject: &attr.NameRange, + }) } if attr, exists := content.Attributes["provider"]; exists { @@ -303,17 +322,23 @@ func decodeDataBlock(block *hcl.Block) (*Resource, hcl.Diagnostics) { } for _, block := range content.Blocks { - // Our schema only allows for "lifecycle" blocks, so we can assume - // that this is all we will see here. We don't have any lifecycle - // attributes for data resources currently, so we'll just produce - // an error. - 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, - }) - break + // All of the block types we accept are just reserved for future use, but some get a specialized error message. + switch block.Type { + 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, + }) + default: + 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, + }) + } } return r, diags @@ -431,25 +456,18 @@ var commonResourceAttributes = []hcl.AttributeSchema{ var resourceBlockSchema = &hcl.BodySchema{ Attributes: commonResourceAttributes, Blocks: []hcl.BlockHeaderSchema{ - { - Type: "lifecycle", - }, - { - Type: "connection", - }, - { - Type: "provisioner", - LabelNames: []string{"type"}, - }, + {Type: "locals"}, // reserved for future use + {Type: "lifecycle"}, + {Type: "connection"}, + {Type: "provisioner", LabelNames: []string{"type"}}, }, } var dataBlockSchema = &hcl.BodySchema{ Attributes: commonResourceAttributes, Blocks: []hcl.BlockHeaderSchema{ - { - Type: "lifecycle", - }, + {Type: "lifecycle"}, // reserved for future use + {Type: "locals"}, // reserved for future use }, } diff --git a/configs/test-fixtures/invalid-files/data-reserved-for_each.tf b/configs/test-fixtures/invalid-files/data-reserved-for_each.tf new file mode 100644 index 0000000000..1ccc01bfbc --- /dev/null +++ b/configs/test-fixtures/invalid-files/data-reserved-for_each.tf @@ -0,0 +1,3 @@ +data "test" "foo" { + for_each = ["a"] +} diff --git a/configs/test-fixtures/invalid-files/data-reserved-lifecycle.tf b/configs/test-fixtures/invalid-files/data-reserved-lifecycle.tf new file mode 100644 index 0000000000..a1e1a1e6e7 --- /dev/null +++ b/configs/test-fixtures/invalid-files/data-reserved-lifecycle.tf @@ -0,0 +1,3 @@ +data "test" "foo" { + lifecycle {} +} diff --git a/configs/test-fixtures/invalid-files/data-reserved-locals.tf b/configs/test-fixtures/invalid-files/data-reserved-locals.tf new file mode 100644 index 0000000000..d6558f87ed --- /dev/null +++ b/configs/test-fixtures/invalid-files/data-reserved-locals.tf @@ -0,0 +1,3 @@ +data "test" "foo" { + locals {} +} diff --git a/configs/test-fixtures/valid-files/module-calls.tf b/configs/test-fixtures/invalid-files/module-calls.tf similarity index 100% rename from configs/test-fixtures/valid-files/module-calls.tf rename to configs/test-fixtures/invalid-files/module-calls.tf diff --git a/configs/test-fixtures/invalid-files/provider-reserved.tf b/configs/test-fixtures/invalid-files/provider-reserved.tf new file mode 100644 index 0000000000..559474de99 --- /dev/null +++ b/configs/test-fixtures/invalid-files/provider-reserved.tf @@ -0,0 +1,16 @@ +provider "test" { + # These are okay + alias = "foo" + version = "1.0.0" + + # Provider-specific arguments are also okay + arbitrary = true + + # These are all reserved and should generate errors. + count = 3 + depends_on = ["foo.bar"] + for_each = ["a", "b"] + source = "foo.example.com/baz/bar" + lifecycle {} + locals {} +} diff --git a/configs/test-fixtures/invalid-files/resource-reserved-for_each.tf b/configs/test-fixtures/invalid-files/resource-reserved-for_each.tf new file mode 100644 index 0000000000..9cfebd89af --- /dev/null +++ b/configs/test-fixtures/invalid-files/resource-reserved-for_each.tf @@ -0,0 +1,3 @@ +resource "test" "foo" { + for_each = ["a"] +} diff --git a/configs/test-fixtures/invalid-files/resource-reserved-locals.tf b/configs/test-fixtures/invalid-files/resource-reserved-locals.tf new file mode 100644 index 0000000000..368c21f7cb --- /dev/null +++ b/configs/test-fixtures/invalid-files/resource-reserved-locals.tf @@ -0,0 +1,3 @@ +resource "test" "foo" { + locals {} +}