feat(aws-stepfunctions-tasks): allow specifying waitForTaskToken suffix in resourceArn #2686

albegali · 2019-05-30T13:44:09Z

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

eladb

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

eladb · 2019-05-30T13:47:14Z

packages/@aws-cdk/aws-stepfunctions-tasks/lib/invoke-function.ts

+
+  private readonly waitForTaskToken: boolean;
+
+  constructor(private readonly lambdaFunction: lambda.IFunction, private readonly props: InvokeFunctionProps) {


props should be optional (add = { })

eladb · 2019-05-30T13:47:34Z

packages/@aws-cdk/aws-stepfunctions-tasks/lib/invoke-function.ts

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


I believe this should be "string => any"

packages/@aws-cdk/aws-stepfunctions-tasks/lib/invoke-function.ts

eladb · 2019-05-30T13:50:25Z

packages/@aws-cdk/aws-stepfunctions-tasks/test/invoke-function.test.ts

@@ -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 })


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.

eladb · 2019-05-30T13:50:31Z

packages/@aws-cdk/aws-stepfunctions-tasks/test/invoke-function.test.ts

-        { "Fn::GetAtt": ["Fn9270CBC0", "Arn"] },
-        "\"}}}"
-      ]]
+      "Fn::Join": [


rix0rrr · 2019-05-30T15:23:29Z

packages/@aws-cdk/aws-stepfunctions-tasks/lib/invoke-function.ts

      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 },


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

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.

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?

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.

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

What you're going to need is in #2706

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

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

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?

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

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

wqzoww

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.

packages/@aws-cdk/aws-stepfunctions-tasks/lib/invoke-function.ts

…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 · 2019-06-11T12:30:51Z

@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 · 2019-06-11T18:41:23Z

@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.

wqzoww

@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.

wqzoww · 2019-06-11T22:40:47Z

packages/@aws-cdk/aws-stepfunctions-tasks/lib/invoke-function.ts

-   * 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


SendTaskSuccess, SendTaskFailure or SendTaskHeartbeat

rix0rrr · 2019-06-12T08:59:16Z

"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 · 2019-06-12T09:06:09Z

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 · 2019-06-12T18:10:00Z

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 · 2019-06-12T19:36:29Z

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 · 2019-06-13T10:16:01Z

"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 · 2019-06-13T10:19:03Z

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 · 2019-06-13T16:13:49Z

.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 · 2019-06-13T16:21:36Z

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 · 2019-06-13T16:22:29Z

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

rix0rrr · 2019-06-17T11:55:16Z

I used RunLambdaTask to align it with the ECS execution.

I like it!

rix0rrr · 2019-06-17T14:52:11Z

You are missing the Qualifier field in the task definition, but maybe we can put it as a next feature (support version and aliases).

I explicitly don't want that, because it doesn't play nicely with permissions.

Stale

albegali requested a review from a team as a code owner May 30, 2019 13:44

eladb previously requested changes May 30, 2019

View reviewed changes

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

rix0rrr reviewed May 30, 2019

View reviewed changes

wqzoww reviewed Jun 2, 2019

View reviewed changes

packages/@aws-cdk/aws-stepfunctions-tasks/lib/invoke-function.ts Outdated Show resolved Hide resolved

rix0rrr self-assigned this Jun 4, 2019

alberto-galimberti added 2 commits June 4, 2019 10:04

feat(aws-stepfunctions-tasks): allow specifying waitForTaskToken suff…

ccf38b9

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

feat(aws-stepfunctions-tasks): allow specifying waitForTaskToken suff…

9dd544b

…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 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

Merge remote-tracking branch 'origin/master' into pr/albegali/2686

5b49cde

Remove invokeAsIntegratedService, fix tests

38faecd

wqzoww reviewed Jun 12, 2019

View reviewed changes

julienlepine mentioned this pull request Jun 13, 2019

feat(aws-stepfunctions-tasks): service integration invocation for lambda (including callback pattern) #2864

Closed

4 tasks

Rico Huijbers added 2 commits June 17, 2019 14:21

Merge remote-tracking branch 'origin/master' into pr/albegali/2686

6dbc5ce

Split off into RunLambdaTask class.

3769f13

Fix

8788fc3

rix0rrr approved these changes Jun 18, 2019

View reviewed changes

rix0rrr merged commit d017a14 into aws:master Jun 18, 2019

wqzoww mentioned this pull request Jun 19, 2019

fix(aws-stepfunctions, aws-stepfunctions-tasks): missing suffix ".$" in field names of reference paths #2937

Closed

This was referenced Jan 20, 2020

[Snyk] Upgrade @aws-cdk/aws-route53 from 0.22.0 to 0.39.0 MechanicalRock/tech-radar#20

Closed

[Snyk] Upgrade @aws-cdk/aws-s3 from 0.22.0 to 0.39.0 MechanicalRock/tech-radar#21

Closed

shivlaks mentioned this pull request Apr 16, 2020

StepFunctions - RunLambdaTask with Payload not working #7371

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(aws-stepfunctions-tasks): allow specifying waitForTaskToken suffix in resourceArn #2686

feat(aws-stepfunctions-tasks): allow specifying waitForTaskToken suffix in resourceArn #2686

albegali commented May 30, 2019 •

edited by rix0rrr

Loading

eladb left a comment

eladb May 30, 2019

eladb May 30, 2019

eladb May 30, 2019

eladb May 30, 2019

rix0rrr May 30, 2019

rix0rrr May 30, 2019

albegali May 30, 2019

rix0rrr May 31, 2019

rix0rrr May 31, 2019

rix0rrr May 31, 2019

rix0rrr Jun 4, 2019

albegali Jun 4, 2019

rix0rrr Jun 4, 2019

albegali Jun 4, 2019

wqzoww left a comment

rix0rrr commented Jun 11, 2019

julienlepine commented Jun 11, 2019

wqzoww left a comment •

edited

Loading

wqzoww Jun 11, 2019

rix0rrr commented Jun 12, 2019 •

edited

Loading

rix0rrr commented Jun 12, 2019

wqzoww commented Jun 12, 2019

julienlepine commented Jun 12, 2019

rix0rrr commented Jun 13, 2019

rix0rrr commented Jun 13, 2019

julienlepine commented Jun 13, 2019

julienlepine commented Jun 13, 2019

julienlepine commented Jun 13, 2019

rix0rrr commented Jun 17, 2019

rix0rrr commented Jun 17, 2019


		private readonly waitForTaskToken: boolean;

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

feat(aws-stepfunctions-tasks): allow specifying waitForTaskToken suffix in resourceArn #2686

feat(aws-stepfunctions-tasks): allow specifying waitForTaskToken suffix in resourceArn #2686

Conversation

albegali commented May 30, 2019 • edited by rix0rrr Loading

Pull Request Checklist

eladb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wqzoww left a comment

Choose a reason for hiding this comment

rix0rrr commented Jun 11, 2019

julienlepine commented Jun 11, 2019

wqzoww left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr commented Jun 12, 2019 • edited Loading

rix0rrr commented Jun 12, 2019

wqzoww commented Jun 12, 2019

julienlepine commented Jun 12, 2019

rix0rrr commented Jun 13, 2019

rix0rrr commented Jun 13, 2019

julienlepine commented Jun 13, 2019

julienlepine commented Jun 13, 2019

julienlepine commented Jun 13, 2019

rix0rrr commented Jun 17, 2019

rix0rrr commented Jun 17, 2019

albegali commented May 30, 2019 •

edited by rix0rrr

Loading

wqzoww left a comment •

edited

Loading

rix0rrr commented Jun 12, 2019 •

edited

Loading