[WIP] reverse proxy add headers support. #1126
Conversation
|
Thank you for the PR, sorry for the belated response. Regarding code-reuse, how about taking the following approach? The basic idea is not store common code in util.c, while retaining the distinction between the core library and the configurators.
|
…/configurator/proxy.c
|
Now related code is reused & proxy handler now support all header directives with prefix |
| /** | ||
| * lib/handler/configurator/util.c | ||
| */ | ||
| int h2o_on_config_header_2arg(h2o_configurator_command_t *cmd, h2o_configurator_context_t *ctx, int cmd_id, yoml_t *node, void *header_cmd_vector); |
There was a problem hiding this comment.
Please prefix the functions added to configurator.h with h2o_configurator_.
| /** | ||
| * lib/handler/configurator/util.c | ||
| */ | ||
| int h2o_on_config_header_2arg(h2o_configurator_command_t *cmd, h2o_configurator_context_t *ctx, int cmd_id, yoml_t *node, void *header_cmd_vector); |
There was a problem hiding this comment.
Please create a typedef for H2O_VECTOR(h2o_headers_command_t) (e.g. typedef H2O_VECTOR(h2o_headers_command_t) h2o_headers_commands_t) and use that type, instead of using void *header_cmd_vector.
| DEFINE_CMD("proxy.header.set", on_config_header_set); | ||
| DEFINE_CMD("proxy.header.setifempty", on_config_header_setifempty); | ||
| DEFINE_CMD("proxy.header.unset", on_config_header_unset); | ||
| #undef DEFINE_CMD |
There was a problem hiding this comment.
How about creating a function that adds various favors (e.g. add, append, ...) in lib/handler/configurator/util.c, instead of adding h2o_on_config_header_2arg and using macro to create variations in each source file.
E.g., How aboutcreating a function that looks something like h2o_configurator_define_headers_commands(h2o_configurator_t *conf, const char *prefix, h2o_headers_commands_t *(*get_commands)(h2o_configurator_t *)), and call it here like:
h2o_configurator_define_headers_comands(&c->super, "proxy.header", get_commands);The invocation would define all the proxy.header.* directives.
get_commands function could be defined as:
static h2o_headers_commands_t *get_commands(h2o_configurator_t *conf)
{
struct proxy_configurator_t *self = (void *)cmd->configurator;
return &self->vars->header_cmds;
}The merits of using this approach would be that the code will become less redundant, and that new directives that modifies the header modification commands can be added just be changing one place.
| return 0; | ||
| } | ||
|
|
||
| int add_cmd(h2o_configurator_command_t *cmd, yoml_t *node, int cmd_id, h2o_iovec_t *name, h2o_iovec_t value, void *header_cmd_vector) |
| h2o_headers_command_t *cmd; | ||
| for (cmd = self->config.header_cmds.entries; cmd->cmd != H2O_HEADERS_CMD_NULL; ++cmd) | ||
| h2o_rewrite_headers(&req->pool, &req->headers, cmd); | ||
| } |
There was a problem hiding this comment.
Could we just set the list of header modification commands to req->overrides and apply them only when the request is sent upstream in lib/core/proxy.c?
The reason I ask is because an upstream server (accessed using the modified headers) might return 399. In such case, the request would be delegated to the next handler. I think that the headers of the original request should be passed to the next handler, since per my understanding the intended behavior of proxy.header.* is to modify the headers passed to the upstream server only.
|
Sorry for not replying so long because there was so much to do at work. Looks like all necessary changes according to previous comments are made. |
|
Thank you for the changes. All of them look good to me. I'll merge this (might make tiny changes on my side) after we release 2.1. Thank you very much! |
Directives for tweaking headers sent to the application server
|
@zlm2012 Sorry it took so long. The branch has now been merged to master. Thank you for your work and for your patience. I would appreciate it if you could look into adding docs as well as adding an end-to-end test (e.g. create t/50reverse-proxy-tweak-headers.t), when you have time. |
This pull request is for supporting add custom constant headers when sending request to application server as a reverse proxy. Currently only proxy.header.add is supported. Since lib/headers.c is a filter, we cannot directly reuse the existed implementation. To show how I think this feature should be implemented, this pull request now has some copy & paste codes which should be eliminated. Waiting for advice.