From 4c6dea27e96c6a54ecefdf6b7ef666c26d3f2642 Mon Sep 17 00:00:00 2001 From: yupwei68 Date: Tue, 18 May 2021 09:22:23 +0800 Subject: [PATCH 1/4] update --- .../storage/storage_account_resource.go | 110 +++++++++++--- .../storage/storage_account_resource_test.go | 137 ++++++++++++++++-- website/docs/r/storage_account.html.markdown | 6 +- 3 files changed, 219 insertions(+), 34 deletions(-) diff --git a/azurerm/internal/services/storage/storage_account_resource.go b/azurerm/internal/services/storage/storage_account_resource.go index 2b7260286296..f3f973b17f47 100644 --- a/azurerm/internal/services/storage/storage_account_resource.go +++ b/azurerm/internal/services/storage/storage_account_resource.go @@ -19,6 +19,8 @@ import ( "github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/tf" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/clients" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/locks" + msiparse "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/msi/parse" + msiValidate "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/msi/validate" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/network" networkValidate "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/network/validate" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/storage/migration" @@ -316,7 +318,6 @@ func resourceStorageAccount() *schema.Resource { "identity": { Type: schema.TypeList, Optional: true, - Computed: true, MaxItems: 1, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ @@ -326,6 +327,8 @@ func resourceStorageAccount() *schema.Resource { DiffSuppressFunc: suppress.CaseDifference, ValidateFunc: validation.StringInSlice([]string{ string(storage.IdentityTypeSystemAssigned), + string(storage.IdentityTypeSystemAssignedUserAssigned), + string(storage.IdentityTypeUserAssigned), }, true), }, "principal_id": { @@ -336,6 +339,15 @@ func resourceStorageAccount() *schema.Resource { Type: schema.TypeString, Computed: true, }, + "identity_ids": { + Type: schema.TypeSet, + Optional: true, + MinItems: 1, + Elem: &schema.Schema{ + Type: schema.TypeString, + ValidateFunc: msiValidate.UserAssignedIdentityID, + }, + }, }, }, }, @@ -836,10 +848,11 @@ func resourceStorageAccountCreate(d *schema.ResourceData, meta interface{}) erro parameters.AccountPropertiesCreateParameters.MinimumTLSVersion = storage.MinimumTLSVersion(minimumTLSVersion) } - if _, ok := d.GetOk("identity"); ok { - storageAccountIdentity := expandAzureRmStorageAccountIdentity(d) - parameters.Identity = storageAccountIdentity + storageAccountIdentity, err := expandAzureRmStorageAccountIdentity(d.Get("identity").([]interface{})) + if err != nil { + return err } + parameters.Identity = storageAccountIdentity if v, ok := d.GetOk("azure_files_authentication"); ok { expandAADFilesAuthentication, err := expandArmStorageAccountAzureFilesAuthentication(v.([]interface{})) @@ -1181,12 +1194,16 @@ func resourceStorageAccountUpdate(d *schema.ResourceData, meta interface{}) erro } if d.HasChange("identity") { + storageAccountIdentity, err := expandAzureRmStorageAccountIdentity(d.Get("identity").([]interface{})) + if err != nil { + return err + } opts := storage.AccountUpdateParameters{ - Identity: expandAzureRmStorageAccountIdentity(d), + Identity: storageAccountIdentity, } if _, err := client.Update(ctx, resourceGroupName, storageAccountName, opts); err != nil { - return fmt.Errorf("Error updating Azure Storage Account identity %q: %+v", storageAccountName, err) + return fmt.Errorf("updating Azure Storage Account identity %q: %+v", storageAccountName, err) } } @@ -1484,7 +1501,10 @@ func resourceStorageAccountRead(d *schema.ResourceData, meta interface{}) error d.Set("secondary_access_key", storageAccountKeys[1].Value) } - identity := flattenAzureRmStorageAccountIdentity(resp.Identity) + identity, err := flattenAzureRmStorageAccountIdentity(resp.Identity) + if err != nil { + return err + } if err := d.Set("identity", identity); err != nil { return err } @@ -2441,32 +2461,76 @@ func flattenStorageAccountBypass(input storage.Bypass) []interface{} { return bypass } -func expandAzureRmStorageAccountIdentity(d *schema.ResourceData) *storage.Identity { - identities := d.Get("identity").([]interface{}) - identity := identities[0].(map[string]interface{}) - identityType := identity["type"].(string) - return &storage.Identity{ - Type: storage.IdentityType(identityType), +func expandAzureRmStorageAccountIdentity(vs []interface{}) (*storage.Identity, error) { + if len(vs) == 0 { + return &storage.Identity{ + Type: storage.IdentityTypeNone, + }, nil } -} -func flattenAzureRmStorageAccountIdentity(identity *storage.Identity) []interface{} { - if identity == nil { - return make([]interface{}, 0) + v := vs[0].(map[string]interface{}) + identity := storage.Identity{ + Type: storage.IdentityType(v["type"].(string)), } - result := make(map[string]interface{}) - if identity.Type != "" { - result["type"] = string(identity.Type) + var identityIdSet []interface{} + if identityIds, exists := v["identity_ids"]; exists { + identityIdSet = identityIds.(*schema.Set).List() } + + // If type contains `UserAssigned`, `identity_ids` must be specified and have at least 1 element + if identity.Type == storage.IdentityTypeUserAssigned || identity.Type == storage.IdentityTypeSystemAssignedUserAssigned { + if len(identityIdSet) == 0 { + return nil, fmt.Errorf("`identity_ids` must have at least 1 element when `type` includes `UserAssigned`") + } + + userAssignedIdentities := make(map[string]*storage.UserAssignedIdentity) + for _, id := range identityIdSet { + userAssignedIdentities[id.(string)] = &storage.UserAssignedIdentity{} + } + + identity.UserAssignedIdentities = userAssignedIdentities + } else if len(identityIdSet) > 0 { + // If type does _not_ contain `UserAssigned` (i.e. is set to `SystemAssigned` or defaulted to `None`), `identity_ids` is not allowed + return nil, fmt.Errorf("`identity_ids` can only be specified when `type` includes `UserAssigned`; but `type` is currently %q", identity.Type) + } + + return &identity, nil +} + +func flattenAzureRmStorageAccountIdentity(identity *storage.Identity) ([]interface{}, error) { + if identity == nil || identity.Type == storage.IdentityTypeNone { + return make([]interface{}, 0), nil + } + + var principalId, tenantId string if identity.PrincipalID != nil { - result["principal_id"] = *identity.PrincipalID + principalId = *identity.PrincipalID } + if identity.TenantID != nil { - result["tenant_id"] = *identity.TenantID + tenantId = *identity.TenantID } - return []interface{}{result} + identityIds := make([]interface{}, 0) + if identity.UserAssignedIdentities != nil { + for key := range identity.UserAssignedIdentities { + parsedId, err := msiparse.UserAssignedIdentityID(key) + if err != nil { + return nil, err + } + identityIds = append(identityIds, parsedId.ID()) + } + } + + return []interface{}{ + map[string]interface{}{ + "type": string(identity.Type), + "principal_id": principalId, + "tenant_id": tenantId, + "identity_ids": schema.NewSet(schema.HashString, identityIds), + }, + }, nil } func getBlobConnectionString(blobEndpoint *string, acctName *string, acctKey *string) string { diff --git a/azurerm/internal/services/storage/storage_account_resource_test.go b/azurerm/internal/services/storage/storage_account_resource_test.go index 1b78d157b5c5..6f6f266bb292 100644 --- a/azurerm/internal/services/storage/storage_account_resource_test.go +++ b/azurerm/internal/services/storage/storage_account_resource_test.go @@ -420,13 +420,13 @@ func TestAccStorageAccount_NonStandardCasing(t *testing.T) { }) } -func TestAccStorageAccount_enableIdentity(t *testing.T) { +func TestAccStorageAccount_systemAssignedIdentity(t *testing.T) { data := acceptance.BuildTestData(t, "azurerm_storage_account", "test") r := StorageAccountResource{} data.ResourceTest(t, r, []resource.TestStep{ { - Config: r.identity(data), + Config: r.systemAssignedIdentity(data), Check: resource.ComposeTestCheckFunc( check.That(data.ResourceName).ExistsInAzure(r), check.That(data.ResourceName).Key("identity.0.type").HasValue("SystemAssigned"), @@ -437,27 +437,76 @@ func TestAccStorageAccount_enableIdentity(t *testing.T) { }) } +func TestAccStorageAccount_userAssignedIdentity(t *testing.T) { + data := acceptance.BuildTestData(t, "azurerm_storage_account", "test") + r := StorageAccountResource{} + + data.ResourceTest(t, r, []resource.TestStep{ + { + Config: r.userAssignedIdentity(data), + Check: resource.ComposeTestCheckFunc( + check.That(data.ResourceName).ExistsInAzure(r), + ), + }, + }) +} + +func TestAccStorageAccount_systemAssignedUserAssignedIdentity(t *testing.T) { + data := acceptance.BuildTestData(t, "azurerm_storage_account", "test") + r := StorageAccountResource{} + + data.ResourceTest(t, r, []resource.TestStep{ + { + Config: r.systemAssignedUserAssignedIdentity(data), + Check: resource.ComposeTestCheckFunc( + check.That(data.ResourceName).ExistsInAzure(r), + check.That(data.ResourceName).Key("identity.0.principal_id").MatchesRegex(validate.UUIDRegExp), + check.That(data.ResourceName).Key("identity.0.tenant_id").MatchesRegex(validate.UUIDRegExp), + ), + }, + }) +} + func TestAccStorageAccount_updateResourceByEnablingIdentity(t *testing.T) { data := acceptance.BuildTestData(t, "azurerm_storage_account", "test") r := StorageAccountResource{} data.ResourceTest(t, r, []resource.TestStep{ { - Config: r.basic(data), + Config: r.identityDisabled(data), Check: resource.ComposeTestCheckFunc( check.That(data.ResourceName).ExistsInAzure(r), - check.That(data.ResourceName).Key("identity.#").HasValue("0"), ), }, { - Config: r.identity(data), + Config: r.systemAssignedIdentity(data), Check: resource.ComposeTestCheckFunc( check.That(data.ResourceName).ExistsInAzure(r), - check.That(data.ResourceName).Key("identity.0.type").HasValue("SystemAssigned"), check.That(data.ResourceName).Key("identity.0.principal_id").MatchesRegex(validate.UUIDRegExp), check.That(data.ResourceName).Key("identity.0.tenant_id").MatchesRegex(validate.UUIDRegExp), ), }, + { + Config: r.userAssignedIdentity(data), + Check: resource.ComposeTestCheckFunc( + check.That(data.ResourceName).ExistsInAzure(r), + ), + }, + { + Config: r.systemAssignedUserAssignedIdentity(data), + Check: resource.ComposeTestCheckFunc( + check.That(data.ResourceName).ExistsInAzure(r), + check.That(data.ResourceName).Key("identity.0.principal_id").MatchesRegex(validate.UUIDRegExp), + check.That(data.ResourceName).Key("identity.0.tenant_id").MatchesRegex(validate.UUIDRegExp), + ), + }, + { + Config: r.identityDisabled(data), + Check: resource.ComposeTestCheckFunc( + check.That(data.ResourceName).ExistsInAzure(r), + check.That(data.ResourceName).Key("identity.#").HasValue("0"), + ), + }, }) } @@ -1502,7 +1551,7 @@ resource "azurerm_storage_account" "test" { `, data.RandomInteger, data.Locations.Primary, data.RandomString) } -func (r StorageAccountResource) identity(data acceptance.TestData) string { +func (r StorageAccountResource) identityTemplate(data acceptance.TestData) string { return fmt.Sprintf(` provider "azurerm" { features {} @@ -1513,6 +1562,34 @@ resource "azurerm_resource_group" "test" { location = "%s" } +resource "azurerm_user_assigned_identity" "test" { + name = "acctestUAI-%d" + location = azurerm_resource_group.test.location + resource_group_name = azurerm_resource_group.test.name +} +`, data.RandomInteger, data.Locations.Primary, data.RandomInteger) +} + +func (r StorageAccountResource) identityDisabled(data acceptance.TestData) string { + return fmt.Sprintf(` +%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" + +} +`, r.identityTemplate(data), data.RandomString) +} + +func (r StorageAccountResource) systemAssignedIdentity(data acceptance.TestData) string { + return fmt.Sprintf(` +%s + resource "azurerm_storage_account" "test" { name = "unlikely23exst2acct%s" resource_group_name = azurerm_resource_group.test.name @@ -1524,12 +1601,52 @@ resource "azurerm_storage_account" "test" { identity { type = "SystemAssigned" } +} +`, r.identityTemplate(data), data.RandomString) +} - tags = { - environment = "production" +func (r StorageAccountResource) userAssignedIdentity(data acceptance.TestData) string { + return fmt.Sprintf(` +%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" + + identity { + type = "UserAssigned" + identity_ids = [ + azurerm_user_assigned_identity.test.id, + ] } } -`, data.RandomInteger, data.Locations.Primary, data.RandomString) +`, r.identityTemplate(data), data.RandomString) +} + +func (r StorageAccountResource) systemAssignedUserAssignedIdentity(data acceptance.TestData) string { + return fmt.Sprintf(` +%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" + + identity { + type = "SystemAssigned,UserAssigned" + identity_ids = [ + azurerm_user_assigned_identity.test.id, + ] + } +} +`, r.identityTemplate(data), data.RandomString) } func (r StorageAccountResource) networkRulesTemplate(data acceptance.TestData) string { diff --git a/website/docs/r/storage_account.html.markdown b/website/docs/r/storage_account.html.markdown index f87fcfd0555e..b260d9033c04 100644 --- a/website/docs/r/storage_account.html.markdown +++ b/website/docs/r/storage_account.html.markdown @@ -205,10 +205,14 @@ A `hour_metrics` block supports the following: A `identity` block supports the following: -* `type` - (Required) Specifies the identity type of the Storage Account. At this time the only allowed value is `SystemAssigned`. +* `type` - (Required) Specifies the identity type of the Storage Account. Possible values are `SystemAssigned`, `UserAssigned`, `SystemAssigned,UserAssigned` (to enable both). ~> The assigned `principal_id` and `tenant_id` can be retrieved after the identity `type` has been set to `SystemAssigned` and Storage Account has been created. More details are available below. +* `identity_ids` - (Optional) A list of IDs for User Assigned Managed Identity resources to be assigned. + +~> **NOTE:** This is required when `type` is set to `UserAssigned` or `SystemAssigned, UserAssigned`. + --- A `logging` block supports the following: From b50f201e8eaf4c9e4c00fe2f54d0934b60c97b0a Mon Sep 17 00:00:00 2001 From: yupwei68 Date: Thu, 27 May 2021 15:20:33 +0800 Subject: [PATCH 2/4] update --- .../services/storage/storage_account_resource.go | 11 ++++++----- .../services/storage/storage_account_resource_test.go | 4 ++-- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/azurerm/internal/services/storage/storage_account_resource.go b/azurerm/internal/services/storage/storage_account_resource.go index 30f343b9f68a..b8838776c97c 100644 --- a/azurerm/internal/services/storage/storage_account_resource.go +++ b/azurerm/internal/services/storage/storage_account_resource.go @@ -3,6 +3,7 @@ package storage import ( "context" "fmt" + "github.com/hashicorp/terraform-plugin-sdk/helper/schema" "log" "net/http" "strings" @@ -338,11 +339,11 @@ func resourceStorageAccount() *pluginsdk.Resource { Computed: true, }, "identity_ids": { - Type: schema.TypeSet, + Type: pluginsdk.TypeSet, Optional: true, MinItems: 1, - Elem: &schema.Schema{ - Type: schema.TypeString, + Elem: &pluginsdk.Schema{ + Type: pluginsdk.TypeString, ValidateFunc: msiValidate.UserAssignedIdentityID, }, }, @@ -2473,7 +2474,7 @@ func expandAzureRmStorageAccountIdentity(vs []interface{}) (*storage.Identity, e var identityIdSet []interface{} if identityIds, exists := v["identity_ids"]; exists { - identityIdSet = identityIds.(*schema.Set).List() + identityIdSet = identityIds.(*pluginsdk.Set).List() } // If type contains `UserAssigned`, `identity_ids` must be specified and have at least 1 element @@ -2526,7 +2527,7 @@ func flattenAzureRmStorageAccountIdentity(identity *storage.Identity) ([]interfa "type": string(identity.Type), "principal_id": principalId, "tenant_id": tenantId, - "identity_ids": schema.NewSet(schema.HashString, identityIds), + "identity_ids": pluginsdk.NewSet(schema.HashString, identityIds), }, }, nil } diff --git a/azurerm/internal/services/storage/storage_account_resource_test.go b/azurerm/internal/services/storage/storage_account_resource_test.go index abf37330e23e..613bca1823cd 100644 --- a/azurerm/internal/services/storage/storage_account_resource_test.go +++ b/azurerm/internal/services/storage/storage_account_resource_test.go @@ -440,7 +440,7 @@ func TestAccStorageAccount_userAssignedIdentity(t *testing.T) { data := acceptance.BuildTestData(t, "azurerm_storage_account", "test") r := StorageAccountResource{} - data.ResourceTest(t, r, []resource.TestStep{ + data.ResourceTest(t, r, []acceptance.TestStep{ { Config: r.userAssignedIdentity(data), Check: acceptance.ComposeTestCheckFunc( @@ -454,7 +454,7 @@ func TestAccStorageAccount_systemAssignedUserAssignedIdentity(t *testing.T) { data := acceptance.BuildTestData(t, "azurerm_storage_account", "test") r := StorageAccountResource{} - data.ResourceTest(t, r, []resource.TestStep{ + data.ResourceTest(t, r, []acceptance.TestStep{ { Config: r.systemAssignedUserAssignedIdentity(data), Check: acceptance.ComposeTestCheckFunc( From e0e42bd2c0b5fabccd6654950670db597c3132ce Mon Sep 17 00:00:00 2001 From: yupwei68 Date: Thu, 27 May 2021 15:24:17 +0800 Subject: [PATCH 3/4] update --- azurerm/internal/services/storage/storage_account_resource.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/azurerm/internal/services/storage/storage_account_resource.go b/azurerm/internal/services/storage/storage_account_resource.go index b8838776c97c..de6a96e2d268 100644 --- a/azurerm/internal/services/storage/storage_account_resource.go +++ b/azurerm/internal/services/storage/storage_account_resource.go @@ -3,7 +3,6 @@ package storage import ( "context" "fmt" - "github.com/hashicorp/terraform-plugin-sdk/helper/schema" "log" "net/http" "strings" @@ -2527,7 +2526,7 @@ func flattenAzureRmStorageAccountIdentity(identity *storage.Identity) ([]interfa "type": string(identity.Type), "principal_id": principalId, "tenant_id": tenantId, - "identity_ids": pluginsdk.NewSet(schema.HashString, identityIds), + "identity_ids": pluginsdk.NewSet(pluginsdk.HashString, identityIds), }, }, nil } From 6c70914fa8588872063d6d7fa0792319a7919f26 Mon Sep 17 00:00:00 2001 From: yupwei68 Date: Thu, 3 Jun 2021 10:29:24 +0800 Subject: [PATCH 4/4] update --- .../services/storage/storage_account_resource.go | 1 + .../services/storage/storage_account_resource_test.go | 9 +-------- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/azurerm/internal/services/storage/storage_account_resource.go b/azurerm/internal/services/storage/storage_account_resource.go index de6a96e2d268..102d10769b8d 100644 --- a/azurerm/internal/services/storage/storage_account_resource.go +++ b/azurerm/internal/services/storage/storage_account_resource.go @@ -316,6 +316,7 @@ func resourceStorageAccount() *pluginsdk.Resource { "identity": { Type: pluginsdk.TypeList, Optional: true, + Computed: true, MaxItems: 1, Elem: &pluginsdk.Resource{ Schema: map[string]*pluginsdk.Schema{ diff --git a/azurerm/internal/services/storage/storage_account_resource_test.go b/azurerm/internal/services/storage/storage_account_resource_test.go index 613bca1823cd..17b144692316 100644 --- a/azurerm/internal/services/storage/storage_account_resource_test.go +++ b/azurerm/internal/services/storage/storage_account_resource_test.go @@ -499,13 +499,6 @@ func TestAccStorageAccount_updateResourceByEnablingIdentity(t *testing.T) { check.That(data.ResourceName).Key("identity.0.tenant_id").MatchesRegex(validate.UUIDRegExp), ), }, - { - Config: r.identityDisabled(data), - Check: acceptance.ComposeTestCheckFunc( - check.That(data.ResourceName).ExistsInAzure(r), - check.That(data.ResourceName).Key("identity.#").HasValue("0"), - ), - }, }) } @@ -981,7 +974,7 @@ resource "azurerm_storage_account" "test" { account_replication_type = "LRS" tags = { - %s + %s } } `, data.RandomInteger, data.Locations.Primary, data.RandomString, tags)