Skip to content
This repository has been archived by the owner on Feb 14, 2024. It is now read-only.

feat(module/gwlb): Add variable to manage stickiness type for target group #317

Merged
merged 4 commits into from
Jun 22, 2023

Conversation

sebastianczech
Copy link
Contributor

@sebastianczech sebastianczech commented Jun 5, 2023

Description

By default the Gateway Load Balancer uses 5-tuple to maintain flow stickiness to a specific target appliance. In order to modify the stickiness type and customize it to 3-tuple or 2-tuple, it was introduced new variable stickiness_type for module gwlb.

Motivation and Context

Detailed description can be found in #316 .

By purpose I haven't used approach described in #316 with dynamic, because in below scenario it doesn't work as expected:

  • we are deploying GWLB with target group with stickiness type 3-tuple, so we use stickiness_type = "source_ip_dest_ip_proto"
  • after deployment, we want to change to 5-tuple (default behaviour) so we don't define value for stickiness_type and default (null) is used
  • list used by for_each in dynamic is empty, so stickiness configuration isn't provided and old settings remains in AWS for target group

In approach proposed in PR it's defined enabled for stickiness, which is false for 5-tuple and true for:

  • 3-tuple
  • 2-tuple

so in every scenario stickiness configuration is provided, even if we are using default behaviour with 5-tuple.

How Has This Been Tested?

Code was tested by deploying combined_design with GWLB and then by changing:

stickiness_type = "source_ip_dest_ip"

and

stickiness_type = "source_ip_dest_ip_proto"

for gwlb module.

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist

  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes if appropriate.
  • All new and existing tests passed.

@sebastianczech sebastianczech marked this pull request as ready for review June 5, 2023 13:49
@sebastianczech sebastianczech requested a review from a team as a code owner June 5, 2023 13:49
@willoak84
Copy link

Hi,

Thank for the PR, this was really fast and did not see that while was pushing my suggested alternative.
As you mention I overlooked the default behaviour when the change is rollback, as you can guess I am quite a newbee on terraform.

However, I have few doubts/comments regarding the suggested code.

  1. Does it make sense to include "source_ip" as possible value for the stickiness_type in the validation?
  • This value is technically possible considering required_version is "~> 4.25" for the module and the new values were introduced in the version 4.38 as per aws/provider documentation.
  1. Is it required to control the values of the variable in the condition to set "enabled" attribute when those values are already controlled by the validation statement in the variables.tf ?

Thanks!

@sebastianczech
Copy link
Contributor Author

Hi @willoak84 ,

thank you for quick feedback . Regarding your questions:
1 - values source_ip_dest_ip and source_ip_dest_ip_proto were introduced as you have written in version 4.38.0 and in module gwlb and example combined_design, on which I was testing it, we are using provider in version ~> 4.25 :

  required_providers {
    aws = {
      source  = "hashicorp/aws"
      version = "~> 4.25"
    }
  }

According to Terraform documentation ~> allows only the rightmost version component to increment, what can be checked:

$ terraform init --upgrade | grep aws
- Finding hashicorp/aws versions matching "~> 4.25"...
- Using previously-installed hashicorp/aws v4.67.0

So to summarise - below code works, because Terraform is using provider in version 4.67 and both values source_ip_dest_ip, source_ip_dest_ip_proto are possible.

2 - you are right, it's not necessary to check it in main.tf after I've already was checking it in validation in variables.tf. I updated the code according to your suggestion, thanks

Copy link
Contributor

@pimielowski pimielowski left a comment

Choose a reason for hiding this comment

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

LGTM 👍 +1

@sebastianczech sebastianczech merged commit 34374ce into main Jun 22, 2023
@sebastianczech sebastianczech deleted the 316-stickiness-target-group branch June 22, 2023 09:49
@github-actions
Copy link
Contributor

🎉 This PR is included in version 0.4.2 🎉

The release is available on Terraform Registry and GitHub release

Posted by semantic-release bot

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants