From 54173a6fd077bcf3af11907fa728353984887486 Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Fri, 23 Apr 2021 15:48:29 -0400 Subject: [PATCH 1/2] resource/aws_vpn_connection: Prevent flipped `tunnel1_*` and `tunnel2_*` 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: https://github.com/hashicorp/terraform-provider-aws/issues/396 Reference: https://github.com/hashicorp/terraform-provider-aws/issues/3359 Reference: https://github.com/hashicorp/terraform-provider-aws/issues/4728 Reference: https://github.com/hashicorp/terraform-provider-aws/issues/5809 Reference: https://github.com/hashicorp/terraform-provider-aws/issues/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 --- .changelog/pending.txt | 3 + aws/resource_aws_vpn_connection.go | 46 +++- aws/resource_aws_vpn_connection_test.go | 320 ++++++++++++++++++++---- 3 files changed, 311 insertions(+), 58 deletions(-) create mode 100644 .changelog/pending.txt diff --git a/.changelog/pending.txt b/.changelog/pending.txt new file mode 100644 index 00000000000..d5cfe5447e2 --- /dev/null +++ b/.changelog/pending.txt @@ -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 +``` diff --git a/aws/resource_aws_vpn_connection.go b/aws/resource_aws_vpn_connection.go index d0106992b83..2cb47ce9cc0 100644 --- a/aws/resource_aws_vpn_connection.go +++ b/aws/resource_aws_vpn_connection.go @@ -5,6 +5,7 @@ import ( "encoding/xml" "fmt" "log" + "net" "regexp" "sort" "time" @@ -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) @@ -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, diff --git a/aws/resource_aws_vpn_connection_test.go b/aws/resource_aws_vpn_connection_test.go index 8f5c4a2f992..ff323ea4043 100644 --- a/aws/resource_aws_vpn_connection_test.go +++ b/aws/resource_aws_vpn_connection_test.go @@ -3,6 +3,7 @@ package aws import ( "fmt" "log" + "reflect" "regexp" "testing" @@ -161,6 +162,90 @@ func TestAccAWSVpnConnection_TransitGatewayID(t *testing.T) { }) } +func TestAccAWSVpnConnection_Tunnel1InsideCidr(t *testing.T) { + rBgpAsn := acctest.RandIntRange(64512, 65534) + resourceName := "aws_vpn_connection.test" + var vpn ec2.VpnConnection + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + ErrorCheck: testAccErrorCheck(t, ec2.EndpointsID), + Providers: testAccProviders, + CheckDestroy: testAccAwsVpnConnectionDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAwsVpnConnectionConfigTunnel1InsideCidr(rBgpAsn, "169.254.8.0/30", "169.254.9.0/30"), + Check: resource.ComposeAggregateTestCheckFunc( + testAccAwsVpnConnectionExists(resourceName, &vpn), + resource.TestCheckResourceAttr(resourceName, "tunnel1_inside_cidr", "169.254.8.0/30"), + resource.TestCheckResourceAttr(resourceName, "tunnel2_inside_cidr", "169.254.9.0/30"), + ), + }, + // NOTE: Import does not currently have access to the Terraform configuration, + // so proper tunnel ordering is not guaranteed on import. The import + // identifier could potentially be updated to accept optional tunnel + // configuration information, however the format for this could be + // confusing and/or difficult to implement. + }, + }) +} + +func TestAccAWSVpnConnection_Tunnel1InsideIpv6Cidr(t *testing.T) { + rBgpAsn := acctest.RandIntRange(64512, 65534) + resourceName := "aws_vpn_connection.test" + var vpn ec2.VpnConnection + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + ErrorCheck: testAccErrorCheck(t, ec2.EndpointsID), + Providers: testAccProviders, + CheckDestroy: testAccAwsVpnConnectionDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAwsVpnConnectionConfigTunnel1InsideIpv6Cidr(rBgpAsn, "fd00:2001:db8:2:2d1:81ff:fe41:d200/126", "fd00:2001:db8:2:2d1:81ff:fe41:d204/126"), + Check: resource.ComposeAggregateTestCheckFunc( + testAccAwsVpnConnectionExists(resourceName, &vpn), + resource.TestCheckResourceAttr(resourceName, "tunnel1_inside_ipv6_cidr", "fd00:2001:db8:2:2d1:81ff:fe41:d200/126"), + resource.TestCheckResourceAttr(resourceName, "tunnel2_inside_ipv6_cidr", "fd00:2001:db8:2:2d1:81ff:fe41:d204/126"), + ), + }, + // NOTE: Import does not currently have access to the Terraform configuration, + // so proper tunnel ordering is not guaranteed on import. The import + // identifier could potentially be updated to accept optional tunnel + // configuration information, however the format for this could be + // confusing and/or difficult to implement. + }, + }) +} + +func TestAccAWSVpnConnection_Tunnel1PresharedKey(t *testing.T) { + rBgpAsn := acctest.RandIntRange(64512, 65534) + resourceName := "aws_vpn_connection.test" + var vpn ec2.VpnConnection + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + ErrorCheck: testAccErrorCheck(t, ec2.EndpointsID), + Providers: testAccProviders, + CheckDestroy: testAccAwsVpnConnectionDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAwsVpnConnectionConfigTunnel1PresharedKey(rBgpAsn, "tunnel1presharedkey", "tunnel2presharedkey"), + Check: resource.ComposeAggregateTestCheckFunc( + testAccAwsVpnConnectionExists(resourceName, &vpn), + resource.TestCheckResourceAttr(resourceName, "tunnel1_preshared_key", "tunnel1presharedkey"), + resource.TestCheckResourceAttr(resourceName, "tunnel2_preshared_key", "tunnel2presharedkey"), + ), + }, + // NOTE: Import does not currently have access to the Terraform configuration, + // so proper tunnel ordering is not guaranteed on import. The import + // identifier could potentially be updated to accept optional tunnel + // configuration information, however the format for this could be + // confusing and/or difficult to implement. + }, + }) +} + func TestAccAWSVpnConnection_tunnelOptions(t *testing.T) { badCidrRangeErr := regexp.MustCompile(`expected \w+ to not be any of \[[\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}\/30\s?]+\]`) rBgpAsn := acctest.RandIntRange(64512, 65534) @@ -305,7 +390,11 @@ func TestAccAWSVpnConnection_tunnelOptions(t *testing.T) { resource.TestCheckResourceAttr(resourceName, "tunnel2_preshared_key", "abcdefgh"), ), }, - // TODO: Once #396, #3359, #5809 are fixed, an import test step should be added here + // NOTE: Import does not currently have access to the Terraform configuration, + // so proper tunnel ordering is not guaranteed on import. The import + // identifier could potentially be updated to accept optional tunnel + // configuration information, however the format for this could be + // confusing and/or difficult to implement. }, }) } @@ -535,50 +624,112 @@ func testAccAwsVpnConnectionExists(vpnConnectionResource string, vpnConnection * } } -func TestAWSVpnConnection_xmlconfig(t *testing.T) { - tunnelInfo, err := xmlConfigToTunnelInfo(testAccAwsVpnTunnelInfoXML) - if err != nil { - t.Fatalf("Error unmarshalling XML: %s", err) - } - if tunnelInfo.Tunnel1Address != "FIRST_ADDRESS" { - t.Fatalf("First address from tunnel XML was incorrect.") - } - if tunnelInfo.Tunnel1CgwInsideAddress != "FIRST_CGW_INSIDE_ADDRESS" { - t.Fatalf("First Customer Gateway inside address from tunnel" + - " XML was incorrect.") - } - if tunnelInfo.Tunnel1VgwInsideAddress != "FIRST_VGW_INSIDE_ADDRESS" { - t.Fatalf("First VPN Gateway inside address from tunnel " + - " XML was incorrect.") - } - if tunnelInfo.Tunnel1PreSharedKey != "FIRST_KEY" { - t.Fatalf("First key from tunnel XML was incorrect.") - } - if tunnelInfo.Tunnel1BGPASN != "FIRST_BGP_ASN" { - t.Fatalf("First bgp asn from tunnel XML was incorrect.") - } - if tunnelInfo.Tunnel1BGPHoldTime != 31 { - t.Fatalf("First bgp holdtime from tunnel XML was incorrect.") - } - if tunnelInfo.Tunnel2Address != "SECOND_ADDRESS" { - t.Fatalf("Second address from tunnel XML was incorrect.") - } - if tunnelInfo.Tunnel2CgwInsideAddress != "SECOND_CGW_INSIDE_ADDRESS" { - t.Fatalf("Second Customer Gateway inside address from tunnel" + - " XML was incorrect.") - } - if tunnelInfo.Tunnel2VgwInsideAddress != "SECOND_VGW_INSIDE_ADDRESS" { - t.Fatalf("Second VPN Gateway inside address from tunnel " + - " XML was incorrect.") - } - if tunnelInfo.Tunnel2PreSharedKey != "SECOND_KEY" { - t.Fatalf("Second key from tunnel XML was incorrect.") - } - if tunnelInfo.Tunnel2BGPASN != "SECOND_BGP_ASN" { - t.Fatalf("Second bgp asn from tunnel XML was incorrect.") +func TestXmlConfigToTunnelInfo(t *testing.T) { + testCases := []struct { + Name string + XML string + Tunnel1PreSharedKey string + Tunnel1InsideCidr string + Tunnel1InsideIpv6Cidr string + ExpectError bool + ExpectTunnelInfo TunnelInfo + }{ + { + Name: "outside address sort", + XML: testAccAwsVpnTunnelInfoXML, + ExpectTunnelInfo: TunnelInfo{ + Tunnel1Address: "1.1.1.1", + Tunnel1BGPASN: "1111", + Tunnel1BGPHoldTime: 31, + Tunnel1CgwInsideAddress: "169.254.11.1", + Tunnel1PreSharedKey: "FIRST_KEY", + Tunnel1VgwInsideAddress: "168.254.11.2", + Tunnel2Address: "2.2.2.2", + Tunnel2BGPASN: "2222", + Tunnel2BGPHoldTime: 32, + Tunnel2CgwInsideAddress: "169.254.12.1", + Tunnel2PreSharedKey: "SECOND_KEY", + Tunnel2VgwInsideAddress: "169.254.12.2", + }, + }, + { + Name: "Tunnel1PreSharedKey", + XML: testAccAwsVpnTunnelInfoXML, + Tunnel1PreSharedKey: "SECOND_KEY", + ExpectTunnelInfo: TunnelInfo{ + Tunnel1Address: "2.2.2.2", + Tunnel1BGPASN: "2222", + Tunnel1BGPHoldTime: 32, + Tunnel1CgwInsideAddress: "169.254.12.1", + Tunnel1PreSharedKey: "SECOND_KEY", + Tunnel1VgwInsideAddress: "169.254.12.2", + Tunnel2Address: "1.1.1.1", + Tunnel2BGPASN: "1111", + Tunnel2BGPHoldTime: 31, + Tunnel2CgwInsideAddress: "169.254.11.1", + Tunnel2PreSharedKey: "FIRST_KEY", + Tunnel2VgwInsideAddress: "168.254.11.2", + }, + }, + { + Name: "Tunnel1InsideCidr", + XML: testAccAwsVpnTunnelInfoXML, + Tunnel1InsideCidr: "169.254.12.0/30", + ExpectTunnelInfo: TunnelInfo{ + Tunnel1Address: "2.2.2.2", + Tunnel1BGPASN: "2222", + Tunnel1BGPHoldTime: 32, + Tunnel1CgwInsideAddress: "169.254.12.1", + Tunnel1PreSharedKey: "SECOND_KEY", + Tunnel1VgwInsideAddress: "169.254.12.2", + Tunnel2Address: "1.1.1.1", + Tunnel2BGPASN: "1111", + Tunnel2BGPHoldTime: 31, + Tunnel2CgwInsideAddress: "169.254.11.1", + Tunnel2PreSharedKey: "FIRST_KEY", + Tunnel2VgwInsideAddress: "168.254.11.2", + }, + }, + // IPv6 logic is equivalent to IPv4, so we can reuse configuration, expected, etc. + { + Name: "Tunnel1InsideIpv6Cidr", + XML: testAccAwsVpnTunnelInfoXML, + Tunnel1InsideIpv6Cidr: "169.254.12.1", + ExpectTunnelInfo: TunnelInfo{ + Tunnel1Address: "2.2.2.2", + Tunnel1BGPASN: "2222", + Tunnel1BGPHoldTime: 32, + Tunnel1CgwInsideAddress: "169.254.12.1", + Tunnel1PreSharedKey: "SECOND_KEY", + Tunnel1VgwInsideAddress: "169.254.12.2", + Tunnel2Address: "1.1.1.1", + Tunnel2BGPASN: "1111", + Tunnel2BGPHoldTime: 31, + Tunnel2CgwInsideAddress: "169.254.11.1", + Tunnel2PreSharedKey: "FIRST_KEY", + Tunnel2VgwInsideAddress: "168.254.11.2", + }, + }, } - if tunnelInfo.Tunnel2BGPHoldTime != 32 { - t.Fatalf("Second bgp holdtime from tunnel XML was incorrect.") + + for _, testCase := range testCases { + testCase := testCase + + t.Run(testCase.Name, func(t *testing.T) { + tunnelInfo, err := xmlConfigToTunnelInfo(testCase.XML, testCase.Tunnel1PreSharedKey, testCase.Tunnel1InsideCidr, testCase.Tunnel1InsideIpv6Cidr) + + if err == nil && testCase.ExpectError { + t.Fatalf("expected error, got none") + } + + if err != nil && !testCase.ExpectError { + t.Fatalf("expected no error, got: %s", err) + } + + if actual, expected := *tunnelInfo, testCase.ExpectTunnelInfo; !reflect.DeepEqual(actual, expected) { // nosemgrep: prefer-aws-go-sdk-pointer-conversion-assignment + t.Errorf("expected TunnelInfo:\n%+v\n\ngot:\n%+v\n\n", expected, actual) + } + }) } } @@ -738,6 +889,67 @@ resource "aws_vpn_connection" "test" { `, rBgpAsn) } +func testAccAwsVpnConnectionConfigTunnel1InsideCidr(rBgpAsn int, tunnel1InsideCidr string, tunnel2InsideCidr string) string { + return fmt.Sprintf(` +resource "aws_customer_gateway" "test" { + bgp_asn = %[1]d + ip_address = "178.0.0.1" + type = "ipsec.1" +} + +resource "aws_vpn_gateway" "test" {} + +resource "aws_vpn_connection" "test" { + customer_gateway_id = aws_customer_gateway.test.id + tunnel1_inside_cidr = %[2]q + tunnel2_inside_cidr = %[3]q + type = "ipsec.1" + vpn_gateway_id = aws_vpn_gateway.test.id +} +`, rBgpAsn, tunnel1InsideCidr, tunnel2InsideCidr) +} + +func testAccAwsVpnConnectionConfigTunnel1InsideIpv6Cidr(rBgpAsn int, tunnel1InsideIpv6Cidr string, tunnel2InsideIpv6Cidr string) string { + return fmt.Sprintf(` +resource "aws_customer_gateway" "test" { + bgp_asn = %[1]d + ip_address = "178.0.0.1" + type = "ipsec.1" +} + +resource "aws_ec2_transit_gateway" "test" {} + +resource "aws_vpn_connection" "test" { + customer_gateway_id = aws_customer_gateway.test.id + transit_gateway_id = aws_ec2_transit_gateway.test.id + tunnel_inside_ip_version = "ipv6" + tunnel1_inside_ipv6_cidr = %[2]q + tunnel2_inside_ipv6_cidr = %[3]q + type = "ipsec.1" +} +`, rBgpAsn, tunnel1InsideIpv6Cidr, tunnel2InsideIpv6Cidr) +} + +func testAccAwsVpnConnectionConfigTunnel1PresharedKey(rBgpAsn int, tunnel1PresharedKey string, tunnel2PresharedKey string) string { + return fmt.Sprintf(` +resource "aws_customer_gateway" "test" { + bgp_asn = %[1]d + ip_address = "178.0.0.1" + type = "ipsec.1" +} + +resource "aws_vpn_gateway" "test" {} + +resource "aws_vpn_connection" "test" { + customer_gateway_id = aws_customer_gateway.test.id + tunnel1_preshared_key = %[2]q + tunnel2_preshared_key = %[3]q + type = "ipsec.1" + vpn_gateway_id = aws_vpn_gateway.test.id +} +`, rBgpAsn, tunnel1PresharedKey, tunnel2PresharedKey) +} + func testAccAwsVpnConnectionConfigTunnelOptions( rBgpAsn int, localIpv4NetworkCidr string, @@ -916,25 +1128,25 @@ const testAccAwsVpnTunnelInfoXML = ` - 123.123.123.123 + 22.22.22.22 - SECOND_CGW_INSIDE_ADDRESS + 169.254.12.1 255.255.255.252 30 - SECOND_ADDRESS + 2.2.2.2 - SECOND_VGW_INSIDE_ADDRESS + 169.254.12.2 255.255.255.252 30 - SECOND_BGP_ASN + 2222 32 @@ -945,25 +1157,25 @@ const testAccAwsVpnTunnelInfoXML = ` - 123.123.123.123 + 11.11.11.11 - FIRST_CGW_INSIDE_ADDRESS + 169.254.11.1 255.255.255.252 30 - FIRST_ADDRESS + 1.1.1.1 - FIRST_VGW_INSIDE_ADDRESS + 168.254.11.2 255.255.255.252 30 - FIRST_BGP_ASN + 1111 31 From e952d0b831374cf866807b8e3166b4e923c4d693 Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Fri, 23 Apr 2021 17:30:12 -0400 Subject: [PATCH 2/2] Update CHANGELOG for #19097 --- .changelog/{pending.txt => 19097.txt} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename .changelog/{pending.txt => 19097.txt} (100%) diff --git a/.changelog/pending.txt b/.changelog/19097.txt similarity index 100% rename from .changelog/pending.txt rename to .changelog/19097.txt