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.

Sign up for GitHub

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

Jump to bottom

add default response format for when the 'Accept' header is missing #2500

Closed
wants to merge 5 commits into from
Closed

add default response format for when the 'Accept' header is missing #2500

wants to merge 5 commits into from

Conversation

therealgambo
Copy link

@therealgambo therealgambo commented May 5, 2017 •
edited

Summary

Implementation of #2332 providing a configurable default response type to be used when the Accept header is not present in the request and an unhandled error is being returned from Nginx / Kong.

Full changelog

  • Add error_default_type Kong configuration property.

Issues resolved

Fix #2332

@p0pr0ck5
Copy link
Contributor

p0pr0ck5 commented May 5, 2017

Hmm, this didn't trigger a CI build. @therealgambo is there anything odd about this PR or your fork that would stop Travis from executing?

p0pr0ck5
p0pr0ck5 previously requested changes May 5, 2017
View reviewed changes
Copy link
Contributor

@p0pr0ck5 p0pr0ck5 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! A few small notes. :)

kong/core/error_handlers.lua Outdated
template = text_template
content_type = TYPE_PLAIN
elseif find(accept_header, TYPE_HTML, nil, true) then
accept_header = singletons.configuration.response_format
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I missing something, or is this missing an end?

kong.conf.default Outdated
@@ -178,6 +178,9 @@
# See http://nginx.org/en/docs/http/ngx_http_realip_module.html#real_ip_recursive
# for a description of this directive.

#response_format = text/plain # The default response format to use when the
# 'Accept' header is missing in the request
Copy link
Contributor

Choose a reason for hiding this comment

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

would be nice here to have a note of the valid elements based on the defined enum. Also, this needs clarification on exactly when this is used (e.g. for Nginx error handling, and not always errors generated by Kong, by plugins, or by upstream services).

Copy link
Member

Choose a reason for hiding this comment

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

Also, a better name for this property would be something closer to the Nginx default_type directive, and carrying more context, like error_default_type maybe.

@p0pr0ck5 p0pr0ck5 added the pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. label May 5, 2017
@therealgambo
Copy link
Author

therealgambo commented May 7, 2017 •
edited

Updated with changes.

@p0pr0ck5 , this covers any Nginx error response / Kong error that makes its way into the error_handlers.lua. The specific case I was encountering was when a configured upstream is no longer accessible, upstream server is essentially offline.

  1. Make a dummy upstream that will never respond to simulate an offline server.
    curl -X POST http://localhost:8001/apis -d 'name=example' -d 'hosts=example.com' -d 'upstream_url=http://127.0.0.1:3333'

  2. Make a request - response will be in the format of that specified in the kong config.
    curl -v -H 'Accept:' -H 'Host: example.com' http://127.0.0.1:8000/

As for the CI not being triggered, I'm unsure why its not doing its magic.

@p0pr0ck5 p0pr0ck5 added pr/status/please review and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels May 8, 2017
@p0pr0ck5 p0pr0ck5 dismissed their stale review May 8, 2017 15:52

lgtm, still needs to have travis run. not sure why the PR portion isn't executing.

thibaultcha
thibaultcha requested changes May 12, 2017
View reviewed changes
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.

This will now need some tests :) Thanks!

@thibaultcha thibaultcha added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/status/please review labels May 12, 2017
@therealgambo
Copy link
Author

@thibaultcha tests added. Please let me know if I have missed something in them or if you would like any additional checks.

@p0pr0ck5
Copy link
Contributor

This seems okay to me, ran the full test suite against it without problems. Should we target master instead of next as this isn't a breaking change?

@therealgambo
Copy link
Author

I'm fine with it going into either.

thibaultcha
thibaultcha reviewed May 30, 2017
View reviewed changes
kong.conf.default Outdated
@@ -178,6 +178,10 @@
# See http://nginx.org/en/docs/http/ngx_http_realip_module.html#real_ip_recursive
# for a description of this directive.

#error_default_type = text/plain # Default response format to use when the
Copy link
Member

Choose a reason for hiding this comment

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

"Default MIME type" instead maybe?

thibaultcha
thibaultcha reviewed May 30, 2017
View reviewed changes
kong.conf.default Outdated
@@ -178,6 +178,10 @@
# See http://nginx.org/en/docs/http/ngx_http_realip_module.html#real_ip_recursive
# for a description of this directive.

#error_default_type = text/plain # Default response format to use when the
# 'Accept' header is missing and Nginx
# is returning an error for the request.
Copy link
Member

Choose a reason for hiding this comment

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

This should also document which values are accepted if they are validated via an enum in the config loader.

@therealgambo
Copy link
Author

@thibaultcha updated. lgtm

@thibaultcha thibaultcha added pr/status/do not merge (to discuss) and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels May 31, 2017
thibaultcha pushed a commit that referenced this pull request May 31, 2017
@thibaultcha
feat(conf) new error_default_type setting
b737179 
Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>

When the request has no `Accept` header and Nginx returns an error, this
setting defines the MIME type of the response sent back to the client.

From #2500
Implements #2332
@thibaultcha
Copy link
Member

Merged to master in b737179 with the appropriate changes. Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thibaultcha thibaultcha thibaultcha requested changes

@p0pr0ck5 p0pr0ck5 p0pr0ck5 left review comments

Assignees

@p0pr0ck5 p0pr0ck5

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@therealgambo @p0pr0ck5 @thibaultcha