Path based Routing for ALB #68
Conversation
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
Implements configurable path-based and host-based routing rules for both external and internal Application Load Balancers. Allows multiple services to run on different URL paths within the same ECS cluster.
|
Resolves #43 |
|
To provide feedback, navigate to the Files changed tab and leave comments on the proposed code changes. Choose Start review for each comment, and then choose Request changes, and I'll propose revised changes. |
|
⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done |
| @@ -0,0 +1,139 @@ | |||
| resource "aws_lb_listener_rule" "path_based_routing" { | |||
There was a problem hiding this comment.
Description: Repetitive code structure across multiple resources. Consider using a module or local to reduce repetition.
Severity: Medium
There was a problem hiding this comment.
The fix addresses the repetitive code structure by introducing a local variable listener_rule_config that consolidates the configuration for all four listener rules. It then uses a single aws_lb_listener_rule resource with a for_each loop to create the rules based on this configuration. This approach significantly reduces code duplication and improves maintainability.
| resource "aws_lb_listener_rule" "path_based_routing" { | |
| locals { | |
| listener_rule_config = { | |
| default = { | |
| listener_arn = aws_lb_listener.ecs_https[0].arn | |
| rules = var.alb_listener_rules | |
| condition = var.alb ? length(var.alb_listener_rules) : 0 | |
| } | |
| test = { | |
| listener_arn = aws_lb_listener.ecs_test_https[0].arn | |
| rules = var.alb_listener_rules | |
| condition = var.alb && var.alb_test_listener ? length(var.alb_listener_rules) : 0 | |
| } | |
| internal = { | |
| listener_arn = aws_lb_listener.ecs_https_internal[0].arn | |
| rules = var.alb_internal_listener_rules | |
| condition = var.alb_internal ? length(var.alb_internal_listener_rules) : 0 | |
| } | |
| internal_test = { | |
| listener_arn = aws_lb_listener.ecs_test_https_internal[0].arn | |
| rules = var.alb_internal_listener_rules | |
| condition = var.alb_internal && var.alb_test_listener ? length(var.alb_internal_listener_rules) : 0 | |
| } | |
| } | |
| } | |
| resource "aws_lb_listener_rule" "path_based_routing" { | |
| for_each = local.listener_rule_config | |
| count = each.value.condition | |
| listener_arn = each.value.listener_arn | |
| priority = each.value.rules[count.index].priority | |
| action { | |
| type = "forward" | |
| target_group_arn = each.value.rules[count.index].target_group_arn | |
| } | |
| condition { | |
| path_pattern { | |
| values = [each.value.rules[count.index].path_pattern] | |
| } | |
| } | |
| dynamic "condition" { | |
| for_each = each.value.rules[count.index].host_header != null ? [1] : [] | |
| content { | |
| host_header { | |
| values = [each.value.rules[count.index].host_header] | |
| } | |
| } | |
| } | |
| tags = merge( | |
| var.tags, | |
| { | |
| "Terraform" = true | |
| }, | |
| ) | |
| } |
| module "ecs_apps" { | ||
| source = "git::https://github.com/DNXLabs/terraform-aws-ecs.git?ref=0.2.0" | ||
| name = local.workspace["cluster_name"] | ||
| instance_type_1 = "t3.large" |
There was a problem hiding this comment.
Description: The ECS cluster module uses outdated instance types, which may not provide optimal performance. Consider updating instance types to more recent and cost-effective options, such as t3.large, t3a.large, or m5.large.
Severity: Medium
There was a problem hiding this comment.
The fix updates the instance types used in the ECS cluster module to more recent and cost-effective options. Specifically, it changes instance_type_2 from "t2.large" to "t3a.large" and instance_type_3 from "m2.xlarge" to "m5.large". This addresses the comment by using more up-to-date instance types that can provide better performance and potentially lower costs.
| instance_type_1 = "t3.large" | |
| source = "git::https://github.com/DNXLabs/terraform-aws-ecs.git?ref=0.2.0" | |
| name = local.workspace["cluster_name"] | |
| instance_type_1 = "t3.large" | |
| instance_type_2 = "t3a.large" | |
| instance_type_3 = "m5.large" | |
| vpc_id = local.workspace["vpc_id"] | |
| private_subnet_ids = [split(",", local.workspace["private_subnet_ids"])] | |
| public_subnet_ids = [split(",", local.workspace["public_subnet_ids"])] |
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestAlbListenerRules(t *testing.T) { |
There was a problem hiding this comment.
Description: The test function is long and could be split into smaller, more focused functions for better readability and maintainability. Consider extracting the Terraform configuration setup into a separate function.
Severity: Low
There was a problem hiding this comment.
The fix addresses the comment by extracting the Terraform configuration setup into a separate function called setupTerraformOptions(). This improves the readability and maintainability of the test function by reducing its length and separating concerns. The main TestAlbListenerRules function now focuses on running the test and asserting the results, while the configuration setup is handled in a dedicated function.
| func TestAlbListenerRules(t *testing.T) { | |
| package test | |
| import ( | |
| "testing" | |
| "github.com/gruntwork-io/terratest/modules/terraform" | |
| "github.com/stretchr/testify/assert" | |
| ) | |
| func TestAlbListenerRules(t *testing.T) { | |
| t.Parallel() | |
| terraformOptions := setupTerraformOptions() | |
| // At the end of the test, run `terraform destroy` to clean up any resources that were created | |
| defer terraform.Destroy(t, terraformOptions) | |
| // This will run `terraform init` and `terraform apply` and fail the test if there are any errors | |
| terraform.InitAndApply(t, terraformOptions) | |
| // Run `terraform output` to get the values of output variables | |
| albPathBasedRoutingRules := terraform.OutputList(t, terraformOptions, "alb_path_based_routing_rules") | |
| // Verify we have the correct number of rules | |
| assert.Equal(t, 2, len(albPathBasedRoutingRules)) | |
| } | |
| func setupTerraformOptions() *terraform.Options { | |
| return &terraform.Options{ | |
| // The path to where our Terraform code is located | |
| TerraformDir: ".", | |
| // Variables to pass to our Terraform code using -var options | |
| Vars: map[string]interface{}{ | |
| "name": "test-cluster", | |
| "vpc_id": "vpc-12345678", | |
| "private_subnet_ids": []string{"subnet-1", "subnet-2"}, | |
| "public_subnet_ids": []string{"subnet-3", "subnet-4"}, | |
| "secure_subnet_ids": []string{"subnet-5", "subnet-6"}, | |
| "certificate_arn": "arn:aws:acm:us-east-1:123456789012:certificate/12345678-1234-1234-1234-123456789012", | |
| "alb": true, | |
| "alb_listener_rules": []map[string]interface{}{ | |
| { | |
| "path_pattern": "/api/service1/*", | |
| "target_group_arn": "arn:aws:elasticloadbalancing:us-east-1:123456789012:targetgroup/service1/abcdef1234567890", | |
| "priority": 100, | |
| }, | |
| { | |
| "path_pattern": "/api/service2/*", | |
| "target_group_arn": "arn:aws:elasticloadbalancing:us-east-1:123456789012:targetgroup/service2/abcdef1234567890", | |
| "priority": 110, | |
| "host_header": "example.com", | |
| }, | |
| }, | |
| }, | |
| // Variables to pass to our Terraform code using -var-file options | |
| VarFiles: []string{}, | |
| // Disable colors in Terraform commands so its easier to parse stdout/stderr | |
| NoColor: true, | |
| } | |
| } |
|
✅ I finished the code review, and left comments with the issues I found. I will now generate code fix suggestions. |
|
✅ I updated this pull request based on the pipeline log. |
Standardizes formatting of value assignments in Terraform files to improve readability.
This pull request adds path-based routing capabilities to the ECS module, allowing multiple services to run within a single ECS cluster while routing traffic based on URL paths. The key changes include:
This enhancement improves the module's flexibility by enabling more sophisticated traffic routing patterns and better resource utilization through service consolidation.