> 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 <mdounin@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-bounces@nginx.org
>>
>> Hello!
>>
>> On Sat, Jun 25, 2022 at 01:02:21AM +0000, Pavel Pautov via nginx-devel wrote:
>>
>>>> -----Original Message-----
>>>> From: Maxim Dounin <mdounin@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 <mdounin@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 <mdounin@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-leave@nginx.org

--
Sergey Kandaurov

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