feat!: Refactor module to use maps instead of lists #305

bryantbiggs · 2023-08-23T15:30:36Z

Description

Backwards incompatible changes

target_groups previously were defined by an array of target group definitions that were created using the count meta-argument. This has been replaced with a map of target group definitions that are created using the for_each meta-argument in order to provide better stability when adding/removing target group definitions.
target_groups no longer support multiple targets per target group. There are alternate methods to achieve similar functionality such as weighted target groups or using an autoscaling group as a target when targetting EC2 instances.
The previous methods for creating listeners have been removed in favor of one argument, listeners, which take a map of listener definitions that are created using the for_each meta-argument in order to provide better stability when adding/removing listener definitions. Previously the target_group_index was used to associate/reference a target group; that is now replaced with target_group_key which is the key of the target group definition in the target_groups map.
security_group_rules has been replaced by security_group_ingress_rules and security_group_egress_rules to align with the new underlying resources.
Minimum supported version of Terraform AWS provider updated to v5.13 to support the latest features provided via the resources utilized.
Minimum supported version of Terraform updated to v1.0
The Name tag has been removed from resources

Added

Security group attachment restrictions have been removed now that both ALB and NLB support security groups
Support for creating Route53 records for ALB/NLB DNS names via the route53_records variable

Modified

enable_cross_zone_load_balancing now defaults to true
drop_invalid_header_fields now defaults to true
enable_deletion_protection now defaults to true
associate_web_acl has been added to identify when a WAFv2 Web ACL should be associated with the ALB; previously this was accomplished by checking for the presence of a value passed to web_acl_arn which is known to cause issues when the value does not exist and is computed.

See UPGRADE-9.0.md for further details

Motivation and Context

Resolves Putting listener rules in the middle of the list causes erratic terraform behavior #289
Resolves feat: Add forward group for https listener #297
Resolves Associate WAF variable #298
Resolves Weighted target groups don't work #299
Resolves getting Invalid index while crearing alb with huge # of listener rules #301
Resolves Support for network load balancer (NLB) security groups #304
Resolves feat!: Refactor module to use maps instead of lists #305
Resolves feat: Update default of enable_xff_client_port to match the provider #310

Breaking Changes

Yes

How Has This Been Tested?

I have updated at least one of the examples/* to demonstrate and validate my change(s)
I have tested and validated these changes using one or more of the provided examples/* projects
I have executed pre-commit run -a on my pull request

antonbabenko · 2023-08-23T16:14:02Z

Nice! Let me know when this is ready for my review :)

bryantbiggs · 2023-08-23T16:17:08Z

yes, sorry forgot to hit the button there 😅 - but its ready for review

ChristopheBougere

I'm facing #289 so I gave a try to your MR.

main.tf

spr-mweber3 · 2023-09-05T05:29:32Z

@antonbabenko What do you think? Is this ready to be merged?

lblazewski · 2023-09-12T13:17:37Z

Highly interested in this refactor too!

antonbabenko

It looks great, mostly small questions, but also some thoughts about simplifying the map value (WDYT?).

README.md

UPGRADE-9.0.md

antonbabenko · 2023-09-13T10:05:40Z

UPGRADE-9.0.md

+
+      rules = {
+        cognito = {
+          actions = [


Variable structure and naming (in general) is hard.

There are multiple levels inside of the map inside of listeners, and I am wondering if we can simplify it somehow? For example:

listeners = { https = { # It is not obvious if this is a "type" of a listener of just any name? # ... some common arguments like "port", "protocol", "ssl_policy"... forward = { # "forward" is an action, but it looks like it can be also a common argument for the listener like those listed above # omitted... } rules = { # is "rules" a required argument? cognito = { # is this key "cognito" a keyword for some type of a rule? actions = [ # a list of actions to apply in which situation?

It is pretty hard to navigate such multiple-level maps and understand what are the required arguments for the listeners, for the type of action ("forward", "weighted_forward", "fixed_response", "redirect", etc), or for something else.

Maybe we can improve it by making more granular examples to show multiple combinations of such values, too?

yes - any of the networking resources tend to get very gnarly with just miles of nested maps. I actually created an AppMesh module awhile ago but after seeing such deeply nested maps everywhere, I felt like that wasn't going to do any good so it never saw the light of day 😅 (see snippet below)

I think this starts getting into the specific examples/blueprints territory - a few questions for you:

Do we want to show actual examples or just snippets in docs

Should that be added in this PR or follow up

Ugly, deeply nested maps 🙈

Nested maps are, unfortunately, the best solution. We can help users by showing snippets in the docs.

Let's update README in the root module and show how to use this module "in general for ALB" and include multiple small chunks of configurations to illustrate different "actions" (forward, redirect, fixed-response, etc).

I've updated the main README to show some examples and then I added this https://github.com/bryantbiggs/terraform-aws-alb/blob/refactor/for-each/docs/patterns.md to start collecting contributions on usage patterns from the community. I'll open an issue to signal this and see if we get an Hacktoberfest contributors to these usage patterns! (ALBs and NLBs aren't my wheelhouse so, struggling a bit in the usage pattern area here 😅 )

main.tf

examples/complete-alb/main.tf

bryantbiggs · 2023-09-14T20:13:54Z

also, do you know whats up with the wrappers? I can't figure out why the CI keeps showing a diff, even if I manually fix it

spr-mweber3 · 2023-09-23T14:34:14Z

@antonbabenko Looks good to me.

LeszekBlazewski · 2023-10-02T12:53:15Z

Waiting for this feature too!

LeszekBlazewski · 2023-10-24T10:12:41Z

Any update on this PR? @antonbabenko

I think the majority of the work has been done and only few tweaks are needed to push this through. I think many consumers of the module can benefit from it!

antonbabenko

Looks very good (and sorry for such a long delay with the review)!

UPGRADE-9.0.md

.pre-commit-config.yaml

Co-authored-by: Anton Babenko <[email protected]>

spr-mweber3 · 2023-10-27T15:13:00Z

Almost at the finish line. Love you guys!

…on patterns

bryantbiggs · 2023-10-27T15:50:34Z

Not sure if CI will correct itself once this is merged, but I had to manually adjust the wrappers to get it to pass CI. I'll follow up with an issue to ask for contributions to the patterns doc to collect common configuration/usage patterns to make groking the nested maps easier for folks. For now, I think we're close enough so here we go 🚀

antonbabenko · 2023-10-27T15:50:39Z

@bryantbiggs I have just run the hook on this PR locally, and it behaves as on CI. Maybe you have the wrong versions of hcledit on your side? (blind guess)

antonbabenko

LGTM 🚀

bryantbiggs · 2023-10-27T15:52:15Z

hmm, this is what I have which is the latest

## [9.0.0](v8.7.0...v9.0.0) (2023-10-27) ### ⚠ BREAKING CHANGES * Refactor module to use maps instead of lists (#305) ### Features * Refactor module to use maps instead of lists ([#305](#305)) ([39d5627](39d5627))

antonbabenko · 2023-10-27T15:52:58Z

This PR is included in version 9.0.0 🎉

bryantbiggs · 2023-10-27T15:58:46Z

For all following this PR, or those who may find there way here - please come help us contribute the configuration snippets that demonstrate the common usage patterns under the new v9.0 module definition. Details can be found here, thanks all! #311

github-actions · 2023-12-14T02:04:51Z

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.

feat!: Refactor module to use maps instead of lists

9707518

bryantbiggs requested a review from antonbabenko August 23, 2023 16:16

ChristopheBougere reviewed Aug 24, 2023

View reviewed changes

main.tf Show resolved Hide resolved

fix: Only call target group attachment when a target_id is provided

d112a97

antonbabenko requested changes Sep 13, 2023

View reviewed changes

bryantbiggs added 2 commits September 14, 2023 15:49

fix: Updates from PR feedback

522c070

fix: Manually fix to pass CI checks

2cfe157

antonbabenko approved these changes Oct 27, 2023

View reviewed changes

UPGRADE-9.0.md Outdated Show resolved Hide resolved

UPGRADE-9.0.md Outdated Show resolved Hide resolved

.pre-commit-config.yaml Outdated Show resolved Hide resolved

bryantbiggs and others added 3 commits October 27, 2023 10:29

Update UPGRADE-9.0.md

d71d439

Co-authored-by: Anton Babenko <[email protected]>

Update UPGRADE-9.0.md

ad0a696

Co-authored-by: Anton Babenko <[email protected]>

chore: Update pre-commit versions

c016541

bryantbiggs added 4 commits October 27, 2023 11:18

docs: Update main README to ensure examples reflect current changes

a2fe2ed

docs: Add patterns file to allow contributions for common configurati…

e4822e1

…on patterns

chore: Boop the CI to see if it will flip back to green

3bc755e

fix: Update XFF value to follow API defaults

12bb6e3

bryantbiggs force-pushed the refactor/for-each branch from 9a16796 to 12bb6e3 Compare October 27, 2023 15:43

fix: Manually fix wrapper for now to pass CI

c6d603a

antonbabenko approved these changes Oct 27, 2023

View reviewed changes

bryantbiggs merged commit 39d5627 into terraform-aws-modules:master Oct 27, 2023
8 checks passed

bryantbiggs deleted the refactor/for-each branch October 27, 2023 15:52

This was referenced Oct 27, 2023

enable_xff_client_port No Longer Defaults to True in AWS Provider #308

Closed

Request for contributions - Usage patterns #311

Closed

bryantbiggs mentioned this pull request Dec 12, 2023

More than one attach target in target groups #316

Closed

github-actions bot locked as resolved and limited conversation to collaborators Dec 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Refactor module to use maps instead of lists #305

feat!: Refactor module to use maps instead of lists #305

bryantbiggs commented Aug 23, 2023 •

edited

Loading

antonbabenko commented Aug 23, 2023

bryantbiggs commented Aug 23, 2023

ChristopheBougere left a comment

spr-mweber3 commented Sep 5, 2023

lblazewski commented Sep 12, 2023

antonbabenko left a comment

antonbabenko Sep 13, 2023

bryantbiggs Sep 14, 2023

antonbabenko Oct 27, 2023

bryantbiggs Oct 27, 2023

bryantbiggs commented Sep 14, 2023

spr-mweber3 commented Sep 23, 2023

LeszekBlazewski commented Oct 2, 2023

LeszekBlazewski commented Oct 24, 2023

antonbabenko left a comment

spr-mweber3 commented Oct 27, 2023

bryantbiggs commented Oct 27, 2023

antonbabenko commented Oct 27, 2023

antonbabenko left a comment

bryantbiggs commented Oct 27, 2023

antonbabenko commented Oct 27, 2023

bryantbiggs commented Oct 27, 2023

github-actions bot commented Dec 14, 2023

feat!: Refactor module to use maps instead of lists #305

feat!: Refactor module to use maps instead of lists #305

Conversation

bryantbiggs commented Aug 23, 2023 • edited Loading

Description

Backwards incompatible changes

Added

Modified

Motivation and Context

Breaking Changes

How Has This Been Tested?

antonbabenko commented Aug 23, 2023

bryantbiggs commented Aug 23, 2023

ChristopheBougere left a comment

Choose a reason for hiding this comment

spr-mweber3 commented Sep 5, 2023

lblazewski commented Sep 12, 2023

antonbabenko left a comment

Choose a reason for hiding this comment

antonbabenko Sep 13, 2023

Choose a reason for hiding this comment

bryantbiggs Sep 14, 2023

Choose a reason for hiding this comment

antonbabenko Oct 27, 2023

Choose a reason for hiding this comment

bryantbiggs Oct 27, 2023

Choose a reason for hiding this comment

bryantbiggs commented Sep 14, 2023

spr-mweber3 commented Sep 23, 2023

LeszekBlazewski commented Oct 2, 2023

LeszekBlazewski commented Oct 24, 2023

antonbabenko left a comment

Choose a reason for hiding this comment

spr-mweber3 commented Oct 27, 2023

bryantbiggs commented Oct 27, 2023

antonbabenko commented Oct 27, 2023

antonbabenko left a comment

Choose a reason for hiding this comment

bryantbiggs commented Oct 27, 2023

antonbabenko commented Oct 27, 2023

bryantbiggs commented Oct 27, 2023

github-actions bot commented Dec 14, 2023

bryantbiggs commented Aug 23, 2023 •

edited

Loading