cleanup for maintaining subresource properties

[EronWright]

This PR is a follow-up to #3040 to fix another variation of same basic problem, that the writePropertiesToBody function generates a malformed REST body. This variation happens in update cases, where the remoteState has existing values and the bodyParams are empty (see test case).

Note that the problem is not a 3.x regression, it repros in 2.x.

For example, given a basic NSG:

new NetworkSecurityGroup($"data-center-{_name}-nsg", new NetworkSecurityGroupArgs

        ResourceGroupName = resourceGroup.Name,
        Tags = 
        {
            { "revision", "1" },
        },
})

Imagine that the NSG has been deployed, and then we change the "revision" tag to force an update of the NSG.

The provider would a generate REST body containing a spurious securityRules container node, apparently due to how writePropertiesToBody ranges over the whole property path, and how it misinterprets the propertyName field.

I0602 14:39:34.104400   43876 eventsink.go:59] {"location":"WestUS2","properties":{"securityRules":{}},"tags":{"revision":"2"}}
I0602 14:39:34.617896   43876 eventsink.go:59]    RESPONSE Status: 400 Bad Request

To make writePropertiesToBody easier to understand and be more reliable, this PR brings in a simpler approach. For each property path, we get the value from the remote state, and if present, we set the value into the body parameters. This approach avoids prematurely creating intermediate container nodes. Note that nestedField and setNestedField implementations were drawn from: https://github.com/kubernetes/apimachinery/blob/a925cd7fb3fd137a00c75870fbf7381f038993d0/pkg/apis/meta/v1/unstructured/helpers.go#L54

With the fix, we see the actual securityRules be maintained:

I0604 10:18:04.029199   72054 eventsink.go:59] {"location":"WestUS2","properties":{"securityRules":[]},"tags":{"revision":"2"}}

Some test cases were updated to use the propertyPath structure correctly. Looking at findUnsetPropertiesToMaintain that generates these propertyPath structs, the path field always contains the full path, and propertyName contains the path element that wasn't found in the input map (which may be an intermediate node like properties). Also, propertyName is not actually used anymore, is purely informational.

Closes #4094

[github-actions]

Does the PR have any schema changes?

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

[codecov]

Codecov Report

Attention: Patch coverage is 81.63265% with 9 lines in your changes missing coverage. Please review.

Project coverage is 58.08%. Comparing base (b08c8f0) to head (d4a08c0).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
provider/pkg/provider/crud/crud.go	81.63%	8 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4177      +/-   ##
==========================================
- Coverage   58.08%   58.08%   -0.01%     
==========================================
  Files          84       84              
  Lines       13535    13546      +11     
==========================================
+ Hits         7862     7868       +6     
- Misses       5081     5085       +4     
- Partials      592      593       +1     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[blampe]

It seems reasonable but Daniel should also take a look.

All the talk of deep copying makes me nervous some other kind of mutation bug could sneak in, but I don't know enough about this.

[EronWright]

This change is part of the following stack:

• cleanup for maintaining subresource properties #4177 ◀
  • Preserve sub-resources in state on refresh #4178

_{Change managed by git-spice.}

[danielrbradley]

I'm a little nervous that the tests have changed in the same commit as the refactor work. Was the behavour documented by the tests incorrect or was there another reason for the test reworks?

I can't see anything obviously wrong about the refactored method - it's certainly nicer split into the smaller methods.

[danielrbradley]

Approved with a final cleanup of the unused propertyName struct property 👍

[pulumi-bot]

This PR has been shipped in release v3.5.1.