Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Update gwlb deregistration behavior #73

Open
wants to merge 7 commits into
base: update-gwlb-deregistration-behavior
Choose a base branch
from

Conversation

ntwrkguru
Copy link
Contributor

Description

The target group behavior upon deregistration is not currently exposed and only follows the provider defaults. This PR adds the possibility to manage this behavior via our module.

Motivation and Context

Client requested the ability to manage this behavior.

How Has This Been Tested?

Local testing

Screenshots (if appropriate)

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.

@ntwrkguru ntwrkguru changed the title Update gwlb deregistration behavior feat: Update gwlb deregistration behavior Aug 19, 2024
Copy link
Contributor

@sebastianczech sebastianczech left a comment

Choose a reason for hiding this comment

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

After merging main in commit 8c1e3fe (https://github.com/PaloAltoNetworks/terraform-aws-swfw-modules/pull/73/commits) , all your changes were overwritten. Are you able to restore what you had in 8bb0664 , please ?

Regarding validations introduced in this commit - I'm not sure if this is good approach as currently module show work with Terraform 1.0 or newer:

https://github.com/PaloAltoNetworks/terraform-aws-swfw-modules/blob/main/modules/gwlb/versions.tf

Referencing to other variables in validations was introduced in Terraform 1.9:

Previously, the condition block of an input validation could refer only to the variable itself. With Terraform 1.9, conditions can now refer to other input variables and even to other objects, such as data sources and local values, greatly expanding the kinds of scenarios that authors can validate. (source: https://www.hashicorp.com/blog/terraform-1-9-enhances-input-variable-validations)

@migara
Copy link
Member

migara commented Aug 29, 2024

@ntwrkguru can you please restore your commits in here as @sebastianczech suggested?

Also can you please consolidate the two variables in a single one with two keys and change the validation logic to something like this?

@ntwrkguru ntwrkguru force-pushed the update-gwlb-deregistration-behavior branch from f0cbc64 to 6087321 Compare August 29, 2024 19:36
@ntwrkguru ntwrkguru requested a review from a team as a code owner August 29, 2024 19:36
@@ -62,8 +62,8 @@ resource "aws_lb_target_group" "this" {
tags = var.lb_target_group_tags

target_failover {
on_deregistration = var.on_deregistration
on_unhealthy = var.on_unhealthy
on_deregistration = var.rebalance_flows
Copy link
Contributor

Choose a reason for hiding this comment

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

In variables.tf variable rebalance_flows is defined as bool.
In Terraform registry https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/lb_target_group for on_deregistration and on_unhealthy we have 2 possible values, which are string, not bool:

  • rebalance
  • no_rebalance

Does it work for you with bool values?

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

Successfully merging this pull request may close these issues.

4 participants