-
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
Closed
mrgrain
wants to merge
4
commits into
aws:main
from
mrgrain:fix/stepfunctions-tasks/lambda-versions-permissions
Closed
fix(stepfunctions-tasks): in-flight stepfunction executions fail on update of versioned lambdas #21233
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
c0a03d6
test(stepfunctions-tasks): add baseline integration tests for invokin…
mrgrain 1d28826
test(stepfunctions-tasks): invoking a lambda version from a step func…
mrgrain 771cbe5
fix(stepfunctions-tasks): grant step functions task permissions to in…
mrgrain 5263ffb
test(stepfunctions-tasks): update integration tests after for invokin…
mrgrain File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
80 changes: 80 additions & 0 deletions
80
packages/@aws-cdk/aws-stepfunctions-tasks/test/lambda/integ.invoke.qualifiers.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. This file is copied from |
||
import * as sfn from '@aws-cdk/aws-stepfunctions'; | ||
import * as cdk from '@aws-cdk/core'; | ||
import * as integ from '@aws-cdk/integ-tests'; | ||
import { LambdaInvoke } from '../../lib'; | ||
|
||
|
||
/* | ||
* Creates a state machine with a task state to invoke a Lambda function | ||
* via Alias and Version qualifiers. | ||
* | ||
* The state machine creates a couple of Lambdas that pass results forward | ||
* and into a Choice state that validates the output. | ||
*/ | ||
const app = new cdk.App(); | ||
const stack = new cdk.Stack(app, 'aws-stepfunctions-tasks-lambda-invoke-integ'); | ||
|
||
const submitJobLambda = new Function(stack, 'submitJobLambda', { | ||
code: Code.fromInline(`exports.handler = async () => { | ||
return { | ||
statusCode: '200', | ||
body: 'hello, world!' | ||
}; | ||
};`), | ||
runtime: Runtime.NODEJS_14_X, | ||
handler: 'index.handler', | ||
}); | ||
|
||
const submitJob = new LambdaInvoke(stack, 'Invoke Handler', { | ||
lambdaFunction: submitJobLambda.currentVersion, | ||
outputPath: '$.Payload', | ||
}); | ||
|
||
const checkJobStateLambda = new Function(stack, 'checkJobStateLambda', { | ||
code: Code.fromInline(`exports.handler = async function(event, context) { | ||
return { | ||
status: event.statusCode === '200' ? 'SUCCEEDED' : 'FAILED' | ||
}; | ||
};`), | ||
runtime: Runtime.NODEJS_14_X, | ||
handler: 'index.handler', | ||
}); | ||
const checkJobStateLambdaAlias = checkJobStateLambda.addAlias('IntegTest'); | ||
|
||
const checkJobState = new LambdaInvoke(stack, 'Check the job state', { | ||
lambdaFunction: checkJobStateLambdaAlias, | ||
resultSelector: { | ||
status: sfn.JsonPath.stringAt('$.Payload.status'), | ||
}, | ||
}); | ||
|
||
const isComplete = new sfn.Choice(stack, 'Job Complete?'); | ||
const jobFailed = new sfn.Fail(stack, 'Job Failed', { | ||
cause: 'Job Failed', | ||
error: 'Received a status that was not 200', | ||
}); | ||
const finalStatus = new sfn.Pass(stack, 'Final step'); | ||
|
||
const chain = sfn.Chain.start(submitJob) | ||
.next(checkJobState) | ||
.next( | ||
isComplete | ||
.when(sfn.Condition.stringEquals('$.status', 'FAILED'), jobFailed) | ||
.when(sfn.Condition.stringEquals('$.status', 'SUCCEEDED'), finalStatus), | ||
); | ||
|
||
const sm = new sfn.StateMachine(stack, 'StateMachine', { | ||
definition: chain, | ||
timeout: cdk.Duration.seconds(30), | ||
}); | ||
|
||
new cdk.CfnOutput(stack, 'stateMachineArn', { | ||
value: sm.stateMachineArn, | ||
}); | ||
|
||
new integ.IntegTest(app, 'StepFunctionsInvokeLambdaQualifiersTest', { | ||
testCases: [stack], | ||
}); | ||
|
||
app.synth(); |
1 change: 1 addition & 0 deletions
1
...shot/StepFunctionsInvokeLambdaQualifiersTestDefaultTestDeployAssert57F7A2BF.template.json
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
{} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 dolambdaVersion.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?
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.