Hi,
> -----Original Message-----
> From: Maxim Dounin <mdou...@mdounin.ru>
> Sent: Wednesday, May 18, 2022 11:32
[..]
> > At very least, ngx_http_proxy_set_ssl() needs to be converted
> > into ngx_http_proxy_create_ssl().
>
> You may want to focus on actually making the code more readable
> and abstracting it into ngx_http_proxy_set_ssl() instead.
> Something like ngx_http_upstream_hide_headers_hash() might be a
> good example on how to do it properly.
Do you suggest to make ngx_http_proxy_set_ssl() responsible for merging context
related settings? I guess, we can do that, but something like
ngx_http_proxy_create_ssl() might be still beneficial.
> Also, it should be a good idea to avoid creating SSL contexts if
> there is no SSL proxying configured. Or, at very least, make sure
> only one context at the http level is used in such cases, so
> configurations with many servers won't suddenly blow up.
The patch shouldn't cause a blow up to my understanding, expect perhaps for
very specific configs like:
server {
proxy_ssl_session_reuse off;
location / {
proxy_ssl_session_reuse on;
proxy_pass https://backend;
}
}
Basically, one have to have "proxy_ssl_*" directives scattered across hierarchy
of locations with a single terminal "proxy_pass https". If there are many
terminal locations with different proxy_passes, then increase shouldn't be that
big. If there is no terminal "proxy_pass https", then why are these proxy_ssl_*
options even there?
It might be interesting to measure the memory cost of "default" ssl context
per server. I assume, unless user loads big certificate bundle, the impact
should be tolerable.
For configs where proxy_ssl_* directives are grouped at server level or absent
altogether there is no impact with current patch.
That being said, I think, we can actually satisfy "no SSL contexts without
https" requirement by linking location configs and traversing them backwards in
a search for location with "proxy_ssl_*" directives. I'd update patch with that
approach in mind.
I'd like to avoid forcing users into creating wrapper locations with dummy
"proxy_pass https" for the sake of runtime optimization, i.e. just moving
"proxy_ssl_*" directives to the server level should be enough.
> > But there are also a couple of things to discuss:
> >
> > 1. Patch uses pretty straightforward reuse criteria (absence of
> > directives), but shall we go further, say, compare directive
> > arguments (with special treatment of complex values with
> > variables)?
>
> Just checking if the relevant directives were inherited from the
> previous level should be enough, as it will allow creating
> memory-effective configurations.
>
> > 2. Since similar change also makes sense for "grpc", "uwsgi"
> > (and may be "stream proxy") modules, perhaps it's time to factor
> > out SSL upstream settings code for all these modules to avoid
> > copypasting of above patch? We can introduce something like
> > "ngx_ssl_upstream_conf_t" to keep shared SSL settings and unite
> > ngx_http_(proxy|grpc|uwsgi)_set_ssl functions. Config merge
> > logic (together with attached patch) can be moved to something
> > like ngx_ssl_upstream_conf_merge. Optionally,
> > ngx_http_upstream_conf_t can be updated to contain
> > ngx_ssl_upstream_conf_t.
>
> I don't think it the effort. Further, there are protocol-specific
> differences, such as ALPN in gRPC proxy, so you can't fully
> abstract it anyway.
Worth the effort? It doesn't look like a lot of effort to me compared with
copy-pasting (and later maintaining) the optimized merge logic everywhere. I've
noted the ALPN related peace in gRPC, but we can just leave it in the gRPC
module. Modules still could have some specific settings in addition to the
standard set.
Anyway, we need to agree on the solution for proxy module first.
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-le...@nginx.org
