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

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

Merged
merged 14 commits into from
Aug 27, 2023

Conversation

filletofish
Copy link
Contributor

@filletofish filletofish commented Jul 30, 2023

This PR contains implementation for Schedule Targets:

  1. Creates a separate module for targets
  2. 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),
  3. 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 aws-cdk-automation requested a review from a team July 30, 2023 13:25
@github-actions github-actions bot added repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK p2 labels Jul 30, 2023
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines 13 to 17
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');
});
Copy link
Contributor Author

@filletofish filletofish Jul 30, 2023

Choose a reason for hiding this comment

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

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

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

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

packages/@aws-cdk/aws-scheduler-alpha/lib/target.ts Outdated Show resolved Hide resolved
* @internal
*/
export function singletonEventRole(scope: IConstruct): iam.IRole {
const id = 'SchedulesRole';
Copy link
Contributor

Choose a reason for hiding this comment

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

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

  1. 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.
  2. 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally preferred this implementation inspired by events.target (

/**
* 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.


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

Choose a reason for hiding this comment

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

@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);

Comment on lines 188 to 193
queue.addToResourcePolicy(new iam.PolicyStatement({
sid: policyStatementId,
principals: [new iam.ServicePrincipal('events.amazonaws.com')],
effect: iam.Effect.ALLOW,
actions: ['sqs:SendMessage'],
resources: [queue.queueArn],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Comment on lines 19 to 30
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,
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-scheduler-alpha/lib/target.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-scheduler-alpha/lib/target.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-scheduler-alpha/lib/target.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-scheduler-alpha/lib/target.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-scheduler-alpha/test/input.test.ts Outdated Show resolved Hide resolved
@aws-cdk-automation
Copy link
Collaborator

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 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 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
Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

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

@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 Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-scheduler-alpha/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-scheduler-alpha/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-scheduler-alpha/lib/schedule.ts Outdated Show resolved Hide resolved
});

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');
Copy link
Contributor

Choose a reason for hiding this comment

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

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

tl;dr: cross env tests need to use 234567890123

@kaizencc kaizencc added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Aug 25, 2023
@aws-cdk-automation aws-cdk-automation dismissed their stale review August 25, 2023 14:30

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

kaizencc
kaizencc previously approved these changes Aug 25, 2023
@kaizencc
Copy link
Contributor

@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?

@mergify mergify bot dismissed kaizencc’s stale review August 25, 2023 22:38

Pull request has been modified.

@kaizencc
Copy link
Contributor

@filletofish still another error related to this PR it seems

@aws-cdk-automation
Copy link
Collaborator

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

@aws-cdk-automation 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
@mergify
Copy link
Contributor

mergify bot commented Aug 27, 2023

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2 pr-linter/do-not-close The PR linter will not close this PR while this label is present pr-linter/exempt-integ-test The PR linter will not require integ test changes repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants