Skip to content

Commit

Permalink
resource/aws_vpn_connection: Prevent flipped tunnel1_* and `tunnel2…
Browse files Browse the repository at this point in the history
…_*` ordering when `tunnel1_inside_cidr`, `tunnel1_inside_ipv6_cidr`, or `tunnel1_preshared_key` is configured (#19077)

* resource/aws_vpn_connection: Prevent flipped `tunnel1_*` and `tunnel2_*` ordering when `tunnel1_inside_cidr`, `tunnel1_inside_ipv6_cidr`, or `tunnel1_preshared_key` is configured

Reference: #396
Reference: #3359
Reference: #4728
Reference: #5809
Reference: #11293

Previously (race condition of automatically assigned outside IP addresses):

```
=== CONT  TestAccAWSVpnConnection_tunnelOptions
resource_aws_vpn_connection_test.go:210: Step 15/15 error: Check failed: Check 4/6 error: aws_vpn_connection.test: Attribute 'tunnel1_preshared_key' expected "12345678", got "abcdefgh"
--- FAIL: TestAccAWSVpnConnection_tunnelOptions (738.28s)
```

Output from acceptance testing:

```
--- PASS: TestAccAWSVpnConnection_Tunnel1PresharedKey (251.02s)
--- PASS: TestAccAWSVpnConnection_withoutStaticRoutes (263.77s)
--- PASS: TestAccAWSVpnConnection_Tunnel1InsideCidr (335.14s)
--- PASS: TestAccAWSVpnConnection_tunnelOptions (342.30s)
--- PASS: TestAccAWSVpnConnection_disappears (388.07s)
--- PASS: TestAccAWSVpnConnection_tags (445.29s)
--- PASS: TestAccAWSVpnConnection_basic (797.33s)
--- PASS: TestAccAWSVpnConnection_withIpv6 (1235.35s)
--- PASS: TestAccAWSVpnConnection_TransitGatewayID (1235.72s)
--- PASS: TestAccAWSVpnConnection_withEnableAcceleration (1352.28s)
--- PASS: TestAccAWSVpnConnection_Tunnel1InsideIpv6Cidr (1595.79s)
```

* tests/resource/aws_vpn_connection: Add nosemgrep comment for errant situation

* resource/aws_vpn_connection: Fix comment typo
  • Loading branch information
bflad committed Apr 23, 2021
1 parent e0682fa commit 54173a6
Show file tree
Hide file tree
Showing 3 changed files with 311 additions and 58 deletions.
3 changes: 3 additions & 0 deletions .changelog/pending.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
resource/aws_vpn_connection: Prevent flipped `tunnel1_*` and `tunnel2_*` ordering when `tunnel1_inside_cidr`, `tunnel1_inside_ipv6_cidr`, or `tunnel1_preshared_key` is configured
```
46 changes: 42 additions & 4 deletions aws/resource_aws_vpn_connection.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"encoding/xml"
"fmt"
"log"
"net"
"regexp"
"sort"
"time"
Expand Down Expand Up @@ -704,7 +705,14 @@ func resourceAwsVpnConnectionRead(d *schema.ResourceData, meta interface{}) erro
d.Set("transit_gateway_attachment_id", transitGatewayAttachmentID)

if vpnConnection.CustomerGatewayConfiguration != nil {
if tunnelInfo, err := xmlConfigToTunnelInfo(*vpnConnection.CustomerGatewayConfiguration); err != nil {
tunnelInfo, err := xmlConfigToTunnelInfo(
aws.StringValue(vpnConnection.CustomerGatewayConfiguration),
d.Get("tunnel1_preshared_key").(string), // Not currently available during import
d.Get("tunnel1_inside_cidr").(string), // Not currently available during import
d.Get("tunnel1_inside_ipv6_cidr").(string), // Not currently available during import
)

if err != nil {
log.Printf("[ERR] Error unmarshaling XML configuration for (%s): %s", d.Id(), err)
} else {
d.Set("tunnel1_address", tunnelInfo.Tunnel1Address)
Expand Down Expand Up @@ -1626,14 +1634,44 @@ func waitForEc2VpnConnectionDeletion(conn *ec2.EC2, id string) error {
return err
}

func xmlConfigToTunnelInfo(xmlConfig string) (*TunnelInfo, error) {
// The tunnel1 parameters are optionally used to correctly order tunnel configurations.
func xmlConfigToTunnelInfo(xmlConfig string, tunnel1PreSharedKey string, tunnel1InsideCidr string, tunnel1InsideIpv6Cidr string) (*TunnelInfo, error) {
var vpnConfig XmlVpnConnectionConfig
if err := xml.Unmarshal([]byte(xmlConfig), &vpnConfig); err != nil {
return nil, fmt.Errorf("Error Unmarshalling XML: %s", err)
}

// don't expect consistent ordering from the XML
sort.Sort(vpnConfig)
// XML tunnel ordering was commented here as being inconsistent since
// this logic was originally added. The original sorting is based on
// outside address. Given potential tunnel identifying configuration,
// we try to correctly align the tunnel ordering before preserving the
// original outside address sorting fallback for backwards compatibility
// as to not inadvertently flip existing configurations.
if tunnel1PreSharedKey != "" {
if tunnel1PreSharedKey != vpnConfig.Tunnels[0].PreSharedKey && tunnel1PreSharedKey == vpnConfig.Tunnels[1].PreSharedKey {
vpnConfig.Tunnels[0], vpnConfig.Tunnels[1] = vpnConfig.Tunnels[1], vpnConfig.Tunnels[0]
}
} else if cidr := tunnel1InsideCidr; cidr != "" {
if _, ipNet, err := net.ParseCIDR(cidr); err == nil && ipNet != nil {
vgwInsideAddressIP1 := net.ParseIP(vpnConfig.Tunnels[0].VgwInsideAddress)
vgwInsideAddressIP2 := net.ParseIP(vpnConfig.Tunnels[1].VgwInsideAddress)

if !ipNet.Contains(vgwInsideAddressIP1) && ipNet.Contains(vgwInsideAddressIP2) {
vpnConfig.Tunnels[0], vpnConfig.Tunnels[1] = vpnConfig.Tunnels[1], vpnConfig.Tunnels[0]
}
}
} else if cidr := tunnel1InsideIpv6Cidr; cidr != "" {
if _, ipNet, err := net.ParseCIDR(cidr); err == nil && ipNet != nil {
vgwInsideAddressIP1 := net.ParseIP(vpnConfig.Tunnels[0].VgwInsideAddress)
vgwInsideAddressIP2 := net.ParseIP(vpnConfig.Tunnels[1].VgwInsideAddress)

if !ipNet.Contains(vgwInsideAddressIP1) && ipNet.Contains(vgwInsideAddressIP2) {
vpnConfig.Tunnels[0], vpnConfig.Tunnels[1] = vpnConfig.Tunnels[1], vpnConfig.Tunnels[0]
}
}
} else {
sort.Sort(vpnConfig)
}

tunnelInfo := TunnelInfo{
Tunnel1Address: vpnConfig.Tunnels[0].OutsideAddress,
Expand Down
Loading

0 comments on commit 54173a6

Please sign in to comment.