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.

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

fix(aws-autoscaling): notificationTargetArn should be optional in LifecycleHook #16187

Merged
merged 42 commits into from
Dec 11, 2021
Merged
Show file tree
Hide file tree
Changes from 38 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
8049337
made notification target an optional parameter
comcalvi Aug 23, 2021
7f341aa
do not need to check the dependency, because it should not depend on …
comcalvi Aug 23, 2021
aecde32
removed blank line
comcalvi Aug 23, 2021
71570cb
added test cases and fixed the CFN deploy error
comcalvi Aug 24, 2021
fc1f700
tmp
comcalvi Aug 24, 2021
0d56782
updated integration test
comcalvi Aug 24, 2021
fac2b68
added new integ tests
comcalvi Aug 24, 2021
5d2cb32
Revert "added new integ tests"
comcalvi Aug 24, 2021
7552fd6
resolved build issue
comcalvi Aug 24, 2021
65adb8c
updated error messages
comcalvi Aug 24, 2021
7713834
integ test now passes
comcalvi Aug 25, 2021
d7fc329
added unit tests to verify errors thrown if roles are undefined in hooks
comcalvi Aug 25, 2021
d8975f3
removed duplicate expected file
comcalvi Aug 25, 2021
8ed2e95
removed old comments
comcalvi Aug 25, 2021
5899ec4
improved formatting and docs
comcalvi Aug 25, 2021
3cac985
improved formatting
comcalvi Aug 25, 2021
d7fc092
updated formatting
comcalvi Aug 25, 2021
0f60a5a
improved formatting and naming
comcalvi Aug 25, 2021
6d09e28
improved integ testing format
comcalvi Aug 25, 2021
096a8e0
moving role creation to hook implemementations
comcalvi Aug 26, 2021
b06a3ff
need to finish updating test utilities to generate roles
comcalvi Aug 26, 2021
d8ab53b
currently updating code to use the newly defined role get / set methods
comcalvi Aug 27, 2021
944d8b3
removed unneeded try-catch paradigm
comcalvi Aug 27, 2021
a571636
updated tests and fixed role not found errors
comcalvi Aug 30, 2021
6d9e1dd
changed test descriptions
comcalvi Aug 30, 2021
7ab7ce2
updated test formatting
comcalvi Aug 30, 2021
35524cc
improved formatting
comcalvi Aug 30, 2021
9d30712
made formatting of assertion consistent
comcalvi Aug 30, 2021
aac3c1c
fixed breaking change
comcalvi Aug 31, 2021
e3856e3
resolved merge conflicts with master
comcalvi Oct 28, 2021
d36614d
removed unneeded space
comcalvi Oct 28, 2021
4c2d73f
implemented new API approach
comcalvi Oct 29, 2021
00c8c39
added docs
comcalvi Oct 29, 2021
1051f9a
Merge branch 'master' of https://github.com/aws/aws-cdk into makeNoti…
comcalvi Dec 9, 2021
84dec21
fixed merge issues
comcalvi Dec 9, 2021
4c5f614
fixed commit issues
comcalvi Dec 9, 2021
a38cdda
add to breaking changes
comcalvi Dec 9, 2021
73109fc
add to breaking changes
comcalvi Dec 9, 2021
a2773f5
moved createRole() out of autoscaling
comcalvi Dec 10, 2021
16e5a1f
fixed circular dependency
comcalvi Dec 10, 2021
e43dc23
Merge branch 'master' into makeNotificationTargetOptional
comcalvi Dec 10, 2021
60131cc
Merge branch 'master' into makeNotificationTargetOptional
mergify[bot] Dec 11, 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
7 changes: 7 additions & 0 deletions allowed-breaking-changes.txt
Original file line number Diff line number Diff line change
Expand Up @@ -86,3 +86,10 @@ removed:@aws-cdk/aws-stepfunctions-tasks.EmrCreateClusterProps.autoTerminationPo
# Changed property securityGroupId to optional because either securityGroupId or
# securityGroupName is required. Therefore securityGroupId is no longer mandatory.
weakened:@aws-cdk/cloud-assembly-schema.SecurityGroupContextQuery

# refactor autoscaling lifecycle hook target bind() methods to make role optional by
# having bind() methods create the role if it isn't passed to them
incompatible-argument:@aws-cdk/aws-autoscaling-hooktargets.FunctionHook.bind
incompatible-argument:@aws-cdk/aws-autoscaling-hooktargets.QueueHook.bind
incompatible-argument:@aws-cdk/aws-autoscaling-hooktargets.TopicHook.bind
incompatible-argument:@aws-cdk/aws-autoscaling.ILifecycleHookTarget.bind
17 changes: 12 additions & 5 deletions packages/@aws-cdk/aws-autoscaling-hooktargets/lib/lambda-hook.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { TopicHook } from './topic-hook';

// keep this import separate from other imports to reduce chance for merge conflicts with v2-main
// eslint-disable-next-line no-duplicate-imports, import/order
import { Construct } from '@aws-cdk/core';
import { Construct } from 'constructs';

/**
* Use a Lambda Function as a hook target
Expand All @@ -23,16 +23,23 @@ export class FunctionHook implements autoscaling.ILifecycleHookTarget {
constructor(private readonly fn: lambda.IFunction, private readonly encryptionKey?: kms.IKey) {
}

public bind(scope: Construct, lifecycleHook: autoscaling.ILifecycleHook): autoscaling.LifecycleHookTargetConfig {
const topic = new sns.Topic(scope, 'Topic', {
/**
* If the `IRole` does not exist in `options`, will create an `IRole` and an SNS Topic and attach both to the lifecycle hook.
* If the `IRole` does exist in `options`, will only create an SNS Topic and attach it to the lifecycle hook.
*/
public bind(_scope: Construct, options: autoscaling.BindHookTargetOptions): autoscaling.LifecycleHookTargetConfig {
const topic = new sns.Topic(_scope, 'Topic', {
masterKey: this.encryptionKey,
});

const role = autoscaling.createRole(_scope, options.role);

// Per: https://docs.aws.amazon.com/sns/latest/dg/sns-key-management.html#sns-what-permissions-for-sse
// Topic's grantPublish() is in a base class that does not know there is a kms key, and so does not
// grant appropriate permissions to the kms key. We do that here to ensure the correct permissions
// are in place.
this.encryptionKey?.grant(lifecycleHook.role, 'kms:Decrypt', 'kms:GenerateDataKey');
this.encryptionKey?.grant(role, 'kms:Decrypt', 'kms:GenerateDataKey');
topic.addSubscription(new subs.LambdaSubscription(this.fn));
return new TopicHook(topic).bind(scope, lifecycleHook);
return new TopicHook(topic).bind(_scope, { lifecycleHook: options.lifecycleHook, role });
}
}
22 changes: 18 additions & 4 deletions packages/@aws-cdk/aws-autoscaling-hooktargets/lib/queue-hook.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import * as autoscaling from '@aws-cdk/aws-autoscaling';
import * as sqs from '@aws-cdk/aws-sqs';
import { Construct } from '@aws-cdk/core';

// keep this import separate from other imports to reduce chance for merge conflicts with v2-main
// eslint-disable-next-line no-duplicate-imports, import/order
import { Construct } from 'constructs';

/**
* Use an SQS queue as a hook target
Expand All @@ -9,8 +12,19 @@ export class QueueHook implements autoscaling.ILifecycleHookTarget {
constructor(private readonly queue: sqs.IQueue) {
}

public bind(_scope: Construct, lifecycleHook: autoscaling.ILifecycleHook): autoscaling.LifecycleHookTargetConfig {
this.queue.grantSendMessages(lifecycleHook.role);
return { notificationTargetArn: this.queue.queueArn };
/**
* If an `IRole` is found in `options`, grant it access to send messages.
* Otherwise, create a new `IRole` and grant it access to send messages.
*
* @returns the `IRole` with access to send messages and the ARN of the queue it has access to send messages to.
*/
public bind(_scope: Construct, options: autoscaling.BindHookTargetOptions): autoscaling.LifecycleHookTargetConfig {
const role = autoscaling.createRole(_scope, options.role);
this.queue.grantSendMessages(role);

return {
notificationTargetArn: this.queue.queueArn,
createdRole: role,
};
}
}
22 changes: 18 additions & 4 deletions packages/@aws-cdk/aws-autoscaling-hooktargets/lib/topic-hook.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import * as autoscaling from '@aws-cdk/aws-autoscaling';
import * as sns from '@aws-cdk/aws-sns';
import { Construct } from '@aws-cdk/core';

// keep this import separate from other imports to reduce chance for merge conflicts with v2-main
// eslint-disable-next-line no-duplicate-imports, import/order
import { Construct } from 'constructs';

/**
* Use an SNS topic as a hook target
Expand All @@ -9,8 +12,19 @@ export class TopicHook implements autoscaling.ILifecycleHookTarget {
constructor(private readonly topic: sns.ITopic) {
}

public bind(_scope: Construct, lifecycleHook: autoscaling.ILifecycleHook): autoscaling.LifecycleHookTargetConfig {
this.topic.grantPublish(lifecycleHook.role);
return { notificationTargetArn: this.topic.topicArn };
/**
* If an `IRole` is found in `options`, grant it topic publishing permissions.
* Otherwise, create a new `IRole` and grant it topic publishing permissions.
*
* @returns the `IRole` with topic publishing permissions and the ARN of the topic it has publishing permission to.
*/
public bind(_scope: Construct, options: autoscaling.BindHookTargetOptions): autoscaling.LifecycleHookTargetConfig {
const role = autoscaling.createRole(_scope, options.role);
this.topic.grantPublish(role);

return {
notificationTargetArn: this.topic.topicArn,
createdRole: role,
};
}
}
Loading