Actions enhancements rollup#38668
Draft
jbardin wants to merge 35 commits into
Draft
Conversation
Actions need to execute from the resource instance. To represent that more directly, we're embedding the triggers in the resource, and then referencing the action nodes from those triggers. Actions nodes are only left in the graph as a representation of the configuration. This allows for those nodes to be used to resolve their own providers using the same methods as other nodes, while then be used from within the resource later. squashed merge of #38547
Because actions are written outside of resource, but are effectively evaluated like providers, we create a "caller" object which is a pseudo-alias for "self". The word "self" would be confusing since it typically refers to the surrounding object, but there is no surrounding object in this context. The term "caller" makes it more clear that there is a special object coming from outside of the context of the action config block.
The "self" and "caller" object are evaluated the same internally, but we need different diagnostic text for each.
We need to thread "caller" through all action evaluation. We do this by using the same mechanism as "self", since this is effectively just "self at a distance". That means we can leverage all the same mechanisms built around "self" and only need to change some diagnostic output to match the context. In order to validate action blocks however, it now means there needs to be a `caller` supplied, so we can't validate them before we get to the triggering resource. In order to do that we manually validate actions from the triggering node, and only insert a `NodeValidateAction` for standalone actions that are not referenced otherwise.
Check that caller is valid for trigger resource Check that caller is invalid for standalone action Rename which was incorrectly named, and use to to verify that old direct references still work as expected.
Yes it is true that the order here would matter if the resource is otherwise being evaluated, but that happens in many cases throughout the codebase, and we already rely on having a correct graph to ensure correct ordering of operations. Calling this out here with a FIXME is needlessly specific when it's actually a question about the overall architecture, and likely to produce a change which doesn't have any better global guarantees.
Remove unnecessary err checks. Check yield returns to break from loop.
While action nodes are not being evaluated directly during the graph walk, it's useful to record their expansion information ahead of time. This will also allow resource instances to know if they are being deferred by an unknown action expansion before the resource is planned.
We still may not make use of all the deferral code around actions, but at least make it work with the existing implementation.
This copies the same logic as the partially expanded resource
ShouldDeferActionInvocation didn't check if the action expansion itself was previously deferred.
The logic checking for action deferral was using the check itself as a way to detect if an action was being invoked, and considered it an error if the action config was already deferred. This was opposite the other methods, which check for more general deferrals that would defer the instance. Allow action expansion to be deferred. This is unavoidable when deferrals are globally allowed, so we must deal with it. A previously deferred action config, or an action which reports that its provider was deferred must also defer any calling resource.
These are valid values, and excluding them is only a superficial attempt at preventing unknown values.
Before actions we not evaluating the caller correctly, and would end up with the prior state rather than the current planned state in the evaluation context. After the final plan is created, we need to update the stored change so that all known values are present. We also need to update the working state to indicate there is a change. This was never needed before, because there was never a situation where an object was evaluated during apply, but before that object was itself applied.
While this is also technically possible to use correctly, more often than not it's an error, so prevent it for now.
This should ever have been valid
Refactor some of the action ref parsing, and remove redundant code. Make sure we can validate when action indexes are incorrect or missing.
The action response diagnostics were being munged into simple hcl diagnostics instead of using the usual attribution mechanisms. The fact that actions have an optional config block means we have to doublecheck that every time, since we don't want to apply the diagnostic to the wrong block in case the provider returns any attribute paths with the diagnostic.
Actions were initially allowed to refer back to the trigger resource via their full address. In order to do that terraform needs to specifically catch the reference and remove the cycle it causes. If this reference is indirected in any way, we lose that ability and the apply will fail with a graph cycle. We've since introduced the "caller" object, which is a alias for "self" from the triggering resource. Detect when the user has an action config which should be using "caller" instead of a resource address, and present a warning about the old syntax.
Fill in some missing targeting logic for actions and ConfigResource. Find and record callers for invoke nodes, so `caller` can be evaluated without having to process the entire calling resource. Add the caller address to the invoke structures, and store it in the planfile. This lets us get our caller reference automatically during apply, without having to search through all nodes for resource action triggers again.
Member
Author
|
Turns out the temporary handling of evaluation in |
Destroy actions can now be used in resources, with some constraints in place. To avoid dependency cycles we must fully plan the action. This means that the action config and the triggering condition are completely known at plan time, so that the stored values can be used during apply. This also means that ephemeral values cannot be used in destroy action configuration, because they are not guaranteed to be the same during apply.
The nested maps for requested providers weren't required, and the recent refactoring made it more apparent that the nested would have been overwritten even if multiple providers were allowed. Action providers were not being connected, and because we need to avoid using them as resolved providers, we need to connect those in a separate loop. For now we can accomplish that by capturing the existing loop as a closure, and running in twice for each map.
Pass in a correctly-typed caller value for static validation.
Action config block evaluation from within destroy context
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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 merges the actions work feature branch, which consists of the previous PRs:
#38547
#38572
#38618
#38644
#38654
#38657
#38658
#38664
Most of this was internal refactoring, but there are a few notable external changes:
before_actions can now be usedcallersymbol when called from a resourceaction_trigger. This new symbol contains the object value from the calling resource to avoid dependency cycle problems inherent with indirect references.Fixes: #37975
Fixes: #37927