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

nonsensitive: no longer produces error when applied to nonsensitive values #33856

Merged
merged 4 commits into from Nov 8, 2023

Conversation

joaocc
Copy link
Contributor

@joaocc joaocc commented Sep 9, 2023

#28222 and #31693 and other issues describe a set of situations where users are effectively locked into a situation where they can't use derivations of collections of objects where some fields are marked as sensitive into for_each, and are also unable to use nonsensitive because some of the values are indeed nonsensitive already. It also applies to arrays of elements where some are marked as sensitive while others are not.
In these hybrid situations, there doesn't seem to be an way to mark everything as nonsensitive.

The behaviour in 1.5.x causes an error if nonsensitive is called on a value that is not marked as "sensitive".
The documentation at https://developer.hashicorp.com/terraform/language/functions/nonsensitive offers a rationale

nonsensitive will return an error if you pass a value that isn't marked as sensitive, because such a call would be redundant and potentially confusing or misleading to a future maintainer of your module.

Also, this behaviour is not consistent with that of sensitive, where sensitive can be applied to any value, regardless of whether it is already sensitive or not.

However, from the sheer amount of issues complaining about the implications of this decision, this well-intentioned decision is causing unintended pain.
The current behaviour is not only interfering with developer decision of determining what is and what isn't sensitive, but also doing that when there is not even a security risk (fields are already nonsensitive). The workarounds proposed force developers away from simpler and more straightforward solutions, more difficult to understand and maintain, without any tangible benefit in terms of security.

For this reason, this PR proposes that nonsensitive does what it is meant to do, which is to allow developers to mark fields as nonsensitive, leaving softer aspects of maintainability and maintainer confusion to be decided by each developer.

This PR should not cause breaking behaviours, as it is enabling a behaviour that wasn't allowed before.

Fixes #
#31693

Relates #
#32880
#32828
#31646
#31609
#31609
#29744
#28222 (comment)
#28222

Target Release

1.5.x

Draft CHANGELOG entry

  • nonsensitive no longer produces error when applied to values that are not sensitive

BUG FIXES

  • nonsensitive can now be used to remove sensitive from any element, regardless of it's original sensitivity flag.

@hashicorp-cla
Copy link

hashicorp-cla commented Sep 9, 2023

CLA assistant check
All committers have signed the CLA.

@joaocc joaocc changed the title Patch 1 nonsensitive no longer produces error when applied to nonsensitive values Sep 9, 2023
@joaocc joaocc changed the title nonsensitive no longer produces error when applied to nonsensitive values nonsensitive: no longer produces error when applied to nonsensitive values Sep 9, 2023
@crw
Copy link
Collaborator

crw commented Sep 11, 2023

Thanks for this submission. I've added it to the triage queue. Just a reminder that we will not be able to review / merge unless the CLA is signed. Thanks!

@joaocc joaocc marked this pull request as ready for review September 14, 2023 12:23
Copy link
Member

@liamcervante liamcervante left a comment

Choose a reason for hiding this comment

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

Hi @joaocc, the code changes look good to me. Just have a note around the documentation.

Also, can you change your description so it says it is fixing just #31693, and then simply relates to all the others. I agree this change is related to them, but I don't think it actually fixes them.

Thanks!

@@ -73,8 +73,8 @@ due to an inappropriate call to `nonsensitive` in your module, that's a bug in
your module and not a bug in Terraform itself.
**Use this function sparingly and only with due care.**

`nonsensitive` will return an error if you pass a value that isn't marked
as sensitive, because such a call would be redundant and potentially confusing
`nonsensitive` will no longer return an error if you pass a value that isn't marked
Copy link
Member

Choose a reason for hiding this comment

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

I think we don't need to clarify this with no longer as new readers won't have the context that this used to not work.

Let's change this to:

nonsensitive will make no changes to values that aren't marked as sensitive, even though such a call may be redundant and potentially confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed as per @liamcervante indication. Thx

@liamcervante liamcervante self-assigned this Sep 19, 2023
@liamcervante liamcervante added the 1.6-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label Sep 19, 2023
@liamcervante liamcervante removed the 1.6-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label Sep 20, 2023
@liamcervante
Copy link
Member

The cutoff for v1.6 is next week, I'll add the backport label in again if we get this approved and merged before then. Otherwise it'll have to wait for v1.7.

@joaocc
Copy link
Contributor Author

joaocc commented Nov 7, 2023

@liamcervante any news on when this is planned for merge and release?

@liamcervante
Copy link
Member

Hi @joaocc , apologies for the delay. Could you update the documentation to address my comments? Thanks!

joaocc added a commit to joaocc/terraform that referenced this pull request Nov 7, 2023
@joaocc
Copy link
Contributor Author

joaocc commented Nov 7, 2023

Updated as per instructions, and rebased on main. Thx

@liamcervante liamcervante merged commit 47beda9 into hashicorp:main Nov 8, 2023
7 checks passed
Copy link

github-actions bot commented Nov 8, 2023

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

@liamcervante
Copy link
Member

This is merged and the CHANGELOG updated, will be released in Terraform v1.7.0.

Thanks @joaocc for your contribution!

Copy link

github-actions bot commented Dec 9, 2023

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 Dec 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants