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

@@ -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
 }
 

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.
+`

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())
+		}
+	})
+}