-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Implementation of TDE for MS SQL Server #11148
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @MoAhuja - Thanks for this, it's off to a good start. I've left some comments below that I think need to be addressed before continuing to review. If you can fix those up, I'll continue asap.
Thanks!
azurerm/internal/services/mssql/mssql_server_transparent_data_encryption_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/mssql/mssql_server_transparent_data_encryption_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/mssql/mssql_server_transparent_data_encryption_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/mssql/mssql_server_transparent_data_encryption_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/mssql/mssql_server_transparent_data_encryption_resource_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/mssql/mssql_server_transparent_data_encryption_resource_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/mssql/mssql_server_transparent_data_encryption_resource_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/mssql/mssql_server_transparent_data_encryption_resource_test.go
Outdated
Show resolved
Hide resolved
@jackofallops thanks for the review! I've made the changes you suggested and it's ready for another review. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @MoAhuja - Thanks for the quick turn-around on the changes. There's a couple more below, in particular the behaviour of the resource Delete. Again, I'll loop back as quickly as I can once these are addressed.
azurerm/internal/services/mssql/mssql_server_transparent_data_encryption_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/mssql/mssql_server_transparent_data_encryption_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/mssql/mssql_server_transparent_data_encryption_resource_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/mssql/mssql_server_transparent_data_encryption_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/mssql/mssql_server_transparent_data_encryption_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/mssql/mssql_server_transparent_data_encryption_resource.go
Outdated
Show resolved
Hide resolved
6310a9a
to
4561a0e
Compare
@jackofallops Just an FYI, I rebased on top of the latest changes from master as there were some conflicts in the mssql clients.go file. |
6c0a31d
to
4560946
Compare
… instead of server_name
…rvice managed mode. Delete resets mode to service managed.
4560946
to
a97eaa7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @MoAhuja, thanks for addressing @jackofallops' earlier comments. I've rebased and pushed the branch to resolve the merge conflict, and the acceptance tests look good and are passing 🎉
I have a few additional comments inline - if you can take a look at these then this should be good to merge. Thanks!
azurerm/internal/services/mssql/mssql_server_transparent_data_encryption_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/mssql/mssql_server_transparent_data_encryption_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/mssql/mssql_server_transparent_data_encryption_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/mssql/mssql_server_transparent_data_encryption_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/mssql/mssql_server_transparent_data_encryption_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/mssql/mssql_server_transparent_data_encryption_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/mssql/mssql_server_transparent_data_encryption_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/mssql/mssql_server_transparent_data_encryption_resource.go
Outdated
Show resolved
Hide resolved
…r-azurerm into feature/SqlTDE
This has been released in version 2.57.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example: provider "azurerm" {
version = "~> 2.57.0"
}
# ... other configuration ... |
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. |
Fixes #87
TF_ACC=1 go test -v ./azurerm/internal/services/mssql -run=TestAccMsSqlServerTransparentDataEncryption -timeout 60m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
2021/03/29 14:47:59 [DEBUG] not using binary driver name, it's no longer needed
2021/03/29 14:47:59 [DEBUG] not using binary driver name, it's no longer needed
=== RUN TestAccMsSqlServerTransparentDataEncryption_byok
=== PAUSE TestAccMsSqlServerTransparentDataEncryption_byok
=== RUN TestAccMsSqlServerTransparentDataEncryption_systemManaged
=== PAUSE TestAccMsSqlServerTransparentDataEncryption_systemManaged
=== RUN TestAccMsSqlServerTransparentDataEncryption_update
=== PAUSE TestAccMsSqlServerTransparentDataEncryption_update
=== CONT TestAccMsSqlServerTransparentDataEncryption_byok
=== CONT TestAccMsSqlServerTransparentDataEncryption_update
=== CONT TestAccMsSqlServerTransparentDataEncryption_systemManaged
2021/03/29 14:48:04 [DEBUG] TF_ACCTEST_REATTACH set to 1 and acctest.TestHelper is not nil, using reattach-based testing
--- PASS: TestAccMsSqlServerTransparentDataEncryption_systemManaged (246.96s)
--- PASS: TestAccMsSqlServerTransparentDataEncryption_byok (491.30s)
--- PASS: TestAccMsSqlServerTransparentDataEncryption_update (523.55s)
PASS
ok github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/mssql 523.850s