Don't add empty objects that may be of the wrong type when maintaining subresource properties#3040
Merged
Merged
Conversation
Contributor
Does the PR have any schema changes?Looking good! No breaking changes found. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3040 +/- ##
==========================================
+ Coverage 60.66% 60.70% +0.03%
==========================================
Files 66 66
Lines 11024 11028 +4
==========================================
+ Hits 6688 6694 +6
+ Misses 3788 3787 -1
+ Partials 548 547 -1 ☔ View full report in Codecov by Sentry. |
danielrbradley
left a comment
danielrbradley
left a comment
Contributor
There was a problem hiding this comment.
What you're writing makes sense (not creating empty containers) but I think some requests require the empty container to be maintained.
I think the previous issue was #2578 and was fixed by #2856
Though is this fine because the required containers is applied after the missing properties so would still be added back in later?
danielrbradley
approved these changes
Jan 25, 2024
danielrbradley
left a comment
danielrbradley
left a comment
Contributor
There was a problem hiding this comment.
Existing integration tests are passing so we should be good 👍
Contributor
Author
|
Verified locally on the exact repro of #3036: |
This was referenced Jun 3, 2025
EronWright
added a commit
that referenced
this pull request
Jun 5, 2025
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](https://github.com/pulumi/pulumi-azure-native/pull/4177/files#diff-71d16d91f35581e90d853408aa33c75e1de68e76a329c82cf6beb04efa67f9c7R157-R177)). Note that the problem is not a 3.x regression, it repros in 2.x. For example, given a basic NSG: ```cs 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. <img width="631" alt="image" src="https://github.com/user-attachments/assets/1b80edd8-4c56-4043-afb8-8dcec268926a" /> Closes #4094
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue #3036 happens because of the logic to maintain unset sub-resource properties.
In the Update path, in
maintainSubResourcePropertiesIfNotSet->writePropertiesToBody, the property to maintain isproperties. privateEndpointConnections. The remote state doesn't have it, either. So the code adds an empty containerprivateEndpointConnections: map[string]anyto the body, then the algorithm stops as there's no more remote state to traverse.However,
privateEndpointConnections: {}is wrong because this property is of type array, and should beprivateEndpointConnections: [].This PR removes the addition of empty containers when there's no corresponding remote state. This does change existing logic, as one changed unit test shows. As far as I can see, the new algorithm makes more sense: leaving out the empty container matches the remote state.
Resolves #3036