azurerm_storage_account - azure_file_authentication.0.active_directory supports setting domain_name and domain_guid when directory_type is AADKERB

[magodo]

This PR changes downgrades the following properties in azure_file_authentication.0.active_directory block from Required to Optional + (the unfortunate) Computed:

• domain_sid
• storage_sid
• forest_name
• netbios_domain_name

These 4 properties are not needed to be set in the active_directory when the directory_type is set to AADKERB

The reason to add the Computed for them is to align with the current user experience of the whole active_directory block (which was set to be O+C for now). Since when setting to AADKERB with omitting the domain_name and domain_guid, the whole block is returned back. Otherwise, users might be confused to be asked to explicitly ignore changes for those four properties once they set the domain_name and domain_guid.

Fix #22784

Test

[stephybun]

Thanks for this @magodo. I don't believe that asking users to add these fields to ignore_changes is a confusing requirement or a strong enough reason to be setting these to Computed as well.

It's an expectation in Terraform that users should explicitly ignore fields that they do not wish to configure. Given that at some point in the future we will have to go through and re-evaluate which fields can remain O+C I think it's better to introduce and enforce this behaviour now, than further down the track and all in one go.

[magodo]

@stephybun Thank you for the review! I've now removed the Computed and update the test to put them into the ignore_changes, the test is passing:

Please take another look!

[stephybun]

Thanks for removing Computed from those fields @magodo! I think this is almost good to go - an additional note on the docs would be good to clarify that all the fields are needed when the directory_type is AD and there is also potential for some simplification. This should be good to go once that's done 👍

[stephybun]

Minor but we need to call the expand function for the AD properties in all cases so I think we can simplify this to

Suggested change

	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["active_directory"]; ok {
	ad = expandArmStorageAccountActiveDirectoryProperties(v["active_directory"].([]interface{}))
	}
	}
	ad := expandArmStorageAccountActiveDirectoryProperties(v["active_directory"].([]interface{}))
	if directoryOption == storage.DirectoryServiceOptionsAD {
	if ad == nil {
	return nil, fmt.Errorf("`active_directory` is required when `directory_type` is `AD`")
	}
	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`")
	}
	}

[magodo]

If the below check is removed, then the type assertion in the expand function will panic if there is no active_directory specified?

		if _, ok := v["active_directory"]; !ok {
			return nil, fmt.Errorf("`active_directory` is required when `directory_type` is `AD`")
		}

[stephybun]

I'm not sure I follow? I pulled your branch and ran the test with those changes and it appears to be fine?

[stephybun]

@magodo do you have an update?

[magodo]

@stephybun Thank you for the review! I've updated the code per your comment, please take another look!

terraform-provider-azurerm on  storage_account_azf_auth_aadkerb via 🐹 v1.21.1 took 5m40s
💤  TF_ACC=1 go test -v -timeout=20h -run='TestAccAzureRMStorageAccount_azureFilesAuthentication' ./internal/services/storage
=== RUN   TestAccAzureRMStorageAccount_azureFilesAuthentication
=== PAUSE TestAccAzureRMStorageAccount_azureFilesAuthentication
=== CONT  TestAccAzureRMStorageAccount_azureFilesAuthentication
--- PASS: TestAccAzureRMStorageAccount_azureFilesAuthentication (348.23s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/storage       348.242s

[stephybun]

Thanks @magodo LGTM 🦕

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.