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

Add resources azurerm_virtual_network_gateway and azurerm_virtual_network_gateway_connection #133

Merged

Conversation

dominik-lekse
Copy link
Contributor

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.

It has been migrated from the pull request hashicorp/terraform#13886 in the original terraform repository after provider separation.

@dominik-lekse
Copy link
Contributor Author

@tombuildsstuff Please consider that I commented your review comments in the original pull request #13886 and not in this one. Most comments have been addressed, however the question how to properly handle the subnet deletion issue remains open.

@veeresh1982
Copy link

@dominik-lekse : Just wanted to check any ETA when will this be pushed to the master branch. We have a requirement and want to use this new resource feature. Is it OK now to push the current changes to the master and improve upon the subnet deletion issue later as a new PR ?
Let us know your thoughts!

@tombuildsstuff
Copy link
Contributor

Hey @veeresh1982

Just wanted to check any ETA when will this be pushed to the master branch. We have a requirement and want to use this new resource feature. Is it OK now to push the current changes to the master and improve upon the subnet deletion issue later as a new PR ?

Unfortunately we're in a holding pattern with this PR until the underlying Azure API is fixed - as the API's returning that the Virtual Network Gateway's been deleted when it's not. There's an Github issue tracking the bug here - and I've commented asking for an update / rough timeline for this being fixed.

Whilst we could potentially merge this in now, it'd give end users random failures when applying their Terraform configs - which isn't an optimal user experience. In the interim - you should be able to provision a Virtual Network Gateway using the azurerm_template_deployment resource - which (whilst isn't ideal) does allow for this resource to be provisioned via Terraform until this is natively supported.

Thanks!

@dominik-lekse
Copy link
Contributor Author

Hi @tombuildsstuff

Thanks for the status update on this. Besides the subnet deletion issue, did you have a chance to take a look at the changes which have been applied in accordance to your last review?

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

Hey @dominik-lekse

Thanks for porting this PR over to the new repo - apologies for the delay in reviewing this :)

I've taken another look through this PR and it's looking good - most of the points I've raised are about consistency with other resources. Unfortunately I've not heard back about the Subnet deletion issue yet - as such I'm going to reach out via some backchannels and see if I can at least get a rough estimate of when this'll be fixed, so we know how to proceed :)

Thanks!

resourceName := "azurerm_virtual_network_gateway_connection.test"

ri := acctest.RandInt()
config := fmt.Sprintf(testAccAzureRMVirtualNetworkGatewayConnection_sitetosite, ri)
Copy link
Contributor

Choose a reason for hiding this comment

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

minor could we switch this over to a function returning a formatted string to match the newer resources?

if resp.StatusCode == http.StatusNotFound {
return nil, false, nil
}
return nil, false, fmt.Errorf("Error making Read request on Azure LocalNetworkGateway %s: %s", name, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

it'd be good to return the full error by making the second argument %+v

// Retry deletion of gateway subnet if provisioning state is failed
if *resp.SubnetPropertiesFormat.ProvisioningState == "Failed" {
log.Printf("[DEBUG] Retry deleting Gateway Subnet %s/%s after failed provisioning state.", vnetName, name)
subnetClient.Delete(resGroup, vnetName, name, make(chan struct{}))
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to check the result of this call? in addition - do we need to implement some kind of "max attempts" here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This workaround should not be relevant anymore.

Timeout: 15 * time.Minute,
}
if _, err := stateConf.WaitForState(); err != nil {
return fmt.Errorf("Error waiting for Gateway Subnet %s/%s to be removed: %s", vnetName, name, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

can this last argument become %+v?


"default_local_network_gateway_id": {
Type: schema.TypeString,
Optional: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

should this also be computed?


* `express_route_circuit_id` - (Optional) The full Azure resource ID of the
Express Route Circuit when creating an ExpressRoute connection. The
Express Route Circuit can be in the same or in a different subscription.
Copy link
Contributor

Choose a reason for hiding this comment

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

We'd normally phrase this as "The ID of the Express Route Circuit"

* `local_network_gateway_id` - (Optional) The full Azure resource ID of the
local network gateway when creating Site-to-Site connection.

* `routing_weight` - (Optional) The routing weight. The default value is 10.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we quote 10 here as it's the value?

The peer virtual network gateway can be in the same or in a different subscription.

* `local_network_gateway_id` - (Optional) The full Azure resource ID of the
local network gateway when creating Site-to-Site connection.
Copy link
Contributor

Choose a reason for hiding this comment

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

We'd normally phrase this as "The Local Network Gateway ID"


* `peer_virtual_network_gateway_id` - (Optional) The full Azure resource ID
of the peer virtual network gateway when creating a VNet-to-VNet connection.
The peer virtual network gateway can be in the same or in a different subscription.
Copy link
Contributor

Choose a reason for hiding this comment

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

We'd normally phase this as "The ID of the Peer Virtual Network Gateway"

* `shared_key` - (Optional) The shared IPSec key.

* `enable_bgp` - (Optional) If true, BGP (Border Gateway Protocol) is enabled
for this connection. By default, BGP is disabled.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we quote the values and change the last sentence to "Defaults to false (disabled)" to match the other resources?

@KandrewC
Copy link

KandrewC commented Aug 3, 2017

I'm taking a look at this PR right now and trying to build the provider, how would I pull it down and run it locally?

@tombuildsstuff
Copy link
Contributor

Hey @KandrewC

I'm taking a look at this PR right now and trying to build the provider, how would I pull it down and run it locally?

There's instructions on the README for how to build and run this - however bear in mind PR's aren't fully tested and may not necessarily be 100% bug free. As such in the interim we'd recommend using the azurerm_template_deployment resource to provision Virtual Network Gateway's at this point in time for Production usage.

Thanks!

# Conflicts:
#	azurerm/provider.go
#	website/azurerm.erb
…_network_gateway_connection based on latest review
…d TestAccAzureRMVirtualNetworkGatewayConnection_importSiteToSite
@dominik-lekse
Copy link
Contributor Author

@tombuildsstuff Thanks for the review, in the the latest changes I have addressed most of your comments.

Regarding the subnet deletion issue, I have another idea for a workaround which I wanted to discuss before implementing: In resourceArmVirtualNetworkGatewayDelete after deleting the gateway resource we could also delete the associated gateway subnet and recreate it immediately afterwards with the same parameters. This should not interfer with other resources since there is a one-to-one-relationship between the gateway subnet and the gateway. Further, we could take out the special gateway subnet retry deletion handling from resourceArmSubnetDelete. Do you think this could be a workaround to allow this pull request to be merged before Microsoft finds and fixes the problem?

@StephenThomson
Copy link

Looks great. I'm looking forward to it being merged 👍

@dominik-lekse
Copy link
Contributor Author

@tombuildsstuff As @MohitGargVpn announced that the fix for the subnet deletion issue will be released in the Azure Api soon, I will remove the workaround from the subnet resource in this pull request and we wait for the acceptance tests to turn green.

@mynkow
Copy link

mynkow commented Aug 22, 2017

Is there a beta where I can test this?

@nbering
Copy link

nbering commented Aug 22, 2017

@mynkow You could checkout the pull request and build it yourself. Not trivial by any means, but not terribly difficult, either.

There are build instructions in the project readme, and and little bit about pulling in custom your own plugin binaries in the Terraform docs under Running Terraform in Automation. Should really be it's own section and cross-linked from plugin development, but if I have time I'll raise an issue about that.

@fstuck37
Copy link

fstuck37 commented Sep 6, 2017

Hi All,
Any update on when this will be available?
Working with the null_resource and the az CLI as of now but would much rather have both virtual_network_gateway and azurerm_virtual_network_gateway_connection available.

Thanks and your work on this is very much appreciated,
Fred

@tombuildsstuff
Copy link
Contributor

@fstuck37 we're still waiting on Microsoft to fix the API which is scheduled for the end of next week, we'll take another look after that :)

# Conflicts:
#	azurerm/provider.go
#	azurerm/resource_arm_subnet.go
#	azurerm/validators.go
@tombuildsstuff
Copy link
Contributor

@dominik-lekse not yet - I've been working through the arm folder that's vendored in in reverse order - so I'd expect to Networking later at some point in the next few days - I'll let you know when that's done (and apologies for the delay here!)

# Conflicts:
#	azurerm/provider.go
#	azurerm/resource_arm_local_network_gateway.go
…irtual_network_gateway_connection to the v12 networking sdk
@dominik-lekse
Copy link
Contributor Author

@tombuildsstuff With the recent migration to the v12 networking sdk in #738, I also migrated the resources of this pull request.

Finally the good news is, that the eventual consistency bug in the Azure API is fixed and that the v12 SDK handles the return code in the correct way. Up to now, I only run the test TestAccAzureRMVirtualNetworkGateway_basic targeting the West Europe region which finished without the errors we have seen in the past.

=== RUN   TestAccAzureRMVirtualNetworkGateway_basic
--- PASS: TestAccAzureRMVirtualNetworkGateway_basic (2107.51s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	2107.532s

Running the other tests is pending since each test takes ~45 minutes for the virtual network gateway to create. Could you take over and run these tests on the HashiCorp CI?

@tombuildsstuff
Copy link
Contributor

@dominik-lekse awesome - that's great news! Sure - I'll kick off the test suite now :)

Thanks!

@tombuildsstuff
Copy link
Contributor

@dominik-lekse all the tests are passing 🎉 I'll take another look through this shortly, but that's really exciting :)

screen shot 2018-01-24 at 15 44 04

@tombuildsstuff tombuildsstuff modified the milestones: 1.0.3, 1.0.2 Jan 24, 2018
@dominik-lekse
Copy link
Contributor Author

Sounds great, of course I expected this ;)

Since the original development of these provider resources, Azure has added support for custom IPsec/IKE policies (see https://docs.microsoft.com/en-us/azure/vpn-gateway/vpn-gateway-ipsecikepolicy-rm-powershell). I suggest to add them in a separate pull request for the sake of closing this one.

Adding the custom policies to the provider would be great since this is up to now neither supported by the Portal nor by the official Azure CLI.

@dominik-lekse
Copy link
Contributor Author

Resolved the conflicts with the previous commit. Ready to merge in my view.

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hey @dominik-lekse

Thanks for pushing those updates (and resolving the conflicts) - I've spent some time testing this and have pushed some commits which resolve the issues I've pointed out below and add some extra test cases (I hope you don't mind!) - but this now LGTM.

Since the original development of these provider resources, Azure has added support for custom IPsec/IKE policies (see https://docs.microsoft.com/en-us/azure/vpn-gateway/vpn-gateway-ipsecikepolicy-rm-powershell). I suggest to add them in a separate pull request for the sake of closing this one.

Yeah definitely - I'll open another issue to keep track of that

Thanks for all the work on the Virtual Network Gateway (+ Connection) Resources, it's great to see this ship :)

Thanks!

import (
"github.com/hashicorp/terraform/helper/acctest"
"github.com/hashicorp/terraform/helper/resource"
"testing"
Copy link
Contributor

Choose a reason for hiding this comment

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

minor could we sort the imports here?

import (
"github.com/hashicorp/terraform/helper/acctest"
"github.com/hashicorp/terraform/helper/resource"
"testing"
Copy link
Contributor

Choose a reason for hiding this comment

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

minor could we sort the imports here?

},

"vpn_client_configuration": {
Type: schema.TypeSet,
Copy link
Contributor

Choose a reason for hiding this comment

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

in testing this - it appears this can become a List - I'll push a commit for this

},

"bgp_settings": {
Type: schema.TypeSet,
Copy link
Contributor

Choose a reason for hiding this comment

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

from testing it appears we can make this a List - I'll push a commit for this

@tombuildsstuff
Copy link
Contributor

Virtual Network Gateway & Virtual Network Gateway Connection tests pass:

screen shot 2018-01-26 at 12 22 33

@tombuildsstuff tombuildsstuff merged commit b90ee36 into hashicorp:master Jan 26, 2018
tombuildsstuff added a commit that referenced this pull request Jan 26, 2018
@dominik-lekse
Copy link
Contributor Author

@tombuildsstuff 👏

Thanks for the improvements and merging the resources.

@ghost
Copy link

ghost commented Mar 31, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.