-
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
Adds CMK for CosmosDB Account #8919
Conversation
petems
commented
Oct 16, 2020
- Closes Support for CosmosDB encryption with CMK #7798
76ae89d
to
3b36492
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 @petems - Thanks for this PR, looks off to a good start.
My understanding is that the key_vault_key_id
property can be changed / updated, so I think we'd need to remove the ForceNew
and include it in the update. As it stands, I believe it will remove the key from the account was otherwise updated? Could you add a test that covers the new property also?
Thanks!
Adding to @jackofallops's comment, could we also add a test that updates the |
Currently the
See Azure/azure-rest-api-specs#10323 But I will add a test for it overall |
@@ -993,3 +1016,60 @@ func checkAccAzureRMCosmosDBAccount_basic(data acceptance.TestData, consistency | |||
resource.TestCheckResourceAttrSet(data.ResourceName, "secondary_readonly_key"), | |||
) | |||
} | |||
|
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.
@katbyte @jackofallops I've created a test using a key vault example from a different test, can I get some 👀 on it? And run it as an acc-test if it looks correct?
Just did a manual test, and this config works for me now: provider "azurerm" {
features {}
}
resource "azurerm_resource_group" "test" {
name = "acctestRG-cosmos-123456"
location = "West US 2"
}
data "azurerm_client_config" "current" {}
data "azuread_service_principal" "cosmosdb" {
display_name = "Azure Cosmos DB"
}
resource "azurerm_key_vault" "test" {
name = "acctestkeyvault123456789"
location = azurerm_resource_group.test.location
resource_group_name = azurerm_resource_group.test.name
tenant_id = data.azurerm_client_config.current.tenant_id
sku_name = "premium"
purge_protection_enabled = true
soft_delete_enabled = true
access_policy {
tenant_id = data.azurerm_client_config.current.tenant_id
object_id = data.azurerm_client_config.current.object_id
key_permissions = [
"list",
"create",
"delete",
"get",
"update",
]
secret_permissions = [
"get",
"delete",
"set",
]
}
access_policy {
tenant_id = data.azurerm_client_config.current.tenant_id
object_id = data.azuread_service_principal.cosmosdb.id
key_permissions = [
"list",
"create",
"delete",
"get",
"update",
"unwrapKey",
"wrapKey",
]
secret_permissions = [
"get",
"delete",
"set",
]
}
}
resource "azurerm_key_vault_key" "test" {
name = "examplekey123456"
key_vault_id = azurerm_key_vault.test.id
key_type = "RSA"
key_size = 2048
key_opts = [
"decrypt",
"encrypt",
"sign",
"unwrapKey",
"verify",
"wrapKey",
]
}
resource "azurerm_cosmosdb_account" "test" {
name = "acctest-ca-123456"
location = azurerm_resource_group.test.location
resource_group_name = azurerm_resource_group.test.name
offer_type = "Standard"
kind = "MongoDB"
key_vault_key_uri = "${azurerm_key_vault.test.vault_uri}keys/${azurerm_key_vault_key.test.name}/"
consistency_policy {
consistency_level = "Strong"
}
geo_location {
location = azurerm_resource_group.test.location
failover_priority = 0
}
} Will fix the test to use this |
ab6b7d3
to
9db1add
Compare
azurerm/internal/services/cosmos/cosmosdb_account_resource_test.go
Outdated
Show resolved
Hide resolved
9db1add
to
858c916
Compare
858c916
to
3550914
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.
Thanks @petems - aside from a couple comments this LGTM
azurerm/internal/services/cosmos/cosmosdb_account_resource_test.go
Outdated
Show resolved
Hide resolved
@katbyte All good to merge? I dont have button rights 😄 |
48052d9
to
7c45d74
Compare
7c45d74
to
61b24c2
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.
LGTM 👍
This has been released in version 2.36.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.36.0"
}
# ... other configuration ... |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks! |