#396 fix vpn connection tunnel output sorting

[jochen42]

Community Note

• Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
• Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Closes #396

Release note for CHANGELOG:

BUG FIXES:

* fix incorrect order (tunnel1 and tunnel2) in output of resource aws_vpn_connection

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccXXX'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -count 1 -parallel 20 -run=TestAccXXX -timeout 120m
?       github.com/terraform-providers/terraform-provider-aws   [no test files]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       0.037s [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws/internal/flatmap      0.007s [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags 0.014s [no tests to run]

[n3ph]

This does not fix the problem. You simply sorted the test xml blob yourself.

[jochen42]

This does not fix the problem. You simply sorted the test xml blob yourself.

no! i just removed the senseless sorting of an correct xml wich is the root-problem.
i tested this with many real-world-vpn connections and the result is 100% correct.

the original list is perfectly sorted.
please have a look at the documentations auf aws-sdk-go and the aws api. it just doesn't make sense to apply any sorting to this list.

https://docs.aws.amazon.com/sdk-for-go/api/service/ec2/#DescribeVpnConnectionsOutput

[n3ph]

For the case the order of tunnels in the XML does not match the order of options[] applied to the create request it we need to check whether vpnConfig.Tunnels[0].VgwInsideAddress or vpnConfig.Tunnels[0].CgwInsideAddress matches the CIDR block already defined by tunnel1_inside_cidr and tunnel2_inside_cidr.

Do you agree?

[n3ph]

Checked that again..

Getting rid of sort.Sort(vpnConfig) leads to the same remote state I have with my fix applied. So it seems to be sufficient to just remove it.

I fucked up copying the custom builds around.. Sry

My version applied:

    tunnel1_address                = "52.58.153.17"
    tunnel1_bgp_asn                = "65500"
    tunnel1_bgp_holdtime           = 30
    tunnel1_cgw_inside_address     = "169.254.128.230"
    tunnel1_inside_cidr            = "169.254.128.228/30"
    tunnel1_preshared_key          = (sensitive value)
    tunnel1_vgw_inside_address     = "169.254.128.229"
    tunnel2_address                = "18.185.134.28"
    tunnel2_bgp_asn                = "65500"
    tunnel2_bgp_holdtime           = 30
    tunnel2_cgw_inside_address     = "169.254.128.226"
    tunnel2_inside_cidr            = "169.254.128.224/30"
    tunnel2_preshared_key          = (sensitive value)
    tunnel2_vgw_inside_address     = "169.254.128.225"

Your version applied:

    tunnel1_address                = "18.185.134.28"
    tunnel1_bgp_asn                = "65500"
    tunnel1_bgp_holdtime           = 30
    tunnel1_cgw_inside_address     = "169.254.128.226"
    tunnel1_inside_cidr            = "169.254.128.228/30"
    tunnel1_preshared_key          = (sensitive value)
    tunnel1_vgw_inside_address     = "169.254.128.225"
    tunnel2_address                = "52.58.153.17"
    tunnel2_bgp_asn                = "65500"
    tunnel2_bgp_holdtime           = 30
    tunnel2_cgw_inside_address     = "169.254.128.230"
    tunnel2_inside_cidr            = "169.254.128.224/30"
    tunnel2_preshared_key          = (sensitive value)
    tunnel2_vgw_inside_address     = "169.254.128.229"

As you can see the CIDR notations are mixed up.

[jochen42]

hi @n3ph ,

i think you are facing another issue than me. My issue is just the wrong order of the tunnels itself.
With the removed complex-object-sort i just want to have the correct order like aws-api answers.

it is hard to debug for the operators at the client-side if they are wrong ordered.

f.e. the results of the following command should be in the same order as the provided connection-parameter vom resource-output or statefile.

aws ec2 describe-vpn-connections --vpn-connection-id <some_connection_idf> --query 'VpnConnections[*].VgwTelemetry[*]'

the same for aws console.

your issue seems to be much more complex. for me it looks like you are right with the wrong tunnel*_vgw_inside_address, but your solution:

• will also provide the wrong tunnel-order/sorting (hard to debug)
• tries to fix a problem on aws-side with an assumption of their internal-behavior (what if you are wrong or they change the wrong behavior next week?)

Perhaps your issue should be reported to aws support?

[bflad]

Marking as a breaking change since removing the existing sorting without accounting for what was in the previous Terraform configuration/state was can cause potential differences with existing Terraform environments. My understanding of this issue is that the EC2 API may not return tunnel results consistently, which was why the sorting was originally added a long while ago.

[jochen42]

Marking as a breaking change since removing the existing sorting without accounting for what was in the previous Terraform configuration/state was can cause potential differences with existing Terraform environments. My understanding of this issue is that the EC2 API may not return tunnel results consistently, which was why the sorting was originally added a long while ago.

you are right. it's an breaking change. but the sorting in the current version doesn't make sense at all. a complext object is sorted without an definition which fields are used and how the sorting of complex strings like ip-networks/addresses are done. so: the sorting was never correct with the line of code. and it's also impossible that this sorting fixed any previous sorting-problem on aws-side.

[bflad]

Hi @jochen42 👋 Thank you again for submitting this awhile back. To ensure the tunnel sorting changes are backwards compatible with existing resources, we have opted to take a different approach in #19077, which should be released shortly to fix this issue.

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.