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(apigateway): step functions integration #16827

Merged
merged 38 commits into from Nov 25, 2021

Conversation

saqibdhuka
Copy link
Contributor

  • Added StepFunctionsRestApi and StepFunctionsIntegration implementation

closes #15081.


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

@gitpod-io
Copy link

gitpod-io bot commented Oct 6, 2021

@mergify
Copy link
Contributor

mergify bot commented Oct 6, 2021

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

@saqibdhuka saqibdhuka changed the title StepFunctionsRestApi feat(apigateway): add REST API Integration -- Step Functions Oct 6, 2021
@saqibdhuka saqibdhuka force-pushed the StepFunctionsRestApi-branch-issue#15081 branch from f740690 to c300c8e Compare October 6, 2021 17:46
@saqibdhuka saqibdhuka force-pushed the StepFunctionsRestApi-branch-issue#15081 branch 2 times, most recently from 6352467 to f001265 Compare October 7, 2021 16:23
packages/@aws-cdk/aws-apigateway/lib/stepFunctions-api.ts Outdated Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
@saqibdhuka saqibdhuka force-pushed the StepFunctionsRestApi-branch-issue#15081 branch 2 times, most recently from f13f153 to ef176a9 Compare October 7, 2021 23:05
@nija-at nija-at changed the title feat(apigateway): add REST API Integration -- Step Functions feat(apigateway): step functions integration Oct 11, 2021
@saqibdhuka saqibdhuka force-pushed the StepFunctionsRestApi-branch-issue#15081 branch 2 times, most recently from 4ff8a66 to d3b6f74 Compare October 13, 2021 21:56
@nija-at nija-at self-assigned this Oct 14, 2021
Copy link
Contributor

@nija-at nija-at left a 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.

@saqibdhuka saqibdhuka force-pushed the StepFunctionsRestApi-branch-issue#15081 branch from d3b6f74 to 30372d3 Compare October 14, 2021 17:02
@mergify mergify bot dismissed nija-at’s stale review October 14, 2021 17:03

Pull request has been modified.

@saqibdhuka
Copy link
Contributor Author

Please add a section to the README with usage snippets.

Added

@saqibdhuka saqibdhuka force-pushed the StepFunctionsRestApi-branch-issue#15081 branch from 30372d3 to d41369b Compare October 14, 2021 17:44
@aidansteele
Copy link

aidansteele commented Oct 18, 2021

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

@saqibdhuka saqibdhuka force-pushed the StepFunctionsRestApi-branch-issue#15081 branch from d41369b to 933399b Compare October 18, 2021 22:26
@aidansteele
Copy link

I've been thinking it through a bit more and discussing with @diegotry. I assume this construct is modelled to be similar to apigateway.LambdaRestApi. I think it might help with adoption if the following was possible:

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

@saqibdhuka saqibdhuka closed this Oct 20, 2021
@saqibdhuka saqibdhuka reopened this Oct 20, 2021
@mergify mergify bot dismissed nija-at’s stale review November 24, 2021 15:10

Pull request has been modified.

diegotry and others added 11 commits November 24, 2021 07:44
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.
Copy link
Contributor

@nija-at nija-at left a 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.

Comment on lines 246 to 248
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');
Copy link
Contributor

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.

Comment on lines 187 to 193
const stateMachineDefinition = new stepfunctions.Pass(this, 'PassState');

const stateMachine: stepfunctions.IStateMachine = new stepfunctions.StateMachine(this, 'StateMachine', {
definition: stateMachineDefinition,
stateMachineType: stepfunctions.StateMachineType.EXPRESS,
});

Copy link
Contributor

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.

@mergify mergify bot dismissed nija-at’s stale review November 25, 2021 11:51

Pull request has been modified.

Copy link
Contributor

@nija-at nija-at left a 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! 🙏

@mergify mergify bot merged commit cb31547 into aws:master Nov 25, 2021
@mergify
Copy link
Contributor

mergify bot commented Nov 25, 2021

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-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 57763b8
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

beezly pushed a commit to beezly/aws-cdk that referenced this pull request Nov 29, 2021
- 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*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
- 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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-apigateway Related to Amazon API Gateway
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(aws-stepfunctions): L3 Construct for API Gateway HTTP/REST Api Backed by Step Functions
6 participants