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

Convert variable types before applying defaults #32027

Merged
merged 6 commits into from Nov 2, 2022

Conversation

liamcervante
Copy link
Member

@liamcervante liamcervante commented Oct 17, 2022

Fixes #31978

Currently Terraform attempts to apply defaults in variables before calling go-cty to convert the variables into their actual concrete types. This can lead to a situation where HCL cannot apply the defaults because of a type incompatibility and then panics and crashes Terraform (#31978). It's kind of impossible to recover from or predict this situation without modifying the HCL public functions (eg. make the panic return an error, or add a new function like CheckCanApplyDefaults). We can't do this check with go-cty because as far as go-cty is concerned these are perfectly valid conversions. Note, that HCL and go-cty disagree here about what is or what isn't a valid type conversion.

This PR switches the order of these operations. We now go through go-cty to convert to the concrete type first, then when we call the apply defaults function the types are guaranteed to match and HCL doesn't need to worry about any kind of conversions.

This change requires an upstream fix in go-cty, so go-cty is updated as part of this change.

Target Release

1.3.4

Draft CHANGELOG entry

BUG FIXES

  • Input and Module Variables: Convert variable types before attempting to apply default values

@liamcervante liamcervante requested a review from a team October 17, 2022 08:58
@alisdair
Copy link
Member

I'm not sure this change is valid, but I'm also not sure it's invalid.

We originally applied defaults before converting type in part to allow us to distinguish between missing object attributes and attributes with null value. This distinction was later removed in order to harmonize the treatment of null with other contexts, so it may no longer be necessary.

However, you should be able to assign a map value to an object variable, so the existing test that you changed surprises me. From glancing at the code I'm not immediately sure why that would have broken, but I'll take another look.

@liamcervante
Copy link
Member Author

why that would have broken

It breaks because when it arrives in the convert package it's a map of strings. The convert package tries to turn it into the object but because one of the attributes of the object is a complex object that is not convertable from a string the whole convert attempt fails. This is different to the behaviour of inserting defaults which is quite happy to convert a map of strings into the complex object type as it is going through putting the correct defaults in.

I did have a look at going into go-cty and fixing the convert function to allow this. Here's a test case I wrote in go-cty that matches what the test case here is essentially trying to do: zclconf/go-cty#139.

@liamcervante
Copy link
Member Author

I've reverted the change I had to make to the other test, which means the tests are now going to report as failing. But once the following PRs are merged into go-cty and released this test will start passing again:

@liamcervante
Copy link
Member Author

This is ready for review!

internal/terraform/eval_variable.go Outdated Show resolved Hide resolved
Co-authored-by: alisdair <alisdair@users.noreply.github.com>
@liamcervante liamcervante added the 1.3-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label Nov 2, 2022
@liamcervante liamcervante merged commit 6521355 into main Nov 2, 2022
@liamcervante liamcervante deleted the liamcervante/convert-before-defaults branch November 2, 2022 08:38
@github-actions
Copy link

github-actions bot commented Nov 2, 2022

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

@github-actions
Copy link

github-actions bot commented Dec 3, 2022

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 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1.3-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.

"inconsistent set element types" crash
2 participants