Hi, The patch seems fine and is somewhat similar to what I've posted before.
I guess, the copy-paste can be addressed some time later by someone else. > -----Original Message----- > From: Maxim Dounin <mdou...@mdounin.ru> > Sent: Saturday, June 25, 2022 22:48 > To: Pavel Pautov via nginx-devel <nginx-devel@nginx.org> > Subject: Re: SSL contexts reuse across locations > > EXTERNAL MAIL: nginx-devel-boun...@nginx.org > > Hello! > > On Sat, Jun 25, 2022 at 01:02:21AM +0000, Pavel Pautov via nginx-devel wrote: > > > > -----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. > > Yes, that's unfortunate. You may want to consider asking more > experienced developers in your company for mentorship. > > > > 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? > > Quoting the very first review: > > : 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 tries to be in line with this, and create at most one > proxy SSL context as long as no SSL proxying is configured (it > fails to do so though, see below). If proxy SSL settings are > redefined at some level, this redefinition adds an SSL context, > and this behaviour is something one might expect from the explicit > proxy_ssl_* directives in the configuration. > > > 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). > > My bad, mostly focused on the location level and missed that it is > not possible to properly compare http and server level settings > after the merge. > > Updated patch below, with additional function called just before > the merge of particular directives. The resulting codes tries to > be close to ngx_http_proxy_set_ssl(), where the relevant settings > are used to create SSL contexts, to be self-explanatory. > > Additionally, the patch now avoids creating SSL contexts if there > are no SSL proxying configured. This seems to complicate things > insignificantly given the new code layout, though ensures that > configurations with "proxy_ssl_verify on;" at http or stream level > without proxy_ssl_trusted_certificate set at the same level, as > seen in stream_proxy_ssl_verify.t, won't blow up. > > > 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. > > I don't think this is an issue. SSL contexts can be inherited > from the previous level if proxy_ssl_* directives are not > redefined; if any of them are redefined, this can result in an > additional SSL context. While there are settings which does not > create additional SSL contexts even if set to a different value > (such as proxy_ssl_server_name), the only guarantee we are willing > to provide is that inherited settings will result in optimal > memory usage. > > Either way, this is irrelevant with the updated patch. > > > My last patch already addresses all of above... > > Except you've failed to make it readable. > > > Also, it would be nice to avoid all this copy-paste > > As already explained, upstream SSL settings are protocol-specific > in the current design, as well as upstream SSL context creation. > While introducing some shared part is possible, it does not seem > to worth the effort, especially given the existing > protocol-specific difference. > > > and have the same optimization in the stream module. > > Given there are no locations in the stream module, I don't think > this is an issue there, or at least no more than server-side SSL > contexts. On the other hand, it is trivial to add the same > optimization to the stream module, and good from the code > consistency point of view. Added. > > > > # 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. > > Well, "shared_ssl" shouldn't have been added in the first place. > > Updated patch below. Review and testing appreciated. > > # HG changeset patch > # User Maxim Dounin <mdou...@mdounin.ru> > # Date 1656218606 -10800 > # Sun Jun 26 07:43:26 2022 +0300 > # Node ID f45142380cee37e71dab76d77ead279cf9b2f4e9 > # Parent fecd73db563fb64108f7669eca419badb2aba633 > Upstream: optimized use of SSL contexts (ticket #1234). > > To ensure optimal use of memory, SSL contexts for proxying are now > inherited from previous levels as long as relevant proxy_ssl_* directives > are not redefined. > > Further, when no proxy_ssl_* directives are redefined in a server block, > we now preserve plcf->upstream.ssl in the "http" section configuration > to inherit it to all servers. > > Similar changes made in uwsgi, grpc, and stream proxy. > > diff --git a/src/http/modules/ngx_http_grpc_module.c > b/src/http/modules/ngx_http_grpc_module.c > --- a/src/http/modules/ngx_http_grpc_module.c > +++ b/src/http/modules/ngx_http_grpc_module.c > @@ -209,6 +209,8 @@ static char *ngx_http_grpc_ssl_password_ > ngx_command_t *cmd, void *conf); > static char *ngx_http_grpc_ssl_conf_command_check(ngx_conf_t *cf, void > *post, > void *data); > +static ngx_int_t ngx_http_grpc_merge_ssl(ngx_conf_t *cf, > + ngx_http_grpc_loc_conf_t *conf, ngx_http_grpc_loc_conf_t *prev); > static ngx_int_t ngx_http_grpc_set_ssl(ngx_conf_t *cf, > ngx_http_grpc_loc_conf_t *glcf); > #endif > @@ -562,7 +564,7 @@ ngx_http_grpc_handler(ngx_http_request_t > ctx->host = glcf->host; > > #if (NGX_HTTP_SSL) > - u->ssl = (glcf->upstream.ssl != NULL); > + u->ssl = glcf->ssl; > > if (u->ssl) { > ngx_str_set(&u->schema, "grpcs://"); > @@ -4463,6 +4465,10 @@ ngx_http_grpc_merge_loc_conf(ngx_conf_t > > #if (NGX_HTTP_SSL) > > + if (ngx_http_grpc_merge_ssl(cf, conf, prev) != NGX_OK) { > + return NGX_CONF_ERROR; > + } > + > ngx_conf_merge_value(conf->upstream.ssl_session_reuse, > prev->upstream.ssl_session_reuse, 1); > > @@ -4524,7 +4530,7 @@ ngx_http_grpc_merge_loc_conf(ngx_conf_t > conf->grpc_values = prev->grpc_values; > > #if (NGX_HTTP_SSL) > - conf->upstream.ssl = prev->upstream.ssl; > + conf->ssl = prev->ssl; > #endif > } > > @@ -4874,17 +4880,63 @@ ngx_http_grpc_ssl_conf_command_check(ngx > > > static ngx_int_t > +ngx_http_grpc_merge_ssl(ngx_conf_t *cf, ngx_http_grpc_loc_conf_t *conf, > + ngx_http_grpc_loc_conf_t *prev) > +{ > + ngx_uint_t preserve; > + > + if (conf->ssl_protocols == 0 > + && conf->ssl_ciphers.data == NULL > + && conf->upstream.ssl_certificate == NGX_CONF_UNSET_PTR > + && conf->upstream.ssl_certificate_key == NGX_CONF_UNSET_PTR > + && conf->upstream.ssl_passwords == NGX_CONF_UNSET_PTR > + && conf->upstream.ssl_verify == NGX_CONF_UNSET > + && conf->ssl_verify_depth == NGX_CONF_UNSET_UINT > + && conf->ssl_trusted_certificate.data == NULL > + && conf->ssl_crl.data == NULL > + && conf->upstream.ssl_session_reuse == NGX_CONF_UNSET > + && conf->ssl_conf_commands == NGX_CONF_UNSET_PTR) > + { > + if (prev->upstream.ssl) { > + conf->upstream.ssl = prev->upstream.ssl; > + return NGX_OK; > + } > + > + preserve = 1; > + > + } else { > + preserve = 0; > + } > + > + conf->upstream.ssl = ngx_pcalloc(cf->pool, sizeof(ngx_ssl_t)); > + if (conf->upstream.ssl == NULL) { > + return NGX_ERROR; > + } > + > + conf->upstream.ssl->log = cf->log; > + > + /* > + * special handling to preserve conf->upstream.ssl > + * in the "http" section to inherit it to all servers > + */ > + > + if (preserve) { > + prev->upstream.ssl = conf->upstream.ssl; > + } > + > + return NGX_OK; > +} > + > + > +static ngx_int_t > ngx_http_grpc_set_ssl(ngx_conf_t *cf, ngx_http_grpc_loc_conf_t *glcf) > { > ngx_pool_cleanup_t *cln; > > - glcf->upstream.ssl = ngx_pcalloc(cf->pool, sizeof(ngx_ssl_t)); > - if (glcf->upstream.ssl == NULL) { > - return NGX_ERROR; > + if (glcf->upstream.ssl->ctx) { > + return NGX_OK; > } > > - glcf->upstream.ssl->log = cf->log; > - > if (ngx_ssl_create(glcf->upstream.ssl, glcf->ssl_protocols, NULL) > != NGX_OK) > { [...] _______________________________________________ nginx-devel mailing list -- nginx-devel@nginx.org To unsubscribe send an email to nginx-devel-le...@nginx.org
