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 <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.
>
> 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