r/storage_account: defaulting the value for dns_endpoint_type

[tombuildsstuff]

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 "+1" or "me too" comments, they generate extra noise for PR followers and do not help prioritize for review

Description

This field defaults to null if unset in the API response, therefore needs to be explicitly defaulted on our side

Fixes #25364

[neiser]

@tombuildsstuff Thanks for fixing this so quickly. As #22583 was quite a dangerous change and was apparently released without notice, is there any further plan to devise automated checks protecting against such serious regressions?

[stephybun]

Thanks @tombuildsstuff LGTM 👍

[tombuildsstuff]

@neiser

As #22583 was quite a dangerous change and was apparently released without notice, is there any further plan to devise automated checks protecting against such serious regressions?

The root cause of this one is that the API returns null rather than a default value for this field when it's not explicitly configured. We run (a bunch) of automated tests, however, given that design choice this would have been hard to capture without doing upgrade testing (i.e. provision on the latest release [last week] and then against the branch introducing this change - and check there's no diff) but that's not possible to do in an automated manner at this point in time.

I suspect that were this resource fully using hashicorp/go-azure-sdk rather than the legacy Azure/azure-sdk-for-go then this bug would have been caught during development, however. This is because the legacy Azure/azure-sdk-for-go outputs this value as an string constant (that crucially is not a pointer/reference):

terraform-provider-azurerm/vendor/github.com/Azure/azure-sdk-for-go/services/storage/mgmt/2021-09-01/storage/models.go

Lines 640 to 641 in 571e6d6

	// DNSEndpointType - Allows you to specify the type of endpoint. Set this to AzureDNSZone to create a large number of accounts in a single subscription, which creates accounts in an Azure DNS Zone and the endpoint URL will have an alphanumeric DNS Zone identifier. Possible values include: 'DNSEndpointTypeStandard', 'DNSEndpointTypeAzureDNSZone'
	DNSEndpointType DNSEndpointType `json:"dnsEndpointType,omitempty"`

On the other hand hashicorp/go-azure-sdk correctly outputs this as an optional string constant, which would have meant we'd have done a change similar to this PR during development:

terraform-provider-azurerm/vendor/github.com/hashicorp/go-azure-sdk/resource-manager/storage/2023-01-01/storageaccounts/model_storageaccountproperties.go

Line 24 in 571e6d6

DnsEndpointType *DnsEndpointType `json:"dnsEndpointType,omitempty"`

Whilst it's disappointing this bug slipped through (and we'll ship a fix once the tests have passed) - to be honest the behaviour of the Storage Accounts API is problematic here - if there's an implied default value for a field then (IMO) it should be returned rather than being null (so in this case anything created prior to v3.97.0 has a default value of Standard from the API, therefore imo the API should be returning that).

All that to say, apologies for the inconvenience here - it's unfortunate this slipped through, however I suspect that moving this resource towards hashicorp/go-azure-sdk would probably have helped pick this up during development (but it's hard to test this effectively in an automated manner given the behaviour of the API) - at any rate we'll look to get this resource fully ported over as soon as time allows, to try and help avoid this type of issue in the future.

Thanks!

[github-actions]

This functionality has been released in v3.97.1 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

[ChrisPetr0]

Shouldn't release 3.97.0 be yanked completely? We don't have our version pinned and got caught with 3.97.0 attempting to wipe out multiple storage accounts. I'd hate to have someone inadvertently lose a production workload on this one. I appreciate the fix was released so quickly. Thanks

[DTurin-IH]

Same issue, multiple storage accounts wiped out with 3.97.0, should have been yanked 4 sure. Thumbed up.

[neiser]

@tombuildsstuff

without doing upgrade testing (i.e. provision on the latest release [last week] and then against the branch introducing this change - and check there's no diff) but that's not possible to do in an automated manner at this point in time.

I'm sure you're aware that the vast majority of users use the azurerm provider in exactly that way: Have infrastructure created with an old release, and then continue managing this infrastructure state with the newly released provider. So, in my opinion, having automated tests exactly for that main use case of your "customers" should be a very high priority.

I've built such end-to-end tests for infrastructure-as-code systems already, and they're tough when it comes to details, such as making them finish in less than a day without much flakiness 😺 But they really do pay off! Is there some public place where I could have a look at the current state of affairs regarding tests? I suppose you already have an Azure subscription/account to run azurerm provider against "the real thing" (aka Azure APIs)?

I also agree that you should really delete the release 3.97.0 as proposed by @ChrisPetr0.

[tom-reinders]

@tombuildsstuff Running a upgrade test every week before release sounds to me like something very useful to stop azurerm from making breaking changes in minor version changes all the time.

[marcindulak]

There is a another, recent #25244 which results in a breaking change due to handling of null values, opening the firewall of the app service.

I opened a meta issue #25394 about possibilities to improve this situation.

[phoehnel]

This currently leads to terraform re-creating storage-accounts on provider-update when dns_endpoint_type has been null before 3.97.1 and no explicit value is set in the configuration. This probably isn't ideal and should be considered a breaking change.

[sai-gunaranjan]

#25424

[manicminer]

@phoehnel I'm trying to look into this - would you be able to add any relevant reproduction info to #25424, as so far I am unable to reproduce this with v3.97.1. Thanks!

[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.