Skip to content

Commit

Permalink
chore(cli): fix security related changes integ test (#8274)
Browse files Browse the repository at this point in the history
The test was passing because (1) the stack contained an incorrect service principal
and (2) `--require-approval` was set to `never` by default. This means that the stack
was actually deployed but failed, making the test pass.

Corrected the service principal, added an expectation to ensure that the stack
did not deploy and removed the `--require-approval` CLI option during this test.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
jogold authored Jun 2, 2020
1 parent b7e328d commit 171b039
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 3 deletions.
2 changes: 1 addition & 1 deletion packages/aws-cdk/test/integ/cli/app/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ class IamStack extends cdk.Stack {
super(parent, id, props);

new iam.Role(this, 'SomeRole', {
assumedBy: new iam.ServicePrincipal('ec2.amazon.aws.com')
assumedBy: new iam.ServicePrincipal('ec2.amazonaws.com')
});
}
}
Expand Down
5 changes: 4 additions & 1 deletion packages/aws-cdk/test/integ/cli/cdk-helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ export interface ShellOptions extends child_process.SpawnOptions {

export interface CdkCliOptions extends ShellOptions {
options?: string[];
neverRequireApproval?: boolean;
}

export function log(x: string) {
Expand All @@ -48,8 +49,10 @@ export function log(x: string) {
export async function cdkDeploy(stackNames: string | string[], options: CdkCliOptions = {}) {
stackNames = typeof stackNames === 'string' ? [stackNames] : stackNames;

const neverRequireApproval = options.neverRequireApproval ?? true;

return await cdk(['deploy',
'--require-approval=never', // We never want a prompt in an unattended test
...(neverRequireApproval ? ['--require-approval=never'] : []), // Default to no approval in an unattended test
...(options.options ?? []),
...fullStackName(stackNames)], options);
}
Expand Down
9 changes: 8 additions & 1 deletion packages/aws-cdk/test/integ/cli/cli.integtest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -160,9 +160,16 @@ test('security related changes without a CLI are expected to fail', async () =>
// redirect /dev/null to stdin, which means there will not be tty attached
// since this stack includes security-related changes, the deployment should
// immediately fail because we can't confirm the changes
await expect(cdkDeploy('iam-test', {
const stackName = 'iam-test';
await expect(cdkDeploy(stackName, {
options: ['<', '/dev/null'], // H4x, this only works because I happen to know we pass shell: true.
neverRequireApproval: false,
})).rejects.toThrow('exited with error');

// Ensure stack was not deployed
await expect(cloudFormation('describeStacks', {
StackName: fullStackName(stackName),
})).rejects.toThrow('does not exist');
});

test('deploy wildcard with outputs', async () => {
Expand Down

0 comments on commit 171b039

Please sign in to comment.