Multiple Radius Servers for VPN scenario #11550

henry416 · 2020-04-10T22:41:34Z

Description

Add support for multiple radius servers in VPN scenarios (VPN Gateway and vWAN VpnServerConfiguration).

This cmdlet change requires 2020-03-01 network package.

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)
- the markdown help files have been regenerated using the commands listed here

henry416 · 2020-04-10T22:42:32Z

https://github.com/Azure/azure-powershell-cmdlet-review-pr/issues/553

msJinLei · 2020-04-13T03:03:34Z

@henry416 Please fix the failed tests

anton-evseev · 2020-04-13T16:25:05Z

Hi @henry416, changes from network-march were merged into master. Please retarget your PR to master as well to make the release. Please also note that master was rebased due to credentials leaking into it so it's basically incompatible with network-march. My suggestion is to create a new branch from master and copy your changes there.

msJinLei · 2020-04-14T01:35:59Z

cmdlet review https://github.com/Azure/azure-powershell-cmdlet-review-pr/issues/553

henry416 · 2020-04-23T20:56:33Z

@msJinLei Can you please review the changes. Specifically the behavior with parametersets (cannot have RadiusServerConfiguration and MultipleRadiusServersConfiguration coexist)

msJinLei

@henry416, Please check "VirtualNetworkGatewayTests.ps1" for the credential scan error.

msJinLei · 2020-04-28T04:45:51Z

/azp run

azure-pipelines · 2020-04-28T04:46:07Z

Azure Pipelines successfully started running 3 pipeline(s).

…into radius

henry416 · 2020-05-04T17:29:50Z

@msJinLei Please review

msJinLei · 2020-05-05T09:14:29Z

@msJinLei Please review

@msJinLei Seems like someone else's change is causing signature issues. I am suppressing this cmdlet that is unrelated to mine and it doesn't seem to be an actual issue.

"/Users/runner/runners/2.166.4/work/1/s/artifacts/Debug/Az.Network/Microsoft.Azure.PowerShell.Cmdlets.Network.dll","Microsoft.Azure.Commands.Network.RemoveAzureSecurityPartnerProviderCommand","Remove-AzSecurityPartnerProvider","1","8510","Cmdlet 'Remove-AzSecurityPartnerProvider' has multiple parameter sets, but no defined default parameter set.","Define a default parameter set in the cmdlet attribute."

Yes, we just find signature check doesn't work for a long time. We just fix it recently.
For the issue you mentioned, can you provide a default parameter set for this cmdlet?

henry416 · 2020-05-06T02:40:02Z

@msJinLei Fixed

msJinLei · 2020-05-06T05:53:27Z

tools/StaticAnalysis/Exceptions/Az.Network/SignatureIssues.csv

Please do not suppress the error. You can set a default parameter for "GetAzureSecurityPartnerProviderCommand".

@msJinLei I'm not suppressing the error for GetAzureSecurityPartnerProviderCommand ? I don't alter that file at all in this PR?

@msJinLei If you are referring to the ShouldProcess line for New-AzRadiusServer, it is because RadiusServer is a sub-object of gateway's VPN server configuration, simply describing a configuration (ip address, score, etc.). There is no need to implement shouldProcess for this subobject especially since running this cmdlet doesn't call Azure.

@msJinLei I'm not suppressing the error for GetAzureSecurityPartnerProviderCommand ? I don't alter that file at all in this PR?

Default parameter set helps user to choose a parameter set when there is a conflict to bind inputs to existing parameter sets. And so this signature error cannot be suppressed.

Even if the issue is not caused by your PR, it does block your PR from merging. That's why I recommend you fix it in your PR.

@msJinLei Ok, but I don't see a "GetAzureSecurityPartnerProviderCommand" error being thrown anywhere? The checks both in this PR and in my local static analysis runs don't show GetAzureSecurityPartnerProviderCommand throwing a static analysis error. The checks are passing all green.

@msJinLei Ok, but I don't see a "GetAzureSecurityPartnerProviderCommand" error being thrown anywhere? The checks both in this PR and in my local static analysis runs don't show GetAzureSecurityPartnerProviderCommand throwing a static analysis error. The checks are passing all green.

You should remove this line.

"Microsoft.Azure.PowerShell.Cmdlets.Network.dll","Microsoft.Azure.Commands.Network.RemoveAzureSecurityPartnerProviderCommand","Remove-AzSecurityPartnerProvider","1","8510","Cmdlet 'Remove-AzSecurityPartnerProvider' has multiple parameter sets, but no defined default parameter set.","Define a default parameter set in the cmdlet attribute."

@msJinLei Removing that existing static analysis line will cause checks to fail. RemoveAzureSecurityPartnerProviderCommand is owned by a separate team and I do not know the context of which parameter set to make as default for their powershell. Please ask them to change it.

@msJinLei Removing that existing static analysis line will cause checks to fail. RemoveAzureSecurityPartnerProviderCommand is owned by a separate team and I do not know the context of which parameter set to make as default for their powershell. Please ask them to change it.
@henry416 We have fixed the issue in the master, please rebase the latest one.

@msJinLei Done.

msJinLei · 2020-05-06T08:22:07Z

tools/StaticAnalysis/Exceptions/Az.Network/SignatureIssues.csv

@msJinLei Ok, but I don't see a "GetAzureSecurityPartnerProviderCommand" error being thrown anywhere? The checks both in this PR and in my local static analysis runs don't show GetAzureSecurityPartnerProviderCommand throwing a static analysis error. The checks are passing all green.

You should remove this line.

"Microsoft.Azure.PowerShell.Cmdlets.Network.dll","Microsoft.Azure.Commands.Network.RemoveAzureSecurityPartnerProviderCommand","Remove-AzSecurityPartnerProvider","1","8510","Cmdlet 'Remove-AzSecurityPartnerProvider' has multiple parameter sets, but no defined default parameter set.","Define a default parameter set in the cmdlet attribute."

…comments

…s per PRcomments" This reverts commit b09407e.

henry416 · 2020-05-06T09:22:41Z

@msJinLei Removing that existing static analysis line will cause checks to fail. RemoveAzureSecurityPartnerProviderCommand is owned by a separate team and I do not know the context of which parameter set to make as default for their powershell. Please ask them to change it. (Git history shows Ishani Gupta as last author)

msJinLei · 2020-05-07T07:42:11Z

tools/StaticAnalysis/Exceptions/Az.Network/SignatureIssues.csv

@msJinLei Removing that existing static analysis line will cause checks to fail. RemoveAzureSecurityPartnerProviderCommand is owned by a separate team and I do not know the context of which parameter set to make as default for their powershell. Please ask them to change it.
@henry416 We have fixed the issue in the master, please rebase the latest one.

msJinLei assigned msJinLei and henry416 Apr 13, 2020

msJinLei added the needs-revision label Apr 13, 2020

henry416 force-pushed the radius branch from 96128b5 to fbdb706 Compare April 13, 2020 17:05

henry416 changed the base branch from network-march to master April 13, 2020 17:05

msJinLei added the Cmdlet Review Required ⚠️ label Apr 14, 2020

henry416 added 5 commits April 20, 2020 11:28

multiple radius initial commit

291ef21

new cmdlet

3a3b8d5

cortex scenario

60a3689

updated tests

689cc88

rebased and PR comments

51985c6

henry416 force-pushed the radius branch from 7d53009 to 51985c6 Compare April 20, 2020 20:44

henry416 added 3 commits April 22, 2020 10:47

fixed test issue

5ee0e3f

reworked parametersets

2178f14

revert Set test

409fb72

msJinLei removed the Cmdlet Review Required ⚠️ label Apr 26, 2020

msJinLei suggested changes Apr 27, 2020

View reviewed changes

supress a junk secret

b66c9c1

henry416 added 5 commits April 28, 2020 14:36

multiple radius initial commit

ab7a846

new cmdlet

7c571f0

cortex scenario

5710d7a

updated tests

2aa5d9c

rebased and PR comments

737715e

henry416 added 4 commits April 29, 2020 13:53

suppress other cmdlet signature issue

52633f8

Merge branch 'master' into radius

767508a

fix

4a36dd0

Merge branch 'radius' of https://github.com/henry416/azure-powershell …

1d49b58

…into radius

henry416 added 2 commits May 5, 2020 14:20

reset static analysis and fixed new cmdlet for default parameter

1c36974

merged signatureissue fix

0bc94a6

fixed typo

0fb7c2a

isra-fel added needs-review and removed needs-revision labels May 6, 2020

msJinLei suggested changes May 6, 2020

View reviewed changes

msJinLei added needs-revision and removed needs-review labels May 6, 2020

henry416 requested a review from msJinLei May 6, 2020 07:03

henry416 added needs-review and removed needs-revision labels May 6, 2020

msJinLei suggested changes May 6, 2020

View reviewed changes

removed RemoveAzureSecurityPartnerProviderCommand exception as per PR…

b09407e

…comments

henry416 requested a review from msJinLei May 6, 2020 08:33

Revert "removed RemoveAzureSecurityPartnerProviderCommand exception a…

e4311c3

…s per PRcomments" This reverts commit b09407e.

msJinLei added needs-revision and removed needs-review labels May 7, 2020

msJinLei suggested changes May 7, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into radius

2adb21b

henry416 requested a review from msJinLei May 7, 2020 09:22

msJinLei approved these changes May 7, 2020

View reviewed changes

msJinLei merged commit 88c5248 into Azure:master May 7, 2020

Multiple Radius Servers for VPN scenario #11550

Multiple Radius Servers for VPN scenario #11550

Uh oh!

Conversation

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

Uh oh!

Description

Checklist

Uh oh!

henry416 commented Apr 10, 2020

Uh oh!

msJinLei commented Apr 13, 2020

Uh oh!

anton-evseev commented Apr 13, 2020

Uh oh!

msJinLei commented Apr 14, 2020

Uh oh!

henry416 commented Apr 23, 2020

Uh oh!

msJinLei left a comment

Choose a reason for hiding this comment

Uh oh!

msJinLei commented Apr 28, 2020

Uh oh!

azure-pipelines bot commented Apr 28, 2020

Uh oh!

henry416 commented May 4, 2020

Uh oh!

msJinLei commented May 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henry416 commented May 6, 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!

msJinLei May 6, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

henry416 commented May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

henry416 commented Apr 10, 2020 •

edited

Loading

msJinLei commented May 5, 2020 •

edited

Loading

msJinLei May 6, 2020 •

edited

Loading

henry416 commented May 6, 2020 •

edited

Loading