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.

Sign up for GitHub

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 support for custom IPsec/IKE policies for VPN connections #834

Merged
merged 3 commits into from
Mar 1, 2018
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fc1e15a
Added support for custom ipsec policies in azurerm_virtual_network_ga…
dominik-lekse Feb 14, 2018
3025507
Revision of custom IPsec/IKE policies after review
dominik-lekse Feb 20, 2018
e926237
Revision of custom IPsec/IKE policies after review
dominik-lekse Feb 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
237 changes: 225 additions & 12 deletions azurerm/resource_arm_virtual_network_gateway_connection.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,13 +79,19 @@ func resourceArmVirtualNetworkGatewayConnection() *schema.Resource {
"enable_bgp": {
Type: schema.TypeBool,
Optional: true,
Default: false,
Computed: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

is this same default returned from Azure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

},

"use_policy_based_traffic_selectors": {
Type: schema.TypeBool,
Optional: true,
Computed: true,
},

"routing_weight": {
Type: schema.TypeInt,
Optional: true,
Default: 10,
Computed: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

is this same default returned from Azure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

},

"shared_key": {
Expand All @@ -94,6 +100,109 @@ func resourceArmVirtualNetworkGatewayConnection() *schema.Resource {
Sensitive: true,
},

"ipsec_policy": {
Type: schema.TypeList,
Optional: true,
MaxItems: 1,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"dh_group": {
Type: schema.TypeString,
Required: true,
DiffSuppressFunc: ignoreCaseDiffSuppressFunc,
ValidateFunc: validation.StringInSlice([]string{
string(network.DHGroup1),
string(network.DHGroup14),
string(network.DHGroup2),
string(network.DHGroup2048),
string(network.DHGroup24),
string(network.ECP256),
string(network.ECP384),
string(network.None),
}, true),
},
"ike_encryption": {
Type: schema.TypeString,
Required: true,
DiffSuppressFunc: ignoreCaseDiffSuppressFunc,
ValidateFunc: validation.StringInSlice([]string{
string(network.AES128),
string(network.AES192),
string(network.AES256),
string(network.DES),
string(network.DES3),
}, true),
},
"ike_integrity": {
Type: schema.TypeString,
Required: true,
DiffSuppressFunc: ignoreCaseDiffSuppressFunc,
ValidateFunc: validation.StringInSlice([]string{
string(network.MD5),
string(network.SHA1),
string(network.SHA256),
string(network.SHA384),
}, true),
},
"ipsec_encryption": {
Type: schema.TypeString,
Required: true,
DiffSuppressFunc: ignoreCaseDiffSuppressFunc,
ValidateFunc: validation.StringInSlice([]string{
string(network.IpsecEncryptionAES128),
string(network.IpsecEncryptionAES192),
string(network.IpsecEncryptionAES256),
string(network.IpsecEncryptionDES),
string(network.IpsecEncryptionDES3),
string(network.IpsecEncryptionGCMAES128),
string(network.IpsecEncryptionGCMAES192),
string(network.IpsecEncryptionGCMAES256),
string(network.IpsecEncryptionNone),
}, true),
},
"ipsec_integrity": {
Type: schema.TypeString,
Required: true,
DiffSuppressFunc: ignoreCaseDiffSuppressFunc,
ValidateFunc: validation.StringInSlice([]string{
string(network.IpsecIntegrityGCMAES128),
string(network.IpsecIntegrityGCMAES192),
string(network.IpsecIntegrityGCMAES256),
string(network.IpsecIntegrityMD5),
string(network.IpsecIntegritySHA1),
string(network.IpsecIntegritySHA256),
}, true),
},
"pfs_group": {
Type: schema.TypeString,
Required: true,
DiffSuppressFunc: ignoreCaseDiffSuppressFunc,
ValidateFunc: validation.StringInSlice([]string{
string(network.PfsGroupECP256),
string(network.PfsGroupECP384),
string(network.PfsGroupNone),
string(network.PfsGroupPFS1),
string(network.PfsGroupPFS2),
string(network.PfsGroupPFS2048),
string(network.PfsGroupPFS24),
}, true),
},
"sa_datasize": {
Type: schema.TypeInt,
Optional: true,
Computed: true,
ValidateFunc: validation.IntAtLeast(1024),
},
"sa_lifetime": {
Type: schema.TypeInt,
Optional: true,
Computed: true,
ValidateFunc: validation.IntAtLeast(300),
},
},
},
},

