-
Notifications
You must be signed in to change notification settings - Fork 419
create alb-service #134
base: master
Are you sure you want to change the base?
create alb-service #134
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noted a bunch of places where "ELB" looks like it should be "ALB" but now I'm not totally sure what the correct terminology is. Seems like "ALBs" are considered a type of "ELB"? In any case, I think using "ALB" across the board for this module would reduce confusion with the existing ELB module.
I'll give this a try -- I like this project but using a Classic ELB is non-starter for me.
@@ -0,0 +1,179 @@ | |||
/** | |||
* The ELB module creates an ELB, security group |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/ELB/ALB/
*/ | ||
|
||
variable "name" { | ||
description = "ELB name, e.g cdn" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/ELB/ALB/
} | ||
|
||
variable "external_dns_name" { | ||
description = "The subdomain under which the ELB is exposed externally, defaults to the task name" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/ELB/ALB/
} | ||
|
||
variable "internal_dns_name" { | ||
description = "The subdomain under which the ELB is exposed internally, defaults to the task name" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/ELB/ALB/
* Outputs. | ||
*/ | ||
|
||
// The ELB name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/ELB/ALB/
value = "${aws_alb.main.id}" | ||
} | ||
|
||
// The ELB dns_name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/ELB/ALB/
value = "${aws_route53_record.internal.fqdn}" | ||
} | ||
|
||
// The zone id of the ELB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/ELB/ALB/
@@ -0,0 +1,233 @@ | |||
/** | |||
* The web-service is similar to the `service` module, but the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/web-service/alb-service/
* Usage: | ||
* | ||
* module "auth_service" { | ||
* source = "github.com/segmentio/stack/service" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait, isn't this wrong here and in stack/master/web-service/main.tf
? Seems like this line should be:
source = "github.com/segmentio/stack/alb-service"
no?
} | ||
|
||
variable "log_bucket" { | ||
description = "S3 bucket name to write ELB logs into" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/ELB/ALB/
@jalessio your comments are all valid. If this is an acceptable approach and we can agree on the caveats I listed, I'm happy to update the PR based on both comments and also fix the reference in web-service from your last comment. |
@slajax I'm not affiliated with Segment or this project so I can't weigh in on whether the maintainers will find this to be an acceptable approach. For my purposes I could just drop ELB support altogether but I understand the need to maintain backward compatibility. As far as how to structure this, on first pass it seems like one of these options would be nice:
|
@jalessio thanks for the feedback.
|
Just an update here. We also added alb to the internal service in our fork which we are actively maintaining. Feel free to hit us up if you are also working off a fork and want to share updates. |
@slajax Is https://github.com/HackCapital/stack your fork? The last update there is a few months older than your comment here... |
Yes that is the fork. |
overview
#101 attempts to overwrite web-service's ELB with ALB. Some of the reasons that this is important are:
...however it seems it was rejected due to the implementation.
this PR attempts to re-introduce this ALB implementation in a non breaking way by adding a different type of service that can be explicitly chosen as an alternative to the elb enabled
web-service
. I've called italb-service
- yuck, i know.tasks
note
if this approach is accepted in principal, then here are the questions up for discussion:
Seems to me we might have to think about a breaking change at some point to get this in with good logical naming, otherwise this project is going to be getting forked a lot because ALB is a huge improvement and while we really should try to support it in a somewhat non breaking manner, it's nearly impossible to ignore the fact that ELB doesn't support sockets at this point. I mean c'mon AWS, what were you thinking? Please accept this approach, my sanity depends on it.