-
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
add resource "azurerm_cognitive_account_customer_managed_key" #12544
add resource "azurerm_cognitive_account_customer_managed_key" #12544
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 @ms-henglu, thanks for this PR. I started reviewing but stopped when I got the the delete function - if I understand correctly, this is deleting the associated cognitive account resource and recreating it in order to achieve deletion of the CMK? If this is the case, we can't do that, and this resource would not be viable.
azurerm/internal/services/cognitive/cognitive_account_customer_managed_key_resource.go
Outdated
Show resolved
Hide resolved
locks.ByName(id.Name, "azurerm_cognitive_account") | ||
defer locks.UnlockByName(id.Name, "azurerm_cognitive_account") | ||
|
||
resp, err := client.Get(ctx, id.ResourceGroup, id.Name) |
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.
This can be moved inside the following if block, no need to get the existing resource unless this is new
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.
Actually it can't. Because we need to update cmk of cognitive account, we need to make sure it exists.
azurerm/internal/services/cognitive/cognitive_account_customer_managed_key_resource.go
Show resolved
Hide resolved
azurerm/internal/services/cognitive/cognitive_account_customer_managed_key_resource.go
Show resolved
Hide resolved
azurerm/internal/services/cognitive/cognitive_account_customer_managed_key_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/cognitive/cognitive_account_customer_managed_key_resource.go
Outdated
Show resolved
Hide resolved
} | ||
|
||
d.Set("cognitive_account_id", id.ID()) | ||
d.Set("key_source", resp.Properties.Encryption.KeySource) |
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.
Potential crash here, needs nil checking
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.
I added nil-check above, so it won't be nil here.
|
||
d.Set("cognitive_account_id", id.ID()) | ||
d.Set("key_source", resp.Properties.Encryption.KeySource) | ||
if props := resp.Properties.Encryption.KeyVaultProperties; props != nil { |
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.
Same here
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.
I added nil-check above, so it won't be nil here.
azurerm/internal/services/cognitive/cognitive_account_customer_managed_key_resource.go
Outdated
Show resolved
Hide resolved
// Since this isn't a real object and it cannot be disabled once Customer Managed Key at rest has been enabled | ||
// And it must keep at least one key once Customer Managed Key is enabled | ||
// So for the delete operation, it has to recreate the Cognitive Account with disabled Customer Managed 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.
We can't just delete and recreate the associated Cognitive account?
With these limitations, this resource likely isn't viable. Perhaps it would be better as a block on the azurerm_cognitive_account
resource?
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 @manicminer , we can't add it to azurerm_cognitive_account
. Because we have a key access policy depends on azurerm_cognitive_account#identity
, and a key vault key depends this policy, then cmk depends on the key vault key.
This means if we add it to azurerm_cognitive_account
, it will cause circular dependency.
So it has to be a separate resource like other existing cmk resource.
refs: #12159
@manicminer , hi, thanks for code review! I've added a commit according to your suggestions, please take another look. 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.
It looks like we need to add something to the docs on how to enable this?
------- Stdout: -------
=== RUN TestAccCognitiveAccountCustomerManagedKey_basic
=== PAUSE TestAccCognitiveAccountCustomerManagedKey_basic
=== CONT TestAccCognitiveAccountCustomerManagedKey_basic
testcase.go:89: Step 1/2 error: Error running apply: exit status 1
Error: creating Account: (Name "acctest-cogacc-210716003904684244" / Resource Group "acctestRG-cognitive-210716003904684244"): cognitiveservices.AccountsClient#Create: Failure sending request: StatusCode=0 -- Original Error: Code="ResourceKindRequireAcceptTerms" Message="This subscription is not allowed to create Face. For details and taking actions please go to https://go.microsoft.com/fwlink/?linkid=2164191"
with azurerm_cognitive_account.test,
on terraform_plugin_test.tf line 25, in resource "azurerm_cognitive_account" "test":
25: resource "azurerm_cognitive_account" "test" {
--- FAIL: TestAccCognitiveAccountCustomerManagedKey_basic (269.53s)
FAIL
Hi @katbyte, it seems that doc is helpful. It says
Could you please create a face resource from portal for the subscription? Thanks! I'll add this to the doc. |
We have done that but the tests are still failing:
|
I'll confirm it with service team. |
@katbyte , Hi, service team confirmed both of those subscriptions have the feature flag for CMK. Can you let me know if you see the E0 offer in the dropdown from the Ibiza portal when trying to create a Face resource? |
@katbyte , Hi, could you please check whether you can see a |
Hi @ms-henglu - After some digging we discovered that the blocker for this was actually a UI element in the portal that is only displayed when a US region is selected in the Face creation blade: Since that tick box was set (resource creation was not necessary) the tests now pass: |
@jackofallops , thanks! I'll report it to service team. |
@@ -46,6 +46,8 @@ The following arguments are supported: | |||
|
|||
* `kind` - (Required) Specifies the type of Cognitive Service Account that should be created. Possible values are `Academic`, `AnomalyDetector`, `Bing.Autosuggest`, `Bing.Autosuggest.v7`, `Bing.CustomSearch`, `Bing.Search`, `Bing.Search.v7`, `Bing.Speech`, `Bing.SpellCheck`, `Bing.SpellCheck.v7`, `CognitiveServices`, `ComputerVision`, `ContentModerator`, `CustomSpeech`, `CustomVision.Prediction`, `CustomVision.Training`, `Emotion`, `Face`,`FormRecognizer`, `ImmersiveReader`, `LUIS`, `LUIS.Authoring`, `MetricsAdvisor`, `Personalizer`, `QnAMaker`, `Recommendations`, `SpeakerRecognition`, `Speech`, `SpeechServices`, `SpeechTranslation`, `TextAnalytics`, `TextTranslation` and `WebLM`. Changing this forces a new resource to be created. | |||
|
|||
-> **NOTE:** You must create your first Face, Text Analytics, or Computer Vision resources from the Azure portal to review and acknowledge the terms and conditions. More information on [Prerequisites](https://docs.microsoft.com/en-us/azure/cognitive-services/cognitive-services-apis-create-account-cli?tabs=windows#prerequisites). |
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.
@ms-henglu - could we add the information steve found out here so the users know to tick that ui element??
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 @ms-henglu - LGTM 🙌
@ms-henglu per your comment here it appears given the current API State this resource doesn't appear to be suitable for Terraform at this time, since deleting and recreating the parent resource during deletion will delete any data within this account, which isn't tracked. For now I'm going to open a PR to revert this resource - but you'll need to reach out to the Service Team here. |
…ccount-cmk Revert "add resource "azurerm_cognitive_account_customer_managed_key" #12544
This functionality has been released in v2.71.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! |
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. |
The tests are listed as the followings.