Skip to content
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

{Documentation}Command guideline for private endpoint connection and private link resource #12403

Merged
merged 9 commits into from
Mar 4, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
100 changes: 100 additions & 0 deletions doc/private_endpoint_connection_command_guideline.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
## Command Guideline

#### Private Endpoint Connection

- The parent resource should expose a single command group called `private-endpoint-connection` with four commands: `approve`, `reject`, `delete`, `show`.
- If `approve`, `reject` and `delete` commands are long running operations, please also provide `... private-endpoint-connection wait` command and support `--no-wait` in `approve` and `reject` commands.
- The `... private-endpoint-connection approve` command should look similar to the following, depending on which features are supported by the service.
```
Arguments
--description : Comments for the approval.
--id : The ID of the private endpoint connection associated with the Key
Vault(Storage Account). If specified --vault-name and --name/-n, this should be omitted.
--name -n : The name of the private endpoint connection associated with the Key
Vault(Storage Account). Required if --id is not specified.
--resource-group -g : Proceed only if Key Vault(Storage Account) belongs to the specified resource group.
--vault-name(--account-name) : Name of the Key Vault(Storage Account). Required if --id is not specified.
Copy link
Member

Choose a reason for hiding this comment

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

add --wait --nowait as optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It depends on service. If it's not a long running operation such as storage, we cannot add --no-wait argument and wait command. But for a long running operation, it should be required.

```
- The `... private-endpoint-connection reject` command should look similar to the following, depending on which features are supported by the service.
```
Arguments
--description : Comments for the rejection.
--id : The ID of the private endpoint connection associated with the Key
Vault(Storage Account). If specified --vault-name and --name/-n, this should be omitted.
--name -n : The name of the private endpoint connection associated with the Key
Vault(Storage Account). Required if --id is not specified.
--resource-group -g : Proceed only if Key Vault(Storage Account) belongs to the specified resource group.
--vault-name(--account-name) : Name of the Key Vault(Storage Account). Required if --id is not specified.
```
- The `... private-endpoint-connection show/delete` command should look similar to the following, depending on which features are supported by the service.
```
Arguments
--id : The ID of the private endpoint connection associated with the Key
Vault(Storage Account). If specified --vault-name and --name/-n, this should be omitted.
--name -n : The name of the private endpoint connection associated with the Key
Vault(Storage Account). Required if --id is not specified.
--resource-group -g : Proceed only if Key Vault(Storage Account) belongs to the specified resource group.
--vault-name(--account-name) : Name of the Key Vault(Storage Account). Required if --id is not specified.
```

#### Private Link Resource

- The parent resource should expose a single command group called `private-link-resource` with one commands: `list`.
- The `... private-link-resource list` command should look similar to the following, depending on which features are supported by the service.

```
Arguments
--vault-name(--account-name) [Required] : Name of the Key Vault(Storage Account).
--resource-group -g : Proceed only if Key Vault belongs to the specified resource group.
bim-msft marked this conversation as resolved.
Show resolved Hide resolved
```
- The output of `... private-link-resource list` should be a array instead of a dictionary.

## Command Authoring

Storage and keyvault modules both are good examples. Feel free to use them as reference.
mmyyrroonn marked this conversation as resolved.
Show resolved Hide resolved

*Storage*: [PR Link](https://github.com/Azure/azure-cli/pull/12383)

*Keyvault*: [Command Module](https://github.com/Azure/azure-cli/tree/dev/src/azure-cli/azure/cli/command_modules/keyvault)

#### Parameters
We provide a build-in function `parse_proxy_resource_id` to parse private endpoint connection id. It can be used to support the `--id` argument.
```
from azure.cli.core.util import parse_proxy_resource_id
pe_resource_id = "/subscriptions/0000/resourceGroups/clirg/" \
"providers/Microsoft.Network/privateEndpoints/clipe/" \
"privateLinkServiceConnections/peconnection"
result = parse_proxy_resource_id(pe_resource_id)
namespace.resource_group = result['resource_group']
namespace.endpoint = result['name']
namespace.name = result['child_name_1']
mmyyrroonn marked this conversation as resolved.
Show resolved Hide resolved
```
The best practice to support extra `--id` is to add extra argument in `_param.py`. Then you can use the `parse_proxy_resource_id` to parse the `--id` and delete this extra argument from the namspace. Storage's PR is a good example.
```
with self.argument_context('storage account private-endpoint-connection {}'.format(item), resource_type=ResourceType.MGMT_STORAGE) as c:
c.extra('connection_id', options_list=['--id'], help='help='The ID of the private endpoint connection associated with the Storage Account.')
```
```
from azure.cli.core.util import parse_proxy_resource_id
result = parse_proxy_resource_id(namespace.connection_id)
namespace.resource_group = result['resource_group']
namespace.endpoint = result['name']
namespace.name = result['child_name_1']
del namespace.connection_id
```
#### Transform
In order to transform the output of the `list` command, we provide a transform function `gen_dict_to_list_transform`.
```
Copy link
Contributor

Choose a reason for hiding this comment

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

where reference this gen_dict_to_list_transform?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

g.command('list', transform=gen_dict_to_list_transform(key='values'))

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you confirm all the services have the same output structure with values?

from azure.cli.core.command.transform import gen_dict_to_list_transform
Copy link
Contributor

Choose a reason for hiding this comment

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

it should be "commands" not command

g.command('list', transform=gen_dict_to_list_transform(key='values'))
Copy link
Contributor

Choose a reason for hiding this comment

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

It is "value" in storage part.

```

#### Test
- Integration test is mandatory. It should contain the following steps at least.
- Create a resource such as storage account or key vault.
- List all private link resources for the created resource.
- Create a private endpoint for the resource.
- Approve the private endpoint connection.
- Reject the private endpoint connection.
- Show the private endpoint connection.
- Delete the private endpoint connection.
10 changes: 10 additions & 0 deletions src/azure-cli-core/azure/cli/core/commands/transform.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,3 +63,13 @@ def _resource_group_transform(_, **kwargs):

def _x509_from_base64_to_hex_transform(_, **kwargs):
_add_x509_hex(kwargs['event_data']['result'])


def gen_dict_to_list_transform(key='value'):
Copy link
Contributor Author

Choose a reason for hiding this comment

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


def _dict_to_list_transform(result):
if hasattr(result, key):
return getattr(result, key)
return result

return _dict_to_list_transform
27 changes: 26 additions & 1 deletion src/azure-cli-core/azure/cli/core/tests/test_util.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
from azure.cli.core.util import \
(get_file_json, truncate_text, shell_safe_json_parse, b64_to_hex, hash_string, random_string,
open_page_in_browser, can_launch_browser, handle_exception, ConfiguredDefaultSetter, send_raw_request,
should_disable_connection_verify)
should_disable_connection_verify, parse_proxy_resource_id)


class TestUtils(unittest.TestCase):
Expand Down Expand Up @@ -121,6 +121,31 @@ def _run_test(length, force_lower):
# Test force_lower
_run_test(16, True)

def test_proxy_resource_parse(self):
mock_proxy_resource_id = "/subscriptions/0000/resourceGroups/clirg/" \
"providers/Microsoft.Network/privateEndpoints/cli/" \
"privateLinkServiceConnections/cliPec/privateLinkServiceConnectionsSubTypes/cliPecSubName"
result = parse_proxy_resource_id(mock_proxy_resource_id)
Copy link
Contributor Author

@mmyyrroonn mmyyrroonn Mar 3, 2020

Choose a reason for hiding this comment

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

@qianwens Hi. This is the usage scenario. It's impossible to have same key. This is the similar pattern with parse_resource_id.

Copy link
Member

@qianwens qianwens Mar 3, 2020

Choose a reason for hiding this comment

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

for client who use this function, how do they know they need to get value of "chile_type" by key "child_type_1", I get your points now, how about change to valid_dict_values = {
'subscription': '0000',
'resource_group': 'clirg',
'namespace': 'Microsoft.Network',
'type': 'privateEndpoints',
'name': 'cli',
'subValues':[
{
'child_type_1': 'privateLinkServiceConnections',
'child_name_1': 'child_name_1',
} ,{'child_type_2': 'child_type_2',
'child_name_2': 'child_name_2'}]
}
The object model for the child values should be a sequence list


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

Copy link
Contributor Author

@mmyyrroonn mmyyrroonn Mar 3, 2020

Choose a reason for hiding this comment

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

Hi. Sounds good to have a sub sequence list. However, as I said, I follow the structure of msrestazure's parse_resource_id, which was developed two years ago and commonly used in azure-cli. I think it's more intuitive to keep them consistent and easier for users.
https://github.com/Azure/msrestazure-for-python/blob/0a07586816351e3b7abd60a55a8b736e007fb7a8/msrestazure/tools.py#L106

Copy link
Member

Choose a reason for hiding this comment

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

The original design of parse_resource_id seems not intuitive and not common design for a method, we do not need to follow that method unless they are in the same module. There is no need to convert a sequence list to a dictionary with the naming conversion.


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

valid_dict_values = {
'subscription': '0000',
'resource_group': 'clirg',
'namespace': 'Microsoft.Network',
'type': 'privateEndpoints',
'name': 'cli',
'child_type_1': 'privateLinkServiceConnections',
'child_name_1': 'cliPec',
'child_type_2': 'privateLinkServiceConnectionsSubTypes',
'child_name_2': 'cliPecSubName',
'last_child_num': 2
}
self.assertEqual(len(result.keys()), len(valid_dict_values.keys()))
for key, value in valid_dict_values.items():
self.assertEqual(result[key], value)

invalid_proxy_resource_id = "invalidProxyResourceID"
result = parse_proxy_resource_id(invalid_proxy_resource_id)
self.assertIsNone(result)

@mock.patch('webbrowser.open', autospec=True)
@mock.patch('subprocess.Popen', autospec=True)
def test_open_page_in_browser(self, sunprocess_open_mock, webbrowser_open_mock):
Expand Down
43 changes: 43 additions & 0 deletions src/azure-cli-core/azure/cli/core/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import platform
import ssl
import six
import re

from six.moves.urllib.request import urlopen # pylint: disable=import-error
from knack.log import get_logger
Expand All @@ -29,6 +30,12 @@
'Please add this certificate to the trusted CA bundle: https://github.com/Azure/azure-cli/blob/dev/doc/use_cli_effectively.md#working-behind-a-proxy. '
'Error detail: {}')

_PROXYID_RE = re.compile(
mmyyrroonn marked this conversation as resolved.
Show resolved Hide resolved
'(?i)/subscriptions/(?P<subscription>[^/]*)(/resourceGroups/(?P<resource_group>[^/]*))?'
'(/providers/(?P<namespace>[^/]*)/(?P<type>[^/]*)/(?P<name>[^/]*)(?P<children>.*))?')

_CHILDREN_RE = re.compile('(?i)/(?P<child_type>[^/]*)/(?P<child_name>[^/]*)')


def handle_exception(ex): # pylint: disable=too-many-return-statements
# For error code, follow guidelines at https://docs.python.org/2/library/sys.html#sys.exit,
Expand Down Expand Up @@ -654,3 +661,39 @@ def _ssl_context():
def urlretrieve(url):
req = urlopen(url, context=_ssl_context())
return req.read()


def parse_proxy_resource_id(rid):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"""Parses a resource_id into its various parts.

Return an empty dictionary, if invalid resource id.

:param rid: The resource id being parsed
:type rid: str
:returns: A dictionary with with following key/value pairs (if found):

- subscription: Subscription id
- resource_group: Name of resource group
- namespace: Namespace for the resource provider (i.e. Microsoft.Compute)
- type: Type of the root resource (i.e. virtualMachines)
- name: Name of the root resource
- child_type_{level}: Type of the child resource of that level
- child_name_{level}: Name of the child resource of that level
- last_child_num: Level of the last child

:rtype: dict[str,str]
"""
if not rid:
return {}
match = _PROXYID_RE.match(rid)
if match:
result = match.groupdict()
children = _CHILDREN_RE.finditer(result['children'] or '')
count = None
for count, child in enumerate(children):
result.update({
key + '_%d' % (count + 1): group for key, group in child.groupdict().items()})
Copy link
Member

@qianwens qianwens Mar 2, 2020

Choose a reason for hiding this comment

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

why do you convert the collection to a dictionary by add count _1 in the key, is it possible that there are duplicated key?s How should client use this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The regex pattern is (?P<child_type>[^/]*)/(?P<child_name>[^/]*)'). The original key and group would be child_type and child_name. In order to make this function more general, we add count number into the result.

result['last_child_num'] = count + 1 if isinstance(count, int) else None
result.pop('children', None)
return {key: value for key, value in result.items() if value is not None}
return None
Original file line number Diff line number Diff line change
Expand Up @@ -755,7 +755,7 @@ def load_arguments(self, _):
c.argument('subnet', validator=get_subnet_validator(), help='Name or ID of an existing subnet. If name is specified, also specify --vnet-name.', id_part=None)
c.argument('virtual_network_name', help='The virtual network (VNet) associated with the subnet (Omit if supplying a subnet id).', metavar='', id_part=None)
c.argument('private_connection_resource_id', help='The resource id of which private enpoint connect to')
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.')
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-link-resource list" to obtain the list of group ids.')
c.argument('request_message', help='A message passed to the owner of the remote resource with this connection request. Restricted to 140 chars.')
c.argument('manual_request', help='Use manual request to establish the connection', arg_type=get_three_state_flag())
c.argument('connection_name', help='Name of the private link service connection.')
Expand Down