From d95a02196c864475c9f599e7f9e4388ea87a5e96 Mon Sep 17 00:00:00 2001 From: sinbai Date: Tue, 26 Nov 2024 17:21:39 +0800 Subject: [PATCH] fix comments --- ...active_directory_administrator_resource.go | 12 +++- ...e_directory_administrator_resource_test.go | 26 ++++---- .../mssql_managed_instance_resource.go | 25 +++----- .../mssql_managed_instance_resource_test.go | 60 +++++++++++++++++++ .../r/mssql_managed_instance.html.markdown | 2 +- 5 files changed, 95 insertions(+), 30 deletions(-) diff --git a/internal/services/mssqlmanagedinstance/mssql_managed_instance_active_directory_administrator_resource.go b/internal/services/mssqlmanagedinstance/mssql_managed_instance_active_directory_administrator_resource.go index 69cbfa68ab1e..367cf1627cd9 100644 --- a/internal/services/mssqlmanagedinstance/mssql_managed_instance_active_directory_administrator_resource.go +++ b/internal/services/mssqlmanagedinstance/mssql_managed_instance_active_directory_administrator_resource.go @@ -276,9 +276,17 @@ func (r MsSqlManagedInstanceActiveDirectoryAdministratorResource) Delete() sdk.R managedInstanceId := commonids.NewSqlManagedInstanceID(id.SubscriptionId, id.ResourceGroup, id.ManagedInstanceName) - err = aadAuthOnlyClient.DeleteThenPoll(ctx, managedInstanceId) + // Before deleting an AAD admin, it is necessary to disable `AzureADOnlyAuthentication` first, as deleting an AAD admin when `AzureADOnlyAuthentication` feature is enabled is not supported. + // Use `CreateOrUpdateThenPoll` instead of `DeleteThenPoll`, because the actual deletion behavior of the API is not to really delete the record, but to update `AzureADOnlyAuthentication` to false. Therefore, using `DeleteThenPoll` will cause pull till done to never end until it times out. + aadAuthOnlyParams := managedinstanceazureadonlyauthentications.ManagedInstanceAzureADOnlyAuthentication{ + Properties: &managedinstanceazureadonlyauthentications.ManagedInstanceAzureADOnlyAuthProperties{ + AzureADOnlyAuthentication: false, + }, + } + + err = aadAuthOnlyClient.CreateOrUpdateThenPoll(ctx, managedInstanceId, aadAuthOnlyParams) if err != nil { - return fmt.Errorf("removing `azuread_authentication_only` for %s: %+v", managedInstanceId, err) + return fmt.Errorf("disabling `azuread_authentication_only` for %s: %+v", id, err) } err = client.DeleteThenPoll(ctx, managedInstanceId) diff --git a/internal/services/mssqlmanagedinstance/mssql_managed_instance_active_directory_administrator_resource_test.go b/internal/services/mssqlmanagedinstance/mssql_managed_instance_active_directory_administrator_resource_test.go index 497b914ac6ed..cea46bd10247 100644 --- a/internal/services/mssqlmanagedinstance/mssql_managed_instance_active_directory_administrator_resource_test.go +++ b/internal/services/mssqlmanagedinstance/mssql_managed_instance_active_directory_administrator_resource_test.go @@ -7,6 +7,7 @@ import ( "context" "fmt" "testing" + "time" "github.com/hashicorp/go-azure-helpers/lang/response" "github.com/hashicorp/go-azure-helpers/resourcemanager/commonids" @@ -25,9 +26,6 @@ func TestAccMsSqlManagedInstanceActiveDirectoryAdministrator_basic(t *testing.T) r := MsSqlManagedInstanceActiveDirectoryAdministratorResource{} data.ResourceTest(t, r, []acceptance.TestStep{ - { - Config: r.template(data), - }, { Config: r.basic(data, true), Check: acceptance.ComposeTestCheckFunc( @@ -36,7 +34,8 @@ func TestAccMsSqlManagedInstanceActiveDirectoryAdministrator_basic(t *testing.T) }, data.ImportStep("administrator_login_password"), { - Config: r.basic(data, false), + PreConfig: func() { time.Sleep(5 * time.Minute) }, + Config: r.basic(data, false), Check: acceptance.ComposeTestCheckFunc( check.That(data.ResourceName).ExistsInAzure(r), ), @@ -66,6 +65,18 @@ func (r MsSqlManagedInstanceActiveDirectoryAdministratorResource) Exists(ctx con func (r MsSqlManagedInstanceActiveDirectoryAdministratorResource) template(data acceptance.TestData) string { return fmt.Sprintf(` +provider "azurerm" { + features { + resource_group { + /* Due to the creation of unmanaged Microsoft.Network/networkIntentPolicies in this service, + prevent_deletion_if_contains_resources has been added here to allow the test resources to be + deleted until this can be properly investigated + */ + prevent_deletion_if_contains_resources = false + } + } +} + %[1]s resource "azurerm_mssql_managed_instance" "test" { @@ -95,13 +106,6 @@ resource "azurerm_mssql_managed_instance" "test" { environment = "staging" database = "test" } - - # changing azure_active_directory_administrator is ignored since the value of azure_active_directory_administrator is returned - # when applying a terraform refresh after step 2/5 of testcase TestAccMsSqlManagedInstanceActiveDirectoryAdministrator_basic - # this is expected since the azuread_authentication_only is set to true in step 2/5 - lifecycle { - ignore_changes = [azure_active_directory_administrator] - } } data "azuread_client_config" "test" {} diff --git a/internal/services/mssqlmanagedinstance/mssql_managed_instance_resource.go b/internal/services/mssqlmanagedinstance/mssql_managed_instance_resource.go index 1f44f8e2f640..f92326d61568 100644 --- a/internal/services/mssqlmanagedinstance/mssql_managed_instance_resource.go +++ b/internal/services/mssqlmanagedinstance/mssql_managed_instance_resource.go @@ -71,9 +71,11 @@ type AzureActiveDirectoryAdministrator struct { TenantID string `tfschema:"tenant_id"` } -var _ sdk.Resource = MsSqlManagedInstanceResource{} -var _ sdk.ResourceWithUpdate = MsSqlManagedInstanceResource{} -var _ sdk.ResourceWithCustomizeDiff = MsSqlManagedInstanceResource{} +var ( + _ sdk.Resource = MsSqlManagedInstanceResource{} + _ sdk.ResourceWithUpdate = MsSqlManagedInstanceResource{} + _ sdk.ResourceWithCustomizeDiff = MsSqlManagedInstanceResource{} +) type MsSqlManagedInstanceResource struct{} @@ -615,9 +617,10 @@ func (r MsSqlManagedInstanceResource) Read() sdk.ResourceFunc { model.AdministratorLogin = pointer.From(props.AdministratorLogin) - if props.Administrators != nil { - model.AzureActiveDirectoryAdministrator = flattenMsSqlManagedInstanceAdministrators(*props.Administrators) - } + // read from state since when `azuread_authentication_only` is enabled via resource `azurerm_mssql_managed_instance_active_directory_administrator`, + // the API returns the value of `AzureActiveDirectoryAdministrator` which causes diff. + model.AzureActiveDirectoryAdministrator = state.AzureActiveDirectoryAdministrator + model.Collation = pointer.From(props.Collation) model.DnsZone = pointer.From(props.DnsZone) model.Fqdn = pointer.From(props.FullyQualifiedDomainName) @@ -837,13 +840,3 @@ func expandMsSqlManagedInstanceAdministrators(input []AzureActiveDirectoryAdmini return pointer.To(adminProps) } - -func flattenMsSqlManagedInstanceAdministrators(admin managedinstances.ManagedInstanceExternalAdministrator) []AzureActiveDirectoryAdministrator { - results := make([]AzureActiveDirectoryAdministrator, 0) - return append(results, AzureActiveDirectoryAdministrator{ - LoginUserName: pointer.From(admin.Login), - ObjectID: pointer.From(admin.Sid), - TenantID: pointer.From(admin.TenantId), - AzureADAuthenticationOnlyEnabled: pointer.From(admin.AzureADOnlyAuthentication), - }) -} diff --git a/internal/services/mssqlmanagedinstance/mssql_managed_instance_resource_test.go b/internal/services/mssqlmanagedinstance/mssql_managed_instance_resource_test.go index 2d1faa52d445..da1ed9872829 100644 --- a/internal/services/mssqlmanagedinstance/mssql_managed_instance_resource_test.go +++ b/internal/services/mssqlmanagedinstance/mssql_managed_instance_resource_test.go @@ -486,6 +486,18 @@ resource "azurerm_mssql_managed_instance" "test" { func (r MsSqlManagedInstanceResource) withoutAadAdmin(data acceptance.TestData) string { return fmt.Sprintf(` +provider "azurerm" { + features { + resource_group { + /* Due to the creation of unmanaged Microsoft.Network/networkIntentPolicies in this service, + prevent_deletion_if_contains_resources has been added here to allow the test resources to be + deleted until this can be properly investigated + */ + prevent_deletion_if_contains_resources = false + } + } +} + %[1]s resource "azurerm_mssql_managed_instance" "test" { @@ -962,6 +974,18 @@ func TestAccMsSqlManagedInstance_aadAdminUpdate(t *testing.T) { func (r MsSqlManagedInstanceResource) aadAdmin(data acceptance.TestData) string { return fmt.Sprintf(` +provider "azurerm" { + features { + resource_group { + /* Due to the creation of unmanaged Microsoft.Network/networkIntentPolicies in this service, + prevent_deletion_if_contains_resources has been added here to allow the test resources to be + deleted until this can be properly investigated + */ + prevent_deletion_if_contains_resources = false + } + } +} + %[1]s provider "azuread" {} @@ -1044,6 +1068,18 @@ resource "azuread_directory_role_member" "test" { func (r MsSqlManagedInstanceResource) aadAdminWithAadAuthOnlyEnabled(data acceptance.TestData) string { return fmt.Sprintf(` +provider "azurerm" { + features { + resource_group { + /* Due to the creation of unmanaged Microsoft.Network/networkIntentPolicies in this service, + prevent_deletion_if_contains_resources has been added here to allow the test resources to be + deleted until this can be properly investigated + */ + prevent_deletion_if_contains_resources = false + } + } +} + %[1]s provider "azuread" {} @@ -1101,6 +1137,18 @@ resource "azurerm_mssql_managed_instance" "test" { func (r MsSqlManagedInstanceResource) setAadAdmin(data acceptance.TestData) string { return fmt.Sprintf(` +provider "azurerm" { + features { + resource_group { + /* Due to the creation of unmanaged Microsoft.Network/networkIntentPolicies in this service, + prevent_deletion_if_contains_resources has been added here to allow the test resources to be + deleted until this can be properly investigated + */ + prevent_deletion_if_contains_resources = false + } + } +} + %[1]s provider "azuread" {} @@ -1165,6 +1213,18 @@ resource "azurerm_mssql_managed_instance" "test" { func (r MsSqlManagedInstanceResource) aadAdminWithAadAuthOnlyEnabledUpdate(data acceptance.TestData) string { return fmt.Sprintf(` +provider "azurerm" { + features { + resource_group { + /* Due to the creation of unmanaged Microsoft.Network/networkIntentPolicies in this service, + prevent_deletion_if_contains_resources has been added here to allow the test resources to be + deleted until this can be properly investigated + */ + prevent_deletion_if_contains_resources = false + } + } +} + %[1]s provider "azuread" {} diff --git a/website/docs/r/mssql_managed_instance.html.markdown b/website/docs/r/mssql_managed_instance.html.markdown index a0be358387ca..6251976a49f6 100644 --- a/website/docs/r/mssql_managed_instance.html.markdown +++ b/website/docs/r/mssql_managed_instance.html.markdown @@ -173,7 +173,7 @@ resource "azurerm_route_table" "example" { name = "routetable-mi" location = azurerm_resource_group.example.location resource_group_name = azurerm_resource_group.example.name - disable_bgp_route_propagation = false + bgp_route_propagation_enabled = true depends_on = [ azurerm_subnet.example, ]