From c7fe6b9160cbf0038bc254b819561799e743bef7 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Wed, 22 Apr 2020 17:12:33 -0700 Subject: [PATCH] command/cliconfig: handle provider_installation block in JSON syntax The CLI config can be written in both native HCL and HCL JSON syntaxes, so the provider_installation block must be expressible using JSON too. Our previous checks to approximate HCL 2-level strictness were too strict for HCL JSON where things are more ambiguous even in HCL 2, so this includes some additional relaxations if we detect that we're decoding an AST produced from a JSON file. This is still subject to the quirky ways HCL 1 handles JSON though, so the JSON value must be structured in a way that doesn't trigger HCL's heuristics that try to guess what is a block and what is an attribute. (This is the issue that HCL 2 fixes by always decoding using a schema; there's more context on this in: https://log.martinatkins.me/2019/04/25/hcl-json/ ) --- command/cliconfig/provider_installation.go | 44 +++++++++++--- .../cliconfig/provider_installation_test.go | 58 ++++++++++--------- .../testdata/provider-installation.json | 19 ++++++ 3 files changed, 85 insertions(+), 36 deletions(-) create mode 100644 command/cliconfig/testdata/provider-installation.json diff --git a/command/cliconfig/provider_installation.go b/command/cliconfig/provider_installation.go index 28ca74b2c6..3d4919950c 100644 --- a/command/cliconfig/provider_installation.go +++ b/command/cliconfig/provider_installation.go @@ -42,7 +42,12 @@ func decodeProviderInstallationFromConfig(hclFile *hclast.File) ([]*ProviderInst if block.Keys[0].Token.Value() != "provider_installation" { continue } - if block.Assign.Line != 0 { + // HCL only tracks whether the input was JSON or native syntax inside + // individual tokens, so we'll use our block type token to decide + // and assume that the rest of the block must be written in the same + // syntax, because syntax is a whole-file idea. + isJSON := block.Keys[0].Token.JSON + if block.Assign.Line != 0 && !isJSON { // Seems to be an attribute rather than a block diags = diags.Append(tfdiags.Sourceless( tfdiags.Error, @@ -51,7 +56,7 @@ func decodeProviderInstallationFromConfig(hclFile *hclast.File) ([]*ProviderInst )) continue } - if len(block.Keys) > 1 { + if len(block.Keys) > 1 && !isJSON { diags = diags.Append(tfdiags.Sourceless( tfdiags.Error, "Invalid provider_installation block", @@ -61,13 +66,22 @@ func decodeProviderInstallationFromConfig(hclFile *hclast.File) ([]*ProviderInst pi := &ProviderInstallation{} - // Because we checked block.Assign was unset above we can assume that - // we're reading something produced with block syntax and therefore - // it will always be an hclast.ObjectType. - body := block.Val.(*hclast.ObjectType) + body, ok := block.Val.(*hclast.ObjectType) + if !ok { + // We can't get in here with native HCL syntax because we + // already checked above that we're using block syntax, but + // if we're reading JSON then our value could potentially be + // anything. + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Invalid provider_installation block", + fmt.Sprintf("The provider_installation block at %s must not be introduced with an equals sign.", block.Pos()), + )) + continue + } for _, methodBlock := range body.List.Items { - if methodBlock.Assign.Line != 0 { + if methodBlock.Assign.Line != 0 && !isJSON { // Seems to be an attribute rather than a block diags = diags.Append(tfdiags.Sourceless( tfdiags.Error, @@ -76,7 +90,7 @@ func decodeProviderInstallationFromConfig(hclFile *hclast.File) ([]*ProviderInst )) continue } - if len(methodBlock.Keys) > 1 { + if len(methodBlock.Keys) > 1 && !isJSON { diags = diags.Append(tfdiags.Sourceless( tfdiags.Error, "Invalid provider_installation method block", @@ -84,7 +98,19 @@ func decodeProviderInstallationFromConfig(hclFile *hclast.File) ([]*ProviderInst )) } - methodBody := methodBlock.Val.(*hclast.ObjectType) + methodBody, ok := methodBlock.Val.(*hclast.ObjectType) + if !ok { + // We can't get in here with native HCL syntax because we + // already checked above that we're using block syntax, but + // if we're reading JSON then our value could potentially be + // anything. + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Invalid provider_installation method block", + fmt.Sprintf("The items inside the provider_installation block at %s must all be blocks.", block.Pos()), + )) + continue + } methodTypeStr := methodBlock.Keys[0].Token.Value().(string) var location ProviderInstallationLocation diff --git a/command/cliconfig/provider_installation_test.go b/command/cliconfig/provider_installation_test.go index 0712e0e9f6..5eb0b412f3 100644 --- a/command/cliconfig/provider_installation_test.go +++ b/command/cliconfig/provider_installation_test.go @@ -8,38 +8,42 @@ import ( ) func TestLoadConfig_providerInstallation(t *testing.T) { - got, diags := loadConfigFile(filepath.Join(fixtureDir, "provider-installation")) - if diags.HasErrors() { - t.Errorf("unexpected diagnostics: %s", diags.Err().Error()) - } + for _, configFile := range []string{"provider-installation", "provider-installation.json"} { + t.Run(configFile, func(t *testing.T) { + got, diags := loadConfigFile(filepath.Join(fixtureDir, configFile)) + if diags.HasErrors() { + t.Errorf("unexpected diagnostics: %s", diags.Err().Error()) + } - want := &Config{ - ProviderInstallation: []*ProviderInstallation{ - { - Methods: []*ProviderInstallationMethod{ + want := &Config{ + ProviderInstallation: []*ProviderInstallation{ { - Location: ProviderInstallationFilesystemMirror("/tmp/example1"), - Include: []string{"example.com/*/*"}, - }, - { - Location: ProviderInstallationNetworkMirror("https://tf-Mirror.example.com/"), - Include: []string{"registry.terraform.io/*/*"}, - Exclude: []string{"registry.Terraform.io/foobar/*"}, - }, - { - Location: ProviderInstallationFilesystemMirror("/tmp/example2"), - }, - { - Location: ProviderInstallationDirect, - Exclude: []string{"example.com/*/*"}, + Methods: []*ProviderInstallationMethod{ + { + Location: ProviderInstallationFilesystemMirror("/tmp/example1"), + Include: []string{"example.com/*/*"}, + }, + { + Location: ProviderInstallationNetworkMirror("https://tf-Mirror.example.com/"), + Include: []string{"registry.terraform.io/*/*"}, + Exclude: []string{"registry.Terraform.io/foobar/*"}, + }, + { + Location: ProviderInstallationFilesystemMirror("/tmp/example2"), + }, + { + Location: ProviderInstallationDirect, + Exclude: []string{"example.com/*/*"}, + }, + }, }, }, - }, - }, - } + } - if diff := cmp.Diff(want, got); diff != "" { - t.Errorf("wrong result\n%s", diff) + if diff := cmp.Diff(want, got); diff != "" { + t.Errorf("wrong result\n%s", diff) + } + }) } } diff --git a/command/cliconfig/testdata/provider-installation.json b/command/cliconfig/testdata/provider-installation.json new file mode 100644 index 0000000000..826d68d470 --- /dev/null +++ b/command/cliconfig/testdata/provider-installation.json @@ -0,0 +1,19 @@ +{ + "provider_installation": { + "filesystem_mirror": [{ + "path": "/tmp/example1", + "include": ["example.com/*/*"] + }], + "network_mirror": [{ + "url": "https://tf-Mirror.example.com/", + "include": ["registry.terraform.io/*/*"], + "exclude": ["registry.Terraform.io/foobar/*"] + }], + "filesystem_mirror": [{ + "path": "/tmp/example2" + }], + "direct": [{ + "exclude": ["example.com/*/*"] + }] + } +}