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

Make sure no nil lists are passed to TF #1688

Merged
merged 13 commits into from
Feb 23, 2024

Conversation

VenelinMartinov
Copy link
Contributor

@VenelinMartinov VenelinMartinov commented Feb 20, 2024

Addresses pulumi/pulumi-aws#3427

We should make sure we send the upstream provider an empty array where MaxItemsOne properties are expected instead of a nil, since that is what TF does.

// Iterate over the TF schema and add an empty list for each list type not in the results.
tfs.Range(func(key string, value shim.Schema) bool {
schema := tfs.Get(key)
if schema.Type() == shim.TypeList && result[key] == nil {
Copy link
Member

Choose a reason for hiding this comment

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

Does terraform distinguish between nil and missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

In maps yes, in objects no, an object having an optional field F will have it nil if it's missing, there's no other way for it to have it missing.

@VenelinMartinov
Copy link
Contributor Author

VenelinMartinov commented Feb 21, 2024

After a discussion with @t0yv0 I dug a bit into the code related to makeTerraformInput trying to trace why we are not converting the missing input into an empty array for TF.

He suggested this is likely related to MaxItemsOne since we don't have a concept of empty array for these properties, while TF does.

Create calls UnmarshalTerraformConfig:

config, assets, err := UnmarshalTerraformConfig(ctx,

which one call down calls makeTerraformInputs:

inputs, err := cctx.makeTerraformInputs(nil, m, tfs, ps)

Which then calls makeObjectTerraformInputs:

return ctx.makeObjectTerraformInputs(olds, news, tfs, ps, false /*rawNames*/)

which only iterates over the news:

for key, value := range news {

At this point the forward property is not in the news since it was not specified in the pulumi program.

This means that the code which is meant to fill the empty array in place of the MaxItemsOne nil is never called:

if IsMaxItemsOne(tfs, ps) {
wrap := func(val resource.PropertyValue) resource.PropertyValue {
if val.IsNull() {
return resource.NewArrayProperty([]resource.PropertyValue{})
}

This means the issue is either:

  1. news should have contained a nil forward value, it should NOT have been missing.
  2. We should call the MaxItemsOne nil handling code for all TF properties, not just the ones in news

The second one seems more likely to me, happy to be corrected here.

@t0yv0
Copy link
Member

t0yv0 commented Feb 21, 2024

2 sounds exact to me. The transformation is schema-aware and should be walking object fields even if they're not present in the source PropertyMap but are present in the schema, so it can apply the unflattening treatment as necessary.

@VenelinMartinov VenelinMartinov marked this pull request as ready for review February 22, 2024 18:16
@VenelinMartinov VenelinMartinov marked this pull request as draft February 22, 2024 18:21
@VenelinMartinov
Copy link
Contributor Author

I still need some tests for this.

@VenelinMartinov VenelinMartinov marked this pull request as ready for review February 22, 2024 20:19
Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

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

Looks good to me. LGTM

@VenelinMartinov VenelinMartinov merged commit 3425c55 into master Feb 23, 2024
7 checks passed
@VenelinMartinov VenelinMartinov deleted the vvm/fix_list_nils_inputs branch February 23, 2024 10:41
t0yv0 added a commit that referenced this pull request Feb 23, 2024
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

3 participants