[Synapse] Add customer-managed key related cmdlets by sunsw1994 · Pull Request #16224 · Azure/azure-cli

sunsw1994 · 2020-12-10T07:58:46Z

Description

Add some new commands to manage customer-managed key under synapse workspace.

Testing Guide

az synapse workspace create -h
az synapse workspace key -h
az synapse workspace managed-identity -h

History Notes

[Synapse] az synapse workspace create : Add key-identifier parameter to support to create workspace using customer-managed key.
[Synapse] az synapse workspace key: Add CRUD cmdlets to support to manage keys under specified synapse workspace.
[Synapse] az synapse workspace managed-identity: Add cmdlets to support CRUD managed identity to sql access setting.
[Synapse] az synapse workspace: Add data exfiltration protection support, add allowed_aad_tenant_ids parameter.

This checklist is used to make sure that common guidelines for a pull request are followed.

The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.

yonzhan · 2020-12-10T10:50:42Z

Synapse

sunsw1994 · 2020-12-10T10:54:49Z

Existing ci errors are not caused by the code change from this PR.

aim-for-better · 2020-12-10T10:57:14Z

src/azure-cli/azure/cli/command_modules/synapse/operations/workspace.py


 def create_workspace(cmd, client, resource_group_name, workspace_name, storage_account, file_system,
-                     sql_admin_login_user, sql_admin_login_password, location, enable_managed_virtual_network=None,
+                     sql_admin_login_user, sql_admin_login_password, location, key_name="default", key_identifier=None, enable_managed_virtual_network=None,


just curious here:
As you know key identifier contains the key name, if customer only provides the key_identifier and maybe the key name in key identifier is not "default", what will happen?

The key name is the display name at synapse workspace, different from the name of key-vault.

The key name is the display name at synapse workspace, different from the name of key-vault.

But the parameter is --key-name, maybe customer will think it is the key vault name, do you think it is a little confused?
@idear1203 How do you like?
I suggest to change the parameter name and give a clear description.

I think the current name --key-name is fine. It it different from --key-vault-name

In reply to: 540663549 [](ancestors = 540663549)

aim-for-better · 2020-12-10T11:09:11Z

Could you please also update the related test cases?

aim-for-better · 2020-12-11T03:30:13Z

src/azure-cli/azure/cli/command_modules/synapse/_params.py

            c.argument('sql_admin_login_password', options_list=['--sql-admin-login-password', '-p'],
                       help='The sql administrator login password.')
            c.argument('tags', arg_type=tags_type)
+            c.argument('key_name', help='The workspace key name.')


The workspace key name. [](start = 41, length = 23)

Shall we give more concrete description to make it clear that it is the workspace display key name instead of the name of key vault?

…e==0.6.0

sunsw1994 · 2021-01-13T05:16:58Z

@jsntcy Could you please help to review this PR? Thanks.

idear1203 · 2021-01-19T06:17:12Z

src/azure-cli/azure/cli/command_modules/synapse/operations/workspace.py

    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('/'):


key_identifier [](start = 11, length = 14)

If key_identifier is none, will logic here fail or not?

str(None).endswith('/') will return false.

I think it can be removed. Do you think so?

Agree to remove the check logic. It seems not quite necessary. Server side should determine whether to accept backslash.

idear1203 · 2021-01-19T06:17:57Z

src/azure-cli/azure/cli/command_modules/synapse/operations/workspace.py

+    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)


preventDataExfiltration=True [](start = 73, length = 28)

I remember from portal, this property can be False

Will add a parameter to indicate whether enable data exfiltration.

--enable_data_exfiltration is fine?

Yes, sounds good.

idear1203 · 2021-01-19T06:21:10Z

src/azure-cli/azure/cli/command_modules/synapse/operations/workspace.py

 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):


allowed_aad_tenant_ids=None, disable_all_allowed_aad_tenant_ids=None, [](start = 21, length = 69)

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?

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.

if allowed_aad_tenant_ids is not None and "None" in allowed_aad_tenant_ids: allowed_aad_tenant_ids = []

@jsntcy , could you please provide some insights on this? What is the general guidelines for users to update a list to none or empty.

I think you should use another command to clear the list, for example az synapse workspace disable_allowed_aad_tenant_ids to do that.

Find a similar cmdlet:

nargs='*' instead of nargs='+' , user can pass an empty list

azure-cli/src/azure-cli/azure/cli/command_modules/keyvault/_params.py

Line 298 in e015d5b

c.argument('key_ops', arg_type=get_enum_type(JsonWebKeyOperation), options_list=['--ops'], nargs='*',

Refer to

azure-cli/src/azure-cli/azure/cli/command_modules/network/_params.py

Line 1106 in 5aa3d4e

c.argument('dns_servers', help='Space-separated list of DNS server IP addresses. Use ""(\'""\' in PowerShell) to revert to default Azure servers.', nargs='+', arg_group='DNS')

Use "" to clear.
@sunsw1994

idear1203 · 2021-01-19T06:29:11Z

src/azure-cli/azure/cli/command_modules/synapse/commands.py

        g.custom_command('list', 'list_workspaces')
        g.custom_command('create', 'create_workspace', supports_no_wait=True)
+        g.custom_command('activate', 'activate_workspace', client_factory=cf_synapse_client_cmk_factory, supports_no_wait=True)
        g.custom_command('update', 'update_workspace', supports_no_wait=True)


For what purpose that you make it a separate command rather than integrating it into az synapse workspace update?

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

I think there must be some reasons that you use different SDK update method. Could you please share more details?

Sorry didn't descript it clearly.
activate_workspace using the SDK defined at Key.json(CreateOrUpdate)

g.custom_command('activate', 'activate_workspace', client_factory=cf_synapse_client_cmk_factory, supports_no_wait=True)

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?

Will move it to az synapse workspace key update according to offline discussion.

sunsw1994 · 2021-01-22T02:34:14Z

hi @jsntcy, please help to merge this PR if that looks good to you. Thanks a lot.

sunsw1994 · 2021-01-25T06:11:42Z

Hi @Juliehzl ，please help to merge this PR if that looks good to you. Thanks a lot.

add cmk support

110b84e

sunsw1994 requested review from aim-for-better and idear1203 December 10, 2020 07:58

sunsw1994 added 2 commits December 10, 2020 16:24

fix ci error

b78ccc9

fix ci error

c578320

yonzhan assigned jsntcy Dec 10, 2020

yonzhan requested a review from jsntcy December 10, 2020 10:51

yonzhan added this to the S180 milestone Dec 10, 2020

aim-for-better reviewed Dec 10, 2020

View reviewed changes

aim-for-better reviewed Dec 11, 2020

View reviewed changes

sunsw1994 marked this pull request as draft December 11, 2020 05:10

sunsw1994 and others added 5 commits December 11, 2020 15:45

add data exfiltration protection support

2c68b10

1.support sql control to managed identity 2.upgrade azure-mgmt-synaps…

b0c48fe

…e==0.6.0

code style updated

419b47c

Merge branch 'dev' into synapse-cmk

64dd329

fix ci error

869129e

sunsw1994 marked this pull request as ready for review December 25, 2020 07:04

yonzhan modified the milestones: S180, S181 Dec 26, 2020

jsntcy requested a review from Juliehzl January 15, 2021 01:56

jsntcy approved these changes Jan 15, 2021

View reviewed changes

yonzhan modified the milestones: S181, S182 Jan 15, 2021

idear1203 reviewed Jan 19, 2021

View reviewed changes

sunsw1994 added 4 commits January 19, 2021 16:31

address comments

a4ebe49

address comments

6510226

fix ci issuses

809e79e

fix ci issuses

4a8819f

idear1203 self-requested a review January 21, 2021 08:14

idear1203 approved these changes Jan 21, 2021

View reviewed changes

add scenario test and test recording

f52f74a

address comment about disable allowed tenant ids

f78679a

jsntcy merged commit d73236e into Azure:dev Jan 26, 2021

idear1203 mentioned this pull request Feb 1, 2021

Setting AD-Admin via AZ CLI/Powershell for Synapse #16025

Closed

Conversation

sunsw1994 commented Dec 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yonzhan commented Dec 10, 2020

Uh oh!

sunsw1994 commented Dec 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aim-for-better commented Dec 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sunsw1994 commented Jan 13, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sunsw1994 Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsntcy Jan 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sunsw1994 commented Jan 22, 2021

Uh oh!

sunsw1994 commented Jan 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sunsw1994 commented Dec 10, 2020 •

edited

Loading

sunsw1994 Jan 21, 2021 •

edited

Loading

jsntcy Jan 25, 2021 •

edited

Loading