From 711c0e68d3c83eba6dd7643a5d358732da9398c3 Mon Sep 17 00:00:00 2001 From: Catriona Date: Mon, 16 Aug 2021 15:17:35 +0100 Subject: [PATCH 1/5] support for property allow_shared_key_access in storage_account_resource --- .../storage/storage_account_resource.go | 177 +++++++++--------- .../storage/storage_account_resource_test.go | 86 +++++++++ 2 files changed, 179 insertions(+), 84 deletions(-) diff --git a/internal/services/storage/storage_account_resource.go b/internal/services/storage/storage_account_resource.go index 23a8209bfbf0..b0247e09e942 100644 --- a/internal/services/storage/storage_account_resource.go +++ b/internal/services/storage/storage_account_resource.go @@ -21,6 +21,7 @@ import ( msiValidate "github.com/hashicorp/terraform-provider-azurerm/internal/services/msi/validate" "github.com/hashicorp/terraform-provider-azurerm/internal/services/network" "github.com/hashicorp/terraform-provider-azurerm/internal/services/storage/migration" + "github.com/hashicorp/terraform-provider-azurerm/internal/services/storage/parse" "github.com/hashicorp/terraform-provider-azurerm/internal/services/storage/validate" "github.com/hashicorp/terraform-provider-azurerm/internal/tags" "github.com/hashicorp/terraform-provider-azurerm/internal/tf/pluginsdk" @@ -47,8 +48,10 @@ func resourceStorageAccount() *pluginsdk.Resource { 1: migration.AccountV1ToV2{}, }), - // TODO: replace this with an importer which validates the ID during import - Importer: pluginsdk.DefaultImporter(), + Importer: pluginsdk.ImporterValidatingResourceId(func(id string) error { + _, err := parse.StorageAccountID(id) + return err + }), Timeouts: &pluginsdk.ResourceTimeout{ Create: pluginsdk.DefaultTimeout(60 * time.Minute), @@ -239,6 +242,12 @@ func resourceStorageAccount() *pluginsdk.Resource { Default: false, }, + "allow_shared_key_access": { + Type: pluginsdk.TypeBool, + Optional: true, + Default: true, + }, + "network_rules": { Type: pluginsdk.TypeList, Optional: true, @@ -885,8 +894,11 @@ func resourceStorageAccountCreate(d *pluginsdk.ResourceData, meta interface{}) e ctx, cancel := timeouts.ForCreate(meta.(*clients.Client).StopContext, d) defer cancel() - storageAccountName := d.Get("name").(string) + subscriptionId := meta.(*clients.Client).Account.SubscriptionId resourceGroupName := d.Get("resource_group_name").(string) + storageAccountName := d.Get("name").(string) + + id := parse.NewStorageAccountID(subscriptionId, resourceGroupName, storageAccountName) locks.ByName(storageAccountName, storageAccountResourceName) defer locks.UnlockByName(storageAccountName, storageAccountResourceName) @@ -910,15 +922,11 @@ func resourceStorageAccountCreate(d *pluginsdk.ResourceData, meta interface{}) e isHnsEnabled := d.Get("is_hns_enabled").(bool) nfsV3Enabled := d.Get("nfsv3_enabled").(bool) allowBlobPublicAccess := d.Get("allow_blob_public_access").(bool) + allowSharedKeyAccess := d.Get("allow_shared_key_access").(bool) accountTier := d.Get("account_tier").(string) replicationType := d.Get("account_replication_type").(string) storageType := fmt.Sprintf("%s_%s", accountTier, replicationType) - // this is the default behavior for the resource if the attribute is nil - // we are making this change in Terraform https://github.com/hashicorp/terraform-provider-azurerm/issues/11689 - // because the portal UI team has a bug in their code ignoring the ARM API documention which state that nil is true - // TODO: Remove code when Portal UI team fixes their code - allowSharedKeyAccess := true parameters := storage.AccountCreateParameters{ Location: &location, @@ -932,8 +940,7 @@ func resourceStorageAccountCreate(d *pluginsdk.ResourceData, meta interface{}) e NetworkRuleSet: expandStorageAccountNetworkRules(d, tenantId), IsHnsEnabled: &isHnsEnabled, EnableNfsV3: &nfsV3Enabled, - // TODO: Remove AllowSharedKeyAcces assignment when Portal UI team fixes their code (e.g. nil is true) - AllowSharedKeyAccess: &allowSharedKeyAccess, + AllowSharedKeyAccess: &allowSharedKeyAccess, }, } @@ -1029,17 +1036,8 @@ func resourceStorageAccountCreate(d *pluginsdk.ResourceData, meta interface{}) e return fmt.Errorf("waiting for Azure Storage Account %q to be created: %+v", storageAccountName, err) } - account, err := client.GetProperties(ctx, resourceGroupName, storageAccountName, "") - if err != nil { - return fmt.Errorf("retrieving Azure Storage Account %q: %+v", storageAccountName, err) - } - - if account.ID == nil { - return fmt.Errorf("Cannot read Storage Account %q (resource group %q) ID", - storageAccountName, resourceGroupName) - } - log.Printf("[INFO] storage account %q ID: %q", storageAccountName, *account.ID) - d.SetId(*account.ID) + log.Printf("[INFO] storage account %q ID: %q", storageAccountName, id) + d.SetId(id.ID()) if val, ok := d.GetOk("blob_properties"); ok { // FileStorage does not support blob settings @@ -1144,12 +1142,13 @@ func resourceStorageAccountUpdate(d *pluginsdk.ResourceData, meta interface{}) e ctx, cancel := timeouts.ForUpdate(meta.(*clients.Client).StopContext, d) defer cancel() - id, err := azure.ParseAzureResourceID(d.Id()) + id, err := parse.StorageAccountID(d.Id()) if err != nil { return err } - storageAccountName := id.Path["storageAccounts"] + resourceGroupName := id.ResourceGroup + storageAccountName := id.Name locks.ByName(storageAccountName, storageAccountResourceName) defer locks.UnlockByName(storageAccountName, storageAccountResourceName) @@ -1165,36 +1164,43 @@ func resourceStorageAccountUpdate(d *pluginsdk.ResourceData, meta interface{}) e } } - // AllowSharedKeyAccess can only be true due to issue: https://github.com/hashicorp/terraform-provider-azurerm/issues/11460 - // if value is nil that brakes the Portal UI as reported in https://github.com/hashicorp/terraform-provider-azurerm/issues/11689 - // currently the Portal UI reports nil as false, and per the ARM API documentation nil is true. This manafests itself in the Portal UI - // when a storage account is created by terraform that the AllowSharedKeyAccess is Disabled when it is actually Enabled, thus confusing out customers - // to fix this, I have added this code to explicitly to set the value to true if is nil to workaround the Portal UI bug for our customers. - // this is designed as a passive change, meaning the change will only take effect when the existing storage account is modified in some way if the - // account already exists. since I have also switched up the default behavor for net new storage accounts to always set this value as true, this issue - // should automatically correct itself over time with these changes. - // TODO: Remove code when Portal UI team fixes their code - existing, err := client.GetProperties(ctx, resourceGroupName, storageAccountName, "") - if err == nil { - if sharedKeyAccess := existing.AccountProperties.AllowSharedKeyAccess; sharedKeyAccess == nil { - allowSharedKeyAccess := true + var allowSharedKeyAccess interface{} = true + if d.HasChange("allow_shared_key_access") { + allowSharedKeyAccess = d.Get("allow_shared_key_access") - opts := storage.AccountUpdateParameters{ - AccountPropertiesUpdateParameters: &storage.AccountPropertiesUpdateParameters{ - AllowSharedKeyAccess: &allowSharedKeyAccess, - }, + // If AllowSharedKeyAccess is nil that breaks the Portal UI as reported in https://github.com/hashicorp/terraform-provider-azurerm/issues/11689 + // currently the Portal UI reports nil as false, and per the ARM API documentation nil is true. This manifests itself in the Portal UI + // when a storage account is created by terraform that the AllowSharedKeyAccess is Disabled when it is actually Enabled, thus confusing out customers + // to fix this, I have added this code to explicitly to set the value to true if is nil to workaround the Portal UI bug for our customers. + // this is designed as a passive change, meaning the change will only take effect when the existing storage account is modified in some way if the + // account already exists. since I have also switched up the default behaviour for net new storage accounts to always set this value as true, this issue + // should automatically correct itself over time with these changes. + // TODO: Remove code when Portal UI team fixes their code + } else { + existing, err := client.GetProperties(ctx, resourceGroupName, storageAccountName, "") + if err == nil { + if sharedKeyAccess := existing.AccountProperties.AllowSharedKeyAccess; sharedKeyAccess != nil { + allowSharedKeyAccess = sharedKeyAccess } - if _, err := client.Update(ctx, resourceGroupName, storageAccountName, opts); err != nil { - return fmt.Errorf("updating Azure Storage Account AllowSharedKeyAccess %q: %+v", storageAccountName, err) - } + } else { + // Should never hit this, but added due to an abundance of caution + return fmt.Errorf("retrieving Azure Storage Account %q AllowSharedKeyAccess: %+v", storageAccountName, err) } - } else { - // Should never hit this, but added due to an abundance of caution - return fmt.Errorf("retrieving Azure Storage Account %q AllowSharedKeyAccess: %+v", storageAccountName, err) } // TODO: end remove changes when Portal UI team fixed their code + allowSharedKeyAccessValue := allowSharedKeyAccess.(bool) + opts := storage.AccountUpdateParameters{ + AccountPropertiesUpdateParameters: &storage.AccountPropertiesUpdateParameters{ + AllowSharedKeyAccess: &allowSharedKeyAccessValue, + }, + } + + if _, err := client.Update(ctx, resourceGroupName, storageAccountName, opts); err != nil { + return fmt.Errorf("updating Azure Storage Account AllowSharedKeyAccess %q: %+v", storageAccountName, err) + } + if d.HasChange("account_replication_type") { sku := storage.Sku{ Name: storage.SkuName(storageType), @@ -1511,20 +1517,16 @@ func resourceStorageAccountRead(d *pluginsdk.ResourceData, meta interface{}) err ctx, cancel := timeouts.ForRead(meta.(*clients.Client).StopContext, d) defer cancel() - id, err := azure.ParseAzureResourceID(d.Id()) - if err != nil { - return err - } - name := id.Path["storageAccounts"] - resGroup := id.ResourceGroup + storageAccountName := d.Get("name").(string) + resourceGroupName := d.Get("resource_group_name").(string) - resp, err := client.GetProperties(ctx, resGroup, name, "") + resp, err := client.GetProperties(ctx, resourceGroupName, storageAccountName, "") if err != nil { if utils.ResponseWasNotFound(resp.Response) { d.SetId("") return nil } - return fmt.Errorf("reading the state of AzureRM Storage Account %q: %+v", name, err) + return fmt.Errorf("reading the state of AzureRM Storage Account %q: %+v", storageAccountName, err) } // handle the user not having permissions to list the keys @@ -1535,7 +1537,7 @@ func resourceStorageAccountRead(d *pluginsdk.ResourceData, meta interface{}) err d.Set("primary_access_key", "") d.Set("secondary_access_key", "") - keys, err := client.ListKeys(ctx, resGroup, name, storage.Kerb) + keys, err := client.ListKeys(ctx, resourceGroupName, storageAccountName, storage.Kerb) if err != nil { // the API returns a 200 with an inner error of a 409.. var hasWriteLock bool @@ -1548,12 +1550,12 @@ func resourceStorageAccountRead(d *pluginsdk.ResourceData, meta interface{}) err } if !hasWriteLock && !doesntHavePermissions { - return fmt.Errorf("listing Keys for Storage Account %q (Resource Group %q): %s", name, resGroup, err) + return fmt.Errorf("listing Keys for Storage Account %q (Resource Group %q): %s", storageAccountName, resourceGroupName, err) } } d.Set("name", resp.Name) - d.Set("resource_group_name", resGroup) + d.Set("resource_group_name", resourceGroupName) if location := resp.Location; location != nil { d.Set("location", azure.NormalizeLocation(*location)) } @@ -1649,6 +1651,12 @@ func resourceStorageAccountRead(d *pluginsdk.ResourceData, meta interface{}) err if props.LargeFileSharesState != "" { d.Set("large_file_share_enabled", props.LargeFileSharesState == storage.LargeFileSharesStateEnabled) } + + allowSharedKeyAccess := true + if props.AllowSharedKeyAccess != nil { + allowSharedKeyAccess = *props.AllowSharedKeyAccess + } + d.Set("allow_shared_key_access", allowSharedKeyAccess) } if accessKeys := keys.Keys; accessKeys != nil { @@ -1666,27 +1674,27 @@ func resourceStorageAccountRead(d *pluginsdk.ResourceData, meta interface{}) err } storageClient := meta.(*clients.Client).Storage - account, err := storageClient.FindAccount(ctx, name) + account, err := storageClient.FindAccount(ctx, storageAccountName) if err != nil { - return fmt.Errorf("retrieving Account %q: %s", name, err) + return fmt.Errorf("retrieving Account %q: %s", storageAccountName, err) } if account == nil { - return fmt.Errorf("Unable to locate Storage Account %q!", name) + return fmt.Errorf("Unable to locate Storage Account %q!", storageAccountName) } blobClient := storageClient.BlobServicesClient // FileStorage does not support blob settings if resp.Kind != storage.FileStorage { - blobProps, err := blobClient.GetServiceProperties(ctx, resGroup, name) + blobProps, err := blobClient.GetServiceProperties(ctx, resourceGroupName, storageAccountName) if err != nil { if !utils.ResponseWasNotFound(blobProps.Response) { - return fmt.Errorf("reading blob properties for AzureRM Storage Account %q: %+v", name, err) + return fmt.Errorf("reading blob properties for AzureRM Storage Account %q: %+v", storageAccountName, err) } } if err := d.Set("blob_properties", flattenBlobProperties(blobProps)); err != nil { - return fmt.Errorf("setting `blob_properties `for AzureRM Storage Account %q: %+v", name, err) + return fmt.Errorf("setting `blob_properties `for AzureRM Storage Account %q: %+v", storageAccountName, err) } } @@ -1694,21 +1702,21 @@ func resourceStorageAccountRead(d *pluginsdk.ResourceData, meta interface{}) err // FileStorage does not support blob kind, FileStorage Premium is supported if resp.Kind == storage.FileStorage || resp.Kind != storage.BlobStorage && resp.Kind != storage.BlockBlobStorage && resp.Sku != nil && resp.Sku.Tier != storage.Premium { - shareProps, err := fileServiceClient.GetServiceProperties(ctx, resGroup, name) + shareProps, err := fileServiceClient.GetServiceProperties(ctx, resourceGroupName, storageAccountName) if err != nil { if !utils.ResponseWasNotFound(shareProps.Response) { - return fmt.Errorf("reading share properties for AzureRM Storage Account %q: %+v", name, err) + return fmt.Errorf("reading share properties for AzureRM Storage Account %q: %+v", storageAccountName, err) } } if err := d.Set("share_properties", flattenShareProperties(shareProps)); err != nil { - return fmt.Errorf("setting `share_properties `for AzureRM Storage Account %q: %+v", name, err) + return fmt.Errorf("setting `share_properties `for AzureRM Storage Account %q: %+v", storageAccountName, err) } } // queue is only available for certain tier and kind (as specified below) if resp.Sku == nil { - return fmt.Errorf("retrieving Storage Account %q (Resource Group %q): `sku` was nil", name, resGroup) + return fmt.Errorf("retrieving Storage Account %q (Resource Group %q): `sku` was nil", storageAccountName, resourceGroupName) } if resp.Sku.Tier == storage.Standard { @@ -1718,9 +1726,9 @@ func resourceStorageAccountRead(d *pluginsdk.ResourceData, meta interface{}) err return fmt.Errorf("building Queues Client: %s", err) } - queueProps, err := queueClient.GetServiceProperties(ctx, account.ResourceGroup, name) + queueProps, err := queueClient.GetServiceProperties(ctx, account.ResourceGroup, storageAccountName) if err != nil { - return fmt.Errorf("reading queue properties for AzureRM Storage Account %q: %+v", name, err) + return fmt.Errorf("reading queue properties for AzureRM Storage Account %q: %+v", storageAccountName, err) } if err := d.Set("queue_properties", flattenQueueProperties(queueProps)); err != nil { @@ -1735,9 +1743,9 @@ func resourceStorageAccountRead(d *pluginsdk.ResourceData, meta interface{}) err if resp.Kind == storage.StorageV2 || resp.Kind == storage.BlockBlobStorage { storageClient := meta.(*clients.Client).Storage - account, err := storageClient.FindAccount(ctx, name) + account, err := storageClient.FindAccount(ctx, storageAccountName) if err != nil { - return fmt.Errorf("retrieving Account %q: %s", name, err) + return fmt.Errorf("retrieving Account %q: %s", storageAccountName, err) } accountsClient, err := storageClient.AccountsDataPlaneClient(ctx, *account) @@ -1745,10 +1753,10 @@ func resourceStorageAccountRead(d *pluginsdk.ResourceData, meta interface{}) err return fmt.Errorf("building Accounts Data Plane Client: %s", err) } - staticWebsiteProps, err := accountsClient.GetServiceProperties(ctx, name) + staticWebsiteProps, err := accountsClient.GetServiceProperties(ctx, storageAccountName) if err != nil { if staticWebsiteProps.Response.Response != nil && !utils.ResponseWasNotFound(staticWebsiteProps.Response) { - return fmt.Errorf("reading static website for AzureRM Storage Account %q: %+v", name, err) + return fmt.Errorf("reading static website for AzureRM Storage Account %q: %+v", storageAccountName, err) } } @@ -1756,7 +1764,7 @@ func resourceStorageAccountRead(d *pluginsdk.ResourceData, meta interface{}) err } if err := d.Set("static_website", staticWebsite); err != nil { - return fmt.Errorf("setting `static_website `for AzureRM Storage Account %q: %+v", name, err) + return fmt.Errorf("setting `static_website `for AzureRM Storage Account %q: %+v", storageAccountName, err) } return tags.FlattenAndSet(d, resp.Tags) @@ -1768,23 +1776,24 @@ func resourceStorageAccountDelete(d *pluginsdk.ResourceData, meta interface{}) e ctx, cancel := timeouts.ForDelete(meta.(*clients.Client).StopContext, d) defer cancel() - id, err := azure.ParseAzureResourceID(d.Id()) + id, err := parse.StorageAccountID(d.Id()) if err != nil { return err } - name := id.Path["storageAccounts"] - resourceGroup := id.ResourceGroup - locks.ByName(name, storageAccountResourceName) - defer locks.UnlockByName(name, storageAccountResourceName) + resourceGroupName := id.ResourceGroup + storageAccountName := id.Name + + locks.ByName(storageAccountName, storageAccountResourceName) + defer locks.UnlockByName(storageAccountName, storageAccountResourceName) - read, err := client.GetProperties(ctx, resourceGroup, name, "") + read, err := client.GetProperties(ctx, resourceGroupName, storageAccountName, "") if err != nil { if utils.ResponseWasNotFound(read.Response) { return nil } - return fmt.Errorf("retrieving Storage Account %q (Resource Group %q): %+v", name, resourceGroup, err) + return fmt.Errorf("retrieving Storage Account %q (Resource Group %q): %+v", storageAccountName, resourceGroupName, err) } // the networking api's only allow a single change to be made to a network layout at once, so let's lock to handle that @@ -1817,15 +1826,15 @@ func resourceStorageAccountDelete(d *pluginsdk.ResourceData, meta interface{}) e locks.MultipleByName(&virtualNetworkNames, network.VirtualNetworkResourceName) defer locks.UnlockMultipleByName(&virtualNetworkNames, network.VirtualNetworkResourceName) - resp, err := client.Delete(ctx, resourceGroup, name) + resp, err := client.Delete(ctx, resourceGroupName, storageAccountName) if err != nil { if !response.WasNotFound(resp.Response) { - return fmt.Errorf("issuing delete request for Storage Account %q (Resource Group %q): %+v", name, resourceGroup, err) + return fmt.Errorf("issuing delete request for Storage Account %q (Resource Group %q): %+v", storageAccountName, resourceGroupName, err) } } // remove this from the cache - storageClient.RemoveAccountFromCache(name) + storageClient.RemoveAccountFromCache(storageAccountName) return nil } diff --git a/internal/services/storage/storage_account_resource_test.go b/internal/services/storage/storage_account_resource_test.go index caa2aed12103..9065ee6cd547 100644 --- a/internal/services/storage/storage_account_resource_test.go +++ b/internal/services/storage/storage_account_resource_test.go @@ -1016,6 +1016,36 @@ func TestAccAzureRMStorageAccount_shareSoftDelete(t *testing.T) { }) } +func TestAccStorageAccount_allowSharedKeyAccess(t *testing.T) { + data := acceptance.BuildTestData(t, "azurerm_storage_account", "test") + r := StorageAccountResource{} + + data.ResourceTest(t, r, []acceptance.TestStep{ + { + Config: r.allowSharedKeyAccess(data), + Check: acceptance.ComposeTestCheckFunc( + check.That(data.ResourceName).ExistsInAzure(r), + check.That(data.ResourceName).Key("account_tier").HasValue("Standard"), + check.That(data.ResourceName).Key("account_replication_type").HasValue("LRS"), + check.That(data.ResourceName).Key("tags.%").HasValue("1"), + check.That(data.ResourceName).Key("tags.environment").HasValue("production"), + check.That(data.ResourceName).Key("allow_shared_key_access").HasValue("false"), + ), + }, + { + Config: r.allowSharedKeyAccessUpdated(data), + Check: acceptance.ComposeTestCheckFunc( + check.That(data.ResourceName).ExistsInAzure(r), + check.That(data.ResourceName).Key("account_tier").HasValue("Standard"), + check.That(data.ResourceName).Key("account_replication_type").HasValue("LRS"), + check.That(data.ResourceName).Key("tags.%").HasValue("1"), + check.That(data.ResourceName).Key("tags.environment").HasValue("production"), + check.That(data.ResourceName).Key("allow_shared_key_access").HasValue("true"), + ), + }, + }) +} + func (r StorageAccountResource) Exists(ctx context.Context, client *clients.Client, state *pluginsdk.InstanceState) (*bool, error) { id, err := parse.StorageAccountID(state.ID) if err != nil { @@ -2962,3 +2992,59 @@ resource "azurerm_storage_account" "test" { } `, data.RandomInteger, data.Locations.Primary, data.RandomString) } + +func (r StorageAccountResource) allowSharedKeyAccess(data acceptance.TestData) string { + return fmt.Sprintf(` +provider "azurerm" { + features {} + storage_use_azuread = true +} + +resource "azurerm_resource_group" "test" { + name = "acctestRG-storage-%d" + location = "%s" +} + +resource "azurerm_storage_account" "test" { + name = "unlikely23exst2acct%s" + resource_group_name = azurerm_resource_group.test.name + + location = azurerm_resource_group.test.location + account_tier = "Standard" + account_replication_type = "LRS" + allow_shared_key_access = false + + tags = { + environment = "production" + } +} +`, data.RandomInteger, data.Locations.Primary, data.RandomString) +} + +func (r StorageAccountResource) allowSharedKeyAccessUpdated(data acceptance.TestData) string { + return fmt.Sprintf(` +provider "azurerm" { + features {} + storage_use_azuread = true +} + +resource "azurerm_resource_group" "test" { + name = "acctestRG-storage-%d" + location = "%s" +} + +resource "azurerm_storage_account" "test" { + name = "unlikely23exst2acct%s" + resource_group_name = azurerm_resource_group.test.name + + location = azurerm_resource_group.test.location + account_tier = "Standard" + account_replication_type = "LRS" + allow_shared_key_access = true + + tags = { + environment = "production" + } +} +`, data.RandomInteger, data.Locations.Primary, data.RandomString) +} From 02a58f985e81af36bdf5bd8fdea5845d685b316a Mon Sep 17 00:00:00 2001 From: Catriona Date: Mon, 16 Aug 2021 15:24:38 +0100 Subject: [PATCH 2/5] update storage_account docs with allow_shared_key_access --- website/docs/r/storage_account.html.markdown | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/website/docs/r/storage_account.html.markdown b/website/docs/r/storage_account.html.markdown index 450015f7f6b9..d6f42b341731 100644 --- a/website/docs/r/storage_account.html.markdown +++ b/website/docs/r/storage_account.html.markdown @@ -105,6 +105,10 @@ The following arguments are supported: -> **NOTE:** At this time `allow_blob_public_access` is only supported in the Public Cloud, China Cloud, and US Government Cloud. +* `allow_shared_key_access` - Indicates whether the storage account permits requests to be authorized with the account access key via Shared Key. If false, then all requests, including shared access signatures, must be authorized with Azure Active Directory (Azure AD). The default value is `true`. + +~> **Note:** Terraform uses Shared Key Authorisation to provision Storage Containers, Blobs and other items - when Shared Key Access is disabled, you will need to enable [the `storage_use_azuread` flag in the Provider block](https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs#storage_use_azuread) to use Azure AD for authentication, however not all Azure Storage services support Active Directory authentication. + * `is_hns_enabled` - (Optional) Is Hierarchical Namespace enabled? This can be used with Azure Data Lake Storage Gen 2 ([see here for more information](https://docs.microsoft.com/en-us/azure/storage/blobs/data-lake-storage-quickstart-create-account/)). Changing this forces a new resource to be created. -> **NOTE:** This can only be `true` when `account_tier` is `Standard` or when `account_tier` is `Premium` *and* `account_kind` is `BlockBlobStorage` From 1d7479193161c375097238955107bb7fe275f42b Mon Sep 17 00:00:00 2001 From: Catriona Date: Tue, 17 Aug 2021 15:26:49 +0100 Subject: [PATCH 3/5] use parser in storage_account read --- .../storage/storage_account_resource.go | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/internal/services/storage/storage_account_resource.go b/internal/services/storage/storage_account_resource.go index b0247e09e942..e35712bcd496 100644 --- a/internal/services/storage/storage_account_resource.go +++ b/internal/services/storage/storage_account_resource.go @@ -1164,9 +1164,9 @@ func resourceStorageAccountUpdate(d *pluginsdk.ResourceData, meta interface{}) e } } - var allowSharedKeyAccess interface{} = true + allowSharedKeyAccess := true if d.HasChange("allow_shared_key_access") { - allowSharedKeyAccess = d.Get("allow_shared_key_access") + allowSharedKeyAccess = d.Get("allow_shared_key_access").(bool) // If AllowSharedKeyAccess is nil that breaks the Portal UI as reported in https://github.com/hashicorp/terraform-provider-azurerm/issues/11689 // currently the Portal UI reports nil as false, and per the ARM API documentation nil is true. This manifests itself in the Portal UI @@ -1180,9 +1180,8 @@ func resourceStorageAccountUpdate(d *pluginsdk.ResourceData, meta interface{}) e existing, err := client.GetProperties(ctx, resourceGroupName, storageAccountName, "") if err == nil { if sharedKeyAccess := existing.AccountProperties.AllowSharedKeyAccess; sharedKeyAccess != nil { - allowSharedKeyAccess = sharedKeyAccess + allowSharedKeyAccess = *sharedKeyAccess } - } else { // Should never hit this, but added due to an abundance of caution return fmt.Errorf("retrieving Azure Storage Account %q AllowSharedKeyAccess: %+v", storageAccountName, err) @@ -1190,10 +1189,9 @@ func resourceStorageAccountUpdate(d *pluginsdk.ResourceData, meta interface{}) e } // TODO: end remove changes when Portal UI team fixed their code - allowSharedKeyAccessValue := allowSharedKeyAccess.(bool) opts := storage.AccountUpdateParameters{ AccountPropertiesUpdateParameters: &storage.AccountPropertiesUpdateParameters{ - AllowSharedKeyAccess: &allowSharedKeyAccessValue, + AllowSharedKeyAccess: &allowSharedKeyAccess, }, } @@ -1517,8 +1515,12 @@ func resourceStorageAccountRead(d *pluginsdk.ResourceData, meta interface{}) err ctx, cancel := timeouts.ForRead(meta.(*clients.Client).StopContext, d) defer cancel() - storageAccountName := d.Get("name").(string) - resourceGroupName := d.Get("resource_group_name").(string) + id, err := parse.StorageAccountID(d.Id()) + if err != nil { + return err + } + storageAccountName := id.Name + resourceGroupName := id.ResourceGroup resp, err := client.GetProperties(ctx, resourceGroupName, storageAccountName, "") if err != nil { From 4420cad966033ce18b2c67d351aed023a2b10f30 Mon Sep 17 00:00:00 2001 From: Catriona Date: Mon, 23 Aug 2021 11:19:36 +0100 Subject: [PATCH 4/5] changed allow_shared_key_access to shared_access_key_enabled --- internal/services/storage/storage_account_resource.go | 10 +++++----- .../services/storage/storage_account_resource_test.go | 8 ++++---- website/docs/r/storage_account.html.markdown | 2 +- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/internal/services/storage/storage_account_resource.go b/internal/services/storage/storage_account_resource.go index e35712bcd496..2f327cb0b322 100644 --- a/internal/services/storage/storage_account_resource.go +++ b/internal/services/storage/storage_account_resource.go @@ -242,7 +242,7 @@ func resourceStorageAccount() *pluginsdk.Resource { Default: false, }, - "allow_shared_key_access": { + "shared_access_key_enabled": { Type: pluginsdk.TypeBool, Optional: true, Default: true, @@ -922,7 +922,7 @@ func resourceStorageAccountCreate(d *pluginsdk.ResourceData, meta interface{}) e isHnsEnabled := d.Get("is_hns_enabled").(bool) nfsV3Enabled := d.Get("nfsv3_enabled").(bool) allowBlobPublicAccess := d.Get("allow_blob_public_access").(bool) - allowSharedKeyAccess := d.Get("allow_shared_key_access").(bool) + allowSharedKeyAccess := d.Get("shared_access_key_enabled").(bool) accountTier := d.Get("account_tier").(string) replicationType := d.Get("account_replication_type").(string) @@ -1165,8 +1165,8 @@ func resourceStorageAccountUpdate(d *pluginsdk.ResourceData, meta interface{}) e } allowSharedKeyAccess := true - if d.HasChange("allow_shared_key_access") { - allowSharedKeyAccess = d.Get("allow_shared_key_access").(bool) + if d.HasChange("shared_access_key_enabled") { + allowSharedKeyAccess = d.Get("shared_access_key_enabled").(bool) // If AllowSharedKeyAccess is nil that breaks the Portal UI as reported in https://github.com/hashicorp/terraform-provider-azurerm/issues/11689 // currently the Portal UI reports nil as false, and per the ARM API documentation nil is true. This manifests itself in the Portal UI @@ -1658,7 +1658,7 @@ func resourceStorageAccountRead(d *pluginsdk.ResourceData, meta interface{}) err if props.AllowSharedKeyAccess != nil { allowSharedKeyAccess = *props.AllowSharedKeyAccess } - d.Set("allow_shared_key_access", allowSharedKeyAccess) + d.Set("shared_access_key_enabled", allowSharedKeyAccess) } if accessKeys := keys.Keys; accessKeys != nil { diff --git a/internal/services/storage/storage_account_resource_test.go b/internal/services/storage/storage_account_resource_test.go index 9065ee6cd547..61d4c481f0cc 100644 --- a/internal/services/storage/storage_account_resource_test.go +++ b/internal/services/storage/storage_account_resource_test.go @@ -1029,7 +1029,7 @@ func TestAccStorageAccount_allowSharedKeyAccess(t *testing.T) { check.That(data.ResourceName).Key("account_replication_type").HasValue("LRS"), check.That(data.ResourceName).Key("tags.%").HasValue("1"), check.That(data.ResourceName).Key("tags.environment").HasValue("production"), - check.That(data.ResourceName).Key("allow_shared_key_access").HasValue("false"), + check.That(data.ResourceName).Key("shared_access_key_enabled").HasValue("false"), ), }, { @@ -1040,7 +1040,7 @@ func TestAccStorageAccount_allowSharedKeyAccess(t *testing.T) { check.That(data.ResourceName).Key("account_replication_type").HasValue("LRS"), check.That(data.ResourceName).Key("tags.%").HasValue("1"), check.That(data.ResourceName).Key("tags.environment").HasValue("production"), - check.That(data.ResourceName).Key("allow_shared_key_access").HasValue("true"), + check.That(data.ResourceName).Key("shared_access_key_enabled").HasValue("true"), ), }, }) @@ -3012,7 +3012,7 @@ resource "azurerm_storage_account" "test" { location = azurerm_resource_group.test.location account_tier = "Standard" account_replication_type = "LRS" - allow_shared_key_access = false + shared_access_key_enabled = false tags = { environment = "production" @@ -3040,7 +3040,7 @@ resource "azurerm_storage_account" "test" { location = azurerm_resource_group.test.location account_tier = "Standard" account_replication_type = "LRS" - allow_shared_key_access = true + shared_access_key_enabled = true tags = { environment = "production" diff --git a/website/docs/r/storage_account.html.markdown b/website/docs/r/storage_account.html.markdown index d6f42b341731..3c70bb5194f1 100644 --- a/website/docs/r/storage_account.html.markdown +++ b/website/docs/r/storage_account.html.markdown @@ -105,7 +105,7 @@ The following arguments are supported: -> **NOTE:** At this time `allow_blob_public_access` is only supported in the Public Cloud, China Cloud, and US Government Cloud. -* `allow_shared_key_access` - Indicates whether the storage account permits requests to be authorized with the account access key via Shared Key. If false, then all requests, including shared access signatures, must be authorized with Azure Active Directory (Azure AD). The default value is `true`. +* `shared_access_key_enabled` - Indicates whether the storage account permits requests to be authorized with the account access key via Shared Key. If false, then all requests, including shared access signatures, must be authorized with Azure Active Directory (Azure AD). The default value is `true`. ~> **Note:** Terraform uses Shared Key Authorisation to provision Storage Containers, Blobs and other items - when Shared Key Access is disabled, you will need to enable [the `storage_use_azuread` flag in the Provider block](https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs#storage_use_azuread) to use Azure AD for authentication, however not all Azure Storage services support Active Directory authentication. From 9e14d89e7b1d81c6f9c7b2c3d8e725178e0893f6 Mon Sep 17 00:00:00 2001 From: Catriona Date: Mon, 23 Aug 2021 11:38:34 +0100 Subject: [PATCH 5/5] correct tf formatting --- .../storage/storage_account_resource_test.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/internal/services/storage/storage_account_resource_test.go b/internal/services/storage/storage_account_resource_test.go index 61d4c481f0cc..dccd12df4abc 100644 --- a/internal/services/storage/storage_account_resource_test.go +++ b/internal/services/storage/storage_account_resource_test.go @@ -3009,10 +3009,10 @@ resource "azurerm_storage_account" "test" { name = "unlikely23exst2acct%s" resource_group_name = azurerm_resource_group.test.name - location = azurerm_resource_group.test.location - account_tier = "Standard" - account_replication_type = "LRS" - shared_access_key_enabled = false + location = azurerm_resource_group.test.location + account_tier = "Standard" + account_replication_type = "LRS" + shared_access_key_enabled = false tags = { environment = "production" @@ -3037,10 +3037,10 @@ resource "azurerm_storage_account" "test" { name = "unlikely23exst2acct%s" resource_group_name = azurerm_resource_group.test.name - location = azurerm_resource_group.test.location - account_tier = "Standard" - account_replication_type = "LRS" - shared_access_key_enabled = true + location = azurerm_resource_group.test.location + account_tier = "Standard" + account_replication_type = "LRS" + shared_access_key_enabled = true tags = { environment = "production"