From 4db785587057f39a00b3553c91ce8ff83e74b9fb Mon Sep 17 00:00:00 2001 From: magodo Date: Tue, 19 Nov 2024 16:39:53 +1100 Subject: [PATCH] `azurerm_storage_share` - Fix a bug that using `storage_account_id` accidentally leaks to the wrong branch for creation --- .../storage/storage_share_data_source.go | 6 +- .../storage/storage_share_resource.go | 177 +++++++++++------- .../storage/storage_share_resource_test.go | 16 +- 3 files changed, 118 insertions(+), 81 deletions(-) diff --git a/internal/services/storage/storage_share_data_source.go b/internal/services/storage/storage_share_data_source.go index c53eb099804f..afffa4a657ff 100644 --- a/internal/services/storage/storage_share_data_source.go +++ b/internal/services/storage/storage_share_data_source.go @@ -196,7 +196,11 @@ func dataSourceStorageShareRead(d *pluginsdk.ResourceData, meta interface{}) err if model := share.Model; model != nil { if props := model.Properties; props != nil { d.Set("quota", props.ShareQuota) - d.Set("acl", flattenStorageShareACLs(pointer.From(props.SignedIdentifiers))) + acl, err := flattenStorageShareACLs(pointer.From(props.SignedIdentifiers)) + if err != nil { + return err + } + d.Set("acl", acl) d.Set("metadata", FlattenMetaData(pointer.From(props.Metadata))) } } diff --git a/internal/services/storage/storage_share_resource.go b/internal/services/storage/storage_share_resource.go index eabcfe900324..deb8a06dbe51 100644 --- a/internal/services/storage/storage_share_resource.go +++ b/internal/services/storage/storage_share_resource.go @@ -160,6 +160,7 @@ func resourceStorageShare() *pluginsdk.Resource { r.Schema["storage_account_name"] = &pluginsdk.Schema{ Type: pluginsdk.TypeString, Optional: true, + Computed: true, ForceNew: true, ExactlyOneOf: []string{ "storage_account_name", @@ -171,6 +172,7 @@ func resourceStorageShare() *pluginsdk.Resource { r.Schema["storage_account_id"] = &pluginsdk.Schema{ Type: pluginsdk.TypeString, Optional: true, + Computed: true, ForceNew: true, ExactlyOneOf: []string{ "storage_account_name", @@ -196,83 +198,91 @@ func resourceStorageShareCreate(d *pluginsdk.ResourceData, meta interface{}) err if !features.FivePointOhBeta() { storageClient := meta.(*clients.Client).Storage - if accountName := d.Get("storage_account_name").(string); accountName != "" { - shareName := d.Get("name").(string) - quota := d.Get("quota").(int) - metaDataRaw := d.Get("metadata").(map[string]interface{}) - metaData := ExpandMetaData(metaDataRaw) - account, err := storageClient.FindAccount(ctx, subscriptionId, accountName) + accountName := d.Get("storage_account_name").(string) + if accountName == "" { + accountId, err := commonids.ParseStorageAccountID(d.Get("storage_account_id").(string)) if err != nil { - return fmt.Errorf("retrieving Account %q for Share %q: %v", accountName, shareName, err) - } - if account == nil { - return fmt.Errorf("locating Storage Account %q", accountName) + return err } + accountName = accountId.StorageAccountName + } - // Determine the file endpoint, so we can build a data plane ID - endpoint, err := account.DataPlaneEndpoint(client.EndpointTypeFile) - if err != nil { - return fmt.Errorf("determining File endpoint: %v", err) - } + shareName := d.Get("name").(string) + quota := d.Get("quota").(int) + metaDataRaw := d.Get("metadata").(map[string]interface{}) + metaData := ExpandMetaData(metaDataRaw) - // Parse the file endpoint as a data plane account ID - accountId, err := accounts.ParseAccountID(*endpoint, storageClient.StorageDomainSuffix) - if err != nil { - return fmt.Errorf("parsing Account ID: %v", err) - } + account, err := storageClient.FindAccount(ctx, subscriptionId, accountName) + if err != nil { + return fmt.Errorf("retrieving Account %q for Share %q: %v", accountName, shareName, err) + } + if account == nil { + return fmt.Errorf("locating Storage Account %q", accountName) + } - id := shares.NewShareID(*accountId, shareName) + // Determine the file endpoint, so we can build a data plane ID + endpoint, err := account.DataPlaneEndpoint(client.EndpointTypeFile) + if err != nil { + return fmt.Errorf("determining File endpoint: %v", err) + } - protocol := shares.ShareProtocol(d.Get("enabled_protocol").(string)) - if protocol == shares.NFS { - // Only FileStorage (whose sku tier is Premium only) storage account is able to have NFS file shares. - // See: https://learn.microsoft.com/en-us/azure/storage/files/storage-files-quick-create-use-linux#applies-to - if account.Kind != storageaccounts.KindFileStorage { - return fmt.Errorf("NFS File Share is only supported for Storage Account with kind %q but got `%s`", string(storageaccounts.KindFileStorage), account.Kind) - } - } + // Parse the file endpoint as a data plane account ID + accountId, err := accounts.ParseAccountID(*endpoint, storageClient.StorageDomainSuffix) + if err != nil { + return fmt.Errorf("parsing Account ID: %v", err) + } - // The files API does not support bearer tokens (@manicminer, 2024-02-15) - fileSharesDataPlaneClient, err := storageClient.FileSharesDataPlaneClient(ctx, *account, storageClient.DataPlaneOperationSupportingOnlySharedKeyAuth()) - if err != nil { - return fmt.Errorf("building File Share Client: %v", err) - } + id := shares.NewShareID(*accountId, shareName) - exists, err := fileSharesDataPlaneClient.Exists(ctx, shareName) - if err != nil { - return fmt.Errorf("checking for existing %s: %v", id, err) - } - if exists != nil && *exists { - return tf.ImportAsExistsError("azurerm_storage_share", id.ID()) + protocol := shares.ShareProtocol(d.Get("enabled_protocol").(string)) + if protocol == shares.NFS { + // Only FileStorage (whose sku tier is Premium only) storage account is able to have NFS file shares. + // See: https://learn.microsoft.com/en-us/azure/storage/files/storage-files-quick-create-use-linux#applies-to + if account.Kind != storageaccounts.KindFileStorage { + return fmt.Errorf("NFS File Share is only supported for Storage Account with kind %q but got `%s`", string(storageaccounts.KindFileStorage), account.Kind) } + } - log.Printf("[INFO] Creating Share %q in Storage Account %q", shareName, accountName) - input := shares.CreateInput{ - QuotaInGB: quota, - MetaData: metaData, - EnabledProtocol: protocol, - } + // The files API does not support bearer tokens (@manicminer, 2024-02-15) + fileSharesDataPlaneClient, err := storageClient.FileSharesDataPlaneClient(ctx, *account, storageClient.DataPlaneOperationSupportingOnlySharedKeyAuth()) + if err != nil { + return fmt.Errorf("building File Share Client: %v", err) + } - if accessTier := d.Get("access_tier").(string); accessTier != "" { - tier := shares.AccessTier(accessTier) - input.AccessTier = &tier - } + exists, err := fileSharesDataPlaneClient.Exists(ctx, shareName) + if err != nil { + return fmt.Errorf("checking for existing %s: %v", id, err) + } + if exists != nil && *exists { + return tf.ImportAsExistsError("azurerm_storage_share", id.ID()) + } - if err = fileSharesDataPlaneClient.Create(ctx, shareName, input); err != nil { - return fmt.Errorf("creating %s: %v", id, err) - } + log.Printf("[INFO] Creating Share %q in Storage Account %q", shareName, accountName) + input := shares.CreateInput{ + QuotaInGB: quota, + MetaData: metaData, + EnabledProtocol: protocol, + } - d.SetId(id.ID()) + if accessTier := d.Get("access_tier").(string); accessTier != "" { + tier := shares.AccessTier(accessTier) + input.AccessTier = &tier + } - aclsRaw := d.Get("acl").(*pluginsdk.Set).List() - acls := expandStorageShareACLsDeprecated(aclsRaw) - if err = fileSharesDataPlaneClient.UpdateACLs(ctx, shareName, shares.SetAclInput{SignedIdentifiers: acls}); err != nil { - return fmt.Errorf("setting ACLs for %s: %v", id, err) - } + if err = fileSharesDataPlaneClient.Create(ctx, shareName, input); err != nil { + return fmt.Errorf("creating %s: %v", id, err) + } - return resourceStorageShareRead(d, meta) + d.SetId(id.ID()) + + aclsRaw := d.Get("acl").(*pluginsdk.Set).List() + acls := expandStorageShareACLsDeprecated(aclsRaw) + if err = fileSharesDataPlaneClient.UpdateACLs(ctx, shareName, shares.SetAclInput{SignedIdentifiers: acls}); err != nil { + return fmt.Errorf("setting ACLs for %s: %v", id, err) } + + return resourceStorageShareRead(d, meta) } accountId, err := commonids.ParseStorageAccountID(d.Get("storage_account_id").(string)) @@ -323,7 +333,7 @@ func resourceStorageShareRead(d *pluginsdk.ResourceData, meta interface{}) error ctx, cancel := timeouts.ForRead(meta.(*clients.Client).StopContext, d) defer cancel() - if !features.FivePointOhBeta() && !strings.HasPrefix(d.Id(), "/subscriptions/") { + if !features.FivePointOhBeta() { storageClient := meta.(*clients.Client).Storage id, err := shares.ParseShareID(d.Id(), storageClient.StorageDomainSuffix) if err != nil { @@ -357,7 +367,11 @@ func resourceStorageShareRead(d *pluginsdk.ResourceData, meta interface{}) error } d.Set("name", id.ShareName) + + // Setting both + d.Set("storage_account_id", account.StorageAccountId.ID()) d.Set("storage_account_name", id.AccountId.AccountName) + d.Set("quota", props.QuotaGB) d.Set("url", id.ID()) d.Set("enabled_protocol", string(props.EnabledProtocol)) @@ -410,15 +424,16 @@ func resourceStorageShareRead(d *pluginsdk.ResourceData, meta interface{}) error } d.Set("enabled_protocol", string(enabledProtocols)) d.Set("access_tier", string(pointer.From(props.AccessTier))) - d.Set("acl", flattenStorageShareACLs(pointer.From(props.SignedIdentifiers))) + + acl, err := flattenStorageShareACLs(pointer.From(props.SignedIdentifiers)) + if err != nil { + return err + } + d.Set("acl", acl) d.Set("metadata", FlattenMetaData(pointer.From(props.Metadata))) } } - if !features.FivePointOhBeta() { - d.Set("resource_manager_id", id.ID()) - } - // TODO - The following section for `url` will need to be updated to go-azure-sdk when the Giovanni Deprecation process has been completed account, err := meta.(*clients.Client).Storage.FindAccount(ctx, subscriptionId, id.StorageAccountName) if err != nil { @@ -452,7 +467,7 @@ func resourceStorageShareUpdate(d *pluginsdk.ResourceData, meta interface{}) err ctx, cancel := timeouts.ForUpdate(meta.(*clients.Client).StopContext, d) defer cancel() - if !features.FivePointOhBeta() && !strings.HasPrefix(d.Id(), "/subscriptions/") { + if !features.FivePointOhBeta() { id, err := shares.ParseShareID(d.Id(), storageClient.StorageDomainSuffix) if err != nil { return err @@ -575,7 +590,7 @@ func resourceStorageShareDelete(d *pluginsdk.ResourceData, meta interface{}) err ctx, cancel := timeouts.ForDelete(meta.(*clients.Client).StopContext, d) defer cancel() - if !features.FivePointOhBeta() && !strings.HasPrefix(d.Id(), "/subscriptions/") { + if !features.FivePointOhBeta() { storageClient := meta.(*clients.Client).Storage id, err := shares.ParseShareID(d.Id(), storageClient.StorageDomainSuffix) if err != nil { @@ -687,16 +702,34 @@ func expandStorageShareACLs(input []interface{}) *[]fileshares.SignedIdentifier return pointer.To(results) } -func flattenStorageShareACLs(input []fileshares.SignedIdentifier) []interface{} { +func flattenStorageShareACLs(input []fileshares.SignedIdentifier) ([]interface{}, error) { result := make([]interface{}, 0) for _, v := range input { + var ( + start string + expiry string + ) + if vv := v.AccessPolicy.StartTime; vv != nil { + t, err := time.Parse("2006-01-02T15:04:05Z", *vv) + if err != nil { + return nil, fmt.Errorf("parsing start time %q: %v", *vv, err) + } + start = t.Format("2006-01-02T15:04:05.0000000Z") + } + if vv := v.AccessPolicy.ExpiryTime; vv != nil { + t, err := time.Parse("2006-01-02T15:04:05Z", *vv) + if err != nil { + return nil, fmt.Errorf("parsing expiry time %q: %v", *vv, err) + } + expiry = t.Format("2006-01-02T15:04:05.0000000Z") + } output := map[string]interface{}{ "id": v.Id, "access_policy": []interface{}{ map[string]interface{}{ - "start": v.AccessPolicy.StartTime, - "expiry": v.AccessPolicy.ExpiryTime, + "start": start, + "expiry": expiry, "permissions": v.AccessPolicy.Permission, }, }, @@ -705,5 +738,5 @@ func flattenStorageShareACLs(input []fileshares.SignedIdentifier) []interface{} result = append(result, output) } - return result + return result, nil } diff --git a/internal/services/storage/storage_share_resource_test.go b/internal/services/storage/storage_share_resource_test.go index 832930121a28..76b050d43677 100644 --- a/internal/services/storage/storage_share_resource_test.go +++ b/internal/services/storage/storage_share_resource_test.go @@ -660,8 +660,8 @@ resource "azurerm_storage_share" "test" { access_policy { permissions = "rwd" - start = "2019-07-02T09:38:21Z" - expiry = "2019-07-02T10:38:21Z" + start = "2019-07-02T09:38:21.0000000Z" + expiry = "2019-07-02T10:38:21.0000000Z" } } @@ -781,8 +781,8 @@ resource "azurerm_storage_share" "test" { access_policy { permissions = "rwd" - start = "2019-07-02T09:38:21Z" - expiry = "2019-07-02T10:38:21Z" + start = "2019-07-02T09:38:21.0000000Z" + expiry = "2019-07-02T10:38:21.0000000Z" } } } @@ -876,8 +876,8 @@ resource "azurerm_storage_share" "test" { access_policy { permissions = "rwd" - start = "2019-07-02T09:38:21Z" - expiry = "2019-07-02T10:38:21Z" + start = "2019-07-02T09:38:21.0000000Z" + expiry = "2019-07-02T10:38:21.0000000Z" } } acl { @@ -885,8 +885,8 @@ resource "azurerm_storage_share" "test" { access_policy { permissions = "rwd" - start = "2019-07-02T09:38:21Z" - expiry = "2019-07-02T10:38:21Z" + start = "2019-07-02T09:38:21.0000000Z" + expiry = "2019-07-02T10:38:21.0000000Z" } } }