providers/terraform: Explicit validate step

We were previously catching some errors at read time, but some type errors
were panicking because the cty.DynamicPseudoType arguments have no
automatic pre-type-checking done but this code was assuming they would
be objects.

Here we add an explicit validation step that includes both the backend
validation we were previously doing during read and some additional
type checking to ensure the two dynamic arguments are suitably-typed.
Having the separate validation step means that these problems can be
detected by "terraform validate", rather than only in "terraform plan"
or "terraform apply".

builtin/providers/terraform/data_source_state.go

@@ -42,79 +42,65 @@ func dataSourceRemoteStateGetSchema() providers.Schema {
 	}
 }
 
-func dataSourceRemoteStateRead(d *cty.Value) (cty.Value, tfdiags.Diagnostics) {
+func dataSourceRemoteStateValidate(cfg cty.Value) tfdiags.Diagnostics {
 	var diags tfdiags.Diagnostics
 
-	newState := make(map[string]cty.Value)
-	newState["backend"] = d.GetAttr("backend")
-
-	backendType := d.GetAttr("backend").AsString()
-
-	// Don't break people using the old _local syntax - but note warning above
-	if backendType == "_local" {
-		log.Println(`[INFO] Switching old (unsupported) backend "_local" to "local"`)
-		backendType = "local"
+	// Getting the backend implicitly validates the configuration for it,
+	// but we can only do that if it's all known already.
+	if cfg.GetAttr("config").IsWhollyKnown() && cfg.GetAttr("backend").IsKnown() {
+		_, moreDiags := getBackend(cfg)
+		diags = diags.Append(moreDiags)
+	} else {
+		// Otherwise we'll just type-check the config object itself.
+		configTy := cfg.GetAttr("config").Type()
+		if configTy != cty.DynamicPseudoType && !(configTy.IsObjectType() || configTy.IsMapType()) {
+			diags = diags.Append(tfdiags.AttributeValue(
+				tfdiags.Error,
+				"Invalid backend configuration",
+				"The configuration must be an object value.",
+				cty.GetAttrPath("config"),
+			))
+		}
 	}
 
-	// Create the client to access our remote state
-	log.Printf("[DEBUG] Initializing remote state backend: %s", backendType)
-	f := backendInit.Backend(backendType)
-	if f == nil {
-		diags = diags.Append(tfdiags.AttributeValue(
-			tfdiags.Error,
-			"Invalid backend configuration",
-			fmt.Sprintf("Unknown backend type: %s", backendType),
-			cty.Path(nil).GetAttr("backend"),
-		))
-		return cty.NilVal, diags
+	{
+		defaultsTy := cfg.GetAttr("defaults").Type()
+		if defaultsTy != cty.DynamicPseudoType && !(defaultsTy.IsObjectType() || defaultsTy.IsMapType()) {
+			diags = diags.Append(tfdiags.AttributeValue(
+				tfdiags.Error,
+				"Invalid default values",
+				"Defaults must be given in an object value.",
+				cty.GetAttrPath("defaults"),
+			))
+		}
 	}
-	b := f()
 
-	config := d.GetAttr("config")
-	if config.IsNull() {
-		// We'll treat this as an empty configuration and see if the backend's
-		// schema and validation code will accept it.
-		config = cty.EmptyObjectVal
-	}
-	newState["config"] = config
+	return diags
+}
 
-	schema := b.ConfigSchema()
-	// Try to coerce the provided value into the desired configuration type.
-	configVal, err := schema.CoerceValue(config)
-	if err != nil {
-		diags = diags.Append(tfdiags.AttributeValue(
-			tfdiags.Error,
-			"Invalid backend configuration",
-			fmt.Sprintf("The given configuration is not valid for backend %q: %s.", backendType,
-				tfdiags.FormatError(err)),
-			cty.Path(nil).GetAttr("config"),
-		))
-		return cty.NilVal, diags
-	}
+func dataSourceRemoteStateRead(d cty.Value) (cty.Value, tfdiags.Diagnostics) {
+	var diags tfdiags.Diagnostics
 
-	newVal, validateDiags := b.PrepareConfig(configVal)
-	diags = diags.Append(validateDiags)
-	if validateDiags.HasErrors() {
+	b, moreDiags := getBackend(d)
+	diags = diags.Append(moreDiags)
+	if diags.HasErrors() {
 		return cty.NilVal, diags
 	}
-	configVal = newVal
 
-	configureDiags := b.Configure(configVal)
-	if configureDiags.HasErrors() {
-		diags = diags.Append(configureDiags.Err())
-		return cty.NilVal, diags
-	}
+	newState := make(map[string]cty.Value)
+	newState["backend"] = d.GetAttr("backend")
+	newState["config"] = d.GetAttr("config")
 
-	name := backend.DefaultStateName
+	workspaceName := backend.DefaultStateName
 
 	if workspaceVal := d.GetAttr("workspace"); !workspaceVal.IsNull() {
 		newState["workspace"] = workspaceVal
-		name = workspaceVal.AsString()
+		workspaceName = workspaceVal.AsString()
 	}
 
-	newState["workspace"] = cty.StringVal(name)
+	newState["workspace"] = cty.StringVal(workspaceName)
 
-	state, err := b.StateMgr(name)
+	state, err := b.StateMgr(workspaceName)
 	if err != nil {
 		diags = diags.Append(tfdiags.AttributeValue(
 			tfdiags.Error,
@@ -165,3 +151,69 @@ func dataSourceRemoteStateRead(d *cty.Value) (cty.Value, tfdiags.Diagnostics) {
 
 	return cty.ObjectVal(newState), diags
 }
+
+func getBackend(cfg cty.Value) (backend.Backend, tfdiags.Diagnostics) {
+	var diags tfdiags.Diagnostics
+
+	backendType := cfg.GetAttr("backend").AsString()
+
+	// Don't break people using the old _local syntax - but note warning above
+	if backendType == "_local" {
+		log.Println(`[INFO] Switching old (unsupported) backend "_local" to "local"`)
+		backendType = "local"
+	}
+
+	// Create the client to access our remote state
+	log.Printf("[DEBUG] Initializing remote state backend: %s", backendType)
+	f := backendInit.Backend(backendType)
+	if f == nil {
+		diags = diags.Append(tfdiags.AttributeValue(
+			tfdiags.Error,
+			"Invalid backend configuration",
+			fmt.Sprintf("There is no backend type named %q.", backendType),
+			cty.Path(nil).GetAttr("backend"),
+		))
+		return nil, diags
+	}
+	b := f()
+
+	config := cfg.GetAttr("config")
+	if config.IsNull() {
+		// We'll treat this as an empty configuration and see if the backend's
+		// schema and validation code will accept it.
+		config = cty.EmptyObjectVal
+	}
+
+	if config.Type().IsMapType() { // The code below expects an object type, so we'll convert
+		config = cty.ObjectVal(config.AsValueMap())
+	}
+
+	schema := b.ConfigSchema()
+	// Try to coerce the provided value into the desired configuration type.
+	configVal, err := schema.CoerceValue(config)
+	if err != nil {
+		diags = diags.Append(tfdiags.AttributeValue(
+			tfdiags.Error,
+			"Invalid backend configuration",
+			fmt.Sprintf("The given configuration is not valid for backend %q: %s.", backendType,
+				tfdiags.FormatError(err)),
+			cty.Path(nil).GetAttr("config"),
+		))
+		return nil, diags
+	}
+
+	newVal, validateDiags := b.PrepareConfig(configVal)
+	diags = diags.Append(validateDiags)
+	if validateDiags.HasErrors() {
+		return nil, diags
+	}
+	configVal = newVal
+
+	configureDiags := b.Configure(configVal)
+	if configureDiags.HasErrors() {
+		diags = diags.Append(configureDiags.Err())
+		return nil, diags
+	}
+
+	return b, diags
+}

builtin/providers/terraform/data_source_state_test.go

@@ -1,6 +1,7 @@
 package terraform
 
 import (
+	"github.com/hashicorp/terraform/tfdiags"
 	"testing"
 
 	"github.com/apparentlymart/go-dump/dump"
@@ -138,6 +139,80 @@ func TestState_basic(t *testing.T) {
 			}),
 			true,
 		},
+		"wrong type for config": {
+			cty.ObjectVal(map[string]cty.Value{
+				"backend": cty.StringVal("local"),
+				"config":  cty.StringVal("nope"),
+			}),
+			cty.NilVal,
+			true,
+		},
+		"wrong type for config with unknown backend": {
+			cty.ObjectVal(map[string]cty.Value{
+				"backend": cty.UnknownVal(cty.String),
+				"config":  cty.StringVal("nope"),
+			}),
+			cty.NilVal,
+			true,
+		},
+		"wrong type for config with unknown config": {
+			cty.ObjectVal(map[string]cty.Value{
+				"backend": cty.StringVal("local"),
+				"config":  cty.UnknownVal(cty.String),
+			}),
+			cty.NilVal,
+			true,
+		},
+		"wrong type for defaults": {
+			cty.ObjectVal(map[string]cty.Value{
+				"backend": cty.StringVal("local"),
+				"config": cty.ObjectVal(map[string]cty.Value{
+					"path": cty.StringVal("./test-fixtures/basic.tfstate"),
+				}),
+				"defaults": cty.StringVal("nope"),
+			}),
+			cty.NilVal,
+			true,
+		},
+		"config as map": {
+			cty.ObjectVal(map[string]cty.Value{
+				"backend": cty.StringVal("local"),
+				"config": cty.MapVal(map[string]cty.Value{
+					"path": cty.StringVal("./test-fixtures/empty.tfstate"),
+				}),
+			}),
+			cty.ObjectVal(map[string]cty.Value{
+				"backend": cty.StringVal("local"),
+				"config": cty.MapVal(map[string]cty.Value{
+					"path": cty.StringVal("./test-fixtures/empty.tfstate"),
+				}),
+				"defaults":  cty.NullVal(cty.DynamicPseudoType),
+				"outputs":   cty.EmptyObjectVal,
+				"workspace": cty.StringVal(backend.DefaultStateName),
+			}),
+			false,
+		},
+		"defaults as map": {
+			cty.ObjectVal(map[string]cty.Value{
+				"backend": cty.StringVal("local"),
+				"config": cty.ObjectVal(map[string]cty.Value{
+					"path": cty.StringVal("./test-fixtures/basic.tfstate"),
+				}),
+				"defaults": cty.MapValEmpty(cty.String),
+			}),
+			cty.ObjectVal(map[string]cty.Value{
+				"backend": cty.StringVal("local"),
+				"config": cty.ObjectVal(map[string]cty.Value{
+					"path": cty.StringVal("./test-fixtures/basic.tfstate"),
+				}),
+				"defaults": cty.MapValEmpty(cty.String),
+				"outputs": cty.ObjectVal(map[string]cty.Value{
+					"foo": cty.StringVal("bar"),
+				}),
+				"workspace": cty.StringVal(backend.DefaultStateName),
+			}),
+			false,
+		},
 	}
 	for name, test := range tests {
 		t.Run(name, func(t *testing.T) {
@@ -146,7 +221,15 @@ func TestState_basic(t *testing.T) {
 			if err != nil {
 				t.Fatalf("unexpected error: %s", err)
 			}
-			got, diags := dataSourceRemoteStateRead(&config)
+
+			diags := dataSourceRemoteStateValidate(config)
+
+			var got cty.Value
+			if !diags.HasErrors() && config.IsWhollyKnown() {
+				var moreDiags tfdiags.Diagnostics
+				got, moreDiags = dataSourceRemoteStateRead(config)
+				diags = diags.Append(moreDiags)
+			}
 
 			if test.Err {
 				if !diags.HasErrors() {
@@ -156,8 +239,8 @@ func TestState_basic(t *testing.T) {
 				t.Fatalf("unexpected errors: %s", diags.Err())
 			}
 
-			if !test.Want.RawEquals(got) {
-				t.Errorf("wrong result\nconfig: %sgot: %swant: %s", dump.Value(config), dump.Value(got), dump.Value(test.Want))
+			if test.Want != cty.NilVal && !test.Want.RawEquals(got) {
+				t.Errorf("wrong result\nconfig: %sgot:    %swant:   %s", dump.Value(config), dump.Value(got), dump.Value(test.Want))
 			}
 		})
 	}

builtin/providers/terraform/provider.go

@@ -40,11 +40,21 @@ func (p *Provider) PrepareProviderConfig(req providers.PrepareProviderConfigRequ
 }
 
 // ValidateDataSourceConfig is used to validate the data source configuration values.
-func (p *Provider) ValidateDataSourceConfig(providers.ValidateDataSourceConfigRequest) providers.ValidateDataSourceConfigResponse {
+func (p *Provider) ValidateDataSourceConfig(req providers.ValidateDataSourceConfigRequest) providers.ValidateDataSourceConfigResponse {
 	// FIXME: move the backend configuration validate call that's currently
 	// inside the read method  into here so that we can catch provider configuration
 	// errors in terraform validate as well as during terraform plan.
 	var res providers.ValidateDataSourceConfigResponse
+
+	// This should not happen
+	if req.TypeName != "terraform_remote_state" {
+		res.Diagnostics.Append(fmt.Errorf("Error: unsupported data source %s", req.TypeName))
+		return res
+	}
+
+	diags := dataSourceRemoteStateValidate(req.Config)
+	res.Diagnostics = diags
+
 	return res
 }
 
@@ -67,7 +77,7 @@ func (p *Provider) ReadDataSource(req providers.ReadDataSourceRequest) providers
 		return res
 	}
 
-	newState, diags := dataSourceRemoteStateRead(&req.Config)
+	newState, diags := dataSourceRemoteStateRead(req.Config)
 
 	res.State = newState
 	res.Diagnostics = diags