Detect when provider and resource/module have identical for_each #2186
+628
−36
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
|
Reminder for the PR assignee: If this is a user-visible change, please update the changelog as part of the PR. |
cam72cam
force-pushed
the
300_foot_gun_detect
branch
from
72028a4
to
67d0b99
Compare
Signed-off-by: Christian Mesh <[email protected]>
cam72cam
force-pushed
the
300_foot_gun_detect
branch
from
67d0b99
to
0ba6a46
Compare
Signed-off-by: Christian Mesh <[email protected]>
Signed-off-by: Christian Mesh <[email protected]>
Co-authored-by: Martin Atkins <[email protected]> Signed-off-by: Christian Mesh <[email protected]>
Signed-off-by: Christian Mesh <[email protected]>
cam72cam
force-pushed
the
300_foot_gun_detect
branch
from
4720e52
to
6f484ab
Compare
Signed-off-by: Christian Mesh <[email protected]>
23 tasks
| // resembles HCL native syntax, and is suitable for display in the UI. | ||
| // | ||
| // This was copied (and simplified) from internal/command/views/json/diagnostic.go. | ||
| func TraversalStr(traversal hcl.Traversal) string { |
There was a problem hiding this comment.
I didn't suggest this originally because I was suggesting to use reference-based comparison instead, but if we're going to continue to compare purely on a raw traversal basis (accepting that this won't deal with any normalization that addrs.ParseRef would perform) then I think it's worth writing an actual comparison function that doesn't rely on a string representation, since string representation is a little messy for numbers in particular.
For example
func TraversalsEquivalent(a, b hcl.Traversal) bool {
if len(a) != len(b) {
return false
}
for idx, stepA := range a {
stepB := b[idx]
switch stepA := step.(type) {
case hcl.TraverseRoot:
stepB, ok := stepB.(hcl.TraverseRoot)
if !ok || stepA.Name != stepB.Name {
return false
}
case hcl.TraverseAttr:
stepB, ok := stepB.(hcl.TraverseAttr)
if !ok || stepA.Name != stepB.Name {
return false
}
case hcl.TraverseIndex:
stepB, ok := stepB.(hcl.TraverseIndex)
if !ok || stepA.Key.Equals(stepB.Key) != cty.True {
return false
}
default:
// The above should be exhaustive for all traversal
// step types that HCL can possibly generate. We'll
// treat any unsupported stepA types as non-equal
// because that matches what would happen if
// any stepB were unsupported: the type assertions
// in the above cases would fail.
return false
}
}
return true
}Compared to comparing by string representation this only significantly changes the handling of hcl.TraverseIndex, performing the equality test using cty's definition of equal, which therefore matches HCL's == operator. cty's equality test for number and string types both carefully handle some subtle situations that arise from number being a big float and string comparison honoring unicode normalization rules.
This addition documents the warning we plan to introduce to help module authors avoid a trap where they would be unable to successfully remove a dynamically-declared provider instance at the same time as managed resource instances that belong to it. Signed-off-by: Martin Atkins <[email protected]>
apparentlymart
force-pushed
the
300_foot_gun_detect
branch
from
5f9923c
to
b0e3c37
Compare
Signed-off-by: Christian Mesh <[email protected]>
Signed-off-by: Christian Mesh <[email protected]>
| @@ -852,8 +856,8 @@ func providerIterationIdenticalWarning(blockType string, sourceExpr, instanceExp | |||
| Severity: hcl.DiagWarning, | |||
| Summary: "Provider configuration for_each matches " + blockType, | |||
| Detail: fmt.Sprintf( | |||
| "This provider configuration uses the same for_each expression as a %s, which means that subsequent removal of elements from this collection would cause a planning error.\n\nOpenTofu relies on a provider instance to destroy resource instances that are associated with it, and so the provider instance must outlive all of its resource instances by at least one plan/apply round. For removal of instances to succeed in future you must structure the configuration so that the provider block's for_each expression can produce a superset of the instances of the resources associated with the provider configuration. Refer to the OpenTofu documentation for specific suggestions.", | |||
| blockType, | |||
| "This provider configuration uses the same for_each expression as a %s, which means that subsequent removal of elements from this collection would cause a planning error.\n\nOpenTofu relies on a provider instance to destroy resource instances that are associated with it, and so the provider instance must outlive all of its resource instances by at least one plan/apply round. For removal of instances to succeed in future you must structure the configuration so that the provider block's for_each expression can produce a superset of the instances of the resources associated with the provider configuration. Refer to the OpenTofu documentation for specific suggestions.\n\nIf you are stuck trying to remove a configured provider instance, targeting may be used: tofu apply -destroy %s", | |||
There was a problem hiding this comment.
Suggested change
| "This provider configuration uses the same for_each expression as a %s, which means that subsequent removal of elements from this collection would cause a planning error.\n\nOpenTofu relies on a provider instance to destroy resource instances that are associated with it, and so the provider instance must outlive all of its resource instances by at least one plan/apply round. For removal of instances to succeed in future you must structure the configuration so that the provider block's for_each expression can produce a superset of the instances of the resources associated with the provider configuration. Refer to the OpenTofu documentation for specific suggestions.\n\nIf you are stuck trying to remove a configured provider instance, targeting may be used: tofu apply -destroy %s", | |
| "This provider configuration uses the same for_each expression as a %s, which means that subsequent removal of elements from this collection would cause a planning error.\n\nOpenTofu relies on a provider instance to destroy resource instances that are associated with it, and so the provider instance must outlive all of its resource instances by at least one plan/apply round. For removal of instances to succeed in future you must structure the configuration so that the provider block's for_each expression can produce a superset of the instances of the resources associated with the provider configuration. Refer to the OpenTofu documentation for specific suggestions.\n\nTo destroy this object before removing the provider configuration, consider first performing a targeted destroy:\n tofu apply -destroy %s", |
The existing "error message" voice avoids using passive voice and tends to put example commands to run indented on their own line so that they are easier to copy-paste correctly.
This function returns true if the two given traversals are "equivalent", which means that HCL would consider them to represent the same information even if they aren't using exactly the same syntax. In particular it deals with some quirks in how HCL (via cty) decides whether two index values are equal. Signed-off-by: Martin Atkins <[email protected]>
We now have a function directly intended for comparing two traversals for equivalence, so we no longer need to cheat and use comparison of the string representations that were intended for use only in the UI. Signed-off-by: Martin Atkins <[email protected]>
apparentlymart
force-pushed
the
300_foot_gun_detect
branch
from
deccbfe
to
99cfa66
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
As discussed in #300, users can accidentally shoot themselves in the foot by using the same expressions for provider for_each and module/resource for_each. It creates a situation where removing a key from the for_each value will orphan resources without a corresponding provider.
Resolves #300
Target Release
1.9.0
Checklist
Go checklist
Website/documentation checklist