From 096010600d75a334c9a77a3d4cd47d84ca34ba40 Mon Sep 17 00:00:00 2001 From: Kristin Laemmert Date: Fri, 25 Jun 2021 08:48:47 -0400 Subject: [PATCH] =?UTF-8?q?terraform:=20use=20hcl.MergeBodies=20instead=20?= =?UTF-8?q?of=20configs.MergeBodies=20for=20pro=E2=80=A6=20(#29000)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 | 92 +++++++++++++++++++ .../testdata/plan-provider-input/main.tf | 20 ++++ internal/terraform/eval_provider.go | 8 +- internal/terraform/node_provider_test.go | 38 ++++++-- 4 files changed, 144 insertions(+), 14 deletions(-) create mode 100644 internal/command/testdata/plan-provider-input/main.tf diff --git a/internal/command/plan_test.go b/internal/command/plan_test.go index d2012bdae1..84bf5f6ae8 100644 --- a/internal/command/plan_test.go +++ b/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) diff --git a/internal/command/testdata/plan-provider-input/main.tf b/internal/command/testdata/plan-provider-input/main.tf new file mode 100644 index 0000000000..4211ba3524 --- /dev/null +++ b/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" {} \ No newline at end of file diff --git a/internal/terraform/eval_provider.go b/internal/terraform/eval_provider.go index 0f257b8820..31f7b0453a 100644 --- a/internal/terraform/eval_provider.go +++ b/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 - // 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) + return hcl.MergeBodies([]hcl.Body{inputBody, configBody}) case configBody != nil: log.Printf("[TRACE] buildProviderConfig for %s: using explicit config only", addr) return configBody diff --git a/internal/terraform/node_provider_test.go b/internal/terraform/node_provider_test.go index 3551debb87..83ef7e1c16 100644 --- a/internal/terraform/node_provider_test.go +++ b/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 { - t.Fatalf("err: %s", err) + ctx.ProviderInputValues = map[string]cty.Value{ + "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") { - t.Fatal("configuration object does not have \"test_string\" attribute") + if !gotObj.Type().HasAttribute("user") { + 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) } }