-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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_databricks_workspace
: managed service/disk support managed hsm key
#27849
base: main
Are you sure you want to change the base?
Conversation
e966538
to
3f24aa5
Compare
904ccf6
to
bd0c744
Compare
azurerm_databricks_workspace
: managed service support managed hsm keyazurerm_databricks_workspace
: managed service/disk support managed hsm key
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.
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. 🚀
"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", |
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.
Shouldn't this conflict with managed_services_cmk_key_vault_key_id
?
"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"}, |
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.
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.
|
||
"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", |
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.
"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"}, |
Type: pluginsdk.TypeString, | ||
Optional: true, | ||
ValidateFunc: keyVaultValidate.KeyVaultChildID, | ||
ConflictsWith: []string{"managed_disk_cmk_managed_hsm_key_id"}, |
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.
Shouldn't this also conflict with managed_disk_cmk_key_vault_id
?
ConflictsWith: []string{"managed_disk_cmk_managed_hsm_key_id"}, | |
ConflictsWith: []string{"managed_disk_cmk_managed_hsm_key_id", "managed_disk_cmk_key_vault_id"}, |
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.
Like above, we cannot have it conflict with managed_disk_cmk_key_vault_id
here.
eb448b5
to
ff0df84
Compare
@wuxu92, thanks for pushing those changes so quickly. This LGTM now! 🚀 |
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.
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!
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. |
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. |
b934160
to
2d6f936
Compare
a246a75
to
ca16ca4
Compare
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! |
ca16ca4
to
6bab973
Compare
@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. |
6bab973
to
51ae7a1
Compare
@stephybun I rebased the main branch and updated the corresponding code. |
…deprecate xx_key_vault_id for cross subscription support as not required add deprecate in 5.0 beta
8b0333a
to
c4c39a7
Compare
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 for rebasing @wuxu92.
In addition to the comments left in-line, can you please provide local testing evidence for the tests in 4.x as well as 5.0 mode?
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 v4.0 of the provider", |
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.
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 v4.0 of the provider", | |
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 AzureRM provider", |
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 v4.0 of the provider", |
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.
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 v4.0 of the provider", | |
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 AzureRM provider", |
clientHub := meta.(*clients.Client) | ||
client := clientHub.DataBricks.WorkspacesClient | ||
acClient := clientHub.DataBricks.AccessConnectorClient | ||
lbClient := clientHub.LoadBalancers.LoadBalancersClient | ||
accountClient := clientHub.Account | ||
subscriptionId := accountClient.SubscriptionId |
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.
There was no real need to change this and it introduces an inconsistent style around the client variable declarations in the provider, can you please leave these long hand? I'll make a note of this and see if we can look to publish a style guide for the provider
clientHub := meta.(*clients.Client) | |
client := clientHub.DataBricks.WorkspacesClient | |
acClient := clientHub.DataBricks.AccessConnectorClient | |
lbClient := clientHub.LoadBalancers.LoadBalancersClient | |
accountClient := clientHub.Account | |
subscriptionId := accountClient.SubscriptionId | |
client := meta.(*clients.Client).DataBricks.WorkspacesClient | |
acClient := meta.(*clients.Client).DataBricks.AccessConnectorClient | |
lbClient := meta.(*clients.Client).LoadBalancers.LoadBalancersClient | |
accountClient := meta.(*clients.Client).Account | |
subscriptionId := accountClient.SubscriptionId |
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.
Sure, I'll adjust this style as you suggested. I just don't like repeating the code, but I'm fine with it to stay consistent with the provider.
// VersionPtr returns a pointer to the version string if it exists, otherwise nil. | ||
// This is useful for resources supporting versioned and versionless keys. | ||
func (k *KeyVaultOrManagedHSMKey) VersionPtr() *string { | ||
if k.KeyVaultKeyId != nil && k.KeyVaultKeyId.Version != "" { | ||
return pointer.To(k.KeyVaultKeyId.Version) | ||
} | ||
|
||
if k.ManagedHSMKeyId != nil { | ||
return pointer.To(k.ManagedHSMKeyId.KeyVersion) | ||
} | ||
|
||
return nil | ||
} |
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.
I don't see any usages of this yet, where do you foresee this being used?
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.
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.
I noticed azurerm_log_analytics_cluster_customer_managed_key
is on your list here, I have started work on this as part of #27433
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.
@sreallymatt Thank you for letting me know! It hasn't started yet, so I'll skip it as per your works.
@@ -197,3 +241,9 @@ func FlattenKeyVaultOrManagedHSMID(id string, hsmEnv environments.Api) (*KeyVaul | |||
|
|||
return nil, fmt.Errorf("cannot parse given id to key vault key nor managed hsm key: %s", id) | |||
} | |||
|
|||
func FlattenKeyVaultOrManagedHSMIDByComponents(baseUri, name, version string, hsmEnv environments.Api) (*KeyVaultOrManagedHSMKey, error) { |
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.
It should be possible to determine the key type (key vault or managed hsm) based on the contents of the base URI. With this we can instantiate and return the correct object ID using the relevant NewSomeResourceID
function and wouldn't need to call the FlattenKeyVaultOrManagedHSMID
function anymore.
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.
I updated this method by creating an instance using the corresponding construction method. However, the NewHSMXXID function requires the HSM name instead of the baseUri(which is typically returned by the API). Therefore, the HSM ID part still needs to parse the ID format string.
}) | ||
} | ||
|
||
func testAccDatabricksWorkspace_CmkManagedHSMDiskOnly(t *testing.T) { |
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.
func testAccDatabricksWorkspace_CmkManagedHSMDiskOnly(t *testing.T) { | |
func testAccDatabricksWorkspace_cmkManagedHSMDiskOnly(t *testing.T) { |
}) | ||
} | ||
|
||
func testAccDatabricksWorkspace_CmkManagedHSMDiskOnlyAltSubscription(t *testing.T) { |
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.
func testAccDatabricksWorkspace_CmkManagedHSMDiskOnlyAltSubscription(t *testing.T) { | |
func testAccDatabricksWorkspace_cmkManagedHSMDiskOnlyAltSubscription(t *testing.T) { |
@@ -48,21 +48,15 @@ The following arguments are supported: | |||
|
|||
~> **Note:** Downgrading to a `trial sku` from a `standard` or `premium sku` will force a new resource to be created. | |||
|
|||
* `managed_services_cmk_key_vault_id` - (Optional) Resource ID of the Key Vault which contains the `managed_services_cmk_key_vault_key_id` key. | |||
* `managed_services_cmk_key_vault_key_id` - (Optional) Customer managed encryption properties using a versioned Key Vaule Key ID for the Databricks Workspace managed resources(e.g. Notebooks and Artifacts). |
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.
* `managed_services_cmk_key_vault_key_id` - (Optional) Customer managed encryption properties using a versioned Key Vaule Key ID for the Databricks Workspace managed resources(e.g. Notebooks and Artifacts). | |
* `managed_services_cmk_key_vault_key_id` - (Optional) Customer managed encryption properties using a versioned Key Vault Key ID for the Databricks Workspace managed resources e.g. Notebooks and Artifacts. |
|
||
-> **Note:** The `managed_services_cmk_key_vault_id` field is only required if the Key Vault exists in a different subscription than the Databricks Workspace. If the `managed_services_cmk_key_vault_id` field is not specified it is assumed that the `managed_services_cmk_key_vault_key_id` is hosted in the same subscriptioin as the Databricks Workspace. | ||
* `managed_services_cmk_managed_hsm_key_id` - (Optional) Customer managed encryption properties using a versioned Managed Hardware Security Key ID for the Databricks Workspace managed resources(e.g. Notebooks and Artifacts). |
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.
* `managed_services_cmk_managed_hsm_key_id` - (Optional) Customer managed encryption properties using a versioned Managed Hardware Security Key ID for the Databricks Workspace managed resources(e.g. Notebooks and Artifacts). | |
* `managed_services_cmk_managed_hsm_key_id` - (Optional) Customer managed encryption properties using a versioned Managed Hardware Security Key ID for the Databricks Workspace managed resources e.g. Notebooks and Artifacts. |
|
||
-> **Note:** If you are using multiple service principals to execute Terraform across subscriptions you will need to add an additional `azurerm_key_vault_access_policy` resource granting the service principal access to the key vault in that subscription. | ||
|
||
* `managed_disk_cmk_key_vault_id` - (Optional) Resource ID of the Key Vault which contains the `managed_disk_cmk_key_vault_key_id` key. | ||
* `managed_disk_cmk_key_vault_key_id` - (Optional) Customer managed encryption properties using a versioned Key Vaule Key ID for the Databricks Workspace managed disks. |
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.
* `managed_disk_cmk_key_vault_key_id` - (Optional) Customer managed encryption properties using a versioned Key Vaule Key ID for the Databricks Workspace managed disks. | |
* `managed_disk_cmk_key_vault_key_id` - (Optional) Customer managed encryption properties using a versioned Key Vault Key ID for the Databricks Workspace managed disks. |
tests with 5.0 flag enabled, there is a new failure which is not related to this PR, I can fix it in a separate PR: |
Community Note
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
andmanaged_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
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_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):
Related Issue(s)
Fixes #27738
Note
If this PR changes meaningfully during the course of review please update the title and description as required.