A "unknown_instances" language experiment, allowing unknown values in count and for_each #34561
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is early work toward supporting the handling of named values inside incompletely-expanded module prefixes, which is not yet possible but will become possible in future once we allow unknown values in count and for_each expressions inside module blocks. These new stub nodes don't do anything yet. The goal here is just to start staking out in preparation for future work, since support for unknown values in expansion arguments will arrive over quite a few later commits.
This exposes the set of active language experiments to graph node Execute and DynamicExpand methods, so that they can vary their behavior when certain experiments are active.
This doesn't do anything yet as of this commit, but in future commits it will gate the possibility for count and for_each arguments to have unknown values, which will then cause affected objects to have their planning deferred to a future run.
This is currently an opt-in argument, with no callers (except some tests) using it. Therefore this should not change any externally-visible behavior yet. Future commits will gradually incorporate support for modules and resources whose expansion isn't yet known, which will then cause planning of anything downstream of them to be deferred to a future run.
When the unknown_instances language experiment is active, Terraform will accept unknown values in count and for_each arguments for resource, data, and module blocks. The unknown-ness of the expansion will be registered with the instance expander for later use, but much of the rest of the modules runtime is not yet equipped to deal with that situation, and so as of this commit the experiment will just cause some broken behavior if used. There are therefore no integration tests for this behavior just yet, because there's no reasonable expected behavior for such tests to assert. Behavior when the experiment is not enabled should remain unchanged. Proper handling of the unknown-expansion situation will develop over subsequent commits, along with integration tests once there's enough in place to write them, with this experiment being concluded only after all of that work has completed.
apparentlymart
added
enhancement
core
unknown-values
jbardin
approved these changes
Jan 24, 2024
|
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This set of changes aims to finally expose some of the previously-unreachable codepaths dealing with modules or resources that have unknown values as their
countorfor_each.The work here is far from done, but I've reached the point where I can no longer continue just adding dead code that isn't actually exercisable, and so this PR introduces a new language experiment named
unknown_instanceswhich, if enabled, will cause the expansion logic to tolerate unknown values in the relevant arguments and register the unknown-ness with the instance expander.As usual with experiments, this is available only in dev and alpha builds. Even in alpha builds, it's enabled only when explicitly enabled for the modules that would rely on it. If the next minor release comes before the remaining work is completed, then this unfinished stuff will not actually be reachable in beta, RC, or final releases in that new series.
For now, turning this on just gives you a way to break yourself, because the rest of the modules runtime is not yet equipped to deal with this situation. I will continue to wire this in to the rest of the runtime over subsequent PRs, so that this experiment will eventually be guarding a working feature rather than a broken one.
Concretely, using this experiment to provide an unknown
for_eachorcountas of this PR will:Cause some no-op graph nodes to get added to the dynamic subgraphs of the nodes representing input variables, output values, and local values.
These nodes don't yet implement
Execute, so they won't do anything at all when visited. They're included here just to illustrate how the instance expander API models unknown expansion and how ourDynamicExpandfunctions will make use of that API.Make any resource that has unknown expansion (either directly or via a containing module) be essentially ignored during graph walk, because the resource
DynamicExpanddoesn't yet know it should go looking for unexpanded instance sets and so will not add graph nodes for them at all.(The final approach for this will have a similar shape to the handling of unexpanded named values in this PR, but with some extra complexity to make sure that any objects already known in the state will get refreshed even though their planning will eventually get deferred.)
Due to the above, also cause some expression evaluation to produce strange results, because the expression-reference evaluation logic largely has no awareness yet of how to decide placeholder values for objects whose evaluation was deferred.
This is yet another incremental step toward resolving #30937, but there are many steps remaining after this one. Since this is just an intermediate step and not a useful final state, there aren't any new integration tests here just yet. I'll add those in later PRs once there's actually some working behavior to test.
I suggest reviewing this on a commit-by-commit basis, because there's some distracting noise in the diff from updating some callers and I expect it'll be easier to understand what that's all about when viewing just the commit that introduced it.