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

[Piotr Sikora]

Hey Maxim,

On Tue, Jul 25, 2017 at 6:28 PM, Piotr Sikora <piotrsikora at google.com> wrote:
> Hey Maxim,
>
>> 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.
>
> I strongly disagree. Changes introduced to ngx_http_v2.c and
> ngx_http_v2_filter_module.c are minimal.
>
> Also, having separate client-side and server-side code would result in
> thousands of lines of duplicated code, for no reason, really.
>
>> 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;
>
> But that's basically:
>
>     cat ngx_http_proxy_module.c \
>         | sed 's/ngx_http_proxy/ngx_http_proxy_v2/g' \
>         > ngx_http_proxy_v2_module.c
>
> I don't see how copying ~4500 lines of code is a good idea.
>
> Also, as mentioned elsewhere, one of the reasons for adding HTTP/2
> code to the proxy module was the ability to negotiate HTTP/1.1 or
> HTTP/2 via ALPN. Creating a separate HTTP/2-to-upstreams module won't
> allow to add such feature in the future.
>
>> - start working on the different protocols in the proxy module,
>>   and introduce HTTP/2 proxying after this work is done.
>
> How is that even an option here? It's going to take forever before
> such rewrite is done, and I have no desire nor time to work on that.
>
>> 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).
>
> Sure, this and a few other functions could be added to different files.
>
>> - 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.
>
> I agree that ngx_http_v2_init_connection() isn't perfect, however,
> fake connections are an abstraction layer that needs to be added
> somewhere. The same is done for SSL, and it's somehow acceptable.
>
> I'm happy to use whatever generic mechanism is available, but there is
> none at the moment, and I don't see how adding even more code and
> complexity to this already big patchset is going to get us anywhere.
>
>> - 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.
>
> Except that HTTP/2 headers as read from the wire, even if parsed in
> separate module, couldn't be simply written to a cache file, because
> of stateful HPACK encoding, so serialization would need to be done
> anyway.
>
> Anyway, it appears that your "serious concerns" are mostly about
> organization of the code, and not the code itself. What's unclear to
> me is how much of the code review this patchset actually received,
> i.e. if the existing code would be moved to a separate
> HTTP/2-to-upstreams module, would it be acceptable or do you have
> other issues with the code?

Ping.

Best regards,
Piotr Sikora