azurerm_storage_account - Mark shared_properties.smb as O+C

[magodo]

For premium file storage account, even there is no special config for the shared_properties, the smb will be returned with multichannel.enabled = false. This causing plan diff as is reported by #21182. Making the smb block as O+C is one solution. Alternatively, we can guide the users to explicitly ignore that change.

Fix #21182

Test

💤  TF_ACC=1 go test -v -timeout=20h ./internal/services/storage -run=TestAccStorageAccount_emptySharePropertiesPremiumFileStorage
=== RUN   TestAccStorageAccount_emptySharePropertiesPremiumFileStorage
=== PAUSE TestAccStorageAccount_emptySharePropertiesPremiumFileStorage
=== CONT  TestAccStorageAccount_emptySharePropertiesPremiumFileStorage
--- PASS: TestAccStorageAccount_emptySharePropertiesPremiumFileStorage (170.23s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/storage       170.241s

[tombuildsstuff]

hi @magodo

Thanks for this PR - taking a look through here we'll want to update the flatten function to ensure that we only flatten a value for this item when a field within the block has a value. Whilst it's unfortunate that the API returns a value for this block when nothing's configured within it, we'll need to normalise that value within the flatten function instead.

At this point in time we shouldn't really be marking any new fields as Optional & Computed, since we're going to need to change these in the future so that these aren't Computed (and instead update the resource to use a split Create and Update method, so that we can support conditional updates via d.HasChanges, which allows users to specify ignore_changes.

As such would you mind updating the flatten function to take this into account, which would mean that we can remove computed here?

Thanks!

[tombuildsstuff]

This shouldn't be marked as computed, if all the inner members are empty/have a default value of false (in the case of multichannel_enabled) then we should be returning an empty interface when flattening the block - so can we update this here instead?

We shouldn't really be marking any new fields as Optional & Computed at this point, since we're going to need to undo that in the future (instead, using a Delta/Patch Update method, users should specify ignore_changes if they wish to ignore the value for a field - however we need to flatten these correctly, per-above).

As such would you mind updating this?

[magodo]

If I understand correctly, you meant to add something similar to:

+       // The service API returns smb for premium file storage account no matter it is specified in the creation request.
+       // The returned default value in this case only has the `smb.multichannel.enabled = false`.
+       // We need to explicitly check for above case to ensure we didn't write this "unset" block back to the state file, which wil result into a plan diff.
+       if input.Versions == nil && input.ChannelEncryption == nil && input.AuthenticationMethods == nil && input.KerberosTicketEncryption == nil {
+               if input.Multichannel == nil {
+                       return []interface{}{}
+               }
+               if input.Multichannel.Enabled == nil || !*input.Multichannel.Enabled {
+                       return []interface{}{}
+               }
+       }

at the head of the flattenedSharePropertiesSMB.

Though that can solve the reported issue when users didn't specify the smb block, whilst it will break the other half of users who have explicitly specified the smb.multichannel_enabled = false as below:

  share_properties {
    smb {
      multichannel_enabled = false
    }
    retention_policy {
      days = 5
    }
  }

The terraform plan then complains:

      ~ share_properties {

          + smb {
              + multichannel_enabled = false
            }

            # (1 unchanged block hidden)
        }

        # (1 unchanged block hidden)
    }

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

[tombuildsstuff]

in this case this configuration shouldn't be valid, if the share_properties block is specified then a field within it should be (or the block should be omitted) - can we remove this (and update the schema to ensure that at least one of the items within this block is set?)

[magodo]

Adding the AtLeastOneOf will breaks the existing test case: https://github.com/hashicorp/terraform-provider-azurerm/pull/21226/files#diff-4f9906e7fa788bca7bf275f811f2bb83e477040899f0da7b51ee414c8175340dL4355, where we already has supported the empty share_properties. This will then introduce a breaking change.

[tombuildsstuff]

LGTM 👍

[stephybun]

@magodo ran the tests one last time before merging and found this test failing. Does this test need to have an ignore_changes added to it as part of this fix?

=== CONT  TestAccStorageAccount_smbMultichannel
    testcase.go:113: Step 3/4 error: After applying this test step, the plan was not empty.
        stdout:
        Terraform used the selected providers to generate the following execution
        plan. Resource actions are indicated with the following symbols:
          ~ update in-place
        Terraform will perform the following actions:
          # azurerm_storage_account.test will be updated in-place
          ~ resource "azurerm_storage_account" "test" {
                id                                = "/subscriptions/*******/resourceGroups/acctestRG-storage-231211153651096957/providers/Microsoft.Storage/storageAccounts/unlikely23exst2acctiolle"
                name                              = "unlikely23exst2acctiolle"
                tags                              = {}
                # (28 unchanged attributes hidden)
              ~ share_properties {
                  + smb {
                      + multichannel_enabled = false
                    }
                }
                # (1 unchanged block hidden)
            }
        Plan: 0 to add, 1 to change, 0 to destroy.
--- FAIL: TestAccStorageAccount_smbMultichannel (179.91s)
FAIL

[magodo]

@stephybun Good catch! With my above change, it will unfortunately break existing config that have the multichannel_enabled explicitly specified, where the plan will introduce a diff.

Given this, do you think we shall revert the change for this pr, and guide the users to use the ignore_changes for this specific case. Meanwhile, we can keep the acctest in this pr to track this case.

[stephybun]

@magodo thanks for the suggestions, I agree I think we should guide users to use ignore_changes on this block.

[magodo]

@stephybun I've made the change.

Test

💤  TF_ACC=1 go test -v -timeout=20h ./internal/services/storage -run='TestAccStorageAccount_minimalSharePropertiesPremiumFileStorage'
=== RUN   TestAccStorageAccount_minimalSharePropertiesPremiumFileStorage
=== PAUSE TestAccStorageAccount_minimalSharePropertiesPremiumFileStorage
=== CONT  TestAccStorageAccount_minimalSharePropertiesPremiumFileStorage
--- PASS: TestAccStorageAccount_minimalSharePropertiesPremiumFileStorage (159.46s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/storage       159.474s

[stephybun]

Thanks @magodo 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.