Reset, don't remove sub-resource properties to avoid wrong diffs by thomas11 · Pull Request #3054 · pulumi/pulumi-azure-native

thomas11 · 2024-01-31T11:25:00Z

This PR fixes a defect introduced by #2950. When handling sub-resource properties that may be maintained as stand-alone resources, we need to overwrite them in several code paths. Before, we set them to their default value [] on Create, but removed them entirely on Read. This causes unnecessary diffs on Refresh. This PR standardizes the handling on resetting to the default value [].

Resolves #3049

github-actions · 2024-01-31T11:31:51Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

codecov · 2024-01-31T14:30:03Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (fddd80c) 60.70% compared to head (fbb0c27) 60.68%.

Files	Patch %	Lines
provider/pkg/provider/provider.go	92.85%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3054      +/-   ##
==========================================
- Coverage   60.70%   60.68%   -0.02%     
==========================================
  Files          66       66              
  Lines       11028    11007      -21     
==========================================
- Hits         6694     6680      -14     
+ Misses       3787     3782       -5     
+ Partials      547      545       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mikhailshilkov · 2024-01-31T16:13:15Z

My PR here has the tests that caught the original bug that I filed: #3056

danielrbradley

I think this is probably good, though it's still tricky to reason about how wide the impact will be.

I feel like we need a succinct description of this feature written down somewhere - like a short spec which describes the behaviour. Perhaps this could go into the contributing guide where we discuss things like the versioning mechanisms or perhaps a block alongside the tests could be easier to find?

My gut feel is that the tests are still a little patchy and are missing a few edge cases - though I haven't gathered proper evidence for this.

danielrbradley · 2024-02-01T13:17:16Z

 		oldInputs := resource.PropertyMap{}
 		sdkResponse := map[string]any{
+			"properties": map[string]any{
+				"accessPolicies": []any{
+					"a policy",
+				},
+			},
+		}
+		actual := provider.resetUnsetSubResourceProperties(ctx, "urn", sdkResponse, oldInputs, res)
+		expected := map[string]any{
 			"properties": map[string]any{
 				"accessPolicies": []any{},
 			},
 		}


Looks like this is the actual core of the change right? We're now expecting the access policies to still be there but be set to an empty value?

Correct. Because on Create, we set such properties to their default value, which is [], so we want it here, too, to avoid diffs.

## Overview This PR fixes a regression causing the provider to inadvertently drop the sub-resources following a `pulumi refresh`. This leads to flapping attributes as described in #4145, upon a subsequent `pulumi up` or simply another `pulumi refresh`. The problem manifests only for externally-managed sub-resources (as is generally recommended). For example, a `NetworkSecurityGroup` resource with separate (not inline) `SecurityRule` resources. In more detail, `refresh` causes two erroneous changes to the checkpointed state of a given sub-resource property (e.g. `privateEndpointConnections` or `securityRules`): 1. sets the _output property_ to `[]`, even if there's a meaningful value (the discovered sub-resources). 2. sets the _input property_ to `[]`, even though the program has no inline resources. From this point, a subsequent `pulumi up` or `pulumi refresh` will show a spurious change. Closes #4145 ## Background This PR essentially reverts some undesirable changes in 642a38b. In #3054, the Read method was altered to remove sub-resources from the checkpointed state ([here](https://github.com/pulumi/pulumi-azure-native/pull/3054/files#diff-418e6e7d93aeacebe7c22d5fb674b48bfa15c6a514eb3ae4744b86d447595d8eR1143)). The property map named `outputsWithoutIgnores` seems to have been mistaken to contain outputs, but is best understood as an intermediate form of the inferred inputs, which is why the sub-resources are stripped. ![image](https://github.com/user-attachments/assets/4d662b8f-db32-427a-8af8-dbfc53692b64) PR #3054 also switches from _removing_ to _resetting_ the properties, with the unexpected effect of adding the property into the _inputs_. This causes flapping in a subsequent `refresh` because it confuses `findUnsetPropertiesToMaintain` to think that the sub-resources are inline. ![image](https://github.com/user-attachments/assets/4b3f9d77-a96a-4007-9db6-fc30d4540461) I believe that #3054 was effective at solving #3049 because it incorporated Mikhail's insight about the SDK shape, [here](https://github.com/pulumi/pulumi-azure-native/pull/3054/files#diff-418e6e7d93aeacebe7c22d5fb674b48bfa15c6a514eb3ae4744b86d447595d8eR1507). The changes that we now seek to revert were not germane to #3049. ## Example 1 Deploy the following program twice, setting the `revision` tag to `2` after the first deployment. Twice is needed because this program creates an security rule within the NSG, but the NSG's state will not reflect the security rule until after an update to the NSG. Refresh would not be effective because that's the very problem this PR seeks to fix. ```csharp using Pulumi; using Pulumi.AzureNative.Resources; using Pulumi.AzureNative.Network; // using Pulumi.AzureNative.Network.Inputs; using System.Collections.Generic; return await Pulumi.Deployment.RunAsync(() => { // Create an Azure Resource Group var resourceGroup = new ResourceGroup("resourceGroup"); var _name = "eron"; var nsg = new NetworkSecurityGroup($"data-center-{_name}-nsg", new NetworkSecurityGroupArgs { ResourceGroupName = resourceGroup.Name, Tags = { { "revision", "1" }, }, }); var rule = new SecurityRule($"NSG-AllowVirtualNetworkRdpInbound-{_name}", new SecurityRuleArgs { SecurityRuleName = "AllowVirtualNetworkRdpInbound", Priority = 120, Direction = SecurityRuleDirection.Inbound, Access = SecurityRuleAccess.Allow, Protocol = SecurityRuleProtocol.Tcp, SourcePortRange = "*", DestinationPortRange = "3389", SourceAddressPrefix = "VirtualNetwork", DestinationAddressPrefix = "VirtualNetwork", ResourceGroupName = resourceGroup.Name, NetworkSecurityGroupName = nsg.Name, Description = "Allows internal RDP Traffic" }); // Export the primary key of the Storage Account return new Dictionary<string, object?> { // ["nsg"] = nsg.ID() }; }); ``` Export the state, run `pulumi refresh`, and export again. You should observe that the NSG's inputs and outputs have been destabilized. Try refreshes and ups to see flapping. See attachments, for a series of state exports: - [outputs-repro.zip](https://github.com/user-attachments/files/20600583/outputs-repro.zip) - [outputs-fix.zip](https://github.com/user-attachments/files/20600584/outputs-fix.zip) ## Example 2 This example is closer to what's described in #4145. The program creates a private endpoint, so that the EventHub namespace has a private endpoint connection that we may observe to be 'flappy'. ```ts import * as pulumi from "@pulumi/pulumi"; import * as resources from "@pulumi/azure-native/resources"; import * as eventhub from "@pulumi/azure-native/eventhub"; import * as network from "@pulumi/azure-native/network"; // Configuration for the subnet ID const config = new pulumi.Config(); const subnetId = config.require("subnetId"); // Create an Azure Resource Group const resourceGroup = new resources.ResourceGroup("eventhub-rg"); const namespace = new eventhub.v20230101preview.Namespace("namespace", { isAutoInflateEnabled: true, maximumThroughputUnits: 40, minimumTlsVersion: "1.2", publicNetworkAccess: "Enabled", resourceGroupName: resourceGroup.name, sku: { capacity: 1, name: "Standard", tier: "Standard" }, tags: { "Name": "mynamespace", "revision": "3" } }); const privateEndpoint = new network.PrivateEndpoint("privateEndpoint", { resourceGroupName: resourceGroup.name, location: resourceGroup.location, privateEndpointName: "myPrivateEndpoint", subnet: { id: subnetId, }, privateLinkServiceConnections: [{ name: "myPrivateLinkServiceConnection", privateLinkServiceId: namespace.id, groupIds: ["namespace"], }], }); const privateEndpointConnections = namespace.privateEndpointConnections; ``` [repro.zip](https://github.com/user-attachments/files/20619741/repro.zip)

