pulumi / pulumi-terraform-bridge

A library allowing providers built with the Terraform Plugin SDK to be bridged into Pulumi.
Apache License 2.0
184 stars 42 forks source link

PlanResourceChange dirty refresh on empty maps #2047

Closed VenelinMartinov closed 5 days ago

VenelinMartinov commented 4 weeks ago

What happened?

On master with PlanResourceChange enabled it looks like empty maps now refresh badly.

Example

TestAccBucketPy

https://github.com/pulumi/pulumi-gcp/actions/runs/9215462858/job/25354722934

Output of pulumi about

.

Additional context

No response

Contributing

Vote on this issue by adding a 👍 reaction. To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

VenelinMartinov commented 3 weeks ago

~This doesn't repro in TF, so likely a bridge issue~

VenelinMartinov commented 3 weeks ago

Ah, this actually reproes in TF ~but TF does not show it to the user~:

after the first apply we get labels: nil in the state

and after the refresh it changes to labels: {}

~But it doesn't show anything about it on the CLI~ - I was using terraform refresh which is apparently deprecated in favour of --refresh-only

VenelinMartinov commented 3 weeks ago

Ok it does show it but with terraform plan --refresh-only:

esc run providers.all -- terraform plan --refresh-only
google_storage_bucket.bucket: Refreshing state... [id=example-bucket123123223]

Note: Objects have changed outside of Terraform

Terraform detected the following changes made outside of Terraform since the
last "terraform apply" which may have affected this plan:

  # google_storage_bucket.bucket has changed
  ~ resource "google_storage_bucket" "bucket" {
        id                          = "example-bucket123123223"
      + labels                      = {}
        name                        = "example-bucket123123223"
        # (15 unchanged attributes hidden)

        # (1 unchanged block hidden)
    }

This is a refresh-only plan, so Terraform will not take any actions to undo
these. If you were expecting these changes then you can apply this plan to
record the updated values in the Terraform state without changing any remote
objects.

─────────────────────────────────────────────────────────────────────────────

Note: You didn't use the -out option to save this plan, so Terraform can't
guarantee to take exactly these actions if you run "terraform apply" now.
VenelinMartinov commented 3 weeks ago

Some potential ways around this:

  1. Accept the diff as correct in the bridge and patch each provider to ignore diffs in empty/nil maps where relevant
  2. Restore the non-PRC behaviour which saved nil maps as empty, purposefully diverging from the way TF handles these inputs. Could be problematic for cases where nil maps are meaningful.
  3. Add some thing for diffing maps?
VenelinMartinov commented 3 weeks ago

@t0yv0 pointed out the culprit is likely normalizeNullValues: https://github.com/hashicorp/terraform-plugin-sdk/blob/70fb6b9b15e8e5fc73f424e24084c28fedd1e013/helper/schema/grpc_provider.go#L1565

VenelinMartinov commented 3 weeks ago

as suggested by @t0yv0 https://github.com/pulumi/pulumi/pull/16146 fixes this

VenelinMartinov commented 3 weeks ago

Also explored in https://github.com/pulumi/pulumi-terraform-bridge/pull/2063 by @t0yv0

VenelinMartinov commented 3 weeks ago

Possibly related? https://github.com/pulumi/pulumi-terraform-bridge/issues/1106

t0yv0 commented 3 weeks ago

Not sure if 1106 is very related, it's read returning null when we want it not to, here it's the reverse. The workaround I would like to try here is not likely to solve 1106 though I can test easily.

t0yv0 commented 3 weeks ago

Got https://github.com/pulumi/pulumi-aws/pull/4013 to get this fixed for AWS. Curious if other providers satisfied as well.

VenelinMartinov commented 5 days ago

This is no longer an issue with the new refresh behaviour in the CLI.