feat: Add support for creating a security group along with the load balancer #273

bryantbiggs · 2023-01-29T22:22:52Z

Description

Add support for creating a security group along with the load balancer

Motivation and Context

All ALBs created require a security group which then means another module definition for users. Instead, this now adds the ability to add the necessary rules directly into the ALB module. In the future, I'd like to look at mapping the listeners into rules to where users only need to specify the port and protocol once and this will create the necessary network access (they will still have to add CIDR blocks, etc., to know where that traffic is coming from or going to, but the port and protocol won't be duplicated)

Breaking Changes

No

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

…alancer

antonbabenko

Minor comments here and there...

antonbabenko · 2023-02-03T12:01:39Z

main.tf

+  name        = var.security_group_use_name_prefix ? null : local.security_group_name
+  name_prefix = var.security_group_use_name_prefix ? "${local.security_group_name}-" : null
+  description = var.security_group_description
+  vpc_id      = data.aws_subnet.this[0].vpc_id


There is already var.vpc_id, so no need to use data-source aws_subnet.this to fetch it.

updated in 0ad1649

antonbabenko · 2023-02-03T12:04:19Z

main.tf

+  description = var.security_group_description
+  vpc_id      = data.aws_subnet.this[0].vpc_id
+
+  tags = merge(var.tags, var.security_group_tags)


Let's add Name tag at the end, as we do for other resources already (like this one):

{ "Name" = ... (something like `var.security_group_use_name_prefix ? null : local.security_group_name` maybe) },

added in 0ad1649

antonbabenko · 2023-02-03T12:10:30Z

examples/complete-alb/main.tf

-  subnets         = data.aws_subnets.all.ids
+  vpc_id  = module.vpc.vpc_id
+  subnets = module.vpc.public_subnets
+  security_group_rules = {


Let's add back the line and the call to security-group module as an example:

security_groups = [module.security_group.security_group_id]

re-added in 0ad1649

antonbabenko · 2023-02-03T12:10:41Z

examples/complete-alb/main.tf

-  user_pool_id = aws_cognito_user_pool.this.id
+  tags = {
+    Example    = local.name
+    GithubRepo = "terraform-aws-eks"


Suggested change

GithubRepo = "terraform-aws-eks"

GithubRepo = "terraform-aws-alb"

whoops - how did that get there 😅

## [8.3.0](v8.2.2...v8.3.0) (2023-02-07) ### Features * Add support for creating a security group along with the load balancer ([#273](#273)) ([8232b47](8232b47))

antonbabenko · 2023-02-07T19:20:54Z

This PR is included in version 8.3.0 🎉

gidimariastorm · 2023-02-08T13:49:17Z

please rollback or fix it.
When I'm creating a NLB I'm receiving this error:

│ Error: creating network Load Balancer: InvalidConfigurationRequest: Security groups are not supported for load balancers with type 'network'
│       status code: 400, request id: 81f69f77-12d9-44ec-8cf3-90131a19631d
│ 
│   with module.nlb.aws_lb.this[0],
│   on .terraform/modules/nlb/main.tf line 5, in resource "aws_lb" "this":
│    5: resource "aws_lb" "this" {

jcolfej · 2023-02-08T16:00:01Z

Why is create_security_group enabled by default ?
This is breaking change for all user that previously use the module.
We need to add create_security_group = false to continue to use the module now...

bryantbiggs · 2023-02-08T16:23:59Z

This is breaking change for all user that previously use the module.

Could you elaborate on why this is a breaking change? I understand that there was an unfortunate bug which has since been patched in https://github.com/terraform-aws-modules/terraform-aws-alb/releases/tag/v8.3.1, but otherwise its not a breaking change

jcolfej · 2023-02-09T16:39:44Z

My bad, it's not breaking change, I'm a bit strong with my words...

Just that the change to this new version means that all users of the previous versions will have, on next apply, a creation of a security_group, which is useless, and which will also modify the loadbalancer for nothing.

It would have been better to set create_security_group = false by default to keep the old way of working and to allow new implementations to use this feature if needed.

falc410 · 2023-03-01T12:17:25Z

Also broke our projects since we already had a security group and now the module wanted to create another one. So I second that you should set create_security_group = false by default

github-actions · 2023-04-01T02:10:31Z

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: Add support for creating a security group along with the load b…

a840ae3

…alancer

bryantbiggs requested a review from antonbabenko January 29, 2023 22:26

antonbabenko reviewed Feb 3, 2023

View reviewed changes

chore: Update based on PR review feedback

0ad1649

antonbabenko approved these changes Feb 7, 2023

View reviewed changes

antonbabenko merged commit 8232b47 into terraform-aws-modules:master Feb 7, 2023

antonbabenko pushed a commit that referenced this pull request Feb 7, 2023

chore(release): version 8.3.0 [skip ci]

ffc7c9b

## [8.3.0](v8.2.2...v8.3.0) (2023-02-07) ### Features * Add support for creating a security group along with the load balancer ([#273](#273)) ([8232b47](8232b47))

bryantbiggs deleted the feat/security-group branch February 7, 2023 19:21

jcolfej mentioned this pull request Feb 14, 2023

8.3.0 alb target group stickiness not working. #275

Closed

1 task

github-actions bot locked as resolved and limited conversation to collaborators Apr 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for creating a security group along with the load balancer #273

feat: Add support for creating a security group along with the load balancer #273

bryantbiggs commented Jan 29, 2023

antonbabenko left a comment

antonbabenko Feb 3, 2023

bryantbiggs Feb 7, 2023

antonbabenko Feb 3, 2023

bryantbiggs Feb 7, 2023

antonbabenko Feb 3, 2023

bryantbiggs Feb 7, 2023

antonbabenko Feb 3, 2023

bryantbiggs Feb 7, 2023

antonbabenko commented Feb 7, 2023

gidimariastorm commented Feb 8, 2023 •

edited

Loading

jcolfej commented Feb 8, 2023

bryantbiggs commented Feb 8, 2023

jcolfej commented Feb 9, 2023

falc410 commented Mar 1, 2023

github-actions bot commented Apr 1, 2023

	GithubRepo = "terraform-aws-eks"
	GithubRepo = "terraform-aws-alb"

feat: Add support for creating a security group along with the load balancer #273

feat: Add support for creating a security group along with the load balancer #273

Conversation

bryantbiggs commented Jan 29, 2023

Description

Motivation and Context

Breaking Changes

How Has This Been Tested?

antonbabenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonbabenko commented Feb 7, 2023

gidimariastorm commented Feb 8, 2023 • edited Loading

jcolfej commented Feb 8, 2023

bryantbiggs commented Feb 8, 2023

jcolfej commented Feb 9, 2023

falc410 commented Mar 1, 2023

github-actions bot commented Apr 1, 2023

gidimariastorm commented Feb 8, 2023 •

edited

Loading