Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 12 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
53d9b54
enableExecuteCommand for select ecs-patterns constructs
mzizzi Jul 11, 2021
73f585e
Merge branch 'master' into fargate-enable-execute-command
mzizzi Jul 11, 2021
5ca1032
fix busted test
mzizzi Jul 11, 2021
584d85f
Merge branch 'master' into fargate-enable-execute-command
mzizzi Jul 12, 2021
0b9dd0e
Merge branch 'master' into fargate-enable-execute-command
mzizzi Jul 12, 2021
ecfa5de
Merge branch 'master' into fargate-enable-execute-command
mzizzi Jul 13, 2021
baf123f
Merge branch 'master' into fargate-enable-execute-command
mzizzi Jul 20, 2021
82d3b80
Merge branch 'master' into fargate-enable-execute-command
mzizzi Jul 22, 2021
a290dea
Merge branch 'master' into fargate-enable-execute-command
mzizzi Sep 2, 2021
2addc15
fix typos in enableExecuteCommand tests
mzizzi Sep 2, 2021
5315819
Merge branch 'master' into fargate-enable-execute-command
mzizzi Sep 3, 2021
41c2a21
Merge branch 'master' into fargate-enable-execute-command
mzizzi Sep 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions packages/@aws-cdk/aws-ecs-patterns/lib/fargate/application-load-balanced-fargate-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,13 @@ export interface ApplicationLoadBalancedFargateServiceProps extends ApplicationL
* @default - A new security group is created.
*/
readonly securityGroups?: ISecurityGroup[];

/**
* Whether to enable the ability to execute into a container.
*
* @default - undefined
*/
readonly enableExecuteCommand?: boolean;
upparekh marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down Expand Up @@ -174,6 +181,7 @@ export class ApplicationLoadBalancedFargateService extends ApplicationLoadBalanc
circuitBreaker: props.circuitBreaker,
securityGroups: props.securityGroups,
vpcSubnets: props.taskSubnets,
enableExecuteCommand: props.enableExecuteCommand,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

});
this.addServiceAsTarget(this.service);
}
Expand Down
69 changes: 68 additions & 1 deletion packages/@aws-cdk/aws-ecs-patterns/test/fargate/test.load-balanced-fargate-service-v2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { expect, haveResource, haveResourceLike } from '@aws-cdk/assert-internal';
import { ABSENT, expect, haveResource, haveResourceLike } from '@aws-cdk/assert-internal';
import { Vpc } from '@aws-cdk/aws-ec2';
import * as ecs from '@aws-cdk/aws-ecs';
import { CompositePrincipal, Role, ServicePrincipal } from '@aws-cdk/aws-iam';
Expand Down Expand Up @@ -386,6 +386,73 @@ export = {

test.done();
},

'test with enableExecuteCommand set to true'(test: Test) {
// GIVEN
const stack = new Stack();
const vpc = new Vpc(stack, 'VPC');
const cluster = new ecs.Cluster(stack, 'Cluster', { vpc });

// WHEN
new ApplicationLoadBalancedFargateService(stack, 'Service', {
cluster,
taskImageOptions: {
image: ecs.ContainerImage.fromRegistry('test'),
},
enableExecuteCommand: true,
});

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

test.done();
},

'test with enableExecuteCommand omitted'(test: Test) {
// GIVEN
const stack = new Stack();
const vpc = new Vpc(stack, 'VPC');
const cluster = new ecs.Cluster(stack, 'Cluster', { vpc });

// WHEN
new ApplicationLoadBalancedFargateService(stack, 'Service', {
cluster,
taskImageOptions: {
image: ecs.ContainerImage.fromRegistry('test'),
},
});

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

@paragbhingre paragbhingre Jul 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@mzizzi mzizzi Sep 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@upparekh upparekh Oct 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}));

test.done();
},

'test with enableExecuteCommand false'(test: Test) {
// GIVEN
const stack = new Stack();
const vpc = new Vpc(stack, 'VPC');
const cluster = new ecs.Cluster(stack, 'Cluster', { vpc });

// WHEN
new ApplicationLoadBalancedFargateService(stack, 'Service', {
cluster,
taskImageOptions: {
image: ecs.ContainerImage.fromRegistry('test'),
},
});

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

test.done();
},
},

'When Network Load Balancer': {
Expand Down