Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

instances.Expander: A utility for encapsulating "count" and "for_each" concerns #23462

merged 6 commits into from
Feb 14, 2020


Copy link

@apparentlymart apparentlymart commented Nov 22, 2019

Our existing handling of count and for_each for resources is rather scattered across different parts of Terraform.

This PR represents some initial experimentation towards centralizing the expansion logic for resources and for modules (looking forward to future module count and for_each) to help the different parts of Terraform Core that deal with the repetition constructs to coordinate with each other in a more organized way.

The Expander type includes three different families of methods:

  • Set* methods (SetResourceSingle, SetResourceCount, SetResourceForEach, SetModuleCount) are used to record the results of evaluating the count and for_each expressions in the configuration for later use by the Expand* family.

  • Expand* methods (ExpandResource and ExpandModule) take the addresses of static (unexpanded) objects in the configuration and return all of the instance addresses that result from the expansions that were recorded by earlier calls to the Set* methods.

    Because modules can nest inside other modules, the Expand* methods fully expand all levels of the tree and so the number of instances for a particular object can grow exponentially for objects in deeper modules.

  • Get*RepetitionData methods (GetResourceInstanceRepetitionData and GetModuleInstanceRepetitionData) take the absolute address of a particular module instance or resource instance identified by the Expand* family and return a decision about what values should appear for each.key, each.value, and count.index when evaluating the configuration for the identified instance.

Each family feeds into the next, so they must be called in a particular order for correct operation. The graph walk can take care of ensuring the correct ordering of calls so that data is always available before it is needed.

As an initial proof-of-concept I integrated instances.Expander in a rough way to replace some -- but not all -- of the scattered count and for_each logic for resources. This now explicitly registers that every module in the configuration is a singleton (count and for_each aren't supported there yet) and then registers the repetition mode of each resource, which then allows the DynamicExpand logic to just ask the expander what instances it ought to be creating.

This Expander thing is also intended to be responsible for deciding what goes in each.key, each.value, and count.index for expressions, but I wasn't able to wire that up just yet because the apply graph (which is pre-expanded with nodes representing resource instances, not resources) isn't interacting with expander quite right yet, and the codepaths that do expression evaluation all run in both the plan and apply phases.

In principle though, if the Expander is being populated properly on all walks, we would be able to get the values representing the current repetition using the GetResourceInstanceRepetitionData method, which takes an absolute resource address and returns an object giving the cty.Value to use for each.key, each.value and count.index.

If this were to be taken forward as the basis for module count and for_each, that would require us to do some rework on the way Terraform Core implements the plan walk so that it's clear that the nodes in the main plan graph represent configuration constructs belonging to unexpanded modules only (not to module instances), and add DynamicExpand to all of the nodes representing configuration constructs which calls into the Expander instance in order to find all of the module instances that the configuration construct is instantiated in. That would be a far more disruptive change, so I've not attempted that yet, but the new DynamicExpand implementation for resources is ready to deal with count and for_each on modules once the rest of the plan graph catches up.

This does appear to work and pass all the existing context tests without modification, but for the moment it's mainly intended as a conversation starter for whether this particular direction -- DynamicExpand on everything -- feels good for module count and for_each.

@apparentlymart apparentlymart self-assigned this Nov 22, 2019
@ghost ghost added the sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK label Nov 22, 2019
When ModuleInstanceStep values appear alone in debug messages, it's easier
to read them in a compact, HCL-like form than as the default struct
printing style.
This returns the address of the module that the module instance is an
instance of.
This package aims to encapsulate the module/resource repetition problem
so that Terraform Core's graph node DynamicExpand implementations can be

This is also a building block on the path towards module repetition, by
modelling the recursive expansion of modules and their contents. This will
allow the Terraform Core plan graph to have one node per configuration
construct, each of which will DynamicExpand into as many sub-nodes as
necessary to cover all of the recursive module instantiations.

For the moment this is just dead code, because Terraform Core isn't yet
updated to use it.
This is not used yet, but in future commits will be used as a
"blackboard" to centrally aggregate the information pertaining to
expansion of resources and modules (using "count" or "for_each") to help
ensure consistent treatment of the expansion process during a graph walk.

In practice this only really makes sense for the plan walk, because the
apply walk doesn't do any dynamic expansion.
This is a minimal integration of instances.Expander used just for resource
count and for_each, for now just forcing modules to always be singletons
because the rest of Terraform Core isn't ready to deal with expanding
module calls yet.

This doesn't integrate super cleanly yet because we still have some
cleanup work to do in the design of the plan walk, to make it explicit
that the nodes in the plan graph represent static configuration objects
rather than expanded instances, including for modules. To make this work
in the meantime, there is some shimming between addrs.Module and
addrs.ModuleInstance to correct for the discontinuities that result from
the fact that Terraform currently assumes that modules are always
We're not far enough along yet to be able to actually use the
RepetitionData instances provided by the instances package, but having
these types be considered identical will help us to gradually migrate over
as we prepare the rest of Terraform to properly populate the Expander.
@jbardin jbardin marked this pull request as ready for review February 14, 2020 20:11
Copy link

@jbardin jbardin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The module expander work is being based off this branch. Since this applies cleanly to master I think we can merge it now to reduce the diff and make rebasing easier.

@apparentlymart apparentlymart merged commit c02a898 into master Feb 14, 2020
@apparentlymart apparentlymart deleted the f-instance-expander branch February 14, 2020 23:20
Copy link

ghost commented Apr 1, 2020

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 ghost locked and limited conversation to collaborators Apr 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
core enhancement sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK
None yet

Successfully merging this pull request may close these issues.

None yet

3 participants