azurerm_virtual_network - Remove read-only properties from subnet payloads to avoid ARM API limit errors

[ezhong-msft]

Community Note

• Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
• Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

Doing an update on virtual network in Terraform involves pulling the existing state from the API. This contains some properties that aren't tracked by Terraform and are read-only so are not required for an update. For large vnet architectures (e.g. >10k private endpoints), these extraneous properties being passed back in the update payload can cause a RequestContentTooLarge ARM API error as it exceeds the 4MB payload limit, blocking any updating occuring.

PR Checklist

• I have followed the guidelines in our Contributing Documentation.
• I have checked to ensure there aren't other open Pull Requests for the same update/change.
• I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
• n\a I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
• I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
  For example: “resource_name_here - description of change e.g. adding property new_property_name_here”

Changes to existing Resource / Data Source

• I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
• n/a I have written new tests for my resource or datasource changes & updated any relevant documentation.
• I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
• n\a (For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

• My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

https://hashicorp.teamcity.com/buildConfiguration/TF_AzureRM_AZURERM_SERVICE_PUBLIC_NETWORK/510686?buildTab=tests

Some other resources with dependencies on azurerm_virtual_network:
https://hashicorp.teamcity.com/buildConfiguration/TF_AzureRM_AZURERM_SERVICE_PUBLIC_APPSERVICE/510690?buildTab=overview
https://hashicorp.teamcity.com/buildConfiguration/TF_AzureRM_AZURERM_SERVICE_PUBLIC_NETWORK/510688?buildTab=tests
https://hashicorp.teamcity.com/buildConfiguration/TF_AzureRM_AZURERM_SERVICE_PUBLIC_COMPUTE/510689?buildTab=tests
Failures seem to be pre-existing

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

• azurerm_resource - support for the thing1 property [GH-00000]

This is a (please select all that apply):

• Bug Fix
• New Feature (ie adding a service, resource, or data source)
• Enhancement
• Breaking Change

Related Issue(s)

Fixes #0000

AI Assistance Disclosure

• AI Assisted - This contribution was made by, or with the assistance of, AI/LLMs

Rollback Plan

If a change needs to be reverted, we will publish an updated version of the provider.

Changes to Security Controls

Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

[ms-henglu]

LGTM!

[WodansSon]

Hi @ezhong-msft, this mostly LGTM, but I did have a question about the implementation. I just want to make sure I understand it correctly without a whole lot of context. I believe I get it but I wanted to touch base with you to make sure I fully grok the issue. 🚀

[WodansSon]

So we only want to remove these read-only properties if the subnet field hasn't change because we are getting the payload from the model being returned from Azure instead of building it in the expand function which means it will contain all of the read-only fields, is that correct?

[ezhong-msft]

I thought the expand builds the subnet from scratch without the read-only fields but seems like it actually fetches the subnet from API too... I don't think the else-if condition is even necessary anyway so I'll remove it and clear it in all cases

[WodansSon]

@ezhong-msft, thank you for pushing those changes, this LGTM now! 🚀

[jackofallops]

Thanks @ezhong-msft - One comment to take a look at if you could, then I think this will be fine.

[jackofallops]

payload.Properties could be nil here, causing a crash. Can we move this reset of the R/O properties to inside the correctly nil-checked if payload.Properties != nil && payload.Properties.Subnets != nil { below so we don't need to perform the same check twice?

[ezhong-msft]

I would have thought it would be captured by the check above.

https://github.com/ezhong-msft/terraform-provider-azurerm/blob/54d470a3fadca1c4aaa7ce6bcc2bf15ef72bb24a/internal/services/network/virtual_network_resource.go#L515-L517

But have moved it down to avoid the duplicate check and looping through the subnets 👍

[jackofallops]

Thanks @ezhong-msft - This LGTM now 👍

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.