From 7f0acfac35bab8624e9795c161d9719043c23bca Mon Sep 17 00:00:00 2001 From: magodo Date: Mon, 7 Aug 2023 12:52:22 +0800 Subject: [PATCH 1/9] `azurerm_storage_account` - `azure_file_authentication.0.active_directory` supports setting `domain_name` and `domain_guid` when `directory_type` is `AADKERB` --- .../storage/storage_account_resource.go | 73 +++++++++++++------ .../storage/storage_account_resource_test.go | 40 ++++++++++ 2 files changed, 92 insertions(+), 21 deletions(-) diff --git a/internal/services/storage/storage_account_resource.go b/internal/services/storage/storage_account_resource.go index faf76407fdcf..8b7e2f281dae 100644 --- a/internal/services/storage/storage_account_resource.go +++ b/internal/services/storage/storage_account_resource.go @@ -190,12 +190,6 @@ func resourceStorageAccount() *pluginsdk.Resource { MaxItems: 1, Elem: &pluginsdk.Resource{ Schema: map[string]*pluginsdk.Schema{ - "storage_sid": { - Type: pluginsdk.TypeString, - Required: true, - ValidateFunc: validation.StringIsNotEmpty, - }, - "domain_guid": { Type: pluginsdk.TypeString, Required: true, @@ -208,21 +202,27 @@ func resourceStorageAccount() *pluginsdk.Resource { ValidateFunc: validation.StringIsNotEmpty, }, + "storage_sid": { + Type: pluginsdk.TypeString, + Optional: true, + ValidateFunc: validation.StringIsNotEmpty, + }, + "domain_sid": { Type: pluginsdk.TypeString, - Required: true, + Optional: true, ValidateFunc: validation.StringIsNotEmpty, }, "forest_name": { Type: pluginsdk.TypeString, - Required: true, + Optional: true, ValidateFunc: validation.StringIsNotEmpty, }, "netbios_domain_name": { Type: pluginsdk.TypeString, - Required: true, + Optional: true, ValidateFunc: validation.StringIsNotEmpty, }, }, @@ -2462,13 +2462,34 @@ func expandArmStorageAccountAzureFilesAuthentication(input []interface{}) (*stor v := input[0].(map[string]interface{}) directoryOption := storage.DirectoryServiceOptions(v["directory_type"].(string)) - if _, ok := v["active_directory"]; directoryOption == storage.DirectoryServiceOptionsAD && !ok { - return nil, fmt.Errorf("`active_directory` is required when `directory_type` is `AD`") - } + var ad *storage.ActiveDirectoryProperties + switch string(directoryOption) { + case string(storage.DirectoryServiceOptionsAD): + if _, ok := v["active_directory"]; !ok { + return nil, fmt.Errorf("`active_directory` is required when `directory_type` is `AD`") + } + ad = expandArmStorageAccountActiveDirectoryProperties(v["active_directory"].([]interface{})) + if ad.AzureStorageSid == nil { + return nil, fmt.Errorf("`active_directory.0.storage_sid` is required when `directory_type` is `AD`") + } + if ad.DomainSid == nil { + return nil, fmt.Errorf("`active_directory.0.domain_sid` is required when `directory_type` is `AD`") + } + if ad.ForestName == nil { + return nil, fmt.Errorf("`active_directory.0.forest_name` is required when `directory_type` is `AD`") + } + if ad.NetBiosDomainName == nil { + return nil, fmt.Errorf("`active_directory.0.netbios_domain_name` is required when `directory_type` is `AD`") + } + case string(storageaccounts.DirectoryServiceOptionsAADKERB): + if _, ok := v["azure_active_directory_kerb"]; ok { + ad = expandArmStorageAccountActiveDirectoryProperties(v["active_directory"].([]interface{})) + } + } return &storage.AzureFilesIdentityBasedAuthentication{ DirectoryServiceOptions: directoryOption, - ActiveDirectoryProperties: expandArmStorageAccountActiveDirectoryProperties(v["active_directory"].([]interface{})), + ActiveDirectoryProperties: ad, }, nil } @@ -2476,15 +2497,25 @@ func expandArmStorageAccountActiveDirectoryProperties(input []interface{}) *stor if len(input) == 0 { return nil } - v := input[0].(map[string]interface{}) - return &storage.ActiveDirectoryProperties{ - AzureStorageSid: utils.String(v["storage_sid"].(string)), - DomainGUID: utils.String(v["domain_guid"].(string)), - DomainName: utils.String(v["domain_name"].(string)), - DomainSid: utils.String(v["domain_sid"].(string)), - ForestName: utils.String(v["forest_name"].(string)), - NetBiosDomainName: utils.String(v["netbios_domain_name"].(string)), + m := input[0].(map[string]interface{}) + + output := &storage.ActiveDirectoryProperties{ + DomainGUID: utils.String(m["domain_guid"].(string)), + DomainName: utils.String(m["domain_name"].(string)), + } + if v := m["storage_sid"]; v != "" { + output.AzureStorageSid = utils.String(v.(string)) + } + if v := m["domain_sid"]; v != "" { + output.DomainSid = utils.String(v.(string)) + } + if v := m["forest_name"]; v != "" { + output.ForestName = utils.String(v.(string)) + } + if v := m["netbios_domain_name"]; v != "" { + output.NetBiosDomainName = utils.String(v.(string)) } + return output } func expandArmStorageAccountRouting(input []interface{}) *storage.RoutingPreference { diff --git a/internal/services/storage/storage_account_resource_test.go b/internal/services/storage/storage_account_resource_test.go index 75c41b4c46df..75e052d4909d 100644 --- a/internal/services/storage/storage_account_resource_test.go +++ b/internal/services/storage/storage_account_resource_test.go @@ -909,6 +909,13 @@ func TestAccAzureRMStorageAccount_azureFilesAuthentication(t *testing.T) { ), }, data.ImportStep(), + { + Config: r.azureFilesAuthenticationAADKERBUpdate(data), + Check: acceptance.ComposeTestCheckFunc( + check.That(data.ResourceName).ExistsInAzure(r), + ), + }, + data.ImportStep(), { Config: r.basic(data), Check: acceptance.ComposeTestCheckFunc( @@ -3269,6 +3276,39 @@ resource "azurerm_resource_group" "test" { 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" + + azure_files_authentication { + directory_type = "AADKERB" + active_directory { + domain_name = "adtest2.com" + domain_sid = "S-1-5-21-2400535526-2334094090-2402026252-1112" + } + } + + tags = { + environment = "production" + } +} +`, data.RandomInteger, data.Locations.Primary, data.RandomString) +} + +func (r StorageAccountResource) azureFilesAuthenticationAADKERBUpdate(data acceptance.TestData) string { + return fmt.Sprintf(` +provider "azurerm" { + features {} +} + +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 From 3a5c74a769f03a68aeea7236f4a2564173d8ab7b Mon Sep 17 00:00:00 2001 From: magodo Date: Mon, 7 Aug 2023 13:05:09 +0800 Subject: [PATCH 2/9] Update test --- internal/services/storage/storage_account_resource_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/services/storage/storage_account_resource_test.go b/internal/services/storage/storage_account_resource_test.go index 75e052d4909d..4039487f3c44 100644 --- a/internal/services/storage/storage_account_resource_test.go +++ b/internal/services/storage/storage_account_resource_test.go @@ -3287,7 +3287,7 @@ resource "azurerm_storage_account" "test" { directory_type = "AADKERB" active_directory { domain_name = "adtest2.com" - domain_sid = "S-1-5-21-2400535526-2334094090-2402026252-1112" + domain_guid = "13a20c9a-d491-47e6-8a39-299e7a32ea27" } } From 999a02ddef9db53af9a06973f73a4fe0035cf0c6 Mon Sep 17 00:00:00 2001 From: magodo Date: Mon, 7 Aug 2023 13:38:30 +0800 Subject: [PATCH 3/9] Update --- internal/services/storage/storage_account_resource.go | 2 +- .../services/storage/storage_account_resource_test.go | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/services/storage/storage_account_resource.go b/internal/services/storage/storage_account_resource.go index 8b7e2f281dae..756e223c3695 100644 --- a/internal/services/storage/storage_account_resource.go +++ b/internal/services/storage/storage_account_resource.go @@ -2483,7 +2483,7 @@ func expandArmStorageAccountAzureFilesAuthentication(input []interface{}) (*stor return nil, fmt.Errorf("`active_directory.0.netbios_domain_name` is required when `directory_type` is `AD`") } case string(storageaccounts.DirectoryServiceOptionsAADKERB): - if _, ok := v["azure_active_directory_kerb"]; ok { + if _, ok := v["active_directory"]; ok { ad = expandArmStorageAccountActiveDirectoryProperties(v["active_directory"].([]interface{})) } } diff --git a/internal/services/storage/storage_account_resource_test.go b/internal/services/storage/storage_account_resource_test.go index 4039487f3c44..793bed167e43 100644 --- a/internal/services/storage/storage_account_resource_test.go +++ b/internal/services/storage/storage_account_resource_test.go @@ -3285,10 +3285,6 @@ resource "azurerm_storage_account" "test" { azure_files_authentication { directory_type = "AADKERB" - active_directory { - domain_name = "adtest2.com" - domain_guid = "13a20c9a-d491-47e6-8a39-299e7a32ea27" - } } tags = { @@ -3318,6 +3314,10 @@ resource "azurerm_storage_account" "test" { azure_files_authentication { directory_type = "AADKERB" + active_directory { + domain_name = "adtest2.com" + domain_guid = "13a20c9a-d491-47e6-8a39-299e7a32ea27" + } } tags = { From 58e26b91e407bb8032fb8c35605deebd5bf68755 Mon Sep 17 00:00:00 2001 From: magodo Date: Mon, 7 Aug 2023 14:12:42 +0800 Subject: [PATCH 4/9] Update --- internal/services/storage/storage_account_resource.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/services/storage/storage_account_resource.go b/internal/services/storage/storage_account_resource.go index 756e223c3695..5be69b71f289 100644 --- a/internal/services/storage/storage_account_resource.go +++ b/internal/services/storage/storage_account_resource.go @@ -205,24 +205,28 @@ func resourceStorageAccount() *pluginsdk.Resource { "storage_sid": { Type: pluginsdk.TypeString, Optional: true, + Computed: true, ValidateFunc: validation.StringIsNotEmpty, }, "domain_sid": { Type: pluginsdk.TypeString, Optional: true, + Computed: true, ValidateFunc: validation.StringIsNotEmpty, }, "forest_name": { Type: pluginsdk.TypeString, Optional: true, + Computed: true, ValidateFunc: validation.StringIsNotEmpty, }, "netbios_domain_name": { Type: pluginsdk.TypeString, Optional: true, + Computed: true, ValidateFunc: validation.StringIsNotEmpty, }, }, From 723eadf6de2606afaba1d382c6ce714b7df4bf11 Mon Sep 17 00:00:00 2001 From: magodo Date: Mon, 7 Aug 2023 16:25:15 +0800 Subject: [PATCH 5/9] Update doc --- website/docs/r/storage_account.html.markdown | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/website/docs/r/storage_account.html.markdown b/website/docs/r/storage_account.html.markdown index 166b2ecb83c4..e301ad8edd02 100644 --- a/website/docs/r/storage_account.html.markdown +++ b/website/docs/r/storage_account.html.markdown @@ -344,23 +344,21 @@ A `azure_files_authentication` block supports the following: * `active_directory` - (Optional) A `active_directory` block as defined below. Required when `directory_type` is `AD`. -~> **Note:** If `directory_type` is set to `AADKERB`, `active_directory` is not supported. Use [icals](https://learn.microsoft.com/en-us/azure/storage/files/storage-files-identity-auth-azure-active-directory-enable?tabs=azure-portal#configure-directory-and-file-level-permissions) to configure directory and file level permissions. - --- A `active_directory` block supports the following: -* `storage_sid` - (Required) Specifies the security identifier (SID) for Azure Storage. - * `domain_name` - (Required) Specifies the primary domain that the AD DNS server is authoritative for. -* `domain_sid` - (Required) Specifies the security identifier (SID). - * `domain_guid` - (Required) Specifies the domain GUID. -* `forest_name` - (Required) Specifies the Active Directory forest. +* `domain_sid` - (Optional) Specifies the security identifier (SID). This is required when `directory_type` is set to `AD`. + +* `storage_sid` - (Optional) Specifies the security identifier (SID) for Azure Storage. This is required when `directory_type` is set to `AD`. + +* `forest_name` - (Optional) Specifies the Active Directory forest. This is required when `directory_type` is set to `AD`. -* `netbios_domain_name` - (Required) Specifies the NetBIOS domain name. +* `netbios_domain_name` - (Optional) Specifies the NetBIOS domain name. This is required when `directory_type` is set to `AD`. --- From d2602659abaf6756591ec9810065e4d8baea32c2 Mon Sep 17 00:00:00 2001 From: magodo Date: Mon, 7 Aug 2023 16:25:59 +0800 Subject: [PATCH 6/9] terrafmt --- internal/services/storage/storage_account_resource_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/services/storage/storage_account_resource_test.go b/internal/services/storage/storage_account_resource_test.go index 793bed167e43..489b13fa56f3 100644 --- a/internal/services/storage/storage_account_resource_test.go +++ b/internal/services/storage/storage_account_resource_test.go @@ -3315,8 +3315,8 @@ resource "azurerm_storage_account" "test" { azure_files_authentication { directory_type = "AADKERB" active_directory { - domain_name = "adtest2.com" - domain_guid = "13a20c9a-d491-47e6-8a39-299e7a32ea27" + domain_name = "adtest2.com" + domain_guid = "13a20c9a-d491-47e6-8a39-299e7a32ea27" } } From 1c5fb8c237ede2a959e6a61087fd9512b99e2641 Mon Sep 17 00:00:00 2001 From: magodo Date: Fri, 11 Aug 2023 09:09:00 +0800 Subject: [PATCH 7/9] Avoid O+C --- .../services/storage/storage_account_resource.go | 4 ---- .../storage/storage_account_resource_test.go | 16 +++++++++++++++- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/internal/services/storage/storage_account_resource.go b/internal/services/storage/storage_account_resource.go index 5be69b71f289..756e223c3695 100644 --- a/internal/services/storage/storage_account_resource.go +++ b/internal/services/storage/storage_account_resource.go @@ -205,28 +205,24 @@ func resourceStorageAccount() *pluginsdk.Resource { "storage_sid": { Type: pluginsdk.TypeString, Optional: true, - Computed: true, ValidateFunc: validation.StringIsNotEmpty, }, "domain_sid": { Type: pluginsdk.TypeString, Optional: true, - Computed: true, ValidateFunc: validation.StringIsNotEmpty, }, "forest_name": { Type: pluginsdk.TypeString, Optional: true, - Computed: true, ValidateFunc: validation.StringIsNotEmpty, }, "netbios_domain_name": { Type: pluginsdk.TypeString, Optional: true, - Computed: true, ValidateFunc: validation.StringIsNotEmpty, }, }, diff --git a/internal/services/storage/storage_account_resource_test.go b/internal/services/storage/storage_account_resource_test.go index 489b13fa56f3..eeeada578c19 100644 --- a/internal/services/storage/storage_account_resource_test.go +++ b/internal/services/storage/storage_account_resource_test.go @@ -915,7 +915,12 @@ func TestAccAzureRMStorageAccount_azureFilesAuthentication(t *testing.T) { check.That(data.ResourceName).ExistsInAzure(r), ), }, - data.ImportStep(), + data.ImportStep( + "azure_files_authentication.0.active_directory.0.storage_sid", + "azure_files_authentication.0.active_directory.0.domain_sid", + "azure_files_authentication.0.active_directory.0.forest_name", + "azure_files_authentication.0.active_directory.0.netbios_domain_name", + ), { Config: r.basic(data), Check: acceptance.ComposeTestCheckFunc( @@ -3323,6 +3328,15 @@ resource "azurerm_storage_account" "test" { tags = { environment = "production" } + + lifecycle { + ignore_changes = [ + azure_files_authentication.0.active_directory.0.storage_sid, + azure_files_authentication.0.active_directory.0.domain_sid, + azure_files_authentication.0.active_directory.0.forest_name, + azure_files_authentication.0.active_directory.0.netbios_domain_name, + ] + } } `, data.RandomInteger, data.Locations.Primary, data.RandomString) } From 385441898069677ecd0eb14b7116d1eedf55399c Mon Sep 17 00:00:00 2001 From: magodo Date: Fri, 11 Aug 2023 09:50:05 +0800 Subject: [PATCH 8/9] fmt --- .../storage/storage_account_resource_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/services/storage/storage_account_resource_test.go b/internal/services/storage/storage_account_resource_test.go index eeeada578c19..e6bc663bddf0 100644 --- a/internal/services/storage/storage_account_resource_test.go +++ b/internal/services/storage/storage_account_resource_test.go @@ -3330,12 +3330,12 @@ resource "azurerm_storage_account" "test" { } lifecycle { - ignore_changes = [ - azure_files_authentication.0.active_directory.0.storage_sid, - azure_files_authentication.0.active_directory.0.domain_sid, - azure_files_authentication.0.active_directory.0.forest_name, - azure_files_authentication.0.active_directory.0.netbios_domain_name, - ] + ignore_changes = [ + azure_files_authentication.0.active_directory.0.storage_sid, + azure_files_authentication.0.active_directory.0.domain_sid, + azure_files_authentication.0.active_directory.0.forest_name, + azure_files_authentication.0.active_directory.0.netbios_domain_name, + ] } } `, data.RandomInteger, data.Locations.Primary, data.RandomString) From b12234cdd48df6426c032910729a32ad93cfb2ae Mon Sep 17 00:00:00 2001 From: magodo Date: Sat, 16 Sep 2023 10:05:28 +0800 Subject: [PATCH 9/9] Always expand the ad block --- .../services/storage/storage_account_resource.go | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/internal/services/storage/storage_account_resource.go b/internal/services/storage/storage_account_resource.go index 756e223c3695..33bb641ca451 100644 --- a/internal/services/storage/storage_account_resource.go +++ b/internal/services/storage/storage_account_resource.go @@ -2461,15 +2461,14 @@ func expandArmStorageAccountAzureFilesAuthentication(input []interface{}) (*stor v := input[0].(map[string]interface{}) + ad := expandArmStorageAccountActiveDirectoryProperties(v["active_directory"].([]interface{})) + directoryOption := storage.DirectoryServiceOptions(v["directory_type"].(string)) - var ad *storage.ActiveDirectoryProperties - switch string(directoryOption) { - case string(storage.DirectoryServiceOptionsAD): - if _, ok := v["active_directory"]; !ok { + if directoryOption == storage.DirectoryServiceOptionsAD { + if ad == nil { return nil, fmt.Errorf("`active_directory` is required when `directory_type` is `AD`") } - ad = expandArmStorageAccountActiveDirectoryProperties(v["active_directory"].([]interface{})) if ad.AzureStorageSid == nil { return nil, fmt.Errorf("`active_directory.0.storage_sid` is required when `directory_type` is `AD`") } @@ -2482,11 +2481,8 @@ func expandArmStorageAccountAzureFilesAuthentication(input []interface{}) (*stor if ad.NetBiosDomainName == nil { return nil, fmt.Errorf("`active_directory.0.netbios_domain_name` is required when `directory_type` is `AD`") } - case string(storageaccounts.DirectoryServiceOptionsAADKERB): - if _, ok := v["active_directory"]; ok { - ad = expandArmStorageAccountActiveDirectoryProperties(v["active_directory"].([]interface{})) - } } + return &storage.AzureFilesIdentityBasedAuthentication{ DirectoryServiceOptions: directoryOption, ActiveDirectoryProperties: ad,