pulumi / pulumi-azure-native

Azure Native Provider
Apache License 2.0
127 stars 34 forks source link

Unexpected diffs on network resources with v2 alpha #2475

Closed mnlumi closed 1 year ago

mnlumi commented 1 year ago

What happened?

When working with Pulumi templates and testing out 2.0.0-alpha.1, there are diffs in network resources. ~that do not occur in Python or Typescript~. Edit: Upon further testing, these are also occurring in Typescript and Python. Screenshot 2023-05-26 at 11 38 03 AM Screenshot 2023-05-26 at 11 37 59 AM Screenshot 2023-05-26 at 11 37 54 AM Screenshot 2023-05-26 at 11 37 49 AM

Expected Behavior

I expect ~consistent behavior across languages and~ no diffs for these resources when upgrading from v1.102.0 to 2.0.0-alpha.1.

Steps to reproduce

Using any of the templates listed, run the program first with v1.102.0 to create the resources, then upgrade the provider to 2.0.0-alpha.1 and run pulumi up.

container-azure-csharp kubernetes-azure-csharp serverless-azure-csharp vm-azure-csharp

Output of pulumi about

CLI
Version      3.68.0
Go Version   go1.20.4
Go Compiler  gc

Plugins
NAME           VERSION
azure-native   2.0.0-alpha.1
dotnet         unknown
synced-folder  0.10.2

Host
OS       darwin
Version  13.0.1
Arch     arm64

This project is written in dotnet: executable='/usr/local/share/dotnet/dotnet' version='7.0.302'

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

mikhailshilkov commented 1 year ago

I got the same diff with container-azure-typescript. I suspect it's expected due to API version bumps.

image
mnlumi commented 1 year ago

After further testing, I am seeing the diffs pending in python as well, so this is not language specific. I've updated the issue title and description. Screenshot 2023-05-26 at 11 37 05 AM

mikhailshilkov commented 1 year ago

That's good to hear, one question answers.

As for the changes, I believe they are according to our current design for v2.

Let's take one change as an example, say azure-native:containerregistry:Registry added publicNetworkAccess : "Enabled". Here are the steps to investigate:

  1. Check the old API version, it's 2019-05-01.
  2. Check the new API version, it's 2022-12-01.
  3. Check if the old API version has a property called publicNetworkAccess: it does not.
  4. Check if the new API version has a property called publicNetworkAccess: it does.
  5. It also has a default value set to Enabled. That's why the diff is shown when migrating.

@mnlumi Should we close this as by-design? Or what other actions would you like us to do?

mnlumi commented 1 year ago

Yes, let's close this as by-design. Thanks!

kpitzen commented 1 year ago

Reopening due to this actually not being the behavior on other providers. Context to follow.

t0yv0 commented 1 year ago

I can confirm experimentally this is happening in pkg/v3 bridge. I've added an optional field to a gcp.storage.Bucket and I've ran pulumi preview on top of a stack where a Bucket was provisioned using the real published version. It's generating zero diff during provider upgrade or, interestingly if the user manually sets a value that matches the default value. So this appears to be a feature.

I'm less sure without further digging which system is responsible, the bridge does a complicated dance of emulating TF default value application while also calling some TF internals to compute the diffs, but it seems to behave like this overall. If we need to dig in further I could run an experiment on a pure TF provider under TF CLI to see if this behavior is observed for TF as well or it's purely something the bridge guarantees.

danielrbradley commented 1 year ago

As discussed in this comment, I'm also of the opinion that this is currently fuctioning as designed: https://github.com/pulumi/pulumi-azure-native/pull/2506#issuecomment-1584396048

  1. If the version (1) of a resource has set a default pricing tier of "A".
  2. If we upgrade to version (2) of the resource where the new API version sets the default pricing tier to "B" (perhaps more expensive).
  3. If the user hasn't set the pricing tier explicitly, would they not want to be notified that their pricing tier is changing?
mikhailshilkov commented 1 year ago

@danielrbradley Yes, we want to see a diff in the situation that you describe.

What we want to avoid is a diff when

  1. Version (1) has no property for the pricing tier, so whatever default A is used behind the scenes.
  2. Version (2) has a new property for the pricing tier which sets it to A.

In theory, 2 could set it to B, but I don't think that's a reasonable API design that exists in reality.

thomas11 commented 1 year ago

The just merged #2506 solves the issue for top-level properties. We still need to recurse into nested and array properties to handle their new defaults.

mikhailshilkov commented 1 year ago

We think we resolved all the main scenarios. If diffs pop up again, we'll track them in a separate issue.