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

Handle marks on ignore_changes values #29193

Merged
merged 4 commits into from
Jul 21, 2021
Merged

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Jul 19, 2021

Up until now marks were not considered by ignore_changes, that however
means changes to sensitivity within a configuration cannot be ignored, even
though they are planned as changes.

Rather than separating the marks and tracking their paths, we can easily
update the processIgnoreChanges routine to handle the marked values
directly. Moving the processIgnoreChanges call also cleans up some of
the variable naming, making it more consistent through the body of the
function.

Fixes #29173 in conjunction with hashicorp/hcl#478

@jbardin jbardin requested a review from a team July 19, 2021 19:56
jbardin added 3 commits July 19, 2021 16:42
Up until now marks were not considered by `ignore_changes`, that however
means changes to sensitivity within a configuration cannot ignored, even
though they are planned as changes.

Rather than separating the marks and tracking their paths, we can easily
update the processIgnoreChanges routine to handle the marked values
directly. Moving the `processIgnoreChanges` call also cleans up some of
the variable naming, making it more consistent through the body of the
function.
@jbardin jbardin force-pushed the jbardin/ignore-changes-marks branch from a2c977d to 66a5950 Compare July 19, 2021 20:42
@jbardin jbardin added the 1.0-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label Jul 20, 2021
Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

I had to mull this one a bit because the idea of ignoring changes to sensitivity had me a little worried about the case of ignoring a change from nonsensitive to sensitive.

However, after some thought experiments I think this is safe and sound because the meaning of ignore_changes is to keep what's in the prior state regardless of what the configuration said, and dynamic sensitivity (as opposed to the static sensitivity from the provider schema) always originates in the configuration, and so there shouldn't be any case where the remote system can end up putting a newly-sensitive value into an argument that was originally assigned as non-sensitive... that would be possible only if the provider had some way to signal dynamically that a particular value is sensitive, but even if we do add that later the provider would hopefully indicate that during ReadResource rather than in PlanResourceChange and so the change in sensitivity would come through as a funny sort of drift rather than as part of the planned change.

The one exception to this rule I can think of is terraform_remote_state, which gets to cheat a bit by virtue of being built in to Terraform and thus not having to traverse the provider wire protocol. It can and does return dynamically-sensitive values, but it's a data source and so not subject to ignore_changes anyway.

If the above all seems to add up to you too (and doesn't spark any ideas for new potential problems I didn't consider), then this LGTM!

@jbardin
Copy link
Member Author

jbardin commented Jul 21, 2021

The situation you are describing is basically what led me to fixing this in the first place. A combination of the impure function behavior with a bug in HCL was causing the value to change it's sensitivity during planning, resulting in a planned change that could not be ignored. While it's unlikely most users would ever encounter this under normal use, it could also be triggered by manual changes in the config (shown in the existing test I had to fix), and generally seems more "correct" in the behavior of ignoring any changes caused by the configuration to the existing resource state.

@jbardin jbardin merged commit 6fa0409 into main Jul 21, 2021
@jbardin jbardin deleted the jbardin/ignore-changes-marks branch July 21, 2021 13:05
jbardin added a commit that referenced this pull request Jul 21, 2021
@github-actions
Copy link
Contributor

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1.0-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ignore_changes with an impure function with sensitive arguments cause change in plan
2 participants