thomas11 requested review from a team, danielrbradley and mikhailshilkov January 31, 2024 11:25

thomas11 force-pushed the tkappler/3049-refresh branch from 6c20810 to ec2b31e Compare January 31, 2024 14:14

danielrbradley reviewed Feb 1, 2024

View reviewed changes

thomas11 added 2 commits February 2, 2024 16:06

Reset, don't remove sub-resource properties to avoid wrong diffs

3a4b59f

Document sub-resource properties in CONTRIBUTING

d39092b

thomas11 force-pushed the tkappler/3049-refresh branch from ec2b31e to d39092b Compare February 2, 2024 15:52

danielrbradley approved these changes Feb 2, 2024

View reviewed changes

mjeffryes assigned thomas11 Feb 3, 2024

Point to new docs in comments

fbb0c27

thomas11 enabled auto-merge (squash) February 3, 2024 07:03

thomas11 merged commit 642a38b into master Feb 3, 2024

thomas11 deleted the tkappler/3049-refresh branch February 3, 2024 07:30

This was referenced Jun 3, 2025

Provider v3 breaks azure-native:network:NetworkSecurityGroup update #4094

Closed

Refresh for VirtualNetwork with external subnets still shows subnets #3049

Closed

Preserve sub-resources in state on refresh #4178

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reset, don't remove sub-resource properties to avoid wrong diffs#3054

Reset, don't remove sub-resource properties to avoid wrong diffs#3054
thomas11 merged 3 commits into
masterfrom
tkappler/3049-refresh

thomas11 commented Jan 31, 2024 •

edited

Loading

Uh oh!

github-actions Bot commented Jan 31, 2024

Uh oh!

codecov Bot commented Jan 31, 2024 •

edited

Loading

Uh oh!

mikhailshilkov commented Jan 31, 2024

Uh oh!

danielrbradley left a comment

Uh oh!

danielrbradley Feb 1, 2024

Uh oh!

thomas11 Feb 2, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

thomas11 commented Jan 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jan 31, 2024

Does the PR have any schema changes?

Uh oh!

codecov Bot commented Jan 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mikhailshilkov commented Jan 31, 2024

Uh oh!

danielrbradley left a comment

Choose a reason for hiding this comment

Uh oh!

danielrbradley Feb 1, 2024

Choose a reason for hiding this comment

Uh oh!

thomas11 Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

thomas11 commented Jan 31, 2024 •

edited

Loading

codecov Bot commented Jan 31, 2024 •

edited

Loading