Skip to content

feature: added the preserve_method_on_error_page patch. #396

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

Conversation

thibaultcha
Copy link
Member

@thibaultcha thibaultcha commented Aug 7, 2018

I hereby granted the copyright of the changes in this pull request
to the authors of this openresty project.

Wondering if this is something that we may want; please advise.

In cases where we want to access to a yield-able Lua context upon Nginx-produced or upstream errors, the error_page directive is our de-facto choice. However, when logging such errors (along with the request that triggered it) to an external logging stack, the request's HTTP method is overridden by Nginx to GET, causing us to lose some context about the original request.
My guess is that the override makes sense by default since most Nginx instances would then serve their HTML error page upon this internal redirect. I tracked down the behaviour from a release dating back to 2005:

nginx/nginx@ceb9929#diff-05d6b8ae21d192f78b349319edaeb9bfR295

However, there seems to be no particular reason why this would have to be the case when one produces their own content in the error_page location, e.g. via content_by_lua_*. Hence, a directive with backwards-compatibility defaults seems to make sense.

@jeremyjpj0916
Copy link

+1 , any updates around this? We still get week to week confusion from users seeing HTTP GET method in the logs with the reality was the client sent a POST/DELETE/PUT etc. and a L4 connection failure occurred so it was mis-logged as a GET.

@thibaultcha
Copy link
Member Author

Closing this as we will be addressing the issue differently in Kong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants