design(ecs): multiple target group support design doc

[iamhopaul123]

Multiple target group support: new registering target API and current API improve for ECS.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[mergify]

Codebuild (Continuous Integration) build failed for current commits. Please check log and resolve before PR is merged.

[karlpatr]

This will need to be updated to reflect the desired host port for the container target.

[karlpatr]

Depending on the order of operations, use case #2 would either effectively make this.targetContainers a scratchpad that updates on each target registration (if depth first) or not work properly (if both registrations happen before both attachments). Do you know which is the case?

If the ordering is register/attach/register/attach I would suggest an annotation that the property is unstable/transient.

[iamhopaul123]

I think the first one will be the case. However, I do agree that if we are going to implement this method then it should be annotated.

BTW, do you have any question or concern about the new API?

[karlpatr]

Added thoughts below :)

[karlpatr]

I was requested to give this a review from #3891 since I have a real-world service that needs the second use case and proposed an alternative patch. This API design does give me what I need.

I am a little concerned that it skips the abstraction layer of container:host port mapping I'm used to dealing with since that means that changing a port in software necessitates a target update, but I don't know how likely or practical that concern is in the real world.

[iamhopaul123]

I was requested to give this a review from #3891 since I have a real-world service that needs the second use case and proposed an alternative patch. This API design does give me what I need.

I am a little concerned that it skips the abstraction layer of container:host port mapping I'm used to dealing with since that means that changing a port in software necessitates a target update, but I don't know how likely or practical that concern is in the real world.

Thank you @karlpatr for your review! Really appreciate your feedback. And to address your concern about the port mapping, I would say it is a good point and will take into consideration when updating and finalizing the design doc.

[karlpatr]

After a night of thinking this over, I do have a few more pieces of feedback; I do want to reiterate that this proposal already meets my need as it stands--these are just friction points that might benefit from additional thought.

The proposed improvement to the current API has a comment that the array targets should only accept one target, and further maps a required function return to that element; I think one of the greatest strengths of CDK is the ability to navigate the API through autocompletion, but that isn't available here. I think documentation can definitely fill that gap, but am calling it out as not intuitive.

The call to service.registerTarget also can lead to an unintended misconfiguration if it is not inlined properly. As noted above, that function creates a scratchpad; when inlined I agree that each call will be properly applied, but the API allows this declaration style as well:

const target1 =  service.registerTarget({
  containerName: "myContainer",
  containerPort: 8080
});
const target2 = service.registerTarget({
  containerName: "myContainer",
  containerPort: 9080
});
listener1.addTargets('api', {
  targets: [target1],
  pathPattern: "/api/*"
  ... // other target group props and rule props
});
listener2.addTargets('management', {
  targets: [target2],
  pathPattern: "/management/*"
});

In that case both listeners would actually be using the settings for target2 due to the overwrite of the scratchpad before any targets have been attached. The simple answer is "don't do that", but I thought it's worth pointing out that the API allows people to shoot themselves in the foot. You could throw an exception if you see two attachments in a row after a register to prevent that edge case.

[karlpatr]

Additional feedback for the new API:

While I think that registerContainerTargets does describe what is happening to the service, implicitly it feels like that configuration block is designed to do much more than that and depending on what the possible target types are I would suggest a name that I'd be more likely to search for when trying to solve the problem of connecting a load balancer to a service.

From a broader perspective, I'm also unsure if being able to set up the listener-service relationship from either service or listener methods will actually reduce the clarity of the API; as a CDK user I would want to know what the benefits and drawbacks of each approach were and be concerned about feature parity between the two methods as well as whether the generated templates would be compatible if I decided to change my approach.

[iamhopaul123]

Additional feedback for the new API:

While I think that registerContainerTargets does describe what is happening to the service, implicitly it feels like that configuration block is designed to do much more than that and depending on what the possible target types are I would suggest a name that I'd be more likely to search for when trying to solve the problem of connecting a load balancer to a service.

From a broader perspective, I'm also unsure if being able to set up the listener-service relationship from either service or listener methods will actually reduce the clarity of the API; as a CDK user I would want to know what the benefits and drawbacks of each approach were and be concerned about feature parity between the two methods as well as whether the generated templates would be compatible if I decided to change my approach.

Thanks for more feedback! I do agree that the enhanced API suffers from that kind of issues and me and my teammates might finalize the design doc with only the new API, since the template that generated by the new API will be the same as the current one which set up the listener-service relationship from listeners. In terms of clarity, there are many nested interfaces in the new API, which might be simplify by introducing default values.

[iamhopaul123]

Hi @karlpatr, your feedback is really helpful and valued. Just updated "improving current API" part according to your feedback as well as the method in #3891. Feel free to review and leave any comment!

[mergify]

Continuous integration build failed

[rix0rrr]

Without having read through everything thoroughly (bad me!), I have some observations upon skimming:

• I would hate to specialize anything in the ELB package towards ECS. The dependency arrow should point the other way. That means I definitely do not want an ecsTargets property in my listener.addTargets() call. We should make sure that our method of indirecting (IApplicationLoadBalancerTarget) can do everything to the ALB configuration that ECS needs to operate, but as a generic mechanism, without ALB having to be aware of what is using it.
• alb.addListener(... { targets: [ ] }) is not the only way to attach services to TargetGroups. Using targets directly is a convenience feature which will automatically create a TG for you. If you want control over the target groups, you create them yourself with the targets that you need, or call targetGroup.addTarget().

That is, registering a service into multiple targetgroups should be as easy as:

targetGroup1.addTarget(ecsService);
targetGroup2.addTarget(ecsService);

And this would probably work today already, wouldn't it? The only feature left to implement seems to be more control over the target container/target port.

• Load balancer targets are represented by an IApplicationLoadBalancerTarget. Yes, BaseService implements IApplicationLoadBalancerTarget and has a default implementation, but there can be other implementations of IApplicationLoadBalancerTarget with more finesse and more control. A pattern we've taken to for other services is something like this:

targetGroup1.addTarget(new EcsServiceTarget(ecsService, {
   // configuration here
})));

You can leave the default implementation (using the first port on the first essential container) of EcsService implements IApplicationLoadBalancerTarget , and add another integration class with more control for the people that use it.

In that sense, I love @karlpatr's proposal the most, which is essentially this, but the difference is how you get the integration class:

targetGroup1.addTarget(new EcsServiceTarget(ecsService, { ... }));
// vs
targetGroup1.addTarget(ecsService.registerTarget({ ... }));

I think I even prefer the 2nd one, though it's slightly breaking with some conventions that we have in other services (where we use the 1st approach).

We don't have this in other services though is because the 2nd approach (factory method) requires a dependency on the ELB package, whereas in other integration scenarios (say: Lambda and SNS) we avoid having dependencies between packages at all, and instead put the integration class into its own package. Since that ship has sailed here already (ECS already depends on ELB package), we're free to use the 2nd approach.

• One additional comment on this proposal: I would not call the method registerTarget(), and I would avoid having it have side effects. All state mutation should happen at the time when attachToApplicationLoadBalancer() is called on the object that is returned from the factory method. That is, calling registerTarget() and then dropping the object on the floor (without actually using it with a load balancer) should not have any side effects.

I would propose a method name without a verb to avoid the impression of a side effect happening, like loadBalancerTarget().

targetGroup1.addTarget(ecsService);
targetGroup2.addTarget(ecsService.loadBalancerTarget({
  containerName: 'mycontainer',
  containerPort: 8080
})); 

The loadBalancerTarget() can and probably should do validation on the presence of the given containerName and the fact whether a port mapping exists for the containerPort (as required).

[iamhopaul123]

