Preserve sub-resources in state on refresh

provider/pkg/provider/provider.go

@@ -1172,50 +1172,6 @@ Verbose logs contain more information, see https://www.pulumi.com/docs/support/t
 	return response
 }
 
-// Properties pointing to sub-resources that can be maintained as separate resources might not be
-// present in the inputs because the user wants to manage them as standalone resources. However,
-// such a property might be required by Azure even if it's not annotated as such in the spec, e.g.,
-// Key Vault's accessPolicies. Therefore, we set these properties to their default value here,
-// an empty array. For more details, see section "Sub-resources" in CONTRIBUTING.md.
-//
-// During create, no sub-resources can exist yet so there's no danger of overwriting existing values.
-//
-// The `input` param is used to determine the unset sub-resource properties. They are then reset in
-// the `output` parameter which is modified in-place.
-//
-// Implementation note: we should make it possible to write custom resources that call code from
-// the default implementation as needed. This would allow us to cleanly implement special logic
-// like for Key Vault into custom resources without duplicating much code. In the Key Vault case,
-// the custom Read() would look like
-//
-//	provider.azureCanCreate(ctx, id, &res)
-//	setUnsetSubresourcePropertiesToDefaults(res, bodyParams) // custom
-//	k.azureCreateOrUpdate
-//	...
-func (k *azureNativeProvider) setUnsetSubresourcePropertiesToDefaults(res resources.AzureAPIResource,
-	input, output map[string]interface{}, outputIsInApiShape bool) {
-	unset := k.findUnsetPropertiesToMaintain(&res, input, outputIsInApiShape)
-
-	for _, p := range unset {
-		cur := output
-		for _, pathEl := range p.path[:len(p.path)-1] {
-			curObj, ok := cur[pathEl]
-			if !ok {
-				newContainer := map[string]any{}
-				cur[pathEl] = newContainer
-				cur = newContainer
-				continue
-			}
-			cur, ok = curObj.(map[string]any)
-			if !ok {
-				break
-			}
-		}
-
-		cur[p.path[len(p.path)-1]] = []any{}
-	}
-}
-
 type propertyPath struct {
 	path         []string
 	propertyName string
@@ -1334,11 +1290,9 @@ func (k *azureNativeProvider) Read(ctx context.Context, req *rpc.ReadRequest) (*
 		return nil, err
 	}
 
-	var outputsWithoutIgnores = outputs
-
 	plainOldState := oldState.Mappable()
 	if oldState.HasValue(customresources.OriginalStateKey) {
-		outputsWithoutIgnores[customresources.OriginalStateKey] = plainOldState[customresources.OriginalStateKey]
+		outputs[customresources.OriginalStateKey] = plainOldState[customresources.OriginalStateKey]
 	}
 
 	if inputs == nil {
@@ -1369,7 +1323,7 @@ func (k *azureNativeProvider) Read(ctx context.Context, req *rpc.ReadRequest) (*
 		oldInputProjection := k.converter.SdkOutputsToSdkInputs(res.PutParameters, plainOldState)
 		// 3a. Remove sub-resource properties from new outputs which weren't set in the old inputs.
 		// If the user didn't specify them inline originally, we don't want to push them into the inputs now.
-		outputsWithoutIgnores = k.resetUnsetSubResourceProperties(ctx, urn, outputs, inputs, res)
+		outputsWithoutIgnores := k.removeUnsetSubResourceProperties(ctx, urn, outputs, inputs, res)
 		// 3b. Project new outputs to their corresponding input shape (exclude read-only properties).
 		newInputProjection := k.converter.SdkOutputsToSdkInputs(res.PutParameters, outputsWithoutIgnores)
 		// 4. Calculate the difference between two projections. This should give us actual significant changes
@@ -1382,7 +1336,7 @@ func (k *azureNativeProvider) Read(ctx context.Context, req *rpc.ReadRequest) (*
 	}
 
 	// Store both outputs and inputs into the state.
-	obj := checkpointObject(res, inputs, outputsWithoutIgnores)
+	obj := checkpointObject(res, inputs, outputs)
 
 	// Serialize and return RPC outputs.
 	checkpoint, err := plugin.MarshalProperties(
@@ -1419,8 +1373,14 @@ func removeDefaults(res resources.AzureAPIResource, plainOldState, previousInput
 // removeUnsetSubResourceProperties resets sub-resource properties in the outputs if they weren't set in the old inputs.
 // If the user didn't specify them inline originally, we don't want to push them into the inputs now.
 // For more details, see section "Sub-resources" in CONTRIBUTING.md.
-func (k *azureNativeProvider) resetUnsetSubResourceProperties(ctx context.Context, urn resource.URN, sdkResponse map[string]any,
+func (k *azureNativeProvider) removeUnsetSubResourceProperties(ctx context.Context, urn resource.URN, sdkResponse map[string]any,
 	oldInputs resource.PropertyMap, res *resources.AzureAPIResource) map[string]any {
+	propertiesToRemove := k.findUnsetPropertiesToMaintain(res, oldInputs.Mappable(), false)
+
+	if len(propertiesToRemove) == 0 {
+		return sdkResponse
+	}
+
 	// Take a deep copy so we don't modify the original which is also used later for diffing.
 	copy := deepcopy.Copy(sdkResponse)
 	result, ok := copy.(map[string]interface{})
@@ -1432,11 +1392,35 @@ Verbose logs contain more information, see https://www.pulumi.com/docs/support/t
 		return sdkResponse
 	}
 
-	k.setUnsetSubresourcePropertiesToDefaults(*res, oldInputs.Mappable(), result, false)
-
+	for _, prop := range propertiesToRemove {
+		deleteFromMap(result, prop.path)
+	}
 	return result
 }
 
+func deleteFromMap(m map[string]interface{}, path []string) bool {
+	container := m
+	for i, key := range path {
+		if i == len(path)-1 {
+			_, found := container[key]
+			if found {
+				delete(container, key)
+			}
+			return found
+		}
+
+		value, ok := container[key]
+		if !ok {
+			return false
+		}
+		container, ok = value.(map[string]interface{})
+		if !ok {
+			return false
+		}
+	}
+	return false
+}
+
 // Update updates an existing resource with new values.
 func (k *azureNativeProvider) Update(ctx context.Context, req *rpc.UpdateRequest) (*rpc.UpdateResponse, error) {
 	// Use the global context to handle provider shutdown.

provider/pkg/provider/provider_e2e_test.go

@@ -22,8 +22,10 @@ import (
 	"github.com/pulumi/providertest/pulumitest"
 	"github.com/pulumi/providertest/pulumitest/assertpreview"
 	"github.com/pulumi/providertest/pulumitest/assertrefresh"
+	"github.com/pulumi/providertest/pulumitest/changesummary"
 	"github.com/pulumi/providertest/pulumitest/opttest"
 
+	"github.com/pulumi/pulumi/sdk/v3/go/common/apitype"
 	pulumirpc "github.com/pulumi/pulumi/sdk/v3/proto/go"
 )
 
@@ -67,6 +69,31 @@ func TestRequiredContainers(t *testing.T) {
 	assertrefresh.HasNoChanges(t, pt.Refresh(t))
 }
 
+func TestSubResources(t *testing.T) {
+	t.Parallel()
+	pt := newPulumiTest(t, "subresources")
+
+	// deploy an NSG with an "external" security rule, and an NSG with an inline security rule.
+	up := pt.Up(t)
+	assert.Len(t, up.Outputs["external-nsg-security-rules"].Value, 0)
+	assert.Len(t, up.Outputs["inline-nsg-security-rules"].Value, 1)
+	inlineRule := up.Outputs["inline-nsg-security-rules"].Value.([]any)[0].(map[string]any)
+	assert.Equal(t, "inline", inlineRule["name"])
+
+	// update a tag on the NSGs, and then check that the external security rules are now available as outputs.
+	pt.SetConfig(t, "subresources:revision", "2")
+	up = pt.Up(t)
+	upSummary := changesummary.FromStringIntMap(*up.Summary.ResourceChanges)
+	assert.Equal(t, 2, upSummary[apitype.OpUpdate])
+	assert.Len(t, up.Outputs["inline-nsg-security-rules"].Value, 1)
+	assert.Len(t, up.Outputs["external-nsg-security-rules"].Value, 1)
+	externalRule := up.Outputs["external-nsg-security-rules"].Value.([]any)[0].(map[string]any)
+	assert.Equal(t, "external", externalRule["name"])
+
+	// check that the state is stable after a refresh.
+	assertrefresh.HasNoChanges(t, pt.Refresh(t))
+}
+
 func TestParallelSubnetCreation(t *testing.T) {
 	t.Parallel()
 	if !util.EnableAzcoreBackend() {

provider/pkg/provider/provider_test.go

@@ -159,7 +159,7 @@ func TestMappableOldStatePreservesDefaultsThatWereNotInputs(t *testing.T) {
 	assert.Contains(t, m, "networkRuleSet")
 }
 
-func TestResetUnsetSubResourceProperties(t *testing.T) {
+func TestRemoveUnsetSubResourceProperties(t *testing.T) {
 	ctx := context.Background()
 
 	res, provider := setUpResourceWithRefAndProviderWithTypeLookup("")
@@ -168,7 +168,7 @@ func TestResetUnsetSubResourceProperties(t *testing.T) {
 		empty := &resources.AzureAPIResource{}
 		oldInputs := resource.PropertyMap{}
 		sdkResponse := map[string]any{}
-		actual := provider.resetUnsetSubResourceProperties(ctx, "urn", sdkResponse, oldInputs, empty)
+		actual := provider.removeUnsetSubResourceProperties(ctx, "urn", sdkResponse, oldInputs, empty)
 		expected := map[string]any{}
 		assert.Equal(t, expected, actual)
 	})
@@ -182,11 +182,9 @@ func TestResetUnsetSubResourceProperties(t *testing.T) {
 				},
 			},
 		}
-		actual := provider.resetUnsetSubResourceProperties(ctx, "urn", sdkResponse, oldInputs, res)
+		actual := provider.removeUnsetSubResourceProperties(ctx, "urn", sdkResponse, oldInputs, res)
 		expected := map[string]any{
-			"properties": map[string]any{
-				"accessPolicies": []any{},
-			},
+			"properties": map[string]any{},
 		}
 		assert.Equal(t, expected, actual)
 	})
@@ -202,12 +200,40 @@ func TestResetUnsetSubResourceProperties(t *testing.T) {
 				"accessPolicies": []any{},
 			},
 		}
-		actual := provider.resetUnsetSubResourceProperties(ctx, "urn", sdkResponse, oldInputs, res)
+		actual := provider.removeUnsetSubResourceProperties(ctx, "urn", sdkResponse, oldInputs, res)
 		expected := sdkResponse
 		assert.Equal(t, expected, actual)
 	})
 }
 
+func TestDeleteFromMap(t *testing.T) {
+	m := map[string]any{
+		"a": "scalar",
+		"b": map[string]any{
+			"b.a": "scalar",
+			"b.b": map[string]any{
+				"b.b.a": map[string]any{},
+			},
+		},
+	}
+
+	deleted := deleteFromMap(m, []string{"a"})
+	assert.True(t, deleted)
+	assert.NotContains(t, m, "a")
+	assert.Contains(t, m, "b")
+
+	deleted = deleteFromMap(m, []string{"b", "b.b", "b.b.a"})
+	assert.True(t, deleted)
+	assert.Contains(t, m, "b")
+	b := m["b"].(map[string]any)
+	assert.Contains(t, b, "b.a")
+	assert.Contains(t, b, "b.b")
+	bb := b["b.b"].(map[string]any)
+	assert.NotContains(t, bb, "b.b.a")
+
+	assert.False(t, deleteFromMap(m, []string{"b", "notfound"}))
+}
+
 // Helper to avoid repeating the same setup code in multiple tests. Returns a resource with a
 // "properties" property of type azure-native:keyvault:VaultProperties, which the returned provider
 // will return when asked to look up that type.
@@ -275,66 +301,6 @@ func setUpResourceWithRefAndProviderWithTypeLookup(version string) (*resources.A
 	return &res, &provider
 }
 
-func TestSetUnsetSubresourcePropertiesToDefaults(t *testing.T) {
-	res, provider := setUpResourceWithRefAndProviderWithTypeLookup("")
-
-	t.Run("unchanged", func(t *testing.T) {
-		body := map[string]any{
-			"container": map[string]any{
-				"properties": map[string]any{
-					"accessPolicies": []any{},
-				},
-			},
-		}
-		provider.setUnsetSubresourcePropertiesToDefaults(*res, body, body, true)
-		assert.Equal(t, map[string]any{
-			"container": map[string]any{
-				"properties": map[string]any{
-					"accessPolicies": []any{},
-				},
-			},
-		}, body)
-	})
-
-	t.Run("simple missing", func(t *testing.T) {
-		body := map[string]any{
-			"container": map[string]any{
-				"properties": map[string]any{},
-			},
-		}
-		provider.setUnsetSubresourcePropertiesToDefaults(*res, body, body, true)
-		assert.Equal(t, map[string]any{
-			"container": map[string]any{
-				"properties": map[string]any{
-					"accessPolicies": []any{},
-				},
-			},
-		}, body)
-	})
-
-	t.Run("nested missing", func(t *testing.T) {
-		body := map[string]any{}
-		provider.setUnsetSubresourcePropertiesToDefaults(*res, body, body, true)
-		assert.Equal(t, map[string]any{
-			"container": map[string]any{
-				"properties": map[string]any{
-					"accessPolicies": []any{},
-				},
-			},
-		}, body)
-	})
-
-	t.Run("nested missing in SDK shape", func(t *testing.T) {
-		body := map[string]any{}
-		provider.setUnsetSubresourcePropertiesToDefaults(*res, body, body, false)
-		assert.Equal(t, map[string]any{
-			"properties": map[string]any{
-				"accessPolicies": []any{},
-			},
-		}, body)
-	})
-}
-
 func TestInvokeResponseToOutputs(t *testing.T) {
 	invoke := resources.AzureAPIInvoke{
 		APIVersion: "v20241101",

provider/pkg/provider/test-programs/subresources/Pulumi.yaml

@@ -0,0 +1,55 @@
+name: subresources
+runtime: yaml
+description: Tests for subresources
+config:
+  revision:
+    type: string
+    default: "1"
+resources:
+  rg:
+    type: azure-native:resources:ResourceGroup
+
+  external-nsg:
+    type: azure-native:network:NetworkSecurityGroup
+    properties:
+      resourceGroupName: ${rg.name}
+      tags:
+        revision: ${revision}
+
+  external-rule:
+    type: azure-native:network:SecurityRule
+    properties:
+      resourceGroupName: ${rg.name}
+      networkSecurityGroupName: ${external-nsg.name}
+      securityRuleName: external
+      priority: 120
+      direction: Inbound
+      access: Deny
+      protocol: Tcp
+      sourcePortRange: "*"
+      destinationPortRange: "80"
+      sourceAddressPrefix: "*"
+      destinationAddressPrefix: "*"
+      description: Denies inbound HTTP traffic
+
+  inline-nsg:
+    type: azure-native:network:NetworkSecurityGroup
+    properties:
+      resourceGroupName: ${rg.name}
+      tags:
+        revision: ${revision}
+      securityRules:
+        - name: inline
+          priority: 120
+          direction: Inbound
+          access: Deny
+          protocol: Tcp
+          sourcePortRange: "*"
+          destinationPortRange: "80"
+          sourceAddressPrefix: "*"
+          destinationAddressPrefix: "*"
+          description: Denies inbound HTTP traffic
+
+outputs:
+  external-nsg-security-rules: ${external-nsg.securityRules}
+  inline-nsg-security-rules: ${inline-nsg.securityRules}