Ipsec Policy for Virtual Network Gateway Connection by henry416 · Pull Request #3666 · Azure/azure-powershell

henry416 · 2017-03-22T21:34:57Z

Description

THIS CHANGE MUST GO IN EARLY APRIL RELEASE

IPsec Policy support for Virtual Network Gateway Connection

updated Network nuget package (2017-03-01 api version)
updated existing Virtual Network Gateway Connection model + cmdlets
added IPsec Policy model + cmdlet
added connection test for IPsec Policy

Reference PR: Azure/azure-sdk-for-net#2943

This checklist is used to make sure that common guidelines for a pull request are followed. You can find a more complete discussion of PowerShell cmdlet best practices here.

I have read the contribution guidelines.
If changes were made to any cmdlet, the XML help was regenerated using the platyPSHelp module.
If any large changes are made to a service, they are reflected in the respective change log.

General Guidelines

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.
The pull request does not introduce breaking changes (unless a major version change occurs in the assembly and module).

Testing Guidelines

Pull request includes test coverage for the included changes.
PowerShell scripts used in tests should do any necessary setup as part of the test or suite setup, and should not use hard-coded values for locations or existing resources.

Cmdlet Signature Guidelines

New cmdlets that make changes or have side effects should implement ShouldProcess and have SupportShouldProcess=true specified in the cmdlet attribute. You can find more information on ShouldProcess here.
Cmdlet specifies OutputType attribute if any output is produced - if the cmdlet produces no output, it should implement a PassThru parameter.

Cmdlet Parameter Guidelines

Parameter types should not expose types from the management library - complex parameter types should be defined in the module.
Complex parameter types are discouraged - a parameter type should be simple types as often as possible. If complex types are used, they should be shallow and easily creatable from a constructor or another cmdlet.
Cmdlet parameter sets should be mutually exclusive - each parameter set must have at least one mandatory parameter not in other parameter sets.

…l + test case

… added

msftclas · 2017-03-22T21:35:02Z

@henry416,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

azuresdkci · 2017-03-22T21:35:13Z

Can one of the admins verify this patch?

henry416 · 2017-03-22T21:37:32Z

This PR is pending a merge from azure-sdk-for-net, will update nuget packages properly when this occurs.
Please let me know if there are any other missing items from the PR while we wait.

henry416 · 2017-03-23T16:29:54Z

@cormacpayne Are there any steps / files I am missing in this PR? (other than the help files)

cormacpayne · 2017-03-23T16:37:58Z

@azuresdkci add to whitelist

henry416 · 2017-03-23T20:35:51Z

@cormacpayne can we rerun the check? It seems to be stuck.

cormacpayne · 2017-03-23T20:43:48Z

@azuresdkci test this please

henry416 · 2017-03-23T21:35:29Z

@cormacpayne I think the checks are failing b/c we haven't built and published a nuget package for the azure-sdk network branch. In the meantime is there any other component of this PR that is missing?

cormacpayne

@henry416 Hey Henry, I left a couple of comments (most are about the version bump) that need to be resolved

cormacpayne · 2017-03-23T22:13:42Z

-    <Reference Include="Microsoft.Azure.Management.Network, Version=9.1.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
-      <HintPath>..\..\..\packages\Microsoft.Azure.Management.Network.9.1.0-preview\lib\net45\Microsoft.Azure.Management.Network.dll</HintPath>
+    <Reference Include="Microsoft.Azure.Management.Network, Version=9.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
+      <HintPath>..\..\..\packages\Microsoft.Azure.Management.Network.9.2.0-preview\lib\net45\Microsoft.Azure.Management.Network.dll</HintPath>


@henry416 this will need to be bumped up to 10.0.0-preview

@cormacpayne Shouldn't the version be 9.2.0? Anyways, the azure-sdk branch hasn't been merged yet so the nuget package to be published isn't available yet - think of the references as a placeholder for now.

@henry416 it will be 10.0.0-preview after the changes that @jobatzil made, but yes, this is a temporary placeholder until the SDK gets published 😄

cormacpayne · 2017-03-23T22:13:49Z

  <package id="Microsoft.Azure.Management.Authorization" version="1.0.0" targetFramework="net45" />
  <package id="Microsoft.Azure.Management.Compute" version="14.0.0-prerelease" targetFramework="net45" />
-  <package id="Microsoft.Azure.Management.Network" version="9.1.0-preview" targetFramework="net45" />
+  <package id="Microsoft.Azure.Management.Network" version="9.2.0-preview" targetFramework="net45" />


@henry416 same comment

cormacpayne · 2017-03-23T22:13:57Z

-    <Reference Include="Microsoft.Azure.Management.Network, Version=9.1.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
-      <HintPath>..\..\..\packages\Microsoft.Azure.Management.Network.9.1.0-preview\lib\net45\Microsoft.Azure.Management.Network.dll</HintPath>
+    <Reference Include="Microsoft.Azure.Management.Network, Version=9.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
+      <HintPath>..\..\..\packages\Microsoft.Azure.Management.Network.9.2.0-preview\lib\net45\Microsoft.Azure.Management.Network.dll</HintPath>


@henry416 same comment

cormacpayne · 2017-03-23T22:14:05Z

    <None Include="..\..\Common\Commands.ScenarioTests.ResourceManager.Common\AzureRM.Resources.ps1">
      <Link>ScenarioTests\AzureRM.Resources.ps1</Link>
    </None>
+    <None Include="app.config" />


@henry416 please remove this

cormacpayne · 2017-03-23T22:14:37Z

        }

-        [Fact]
+        [Fact(Skip = "Rerecord tests")]


@henry416 are these tests going to be re-recorded before this PR gets merged?

Is that the standard procedure? I can change both tests back to [Fact]

@henry416 is there any reason you are skipping these tests?

I changed them back to [Fact]

cormacpayne · 2017-03-23T22:18:59Z

+{
+    public class PSIpsecPolicy
+    {
+        public int SALifeTimeSeconds { get; set; }


@henry416 can you add a quick summary for each the properties?

/// <summary> /// Brief description of property. /// <summary> public int SALifeTimeSeconds { get; set; }

cormacpayne · 2017-03-23T22:22:02Z

+            ipsecPolicy.DhGroup = this.DhGroup;
+            ipsecPolicy.PfsGroup = this.PfsGroup;
+
+            WriteObject(ipsecPolicy);


@henry416 as an FYI, StaticAnalysis is going to throw an exception because this cmdlet uses the New verb, but doesn't implement ShouldProcess . Since this cmdlet is creating a local object, you do not need to implement this attribute, so you can suppress the exception. I will show you how to do so when the time comes 😄

@cormacpayne Ah ok, that was an error that was confusing me. What would be the procedure to suppress that?

@henry416 we have a suppression .csv file for each of the StaticAnalysis analyzers that we have, so the one you will be concerned about is tools/StaticAnalysis/Exceptions/SignatureIssues.csv, which is where you will paste the exception you want to suppress.

If you run the build locally, you will find a SignatureIssues.csv file in src/Package/Debug that contains all of the errors picked up during the build, and you will just need to copy the severity 1 issues from this file into the suppressed exceptions file mentioned previously.

@cormacpayne I have suppressed the exceptions for the files I have altered.

cormacpayne · 2017-03-23T22:32:59Z

+            Mandatory = false,
+            ValueFromPipelineByPropertyName = true,
+            HelpMessage = "Whether to enable policy-based traffic selectors for a S2S connection")]
+        public string UsePolicyBasedTrafficSelectors { get; set; }


