pulumi / pulumi-terraform-bridge

A library allowing Terraform providers to be bridged into Pulumi.
Apache License 2.0
199 stars 43 forks source link

Read-for-import does not apply Default values under PlanResourceChange #2372

Open t0yv0 opened 2 months ago

t0yv0 commented 2 months ago

What happened?

There is an observable change in behavior with the PlanResourceChange flag for Read for import (Read with empty properties) that leads to the import resource option being broken for SDKv2-based providers.

This got discovered in: https://github.com/pulumi/pulumi-aws/pull/4403

Before PRC:

  Read(properties={}) ==>
  {"properties": {"acl": null, "forceDestroy": null},
   "inputs":     {"acl": "private"}, {"forceDestroy": false}}

  Check(olds={"acl": "private", "forceDestroy": false}, news={}) ==>
  {"acl": "private", "forceDestroy": false}

  Diff(olds={"acl": "private", "forceDestroy": false},
       news={"acl": "private", "forceDestroy": false}) ==>

After PRC:

    Read(properties={}) ==>
    {"acl": null, "forceDestroy": null}

    Check(olds={"acl": null, "forceDestroy": null}, news={}) ==>
    {"acl": "private", "forceDestroy": false}

    Diff(olds={"acl": null, "forceDestroy": null},
         news={"acl": "private", "forceDestroy": false}) ==>
    {"diffs": ["acl", "forceDestroy"]}

Unfortunately this Diff is very sensitive as subsequently pulumi up fails to import without giving the user any good options to move forward.

The issue was discovered on legacy bucket but likely applies to more providers. The "private" and "forceDestroy" values are specified as SDKv2 defaults using something like this:

            "acl": {
                Type:          schema.TypeString,
                Default:       "private",
                Optional:      true,
                ConflictsWith: []string{"grant"},
                ValidateFunc:  validation.StringInSlice(BucketCannedACL_Values(), false),
            },

Looks like Read needs to apply these defaults to the returned inputs section in the case of import.

Example

See above

Output of pulumi about

N/A

Additional context

This issue is for SDKv2 resources. For PF resources it's more complicated since we cannot easily infer default values for them, causing https://github.com/pulumi/pulumi-terraform-bridge/issues/2272 - it is possible that solving 2272 could unblock this problem as well.

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).

t0yv0 commented 2 months ago

Investigated some more in https://github.com/pulumi/pulumi-terraform-bridge/pull/2373 - it's not that before PRC defaults were applied to the results of Read, they were not, but we are running into the problem https://github.com/pulumi/pulumi-terraform-bridge/issues/2272 variation, looks like this diff was a DIFF_NONE previously but becomes a DIFF_SOME under PRC.

{
  "method": "/pulumirpc.ResourceProvider/Diff",
  "request": {
    "id": "b1",
    "urn": "urn:pulumi:test::test::prov:index/legacyBucket:LegacyBucket::mainRes",
    "olds": {
      "id": "b1"
    },
    "news": {
      "__defaults": [
        "acl",
        "forceDestroy"
      ],
      "acl": "private",
      "forceDestroy": false
    },
    "oldInputs": {
      "__defaults": []
    }
  },
  "response": {
    "changes": "DIFF_NONE",
    "hasDetailedDiff": true
  }
}
VenelinMartinov commented 2 months ago

This looks like a provider issue - the provider isn't returning a value for acl when read, even though it must be set since it has a default.

Should we fix this in the provider?

t0yv0 commented 2 months ago

We can't fix it in the provider because Read() is not going to guarantee applying defaults in Terraform. I think the impact here might be that every single bridged provider resource with SDKV2 Defaults has the import option broken?

https://github.com/pulumi/pulumi-terraform-bridge/pull/2373

VenelinMartinov commented 2 months ago

I believe the contract here is that Read should return all properties of the resource - if the resource has a property set, regardless if it is set by the user or a default value it should be returned by Read.

This seems to me like an issue with the TF provider - the property in question is set, because it has a default value. Read, however, returns no value for it

VenelinMartinov commented 2 months ago

Depending on how wide spread this issue is, we could add a workaround in the bridge

VenelinMartinov commented 2 months ago

I wonder if Refresh is also affected here - we call Read then too.

t0yv0 commented 2 months ago

We cross-checked a few resources. It seems like the issue is not widespread - most resources actually return the default values from Read() so import option works for them. I've checked a few popular GCP resources with Default: values

t0yv0 commented 2 months ago

The direction so far is to treat it as won't fix in the bridge, and instead treat it as resource-level issue fixed by fixing up Read() as necessary. The change in Diff behavior under Plan Resource Change that makes the following a DIFF_SOME seems sound:

{
  "method": "/pulumirpc.ResourceProvider/Diff",
  "request": {
    "id": "b1",
    "urn": "urn:pulumi:test::test::prov:index/legacyBucket:LegacyBucket::mainRes",
    "olds": {
      "id": "b1"
    },
    "news": {
      "__defaults": [
        "acl",
        "forceDestroy"
      ],
      "acl": "private",
      "forceDestroy": false
    },
    "oldInputs": {
      "__defaults": []
    }
  },
  "response": {
    "changes": "DIFF_NONE",
    "hasDetailedDiff": true
  }
}
t0yv0 commented 2 months ago

I was curious how exactly this works in the bridge right now and I debugged this example (with PlanResourceChange=false). It seems to be an implicit logic, not something coded up explicitly. Notable references:

https://github.com/pulumi/pulumi-terraform-bridge/blob/master/pkg/tfbridge/provider.go#L1388 https://github.com/pulumi/pulumi-terraform-bridge/blob/master/pkg/tfshim/sdk-v2/resource.go#L86

func (r v2Resource) InstanceState goes ahead and applies default values for data that was not passed in. This is where it's happening. So then the diff is comparing "effective state" with defaults against news with defaults.

This still seems wrong to me. IN TF InstateState is frozen from the statefile and is not affected by the current provider's settings such as current version's defaults. So I guess this is good this behavior is changing.

flostadler commented 2 months ago

This also affects the ASG resource in pulumi-aws: https://github.com/pulumi/pulumi-aws/issues/4457.

t0yv0 commented 1 month ago

Proposing to fix ASG via diff suppression. If we like this approach it can be scaled out to the rest. https://github.com/pulumi/pulumi-aws/pull/4510