Hi @rix0rrr, thanks for the review. Have you taken a look at my previous commit? This has the same as targetGroup2.addTarget(ecsService.loadBalancerTarget({ containerName: 'mycontainer', containerPort: 8080 })); . However, the only reason that we don't like this method is because of the ordering problem addressed in @karlpatr's comment (I do agree even though that might be the only way that doesn't need to do any extra work to ELBv2 package). @pkandasamy91 also told me that you don't like that kind of way because of the atomicity problem.

"In that sense, I love @karlpatr's proposal the most, which is essentially this..."

FMHO isn't this also adding a new property "targetPort" only for ECS in ELBv2's target group class, instead of creating a new integration class?

Besides, I don't think introducing a new integration class will help avoid the ordering problem. It will instead be an encapsulated alternative for "targetGroup1.addTarget(service.registerTarget(...))", which still suffer from the "register - add target -register another" kind of ordering problem if users put "service.registerTarget(...)" outside.

From ECS's customers' perspective, the current proposal may be the best one, which is easy and robust for them to use. Hence, I do admit that will lead to a new "ECS-only" property in the ELBv2 package. What do you think? @SoManyHs @pkandasamy91

[mergify]

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

• PR title type(scope): text
  • type: fix, feat, refactor go into CHANGELOG, chore is hidden
  • scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
  • text: use all lower-case, do not end with a period, do not include issue refs
• PR Description
  • Rationale: describe rationale of change and approach taken
  • Issues: indicate issues fixed via: fixes #xxx or closes #xxx
  • Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
• Testing
  • Unit test added. Prefer to add a new test rather than modify existing tests
  • CLI or init templates change? Re-run/add CLI integration tests
• Documentation
  • README: update module README to describe new features
  • API docs: public APIs must be documented. Copy from official AWS docs when possible
  • Design: for significant features, follow design process

[SoManyHs]

Agree that we shouldn't have a method that unexpectedly changes state or requires order-dependent method calls.

Also agree that we shouldn't add an ECS-specific target in the ELB library -- feels like that breaks the philosophy of how we are using interfaces. Is it possible to invert the dependency (ie. go with the approach of attaching a service's container to a target group) without modifying the ELB lib?

[rix0rrr]

iamhopaul:
However, the only reason that we don't like this method is because of the ordering problem addressed in @karlpatr's comment

karlpatr:
In that case both listeners would actually be using the settings for target2 due to the overwrite of the scratchpad before any targets have been attached.

If that is truly what happens, I would classify that as an implementation bug, but I don't think this is an inherent consequence of this kind of API. The whole idea of a "scratch pad" is an implementation detail that doesn't need to happen. As I stated before in my response, registerTarget() should not have any side effects. All the "scratch pad" information should be locked up in the IApplicationLoadBalancerTarget object that gets returned from that function, and should take effect once its attachToXxx() method is called.

@pkandasamy91 also told me that you don't like that kind of way because of the atomicity problem.

