Clean up repeated aws_vpn_connection

[cdsre]

Is your request related to a new offering from AWS?

No

Is your request related to a problem? Please describe.

No

Describe the solution you'd like.

The module has 4 implementations of aws_vpn_connection however 95% of the code in each resource is identical. With only a few differing lines. These seem to center around 4 differnt variables creating several flavours tunnel1_preshared_key, tunnel2_preshared_key, tunnel1_inside_cidr, tunnel2_inside_cidr. These variables are then used to form 4 flavours

• default - No preshared keys or inside cidrs
• tunnel - no preshared keys but inside cidrs
• preshared - preshared keys but no inside cidrs
• preshared_tunnel - preshared keys and inside cidrs.

This issue is raised to consider reducing the complexity of the flavours and module and make the code DRY to reduce having to make new changes in several places in the code. My general idea is to always pass these variables however with a conditional that checks the value and if its an empty string then pass a null value. However this functionality would only work for versions of terraform V0.12.0 onwards as documented in https://github.com/hashicorp/terraform/releases/tag/v0.12.0

Nullable argument values: It is now possible to use a conditional expression like var.foo != "" ? var.foo : null to conditionally leave an argument value unset, whereas before Terraform required the configuration author to provide a specific default value in this case. Assigning null to an argument is equivalent to omitting that argument entirely.

An example might look like:

  tunnel1_inside_cidr = var.tunnel1_inside_cidr != "" ? var.tunnel1_inside_cidr : null
  tunnel2_inside_cidr = var.tunnel2_inside_cidr != "" ? var.tunnel2_inside_cidr : null

  tunnel1_preshared_key = var.tunnel1_preshared_key != "" ? var.tunnel1_preshared_key : null
  tunnel2_preshared_key = var.tunnel2_preshared_key != "" ? var.tunnel2_preshared_key : null

This could mean we only need to define aws_vpn_connection resource and remove a lot of the repeated code and the local variables. I have not tested any of this. I wanted to float the idea here first before refactoring any of the code. By testing the variables against an empty string we keep the interface of the module intact. This should keep the module backwards compatible with any calling code.

Describe alternatives you've considered.

If we wanted to remove the empty string ternary check when setting the attribute in the resource we could change the default from "" to null but this would not be backwards compatible.

Additional context

If the community feels there is value in this I am happy to pick up the work and refactor and submit a PR for it.

[github-actions[bot]]

This issue has been automatically marked as stale because it has been open 30 days with no activity. Remove stale label or comment or this issue will be closed in 10 days

[cdsre]

I am happy to take on this work if the community feels it will reduce the code complexity but still provide a backwards compatible experience

[antonbabenko]

@cdsre Please go ahead and do the required refactoring (including upgrade guide docs).

Please take a look at major releases in other modules where similar work has already happened (for e.g., https://github.com/terraform-aws-modules/terraform-aws-ec2-instance )

[cdsre]

Sorry I missed this response / update. I am on annual leave tomorrow so will try to pick this up then.

[github-actions[bot]]

This issue has been automatically marked as stale because it has been open 30 days with no activity. Remove stale label or comment or this issue will be closed in 10 days

[brianabston001]

When is the refactor going to be merged in?

[cdsre]

@bryantbiggs This is the original Issue I opened for the code clean up and refactor. The PR I submitted was closed for further discussion on the work through an issue. Is there anything you want to add to the issue that can help me modify the refactoring to get it into an acceptable state and reduces its code complexity and repetition.

[github-actions[bot]]

This issue has been automatically marked as stale because it has been open 30 days with no activity. Remove stale label or comment or this issue will be closed in 10 days

[github-actions[bot]]

This issue was automatically closed because of stale in 10 days

[github-actions[bot]]

I'm going to lock this issue 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 similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.