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

CONTRIBUTING.md

@@ -4,11 +4,11 @@
 
 ### Dependencies
 
-- Go 1.20
+- Go 1.21
 - NodeJS 18.X.X or later
 - Python 3.10 or later
-- .NET 7 or later
-- Gradle 7.6 or later
+- .NET 6 or later
+- Gradle 8 or later
 
 Please refer to [Contributing to Pulumi](https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md) for installation
 guidance.
@@ -34,6 +34,7 @@ yarn link @pulumi/azure-native
 pulumi up
 ```
 
+
 ## Azure Versions
 
 Key facts about Azure Versions:
@@ -119,8 +120,49 @@ The default version is calculated and written to a 'lock' file which list every
 - `deprecated.json` is a list of API versions which are older than the versions included in the default version **and** at least 2 years old. These will be removed in the next major version of the provider.
 - `pending.json` is a list of new API versions which aren't yet included in the default version. These should be included in the default version at the next major release of the provider.
 
+
 ## New Go SDK
 
 As the size of the Go SDK is close to exceeding the limit of 512Mb, we've created a new SDK which we're publishing in parallel which defined a Go module per Azure namespace rather than a single root Go module. The additional go modules are auto-generated with the required dependencies in [provider/cmd/pulumi-gen-azure-native/main.go](./provider/cmd/pulumi-gen-azure-native/main.go#L312).
 
 This new SDK is published to its own repository at [github.com/pulumi/pulumi-azure-native-sdk](https://github.com/pulumi/pulumi-azure-native-sdk). This is separate partly due to the large number of tags which are created in this new repository per release, but also to remove the need to commit the SDK code into this repository.
+
+
+## Sub-resources
+
+Some resources which are also settable on a parent resource. For instance, the subnets of a virtual network can be specified inline with the virtual network resource:
+```typescript
+new network.VirtualNetwork("inline", {
+    subnets: [
+      { name: "default", addressPrefix: "10.4.1.0/24" },
+    ]
+})
+```
+
+But they can also be specified as stand-alone resources:
+```typescript
+new network.Subnet("third", { ... })
+```
+
+In this case, we call `VirtualNetwork.subnets` a _sub-resource property_.
+
+The choice between inline and stand-alone sub-resource definitions offers flexibility but needs some special handling in the CRUD lifecycle.
+
+On Create: when the user opts for stand-alone representations of the sub-resource, they omit the sub-resource property on the parent. For instance, `new network.VirtualNetwork` will be defined without `subnets`. In this example, however, creating a `VirtualNetwork` without subnets will fail in Azure because it's a required property. Therefore, on Create, we find sub-resource properties that are not set, and set them to their default value in the request payload. This happens in the `azureNativeProvider.setUnsetSubresourcePropertiesToDefaults` method.
+
+On Update: consider the naive implementation. When a virtual network `v` is updated, any stand-alone subnets are not in `v.subnets`, therefore they would be removed on update. To prevent this, we need to retrieve the existing sub-resources and fill them into the parent's sub-resource property. This is done in the `azureNativeProvider.maintainSubResourcePropertiesIfNotSet` method. For example:
+```typescript
+new network.VirtualNetwork("vnet", { ... })
+
+new network.Subnet("sub1", { ... })
+```
+When `vnet` is updated, the provider first reads `vnet` from Azure and populates `vnets.subnets` with the subnets from the response. This way, the subnets are simply round-tripped and not removed on update.
+
+On Read: when reading a parent resource, the Azure response will contain the sub-resources. If the user defined them stand-alone, we need to reset them to the empty value to avoid recording them in state. This is done in the `azureNativeProvider.resetUnsetSubResourceProperties` method.
+
+A note on the "default value" mentioned above: it's hard-coded as a an empty array currently. We haven't seen any other type, and it's the only one that really makes sense for sub-resources: they must be a variable number of items.
+
+Relevant PRs:
+- #2755
+- #2950
+- #3054

provider/pkg/provider/provider.go

@@ -922,7 +922,7 @@ func (k *azureNativeProvider) Create(ctx context.Context, req *rpc.CreateRequest
 		ctx, cancel := azureContext(ctx, req.Timeout)
 		defer cancel()
 
-		k.setUnsetSubresourcePropertiesToDefaults(res, bodyParams)
+		k.setUnsetSubresourcePropertiesToDefaults(res, bodyParams, bodyParams, true)
 
 		// Submit the `PUT` against the ARM endpoint
 		response, created, err := k.azureCreateOrUpdate(ctx, id, bodyParams, queryParams, res.UpdateMethod, res.PutAsyncStyle)
@@ -967,11 +967,15 @@ func (k *azureNativeProvider) Create(ctx context.Context, req *rpc.CreateRequest
 
 // 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,
-// auch a property might be required by Azure even if it's not annotated as such in the spec, e.g.,
+// 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.
+// 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,
@@ -981,10 +985,12 @@ func (k *azureNativeProvider) Create(ctx context.Context, req *rpc.CreateRequest
 //	setUnsetSubresourcePropertiesToDefaults(res, bodyParams) // custom
 //	k.azureCreateOrUpdate
 //	...
-func (k *azureNativeProvider) setUnsetSubresourcePropertiesToDefaults(res resources.AzureAPIResource, bodyParams map[string]interface{}) {
-	unset := k.findUnsetPropertiesToMaintain(&res, bodyParams)
+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 := bodyParams
+		cur := output
 		for _, pathEl := range p.path[:len(p.path)-1] {
 			curObj, ok := cur[pathEl]
 			if !ok {
@@ -1091,6 +1097,8 @@ func (k *azureNativeProvider) Read(ctx context.Context, req *rpc.ReadRequest) (*
 	if err != nil {
 		return nil, err
 	}
+
+	var outputsWithoutIgnores = outputs
 	if inputs == nil {
 		// There may be no old state (i.e., importing a new resource).
 		// Extract inputs from resource's ID and response body.
@@ -1119,7 +1127,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.removeUnsetSubResourceProperties(ctx, urn, outputs, inputs, &res)
+		outputsWithoutIgnores = k.resetUnsetSubResourceProperties(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
@@ -1132,7 +1140,7 @@ func (k *azureNativeProvider) Read(ctx context.Context, req *rpc.ReadRequest) (*
 	}
 
 	// Store both outputs and inputs into the state.
-	obj := checkpointObject(inputs, outputs)
+	obj := checkpointObject(inputs, outputsWithoutIgnores)
 
 	// Serialize and return RPC outputs.
 	checkpoint, err := plugin.MarshalProperties(
@@ -1174,15 +1182,11 @@ func mappableOldState(res resources.AzureAPIResource, oldState resource.Property
 	return plainOldState
 }
 
-// removeUnsetSubResourceProperties removes sub-resource properties from new outputs which weren't set in the old inputs.
+// 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.
-func (k *azureNativeProvider) removeUnsetSubResourceProperties(ctx context.Context, urn resource.URN, sdkResponse map[string]interface{}, oldInputs resource.PropertyMap, res *resources.AzureAPIResource) map[string]interface{} {
-	propertiesToRemove := k.findUnsetPropertiesToMaintain(res, oldInputs.Mappable())
-
-	if len(propertiesToRemove) == 0 {
-		return sdkResponse
-	}
-
+// For more details, see section "Sub-resources" in CONTRIBUTING.md.
+func (k *azureNativeProvider) resetUnsetSubResourceProperties(ctx context.Context, urn resource.URN, sdkResponse map[string]any,
+	oldInputs resource.PropertyMap, res *resources.AzureAPIResource) map[string]any {
 	// 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{})
@@ -1193,33 +1197,9 @@ func (k *azureNativeProvider) removeUnsetSubResourceProperties(ctx context.Conte
 		return sdkResponse
 	}
 
-	for _, prop := range propertiesToRemove {
-		deleteFromMap(result, prop.path)
-	}
-	return result
-}
+	k.setUnsetSubresourcePropertiesToDefaults(*res, oldInputs.Mappable(), result, false)
 
-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
+	return result
 }
 
 // Update updates an existing resource with new values.
@@ -1459,9 +1439,10 @@ type propertyPath struct {
 	propertyName string
 }
 
+// For details, see section "Sub-resources" in CONTRIBUTING.md.
 func (k *azureNativeProvider) maintainSubResourcePropertiesIfNotSet(ctx context.Context, res *resources.AzureAPIResource, id string, bodyParams map[string]interface{}) error {
 	// Identify the properties we need to read
-	missingProperties := k.findUnsetPropertiesToMaintain(res, bodyParams)
+	missingProperties := k.findUnsetPropertiesToMaintain(res, bodyParams, true /* returnApiShapePaths */)
 
 	if len(missingProperties) == 0 {
 		// Everything's already specified explicitly by the user, no need to do read.
@@ -1521,9 +1502,9 @@ func writePropertiesToBody(missingProperties []propertyPath, bodyParams map[stri
 	return writtenProperties
 }
 
-func (k *azureNativeProvider) findUnsetPropertiesToMaintain(res *resources.AzureAPIResource, bodyParams map[string]interface{}) []propertyPath {
+func (k *azureNativeProvider) findUnsetPropertiesToMaintain(res *resources.AzureAPIResource, bodyParams map[string]interface{}, returnApiShapePaths bool) []propertyPath {
 	missingProperties := []propertyPath{}
-	for _, path := range res.PathsToSubResourcePropertiesToMaintain(true /* includeContainers i.e. API-shape */, k.lookupType) {
+	for _, path := range res.PathsToSubResourcePropertiesToMaintain(returnApiShapePaths, k.lookupType) {
 		curBody := bodyParams
 		for i, pathEl := range path {
 			v, ok := curBody[pathEl]