feat(ecs): enable fault injection flag #32598

mazyu36 · 2024-12-19T22:20:18Z

Issue # (if applicable)

N/A

Reason for this change

Add a missing property.

Description of changes

Add enableFaultInjection property.

Describe any new or updated permissions being added

No

Description of how you validated changes

Add unit test and integ test

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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.

codecov · 2024-12-19T22:33:17Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.52%. Comparing base (3606deb) to head (11b6572).

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #32598   +/-   ##
=======================================
  Coverage   81.52%   81.52%           
=======================================
  Files         222      222           
  Lines       13715    13715           
  Branches     2417     2417           
=======================================
  Hits        11181    11181           
  Misses       2254     2254           
  Partials      280      280

Flag	Coverage Δ
suite.unit	`81.52% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`80.97% <ø> (ø)`
packages/aws-cdk-lib/core	`82.09% <ø> (ø)`

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

nmussy

LGTM, mostly docs nits and additional integ coverage 👍

packages/aws-cdk-lib/aws-ecs/README.md

packages/aws-cdk-lib/aws-ecs/lib/base/task-definition.ts

nmussy · 2024-12-24T09:14:35Z

...dk-testing/framework-integ/test/aws-ecs/test/integ.task-definition-enable-fault-injection.ts

+  memoryLimitMiB: 512,
+});
+
+new IntegTest(app, 'TaskDefinitionEnableFaultInjection', {


Could you add assertions to a DescribeTaskDefinition API call? You should be able to retrieve the enableFaultInjection prop there

I'm struggling to add integration test assertions.

The enable fault injection was correctly set to true in the management console after deployment.

However, the assertions are failing.
I'm not yet sure of the root cause, but I might need to update the SDK version used in the integ tests.

test.assertions.awsApiCall('@aws-sdk/client-ecs', 'DescribeTaskDefinition', { taskDefinition: ec2TaskDefinition.taskDefinitionArn, }) .expect(ExpectedResult.objectLike({ taskDefinition: { enableFaultInjection: true, }, }));

ASSERT aws-ecs/test/integ.task-definition-enable-fault-injection-TaskDefinitionEnableFaultInjection/DefaultTest (undefined/us-east-1) 128.939s AssertionResultsAwsApiCallawssdkclientecsDescribeTaskDefinitionfd8b5aaee373c34a7ec063a10e6c89a0 { "tags": [], "taskDefinition": { "compatibilities": [ "EC2", "FARGATE" ], "containerDefinitions": [ { ... } ], "cpu": "256", !! Missing key 'enableFaultInjection' "enableFaultInjection": undefined,

I don't think it's a SDK issue, but one with the API, or at least its docs, as DescribeTaskDefinition should return enableFaultInjection, whether or not it was set. I'm not getting the property from the CLI either with aws ecs describe-task-definition, with both the EC2 or the Fargate definition.

Checking the task definition JSON in the web console does indicate that enableFaultInjection is being properly set by your integration, but I don't think we can test it as is. You could try to raise this issue with the AWS support but it might be better if a CDK team member could forward this issue.

Thank you for the approval.
After updating the version in AWS CLI, I was able to retrieve it using describe-task-definition by AWS CLI.
Therefore, I think it might be an issue with a tool.

I'll investigate this further separately from this PR.

You're absolutely right, I upgraded my CLI from 2.22.7 to 2.22.23 and the field now appears:

$ aws ecs describe-task-definition --task-definition <arn> | jq '.taskDefinition.enableFaultInjection' true

I assumed the AWS API would do all the work with describe* commands, but I guess either the implementations map the results, or the API only returns certain fields after a certain version? Either way, kind of odd.

From what I can tell, since it's no longer documented, Node Lambdas ship with version 3.632.0 of the SDK (at least on us-east-1), so there's not much we can do about this as the assertions are ran on a custom resource. I don't think it's worth bundling and shipping the SDK for the version lag we're experiencing, even if this version is over 4 months old

Co-authored-by: Jimmy Gaussen <[email protected]>

godwingrs22

Thanks @mazyu36 for the contribution. LGTM overall. Just left two nit comments.

godwingrs22 · 2025-01-06T20:52:16Z

packages/aws-cdk-lib/aws-ecs/lib/base/task-definition.ts

+    if (props.enableFaultInjection && ![NetworkMode.AWS_VPC, NetworkMode.HOST].includes(this.networkMode)) {
+      throw new Error(`Only AWS_VPC and HOST Network Modes are supported for enabling Fault Injection, got ${this.networkMode} mode.`);
+    }
+


nit: It would be good to mention the behaviour link also above this code as a comment.

Thanks. I've added the link.

godwingrs22 · 2025-01-06T20:55:31Z

packages/aws-cdk-lib/aws-ecs/lib/base/task-definition.ts

@@ -514,6 +527,7 @@ export class TaskDefinition extends TaskDefinitionBase {
        cpuArchitecture: this.runtimePlatform?.cpuArchitecture?._cpuArchitecture,
        operatingSystemFamily: this.runtimePlatform?.operatingSystemFamily?._operatingSystemFamily,
      } : undefined,
+      enableFaultInjection: props.enableFaultInjection,


nit: since the enableFaultInjection is default false (as mentioned in jsdoc), it would be better we specify as false instead of leaving it as undefined. This ensures the default behavior is clear.

enableFaultInjection: props.enableFaultInjection ?? false

In my understanding, if default false is set, all existing templates will change, and it should be avoided.
I have fixed the JSDoc. Please correct me if I'm wrong.

mergify · 2025-01-07T17:53:36Z

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 · 2025-01-07T17:58:20Z

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).

godwingrs22 · 2025-01-07T20:14:35Z

@Mergifyio update

mergify · 2025-01-07T20:14:46Z

update

✅ Branch has been successfully updated

aws-cdk-automation · 2025-01-07T20:46:09Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 11b6572
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2025-01-07T20:46:33Z

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).

github-actions · 2025-01-07T20:46:53Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

feat(ecs): enbale fault injection flag

26be3e9

github-actions bot added the p2 label Dec 19, 2024

aws-cdk-automation requested a review from a team December 19, 2024 22:20

github-actions bot added the distinguished-contributor [Pilot] contributed 50+ PRs to the CDK label Dec 19, 2024

aws-cdk-automation previously requested changes Dec 19, 2024

View reviewed changes

update

35008e6

mazyu36 marked this pull request as ready for review December 19, 2024 23:14

Merge branch 'main' into ecs-enable-fis

50478e4

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 20, 2024

nmussy suggested changes Dec 24, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 24, 2024

mazyu36 and others added 4 commits December 24, 2024 19:43

Update packages/aws-cdk-lib/aws-ecs/README.md

3edc10a

Co-authored-by: Jimmy Gaussen <[email protected]>

Update packages/aws-cdk-lib/aws-ecs/lib/base/task-definition.ts

62fd490

Co-authored-by: Jimmy Gaussen <[email protected]>

Update packages/aws-cdk-lib/aws-ecs/lib/base/task-definition.ts

94e40b8

Co-authored-by: Jimmy Gaussen <[email protected]>

update unit test

67d97e6

nmussy approved these changes Dec 25, 2024

View reviewed changes

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 25, 2024

Merge branch 'main' into ecs-enable-fis

181e964

github-actions bot mentioned this pull request Jan 1, 2025

Monthly PR metrics report #32703

Open

Merge branch 'main' into ecs-enable-fis

e9fd144

godwingrs22 self-requested a review January 6, 2025 20:40

godwingrs22 changed the title ~~feat(ecs): enbale fault injection flag~~ feat(ecs): enable fault injection flag Jan 6, 2025

godwingrs22 reviewed Jan 6, 2025

View reviewed changes

mazyu36 added 2 commits January 7, 2025 12:48

update

ef75f21

Merge branch 'main' into ecs-enable-fis

2b263a4

mazyu36 requested a review from godwingrs22 January 7, 2025 07:35

Merge branch 'main' into ecs-enable-fis

7f53f6b

godwingrs22 approved these changes Jan 7, 2025

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 7, 2025

Merge branch 'main' into ecs-enable-fis

11b6572

mergify bot merged commit ed366ce into aws:main Jan 7, 2025
19 checks passed

github-actions bot locked as resolved and limited conversation to collaborators Jan 7, 2025

mazyu36 deleted the ecs-enable-fis branch January 7, 2025 23:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ecs): enable fault injection flag #32598

feat(ecs): enable fault injection flag #32598

mazyu36 commented Dec 19, 2024 •

edited

Loading

aws-cdk-automation left a comment

codecov bot commented Dec 19, 2024 •

edited

Loading

nmussy left a comment

nmussy Dec 24, 2024

mazyu36 Dec 25, 2024

nmussy Dec 25, 2024

mazyu36 Dec 25, 2024

nmussy Dec 26, 2024 •

edited

Loading

godwingrs22 left a comment

godwingrs22 Jan 6, 2025

mazyu36 Jan 7, 2025

godwingrs22 Jan 6, 2025

mazyu36 Jan 7, 2025

mergify bot commented Jan 7, 2025

mergify bot commented Jan 7, 2025

godwingrs22 commented Jan 7, 2025

mergify bot commented Jan 7, 2025

aws-cdk-automation commented Jan 7, 2025

mergify bot commented Jan 7, 2025

github-actions bot commented Jan 7, 2025

feat(ecs): enable fault injection flag #32598

feat(ecs): enable fault injection flag #32598

Conversation

mazyu36 commented Dec 19, 2024 • edited Loading

Issue # (if applicable)

Reason for this change

Description of changes

Describe any new or updated permissions being added

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 19, 2024 • edited Loading

Codecov Report

nmussy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmussy Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

godwingrs22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Jan 7, 2025

mergify bot commented Jan 7, 2025

godwingrs22 commented Jan 7, 2025

mergify bot commented Jan 7, 2025

✅ Branch has been successfully updated

aws-cdk-automation commented Jan 7, 2025

AWS CodeBuild CI Report

mergify bot commented Jan 7, 2025

github-actions bot commented Jan 7, 2025

mazyu36 commented Dec 19, 2024 •

edited

Loading

codecov bot commented Dec 19, 2024 •

edited

Loading

nmussy Dec 26, 2024 •

edited

Loading