Module Expansion Activate! #24461

jbardin · 2020-03-25T19:46:10Z

This turns on module expansion in the config, with most basic functionality working. A major exception is that while providers cannot be configured within an expanded module, there is no validation for that yet and the results are going to be undefined.

There are some UnkeyedInstanceShim calls are left in the abstract provider, because providers need to be evaluated before expansion happens. This is OK, since we will pre-validate that providers cannot be configured within expanding modules. There is also a call in resource validation, because we need to create a context within which we can validate, but there is no expansion to create module instances. This type of usage of the UnkeyedInstanceShim will likely remain for the time being.

To link the bulk of the work together here, the series of PRs directly leading to this is #24154, #24296, #24331, #24346, #24362, #24389, #24444, #24454.

Other key points in this PR:

Add EvalContext.WithPath

As the Graph is walked, the current way to set the context path was to
have the walker return a context from EnterPath. This required that
every node know it's absolute path, which can no longer be the case
when modules have not been expanded.

This introduces a new method called WithPath, which returns a copy of
the context with the internal path updated to reflect the method
argument. Any use of the EvalContext that requires knowing the path will
now panic if it wasn't explicitly set to ensure that evaluations always occur
in the correct path. While this may be beneficial for development, this
should be converted to a contextual error later on, so in the rare event that
we evaluate an unknown path the user can report the error in a useful way.

Add EvalContext to the GraphWalker interface.

EvalContext returns an EvalContext that has not yet set a path. This
will allow us to enforce that all context operations requiring a module
instance path will require that a path be explicitly set rather than
evaluating within the wrong path.

Add separate expand nodes to all Resource nodes that will need expansion.

While the Expander itself now handles the recursive expansion of
modules, Resources still need to be expanded twice, because
the evaluation of the Resource, which entails evaluating the for_each or
count expressions, is separate from the ResourceInstance expansion.
Adding expander nodes ensures that the individual eval nodes can be
created with the correct path for evaluation.

Track dependencies via addr.ConfigResource

We don't have access to module instances during the full graph transformations,
so we can't record these in the state. Using addrs.ConfigResource won't change
the current behavior, but makes the dependencies tracking less precise when
instances are connected to resources in other modules. We may likely revisit
this once we can ensure Refresh no longer needs to evaluate Resource
configuration.

Implements #17519, but leaving open until we are more feature complete.

As the Graph is walked, the current way to set the context path was to have the walker return a context from EnterPath. This required that every node know it's absolute path, which can no longer be the case during plan when modules have not been expanded. This introduces a new method called WithPath, which returns a copy of the context with the internal path updated to reflect the method argument. Any use of the EvalContext that requires knowing the path will now panic if it wasn't explicitly set to ensure that evaluations always occur in the correct path. Add EvalContext to the GraphWalker interface. EvalContext returns an EvalContext that has not yet set a path. This will allow us to enforce that all context operations requiring a module instance path will require that a path be explicitly set rather than evaluating within the wrong path.

While the Expander itself now handles the recursive expansion of modules, Resources themselves still need to be expanded twice, because the evaluation of the Resource, which entails evaluating the for_each or count expressions, is separate from the ResourceInstance expansion. Add a nodeExpandPlannableResource to do handle this expansion to allow all NodePlannableResources to call EvalWriteResourceState with an absolute address.

Resources also need to be expanded during apply, which cannot be done via EvalTree due to the lack of EvalContext.

Use the new addrs type here. Also remove the uniqueMap from the config transformer. We enforce uniqueness during config loading, and this is more likely to have false positives due to stringification than anything.

We can't get module instances during transformation, so we need to reduce the Dependencies to using `addrs.ConfigResource` for now.

Remove the shims where they aren't necessary from the Init and Close provider nodes. This also removed some provider path checks from the builtin eval context, which cannot be resolved since the context may not be created with a ModuleInstance path.

The expand logic was separated into nodeExpandRefreshableManagedResource, but the orphan logic wasn't updated.

simplify the test a bit and add a few more combinations to the config

eval nodes no longer always have a context path

mildwonkey · 2020-03-26T12:39:51Z

terraform/node_data_refresh.go

+	case forEachMap != nil:
+		expander.SetResourceForEach(n.Addr.Module, n.Addr.Resource, forEachMap)
+	default:
+		expander.SetResourceSingle(n.Addr.Module, n.Addr.Resource)


Question: what is the expected behavior when SetResourceSingle is called when count = 0 ?

Ah! I found the answer in a comment just below this (and was misreading the actual condition, still on my first ☕️):

// -1 signals "count not set"

yup, count = 0 is still in list-mode, just with 0 instances.

mildwonkey

This looks reasonable to me, though I suggest you get a second review with someone with more context :)

pselle

Mostly 🎉 and a comment about where comment might be improved/more understanding left for Future Readers

pselle · 2020-03-26T14:58:28Z

terraform/context_plan_test.go