Not sure atomicity is the right word (or I might misunderstand what you're saying here) but in general I don't like ordering constraints, that is true, because it's just one more way for users to mess things up. In a very simple example:

lb.registerListener(listener);
listener.addTarget(target);

# VS
listener.addTarget(target);
lb.registerListener(listener);

Should do the same thing (Which is actually trickier than it looks, because to make it all Just Work for you, we need to modify the security group of the LB which we only have access to once everything is hooked up). What I find worst about this, is that IF there was an ordering dependency (must hook up this first, and then the other thing) and you did it wrong, there's no way for the user to tell that they did it wrong.

However, in this case, the pattern is:

taskDef.addContainer('foo');
service.referenceContainer('foo');

And the second line could throw a descriptive error if you didn't do things in the right order, which I find much less troublesome.

Hence, I do admit that will lead to a new "ECS-only" property in the ELBv2 package.

Let me be more blunt: I will not have it, especially as I'm not convinced that an integration class couldn't do the same job as effectively.

SoManyHs:
Is it possible to invert the dependency (ie. go with the approach of attaching a service's container to a target group) without modifying the ELB lib?

Not really. The underlying mechanism still relies on SOMETHING implementing IApplicationLoadBalancerTarget, its attachToXxx() getting called, and that method doing:

1. Whatever side effects it needs to
2. Returning some properties to the LB which will configure it in a certain way.

In fact, this is 80% of the implementation of loadBalancerTarget() that doesn't suffer from any ordering effects. The idea is that the object we return holds on to some information (container options) until it gets combined with a second piece of information later (the target group), and then we do the side effect:

interface EcsLoadBalancerTarget extends IApplicationLoadBalancerTarget, INetworkLoadBalancerTarget {
}

class BaseService {

  public loadBalancerTarget(options: LoadBalancerTargetOptions): EcsLoadBalancerTarget {
    const self = this;

    return {
      attachToApplicationTargetGroup(targetGroup: ApplicationTargetGroup): LoadBalancerTargetProps {
        return self.attachToELBv2(targetGroup, options);
      },

        attachToApplicationTargetGroup(targetGroup: ApplicationTargetGroup): LoadBalancerTargetProps {
          return self.attachToELBv2(targetGroup, options);
        },
    };
  }

  private attachToELBv2(targetGroup: elbv2.ITargetGroup, options: LoadBalancerTargetOptions) {
    this.loadBalancers.push({
      containerName: options.containerName,
      containerPort: options.containerPort,
      targetGroupArn: targetGroup.targetGroupArn
    });

    return { targetType: elbv2.TargetType.INSTANCE, portRange: /* whatever we used to return here  */ };
  }
}

I'm using closures and object literals here to give the returned object access to a private member attachToELBv2 (which regular consumers shouldn't have access to but the binding object should). This may make your eyes bleed or make you go "well I didn't know I could do THAT I'm used to another programming language". Let's take the boringest language of all, Java, in which you could totally do the same thing, except it requires an additional class declaration:

interface EcsLoadBalancerTarget extends IApplicationLoadBalancerTarget, INetworkLoadBalancerTarget {
}

class BaseService {

  public EcsLoadBalancerTarget loadBalancerTarget(options: LoadBalancerTargetOptions) {
    return new EcsLoadBalancerTarget();
  }

  private LoadBalancerTargetProps attachToELBv2(targetGroup: elbv2.ITargetGroup, options: LoadBalancerTargetOptions) {
    loadBalancers.append({
      containerName: options.containerName,
      containerPort: options.containerPort,
      targetGropuArn: targetGroup.targetGroupArn
    });

    return /* whatever we used to return here */;
  }

  private class EcsLoadBalancerTargetImpl implements EcsLoadBalancerTarget {
    public LoadBalancerTargetProps attachToApplicationTargetGroup(targetGroup: ApplicationTargetGroup) {
      return attachToELBv2(targetGroup, options);
    }

    public LoadBalancerTargetProps attachToApplicationTargetGroup(targetGroup: ApplicationTargetGroup) {
      return attachToELBv2(targetGroup, options);
    }
  }
}

[iamhopaul123]

@rix0rrr Sounds good to me! :eyesbleeding:

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[karlpatr]

rix0rr
The idea is that the object we return holds on to some information (container options) until it gets combined with a second piece of information later (the target group), and then we do the side effect

I actually really like this approach; I think if you forward the existing 1 arg public attach functions on BaseService (for when the loadBalancerTarget function has not been used) to the 2 arg version with LoadBalancerTargetOptions constructed from defaultContainer at call time you end up with something that is really clean.

I think the remaining issue here would be validation, which could be performed during the overloaded attachment using an approach similar to the one I proposed in my PR.

[rix0rrr]

By the way, don't forget to declare the return value of your method to be the interface (instead of having the return type autoderived by TypeScript).

If the thing you're returning is an literal or a private class (which it should be) jsii won't let it typecheck because the hidden type won't exist in client languages.

[rix0rrr]

I think this can be closed out now, since the PR for it has been merged.