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(stepfunctions-tasks): Support calling ApiGateway REST and HTTP APIs #13033

Merged
merged 9 commits into from
Mar 10, 2021
Merged

feat(stepfunctions-tasks): Support calling ApiGateway REST and HTTP APIs #13033

merged 9 commits into from
Mar 10, 2021

Conversation

ayush987goyal
Copy link
Contributor

@ayush987goyal ayush987goyal commented Feb 14, 2021

feat(stepfunctions-tasks): Support calling APIGW REST and HTTP APIs

Taking ownership of the original PR #11565 by @Sumeet-Badyal

API as per documentation here:
https://docs.aws.amazon.com/step-functions/latest/dg/connect-api-gateway.html
https://docs.aws.amazon.com/apigateway/latest/developerguide/api-gateway-control-access-using-iam-policies-to-invoke-api.html

closes #11566
closes #11565


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 Feb 14, 2021

@ayush987goyal
Copy link
Contributor Author

Hi @shivlaks and @nija-at ,

Could you please take a look at it? I added support for both the API types since there was a lot of common code and it gave a right direction for abstraction upfront.

@shivlaks
Copy link
Contributor

awesome, thanks @ayush987goyal - I'll review it this week.

@ayush987goyal
Copy link
Contributor Author

Hi @shivlaks , quick reminder for this :)

@mariobittencourt
Copy link

Hi @ayush987goyal , there is a conflict. Could you solve it?
@shivlaks any chance this could make it to this week's build?

const httpApi = new apigatewayv2.HttpApi(stack, 'HttpApi');

// WHEN
const task = new InvokeApiGatewayHttpApi(stack, 'Invoke', {
Copy link

Choose a reason for hiding this comment

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

Bookmarks to see if there's a way to grant permissions in the case of the API route having AWS_IAM auth on it

Copy link

Choose a reason for hiding this comment

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

Yep; looks like the props here take an authType that can be set to AuthType.IAM_ROLE

Look forward to putting this to use when it lands

Copy link
Contributor

@shivlaks shivlaks left a comment

Choose a reason for hiding this comment

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

@ayush987goyal thanks for picking this one up! looking good. left some minor feedback.

would also love if @nija-at could take a look as the primary keeper of the API Gateway modules

@ayush987goyal
Copy link
Contributor Author

@shivlaks I think this change is ready to be closed now. Could you please take a look?

@shivlaks shivlaks added the pr/do-not-merge This PR should not be merged at this time. label Mar 5, 2021
shivlaks
shivlaks previously approved these changes Mar 5, 2021
Copy link
Contributor

@shivlaks shivlaks left a comment

Choose a reason for hiding this comment

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

@ayush987goyal - LGTM!! added a do-not-merge as I'd love for @nija-at to take a look as well, primarily because these are new stable APIs and he reviewed the predecessor PR :)

great work!! 🎉

@ayush987goyal
Copy link
Contributor Author

@nija-at We meet again so soon! :P

*
* @see https://docs.aws.amazon.com/step-functions/latest/dg/connect-api-gateway.html
*/
export class InvokeApiGatewayHttpApi extends InvokeApiGatewayApiBase {
Copy link
Contributor

Choose a reason for hiding this comment

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

We use the word Invoke in the context of lambda because that's intuitive to lambda and is the official term for its operation. Should we find a different word here, say Call? (This is what the official docs call it)

Why have you chosen to separate these into two different classes? How would it be if this was modified to a single class, say CallApiGatewayEndpoint, which takes two optional properties of types IHttpApi and IRestApi and enforces one, and only one, of them is provided?

Copy link
Contributor Author

@ayush987goyal ayush987goyal Mar 8, 2021

Choose a reason for hiding this comment

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

  1. I agree on using Call instead of Invoke.
  2. I implemented it this way to make the separation explicit so that if in future there is a way to call WebSocket API as well (which I believe might have different way compared to these two), we have clear separation and callers will not get confused. Given that, I am still okay with implementing a single class with validations around the passed props. Let me know your thoughts @nija-at @shivlaks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Another point for separation could be that some props (eg. stageName) can be optional for one but required for other. Also, some prop might not even be supported for one of them.

@mergify mergify bot dismissed shivlaks’s stale review March 9, 2021 04:52

Pull request has been modified.

This was referenced Mar 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[stepfunctions-tasks] API Gateway support
6 participants