`azurerm_mssql_server` : fix for `administrator_login_password` not being able to be updated #25182

sinbai · 2024-03-08T07:36:13Z

The behavior of the API when updating the password of resource azurerm_mssql_server:

If azuread_authentication_only is set to true when creating resource azurerm_mssql_server. In this case, the password is not allowed to be updated. Password is allowed to be updated when azuread_authentication_only is set to false.

Issue mentioned in [azurerm_mssql_server] updating administrator_login_password throws error even though azuread_authentication_only is disabled #25116:

Create a resource azurerm_mssql_serverwith azuread_administrator.0.azuread_authentication_only = true.

Update the password of azurerm_mssql_server and update azuread_administrator.0.azuread_authentication_only = false.
Actual: TF returns error: administrator_login_passwordcannot be changed onceazuread_administrator.0.azuread_authentication_only = true

TF’s current implementation:

The validation in code prevents the password from being updated because of the old value of azuread_authentication_only is true (this is the value set when creating the new resource).

Even if the validation in 1. is deleted, it is still not possible to update the password, because the password is updated first in code （this is not allowed by API since the azuread_authentication_only is true）, and then updating the azuread_authentication_only in code.

Therefore, I removed the validation in code, and adjusted the update order in 2. to fix #25116.

Test results:

Bug Fix
New Feature (ie adding a service, resource, or data source)
Enhancement
Breaking Change

AtakanColak

Thanks very much for your PR. I'm not really familiar with this codebase, but I wanted to review to the best of my ability.

AtakanColak · 2024-03-09T18:16:01Z

internal/services/mssql/mssql_server_resource_test.go

+  version             = "12.0"
+
+  administrator_login          = "missadministrator"
+  administrator_login_password = "thisIsKat11"


I'm not sure if having azuread_authentication_only = true and administrator_login_password present is a feasible state. My bug report's initial state is aad auth is true and there is no additional administrator_login setting.

I assume that it is possible because in the Terraform documentation it is stated that When true, the administrator_login and administrator_login_password properties can be omitted. . I assuming this means omitted means azure authentication only, not omitted means both. Also, the test case TestAccMsSqlServer_passwordUpdate passed.

AtakanColak · 2024-03-09T18:17:16Z

internal/services/mssql/mssql_server_resource_test.go

@@ -762,6 +784,72 @@ resource "azurerm_mssql_server" "test" {
 `, data.RandomInteger, data.Locations.Primary)
 }

+func (MsSqlServerResource) password(data acceptance.TestData) string {


Not knowing much about the code styling of this repositiry, I think this function naming could be improved to something like mssqlServerResourceWithAadAuthOnly

Thanks, changed to aadAdminWithAADAuthOnlyWithPassword.

AtakanColak · 2024-03-09T18:18:39Z

internal/services/mssql/mssql_server_resource_test.go

+`, data.RandomInteger, data.Locations.Primary)
+}
+
+func (MsSqlServerResource) passwordUpdate(data acceptance.TestData) string {


Similarly would rename to something like mssqServerResourceWithAdminLogin

Thanks, changed to aadAdminWithAADAuthOnlyWithPasswordUpdate.

AtakanColak · 2024-03-09T18:19:37Z

internal/services/mssql/mssql_server_resource_test.go

@@ -177,6 +177,28 @@ func TestAccMsSqlServer_azureadAdmin(t *testing.T) {
 	})
 }

+func TestAccMsSqlServer_passwordUpdate(t *testing.T) {


Might prefer rename to be more precise (see comments below)

Maybe the TestAccMsSqlServer_azureadAdminWithAADAuthOnlyWithPasswordUpdate would be more appropriate.

AtakanColak · 2024-03-09T18:21:12Z

internal/services/mssql/mssql_server_resource.go

+	aadOnlyAuthentictionsEnabled := expandMsSqlServerAADOnlyAuthentictions(d.Get("azuread_administrator").([]interface{}))
+	if _, ok := d.GetOk("administrator_login_password"); ok && aadOnlyAuthentictionsEnabled && d.HasChange("administrator_login_password") {
+		return fmt.Errorf("`administrator_login_password` cannot be changed when `azuread_administrator.0.azuread_authentication_only = true`")


Authentiction typos?

Thanks for the correction.

AtakanColak · 2024-03-09T18:22:50Z

internal/services/mssql/mssql_server_resource.go

-			Properties: &serverazureadonlyauthentications.AzureADOnlyAuthProperties{
-				AzureADOnlyAuthentication: aadOnlyAuthentictionsEnabled,
-			},
+	if payload := existing.Model; payload != nil {


Sorry this may be a silly question but was there a reason for moving this part of the code?

Moving this part of the code is key to solving issue #25116, where the Azure Rest API does not allow administrator_login_password updates when azuread_authentication_only = true. Because the API has such limitations, the correct behavior of TF should be to first update azuread_authentication_only = false (specified by the user), and then update administrator_login_password, so that the update can be successful.

sinbai · 2024-03-11T07:20:22Z

Thanks very much for your PR. I'm not really familiar with this codebase, but I wanted to review to the best of my ability.

Hi @AtakanColak thanks for your time and feedback.

manicminer

Thanks for looking at this @sinbai. Isn't the problem here due to azuread_authentication_only being optional + computed? If we make this not computed, the validation should pass as-is?

manicminer · 2024-03-27T16:23:37Z

internal/services/mssql/mssql_server_resource.go

@@ -319,6 +317,11 @@ func resourceMsSqlServerUpdate(d *pluginsdk.ResourceData, meta interface{}) erro
 	ctx, cancel := timeouts.ForUpdate(meta.(*clients.Client).StopContext, d)
 	defer cancel()

+	aadOnlyAuthenticationsEnabled := expandMsSqlServerAADOnlyAuthentictions(d.Get("azuread_administrator").([]interface{}))


(typo)

Suggested change

aadOnlyAuthenticationsEnabled := expandMsSqlServerAADOnlyAuthentictions(d.Get("azuread_administrator").([]interface{}))

aadOnlyAuthenticationsEnabled := expandMsSqlServerAADOnlyAuthentications(d.Get("azuread_administrator").([]interface{}))

manicminer · 2024-03-27T16:31:45Z

internal/services/mssql/mssql_server_resource.go

@@ -443,18 +388,80 @@ func resourceMsSqlServerUpdate(d *pluginsdk.ResourceData, meta interface{}) erro
 				return fmt.Errorf("deleting Azure Active Directory Administrator %s: %+v", id, err)
 			}
 		}
+
+		if aadOnlyAuthenticationsEnabled {
+			aadOnlyAuthentictionsProps := serverazureadonlyauthentications.ServerAzureADOnlyAuthentication{


(typo)

Suggested change

aadOnlyAuthentictionsProps := serverazureadonlyauthentications.ServerAzureADOnlyAuthentication{

aadOnlyAuthenticationsProps := serverazureadonlyauthentications.ServerAzureADOnlyAuthentication{

sinbai · 2024-03-28T08:28:48Z

Thanks for looking at this @sinbai. Isn't the problem here due to azuread_authentication_only being optional + computed? If we make this not computed, the validation should pass as-is?

Hi @manicminer thanks for your time and feedback. I assume that even if we make azuread_authentication_only not computed, the validation still fails. Detail see the explanation below for details.

The behavior of the API when updating the password of resource azurerm_mssql_server:

If azuread_authentication_only is set to true when creating resource azurerm_mssql_server. In this case, the password is not allowed to be updated. Password is allowed to be updated when azuread_authentication_only is set to false.

Issue mentioned in [azurerm_mssql_server] updating administrator_login_password throws error even though azuread_authentication_only is disabled #25116:

Create a resource azurerm_mssql_serverwith azuread_administrator.0.azuread_authentication_only = true.

Update the password of azurerm_mssql_server and update azuread_administrator.0.azuread_authentication_only = false.
Actual: TF returns error: administrator_login_passwordcannot be changed onceazuread_administrator.0.azuread_authentication_only = true

TF’s current implementation:

The validation in code prevents the password from being updated because of the old value of azuread_authentication_only is true (this is the value set when creating the new resource).

Even if the validation in 1. is deleted, it is still not possible to update the password, because the password is updated first in code （this is not allowed by API since the azuread_authentication_only is true）, and then updating the azuread_authentication_only in code.

Therefore, I removed the validation and adjusted the update order in 2. to fix #25116. Please let me know if you have any feedback. Thanks in advance.

github-actions bot added service/mssql Microsoft SQL Server size/L labels Mar 8, 2024

sinbai marked this pull request as ready for review March 8, 2024 08:56

sinbai mentioned this pull request Mar 8, 2024

[azurerm_mssql_server] updating administrator_login_password throws error even though azuread_authentication_only is disabled #25116

Open

1 task

AtakanColak reviewed Mar 9, 2024

View reviewed changes

manicminer reviewed Mar 27, 2024

View reviewed changes

github-actions bot added the waiting-response label Mar 27, 2024

sinbai added 3 commits October 14, 2024 16:57

fix gh issue 25116

f1be63f

update code

00b885a

fix comments

e52def6

sinbai force-pushed the mssql/fix_25116 branch from 0713f79 to e52def6 Compare October 14, 2024 09:33

github-actions bot added the size/L label Oct 14, 2024

rcskosir added the bug label Oct 14, 2024

katbyte requested review from katbyte and a team as code owners November 14, 2024 00:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_mssql_server` : fix for `administrator_login_password` not being able to be updated #25182

`azurerm_mssql_server` : fix for `administrator_login_password` not being able to be updated #25182

sinbai commented Mar 8, 2024 •

edited by rcskosir

Loading

AtakanColak left a comment

AtakanColak Mar 9, 2024

sinbai Mar 11, 2024

AtakanColak Mar 9, 2024

sinbai Mar 11, 2024

AtakanColak Mar 9, 2024

sinbai Mar 11, 2024 •

edited

Loading

AtakanColak Mar 9, 2024

sinbai Mar 11, 2024

AtakanColak Mar 9, 2024

sinbai Mar 11, 2024

AtakanColak Mar 9, 2024

sinbai Mar 11, 2024

sinbai commented Mar 11, 2024

manicminer left a comment

manicminer Mar 27, 2024

sinbai Mar 28, 2024

manicminer Mar 27, 2024

sinbai Mar 28, 2024

sinbai commented Mar 28, 2024

	aadOnlyAuthenticationsEnabled := expandMsSqlServerAADOnlyAuthentictions(d.Get("azuread_administrator").([]interface{}))
	aadOnlyAuthenticationsEnabled := expandMsSqlServerAADOnlyAuthentications(d.Get("azuread_administrator").([]interface{}))

	aadOnlyAuthentictionsProps := serverazureadonlyauthentications.ServerAzureADOnlyAuthentication{
	aadOnlyAuthenticationsProps := serverazureadonlyauthentications.ServerAzureADOnlyAuthentication{

azurerm_mssql_server : fix for administrator_login_password not being able to be updated #25182

Are you sure you want to change the base?

azurerm_mssql_server : fix for administrator_login_password not being able to be updated #25182

Conversation

sinbai commented Mar 8, 2024 • edited by rcskosir Loading

AtakanColak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sinbai Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sinbai commented Mar 11, 2024

manicminer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sinbai commented Mar 28, 2024

`azurerm_mssql_server` : fix for `administrator_login_password` not being able to be updated #25182

`azurerm_mssql_server` : fix for `administrator_login_password` not being able to be updated #25182

sinbai commented Mar 8, 2024 •

edited by rcskosir

Loading

sinbai Mar 11, 2024 •

edited

Loading