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

[WIP] reverse proxy add headers support. #1126

Merged
merged 8 commits into from Jan 19, 2017

Conversation

Projects
None yet
2 participants
@zlm2012
Contributor

zlm2012 commented Nov 23, 2016

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.

@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho Nov 27, 2016

Member

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.

  • move common code not related to configurator (e.g. rewrite_headers) to lib/handler/util.c
  • create lib/handler/configurator/util.c, and move the common code between multiple configurators to the file
  • functions / structures that become exposed in the headers file should be prefixed with h2o_ (as usual)
Member

kazuho commented Nov 27, 2016

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.

  • move common code not related to configurator (e.g. rewrite_headers) to lib/handler/util.c
  • create lib/handler/configurator/util.c, and move the common code between multiple configurators to the file
  • functions / structures that become exposed in the headers file should be prefixed with h2o_ (as usual)
@zlm2012

This comment has been minimized.

Show comment
Hide comment
@zlm2012

zlm2012 Nov 29, 2016

Contributor

Now related code is reused & proxy handler now support all header directives with prefix proxy. (eg. proxy.header.add)

Contributor

zlm2012 commented Nov 29, 2016

Now related code is reused & proxy handler now support all header directives with prefix proxy. (eg. proxy.header.add)

@kazuho

Thank you for the updates.

I've left my ideas. Please let me know what you think (or make changes if my comments make sense to you).

Thank you in advance.

Show outdated Hide outdated include/h2o/configurator.h
/**
* 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);

This comment has been minimized.

@kazuho

kazuho Nov 29, 2016

Member

Please prefix the functions added to configurator.h with h2o_configurator_.

@kazuho

kazuho Nov 29, 2016

Member

Please prefix the functions added to configurator.h with h2o_configurator_.

Show outdated Hide outdated include/h2o/configurator.h
/**
* 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);

This comment has been minimized.

@kazuho

kazuho Nov 29, 2016

Member

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.

@kazuho

kazuho Nov 29, 2016

Member

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.

Show outdated Hide outdated lib/handler/configurator/proxy.c
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

This comment has been minimized.

@kazuho

kazuho Nov 29, 2016

Member

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.

@kazuho

kazuho Nov 29, 2016

Member

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.

Show outdated Hide outdated lib/handler/configurator/util.c
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)

This comment has been minimized.

@kazuho

kazuho Nov 29, 2016

Member

Maybe static is missing here?

@kazuho

kazuho Nov 29, 2016

Member

Maybe static is missing here?

Show outdated Hide outdated lib/handler/proxy.c
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);
}

This comment has been minimized.

@kazuho

kazuho Nov 29, 2016

Member

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.

@kazuho

kazuho Nov 29, 2016

Member

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.

@zlm2012

This comment has been minimized.

Show comment
Hide comment
@zlm2012

zlm2012 Dec 3, 2016

Contributor

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.

Contributor

zlm2012 commented Dec 3, 2016

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.

@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho Dec 6, 2016

Member

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!

Member

kazuho commented Dec 6, 2016

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!

@kazuho kazuho merged commit 3eef73c into h2o:master Jan 19, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

kazuho added a commit that referenced this pull request Jan 19, 2017

Merge pull request #1126
Directives for tweaking headers sent to the application server
@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho Jan 19, 2017

Member

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

Member

kazuho commented Jan 19, 2017

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

@kazuho kazuho added this to the v2.2 milestone Jan 19, 2017

kazuho added a commit that referenced this pull request Feb 24, 2017

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