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

fix(aws-lambda) Fix typo in X-Amz-Function-Error header #2676

Merged
merged 1 commit into from
Jul 6, 2017

Conversation

erran
Copy link
Contributor

@erran erran commented Jul 6, 2017

HTTP/1.1 200 OK
Content-Type: application/json
Connection: keep-alive
Content-Length: 444
Connection: keep-alive
x-amzn-RequestId: ...
X-Amzn-Trace-Id: ...
Date: Thu, 06 Jul 2017 16:23:07 GMT
X-Amz-Function-Error: Handled
x-amzn-Remapped-Content-Length: 0
Server: kong/0.10.2

Today I noticed that I mistakenly added a n to the aws lambda function http header. ☹️

This fixes the aws-lambda plugin and specs to use the correct function header name as per the latest documentation at http://docs.aws.amazon.com/lambda/latest/dg/API_Invoke.html#API_Invoke_ResponseSyntax

Response Syntax

HTTP/1.1 StatusCode
X-Amz-Function-Error: FunctionError
X-Amz-Log-Result: LogResult

Payload

I'll open a separate PR to support updating the status code for Handled errors.

suggested reviewers: @thibaultcha and @p0pr0ck5 since they helped me land #2587.

Copy link
Member

@thibaultcha thibaultcha left a comment

Choose a reason for hiding this comment

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

Nice catch! Sorry that we let this slip through!

@@ -279,7 +279,7 @@ http {
local function say(res, status)
ngx.header["x-amzn-RequestId"] = "foo"
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to use the same prefix as well here? X-Amz-RequestId?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thibaultcha this is actually correct based on the response from Lambda (you can see the response headers in my original issue comment).

Copy link
Member

Choose a reason for hiding this comment

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

Interesting... They have different response headers prefixes then... 🤔 LGTM then!

@thibaultcha thibaultcha modified the milestones: 0.10.4, 0.11.0rc2 Jul 6, 2017
@thibaultcha 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/please review labels Jul 6, 2017
@erran
Copy link
Contributor Author

erran commented Jul 6, 2017

Nice catch! Sorry that we let this slip through!

@thibaultcha yea, was confused when I created a custom plugin for testing since we'll start depending on these changes out today (we were previously pushing back until 10.4 was released).

After these changes the response code is correctly updated for unhandled errors (I'll add handled error support in another PR).

Next time around we'll have the automation ready to test custom plugins on our kong installations vs. depending on unit tests etc. 👌

@thibaultcha thibaultcha merged commit 926bdc8 into Kong:master Jul 6, 2017
@erran erran deleted the fix-amz-function-error-header branch July 7, 2017 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants