Hi,

> -----Original Message-----
> From: Maxim Dounin <mdou...@mdounin.ru>
> Sent: Thursday, June 16, 2022 18:51
> 
> Hello!
> 
> On Thu, Jun 16, 2022 at 08:26:48AM +0000, Pavel Pautov via nginx-devel wrote:
> 
> > Looks like, we've made a full circle then... I've replied to
> > that suggestion already and in last e-mail (with patch) I note
> > that moving additional logic into the ngx_http_proxy_set_ssl()
> > has its own drawbacks, but I can certainly move more stuff into
> > it.
> >
> > So do you envision something like "ngx_http_proxy_set_ssl(cf,
> > conf, prev, reuse_ssl)"? As previously we've established that
> > directives merging stays out of ngx_http_proxy_set_ssl (and
> > reuse_ssl calculation has to happen before it).
> 
> I don't think further attempts to explain how to write readable
> code worth the effort.
> 

Too bad.

> Please see the patch below.  Review and testing appreciated.
> 

The patch seems to contradict some previously discussed points. For example, it 
can actually increase memory usage in certain configurations (say, when 
proxy_ssl_* directives in location override server level directives or when 
proxy_pass "https://..."; is absent). Or you don't consider this an issue 
anymore?

More importantly, it doesn't really solve the use case from #1234, i.e. http 
level proxy_ssl_* directives with many servers (as by default http level values 
are remain unset and thus are not equal to server level values).

Also, you effectively compare some directives by value (instead of checking the 
presence), so it might be surprising to the user that repeating some directives 
on inner level increases memory consumption and repeating others doesn't.

My last patch already addresses all of above...

Also, it would be nice to avoid all this copy-paste and have the same 
optimization in the stream module.

> # HG changeset patch
> # User Maxim Dounin <mdou...@mdounin.ru>
> # Date 1655429915 -10800
> #      Fri Jun 17 04:38:35 2022 +0300
> # Node ID e4a0eeb3edba037f0d090023a2242bda2f8dcb03
> # Parent  e23a385cd0ec866a3eb1d8c9c956991e1ed50d78
> Upstream: optimized use of SSL contexts (ticket #1234).
[...]
> 
>  #if (NGX_HTTP_SSL)
> -        u->ssl = (glcf->upstream.ssl != NULL);
> +        u->ssl = glcf->ssl;

I like this change, with it additional 'shared_ssl' pointer can be removed from 
my patch.

_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-le...@nginx.org

Reply via email to