provider/azurerm: Add resources virtual network gateway and connection #13886

dominik-lekse · 2017-04-23T21:02:20Z

This pull request adds the resources azurerm_virtual_network_gateway and azurerm_virtual_network_gateway_connection which can be used to manage Azure VPN Gateways and Connections.

In pull request #9255, @pmcatominey provided the major part the resource azurerm_virtual_network_gateway implementation. This pull request bases on the top of this and contains the original commits of #9255. In addition, the resource azurerm_virtual_network_gateway has been completed and implementation and tests of the resource azurerm_virtual_network_gateway_connection have been added. Further, documentation for both resources is included.

Please consider the following notes when reviewing:

Expect the resource azurerm_virtual_network_gateway to run for a while since "creating a gateway can take a long time to complete. Often 45 minutes or more." (see https://docs.microsoft.com/en-us/azure/vpn-gateway/vpn-gateway-create-site-to-site-rm-powershell#a-namecreategatewaya6-create-the-virtual-network-gateway). The Terraform resource polls for the status until the virtual network gateways is fully provisioned and can be used with other resources such as connections.
In [WIP] provider/azurerm: virtual_network gateway and connection #9255, @pmcatominey mentioned a bug of the Azure management API when destroying a gateway subnet which has been associated with a virtual network gateway which has been successfully destroyed seconds before. I can confirm that the bug is still present, i.e. that such a gateway subnet cannot be deleted for a couple of minutes. Unfortunately, the API does not expose a status on such a subnet which we could use to distinguish. Instead of waiting for a fixed length of time at the end of destroying a virtual network gateway, I have implemented an alternative solution. When a gateway subnet cannot be deleted with a response which occurs when an associated virtual network gateway has been removed previously, Terraform will retry the deletion for a fixed period of time. In my opinion, this workaround is reasonable although it has to be implemented in the subnet resource.
Both resources should support Azure ExpressRoute functionality, however this is not part of the test coverage since I do not have access to an Azure subscription which is ready for this.

Acceptance tests:

TestAccAzureRMVirtualNetworkGateway_basic: Create a virtual network gateway and destroys it
TestAccAzureRMVirtualNetworkGatewayConnection_sitetosite: Creates a site-to-site connection and destroys it
TestAccAzureRMVirtualNetworkGatewayConnection_vnettovnet: Creates virtual network gateways in two Azure regions and connects them using a vnet-to-vnet connection
TestAccAzureRMVirtualNetworkGateway_importBasic: Verifies import of a virtual network gateway
TestAccAzureRMVirtualNetworkGatewayConnection_importSiteToSite: Verifies import of a virtual network gateway connection

=== RUN   TestAccAzureRMVirtualNetworkGateway_basic
--- PASS: TestAccAzureRMVirtualNetworkGateway_basic (2190.70s)
=== RUN   TestAccAzureRMVirtualNetworkGateway_importBasic
--- PASS: TestAccAzureRMVirtualNetworkGateway_importBasic (2280.54s)
=== RUN   TestAccAzureRMVirtualNetworkGatewayConnection_sitetosite
--- PASS: TestAccAzureRMVirtualNetworkGatewayConnection_sitetosite (2401.24s)
=== RUN   TestAccAzureRMVirtualNetworkGatewayConnection_importSiteToSite
--- PASS: TestAccAzureRMVirtualNetworkGatewayConnection_importSiteToSite (2664.22s)
=== RUN   TestAccAzureRMVirtualNetworkGatewayConnection_vnettovnet
--- PASS: TestAccAzureRMVirtualNetworkGatewayConnection_vnettovnet (2480.51s)

Open tasks:

Successful create and delete resources using azurerm_virtual_network_gateway and azurerm_virtual_network_gateway_connection in a scenario
Implement test for azurerm_virtual_network_gateway (TestAccAzureRMVirtualNetworkGateway_basic)
Implement import test for azurerm_virtual_network_gateway (TestAccAzureRMVirtualNetworkGateway_importBasic)
Documentation for azurerm_virtual_network_gateway
Implement test for azurerm_virtual_network_gateway_connection to test site-to-site connection (TestAccAzureRMVirtualNetworkGatewayConnection_sitetosite)
Implement test for azurerm_virtual_network_gateway_connection to test vnet-to-vnet connection (TestAccAzureRMVirtualNetworkGatewayConnection_vnettovnet)
Implement import test for azurerm_virtual_network_gateway_connection (TestAccAzureRMVirtualNetworkGatewayConnection_importSiteToSite)
Documentation for azurerm_virtual_network_gateway_connection
Review

This pull request will resolve the issues, #8372, #10634 and includes the pull request #9255. It would be great to get feedback from you waiting for these resources.

// Dominik

…m_virtual_network_gateway_connection`

anniehedgpeth · 2017-05-04T16:20:48Z

Hey @tombuildsstuff, any word on when this will be merged? I have a new template that requires this resource. :) Thanks!

# Conflicts: # builtin/providers/azurerm/provider.go

dominik-lekse · 2017-05-04T22:15:51Z

Just resolved recent conflicts with master.

I assume the Terraform team has fixed their sprint goal for the current release cycle and there are also lots of other community pull requests which consume review time.

Would appreciate if this could fit in one of the upcoming release cycles.

tombuildsstuff

Hi @dominik-lekse

Thanks for opening this PR - apologies for the delay in reviewing this. I've taken a look through and left some comments inline - and also opened a bug on the Azure Rest API Specs Repository about the broken API.

After chatting internally, until the API is fixed - would it be possible to investigate containing this logic within the Virtual Network Gateway resource, rather than within the Subnet resource (as recently we've discovered some resources which can only be modified separately, rather than in parallel). There's a couple of different approaches here, either by utilising a State Func with a ContinuousTargetOccurence set - or to see if the Virtual Network Gateway is visible through the either Virtual Network Gateway's List method / Resource Group's contents?

Thanks!

tombuildsstuff · 2017-05-16T09:44:52Z

builtin/providers/azurerm/resource_arm_virtual_network_gateway.go

+								string(network.VirtualNetworkGatewaySkuTierBasic),
+								string(network.VirtualNetworkGatewaySkuTierStandard),
+								string(network.VirtualNetworkGatewaySkuTierHighPerformance),
+								string(network.VirtualNetworkGatewaySkuTierUltraPerformance),


Same as above, however if these fields are going to be the same value is it worth consolidating this on a single field for now?

tombuildsstuff · 2017-05-16T09:44:56Z

builtin/providers/azurerm/resource_arm_virtual_network_gateway.go

+								string(network.VirtualNetworkGatewaySkuNameBasic),
+								string(network.VirtualNetworkGatewaySkuNameStandard),
+								string(network.VirtualNetworkGatewaySkuNameHighPerformance),
+								string(network.VirtualNetworkGatewaySkuNameUltraPerformance),


Comparing the API documentation with the Portal - it appears that whilst the API supports Ultra, it's only supported for ExpressRoute's, rather than Virtual Network Gateway's:

As such - is it worth removing Ultra from this list for the moment?

As an alternative, I would add a validation function which cross checks gateway type and sku. Would that be fine?

tombuildsstuff · 2017-05-16T09:53:24Z

builtin/providers/azurerm/resource_arm_local_network_gateway.go

+	return resGroup, name, nil
+}
+
+func retrieveLocalNetworkGatewayById(localNetworkGatewayId string, meta interface{}) (*network.LocalNetworkGateway, bool, error) {


from what I can see, I don't believe this method's being used, as such can we remove it?

tombuildsstuff · 2017-05-16T10:32:08Z

builtin/providers/azurerm/resource_arm_virtual_network_gateway.go

+
+			"enable_bgp": {
+				Type:     schema.TypeBool,
+				Optional: true,


I believe this also wants to be Computed: true?

tombuildsstuff · 2017-05-16T10:34:14Z

builtin/providers/azurerm/resource_arm_virtual_network_gateway.go

+			"active_active": {
+				Type:     schema.TypeBool,
+				Optional: true,
+			},


I believe this also wants to be Computed: true?

tombuildsstuff · 2017-05-16T12:24:52Z