azurerm_eventhub_namespace_customer_managed_key - support for user_assigned_identity_id #23635
Conversation
github-actions
bot
added
documentation
service/event-hubs
There was a problem hiding this comment.
Thanks @bruceharrison1984 - have given this a quick review and will touch base internally next
| // we can only have a single managed id, so just take the first one and call it a day | ||
| for _, item := range *input.KeyVaultProperties { | ||
| if item.Identity != nil { | ||
| return &[]interface{}{ | ||
| map[string]interface{}{ | ||
| "type": "UserAssigned", | ||
| "identity_ids": []string{*item.Identity.UserAssignedIdentity}, | ||
| "principal_id": "", | ||
| "tenant_id": "", | ||
| }, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
this seems odd to me, it looks like each key vault can have a different managed identity?
There was a problem hiding this comment.
Strangely, that is not the case. While multiple keys in multiple vaults can be assigned, only one user assigned ID can be assigned to the event hub encryption resource. They can only be added one at a time, and adding a new one removes the old one.
internal/services/eventhub/eventhub_namespace_customer_managed_key_resource.go
Outdated
Show resolved
Hide resolved
internal/services/eventhub/eventhub_namespace_customer_managed_key_resource.go
Outdated
Show resolved
Hide resolved
internal/services/eventhub/eventhub_namespace_customer_managed_key_resource.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks for this PR - we're using a common set of Expand and Flatten functions for identity across the provider to ensure these are consistently mapped, would you mind updating this to use the common Flatten function so that this behaviour is consistent?
Thanks!
| // we can only have a single user managed id for N number of keys, azure portal only allows setting a single one and then applies it to each key | ||
| for _, item := range *input.KeyVaultProperties { | ||
| if item.Identity != nil { | ||
| return &[]interface{}{ | ||
| map[string]interface{}{ | ||
| "type": "UserAssigned", | ||
| "identity_ids": []string{*item.Identity.UserAssignedIdentity}, | ||
| "principal_id": "", | ||
| "tenant_id": "", | ||
| }, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
we've got consistent Expand and Flatten functions we use across the Provider for this - we're using the Expand above, so can we update this to use the Flatten method here so that this behaviour is normalised across the provider?
There was a problem hiding this comment.
So i dug into this a bit, and the reason I didn't use the existing Flatten method is because the data type that is returned for the encryption.keyVaultProperties.identity doesn't match the existing SystemOrUserAssignedList type. The identity block declared in this resource was used since it already existed, but the Identity data that is returned is just the ID of the User Managed ID that is associated with the encryption, alongside the Key Vault details.
We could do a one-off transform so it fits with FlattenSystemAssignedOrUserAssignedList, but it seems like then we accomplish the same thing with more cycles, and we still are left with a one-off transformation.
Comparison of payloads:
{
"sku": {
"name": "Premium",
"tier": "Premium",
"capacity": 1
},
"id": "/subscriptions/c6d2f82d-4baf-4b80-8d69-1fc6fbf92ad8/resourceGroups/bharrison-Tz6Ium/providers/Microsoft.EventHub/namespaces/bharrison-namespace-Tz6Ium",
"name": "bharrison-namespace-Tz6Ium",
"type": "Microsoft.EventHub/Namespaces",
"location": "East US",
"tags": {},
"identity": { <-- Parent Event Hub Identity Block (standard shape)
"type": "UserAssigned",
"userAssignedIdentities": {
"/subscriptions/c6d2f82d-4baf-4b80-8d69-1fc6fbf92ad8/resourceGroups/bharrison-Tz6Ium/providers/Microsoft.ManagedIdentity/userAssignedIdentities/test": {
"clientId": "5d7f143c-610c-4342-a574-941237de2d13",
"principalId": "5fe787f4-e013-4202-8ef7-70e03c79b574"
}
}
},
"properties": {
"minimumTlsVersion": "1.2",
"publicNetworkAccess": "Enabled",
"disableLocalAuth": false,
"zoneRedundant": true,
"isAutoInflateEnabled": false,
"maximumThroughputUnits": 0,
"encryption": {
"keySource": "Microsoft.KeyVault",
"keyVaultProperties": [
{
"keyName": "bharrisonkey",
"keyVaultUri": "https://bharrisonkvtz6ium.vault.azure.net",
"keyVersion": "c90e6fcef73041d69801250f8bb60a86",
"identity": { <-- Encryption Identity block
"userAssignedIdentity": "/subscriptions/c6d2f82d-4baf-4b80-8d69-1fc6fbf92ad8/resourceGroups/bharrison-Tz6Ium/providers/Microsoft.ManagedIdentity/userAssignedIdentities/test"
}
}
],
"requireInfrastructureEncryption": false
},
"kafkaEnabled": true,
"provisioningState": "Succeeded",
"metricId": "c6d2f82d-4baf-4b80-8d69-1fc6fbf92ad8:bharrison-namespace-tz6ium",
"createdAt": "2023-10-25T16:27:16.77Z",
"updatedAt": "2023-10-25T16:34:43.68Z",
"serviceBusEndpoint": "https://bharrison-namespace-Tz6Ium.servicebus.windows.net:443/",
"status": "Active"
}
}There was a problem hiding this comment.
In which case we're modelling this elsewhere in the provider as a single (string) field (user_assigned_identity_id) - so I think we should be able to take that approach here?
There was a problem hiding this comment.
So ditch the ID block and just go with a string?
I've got no issue with that, but I'll ask @katbyte to weigh in since she originally steered me towards the ID block.
I've got no preference, so either is fine with me.
There was a problem hiding this comment.
Thanks @bruceharrison1984 - LGTM aside with two minor nil check comments
internal/services/eventhub/eventhub_namespace_customer_managed_key_resource.go
Show resolved
Hide resolved
internal/services/eventhub/eventhub_namespace_customer_managed_key_resource.go
Show resolved
Hide resolved
katbyte
changed the title
|
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. |
Completes:
#22853