Repository navigation
Support ECS managed instances capacity provider. - #2971
connoromalleyatwork wants to merge 4 commits into
Conversation
🦋 Changeset detectedLatest commit: ca37337 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
connoromalleyatwork
added
feature
|
🐥 |
604a2af to
590f4fb
Compare
|
🐥 |
590f4fb to
528e9b3
Compare
|
🐥 |
|
🐥 |
bdb9694 to
52c967c
Compare
|
🐥 |
|
🐥 |
|
🐥 |
1887a5c to
f6c2871
Compare
a0c1473 to
3caf4b4
Compare
|
🐥 |
| /** | ||
| * Use ECS Manages Instances instead of Fargate to run the app. | ||
| */ | ||
| useManagedInstances?: boolean; |
There was a problem hiding this comment.
Would it be more direct to provide the necessary configuration here, and use its presence/absence to decide whether to defer to Fargate?
At the moment we're hard-coding the instanceRequirements, but I think one of the benefits of this approach is the ability to specify an instance class. If so, perhaps we'd encourage teams to make an informed decision here, up front?
Equally, maybe we're happy to take charge of this for most teams - in which case offering a boolean for now might make sense if we imagine extending this in the future with something like boolean | specificConfigRequirements.
No dramas here, whatever you think is best.
There was a problem hiding this comment.
Makes sense! I've opted for exposing the instanceRequirements as part of a new property managedInstancesProps whose presence means managed instances will be used for the app. The default instanceRequirements is the same as what was hard coded before.
I went for managedInstancesProps as we may want to expose some other settings in future.
connoromalleyatwork
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
Default Fargate services regress to unrestricted egress, and new IAM roles lack required App tags.
1 open finding
What changed in this PR
Adds optional ECS Managed Instances support while retaining Fargate as the default.
Changes:
- Adds
useManagedInstancesconfiguration and supporting ECS/IAM resources. - Runs existing ECS tests against both modes.
- Adds snapshots and a minor changeset.
| File | Description |
|---|---|
gu-load-balanced-app.ts |
Implements Managed Instances support. |
gu-load-balanced-app.test.ts |
Exercises Fargate and Managed Instances. |
gu-load-balanced-app.test.ts.snap |
Captures synthesized templates. |
lovely-falcons-occur.md |
Records the feature release. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ee81faa to
aa90552
Compare
| const ecsInstanceRolePolicy = ManagedPolicy.fromManagedPolicyArn( | ||
| scope, | ||
| "ECSInstanceRole", | ||
| "arn:aws:iam::aws:policy/AmazonECSInstanceRolePolicyForManagedInstances", |
There was a problem hiding this comment.
This role does seem somewhat broad, seemingly let the instance register with any cluster and submit task change events?
Non blocking, but the policy looks fairly basic, might be worth making a more explicit one.
There was a problem hiding this comment.
My thinking is we probably want the simplicity of using this policy since it is managed by AWS.
Capacity providers are cluster scoped according to this doc https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_CapacityProvider.html. This doesn't seam like it is reflected in the managed policy, so I'm not sure where this gets enforced.
I'll have a look into understanding how this is enforced by AWS.
There was a problem hiding this comment.
To enforce a capacity providers ec2 instances being scoped to a cluster i re created the policy and now it is scoped it only allows actions to a specific cluster, this means its a bit more secure now. 🔐
eb53f86 to
5b6b647
Compare
d25d909 to
ca37337
Compare
What does this change?
Before:
GuLoadBalancedAppExperimentalprovisions only Fargate constructs for ECS (Elastic Container Service).After:
GuLoadBalancedAppExperimentalcan provision ECS Managed Instances to run ECS tasks and services instead of Fargate by passingmanagedInstancesProps: {},in theecsProps.Why this change?
When investigating potentially using ECS Managed Instances instead of Fargate as part of ADR for ECS Managed Instances, it became clear that we need to learn more about Managed Instances by trying it out with an actual service.
Managed Instances can optimise the provisioned infrastructure over time for a given ECS cluster, and so the only way to understand the potential cost benefits for a given service is to try it in production.
How to Review
Tip
I recommend reviewing each commit in turn.
managedInstancesProps: {},toecsPropsand pulls out the managed instance modifications into functions that are applied where needed. InstanceHow to test
Check for regressions: Use the latest canary published from this branch in your project that uses the
GuLoadBalancedAppExperimentalconstruct causes no changes to the cloud formation templates created bycdk synth.Check Managed Instances constructs are
synthed: addmanagedInstancesProps: {},in theecsPropsofGuLoadBalancedAppExperimental.Checklist
Footnotes
If you are adding a new construct or pattern, has new documentation been added? If you are amending defaults or changing behaviour, are the existing docs still valid? ↩
Consider whether this is something that will mean changes to projects that have already been migrated, or to the CDK CLI tool. If changes are required, consider adding a checklist here and/or linking to related PRs. ↩