feat(scheduler): base target methods and lambda invoke target #26575

filletofish · 2023-07-30T13:25:07Z

This PR contains implementation for Schedule Targets:

Creates a separate module for targets
Support imported resources, but not cross account, cross region resources as we discussed in RFC. The unit tests should cover 4 cases (target and role within the same stack, target is imported, role is imported, target and role are imported),
I have moved out class Schedule from private package to depend on it in schedule-targets unit tests.

Implementation is based on RFC: https://github.com/aws/aws-cdk-rfcs/blob/master/text/0474-event-bridge-scheduler-l2.md

Advances #23394

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

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

packages/@aws-cdk/aws-scheduler-alpha/test/group.test.ts

filletofish · 2023-07-30T13:43:18Z

packages/@aws-cdk/aws-scheduler-alpha/test/target.test.ts

+ beforeEach(() => {
+ const app = new App();
+ stack = new Stack(app, 'Stack', { env: { region: 'us-east-1', account: '123456789012' } });
+ func = lambda.Function.fromFunctionArn(stack, 'Function', 'arn:aws:lambda:us-east-1:123456789012:function/somefunc');
+ });


Created role and stack in the same account. Otherwise lambda.Function.grantInvoke fails as it assumes that function is imported (from a different account). See

aws-cdk/packages/aws-cdk-lib/aws-lambda/lib/function-base.ts

Line 528 in 0e2c169

if (!permissionNode && !this._skipPermissions) {

Also, grantInvoke method creates permission on Lambda function itself (updates resource policy) if the role is in the different account. See

aws-cdk/packages/aws-cdk-lib/aws-lambda/test/function.test.ts

Line 1425 in 0e2c169

test('with an imported role (from a different account)', () => {

packages/@aws-cdk/aws-scheduler-alpha/lib/target.ts

Jacco · 2023-08-04T06:37:27Z

packages/@aws-cdk/aws-scheduler-alpha/lib/target.ts

+ * @internal
+ */
+export function singletonEventRole(scope: IConstruct): iam.IRole {
+ const id = 'SchedulesRole';


In the code we prepared this had a lot more beef.

You use one role named 'SchedulesRole' for all targets in the current stack. The more targets there are the less "least privilege' this will get. Personally I think one role per target makes more sense then one role per stack.

The code we started with had Stack.of(schedule) and then stack.node.tryFindChild(id) in stead of scope.node.tryFindChild. If you find the role for a target it is fine.

Why do we not also narrow down of the use of the role to "aws:SourceArn" "aws:SourceAccount"

I originally preferred this implementation inspired by events.target (

aws-cdk/packages/aws-cdk-lib/aws-events-targets/lib/util.ts

Lines 63 to 81 in e479bd4

/**

* Obtain the Role for the EventBridge event

*

* If a role already exists, it will be returned. This ensures that if multiple

* events have the same target, they will share a role.

* @internal

*/

export function singletonEventRole(scope: IConstruct): iam.IRole {

const id = 'EventsRole';

const existing = scope.node.tryFindChild(id) as iam.IRole;

if (existing) { return existing; }

const role = new iam.Role(scope as Construct, id, {

roleName: PhysicalName.GENERATE_IF_NEEDED,

assumedBy: new iam.ServicePrincipal('events.amazonaws.com'),

});

return role;

}

) as it seemed simpler to me. However, I agree that the least privilege policy is a good point. Now I understand where the complexity is coming from.

filletofish · 2023-08-11T10:16:15Z

packages/@aws-cdk/aws-scheduler-alpha/lib/target.ts

+
+ const principal = new iam.PrincipalWithConditions(new iam.ServicePrincipal('scheduler.amazonaws.com'), {
+ StringEquals: {
+ 'aws:SourceAccount': schedule.env.account,


@Jacco I have removed condition for checking sourceArn as it created a circular dependency between schedule -> target -> role -> schedule.

In your original prototype it was implemented as:

const scheduleArn = Arn.format({ service: 'scheduler', resourceName: `${schedule.group.groupName}/${schedule.scheduleName}`, resource: 'schedule' }, Stack.of(schedule)); const principal = new iam.PrincipalWithConditions(new iam.ServicePrincipal('scheduler.amazonaws.com'), { 'StringEquals': { "aws:SourceArn": scheduleArn, "aws:SourceAccount": schedule.env.account, } })

It didn't create a dependency cycle in your code, as now the scheduleName is a reference to the CFN resource instead of just a string:

this.scheduleName = this.getResourceNameAttribute(resource.ref);

filletofish · 2023-08-11T10:17:18Z

packages/@aws-cdk/aws-scheduler-alpha/lib/target.ts

+ queue.addToResourcePolicy(new iam.PolicyStatement({
+ sid: policyStatementId,
+ principals: [new iam.ServicePrincipal('events.amazonaws.com')],
+ effect: iam.Effect.ALLOW,
+ actions: ['sqs:SendMessage'],
+ resources: [queue.queueArn],


@Jacco similarly, there was an option to create a condition for sourceArn here. But it would create a dependency cycle between target and schedule.

filletofish · 2023-08-11T10:19:02Z

packages/@aws-cdk/aws-scheduler-alpha/test/group.test.ts

+ const app = new App();
+ stack = new Stack(app, 'Stack', { env: { region: 'us-east-1', account: '123456789012' } });
+ role = new iam.Role(stack, 'Role', {
+ roleName: 'someRole',
+ assumedBy: new iam.AccountRootPrincipal(),
+ });
+ func = new lambda.Function(stack, 'MyLambda', {
+ code: new lambda.InlineCode('foo'),
+ handler: 'index.handler',
+ runtime: lambda.Runtime.NODEJS_14_X,
+ tracing: lambda.Tracing.PASS_THROUGH,
+ });


Had to specify environment here, otherwise schedule creation failed with an error that target and schedule should be in the same region / account. That's because we consider target and schedule to be in different accounts if one of their accounts is unresolved.

See sameEnvDimension implementation.

Let me know if that's a problem

packages/@aws-cdk/aws-scheduler-alpha/test/input.test.ts

kaizencc

A few minor comments mostly to get more information. But my main comment is that I would like to see the README first, as that is usually the entrypoint to the PR as a reviewer.

packages/@aws-cdk/aws-scheduler-alpha/lib/target.ts

packages/@aws-cdk/aws-scheduler-alpha/test/input.test.ts

packages/@aws-cdk/aws-scheduler-alpha/lib/schedule.ts

packages/@aws-cdk/aws-scheduler-alpha/rosetta/default.ts-fixture

aws-cdk-automation · 2023-08-21T00:07:57Z

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

kaizencc

@filletofish I'm ready to approve this, just some minor stuff surrounding docs.

Next up has to be adding integ tests where appropriate. We won't accept any more additions until the integ tests are up and running.

packages/@aws-cdk/aws-scheduler-alpha/README.md

packages/@aws-cdk/aws-scheduler-alpha/lib/schedule.ts

packages/@aws-cdk/aws-scheduler-alpha/lib/target.ts

kaizencc · 2023-08-25T14:25:42Z

packages/@aws-cdk/aws-scheduler-targets-alpha/test/target.test.ts

+ });
+
+ test('throws when lambda function is imported from different account', () => {
+ const importedFunc = lambda.Function.fromFunctionArn(stack, 'ImportedFunction', 'arn:aws:lambda:us-east-1:111122223333:function/somefunc');


TIL as well, but take a look at this file here

tl;dr: cross env tests need to use 234567890123

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

kaizencc · 2023-08-25T19:57:58Z

@aws-cdk/aws-lambda-python-alpha: ImportError: urllib3 v2.0 only supports OpenSSL 1.1.1+, currently the 'ssl' module is compiled with 'OpenSSL 1.0.2k-fips  26 Jan 2017'. See: https://github.com/urllib3/urllib3/issues/2168

wtf?

Pull request has been modified.

kaizencc · 2023-08-26T00:45:50Z

@filletofish still another error related to this PR it seems

aws-cdk-automation · 2023-08-27T15:11:06Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 0dfc2aa
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2023-08-27T17:23:49Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Implement base target methods and lambda invoke target

9f75472

aws-cdk-automation requested a review from a team July 30, 2023 13:25

github-actions bot added repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK p2 labels Jul 30, 2023

aws-cdk-automation previously requested changes Jul 30, 2023

View reviewed changes

filletofish commented Jul 30, 2023

View reviewed changes

packages/@aws-cdk/aws-scheduler-alpha/test/group.test.ts Outdated Show resolved Hide resolved

filletofish commented Jul 30, 2023

View reviewed changes

github-actions bot mentioned this pull request Aug 1, 2023

Monthly PRs metrics report - July 2023 #26583

Closed

Jacco reviewed Aug 4, 2023

View reviewed changes

filletofish added 3 commits August 11, 2023 10:04

Add group to ISchedule

83093ab

Update target and target test

d42d9c0

Update target dlq tests

9d80485

filletofish commented Aug 11, 2023

View reviewed changes

packages/@aws-cdk/aws-scheduler-alpha/test/input.test.ts Show resolved Hide resolved

kaizencc reviewed Aug 16, 2023

View reviewed changes

filletofish added 4 commits August 20, 2023 07:49

Remove role cache for creating singleton roles

9446b8e

Move targets to a separate package

69b96ab

Update READMEs

911555b

Refactor helper methods to ScheduleTargetBase abstract class

bb0cd71

filletofish commented Aug 20, 2023

View reviewed changes

packages/@aws-cdk/aws-scheduler-alpha/lib/schedule.ts Show resolved Hide resolved

filletofish commented Aug 20, 2023

View reviewed changes

packages/@aws-cdk/aws-scheduler-alpha/rosetta/default.ts-fixture Outdated Show resolved Hide resolved

kaizencc added the pr-linter/do-not-close The PR linter will not close this PR while this label is present label Aug 23, 2023

kaizencc changed the title ~~feat(scheduler): Implement base target methods and lambda invoke target~~ feat(scheduler): base target methods and lambda invoke target Aug 24, 2023

kaizencc reviewed Aug 24, 2023

View reviewed changes

filletofish added 2 commits August 25, 2023 09:46

Update readme and doc strings

68a9d40

Add schedule examples to README.md

a1d6468

kaizencc reviewed Aug 25, 2023

View reviewed changes

kaizencc added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Aug 25, 2023

filletofish and others added 2 commits August 25, 2023 20:12

Update aws account id for cross-env tests

6f4a8b0

Merge branch 'main' into feat/23394-targets

079a54f

kaizencc previously approved these changes Aug 25, 2023

View reviewed changes

Merge branch 'main' into feat/23394-targets

1cf42ab

Update yarn.lock file

0dfc2aa

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Aug 27, 2023

kaizencc approved these changes Aug 27, 2023

View reviewed changes

mergify bot merged commit 39cbd46 into aws:main Aug 27, 2023
13 checks passed

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Aug 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(scheduler): base target methods and lambda invoke target #26575

feat(scheduler): base target methods and lambda invoke target #26575

filletofish commented Jul 30, 2023 •

edited

Loading

aws-cdk-automation left a comment

filletofish Jul 30, 2023 •

edited

Loading

Jacco Aug 4, 2023

Jacco Aug 4, 2023

filletofish Aug 7, 2023

filletofish Aug 11, 2023

filletofish Aug 11, 2023

filletofish Aug 11, 2023

kaizencc left a comment

aws-cdk-automation commented Aug 21, 2023

kaizencc left a comment

kaizencc Aug 25, 2023

kaizencc commented Aug 25, 2023

kaizencc commented Aug 26, 2023

aws-cdk-automation commented Aug 27, 2023

mergify bot commented Aug 27, 2023

	/**
	* Obtain the Role for the EventBridge event
	*
	* If a role already exists, it will be returned. This ensures that if multiple
	* events have the same target, they will share a role.
	* @internal
	*/
	export function singletonEventRole(scope: IConstruct): iam.IRole {
	const id = 'EventsRole';
	const existing = scope.node.tryFindChild(id) as iam.IRole;
	if (existing) { return existing; }

	const role = new iam.Role(scope as Construct, id, {
	roleName: PhysicalName.GENERATE_IF_NEEDED,
	assumedBy: new iam.ServicePrincipal('events.amazonaws.com'),
	});

	return role;
	}

feat(scheduler): base target methods and lambda invoke target #26575

feat(scheduler): base target methods and lambda invoke target #26575

Conversation

filletofish commented Jul 30, 2023 • edited Loading

aws-cdk-automation left a comment

Choose a reason for hiding this comment

filletofish Jul 30, 2023 • edited Loading

Choose a reason for hiding this comment

Jacco Aug 4, 2023

Choose a reason for hiding this comment

Jacco Aug 4, 2023

Choose a reason for hiding this comment

filletofish Aug 7, 2023

Choose a reason for hiding this comment

filletofish Aug 11, 2023

Choose a reason for hiding this comment

filletofish Aug 11, 2023

Choose a reason for hiding this comment

filletofish Aug 11, 2023

Choose a reason for hiding this comment

kaizencc left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Aug 21, 2023

kaizencc left a comment

Choose a reason for hiding this comment

kaizencc Aug 25, 2023

Choose a reason for hiding this comment

kaizencc commented Aug 25, 2023

kaizencc commented Aug 26, 2023

aws-cdk-automation commented Aug 27, 2023

AWS CodeBuild CI Report

mergify bot commented Aug 27, 2023

filletofish commented Jul 30, 2023 •

edited

Loading

filletofish Jul 30, 2023 •

edited

Loading