Hello! On Thu, Jan 26, 2023 at 08:00:53PM +0400, Sergey Kandaurov wrote:
> > On 26 Jan 2023, at 01:29, Maxim Dounin <mdou...@mdounin.ru> wrote: > > > > On Mon, Jan 23, 2023 at 02:21:33PM +0400, Sergey Kandaurov wrote: > > > >>> On 31 Dec 2022, at 18:35, Maxim Dounin <mdou...@mdounin.ru> wrote: > >>> > >>> # HG changeset patch > >>> # User Maxim Dounin <mdou...@mdounin.ru> > >>> # Date 1672497248 -10800 > >>> # Sat Dec 31 17:34:08 2022 +0300 > >>> # Node ID c215d5cf25732ece1819cf1cd48ebb480bb642c7 > >>> # Parent 07b0bee87f32be91a33210bc06973e07c4c1dac9 > >>> Added warning about redefinition of listen socket protocol options. > >>> > >>> The "listen" directive in the http module can be used multiple times > >>> in different server blocks. Originally, it was supposed to be specified > >>> once with various socket options, and without any parameters in virtual > >>> server blocks. For example: > >>> > >>> server { listen 80 backlog=1024; server_name foo; ... } > >>> server { listen 80; server_name bar; ... } > >>> server { listen 80; server_name bazz; ... } > >>> > >>> The address part of the syntax ("address[:port]" / "port" / "unix:path") > >>> uniquely identifies the listening socket, and therefore is enough for > >>> name-based virtual servers (to let nginx know that the virtual server > >>> accepts requests on the listening socket in question). > >>> > >>> To ensure that listening options do not conflict between virtual servers, > >>> they were allowed only once. For example, the following configuration > >>> will be rejected ("duplicate listen options for 0.0.0.0:80 in ..."): > >>> > >>> server { listen 80 backlog=1024; server_name foo; ... } > >>> server { listen 80 backlog=512; server_name bar; ... } > >>> > >>> At some point it was, however, noticed, that it is sometimes convenient > >>> to repeat some options for clarity. In nginx 0.8.51 the "ssl" parameter > >>> was allowed to be specified multiple times, e.g.: > >>> > >>> server { listen 443 ssl backlog=1024; server_name foo; ... } > >>> server { listen 443 ssl; server_name bar; ... } > >>> server { listen 443 ssl; server_name bazz; ... } > >>> > >>> This approach makes configuration more readable, since SSL sockets are > >>> immediately visible in the configuration. If this is not needed, just the > >>> address can still be used. > >>> > >>> Later, additional protocol-specific options similar to "ssl" were > >>> introduced, notably "http2" and "proxy_protocol". With these options, > >>> one can write: > >>> > >>> server { listen 443 ssl backlog=1024; server_name foo; ... } > >>> server { listen 443 http2; server_name bar; ... } > >>> server { listen 443 proxy_protocol; server_name bazz; ... } > >>> > >>> The resulting socket will use ssl, http2, and proxy_protocol, but this > >>> is not really obvious from the configuration. > >>> > >>> To ensure such misleading configurations are not allowed, nginx now > >>> warns as long as the "listen" directive is used with options different > >> > >> nitpicking: > >> > >> "ensure .. are not allowed" and "warns" don't seem to be equally strong. > >> As such, I'd rewrite to something like: > >> > >> To emphasize such misleading configurations are discouraged, nginx now > > > > Thanks, changed. > > > >>> from the options previously used if these are potentially confusing. > >> > >> Not really confident what "these" refers to. > >> > >> s/these/they/ ? > > > > I don't think that there is much difference between "these" and > > "they" when it comes to what they refer to. Either way, "if this > > is potentially confusing" is probably better and unambiguously > > refers to the fact that the "listen" directive is used with the > > different options. Changed. > > Sounds good as well, tnx. > > > > >>> In particular, the following configurations are allowed: > >>> > >>> server { listen 8401 ssl backlog=1024; server_name foo; } > >>> server { listen 8401 ssl; server_name bar; } > >>> server { listen 8401 ssl; server_name bazz; } > >>> > >>> server { listen 8402 ssl http2 backlog=1024; server_name foo; } > >>> server { listen 8402 ssl; server_name bar; } > >>> server { listen 8402 ssl; server_name bazz; } > >>> > >>> server { listen 8403 ssl; server_name bar; } > >>> server { listen 8403 ssl; server_name bazz; } > >>> server { listen 8403 ssl http2; server_name foo; } > >>> > >>> server { listen 8404 ssl http2 backlog=1024; server_name foo; } > >>> server { listen 8404 http2; server_name bar; } > >>> server { listen 8404 http2; server_name bazz; } > >>> > >>> server { listen 8405 ssl http2 backlog=1024; server_name foo; } > >>> server { listen 8405 ssl http2; server_name bar; } > >>> server { listen 8405 ssl http2; server_name bazz; } > >>> > >>> server { listen 8406 ssl; server_name foo; } > >>> server { listen 8406; server_name bar; } > >>> server { listen 8406; server_name bazz; } > >>> > >>> And the following configurations will generate warnings: > >>> > >>> server { listen 8501 ssl http2 backlog=1024; server_name foo; } > >>> server { listen 8501 http2; server_name bar; } > >>> server { listen 8501 ssl; server_name bazz; } > >>> > >>> server { listen 8502 backlog=1024; server_name foo; } > >>> server { listen 8502 ssl; server_name bar; } > >>> > >>> server { listen 8503 ssl; server_name foo; } > >>> server { listen 8503 http2; server_name bar; } > >>> > >>> server { listen 8504 ssl; server_name foo; } > >>> server { listen 8504 http2; server_name bar; } > >>> server { listen 8504 proxy_protocol; server_name bazz; } > >>> > >>> server { listen 8505 ssl http2 proxy_protocol; server_name foo; } > >>> server { listen 8505 ssl http2; server_name bar; } > >>> server { listen 8505 ssl; server_name bazz; } > >>> > >>> server { listen 8506 ssl http2; server_name foo; } > >>> server { listen 8506 ssl; server_name bar; } > >>> server { listen 8506; server_name bazz; } > >>> > >>> server { listen 8507 ssl; server_name bar; } > >>> server { listen 8507; server_name bazz; } > >>> server { listen 8507 ssl http2; server_name foo; } > >>> > >>> server { listen 8508 ssl; server_name bar; } > >>> server { listen 8508; server_name bazz; } > >>> server { listen 8508 ssl backlog=1024; server_name foo; } > >>> > >>> server { listen 8509; server_name bazz; } > >>> server { listen 8509 ssl; server_name bar; } > >>> server { listen 8509 ssl backlog=1024; server_name foo; } > >>> > >> > >> 15 examples of dos and don'ts looks slightly excessive. > >> The accurate description (such as provided by you below) allows > >> to reduce most of them to e.g. four common invalid configurations: > >> > >> A lesser option set with socket option: > >> > >> server { listen 8443 backlog=1024; server_name foo; } > >> server { listen 8443 http2; server_name bar; } > >> > >> The main option set is repeated at least twice: > >> > >> server { listen 127.0.0.1:8443; server_name foo; } > >> server { listen 127.0.0.1:8443 ssl; server_name bar; } > >> server { listen 127.0.0.1:8443 ssl; server_name baz; } > >> > >> Option sets partially overlap: > >> > >> server { listen 127.0.0.1:8443 ssl; server_name foo; } > >> server { listen 127.0.0.1:8443 http2; server_name bar; } > >> > >> More than two option sets: > >> > >> server { listen 127.0.0.1:8443 http2 ssl; server_name foo; } > >> server { listen 127.0.0.1:8443 http2; server_name bar; } > >> server { listen 127.0.0.1:8443 ssl; server_name baz; } > > > > While your approach might be better from documentation point > > of view, the way it is currently described in the commit log is > > how it was designed: from the examples of valid and invalid > > configurations. > > > > My current working tests contain 18 valid and 22 invalid > > configurations, derived from the ones provided in the commit log > > with additional shuffling. But since these are derived I've > > decided to avoid providing all of them in the commit log. > > > >>> The basic idea is that at most two sets of protocol options are allowed: > >>> the main one (with socket options, if any), and a shorter one, with > >>> options > >>> being a subset of the main options, repeated for clarity. As long as the > >>> shorter set of protocol options is used, all listen directives except the > >>> main one should use it. > >> > >> I'd move this paragraph somewhere before examples, as this is the most > >> specific description of things actually changed. > > > > This paragraph summarizes changes made to address the issue > > described, and I don't think moving it will improve things. > > The above looks sane now, thanks for the explanation. > I won't insist on further adjustments. > > > > >> BTW, while reviewing I caught sort of a bug. > >> As I understand the above explanation, if there are both full and short > >> sets present, then at most one listen directive can have the full set, > >> while shorter sets can be repeated. If so, then with the proposed patch > >> the next configuration is expectedly invalid: > >> > >> server { listen 127.0.0.1:8443 ssl; server_name foo; } > >> server { listen 127.0.0.1:8443 ssl; server_name bar; } > >> server { listen 127.0.0.1:8443; server_name baz; } > >> > >> This is expected since first two servers with same options are > >> interpreted as a short form (with full form seen potentially later on), > >> but 3rd server has lesser options, which is caught by this check: > >> (addr[i].protocols_set && protocols != addr[i].protocols) > >> Which is interpreted as: > >> "this server has a lesser set that doesn't match a a shorter set". > >> > >> Now, if 3rd server is moved first, configuration starts to pass: > >> > >> server { listen 127.0.0.1:8443; server_name baz; } > >> server { listen 127.0.0.1:8443 ssl; server_name foo; } > >> server { listen 127.0.0.1:8443 ssl; server_name bar; } > >> > >> This is because after (now) 2nd server, it is parsed as: > >> 1st server has a short form, and 2nd server has a full form. > >> Then 3rd server goes to "the same options" case. This also > >> overwrites the remembered shorter set in addr[i].protocols. > >> > >> I guess an additional check should be added to address this. > >> It is similar to the "options removed" case and ensures that > >> the repeated options set actually matches a shorter set: > >> > >> diff --git a/src/http/ngx_http.c b/src/http/ngx_http.c > >> --- a/src/http/ngx_http.c > >> +++ b/src/http/ngx_http.c > >> @@ -1345,7 +1345,9 @@ ngx_http_add_addresses(ngx_conf_t *cf, n > >> > >> /* the same options */ > >> > >> - if (lsopt->set && addr[i].protocols_changed) { > >> + if ((lsopt->set && addr[i].protocols_changed) > >> + || (addr[i].protocols_set && protocols != > >> addr[i].protocols)) > >> + { > >> ngx_conf_log_error(NGX_LOG_WARN, cf, 0, > >> "protocol options redefined for %V", > >> &addr[i].opt.addr_text); > > > > Thanks for catching this, applied. > > > > Updated patch: > > > > # HG changeset patch > > # User Maxim Dounin <mdou...@mdounin.ru> > > # Date 1674624978 -10800 > > # Wed Jan 25 08:36:18 2023 +0300 > > # Node ID 1619d1563c3231d4283937610ce97aa1e3824fb8 > > # Parent c7e103acb409f0352cb73997c053b3bdbb8dd5db > > Added warning about redefinition of listen socket protocol options. > > > > The "listen" directive in the http module can be used multiple times > > in different server blocks. Originally, it was supposed to be specified > > once with various socket options, and without any parameters in virtual > > server blocks. For example: > > > > server { listen 80 backlog=1024; server_name foo; ... } > > server { listen 80; server_name bar; ... } > > server { listen 80; server_name bazz; ... } > > > > The address part of the syntax ("address[:port]" / "port" / "unix:path") > > uniquely identifies the listening socket, and therefore is enough for > > name-based virtual servers (to let nginx know that the virtual server > > accepts requests on the listening socket in question). > > > > To ensure that listening options do not conflict between virtual servers, > > they were allowed only once. For example, the following configuration > > will be rejected ("duplicate listen options for 0.0.0.0:80 in ..."): > > > > server { listen 80 backlog=1024; server_name foo; ... } > > server { listen 80 backlog=512; server_name bar; ... } > > > > At some point it was, however, noticed, that it is sometimes convenient > > to repeat some options for clarity. In nginx 0.8.51 the "ssl" parameter > > was allowed to be specified multiple times, e.g.: > > > > server { listen 443 ssl backlog=1024; server_name foo; ... } > > server { listen 443 ssl; server_name bar; ... } > > server { listen 443 ssl; server_name bazz; ... } > > > > This approach makes configuration more readable, since SSL sockets are > > immediately visible in the configuration. If this is not needed, just the > > address can still be used. > > > > Later, additional protocol-specific options similar to "ssl" were > > introduced, notably "http2" and "proxy_protocol". With these options, > > one can write: > > > > server { listen 443 ssl backlog=1024; server_name foo; ... } > > server { listen 443 http2; server_name bar; ... } > > server { listen 443 proxy_protocol; server_name bazz; ... } > > > > The resulting socket will use ssl, http2, and proxy_protocol, but this > > is not really obvious from the configuration. > > > > To emphasize such misleading configurations are discouraged, nginx now > > warns as long as the "listen" directive is used with options different > > from the options previously used if this is potentially confusing. > > > > In particular, the following configurations are allowed: > > > > server { listen 8401 ssl backlog=1024; server_name foo; } > > server { listen 8401 ssl; server_name bar; } > > server { listen 8401 ssl; server_name bazz; } > > > > server { listen 8402 ssl http2 backlog=1024; server_name foo; } > > server { listen 8402 ssl; server_name bar; } > > server { listen 8402 ssl; server_name bazz; } > > > > server { listen 8403 ssl; server_name bar; } > > server { listen 8403 ssl; server_name bazz; } > > server { listen 8403 ssl http2; server_name foo; } > > > > server { listen 8404 ssl http2 backlog=1024; server_name foo; } > > server { listen 8404 http2; server_name bar; } > > server { listen 8404 http2; server_name bazz; } > > > > server { listen 8405 ssl http2 backlog=1024; server_name foo; } > > server { listen 8405 ssl http2; server_name bar; } > > server { listen 8405 ssl http2; server_name bazz; } > > > > server { listen 8406 ssl; server_name foo; } > > server { listen 8406; server_name bar; } > > server { listen 8406; server_name bazz; } > > > > And the following configurations will generate warnings: > > > > server { listen 8501 ssl http2 backlog=1024; server_name foo; } > > server { listen 8501 http2; server_name bar; } > > server { listen 8501 ssl; server_name bazz; } > > > > server { listen 8502 backlog=1024; server_name foo; } > > server { listen 8502 ssl; server_name bar; } > > > > server { listen 8503 ssl; server_name foo; } > > server { listen 8503 http2; server_name bar; } > > > > server { listen 8504 ssl; server_name foo; } > > server { listen 8504 http2; server_name bar; } > > server { listen 8504 proxy_protocol; server_name bazz; } > > > > server { listen 8505 ssl http2 proxy_protocol; server_name foo; } > > server { listen 8505 ssl http2; server_name bar; } > > server { listen 8505 ssl; server_name bazz; } > > > > server { listen 8506 ssl http2; server_name foo; } > > server { listen 8506 ssl; server_name bar; } > > server { listen 8506; server_name bazz; } > > > > server { listen 8507 ssl; server_name bar; } > > server { listen 8507; server_name bazz; } > > server { listen 8507 ssl http2; server_name foo; } > > > > server { listen 8508 ssl; server_name bar; } > > server { listen 8508; server_name bazz; } > > server { listen 8508 ssl backlog=1024; server_name foo; } > > > > server { listen 8509; server_name bazz; } > > server { listen 8509 ssl; server_name bar; } > > server { listen 8509 ssl backlog=1024; server_name foo; } > > > > The basic idea is that at most two sets of protocol options are allowed: > > the main one (with socket options, if any), and a shorter one, with options > > being a subset of the main options, repeated for clarity. As long as the > > shorter set of protocol options is used, all listen directives except the > > main one should use it. > > > > diff --git a/src/http/ngx_http.c b/src/http/ngx_http.c > > --- a/src/http/ngx_http.c > > +++ b/src/http/ngx_http.c > > @@ -1228,7 +1228,8 @@ static ngx_int_t > > ngx_http_add_addresses(ngx_conf_t *cf, ngx_http_core_srv_conf_t *cscf, > > ngx_http_conf_port_t *port, ngx_http_listen_opt_t *lsopt) > > { > > - ngx_uint_t i, default_server, proxy_protocol; > > + ngx_uint_t i, default_server, proxy_protocol, > > + protocols, protocols_prev; > > ngx_http_conf_addr_t *addr; > > #if (NGX_HTTP_SSL) > > ngx_uint_t ssl; > > @@ -1264,12 +1265,18 @@ ngx_http_add_addresses(ngx_conf_t *cf, n > > default_server = addr[i].opt.default_server; > > > > proxy_protocol = lsopt->proxy_protocol || > > addr[i].opt.proxy_protocol; > > + protocols = lsopt->proxy_protocol; > > + protocols_prev = addr[i].opt.proxy_protocol; > > > > #if (NGX_HTTP_SSL) > > ssl = lsopt->ssl || addr[i].opt.ssl; > > + protocols |= lsopt->ssl << 1; > > + protocols_prev |= addr[i].opt.ssl << 1; > > #endif > > #if (NGX_HTTP_V2) > > http2 = lsopt->http2 || addr[i].opt.http2; > > + protocols |= lsopt->http2 << 2; > > + protocols_prev |= addr[i].opt.http2 << 2; > > #endif > > > > if (lsopt->set) { > > @@ -1299,6 +1306,57 @@ ngx_http_add_addresses(ngx_conf_t *cf, n > > addr[i].default_server = cscf; > > } > > > > + /* check for conflicting protocol options */ > > + > > + if ((protocols | protocols_prev) != protocols_prev) { > > + > > + /* options added */ > > + > > + if ((addr[i].opt.set && !lsopt->set) > > + || addr[i].protocols_changed > > + || (protocols | protocols_prev) != protocols) > > + { > > + ngx_conf_log_error(NGX_LOG_WARN, cf, 0, > > + "protocol options redefined for %V", > > + &addr[i].opt.addr_text); > > + } > > + > > + addr[i].protocols = protocols_prev; > > + addr[i].protocols_set = 1; > > + addr[i].protocols_changed = 1; > > + > > + } else if ((protocols_prev | protocols) != protocols) { > > + > > + /* options removed */ > > + > > + if (lsopt->set > > + || (addr[i].protocols_set && protocols != > > addr[i].protocols)) > > + { > > + ngx_conf_log_error(NGX_LOG_WARN, cf, 0, > > + "protocol options redefined for %V", > > + &addr[i].opt.addr_text); > > + } > > + > > + addr[i].protocols = protocols; > > + addr[i].protocols_set = 1; > > + addr[i].protocols_changed = 1; > > + > > + } else { > > + > > + /* the same options */ > > + > > + if ((lsopt->set && addr[i].protocols_changed) > > + || (addr[i].protocols_set && protocols != > > addr[i].protocols)) > > + { > > + ngx_conf_log_error(NGX_LOG_WARN, cf, 0, > > + "protocol options redefined for %V", > > + &addr[i].opt.addr_text); > > + } > > + > > + addr[i].protocols = protocols; > > + addr[i].protocols_set = 1; > > + } > > + > > addr[i].opt.default_server = default_server; > > addr[i].opt.proxy_protocol = proxy_protocol; > > #if (NGX_HTTP_SSL) > > @@ -1355,6 +1413,9 @@ ngx_http_add_address(ngx_conf_t *cf, ngx > > } > > > > addr->opt = *lsopt; > > + addr->protocols = 0; > > + addr->protocols_set = 0; > > + addr->protocols_changed = 0; > > addr->hash.buckets = NULL; > > addr->hash.size = 0; > > addr->wc_head = NULL; > > diff --git a/src/http/ngx_http_core_module.h > > b/src/http/ngx_http_core_module.h > > --- a/src/http/ngx_http_core_module.h > > +++ b/src/http/ngx_http_core_module.h > > @@ -274,6 +274,10 @@ typedef struct { > > typedef struct { > > ngx_http_listen_opt_t opt; > > > > + unsigned protocols:3; > > + unsigned protocols_set:1; > > + unsigned protocols_changed:1; > > + > > ngx_hash_t hash; > > ngx_hash_wildcard_t *wc_head; > > ngx_hash_wildcard_t *wc_tail; > > > > Looks good now, thanks. Thanks for the review, pushed to http://mdounin.ru/hg/nginx. Also, a fix to the only warning generated by tests is pushed to http://mdounin.ru/hg/nginx-tests. -- Maxim Dounin http://mdounin.ru/ _______________________________________________ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel