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

Modules for ALB, NLB & corresponding listeners #13

Merged
merged 6 commits into from
Dec 20, 2017
Merged

Modules for ALB, NLB & corresponding listeners #13

merged 6 commits into from
Dec 20, 2017

Conversation

ringods
Copy link
Contributor

@ringods ringods commented Dec 18, 2017

Fix for: https://github.com/skyscrapers/core/issues/12

  • Module alb_listener between alb and alb_rule_target. Makes it easier to manage target groups, listeners and listener rules.
  • Modules nlb and nlb_listener.

…akes it easier to manage the target groups and the listeners.
Further split into `alb` and `alb_listener` module.
@ringods ringods changed the title WIP: Modules for ALB & NLB. Modules for ALB, NLB & corresponding listeners Dec 19, 2017
Copy link
Contributor

@MattiasGees MattiasGees left a comment

Choose a reason for hiding this comment

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

Look ok, for me this is a major version bump

Copy link
Contributor

@venturel venturel left a comment

Choose a reason for hiding this comment

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

It looks good also to me. I agree with Mattias on the major version bump

Copy link
Contributor

@SamClinckspoor SamClinckspoor left a comment

Choose a reason for hiding this comment

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

is there any value in the nlb and alb parts? I can understand for the listener but the main aws_lb modules don't do much

nlb/main.tf Outdated
# Create a new load balancer
resource "aws_lb" "nlb" {
load_balancer_type = "network"
name = "${var.project}-${var.environment}-${var.name}-nlb"
Copy link
Contributor

Choose a reason for hiding this comment

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

please use name_prefix

alb/main.tf Outdated
@@ -1,5 +1,6 @@
# Create a new load balancer
resource "aws_alb" "alb" {
resource "aws_lb" "alb" {
load_balancer_type = "application"
name = "${var.project}-${var.environment}-${var.name}-alb"
Copy link
Contributor

Choose a reason for hiding this comment

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

please use name_prefix

nlb/sg.tf Outdated
@@ -0,0 +1,11 @@
resource "aws_security_group" "sg_nlb" {
name = "sg_alb_${var.project}_${var.environment}_${var.name}"
Copy link
Contributor

Choose a reason for hiding this comment

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

please use name_prefix

@ringods
Copy link
Contributor Author

ringods commented Dec 19, 2017

@SamClinckspoor the reason is cost. Contrary to an ELB, we will no longer set up a application or network load balancer per application. The LB will be a shared resource.

For an application load balancer, we add HTTP and/or HTTPS listeners, then add aws_lb_listener_rule with different target groups to route to different hosts.

For a network load balancer, we add additional port specific listeners.

@SamClinckspoor
Copy link
Contributor

I mean, what value does the module bring. As far as I can tell the alb and nlb don't provide a lot of abstraction to make it easier to use compared to using the resource instead of the module

@ringods
Copy link
Contributor Author

ringods commented Dec 20, 2017

It's debatable indeed, but having the modules (lb + sg) makes the setup consistent module wise.

@ringods ringods merged commit 06af644 into master Dec 20, 2017
@ringods ringods deleted the alb-nlb branch December 20, 2017 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants