-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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(stepfunctions): execution history logging options #6933
Conversation
0f356da
to
5842aab
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
// https://docs.aws.amazon.com/step-functions/latest/dg/cw-logs.html#cloudwatch-iam-policy | ||
this.addToRolePolicy(new iam.PolicyStatement({ | ||
effect: iam.Effect.ALLOW, | ||
actions: [ | ||
"logs:CreateLogDelivery", | ||
"logs:GetLogDelivery", | ||
"logs:UpdateLogDelivery", | ||
"logs:DeleteLogDelivery", | ||
"logs:ListLogDeliveries", | ||
"logs:PutResourcePolicy", | ||
"logs:DescribeResourcePolicies", | ||
"logs:DescribeLogGroups" | ||
], | ||
resources: ["*"] | ||
})); |
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.
We try our best to keep least privilege, so avoid *
resource.
Use loggroup.grant()
API instead.
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.
Those permissions are not bound to resources but more general CloudWatch Logs permissions allowing StepFunctions to write into the account: https://docs.aws.amazon.com/IAM/latest/UserGuide/list_amazoncloudwatchlogs.html
There is no resource which can be referenced.
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.
Weirdly, I can't seem to find any documentation on log delivery, so I can't quite say what this is or what it's about. I'll try to gather more information.
On the other hand, the ability to PutResourcePolicy
across all log groups in the account seems a bit excessive. Could you check if the following works instead -
In addition to the policy around log delivery
effect: iam.Effect.ALLOW,
actions: [ 'PutLogEvents', 'GetLogEvents' ],
resources: [ `${conf.destination.logGroupArn}:log-stream:*` ]
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.
As I mentioned the permissions are not bound to a resource but for the whole account. That's also shown in the docs I linked. It does not work with a LogGroup/LogStream restriction.
packages/@aws-cdk/aws-stepfunctions/test/integ.stepfunctions-logging.ts
Outdated
Show resolved
Hide resolved
Thanks for your contribution! Once you have addressed these changes and the PR is ready for review, dismiss my review and it'll show up in my inbox. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
// https://docs.aws.amazon.com/step-functions/latest/dg/cw-logs.html#cloudwatch-iam-policy | ||
this.addToRolePolicy(new iam.PolicyStatement({ | ||
effect: iam.Effect.ALLOW, | ||
actions: [ | ||
"logs:CreateLogDelivery", | ||
"logs:GetLogDelivery", | ||
"logs:UpdateLogDelivery", | ||
"logs:DeleteLogDelivery", | ||
"logs:ListLogDeliveries", | ||
"logs:PutResourcePolicy", | ||
"logs:DescribeResourcePolicies", | ||
"logs:DescribeLogGroups" | ||
], | ||
resources: ["*"] | ||
})); |
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.
Weirdly, I can't seem to find any documentation on log delivery, so I can't quite say what this is or what it's about. I'll try to gather more information.
On the other hand, the ability to PutResourcePolicy
across all log groups in the account seems a bit excessive. Could you check if the following works instead -
In addition to the policy around log delivery
effect: iam.Effect.ALLOW,
actions: [ 'PutLogEvents', 'GetLogEvents' ],
resources: [ `${conf.destination.logGroupArn}:log-stream:*` ]
Co-Authored-By: Niranjan Jayakar <nija@amazon.com>
Co-Authored-By: Niranjan Jayakar <nija@amazon.com>
Co-Authored-By: Niranjan Jayakar <nija@amazon.com>
Co-Authored-By: Niranjan Jayakar <nija@amazon.com>
Not needed according to the PR feedback.
cc7210e
to
bb6f9fd
Compare
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 master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
@workeitel - I'm going to approve this PR again. Please don't click on 'Update branch'. The bot will take care of everything. |
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
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 master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Commit Message
feat(stepfunctions): execution history logging options (#6933)
closes #5754
End Commit Message
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license