New feature: extra_headers argument for ngx.location.capture by vadim-pavlov · Pull Request #139 · openresty/lua-nginx-module

vadim-pavlov · 2012-07-12T22:59:08Z

Hello, this feature allows to set custom http headers for sub request using an 'extra_headers' argument.
I would appreciate it if you could run tests and merge this feature with your branch :)

…function.

agentzh · 2012-07-12T23:44:00Z

thank you for the patch! just some quick suggestions:

could you remove line-trailing spaces from the code?
could you leave 2 blank lines between C function definitions?
could you avoid the C++-style line comments?
could you add some test cases for this new feature?

Thanks!
-agentzh

bakins · 2012-07-13T00:39:10Z

+1 for the concept, FWIW.

…ions.

vadim-pavlov · 2012-07-13T04:59:57Z

Thanks for the prompt reply guys! I've not figured out yet how to run tests. I think I missing something in the test environment configuration.

Maybe you can help me to set it up, please look at the errors below:

$prove t/056-flush.t

t/056-flush.t .. 1/82 
#   Failed test 'TEST 6: http 1.0 (async) - pattern "lua buffering output bufs for the HTTP 1.0 request" matches a line in error.log'
#   at /usr/local/share/perl/5.14.2/Test/Nginx/Socket.pm line 744.

#   Failed test 'TEST 6: http 1.0 (async) - pattern "lua http 1.0 buffering makes ngx.flush() a no-op" matches a line in error.log'
#   at /usr/local/share/perl/5.14.2/Test/Nginx/Socket.pm line 744.

#   Failed test 'TEST 6: http 1.0 (async) - pattern "lua buffering output bufs for the HTTP 1.0 request" matches a line in error.log'
#   at /usr/local/share/perl/5.14.2/Test/Nginx/Socket.pm line 744.

vadim-pavlov · 2012-07-13T09:50:11Z

Building nginx with debug option, solved this issue.

vadim-pavlov · 2012-07-14T20:55:48Z

I think I've fulfilled all requests, please feel free to review and hopefully merge new feature with your codebase :)

agentzh · 2012-07-15T00:19:14Z

Hello!

On Sat, Jul 14, 2012 at 1:55 PM, vadim-pavlov
reply@reply.github.com
wrote:

I think I've fulfilled all requests, please feel free to review and hopefully merge new feature with your codebase :)

I'll try merging your patches in the next week :)

Thanks for your contribution!

Best regards,
-agentzh

…ould now be safe to use for sub-requests to application backend server. New tests added. (fixed vars: request_method, content_type, http_referer, http_host, http_user_agent, http_cookie)

vadim-pavlov · 2012-07-17T03:26:26Z

Hi! I encountered an issue with request_method variable in the sub-request. Nginx takes a value for it from r->main structure, which I believe not the best choice for us. So I added a fix for this problem in the separate function – ngx_http_lua_subrequest_fix_request_method_variable. This fix helps a lot when you make a request directly to application server like fastcgi, uwsgi, etc, as they hardly relies on this variable when forming a request. Same problem when we overwrite http headers using new 'extra_headers' argument -- corresponding variables in the sub-request should be also updated. In order to achieve this goal I implemented a mapping between these special headers and handlers which updates related fields of the request structure when you set a new header using an extra_header arguments. To cover new cases I've implemented about 10 extra tests. Your test are also works well with these fixes.

I hope I didn't add to much complexity, and merging would not take much time.

Thank you for making my life easier with killer features of ngx_lua :)

Regards,
Vadim

agentzh · 2012-07-18T04:26:31Z

One quick question: is it better to rename "extra_headers" to "headers"? do you really want to "add" headers to the parent request's headers?

vadim-pavlov · 2012-07-18T08:31:50Z

I think it would be reasonable to rename this argument to a short form 'headers' because it allows to overwrite headers inherited from the sub-request as well as set new headers. I can rename it if you wish. But we should keep the default behaviour of this function to inherit properties of the parent request and keep parent headers until these headers are explicitly overwritten in 'headers' argument.

agentzh · 2012-07-21T18:34:45Z

Hello!

Sorry for the delay! I've been extremely busy at $work over the last week :)

On Mon, Jul 16, 2012 at 8:26 PM, vadim-pavlov
reply@reply.github.com
wrote:

Hi! I encountered an issue with request_method variable in the sub-request. Nginx takes a value for it from r->main structure, which I believe not the best choice for us. So I added a fix for this problem in the separate function – ngx_http_lua_subrequest_fix_request_method_variable.

The standard $request_method variable always evaluates to the main
request method. I don't want to change its behavior by overriting its
value in ngx_lua because it is too evil. When the user uses some other
module to issue a subrequest, she'll be surprised.

I introduced the $echo_request_method in the ngx_echo module that
evaluates to the current (sub)request's method name, see

http://wiki.nginx.org/HttpEchoModule#.24echo_request_method

I think you should use this variable in the subrequest locations
instead of $request_method.

Please remove the ngx_http_lua_subrequest_fix_request_method_variable
method from the patch. It's just too hacky and make the situation even
worse IMHO :)

Thanks!
-agentzh

pbby · 2015-09-06T08:48:35Z

Hi, guys,
The feature is useful for me, but it seems like that have not be done?

agentzh · 2015-09-06T14:09:46Z

@SurFire91 Not yet though it's been on my TODO list.

BTW, there exists work-arounds:

use ngx.req.set_header and ngx.req.clear_header in the parent request's Lua code, and
use proxy_set_header and nginx variables as the header names or values in the subrequest location if ngx_proxy is used and pass values from the parent to the subrequest via nginx variables or something like that.

Because of the existence of these work-arounds, the priority of this PR has been relatively low. But still I'd like to see this to get reviewed and eventually merged.

pbby · 2015-09-07T03:58:29Z

@agentzh Thanks for reply.
I just use the ngx.req.set_header on my current work, but I think the TODO new feature is more flexible.
Excepting merged.

agentzh · 2015-09-08T07:48:13Z

@SurFire91 Yeah, I know :)

RoryCrispin · 2017-08-29T21:49:51Z

It'd be great to see this feature merged - the workaround seems clumsy especially for use cases such as making an upstream json POST:
copy the original content-type header into a temp variable
set the content type text/json header
make request
set the content type header back to it's original value

Thanks

agentzh · 2018-11-12T18:24:59Z

Yeah, I agree this is useful though we're really moving to cosockets these years. I don't mind merging a modified and rebased version of this PR if it does not slow down existing user code. @thibaultcha Will be interested in reviewing and possibly taking over this one?

Naidile-P-N · 2019-07-18T07:56:10Z

Hi, Is this feature available now ? Would be of great help

tanguilp · 2020-03-17T15:43:10Z

Would still be useful in 2020 :)

mergify · 2020-06-26T00:42:28Z