> On 28 Jun 2022, at 09:26, Pavel Pautov via nginx-devel > <nginx-devel@nginx.org> wrote: > > 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.
I agree, the patch looks good to me, tested in various configurations (including if() block, etc.) > >> -----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. Well, redefining proxy_ssl_* directives by itself doesn't result in additional SSL context, it can be created in locations with proxy_pass. In my tests with basic configuration, without proxy_ssl_certificate and proxy_ssl_trusted_certificate and all that, on OpenSSL 3.0.x, creating SSL context results in allocation of by order of 10k memory (tested using FreeBSD ktrace + utrace(2) malloc(3) records.) In configuration levels with just redefined proxy_ssl_* directives, it's an additional sizeof(ngx_ssl_t) allocation, currently of 24 bytes. So, for example, if proxy_ssl_* directives are defined at http level, and then redefined at server level only, this results in one context per server (given that it has proxy_pass in locations). Just to make it clear. >> >> 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 -- Sergey Kandaurov _______________________________________________ nginx-devel mailing list -- nginx-devel@nginx.org To unsubscribe send an email to nginx-devel-le...@nginx.org
