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] Support active-active VNet gateways #2751

Merged
merged 7 commits into from
Apr 6, 2017
Merged

[Network] Support active-active VNet gateways #2751

merged 7 commits into from
Apr 6, 2017

Conversation

tjprescott
Copy link
Member

@tjprescott tjprescott commented Apr 4, 2017

Closes #2050.

Adds support for active-active VNet gateways according to: https://docs.microsoft.com/en-us/azure/vpn-gateway/vpn-gateway-activeactive-rm-powershell

Essentially these scenarios all boil down to the number of public IP addresses associated with the gateway. Active-standby gateways have only one, while active-active gateways have 2. So the CLI will accept 1 or 2 public IP addresses on create or update and enables or disables active-active mode accordingly. This simplifies the scenarios as they exist in Powershell with minimal expansion of the CLI command's surface area.


This checklist is used to make sure that common guidelines for a pull request are followed.

General Guidelines

  • The PR has modified HISTORY.rst with an appropriate description of the change.

Command Guidelines

  • Each command and parameter has a meaningful description.
  • Each new command has a test.

(see Authoring Command Modules)

@tjprescott tjprescott added the Network az network vnet/lb/nic/dns/etc... label Apr 4, 2017
@tjprescott tjprescott added this to the Sprint 15 milestone Apr 4, 2017
@codecov-io
Copy link

codecov-io commented Apr 4, 2017

Codecov Report

Merging #2751 into master will decrease coverage by <.01%.
The diff coverage is 46.51%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2751      +/-   ##
==========================================
- Coverage   62.86%   62.86%   -0.01%     
==========================================
  Files         480      480              
  Lines       25855    25886      +31     
  Branches     3915     3923       +8     
