feat(aws-lambda) add support for forwarded request method/uri/body #2823

thibaultcha · 2017-08-19T02:28:49Z

Summary

Depends on: #2822

Reworked version of #2470 from @akh00.

This fixes a few issues with #2470 regarding inconsistent request body parsing, some inconsistent naming for the new schema values and a slightly different approach regarding the forging of the forwarded upstream request body. It also addresses quite a few style and maintainability issues.

Full changelog

New optional configuration properties:

forward_request_method
forward_request_uri
forward_request_headers
forward_request_body

If specified, the request body sent to the invoked Lambda will contain
the desired attributes of the client's request as a JSON payload.

If none of the above values are specified, the upstream body contains a
JSON payload which is merged from the request's body data and its
query string. This is to preserve backwards compatibility with previous
versions of this plugin.

Issues resolved

Fix #2379
Fix #2469

akh00 · 2017-08-22T09:16:18Z

kong/tools/public.lua

-      map_type = 3
+      req_mime = MIME_TYPES.form_url_encoded
+
+    elseif str_find(content_type, "application/xml", nil, true) then


I think it's better to add "text/xml" (for SOAP 1.1 etc) and "application/soap+xml" (for SOAP 1.2)

I feel like SOAP should be made into its own MIME type (in the above MIME_TYPE enum), but let's maybe keep it as XML for now.

Added in the latest revision of this patch!

akh00 · 2017-08-22T09:21:24Z

The pull request solves the issue I addressed in #2470 with the exception of Content-type: text/xml and Content-type: application/soap+xml

thibaultcha · 2017-08-22T20:12:58Z

The pull request solves the issue I addressed

Did you notice a few of the differences, namely:

the new forwarding options have new names are not required anymore.
forward_body will mean both the plain and parsed (args) body. Ditto for forward_uri. The previous behavior was to forward either plain or parsed, but never both at the same time.

Are you fine with those changes and do they impact one of your use-cases?

p0pr0ck5

two minor comments, lgtm otherwise

p0pr0ck5 · 2017-08-23T00:55:21Z

kong/plugins/aws-lambda/handler.lua

+    local body_args, err_code, body_raw = public_utils.get_body_info()
+    if err_code == public_utils.req_body_errors.unknown_ct then
+      -- don't know what this body MIME type is, base64 it just in case
+      body_raw = ngx_encode_base64(body_raw)


initially i was thinking im not entirely sure about this approach. what about content types that are already base64 encoded. perhaps a flag to encode unknown types vs. passing them through. but not a blocker, and we start to run into edge case territory here

I'm wondering if it's worse to insert property indicates that body has been base64 encoded (when it's done in plugin) into request to lambda

I'm wondering if it's worse to insert property indicates that body has been base64 encoded (when it's done in plugin) into request to lambda

Yes, I was having the same thought. Something like upstream_body.request_body.base64 = true maybe? We could also not send what we think is binary data to the function - until someone comes to us with a good-case for that? Do you already have such a use-case for processing binary data in Lambda functions?

@thibaultcha "upstream_body.request_body.base64 = true" - sounds good. The use case for sending binary to lambda is replacing with lambda some existing services that are using binary protocols (or just compressed data to decrease traffic) for instance: fastinfoset, avro, protobuf. Also AWS API Gateway supports binary content https://aws.amazon.com/blogs/compute/binary-support-for-api-integrations-with-amazon-api-gateway/

@akh00 Flag added! ;)

p0pr0ck5 · 2017-08-23T00:55:37Z

kong/plugins/aws-lambda/handler.lua

+      body_raw = ngx_encode_base64(body_raw)
+    end
+
+    if conf.forward_request_body   or conf.forward_request_headers


extra whitespace in front of or

akh00 · 2017-08-23T09:11:41Z

@thibaultcha

Did you notice a few of the differences, namely:
Yes I noticed the differences and I'm find with that. Thank you

thibaultcha · 2017-08-25T18:12:38Z

@akh00 @p0pr0ck5 I think this is approaching readiness, I would like to merge this soon so we can ship it in our next release. If everything looks square for you, no need to add more, otherwise please speak up! :)

decoursin · 2017-08-28T08:07:26Z

How can I help to get this merged?

thibaultcha · 2017-08-28T22:22:29Z

@decoursin @akh00 At this point, I believe we only need docs to be written for this new feature. Docs are open source and can be modified here: https://github.com/Mashape/getkong.org

I would very much like for open source contributors to be responsible for writing docs as well, instead of dropping the burden on the Kong team, who is already very busy :)

Thanks!

decoursin · 2017-08-29T08:02:43Z

Here, I've updated docs for forward_request_headers and forward_request_body: Kong/docs.konghq.com#505. I didn't know what to write for forward_request_uri, forward_request_method, forward_request_headers.

akh00 · 2017-08-29T16:18:54Z

@thibaultcha @p0pr0ck5 @decoursin Added pull request with updated documentation Kong/docs.konghq.com#506

decoursin · 2017-08-29T19:09:48Z

Can we please merge this now?

thibaultcha · 2017-08-29T19:30:04Z

@decoursin If you really need this, feel free to apply the patch yourself and you could use it as of today. Even merged, this won't be released until 0.11.1, for which we don't have a release date yet. The Kong team is extremely busy with its own road-map already.

Options added in Kong/kong#2823 Signed-off-by: Thibault Charbonnier <[email protected]>

New optional configuration properties: - `forward_request_method` - `forward_request_uri` - `forward_request_headers` - `forward_request_body` If specified, the request body sent to the invoked Lambda will contain the desired attributes of the client's request as a JSON payload. If none of the above values are specified, the upstream body contains a JSON payload which is merged from the request's body data and its query string. This is to preserve backwards compatibility with previous versions of this plugin. Original patch from Andrei Kishkevich <[email protected]> Reworked by Thibault Charbonnier <[email protected]> Signed-off-by: Thibault Charbonnier <[email protected]>

thibaultcha · 2017-09-01T23:01:35Z

@akh00 @decoursin Merged! 🎉 Thanks both of you for your help and contributions!

Options added in Kong/kong#2823 Signed-off-by: Thibault Charbonnier <[email protected]>

thibaultcha added the pr/status/please review label Aug 19, 2017

This was referenced Aug 19, 2017

feat(utils) new get_body_info() public API #2822

Merged

Adding support for request body and http headers #2470

Closed

thibaultcha force-pushed the feat/aws-lambda-forward-request branch from 7846bc2 to df757ba Compare August 21, 2017 21:10

akh00 reviewed Aug 22, 2017

View reviewed changes

thibaultcha force-pushed the feat/aws-lambda-forward-request branch from df757ba to b3bd2d2 Compare August 22, 2017 20:27

p0pr0ck5 reviewed Aug 23, 2017

View reviewed changes

thibaultcha force-pushed the feat/aws-lambda-forward-request branch from b3bd2d2 to 86fbe37 Compare August 25, 2017 03:02

akh00 approved these changes Aug 25, 2017

View reviewed changes

thibaultcha added pr/status/needs docs and removed pr/status/please review labels Aug 28, 2017

decoursin mentioned this pull request Aug 29, 2017

Update docs: aws-lambda plugin Kong/docs.konghq.com#505

Closed

thibaultcha pushed a commit to Kong/docs.konghq.com that referenced this pull request Aug 30, 2017

docs(aws-lambda) add request forwarding options

fdd51d2

Options added in Kong/kong#2823 Signed-off-by: Thibault Charbonnier <[email protected]>

thibaultcha added pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes) and removed pr/status/needs docs labels Sep 1, 2017

thibaultcha force-pushed the feat/aws-lambda-forward-request branch from 86fbe37 to 8381735 Compare September 1, 2017 22:14

thibaultcha force-pushed the feat/aws-lambda-forward-request branch from 8381735 to 6d6e68b Compare September 1, 2017 22:38

thibaultcha merged commit 752f3b8 into master Sep 1, 2017

thibaultcha deleted the feat/aws-lambda-forward-request branch September 1, 2017 23:01

thibaultcha pushed a commit to Kong/docs.konghq.com that referenced this pull request Oct 19, 2017

docs(aws-lambda) add request forwarding options

fb04726

Options added in Kong/kong#2823 Signed-off-by: Thibault Charbonnier <[email protected]>

thibaultcha pushed a commit to Kong/docs.konghq.com that referenced this pull request Oct 24, 2017

docs(aws-lambda) add request forwarding options

7df029f

Options added in Kong/kong#2823 Signed-off-by: Thibault Charbonnier <[email protected]>

decoursin mentioned this pull request Nov 2, 2017

incorrectly parses multipart/form-data #2851

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(aws-lambda) add support for forwarded request method/uri/body #2823

feat(aws-lambda) add support for forwarded request method/uri/body #2823

thibaultcha commented Aug 19, 2017

akh00 Aug 22, 2017

thibaultcha Aug 22, 2017

thibaultcha Aug 22, 2017

thibaultcha Aug 22, 2017

akh00 commented Aug 22, 2017

thibaultcha commented Aug 22, 2017

p0pr0ck5 left a comment

p0pr0ck5 Aug 23, 2017

akh00 Aug 23, 2017

thibaultcha Aug 23, 2017

akh00 Aug 24, 2017

thibaultcha Aug 25, 2017

p0pr0ck5 Aug 23, 2017

akh00 commented Aug 23, 2017

thibaultcha commented Aug 25, 2017 •

edited

Loading

decoursin commented Aug 28, 2017

thibaultcha commented Aug 28, 2017

decoursin commented Aug 29, 2017 •

edited

Loading

akh00 commented Aug 29, 2017

decoursin commented Aug 29, 2017

thibaultcha commented Aug 29, 2017

thibaultcha commented Sep 1, 2017

feat(aws-lambda) add support for forwarded request method/uri/body #2823

feat(aws-lambda) add support for forwarded request method/uri/body #2823

Conversation

thibaultcha commented Aug 19, 2017

Summary

Full changelog

Issues resolved

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

akh00 commented Aug 22, 2017

thibaultcha commented Aug 22, 2017

p0pr0ck5 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

akh00 commented Aug 23, 2017

thibaultcha commented Aug 25, 2017 • edited Loading

decoursin commented Aug 28, 2017

thibaultcha commented Aug 28, 2017

decoursin commented Aug 29, 2017 • edited Loading

akh00 commented Aug 29, 2017

decoursin commented Aug 29, 2017

thibaultcha commented Aug 29, 2017

thibaultcha commented Sep 1, 2017

thibaultcha commented Aug 25, 2017 •

edited

Loading

decoursin commented Aug 29, 2017 •

edited

Loading