-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Adding basic SQL virtual network rules functionality #4404
Conversation
Adding virtual network rules commands and tests
Editing tests and updating session records
This reverts commit 630830b.
@dnayantara, |
Can one of the admins verify this patch? |
@azuresdkci add to whitelist |
'New-AzureRmSqlSyncAgent', 'Get-AzureRmSqlSyncAgent', | ||
'Remove-AzureRmSqlSyncAgent', 'New-AzureRmSqlSyncAgentKey', | ||
'Get-AzureRmSqlSyncAgentLinkedDatabase' | ||
'Switch-AzureRmSqlDatabaseFailoverGroup', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
intentional change? please undo to improve reviewability
#> | ||
function Get-VirtualNetworkSubnetId ($vnetName) | ||
{ | ||
$vnetSubscriptionId = "d513e2e9-97db-40f6-8d1a-ab3b340cc81a" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will make the test unrecordable, so I will not approve until the test is recordable. you can either import vnet cmdlets or directly use the vnet sdk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The networking team has removed their vnet cmdlets because of a naming change. So I cannot use their cmdlets. I will create a new task to re-record tests once their cmdlets are in. Would that be ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case you should be able to directly call the networking .net sdk from the test code
[Parameter(Mandatory = true, | ||
HelpMessage = "The Virtual Network Subnet Id that specifies the Microsoft.Network details")] | ||
[ValidateNotNull] | ||
public string VirtualNetworkSubnetId { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is open to debate but resource Id is IMO annoying to type, I would prefer to have another parameter set that has -VirtualNetworkSubnetName
, -VirtualNetworkName
, -VirtualNetworkResourceGroupName
etc. The subnet's resource group can be optional, default to the same resource group as the sql server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the customer's perspective, they will get this entire id the minute they create the vnet using networking cmdlets. So it will be a copy paste for them. If we separate it out, they would have to manually separate out parts of the id and then pass it to us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed and it's fine as is, just wanted to make sure this was a conscious design choice by Nayantara.
[Parameter(Mandatory = true, | ||
HelpMessage = "The Virtual Network Subnet Id for the rule.")] | ||
[ValidateNotNull] | ||
public string VirtualNetworkSubnetId { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about resource id as earlier.
{ | ||
AzureSqlServerVirtualNetworkRuleModel vnetFirewallRuleName = new AzureSqlServerVirtualNetworkRuleModel(); | ||
|
||
vnetFirewallRuleName.ResourceGroupName = resourceGroup; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor style comment, low priority: I like to use object initializer syntax in cases like this
I will make the changes and then reopen the pull request |
This reverts commit e04d6cb.
Adding fix for jenkins job
This reverts commit 8133016.
Reopening this pull request after addressing above comments and testing in production |
@dnayantara, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor comments. I only reviewed files under ResourceManager/Sql.
@@ -177,7 +177,11 @@ CmdletsToExport = 'Get-AzureRmSqlDatabaseTransparentDataEncryption', | |||
'Get-AzureRmSqlSyncMember', 'Remove-AzureRmSqlSyncMember', | |||
'New-AzureRmSqlSyncAgent', 'Get-AzureRmSqlSyncAgent', | |||
'Remove-AzureRmSqlSyncAgent', 'New-AzureRmSqlSyncAgentKey', | |||
'Get-AzureRmSqlSyncAgentLinkedDatabase' | |||
'Get-AzureRmSqlSyncAgentLinkedDatabase', | |||
'New-AzureRmSqlServerVirtualNetworkRule', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: fix indentation here
<value>Permanently removing Virtual Network Rule '{0}' for Azure Sql Server '{1}'</value> | ||
</data> | ||
<data name="RemoveAzureSqlServerVirtualNetworkRuleWarning" xml:space="preserve"> | ||
<value>Are you sure you want to remove the Virtual Network Rule '{0}' for Azure Sql Server '{1}'?</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: 2 spaces just before '{1}'
{ | ||
if (ex.Response.StatusCode == System.Net.HttpStatusCode.NotFound) | ||
{ | ||
// This is what we want. We looked and there is no server with this name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: We looked and there is no virtual network rule with this name.
{ | ||
ResourceGroupName = this.ResourceGroupName.Trim(), | ||
ServerName = this.ServerName.Trim(), | ||
VirtualNetworkRuleName = this.VirtualNetworkRuleName.Trim(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it standard to trim like this? I wonder if somebody does new-vnetrule -rulename "asdf "
and then 'get-vnetrule -rulename "asdf "
will they get the rule back? I mean I think Trim has to be done everywhere or nowhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested it. This is working the way it should.
@@ -1556,9 +1556,6 @@ | |||
<Component Id="cmpC67F14F377421F1BCD74EC5A579D43CB" Guid="*"> | |||
<File Id="filE9553CEC1D67AB3D4B77759239D60B18" KeyPath="yes" Source="$(var.sourceDir)\ResourceManager\AzureResourceManager\AzureRM.IotHub\Microsoft.Azure.Commands.IotHub.dll-Help.xml" /> | |||
</Component> | |||
<Component Id="cmpD844DA64698210481D550B5CD8799D26" Guid="*"> | |||
<File Id="filA23D9B350E9B87D17B90B2181AE71E4B" KeyPath="yes" Source="$(var.sourceDir)\ResourceManager\AzureResourceManager\AzureRM.IotHub\Microsoft.Azure.Commands.IotHub.format.ps1xml" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this an intentional change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a change to fix broken build. This change - #4503 removed a file, but this file was not updated.
@@ -177,7 +177,11 @@ CmdletsToExport = 'Get-AzureRmSqlDatabaseTransparentDataEncryption', | |||
'Get-AzureRmSqlSyncMember', 'Remove-AzureRmSqlSyncMember', | |||
'New-AzureRmSqlSyncAgent', 'Get-AzureRmSqlSyncAgent', | |||
'Remove-AzureRmSqlSyncAgent', 'New-AzureRmSqlSyncAgentKey', | |||
'Get-AzureRmSqlSyncAgentLinkedDatabase' | |||
'Get-AzureRmSqlSyncAgentLinkedDatabase', | |||
'New-AzureRmSqlServerVirtualNetworkRule', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix spacing (use spaces not tabs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will make these changes
|
||
<# | ||
.SYNOPSIS | ||
Tests creating and updating a virtual network rule |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please be more specific.
try | ||
{ | ||
# Create rule | ||
$virtualNetworkRule = New-AzureRmSqlServerVirtualNetworkRule -ResourceGroupName $rg.ResourceGroupName -ServerName $server.ServerName -VirtualNetworkRuleName $virtualNetworkRuleName -VirtualNetworkSubnetId $virtualNetworkSubnetId1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please make these lines narrower to make it easier to review. You can use the ` character to have a command span multiple lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something like this would work?
New-AzureRmSqlServerVirtualNetworkRule -ResourceGroupName $rg.ResourceGroupName -ServerName $server.ServerName
` -VirtualNetworkRuleName $virtualNetworkRuleName -VirtualNetworkSubnetId $virtualNetworkSubnetId1
$resp = Remove-AzureRmSqlServerVirtualNetworkRule -ResourceGroupName $rg.ResourceGroupName -ServerName $server.ServerName -VirtualNetworkRuleName $virtualNetworkRuleName -Force | ||
$all = Get-AzureRmSqlServerVirtualNetworkRule -ResourceGroupName $rg.ResourceGroupName -ServerName $server.ServerName | ||
Assert-AreEqual $all.Count 0 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove unnecessary blank lines.
[ValidateNotNull] | ||
public string VirtualNetworkSubnetId { get; set; } | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove extra blank line. Look through all files.
using System.Security; | ||
using System.Security.Permissions; | ||
using Microsoft.Azure.Commands.ResourceManager.Common.Tags; | ||
using Microsoft.Azure.Commands.Common.Authentication.Abstractions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please go through all .cs files and sort the usings and remove the ones not used.
``` | ||
|
||
## DESCRIPTION | ||
This command creates an Azure SQL Server Virtual Network Rule. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you provide more detail describing what a "Azure SQL Server Virtual Network Rule" is or a link to MSDN?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have an official MSDN yet. But I will provide more details
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! I edited the synopsis instead of the description. Will make that change with the next set of edits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in most recent commit
@@ -1,8 +1,8 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<packages> | |||
<package id="Microsoft.Azure.KeyVault.Core" version="1.0.0" targetFramework="net45" /> | |||
<package id="Microsoft.Azure.Management.Sql" version="1.5.0-preview" targetFramework="net452" /> | |||
<package id="Microsoft.Azure.Management.Network" version="12.0.0-preview" targetFramework="net452" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see where this was used. Please remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our tests have a dependency on the Network team's cmdlets. Thus this has been added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the test package config. This is the config for the cmdlets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats true. I assumed this was required in the commands project as well. I will try removing it and checking if it works.
@@ -91,6 +91,11 @@ internal static IList<string> RequiredProvidersForResourceManager<T>() | |||
return new[] { StorageProviderNamespace }; | |||
} | |||
|
|||
if (typeof(T).FullName.Equals("Microsoft.Azure.Management.Network.NetworkManagementClient")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For creating and accessing the Network Management client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is in the ServiceManagement path/namespace. Is it used in the ResourceManager path/namespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not exactly sure what you mean. Let me test it after removing it, probably I won't even need it.
DR drill is now over. If you start another build, I would expect the tests to pass. I ran them once again on my machine, |
@azuresdkci test this please |
@akormm @jaredmoo would one of you mind signing off on the changes made by Nayantara? We are looking to shut down the public repository for Ignite and want to merge all PRs that are ready by the end of the day or we will re-open them in the private repository. |
…verVirtualNetworkRule.md
I addressed Adam's requested change. Jared signed off and Adam is OOF for a month. Is there anything else that needs to be done? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kisantia @jaredmoo please take a look at the comments when you get the chance.
We are shutting down the public repository for the Ignite release, so you should re-open this PR in the private repository. Here is a guide for migrating your PR from the public repository to the private repository: https://github.com/Azure/azure-powershell/wiki/Public-to-Private-Migration
@@ -1556,9 +1556,6 @@ | |||
<Component Id="cmpC67F14F377421F1BCD74EC5A579D43CB" Guid="*"> | |||
<File Id="filE9553CEC1D67AB3D4B77759239D60B18" KeyPath="yes" Source="$(var.sourceDir)\ResourceManager\AzureResourceManager\AzureRM.IotHub\Microsoft.Azure.Commands.IotHub.dll-Help.xml" /> | |||
</Component> | |||
<Component Id="cmpD844DA64698210481D550B5CD8799D26" Guid="*"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kisantia pulling the latest changes from the preview branch will remove the need for this change
@@ -50,6 +50,18 @@ | |||
<AllowUnsafeBlocks>true</AllowUnsafeBlocks> | |||
</PropertyGroup> | |||
<ItemGroup> | |||
<Reference Include="Microsoft.Azure.Management.Network, Version=14.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kisantia this reference to the Network management library should be removed. Only the Network project should have a dependency on the management library. If you need to use the code from their library, please use the Commands.Common.Network
project.
@@ -6,12 +6,13 @@ | |||
<package id="Microsoft.Azure.Common" version="2.1.0" targetFramework="net452" /> | |||
<package id="Microsoft.Azure.Common.Dependencies" version="1.0.0" targetFramework="net452" /> | |||
<package id="Microsoft.Azure.KeyVault.Core" version="1.0.0" targetFramework="net452" /> | |||
<package id="Microsoft.Azure.Management.Network" version="14.0.0-preview" targetFramework="net452" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kisantia same comment
@@ -135,6 +135,15 @@ public string RMStorageDataPlaneModule | |||
} | |||
} | |||
|
|||
public string RMNetworkModule |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kisantia same comment
@@ -82,10 +82,22 @@ | |||
<HintPath>..\..\..\packages\Microsoft.Data.Services.Client.5.8.2\lib\net40\Microsoft.Data.Services.Client.dll</HintPath> | |||
<Private>True</Private> | |||
</Reference> | |||
<Reference Include="Microsoft.Rest.ClientRuntime, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kisantia please revert the changes in this file. All changes to dependencies should be done in Common.Dependencies.targets
and the packages.config
in Commands.ResourceManager.Common
.
/// Defines whether it is ok to skip the requesting of rule removal confirmation | ||
/// </summary> | ||
[Parameter(HelpMessage = "Skip confirmation message for performing the action")] | ||
public SwitchParameter Force { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kisantia this Force
parameter should not be needed unless there is a special circumstance where you want to prompt the user to confirm their actions. I would take a look at Adam's comment below to see if this holds true.
/// <summary> | ||
/// Defines the Remove-AzureRmSqlServerVirtualNetworkRule cmdlet | ||
/// </summary> | ||
[Cmdlet(VerbsCommon.Remove, "AzureRmSqlServerVirtualNetworkRule", SupportsShouldProcess = true, ConfirmImpact = ConfirmImpact.Medium)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kisantia this cmdlet has no output type
/// <summary> | ||
/// Defines the Set-AzureRmSqlServerVirtualNetworkRule cmdlet | ||
/// </summary> | ||
[Cmdlet(VerbsCommon.Set, "AzureRmSqlServerVirtualNetworkRule", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kisantia this cmdlet has no output type
/// <summary> | ||
/// Defines the Set-AzureRmSqlServerVirtualNetworkRule cmdlet | ||
/// </summary> | ||
[Cmdlet(VerbsCommon.Set, "AzureRmSqlServerVirtualNetworkRule", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kisantia same comment about adding a parameter set with the VirtualNetworkRule
object
|
||
## OUTPUTS | ||
|
||
### System.Object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kisantia the markdown help will need to be updated whenever you add the output attribute to each cmdlet
Closing in favor of https://github.com/Azure/azure-powershell-pr/pull/278 |
This PR adds the basic functionality of SQL virtual network rules. The nuget has been published in 1.6.0-preview. The SQL code has been updated to reference the new nuget package.
Description
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.
General Guidelines
Testing Guidelines
Cmdlet Signature Guidelines
ShouldProcess
and haveSupportShouldProcess=true
specified in the cmdlet attribute. You can find more information onShouldProcess
here.OutputType
attribute if any output is produced - if the cmdlet produces no output, it should implement aPassThru
parameter.Cmdlet Parameter Guidelines