==========================================
+ Hits        16254    16273      +19     
- Misses       8589     8603      +14     
+ Partials     1012     1010       -2
Impacted Files Coverage Δ
...twork/azure/cli/command_modules/network/_params.py 92.49% <100%> (-0.04%) ⬇️
...network/azure/cli/command_modules/network/_help.py 100% <100%> (ø) ⬆️
...etwork/azure/cli/command_modules/network/custom.py 62.46% <21.05%> (-0.37%) ⬇️
...k/azure/cli/command_modules/network/_validators.py 63.97% <60%> (+0.34%) ⬆️
...twork/azure/cli/command_modules/network/_format.py 41.66% <0%> (+1.38%) ⬆️
...e/cli/command_modules/network/_template_builder.py 86.17% <0%> (+2.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 678b940...ed1066d. Read the comment docs.

Copy link
Member

@derekbekoe derekbekoe left a comment

Choose a reason for hiding this comment

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

Code looks good to me.
Can you include help output also?

@tjprescott
Copy link
Member Author

@derekbekoe

Command
    az network vnet-gateway create: Create a virtual network gateway.

Arguments
    --name -n             [Required]: Name of the VNet gateway.
    --public-ip-addresses [Required]: Specify a single public IP (name or ID) for an active-standby
                                      gateway. Specify two public IPs for an active-active gateway.
    --resource-group -g   [Required]: Name of resource group. You can configure the default group
                                      using 'az configure --defaults group=<name>'.
    --vnet                [Required]: Name or ID of an existing virtual network which has a subnet
                                      named 'GatewaySubnet'.
    --address-prefixes              : Space separated list of address prefixes to associate with the
                                      VNet gateway.
    --gateway-type                  : The gateway type.  Allowed values: ExpressRoute, Vpn.
                                      Default: Vpn.
    --location -l                   : Location. You can configure the default location using 'az
                                      configure --defaults location=<location>'.
    --no-wait                       : Do not wait for the long running operation to finish.
    --sku                           : VNet gateway SKU.  Allowed values: Basic, HighPerformance,
                                      Standard, UltraPerformance.  Default: Basic.
    --tags                          : Space separated tags in 'key[=value]' format. Use "" to clear
                                      existing tags.
    --vpn-type                      : VPN routing type.  Allowed values: PolicyBased, RouteBased.
                                      Default: RouteBased.

BGP Peering Arguments
    --asn                           : Autonomous System Number to use for the BGP settings.
    --bgp-peering-address           : IP address to use for BGP peering.
    --peer-weight                   : Weight (0-100) added to routes learned through BGP peering.

The relevant text for update is the same.

@derekbekoe
Copy link
Member

@tjprescott Looking at the help for --public-ip-addresses, how do I know how to specify 2 ip addresses? Maybe say that it should be space separated?

Copy link
Member

@derekbekoe derekbekoe left a comment

Choose a reason for hiding this comment

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

LGTM. Added 1 q.

@tjprescott
Copy link
Member Author

@derekbekoe good point. It used to say space-separated. I'll add that back in.

@tjprescott tjprescott merged commit bd467e7 into Azure:master Apr 6, 2017
@tjprescott tjprescott deleted the VnetGatewayFixes branch April 6, 2017 16:05
johanste pushed a commit to johanste/azure-cli that referenced this pull request Apr 7, 2017
* Start active-active test scenario.

* Add active-active parameter.

* Active-active scenario test 1 (cross premise)

* Add second active-active scenario (vnet-to-vnet)

* Refine active-active gateway configuration.

* Pylint...

* Code review feedback
@aanandr
Copy link

aanandr commented Apr 11, 2017 via email

@tjprescott
Copy link
Member Author

  1. I'll look into this, but unless the service returns a junk error or takes a long time to error out, we would simply let the service error bubble through.

  2. You are correct--there's no active-active flag in the CLI. The active-active status is determined solely by whether you specify 1 or 2 public IP addresses when creating/updating the gateway.

  3. I can remove this.

4-5. I'll take a second look at the scenario tests and update accordingly.

  1. If that information is not already displayed in the create output, we would not make additional REST calls to manufacture a customized output structure. However, there are a couple options: (1) add a convenience command to get this information or (2) add a convenience flag to the show/create commands which "opts-in" to this behavior.

tjprescott pushed a commit that referenced this pull request Apr 11, 2017
* Enable delay-load of descriptions for commands (speed up az)

* Update find indexing commands to accept callables for description.

* Command load time in progress

* - Moved previously dead command filter from parser to application configuration.
- Removed unused configuration object/argv on application create.

* Remove unused argument (pylint)

* Remove dummy parameter

* Fix for python 2.7

* Fix yet incorrect passage of parameters

* Fix up additional pylint complaints

* Update tests

* Update tests

* Fix up more tests

* Fix up more core tests

* Enable delay-load of descriptions for commands (speed up az)

* Update find indexing commands to accept callables for description.

* [Network] Remove nulls from VPN connection show/list output (#2748)

* Fix #1615.

* Code review feedback.

* Update test docs for running individual test and all tests in mod (#2763)

* Update test docs for running individual test and all tests in mod

* Made feedback changes

* Make argument parameters match up. (#2717)

Make lock command parameter aliases match up with resource commands.

* [DevTestLabs] Adding scenario test to create simple Linux + Windows VM in lab (#2767)

* WIP create linux + Windows vm in lab

* Adding recording

* Add some more error checking/handling. (#2768)

Add more validation to resolve "lock level" for lock commands.

* Fix doc references to azure.cli.commands (#2740)

* Fix doc references to azure.cli.commands

This module has moved to azure.cli.core.commands

* Fix PyLint

* Add clearer guidelines on modifying changelog (#2739)

* Add clearer guidelines on modifying changelog

* A few smaller changes

* another small format change

* Code review changes

* [DevTestLabs] Exposing commands to manage secrets in the lab (#2691)

* ACS Update: nulling out the windows profile so that there isn't a validation fail… (#2764)

* nulling out the windows profile so that there isn't a valdiation failure for missing password

ACS doesn't return a password on GET. az acs scale command does a GET
then PUT, but since ACS doesn't return the password the verification is
failing before the PUT is sent to ACS.

There is a bug in ACS this exposes. So this shouldn't be merged until
after the ACS rollout finishes. Should be about start of next week.

* updating history

* updating version in history

* removing white space added by editor

* [Compute] Fix issues with VMSS and VM availability set update. (#2773)

* Fix issues with VMSS and VM availability set update.

* Update help. Fix #2762.

* Error out if you try to list resources for a group that doesn't exist. (#2769)

* Minor text fixes (#2776)

* Add docs for az lock update. (#2702)

* [DevTestLab] Explicitly enable usage of saved secrets while lab vm creation (#2686)

* Explicitly enable usage of saved secrets for vm creation

* Better error message with not overriding competing paramters

* Adding export-artifacts commands on formula (#2707)

* core: apply configured defaults on optional arg (#2770)

* Core:apply configured defaults on optional argument

* add a test

* add tests

* update history doc

* address review feedback

* [Network] Support active-active VNet gateways (#2751)

* Start active-active test scenario.

* Add active-active parameter.

* Active-active scenario test 1 (cross premise)

* Add second active-active scenario (vnet-to-vnet)

* Refine active-active gateway configuration.

* Pylint...

* Code review feedback

* Packaged release notes and changes for 0.2.4 (#2735)

* Modify HISTORY.md

* Update Dockerfile

* Update debian also

* Add pip dependencies also

* Command load time in progress

* - Moved previously dead command filter from parser to application configuration.
- Removed unused configuration object/argv on application create.

* Remove unused argument (pylint)

* Remove dummy parameter

* Fix for python 2.7

* Fix yet incorrect passage of parameters

* Fix up additional pylint complaints

* Update tests

* Update tests

* Fix up more tests

* Fix up more core tests

* Improve load time of custom.py for profile, find and configure (speeds up raw az command)

* Pylint + flake8 fixes

* Fix new vm tests that failed due to perf refactoring

* Update redis tests that was broken due to perf refactoring

* Delay-load msrest for command executions that don't need it

* Fix flake8 issues

* Fixing/improving detection of pageable class

* flake8 fixes

* Fix broken merge from upstream/master

* Fix broken merge (again)

* flake8 fixes

* Fix up even more merge errors from last upstream merge

* Flake8 fixes (wrong number of newlines)

* Fix delay load of storage assembly for az

* Update history to reference improved performance
@aanandr
Copy link

aanandr commented Apr 11, 2017 via email

@tjprescott
Copy link
Member Author

tjprescott commented Apr 11, 2017

  1. Generally the errors bubble up from the service, especially since the SDKs (being autogenerated) have very little ability to add client side validation that is business logic relevant. We do this on the CLI side when the service fails to enforce its business logic and it creates a negative experience for customers. Another reason we tend to not do it on the CLI side is because it could artificially inhibit he service. For example, if we added this validation and later the service allows active-active mode with UltraPerformance SKU, then the CLI would be artificially constraining the behavior of the service.

  2. Yes, you are correct:

   --public-ip-addresses [Required]: Specify a single public IP (name or ID) for an active-standby
                                      gateway. Specify two space-separated public IPs for an active-
                                      active gateway.

If you use the update command and change this, it will issue an info (visible with --verbose) that it is switching the gateway mode to active-standby or active-active accordingly. Also, the CLI handles the creation of the IP configs automatically so it cuts that step out of the PS scenario.

@blackhu269
Copy link

Hi
when modify the VPN GW from active-active to active-standby with the “az network vnet-gateway update“ there are the errors below, any suggestion about that ?

az network vnet-gateway update --name Internal-VPN --resource-group TestRG2 --public-ip-addresses Internal-ip
init() got multiple values for argument 'private_ip_allocation_method'
Traceback (most recent call last):
File "/opt/az/lib/python3.6/site-packages/azure/cli/main.py", line 36, in main
cmd_result = APPLICATION.execute(args)
File "/opt/az/lib/python3.6/site-packages/azure/cli/core/application.py", line 216, in execute
result = expanded_arg.func(params)
File "/opt/az/lib/python3.6/site-packages/azure/cli/core/commands/init.py", line 381, in call
return self.handler(*args, **kwargs)
File "/opt/az/lib/python3.6/site-packages/azure/cli/core/commands/arm.py", line 341, in handler
raise ex
File "/opt/az/lib/python3.6/site-packages/azure/cli/core/commands/arm.py", line 296, in handler
instance = custom_function(instance, **custom_func_args)
File "/opt/az/lib/python3.6/site-packages/azure/cli/command_modules/network/custom.py", line 2081, in update_vnet_gateway
private_ip_allocation_method='Dynamic', name='vnetGatewayConfig{}'.format(i))
TypeError: init() got multiple values for argument 'private_ip_allocation_method'

Thanks

@aanandr
Copy link

aanandr commented Dec 21, 2017 via email

@anzaman
Copy link
Member

anzaman commented Dec 21, 2017

@blackhu269 Can you please post the exact command that you used? Also, are you able to set it to active-active using the portal or PowerShell? We'll check the CLI and post the results.

@blackhu269
Copy link

@anzaman

  1. create the VPN GW with active-active through the Azure CLI.
    az group create --name TestRG2 --location chinanorth
    az network vnet create --name TestVNet1 --resource-group TestRG2 --address-prefix 10.11.0.0/16 --location chinanorth --subnet-name Subnet1 --subnet-prefix 10.11.0.0/24
    az network vnet subnet create --address-prefix 10.11.255.0/27 --name GatewaySubnet --resource-group TestRG2 --vnet-name TestVNet1
    az network public-ip create --name Internal-ip-2 --resource-group TestRG2 --allocation-method Dynamic
    az network public-ip create --name Internal-ip --resource-group TestRG2 --allocation-method Dynamic
    az network vnet-gateway create --name Internal-VPN --resource-group TestRG2 --vnet TestVNet1 --public-ip-addresses Internal-ip Internal-ip-2 --gateway-type Vpn --sku VpnGw1

  2. then update the VPN GW to active-standby.
    az network vnet-gateway update --name Internal-VPN --resource-group TestRG2 --public-ip-addresses Internal-ip

@anzaman
Copy link
Member

anzaman commented Dec 22, 2017

@blackhu269

Did you try to set it to active-standby using the portal or PowerShell? and what was the result? I am trying to isolate the issue and this would help narrow it.

@anzaman
Copy link
Member

anzaman commented Dec 22, 2017

@blackhu269

Seems like powershell is the only way to successfully change the mode of the gateway https://docs.microsoft.com/en-us/azure/vpn-gateway/vpn-gateway-activeactive-rm-powershell#a-name-aaupdateapart-4---update-existing-gateway-between-active-active-and-active-standby . We are looking into the issues wit CLI and the portal.

@blackhu269
Copy link

@anzaman
Thanks for your feedback. the portal or PowerShell are both already tested to change from Active-Active to Active-standby, and only the PowerShell worked well.

@haroldrandom haroldrandom added cla-not-required Network az network vnet/lb/nic/dns/etc... labels Oct 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-not-required Network az network vnet/lb/nic/dns/etc...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants