From e938b02337a876fc783ce7acd3ddd55b3179096d Mon Sep 17 00:00:00 2001 From: Kristin Laemmert Date: Fri, 11 Dec 2020 13:18:49 -0500 Subject: [PATCH] terraform: improve provider config related error messages (#27261) * terraform: improve provider config related error messages with nil config If there is no provider configuration present in the config at all, errors related to missing required arguments lack source information or even a reference to the provider in question. This PR adds more specific error messages in three of these situations: - ValidateProvider - ConfigureProvider: provider.PrepareProviderConfig - ConfigureProvider: ctx.ConfigureProvider To test the last case I added a ConfigureProviderFn to the MockContext. * remove newlines and let the diagnost renderer handle fit --- terraform/eval_context_mock.go | 7 + terraform/node_provider.go | 54 +++++++- terraform/node_provider_test.go | 238 ++++++++++++++++++++++++++++++++ 3 files changed, 294 insertions(+), 5 deletions(-) diff --git a/terraform/eval_context_mock.go b/terraform/eval_context_mock.go index 11ae6941f5..ca8b84a6a2 100644 --- a/terraform/eval_context_mock.go +++ b/terraform/eval_context_mock.go @@ -55,6 +55,9 @@ type MockEvalContext struct { SetProviderInputAddr addrs.AbsProviderConfig SetProviderInputValues map[string]cty.Value + ConfigureProviderFn func( + addr addrs.AbsProviderConfig, + cfg cty.Value) tfdiags.Diagnostics // overrides the other values below, if set ConfigureProviderCalled bool ConfigureProviderAddr addrs.AbsProviderConfig ConfigureProviderConfig cty.Value @@ -183,9 +186,13 @@ func (c *MockEvalContext) CloseProvider(addr addrs.AbsProviderConfig) error { } func (c *MockEvalContext) ConfigureProvider(addr addrs.AbsProviderConfig, cfg cty.Value) tfdiags.Diagnostics { + c.ConfigureProviderCalled = true c.ConfigureProviderAddr = addr c.ConfigureProviderConfig = cfg + if c.ConfigureProviderFn != nil { + return c.ConfigureProviderFn(addr, cfg) + } return c.ConfigureProviderDiags } diff --git a/terraform/node_provider.go b/terraform/node_provider.go index 98a8e85a01..585417340f 100644 --- a/terraform/node_provider.go +++ b/terraform/node_provider.go @@ -63,10 +63,22 @@ func (n *NodeApplyableProvider) ValidateProvider(ctx EvalContext, provider provi } configVal, _, evalDiags := ctx.EvaluateBlock(configBody, configSchema, nil, EvalDataForNoInstanceKey) - diags = diags.Append(evalDiags) if evalDiags.HasErrors() { - return diags + if n.Config == nil { + // If there isn't an explicit "provider" block in the configuration, + // this error message won't be very clear. Add some detail to the + // error message in this case. + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Invalid provider configuration", + fmt.Sprintf(providerConfigErr, evalDiags.Err(), n.Addr.Provider), + )) + return diags + } else { + return diags.Append(evalDiags) + } } + diags = diags.Append(evalDiags) // If our config value contains any marked values, ensure those are // stripped out before sending this to the provider @@ -126,10 +138,22 @@ func (n *NodeApplyableProvider) ConfigureProvider(ctx EvalContext, provider prov // PrepareProviderConfig is only used for validation. We are intentionally // ignoring the PreparedConfig field to maintain existing behavior. prepareResp := provider.PrepareProviderConfig(req) - diags = diags.Append(prepareResp.Diagnostics) - if diags.HasErrors() { - return diags + if prepareResp.Diagnostics.HasErrors() { + if config == nil { + // If there isn't an explicit "provider" block in the configuration, + // this error message won't be very clear. Add some detail to the + // error message in this case. + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Invalid provider configuration", + fmt.Sprintf(providerConfigErr, prepareResp.Diagnostics.Err(), n.Addr.Provider), + )) + return diags + } else { + return diags.Append(prepareResp.Diagnostics) + } } + diags = diags.Append(prepareResp.Diagnostics) // If the provider returns something different, log a warning to help // indicate to provider developers that the value is not used. @@ -139,7 +163,27 @@ func (n *NodeApplyableProvider) ConfigureProvider(ctx EvalContext, provider prov } configDiags := ctx.ConfigureProvider(n.Addr, unmarkedConfigVal) + if configDiags.HasErrors() { + if config == nil { + // If there isn't an explicit "provider" block in the configuration, + // this error message won't be very clear. Add some detail to the + // error message in this case. + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Invalid provider configuration", + fmt.Sprintf(providerConfigErr, configDiags.InConfigBody(configBody).Err(), n.Addr.Provider), + )) + return diags + } else { + return diags.Append(configDiags.InConfigBody(configBody)) + } + } diags = diags.Append(configDiags.InConfigBody(configBody)) return diags } + +const providerConfigErr = `%s + +Provider %q requires explicit configuration. Add a provider block to the root module and configure the provider's required arguments as described in the provider documentation. +` diff --git a/terraform/node_provider_test.go b/terraform/node_provider_test.go index d85bd64310..04d566f5c9 100644 --- a/terraform/node_provider_test.go +++ b/terraform/node_provider_test.go @@ -1,10 +1,16 @@ package terraform import ( + "fmt" + "strings" "testing" + "github.com/hashicorp/hcl/v2" "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/configs" + "github.com/hashicorp/terraform/configs/configschema" + "github.com/hashicorp/terraform/providers" + "github.com/hashicorp/terraform/tfdiags" "github.com/zclconf/go-cty/cty" ) @@ -189,3 +195,235 @@ func TestNodeApplyableProviderExecute_sensitiveValidate(t *testing.T) { t.Errorf("wrong configuration value\ngot: %#v\nwant: %#v", got, want) } } + +func TestNodeApplyableProvider_Validate(t *testing.T) { + provider := &MockProvider{ + GetSchemaReturn: &ProviderSchema{ + Provider: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "region": { + Type: cty.String, + Required: true, + }, + }, + }, + }, + } + ctx := &MockEvalContext{ProviderProvider: provider} + ctx.installSimpleEval() + + t.Run("valid", func(t *testing.T) { + config := &configs.Provider{ + Name: "test", + Config: configs.SynthBody("", map[string]cty.Value{ + "region": cty.StringVal("mars"), + }), + } + + node := NodeApplyableProvider{ + NodeAbstractProvider: &NodeAbstractProvider{ + Addr: mustProviderConfig(`provider["registry.terraform.io/hashicorp/aws"]`), + Config: config, + }, + } + + diags := node.ValidateProvider(ctx, provider) + if diags.HasErrors() { + t.Errorf("unexpected error with valid config: %s", diags.Err()) + } + }) + + t.Run("missing required config", func(t *testing.T) { + node := NodeApplyableProvider{ + NodeAbstractProvider: &NodeAbstractProvider{ + Addr: mustProviderConfig(`provider["registry.terraform.io/hashicorp/aws"]`), + }, + } + + diags := node.ValidateProvider(ctx, provider) + if !diags.HasErrors() { + t.Error("missing expected error with invalid config") + } + }) +} + +//This test specifically tests responses from the +//providers.PrepareProviderConfigFn. See +//TestNodeApplyableProvider_ConfigProvider_config_fn_err for +//providers.ConfigureRequest responses. +func TestNodeApplyableProvider_ConfigProvider(t *testing.T) { + provider := &MockProvider{ + GetSchemaReturn: &ProviderSchema{ + Provider: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "region": { + Type: cty.String, + Optional: true, + }, + }, + }, + }, + } + // For this test, we're returning an error for an optional argument. This + // can happen for example if an argument is only conditionally required. + provider.PrepareProviderConfigFn = func(req providers.PrepareProviderConfigRequest) (resp providers.PrepareProviderConfigResponse) { + region := req.Config.GetAttr("region") + if region.IsNull() { + resp.Diagnostics = resp.Diagnostics.Append(fmt.Errorf("value is not found")) + } + return + } + ctx := &MockEvalContext{ProviderProvider: provider} + ctx.installSimpleEval() + + t.Run("valid", func(t *testing.T) { + config := &configs.Provider{ + Name: "test", + Config: configs.SynthBody("", map[string]cty.Value{ + "region": cty.StringVal("mars"), + }), + } + + node := NodeApplyableProvider{ + NodeAbstractProvider: &NodeAbstractProvider{ + Addr: mustProviderConfig(`provider["registry.terraform.io/hashicorp/aws"]`), + Config: config, + }, + } + + diags := node.ConfigureProvider(ctx, provider, false) + if diags.HasErrors() { + t.Errorf("unexpected error with valid config: %s", diags.Err()) + } + }) + + t.Run("missing required config (no config at all)", func(t *testing.T) { + node := NodeApplyableProvider{ + NodeAbstractProvider: &NodeAbstractProvider{ + Addr: mustProviderConfig(`provider["registry.terraform.io/hashicorp/aws"]`), + }, + } + + diags := node.ConfigureProvider(ctx, provider, false) + if !diags.HasErrors() { + t.Fatal("missing expected error with nil config") + } + if !strings.Contains(diags.Err().Error(), "requires explicit configuration") { + t.Errorf("diagnostic is missing \"requires explicit configuration\" message: %s", diags.Err()) + } + }) + + t.Run("missing required config", func(t *testing.T) { + config := &configs.Provider{ + Name: "test", + Config: hcl.EmptyBody(), + } + node := NodeApplyableProvider{ + NodeAbstractProvider: &NodeAbstractProvider{ + Addr: mustProviderConfig(`provider["registry.terraform.io/hashicorp/aws"]`), + Config: config, + }, + } + + diags := node.ConfigureProvider(ctx, provider, false) + if !diags.HasErrors() { + t.Fatal("missing expected error with invalid config") + } + if diags.Err().Error() != "value is not found" { + t.Errorf("wrong diagnostic: %s", diags.Err()) + } + }) + +} + +//This test is similar to TestNodeApplyableProvider_ConfigProvider, but tests responses from the providers.ConfigureRequest +func TestNodeApplyableProvider_ConfigProvider_config_fn_err(t *testing.T) { + provider := &MockProvider{ + GetSchemaReturn: &ProviderSchema{ + Provider: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "region": { + Type: cty.String, + Optional: true, + }, + }, + }, + }, + } + ctx := &MockEvalContext{ProviderProvider: provider} + ctx.installSimpleEval() + // For this test, provider.PrepareConfigFn will succeed every time but the + // ctx.ConfigureProviderFn will return an error if a value is not found. + // + // This is an unlikely but real situation that occurs: + // https://github.com/hashicorp/terraform/issues/23087 + ctx.ConfigureProviderFn = func(addr addrs.AbsProviderConfig, cfg cty.Value) (diags tfdiags.Diagnostics) { + if cfg.IsNull() { + diags = diags.Append(fmt.Errorf("no config provided")) + } else { + region := cfg.GetAttr("region") + if region.IsNull() { + diags = diags.Append(fmt.Errorf("value is not found")) + } + } + return + } + + t.Run("valid", func(t *testing.T) { + config := &configs.Provider{ + Name: "test", + Config: configs.SynthBody("", map[string]cty.Value{ + "region": cty.StringVal("mars"), + }), + } + + node := NodeApplyableProvider{ + NodeAbstractProvider: &NodeAbstractProvider{ + Addr: mustProviderConfig(`provider["registry.terraform.io/hashicorp/aws"]`), + Config: config, + }, + } + + diags := node.ConfigureProvider(ctx, provider, false) + if diags.HasErrors() { + t.Errorf("unexpected error with valid config: %s", diags.Err()) + } + }) + + t.Run("missing required config (no config at all)", func(t *testing.T) { + node := NodeApplyableProvider{ + NodeAbstractProvider: &NodeAbstractProvider{ + Addr: mustProviderConfig(`provider["registry.terraform.io/hashicorp/aws"]`), + }, + } + + diags := node.ConfigureProvider(ctx, provider, false) + if !diags.HasErrors() { + t.Fatal("missing expected error with nil config") + } + if !strings.Contains(diags.Err().Error(), "requires explicit configuration") { + t.Errorf("diagnostic is missing \"requires explicit configuration\" message: %s", diags.Err()) + } + }) + + t.Run("missing required config", func(t *testing.T) { + config := &configs.Provider{ + Name: "test", + Config: hcl.EmptyBody(), + } + node := NodeApplyableProvider{ + NodeAbstractProvider: &NodeAbstractProvider{ + Addr: mustProviderConfig(`provider["registry.terraform.io/hashicorp/aws"]`), + Config: config, + }, + } + + diags := node.ConfigureProvider(ctx, provider, false) + if !diags.HasErrors() { + t.Fatal("missing expected error with invalid config") + } + if diags.Err().Error() != "value is not found" { + t.Errorf("wrong diagnostic: %s", diags.Err()) + } + }) +}