Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

azurerm_storage_account - support for allow_shared_key_access property #13014

Merged
merged 5 commits into from
Aug 24, 2021
Merged

azurerm_storage_account - support for allow_shared_key_access property #13014

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
711c0e6
support for property allow_shared_key_access in storage_account_resource
catriona-m Aug 16, 2021
02a58f9
update storage_account docs with allow_shared_key_access
catriona-m Aug 16, 2021
1d74791
use parser in storage_account read
catriona-m Aug 17, 2021
4420cad
changed allow_shared_key_access to shared_access_key_enabled
catriona-m Aug 23, 2021
9e14d89
correct tf formatting
catriona-m Aug 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
177 changes: 94 additions & 83 deletions internal/services/storage/storage_account_resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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),
Expand Down Expand Up @@ -239,6 +242,12 @@ func resourceStorageAccount() *pluginsdk.Resource {
Default: false,
},

"allow_shared_key_access": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we change this to

Suggested change
"allow_shared_key_access": {
"shared_key_access_enabled": {

for consistency with the rest of the provider? (its how we typically name bools)

Type: pluginsdk.TypeBool,
Optional: true,
Default: true,
},

"network_rules": {
Type: pluginsdk.TypeList,
Optional: true,
Expand Down Expand Up @@ -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)
Expand All @@ -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,
Expand All @@ -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,
},
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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("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
// 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),
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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))
}
Expand Down Expand Up @@ -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("allow_shared_key_access", allowSharedKeyAccess)
}

if accessKeys := keys.Keys; accessKeys != nil {
Expand All @@ -1666,49 +1676,49 @@ 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)
}
}

fileServiceClient := storageClient.FileServicesClient

// 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 {
Expand All @@ -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 {
Expand All @@ -1735,28 +1745,28 @@ 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)
if err != nil {
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)
}
}

staticWebsite = flattenStaticWebsiteProperties(staticWebsiteProps)
}

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)
Expand All @@ -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
Expand Down Expand Up @@ -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
}
Expand Down
Loading