Skip to content

Commit

Permalink
feat(core): configure Stack SNS notification ARNs on the Stack constr…
Browse files Browse the repository at this point in the history
…uct (#31107)

### Issue # (if applicable)

#8581.

### Reason for this change

It is easier and clearer to specify the SNS Topic ARNs on the stack construct itself instead of passing it as a command line argument.

### Description of changes

Added a new optional stack prop, `notificationArns`, that is written to the CloudAssembly and concatenated with the CLI option `--notification-arns`. 

Don't forget to select stacks by hierarchical ID (currently display name, in our tests) when writing certain test code. Otherwise, the tests may not select the stack you expect.

Depends on: cdklabs/cdk-assets#87 and cdklabs/cloud-assembly-schema#58.

### Description of how you validated changes

Unit tests + CLI integ test. Framework integ tests not included because they would require an externally-created SNS Topic, which is not what we want in integ tests; besides, the case is covered by the CLI integ test. 

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
comcalvi authored Sep 23, 2024
1 parent b49032b commit 1593500
Show file tree
Hide file tree
Showing 20 changed files with 527 additions and 165 deletions.
10 changes: 10 additions & 0 deletions packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/app/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -639,6 +639,12 @@ class BuiltinLambdaStack extends cdk.Stack {
}
}

class NotificationArnPropStack extends cdk.Stack {
constructor(parent, id, props) {
super(parent, id, props);
new sns.Topic(this, 'topic');
}
}
class AppSyncHotswapStack extends cdk.Stack {
constructor(parent, id, props) {
super(parent, id, props);
Expand Down Expand Up @@ -708,6 +714,10 @@ switch (stackSet) {
new DockerStack(app, `${stackPrefix}-docker`);
new DockerStackWithCustomFile(app, `${stackPrefix}-docker-with-custom-file`);

new NotificationArnPropStack(app, `${stackPrefix}-notification-arn-prop`, {
notificationArns: [`arn:aws:sns:${defaultEnv.region}:${defaultEnv.account}:${stackPrefix}-test-topic-prop`],
});

// SSO stacks
new SsoInstanceAccessControlConfig(app, `${stackPrefix}-sso-access-control`);
new SsoAssignment(app, `${stackPrefix}-sso-assignment`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {
withCDKMigrateFixture,
withExtendedTimeoutFixture,
randomString,
withoutBootstrap,
} from '../../lib';

jest.setTimeout(2 * 60 * 60_000); // Includes the time to acquire locks, worst-case single-threaded runtime
Expand Down Expand Up @@ -276,9 +277,12 @@ integTest(
}),
);

// bootstrapping also performs synthesis. As it turns out, bootstrap-stage synthesis still causes the lookups to be cached, meaning that the lookup never
// happens when we actually call `cdk synth --no-lookups`. This results in the error never being thrown, because it never tries to lookup anything.
// Fix this by not trying to bootstrap; there's no need to bootstrap anyway, since the test never tries to deploy anything.
integTest(
'context in stage propagates to top',
withDefaultFixture(async (fixture) => {
withoutBootstrap(async (fixture) => {
await expect(
fixture.cdkSynth({
// This will make it error to prove that the context bubbles up, and also that we can fail on command
Expand Down Expand Up @@ -613,12 +617,13 @@ integTest(
);

integTest(
'deploy with notification ARN',
'deploy with notification ARN as flag',
withDefaultFixture(async (fixture) => {
const topicName = `${fixture.stackNamePrefix}-test-topic`;
const topicName = `${fixture.stackNamePrefix}-test-topic-flag`;

const response = await fixture.aws.sns.send(new CreateTopicCommand({ Name: topicName }));
const topicArn = response.TopicArn!;

try {
await fixture.cdkDeploy('test-2', {
options: ['--notification-arns', topicArn],
Expand All @@ -641,6 +646,31 @@ integTest(
}),
);

integTest('deploy with notification ARN as prop', withDefaultFixture(async (fixture) => {
const topicName = `${fixture.stackNamePrefix}-test-topic-prop`;

const response = await fixture.aws.sns.send(new CreateTopicCommand({ Name: topicName }));
const topicArn = response.TopicArn!;

try {
await fixture.cdkDeploy('notification-arn-prop');

// verify that the stack we deployed has our notification ARN
const describeResponse = await fixture.aws.cloudFormation.send(
new DescribeStacksCommand({
StackName: fixture.fullStackName('notification-arn-prop'),
}),
);
expect(describeResponse.Stacks?.[0].NotificationARNs).toEqual([topicArn]);
} finally {
await fixture.aws.sns.send(
new DeleteTopicCommand({
TopicArn: topicArn,
}),
);
}
}));

// NOTE: this doesn't currently work with modern-style synthesis, as the bootstrap
// role by default will not have permission to iam:PassRole the created role.
integTest(
Expand Down
22 changes: 21 additions & 1 deletion packages/@aws-cdk/cx-api/FEATURE_FLAGS.md
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ Flags come in three types:
| [@aws-cdk/custom-resources:logApiResponseDataPropertyTrueDefault](#aws-cdkcustom-resourceslogapiresponsedatapropertytruedefault) | When enabled, the custom resource used for `AwsCustomResource` will configure the `logApiResponseData` property as true by default | 2.145.0 | (fix) |
| [@aws-cdk/aws-s3:keepNotificationInImportedBucket](#aws-cdkaws-s3keepnotificationinimportedbucket) | When enabled, Adding notifications to a bucket in the current stack will not remove notification from imported stack. | 2.155.0 | (fix) |
| [@aws-cdk/aws-stepfunctions-tasks:useNewS3UriParametersForBedrockInvokeModelTask](#aws-cdkaws-stepfunctions-tasksusenews3uriparametersforbedrockinvokemodeltask) | When enabled, use new props for S3 URI field in task definition of state machine for bedrock invoke model. | 2.156.0 | (fix) |
| [@aws-cdk/aws-ecs:reduceEc2FargateCloudWatchPermissions](#aws-cdkaws-ecsreduceec2fargatecloudwatchpermissions) | When enabled, we will only grant the necessary permissions when users specify cloudwatch log group through logConfiguration | 2.159.0 | (fix) |

<!-- END table -->

Expand Down Expand Up @@ -134,7 +135,8 @@ The following json shows the current recommended set of flags, as `cdk init` wou
"@aws-cdk/aws-ec2:ebsDefaultGp3Volume": true,
"@aws-cdk/aws-ecs:removeDefaultDeploymentAlarm": true,
"@aws-cdk/custom-resources:logApiResponseDataPropertyTrueDefault": false,
"@aws-cdk/aws-s3:keepNotificationInImportedBucket": false
"@aws-cdk/aws-s3:keepNotificationInImportedBucket": false,
"@aws-cdk/aws-ecs:reduceEc2FargateCloudWatchPermissions": true
}
}
```
Expand Down Expand Up @@ -1378,4 +1380,22 @@ When this feature flag is enabled, specify newly introduced props 's3InputUri' a
**Compatibility with old behavior:** Disable the feature flag to use input and output path fields for s3 URI


### @aws-cdk/aws-ecs:reduceEc2FargateCloudWatchPermissions

*When enabled, we will only grant the necessary permissions when users specify cloudwatch log group through logConfiguration* (fix)

Currently, we automatically add a number of cloudwatch permissions to the task role when no cloudwatch log group is
specified as logConfiguration and it will grant 'Resources': ['*'] to the task role.

When this feature flag is enabled, we will only grant the necessary permissions when users specify cloudwatch log group.


| Since | Default | Recommended |
| ----- | ----- | ----- |
| (not in v1) | | |
| 2.159.0 | `false` | `true` |

**Compatibility with old behavior:** Disable the feature flag to continue grant permissions to log group when no log group is specified


<!-- END details -->
4 changes: 2 additions & 2 deletions packages/@aws-cdk/cx-api/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -82,12 +82,12 @@
"semver": "^7.6.3"
},
"peerDependencies": {
"@aws-cdk/cloud-assembly-schema": "^36.0.5"
"@aws-cdk/cloud-assembly-schema": "^38.0.0"
},
"license": "Apache-2.0",
"devDependencies": {
"@aws-cdk/cdk-build-tools": "0.0.0",
"@aws-cdk/cloud-assembly-schema": "^36.0.24",
"@aws-cdk/cloud-assembly-schema": "^38.0.0",
"@aws-cdk/pkglint": "0.0.0",
"@types/jest": "^29.5.12",
"@types/mock-fs": "^4.13.4",
Expand Down
5 changes: 3 additions & 2 deletions packages/@aws-cdk/integ-runner/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,12 @@
},
"dependencies": {
"chokidar": "^3.6.0",
"@aws-cdk/cloud-assembly-schema": "^36.0.24",
"@aws-cdk/cloud-assembly-schema": "^38.0.0",
"@aws-cdk/cloudformation-diff": "0.0.0",
"@aws-cdk/cx-api": "0.0.0",
"cdk-assets": "^2.154.0",
"@aws-cdk/aws-service-spec": "^0.1.24",
"cdk-assets": "^2.151.29",

"@aws-cdk/cdk-cli-wrapper": "0.0.0",
"aws-cdk": "0.0.0",
"chalk": "^4",
Expand Down
12 changes: 12 additions & 0 deletions packages/aws-cdk-lib/core/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -1242,6 +1242,18 @@ const stack = new Stack(app, 'StackName', {
});
```

### Receiving CloudFormation Stack Events

You can add one or more SNS Topic ARNs to any Stack:

```ts
const stack = new Stack(app, 'StackName', {
notificationArns: ['arn:aws:sns:us-east-1:23456789012:Topic'],
});
```

Stack events will be sent to any SNS Topics in this list.

### CfnJson

`CfnJson` allows you to postpone the resolution of a JSON blob from
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ export function addStackArtifactToAssembly(
terminationProtection: stack.terminationProtection,
tags: nonEmptyDict(stack.tags.tagValues()),
validateOnSynth: session.validateOnSynth,
notificationArns: stack._notificationArns,
...stackProps,
...stackNameProperty,
};
Expand Down
22 changes: 22 additions & 0 deletions packages/aws-cdk-lib/core/lib/stack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,13 @@ export interface StackProps {
*/
readonly tags?: { [key: string]: string };

/**
* SNS Topic ARNs that will receive stack events.
*
* @default - no notfication arns.
*/
readonly notificationArns?: string[];

/**
* Synthesis method to use while deploying this stack
*
Expand Down Expand Up @@ -364,6 +371,13 @@ export class Stack extends Construct implements ITaggable {
*/
public readonly _crossRegionReferences: boolean;

/**
* SNS Notification ARNs to receive stack events.
*
* @internal
*/
public readonly _notificationArns: string[];

/**
* Logical ID generation strategy
*/
Expand Down Expand Up @@ -451,6 +465,14 @@ export class Stack extends Construct implements ITaggable {
}
this.tags = new TagManager(TagType.KEY_VALUE, 'aws:cdk:stack', props.tags);

for (const notificationArn of props.notificationArns ?? []) {
if (Token.isUnresolved(notificationArn)) {
throw new Error(`Stack '${id}' includes one or more tokens in its notification ARNs: ${props.notificationArns}`);
}
}

this._notificationArns = props.notificationArns ?? [];

if (!VALID_STACK_NAME_REGEX.test(this.stackName)) {
throw new Error(`Stack name must match the regular expression: ${VALID_STACK_NAME_REGEX.toString()}, got '${this.stackName}'`);
}
Expand Down
26 changes: 26 additions & 0 deletions packages/aws-cdk-lib/core/test/stack.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2075,6 +2075,32 @@ describe('stack', () => {
expect(asm.getStackArtifact(stack2.artifactId).tags).toEqual(expected);
});

test('stack notification arns are reflected in the stack artifact properties', () => {
// GIVEN
const NOTIFICATION_ARNS = ['arn:aws:sns:bermuda-triangle-1337:123456789012:MyTopic'];
const app = new App({ stackTraces: false });
const stack1 = new Stack(app, 'stack1', {
notificationArns: NOTIFICATION_ARNS,
});

// WHEN
const asm = app.synth();

// THEN
expect(asm.getStackArtifact(stack1.artifactId).notificationArns).toEqual(NOTIFICATION_ARNS);
});

test('throws if stack notification arns contain tokens', () => {
// GIVEN
const NOTIFICATION_ARNS = ['arn:aws:sns:bermuda-triangle-1337:123456789012:MyTopic'];
const app = new App({ stackTraces: false });

// THEN
expect(() => new Stack(app, 'stack1', {
notificationArns: [...NOTIFICATION_ARNS, Aws.URL_SUFFIX],
})).toThrow('includes one or more tokens in its notification ARNs');
});

test('Termination Protection is reflected in Cloud Assembly artifact', () => {
// if the root is an app, invoke "synth" to avoid double synthesis
const app = new App();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,11 @@ export class CloudFormationStackArtifact extends CloudArtifact {
*/
public readonly tags: { [id: string]: string };

/**
* SNS Topics that will receive stack events.
*/
public readonly notificationArns: string[];

/**
* The physical name of this stack.
*/
Expand Down Expand Up @@ -158,6 +163,7 @@ export class CloudFormationStackArtifact extends CloudArtifact {
// We get the tags from 'properties' if available (cloud assembly format >= 6.0.0), otherwise
// from the stack metadata
this.tags = properties.tags ?? this.tagsFromMetadata();
this.notificationArns = properties.notificationArns ?? [];
this.assumeRoleArn = properties.assumeRoleArn;
this.assumeRoleExternalId = properties.assumeRoleExternalId;
this.cloudFormationExecutionRoleArn = properties.cloudFormationExecutionRoleArn;
Expand Down
18 changes: 18 additions & 0 deletions packages/aws-cdk-lib/cx-api/test/stack-artifact.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,24 @@ afterEach(() => {
rimraf(builder.outdir);
});

test('read notification arns from artifact properties', () => {
// GIVEN
const NOTIFICATION_ARNS = ['arn:aws:sns:bermuda-triangle-1337:123456789012:MyTopic'];
builder.addArtifact('Stack', {
...stackBase,
properties: {
...stackBase.properties,
notificationArns: NOTIFICATION_ARNS,
},
});

// WHEN
const assembly = builder.buildAssembly();

// THEN
expect(assembly.getStackByName('Stack').notificationArns).toEqual(NOTIFICATION_ARNS);
});

test('read tags from artifact properties', () => {
// GIVEN
builder.addArtifact('Stack', {
Expand Down
2 changes: 1 addition & 1 deletion packages/aws-cdk-lib/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@
"@aws-cdk/asset-awscli-v1": "^2.2.202",
"@aws-cdk/asset-kubectl-v20": "^2.1.2",
"@aws-cdk/asset-node-proxy-agent-v6": "^2.1.0",
"@aws-cdk/cloud-assembly-schema": "^36.0.24",
"@aws-cdk/cloud-assembly-schema": "^38.0.0",
"@balena/dockerignore": "^1.0.2",
"case": "1.6.3",
"fs-extra": "^11.2.0",
Expand Down
10 changes: 10 additions & 0 deletions packages/aws-cdk/lib/api/deploy-stack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -644,6 +644,12 @@ async function canSkipDeploy(
return false;
}

// Notification arns have changed
if (!arrayEquals(cloudFormationStack.notificationArns, deployStackOptions.notificationArns ?? [])) {
debug(`${deployName}: notification arns have changed`);
return false;
}

// Termination protection has been updated
if (!!deployStackOptions.stack.terminationProtection !== !!cloudFormationStack.terminationProtection) {
debug(`${deployName}: termination protection has been updated`);
Expand Down Expand Up @@ -694,3 +700,7 @@ function suffixWithErrors(msg: string, errors?: string[]) {
? `${msg}: ${errors.join(', ')}`
: msg;
}

function arrayEquals(a: any[], b: any[]): boolean {
return a.every(item => b.includes(item)) && b.every(item => a.includes(item));
}
11 changes: 10 additions & 1 deletion packages/aws-cdk/lib/api/util/cloudformation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,12 +138,21 @@ export class CloudFormationStack {
/**
* The stack's current tags
*
* Empty list of the stack does not exist
* Empty list if the stack does not exist
*/
public get tags(): CloudFormation.Tags {
return this.stack?.Tags || [];
}

/**
* SNS Topic ARNs that will receive stack events.
*
* Empty list if the stack does not exist
*/
public get notificationArns(): CloudFormation.NotificationARNs {
return this.stack?.NotificationARNs ?? [];
}

/**
* Return the names of all current parameters to the stack
*
Expand Down
Loading

0 comments on commit 1593500

Please sign in to comment.