feat(ecs): container port ranges in port mappings

[ste93cry]

Amazon ECS supports container port ranges for port mapping since quite a bit, however the ContainerDefinition L2 construct does not expose a way to set them. Right now, the only viable solution I found to use the feature is using Escape Hatches. Within this PR, the containerPortRange property is added to the PortMapping object and mapped back to the underlying L1 construct.

The current implementation contains a breaking change: since setting both a port range and a single fixed port doesn't make sense, I had to mark optional all the properties of PortMapping, hence the containerPort property changed its type from non-nullable to nullable. The downside of the proposed solution is that now all the properties are optional and the compiler doesn't complain if an empty object is passed as port mapping. I added some runtime checks to ensure that a valid object is created, but I didn't find a better way to do it at compile time that is also compatible with the transformation done by jsii.

Closes #23509.

---

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

[aws-cdk-automation]

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

[rix0rrr]

Thanks for this. It seems like a rather involved change so I want to make sure I understand the possible implications before we ship it.

Pull request has been modified.

[rix0rrr]

I'm also considering that the breaking change you are making (switching a property from required to optional) is not allowed if the structure you're doing this in is exposed to be read by customer code somewhere.

Someone could have written this code:

const somePort: number = container.portMapping.containerPort;
console.log(somePort);

This code used to be fine, but after your change will fail to compile with number | undefined is not assignable to number.

That type of change is not allowed.

[ste93cry]

I'm also considering that the breaking change you are making (switching a property from required to optional) is not allowed if the structure you're doing this in is exposed to be read by customer code somewhere.

Unfortunately, I don't think there is any other way to implement this feature within the current code as containerPort and containerPortRange are mutually exclusive. I tried more complicated solutions involving two interfaces and type unions but then jsii is incompatible, so I'm stuck 🤷‍♂️

[rix0rrr]

I agree this is not optimal. Something we could consider is passing values through the token system. We could do something like:

class PortRange implements cdk.IResolvable {
  public readonly creationStack: string[] = [];
  public readonly typeHint?: cdk.ResolutionTypeHint;
  public readonly isPortRange = true;

  public static of(startPort: number, endPort: number): number {
    return cdk.Token.asNumber(new PortRange(startPort, endPort));
  }

  public static tryDetect(port: number): PortRange | undefined {
    const x = cdk.Tokenization.reverseNumber(port);
    return x && (x as any).isPortRange ? x as PortRange : undefined;
  }

  private constructor(public readonly startPort: number, public readonly endPort: number) {  }

  public resolve(_context: cdk.IResolveContext) {
    // This should not end up in a CFN template, so does not need to implement
    // resolve()
    throw new Error('PortRange may only be passed to ECS PortMapping\'s containerPort');
  }

  public toString(): string {
    return cdk.Token.asString(this);
  }
}

This is using some dark token magic, but it will work. We can use that to smuggle a PortRange object in the containerPort variable that normally accepts a number.

We would use that instead of adding a new property.

Demo:

const x = PortRange.of(1000, 2000);
console.log(x);
// -1.8881545897087535e+289

console.log(PortRange.tryDetect(x));
/*
PortRange {
  startPort: 1000,
  endPort: 2000,
  creationStack: [],
  typeHint: 'string'
}
*/

[rix0rrr]

Or the following, which is not as nice but much more simple:

const portMapping: PortMapping = {
  containerPort: 0,  // Magic value
  portRange: '1000-2000',
};

Potentially with a const to give a nice name to the 0.

This is probably preferable for understandability and discoverability.

[ste93cry]

Or the following, which is not as nice but much more simple:

If I understand correctly, you would leave containerPort non-nullable and you would ask users to set it to a magic value when they want to use the containerPortRange, right? Doesn't this complicate things for the end user as he needs to know about this quirk?

[rix0rrr]

f I understand correctly, you would leave containerPort non-nullable and you would ask users to set it to a magic value when they want to use the containerPortRange, right?

Indeed.

Doesn't this complicate things for the end user as he needs to know about this quirk?

Yes, but it's not all that bad.

• First of all, we will put an example in the README, and 90% of users will just copy/paste that.
• Second, we will put this information in the docstring of both properties, so anyone inspecting the API or reading the popup in their IDE will see what needs to happen.
• Third, we will put in runtime validation saying if you supply 'portRange' you must set containerPort to 0 and if you set containerPort to 0, you must supply portRange, making it very clear what the deal is.

As much as the trick feels neat, this is more discoverable than pulling in the magic PortRange class.

[ste93cry]

I have to admit that, before opening this PR, I thought a lot about adding a PortRange class with named constructors, but I didn't know it was possible to do it using tokens as backing store. As a user, I find suboptimal to have to know that there is a magic value that changes the behaviour of the object, but on the other side, having to know that a property typehinted as number magically accepts a value obtained from a cdk.IResolvable object is suboptimal too. I pushed the changes and the PR is ready for another round of review. In a future major version of CDK it may be cool to revert to the solution of two nullable properties, if BC is allowed in that case.

[ste93cry]

I have implemented all the requested changes. There are still some outstanding points above, feel free to mark the conversations as done if you are satisfied with the replies or add more feedback on how I should proceed.

[rix0rrr]

I'll ship it! Thanks for this.

[mergify]

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

[mergify]

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

[ste93cry]

It seems that Mergify can't rebase the PR because, while it was open, the GitHub workflows were changed, and the app does not have the appropriate permissions to update them. Should I rebase manually?

Pull request has been modified.

[ste93cry]

Build is now failing because of different NodeJS version used for a resource in the integration test I've added. Do I have to update the snapshot or can you do it for me?

Pull request has been modified.

[ste93cry]

Thanks for updating the test snapshot. Unfortunately, there is one more problem that makes the build fail, but seems to be unrelated 😭 Maybe another pull of the changes from the main branch can solve it?

Pull request has been modified.

[mergify]

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

Pull request has been modified.

[mergify]

Thank you for contributing! Your pull request will be updated from main 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: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: aa7cb6d
• 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 main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).