fix(events): imported ECS Task Definition cannot be used as target

[rli1994hi]

Note

Fixes #12811

This change allows creating EcsTask from an imported task definition. It
does so by changing the taskDefinition type in EcsTaskProps from concrete class
TaskDefinition to interface ITaskDefinition.

Testing

buildup aws-events-targets

---

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

[gitpod-io]

[rix0rrr]

While @default undefined here is technically true, it's not very useful. Anybody who knows JavaScript/TypeScript will know that if you don't pass a property somewhere, it will count as undefined.

What is more interesting is to write what the BEHAVIOR will be if you leave the value out. In this case something like @default - Permissions cannot be granted to the imported task.

[rli1994hi]

Will do.

[rix0rrr]

Almost there. This error message makes sense to a user if they are calling taskDefinition.networkMode directly.

However, in 99% of cases the user will be PASSING this TaskDefinition to some other construct or function, and THAT one will be calling .networkMode. So think of it like this:

// User writes this:
const taskDefintiion = TaskDefintion.fromTaskDefinitionAttributes(...);
startNewService(taskDefinition);

// User gets error:
"NetworkMode is available only if it is given..."

And the user's thought will be "where is this NetworkMode coming from?"

A more accurate and useful message would be something like:

This operation requires the TaskDefinition's network mode to be known. Add the 'networkMode' parameter to the call to 'TaskDefinition.fromNetworkAttributes(...)'

Or somethign.

[rli1994hi]

Got it. Will update.

[rix0rrr]

Can't this whole implementation just defer to TaskDefinition.fromTaskDefinitionAttributes() ?

Oh I guess it couldn't because the types are different...

[rix0rrr]

I wonder if it might be helpful to define a private class that represents the Import class for all 3 types. This would be a file like:

aws-ecs-/lib/base/_imported-task-definition.ts
                  ^^^ underscore means we plan to not export this file to users

And it could contain a:

export class ImportedTaskDefinition extends Resource implements ITaskDefinition, IEc2TaskDefinition, IFargateTaskDefinition {
  // ...
}

To cut down on the duplication between these 3 methods.

If this sounds too daunting, I'm okay leaving it as is.

[rli1994hi]

Thanks. Will try.

A callout is that, there's a little difference in fromTaskDefinitionArn: It does have the return type of interface ITaskDefinition , but internally it's a TaskDefinitionBase (which implements ITaskDefinition) rather than a Resource like what the other two import methods return. Don't see the downside of unifying them into Resource for now tho.

[rix0rrr]

This is a little scary.

Please replace with a

if (!Construct.isConstruct(this.taskDefinition)) {
  throw new Error(...);

Given our classes this casting should work, but someone is free to implement an ITaskDefinition which is not a Construct. Yes, our inheritance hierarchy is a little wonky here :).

OTOH, I wonder if this.taskDefinition.node.host would also work...

[rix0rrr]

P.S.: The actual change of passing an imported TaskDefinition to an event target also needs a test

[rli1994hi]

1. Make sense. Will replace the casting with the check.
2. Any suggestion on verifying this.taskDefinition.node.host?
   1. Similarly, skinny85@ gave a callout, asking for validating if security group can be accessed after we switch to use ITaskDefinition. Unsure how to do so tho. Any suggestions?
3. Will write a test for passing imported TaskDefinition.

[rix0rrr]

2. Any suggestion on verifying this.taskDefinition.node.host

Try it and see if it compiles 😊. If it does I'm satisfied.

Similarly, skinny85@ gave a callout, asking for validating if security group can be accessed after we switch to use ITaskDefinition. Unsure how to do so tho. Any suggestions?

Your test will confirm. If it doesn't throw then that is good enough. What is most likely going to happen is that new securitygroups are going to be created for this task... which is fine.

Looking at it, EcsTask should probably also implement ec2.IConnectable, otherwise it will be hard to configure the SGs. But you've already put so much effort into this change, I would be okay if you left this for someone else to pick up.

[rix0rrr]

I don't understand why executionRole is here, and not in the EC2 TaskDefinitionAttributes 🤷‍♂️. An oversight on our part?

[rli1994hi]

Good catch. We might be able to get rid of executionRole here.

fromTaskDefinitionArn returns an object containing executionRole to satisfy its parent class, TaskDefinitionBase, which containing executionRole?.

I was originally hoping to give users flexibility to define executionRole by themselves, but considering these methods are to import from existing resources, users might actually hope to give as minimal attributes as possible. I probably should keep executionRole to be undefined and drop it from TaskDefinitionAttributes.

Thoughts?

[rix0rrr]

I probably should keep executionRole to be undefined and drop it from TaskDefinitionAttributes.

Agreed. iirc executionRole only makes sense for EC2 taskdefinitions (and is only on Base to satisfy some interface, but is never set there). For now, I think it's fine to take it out altogether.

[rli1994hi]

Mistake. Should be something like No network mode can be provided.

Pull request has been modified.

[rli1994hi]

Open suggestion for how to DRY.

Tried beforeEach + a function consolidating GIVEN and THEN part of the tests, but using beforeEach made the tests correlate to each other, caused failures like naming collision (There is already a Construct with name 'TaskDef' in Stack).

[rix0rrr]

That should not happen if you also create a new stack in the beforeEach().

let stack: cdk.Stack;
let vpc: ec2.Vpc;
let cluster: ec2.Cluster;
beforeEach(() => {
  stack = new cdk.Stack();
  vpc = new ec2.Vpc(stack, 'Vpc', { maxAzs: 1 });
  cluster = new ecs.Cluster(stack, 'EcsCluster', { vpc });
  cluster.addCapacity('DefaultAutoScalingGroup', {
    instanceType: new ec2.InstanceType('t2.micro'),
  });
});

Feels like an appropriate setup suite.

[rli1994hi]

As the last comment mentioned, we'd better to see if this.taskDefinition.node.xxx works. Haven't found a good way to test. Open to suggestion.

[rix0rrr]

You know what? This is good enough to merge. There might be some slight follow-up but that could be a separate PR. Thanks a lot!

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
• Commit ID: 162561a
• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).