Add support for custom IPsec/IKE policies for VPN connections #834

dominik-lekse · 2018-02-14T14:22:07Z

This pull requests adds support for custom IPsec/IKE policies for VPN connections. Also, policy based traffic selectors can now be enabled explicitly for a connection.

Tests

I have added a new test which covers a custom ipsec_policy and the use_policy_based_traffic_selectors switch.

TF_ACC=1 go test ./azurerm -v -run=TestAccAzureRMVirtualNetworkGatewayConnection_ipsecpolicy -timeout 180m
=== RUN   TestAccAzureRMVirtualNetworkGatewayConnection_ipsecpolicy
--- PASS: TestAccAzureRMVirtualNetworkGatewayConnection_ipsecpolicy (2401.33s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	2401.357s

References

Resolves Feature Request: support for custom IPsec/IKE policies #759

…teway_connection

tombuildsstuff

hey @dominik-lekse

Thanks for this PR - I've taken a look though and left some comments inline but this is mostly looking good. The comments I've left are mostly minor, the biggest question/concern being if if the default values have changed.

Thanks!

tombuildsstuff · 2018-02-19T20:30:13Z

azurerm/resource_arm_virtual_network_gateway_connection.go

@@ -79,13 +79,19 @@ func resourceArmVirtualNetworkGatewayConnection() *schema.Resource {
 			"enable_bgp": {
 				Type:     schema.TypeBool,
 				Optional: true,
-				Default:  false,
+				Computed: true,


is this same default returned from Azure?

Yes, false is the API default for enable_bgp. The intention for this change was to increase the transparency of the provider towards the API, i.e. allow the API to decide for the default. Unfortunately, the API docs are not quite elaborate on the default values (https://docs.microsoft.com/en-us/rest/api/network-gateway/virtualnetworkgatewayconnections/createorupdate).

tombuildsstuff · 2018-02-19T20:30:46Z

azurerm/resource_arm_virtual_network_gateway_connection.go

 			},

 			"routing_weight": {
 				Type:     schema.TypeInt,
 				Optional: true,
-				Default:  10,
+				Computed: true,


is this same default returned from Azure?

Same reasoning as above, 10 is the default value, but it should not be fixed in the provider.

tombuildsstuff · 2018-02-19T20:32:39Z

azurerm/resource_arm_virtual_network_gateway_connection.go

-	d.Set("routing_weight", conn.RoutingWeight)
-	d.Set("shared_key", conn.SharedKey)
+	if conn.EnableBgp != nil {
+		d.Set("enable_bgp", conn.EnableBgp)


do we want to dereference this field?

This is currently done in ResourceData.Set

tombuildsstuff · 2018-02-19T20:33:31Z

azurerm/resource_arm_virtual_network_gateway_connection.go

@@ -180,7 +289,9 @@ func resourceArmVirtualNetworkGatewayConnectionRead(d *schema.ResourceData, meta
 		d.Set("virtual_network_gateway_id", conn.VirtualNetworkGateway1.ID)
 	}

-	d.Set("authorization_key", conn.AuthorizationKey)
+	if conn.AuthorizationKey != nil {
+		d.Set("authorization_key", conn.AuthorizationKey)


providing this value is a string, we shouldn't need this check?

It is an optional field in the API, is it then also safe to avoid this check?

Since d.Set handles setting pointer values (even if they're nil - to an empty value) - it should be fine :)

tombuildsstuff · 2018-02-19T20:35:31Z

azurerm/resource_arm_virtual_network_gateway_connection.go

+
+	if conn.IpsecPolicies != nil {
+		ipsec_policies := flattenArmVirtualNetworkGatewayConnectionIpsecPolicies(conn.IpsecPolicies)
+		d.Set("ipsec_policy", ipsec_policies)


given this is a complex object, can we check for errors here via:

if err := d.Set("ipsec_policy", ipsec_policies); err != nil { return fmt.Errorf("Error setting `ipsec_policy`: %+v", err) }

minor could we also rename the variable ipsec_policies to ipsecPolicies to match the other variables?

Regarding the variable naming, I found it helpful to quickly identify the source of which the value originates. I.e. I chose CamelCase for values from the Azure Go SDK whereas snake_case for value from the Terraform schema.

However, to match the other resources, I will revert it.

tombuildsstuff · 2018-02-19T20:39:04Z

azurerm/resource_arm_virtual_network_gateway_connection_test.go

+}
+
+resource "azurerm_local_network_gateway" "test" {
+  name = "test-${var.random}"


(same here) can we change test -> acctest?

tombuildsstuff · 2018-02-19T20:39:12Z

azurerm/resource_arm_virtual_network_gateway_connection_test.go

+}
+
+resource "azurerm_virtual_network_gateway_connection" "test" {
+  name = "test-${var.random}"


(same here) can we change test -> acctest?

tombuildsstuff · 2018-02-19T20:40:25Z

website/docs/r/virtual_network_gateway_connection.html.markdown

+    or `None`.
+
+* `sa_datasize` - (Optional) The IPSec SA payload size in KB. Must be at least
+    `1024` KB. Defaults to `102400000` KB.


given the value can stretch so high, would it be worth converting this value to/from MB?

I would recommend to keep KBytes as a unit here for two reasons: 1) In the Azure docs this values is consistently described in this unit (https://docs.microsoft.com/en-us/azure/vpn-gateway/vpn-gateway-vpn-faq#ipsecike) and 2) it is quite common among VPN vendors to configure this in Kbytes (e.g https://www.cisco.com/c/en/us/td/docs/security/security_management/cisco_security_manager/security_manager/4-1/user/guide/CSMUserGuide_wrapper/vpipsec.html#pgfId-959357)

makes sense 👍

tombuildsstuff · 2018-02-19T20:42:23Z

azurerm/resource_arm_virtual_network_gateway_connection.go

+	if v, ok := d.GetOk("ipsec_policy"); ok {
+		ipsec_policies := v.([]interface{})
+		props.IpsecPolicies = expandArmVirtualNetworkGatewayConnectionIpsecPolicies(ipsec_policies)
+		log.Printf("Ipsec policies: %+q", props.IpsecPolicies)


can we remove this print statement?

Sure, this is a true left over 😄

tombuildsstuff · 2018-02-19T20:43:37Z

azurerm/resource_arm_virtual_network_gateway_connection.go

+func flattenArmVirtualNetworkGatewayConnectionIpsecPolicies(ipsecPolicies *[]network.IpsecPolicy) []interface{} {
+	ipsec_policies := make([]interface{}, 0, len(*ipsecPolicies))
+
+	for _, ipsecPolicy := range *ipsecPolicies {


there's a potential crash here if ipsecPolicies is nil - could we make this:

ipsec_policies := make([]interface{}, 0) if inputPolicies := ipsecPolicies; inputPolicies != nil { for _, ipsecPolicy := range *inputPolicies { # .. } }

Yes, I shouldn't rely on the caller here

stintel · 2018-03-01T00:08:05Z

Tested this PR and it works well for me. Would be nice to have this in the next release. Without a custom IPsec policy, an Azure VPN GW uses crypto algorithms that are no longer considered secure (3DES, SHA1, MODP1024 - see https://wiki.strongswan.org/projects/strongswan/wiki/SecurityRecommendations)

tombuildsstuff

hey @dominik-lekse

Thanks for pushing those changes - this now LGTM 👍 - I'll kick of the test suite now

Thanks!

tombuildsstuff · 2018-03-01T01:24:52Z

Tests pass:

dominik-lekse · 2018-03-01T07:50:02Z

@tombuildsstuff Thanks for merging 👍

stintel · 2018-03-01T10:06:27Z

@tombuildsstuff thanks for merging
@dominik-lekse thanks for your contribution

tombuildsstuff · 2018-03-02T17:30:10Z

👋 hey folks!

Just to let you know that support for this has just been released in v1.2.0 of the AzureRM Provider - full details of what's included are available here: https://github.com/terraform-providers/terraform-provider-azurerm/blob/v1.2.0/CHANGELOG.md#120-march-02-2018

Thanks!

ghost · 2020-03-31T15:27:12Z

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 👉 hashibot-feedback@hashicorp.com. Thanks!

Added support for custom ipsec policies in azurerm_virtual_network_ga…

fc1e15a

…teway_connection

tombuildsstuff added this to the 1.1.3 milestone Feb 16, 2018

tombuildsstuff added enhancement service/virtual-network-gateway labels Feb 19, 2018

tombuildsstuff suggested changes Feb 19, 2018

View reviewed changes

tombuildsstuff added the waiting-response label Feb 19, 2018

dominik-lekse added 2 commits February 20, 2018 22:13

Revision of custom IPsec/IKE policies after review

3025507

Revision of custom IPsec/IKE policies after review

e926237

tombuildsstuff approved these changes Mar 1, 2018

View reviewed changes

tombuildsstuff removed the waiting-response label Mar 1, 2018

tombuildsstuff merged commit 325df2e into hashicorp:master Mar 1, 2018

tombuildsstuff added a commit that referenced this pull request Mar 1, 2018

Updating to include #834

920b1e7

ghost locked and limited conversation to collaborators Mar 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for custom IPsec/IKE policies for VPN connections #834

Add support for custom IPsec/IKE policies for VPN connections #834

dominik-lekse commented Feb 14, 2018

tombuildsstuff left a comment

tombuildsstuff Feb 19, 2018

dominik-lekse Feb 20, 2018

tombuildsstuff Feb 19, 2018

dominik-lekse Feb 20, 2018

tombuildsstuff Feb 19, 2018

dominik-lekse Feb 20, 2018

tombuildsstuff Feb 19, 2018

dominik-lekse Feb 20, 2018

tombuildsstuff Feb 23, 2018

tombuildsstuff Feb 19, 2018

dominik-lekse Feb 20, 2018

tombuildsstuff Feb 19, 2018

tombuildsstuff Feb 19, 2018

tombuildsstuff Feb 19, 2018

dominik-lekse Feb 20, 2018

tombuildsstuff Feb 23, 2018

tombuildsstuff Feb 19, 2018

dominik-lekse Feb 20, 2018

tombuildsstuff Feb 19, 2018

dominik-lekse Feb 20, 2018

stintel commented Mar 1, 2018 •

edited

Loading

tombuildsstuff left a comment

tombuildsstuff commented Mar 1, 2018

dominik-lekse commented Mar 1, 2018

stintel commented Mar 1, 2018

tombuildsstuff commented Mar 2, 2018

ghost commented Mar 31, 2020

Add support for custom IPsec/IKE policies for VPN connections #834

Add support for custom IPsec/IKE policies for VPN connections #834

Conversation

dominik-lekse commented Feb 14, 2018

Tests

References

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stintel commented Mar 1, 2018 • edited Loading

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff commented Mar 1, 2018

dominik-lekse commented Mar 1, 2018

stintel commented Mar 1, 2018

tombuildsstuff commented Mar 2, 2018

ghost commented Mar 31, 2020

stintel commented Mar 1, 2018 •

edited

Loading