`azurerm_databricks_workspace`: managed service/disk support managed hsm key #27849

wuxu92 · 2024-11-01T03:19:48Z

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

DRAFT FOR INTERNAL REVIEW

This PR adds Managed HSM Key support for both managed services and managed disks.

It also deprecates the optional key vault ID property for both, as it is unnecessary. The Databricks server can validate the key's availability, so managed_services_cmk_key_vault_id and managed_disk_cmk_key_vault_id have been removed from the documentation. A deprecated description has been added to them, but they are not being removed from the schema to avoid introducing a breaking change.

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)

Change Log

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

azurerm_databricks_workspace - support managed hsm key as managed service and managed disk encription [azurerm_databricks_workspace - missing support for keys from a Managed HSM Key Vault #27738]

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 #27738

Note

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

WodansSon

Hi @wuxu92, I have given this PR and it mostly looks good, however I have left a few minor comments around the schema and depreciation text. If you can get those fixed up, I think this will be good to move out of draft state. 🚀

WodansSon · 2024-11-15T02:11:12Z

internal/services/databricks/databricks_workspace_resource.go

 			"managed_services_cmk_key_vault_id": {
 				Type:         pluginsdk.TypeString,
 				Optional:     true,
 				ValidateFunc: commonids.ValidateKeyVaultID,
+				Deprecated:   "Use 'managed_services_cmk_key_vault_key_id' instead, this field is not needed for cross subscription key vaults anymore",


Shouldn't this conflict with managed_services_cmk_key_vault_key_id?

Suggested change

"managed_services_cmk_key_vault_id": {

Type: pluginsdk.TypeString,

Optional: true,

ValidateFunc: commonids.ValidateKeyVaultID,

Deprecated: "Use 'managed_services_cmk_key_vault_key_id' instead, this field is not needed for cross subscription key vaults anymore",

"managed_services_cmk_key_vault_id": {

Type: pluginsdk.TypeString,

Optional: true,

ValidateFunc: commonids.ValidateKeyVaultID,

Deprecated: "'managed_services_cmk_key_vault_id' has been deprecated in favor of 'managed_services_cmk_key_vault_key_id' and will be removed in v5.0 of the provider",

ConflictsWith: []string{"managed_services_cmk_key_vault_key_id"},

We cannot add ConflictsWith here because some users may have set both managed_services_cmk_key_vault_id and managed_services_cmk_key_vault_key_id. Do you mean the new managed_disk_cmk_managed_hsm_key_id property should conflict with managed_services_cmk_key_vault_id? That would be reasonable. I updated it.

WodansSon · 2024-11-15T02:26:39Z

internal/services/databricks/databricks_workspace_resource.go

+
 			"managed_disk_cmk_key_vault_id": {
 				Type:         pluginsdk.TypeString,
 				Optional:     true,
 				ValidateFunc: commonids.ValidateKeyVaultID,
+				Deprecated:   "Use 'managed_disk_cmk_key_vault_key_id' instead, this field is not needed for cross subscription key vaults anymore",


Suggested change

"managed_disk_cmk_key_vault_id": {

Type: pluginsdk.TypeString,

Optional: true,

ValidateFunc: commonids.ValidateKeyVaultID,

Deprecated: "Use 'managed_disk_cmk_key_vault_key_id' instead, this field is not needed for cross subscription key vaults anymore",

"managed_disk_cmk_key_vault_id": {

Type: pluginsdk.TypeString,

Optional: true,

ValidateFunc: commonids.ValidateKeyVaultID,

Deprecated: "'managed_disk_cmk_key_vault_id' has been deprecated in favor of 'managed_disk_cmk_key_vault_key_id' and will be removed in v5.0 of the provider",

ConflictsWith: []string{"managed_disk_cmk_key_vault_key_id"},

WodansSon · 2024-11-15T03:30:16Z

internal/services/databricks/databricks_workspace_resource.go

+				Type:          pluginsdk.TypeString,
+				Optional:      true,
+				ValidateFunc:  keyVaultValidate.KeyVaultChildID,
+				ConflictsWith: []string{"managed_disk_cmk_managed_hsm_key_id"},


Shouldn't this also conflict with managed_disk_cmk_key_vault_id?

Suggested change

ConflictsWith: []string{"managed_disk_cmk_managed_hsm_key_id"},

ConflictsWith: []string{"managed_disk_cmk_managed_hsm_key_id", "managed_disk_cmk_key_vault_id"},

Like above, we cannot have it conflict with managed_disk_cmk_key_vault_id here.

WodansSon · 2024-11-15T07:08:20Z

@wuxu92, thanks for pushing those changes so quickly. This LGTM now! 🚀

stephybun

Hey @wuxu92, could you please follow the guidance in our breaking changes guide when deprecating properties? Once those changes have been made can you please also provide testing evidence in both 4.x and 5.0 mode. Thanks!

wuxu92 · 2024-11-15T09:17:50Z

Hi @stephybun, since those two properties are not needed for the business, they are just read from the config and set back to the state even in the original logic. Therefore, I merely marked them as deprecated. If you think it's still necessary, I can add the required work for a breaking change.

stephybun · 2024-11-15T09:27:17Z

Yes, please still follow the guidance that I linked. This will ensure that the property is actually removed in 5.0 and we don't need to remember to remove it from the schema in the week of the major release. Make sure to update any affected tests as well as the upgrade guide too.

wuxu92 · 2024-11-18T09:32:46Z

5.0 Beta flag Not enabled:

5.0 Beta flag enabled

github-actions · 2024-12-30T06:03:45Z

This PR is being labeled as "stale" because it has not been updated for 30 or more days.

If this PR is still valid, please remove the "stale" label. If this PR is blocked, please add it to the "Blocked" milestone.

If you need some help completing this PR, please leave a comment letting us know. Thank you!

stephybun · 2025-01-28T10:51:04Z

@wuxu92 I started a review on this a while ago, but since then the CreateUpdate method in this resource has been split. Could you please rebase this PR? I can continue the review once that's been done.

…deprecate xx_key_vault_id for cross subscription support as not required add deprecate in 5.0 beta

wuxu92 · 2025-02-05T06:43:24Z

@stephybun I rebased the main branch and updated the corresponding code.

wuxu92 force-pushed the databricks/cmk-hsm branch 2 times, most recently from e966538 to 3f24aa5 Compare November 1, 2024 03:21

github-actions bot added waiting-response and removed waiting-response labels Nov 1, 2024

wuxu92 force-pushed the databricks/cmk-hsm branch from 904ccf6 to bd0c744 Compare November 1, 2024 14:25

wuxu92 marked this pull request as ready for review November 1, 2024 14:30

wuxu92 requested review from katbyte and a team as code owners November 1, 2024 14:30

rcskosir added service/databricks enhancement documentation labels Nov 1, 2024

wuxu92 changed the title ~~azurerm_databricks_workspace: managed service support managed hsm key~~ azurerm_databricks_workspace: managed service/disk support managed hsm key Nov 2, 2024

wuxu92 marked this pull request as draft November 3, 2024 02:01

github-actions bot added the size/XL label Nov 6, 2024

WodansSon requested changes Nov 15, 2024

View reviewed changes

github-actions bot added waiting-response and removed waiting-response labels Nov 15, 2024

wuxu92 force-pushed the databricks/cmk-hsm branch from eb448b5 to ff0df84 Compare November 15, 2024 04:12

wuxu92 marked this pull request as ready for review November 15, 2024 06:21

stephybun requested changes Nov 15, 2024

View reviewed changes

github-actions bot added the waiting-response label Nov 15, 2024

github-actions bot removed the waiting-response label Nov 15, 2024

wuxu92 force-pushed the databricks/cmk-hsm branch from b934160 to 2d6f936 Compare November 18, 2024 02:10

github-actions bot added waiting-response and removed waiting-response labels Nov 18, 2024

wuxu92 force-pushed the databricks/cmk-hsm branch from 7fc50cc to efa7b2d Compare November 18, 2024 05:57

github-actions bot added waiting-response and removed waiting-response labels Nov 18, 2024

wuxu92 force-pushed the databricks/cmk-hsm branch from 8ccc2e8 to a246a75 Compare November 18, 2024 08:17

github-actions bot added waiting-response and removed waiting-response labels Nov 18, 2024

github-actions bot removed the waiting-response label Nov 18, 2024

wuxu92 force-pushed the databricks/cmk-hsm branch from a246a75 to ca16ca4 Compare November 28, 2024 06:25

github-actions bot added the waiting-response label Nov 28, 2024

github-actions bot added the stale label Dec 30, 2024

wuxu92 force-pushed the databricks/cmk-hsm branch from ca16ca4 to 6bab973 Compare January 6, 2025 01:52

github-actions bot added waiting-response and removed waiting-response stale labels Jan 6, 2025

data bricks workspaces managed service/disk support managed hsm key, …

51ae7a1

…deprecate xx_key_vault_id for cross subscription support as not required add deprecate in 5.0 beta

wuxu92 force-pushed the databricks/cmk-hsm branch from 6bab973 to 51ae7a1 Compare February 5, 2025 04:26

github-actions bot removed the waiting-response label Feb 5, 2025

update with hsm cmk

8b0333a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_databricks_workspace`: managed service/disk support managed hsm key #27849

`azurerm_databricks_workspace`: managed service/disk support managed hsm key #27849

wuxu92 commented Nov 1, 2024 •

edited

Loading

WodansSon left a comment

WodansSon Nov 15, 2024

wuxu92 Nov 15, 2024

WodansSon Nov 15, 2024

WodansSon Nov 15, 2024

wuxu92 Nov 15, 2024

WodansSon commented Nov 15, 2024

stephybun left a comment

wuxu92 commented Nov 15, 2024

stephybun commented Nov 15, 2024

wuxu92 commented Nov 18, 2024 •

edited

Loading

github-actions bot commented Dec 30, 2024

stephybun commented Jan 28, 2025

wuxu92 commented Feb 5, 2025

	ConflictsWith: []string{"managed_disk_cmk_managed_hsm_key_id"},
	ConflictsWith: []string{"managed_disk_cmk_managed_hsm_key_id", "managed_disk_cmk_key_vault_id"},

azurerm_databricks_workspace: managed service/disk support managed hsm key #27849

Are you sure you want to change the base?

azurerm_databricks_workspace: managed service/disk support managed hsm key #27849

Conversation

wuxu92 commented Nov 1, 2024 • edited Loading

Community Note

Description

PR Checklist

Changes to existing Resource / Data Source

Testing

Change Log

Related Issue(s)

WodansSon left a comment

Choose a reason for hiding this comment

WodansSon Nov 15, 2024

Choose a reason for hiding this comment

wuxu92 Nov 15, 2024

Choose a reason for hiding this comment

WodansSon Nov 15, 2024

Choose a reason for hiding this comment

WodansSon Nov 15, 2024

Choose a reason for hiding this comment

wuxu92 Nov 15, 2024

Choose a reason for hiding this comment

WodansSon commented Nov 15, 2024

stephybun left a comment

Choose a reason for hiding this comment

wuxu92 commented Nov 15, 2024

stephybun commented Nov 15, 2024

wuxu92 commented Nov 18, 2024 • edited Loading

github-actions bot commented Dec 30, 2024

stephybun commented Jan 28, 2025

wuxu92 commented Feb 5, 2025

`azurerm_databricks_workspace`: managed service/disk support managed hsm key #27849

`azurerm_databricks_workspace`: managed service/disk support managed hsm key #27849

wuxu92 commented Nov 1, 2024 •

edited

Loading

wuxu92 commented Nov 18, 2024 •

edited

Loading