Adding support for Private DNS zones Public Preview #5701

muwaqar · 2018-03-07T23:09:18Z

Description

This PR is loosely based on https://github.com/Azure/azure-powershell-pr/pull/294 which was submitted when the Private DNS zones feature was launched in a private preview. The PR keeps most of the changes.

It is built on top of 2018-03-01-preview Swagger version (Azure/azure-rest-api-specs#2587) and version 2.2.0-preview of the .NET SDK (Azure/azure-sdk-for-net#4109). These changes are not merged/live yet, so this PR is just to initiate the design review and get an approval in principle. Once the pre-requisite changes have been merged, only then we plan to complete this PR.

The relevant cmdlet design review for this PR can be found at https://github.com/Azure/azure-powershell-cmdlet-review-pr/issues/22

Checklist

I have read the Submitting Changes section of CONTRIBUTING.md
The title of the PR is clear and informative
The appropriate change log has been updated
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:
- the changes have gone through a cmdlet design review
- the cmdlet markdown files were generated using the platyPS module

maddieclayton · 2018-03-07T23:11:34Z

@muwaqar Do you have cmdlet review associated with this PR? If not, please fill out a review here: https://github.com/Azure/azure-powershell-cmdlet-review-pr. Once this is approved, we will proceed with the PR review.

muwaqar · 2018-03-07T23:46:53Z

The design review can be found at https://github.com/Azure/azure-powershell-cmdlet-review-pr/issues/22

…DnsZone cmdlets.

maddieclayton · 2018-03-08T22:53:59Z

@muwaqar To be able to properly build without a published nuget package, you will need to put your unpublished nuget package here: https://github.com/Azure/azure-powershell/tree/preview/tools/LocalFeed

muwaqar · 2018-03-08T23:55:28Z

@maddieclayton : We are waiting for the Swagger and .NET SDK PRs to complete (mentioned in the description). Once they get merged, I will push another commit here with the final .NET SDK version.

cormacpayne · 2018-03-12T18:26:27Z

@maddieclayton what's the status of this PR?

maddieclayton · 2018-03-12T18:29:25Z

@cormacpayne Still waiting for SDK to be merged and published. @muwaqar please assign this to me once the SDK is published and the PR is updated.

muwaqar · 2018-03-13T17:07:40Z

@maddieclayton : I generated the MSI (https://azuresdkci.westus2.cloudapp.azure.com/view/PowerShell/job/posh-pack/52/). Now, because of PSVirtualNetwork in New/Set-AzureRmDnsZone cmdlets, we have a dependency on AzureRm.Network module. Because of which, I have to do Import-Module AzureRm.Network before Import-Module AzureRm.Dns, otherwise using New/Set-AzureRmDnsZone cmdlet fails saying that the Network DLL was not found. Is there a way to automate this so that importing Dns module always imports Network module? Or is it only because the MSI was delay signed (instead of strong signed).

cormacpayne · 2018-03-13T18:00:29Z

@muwaqar we don't want to have cross-module dependencies -- in this case, we have a Commands.Common.Network project that contains generated code from the Network SDK that you should be using instead of the types exposed from the Network module. I think we need to look at creating a common PSVirtualNetwork type that is stored in Commands.Common.Network and is used by both your module and the Network module, so users don't have to load the AzureRM.Network module anytime they want to use your commands.

maddieclayton · 2018-03-13T17:51:49Z

src/ResourceManager/Dns/Commands.Dns.Test/Commands.Dns.Test.csproj

+    </Reference>
+    <Reference Include="Microsoft.Azure.Management.Network, Version=17.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
+      <SpecificVersion>False</SpecificVersion>
+      <HintPath>..\..\..\packages\Microsoft.Azure.Management.Network.17.0.0-preview\lib\net452\Microsoft.Azure.Management.Network.dll</HintPath>


Need to add your new test here - follow the structure of the other json files.

I don't get this. Can you clarify?

Look at line 240 in this file, and create a new entry for each of your new json files generated for new tests, and add a line so that each of them is copied to the output directory.

maddieclayton · 2018-03-13T17:53:13Z

src/ResourceManager/Dns/Commands.Dns.Test/ScenarioTests/Common.ps1

+function TestSetup-CreateVirtualNetwork($resourceGroup)
+{
+    $virtualNetworkName = Get-VirtualNetworkName
+	$location = Get-ProviderLocation "microsoft.network"


I think you need to do the full provider name/resource type i.e. "microsoft.network/virtualnetwork" (I don't know that that is the correct resource type).

maddieclayton · 2018-03-13T17:56:53Z

src/ResourceManager/Dns/Commands.Dns.Test/ScenarioTests/ZoneTests.ps1

+	Assert-AreEqual 1 $createdZone.NumberOfRecordSets
+	Assert-AreNotEqual $createdZone.NumberOfRecordSets $createdZone.MaxNumberOfRecordSets
+	Assert-AreEqual Private $createdZone.ZoneType
+	Assert-AreEqual 1 $createdZone.RegistrationVirtualNetworkIds.Count


It would be nice to check the actual value of the ids here.

maddieclayton · 2018-03-13T18:04:05Z

src/ResourceManager/Dns/Commands.Dns/Commands.Dns.csproj

@@ -115,6 +121,10 @@
      <Project>{3819d8a7-c62c-4c47-8ddd-0332d9ce1252}</Project>
      <Name>Commands.ResourceManager.Common</Name>
    </ProjectReference>
+    <ProjectReference Include="..\..\Network\Commands.Network\Commands.Network.csproj">


Use this project instead:

azure-powershell/src/ResourceManager/Compute/Commands.Compute/Commands.Compute.csproj

Line 343 in 2001d50

<ProjectReference Include="..\..\..\Common\Commands.Common.Network\Commands.Common.Network.csproj">

maddieclayton · 2018-03-13T18:06:00Z

src/ResourceManager/Dns/Commands.Dns/SetEnv.ps1

@@ -0,0 +1,21 @@
+Param(


What is this and where is it used?

It's a helper file that we use to record the tests. Without it, it's very cumbersome to set it up.

This setup method already exists and is described here: https://github.com/Azure/azure-powershell/blob/preview/documentation/testing-docs/using-azure-test-framework.md. Please remove this ps1 file.

maddieclayton · 2018-03-13T18:06:24Z

src/ResourceManager/Dns/Commands.Dns/Zones/NewAzureDnsZone.cs

 using ProjectResources = Microsoft.Azure.Commands.Dns.Properties.Resources;

 namespace Microsoft.Azure.Commands.Dns
 {
+    using System.Linq;


Move these above

maddieclayton · 2018-03-13T18:08:27Z

src/ResourceManager/Dns/Commands.Dns/Zones/NewAzureDnsZone.cs

@@ -35,10 +43,30 @@ public class NewAzureDnsZone : DnsBaseCmdlet
        [ValidateNotNullOrEmpty]
        public string ResourceGroupName { get; set; }

+        [Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true, HelpMessage = "The type of the zone, Public or Private. This property cannot be changed for a zone.")]
+        [ValidateNotNullOrEmpty]


Please add a validateset here if the only optional allow are public and private.

maddieclayton · 2018-03-13T18:09:57Z

src/ResourceManager/Dns/Commands.Dns/Zones/SetAzureDnsZone.cs

-                    Etag = "*",
-                    Tags = this.Tag,
-                };
+                    this.Name = this.Name.TrimEnd('.');


Would this have previously caused the cmdlet to fail?

This check was already present in the file, on line 76.

maddieclayton · 2018-03-13T18:14:57Z

tools/StaticAnalysis/Exceptions/SignatureIssues.csv

@@ -1087,3 +1087,11 @@
 "Microsoft.Azure.Commands.AnalysisServices.dll","Microsoft.Azure.Commands.AnalysisServices.NewAzureRmAnalysisServicesFirewallRule","New-AzureRmAnalysisServicesFirewallRule","1","8100","New-AzureRmAnalysisServicesFirewallRule Does not support ShouldProcess but the cmdlet verb New indicates that it should.","Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue"
 "Microsoft.Azure.Commands.AnalysisServices.dll","Microsoft.Azure.Commands.AnalysisServices.NewAzureRmAnalysisServicesFirewallConfig","New-AzureRmAnalysisServicesFirewallConfig","1","8100","New-AzureRmAnalysisServicesFirewallConfig Does not support ShouldProcess but the cmdlet verb New indicates that it should.","Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue"
 "Microsoft.WindowsAzure.Commands.Storage.dll","Microsoft.WindowsAzure.Commands.Storage.Common.Cmdlet.EnableAzureStorageServiceDeleteRetentionPolicyCommand","Enable-AzureStorageDeleteRetentionPolicy","1","8410","Parameter RetentionDays of cmdlet Enable-AzureStorageDeleteRetentionPolicy does not follow the enforced naming convention of using a singular noun for a parameter name.","Consider using a singular noun for the parameter name."
+"Microsoft.Azure.Commands.Dns.dll","Microsoft.Azure.Commands.Dns.SetAzureDnsZone","Set-AzureRmDnsZone","1","8410","Parameter RegistrationVirtualNetworkIds of cmdlet Set-AzureRmDnsZone does not follow the enforced naming convention of using a singular noun for a parameter name.","Consider using a singular noun for the parameter name."


Please change all of the "VirtualNetworkIds" to "VirtualNetworkId" and all of the "VirtualNetworks" to "VirtualNetwork". This is a powershell standard. Once you do that you can remove these lines from SignatureIssues.

maddieclayton · 2018-03-13T23:33:35Z

@muwaqar Besides the Network issue, this PR looks good to me.

cormacpayne

@muwaqar a few comments. Also, I might have missed this in discussion, but when is the SDK being published to NuGet? This needs to happen before we can merge this PR

cormacpayne · 2018-03-14T15:36:13Z

src/ResourceManager/Dns/AzureRM.Dns.psd1


 # Assemblies that must be loaded prior to importing this module
-RequiredAssemblies = '.\Microsoft.Azure.Management.Dns.dll'
+RequiredAssemblies = '.\Microsoft.Azure.Management.Dns.dll', '.\Microsoft.Azure.Commands.Network.dll'


@muwaqar this reference to Microsoft.Azure.Commands.Network.dll can be removed

cormacpayne · 2018-03-14T15:41:43Z

src/ResourceManager/Dns/Commands.Dns.Test/Commands.Dns.Test.csproj

+      <HintPath>..\..\..\packages\Microsoft.Azure.Management.Dns.2.2.0-preview\lib\net452\Microsoft.Azure.Management.Dns.dll</HintPath>
+      <Private>True</Private>
+    </Reference>
+    <Reference Include="Microsoft.Azure.Management.Network, Version=17.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">


@muwaqar if your test project is going to depend on the Network SDK, then you should update the Network entry in the TestMappings.json file with a reference to the Dns test project so these tests get run whenever a change in Network is made

cormacpayne · 2018-03-14T15:43:45Z

src/ResourceManager/Dns/Commands.Dns/Commands.Dns.csproj

-      <HintPath>..\..\..\packages\Microsoft.Azure.Management.Dns.2.0.0-preview\lib\net452\Microsoft.Azure.Management.Dns.dll</HintPath>
+      <HintPath>..\..\..\packages\Microsoft.Azure.Management.Dns.2.2.0-preview\lib\net452\Microsoft.Azure.Management.Dns.dll</HintPath>
+    </Reference>
+    <Reference Include="Microsoft.Azure.Management.Network, Version=17.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">


@muwaqar this reference shouldn't be needed anymore, right?

cormacpayne · 2018-03-14T15:50:05Z

src/ResourceManager/Network/Commands.Network/Models/PSVirtualNetwork.cs

+    using Microsoft.Azure.Management.Internal.Network.Common;
+    using Newtonsoft.Json;
+
+    public class PSVirtualNetwork : PSTopLevelResource, IResourceReference


@muwaqar since we're making changes in the Network module, we will need to release a new version of their module along with Dns so users are able to access this updated type. Our mechanism for determining if a module has been updated or not is an entry in the change log, so you will need to update the Network change log with a short snippet about updating model types for compatibility with Dns cmdlets.

muwaqar · 2018-03-14T17:39:32Z

@cormacpayne: Fixed the issues for which you posted comments.
Regarding the .NET SDK, our PR (Azure/azure-sdk-for-python#2132) has been completed and just awaiting nuget generation. I will update this PR as soon as we have a nuget and remove the one I added to the LocalFeed for now. I understand this PR cannot be completed before then, but we want to reach a state where we have a sign-off on all other aspects of the PR besides the nuget change.

cormacpayne · 2018-03-14T18:03:28Z

src/ResourceManager/Dns/Dns.sln

@@ -36,6 +36,8 @@ Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Commands.Common.Network", "
 EndProject
 Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Commands.Common.Storage", "..\..\Common\Commands.Common.Storage\Commands.Common.Storage.csproj", "{65C3A86A-716D-4E7D-AB67-1DB00B3BF72D}"
 EndProject
+Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Commands.Network", "..\Network\Commands.Network\Commands.Network.csproj", "{98CFD96B-A6BC-4F15-AE2C-603FC2B58981}"


@muwaqar you should include the Commands.Common.Network project in this solution as well so it gets built with your solution. Also, do you need to build Commands.Network with this solution, or can we remove it?

Commands.Common.Network is already part of this solution.

muwaqar · 2018-03-15T04:58:46Z

@maddieclayton @cormacpayne : I have updated PR with published .NET SDK nuget. The build seems to be failing due to some unrelated issue. Please re-queue the build, and if you don't have any further comments, please merge it at your earliest convenience.

cormacpayne · 2018-03-16T00:48:25Z

On-demand: https://azuresdkci.westus2.cloudapp.azure.com/job/powershell-demand/606/ (and 607 and 608)

Muhammad Waqar added 4 commits March 7, 2018 14:36

Adding support in cmdlets to create/update Private zones.

ec9d0b5

Adding new tests.

74c9236

Updating help files and changelog.

39b46d2

Re-recording all the tests.

5cdd258

maddieclayton added the Cmdlet Review Required ⚠️ label Mar 7, 2018

maddieclayton assigned muwaqar Mar 7, 2018

azuresdkci added the needs-review label Mar 7, 2018

maddieclayton removed the needs-review label Mar 8, 2018

Adding support for passing virtual network objects to New/Set-AzureRm…

932d7cd

…DnsZone cmdlets.

maddieclayton added Cmdlet Review Complete Do Not Merge 🚫 needs-review and removed Cmdlet Review Required ⚠️ labels Mar 8, 2018

maddieclayton assigned maddieclayton and unassigned muwaqar Mar 8, 2018

Making ZoneType optional.

d4a178b

maddieclayton added needs-revision and removed needs-review labels Mar 8, 2018

maddieclayton assigned muwaqar and unassigned maddieclayton Mar 8, 2018

cormacpayne added needs-review and removed needs-revision labels Mar 12, 2018

cormacpayne assigned maddieclayton and unassigned muwaqar Mar 12, 2018

maddieclayton suggested changes Mar 13, 2018

View reviewed changes

Muhammad Waqar added 2 commits March 13, 2018 13:24

Addressing review comments.

7695007

Including test recordings for new tests

dae3731

Using IResourceReference instead of PSVirtualNetwork

143a1cb

cormacpayne suggested changes Mar 14, 2018

View reviewed changes

Addressing review comments.

ec082f0

cormacpayne reviewed Mar 14, 2018

View reviewed changes

Removing Commands.Network from Dns.sln.

d943c81

maddieclayton assigned cormacpayne and unassigned muwaqar Mar 14, 2018

maddieclayton added needs-review and removed needs-revision labels Mar 14, 2018

muwaqar added 2 commits March 15, 2018 01:38

Adding signed Dns nuget.

18c89ec

Removing committed nuget.

9bf1e27

maddieclayton removed Cmdlet Review Complete Do Not Merge 🚫 labels Mar 15, 2018

muwaqar and others added 2 commits March 16, 2018 02:45

Handling null check in cmdlets.

60003a4

Merge branch 'preview' into muwaqar/private-dns-public-preview

702754c

cormacpayne approved these changes Mar 16, 2018

View reviewed changes

maddieclayton approved these changes Mar 16, 2018

View reviewed changes

cormacpayne added Waiting for On Demand and removed needs-review labels Mar 16, 2018

cormacpayne removed their assignment Mar 16, 2018

cormacpayne merged commit d6d314d into Azure:preview Mar 16, 2018

Adding support for Private DNS zones Public Preview #5701

Adding support for Private DNS zones Public Preview #5701

Uh oh!

Conversation

muwaqar commented Mar 7, 2018 • edited by maddieclayton Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

maddieclayton commented Mar 7, 2018

Uh oh!

muwaqar commented Mar 7, 2018

Uh oh!

maddieclayton commented Mar 8, 2018

Uh oh!

muwaqar commented Mar 8, 2018

Uh oh!

cormacpayne commented Mar 12, 2018

Uh oh!

maddieclayton commented Mar 12, 2018

Uh oh!

muwaqar commented Mar 13, 2018

Uh oh!

cormacpayne commented Mar 13, 2018

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!

maddieclayton commented Mar 13, 2018

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muwaqar commented Mar 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muwaqar commented Mar 15, 2018

Uh oh!

cormacpayne commented Mar 16, 2018 • edited by maddieclayton Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

muwaqar commented Mar 7, 2018 •

edited by maddieclayton

Loading

cormacpayne commented Mar 16, 2018 •

edited by maddieclayton

Loading