From 16a08c6d8bb29724247150979bf31ad1e51398ea Mon Sep 17 00:00:00 2001 From: Ben Drucker Date: Thu, 7 May 2020 20:26:42 -0700 Subject: [PATCH 01/11] validate: ignore providers with no configuration --- terraform/eval_validate.go | 6 ++ terraform/eval_validate_test.go | 115 ++++++++++++++++++++++++++++++++ 2 files changed, 121 insertions(+) diff --git a/terraform/eval_validate.go b/terraform/eval_validate.go index 00b80d37b..5cfa2f46d 100644 --- a/terraform/eval_validate.go +++ b/terraform/eval_validate.go @@ -78,6 +78,12 @@ func (n *EvalValidateProvider) Eval(ctx EvalContext) (interface{}, error) { configBody := buildProviderConfig(ctx, n.Addr, n.Config) + emptySchema := providers.Schema{}.Block + _, _, evalDiags := ctx.EvaluateBlock(configBody, emptySchema, nil, EvalDataForNoInstanceKey) + if !evalDiags.HasErrors() { + return nil, nil + } + resp := provider.GetSchema() diags = diags.Append(resp.Diagnostics) if diags.HasErrors() { diff --git a/terraform/eval_validate_test.go b/terraform/eval_validate_test.go index 6adfeab92..fd2bffa97 100644 --- a/terraform/eval_validate_test.go +++ b/terraform/eval_validate_test.go @@ -520,3 +520,118 @@ func TestEvalValidateProvisioner_connectionInvalid(t *testing.T) { t.Fatalf("wrong errors %q; want something about each of our invalid connInfo keys", errStr) } } + +func TestEvalValidateProvider_empty(t *testing.T) { + mp := simpleMockProvider() + mp.GetSchemaReturn = &ProviderSchema{ + Provider: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "required": { + Required: true, + }, + }, + }, + } + + p := providers.Interface(mp) + rc := &configs.Provider{ + Name: "foo", + Config: hcl.EmptyBody(), + } + + node := &EvalValidateProvider{ + Provider: &p, + Config: rc, + } + + ctx := &MockEvalContext{} + ctx.installSimpleEval() + + _, err := node.Eval(ctx) + if err != nil { + t.Fatalf("err: %s", err) + } +} + +func TestEvalValidateProvider_nonempty_invalid(t *testing.T) { + mp := simpleMockProvider() + mp.GetSchemaReturn = &ProviderSchema{ + Provider: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "required_a": { + Type: cty.String, + Required: true, + }, + "required_b": { + Type: cty.String, + Required: true, + }, + }, + }, + } + + p := providers.Interface(mp) + rc := &configs.Provider{ + Name: "foo", + Config: configs.SynthBody("", map[string]cty.Value{ + "required_a": cty.StringVal("set"), + }), + } + + node := &EvalValidateProvider{ + Provider: &p, + Config: rc, + } + + ctx := &MockEvalContext{} + ctx.installSimpleEval() + + _, err := node.Eval(ctx) + if err == nil { + t.Fatalf("expected error, got nil") + } + + if !strings.Contains(err.Error(), "required_b") || strings.Contains(err.Error(), "required_a") { + t.Fatalf("expected only attribute 'required_b' is required error, got %v", err) + } +} + +func TestEvalValidateProvider_nonempty_valid(t *testing.T) { + mp := simpleMockProvider() + mp.GetSchemaReturn = &ProviderSchema{ + Provider: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "required_a": { + Type: cty.String, + Required: true, + }, + "required_b": { + Type: cty.String, + Required: true, + }, + }, + }, + } + + p := providers.Interface(mp) + rc := &configs.Provider{ + Name: "foo", + Config: configs.SynthBody("", map[string]cty.Value{ + "required_a": cty.StringVal("set"), + "required_b": cty.StringVal("set"), + }), + } + + node := &EvalValidateProvider{ + Provider: &p, + Config: rc, + } + + ctx := &MockEvalContext{} + ctx.installSimpleEval() + + _, err := node.Eval(ctx) + if err != nil { + t.Fatalf("err: %s", err) + } +} From 07dbe7bd5f39701af11f722790b22393e5edb1e1 Mon Sep 17 00:00:00 2001 From: Ben Drucker Date: Thu, 7 May 2020 22:23:11 -0700 Subject: [PATCH 02/11] update context tests to expect provider validation to be skipped --- terraform/context_validate_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/terraform/context_validate_test.go b/terraform/context_validate_test.go index f811dfcad..6ee9985c7 100644 --- a/terraform/context_validate_test.go +++ b/terraform/context_validate_test.go @@ -588,12 +588,12 @@ func TestContext2Validate_providerConfig_badEmpty(t *testing.T) { }) p.PrepareProviderConfigResponse = providers.PrepareProviderConfigResponse{ - Diagnostics: tfdiags.Diagnostics{}.Append(fmt.Errorf("bad")), + Diagnostics: tfdiags.Diagnostics{}.Append(fmt.Errorf("should not be called")), } diags := c.Validate() - if !diags.HasErrors() { - t.Fatalf("succeeded; want error") + if diags.HasErrors() { + t.Fatalf("unexpected error: %v", diags) } } From d79424708e744d4bcdaa26740a6cf8062cf434b9 Mon Sep 17 00:00:00 2001 From: Ben Drucker Date: Thu, 7 May 2020 22:26:21 -0700 Subject: [PATCH 03/11] rename test --- terraform/context_validate_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/terraform/context_validate_test.go b/terraform/context_validate_test.go index 6ee9985c7..f467ae9bb 100644 --- a/terraform/context_validate_test.go +++ b/terraform/context_validate_test.go @@ -564,7 +564,7 @@ func TestContext2Validate_providerConfig_bad(t *testing.T) { } } -func TestContext2Validate_providerConfig_badEmpty(t *testing.T) { +func TestContext2Validate_providerConfig_skippedEmpty(t *testing.T) { m := testModule(t, "validate-bad-pc-empty") p := testProvider("aws") p.GetSchemaReturn = &ProviderSchema{ From 77f082bda2bcaf0b811124410f062046063d9a22 Mon Sep 17 00:00:00 2001 From: Ben Drucker Date: Thu, 7 May 2020 22:36:49 -0700 Subject: [PATCH 04/11] remove assertion that PrepareProviderConfig was called --- backend/local/backend_refresh_test.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/backend/local/backend_refresh_test.go b/backend/local/backend_refresh_test.go index 4b2bcc2df..7b3a2f206 100644 --- a/backend/local/backend_refresh_test.go +++ b/backend/local/backend_refresh_test.go @@ -191,10 +191,6 @@ func TestLocal_refreshValidate(t *testing.T) { } <-run.Done() - if !p.PrepareProviderConfigCalled { - t.Fatal("Prepare provider config should be called") - } - checkState(t, b.StateOutPath, ` test_instance.foo: ID = yes From 1fda88ed56bcbf948d31573c8460ddb422e1c5ee Mon Sep 17 00:00:00 2001 From: Ben Drucker Date: Thu, 7 May 2020 22:38:22 -0700 Subject: [PATCH 05/11] reuse emptySchema, set directly to Block --- terraform/eval_validate.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/terraform/eval_validate.go b/terraform/eval_validate.go index 5cfa2f46d..fb570a4f5 100644 --- a/terraform/eval_validate.go +++ b/terraform/eval_validate.go @@ -78,7 +78,7 @@ func (n *EvalValidateProvider) Eval(ctx EvalContext) (interface{}, error) { configBody := buildProviderConfig(ctx, n.Addr, n.Config) - emptySchema := providers.Schema{}.Block + emptySchema := &configschema.Block{} _, _, evalDiags := ctx.EvaluateBlock(configBody, emptySchema, nil, EvalDataForNoInstanceKey) if !evalDiags.HasErrors() { return nil, nil @@ -95,7 +95,7 @@ func (n *EvalValidateProvider) Eval(ctx EvalContext) (interface{}, error) { // Should never happen in real code, but often comes up in tests where // mock schemas are being used that tend to be incomplete. log.Printf("[WARN] EvalValidateProvider: no config schema is available for %s, so using empty schema", n.Addr) - configSchema = &configschema.Block{} + configSchema = emptySchema } configVal, configBody, evalDiags := ctx.EvaluateBlock(configBody, configSchema, nil, EvalDataForNoInstanceKey) From 22440539c18f689eb70a75a782ede803649e4f9b Mon Sep 17 00:00:00 2001 From: Ben Drucker Date: Mon, 9 Nov 2020 16:26:17 -0800 Subject: [PATCH 06/11] fix error output --- terraform/context_validate_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/terraform/context_validate_test.go b/terraform/context_validate_test.go index d90db6602..4ea281999 100644 --- a/terraform/context_validate_test.go +++ b/terraform/context_validate_test.go @@ -624,7 +624,7 @@ func TestContext2Validate_providerConfig_skippedEmpty(t *testing.T) { diags := c.Validate() if diags.HasErrors() { - t.Fatalf("unexpected error: %v", diags) + t.Fatalf("unexpected error: %s", diags.Err()) } } From cd7d78968c50bb86dcd8f4cda00b55a4683e6cab Mon Sep 17 00:00:00 2001 From: Ben Drucker Date: Mon, 9 Nov 2020 16:27:17 -0800 Subject: [PATCH 07/11] move empty provider logic to node_provider --- terraform/node_provider.go | 9 ++++++++- terraform/node_provider_test.go | 35 +++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/terraform/node_provider.go b/terraform/node_provider.go index 9566a136b..ecf89cd5f 100644 --- a/terraform/node_provider.go +++ b/terraform/node_provider.go @@ -48,6 +48,13 @@ func (n *NodeApplyableProvider) ValidateProvider(ctx EvalContext, provider provi configBody := buildProviderConfig(ctx, n.Addr, n.ProviderConfig()) + // if provider config is empty, return early + emptySchema := &configschema.Block{} + _, _, evalDiags := ctx.EvaluateBlock(configBody, emptySchema, nil, EvalDataForNoInstanceKey) + if !evalDiags.HasErrors() { + return nil + } + resp := provider.GetSchema() diags = diags.Append(resp.Diagnostics) if diags.HasErrors() { @@ -59,7 +66,7 @@ func (n *NodeApplyableProvider) ValidateProvider(ctx EvalContext, provider provi // Should never happen in real code, but often comes up in tests where // mock schemas are being used that tend to be incomplete. log.Printf("[WARN] ValidateProvider: no config schema is available for %s, so using empty schema", n.Addr) - configSchema = &configschema.Block{} + configSchema = emptySchema } configVal, configBody, evalDiags := ctx.EvaluateBlock(configBody, configSchema, nil, EvalDataForNoInstanceKey) diff --git a/terraform/node_provider_test.go b/terraform/node_provider_test.go index 9ce13b9df..cba7e07cf 100644 --- a/terraform/node_provider_test.go +++ b/terraform/node_provider_test.go @@ -5,6 +5,7 @@ import ( "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/configs" + "github.com/hashicorp/terraform/configs/configschema" "github.com/zclconf/go-cty/cty" ) @@ -115,3 +116,37 @@ func TestNodeApplyableProviderExecute_unknownApply(t *testing.T) { t.Errorf("wrong configuration value\ngot: %#v\nwant: %#v", got, want) } } + +func TestNodeApplyableProviderExecute_emptyValidate(t *testing.T) { + config := &configs.Provider{ + Name: "foo", + Config: configs.SynthBody("", map[string]cty.Value{}), + } + provider := mockProviderWithConfigSchema(&configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "test_string": { + Type: cty.String, + Required: true, + }, + }, + }) + providerAddr := addrs.AbsProviderConfig{ + Module: addrs.RootModule, + Provider: addrs.NewDefaultProvider("foo"), + } + + n := &NodeApplyableProvider{&NodeAbstractProvider{ + Addr: providerAddr, + Config: config, + }} + + ctx := &MockEvalContext{ProviderProvider: provider} + ctx.installSimpleEval() + if err := n.Execute(ctx, walkValidate); err != nil { + t.Fatalf("err: %s", err) + } + + if ctx.ConfigureProviderCalled { + t.Fatal("should not be called") + } +} From eacf8b5c553c3167430a71521d85513a93451b23 Mon Sep 17 00:00:00 2001 From: Ben Drucker Date: Sun, 6 Dec 2020 09:35:35 -0800 Subject: [PATCH 08/11] rename empty provider config test --- terraform/context_validate_test.go | 2 +- .../main.tf | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename terraform/testdata/{validate-bad-pc-empty => validate-skipped-pc-empty}/main.tf (100%) diff --git a/terraform/context_validate_test.go b/terraform/context_validate_test.go index 4ea281999..a706bb794 100644 --- a/terraform/context_validate_test.go +++ b/terraform/context_validate_test.go @@ -596,7 +596,7 @@ func TestContext2Validate_providerConfig_bad(t *testing.T) { } func TestContext2Validate_providerConfig_skippedEmpty(t *testing.T) { - m := testModule(t, "validate-bad-pc-empty") + m := testModule(t, "validate-skipped-pc-empty") p := testProvider("aws") p.GetSchemaReturn = &ProviderSchema{ Provider: &configschema.Block{ diff --git a/terraform/testdata/validate-bad-pc-empty/main.tf b/terraform/testdata/validate-skipped-pc-empty/main.tf similarity index 100% rename from terraform/testdata/validate-bad-pc-empty/main.tf rename to terraform/testdata/validate-skipped-pc-empty/main.tf From 186eff96b8ee40f2da6f3288ade8868c25b168cc Mon Sep 17 00:00:00 2001 From: Ben Drucker Date: Sun, 6 Dec 2020 09:36:53 -0800 Subject: [PATCH 09/11] expand early return comment --- terraform/node_provider.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/terraform/node_provider.go b/terraform/node_provider.go index ecf89cd5f..d843b57c1 100644 --- a/terraform/node_provider.go +++ b/terraform/node_provider.go @@ -48,7 +48,9 @@ func (n *NodeApplyableProvider) ValidateProvider(ctx EvalContext, provider provi configBody := buildProviderConfig(ctx, n.Addr, n.ProviderConfig()) - // if provider config is empty, return early + // if a provider config is empty (only an alias), return early and don't continue + // validation. validate doesn't need to fully configure the provider itself, so + // skipping a provider with an implied configuration won't prevent other validation from completing. emptySchema := &configschema.Block{} _, _, evalDiags := ctx.EvaluateBlock(configBody, emptySchema, nil, EvalDataForNoInstanceKey) if !evalDiags.HasErrors() { From a2c31088f446aecf2ba359dcde7526c6b0a4adb5 Mon Sep 17 00:00:00 2001 From: Ben Drucker Date: Sun, 6 Dec 2020 09:38:43 -0800 Subject: [PATCH 10/11] call configBody.Content --- terraform/node_provider.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/terraform/node_provider.go b/terraform/node_provider.go index d843b57c1..ed56c8711 100644 --- a/terraform/node_provider.go +++ b/terraform/node_provider.go @@ -51,9 +51,8 @@ func (n *NodeApplyableProvider) ValidateProvider(ctx EvalContext, provider provi // if a provider config is empty (only an alias), return early and don't continue // validation. validate doesn't need to fully configure the provider itself, so // skipping a provider with an implied configuration won't prevent other validation from completing. - emptySchema := &configschema.Block{} - _, _, evalDiags := ctx.EvaluateBlock(configBody, emptySchema, nil, EvalDataForNoInstanceKey) - if !evalDiags.HasErrors() { + _, noConfigDiags := configBody.Content(&hcl.BodySchema{}) + if !noConfigDiags.HasErrors() { return nil } @@ -68,7 +67,7 @@ func (n *NodeApplyableProvider) ValidateProvider(ctx EvalContext, provider provi // Should never happen in real code, but often comes up in tests where // mock schemas are being used that tend to be incomplete. log.Printf("[WARN] ValidateProvider: no config schema is available for %s, so using empty schema", n.Addr) - configSchema = emptySchema + configSchema = &configschema.Block{} } configVal, configBody, evalDiags := ctx.EvaluateBlock(configBody, configSchema, nil, EvalDataForNoInstanceKey) From 2549e53aed1151532001760be9215a9a16971c1f Mon Sep 17 00:00:00 2001 From: Ben Drucker Date: Sun, 6 Dec 2020 10:02:26 -0800 Subject: [PATCH 11/11] add backend refresh test with provider config --- backend/local/backend_refresh_test.go | 50 +++++++++++++++++++ .../testdata/refresh-provider-config/main.tf | 7 +++ 2 files changed, 57 insertions(+) create mode 100644 backend/local/testdata/refresh-provider-config/main.tf diff --git a/backend/local/backend_refresh_test.go b/backend/local/backend_refresh_test.go index a96587637..73b223df3 100644 --- a/backend/local/backend_refresh_test.go +++ b/backend/local/backend_refresh_test.go @@ -142,6 +142,56 @@ test_instance.foo: `) } +func TestLocal_refreshValidateProviderConfigured(t *testing.T) { + b, cleanup := TestLocal(t) + defer cleanup() + + schema := &terraform.ProviderSchema{ + Provider: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "value": {Type: cty.String, Optional: true}, + }, + }, + ResourceTypes: map[string]*configschema.Block{ + "test_instance": { + Attributes: map[string]*configschema.Attribute{ + "id": {Type: cty.String, Computed: true}, + "ami": {Type: cty.String, Optional: true}, + }, + }, + }, + } + + p := TestLocalProvider(t, b, "test", schema) + testStateFile(t, b.StatePath, testRefreshState()) + p.ReadResourceFn = nil + p.ReadResourceResponse = providers.ReadResourceResponse{NewState: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("yes"), + })} + + // Enable validation + b.OpValidation = true + + op, configCleanup := testOperationRefresh(t, "./testdata/refresh-provider-config") + defer configCleanup() + + run, err := b.Operation(context.Background(), op) + if err != nil { + t.Fatalf("bad: %s", err) + } + <-run.Done() + + if !p.PrepareProviderConfigCalled { + t.Fatal("Prepare provider config should be called") + } + + checkState(t, b.StateOutPath, ` +test_instance.foo: + ID = yes + provider = provider["registry.terraform.io/hashicorp/test"] + `) +} + // This test validates the state lacking behavior when the inner call to // Context() fails func TestLocal_refresh_context_error(t *testing.T) { diff --git a/backend/local/testdata/refresh-provider-config/main.tf b/backend/local/testdata/refresh-provider-config/main.tf new file mode 100644 index 000000000..f3a3ebb85 --- /dev/null +++ b/backend/local/testdata/refresh-provider-config/main.tf @@ -0,0 +1,7 @@ +resource "test_instance" "foo" { + ami = "bar" +} + +provider "test" { + value = "foo" +}