azurerm_storage_account - Ensure infrastructure_encryption_enabled property is included during update

[baaym]

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

When a Storage Account's customer_managed_key is updated, the AzureRM provider did not take into account the infrastructure_encryption_enabled property for the Storage Account encryption settings. This change ensures that infrastructure_encryption_enabled is now included in the update as well, with the same behavior as during resource creation.

This was notably an issue with Azure's policy engine. During resource update, the infrastructure_encryption_enabled property wasn't included at all which most likely meant nothing would change on Azure's side. However any policies that are in place could still expect this property to be included and set to true (as was the case with us).

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.
• 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).
• I have written new tests for my resource or datasource changes & updated any relevent documentation.
• I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
• (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)

I'm not familiar enough with this codebase to determine how to properly add a test case for this change. I have however verified this fix through local execution.

Before the fix:

╷
│ Error: updating Storage Account (Subscription: "xxx"
│ Resource Group Name: "xxx"
│ Storage Account Name: "xxx"): performing Create: unexpected status 403 (403 Forbidden) with error: RequestDisallowedByPolicy: Resource 'xxx' was disallowed by policy. Policy identifiers: [REDACTED].
│ 
│   with azurerm_storage_account.xxx-storage,
│   on storage-account.tf line 11, in resource "azurerm_storage_account" "xxx-storage":
│   11: resource "azurerm_storage_account" "xxx-storage" {
│ 
╵

After this fix:

Terraform will perform the following actions:

  # azurerm_storage_account.xxx-storage will be updated in-place
  ~ resource "azurerm_storage_account" "xxx-storage" {
        id                                 = "/subscriptions/xxx/resourceGroups/xxx/providers/Microsoft.Storage/storageAccounts/xxx"
        name                               = "xxx"
        # (97 unchanged attributes hidden)

      ~ customer_managed_key {
          - key_vault_key_id          = "https://xxx-kv.vault.azure.net/keys/xxx-cmk-d" -> null
          + managed_hsm_key_id        = "https://xxx.managedhsm.azure.net/keys/xxx-d-sa-key/xxx"
            # (1 unchanged attribute hidden)
        }

        # (6 unchanged blocks hidden)
    }

Plan: 0 to add, 1 to change, 0 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

azurerm_storage_account.xxx-storage: Modifying... [id=/subscriptions/xxx/resourceGroups/xxx/providers/Microsoft.Storage/storageAccounts/xxx]
azurerm_storage_account.xxx-storage: Still modifying... [id=/subscriptions/xxx-...t.Storage/storageAccounts/xxx, 10s elapsed]
azurerm_storage_account.xxx-storage: Modifications complete after 19s [id=/subscriptions/xxx/resourceGroups/xxx/providers/Microsoft.Storage/storageAccounts/xxx]

Apply complete! Resources: 0 added, 1 changed, 0 destroyed.

Change Log

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

• azurerm_storage_account - fix omission of infrastructure_encryption_enabled property during CMK update

This is a (please select all that apply):

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

Note

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

[baaym]

Hi all!

Since it's almost been 3 weeks since opening this pull request I'm wondering if there's anything I've missed? In case there are still things required before this PR can be processed please let me know!

[stephybun]

Thanks for this PR @baaym!

This appears to only be an issue when the CMK is updated since it overwrites the entire Encryption model in the payload which we populate with the existing values in the update function.

I think this can be fixed with fewer lines of code and with potentially less confusion since this property is actually ForceNew.

This is the section of the code that is overwriting and clearing RequireInfrastructureEncryption

terraform-provider-azurerm/internal/services/storage/storage_account_resource.go

Lines 1718 to 1727 in 067db3f

	if d.HasChange("customer_managed_key") {
	queueEncryptionKeyType := storageaccounts.KeyType(d.Get("queue_encryption_key_type").(string))
	tableEncryptionKeyType := storageaccounts.KeyType(d.Get("table_encryption_key_type").(string))
	encryptionRaw := d.Get("customer_managed_key").([]interface{})
	encryption, err := expandAccountCustomerManagedKey(ctx, keyVaultClient, id.SubscriptionId, encryptionRaw, accountTier, accountKind, *expandedIdentity, queueEncryptionKeyType, tableEncryptionKeyType)
	if err != nil {
	return fmt.Errorf("expanding `customer_managed_key`: %+v", err)
	}
	props.Encryption = encryption
	}

Could you add the following line below 1726 and verify locally whether that also fixes the issue for you?
props.Encryption.RequireInfrastructureEncryption = existing.Model.Properties.Encryption.RequireInfrastructureEncryption

[baaym]

Thanks for looking into this PR @stephybun!

The suggested change is indeed much simpler, and I just verified it does the job as well. Many thanks!

[stephybun]

Happy to hear that fixes your issue! I left some additional suggestions around nil checking and adding a comment for context. If you could add those in then I can kick off the tests and then this should be good to go!

[stephybun]

To be safe I would suggest only setting this if the existing RequireInfrastructureEncryption is not nil, also to add a comment above this with some context

Suggested change

	props.Encryption.RequireInfrastructureEncryption = existing.Model.Properties.Encryption.RequireInfrastructureEncryption
	// when updating CMK the existing value for `RequireInfrastructureEncryption` gets overwritten which results in an error from the API so we set this back into encryption after it's been overwritten by this update
	if v := existing.Model.Properties.Encryption; if v != nil && v.RequireInfrastructureEncryption != nil {
	props.Encryption.RequireInfrastructureEncryption = v.RequireInfrastructureEncryption
	}

[baaym]

@stephybun I have incorporated your changes (with slight modifications due to things like GitHub UI formatting). Also this change was tested locally and works without issues.

Thanks again!

[stephybun]

Thanks @baaym LGTM 🌵

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