[PATCH 12 of 14] Proxy: add HTTP/2 support

[Maxim Dounin]

Hello!

On Thu, Jun 22, 2017 at 01:33:16PM -0700, Piotr Sikora via nginx-devel wrote:

> # HG changeset patch
> # User Piotr Sikora <piotrsikora at google.com>
> # Date 1490769087 25200
> #      Tue Mar 28 23:31:27 2017 -0700
> # Node ID 7eb807b056da7abe9c679b59e94595d59a1406e6
> # Parent  0637acdb51e29e1f68f5f3e762115c702cab4e72
> Proxy: add HTTP/2 support.
> 
> Signed-off-by: Piotr Sikora <piotrsikora at google.com>

[...]

There are serious concerns about this and with other patches.  
Most generic ones are outlined below.

1. Server-side HTTP/2 code complexity concerns.

As per discussion with Valentin, introducing client-related code 
paths in the server HTTP/2 code seems to complicate things.  Given 
that the complexity of the code is already high, it might be 
better idea to implement separate client-side processing instead.

2. Different protocols in proxy module.

You've introduced a completely different protocol in the proxy 
module, which contradicts the established practice of using 
separate modules for different protocols.

Reasons for having all (or at least may of) the protocols 
available in the proxy module instead are well understood, and 
there is a long-term plan to revise current practice.  The plan is 
to preserve protocol-specific modules as a separate entities, but 
let them share the common configuration directives, similar to how 
it is currently done with upstream{} blocks and the overall 
upstream infrastructure.  So one can write something like

    proxy_pass fastcgi://127.0.0.1:9000;

and get an expected result.

While benefits of having all protocols sharing the same 
user-visible interface are understood, approach taken with HTTP/2 
implementation is considered suboptimal, and will likely lead to 
something hard to maintain.

I see two possible solutions here:

- make HTTP/2-to-upstreams a separate full-featured upstream-based 
  module, similar to proxy or fastcgi;

- start working on the different protocols in the proxy module, 
  and introduce HTTP/2 proxying after this work is done.

Additionally, there are also some minor / related comments:

- Parts of the code, tightly coupled with upstream infrastructure, 
  notably ngx_http_v2_upstream_output_filter(), are placed into 
  v2/ directory.  Instead, they seem to belong to the 
  HTTP/2-to-upstream module implementation, as suggested in (1).

- Upstream infrastructure as available in 
  src/http/ngx_http_upstream.c is expected to be 
  protocol-agnostic.  Introducing calls like 
  ngx_http_v2_init_connection() there is a layering violation.  
  Instead, there should be something more generic.

- Doing protocol parsing elsewhere instead of parsing things based 
  on what nginx got from the connection results in some generic 
  upstream mechanisms rendered not working - notably, it is no 
  longer possible to simply write headers to a cache file.  
  Serialization introduced instead, at least in its current form, 
  looks more like a bandaid.

-- 
Maxim Dounin
http://nginx.org/