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
azurerm_eventhub_namespace_customer_managed_key
- validating that the User Assigned Identity used for accessing the Key Vault is assigned to the EventHub Namespace
#25809
base: main
Are you sure you want to change the base?
Conversation
Thanks @xiaxyi . could you please @ reviewer here to pass this updates |
if err != nil { | ||
return fmt.Errorf("parsing %q as a User Assigned Identity ID: %+v", item, err) | ||
} | ||
if parentEhnUaiId.ID() == userAssignedIdentity { |
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.
Rather than comparing these two IDs by checking the results of .ID()
match - can we compare the Resource ID values instead:
if parentEhnUaiId.ID() == userAssignedIdentity { | |
if resourceids.Match(parentEhnUaiId, userAssignedIdentity) { |
The Match
function is available in hashicorp/go-azure-helpers#234 fwiw
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 @tombuildsstuff for the comment, the userAssignedIdentity in the right side of the equation is a sting that's got by
Do we need to parse the |
Yes, intentionally. By comparing the Resource ID types rather than the literal string value, we can do context-aware comparisons (since we know what each of the Resource ID Segments are, we can compare the IDs with that context - which will help in the future with the some of the casing related items). |
Thanks @tombuildsstuff , code is updated, would you mind taking a look and let me know if everything is cool? |
Hi @tombuildsstuff good day, as we have customers are waiting for this fix to update, could please review an approve it ASAP. Thanks in advanced. |
hi @tombuildsstuff good day, could confrim when this fix would be update? Thanks in advanced. |
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 - thanks for pushing those changes @xiaxyi
azurerm_eventhub_namespace_customer_managed_key
- parsing UAI ID fetched from the parent eventhub namespaceazurerm_eventhub_namespace_customer_managed_key
- validating that the User Assigned Identity used for accessing the Key Vault is assigned to the EventHub Namespace
userAssignedIdentityId, err := commonids.ParseUserAssignedIdentityID(userAssignedIdentity) | ||
if err != nil { | ||
return err | ||
} |
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 needs to be moved within the if
statement below, else the tests fail:
------- Stdout: -------
=== RUN TestAccEventHubNamespaceCustomerManagedKey_basic
=== PAUSE TestAccEventHubNamespaceCustomerManagedKey_basic
=== CONT TestAccEventHubNamespaceCustomerManagedKey_basic
testcase.go:113: Step 1/2 error: Error running apply: exit status 1
Error: parsing "": cannot parse an empty string
with azurerm_eventhub_namespace_customer_managed_key.test,
on terraform_plugin_test.tf line 104, in resource "azurerm_eventhub_namespace_customer_managed_key" "test":
104: resource "azurerm_eventhub_namespace_customer_managed_key" "test" {
--- FAIL: TestAccEventHubNamespaceCustomerManagedKey_basic (14570.71s)
FAIL
@tombuildsstuff Thanks for the comment, adding the empty check for this property. Tests are fine: |
Hi @xiaxyi - Can you take a look at optimising the test config, it's using a dedicated cluster which I believe is unnecessary for this property? This makes the test take 4+ hours and cost a large amount of $ per test - Can you take a look at removing the dedicated cluster resource from the test and posting the test result? Thanks! |
Community Note
Description
PR Checklist
For example: “
resource_name_here
- description of change e.g. adding propertynew_property_name_here
”Changes to existing Resource / Data Source
Testing
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azurerm_eventhub_namespace_customer_managed_key
- validating that the User Assigned Identity used for accessing the Key Vault is assigned to the EventHub Namespace [GH-28509]~originally:
azurerm_eventhub_namespace_customer_managed_key
- parsing UAI resource ID that's returned by the api. ~The
resourceGroups
staticSegment of the UAI ID was changed toresourcegroups
by the api, which caused the mismatch of the same uai that's assigned to the parent eventhub namespace and the uai assigned to the eventhub cmk,This is a (please select all that apply):
Related Issue(s)
Fixes #0000
Note
If this PR changes meaningfully during the course of review please update the title and description as required.