terraform: use hcl.MergeBodies instead of configs.MergeBodies for pro… (#29000)

* terraform: use hcl.MergeBodies instead of configs.MergeBodies for provider configuration

Previously, Terraform would return an error if the user supplied provider configuration via interactive input iff the configuration provided on the command line was missing any required attributes - even if those attributes were already set in config.

That error came from configs.MergeBody, which was designed for overriding valid configuration. It expects that the first ("base") body has all required attributes. However in the case of interactive input for provider configuration, it is perfectly valid if either or both bodies are missing required attributes, as long as the final body has all required attributes. hcl.MergeBodies works very similarly to configs.MergeBodies, with a key difference being that it only checks that all required attributes are present after the two bodies are merged.

I've updated the existing test to use interactive input vars and a schema with all required attributes. This test failed before switching from configs.MergeBodies to hcl.MergeBodies.

* add a command package test that shows that we can still have providers with dynamic configuration + required + interactive input merging

This test failed when buildProviderConfig still used configs.MergeBodies instead of hcl.MergeBodies

internal/command/plan_test.go

@@ -687,6 +687,98 @@ func TestPlan_providerArgumentUnset(t *testing.T) {
 	}
 }
 
+// Test that terraform properly merges provider configuration that's split
+// between config files and interactive input variables.
+// https://github.com/hashicorp/terraform/issues/28956
+func TestPlan_providerConfigMerge(t *testing.T) {
+	td := tempDir(t)
+	testCopyDir(t, testFixturePath("plan-provider-input"), td)
+	defer os.RemoveAll(td)
+	defer testChdir(t, td)()
+
+	// Disable test mode so input would be asked
+	test = false
+	defer func() { test = true }()
+
+	// The plan command will prompt for interactive input of provider.test.region
+	defaultInputReader = bytes.NewBufferString("us-east-1\n")
+
+	p := planFixtureProvider()
+	// override the planFixtureProvider schema to include a required provider argument and a nested block
+	p.GetProviderSchemaResponse = &providers.GetProviderSchemaResponse{
+		Provider: providers.Schema{
+			Block: &configschema.Block{
+				Attributes: map[string]*configschema.Attribute{
+					"region": {Type: cty.String, Required: true},
+					"url":    {Type: cty.String, Required: true},
+				},
+				BlockTypes: map[string]*configschema.NestedBlock{
+					"auth": {
+						Nesting: configschema.NestingList,
+						Block: configschema.Block{
+							Attributes: map[string]*configschema.Attribute{
+								"user":     {Type: cty.String, Required: true},
+								"password": {Type: cty.String, Required: true},
+							},
+						},
+					},
+				},
+			},
+		},
+		ResourceTypes: map[string]providers.Schema{
+			"test_instance": {
+				Block: &configschema.Block{
+					Attributes: map[string]*configschema.Attribute{
+						"id": {Type: cty.String, Optional: true, Computed: true},
+					},
+				},
+			},
+		},
+	}
+
+	view, done := testView(t)
+	c := &PlanCommand{
+		Meta: Meta{
+			testingOverrides: metaOverridesForProvider(p),
+			View:             view,
+		},
+	}
+
+	args := []string{}
+	code := c.Run(args)
+	output := done(t)
+	if code != 0 {
+		t.Fatalf("bad: %d\n\n%s", code, output.Stderr())
+	}
+
+	if !p.ConfigureProviderCalled {
+		t.Fatal("configure provider not called")
+	}
+
+	// For this test, we want to confirm that we've sent the expected config
+	// value *to* the provider.
+	got := p.ConfigureProviderRequest.Config
+	want := cty.ObjectVal(map[string]cty.Value{
+		"auth": cty.ListVal([]cty.Value{
+			cty.ObjectVal(map[string]cty.Value{
+				"user":     cty.StringVal("one"),
+				"password": cty.StringVal("onepw"),
+			}),
+			cty.ObjectVal(map[string]cty.Value{
+				"user":     cty.StringVal("two"),
+				"password": cty.StringVal("twopw"),
+			}),
+		}),
+		"region": cty.StringVal("us-east-1"),
+		"url":    cty.StringVal("example.com"),
+	})
+
+	if !got.RawEquals(want) {
+		t.Fatal("wrong provider config")
+	}
+
+}
+
 func TestPlan_varFile(t *testing.T) {
 	// Create a temporary working directory that is empty
 	td := tempDir(t)

internal/command/testdata/plan-provider-input/main.tf

@@ -0,0 +1,20 @@
+variable "users" {
+    default = {
+        one = "onepw"
+        two = "twopw"
+    }
+}
+
+provider "test" {
+    url = "example.com"
+    
+    dynamic "auth" {
+        for_each = var.users
+        content {
+            user     = auth.key
+            password = auth.value
+        }
+    }
+}
+
+resource "test_instance" "test" {}

internal/terraform/eval_provider.go

@@ -26,13 +26,7 @@ func buildProviderConfig(ctx EvalContext, addr addrs.AbsProviderConfig, config *
 	switch {
 	case configBody != nil && inputBody != nil:
 		log.Printf("[TRACE] buildProviderConfig for %s: merging explicit config and input", addr)
-		// Note that the inputBody is the _base_ here, because configs.MergeBodies
+		return hcl.MergeBodies([]hcl.Body{inputBody, configBody})
-		// expects the base have all of the required fields, while these are
-		// forced to be optional for the override. The input process should
-		// guarantee that we have a value for each of the required arguments and
-		// that in practice the sets of attributes in each body will be
-		// disjoint.
-		return configs.MergeBodies(inputBody, configBody)
 	case configBody != nil:
 		log.Printf("[TRACE] buildProviderConfig for %s: using explicit config only", addr)
 		return configBody

internal/terraform/node_provider_test.go

@@ -18,10 +18,23 @@ func TestNodeApplyableProviderExecute(t *testing.T) {
 	config := &configs.Provider{
 		Name: "foo",
 		Config: configs.SynthBody("", map[string]cty.Value{
-			"test_string": cty.StringVal("hello"),
+			"user": cty.StringVal("hello"),
 		}),
 	}
-	provider := mockProviderWithConfigSchema(simpleTestSchema())
+
+	schema := &configschema.Block{
+		Attributes: map[string]*configschema.Attribute{
+			"user": {
+				Type:     cty.String,
+				Required: true,
+			},
+			"pw": {
+				Type:     cty.String,
+				Required: true,
+			},
+		},
+	}
+	provider := mockProviderWithConfigSchema(schema)
 	providerAddr := addrs.AbsProviderConfig{
 		Module:   addrs.RootModule,
 		Provider: addrs.NewDefaultProvider("foo"),
@@ -34,8 +47,12 @@ func TestNodeApplyableProviderExecute(t *testing.T) {
 
 	ctx := &MockEvalContext{ProviderProvider: provider}
 	ctx.installSimpleEval()
-	if err := n.Execute(ctx, walkApply); err != nil {
+	ctx.ProviderInputValues = map[string]cty.Value{
-		t.Fatalf("err: %s", err)
+		"pw": cty.StringVal("so secret"),
+	}
+
+	if diags := n.Execute(ctx, walkApply); diags.HasErrors() {
+		t.Fatalf("err: %s", diags.Err())
 	}
 
 	if !ctx.ConfigureProviderCalled {
@@ -43,10 +60,17 @@ func TestNodeApplyableProviderExecute(t *testing.T) {
 	}
 
 	gotObj := ctx.ConfigureProviderConfig
-	if !gotObj.Type().HasAttribute("test_string") {
+	if !gotObj.Type().HasAttribute("user") {
-		t.Fatal("configuration object does not have \"test_string\" attribute")
+		t.Fatal("configuration object does not have \"user\" attribute")
 	}
-	if got, want := gotObj.GetAttr("test_string"), cty.StringVal("hello"); !got.RawEquals(want) {
+	if got, want := gotObj.GetAttr("user"), cty.StringVal("hello"); !got.RawEquals(want) {
+		t.Errorf("wrong configuration value\ngot:  %#v\nwant: %#v", got, want)
+	}
+
+	if !gotObj.Type().HasAttribute("pw") {
+		t.Fatal("configuration object does not have \"pw\" attribute")
+	}
+	if got, want := gotObj.GetAttr("pw"), cty.StringVal("so secret"); !got.RawEquals(want) {
 		t.Errorf("wrong configuration value\ngot:  %#v\nwant: %#v", got, want)
 	}
 }