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: Add load_balancers parameter to associate a CLB (Classic Load Balancer) to worker groups ASG #992

Conversation

hacker65536
Copy link
Contributor

@hacker65536 hacker65536 commented Aug 28, 2020

PR o'clock

Description

Add load_balancers for clb be associated to asg.

Resolves #1036

Checklist

@dpiddockcmp
Copy link
Contributor

Please can you duplicate this change in the ASG in workers_launch_template.tf, thank you.

@barryib
Copy link
Member

barryib commented Aug 28, 2020

@hacker65536 Thanks for your contribution.

I was just wondering what is your actual use case. What you're trying to achieve by adding a load balancer to all your worker nodes in a kubernetes cluster. Is the kubernetes model does't fit here (using k8s service or ingress) ?

@hacker65536
Copy link
Contributor Author

I'm trying to using clb + envoy to handle grpc app.

@barryib barryib added this to the v13.0.0 milestone Sep 2, 2020
@@ -37,6 +37,11 @@ resource "aws_autoscaling_group" "workers_launch_template" {
"target_group_arns",
local.workers_group_defaults["target_group_arns"]
)
load_balancers = lookup(
var.worker_groups[count.index],
Copy link

Choose a reason for hiding this comment

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

Shouldn't this line be:

var.worker_groups_launch_template[count.index],

?

Choose a reason for hiding this comment

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

Thank you

@barryib barryib changed the title feat: Add paramter of clb to be associated to the worker_node feat: Add load_balancers parameter to associate a CLB (Classic Load Balancer) to worker groups ASG Oct 5, 2020
@barryib barryib merged commit 8c8f4b5 into terraform-aws-modules:master Oct 5, 2020
@barryib
Copy link
Member

barryib commented Oct 5, 2020

Thanks @hacker65536 for your contribution.

@mumoshu
Copy link

mumoshu commented Oct 6, 2020

Is the kubernetes model does't fit here (using k8s service or ingress) ?

Using K8s service LBs makes it harder to recreate the cluster, because you need to use e.g. Route 53 to switch traffic from the old to the new cluster. So I'd use service LBs and (current) AWS ingresses only for dev clusters that can be deleted without care. So this feature should generally be useful for any production setup. Just my two cents 😃

@barryib
Copy link
Member

barryib commented Oct 6, 2020

Is the kubernetes model does't fit here (using k8s service or ingress) ?

Using K8s service LBs makes it harder to recreate the cluster, because you need to use e.g. Route 53 to switch traffic from the old to the new cluster. So I'd use service LBs and (current) AWS ingresses only for dev clusters that can be deleted without care. So this feature should generally be useful for any production setup. Just my two cents 😃

Great. Thanks @mumoshu for the clarification.

barryib pushed a commit to Polyconseil/terraform-aws-eks that referenced this pull request Oct 25, 2020
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unintended update when attaching an ELB to an ASG created by a worker launch template
6 participants