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

Internal redirect #197

Merged
merged 37 commits into from Mar 1, 2015

Conversation

Projects
None yet
2 participants
@kazuho
Member

kazuho commented Feb 27, 2015

This PR modifies the request handling procedure of H2O to support internal redirects, specifically by:

  • create h2o_req_t::input for preserving the original request
  • create function h2o_reprocess_request that internally re-issues a request with request attributes (e.g. method, authority, path) being altered
  • move the proxy implementation into core, as it would be the default handler for handling internally-redirected responses
  • the reverse proxy handler merely redirects the request (so that it would be handled by the now internalized proxy implementation)

The beauty of the new approach is that the proxy implementation can be shared with the reverse-proxy and the x-reproxy-url handler (see also #187), and that it is capable of handling x-reproxy-url header that points to a local resource (i.e. resources served by the file handler).

@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho Feb 27, 2015

Member

still wip; ToDos include:

  • pass-around proxy.timeout.io (done in 7b28cca)
    • proxy.timeout.keepalive is already recoginized as part of the socket pool
  • check and adjust how link is handled
    • note: we now permit handlers to alter the authority
  • handle redirects internally when necessary
    • once the internal redirect occurs as a result of x-reproxy-url header, then redirect responses from upstream should be resolved locally
  • implement handler for x-reproxy-url (done in 73ecd38; still need tests)
    • we need this for adjusting the above
  • asynchronous name resolution and global (i.e. not hostport-specific) socket pooling
    • mandatory for high-performance x-reproxy-url support
Member

kazuho commented Feb 27, 2015

still wip; ToDos include:

  • pass-around proxy.timeout.io (done in 7b28cca)
    • proxy.timeout.keepalive is already recoginized as part of the socket pool
  • check and adjust how link is handled
    • note: we now permit handlers to alter the authority
  • handle redirects internally when necessary
    • once the internal redirect occurs as a result of x-reproxy-url header, then redirect responses from upstream should be resolved locally
  • implement handler for x-reproxy-url (done in 73ecd38; still need tests)
    • we need this for adjusting the above
  • asynchronous name resolution and global (i.e. not hostport-specific) socket pooling
    • mandatory for high-performance x-reproxy-url support
@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho Feb 27, 2015

Member

Thanks to @lestrrat's contribution x-reproxy-url now works!

Member

kazuho commented Feb 27, 2015

Thanks to @lestrrat's contribution x-reproxy-url now works!

kazuho added some commits Feb 27, 2015

refactor the response status/header smuggling code to middleware so t…
…hat it can be combined with various handlers
@lestrrat

This comment has been minimized.

Show comment
Hide comment
@lestrrat

lestrrat Feb 27, 2015

Contributor

@kazuho wrt test failure

diff --git a/t/assets/upstream.psgi b/t/assets/upstream.psgi
index ca8a40b..273b3bd 100644
--- a/t/assets/upstream.psgi
+++ b/t/assets/upstream.psgi
@@ -15,17 +15,20 @@ builder {
             my $env = shift;
             my $query = Plack::Request->new($env)->query_parameters;
             my $res = $app->($env);
-            if ($query->{"resp:status"}) {
-                $res->[0] = $query->get("resp:status");
-                $query->remove("resp:status");
-            }
-            push @{$res->[1]}, map {
-                my $n = $_;
-                +(substr($n, length "resp:") => $query->get($n))
-            } grep {
-                $_ =~ /^resp:/
-            } $query->keys;
-            $res;
+            Plack::Util::response_cb($res, sub {
+                my $res = shift;
+                if ($query->{"resp:status"}) {
+                    $res->[0] = $query->get("resp:status");
+                    $query->remove("resp:status");
+                }
+                push @{$res->[1]}, map {
+                    my $n = $_;
+                    +(substr($n, length "resp:") => $query->get($n))
+                } grep {
+                    $_ =~ /^resp:/
+                } $query->keys;
+                $res;
+            });
         };
     };
     if ($force_chunked) {
Contributor

lestrrat commented Feb 27, 2015

@kazuho wrt test failure

diff --git a/t/assets/upstream.psgi b/t/assets/upstream.psgi
index ca8a40b..273b3bd 100644
--- a/t/assets/upstream.psgi
+++ b/t/assets/upstream.psgi
@@ -15,17 +15,20 @@ builder {
             my $env = shift;
             my $query = Plack::Request->new($env)->query_parameters;
             my $res = $app->($env);
-            if ($query->{"resp:status"}) {
-                $res->[0] = $query->get("resp:status");
-                $query->remove("resp:status");
-            }
-            push @{$res->[1]}, map {
-                my $n = $_;
-                +(substr($n, length "resp:") => $query->get($n))
-            } grep {
-                $_ =~ /^resp:/
-            } $query->keys;
-            $res;
+            Plack::Util::response_cb($res, sub {
+                my $res = shift;
+                if ($query->{"resp:status"}) {
+                    $res->[0] = $query->get("resp:status");
+                    $query->remove("resp:status");
+                }
+                push @{$res->[1]}, map {
+                    my $n = $_;
+                    +(substr($n, length "resp:") => $query->get($n))
+                } grep {
+                    $_ =~ /^resp:/
+                } $query->keys;
+                $res;
+            });
         };
     };
     if ($force_chunked) {
@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho Feb 27, 2015

Member

@lestrrat Ah! thanks a lot!

Member

kazuho commented Feb 27, 2015

@lestrrat Ah! thanks a lot!

kazuho added some commits Feb 27, 2015

call `proxy_send_prepare` _after_ all the validations are done (or `s…
…truct rp_generator_t::client` may be left uninitialized)
update `path_normalized` and `query_at` after internal redirection (f…
…ixes SEGV caused by the tests added in prev. commit)
handle 30x redirects internally when `h2o_req_t::res_is_delegated` is…
… set (e.g. in case of a reproxied response)
@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho Mar 1, 2015

Member

seems like the work has been done with the exception of:

  • asynchronous name resolution and global (i.e. not hostport-specific) socket pooling
    • mandatory for high-performance x-reproxy-url support
Member

kazuho commented Mar 1, 2015

seems like the work has been done with the exception of:

  • asynchronous name resolution and global (i.e. not hostport-specific) socket pooling
    • mandatory for high-performance x-reproxy-url support

kazuho added a commit that referenced this pull request Mar 1, 2015

Merge pull request #197 from h2o/kazuho/internal-redirect
implements internal redirect (used by proxy and reproxy)

@kazuho kazuho merged commit 05d3d84 into master Mar 1, 2015

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho Mar 1, 2015

Member

The leftovers are covered by the PRs above.

Member

kazuho commented Mar 1, 2015

The leftovers are covered by the PRs above.

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