"tags": tagsSchema(),
},
}
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

if conn.Peer != nil {
d.Set("express_route_circuit_id", conn.Peer.ID)
Expand All @@ -194,9 +305,26 @@ func resourceArmVirtualNetworkGatewayConnectionRead(d *schema.ResourceData, meta
d.Set("local_network_gateway_id", conn.LocalNetworkGateway2.ID)
}

d.Set("enable_bgp", conn.EnableBgp)
d.Set("routing_weight", conn.RoutingWeight)
d.Set("shared_key", conn.SharedKey)
if conn.EnableBgp != nil {
d.Set("enable_bgp", conn.EnableBgp)
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 want to dereference this field?

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 is currently done in ResourceData.Set

}

if conn.UsePolicyBasedTrafficSelectors != nil {
d.Set("use_policy_based_traffic_selectors", conn.UsePolicyBasedTrafficSelectors)
}

if conn.RoutingWeight != nil {
d.Set("routing_weight", conn.RoutingWeight)
}

if conn.SharedKey != nil {
d.Set("shared_key", conn.SharedKey)
}

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}

flattenAndSetTags(d, resp.Tags)

Expand Down Expand Up @@ -227,11 +355,9 @@ func resourceArmVirtualNetworkGatewayConnectionDelete(d *schema.ResourceData, me

func getArmVirtualNetworkGatewayConnectionProperties(d *schema.ResourceData) (*network.VirtualNetworkGatewayConnectionPropertiesFormat, error) {
connectionType := network.VirtualNetworkGatewayConnectionType(d.Get("type").(string))
enableBgp := d.Get("enable_bgp").(bool)

props := &network.VirtualNetworkGatewayConnectionPropertiesFormat{
ConnectionType: connectionType,
EnableBgp: &enableBgp,
}

if v, ok := d.GetOk("virtual_network_gateway_id"); ok {
Expand Down Expand Up @@ -294,14 +420,27 @@ func getArmVirtualNetworkGatewayConnectionProperties(d *schema.ResourceData) (*n
}
}

if v, ok := d.GetOk("enable_bgp"); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

if this is a bool, it'll always have a value (since the default is false) - as such we may as well just assign this above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea was to just not set the field in the API struct, if it is not defined in the Terraform schema instance. Is this overly safe?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's more this won't work as it reads unfortunately due to the way the defaults work in HCL (there's changes coming in this area in the future); given the field in the schema is a Boolean, there'll always be a default value of false, so the ok in this if statement will always be true.

We could instead update this to use GetOkExists which returns the raw value instead of the defaulted value if nothings specified (e.g. an empty string in this case, iirc) - however I don't think there's any harm setting a default to false tbh - what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing to GetOkExists, I did not know about this. Nevertheless, I followed your feedback and rely on the default value.

props.EnableBgp = utils.Bool(v.(bool))
}

if v, ok := d.GetOk("use_policy_based_traffic_selectors"); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

(same 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.

(same here)

props.UsePolicyBasedTrafficSelectors = utils.Bool(v.(bool))
}

if v, ok := d.GetOk("routing_weight"); ok {
routingWeight := int32(v.(int))
props.RoutingWeight = &routingWeight
routing_weight := int32(v.(int))
props.RoutingWeight = &routing_weight
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 revert this variable change?

}

if v, ok := d.GetOk("shared_key"); ok {
sharedKey := v.(string)
props.SharedKey = &sharedKey
props.SharedKey = utils.String(v.(string))
}

if v, ok := d.GetOk("ipsec_policy"); ok {
ipsec_policies := v.([]interface{})
props.IpsecPolicies = expandArmVirtualNetworkGatewayConnectionIpsecPolicies(ipsec_policies)
log.Printf("Ipsec policies: %+q", props.IpsecPolicies)
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 remove this print statement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, this is a true left over 😄

}

if props.ConnectionType == network.ExpressRoute {
Expand Down Expand Up @@ -339,3 +478,77 @@ func resourceGroupAndVirtualNetworkGatewayConnectionFromId(virtualNetworkGateway

return resGroup, name, nil
}

func expandArmVirtualNetworkGatewayConnectionIpsecPolicies(ipsec_policies []interface{}) *[]network.IpsecPolicy {
ipsecPolicies := make([]network.IpsecPolicy, 0, len(ipsec_policies))

for _, d := range ipsec_policies {
ipsec_policy := d.(map[string]interface{})
ipsecPolicy := &network.IpsecPolicy{}

if dh_group, ok := ipsec_policy["dh_group"].(string); ok && dh_group != "" {
ipsecPolicy.DhGroup = network.DhGroup(dh_group)
}

if ike_encryption, ok := ipsec_policy["ike_encryption"].(string); ok && ike_encryption != "" {
ipsecPolicy.IkeEncryption = network.IkeEncryption(ike_encryption)
}

if ike_integrity, ok := ipsec_policy["ike_integrity"].(string); ok && ike_integrity != "" {
ipsecPolicy.IkeIntegrity = network.IkeIntegrity(ike_integrity)
}

if ipsec_encryption, ok := ipsec_policy["ipsec_encryption"].(string); ok && ipsec_encryption != "" {
ipsecPolicy.IpsecEncryption = network.IpsecEncryption(ipsec_encryption)
}

if ipsec_integrity, ok := ipsec_policy["ipsec_integrity"].(string); ok && ipsec_integrity != "" {
ipsecPolicy.IpsecIntegrity = network.IpsecIntegrity(ipsec_integrity)
}

if pfs_group, ok := ipsec_policy["pfs_group"].(string); ok && pfs_group != "" {
ipsecPolicy.PfsGroup = network.PfsGroup(pfs_group)
}

if v, ok := ipsec_policy["sa_datasize"].(int); ok {
sa_datasize := int32(v)
ipsecPolicy.SaDataSizeKilobytes = &sa_datasize
}

if v, ok := ipsec_policy["sa_lifetime"].(int); ok {
sa_lifetime := int32(v)
ipsecPolicy.SaLifeTimeSeconds = &sa_lifetime
}
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 update all of these variables to use camelCase rather than snake_case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In particular in a mapping function like flattenArmVirtualNetworkGatewayConnectionIpsecPolicies, the two casing makes sense to differentiate between Terraform schema and struct of Azure Go SDK.


ipsecPolicies = append(ipsecPolicies, *ipsecPolicy)
}

return &ipsecPolicies
}

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

for _, ipsecPolicy := range *ipsecPolicies {
Copy link
Contributor

Choose a reason for hiding this comment

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

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 {
    # ..
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I shouldn't rely on the caller here

ipsec_policy := make(map[string]interface{})

ipsec_policy["dh_group"] = string(ipsecPolicy.DhGroup)
ipsec_policy["ike_encryption"] = string(ipsecPolicy.IkeEncryption)
ipsec_policy["ike_integrity"] = string(ipsecPolicy.IkeIntegrity)
ipsec_policy["ipsec_encryption"] = string(ipsecPolicy.IpsecEncryption)
ipsec_policy["ipsec_integrity"] = string(ipsecPolicy.IpsecIntegrity)
ipsec_policy["pfs_group"] = string(ipsecPolicy.PfsGroup)

if ipsecPolicy.SaDataSizeKilobytes != nil {
ipsec_policy["sa_datasize"] = int(*ipsecPolicy.SaDataSizeKilobytes)
}

if ipsecPolicy.SaLifeTimeSeconds != nil {
ipsec_policy["sa_lifetime"] = int(*ipsecPolicy.SaLifeTimeSeconds)
}

ipsec_policies = append(ipsec_policies, ipsec_policy)
}

return ipsec_policies
}
Loading