Skip to content
This repository has been archived by the owner on Jan 8, 2024. It is now read-only.

Allow bringing your own alb listener #4453

Closed
wants to merge 3 commits into from
Closed

Allow bringing your own alb listener #4453

wants to merge 3 commits into from

Conversation

izaaklauer
Copy link
Contributor

Merge after #4444

Prior to this, our docs claimed we allowed users to bring their own alb listeners: https://developer.hashicorp.com/waypoint/plugins/aws-ecs#alb-listener_arn

But in practice, they would get this error:

$ waypoint deploy

» Deploying hello-app...
✓ Running deploy v22
✓ Deployment resources created
✓ Discovered service subnets
✓ Discovered alb subnets
✓ Using external security group hello-app-inbound
✓ Using internal security group hello-app-inbound-internal
✓ Using existing log group waypoint-logs
✓ Using existing execution IAM role "ecr-hello-app"
✓ Registered Task definition: waypoint-hello-app
✓ Using existing ECS cluster waypoint
✓ Created target group hello-app-01GQNGJSKZ557FNN7JPG7
✓ Modified ALB Listener to introduce target group
✓ Created ECS Service hello-app-01GQNGJSKZ557FNN7JPG7

» Performing operation locally
✓ Finished building report for ecs deployment
✓ Determining status of ecs service hello-app-01GQNGJSKZ557FNN7JPG7
✓ Found existing ECS cluster: waypoint
⚠️ 1 cluster READY, 1 service READY, 1 task MISSING
⚠️ Waypoint detected that the current deployment is not ready, however your application
might be available or still starting up.

» Releasing...

» Performing operation locally
✓ Running release v17
! ValidationError: A load balancer ARN must be specified
        status code: 400, request id: f6328bdb-0616-484e-8654-140b8c2c1ca9

Waypoint hcl contents:

project = "hello-app"

app "hello-app" {
  build {
    use "pack" {}
    registry {
      use "aws-ecr" {
        region     = "us-east-1"
        repository = "waypoint-example"
        tag        = "latest"
      }
    }
  }

  deploy {
    use "aws-ecs" {
      region = "us-east-1"
      memory = "512"
      alb {
        listener_arn = "arn:aws:elasticloadbalancing:us-east-1:863953081260:listener/app/waypoint-ecs-hello-app/3cdb806e2340b21e/1c2034ee6148da78"
        subnets      = ["subnet-0bd08267365065f8e","subnet-04ff6753f3ba35e49","subnet-02c9aaa7732de7eb7","subnet-047d5841370712b24","subnet-086d533750a535ea5","subnet-0f8dd2e1826266538"]
      }

    }
  }
}

This change brings us in line with our docs.

Prior to this, our docs claimed we allowed users to bring their own alb listeners: https://developer.hashicorp.com/waypoint/plugins/aws-ecs#alb-listener_arn

But in practice, they would get this error:

```
$ waypoint deploy

» Deploying hello-app...
✓ Running deploy v22
✓ Deployment resources created
✓ Discovered service subnets
✓ Discovered alb subnets
✓ Using external security group hello-app-inbound
✓ Using internal security group hello-app-inbound-internal
✓ Using existing log group waypoint-logs
✓ Using existing execution IAM role "ecr-hello-app"
✓ Registered Task definition: waypoint-hello-app
✓ Using existing ECS cluster waypoint
✓ Created target group hello-app-01GQNGJSKZ557FNN7JPG7
✓ Modified ALB Listener to introduce target group
✓ Created ECS Service hello-app-01GQNGJSKZ557FNN7JPG7

» Performing operation locally
✓ Finished building report for ecs deployment
✓ Determining status of ecs service hello-app-01GQNGJSKZ557FNN7JPG7
✓ Found existing ECS cluster: waypoint
⚠️ 1 cluster READY, 1 service READY, 1 task MISSING
⚠️ Waypoint detected that the current deployment is not ready, however your application
might be available or still starting up.

» Releasing...

» Performing operation locally
✓ Running release v17
! ValidationError: A load balancer ARN must be specified
  	status code: 400, request id: f6328bdb-0616-484e-8654-140b8c2c1ca9
```

Waypoint hcl contents:
```
project = "hello-app"

app "hello-app" {
  build {
    use "pack" {}
    registry {
      use "aws-ecr" {
        region     = "us-east-1"
        repository = "waypoint-example"
        tag        = "latest"
      }
    }
  }

  deploy {
    use "aws-ecs" {
      region = "us-east-1"
      memory = "512"
      alb {
        listener_arn = "arn:aws:elasticloadbalancing:us-east-1:863953081260:listener/app/waypoint-ecs-hello-app/3cdb806e2340b21e/1c2034ee6148da78"
        subnets      = ["subnet-0bd08267365065f8e","subnet-04ff6753f3ba35e49","subnet-02c9aaa7732de7eb7","subnet-047d5841370712b24","subnet-086d533750a535ea5","subnet-0f8dd2e1826266538"]
      }

    }
  }
}

```

This change brings us in line with our docs.
Comment on lines +17 to +20
oneof lb_reference {
string load_balancer_arn = 5;
string listener_arn = 8;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks like it could be a breaking protobuf change, but I don't think it is! It's not changing field numbers. I also tried releasing a deployment made with the previous compiled protobufs, and it worked.

@@ -51,43 +54,119 @@ func (r *Releaser) Release(
}
elbsrv := elbv2.New(sess)

dlb, err := elbsrv.DescribeLoadBalancers(&elbv2.DescribeLoadBalancersInput{
LoadBalancerArns: []*string{&target.LoadBalancerArn},
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's the bug. If you specify your own target group arn, target.LoadBalancerArn would be an empty string, and you'd get a 400 on this call.

// or the lb listener if the user specified it.
oneof lb_reference {
string load_balancer_arn = 5;
string listener_arn = 8;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the existing logic, a user could specify a listener arn in the platform config, but we weren't making that available to the releaser.

@izaaklauer izaaklauer marked this pull request as draft January 26, 2023 15:31
@izaaklauer
Copy link
Contributor Author

Converted this back to a draft for now - thinking about modifying it to take an alb arn as input instead of a listener arn. Waypoint should probably always "own" the alb listener, as it's going to need to modify it for every deployment.

@izaaklauer
Copy link
Contributor Author

The problem here is that if someone wants to manage their ALB through terraform, they need to have terraform create the alb listener, and give that to waypoint. But if terraform creates the listener, it also needs to create the listener's default rule (which waypoint needs to modify). So every time you run the terraform after wayoint modifies the listener, terraform modifies it back the the default state, and the app loses traffic!

Replacing this pr with #4457

@izaaklauer izaaklauer closed this Jan 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant