-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[Synapse] Add customer-managed key related cmdlets #16224
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
Changes from 8 commits
110b84e
b78ccc9
c578320
2c68b10
b0c48fe
419b47c
64dd329
869129e
a4ebe49
6510226
809e79e
4a8819f
f52f74a
f78679a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2,10 +2,11 @@ | |||||
| # Copyright (c) Microsoft Corporation. All rights reserved. | ||||||
| # Licensed under the MIT License. See License.txt in the project root for license information. | ||||||
| # -------------------------------------------------------------------------------------------- | ||||||
| # pylint: disable=unused-argument | ||||||
| # pylint: disable=unused-argument, line-too-long | ||||||
| from azure.cli.core.util import sdk_no_wait, CLIError | ||||||
| from azure.mgmt.synapse.models import Workspace, WorkspacePatchInfo, ManagedIdentity, \ | ||||||
| DataLakeStorageAccountDetails | ||||||
| DataLakeStorageAccountDetails, WorkspaceKeyDetails, CustomerManagedKeyDetails, EncryptionDetails, ManagedVirtualNetworkSettings, \ | ||||||
| ManagedIdentitySqlControlSettingsModelPropertiesGrantSqlControlToManagedIdentity | ||||||
|
|
||||||
|
|
||||||
| # Synapse workspace | ||||||
|
|
@@ -15,12 +16,19 @@ def list_workspaces(cmd, client, resource_group_name=None): | |||||
|
|
||||||
|
|
||||||
| def create_workspace(cmd, client, resource_group_name, workspace_name, storage_account, file_system, | ||||||
| sql_admin_login_user, sql_admin_login_password, location=None, enable_managed_virtual_network=None, | ||||||
| tags=None, no_wait=False): | ||||||
| sql_admin_login_user, sql_admin_login_password, location=None, key_name="default", key_identifier=None, enable_managed_virtual_network=None, | ||||||
| allowed_aad_tenant_ids=None, tags=None, no_wait=False): | ||||||
| identity_type = "SystemAssigned" | ||||||
| identity = ManagedIdentity(type=identity_type) | ||||||
| account_url = "https://{}.dfs.{}".format(storage_account, cmd.cli_ctx.cloud.suffixes.storage_endpoint) | ||||||
| default_data_lake_storage = DataLakeStorageAccountDetails(account_url=account_url, filesystem=file_system) | ||||||
| if str(key_identifier).endswith('/'): | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. str(None).endswith('/') will return false.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it can be removed. Do you think so?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree to remove the check logic. It seems not quite necessary. Server side should determine whether to accept backslash. |
||||||
| key_identifier = key_identifier[:-1] | ||||||
| workspace_key_detail = WorkspaceKeyDetails(name=key_name, key_vault_url=key_identifier) | ||||||
| encryption = EncryptionDetails(cmk=CustomerManagedKeyDetails(key=workspace_key_detail)) | ||||||
| managed_virtual_network_settings = None | ||||||
| if enable_managed_virtual_network: | ||||||
| managed_virtual_network_settings = ManagedVirtualNetworkSettings(preventDataExfiltration=True, allowed_aad_tenant_ids_for_linking=allowed_aad_tenant_ids) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I remember from portal, this property can be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will add a parameter to indicate whether enable data exfiltration.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, sounds good. |
||||||
|
|
||||||
| workspace_info = Workspace( | ||||||
| identity=identity, | ||||||
|
|
@@ -29,14 +37,23 @@ def create_workspace(cmd, client, resource_group_name, workspace_name, storage_a | |||||
| sql_administrator_login_password=sql_admin_login_password, | ||||||
| location=location, | ||||||
| managed_virtual_network="default" if enable_managed_virtual_network is True else None, | ||||||
| managed_virtual_network_settings=managed_virtual_network_settings, | ||||||
| encryption=encryption, | ||||||
| tags=tags | ||||||
| ) | ||||||
| return sdk_no_wait(no_wait, client.create_or_update, resource_group_name, workspace_name, workspace_info) | ||||||
|
|
||||||
|
|
||||||
| def update_workspace(cmd, client, resource_group_name, workspace_name, sql_admin_login_password=None, | ||||||
| tags=None, no_wait=False): | ||||||
| workspace_patch_info = WorkspacePatchInfo(tags=tags, sql_admin_login_password=sql_admin_login_password) | ||||||
| allowed_aad_tenant_ids=None, disable_all_allowed_aad_tenant_ids=None, tags=None, key_name=None, key_identifier=None, no_wait=False): | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Is it possible to combine these two parameters? For example, is it possible for users to pass an empty list (v.s. None value) to CLI?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. allowed_aad_tenant_ids is a List type defined in CLI, so it can't be Empty when user add it as parameter. Can we design it like "az synapse workspace update --allowed_aad_tenant_ids None", if allowed_aad_tenant_ids list contains "None", will disable all tenant ids.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jsntcy , could you please provide some insights on this? What is the general guidelines for users to update a list to none or empty.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you should use another command to clear the list, for example
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Find a similar cmdlet:
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Refer to
Use "" to clear.@sunsw1994 |
||||||
| if str(key_identifier).endswith('/'): | ||||||
| key_identifier = key_identifier[:-1] | ||||||
| workspace_key_detail = WorkspaceKeyDetails(name=key_name, key_vault_url=key_identifier) | ||||||
| encryption = EncryptionDetails(cmk=CustomerManagedKeyDetails(key=workspace_key_detail)) | ||||||
| if disable_all_allowed_aad_tenant_ids is True: | ||||||
| allowed_aad_tenant_ids = [] | ||||||
| updated_vnet_settings = ManagedVirtualNetworkSettings(allowed_aad_tenant_ids_for_linking=allowed_aad_tenant_ids) if allowed_aad_tenant_ids is not None else None | ||||||
| workspace_patch_info = WorkspacePatchInfo(tags=tags, sql_admin_login_password=sql_admin_login_password, encryption=encryption, managed_virtual_network_settings=updated_vnet_settings) | ||||||
| return sdk_no_wait(no_wait, client.update, resource_group_name, workspace_name, workspace_patch_info) | ||||||
|
|
||||||
|
|
||||||
|
|
@@ -67,3 +84,21 @@ def update_firewall_rule(cmd, client, resource_group_name, workspace_name, rule_ | |||||
| end_ip_address = end_ip_address or firewall.end_ip_address | ||||||
| return sdk_no_wait(no_wait, client.create_or_update, resource_group_name, workspace_name, rule_name, | ||||||
| start_ip_address=start_ip_address, end_ip_address=end_ip_address) | ||||||
|
|
||||||
|
|
||||||
| def create_workspace_key(cmd, client, resource_group_name, workspace_name, key_name, key_identifier, no_wait=False): | ||||||
| return sdk_no_wait(no_wait, client.create_or_update, resource_group_name, workspace_name, key_name=key_name, key_vault_url=key_identifier) | ||||||
|
|
||||||
|
|
||||||
| def activate_workspace(cmd, client, resource_group_name, workspace_name, key_name, key_identifier, no_wait=False): | ||||||
| return sdk_no_wait(no_wait, client.create_or_update, resource_group_name, workspace_name, is_active_cmk=True, key_name=key_name, key_vault_url=key_identifier) | ||||||
|
|
||||||
|
|
||||||
| def grant_sql_access_to_managed_identity(cmd, client, resource_group_name, workspace_name, no_wait=False): | ||||||
| grant_sql_access_setting = ManagedIdentitySqlControlSettingsModelPropertiesGrantSqlControlToManagedIdentity(desired_state="Enabled") | ||||||
| return sdk_no_wait(no_wait, client.create_or_update, resource_group_name, workspace_name, grant_sql_access_setting) | ||||||
|
|
||||||
|
|
||||||
| def revoke_sql_access_to_managed_identity(cmd, client, resource_group_name, workspace_name, no_wait=False): | ||||||
| revoke_sql_access_setting = ManagedIdentitySqlControlSettingsModelPropertiesGrantSqlControlToManagedIdentity(desired_state="Disabled") | ||||||
| return sdk_no_wait(no_wait, client.create_or_update, resource_group_name, workspace_name, revoke_sql_access_setting) | ||||||
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.
For what purpose that you make it a separate command rather than integrating it into
az synapse workspace update?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.
az synapse workspace update using Workspaces_Update SDK
az synapse workspace activate using Workspaces_CreateOrUpdate SDK
https://github.com/Azure/azure-cli/pull/16224/files/869129e44b85c3dbeb8c49ed7501b8cc7308703f#diff-51cec1d07d29bd22f589a3292d710c02d685386db30e10c283a41fffd6f562d6R94
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 think there must be some reasons that you use different SDK update method. Could you please share more details?
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.
Sorry didn't descript it clearly.
activate_workspace using the SDK defined at Key.json(CreateOrUpdate)
update_workspace using the SDK defined at workspace.json(Update)
That means these two cmdlets will use different client to send request. We can put activate_workspace 's logic into
az synapse workspace key update. But I don't think it may be more clear. How do you design at Powershll?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.
Will move it to
az synapse workspace key updateaccording to offline discussion.