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

enabling lambda functions to return custom status, body, and headers #3427

Closed

Conversation

Projects
None yet
7 participants
@aloisbarreras
Copy link
Contributor

aloisbarreras commented May 1, 2018

NOTE: Please read the CONTRIBUTING.md guidelines before submitting your patch,
and ensure you followed them all:

https://github.com/Kong/kong/blob/master/CONTRIBUTING.md#contributing

Summary

Currently, the AWS Lambda plugin will only return a 200, 202, or 204 HTTP status response code on successful requests. This adds the ability for Lambda functions to define custom response status codes by returning a specific object. For example returning the following from a Lambda function will set custom values on the Kong response.

{
    statusCode = "201",
    body = {
      key1 = "some_value_post1",
      key2 = "some_value_post2",
      key3 = "some_value_post3"
    },
    headers = {
      ["X-Custom-Header"] = "Hello world!"
    }
}

Returning this from your Lambda function would set that status to 201, the response body to body and add the custom headers in headers.

This same functionality is already present with API Gateway, and it would be great to replicate it here. It would make switching from API Gateway to Kong seamless :).

Full changelog

  • Update plugin to look for a statusCode, body, and headers in the Lambda response and apply them to the Kong response.
  • Added test.
@perryao

This comment has been minimized.

Copy link
Contributor

perryao commented May 29, 2018

Any feedback on this? Or just waiting for the next release?

@aloisbarreras

This comment has been minimized.

Copy link
Contributor Author

aloisbarreras commented Jun 14, 2018

Hey guys! Any chance on this making the next release?

@gaffney

This comment has been minimized.

Copy link

gaffney commented Jun 28, 2018

Thanks for the sweet contribution! Works for me on my now super-patched Kong Lambda plugin.

Python syntax of the above:

return {
        'statusCode': 201,
        'body': {
          'key1': 'some_value_post1',
          'key2': 'some_value_post2',
          'key3': 'some_value_post3'
        },
        'headers': {
            'X-Custom-Header': 'Hello world!'
        }
    }
@Curistofa

This comment has been minimized.

Copy link

Curistofa commented Jul 9, 2018

AWS plugin throws an error if the lambda doesn't return a response body
2018/07/09 17:02:27 [error] 6295#0: *20927 [lua] public.lua:64: could not read request body (ngx.req.get_body_data() returned nil), client:, server: kong, request: "GET /test-api/testcustomers/v1/customers HTTP/1.1", host: "" 2018/07/09 17:02:27 [error] 6295#0: *20927 lua coroutine: runtime error: .../local/share/lua/5.1/kong/plugins/aws-lambda/handler.lua:155: attempt to index local 'serialized_body' (a userdata value) stack traceback: coroutine 0: .../local/share/lua/5.1/kong/plugins/aws-lambda/handler.lua: in function <.../local/share/lua/5.1/kong/plugins/aws-lambda/handler.lua:34> coroutine 1: [C]: in function 'resume' coroutine.wrap:21: in function <coroutine.wrap:21>

@aloisbarreras aloisbarreras force-pushed the aloisbarreras:feat/lambda-custom-response branch 2 times, most recently from 8402af7 to 192f337 Jul 10, 2018

@aloisbarreras

This comment has been minimized.

Copy link
Contributor Author

aloisbarreras commented Jul 11, 2018

hey @thibaultcha this one missed the last release. I've rebased it, so I'd love some feedback and to get this merged in!

@Curistofa it should no longer throw any errors if Lambda doesn't return a body.

@aloisbarreras

This comment has been minimized.

Copy link
Contributor Author

aloisbarreras commented Jul 11, 2018

FYI: I'm designing this to be in line with the AWS API Gateway functionality. For a custom response to work with API Gateway, the requirements are that the function must return a JSON object that looks like:

  • statusCode (required)
  • The 'body' property must be a JSON string (optional)
  • The 'headers' property is for custom response headers in addition to standard ones (optional)
@aloisbarreras

This comment has been minimized.

Copy link
Contributor Author

aloisbarreras commented Jul 11, 2018

Not sure why the tests are failing. All the tests for the lambda plugin pass, but it looks like a test with the udp plugin failed as well as some cassandra ring health checks. Do those sometimes spontaneously fail @thibaultcha @bungle ?

@thibaultcha

This comment has been minimized.

Copy link
Member

thibaultcha commented Jul 11, 2018

@aloisbarreras

This comment has been minimized.

Copy link
Contributor Author

aloisbarreras commented Jul 13, 2018

@bungle any feedback on this?

@aloisbarreras

This comment has been minimized.

Copy link
Contributor Author

aloisbarreras commented Jul 23, 2018

any thoughts?

@robjr

This comment has been minimized.

Copy link

robjr commented Aug 14, 2018

Nice job! That's exactly what I was looking for.

@thibaultcha
Copy link
Member

thibaultcha left a comment

This seems to be quite the popular PR, let's give it some more attention! As such, here is my first round of review :)

Thank you @aloisbarreras for the patch!

-- this will return the custom response object, otherwise nil
local function extract_custom_response(content)
local serialized_content = cjson.decode(content)
if (not serialized_content) then return nil end

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Aug 14, 2018

Member

Will you please remove parenthesis from the conditions in this patch? The same way, please avoid 1 liner branches like this one, and prefer:

if thing then
  return
end

Both of these guidelines are detailed in our CONTRIBUTING.md file. Thanks.

local headers = serialized_content.headers or {}
local body
if (serialized_content.body ~= nil)
then

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Aug 14, 2018

Member

style: please keep the then on the previous line as well, since the line is short enough.

local function extract_custom_response(content)
local serialized_content = cjson.decode(content)
if (not serialized_content) then return nil end
local statusCode = tonumber(serialized_content.statusCode)

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Aug 14, 2018

Member

style: please prefer snake_case over camelCase for variable names - this rule is also detailed in our codestyle and enforced throughout the codebase. Of course, the serialized_content.statusCode one is fine.
Thanks!

}
end

return nil

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Aug 14, 2018

Member

We can remove this instruction which is redundant since the function will return nil anyway by default.

@@ -186,6 +215,12 @@ function AWSLambdaHandler:access(conf)
return responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
end

local customResponse = extract_custom_response(content)
if (customResponse) then
local mergedHeaders = utils.table_merge(headers, customResponse.headers)

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Aug 14, 2018

Member

style: ditto, please avoid camel case variable names here and above (L.218).

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Aug 14, 2018

Member

tests: in case of a duplicated header (found in both headers and customResponse.headers), headers from the latter will override headers from the former (due to the underlying implementation of utils.table_merge(). We should probably provide a test for this behavior.

local customResponse = extract_custom_response(content)
if (customResponse) then
local mergedHeaders = utils.table_merge(headers, customResponse.headers)
return send(customResponse.status_code, customResponse.body, mergedHeaders)

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Aug 14, 2018

Member

We cannot directly call send from this plugin (see #3079). Please follow the below pattern L.234 to 245, and set the proper ctx.delayed_response and ctx.delayed_response_callback. Ideally, we should not duplicate this code.

assert.res_status(201, res)
assert.equal(0, tonumber(res.headers["Content-Length"]))
assert.equal(nil, res.headers["X-Custom-Header"])
assert.equal(res:read_body(), "")

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Aug 14, 2018

Member

The order of arguments is wrong here, it should be: (expected, found). Also, the above assert.res_status() call already returns the body, this can be rewritten as:

local body = assert.res_status(201, res)
-- ...
assert.equal("", body)

Ditto in the previous test - thanks!

if (not serialized_content) then return nil end
local statusCode = tonumber(serialized_content.statusCode)

if (statusCode) then

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Aug 14, 2018

Member

tests: there seems to be no test that ensures that if a body is given, but no statusCode, then the Lambda's response has no effect on the proxy's response.

Just to make sure, is this the behavior followed by the AWS API Gateway? Great if it is! If not, then we should probably implement the same.

@thibaultcha

This comment has been minimized.

Copy link
Member

thibaultcha commented Aug 14, 2018

@aloisbarreras We would also ask of you (or anyone else willing to contribute them) that you provide documentation for this change, in the https://github.com/Kong/docs.konghq.com repository - thank you!

@aloisbarreras aloisbarreras force-pushed the aloisbarreras:feat/lambda-custom-response branch 3 times, most recently from cd580a5 to 6927e54 Aug 15, 2018

@aloisbarreras

This comment has been minimized.

Copy link
Contributor Author

aloisbarreras commented Aug 16, 2018

Okay @thibaultcha I've addressed your feedback, and I think everything looks good.

However, I was just reading more about how AWS API Gateway works with Lambda and I didn't realize that you have to pick Lambda Proxy Integration when setting up the proxy route. It does not automatically look for this custom response by default.

That begs the question, should I add an option to the plugin configuration that enables this? I'd love some feedback from you and other community members on what they would like to see.

If we do go with that option, then the required format of the Lambda response is described here.

There's an isBase64Encoded encoded option I'll add as well once we decide if we want to add an option to the plugin config.

@thibaultcha

This comment has been minimized.

Copy link
Member

thibaultcha commented Aug 16, 2018

@aloisbarreras Thanks for verifying. I do feel like this should be an option considering the opinionated behaviour of the plugin on such responses, but I never used it myself.

Yes, we should gather feedback from the rest of the community here. Considering the attention this PR has received so far, it shouldn't be too hard to gather a few opinions I hope :) Please chime in if you are waiting for this PR!

@aloisbarreras aloisbarreras force-pushed the aloisbarreras:feat/lambda-custom-response branch 2 times, most recently from 0828dd2 to 1e54261 Aug 16, 2018

@aloisbarreras

This comment has been minimized.

Copy link
Contributor Author

aloisbarreras commented Aug 16, 2018

Okay here is my updated proposal:

Add an is_proxy_integration option to the aws-lambda plugin. If that property is true, then the plugin will look for a specific structured response from the Lambda function.

I'm modeling the structure after the existing API Gateway functionality, which is this:

With the Lambda proxy integration, API Gateway requires the backend Lambda function to return output according to the following JSON format:

{
    "isBase64Encoded": true|false,
    "statusCode": httpStatusCode,
    "headers": { "headerName": "headerValue", ... },
    "body": "..."
}

In the output, headers can be unspecified if no extra response headers are to be returned. ... The output body is marshalled to the frontend as the method response payload. If body is a binary blob, you can encode it as a Base64-encoded string and set isBase64Encoded to true. Otherwise, you can set it to false or leave it unspecified.

If the function output is of a different format, API Gateway returns a 502 Bad Gateway error response.

The is_proxy_integration option defaults to false and won't affect any existing plugins.

My only hang up now is that I've never used the isBase64Encoded option with API Gateway, so I don't know what the use case for that is. I'd be find to leave it out if no one wants it.

Once we think this looks good, I'll update the docs for the plugin as well.

@aloisbarreras aloisbarreras force-pushed the aloisbarreras:feat/lambda-custom-response branch from 644903b to 7053351 Aug 16, 2018

@romdsj romdsj referenced this pull request Aug 29, 2018

Closed

docs(aws-lambda) Add is_proxy_integration (#3427) #836

2 of 4 tasks complete
@thibaultcha

This comment has been minimized.

Copy link
Member

thibaultcha commented Sep 12, 2018

@aloisbarreras Hey there, just a nudge: will you be able to address the review comments?

@aloisbarreras

This comment has been minimized.

Copy link
Contributor Author

aloisbarreras commented Sep 13, 2018

hey @thibaultcha . Yes! Sorry work has been crazy. I will finish this by EOW.

@aloisbarreras aloisbarreras force-pushed the aloisbarreras:feat/lambda-custom-response branch 3 times, most recently from 87c5d11 to fc15f58 Sep 14, 2018

@thibaultcha
Copy link
Member

thibaultcha left a comment

Thanks for the updates @aloisbarreras! I believe that we are almost there :)

On the base64 encoding point:

I've never used the isBase64Encoded option with API Gateway, so I don't know what the use case for that is.

I believe that is answered by the documentation you quoted:

If body is a binary blob, you can encode it as a Base64-encoded string and set isBase64Encoded to true

I do not believe implementing this option in this PR is a requirement, but I wonder how many of the folks who requested this PR would need it. And if so, we'd welcome a subsequent contribution once this gets merged.

Thank you!

Show resolved Hide resolved kong/plugins/aws-lambda/handler.lua Outdated
Show resolved Hide resolved kong/plugins/aws-lambda/handler.lua Outdated
Show resolved Hide resolved kong/plugins/aws-lambda/handler.lua
Show resolved Hide resolved spec/03-plugins/23-aws-lambda/01-access_spec.lua
Show resolved Hide resolved kong/plugins/aws-lambda/handler.lua Outdated

@aloisbarreras aloisbarreras force-pushed the aloisbarreras:feat/lambda-custom-response branch 3 times, most recently from 47dc67e to 5e8c9bc Sep 21, 2018

Show resolved Hide resolved kong/plugins/aws-lambda/handler.lua Outdated
@@ -59,6 +59,7 @@ local _M = {
HTTP_CONFLICT = 409,
HTTP_UNSUPPORTED_MEDIA_TYPE = 415,
HTTP_INTERNAL_SERVER_ERROR = 500,
HTTP_BAD_GATEWAY = 502,

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Sep 24, 2018

Member

This change warrants that we modify this logic, otherwise, we'd be sending underlying error messages to the client that really are internal error logs. (Talking about the above call in if not proxy_response then. Thoughts? Of course, such a change deserves the appropriate test cases to be added in responses_spec.lua (see

it("calls `ngx.log` if and only if a 500 status code was given", function()
).

This comment has been minimized.

Copy link
@aloisbarreras

aloisbarreras Sep 25, 2018

Author Contributor

Yeah that makes sense. I updated the logic and test, let me know if you have any other feedback.

@aloisbarreras aloisbarreras force-pushed the aloisbarreras:feat/lambda-custom-response branch 2 times, most recently from f81c25b to 0ee79c2 Sep 25, 2018

aloisbarreras added some commits Aug 16, 2018

feat(lambda) check lambda custom response and setting appropriate
ngx response values

A Lambda function can now return an object that sets a custom http
status, body, and headers on the ngx response to the client. Previously,
Kong would return a 200 or 202 no matter what the Lambda function
responded with. This new behavior brings Kong to parity with AWS API
Gateway and will make migrating from that platform much easier.

@aloisbarreras aloisbarreras force-pushed the aloisbarreras:feat/lambda-custom-response branch from 0ee79c2 to 5c64759 Sep 25, 2018

aloisbarreras added some commits Jul 11, 2018

tests(responses) add tests for new send_HTTP_BAD_GATEWAY method
add new response type and updating test to ensure that ngx.log is called to log the internal error

@aloisbarreras aloisbarreras force-pushed the aloisbarreras:feat/lambda-custom-response branch from 5c64759 to 2a53128 Sep 25, 2018

thibaultcha added a commit that referenced this pull request Sep 25, 2018

feat(response) add 502 Bad Gateway status code
Add a new response type and update the tests to ensure that ngx.log is
called to log the internal error.

From #3427

Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>

thibaultcha added a commit that referenced this pull request Sep 25, 2018

feat(aws-lambda) support for Lambda Proxy Integration
Summary
-------

Previously, Kong would return a 200 or 202 no matter what the Lambda
function responded with.

Now, a Lambda function can now return an object that sets a custom HTTP
status code, headers, and body. Kong will use these values to override
the response sent back to the client.

This new behavior brings Kong to parity with AWS API Gateway and will
make migrating from that platform much easier.

Read more on Lambda Proxy Integration here:

https://docs.aws.amazon.com/apigateway/latest/developerguide/getting-started-with-lambda-integration.html

The supported response format from the Lamda is:

    {
      "statusCode": 201,
      "headers": { "headerName": "headerValue", ... },
      "body": "..."
    }

Changes
-------

* Add `is_proxy_integration` option to schema (disabled by default).
  When enabled, the behavior described above will be in effect.
* Invalid response formats will result in a 502 response from Kong.
* Add test cases for returning a custom response from
  Lambda.

NYI
---

Not yet implemented:

* Support for `isBase64Encoded`

See
---

From #3427

Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>
@thibaultcha

This comment has been minimized.

Copy link
Member

thibaultcha commented Sep 25, 2018

Merged to master! Thank you for your hard work on this @aloisbarreras! 🎉 👍

I made a few tweaks locally, added a few test cases, extended the commit message, and did some cleanup/squashing. Kong 1.0 rc2 will ship with this new Lambda Proxy Integration (between now and the next couple of weeks). Thanks to everybody who helped and to those who expressed their interest in this as well (by emojis or even in person).

The docs PR opened by @romdsj can be seen here, and will be published when Kong 1.0 is released. We will be refining it, and until then, the PR can be used as documentation for this yet to be released feature :)

On the "NYI" (not yet implemented) list for this feature, I noted the lack of support for isBase64Encoded in the commit message. We would welcome a contribution for this feature as well!

Tieske added a commit to Tieske/kong-plugin-liamp that referenced this pull request Mar 18, 2019

feat(aws-lambda) support for Lambda Proxy Integration
Feature from Kong/kong#3427 but backported
to support Kong 0.13.1

NOTE: does not include test fixtures, see code comments

Tieske added a commit to Tieske/kong-plugin-liamp that referenced this pull request Mar 20, 2019

feat(aws-lambda) support for Lambda Proxy Integration
Feature from Kong/kong#3427 but backported
to support Kong 0.13.1

NOTE: does not include test fixtures, see code comments

Tieske added a commit to Tieske/kong-plugin-liamp that referenced this pull request Mar 20, 2019

feat(aws-lambda) support for Lambda Proxy Integration
Feature from Kong/kong#3427 but backported
to support Kong 0.13.1

NOTE: does not include test fixtures, see code comments

Tieske added a commit to Tieske/kong-plugin-liamp that referenced this pull request Mar 21, 2019

feat(aws-lambda) support for Lambda Proxy Integration
Feature from Kong/kong#3427 but backported
to support Kong 0.13.1

NOTE: does not include test fixtures, see code comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.