Fixes for ignore_changes with unintended provider behavior #27141
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
The cty.Transform for ignore_changes could return early when building a map that had multiple ignored keys. Refactor the function to try and separate the fast-path a little better, and hopefully make it easier to follow.
Because we allow legacy providers to depart from the contract and return changes to non-computed values, the plan response may have altered values that were already suppressed with ignore_changes. A prime example of this is where providers attempt to obfuscate config data by turning the config value into a hash and storing the hash value in the state. There are enough cases of this in existing providers that we must accommodate the behavior for now, so for ignore_changes to work at all on these values, we will revert the ignored values once more on the planned state.
The ignore_changes option `all` can cause computed attributes to show up in the validation configuration, which will be rejected by the provider SDK. Validate the config before applying the ignore_changes options. In the future it we should probably have a way for processIgnoreChanges to skip computed values based on the schema. Since we also want a way to more easily query the schema for "computed-ness" to validate the ignore_changes arguments for computed values, we can fix these at the same time with a future change to configschema. This will most likely require some sort of method to retrieve info from the configschema.Block via cty.Path, which we cannot easily do right now.
Codecov Report
|
mildwonkey
approved these changes
Dec 4, 2020
| // Because we allow legacy providers to depart from the contract and | ||
| // return changes to non-computed values, the plan response may have | ||
| // altered values that were already suppressed with ignore_changes. | ||
| // A prime example of this is where providers attempt to obfuscate |
There was a problem hiding this comment.
thank you for providing a "good" example in the comments, this is really helpful
|
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
locked as resolved and limited conversation to collaborators
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 covers 3 separate, but related issues that are interacting badly with
ignore_changesand legacy providers.The first is a more straightforward bug, where multiple entries for map keys in
ignore_changescould causecty.Transformto return early, leaving some values unchanged. Refactor the function to try and separate the fast-path a little better, and hopefully make it easier to follow.The second is a side effect of some incorrect provider behavior, but we need to work with it to remain compatible with the legacy SDK. Because we allow legacy providers to depart from the contract and return changes to non-computed values, the plan response may have altered values that were already suppressed with ignore_changes. A prime example of this is where providers attempt to obfuscate config data by turning the config value into a hash and storing the hash value in the state. There are enough cases of this in existing providers that we must accommodate the behavior for now, so for ignore_changes to work at all on these values we re-apply
ignore_changesto the planned state.The effect of this will be limited by the
LegacyTypeSystemflag, so as not to introduce more unintendedignore_changesbehavior with new providers. The primary use case above won't be able to happen with providers using a new SDK, because returning a non-conforming plan will be an error.The third issue concerns the use of the
ignore_changes = alloption, which when applied to the configuration will currently cause computed values to be populated into the temporary config value. This isn't a problem when generating the plan, but does effect providers' validation, as the legacy provider schema will reject these values. We can work around this by validating the original config before applyingignore_changes.(In the future it we should probably have a way for
processIgnoreChangesto skip computed values based on the schema. Since we also want a way to more easily query the schema for "computed-ness" to statically validate theignore_changesarguments, we can fix these at the same time in a future change. This will most likely require some sort of method to retrieve info from theconfigschema.Blockvia acty.Path, which we cannot easily do right now.)Fixes #27106
Fixes #27103
Fixes #27097