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

Better control of marks through conditional and for expressions #710

Merged
merged 2 commits into from
Nov 15, 2024

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Nov 14, 2024

The unknown value paths within conditional and for expressions were not consistently retaining marks.

In the case of for expressions, an unknown collection would lose all marks. If we have the any marks from the collection we can always return those. If the expression contains a conditional include, we can also attempt to obtain the marks from that since the conditional impacts the total collection value, the marks must alway be included in the final result. This gives us a more complete unknown value, but it can never contain a comprehensive set of marks for all cases. If the collection is entirely unknown, we have no idea what the element value marks are going to be, so the final known result could still always include marks we were not anticipating.

The conditional expressions are more straightforward to fix, but also more troublesome from Terraform's perspective because it represents a change in behavior. Previously, the marks for the true and false values were kept distinct and were not included at all if the result was unknown. This poses a problem when behavior is specified by marks on the values, because the behavior changes depending on whether the result is known or not. The only way to reconcile this with conditionals is to always combine all marks, which is what I've done here. The union of marks definitely makes logical sense when the condition is unknown, since we could have either set, but could be surprising for configurations which could never produce an unknown condition.

The change in marks looks like so (using letters for variables and numbers for marks):

x(1) ? y(2) : z(3) x Unknown x True x False
before <unknown>() y(2) z(3)
after <unknown>(1,2,3) y(1,2,3) z(1,2,3)

You can see here that if 1 represented "sensitive" as used in Terraform, the result which is derived from a sensitive value had no indication of that. A case could be made that the y and z values should be y(1,2) and z(1,3), but conditional expressions already need to unify the return type, so it feels natural that it should also unify the meta-type information like marks, making the results more consistent and predictable.

Verified

This commit was signed with the committer’s verified signature.
snyk-bot Snyk bot
Some of the unknown value paths within ForExpr were dropping marks from the
result.
@jbardin jbardin marked this pull request as ready for review November 15, 2024 14:49
@jbardin jbardin requested a review from a team November 15, 2024 14:49
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

LGTM

@jbardin jbardin merged commit 56a9aee into main Nov 15, 2024
13 checks passed
@jbardin jbardin deleted the jbardin/marked-conditions branch November 15, 2024 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants