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

RE: [PATCH 1 of 3] PSK: connection support

[Karstens, Nate]

Maxim,

I'm starting to get back to this now and wanted to follow up on your comments 
in http://mailman.nginx.org/pipermail/nginx-devel/2017-June/010247.html to see 
if you had a preference on when the PSKs should be read from the file. I 
believe we have two options:

1) At startup into a hash.
Advantages: Faster lookups, PSK file can be read-only by root, can warn on 
syntax errors
Disadvantages: Must reload to add/remove PSKs, more config options (for the 
hash max size and bucket size)

2) As needed. This is what was done in the last patch, but changed to no longer 
check for syntax errors at startup.
Advantages: Do not need to reload to add/remove users, can control access using 
groups
Disadvantages: Slower lookups (linear, but this can be improved using a RAM 
disk)

Thanks,

Nate

-Original Message-
From: nginx-devel [mailto:nginx-devel-boun...@nginx.org] On Behalf Of Maxim 
Dounin
Sent: Friday, June 30, 2017 6:58 AM
To: nginx-devel@nginx.org
Subject: Re: [PATCH 1 of 3] PSK: connection support

Hello!

On Thu, Jun 29, 2017 at 10:00:45PM +, Karstens, Nate wrote:

> Thanks for the comments. I'll try to start on those in a couple of
> days.

Just to make it clear: there is no need to hurry.  Likely I won't be able to 
review new patches in at least a couple of weeks, so feel free to spend more 
time polishing the patches.

> My company uses Outlook/Exchange for email, so I don't think I'll be
> able to use hg email, do you have any other suggestions? Thanks also
> for your patience, I've used Git quite a bit but am new to Mercurial.

The "hg email" command can work with any SMTP server, including Exchange.  Or 
you can ensure proper threading manually by using a "reply" function.

> Utkarsh sounds like he is trying to use PSK for TLS v1.3 session
> resumption. Given that each TLS connection could potentially result in
> a new PSK I think only reading them at startup could result in too
> many refreshes. I think there might be some benefit to the original
> approach in regards to storing each PSK in its own file in a
> designated directory. Benefits include:

TLS v1.3 session resumption uses PSK internally, but it is very different from 
internal usage point of view.  It is handled well enough with existing session 
cache / session tickets mechanisms.

[...]

> > +ngx_int_t
> > +ngx_ssl_psk_file(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *file)
> > +
> > +{
>
> Style: extra empty line.
>
> > +ngx_int_t   rc;
> > +
> > +if (SSL_CTX_set_ex_data(ssl->ctx, ngx_ssl_psk_index, file) == 0) {
> > +ngx_ssl_error(NGX_LOG_ALERT, ssl->log, 0,
> > +  "SSL_CTX_set_ex_data() failed");
> > +return NGX_ERROR;
> > +}
> > +
> > +rc = ngx_ssl_psk_read(file, NULL, NULL, 0);
> > +
> > +return rc == 0 ? NGX_OK : NGX_ERROR; }

[...]

> > @@ -800,6 +810,12 @@
> >
> >  }
> >
> > +if (ngx_ssl_psk_file(cf, >ssl, >psk_file)
> > +!= NGX_OK)
> > +{
> > +return NGX_CONF_ERROR;
> > +}

Note: this calls ngx_ssl_psk_file() unconditionally, and
ngx_ssl_psk_file() also doesn't check if a file is configured.  As a result, a 
configuration without ssl_psk_file fails.

--
Maxim Dounin
http://nginx.org/
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel



CONFIDENTIALITY NOTICE: This email and any attachments are for the sole use of 
the intended recipient(s) and contain information that may be Garmin 
confidential and/or Garmin legally privileged. If you have received this email 
in error, please notify the sender by reply email and delete the message. Any 
disclosure, copying, distribution or use of this communication (including 
attachments) by someone other than the intended recipient is prohibited. Thank 
you.
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

[nginx] SSL: fixed typo in the error message.

[Sergey Kandaurov]

details:   http://hg.nginx.org/nginx/rev/07a49cce21ca
branches:  
changeset: 7074:07a49cce21ca
user:  Sergey Kandaurov 
date:  Tue Jul 25 17:21:59 2017 +0300
description:
SSL: fixed typo in the error message.

diffstat:

 src/event/ngx_event_openssl.c |  2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diffs (12 lines):

diff -r 230d16d35ebc -r 07a49cce21ca src/event/ngx_event_openssl.c
--- a/src/event/ngx_event_openssl.c Fri Jul 21 19:47:56 2017 +0300
+++ b/src/event/ngx_event_openssl.c Tue Jul 25 17:21:59 2017 +0300
@@ -3128,7 +3128,7 @@ ngx_ssl_session_ticket_keys(ngx_conf_t *
 {
 if (paths) {
 ngx_log_error(NGX_LOG_WARN, ssl->log, 0,
-  "\"ssl_session_ticket_keys\" ignored, not supported");
+  "\"ssl_session_ticket_key\" ignored, not supported");
 }
 
 return NGX_OK;
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel