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

[Maxim Dounin]

Hello!

On Tue, Jul 25, 2017 at 06:28:16PM -0700, Piotr Sikora 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.

Biggest concert here are various "if (h2c->server) { ... } else { 
... }" constructions.  Certainly there can (and probably should) 
be some degree of code reuse between client and server code, 
though it would be much better to keep them easily separateable.

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

This is how our upstream protocol modules are currently 
implemented: each module provides a configuration-specific parts, 
as well as it's protocol-specific parts.  Reducing duplication of 
configuration parsing is one of the main reasons of the "all 
protocols in the proxy module" plan as described above.  This is 
not what we currently have though, and introducing ad-hock hacks 
to save some lines of code looks much worse than some amount of 
code duplication.

Also, keeping the module separate would actually allow to reduce 
amount of code needed in some cases.  For example, since there is no 
cache support, at least in initial implementation, it would be 
possible to simply omit relevant directives.

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

Such feature is anyway does not look compatible with current 
upstream infrastructure, as request is created once, before even 
connecting to backend servers, and it is not possible to switch to 
a different protocol after a connection.

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

Well, if you insist on doing things in the proxy module, posponing 
HTTP/2 till the rewrite is done is probably the only option.  We 
can consider prioritizing the rewrite rewrite accordingly.

[...]

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

The problem here is that HTTP/2 is not a separate layer, in 
contrast to SSL, but rather tries to be an application-level 
protocol.  On the other hand, this is probably the 

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

[Piotr Sikora via nginx-devel]

Hey Maxim,

On Tue, Jul 25, 2017 at 6:28 PM, Piotr Sikora  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
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

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

[Piotr Sikora via nginx-devel]

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?

Best regards,
Piotr Sikora
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel