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

[t0yv0]

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: pulumi/pulumi-aws#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 #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]

Investigated some more in #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 #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]

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]

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?

#2373

[VenelinMartinov]

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]

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

[VenelinMartinov]

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

[t0yv0]

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

• GCP compute instance works
• GCP storage bucket works
• GCP network works

[t0yv0]

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]

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]

This also affects the ASG resource in pulumi-aws: pulumi/pulumi-aws#4457.

[t0yv0]

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