New feature: extra_headers argument for ngx.location.capture

[vadim-pavlov]

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 :)

[agentzh]

thank you for the patch! just some quick suggestions:

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

Thanks! -agentzh

[bakins]

+1 for the concept, FWIW.

[vadim-pavlov]

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]

Building nginx with debug option, solved this issue.

[vadim-pavlov]

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

[agentzh]

Hello!

On Sat, Jul 14, 2012 at 1:55 PM, vadim-pavlov [email protected] 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

[vadim-pavlov]

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]

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]

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]

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 [email protected] 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]

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

[agentzh]

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

BTW, there exists work-arounds:

1. use ngx.req.set_header and ngx.req.clear_header in the parent request's Lua code, and
2. 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]

@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]

@surfire91 Yeah, I know :)

[RoryCrispin]

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

[sixther-dc]

So, this feature is not merged yet? I meet the same question.

[agentzh]

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]

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

[tanguilp]

Would still be useful in 2020 :)

[mergify[bot]]

This pull request is now in conflict :(