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

ext/dynblock: Preserve marks from for_each expression into result #679

Merged
merged 2 commits into from
May 9, 2024

Conversation

apparentlymart
Copy link
Contributor

@apparentlymart apparentlymart commented May 9, 2024

Previously if the for_each expression was marked then expansion would fail because marked expressions are never directly iterable. (The failure also had a very confusing error message, because it wasn't directly testing whether the value was marked and was just assuming that any non-iterable value is non-iterable because it's of the wrong type.)

Now instead we'll allow marked for_each and preserve the marks into the values produced by the resulting block as much as we can.

This runs into the classic problem that HCL blocks are not values themselves and so cannot carry marks directly, but we can at least make sure that the values of any leaf arguments end up marked. We also ensure that applications that use hcldec (which includes Terraform) will see marks on the objects that represent the blocks themselves, thereby improving the marking accuracy.

This also fixes a nearby bug that I found along the way: dynblock would previously panic if the value specified for a block label was marked. It will now return an error message, although the error message is not very good quality because HCL doesn't understand what any marks represent. If this becomes a problem in practice then perhaps in future we'll allow applications to specify their own label value validator functions so that they can customize the error messages, but dynamic labels are so rarely used right now that it doesn't seem worth that complexity.


Terraform-specific note: Terraform currently only uses its "sensitive" mark and configures dynblock to reject any for_each value that carries that mark, so this change effectively does nothing for today's Terraform because the new codepaths are unreachable anyway.

I've implemented this for hashicorp/terraform#35078, because that introduces a new mark called "ephemeral" which represents that the value isn't required to be consistent between plan and apply and is never persisted in the state. It isn't appropriate to reject values with that mark in dynamic block for_each, because we want to be able to use ephemeral values in provider configuration blocks and several heavily-used providers use nested blocks in their configuration schemas that might need to vary based on an ephemeral value.

(For example, consider an ephemeral input value for providing a JWT to use for AWS's "AssumeRoleWithWebIdentity". A JWT is a good example of something that's likely to vary between plan and apply and so would be declared as ephemeral, but it would be reasonable to use the nullness of that value to choose dynamically whether or not to include the assume_role_with_web_identity block, which requires that we allow the for_each result to be derived from the ephemeral value and therefore be ephemeral itself.)

Previously if the for_each expression was marked then expansion would
fail because marked expressions are never directly iterable.

Now instead we'll allow marked for_each and preserve the marks into the
values produced by the resulting block as much as we can. This runs into
the classic problem that HCL blocks are not values themselves and so
cannot carry marks directly, but we can at least make sure that the values
of any leaf arguments end up marked.
Similar to the previously-added UnknownBody, the new optional interface
MarkedBody allows hcl.Body implementations to suggest a set of marks that
ought to be applied to any value that's generated to represent the content
of that body.

The dynblock extension then uses this to get hcldec to mark the whole
object representing any block that was generated by a dynamic block whose
for_each was marked, for a better representation of the fact that a
block's existence was decided based on a marked value.
@apparentlymart apparentlymart requested a review from a team May 9, 2024 01:15
@apparentlymart apparentlymart self-assigned this May 9, 2024
@apparentlymart apparentlymart merged commit 318bbfe into main May 9, 2024
13 checks passed
@apparentlymart apparentlymart deleted the dynblock-preserve-marks branch May 9, 2024 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants