Skip to content

[KeyVault] Support private endpoint connections #12073

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.

Sign up for GitHub

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

Merged
bim-msft merged 22 commits into from
Feb 11, 2020
Merged

[KeyVault] Support private endpoint connections #12073

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
320ad5a
Modify random vault name prefix
bim-msft Feb 6, 2020
b2ee5aa
Reduce the prefix length and rerun all tests
bim-msft Feb 6, 2020
3f03c0e
Recover test_keyvault_certificate_get_default_policy.yaml
bim-msft Feb 7, 2020
037d321
Support private endpoint connection
bim-msft Dec 30, 2019
dd5b3a4
Use a simple paramter name
bim-msft Jan 8, 2020
f53c836
Support --connection-id
bim-msft Jan 9, 2020
0c4908a
Bump SDK version
bim-msft Feb 4, 2020
1536780
Add recording files
bim-msft Feb 6, 2020
e16e266
Bump SDK version to 2.1.1 and rerun all tests
bim-msft Feb 7, 2020
cc53d1f
Fix linter errors
bim-msft Feb 7, 2020
28a3474
Fix test
bim-msft Feb 7, 2020
58bcd54
Fix tests
bim-msft Feb 7, 2020
0ba10ec
Revert
bim-msft Feb 7, 2020
abfa412
Remove test_keyvault_certificate_get_default_policy.yaml
bim-msft Feb 7, 2020
0c35744
Revert
bim-msft Feb 7, 2020
e816aa9
Update test
bim-msft Feb 7, 2020
eb910a9
Remove unused library
bim-msft Feb 8, 2020
b2d332d
Update src/azure-cli/azure/cli/command_modules/keyvault/tests/latest/…
bim-msft Feb 11, 2020
06412f6
Refine code
bim-msft Feb 11, 2020
2a8f43c
Refine function name
bim-msft Feb 11, 2020
d56da79
Update yaml files
bim-msft Feb 11, 2020
8ac1614
Updated
bim-msft Feb 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/azure-cli/azure/cli/command_modules/keyvault/_client_factory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,14 @@ def keyvault_client_vaults_factory(cli_ctx, _):
return keyvault_client_factory(cli_ctx).vaults


def keyvault_client_private_endpoint_connections_factory(cli_ctx, _):
return keyvault_client_factory(cli_ctx).private_endpoint_connections


def keyvault_client_private_link_resources_factory(cli_ctx, _):
return keyvault_client_factory(cli_ctx).private_link_resources


def keyvault_data_plane_factory(cli_ctx, _):
from azure.keyvault import KeyVaultAuthentication, KeyVaultClient
from azure.cli.core.profiles import ResourceType, get_api_version
Expand Down
25 changes: 25 additions & 0 deletions src/azure-cli/azure/cli/command_modules/keyvault/_help.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,31 @@
short-summary: Manage vault network ACLs.
"""

helps['keyvault private-endpoint'] = """
type: group
short-summary: Manage vault private endpoint connections.
"""
Comment on lines +147 to +150
Copy link
Member

@jiasli jiasli Feb 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, we'd better name it as private-endpoint-connection as it corresponds to privateEndpointConnections from az keyvault show.

    "privateEndpointConnections": [
      {
        "etag": "9fae19d5c0704012a6b0544574fe24d2",
        "id": "/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590/resourceGroups/rg1/providers/Microsoft.KeyVault/vaults/kv0130/privateEndpointConnections/c9e56b4f661f4cf382aeb5d832bf6f9f",
        "privateEndpoint": {
          "id": "/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590/resourceGroups/rg1/providers/Microsoft.Network/privateEndpoints/pe1",
          "resourceGroup": "rg1"
        },
        "privateLinkServiceConnectionState": {
          "actionRequired": null,
          "actionsRequired": "None",
          "description": "",
          "status": "Approved"
        },
        "provisioningState": "Succeeded",
        "resourceGroup": "rg1"
      }

privateEndpoint stands for another resource type Microsoft.Network/privateEndpoints. I think we shouldn't mix these two concepts.

Copy link
Contributor Author

@bim-msft bim-msft Feb 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jiasli Thanks for this, for the convenience, I shortened the group name as private-endpoint (in fact, I copied the group name from network private-endpoint), but as you mentioned, this name stands for another resource type and may cause confusion, I will modify this later.


helps['keyvault private-endpoint delete'] = """
type: command
short-summary: Delete the specified private endpoint connection associated with a Key Vault.
"""

helps['keyvault private-endpoint show'] = """
type: command
short-summary: Show details of a private endpoint connection associated with a Key Vault.
"""

helps['keyvault private-link-resource'] = """
type: group
short-summary: Manage vault private link resources.
"""

helps['keyvault private-link-resource show'] = """
type: command
short-summary: Show the private link resources supported for a Key Vault.
Comment on lines +167 to +169
Copy link
Member

@jiasli jiasli Feb 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Line 745 in c0e345c

c.argument('group_ids', nargs='+', help='The ID(s) of the group(s) obtained from the remote resource that this private endpoint should connect to. You can use "az keyvault(storage/etc) private-endpoint show" to obtain the list of group ids.')

The az network private-endpoint create -h's help message doesn't conform to this command name. @myronfanqiu

"""

helps['keyvault recover'] = """
type: command
short-summary: Recover a key vault.
Expand Down
29 changes: 22 additions & 7 deletions src/azure-cli/azure/cli/command_modules/keyvault/_params.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@
datetime_type, certificate_type,
get_vault_base_url_type, validate_key_import_source,
validate_key_type, validate_policy_permissions,
validate_principal, validate_resource_group_name,
validate_x509_certificate_chain,
validate_principal,
validate_resource_group_name, validate_x509_certificate_chain,
secret_text_encoding_values, secret_binary_encoding_values, validate_subnet,
validate_vault_id, validate_sas_definition_id, validate_storage_account_id, validate_storage_disabled_attribute,
validate_deleted_vault_name)
Expand Down Expand Up @@ -108,15 +108,31 @@ class CLIJsonWebKeyOperation(str, Enum):

with self.argument_context('keyvault set-policy', arg_group='Permission') as c:
c.argument('object_id', validator=validate_principal)
c.argument('key_permissions', arg_type=get_enum_type(KeyPermissions), metavar='PERM', nargs='*', help='Space-separated list of key permissions to assign.', validator=validate_policy_permissions)
c.argument('secret_permissions', arg_type=get_enum_type(SecretPermissions), metavar='PERM', nargs='*', help='Space-separated list of secret permissions to assign.')
c.argument('certificate_permissions', arg_type=get_enum_type(CertificatePermissions), metavar='PERM', nargs='*', help='Space-separated list of certificate permissions to assign.')
c.argument('storage_permissions', arg_type=get_enum_type(StoragePermissions), metavar='PERM', nargs='*', help='Space-separated list of storage permissions to assign.')
c.argument('key_permissions', arg_type=get_enum_type(KeyPermissions), metavar='PERM', nargs='*',
help='Space-separated list of key permissions to assign.', validator=validate_policy_permissions)
c.argument('secret_permissions', arg_type=get_enum_type(SecretPermissions), metavar='PERM', nargs='*',
help='Space-separated list of secret permissions to assign.')
c.argument('certificate_permissions', arg_type=get_enum_type(CertificatePermissions), metavar='PERM', nargs='*',
help='Space-separated list of certificate permissions to assign.')
c.argument('storage_permissions', arg_type=get_enum_type(StoragePermissions), metavar='PERM', nargs='*',
help='Space-separated list of storage permissions to assign.')

with self.argument_context('keyvault network-rule', min_api='2018-02-14') as c:
c.argument('ip_address', help='IPv4 address or CIDR range.')
c.argument('subnet', help='Name or ID of subnet. If name is supplied, `--vnet-name` must be supplied.')
c.argument('vnet_name', help='Name of a virtual network.', validator=validate_subnet)

with self.argument_context('keyvault private-endpoint', min_api='2018-02-14') as c:
c.argument('approval_description', help='Comments for the approval.')
c.argument('connection_name', required=False,
help='The name of the private endpoint connection associated with the Key Vault. '
'Required if --connection-id is not specified')
c.argument('connection_id', required=False,
help='The ID of the private endpoint connection associated with the Key Vault. '
'If specified --vault-name and --connection-name, this should be omitted.')
c.argument('vault_name', vault_name_type, options_list=['--name', '-n'], required=False,
help='Name of the Key Vault. Required if --connection-id is not specified')
c.argument('rejection_description', help='Comments for the rejection.')
# endregion

# region Shared
Expand Down Expand Up @@ -216,7 +232,6 @@ class CLIJsonWebKeyOperation(str, Enum):
# endregion

# region KeyVault Storage Account

with self.argument_context('keyvault storage', arg_group='Id') as c:
c.argument('storage_account_name', options_list=['--name', '-n'], help='Name to identify the storage account in the vault.', id_part='child_name_1', completer=get_keyvault_name_completion_list('storage_account'))
c.argument('vault_base_url', vault_name_type, type=get_vault_base_url_type(self.cli_ctx), id_part=None)
Expand Down
21 changes: 21 additions & 0 deletions src/azure-cli/azure/cli/command_modules/keyvault/_validators.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,27 @@ def validate_policy_permissions(ns):
'--certificate-permissions --storage-permissions')


def validate_private_endpoint_connection_id(cmd, ns):
connection_id = ns.connection_id
connection_name = ns.connection_name
vault_name = ns.vault_name

if not connection_id:
if not all([connection_name, vault_name]):
raise argparse.ArgumentError(
Copy link
Contributor

@mmyyrroonn mmyyrroonn Feb 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on my knowledge, usually we would raise CLIError('usage error:'). We can discuss.

None, 'specify both: --connection-name and --vault-name')
Copy link
Member

@jiasli jiasli Feb 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In az keyvault private-endpoint show -h, it should be --name instead of --vault-name.

Suggested change
None, 'specify both: --connection-name and --vault-name')
None, 'specify both: --connection-name and --name')

Copy link
Contributor Author

@bim-msft bim-msft Feb 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jiasli I prefer to replace all --name to --vault-name as this is consistent with other subgroup like az keyvault key, for the subgroup, --name/-n should represent the name of the sub resource itself (key, secret, private-endpoint), the name of vault should be represented by --vault-name.
How do you think?

Copy link
Member

@jiasli jiasli Feb 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. In that case, it should be --vault-name here:

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

Line 133 in cc53d1f

c.argument('vault_name', vault_name_type, options_list=['--name', '-n'], required=False,

Copy link
Contributor Author

@bim-msft bim-msft Feb 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jiasli Yea, I will change it.

ns.resource_group_name = _get_resource_group_from_vault_name(cmd.cli_ctx, vault_name)
else:
if any([connection_name, vault_name]):
raise argparse.ArgumentError(
None, 'you don\'t need to specify --connection-name or --vault-name if --connection-id is specified')

id_parts = connection_id.split('/')
ns.connection_name = id_parts[-1]
ns.vault_name = id_parts[-3]
ns.resource_group_name = id_parts[-7]


def validate_principal(ns):
num_set = sum(1 for p in [ns.object_id, ns.spn, ns.upn] if p)
if num_set != 1:
Expand Down
38 changes: 36 additions & 2 deletions src/azure-cli/azure/cli/command_modules/keyvault/commands.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,11 @@


from ._client_factory import (
keyvault_client_vaults_factory, keyvault_data_plane_factory)
keyvault_client_vaults_factory, keyvault_client_private_endpoint_connections_factory,
keyvault_client_private_link_resources_factory, keyvault_data_plane_factory)

from ._validators import (
process_secret_set_namespace, process_certificate_cancel_namespace)
process_secret_set_namespace, process_certificate_cancel_namespace, validate_private_endpoint_connection_id)


# pylint: disable=too-many-locals, too-many-statements
Expand All @@ -34,6 +35,18 @@ def load_command_table(self, _):
resource_type=ResourceType.MGMT_KEYVAULT
)

kv_private_endpoint_connections_sdk = CliCommandType(
operations_tmpl='azure.mgmt.keyvault.operations#PrivateEndpointConnectionsOperations.{}',
client_factory=keyvault_client_private_endpoint_connections_factory,
resource_type=ResourceType.MGMT_KEYVAULT
)

kv_private_link_resources_sdk = CliCommandType(
operations_tmpl='azure.mgmt.keyvault.operations#PrivateLinkResourcesOperations.{}',
client_factory=keyvault_client_private_link_resources_factory,
resource_type=ResourceType.MGMT_KEYVAULT
)

kv_data_sdk = CliCommandType(
operations_tmpl='azure.keyvault.key_vault_client#KeyVaultClient.{}',
client_factory=keyvault_data_plane_factory,
Expand Down Expand Up @@ -64,6 +77,27 @@ def load_command_table(self, _):
g.custom_command('remove', 'remove_network_rule')
g.custom_command('list', 'list_network_rules')

with self.command_group('keyvault private-endpoint',
kv_private_endpoint_connections_sdk,
min_api='2018-02-14',
client_factory=keyvault_client_private_endpoint_connections_factory,
is_preview=True) as g:
g.custom_command('approve', 'approve_private_endpoint_connection',
validator=validate_private_endpoint_connection_id)
g.custom_command('reject', 'reject_private_endpoint_connection',
validator=validate_private_endpoint_connection_id)
Comment on lines +85 to +88
Copy link
Member

@jiasli jiasli Feb 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approve and reject are not common CLI commands. An alternative ways may be putting them in keyvault private-endpoint update --status approved/rejected. Has this been confirmed with PowerShell team @isra-fel or Key Vault PM?

It is not very conventional for PowerShell to design corresponding commands as Approve-AzKeyvaultPrivateEndpoint or Reject-AzKeyvaultPrivateEndpoint (in my mind).

Copy link
Contributor Author

@bim-msft bim-msft Feb 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jiasli Yes, this was confirmed with Key Vault team, will forward you the email later.

Copy link
Contributor

@mmyyrroonn mmyyrroonn Feb 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here. I think this design is good.

g.custom_command('delete', 'delete_private_endpoint_connection',
validator=validate_private_endpoint_connection_id)
g.custom_show_command('show', 'show_private_endpoint_connection',
validator=validate_private_endpoint_connection_id)

with self.command_group('keyvault private-link-resource',
kv_private_link_resources_sdk,
min_api='2018-02-14',
client_factory=keyvault_client_private_link_resources_factory,
is_preview=True) as g:
g.show_command('show', 'list_by_vault')
Copy link
Contributor

@mmyyrroonn mmyyrroonn Feb 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should it support id?


# Data Plane Commands
with self.command_group('keyvault key', kv_data_sdk) as g:
g.keyvault_command('list', 'get_keys')
Expand Down
72 changes: 70 additions & 2 deletions src/azure-cli/azure/cli/command_modules/keyvault/custom.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -526,7 +526,7 @@ def add_network_rule(cmd, client, resource_group_name, vault_name, ip_address=No


def remove_network_rule(cmd, client, resource_group_name, vault_name, ip_address=None, subnet=None, vnet_name=None): # pylint: disable=unused-argument
""" Removes a network rule from the network ACLs for a Key Vault. """
""" Remove a network rule from the network ACLs for a Key Vault. """

VaultCreateOrUpdateParameters = cmd.get_models('VaultCreateOrUpdateParameters',
resource_type=ResourceType.MGMT_KEYVAULT)
Expand Down Expand Up @@ -566,7 +566,7 @@ def remove_network_rule(cmd, client, resource_group_name, vault_name, ip_address


def list_network_rules(cmd, client, resource_group_name, vault_name): # pylint: disable=unused-argument
""" Lists the network rules from the network ACLs for a Key Vault. """
""" List the network rules from the network ACLs for a Key Vault. """
vault = client.get(resource_group_name=resource_group_name, vault_name=vault_name)
return vault.properties.network_acls

Expand Down Expand Up @@ -1154,3 +1154,71 @@ def restore_storage_account(client, vault_base_url, file_path):
data = file_in.read()
return client.restore_storage_account(vault_base_url, data)
# endregion


# region private_endpoint
def show_private_endpoint_connection(client, resource_group_name, vault_name, connection_name,
connection_id=None): # pylint: disable=unused-argument
"""Show details of a private endpoint connection associated with a Key Vault."""
return client.get(resource_group_name=resource_group_name, vault_name=vault_name,
private_endpoint_connection_name=connection_name)
Copy link
Member

@jiasli jiasli Feb 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is a 1:1 mapping, maybe we can eliminate the custom function?

Copy link
Contributor Author

@bim-msft bim-msft Feb 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jiasli The only thing I have to do here is to use name connection_name instead of private_endpoint_connection_name, because of this minor mismatch, I can not directly map it to SDK method, do you have any suggestion?

Copy link
Member

@jiasli jiasli Feb 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use extra to attach parameters.

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

Line 148 in cc53d1f

c.extra('identifier', options_list=['--id'], help='Id of the {}. If specified all other \'Id\' arguments should be omitted.'.format(item), validator=validate_vault_id(item))



def delete_private_endpoint_connection(client, resource_group_name, vault_name, connection_name,
connection_id=None): # pylint: disable=unused-argument
""" Delete the specified private endpoint connection associated with a Key Vault."""
return client.delete(resource_group_name=resource_group_name, vault_name=vault_name,
private_endpoint_connection_name=connection_name)


def approve_private_endpoint_connection(cmd, client, resource_group_name, vault_name, connection_name,
approval_description=None,
connection_id=None): # pylint: disable=unused-argument
"""Approve a private endpoint connection request for a Key Vault."""

PrivateEndpointConnection = cmd.get_models('PrivateEndpointConnection', resource_type=ResourceType.MGMT_KEYVAULT)
PrivateLinkServiceConnectionState = cmd.get_models('PrivateLinkServiceConnectionState',
resource_type=ResourceType.MGMT_KEYVAULT)

private_endpoint_connection = client.get(resource_group_name=resource_group_name, vault_name=vault_name,
private_endpoint_connection_name=connection_name)

return client.put(resource_group_name=resource_group_name,
vault_name=vault_name,
private_endpoint_connection_name=connection_name,
private_endpoint=private_endpoint_connection.private_endpoint,
Copy link
Member

@jiasli jiasli Feb 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private_endpoint has been moved down to properties due to the parameter count change. This is caused by autorest's payload-flattening-threshold=2 config which will flatten the parameters when the parameter count <= 2.

Anyway, let's remove it.

properties=PrivateEndpointConnection(
tags=private_endpoint_connection.tags,
location=private_endpoint_connection.location,
Copy link
Member

@jiasli jiasli Feb 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the code of PrivateEndpointConnection, it seems tags and location will be discarded. Also, can we just modify the returned value of client.get?

Copy link
Contributor Author

@bim-msft bim-msft Feb 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, makes sense, will refine this.

private_link_service_connection_state=PrivateLinkServiceConnectionState(
status='Approved',
Copy link
Member

@jiasli jiasli Feb 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not hard-code it but use the value in PrivateEndpointServiceConnectionStatus instead.

Copy link
Contributor Author

@bim-msft bim-msft Feb 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, will refine this.

description=approval_description
)
))


def reject_private_endpoint_connection(cmd, client, resource_group_name, vault_name, connection_name,
Copy link
Member

@jiasli jiasli Feb 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd better to extract the common code and call it with a switch.

Copy link
Contributor Author

@bim-msft bim-msft Feb 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea.

rejection_description=None,
connection_id=None): # pylint: disable=unused-argument
"""Reject a private endpoint connection request for a Key Vault."""

PrivateEndpointConnection = cmd.get_models('PrivateEndpointConnection', resource_type=ResourceType.MGMT_KEYVAULT)
PrivateLinkServiceConnectionState = cmd.get_models('PrivateLinkServiceConnectionState',
resource_type=ResourceType.MGMT_KEYVAULT)

private_endpoint_connection = client.get(resource_group_name=resource_group_name, vault_name=vault_name,
private_endpoint_connection_name=connection_name)

return client.put(resource_group_name=resource_group_name,
vault_name=vault_name,
private_endpoint_connection_name=connection_name,
private_endpoint=private_endpoint_connection.private_endpoint,
properties=PrivateEndpointConnection(
tags=private_endpoint_connection.tags,
location=private_endpoint_connection.location,
private_link_service_connection_state=PrivateLinkServiceConnectionState(
status='Rejected',
description=rejection_description
)
))
# endregion
Loading