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

ecs_load_balancers optional attributes #168

Open
HebertCL opened this issue Jul 19, 2022 · 4 comments
Open

ecs_load_balancers optional attributes #168

HebertCL opened this issue Jul 19, 2022 · 4 comments
Labels
bug 🐛 An issue with the system

Comments

@HebertCL
Copy link

Describe the Bug

I am trying to provision an ECS service + ECS task using an ALB. To configure the options for load balancer, I am using this configuration:

locals {
  alb = [{
    container_name   = coalesce(var.alb_container_name, module.this.id)
    container_port   = var.container_port
    elb_name         = null
    target_group_arn = var.target_group_arn
  }]
}

Such local configuration goes directly to the module input like so:

module "ecs_alb_service_task" {
  source  = "cloudposse/ecs-alb-service-task/aws"
  version = "0.64.1"

  ...
  ecs_load_balancers                 = local.alb
...

  context = module.this.context
}

Both validation and plan runs as expected, but during the apply operation a TF error is returned:

module.ecs_alb_service_task.aws_ecs_service.ignore_changes_task_definition[0]: Creating...
╷
│ Error: error creating ECS service (mcoins-sandbox-analytics-denormalizer): InvalidParameterException: Load Balancer Name can not be blank.
│
│   with module.ecs_alb_service_task.aws_ecs_service.ignore_changes_task_definition[0],
│   on .terraform/modules/ecs_alb_service_task/main.tf line 351, in resource "aws_ecs_service" "ignore_changes_task_definition":
│  351: resource "aws_ecs_service" "ignore_changes_task_definition" {
│
╵

Which means I'm sending an empty string to AWS API and causing the issue. This led me to try getting rid of the elb_name value, and getting this other error:

│ Error: Invalid value for input variable
│
│   on main.tf line 110, in module "ecs_alb_service_task":
│  110:   ecs_load_balancers                 = local.alb
│
│ The given value is not suitable for
│ module.ecs_alb_service_task.var.ecs_load_balancers declared at
│ .terraform/modules/ecs_alb_service_task/variables.tf:11,1-30: element 0:
│ attribute "elb_name" is required.

I've looked into AWS API documentation and Terraform Provider and make sure that only container_port and container_name are required arguments so you can choose between classic ELB or ALB/NLB.

I suggest the option here is to add the optional keyword to both elb_name and target_group_arn arguments, so whenever values are supplied here it's not mandatory to supply both. I can also supply a PR with the suggested change if you're ok with it.

Expected Behavior

Terraform should be able to apply with either Classic or ALB/NLB values, but not require both.

Steps to Reproduce

Steps to reproduce the behavior:

  1. Run an example that supplies ecs_load_balancers value with either classic ELB name or ALB target group ARN.
  2. Run terraform init && terraform apply

Screenshots

Code snippets and errors above

Environment (please complete the following information):

  • OS: OSX, arm64
  • Version: Monterrey 12.4
  • Terraform version:
Terraform v1.2.4
on darwin_arm64
+ provider registry.terraform.io/hashicorp/aws v4.22.0
+ provider registry.terraform.io/hashicorp/local v2.2.3
+ provider registry.terraform.io/hashicorp/null v3.1.1

Additional Context

None.

@HebertCL HebertCL added the bug 🐛 An issue with the system label Jul 19, 2022
@kunalsawhney
Copy link

@HebertCL were you able to resolve this ?

@HebertCL
Copy link
Author

Hi @kunalsawhney, since my use case was very specific and not likely to use classic LB I decided to fork on this repo and simplify the configuration to only expect ALB connectivity. I know this is not flexible nor the most ideal solution, but it was enough for me to move forward. I think for now this issue can be closed.

@prelegalwonder
Copy link

I was able to use this successfully by setting elb_name = null

@jeshicawang
Copy link

jeshicawang commented Sep 27, 2023

confirming elb_name: null works!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 An issue with the system
Projects
None yet
Development

No branches or pull requests

4 participants