-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
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? |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
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! |
@jackofallops The PR is rebased on your latest changes to the eventhub test cases. Would you mind taking a look? Thanks |
Hi @xiaxyi - Looks like there's another error in one of the tests, it may be unrelated to this change, but we'll need it fixed before we can merge this. If you can check the CI log from your last run, you can see it. I'll try and make some time to take a look later today if you don't get to it. |
@jackofallops I changed the random string from using the location to random string when naming the key vault in test cases, otherwise, the tests will fail due to the duplicate kv name, such as "Status= Code="VaultAlreadyExists" Message="The vault name 'acctestkvwesteurope' is already in use." |
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 🍖
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.