Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added new resource azurerm_mssql_managed_instance_transparent_data_encryption, azurerm_mssql_managed_instance changes. #18918

Merged
merged 3 commits into from
Nov 24, 2022

Conversation

dkuzmenok
Copy link
Contributor

@dkuzmenok dkuzmenok commented Oct 21, 2022

Description

There is no existing way to define Transparent Data Encryption for SQL Managed Instance.
UserAssigned identities are not allowed in azurerm_mssql_managed_instance resources.

Issues:

Changes

  • Added separate resource azurerm_mssql_managed_instance_transparent_data_encryption that controls usage of KeyVault Key over SQL Managed Instance.
  • Added a customer_managed_key field to azurerm_mssql_managed_instance data source to include KeyVault Key information.
  • Added support of UserAssigned identity into azurerm_mssql_managed_instance resource and data source.

Tests

 # go test -v -timeout 300000s -run ^TestAccMsSqlManagedInstanceTransparentDataEncryption_ github.com/hashicorp/terraform-provider-azurerm/internal/services/mssql/
=== RUN   TestAccMsSqlManagedInstanceTransparentDataEncryption_keyVaultSystemAssignedIdentity
=== PAUSE TestAccMsSqlManagedInstanceTransparentDataEncryption_keyVaultSystemAssignedIdentity
=== RUN   TestAccMsSqlManagedInstanceTransparentDataEncryption_systemManaged
=== PAUSE TestAccMsSqlManagedInstanceTransparentDataEncryption_systemManaged
=== RUN   TestAccMsSqlManagedInstanceTransparentDataEncryption_update
=== PAUSE TestAccMsSqlManagedInstanceTransparentDataEncryption_update
=== CONT  TestAccMsSqlManagedInstanceTransparentDataEncryption_keyVaultSystemAssignedIdentity
=== CONT  TestAccMsSqlManagedInstanceTransparentDataEncryption_systemManaged
=== CONT  TestAccMsSqlManagedInstanceTransparentDataEncryption_update
--- PASS: TestAccMsSqlManagedInstanceTransparentDataEncryption_systemManaged (15390.06s)
--- PASS: TestAccMsSqlManagedInstanceTransparentDataEncryption_keyVaultSystemAssignedIdentity (15521.54s)
--- PASS: TestAccMsSqlManagedInstanceTransparentDataEncryption_update (15627.88s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/mssql 15627.906s
#

# go test -v -timeout 300000s -run ^TestAccMsSqlManagedInstanceTransparentDataEncryption_keyVaultUserAssignedIdentity$ github.com/hashicorp/terraform-provider-azurerm/internal/services/mssql/
=== RUN   TestAccMsSqlManagedInstanceTransparentDataEncryption_keyVaultUserAssignedIdentity
=== PAUSE TestAccMsSqlManagedInstanceTransparentDataEncryption_keyVaultUserAssignedIdentity
=== CONT  TestAccMsSqlManagedInstanceTransparentDataEncryption_keyVaultUserAssignedIdentity
--- PASS: TestAccMsSqlManagedInstanceTransparentDataEncryption_keyVaultUserAssignedIdentity (16018.16s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/mssql 16018.181s
#

@github-actions github-actions bot added documentation service/mssql Microsoft SQL Server labels Oct 21, 2022
@dkuzmenok
Copy link
Contributor Author

That is almost a copy-paste from azurerm_mssql_server_transparent_data_encryption.
It seems API side is also copy pasted from MSSQL Server TDE, so should work with same logic serverside.

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are getting a bunch of test failures for this resource:

Error: waiting for creation of Managed Instance: (Name "acctestsqlserver221102190109220892" / Resource Group "acctestRG1-sql-221102190109220892"): Code="Failed" Message="The async operation failed." AdditionalInfo=[{"id":"/subscriptions/*******/resourceGroups/acctestRG1-sql-221102190109220892/providers/Microsoft.Sql/managedInstances/acctestsqlserver221102190109220892","identity":{"type":"UserAssigned","userAssignedIdentities":{}},"location":"westeurope","name":"acctestsqlserver221102190109220892","properties":{"administratorLogin":"missadministrator","collation":"SQL_Latin1_General_CP1_CI_AS","licenseType":"BasePrice","maintenanceConfigurationId":"/subscriptions/*******/providers/Microsoft.Maintenance/publicMaintenanceConfigurations/SQL_Default","minimalTlsVersion":"1.2","privateEndpointConnections":[],"provisioningState":"Failed","proxyOverride":"Default","publicDataEndpointEnabled":false,"state":"CreationFailed","storageAccountType":"GRS","storageSizeInGB":32,"subnetId":"/subscriptions/*******/resourceGroups/acctestRG1-sql-221102190109220892/providers/Microsoft.Network/virtualNetworks/acctest-vnet1-221102190109220892/subnets/subnet1-221102190109220892","timezoneId":"UTC","vCores":4,"zoneRedundant":false},"sku":{"capacity":4,"family":"Gen5","name":"GP_Gen5","tier":"GeneralPurpose"},"tags":{"database":"test","environment":"staging"},"type":"Microsoft.Sql/managedInstances"}]

however i'm not sure if its these changes or something on the service side :/

@dkuzmenok
Copy link
Contributor Author

we are getting a bunch of test failures for this resource:

Error: waiting for creation of Managed Instance: (Name "acctestsqlserver221102190109220892" / Resource Group "acctestRG1-sql-221102190109220892"): Code="Failed" Message="The async operation failed." AdditionalInfo=[{"id":"/subscriptions/*******/resourceGroups/acctestRG1-sql-221102190109220892/providers/Microsoft.Sql/managedInstances/acctestsqlserver221102190109220892","identity":{"type":"UserAssigned","userAssignedIdentities":{}},"location":"westeurope","name":"acctestsqlserver221102190109220892","properties":{"administratorLogin":"missadministrator","collation":"SQL_Latin1_General_CP1_CI_AS","licenseType":"BasePrice","maintenanceConfigurationId":"/subscriptions/*******/providers/Microsoft.Maintenance/publicMaintenanceConfigurations/SQL_Default","minimalTlsVersion":"1.2","privateEndpointConnections":[],"provisioningState":"Failed","proxyOverride":"Default","publicDataEndpointEnabled":false,"state":"CreationFailed","storageAccountType":"GRS","storageSizeInGB":32,"subnetId":"/subscriptions/*******/resourceGroups/acctestRG1-sql-221102190109220892/providers/Microsoft.Network/virtualNetworks/acctest-vnet1-221102190109220892/subnets/subnet1-221102190109220892","timezoneId":"UTC","vCores":4,"zoneRedundant":false},"sku":{"capacity":4,"family":"Gen5","name":"GP_Gen5","tier":"GeneralPurpose"},"tags":{"database":"test","environment":"staging"},"type":"Microsoft.Sql/managedInstances"}]

however i'm not sure if its these changes or something on the service side :/

Should not be related to my changes. I was not touching server resource. But usually creation of the SQL MI takes 5 hours each (plus up to 2 hours for removing it).
Maybe your test suite is timing out?

I will fix conflicts and also include latest changes for autoRotation.

Copy link
Contributor

@manicminer manicminer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this @dkuzmenok! The test failures seem unrelated to these changes. I made some documentation tweaks but this otherwise LGTM 🧢

…ncryption` for managing TDE in SQL managed instance.

Added `customer_managed_key` in `azurerm_mssql_managed_instance` data source.
Added support for UserAssigned identities into `azurerm_mssql_managed_instance` resource and data source.
@manicminer manicminer force-pushed the mssql-managed-instance-cmk branch from e174862 to 1039c32 Compare November 7, 2022 11:28
@dkuzmenok
Copy link
Contributor Author

@manicminer Thanks for your review! I want to include a smaller change to include autorotate functionality. That would avoid introducing more changes in the future. It is basically implemented already for azurerm_mssql_server here - #18523

@manicminer
Copy link
Contributor

@dkuzmenok Sounds good, if you want to go ahead and add that in then I'll re-run the acceptance tests. Thanks!

@dkuzmenok
Copy link
Contributor Author

@manicminer Coming late, sorry. I've pushed my auto_rotation_enabled change.
Can't run the full test for it, because my subscription has dangling resources, but the test was vefiried by API without problems, no fingers crossed.

Copy link
Contributor

@manicminer manicminer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thanks @dkuzmenok! Just waiting on the tests to finish before merging.

Copy link
Contributor

@manicminer manicminer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dkuzmenok Looks like we have a test failure if you can take a look?

------- Stdout: -------
=== RUN   TestAccMsSqlManagedInstanceTransparentDataEncryption_autoRotate
=== PAUSE TestAccMsSqlManagedInstanceTransparentDataEncryption_autoRotate
=== CONT  TestAccMsSqlManagedInstanceTransparentDataEncryption_autoRotate
testcase.go:110: Step 1/2 error: Check failed: Check 2/2 error: azurerm_mssql_managed_instance_transparent_data_encryption.test: Attribute 'key_vault_key_id' expected "", got "https://acctestsqlserver06746.vault.azure.net/keys/keyVault/331f19d5415e46839a960ea1594e009f
--- FAIL: TestAccMsSqlManagedInstanceTransparentDataEncryption_autoRotate (19673.48s)
FAIL

@dkuzmenok
Copy link
Contributor Author

@manicminer I have pushed a change with a fix for the test.

Was able to test in on new subscription:

$ go test -v -timeout 30000s -run ^TestAccMsSqlManagedInstanceTransparentDataEncryption_autoRotate$ github.com/hashicorp/terraform-provider-azurerm/internal/services/mssql/
=== RUN   TestAccMsSqlManagedInstanceTransparentDataEncryption_autoRotate
=== PAUSE TestAccMsSqlManagedInstanceTransparentDataEncryption_autoRotate
=== CONT  TestAccMsSqlManagedInstanceTransparentDataEncryption_autoRotate
--- PASS: TestAccMsSqlManagedInstanceTransparentDataEncryption_autoRotate (17414.64s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/mssql 17414.650s

@dkuzmenok dkuzmenok requested a review from manicminer November 11, 2022 21:29
Copy link
Contributor

@manicminer manicminer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dkuzmenok Thanks for the changes, this LGTM, we are just having some trouble with test timeouts (instances taking >12hrs to create/destroy) but hopefully this will pass today and we can merge this 😬

@dkuzmenok
Copy link
Contributor Author

@manicminer On my subscription I've had SQL MI hanged, so had to create a support request to remove them (was not able to remove for about a month)...

@dkuzmenok
Copy link
Contributor Author

@manicminer Are there any news with the testing progress? Are we good?

@manicminer
Copy link
Contributor

@dkuzmenok We continue to have problems testing this resource. However, between the test runs, there has been enough coverage by passing tests (both before and after the addition of auto rotation) that I'm satisfied to merge this.

@manicminer manicminer merged commit e22c918 into hashicorp:main Nov 24, 2022
@manicminer manicminer added this to the v3.33.0 milestone Nov 24, 2022
manicminer added a commit that referenced this pull request Nov 24, 2022
@github-actions
Copy link

This functionality has been released in v3.33.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
3 participants