@henry416 why use a string here if you're going to just parse it into a bool? The recommended pattern is to add a parameter called EnablePolicyBasedTrafficSelectors, and when this switch is enabled, users can provide a value for the IpsecPolicies parameter. These two parameters would need to be in a separate parameter set from the default one, and EnablePolicyBasedTrafficSelectors would need to be a mandatory parameter. Let me know if you have any questions about this.

@cormacpayne This field is meant to act similar to the enableBgp field - it should be completely optional so that it doesn't break any existing PS scripts. PolicyBasedTrafficSelectors is also independent of the IpsecPolicies field.

@henry416 oops, sorry, I thought the two new parameters should be grouped together. In that case, you should just make this string parameter into a SwitchParameter called EnablePolicyBasedTrafficSelectors.

Following this pattern will make for a much better user experience - if the user wants to enable policy-based traffic selectors, then they will add -EnablePolicyBasedTrafficSelectors to the end of the command, and if they don't want to use it, then they don't have to do anything extra. Having the user provide a string that is being parsed into a bool is a less user-friendly way of having using a SwitchParameter.

@cormacpayne I have changed it to SwitchParameter

cormacpayne · 2017-03-23T22:34:15Z

+```
+
+## DESCRIPTION
+{{Fill in the Description}}


@henry416 there are a few templates left behind from platyPS when generating the markdown for the first time. Would you mind replacing these templates (description, examples, etc.) with useful information for the user?

@cormacpayne during platyPS regen, there were also a lot of other .md files changed that are not related to my work + Commands.Network/Microsoft.Azure.Commands.Network.dll-Help.xml - should I add those files as well?

@henry416 you do not need to add those files - I am working on a change that will get rid of the XML help from the repository and refresh all of the markdown files (updating them with the latest changes in the code), so all of that should be taken care of when my PR gets merged

@cormacpayne Synopsis is added.

cormacpayne · 2017-03-23T22:34:27Z

  <package id="Microsoft.Azure.Management.Authorization" version="1.0.0" targetFramework="net45" />
  <package id="Microsoft.Azure.Management.Compute" version="14.0.0-prerelease" targetFramework="net45" />
-  <package id="Microsoft.Azure.Management.Network" version="9.1.0-preview" targetFramework="net45" />
+  <package id="Microsoft.Azure.Management.Network" version="9.2.0-preview" targetFramework="net45" />


@henry416 same comment

…ding option rollback

…ssion

henry416 · 2017-03-24T19:12:04Z

@cormacpayne Will I be able to push in changes after today? I haven't seen the sdk-for-net package being built yet - should I just put in the 10.0.0 version in the nuget references despite the package not existing?

henry416 · 2017-03-24T20:42:05Z

@cormacpayne I have added auto-generated .md files - picked up several changes from other services that weren't in the checked-in files

cormacpayne · 2017-03-24T21:08:58Z

 Creates a new peering configuration to be added to an ExpressRoute circuit.

+### [New-AzureRmIpsecPolicy](New-AzureRmIpsecPolicy.md)
+{{Fill in the Synopsis}}


@henry416 can you fill in this synopsis?

@cormacpayne Done.

cormacpayne · 2017-03-24T21:10:22Z

@henry416 we will make sure the SDK gets published by early next week, and then once it is published (and any other feedback for this PR is resolved), then we can merge this

henry416 · 2017-03-24T21:11:14Z

@cormacpayne Okay, just to make sure, will this PR make the early april release in that case?

henry416 · 2017-03-29T21:21:35Z

@azuresdkci test this please

henry416 · 2017-03-29T23:27:22Z

@cormacpayne I updated the nuget package. Pending sign off if nothing else is blocking.

henry416 · 2017-03-30T18:22:50Z

@cormacpayne Fixed nuget package for compute

henry416 · 2017-03-30T20:57:00Z

@cormacpayne I have fixed two tests, however, I think the application gateway test is failing - could you contact someone on app gw to fix this?

jobatzil · 2017-03-30T23:57:07Z

@henry416 Hey Henry :)
yes, we have breaking changes in the new azure-sdk version (which is the reason for the major version incrementation). I update the Application Gateway related part of PS in this PR: #3678

henry416 · 2017-03-31T04:21:16Z

@cormacpayne Looks clear then - can merge proceed?

henry416 · 2017-03-31T21:01:53Z

@cormacpayne Fixed merge issues and added changelog

henry416 · 2017-04-03T15:46:50Z

@cormacpayne Can this be merged?

henry416 added 5 commits March 20, 2017 11:11

IPSec Policies for VirtualNetworkGatewayConnection: added object mode…

b4f0f3c

…l + test case

merge conflict avoid

ff8d713

merged ipsec policies

1a75de7

Ipsec Policy VirtualNetworkGatewayConnection models + cmdlets + tests…

cf1fe9f

… added

nuget package update in-progress

1d650b2

msftclas added the cla-not-required label Mar 22, 2017

azuresdkci assigned cormacpayne Mar 22, 2017

partial md files for ipsec policy addition

3318799

Network PS xml for ipsec policy

b33895a

cormacpayne suggested changes Mar 23, 2017

View reviewed changes

cormacpayne removed their assignment Mar 23, 2017

cormacpayne added the needs-revision label Mar 23, 2017

cormacpayne assigned henry416 Mar 23, 2017

henry416 added 4 commits March 23, 2017 17:38

PR feedback: Ipsec policy descriptions for .md and model + test recor…

038b4c0

…ding option rollback

Changed usepolicybasedTS input type + signatureIssue exception suppre…

c67eb8e

…ssion

added misc auto-generated documentation from platyPS

77f4a3e

removed App.config from Test csproj

a03392a

cormacpayne reviewed Mar 24, 2017

View reviewed changes

henry416 added 2 commits March 24, 2017 15:56

Added synopsis to network.md file

c0523f9

Network package bump to 10.0.0-preview

7cd3558

henry416 added 3 commits March 29, 2017 14:24

comments on policy based TS test

eefad66

Network nuget package 10.0.0.0 version no. fix

2006b5f

app.config removal

5dcdad7

henry416 added 3 commits March 30, 2017 10:14

modify test session to 2017-03-01 for Network tests

343a658

Compute fix for tests and imports

4989d75

fix for appgw test

7f82146

henry416 added 2 commits March 30, 2017 13:49

Merge remote-tracking branch 'upstream/dev' into dev

4b2fdd4

missing test fix

6447d33

test record fix - build not copying over data

8c15ac6

cormacpayne changed the base branch from dev to release-3.8.0 March 31, 2017 20:49

henry416 added 2 commits March 31, 2017 13:57

merge into release 3.8.0

5cf89f8

changelog for ipsec policies feature

46492e6

cormacpayne approved these changes Apr 3, 2017

View reviewed changes

cormacpayne merged commit 71824da into Azure:release-3.8.0 Apr 3, 2017

Conversation

henry416 commented Mar 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Reference PR: Azure/azure-sdk-for-net#2943

Uh oh!

msftclas commented Mar 22, 2017

Uh oh!

azuresdkci commented Mar 22, 2017

Uh oh!

henry416 commented Mar 22, 2017

Uh oh!

henry416 commented Mar 23, 2017

Uh oh!

cormacpayne commented Mar 23, 2017

Uh oh!

henry416 commented Mar 23, 2017

Uh oh!

cormacpayne commented Mar 23, 2017

Uh oh!

henry416 commented Mar 23, 2017

Uh oh!

cormacpayne left a comment

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!

cormacpayne Mar 24, 2017 • 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henry416 commented Mar 24, 2017

Uh oh!

henry416 commented Mar 24, 2017

henry416 commented Mar 22, 2017 •

edited

Loading

cormacpayne Mar 24, 2017 •

edited

Loading

jobatzil commented Mar 30, 2017 •

edited

Loading