-
-	m := testModule(t, "plan-modules-count")
+func TestContext2Plan_moduleExpand(t *testing.T) {
+	// Test a smattering of plan expansion behavior


smattering!

pselle · 2020-03-26T15:10:09Z

terraform/node_module_expand.go

-	if countDiags.HasErrors() {
-		return nil, countDiags.Err()
-	}
+	// nodeExpandModule itself does not have visibility into how its ancestors


helpful comment!

pselle · 2020-03-26T15:10:36Z

terraform/node_resource_abstract.go

 // NewNodeAbstractResource creates an abstract resource graph node for
 // the given absolute resource address.
-func NewNodeAbstractResource(addr addrs.AbsResource) *NodeAbstractResource {
-	// FIXME: this should probably accept a ConfigResource


And now it does!

pselle · 2020-03-26T15:11:54Z

terraform/node_resource_apply.go

+// nodeExpandApplyableResource handles the first layer of resource
+// expansion during apply. This is required because EvalTree does not have a
+// context with which to expand the resource into multiple instances.
+// This type should be a drop in replacement for NodeApplyableResource, and


As a "drop-in replacement" is the implication that it is used instead of NodeApplyableResource in certain scenarios, but not all? And what are those scenarios? (something that might be helpful to clarify here)

Ah good point, I do that too often in comments. I've already done the replacement, and the comment should only explain the "why"

pselle · 2020-03-26T15:12:29Z

terraform/node_resource_apply_instance.go

@@ -100,12 +100,7 @@ func (n *NodeApplyableResourceInstance) References() []*addrs.Reference {

 // GraphNodeAttachDependencies
 func (n *NodeApplyableResourceInstance) AttachDependencies(deps []addrs.ConfigResource) {
-	var shimmed []addrs.AbsResource
-	for _, r := range deps {
-		shimmed = append(shimmed, r.Absolute(r.Module.UnkeyedInstanceShim()))


This name method won't be called in the full graph, and remove it to prevent confusion with the parent node in logs.

n10000k · 2020-04-26T01:47:13Z

Waiting on this bad boy, in need of for_each on modules urgently </3 going to save so much time and can just pass an object of microservices for lambda and let it all take cause and build up like magic. <3 Keep up the good work everyone.

ghost · 2020-04-26T01:50:19Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

ghost added the sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK label Mar 25, 2020

jbardin requested a review from a team March 25, 2020 20:13

jbardin added 14 commits March 25, 2020 17:03

create nodeExpandApplyableResource

23cebc5

Resources also need to be expanded during apply, which cannot be done via EvalTree due to the lack of EvalContext.

NewNodeAbstractResource accepts a ResourceConfig

0b85eea

Use the new addrs type here. Also remove the uniqueMap from the config transformer. We enforce uniqueness during config loading, and this is more likely to have false positives due to stringification than anything.

Add Path to more nodes that require it.

74d85aa

create refresh node expanders

0afa371

use addrs.ConfigResource for dependency tracking

b3fc0da

We can't get module instances during transformation, so we need to reduce the Dependencies to using `addrs.ConfigResource` for now.

cleanup some expanders

7f0199b

orphan resources needs to use AbsResource

8eb3f2c

The expand logic was separated into nodeExpandRefreshableManagedResource, but the orphan logic wasn't updated.

enable count and for_each in configuration

cd045f6

module expansion test

04a117b

simplify the test a bit and add a few more combinations to the config

don't log path in EvalRaw

5810261

eval nodes no longer always have a context path

comment updates

4f1692c

jbardin force-pushed the jbardin/eval-context-path branch from 5e9b2fb to 4f1692c Compare March 25, 2020 21:07

mildwonkey reviewed Mar 26, 2020

View reviewed changes

mildwonkey approved these changes Mar 26, 2020

View reviewed changes

pselle approved these changes Mar 26, 2020

View reviewed changes

comment update and remove extra Name method

cec989d

This name method won't be called in the full graph, and remove it to prevent confusion with the parent node in logs.

jbardin merged commit 34cab3b into master Mar 26, 2020

jbardin deleted the jbardin/eval-context-path branch March 26, 2020 16:45

gmalkov mentioned this pull request Apr 1, 2020

split framework module broadinstitute/terraform-ap-modules#5

Merged

ghost locked and limited conversation to collaborators Apr 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Module Expansion Activate! #24461

Module Expansion Activate! #24461

jbardin commented Mar 25, 2020 •

edited

Loading

mildwonkey Mar 26, 2020

mildwonkey Mar 26, 2020 •

edited

Loading

jbardin Mar 26, 2020 •

edited

Loading

mildwonkey left a comment

pselle left a comment

pselle Mar 26, 2020

pselle Mar 26, 2020

pselle Mar 26, 2020

pselle Mar 26, 2020

jbardin Mar 26, 2020

pselle Mar 26, 2020

n10000k commented Apr 26, 2020

ghost commented Apr 26, 2020

Module Expansion Activate! #24461

Module Expansion Activate! #24461

Conversation

jbardin commented Mar 25, 2020 • edited Loading

mildwonkey Mar 26, 2020

Choose a reason for hiding this comment

mildwonkey Mar 26, 2020 • edited Loading

Choose a reason for hiding this comment

jbardin Mar 26, 2020 • edited Loading

Choose a reason for hiding this comment

mildwonkey left a comment

Choose a reason for hiding this comment

pselle left a comment

Choose a reason for hiding this comment

pselle Mar 26, 2020

Choose a reason for hiding this comment

pselle Mar 26, 2020

Choose a reason for hiding this comment

pselle Mar 26, 2020

Choose a reason for hiding this comment

pselle Mar 26, 2020

Choose a reason for hiding this comment

jbardin Mar 26, 2020

Choose a reason for hiding this comment

pselle Mar 26, 2020

Choose a reason for hiding this comment

n10000k commented Apr 26, 2020

ghost commented Apr 26, 2020

jbardin commented Mar 25, 2020 •

edited

Loading

mildwonkey Mar 26, 2020 •

edited

Loading

jbardin Mar 26, 2020 •

edited

Loading