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(aws-stepfunctions-tasks): allow specifying waitForTaskToken suffix in resourceArn #2686

Merged
merged 9 commits into from
Jun 18, 2019

Conversation

albegali
Copy link
Contributor

@albegali albegali commented May 30, 2019

This PR allows one to work with Task states that implement the callback service integration pattern.

The supported task types are:

  • InvokeFunction (AWS Lambda)
  • SendToQueue (AWS SQS)
  • PublishToTopic (AWS SNS)

Closes #2658, closes #2735.


Pull Request Checklist

  • Testing
    • Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
    • CLI change?: coordinate update of integration tests with team
    • cdk-init template change?: coordinated update of integration tests with team
  • Docs
    • jsdocs: All public APIs documented
    • README: README and/or documentation topic updated
    • Design: For significant features, design document added to design folder
  • Title and Description
    • Change type: title prefixed with fix, feat and module name in parens, which will appear in changelog
    • Title: use lower-case and doesn't end with a period
    • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
    • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
  • Sensitive Modules (requires 2 PR approvers)
    • IAM Policy Document (in @aws-cdk/aws-iam)
    • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
    • Grant APIs (only if not based on official documentation with a reference)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

…ix in resourceArn (aws#2658)

* Add waitForTaskToken property to SentToQueue, InvokeFunction, PublishToTopic task types
* Add payload parameter to InvokeLambda parameters
* Unit test

BREAKING CHANGE: InvokeFunction now requires props as second argument
@albegali albegali requested a review from a team as a code owner May 30, 2019 13:44
eladb
eladb previously requested changes May 30, 2019
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

  1. Please fix the PR title and message
  2. Missing unit tests for all the new surface area
  3. I would also like to see an integration test


private readonly waitForTaskToken: boolean;

constructor(private readonly lambdaFunction: lambda.IFunction, private readonly props: InvokeFunctionProps) {
Copy link
Contributor

Choose a reason for hiding this comment

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

props should be optional (add = { })

/**
* The JSON that you want to provide to your Lambda function as input.
*/
readonly payload?: { [key: string]: string };
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this should be "string => any"

@@ -14,19 +14,33 @@ test('Lambda function can be used in a Task', () => {
handler: 'index.hello',
runtime: lambda.Runtime.Python27,
});
const task = new sfn.Task(stack, 'Task', { task: new tasks.InvokeFunction(fn) });
const task = new sfn.Task(stack, 'Task', {
task: new tasks.InvokeFunction(fn, { waitForTaskToken: false })
Copy link
Contributor

Choose a reason for hiding this comment

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

We usually try not to change existing tests, but rather add new tests. That's a good way to ensure that we didn't break anything. Since waitForTaskToken is false by default, I'd expect this test to remain untouched throughout this change and if it is changed, we need to understand if the change makes sense.

{ "Fn::GetAtt": ["Fn9270CBC0", "Arn"] },
"\"}}}"
]]
"Fn::Join": [
Copy link
Contributor

Choose a reason for hiding this comment

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

revert

@albegali albegali changed the title feat(aws-stepfunctions-tasks): allow specifying waitForTaskToken suff… feat(aws-stepfunctions-tasks): allow specifying waitForTaskToken suffix in resourceArn May 30, 2019
policyStatements: [new iam.PolicyStatement()
.addResource(this.lambdaFunction.functionArn)
.addActions("lambda:InvokeFunction")
],
metricPrefixSingular: 'LambdaFunction',
metricPrefixPlural: 'LambdaFunctions',
metricDimensions: { LambdaFunctionArn: this.lambdaFunction.functionArn },
parameters: {
FunctionName: this.lambdaFunction.functionName,
...this.props.payload && { Payload: this.props.payload },
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think Parameters.Payload will work in case of a task invocation. Will it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would also like an additional validation that if waitForTaskToken is used, the magic token substitution field is used somewhere in the payload. If not, that should be an error. For all classes that implement this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, if props.waitForTaskToken is true, I have to validate props.payload in order to check if something like "token.$":"$$.Task.Token" is present. Did I get it right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I would like you to make a new function on JsonPath (something like JsonPath.taskToken(), similar to the other functions), and make sure one of the fields in the payload structure contains the value returned. Be aware that the payload could be a deep structure, so you will need to recurse into it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I will do the start of this. There is some work I want done in the SFN module anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

What you're going to need is in #2706

Copy link
Contributor

Choose a reason for hiding this comment

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

That other PR has been merged. You may continue :).

Please also have a look at what @wqzoww has written.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @rix0rrr only one thing: may I ask you how to use FieldUtils.containsTaskToken to validate the payload in order to check if the token is present?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would expect you to call it on the payload structure in the constructor, and throw an error if the function returns false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, i've done it in the latest commit. But tests (unit and integ) are failing now, could you take a look please?

Copy link
Contributor

@wqzoww wqzoww left a comment

Choose a reason for hiding this comment

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

Thank you for your initiative to implement "waitForTaskToken" for different service integrations. We could include "arn:aws:states:::lambda:invoke" to Lambda invocation. Please read the comment for more detailed explanation.

@rix0rrr rix0rrr self-assigned this Jun 4, 2019
…ix in resourceArn (aws#2658)

* Merge awslabs:master into albegali:master
* Resolved conflicts
…ix in resourceArn (aws#2658)

* InvokeFunction props is now optional
* Payload can have multiple nesting levels
* Unit test
* Integration test
* Invoke lambda via SFN integrated service ARN
@rix0rrr rix0rrr added third-party This issue is related to third-party libraries or applications. p0 and removed third-party This issue is related to third-party libraries or applications. labels Jun 10, 2019
@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 11, 2019

@wqzoww I've made some changes in response to your statement, can you check my logic?

  • If the user does not specify waitForTaskToken, then we use the Lambda ARN as the Resource target, and we put all of the payload directly into Parameters.
  • If the user DOES specify waitForTaskToken, then we use the magic ARN, and Parameters is the dictionary { FunctionName: '...', Payload: { ... }}.

Two questions:

a) Does that seem correct?
b) Does it preclude any user scenario's?

For the last one I want to be specific: I know this precludes using the magic ARN without using waitForTaskToken, but is that ever really a user scenario? As in, can users achieve behavior that way that they can't in any other way? Or would the 2 be equivalent otherwise?

Because if they're equivalent, I don't see the point in supporting 2 forms of something that would give exactly the same observable behavior.

@julienlepine
Copy link
Contributor

@rix0rrr, when you ise the Magic ARN (the service integration), the behavior of Step Functions in the integration is different, specifically regarding the Input and Output path.

In the old style, the input has to be passed as a verbatim to the Task, without any way to change it beyond what InputPath provides. In the new model, you can use the Payload parameter and change it, this is quite significant.

In the new style as well, the response that is returned from the execution of the Step Function is different, you have a lot more data included in it, if you want to get just the Lambda Function execution result in synchronous mode (invoke), you have to specify an OutputPath of '$.Payload' to dismiss the entire data.

In my opinion these are two distinct use cases of Lambda integration, the old style being far more limited than the new style. As StepFunctions supports both, we are faced with the choice of either supporting both styles (possibly through having two distinct classes), or masking it in a single class, but it will have to be available for the users to specify the expected behavior of Step Functions.

Advantage of having a new class is that we only have to add a single class and associated tests, drawback is that we have a more leaky abstraction on the actual StepFunctions quirks.

Copy link
Contributor

@wqzoww wqzoww left a comment

Choose a reason for hiding this comment

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

@julienlepine @rix0rrr
In the old style, the input of lambda function can be overridden by "Parameters". This field is different from that of the new style because it can contain any key-value pairs that users want to pass.

On the other hand, users can only specify "ClientContext", "FunctionName(Required)", "InvocationType", "Qualifier" and "Payload" inside "Parameters" in the case of Lambda service integration (the new style). If other fields are included in "Parameters" when they create/update their state machine, the request will be rejected with an error message in red on the console. Besides, the input of lambda function can only be given in "Payload". If customers do not specify it in their state machine definition, an empty json object ("{}") will be past as input.

In summary, the differences between Lambda Task (old style) and Lambda Service Integration (new style) are:

Type "Parameters"
Lambda Task Optional, can contain any key-value pairs
Lambda Service Integration Required, its sub-field "FunctionName" is required as well
Type Condition Source of Input
Lambda Task "Parameters" is given "Parameters"
Lambda Task "Parameters" is NOT given Input of the execution OR output from the previous state
Lambda Service Integration "Payload" is given "Payload"
Lambda Service Integration "Payload" is NOT given empty json node
Type "Resource"
Lambda Task ARN of lambda function
Lambda Service Integration - Request Response "arn:aws:states:::lambda:invoke"
Lambda Service Integration - waitForTaskToken "arn:aws:states:::lambda:invoke.waitForTaskToken"

Here is the doc which explains "Request Response" and ".waitForTaskToken". We can also find the service integration patterns for the other AWS services, including SNS, SQS, ECS etc.

In my opinion, we can introduce enum ServiceIntegrationPattern { RequestResponse, Sync, WaitForTaskToken } and ask users to choose one of the patterns ( with an extra variable pattern in interface InvokeFunctionProps). This can help us avoid additional validation in the case of ECS, where multiple boolean variables (sync and WaitForTaskToken) exist but only one of them can be set to true. When users do not specify pattern, we can define "Lambda Task" by default in case of Lambda function invocation.

* Whether to invoke lambda via integrated service ARN "arn:aws:states:::lambda:invoke"
* or via Function ARN.
* If this is set to true, the Context.taskToken value must be included
* somewhere in the payload and the Lambda must call SendTaskSuccess
Copy link
Contributor

Choose a reason for hiding this comment

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

SendTaskSuccess, SendTaskFailure or SendTaskHeartbeat

@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 12, 2019

"ClientContext", "FunctionName(Required)", "InvocationType", "Qualifier" and "Payload"

If we want to support all of these, I think it makes more sense to make 2 different classes for the 2 different invocation styles. They can then also have 2 different Props objects to mirror the 2 sets of arguments.

In the old style, the input has to be passed as a verbatim to the Task, without any way to change it beyond what InputPath provides. In the new model, you can use the Payload parameter and change it, this is quite significant.

According to the table that @wqzoww provided, this is not true. A Parameters field can be supplied even when using "old-style" invocations.

Here is the doc

I don't lik the term "Request Response" very much. It sounds like it should be called "async", which is in effect what it does, but "Request response" gives me the exact opposite intuition.

But it does give me an idea for an alternate class name. How about InvokeFunctionAsync? To me, it perfectly indicates that you need to do "extra work" to make the call sync again, if you wanted to (by passing waitForTaskToken=true).

Now, that kind of implies that we would have to call all other classes Async as well, which is not ideal. And in fact, since most classes end up being Async, we could say that Async is the default. So how about we make:

  • InvokeFunction - pure new-style invocation, with optional waitForTaskToken?: boolean
  • InvokeFunctionSync - old-style invocation, supporting just an optional payload and nothing else.

EDIT: ah poops, this is not true either because sync/async depends on InvocationType.

@wqzoww: does it even make sense to have InvocationType=RequestResponse with .waitForTaskToken ? Wouldn't you always want InvocationType=Event in that case?

How about InvokeFunctionSimple ?

And we need to note in the docstring very well what the expected task output will be between the two variants.

@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 12, 2019

Another question: seems like the Qualifier field to the Invoke API is pretty useless, since the same information could (and would) normally be encoded into the FunctionName.

Is there a use case where one would want to use Qualifier?

https://docs.aws.amazon.com/lambda/latest/dg/API_Invoke.html

@wqzoww
Copy link
Contributor

wqzoww commented Jun 12, 2019

  • InvokeFunctionSync - old-style invocation, supporting just an optional payload and nothing else.

"parameters", not "payload"

does it even make sense to have InvocationType=RequestResponse with .waitForTaskToken ? Wouldn't you always want InvocationType=Event in that case?

I don't think so. "InvocationType" is one of the request parameters for Invoke API of Lambda. Users of Step Functions do not need to specify it in order to use ".waitForTaskToken".

Is there a use case where one would want to use Qualifier?

Qualifier is somewhere users specify the version/alias of their Lambda function. As FunctionName may not contain the right alias, this field is necessary.

@julienlepine
Copy link
Contributor

Yep, my bad it's Parameters. The behavior is quite similar between Parameters and Payload though. The key difference is in the response, with the AWS Service Integration (new model), you have a complete object context, including the output payload, in ARN (old) style you have only the lambda function output.

Passing the qualifier name in the Parameters object or in the function name has implications on the security side. If your role gives lambda:InvokeFunction on the function Arn, your calls with FunctionName and Qualifier work, whatever the version qualifier. If you use a version qualifier in the FunctionName:Qualifier format, you need to have the specific version in your resources for the lambda:InvokeFunction grant.

On the InvocationType=RequestResponse vs InvocationType=Event for .waitForTaskToken tasks, the error management (exception throwing) if ignored on Event mode, so any task that depends on successfully writing the token somewhere before processing would not be able to raise any error, making the state machine wait until the timeout expires instead of having a direct response with the error, and possibly compensation mechanisms.

@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 13, 2019

"parameters", not "payload"

Well yes, but I'm proposing to ALWAYS call it "payload" in CDK (because that's what it is), and hide the details of whether the payload goes into the "Parameters" field or the "{Parameters: Payload}" field. It's the payload in either case.

How are you all feeling on the 2 class design? Any preferences for names?

@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 13, 2019

Passing the qualifier name in the Parameters object or in the function name has implications on the security side. [...]
[...] the error management (exception throwing) if ignored on Event mode, [...]

:( this API, it breaks my head.

Can someone explain to me what the advantage of .waitForTaskToken is if StepFunctions still needs to have a thread sitting there waiting for the potential failure of the InvokeFunction call? Feels like we might as well have a made a regular synchronous call, no?

I had expected the advantage to be that using .waitForTaskToken frees up SFN's thread pool because of its asyncness.

@julienlepine
Copy link
Contributor

.waitForTaskToken and invocation mode are different things.

You can call any Lambda function in two modes: RequestResponse, and Event (sometimes referred to as fireAndForget), wether you are in Step Functions or not. The RequestResponse mode runs the function, waits for the end of it, returns the result, and handles any error that may arise in the execution of the function. The Eventmode calls the function, and returns directly, before the function is even executed. Any error that happens, any result, or any longer-term processing will not be known to the caller, as the only information that was returned to the state machine was that the function call was successful (not its execution).

The .waitForTaskToken changes the state machine model. When it calls the lambda function (in any mode, or Amazon ECS, Amazon SNS, Amazon SQS, AWS Fargate equally), it passes it generates a token, and then it pauses right after the successful execution of the step. (If the step was a RequestResponse step, then this would be the successful completion of the function execution; if it was Event it would be just after the function call). Step Functions then stops execution (and does not use any resource) until it receives a sendTaskSuccess (or sendTaskFailure) call, and uses the data passed to sendTaskSuccess as a result of the execution.

@julienlepine
Copy link
Contributor

On naming, I had one implemented for a side project, I used RunLambdaTask to align it with the ECS execution.

I will create a PR with my changes, not to be accepted, but can be useful for merging.

@julienlepine
Copy link
Contributor

And 2 classes are what I prefer, they are close but clearly not interchangeable.

@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 17, 2019

I used RunLambdaTask to align it with the ECS execution.

I like it!

This was referenced Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Parameters" Parameter for Step Functions Tasks Support AWS Step Functions callback patterns in workflow
6 participants