-
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
fix(stepfunctions-tasks): in-flight stepfunction executions fail on update of versioned lambdas #21233
fix(stepfunctions-tasks): in-flight stepfunction executions fail on update of versioned lambdas #21233
Conversation
e2c5a4c
to
2038466
Compare
…g lambda functions via qualifiers To establish the current functionality before makes changes to this feature.
…tion should grant permissions for all qualifiers
2038466
to
83459a7
Compare
@@ -0,0 +1,80 @@ | |||
import { Code, Function, Runtime } from '@aws-cdk/aws-lambda'; |
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.
This file is copied from integ.invoke.ts
and adjusted to use a Version and an Alias.
…voke all versions of a lambda function
…g lambda qualifiers The issue has been fixed, now the integration tests need to be updated. As expected, the PolicyDocument now lists and additional resource for the Version.
83459a7
to
5263ffb
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
*/ | ||
private determineResourceArnsForGrantInvoke(lambdaFunction: IFunction): string[] { | ||
if (isVersion(lambdaFunction)) { | ||
return lambdaFunction.lambda.resourceArnsForGrantInvoke; |
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.
Is version.resourceArnsForGrantInvoke
ever correct, in the face of changing versions?
Isn't it easier to just update the behavior of Version
?
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.
Semantically, I'd expect lambdaVersion.grantInvoke(role)
to grant permissions for the specified version and not all qualifiers. Otherwise I would do lambdaVersion.grantInvoke(role)
. That's a far reaching change to make.
The idiomatic way to solve this problem in AWS is to use an Alias. I'm on the fence, but because of this I'm more inclined to argue this "works as intended", than to change the behavior of Version
.
Maybe we're going about this the wrong way and we need to solve the undeniably bad customer experience differently.
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.
@rix0rrr @madeline-k #20177 seems to be of a similar nature. Specifically @kaizencc made the point that if users want to "[..] grant less permissible policies, grant them on the version or alias you've created, not the function itself.". Changing Version
to also grant permissions to all versions, would contradict this.
You've all clearly thought about this in-depth, so I'm very happy to follow your lead.
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 think Kaizen is right. Users should have created an alias, and invoked that. Invoking by currentVersion
seems always incorrect, given the code that CDK generates.
In fact we should probably discourage it in the docs?
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.
@rix0rrr So close this PR, update the docs, and then close the related issue with explanation?
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.
@rix0rrr Can you quickly confirm if I understood you correctly regarding this PR and issue?
So close this PR, update the docs, and then close the related issue with explanation?
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.
Agreed with @rix0rrr that this PR does not address the root cause of the issue.
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.
Please make sure that your PR title confirms to the conventional commit standard (fix
, feat
, chore
) and that it is written in a style that will reflect correctly in the change log (See Contributing Guide, Pull Requests)
This change doesn't address the root cause. See comments above. Further discussions continue on #17515 |
When the Step Function task is invoking a Lambda Version, we used to only grant invoke permissions for the specific Version. This can cause in-flight executions of the Step Function to fail due to missing permissions, since changing the referenced Version would cause the Policy to remove permissions for the previous Version - which is currently in-flight.
The fix here is to add
invokeFunction
permissions for all qualifiers of the Lambda Function whenever a Version is provided. While this is not ideal, it enables users to safely update Lambda Versions withLambdaInvoke
. I believe this is a common enough use case to warrant this fix.However, it's worth noting that the same issue will still occur if e.g. the Function is updated to a completely different one or an Alias is changed.
fixes #17515
All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integ
to deploy the infrastructure and generate the snapshot (i.e.yarn integ
without--dry-run
)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license