`azurerm_mssql_managed_instance` : support new property `azure_active_directory_administrator`

[sinbai]

• Support azure_active_directory_administrator property. Swagger: https://github.com/Azure/azure-rest-api-specs/blob/5e060c76edd4fab38e9202055062154006463018/specification/sql/resource-manager/Microsoft.Sql/stable/2021-11-01/ManagedInstances.json#L669

• Corrected documentation issue that zone_redundant_enabled should be a property of azurerm_mssql_managed_instance instead of azurerm_sql_managed_instance.

Test results: https://hashicorp.teamcity.com/buildConfiguration/TF_AzureRM_AZURERM_SERVICE_PUBLIC_MSSQLMANAGEDINSTANCE/113037?buildTab=tests

Fix #17601.

[nathanblair]

This should be reopened and reconsidered. As of now it is impossible to spin down the managed instance if the AD admin is enabled on it declaratively through terraform and I suspect this would fix it.

[manicminer]

@nathanblair Could you raise an issue for this? This should be supported and we will look to fix this.

[nathanblair]

One already has been raised - #21165

[manicminer]

Great, thanks for the pointer 👍

[manicminer]

Thanks for clarifying the new capabilities of the resource @sinbai, I'll re-open this and review it shortly 👍

[sinbai]

Hi @sinbai, thanks for working on this and for the discussion RE the API behavior. This is looking good but needs some work, if you can take a look at my questions and suggestions below I'll happily review again.

Hi @manicminer thank you very much for your feedback and time. I have fixed the above comments. Tested in TC. Could you please take another look? Thanks in advance.

Test results in TC: https://hashicorp.teamcity.com/buildConfiguration/TF_AzureRM_AZURERM_SERVICE_PUBLIC_MSSQLMANAGEDINSTANCE/135557?buildTab=tests

[sinbai]

Thanks @sinbai, just a couple questions around handling of AAD-auth-only during the Update, and a suggestion for the docs. If you can take a look at these, this should be good to merge.

Hi @manicminer thanks for your feedback. I have fixed the comments. Test results are as follows. Could you please take another look?

https://hashicorp.teamcity.com/buildConfiguration/TF_AzureRM_AZURERM_SERVICE_PUBLIC_MSSQLMANAGEDINSTANCE/140065?buildTab=tests

[sinbai]

@sinbai Thanks for updating the logic around disabling AAD-only auth, however you have not addressed why the AAD Administrator is being set twice in the same operation? We should also add some logging around the API calls.

Hi @manicminer thanks again for your feedback. I have fixed the comments. The test results are as follows. Could you please take another look? https://hashicorp.teamcity.com/buildConfiguration/TF_AzureRM_AZURERM_SERVICE_PUBLIC_MSSQLMANAGEDINSTANCE/141702?buildTab=tests&expandedTest=build%3A%28id%3A141702%29%2Cid%3A2000000001

[manicminer]

Thanks @sinbai, unfortunately still waiting on test results due to apparent service instability

[manicminer]

@sinbai The tests are mostly failing with the following error, which seems related to some out-of-band network provisioning for a managed instance - would you be able to investigate?

[sinbai]

@sinbai The tests are mostly failing with the following error, which seems related to some out-of-band network provisioning for a managed instance - would you be able to investigate?

@manicminer I have investigated the above issue and I think it is an API bug and have submitted a GH issue for this. For details, please see: https://github.com/Azure/azure-rest-api-specs/issues/27991

[MonzT]

Hi, Any ideas when this will get completed?

[sinbai]

@sinbai The tests are mostly failing with the following error, which seems related to some out-of-band network provisioning for a managed instance - would you be able to investigate?

@manicminer I have investigated the above issue and I think it is an API bug and have submitted a GH issue for this. For details, please see: https://github.com/Azure/azure-rest-api-specs/issues/27991

Hi @manicminer The symptom of the GH issue(https://github.com/Azure/azure-rest-api-specs/issues/27991) is that a hidden filed is not deleted when deleting the instance, I assume that the issue is not related to the functionality supported by the current PR. Therefore, is it possible to merged this PR first to unlock users?

[TyranVdMerwe]

Agreed @sinbai this PR should not be blocked by an existing issue which seems to be unrelated to the change.

This change would be really beneficial from a security perspective and I think a few folks are waiting for it to be merged.

[MonzT]

Any updates here please? could really use this in a current project

[TyranVdMerwe]

@manicminer any feedback here?

[sinbai]

Hi @sinbai - The SDK for this has been bumped to Pandora, I suspect that it will need rebasing and updating to account for this and to resolve conflicts before we can continue review.

Hi @jackofallops thanks for your reminder. I have resolved the conflicts, could you please take another look?

[MonzThomas]

+1.. please can we get this one in?