From 003317d7c81a37d107a27af6156b6f746f2ab0a9 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 28 Mar 2019 10:07:16 -0700 Subject: [PATCH] lang: Detect references when a list/set attr is defined using blocks For compatibility with documented patterns from existing providers we are now allowing (via a pre-processing step) any attribute whose type is a list-of-object or set-of-object type to optionally be assigned using one or more blocks whose type is the attribute name. The pre-processing functionality was implemented in previous commits but we were not correctly detecting references within these blocks that are, from the perspective of the primary schema, invalid. Now we'll use an alternative implementation of variable detection that is able to apply the same schema rewriting technique we used to implement the transform and thus can find all of the references as if they were already in their final locations. --- lang/blocktoattr/schema.go | 3 + lang/references.go | 23 ++++-- terraform/graph_builder_plan_test.go | 74 +++++++++++++++++++ .../attr-as-blocks.tf | 8 ++ 4 files changed, 100 insertions(+), 8 deletions(-) create mode 100644 terraform/test-fixtures/graph-builder-plan-attr-as-blocks/attr-as-blocks.tf diff --git a/lang/blocktoattr/schema.go b/lang/blocktoattr/schema.go index b65663d53..161ce15d4 100644 --- a/lang/blocktoattr/schema.go +++ b/lang/blocktoattr/schema.go @@ -7,6 +7,9 @@ import ( ) func ambiguousNames(schema *configschema.Block) map[string]struct{} { + if schema == nil { + return nil + } ambiguousNames := make(map[string]struct{}) for name, attrS := range schema.Attributes { aty := attrS.Type diff --git a/lang/references.go b/lang/references.go index fab468a6e..d688477aa 100644 --- a/lang/references.go +++ b/lang/references.go @@ -1,10 +1,10 @@ package lang import ( - "github.com/hashicorp/hcl2/ext/dynblock" "github.com/hashicorp/hcl2/hcl" "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/configs/configschema" + "github.com/hashicorp/terraform/lang/blocktoattr" "github.com/hashicorp/terraform/tfdiags" ) @@ -51,14 +51,21 @@ func ReferencesInBlock(body hcl.Body, schema *configschema.Block) ([]*addrs.Refe if body == nil { return nil, nil } - spec := schema.DecoderSpec() - // We use dynblock.VariablesHCLDec instead of hcldec.Variables here because - // when we evaluate a block we'll apply the HCL dynamic block extension - // expansion to it first, and so we need this specialized version in order - // to properly understand what the dependencies will be once expanded. - // Otherwise, we'd miss references that only occur inside dynamic blocks. - traversals := dynblock.VariablesHCLDec(body, spec) + // We use blocktoattr.ExpandedVariables instead of hcldec.Variables or + // dynblock.VariablesHCLDec here because when we evaluate a block we'll + // first apply the dynamic block extension and _then_ the blocktoattr + // transform, and so blocktoattr.ExpandedVariables takes into account + // both of those transforms when it analyzes the body to ensure we find + // all of the references as if they'd already moved into their final + // locations, even though we can't expand dynamic blocks yet until we + // already know which variables are required. + // + // The set of cases we want to detect here is covered by the tests for + // the plan graph builder in the main 'terraform' package, since it's + // in a better position to test this due to having mock providers etc + // available. + traversals := blocktoattr.ExpandedVariables(body, schema) return References(traversals) } diff --git a/terraform/graph_builder_plan_test.go b/terraform/graph_builder_plan_test.go index afdbedd2e..2993b650c 100644 --- a/terraform/graph_builder_plan_test.go +++ b/terraform/graph_builder_plan_test.go @@ -152,6 +152,80 @@ test_thing.c } } +func TestPlanGraphBuilder_attrAsBlocks(t *testing.T) { + provider := &MockProvider{ + GetSchemaReturn: &ProviderSchema{ + ResourceTypes: map[string]*configschema.Block{ + "test_thing": { + Attributes: map[string]*configschema.Attribute{ + "id": {Type: cty.String, Computed: true}, + "nested": { + Type: cty.List(cty.Object(map[string]cty.Type{ + "foo": cty.String, + })), + Optional: true, + }, + }, + }, + }, + }, + } + components := &basicComponentFactory{ + providers: map[string]providers.Factory{ + "test": providers.FactoryFixed(provider), + }, + } + + b := &PlanGraphBuilder{ + Config: testModule(t, "graph-builder-plan-attr-as-blocks"), + Components: components, + Schemas: &Schemas{ + Providers: map[string]*ProviderSchema{ + "test": provider.GetSchemaReturn, + }, + }, + DisableReduce: true, + } + + g, err := b.Build(addrs.RootModuleInstance) + if err != nil { + t.Fatalf("err: %s", err) + } + + if g.Path.String() != addrs.RootModuleInstance.String() { + t.Fatalf("wrong module path %q", g.Path) + } + + // This test is here to make sure we properly detect references inside + // the "nested" block that is actually defined in the schema as a + // list-of-objects attribute. This requires some special effort + // inside lang.ReferencesInBlock to make sure it searches blocks of + // type "nested" along with an attribute named "nested". + actual := strings.TrimSpace(g.String()) + expected := strings.TrimSpace(` +meta.count-boundary (EachMode fixup) + provider.test + test_thing.a + test_thing.b +provider.test +provider.test (close) + provider.test + test_thing.a + test_thing.b +root + meta.count-boundary (EachMode fixup) + provider.test (close) +test_thing.a + provider.test +test_thing.b + provider.test + test_thing.a +`) + if actual != expected { + t.Fatalf("expected:\n%s\n\ngot:\n%s", expected, actual) + } +} + func TestPlanGraphBuilder_targetModule(t *testing.T) { b := &PlanGraphBuilder{ Config: testModule(t, "graph-builder-plan-target-module-provider"), diff --git a/terraform/test-fixtures/graph-builder-plan-attr-as-blocks/attr-as-blocks.tf b/terraform/test-fixtures/graph-builder-plan-attr-as-blocks/attr-as-blocks.tf new file mode 100644 index 000000000..d154cc264 --- /dev/null +++ b/terraform/test-fixtures/graph-builder-plan-attr-as-blocks/attr-as-blocks.tf @@ -0,0 +1,8 @@ +resource "test_thing" "a" { +} + +resource "test_thing" "b" { + nested { + foo = test_thing.a.id + } +}