`azurerm_cosmosdb_account`: support for CMK through `managed_hsm_key_id` property #26521

wuxu92 · 2024-07-02T09:43:32Z

Community Note

Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

This PR introduces common helpers in internal/customermanagedkeys/ to parse and flatten CMKs from KeyVault or MHSM, and to generate the necessary configuration for AccTest requiring a Managed HSM key.
This PR adds support for managed HSM keys in Cosmos DB account resources.

PR Checklist

I have followed the guidelines in our Contributing Documentation.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
For example: “resource_name_here - description of change e.g. adding property new_property_name_here”

Changes to existing Resource / Data Source

I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
I have written new tests for my resource or datasource changes & updated any relevent documentation.
I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
(For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

=== PAUSE TestAccCosmosDBAccount_ManagedHSMUri
=== CONT  TestAccCosmosDBAccount_ManagedHSMUri
--- PASS: TestAccCosmosDBAccount_ManagedHSMUri (3225.82s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/cosmos  3225.831s

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

azurerm_cosmosdb_account - support CMK from Managed HSM [azurerm_cosmosdb_account - Support for Key Vault Keys from a Managed HSM #26357]

This is a (please select all that apply):

Bug Fix
New Feature (ie adding a service, resource, or data source)
Enhancement
Breaking Change

Related Issue(s)

Fixes #26357

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

magodo

Thanks for this PR!
I've taken a look through and left some comments inline, but this is mostly looking good to me 👍

internal/customermanagedkeys/hsm_key_acceptance_template.go

website/docs/r/cosmosdb_account.html.markdown

internal/services/cosmos/cosmosdb_account_resource_test.go

internal/customermanagedkeys/key_vault_or_managed_hsm_key.go

internal/services/cosmos/cosmosdb_account_resource.go

internal/customermanagedkeys/key_vault_or_managed_hsm_key.go

magodo · 2024-07-05T00:54:05Z

@wuxu92 Thank you and it now LGTM!

tombuildsstuff · 2024-07-10T08:07:05Z

internal/customermanagedkeys/key_vault_or_managed_hsm_key.go

+	return ""
+}
+
+func parseKeyvauleID(keyRaw string, requireVersion VersionType) (*parse.NestedItemId, error) {


we're going to need the Key Vault Environment here in the future, for when the Key Vault nested items take the environment, so we should probably thread that through now, since this'll be used in a bunch of places?

reasonable, I pushed a commint to update it.

katbyte · 2024-10-22T16:22:10Z

internal/customermanagedkeys/hsm_key_acceptance_template.go

+package customermanagedkeys
+
+import (
+	"fmt"
+	"strings"
+
+	"github.com/hashicorp/go-uuid"
+)
+
+// ManagedHSMKeyTempalte: Helper function to generate a template for HSM key acceptance tests
+// Ensure `azurerm_client_config.current` datasource is defined before using this template.
+// Verify there are no resource address conflicts in the caller of this template.
+func ManagedHSMKeyTempalte(randomInteger int, randomString string, purgeProtectionEnabled bool, principalRefs []string) string {


Lets not do this, we should not be sharing test configuration between services as sometimes different services require slightly different setups. Not to mention it moves the test config away from the tests

so could we please change this to maybe CosmosDBManagedHSMKeyTempalte and include it in the cosmosdb service folder somewhere?

Thanks for the review. I removed this file and moved the test configuration to the Cosmos DB account's test file.

tested with an exsiting hsm key:

--- PASS: TestAccCosmosDBAccount_ManagedHSMUri (952.33s) PASS

mbfrahry

Hey @wuxu92, I took another pass. Could you take a look?

mbfrahry · 2024-08-21T20:39:13Z

internal/services/netapp/netapp_account_encryption_resource.go

+			Description:  "The versionless encryption key url.",
+		},
+
+		"managed_hsm_key_id": {


It looks like this is only specified in the schema. Can we revert this change and open another PR for this change if we're looking to add it to netapp account encryption?

removed and rebased main branch to resolve the conflict.

mbfrahry · 2024-08-22T20:03:14Z

internal/customermanagedkeys/key_vault_or_managed_hsm_key.go

+
+// FlattenKeyVaultOrManagedHSMID uses `KeyVaultOrManagedHSMKey.SetState()` to save the state, which this function is designed not to do.
+func FlattenKeyVaultOrManagedHSMID(id string, keyVaultEnv, hsmEnv environments.Api) (*KeyVaultOrManagedHSMKey, error) {
+	_ = keyVaultEnv


Does this need to be here? It doesn't look like keyVaultEnv is being used in this function at all

I placed the keyvault environment here for future use, similar to the HSM environment. I removed it for now, but we can add it back when needed.

mbfrahry · 2024-10-17T15:47:27Z

internal/customermanagedkeys/hsm_key_acceptance_template.go

+// ManagedHSMKeyTempalte: Helper function to generate a template for HSM key acceptance tests
+// Ensure `azurerm_client_config.current` datasource is defined before using this template.
+// Verify there are no resource address conflicts in the caller of this template.
+func ManagedHSMKeyTempalte(randomInteger int, randomString string, purgeProtectionEnabled bool, principalRefs []string) string {


I do like the idea of having a template for hsm as it's complicated to get going but I also saw in other resources that the setup for hsm was different.

Also, having a template used across services makes it possible that if the template changes for a certain resource, it could break the tests in other resources.

Yeah, I removed the file and moved the config content to the specific test case.

mbfrahry · 2024-10-17T15:40:47Z

internal/customermanagedkeys/key_vault_or_managed_hsm_key_test.go

+			wantErr: false,
+		},
+		{
+			name: "fail with no version provided",


I don't believe we should fail if the version isn't specified as some resources take versionless ids

Should we also expand these tests to include versioned ids?

added new test case for versionless key vault id.

mbfrahry

LGTM! Thanks for getting all those changes in @wuxu92

github-actions · 2024-12-02T02:20:12Z

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

wuxu92 marked this pull request as draft July 2, 2024 09:43

github-actions bot added documentation service/cosmosdb size/XL waiting-response and removed waiting-response labels Jul 2, 2024

wuxu92 force-pushed the cmk/helpers branch 2 times, most recently from 0e151f9 to 6d71df2 Compare July 3, 2024 00:57

magodo requested changes Jul 3, 2024

View reviewed changes

github-actions bot added waiting-response service/netapp and removed waiting-response labels Jul 3, 2024

magodo requested changes Jul 4, 2024

View reviewed changes

internal/customermanagedkeys/key_vault_or_managed_hsm_key.go Outdated Show resolved Hide resolved

internal/customermanagedkeys/key_vault_or_managed_hsm_key.go Outdated Show resolved Hide resolved

internal/customermanagedkeys/key_vault_or_managed_hsm_key.go Outdated Show resolved Hide resolved

github-actions bot added waiting-response and removed waiting-response labels Jul 4, 2024

wuxu92 marked this pull request as ready for review July 5, 2024 05:53

github-actions bot added waiting-response and removed waiting-response labels Jul 9, 2024

tombuildsstuff reviewed Jul 10, 2024

View reviewed changes

github-actions bot added the waiting-response label Jul 10, 2024

tombuildsstuff assigned manicminer Jul 10, 2024

github-actions bot removed the waiting-response label Jul 12, 2024

wuxu92 force-pushed the cmk/helpers branch from 35f4094 to d7466ef Compare September 23, 2024 02:26

rcskosir added the enhancement label Oct 14, 2024

katbyte requested changes Oct 22, 2024

View reviewed changes

github-actions bot added the waiting-response label Oct 22, 2024

wuxu92 requested a review from a team as a code owner October 23, 2024 08:08

github-actions bot removed the waiting-response label Oct 23, 2024

wuxu92 force-pushed the cmk/helpers branch from 9492fe9 to 27f20d1 Compare October 23, 2024 08:10

mbfrahry requested changes Oct 24, 2024

View reviewed changes

wuxu92 added 3 commits October 28, 2024 10:43

add cmk helper functions

955628f

add managed hsm key id support for cosmos db account

caa0995

update flatten remove kv env and test for versionless

6740d73

wuxu92 force-pushed the cmk/helpers branch from 62ff715 to 6740d73 Compare October 28, 2024 03:54

mbfrahry approved these changes Oct 31, 2024

View reviewed changes

mbfrahry changed the title ~~azurerm_cosmosdb_account: support CMK from Managed HSM with managed_hsm_key_id property~~ azurerm_cosmosdb_account: support for CMK through managed_hsm_key_id property Oct 31, 2024

mbfrahry added this to the v4.8.0 milestone Oct 31, 2024

mbfrahry merged commit 0b8ca05 into hashicorp:main Oct 31, 2024
37 checks passed

mbfrahry added a commit that referenced this pull request Oct 31, 2024

Update CHANGELOG.md for #26521

798868f

wuxu92 deleted the cmk/helpers branch October 31, 2024 01:17

github-actions bot locked as resolved and limited conversation to collaborators Dec 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_cosmosdb_account`: support for CMK through `managed_hsm_key_id` property #26521

`azurerm_cosmosdb_account`: support for CMK through `managed_hsm_key_id` property #26521

wuxu92 commented Jul 2, 2024 •

edited

Loading

magodo left a comment

magodo commented Jul 5, 2024

tombuildsstuff Jul 10, 2024

wuxu92 Jul 12, 2024

katbyte Oct 22, 2024

wuxu92 Oct 23, 2024

mbfrahry left a comment

mbfrahry Aug 21, 2024

wuxu92 Oct 28, 2024

mbfrahry Aug 22, 2024

wuxu92 Oct 28, 2024

mbfrahry Oct 17, 2024

wuxu92 Oct 28, 2024

mbfrahry Oct 17, 2024

mbfrahry Oct 24, 2024

wuxu92 Oct 28, 2024

mbfrahry left a comment

github-actions bot commented Dec 2, 2024

azurerm_cosmosdb_account: support for CMK through managed_hsm_key_id property #26521

azurerm_cosmosdb_account: support for CMK through managed_hsm_key_id property #26521

Conversation

wuxu92 commented Jul 2, 2024 • edited Loading

Community Note

Description

PR Checklist

Changes to existing Resource / Data Source

Testing

Change Log

Related Issue(s)

magodo left a comment

Choose a reason for hiding this comment

magodo commented Jul 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbfrahry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbfrahry left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 2, 2024

`azurerm_cosmosdb_account`: support for CMK through `managed_hsm_key_id` property #26521

`azurerm_cosmosdb_account`: support for CMK through `managed_hsm_key_id` property #26521

wuxu92 commented Jul 2, 2024 •

edited

Loading