-
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
feat(apigateway): step functions integration #16827
feat(apigateway): step functions integration #16827
Conversation
Title does not follow the guidelines of Conventional Commits. Please adjust title before merge. |
f740690
to
c300c8e
Compare
packages/@aws-cdk/aws-apigateway/lib/integrations/stepFunctions.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-apigateway/lib/integrations/stepFunctions.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-apigateway/lib/integrations/stepFunctions.ts
Outdated
Show resolved
Hide resolved
6352467
to
f001265
Compare
packages/@aws-cdk/aws-apigateway/lib/integrations/stepFunctions.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-apigateway/lib/integrations/stepFunctions.ts
Outdated
Show resolved
Hide resolved
f13f153
to
ef176a9
Compare
packages/@aws-cdk/aws-apigateway/lib/integrations/stepFunctions.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-apigateway/lib/integrations/stepFunctions.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-apigateway/lib/integrations/stepFunctions.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-apigateway/lib/integrations/stepFunctions.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-apigateway/lib/integrations/stepFunctions.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-apigateway/test/integ.stepFunctions-api.deploymentStack.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-apigateway/test/integ.stepFunctions-api.deploymentStack.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-apigateway/test/stepFunctions-api.test.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-apigateway/test/integrations/stepFunctions.test.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-apigateway/test/integrations/stepFunctions.test.ts
Outdated
Show resolved
Hide resolved
4ff8a66
to
d3b6f74
Compare
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 add a section to the README with usage snippets.
d3b6f74
to
30372d3
Compare
Added |
30372d3
to
d41369b
Compare
packages/@aws-cdk/aws-apigateway/lib/integrations/stepFunctions.ts
Outdated
Show resolved
Hide resolved
@saqibdhuka thank you for making this, it's very exciting. I tried it out yesterday and have a couple of questions/comments. The first mistake I made was forgetting to make my Step Function an express workflow. I only realised my error after trying to invoke the API, because it successfully deployed but failed to invoke. I wonder if there is some way to warn/error if the user provides the wrong type of state machine? Once I fixed that, it worked great. I see that it passes the request body to the sfn execution. In our environment, 99.9% of our APIs have authorization and our integrations need to know the identity of the authorized user. I don't think that is possible currently. I wonder if it preferable to change the execution input to instead more closely reflect the input format provided by API GW to "Lambda proxy integrations". This would also provide a way to receive request headers, etc. This would be more complex for the simple unauthorized use case, but more usable for the authorized use case. I understand that there's value in the opinionated simple configuration, but it would be a shame for this construct to not get much usage due to this limitation. |
d41369b
to
933399b
Compare
I've been thinking it through a bit more and discussing with @diegotry. I assume this construct is modelled to be similar to --- a/stack.ts
+++ b/stack.ts
@@ -1,20 +1,20 @@
declare const backend: lambda.Function;
- new apigateway.LambdaRestApi(this, 'myapi', {
- handler: backend,
- });
+ const task = new tasks.LambdaInvoke(this, "Handle", { lambdaFunction: backend });
+ const stateMachine = new sfn.StateMachine(this, 'StateMachineX', {
+ stateMachineType: sfn.StateMachineType.EXPRESS,
+ definition: task,
+ });
+
+ new apigateway.StepFunctionsRestApi(this, 'mystepapi', {
+ handler: stateMachine,
+ }); What do you think? If we change the VTL template to more closely resemble the Lambda proxy integration input then it makes migration easier for people. |
Pull request has been modified.
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>
…ns.test.ts Co-authored-by: Niranjan Jayakar <nija@amazon.com>
This reverts commit dc3b54f.
Co-authored-by: Niranjan Jayakar <nija@amazon.com>
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.
Looks good. Let me know once the PR is rebased and I'll be happy to approve.
I've made some tweaks to the PR - #16827 (commits). Hope those are fine with you.
const regex = new RegExp(`^${pattern.split('*').map(escapeRegex).join('.*')}$`); | ||
|
||
// needs rebase from https://github.com/aws/aws-cdk/pull/17692/files | ||
const regex = new RegExp(`^${pattern.split('*').map(escapeRegex).join('.*')}$`, 'm'); |
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.
Amazing! I've approved the PR and it is now merged.
const stateMachineDefinition = new stepfunctions.Pass(this, 'PassState'); | ||
|
||
const stateMachine: stepfunctions.IStateMachine = new stepfunctions.StateMachine(this, 'StateMachine', { | ||
definition: stateMachineDefinition, | ||
stateMachineType: stepfunctions.StateMachineType.EXPRESS, | ||
}); | ||
|
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've pushed a commit that fixes this up.
Pull request has been modified.
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.
Thanks for being patient with me and working through this! 🙏
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 |
- Added StepFunctionsRestApi and StepFunctionsIntegration implementation closes aws#15081. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
- Added StepFunctionsRestApi and StepFunctionsIntegration implementation closes aws#15081. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
closes #15081.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license