Add vpn client protocol support for virtual network gateway #946

dcaro · 2018-03-07T06:47:55Z

Added support for vpn client protocol and radius server with the following fields:
vpn_client_protocol
radius_server_address
radius_server_secret

katbyte

Hi @dcaro,

Thank you for adding this! I've taken a look over it and left some comments inline. Outside of that running the tests produces a none empty plan for me:

DIFF:
		
		UPDATE: azurerm_virtual_network_gateway.test
		  vpn_client_configuration.0.radius_server_address: "" => "1.2.3.4"
		  vpn_client_configuration.0.radius_server_secret:  "" => "verysecretsecret"
		
		STATE:

Look forward to getting this merged once these concerns are worked out 🙂

katbyte · 2018-05-21T02:52:49Z

azurerm/resource_arm_virtual_network_gateway.go

+							Type:     schema.TypeList,
+							Optional: true,
+							Elem: &schema.Schema{
+								Type: schema.TypeString,


Is it possible to get some validation of this property?

Good catch! I have added a value validation.

katbyte · 2018-05-21T02:54:40Z

azurerm/resource_arm_virtual_network_gateway.go

@@ -137,9 +137,20 @@ func resourceArmVirtualNetworkGateway() *schema.Resource {
 								Type: schema.TypeString,
 							},
 						},
+						"vpn_client_protocol": {


Could we name this vpn_client_protocols to match the SDK?

That makes sense, done.

katbyte · 2018-05-21T02:59:45Z

azurerm/resource_arm_virtual_network_gateway.go

@@ -137,9 +137,20 @@ func resourceArmVirtualNetworkGateway() *schema.Resource {
 								Type: schema.TypeString,
 							},
 						},
+						"vpn_client_protocol": {
+							Type:     schema.TypeList,


This should probably be a set unless order matters/duplicates are allowed

you are right, order does not matters, changed.

katbyte · 2018-05-21T03:04:02Z

azurerm/resource_arm_virtual_network_gateway.go

 						"root_certificate": {
 							Type:     schema.TypeSet,
-							Required: true,


What happens if neither root_certificate or radius_server_address are set? if that is an error condition we should use a CustomizeDiff to check.

If the root_certificate and radius_server_address are set, this is accepted by azure with no root cert and the vpnclient cannot be downloaded. I've added a CustomizeDiff so that if a radius_server_address is set is had to be a valid IPv4 address and the secret shall not be empty.

katbyte · 2018-05-21T03:15:14Z

azurerm/resource_arm_virtual_network_gateway_test.go

+				Config: config,
+				Check: resource.ComposeTestCheckFunc(
+					testCheckAzureRMVirtualNetworkGatewayExists("azurerm_virtual_network_gateway.test"),
+				),


Could we check here to see if the state is correct:

resource.TestCheckResourceAttr(resourceName, "key", "value"),

done, I have added couple of other attribute checks on basic and lowerCaseSubnetName tests as well.

…o TypeSet

dcaro

I have included the changes and added a regex validation for the IPv4 address using regex. Let me know if you have a preferred method.

dcaro · 2018-05-23T00:29:07Z

azurerm/resource_arm_virtual_network_gateway.go

@@ -137,9 +137,20 @@ func resourceArmVirtualNetworkGateway() *schema.Resource {
 								Type: schema.TypeString,
 							},
 						},
+						"vpn_client_protocol": {


That makes sense, done.

dcaro · 2018-05-23T00:45:35Z

azurerm/resource_arm_virtual_network_gateway.go

@@ -137,9 +137,20 @@ func resourceArmVirtualNetworkGateway() *schema.Resource {
 								Type: schema.TypeString,
 							},
 						},
+						"vpn_client_protocol": {
+							Type:     schema.TypeList,


you are right, order does not matters, changed.

dcaro · 2018-05-23T01:07:29Z

azurerm/resource_arm_virtual_network_gateway.go

+							Type:     schema.TypeList,
+							Optional: true,
+							Elem: &schema.Schema{
+								Type: schema.TypeString,


Good catch! I have added a value validation.

dcaro · 2018-05-28T06:25:29Z

azurerm/resource_arm_virtual_network_gateway.go

 						"root_certificate": {
 							Type:     schema.TypeSet,
-							Required: true,


If the root_certificate and radius_server_address are set, this is accepted by azure with no root cert and the vpnclient cannot be downloaded. I've added a CustomizeDiff so that if a radius_server_address is set is had to be a valid IPv4 address and the secret shall not be empty.

dcaro · 2018-06-02T03:24:43Z

azurerm/resource_arm_virtual_network_gateway_test.go

+				Config: config,
+				Check: resource.ComposeTestCheckFunc(
+					testCheckAzureRMVirtualNetworkGatewayExists("azurerm_virtual_network_gateway.test"),
+				),


done, I have added couple of other attribute checks on basic and lowerCaseSubnetName tests as well.

katbyte

Hi @dcaro,

Thank you for the updates. I've given this another look and left some comments inline.

katbyte · 2018-06-05T00:19:49Z

azurerm/resource_arm_virtual_network_gateway.go

@@ -11,6 +11,7 @@ import (
 	"github.com/hashicorp/terraform/helper/schema"
 	"github.com/hashicorp/terraform/helper/validation"
 	"github.com/terraform-providers/terraform-provider-azurerm/azurerm/utils"
+	"regexp"


Could we sort this and put regex above with the other built in packages?

sure, updated.

katbyte · 2018-06-06T00:56:28Z

azurerm/resource_arm_virtual_network_gateway.go

+	}
+	flat["vpn_client_protocols"] = vpnClientProtocols
+
+	flat["radius_server_address"] = *cfg.RadiusServerAddress


Could we check these for nil?

if v := cfg.RadiusServerAddress; if v != nil { flat["radius_server_address"] = *v }

katbyte · 2018-06-06T00:57:35Z

azurerm/resource_arm_virtual_network_gateway.go

+								"vpn_client_configuration.0.root_certificate",
+								"vpn_client_configuration.0.revoked_certificate",
+							},
+							Elem: &schema.Schema{


This doesn't apply to a TypeString

katbyte · 2018-06-06T00:57:41Z

azurerm/resource_arm_virtual_network_gateway.go

+								"vpn_client_configuration.0.root_certificate",
+								"vpn_client_configuration.0.revoked_certificate",
+							},
+							Elem: &schema.Schema{


This doesn't apply to a TypeString

katbyte · 2018-06-06T01:01:22Z

azurerm/resource_arm_virtual_network_gateway.go

+		if vpnClientConfig, ok := vpnClient.([]interface{})[0].(map[string]interface{}); ok {
+			if vpnClientConfig["radius_server_address"] != "" {
+				if !validIP4(vpnClientConfig["radius_server_address"].(string)) {
+					return fmt.Errorf("radius_server_address is not an IPv4 address")


This validation can be done in the schema, for example:

ValidateFunc: validation.StringMatch( regexp.MustCompile("^[-a-z0-9]{3,50}$"), "Cosmos DB Account name must be 3 - 50 characters long, contain only letters, numbers and hyphens.", ),

The schema is indeed a better place to do this check.

katbyte · 2018-06-06T01:02:29Z

azurerm/resource_arm_virtual_network_gateway.go

+				if !validIP4(vpnClientConfig["radius_server_address"].(string)) {
+					return fmt.Errorf("radius_server_address is not an IPv4 address")
+				}
+				if vpnClientConfig["radius_server_secret"] == "" {


as above, could be validated by schema & regex

katbyte · 2018-06-06T01:06:26Z

azurerm/resource_arm_virtual_network_gateway.go

+
+	if vpnClient, ok := diff.GetOk("vpn_client_configuration"); ok {
+		if vpnClientConfig, ok := vpnClient.([]interface{})[0].(map[string]interface{}); ok {
+			if vpnClientConfig["radius_server_address"] != "" {


I think what you want to do here is check to make sure one or the other is set as mentioned above?

as in:

_, hasRadius := vpnClientConfig["radius_server_address"] _, hasRootCert := vpnClientConfig["root_certificate"] if !hasRadius && !hasRootCert { #error } if hasRadius { if _, hasRadiusSecret := vpnClientConfig["radius_server_secret"]; !hasRadiusSecret { #error } }

With this hasRadius and hasRootCert are set to true. I have changed a bit the logic to check if both radius server address and value are set. If this neither of them are set, we can assume that we are using the certificate method and nil is accepted.

katbyte · 2018-06-06T01:07:56Z

azurerm/resource_arm_virtual_network_gateway.go

+	return nil
+}
+
+func validIP4(ipAddress string) bool {


could we rename this to validateIP4?

removed since this is done in the schema.

katbyte · 2018-06-06T01:09:12Z

azurerm/resource_arm_virtual_network_gateway_test.go

@@ -269,11 +276,20 @@ resource "azurerm_virtual_network_gateway" "test" {
    private_ip_address_allocation = "Dynamic"
    subnet_id = "${azurerm_subnet.test.id}"
  }
+
+  vpn_client_configuration {


Could we add an entirely new test for these properties instead of adding them to an existing test?

sure, done.

…unction and customizeDiff, added new test

dcaro · 2018-06-14T19:30:05Z

I have just pushed an update with the changes that you have suggested. Let me know if there are any other points. Thanks for the review.

…onfig

katbyte

Thanks for the changes @dcaro! This LGTM now 👍

katbyte · 2018-06-15T06:51:39Z

Tests pass aside from the usual flakiness:

Phydeauxman · 2018-07-03T23:48:35Z

I am trying to use the new vpn_client_protocol argument and it is not working. I am running Terraform v0.11.7 and provider.azurerm v1.8.0. The output I am getting from plan is:

Error: azurerm_virtual_network_gateway.sdvnet: vpn_client_configuration.0: invalid or unknown key: vpn_client_protocol

…lient_protocols`, introduced to the resource in #946 Fixes #1496

ghost · 2020-03-30T15:25:26Z

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!

Damien Caro added 2 commits March 6, 2018 22:37

Added support for vpn client protocol and radius server

e10f10a

Added validation of values

b2d5a48

dcaro changed the title ~~Add vpn client protocol~~ Add vpn client protocol support for virtual network gateway Mar 7, 2018

Removed validation function on list

a324d11

tombuildsstuff added enhancement service/virtual-network-gateway labels Mar 7, 2018

tombuildsstuff added this to the 1.3.1 milestone Mar 14, 2018

tombuildsstuff modified the milestones: 1.3.1, 1.3.2 Mar 28, 2018

tombuildsstuff modified the milestones: 1.3.2, 1.3.3 Apr 4, 2018

tombuildsstuff modified the milestones: 1.3.3, 1.4.0 Apr 17, 2018

tombuildsstuff modified the milestones: 1.4.0, 1.5.0 Apr 25, 2018

tombuildsstuff modified the milestones: 1.5.0, 1.6.0 May 8, 2018

katbyte requested changes May 21, 2018

View reviewed changes

tombuildsstuff modified the milestones: 1.6.0, 1.7.0 May 21, 2018

dcaro added 2 commits May 23, 2018 11:05

fixed var name and typeset

ab856d3

resolving merge conflict

fb68e95

tombuildsstuff added the waiting-response label Jun 1, 2018

added CustomizeDiff, validation on vpn_client_protocols and changed t…

33fb164

…o TypeSet

dcaro commented Jun 2, 2018

View reviewed changes

formatted test code with gofmt

eda1b59

katbyte requested changes Jun 6, 2018

View reviewed changes

katbyte modified the milestones: 1.7.0, Soon Jun 12, 2018

Order packages, checked vpn_client values for nil, updated validate f…

d085c60

…unction and customizeDiff, added new test

dcaro added 4 commits June 14, 2018 12:36

reformat test

195dc2d

updated error messages on customizeDiff and use bool

de4740e

added dependency on activeActive test

b450977

added dependency on public_ip for the vnet_gateway for the vpnClientC…

d4d30c3

…onfig

katbyte approved these changes Jun 15, 2018

View reviewed changes

katbyte merged commit 66d9de4 into hashicorp:master Jun 15, 2018

katbyte modified the milestones: Soon, 1.7.0 Jun 15, 2018

katbyte added a commit that referenced this pull request Jun 15, 2018

Update CHANGELOG.md to include #946

13428fa

Phydeauxman mentioned this pull request Jul 3, 2018

azurerm_virtual_network_gateway argument vpn_client_protocol not recognized #1485

Closed

tombuildsstuff added a commit that referenced this pull request Jul 6, 2018

Supporting radius_server_address, radius_server_secret and `vpn_c…

e77d399

…lient_protocols`, introduced to the resource in #946 Fixes #1496

tombuildsstuff mentioned this pull request Jul 6, 2018

Virtual Network Gateway - fixing a set error #1505

Merged

ghost locked and limited conversation to collaborators Mar 30, 2020

Add vpn client protocol support for virtual network gateway #946

Add vpn client protocol support for virtual network gateway #946

Conversation

dcaro commented Mar 7, 2018

katbyte 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

dcaro 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

katbyte 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

dcaro commented Jun 14, 2018

katbyte left a comment

Choose a reason for hiding this comment

katbyte commented Jun 15, 2018

Phydeauxman commented Jul 3, 2018

ghost commented Mar 30, 2020