-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Conversation
There was a problem hiding this 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.
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'); | ||
}); |
There was a problem hiding this comment.
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)', () => { |
* @internal | ||
*/ | ||
export function singletonEventRole(scope: IConstruct): iam.IRole { | ||
const id = 'SchedulesRole'; |
There was a problem hiding this comment.
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.
- 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Why do we not also narrow down of the use of the role to "aws:SourceArn" "aws:SourceAccount"
There was a problem hiding this comment.
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
(
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; | |
} |
|
||
const principal = new iam.PrincipalWithConditions(new iam.ServicePrincipal('scheduler.amazonaws.com'), { | ||
StringEquals: { | ||
'aws:SourceAccount': schedule.env.account, |
There was a problem hiding this comment.
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);
queue.addToResourcePolicy(new iam.PolicyStatement({ | ||
sid: policyStatementId, | ||
principals: [new iam.ServicePrincipal('events.amazonaws.com')], | ||
effect: iam.Effect.ALLOW, | ||
actions: ['sqs:SendMessage'], | ||
resources: [queue.queueArn], |
There was a problem hiding this comment.
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.
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, | ||
}); |
There was a problem hiding this comment.
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
There was a problem hiding this 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/rosetta/default.ts-fixture
Outdated
Show resolved
Hide resolved
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. |
There was a problem hiding this 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.
}); | ||
|
||
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'); |
There was a problem hiding this comment.
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
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
wtf? |
@filletofish still another error related to this PR it seems |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
This PR contains implementation for Schedule Targets:
Schedule
from private package to depend on it inschedule-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