[KeyVault] Support private endpoint connections#12073
Conversation
bim-msft
force-pushed
the
bim_kv_private_ep
branch
from
f6023bf to
f7ba944
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
bim-msft
force-pushed
the
bim_kv_private_ep
branch
from
f7ba944 to
0ba10ec
Compare
| if not connection_id: | ||
| if not all([connection_name, vault_name]): | ||
| raise argparse.ArgumentError( | ||
| None, 'specify both: --connection-name and --vault-name') |
There was a problem hiding this comment.
In az keyvault private-endpoint show -h, it should be --name instead of --vault-name.
| None, 'specify both: --connection-name and --vault-name') | |
| None, 'specify both: --connection-name and --name') |
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
Makes sense. In that case, it should be --vault-name here:
| helps['keyvault private-link-resource show'] = """ | ||
| type: command | ||
| short-summary: Show the private link resources supported for a Key Vault. |
There was a problem hiding this comment.
The az network private-endpoint create -h's help message doesn't conform to this command name. @myronfanqiu
src/azure-cli/azure/cli/command_modules/keyvault/tests/latest/test_keyvault_commands.py
Outdated
Show resolved
Hide resolved
…test_keyvault_commands.py Co-Authored-By: Jiashuo Li <jiasli@microsoft.com>
| retries = 0 | ||
| while self.cmd('keyvault private-endpoint show --connection-id {kv_pe_id}').\ | ||
| get_output_in_json()['provisioningState'] != 'Succeeded' or retries > max_retries: | ||
| time.sleep(5) |
There was a problem hiding this comment.
Better to sleep only during live run:
| self.cmd('keyvault private-endpoint approve -n {kv} --connection-name {kv_pe_name} ' | ||
| '--approval-description "{approval_desc}"', checks=[ | ||
| self.check('privateLinkServiceConnectionState.status', 'Approved'), | ||
| self.check('privateLinkServiceConnectionState.description', '{approval_desc}') | ||
| ]) |
There was a problem hiding this comment.
Let's check provisioningState in the return value as well.
There was a problem hiding this comment.
Sure, will add this.
| 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) |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
@jiasli Yes, this was confirmed with Key Vault team, will forward you the email later.
There was a problem hiding this comment.
here. I think this design is good.
| 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) |
There was a problem hiding this comment.
If this is a 1:1 mapping, maybe we can eliminate the custom function?
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
Let's use extra to attach parameters.
| tags=private_endpoint_connection.tags, | ||
| location=private_endpoint_connection.location, | ||
| private_link_service_connection_state=PrivateLinkServiceConnectionState( | ||
| status='Approved', |
There was a problem hiding this comment.
Let's not hard-code it but use the value in PrivateEndpointServiceConnectionStatus instead.
There was a problem hiding this comment.
Good idea, will refine this.
| tags=private_endpoint_connection.tags, | ||
| location=private_endpoint_connection.location, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yes, makes sense, will refine this.
| )) | ||
|
|
||
|
|
||
| def reject_private_endpoint_connection(cmd, client, resource_group_name, vault_name, connection_name, |
There was a problem hiding this comment.
We'd better to extract the common code and call it with a switch.
| 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, |
There was a problem hiding this comment.
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.
| helps['keyvault private-endpoint'] = """ | ||
| type: group | ||
| short-summary: Manage vault private endpoint connections. | ||
| """ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
|
|
||
| if not connection_id: | ||
| if not all([connection_name, vault_name]): | ||
| raise argparse.ArgumentError( |
There was a problem hiding this comment.
Based on my knowledge, usually we would raise CLIError('usage error:'). We can discuss.
| 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) |
There was a problem hiding this comment.
here. I think this design is good.
| 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') |
| self.check('provisioningState', 'Updating') | ||
| ]) | ||
|
|
||
| max_retries = 20 |
There was a problem hiding this comment.
I think this is because of the errors of the swagger spec. Could we add some comments here?
3 participants
Support for #11038
Bump the Key Vault mgmt-plane SDK version to 2.1.1 (This is the fixed version, 2.1.0 is buggy)
Add two command groups, all of them are marked as
preview:az keyvault private-endpoint: manage vault private endpoint connections.az keyvault private-link-resource: manage vault private link resources.Whole scenario and test instructions:
Key Vaultand aVirtual Networkwithin the same region (preferred region:Central US EUAP).network private-endpoint create -g {rg} -n {pe} --vnet-name {vnet} --subnet {subnet} -l {loc} --connection-name {pe_connection} --private-connection-resource-id {kv_id} --group-ids vault(Thegroup-idscan be obtained from commandaz keyvault private-link-resource show, for Key Vault service, the group id isvault, I remember that @myronfanqiu planned to implement a new command innetworkgroup, which can be used to obtain the group id of all service types).keyvault private-endpoint show --connection-id {kv_pe_id}.keyvault private-endpoint approve -n {kv} --connection-name {kv_pe_name} --approval-description "{approval_desc}",keyvault private-endpoint reject --connection-id {kv_pe_id} --rejection-description "{rejection_desc}"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.