diff --git a/internal/services/storage/storage_account_resource.go b/internal/services/storage/storage_account_resource.go index 23a8209bfbf0..2f327cb0b322 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, }, + "shared_access_key_enabled": { + 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("shared_access_key_enabled").(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,41 @@ 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 - - opts := storage.AccountUpdateParameters{ - AccountPropertiesUpdateParameters: &storage.AccountPropertiesUpdateParameters{ - AllowSharedKeyAccess: &allowSharedKeyAccess, - }, - } - - if _, err := client.Update(ctx, resourceGroupName, storageAccountName, opts); err != nil { - return fmt.Errorf("updating Azure Storage Account AllowSharedKeyAccess %q: %+v", storageAccountName, err) + allowSharedKeyAccess := true + 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 + // 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 } + } 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 + opts := storage.AccountUpdateParameters{ + AccountPropertiesUpdateParameters: &storage.AccountPropertiesUpdateParameters{ + AllowSharedKeyAccess: &allowSharedKeyAccess, + }, + } + + 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 +1515,20 @@ 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()) + id, err := parse.StorageAccountID(d.Id()) if err != nil { return err } - name := id.Path["storageAccounts"] - resGroup := id.ResourceGroup + storageAccountName := id.Name + resourceGroupName := id.ResourceGroup - 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 +1539,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 +1552,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 +1653,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("shared_access_key_enabled", allowSharedKeyAccess) } if accessKeys := keys.Keys; accessKeys != nil { @@ -1666,27 +1676,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 +1704,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 +1728,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 +1745,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 +1755,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 +1766,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 +1778,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 +1828,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..dccd12df4abc 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("shared_access_key_enabled").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("shared_access_key_enabled").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" + shared_access_key_enabled = 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" + shared_access_key_enabled = true + + tags = { + environment = "production" + } +} +`, data.RandomInteger, data.Locations.Primary, data.RandomString) +} diff --git a/website/docs/r/storage_account.html.markdown b/website/docs/r/storage_account.html.markdown index 450015f7f6b9..3c70bb5194f1 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. +* `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. + * `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`