[PATCH] Resolver: relax validation of response flags to allow AD and CD
A correct nameserver implementation should not be sending any of these flags to
a non-security aware client. CD must be copied from a query to the
corresponding response, and AD must only be set if all the RRsets are authentic
and DO or AD were present in the query. The behavior was allowed in the early
DNSSEC standards, though, and the prohibition in the RFC is not strict. The
change is, in fact, motivated by a report about some DNS server passing CD in a
response to the nginx resolver.
There's a valid concern that CD may mean that the upstream nameserver did not
check the response signatures and passed the answer as is. There's nothing we
can do with such nameserver from the nginx side, though - we can't prevent it
from sending unexpected or unchecked responses. And there's already an error
message if an RRSIG record makes it to the answer we receive:
2022/04/28 17:34:10 [error] 27847#27847: unexpected RR type 46 in DNS
response
# HG changeset patch
# User Aleksei Bavshin
# Date 1651176054 25200
# Thu Apr 28 13:00:54 2022 -0700
# Node ID 5a570e610b375d1f3442a5b0fc1844be3909d103
# Parent a736a7a613ea6e182ff86fbadcb98bb0f8891c0b
Resolver: relax validation of response flags to allow AD and CD
The check introduced in aebdca7e8f8f was using reserved must-be-zero bits
definition from RFC1035. Later RFCs for DNS Security Extensions (4035 & 6840)
allocated AD and CD from these reserved bits.
It's said that AD and CD SHOULD only appear in the reply for a query from
a security aware resolver, but that is not a strict prohibition, and it was
not included in the early set of DNSSEC RFCs. We may encounter these bits in
a reply when the upstream nameserver implements pre-RFC4035 (with RFC6840
clarifications) standard or uses overly aggressive caching.
diff --git a/src/core/ngx_resolver.c b/src/core/ngx_resolver.c
--- a/src/core/ngx_resolver.c
+++ b/src/core/ngx_resolver.c
@@ -1746,7 +1746,7 @@ ngx_resolver_process_response(ngx_resolv
(response->nar_hi << 8) + response->nar_lo);
/* response to a standard query */
-if ((flags & 0xf870) != 0x8000 || (trunc && tcp)) {
+if ((flags & 0xf840) != 0x8000 || (trunc && tcp)) {
ngx_log_error(r->log_level, r->log, 0,
"invalid %s DNS response %ui fl:%04Xi",
tcp ? "TCP" : "UDP", ident, flags);
___
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-le...@nginx.org
[PATCH] Stream: don't flush empty buffers created for read errors.
In all honesty, the main motivation for the patch is to address a regression in
UDP healthchecks caused by the behavior described below. It's already covered
by tests, but the corresponding cases were disabled by default.
I considered
cl->buf->flush = !src->read->error;
or even
cl->buf->flush = !src->read->eof;
but neither of those are exactly equivalent to the code below (with the latter
having significantly different behavior), and IMO it would look less obvious.
# HG changeset patch
# User Aleksei Bavshin
# Date 1653330584 25200
# Mon May 23 11:29:44 2022 -0700
# Branch se
# Node ID 5a98e9cb437f7719afa2bde62de68e174fd8e03e
# Parent 8902674cc7fe759cada415c10340f31ae4a90fba
Stream: don't flush empty buffers created for read errors.
When we generate the last_buf buffer for an UDP upstream recv error, it does
not contain any data from the wire. ngx_stream_write_filter attempts to forward
it anyways, which is incorrect (e.g., UDP upstream ECONNREFUSED will be
translated to an empty packet).
Reproduction:
stream {
upstream unreachable {
server 127.0.0.1:8880;
}
server {
listen 127.0.0.1:8998 udp;
proxy_pass unreachable;
}
}
1 0.0127.0.0.1 → 127.0.0.1UDP 47 45588 → 8998 Len=5
2 0.000166300127.0.0.1 → 127.0.0.1UDP 47 51149 → 8880 Len=5
3 0.000172600127.0.0.1 → 127.0.0.1ICMP 75 Destination unreachable (Port
unreachable)
4 0.000202400127.0.0.1 → 127.0.0.1UDP 42 8998 → 45588 Len=0
Fixes d127837c714f.
diff --git a/src/stream/ngx_stream_proxy_module.c
b/src/stream/ngx_stream_proxy_module.c
--- a/src/stream/ngx_stream_proxy_module.c
+++ b/src/stream/ngx_stream_proxy_module.c
@@ -1676,7 +1676,7 @@ ngx_stream_proxy_process(ngx_stream_sess
ssize_t n;
ngx_buf_t*b;
ngx_int_t rc;
-ngx_uint_tflags, *packets;
+ngx_uint_tflags, flush, *packets;
ngx_msec_tdelay;
ngx_chain_t *cl, **ll, **out, **busy;
ngx_connection_t *c, *pc, *src, *dst;
@@ -1803,9 +1803,12 @@ ngx_stream_proxy_process(ngx_stream_sess
break;
}
+flush = 1;
+
if (n == NGX_ERROR) {
src->read->eof = 1;
n = 0;
+flush = 0;
}
if (n >= 0) {
@@ -1846,7 +1849,7 @@ ngx_stream_proxy_process(ngx_stream_sess
cl->buf->temporary = (n ? 1 : 0);
cl->buf->last_buf = src->read->eof;
-cl->buf->flush = 1;
+cl->buf->flush = flush;
(*packets)++;
*received += n;
___
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-le...@nginx.org
[PATCH] Resolver: make TCP write timer event cancelable.
# HG changeset patch # User Aleksei Bavshin # Date 1653579686 25200 # Thu May 26 08:41:26 2022 -0700 # Branch se # Node ID afb3646a08b6ebcf46fdbd2e7bf97cda774b # Parent 5a98e9cb437f7719afa2bde62de68e174fd8e03e Resolver: make TCP write timer event cancelable. In certain circumstances, this timer could become the only remaining non-cancelable timer in a worker, effectively delaying the worker shutdown. We're keeping the resolver alive to avoid disrupting any leftover tasks in the worker that may require up-to-date upstream peer data, but it is unnecessary in absence of other connections. And any useful workload would have its own non-cancelable timers, so it is safe to allow canceling this one. The scenario in question is a periodic resolver task with a period less than the TCP write timeout and responses large enough to warrant fallback to a TCP connection - something that could be reproduced with a dynamic upstream implementation. With each event loop wakeup, we either see a previously set write timer instance or schedule a new one. diff --git a/src/core/ngx_resolver.c b/src/core/ngx_resolver.c --- a/src/core/ngx_resolver.c +++ b/src/core/ngx_resolver.c @@ -1624,6 +1624,7 @@ ngx_resolver_send_tcp_query(ngx_resolver } rec->tcp->data = rec; +rec->tcp->write->cancelable = 1; rec->tcp->write->handler = ngx_resolver_tcp_write; rec->tcp->read->handler = ngx_resolver_tcp_read; rec->tcp->read->resolver = 1; ___ nginx-devel mailing list -- nginx-devel@nginx.org To unsubscribe send an email to nginx-devel-le...@nginx.org
RE: [PATCH] Resolver: make TCP write timer event cancelable.
> -Original Message- > From: Maxim Dounin > Sent: Thursday, May 26, 2022 3:31 PM > To: Aleksei Bavshin via nginx-devel > Subject: Re: [PATCH] Resolver: make TCP write timer event cancelable. > > EXTERNAL MAIL: nginx-devel-boun...@nginx.org > > Hello! > > On Thu, May 26, 2022 at 03:50:22PM +, Aleksei Bavshin via nginx-devel > wrote: > > > # HG changeset patch > > # User Aleksei Bavshin > > # Date 1653579686 25200 > > # Thu May 26 08:41:26 2022 -0700 > > # Branch se > > Looks like a wrong branch to me. Yep, I was already informed about that. As the main reproducer for the problem is the upstream code from 'se', I had to use this branch for verification and forgot to rebase. Sorry ☹ The change is in the oss portion of the code though and applies cleanly. I'll make sure to rebase any further patches on the right branch. > > > # Node ID afb3646a08b6ebcf46fdbd2e7bf97cda774b > > # Parent 5a98e9cb437f7719afa2bde62de68e174fd8e03e > > Resolver: make TCP write timer event cancelable. > > > > In certain circumstances, this timer could become the only remaining > > non-cancelable timer in a worker, effectively delaying the worker > shutdown. > > > > We're keeping the resolver alive to avoid disrupting any leftover tasks in > the > > worker that may require up-to-date upstream peer data, but it is > unnecessary > > in absence of other connections. And any useful workload would have its > own > > non-cancelable timers, so it is safe to allow canceling this one. > > This doesn't sound correct to me: when nginx is doing upstream > dynamic name-to-address resolution during request processing (see > ngx_http_upstream_init_request()), the request has no "own > non-cancelable timers". Sorry, I've likely used a wrong terminology here. An upstream server with 'resolve' parameter monitors the address changes independently from http request processing. There's an effort made to keep these resolver tasks cancelable (including 70e65bf8dfd7 you mentioned), while ensuring that the resolver connections are closed last - ngx_shutdown_timer_handler() ignores connections with resolver flag. As I understand the intention, we wanted to keep updating the upstream servers until the last http (stream, mail, etc) connection is closed and only then allow the worker to shut down. The non-cancelable timer in resolver interferes with this last part. I'll rewrite the description to make that clearer. > Still, there should be resolve task timer, and canceling the tcp > timer should be safe, similarly to canceling the resolver resend > timer, see 70e65bf8dfd7. And, BTW, it might be a good idea to > explicitly mention this changeset in the commit log. > > > > > The scenario in question is a periodic resolver task with a period less than > > the TCP write timeout and responses large enough to warrant fallback to a > TCP > > connection - something that could be reproduced with a dynamic upstream > > implementation. With each event loop wakeup, we either see a previously > > set write timer instance or schedule a new one. > > > > diff --git a/src/core/ngx_resolver.c b/src/core/ngx_resolver.c > > --- a/src/core/ngx_resolver.c > > +++ b/src/core/ngx_resolver.c > > @@ -1624,6 +1624,7 @@ ngx_resolver_send_tcp_query(ngx_resolver > > } > > > > rec->tcp->data = rec; > > +rec->tcp->write->cancelable = 1; > > rec->tcp->write->handler = ngx_resolver_tcp_write; > > rec->tcp->read->handler = ngx_resolver_tcp_read; > > rec->tcp->read->resolver = 1; > > From style point of view, it would be a good idea to set flags > after the handler. Ok, will do in v2. > > -- > Maxim Dounin > https://nam02.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmdo > unin.ru%2F&data=05%7C01%7CA.Bavshin%40F5.com%7C3c7d773d6815 > 4de9e63008da3f6790cd%7Cdd3dfd2f6a3b40d19be0bf8327d81c50%7C0%7C0 > %7C637892011303441223%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA > wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C > %7C&sdata=3G6yY5Ryx6a%2BXsQowcL0V%2F%2F217ByGKWKHAiVgalH > up0%3D&reserved=0 > ___ > nginx-devel mailing list -- nginx-devel@nginx.org > To unsubscribe send an email to nginx-devel-le...@nginx.org ___ nginx-devel mailing list -- nginx-devel@nginx.org To unsubscribe send an email to nginx-devel-le...@nginx.org
RE: [PATCH] Stream: don't flush empty buffers created for read errors.
> -Original Message-
> From: Maxim Dounin
> Sent: Wednesday, June 1, 2022 7:41 AM
> To: Aleksei Bavshin via nginx-devel
> Subject: Re: [PATCH] Stream: don't flush empty buffers created for read
> errors.
>
> EXTERNAL MAIL: nginx-devel-boun...@nginx.org
>
> Hello!
>
> On Tue, May 24, 2022 at 04:37:11PM +, Aleksei Bavshin via nginx-devel
> wrote:
>
> > In all honesty, the main motivation for the patch is to address
> > a regression in UDP healthchecks caused by the behavior
> > described below. It's already covered by tests, but the
> > corresponding cases were disabled by default.
> >
> > I considered
> > cl->buf->flush = !src->read->error;
> > or even
> > cl->buf->flush = !src->read->eof; but neither of those are
> > exactly equivalent to the code below (with the latter having
> > significantly different behavior), and IMO it would look
> > less obvious.
> >
> > # HG changeset patch
> > # User Aleksei Bavshin
> > # Date 1653330584 25200
> > # Mon May 23 11:29:44 2022 -0700
> > # Branch se
> > # Node ID 5a98e9cb437f7719afa2bde62de68e174fd8e03e
> > # Parent 8902674cc7fe759cada415c10340f31ae4a90fba
> > Stream: don't flush empty buffers created for read errors.
> >
> > When we generate the last_buf buffer for an UDP upstream recv error, it
> does
> > not contain any data from the wire. ngx_stream_write_filter attempts to
> forward
> > it anyways, which is incorrect (e.g., UDP upstream ECONNREFUSED will be
> > translated to an empty packet).
> >
> > Reproduction:
> >
> > stream {
> > upstream unreachable {
> > server 127.0.0.1:8880;
> > }
> > server {
> > listen 127.0.0.1:8998 udp;
> > proxy_pass unreachable;
> > }
> > }
> >
> > 1 0.0127.0.0.1 → 127.0.0.1UDP 47 45588 → 8998 Len=5
> > 2 0.000166300127.0.0.1 → 127.0.0.1UDP 47 51149 → 8880 Len=5
> > 3 0.000172600127.0.0.1 → 127.0.0.1ICMP 75 Destination unreachable
> (Port
> > unreachable)
> > 4 0.000202400127.0.0.1 → 127.0.0.1UDP 42 8998 → 45588 Len=0
> >
> > Fixes d127837c714f.
> >
> > diff --git a/src/stream/ngx_stream_proxy_module.c
> b/src/stream/ngx_stream_proxy_module.c
> > --- a/src/stream/ngx_stream_proxy_module.c
> > +++ b/src/stream/ngx_stream_proxy_module.c
> > @@ -1676,7 +1676,7 @@ ngx_stream_proxy_process(ngx_stream_sess
> > ssize_t n;
> > ngx_buf_t*b;
> > ngx_int_t rc;
> > -ngx_uint_tflags, *packets;
> > +ngx_uint_tflags, flush, *packets;
> > ngx_msec_tdelay;
> > ngx_chain_t *cl, **ll, **out, **busy;
> > ngx_connection_t *c, *pc, *src, *dst;
> > @@ -1803,9 +1803,12 @@ ngx_stream_proxy_process(ngx_stream_sess
> > break;
> > }
> >
> > +flush = 1;
> > +
> > if (n == NGX_ERROR) {
> > src->read->eof = 1;
> > n = 0;
> > +flush = 0;
> > }
> >
> > if (n >= 0) {
> > @@ -1846,7 +1849,7 @@ ngx_stream_proxy_process(ngx_stream_sess
> >
> > cl->buf->temporary = (n ? 1 : 0);
> > cl->buf->last_buf = src->read->eof;
> > -cl->buf->flush = 1;
> > +cl->buf->flush = flush;
> >
> > (*packets)++;
> > *received += n;
>
> I tend to think this patch is wrong, for at least two reasons:
>
> 1. It introduces a zero-sized non-special buffer in the chain.
> This is generally wrong, explicitly rejected in many code paths
> (see "zero size buf in ..." alerts), and might easily result in
> infinite loops in common processing patterns unless explicitly
> handled.
I think you misunderstood what the patch does. The buffer is guaranteed to be
special; it will have last_buf set just because it signals an unrecoverable
connection error.
The problem is that if *both* last_buf and flush are set, the buffer becomes
special enough to be sent to the wire even if there's no actual buffered data
(no packet to terminate).
An alternative approach would be to ignore the flush bit for a buffer with
last_buf set:
@@ -234,8 +234,8 @@ ngx_stream_write_filter(ngx_stream_sessi
if (size == 0
RE: [PATCH] Stream: don't flush empty buffers created for read errors.
> I don't think it's the way to go. Semantically, flush and
> last_buf are mutually exclusive: last_buf implies flushing of all
> remaining data, and there should be no flush set if last_buf is
> set.
>
> The "cl->buf->flush = !src->read->eof" approach might be actually
> better and more obvious: it simply ensures that last_buf and flush
> are not set at the same time. I don't immediately see any cases
> where it can break things, and this should fix the original
> problem with empty UDP packets being sent.
Thanks for confirming that, I wasn't sure it is the right approach exactly
because I had doubts regarding the mutual exclusivity of the flags.
Updated patch below; tests are passing on both branches and all systems we have
in CI.
# HG changeset patch
# User Aleksei Bavshin
# Date 1653330584 25200
# Mon May 23 11:29:44 2022 -0700
# Node ID 40926829be83986a3a5a5f941d2014000a0acd2e
# Parent e0cfab501dd11fdd23ad492419692269d3a01fc7
Stream: don't flush empty buffers created for read errors.
When we generate the last_buf buffer for an UDP upstream recv error, it does
not contain any data from the wire. ngx_stream_write_filter attempts to forward
it anyways, which is incorrect (e.g., UDP upstream ECONNREFUSED will be
translated to an empty packet).
This happens because we mark the buffer as both 'flush' and 'last_buf', and
ngx_stream_write_filter has special handling for flush with certain types of
connections (see d127837c714f, 32b0ba4855a6). The flags are meant to be
mutually exclusive, so the fix is to ensure that flush and last_buf are not set
at the same time.
Reproduction:
stream {
upstream unreachable {
server 127.0.0.1:8880;
}
server {
listen 127.0.0.1:8998 udp;
proxy_pass unreachable;
}
}
1 0.0127.0.0.1 → 127.0.0.1UDP 47 45588 → 8998 Len=5
2 0.000166300127.0.0.1 → 127.0.0.1UDP 47 51149 → 8880 Len=5
3 0.000172600127.0.0.1 → 127.0.0.1ICMP 75 Destination unreachable (Port
unreachable)
4 0.000202400127.0.0.1 → 127.0.0.1UDP 42 8998 → 45588 Len=0
Fixes d127837c714f.
diff --git a/src/stream/ngx_stream_proxy_module.c
b/src/stream/ngx_stream_proxy_module.c
--- a/src/stream/ngx_stream_proxy_module.c
+++ b/src/stream/ngx_stream_proxy_module.c
@@ -1735,7 +1735,7 @@ ngx_stream_proxy_process(ngx_stream_sess
cl->buf->temporary = (n ? 1 : 0);
cl->buf->last_buf = src->read->eof;
-cl->buf->flush = 1;
+cl->buf->flush = !src->read->eof;
(*packets)++;
*received += n;
___
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-le...@nginx.org
RE: [PATCH] Resolver: make TCP write timer event cancelable.
Updated patch, with maybe too verbose description. For the record, I did check a few similar modules I'm aware of (nginx-upstream-dynamic-servers, ngx_upstream_jdomain) and haven't found any affected code. So, the effect on third-party modules is hypothetical. # HG changeset patch # User Aleksei Bavshin # Date 1654139843 25200 # Wed Jun 01 20:17:23 2022 -0700 # Node ID 488f7a926ba54303c930b0b1e19c2b27e0af5079 # Parent 40926829be83986a3a5a5f941d2014000a0acd2e Resolver: make TCP write timer event cancelable. Similar to 70e65bf8dfd7, the change is made to ensure that the ability to cancel resolver tasks is fully controlled by the caller. As mentioned in the referenced commit, it is safe to make this timer cancelable because resolve tasks can have their own timeouts that are not cancelable. The scenario where this may become a problem is a periodic background resolve task (not tied to a specific request or a client connection), which receives a response with short TTL, large enough to warrant fallback to a TCP query. With each event loop wakeup, we either have a previously set write timer instance or schedule a new one. The non-cancelable write timer can delay or block graceful shutdown of a worker even if the ngx_resolver_ctx_t->cancelable flag is set by the API user, and there are no other tasks or connections. We use the resolver API in this way to maintain the list of upstream server addresses specified with the 'resolve' parameter, and there could be third-party modules implementing similar logic. diff --git a/src/core/ngx_resolver.c b/src/core/ngx_resolver.c --- a/src/core/ngx_resolver.c +++ b/src/core/ngx_resolver.c @@ -1389,6 +1389,7 @@ ngx_resolver_send_tcp_query(ngx_resolver rec->tcp->data = rec; rec->tcp->write->handler = ngx_resolver_tcp_write; +rec->tcp->write->cancelable = 1; rec->tcp->read->handler = ngx_resolver_tcp_read; rec->tcp->read->resolver = 1; ___ nginx-devel mailing list -- nginx-devel@nginx.org To unsubscribe send an email to nginx-devel-le...@nginx.org
[nginx] Stream: don't flush empty buffers created for read errors.
details: https://hg.nginx.org/nginx/rev/457afc332c67
branches:
changeset: 8044:457afc332c67
user: Aleksei Bavshin
date: Mon May 23 11:29:44 2022 -0700
description:
Stream: don't flush empty buffers created for read errors.
When we generate the last_buf buffer for an UDP upstream recv error, it does
not contain any data from the wire. ngx_stream_write_filter attempts to forward
it anyways, which is incorrect (e.g., UDP upstream ECONNREFUSED will be
translated to an empty packet).
This happens because we mark the buffer as both 'flush' and 'last_buf', and
ngx_stream_write_filter has special handling for flush with certain types of
connections (see d127837c714f, 32b0ba4855a6). The flags are meant to be
mutually exclusive, so the fix is to ensure that flush and last_buf are not set
at the same time.
Reproduction:
stream {
upstream unreachable {
server 127.0.0.1:8880;
}
server {
listen 127.0.0.1:8998 udp;
proxy_pass unreachable;
}
}
1 0.0127.0.0.1 → 127.0.0.1UDP 47 45588 → 8998 Len=5
2 0.000166300127.0.0.1 → 127.0.0.1UDP 47 51149 → 8880 Len=5
3 0.000172600127.0.0.1 → 127.0.0.1ICMP 75 Destination unreachable (Port
unreachable)
4 0.000202400127.0.0.1 → 127.0.0.1UDP 42 8998 → 45588 Len=0
Fixes d127837c714f.
diffstat:
src/stream/ngx_stream_proxy_module.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diffs (12 lines):
diff -r 1afd19dc7161 -r 457afc332c67 src/stream/ngx_stream_proxy_module.c
--- a/src/stream/ngx_stream_proxy_module.c Tue Jun 07 21:58:52 2022 +0300
+++ b/src/stream/ngx_stream_proxy_module.c Mon May 23 11:29:44 2022 -0700
@@ -1737,7 +1737,7 @@ ngx_stream_proxy_process(ngx_stream_sess
cl->buf->temporary = (n ? 1 : 0);
cl->buf->last_buf = src->read->eof;
-cl->buf->flush = 1;
+cl->buf->flush = !src->read->eof;
(*packets)++;
*received += n;
___
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-le...@nginx.org
[nginx] Resolver: make TCP write timer event cancelable.
details: https://hg.nginx.org/nginx/rev/aa28c802409f branches: changeset: 8045:aa28c802409f user: Aleksei Bavshin date: Wed Jun 01 20:17:23 2022 -0700 description: Resolver: make TCP write timer event cancelable. Similar to 70e65bf8dfd7, the change is made to ensure that the ability to cancel resolver tasks is fully controlled by the caller. As mentioned in the referenced commit, it is safe to make this timer cancelable because resolve tasks can have their own timeouts that are not cancelable. The scenario where this may become a problem is a periodic background resolve task (not tied to a specific request or a client connection), which receives a response with short TTL, large enough to warrant fallback to a TCP query. With each event loop wakeup, we either have a previously set write timer instance or schedule a new one. The non-cancelable write timer can delay or block graceful shutdown of a worker even if the ngx_resolver_ctx_t->cancelable flag is set by the API user, and there are no other tasks or connections. We use the resolver API in this way to maintain the list of upstream server addresses specified with the 'resolve' parameter, and there could be third-party modules implementing similar logic. diffstat: src/core/ngx_resolver.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diffs (11 lines): diff -r 457afc332c67 -r aa28c802409f src/core/ngx_resolver.c --- a/src/core/ngx_resolver.c Mon May 23 11:29:44 2022 -0700 +++ b/src/core/ngx_resolver.c Wed Jun 01 20:17:23 2022 -0700 @@ -1389,6 +1389,7 @@ ngx_resolver_send_tcp_query(ngx_resolver rec->tcp->data = rec; rec->tcp->write->handler = ngx_resolver_tcp_write; +rec->tcp->write->cancelable = 1; rec->tcp->read->handler = ngx_resolver_tcp_read; rec->tcp->read->resolver = 1; ___ nginx-devel mailing list -- nginx-devel@nginx.org To unsubscribe send an email to nginx-devel-le...@nginx.org
[PATCH] HTTP: add internal_redirect directive.
The directive in question provides direct access to the ngx_http_internal_redirect/ngx_http_named_location APIs, allowing to simplify and optimize several real-life configurations where the modules should be executed multiple times/in a different order/with a different configuration/etc, sometimes based on a condition evaluated during the request processing. Common examples include passing artifacts of one access module to another (auth_request -> auth_jwt) or deferring the enforcement of pre-access checks (limit_req/limit_conn) after the request is authorized. Most of that was already achievable with the existing tools, so here is a list of reasons for implementing a dedicated internal_redirect directive: * It offers a small, single purpose utility which is easier to document or find in the documentation. We often see configurations using double proxying instead of a redirect, just because the authors weren't aware of the simpler solution and weren't able to find it in the reference; it's not obvious that `try_files` can be used this way, and that's not the main purpose of the directive. The necessary clues are documented, but without highlighting the scenarios in question. It also may look like an abuse of the directive with some more workarounds on top. * There is a certain overhead in comparable configurations with the try_files directive: blocking filesystem access, which may introduce noticeable delays[^1], a few allocations and more code in general. * An ability of conditional execution[^2]. It is common enough with other configuration directives that an empty value means no operation. We can use that to implement the conditions that allow skipping the redirect and proceeding to the content phase handlers. * We already offer an access to the internal redirect through the njs or perl APIs. The directive allows expressing similar logic as a static configuration. * The existing directive has some unexpected configuration pitfalls. Case in point, `alias` directive in the same location may result in a prefix being stripped from the redirect target, but only if the parameter contains variables. The behavior may catch some users unaware (#97 and all the duplicates). [^1]: Which could be partially mitigated with `open_file_cache`. You'll need to read the source or have above average knowledge of the configuration to know that. [^2]: This can be added to the `try_files` as well, but it will further complicate the code and make already overloaded documentation even worse. With best regards, Aleksei nginx.patch Description: nginx.patch ___ nginx-devel mailing list -- nginx-devel@nginx.org To unsubscribe send an email to nginx-devel-le...@nginx.org
RE: [PATCH] HTTP: add internal_redirect directive.
Hello,
> > The directive in question provides direct access to the
> > ngx_http_internal_redirect/ngx_http_named_location APIs,
> > allowing to simplify and optimize several real-life
> > configurations where the modules should be executed multiple
> > times/in a different order/with a different configuration/etc,
> > sometimes based on a condition evaluated during the request
> > processing.
>
> No, thanks.
>
> Discussions about adding the "goto" directive date back to at
> least 2009. Igor's and my position on this are summarized here:
>
> https://mailman.nginx.org/pipermail/nginx-ru/2017-April/059736.html
>
> Hope this helps.
Thank you, I missed these threads while looking through the archives. I'm still
getting used to the vast amount of knowledge that is available only in Russian,
and sometimes forget to retry the search with a translated query.
So, Igor's main point was that a directive like that allows creating
unmaintainable configurations and will be heavily abused? It is a fair point
and it made me hesitate before submitting the patch. But there are already
roundabout ways of achieving the same and providing an official and
well-documented one may be beneficial.
I also wanted to point out that, unlike the previous requests, we're not
looking for a way to reduce duplication in the configuration. There are
genuine cases, where applying several location configs one by one is the most
optimal way to get a desired behavior. E.g., step-up authentication or reuse of
the result of another access module -- the existing methods of implementing
such configurations are complex and may negatively affect max RPS/request
processing time.
---
It doesn't have to be a redirect either; that was just the most direct
approach. The `error_page ...; return` idiom could work for these scenarios, if
we could have a conditional return-like directive processed at POST_ACCESS or
PRECONTENT. Although it would be less obvious and with its own share of
pitfalls.
location /protected {
...
includeregular_error_pages.inc;
error_page 418 = @extra_auth;
recursive_error_pages on;
post_auth_require $not_suspicious $var_from_subrequest ...
error=418; # semantics similar to auth_jwt_require
}
If I failed to convince you on the topic of redirect, would you be open to a
patch with that kind of directive?
___
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-le...@nginx.org
