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

[Network]Add Microsoft.Network/privateLinkServices configuration to support the private endpoint connection #18000

Merged
merged 19 commits into from
May 12, 2022

Conversation

LucasYao93
Copy link
Contributor

@LucasYao93 LucasYao93 commented Apr 28, 2022

  1. Add Add Microsoft.Network/privateLinkServices in ProviderConfiguration.
  2. Add hasSupportResourceURI parameter to check the resource whether support private link resource. True if support, otherwise false.

Checklist

  • I have read the Submitting Changes section of CONTRIBUTING.md
  • The title of the PR is clear and informative
  • The appropriate ChangeLog.md file(s) has been updated:
    • For any service, the ChangeLog.md file can be found at src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
    • A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header -- no new version header should be added
  • The PR does not introduce breaking changes
  • If applicable, the changes made in the PR have proper test coverage
  • For public API changes to cmdlets:
    • a cmdlet design review was approved for the changes in this repository (Microsoft internal only)
      • {Please put the link here}
    • the markdown help files have been regenerated using the commands listed here

@@ -1,63 +1,69 @@
## Applicability
Az.Network supports the retrieval of private link resource in `Get-AzPrivateLinkResource` as well as the management of private endpoint connection in `Approve-AzPrivateEndpointConnect`, `Deny-AzPrivateEndpointConnect`, `Remove-AzPrivateEndpointConnect` and `Set-AzPrivateEndpointConnect`.
Az.Network supports the retrieval of private link resource in `Get-AzPrivateLinkResource` as well as the management of private endpoint connection by `Get-AzPrivateEndpointConnect`, `Approve-AzPrivateEndpointConnect`, `Deny-AzPrivateEndpointConnect`, `Remove-AzPrivateEndpointConnect` and `Set-AzPrivateEndpointConnect`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Az.Network supports the retrieval of private link resource in `Get-AzPrivateLinkResource` as well as the management of private endpoint connection by `Get-AzPrivateEndpointConnect`, `Approve-AzPrivateEndpointConnect`, `Deny-AzPrivateEndpointConnect`, `Remove-AzPrivateEndpointConnect` and `Set-AzPrivateEndpointConnect`.
Az.Network supports the retrieval of private link resource in `Get-AzPrivateLinkResource` as well as the management of private endpoint connection by `Get-AzPrivateEndpointConnection`, `Approve-AzPrivateEndpointConnection`, `Deny-AzPrivateEndpointConnection`, `Remove-AzPrivateEndpointConnection` and `Set-AzPrivateEndpointConnection`.

For providers who
- supports the features of private linke resource and private endpoint connection already
- and want to onboard these features in Azure PowerShell,
This example is for provider who
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This example is for provider who
For providers who

The audience should be providers.

- supports the features of private linke resource and private endpoint connection already
- and want to onboard these features in Azure PowerShell,
This example is for provider who
- supports the features of private link resource and private endpoint connection already
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- supports the features of private link resource and private endpoint connection already
- supports the features of private link resource or private endpoint connection already

```
```
# Get Private Endpoint Connection API
"/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/{provider}/{Top-Level-Resource}/{Top-Level-Resource-Name}/privateEndpointConnections/{PrivateEndpointConnection-Name}"
"/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/{provider}/{Top-Resource}/{Top-Resource-Name}/privateEndpointConnections/{Resource-Name}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/{provider}/{Top-Resource}/{Top-Resource-Name}/privateEndpointConnections/{Resource-Name}"
"/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/{provider}/{Top-Resource}/{Top-Resource-Name}/privateEndpointConnections/{PrivateEndpointConnection-Name}"

The name should be the name of PrivateEndpointConnection

@BethanyZhou
Copy link
Contributor

Please contact me if you have any question about these comments.


if (!GenericProvider.SupportsPrivateLinkResourceType(this.PrivateLinkResourceType))
{
throw new ArgumentException(string.Format(Properties.Resources.UnsupportPrivateLinkResourceType, this.PrivateLinkResourceType));
Copy link
Contributor

Choose a reason for hiding this comment

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

AzPSArgumentException

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use AzPSArgumentException need pass parameter name. We have to hardcode parameter name if use it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used AzPSApplicationException

@LucasYao93 LucasYao93 requested a review from BethanyZhou May 10, 2022 03:16
Copy link
Contributor

@BethanyZhou BethanyZhou left a comment

Choose a reason for hiding this comment

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

Need change log for this feature.

@LucasYao93
Copy link
Contributor Author

Need change log for this feature.

added change log

Copy link
Contributor

@BethanyZhou BethanyZhou left a comment

Choose a reason for hiding this comment

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

LGTM

BethanyZhou
BethanyZhou previously approved these changes May 11, 2022
/// <param name="name">The name of the parameter</param>
/// <param name="runtimeParameter">The returned runtime parameter for context, with appropriate validate set</param>
/// <returns>True if one or more contexts were found, otherwise false</returns>
public static bool TryGetProvideServiceParameter(string name, string parameterSetName, out RuntimeDefinedParameter runtimeParameter)
public static bool TryGetProvideServiceParameter(string serviceType, string name, string parameterSetName, out RuntimeDefinedParameter runtimeParameter)
Copy link
Member

Choose a reason for hiding this comment

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

My understanding is It is not required. Let's discuss further.

/// <param name="name">The name of the parameter</param>
/// <param name="runtimeParameter">The returned runtime parameter for context, with appropriate validate set</param>
/// <returns>True if one or more contexts were found, otherwise false</returns>
public static bool TryGetProvideServiceParameter(string name, string parameterSetName, out RuntimeDefinedParameter runtimeParameter)
public static bool TryGetProvideServiceParameter(string serviceType, string name, string parameterSetName, out RuntimeDefinedParameter runtimeParameter)
Copy link
Member

Choose a reason for hiding this comment

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

If possible, the method should be moved to each cmdlet implementation. So, one should go to privateendpointconnnection, another goes to GetAzurePrivateLinkResource.

@@ -76,6 +77,8 @@ public new object GetDynamicParameters()

protected IPrivateLinkProvider BuildProvider(string subscription, string privateLinkResourceType)
{
if (!GenericProvider.SupportsPrivateLinkResourceType(privateLinkResourceType))
Copy link
Member

Choose a reason for hiding this comment

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

Please help to rename method to SupportsPrivateLinkFeature because it includes resource type and connection endpoint both.

@@ -735,4 +735,10 @@
<data name="VirtualNetworkGatewayNatRuleNotFound" xml:space="preserve">
<value>The VirtualNetworkGatewayNatRule could not be found</value>
</data>
<data name="UnsupportPrivateEndpointConnectionType" xml:space="preserve">
<value>The {0} doesn't register private endpoint connection.</value>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<value>The {0} doesn't register private endpoint connection.</value>
<value>Resource type {0} doesn't support private endpoint connection.</value>

/// Check if the resource id format is valid.
/// </summary>
/// <exception cref="AzPSApplicationException">unvaild throw exception</exception>
public void CheckResourceId ()
Copy link
Member

Choose a reason for hiding this comment

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

Utility method should be defined as protected static method and accept resourceId as input parameter.

@dingmeng-xue dingmeng-xue merged commit 2185ac9 into main May 12, 2022
@LucasYao93 LucasYao93 deleted the issue/16984 branch May 13, 2022 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment