chore(ecs-patterns): ExecuteCommand support in ApplicationLoadBalancedFargateService #15497

mzizzi · 2021-07-11T18:53:43Z

Addresses #15197 by adding support for enabling ExecuteCommand for Services created with ApplicationLoadBalancedFargateService.

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

gitpod-io · 2021-07-11T18:53:45Z

awsiv · 2021-07-21T08:32:35Z

@mzizzi thanks for this - I was waiting for this functionality as well :)

Also - looks like this needs to be rebased against main again

danwiltshire · 2021-07-28T07:39:58Z

Awesome stuff.

Roughly when would this be rolled into a release?

paragbhingre · 2021-07-29T16:56:27Z

packages/@aws-cdk/aws-ecs-patterns/test/fargate/test.load-balanced-fargate-service-v2.ts

+
+      // THEN - stack contains a service with execute-command enabled
+      expect(stack).to(haveResourceLike('AWS::ECS::Service', {
+        EnableExecuteCommand: ABSENT,


Does expected default value is ABSENT? or we do we expect false as a default value?

Hey sorry for the late response. As implemented the default for the interface is to leave that property undefined as an optional with readonly enableExecuteCommand?: boolean;

If you're arguing that we shouldn't be allowed to omit that property and force a true/false then I can make those changes. Cloudformation looks to be happy with either true/false or omitting the property altogether.

Unrelated: I'll fix the copy/pasta comments that are incorrect.

I don't think we need either of the omitted or false tests. The ABSENT test asserts whether the prop is undefined or not set (a check not necessary in our case) and the false test below essentially checks if the enableExecuteCommand is set correctly and so does the true test. It would be great if you could remove both these unit tests.

Hey, thanks for the review!

I don't think we need either of the omitted or false tests. The ABSENT test asserts whether the prop is undefined or not set (a check not necessary in our case)

Why wouldn't it be necessary to test ABSENT behavior? It's a valid config and what most (all) people will be using prior to this PR being merged. It's a good check of back compatibility and that the underlying templates that get synthed aren't changing for people that omit this option.

and the false test below essentially checks if the enableExecuteCommand is set correctly and so does the true test. It would be great if you could remove both these unit tests.

I'd argue that we should be testing to ensure that the proper value is getting set all of the way through for each possible input and not just the happy (true) path. These act as regression tests for each of possible configs related to this feature. If the guts of the implementation change and break things then these tests will fail.

The purpose of the ABSENT assertion is not to check for backwards compatibility. The new test suite will run only against the updated construct. If there is a field that explicitly needs to be false/ undefined for a certain configuration of props then we would use the ABSENT assertion.

You’re right, we should not only check for happy paths. enableExecuteCommand for this L3 construct is only an input property (not a class attribute which can be modified). It will set the corresponding L2 property to what is provided as input and any other testing is taken care by the L2. Thus, we should be good with using the true literal.

camharris · 2021-07-29T21:11:16Z

@mzizzi are you sure this will handle adjusting the iam roles for ssmmessages?

mzizzi · 2021-09-03T17:58:33Z

@mzizzi are you sure this will handle adjusting the iam roles for ssmmessages?

It should ultimately be handled it in the same way that it is handled here:

aws-cdk/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts

Line 907 in 6bc086b

private enableExecuteCommand() {

aws-cdk-automation · 2021-09-03T19:19:45Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: 41c2a21
Result: SUCCEEDED
Build Logs (available for 30 days)

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

upparekh

Hi @mzizzi , the PR looks good but I have two suggestions!

packages/@aws-cdk/aws-ecs-patterns/lib/fargate/application-load-balanced-fargate-service.ts

upparekh · 2021-09-13T23:24:14Z

packages/@aws-cdk/aws-ecs-patterns/test/fargate/test.load-balanced-fargate-service-v2.ts

+
+      // THEN - stack contains a service with execute-command enabled
+      expect(stack).to(haveResourceLike('AWS::ECS::Service', {
+        EnableExecuteCommand: ABSENT,


I don't think we need either of the omitted or false tests. The ABSENT test asserts whether the prop is undefined or not set (a check not necessary in our case) and the false test below essentially checks if the enableExecuteCommand is set correctly and so does the true test. It would be great if you could remove both these unit tests.

LukvonStrom · 2021-12-15T14:10:51Z

packages/@aws-cdk/aws-ecs-patterns/lib/fargate/application-load-balanced-fargate-service.ts

@@ -174,6 +181,7 @@ export class ApplicationLoadBalancedFargateService extends ApplicationLoadBalanc
      circuitBreaker: props.circuitBreaker,
      securityGroups: props.securityGroups,
      vpcSubnets: props.taskSubnets,
+      enableExecuteCommand: props.enableExecuteCommand,


why scope it that small and not patch it into all base classes of the patterns (i.e. application-load-balanced-service-base.ts, application-multiple-target-groups-service-base.ts, network-load-balanced-service-base.ts, network-multiple-target-groups-service-base.ts, queue-processing-service-base.ts, scheduled-task-base.ts)

IIRC ECS Exec works on Fargate and EC2, so I would reccomend patching the base classes to propagate it into the implementations of the patterns

madeline-k · 2022-02-01T19:02:22Z

Closing as this is superseded by #18663

Fixes #15769, #15197 Supersedes #15497 by implementing the change for all patterns. This PR implements support for ECS Exec in all ecs-patterns services. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Fixes aws#15769, aws#15197 Supersedes aws#15497 by implementing the change for all patterns. This PR implements support for ECS Exec in all ecs-patterns services. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

enableExecuteCommand for select ecs-patterns constructs

53d9b54

Merge branch 'master' into fargate-enable-execute-command

73f585e

mzizzi mentioned this pull request Jul 11, 2021

(aws-ecs-patterns): ECS Exec Support #15197

Closed

mzizzi added 5 commits July 11, 2021 16:36

fix busted test

5ca1032

Merge branch 'master' into fargate-enable-execute-command

584d85f

Merge branch 'master' into fargate-enable-execute-command

0b9dd0e

Merge branch 'master' into fargate-enable-execute-command

ecfa5de

Merge branch 'master' into fargate-enable-execute-command

baf123f

peterwoodworth assigned uttarasridhar, SoManyHs and madeline-k Jul 20, 2021

peterwoodworth added the @aws-cdk/aws-ecs-patterns Related to ecs-patterns library label Jul 20, 2021

Merge branch 'master' into fargate-enable-execute-command

82d3b80

paragbhingre reviewed Jul 29, 2021

View reviewed changes

upparekh added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Aug 18, 2021

upparekh changed the title ~~feat(ecs-patterns): ExecuteCommand support in ApplicationLoadBalancedFargateService~~ chore(ecs-patterns): ExecuteCommand support in ApplicationLoadBalancedFargateService Aug 18, 2021

upparekh linked an issue Aug 18, 2021 that may be closed by this pull request

(aws-ecs-patterns): Add enableExecuteCommand configuration property ApplicationLoadBalancedFargateService #15769

Closed

2 tasks

mzizzi added 2 commits September 2, 2021 15:33

Merge branch 'master' into fargate-enable-execute-command

a290dea

fix typos in enableExecuteCommand tests

2addc15

mzizzi added 2 commits September 3, 2021 13:58

Merge branch 'master' into fargate-enable-execute-command

5315819

Merge branch 'master' into fargate-enable-execute-command

41c2a21

upparekh reviewed Sep 13, 2021

View reviewed changes

github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Sep 21, 2021

RomainMuller force-pushed the master branch from 924c117 to ebfd5f2 Compare September 29, 2021 13:07

LukvonStrom reviewed Dec 15, 2021

View reviewed changes

LukvonStrom mentioned this pull request Jan 26, 2022

feat(ecs-patterns): add ecs exec support #18663

Merged

madeline-k closed this Feb 1, 2022

mzizzi deleted the fargate-enable-execute-command branch November 24, 2024 12:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(ecs-patterns): ExecuteCommand support in ApplicationLoadBalancedFargateService #15497

chore(ecs-patterns): ExecuteCommand support in ApplicationLoadBalancedFargateService #15497

mzizzi commented Jul 11, 2021 •

edited

Loading

gitpod-io bot commented Jul 11, 2021

awsiv commented Jul 21, 2021 •

edited

Loading

danwiltshire commented Jul 28, 2021

paragbhingre Jul 29, 2021 •

edited

Loading

mzizzi Sep 2, 2021 •

edited

Loading

upparekh Sep 13, 2021

mzizzi Sep 20, 2021

upparekh Oct 11, 2021 •

edited

Loading

camharris commented Jul 29, 2021

mzizzi commented Sep 3, 2021

aws-cdk-automation commented Sep 3, 2021

upparekh left a comment

upparekh Sep 13, 2021

LukvonStrom Dec 15, 2021

LukvonStrom Dec 15, 2021

madeline-k commented Feb 1, 2022

chore(ecs-patterns): ExecuteCommand support in ApplicationLoadBalancedFargateService #15497

chore(ecs-patterns): ExecuteCommand support in ApplicationLoadBalancedFargateService #15497

Conversation

mzizzi commented Jul 11, 2021 • edited Loading

gitpod-io bot commented Jul 11, 2021

awsiv commented Jul 21, 2021 • edited Loading

danwiltshire commented Jul 28, 2021

paragbhingre Jul 29, 2021 • edited Loading

Choose a reason for hiding this comment

mzizzi Sep 2, 2021 • edited Loading

Choose a reason for hiding this comment

upparekh Sep 13, 2021

Choose a reason for hiding this comment

mzizzi Sep 20, 2021

Choose a reason for hiding this comment

upparekh Oct 11, 2021 • edited Loading

Choose a reason for hiding this comment

camharris commented Jul 29, 2021

mzizzi commented Sep 3, 2021

aws-cdk-automation commented Sep 3, 2021

AWS CodeBuild CI Report

upparekh left a comment

Choose a reason for hiding this comment

upparekh Sep 13, 2021

Choose a reason for hiding this comment

LukvonStrom Dec 15, 2021

Choose a reason for hiding this comment

LukvonStrom Dec 15, 2021

Choose a reason for hiding this comment

madeline-k commented Feb 1, 2022

mzizzi commented Jul 11, 2021 •

edited

Loading

awsiv commented Jul 21, 2021 •

edited

Loading

paragbhingre Jul 29, 2021 •

edited

Loading

mzizzi Sep 2, 2021 •

edited

Loading

upparekh Oct 11, 2021 •

edited

Loading