feat(aws-lambda) Add support for setting a custom response code #2587

erran · 2017-05-31T22:53:05Z

This is a feature request for the Kong aws-lambda plugin. The use case is for allowing users to set a custom response code for APIs they've configured. AWS Lambda's Invoke call returns a 20x code regardless of whether an error occurred. This supports configuring a different response code via the API.

Summary

Adds a new unhandled_status configuration option for the aws-lambda
plugin. If set - when the X-Amzn-Function-Error invoke call response
header is set to Unhandled then the numeric value will be used as the
response code for the API.

Full changelog

Add support to override response code for unhandled exceptions via unhandled_status.
Updated spec for usage without unhandled_status.
Added specs for unhandled_status usage.

Looks like there will be conflicts with #2470 depending on which is merged first. 😅

thibaultcha · 2017-05-31T22:56:49Z

Hi!

Thanks for the patch, this definitely looks like a good addition :) Do you have any resource on the possible values of this X-Amzn-Function-Error header? Just curious about what would be the other possible corner cases where this sort of customization might lead us.

erran · 2017-05-31T22:59:50Z

Do you have any resource on the possible values of this X-Amzn-Function-Error header?

@thibaultcha As per the docs on the AWS Lambda Invoke call:

FunctionError
Indicates whether an error occurred while executing the Lambda function. If an error occurred this field will have one of two values; Handled or Unhandled. Handled errors are errors that are reported by the function while the Unhandled errors are those detected and reported by AWS Lambda. Unhandled errors include out of memory errors and function timeouts. For information about how to report an Handled error, see Programming Model.

erran · 2017-05-31T23:01:31Z

kong/plugins/aws-lambda/handler.lua

@@ -83,7 +83,11 @@ function AWSLambdaHandler:access(conf)
    return responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
  end

-  ngx.status = res.status
+  if headers['X-Amzn-Function-Error'] == 'Unhandled' and conf.unhandled_response ~= -1 then


Can someone confirm the upgrade path this plugin would take? I'm imagining it'd get the default value -1 but I couldn't confirm before submitting this PR.

I was about to mention this as well. When adding a new property in the plugin configuration, one needs to do one of those two things:

create a migration for both C* and Postgres that adds the property (alongside its default) in all the existing rows containing this plugin configuration

handle the possibility that said configuration property is nil in the Lua logic

We historically used to prefer the former approach, to stay consistent with previous cases where a property was added to an existing plugin, and to follow the philosophy that the configuration schema is the source of truth, and a required field is guaranteed to be present. Lately however, I've been more and more of a fan of the second approach, since we avoid adding migrations to the projects, and it allows us to ship new features into our minor releases. If you retrieve the default from the schema when conf.unhandled_response is nil, or if you handle it in a non-breaking fashion I think you should be just fine :)

thibaultcha

Some minor comments and concerns :)

thibaultcha · 2017-05-31T22:59:16Z

kong/plugins/aws-lambda/schema.lua

@@ -14,5 +14,6 @@ return {
    log_type = {type = "string", required = true, default = "Tail", 
                       enum = {"Tail", "None"}},
    port = { type = "number", default = 443 },
+    unhandled_response = {type = "number", default = -1, required = true},


I'm not sure -1 is a healthy default... There might be plenty of HTTP clients being confused by the negative number here (including Nginx itself). Would something in the 5xx range be a healthier default? This is partly why I was wondering about this X-Amzn-Function-Error header and its possible values. Not so obvious to find doc about it at first sight.

As a second note on that line, wouldn't unhandled_status be a more appropriate name?

Would something in the 5xx range be a healthier default?

I was using -1 as the value so I could short circuit if the flag was unset. What happens to numeric values when unset?

As a second note on that line, wouldn't unhandled_status be a more appropriate name?

Yep, I'll change it and was conflicted on what to use as the name myself. 👌

What happens to numeric values when unset?

They would just be nil. A validation function that ensure a valid HTTP status > 99 && <= 999 would also be nice. As we know, HTTP clients don't have a good time when the status is not included in that range.

I like the current approach of only setting ngx.status when unhandled_status is not nil.

thibaultcha · 2017-05-31T23:01:08Z

kong/plugins/aws-lambda/handler.lua

@@ -83,7 +83,11 @@ function AWSLambdaHandler:access(conf)
    return responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
  end

-  ngx.status = res.status
+  if headers['X-Amzn-Function-Error'] == 'Unhandled' and conf.unhandled_response ~= -1 then


style: we use double quotes in this codebase, and try to avoid having lines over 80 characters long. See our style guide: https://github.com/Mashape/kong/blob/master/CONTRIBUTING.md#code-style

thibaultcha · 2017-05-31T23:01:26Z

kong/plugins/aws-lambda/handler.lua

-  ngx.status = res.status
+  if headers['X-Amzn-Function-Error'] == 'Unhandled' and conf.unhandled_response ~= -1 then
+    ngx.status = conf.unhandled_response
+  else


style: we need a line jump before this else

thibaultcha · 2017-05-31T23:02:32Z

spec/03-plugins/23-aws-lambda/01-access_spec.lua

@@ -109,6 +185,7 @@ describe("Plugin: AWS Lambda (access)", function()
    local body = assert.res_status(200, res)
    assert.is_string(res.headers["x-amzn-RequestId"])
    assert.equal([["some_value1"]], body)
+    assert.equal(nil, res.headers["X-Amzn-Function-Error"])


Prefer assert.is_nil :)

erran · 2017-05-31T23:31:06Z

handle the possibility that said configuration property is nil in the Lua logic

I'm not sure -1 is a healthy default... There might be plenty of HTTP clients being confused by the negative number here (including Nginx itself).

Would something in the 5xx range be a healthier default?

Yea, -1 didn't play nice with nginx in my testing but it was fine as a shortcut only value. I like using nil since I we can just check that the value is truthy.

A 5xx code would be a change in behaviour since today hitting a Kong API which forwards to a lambda that encountered an Unhandled error gives a 20x status code.

Looks like I collapsed #2587 (review) already after pushing one of those style fixed but it also included the change to support unhandled_status being nil which should maintain backwards with the plugin as per the above comments.

Going to catch some 💤s — I'll be back online around 10am Dublin time. I'll squash and rebase the commits in this PR against master in the morning.

erran · 2017-06-01T10:10:03Z

~~Had one spec error in TravisCI Build #7298.5:~~

TravisCI Build failure

[ RUN      ] spec/02-integration/02-dao/08-ipc_events_spec.lua @ 42: Quiet with #cassandra insert() propagates event
./kong/dao/schemas/apis.lua:209: attempt to index local 't' (a nil value)
stack traceback:
	./kong/dao/schemas/apis.lua:209: in function 'marshall_event'
	./kong/dao/dao.lua:68: in function 'event'
	./kong/dao/dao.lua:134: in function 'insert'
	spec/02-integration/02-dao/08-ipc_events_spec.lua:26: in function 'do_insert'
	spec/02-integration/02-dao/08-ipc_events_spec.lua:53: in function 
[  ERROR   ] spec/02-integration/02-dao/08-ipc_events_spec.lua @ 42: Quiet with #cassandra insert() propagates event (212.09 ms)

~~Looks like it may be due to the the new value in the schema?~~

Or could be an intermittent failure unrelated to my changes. 😓 I couldn't reproduce the above failure in my kong-vagrant instance running the integration make target. After making additional changes and rebasing/pushing we're green again though the changes weren't functional. 😕

p0pr0ck5 · 2017-06-01T15:16:07Z

@erran yep, I retriggered the Travis build and it did indeed pass :)

p0pr0ck5

Picking up for @thibaultcha while he travels for LuaConf. @erran thanks for the PR! A few minor comments attached.

p0pr0ck5 · 2017-06-01T15:17:20Z

kong/plugins/aws-lambda/schema.lua

@@ -14,5 +14,6 @@ return {
    log_type = {type = "string", required = true, default = "Tail", 
                       enum = {"Tail", "None"}},
    port = { type = "number", default = 443 },
+    unhandled_status = {type = "number"},


pedantic style: can we add spaces around the table elements, e.g.

{ type = "number" }

Also, a validation function to check for status > 99 and status <= 999 would be great here :)

👍 Awesome, sad I removed the validation in one of my earlier commits now.

I'm happy to add spaces. Just getting the hang of this lua thing. 😉

Fixed both of the above in 61dc1db.

erran · 2017-06-01T16:01:57Z

kong/plugins/aws-lambda/schema.lua

@@ -1,3 +1,7 @@
+local check_status = function(status)
+  return status > 99 and status <= 999


This caused a failure due to not nil checking. Fixed locally and will push once specs have run.

@p0pr0ck5 I've pushed up the fix in 731519e and the plugin specs are passing. Looks like one of the restart Kong integration specs failed: https://travis-ci.org/Mashape/kong/jobs/238408326#L551-560

thibaultcha · 2017-06-01T17:07:29Z

kong/plugins/aws-lambda/schema.lua

@@ -1,3 +1,14 @@
+local function check_status(status)
+  if not status then


This should be unnecessary. If there is no status, the func check will simply not run.

I believe I got a null pointer when I didn't check this. I'll confirm when I'm back at the keyboard.

Hmmm possible - it's been too long since I last touched this. If so, then I think the logic can be shortened and we can only use 1 branch, that returns false if status and status isn't in range, and true otherwise.

Yep, see TravisCI Build #7300.4 from before adding the null check.

[----------] Running tests from spec/03-plugins/23-aws-lambda/01-access_spec.lua ./kong/plugins/aws-lambda/schema.lua:2: attempt to compare number with nil stack traceback: ./kong/plugins/aws-lambda/schema.lua:2: in function 'func' ./kong/dao/schemas_validation.lua:215: in function 'validate_entity' ./kong/dao/schemas_validation.lua:186: in function 'validate' ./kong/dao/model_factory.lua:27: in function 'validate' ./kong/dao/dao.lua:118: in function 'insert' spec/03-plugins/23-aws-lambda/01-access_spec.lua:55: in function <spec/03-plugins/23-aws-lambda/01-access_spec.lua:6> [----------] 0 tests from spec/03-plugins/23-aws-lambda/01-access_spec.lua (736.92 ms total)

Pushed an update in 292d9a2404a.

thibaultcha · 2017-06-01T17:08:46Z

kong/plugins/aws-lambda/schema.lua

@@ -1,3 +1,14 @@
+local function check_status(status)


We also need a test for this validation function similar to the schema_spec tests you can found in other parts of the unit test suite.

@thibaultcha lazy me, was actually thinking about it and found the hmac-auth schema validation spec earlier. 😅 Added in 292d9a2404a1a0e40104c6346f14b11cd8b73013.

thibaultcha · 2017-06-01T17:09:39Z

Picking up for @thibaultcha while he travels for LuaConf.

Thanks @p0pr0ck5!

erran · 2017-06-01T20:45:37Z

Thanks for the reviews @thibaultcha and @p0pr0ck5. Rebased against master and pushed up including all the above feedback. 👌

thibaultcha · 2017-06-01T20:48:30Z

Thanks for taking care of the reviews! This looks good now :) Currently on mobile so I'll let @p0pr0ck5 merge this one.

p0pr0ck5 · 2017-06-01T20:50:54Z

spec/03-plugins/23-aws-lambda/02-schema_spec.lua

+    port  = 443,
+  }
+
+  it("accepts correct Unhandled Response Status Code", function()


for completeness, can we add a test for when unhandled_status is undefined?

Added in 2f888f9. Should I squash my commits again?

p0pr0ck5

LGMT! Thanks for the contribution!

p0pr0ck5 · 2017-06-01T21:49:17Z

@erran can you squash these commits, and then we can merge this in?

Adds a new `unhandled_status` configuration option for the aws-lambda plugin. If set the response status will be overridden if the `X-Amzn-Function-Error` lambda invoke respoonse header was set to `Unhandled`. This gives a new feature to users of the plugin allowing their API to return non 20x codes. The previous functionality of responding with Amazon's Lambda Invoke unhandled response status of 200 (RequestResponse), 202 (Event), or 204 (DryRun) remains.

erran · 2017-06-01T21:55:37Z

@p0pr0ck5 @thibaultcha thanks for the quick feedback and help getting my changes up to shape. 😄

Squashed in 52779f2.

p0pr0ck5 · 2017-06-02T07:10:33Z

Merged! Thanks again @erran for the contribution!

erran · 2017-06-02T10:12:15Z

@p0pr0ck5 No problem, thanks for helping me get this in! 🎉

		@@ -1,3 +1,7 @@
		local check_status = function(status)
		return status > 99 and status <= 999

		@@ -1,3 +1,14 @@
		local function check_status(status)
		if not status then

feat(aws-lambda) Add support for setting a custom response code #2587

feat(aws-lambda) Add support for setting a custom response code #2587

Conversation

erran commented May 31, 2017 • edited Loading

Summary

Full changelog

thibaultcha commented May 31, 2017

erran commented May 31, 2017 • edited Loading

Choose a reason for hiding this comment

thibaultcha May 31, 2017 • edited Loading

Choose a reason for hiding this comment

thibaultcha 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

thibaultcha May 31, 2017 • edited Loading

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

erran commented May 31, 2017 • edited Loading

erran commented Jun 1, 2017 • edited Loading

p0pr0ck5 commented Jun 1, 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

erran Jun 1, 2017 • edited Loading

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

thibaultcha commented Jun 1, 2017

erran commented Jun 1, 2017

thibaultcha commented Jun 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0pr0ck5 left a comment

Choose a reason for hiding this comment

p0pr0ck5 commented Jun 1, 2017

erran commented Jun 1, 2017

p0pr0ck5 commented Jun 2, 2017

erran commented Jun 2, 2017

erran commented May 31, 2017 •

edited

Loading

erran commented May 31, 2017 •

edited

Loading

thibaultcha May 31, 2017 •

edited

Loading

thibaultcha May 31, 2017 •

edited

Loading

erran commented May 31, 2017 •

edited

Loading

erran commented Jun 1, 2017 •

edited

Loading

erran Jun 1, 2017 •

edited

Loading