Fix bring-your-own-alb for plugin/ecs #4457

izaaklauer · 2023-01-27T15:46:33Z

Fixes #4456
Replaces #4457

What changed

Introduces a new field load_balancer_arn to the ecs plugin's deploy config. If specified, waypoint will use this alb rather than creating it's own.
Removes listener_arn field from same config

Motivation

Prior to this, to use an existing alb (and prevent waypoint from creating a new one), you would specify a listener_arn in the ecs deployment config. In practice, that was broken (see the above issue)

This approach wan't ideal anyway. Waypoint needs to modify the listener on every deployment, so if someone were to create the listener through terraform and wire the output into waypoint, that terraform would also try to change the default listener on the rule away from waypoint's deployment on every run.

Waypoint should own the listener, but can inherit an existing ALB.

Backwards-compat

While removing a field is generally a breaking change, because the field was broken anyway (see issue), I can't see how anyone could have been depending on it.

How do I test this?

I'd recommend starting with https://github.com/hashicorp/waypoint-examples/tree/main/aws/aws-ecs/nodejs. Run a few waypoint deploy's while observing the aws alb, and notice how the default rule moves traffic to the most recent target group on every release.

To test bring-your-own-alb, you can copy the arn and subnets of the alb waypoint created in the above testing into your deploy config

Example:

...
  deploy {
    use "aws-ecs" {
...
      alb {
        load_balancer_arn = "arn:aws:elasticloadbalancing:us-east-1:863953081260:loadbalancer/app/waypoint-ecs-hello-izaak-2/9d50bd2cdea67345"
        subnets      = ["subnet-0bd08267365065f8e","subnet-04ff6753f3ba35e49","subnet-02c9aaa7732de7eb7","subnet-047d5841370712b24","subnet-086d533750a535ea5","subnet-0f8dd2e1826266538"]
      }
    }
  }

Run a few more releases, and observe the same successful behavior.

Other fixes in this PR

Can now manage an ALB through terraform, and have waypoint use it
Can now allow waypoint to use an ALB with other listeners configured, without waypoint modifying the wrong listener.

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.

izaaklauer · 2023-01-27T15:51:07Z

builtin/aws/ecs/platform.go

+	var listener *elbv2.Listener
+	for _, l := range listeners.Listeners {
+		if *l.Port == int64(externalIngressPort) {
+			listener = l
 		}


Previously, we were always plucking the first listener from the ALB. Now, we make sure to modify the one that corresponds to our port of interest.

This is important if someone brings an ALB that already has a listener on another port that's unrelated to this waypoint app.

izaaklauer · 2023-01-27T15:51:34Z

builtin/aws/ecs/platform.go


-	if !newListener {
+	if listener != nil {
+		s.Update("Modifying existing ALB Listener to introduce target group")


There was no UI output previously! Now it reports progress to users.

izaaklauer · 2023-01-27T15:57:32Z

builtin/aws/ecs/releaser.go

@@ -51,107 +62,90 @@ func (r *Releaser) Release(
 	}
 	elbsrv := elbv2.New(sess)

-	dlb, err := elbsrv.DescribeLoadBalancers(&elbv2.DescribeLoadBalancersInput{


Here's a part of the old bug - if the user specified a listener in the deploy config, we would always try to describe the load balancer here. The deploy plugin didn't set the load balancer arn, so target.LoadBalancerArn would be empty string and we'd get a 400.

izaaklauer · 2023-01-27T15:59:19Z

builtin/aws/ecs/releaser.go

-	} else {
-		log.Info("load-balancer defined", "dns-name", *lb.DNSName)
-
-		lo, err := elbsrv.CreateListener(&elbv2.CreateListenerInput{


I don't think this would ever happen - the deploy plugin was already creating a listener if necessary. Now we're relying on that, and only modifying the listener here.

I'm honestly not sure about this use-case, but it's existing behavior and i'm not going to fight it here.

paladin-devops

This looks great - I think users will definitely take advantage of using an existing ALB (managed with Terraform)! I appreciate the well-commented changes - it helped me better understand what the platform and releaser plugins are intended to do in the first place.

paladin-devops · 2023-01-27T16:35:27Z

builtin/aws/ecs/platform.go

-			if err != nil {
-				return status.Errorf(codes.Internal, "failed to create listener: %s", err)
-			}
+	if alb == nil || alb.Arn == "" {


❓ I see that this if block already existed, but how is it enforced that the ALB is created before the listener is created? In the resource manager I can see that the ALB listener is sequentially after the ALB itself - is that what ensures that it is done first?

Argmapper magic! This resourceAlbListenerCreate function takes an *Resource_Alb as input, and goes into the argmapper soup along with the resourceAlbCreate function that provides it. Argmapper figures out the right order to invoke the whole string of resource functions.

builtin/aws/ecs/platform.go

paladin-devops · 2023-01-27T16:38:21Z

builtin/aws/ecs/platform.go


-		_, err := elbsrv.ModifyListenerWithContext(ctx, &elbv2.ModifyListenerInput{
-			ListenerArn:  listener.ListenerArn,
+			// Setting these on every deployment so that we pick up any potential changes


Co-authored-by: Joe <83741749+paladin-devops@users.noreply.github.com>

And properly using the load_balancer_arn field

The cert is a property of the listener, not the ALB, so it's OK for both of these to be set.

izaaklauer · 2023-01-28T01:17:18Z

builtin/aws/ecs/platform.go

@@ -1054,7 +1054,7 @@ func (p *Platform) resourceAlbListenerCreate(
 	}
 	log.Info("load-balancer defined", "dns-name", alb.DnsName)

-	s.Update("Looking for listeners for ALB %q", alb.Name)
+	s.Update("Looking for listeners for ALB %q", alb.Arn)


The name might not be defined if the user specified an arn.

izaaklauer added 5 commits January 27, 2023 10:42

changelog

82374c2

docs

a011631

First pass at removing the alb listener as input

8f6f0e5

Allowing bring-your-own-alb for ecs

5254429

github-actions bot added plugin plugin/aws website labels Jan 27, 2023

vercel bot deployed to Preview – waypoint-ui January 27, 2023 15:47 View deployment

vercel bot had a problem deploying to Preview – waypoint January 27, 2023 15:48 Failure

Changelog and docs

13444be

izaaklauer commented Jan 27, 2023

View reviewed changes

vercel bot deployed to Preview – waypoint-ui January 27, 2023 15:51 View deployment

izaaklauer commented Jan 27, 2023

View reviewed changes

izaaklauer changed the title ~~Ecs no listener~~ Fix bring-your-own-alb for plugin/ecs Jan 27, 2023

izaaklauer added 2 commits January 27, 2023 11:08

Always setting listener config, as we manage the listener.

ec027df

Allowing successful "releases" when deployments don't use an alb.

dec90fd

I'm honestly not sure about this use-case, but it's existing behavior and i'm not going to fight it here.

vercel bot deployed to Preview – waypoint-ui January 27, 2023 16:11 View deployment

izaaklauer mentioned this pull request Jan 27, 2023

Allow bringing your own alb listener #4453

Closed

Fixing changelog number

6470259

izaaklauer marked this pull request as ready for review January 27, 2023 16:18

izaaklauer requested a review from a team January 27, 2023 16:18

vercel bot deployed to Preview – waypoint-ui January 27, 2023 16:18 View deployment

izaaklauer requested review from a team, catsby and paladin-devops January 27, 2023 16:18

paladin-devops approved these changes Jan 27, 2023

View reviewed changes

Update builtin/aws/ecs/platform.go

c090257

Co-authored-by: Joe <83741749+paladin-devops@users.noreply.github.com>

vercel bot deployed to Preview – waypoint-ui January 27, 2023 18:00 View deployment

izaaklauer added 2 commits January 27, 2023 17:05

Removing listener_arn field

42da790

And properly using the load_balancer_arn field

Fixing tests

d75b0b6

vercel bot deployed to Preview – waypoint-ui January 27, 2023 22:07 View deployment

vercel bot deployed to Preview – waypoint-ui January 27, 2023 22:13 View deployment

Allow users to bring their own alb _and_ specify a cert

a99fff4

The cert is a property of the listener, not the ALB, so it's OK for both of these to be set.

vercel bot deployed to Preview – waypoint-ui January 27, 2023 22:31 View deployment

izaaklauer commented Jan 28, 2023

View reviewed changes

catsby approved these changes Feb 1, 2023

View reviewed changes

izaaklauer merged commit d9c5378 into main Feb 1, 2023

izaaklauer deleted the ecs-no-listener branch February 1, 2023 17:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bring-your-own-alb for plugin/ecs #4457

Fix bring-your-own-alb for plugin/ecs #4457

izaaklauer commented Jan 27, 2023 •

edited

Loading

izaaklauer Jan 27, 2023

izaaklauer Jan 27, 2023

izaaklauer Jan 27, 2023 •

edited

Loading

izaaklauer Jan 27, 2023

paladin-devops left a comment

paladin-devops Jan 27, 2023

izaaklauer Jan 27, 2023

paladin-devops Jan 27, 2023

izaaklauer Jan 28, 2023

Fix bring-your-own-alb for plugin/ecs #4457

Fix bring-your-own-alb for plugin/ecs #4457

Conversation

izaaklauer commented Jan 27, 2023 • edited Loading

What changed

Motivation

Backwards-compat

How do I test this?

Other fixes in this PR

izaaklauer Jan 27, 2023

Choose a reason for hiding this comment

izaaklauer Jan 27, 2023

Choose a reason for hiding this comment

izaaklauer Jan 27, 2023 • edited Loading

Choose a reason for hiding this comment

izaaklauer Jan 27, 2023

Choose a reason for hiding this comment

paladin-devops left a comment

Choose a reason for hiding this comment

paladin-devops Jan 27, 2023

Choose a reason for hiding this comment

izaaklauer Jan 27, 2023

Choose a reason for hiding this comment

paladin-devops Jan 27, 2023

Choose a reason for hiding this comment

izaaklauer Jan 28, 2023

Choose a reason for hiding this comment

izaaklauer commented Jan 27, 2023 •

edited

Loading

izaaklauer Jan 27, 2023 •

edited

Loading