Set subnets used by ECS task rather than relying on cdk context

[philmcmahon]

What does this change?

I ran into an issue when working with the ecs-task role where I found it only worked if you provided a cdk context file. This was because, by default an EcsRunTask will try to assign itself to the 'PRIVATE' subnets identified by the cdk context file. This works for lurch (which was the project the Ecs task role was created for) because that project is a private repo, with a checked in context file.

Relying on a context file isn't ideal. This PR removes the need for a context file by requiring consumers to either specify which subnets to use directly, or falling back to using GuVpc.subnetsFromParameterFixedNumber

But what is GuVpc.subnetsFromParameterFixedNumber?

Unfortunately, when I tried to use just the normal GuVpc.subnetsFromParameter it resulted in CDK creating a custom resource, presumably to deal with some assumed circular dependency. You can see an example of the custom resource mess here.

The issue is caused by this line in GuVpc:

cdk/src/constructs/ec2/vpc.ts

Line 45 in b61bbc6

Subnet.fromSubnetAttributes(scope, `subnet-${subnetId}`, { subnetId, routeTableId: " " }),

With some experimentation, I discovered that if you specify the subnets using .bySubnetId, and use a standard cloudformation list operation (Fn.select), then this custom resource is avoided. The thing is, I was a bit reluctant (scared) to change the existing approach to subnets, as it would affect all GuEc2Apps. The alternative approach of using Fn.select also makes for slightly messier cloudformation output - instead of looking like this:

    "Subnets": {
      "Ref": "testguec2appPublicSubnets",
    },

the subnets definition ends up looking like this:

{"Subnets":["",
              {
                "Fn::Select": [
                  0,
                  {
                    "Ref": "ecstestPrivateSubnets",
                  },
                ],
              },
              "","",
              {
                "Fn::Select": [
                  1,
                  {
                    "Ref": "ecstestPrivateSubnets",
                  },
                ],
              },
              "","",
              {
                "Fn::Select": [
                  2,
                  {
                    "Ref": "ecstestPrivateSubnets",
                  },
                ],
              },
              ""]

The new approach also assumes knowledge of how many subnets there are - whilst we usually have 3, in the tests we sometimes just have one, so more effort would be needed there.

I think this approach is a reasonable compromise, but very happy to talk it through/try to pair on a better solution.

Whilst I try and resolve that, this is a placeholder PR so I can share the work with others

How to test

[changeset-bot]

🦋 Changeset detected

Latest commit: 284f7d2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@guardian/cdk	Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[akash1810]

I think this nested ternary is unnecessary complexity; could we remove the assignPublicIp prop and pass in the correct subnets when instantiating this construct? The consequence is the subnets prop becomes mandatory, but I think it'll result in a simpler interface. For example:

new GuEcsTask(scope, "my-task", {
  subnets: GuVpc.subnetsFromParameterFixedNumber(scope, { type: SubnetType.PUBLIC, app }
});

[philmcmahon]

Good idea, thanks