-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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 vpn client protocol support for virtual network gateway #946
Changes from 7 commits
e10f10a
b2d5a48
a324d11
ab856d3
fb68e95
33fb164
eda1b59
d085c60
195dc2d
de4740e
b450977
d4d30c3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
) | ||
|
||
func resourceArmVirtualNetworkGateway() *schema.Resource { | ||
|
@@ -23,6 +24,8 @@ func resourceArmVirtualNetworkGateway() *schema.Resource { | |
State: schema.ImportStatePassthrough, | ||
}, | ||
|
||
CustomizeDiff: resourceArmVirtualNetworkGatewayCustomizeDiff, | ||
|
||
Schema: map[string]*schema.Schema{ | ||
"name": { | ||
Type: schema.TypeString, | ||
|
@@ -138,9 +141,25 @@ func resourceArmVirtualNetworkGateway() *schema.Resource { | |
Type: schema.TypeString, | ||
}, | ||
}, | ||
"vpn_client_protocols": { | ||
Type: schema.TypeSet, | ||
Optional: true, | ||
Elem: &schema.Schema{ | ||
Type: schema.TypeString, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to get some validation of this property? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch! I have added a value validation. |
||
ValidateFunc: validation.StringInSlice([]string{ | ||
string(network.IkeV2), | ||
string(network.SSTP), | ||
}, true), | ||
}, | ||
}, | ||
"root_certificate": { | ||
Type: schema.TypeSet, | ||
Required: true, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if neither There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
Optional: true, | ||
|
||
ConflictsWith: []string{ | ||
"vpn_client_configuration.0.radius_server_address", | ||
"vpn_client_configuration.0.radius_server_secret", | ||
}, | ||
Elem: &schema.Resource{ | ||
Schema: map[string]*schema.Schema{ | ||
"name": { | ||
|
@@ -158,6 +177,10 @@ func resourceArmVirtualNetworkGateway() *schema.Resource { | |
"revoked_certificate": { | ||
Type: schema.TypeSet, | ||
Optional: true, | ||
ConflictsWith: []string{ | ||
"vpn_client_configuration.0.radius_server_address", | ||
"vpn_client_configuration.0.radius_server_secret", | ||
}, | ||
Elem: &schema.Resource{ | ||
Schema: map[string]*schema.Schema{ | ||
"name": { | ||
|
@@ -172,6 +195,28 @@ func resourceArmVirtualNetworkGateway() *schema.Resource { | |
}, | ||
Set: hashVirtualNetworkGatewayRevokedCert, | ||
}, | ||
"radius_server_address": { | ||
Type: schema.TypeString, | ||
Optional: true, | ||
ConflictsWith: []string{ | ||
"vpn_client_configuration.0.root_certificate", | ||
"vpn_client_configuration.0.revoked_certificate", | ||
}, | ||
Elem: &schema.Schema{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't apply to a |
||
Type: schema.TypeString, | ||
}, | ||
}, | ||
"radius_server_secret": { | ||
Type: schema.TypeString, | ||
Optional: true, | ||
ConflictsWith: []string{ | ||
"vpn_client_configuration.0.root_certificate", | ||
"vpn_client_configuration.0.revoked_certificate", | ||
}, | ||
Elem: &schema.Schema{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't apply to a |
||
Type: schema.TypeString, | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
|
@@ -493,12 +538,24 @@ func expandArmVirtualNetworkGatewayVpnClientConfig(d *schema.ResourceData) *netw | |
revokedCerts = append(revokedCerts, r) | ||
} | ||
|
||
var vpnClientProtocols []network.VpnClientProtocol | ||
for _, vpnClientProtocol := range conf["vpn_client_protocols"].(*schema.Set).List() { | ||
p := network.VpnClientProtocol(vpnClientProtocol.(string)) | ||
vpnClientProtocols = append(vpnClientProtocols, p) | ||
} | ||
|
||
confRadiusServerAddress := conf["radius_server_address"].(string) | ||
confRadiusServerSecret := conf["radius_server_secret"].(string) | ||
|
||
return &network.VpnClientConfiguration{ | ||
VpnClientAddressPool: &network.AddressSpace{ | ||
AddressPrefixes: &addresses, | ||
}, | ||
VpnClientRootCertificates: &rootCerts, | ||
VpnClientRevokedCertificates: &revokedCerts, | ||
VpnClientProtocols: &vpnClientProtocols, | ||
RadiusServerAddress: &confRadiusServerAddress, | ||
RadiusServerSecret: &confRadiusServerSecret, | ||
} | ||
} | ||
|
||
|
@@ -576,6 +633,17 @@ func flattenArmVirtualNetworkGatewayVpnClientConfig(cfg *network.VpnClientConfig | |
} | ||
flat["revoked_certificate"] = schema.NewSet(hashVirtualNetworkGatewayRevokedCert, revokedCerts) | ||
|
||
vpnClientProtocols := &schema.Set{F: schema.HashString} | ||
if vpnProtocols := cfg.VpnClientProtocols; vpnProtocols != nil { | ||
for _, protocol := range *vpnProtocols { | ||
vpnClientProtocols.Add(string(protocol)) | ||
} | ||
} | ||
flat["vpn_client_protocols"] = vpnClientProtocols | ||
|
||
flat["radius_server_address"] = *cfg.RadiusServerAddress | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we check these for if v := cfg.RadiusServerAddress; if v != nil {
flat["radius_server_address"] = *v
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added |
||
flat["radius_server_secret"] = *cfg.RadiusServerSecret | ||
|
||
return []interface{}{flat} | ||
} | ||
|
||
|
@@ -660,3 +728,35 @@ func validateArmVirtualNetworkGatewayExpressRouteSku() schema.SchemaValidateFunc | |
string(network.VirtualNetworkGatewaySkuTierUltraPerformance), | ||
}, true) | ||
} | ||
|
||
func resourceArmVirtualNetworkGatewayCustomizeDiff(diff *schema.ResourceDiff, v interface{}) error { | ||
|
||
if vpnClient, ok := diff.GetOk("vpn_client_configuration"); ok { | ||
if vpnClientConfig, ok := vpnClient.([]interface{})[0].(map[string]interface{}); ok { | ||
if vpnClientConfig["radius_server_address"] != "" { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
if !validIP4(vpnClientConfig["radius_server_address"].(string)) { | ||
return fmt.Errorf("radius_server_address is not an IPv4 address") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.",
), There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The schema is indeed a better place to do this check. |
||
} | ||
if vpnClientConfig["radius_server_secret"] == "" { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as above, could be validated by schema & regex |
||
return fmt.Errorf("radius_server_secret cannot be empty") | ||
} | ||
} else { | ||
if vpnClientConfig["radius_server_secret"] != "" { | ||
return fmt.Errorf("radius_server_secret cannot be set when radius_server_address is empty") | ||
} | ||
} | ||
} | ||
} | ||
return nil | ||
} | ||
|
||
func validIP4(ipAddress string) bool { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could we rename this to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed since this is done in the schema. |
||
ipAddress = strings.Trim(ipAddress, " ") | ||
|
||
re, _ := regexp.Compile(`^(([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\.){3}([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])$`) | ||
if re.MatchString(ipAddress) { | ||
return true | ||
} | ||
|
||
return false | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ import ( | |
|
||
func TestAccAzureRMVirtualNetworkGateway_basic(t *testing.T) { | ||
ri := acctest.RandInt() | ||
resourceName := "azurerm_virtual_network_gateway.test" | ||
config := testAccAzureRMVirtualNetworkGateway_basic(ri, testLocation()) | ||
|
||
resource.Test(t, resource.TestCase{ | ||
|
@@ -22,7 +23,8 @@ func TestAccAzureRMVirtualNetworkGateway_basic(t *testing.T) { | |
{ | ||
Config: config, | ||
Check: resource.ComposeTestCheckFunc( | ||
testCheckAzureRMVirtualNetworkGatewayExists("azurerm_virtual_network_gateway.test"), | ||
testCheckAzureRMVirtualNetworkGatewayExists(resourceName), | ||
resource.TestCheckResourceAttr(resourceName, "sku", "Basic"), | ||
), | ||
}, | ||
}, | ||
|
@@ -31,6 +33,7 @@ func TestAccAzureRMVirtualNetworkGateway_basic(t *testing.T) { | |
|
||
func TestAccAzureRMVirtualNetworkGateway_lowerCaseSubnetName(t *testing.T) { | ||
ri := acctest.RandInt() | ||
resourceName := "azurerm_virtual_network_gateway.test" | ||
config := testAccAzureRMVirtualNetworkGateway_lowerCaseSubnetName(ri, testLocation()) | ||
|
||
resource.Test(t, resource.TestCase{ | ||
|
@@ -41,7 +44,8 @@ func TestAccAzureRMVirtualNetworkGateway_lowerCaseSubnetName(t *testing.T) { | |
{ | ||
Config: config, | ||
Check: resource.ComposeTestCheckFunc( | ||
testCheckAzureRMVirtualNetworkGatewayExists("azurerm_virtual_network_gateway.test"), | ||
testCheckAzureRMVirtualNetworkGatewayExists(resourceName), | ||
resource.TestCheckResourceAttr(resourceName, "sku", "Basic"), | ||
), | ||
}, | ||
}, | ||
|
@@ -50,6 +54,7 @@ func TestAccAzureRMVirtualNetworkGateway_lowerCaseSubnetName(t *testing.T) { | |
|
||
func TestAccAzureRMVirtualNetworkGateway_vpnGw1(t *testing.T) { | ||
ri := acctest.RandInt() | ||
resourceName := "azurerm_virtual_network_gateway.test" | ||
config := testAccAzureRMVirtualNetworkGateway_vpnGw1(ri, testLocation()) | ||
|
||
resource.Test(t, resource.TestCase{ | ||
|
@@ -60,7 +65,9 @@ func TestAccAzureRMVirtualNetworkGateway_vpnGw1(t *testing.T) { | |
{ | ||
Config: config, | ||
Check: resource.ComposeTestCheckFunc( | ||
testCheckAzureRMVirtualNetworkGatewayExists("azurerm_virtual_network_gateway.test"), | ||
testCheckAzureRMVirtualNetworkGatewayExists(resourceName), | ||
resource.TestCheckResourceAttr(resourceName, "vpn_client_configuration.0.radius_server_address", "1.2.3.4"), | ||
resource.TestCheckResourceAttr(resourceName, "vpn_client_configuration.0.vpn_client_protocols.#", "2"), | ||
), | ||
}, | ||
}, | ||
|
@@ -269,11 +276,20 @@ resource "azurerm_virtual_network_gateway" "test" { | |
private_ip_address_allocation = "Dynamic" | ||
subnet_id = "${azurerm_subnet.test.id}" | ||
} | ||
|
||
vpn_client_configuration { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we add an entirely new test for these properties instead of adding them to an existing test? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure, done. |
||
address_space = ["10.2.0.0/24"] | ||
vpn_client_protocols = ["SSTP", "IkeV2"] | ||
|
||
radius_server_address = "1.2.3.4" | ||
radius_server_secret = "1234" | ||
} | ||
} | ||
`, rInt, location, rInt, rInt, rInt) | ||
} | ||
|
||
func testAccAzureRMVirtualNetworkGateway_activeActive(rInt int, location string) string { | ||
|
||
return fmt.Sprintf(` | ||
resource "azurerm_resource_group" "test" { | ||
name = "acctestRG-%d" | ||
|
@@ -294,6 +310,7 @@ resource "azurerm_subnet" "test" { | |
address_prefix = "10.0.1.0/24" | ||
} | ||
|
||
|
||
resource "azurerm_public_ip" "first" { | ||
name = "acctestpip1-%d" | ||
location = "${azurerm_resource_group.test.location}" | ||
|
@@ -303,6 +320,7 @@ resource "azurerm_public_ip" "first" { | |
|
||
resource "azurerm_public_ip" "second" { | ||
name = "acctestpip2-%d" | ||
|
||
location = "${azurerm_resource_group.test.location}" | ||
resource_group_name = "${azurerm_resource_group.test.name}" | ||
public_ip_address_allocation = "Dynamic" | ||
|
@@ -316,16 +334,19 @@ resource "azurerm_virtual_network_gateway" "test" { | |
type = "Vpn" | ||
vpn_type = "RouteBased" | ||
sku = "VpnGw1" | ||
|
||
active_active = true | ||
enable_bgp = true | ||
|
||
ip_configuration { | ||
name = "gw-ip1" | ||
public_ip_address_id = "${azurerm_public_ip.first.id}" | ||
|
||
private_ip_address_allocation = "Dynamic" | ||
subnet_id = "${azurerm_subnet.test.id}" | ||
} | ||
|
||
|
||
ip_configuration { | ||
name = "gw-ip2" | ||
public_ip_address_id = "${azurerm_public_ip.second.id}" | ||
|
@@ -338,4 +359,5 @@ resource "azurerm_virtual_network_gateway" "test" { | |
} | ||
} | ||
`, rInt, location, rInt, rInt, rInt, rInt) | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we sort this and put regex above with the other built in packages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, updated.