[Dnsmasq-discuss] [PATCH] fix dns failover when dns server returns REFUSED

[Hans Dedecker]

If a DNS server replies REFUSED for a given DNS query in strict order mode
no failover to the next DNS server is triggered as the logic in reply_query
excluded strict order mode by mistake.

Also checking for not strict order mode makes the failover logic related
to REFUSED death code as it also checks for forwardall being 0 which can
only be the case for strict order mode.

Fix this by checking for strict order mode now so the failover logic in
case REFUSED is replied is triggered in case forwardall is 0 for a given
forward record. In case all servers mode is configured the fail over logic
won't be triggered just as before.

Signed-off-by: Hans Dedecker <***@gmail.com>
Signed-off-by: Mi Feng <***@gmail.com>
---

Fixes dns failover issue reported in LEDE (https://bugs.lede-project.org/index.php?do=details&task_id=841)

src/forward.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/forward.c b/src/forward.c
index 83f392d..0ce3612 100644
--- a/src/forward.c
+++ b/src/forward.c
@@ -790,7 +790,7 @@ void reply_query(int fd, int family, time_t now)
/* Note: if we send extra options in the EDNS0 header, we can't recreate
the query from the reply. */
if (RCODE(header) == REFUSED &&
- !option_bool(OPT_ORDER) &&
+ option_bool(OPT_ORDER) &&
forward->forwardall == 0 &&
!(forward->flags & FREC_HAS_EXTRADATA))
/* for broken servers, attempt to send to another one. */

[Kevin Darbyshire-Bryant]

This seems like an important fix to get in the next 'patch' release or
whatever it's to be called, a bit like the pxe filename whoops :-)

Remarkably simple fix too...hopefully not too simple.

Cheers,

Kevin

Post by Hans Dedecker
If a DNS server replies REFUSED for a given DNS query in strict order mode
no failover to the next DNS server is triggered as the logic in reply_query
excluded strict order mode by mistake.
Also checking for not strict order mode makes the failover logic related
to REFUSED death code as it also checks for forwardall being 0 which can
only be the case for strict order mode.
Fix this by checking for strict order mode now so the failover logic in
case REFUSED is replied is triggered in case forwardall is 0 for a given
forward record. In case all servers mode is configured the fail over logic
won't be triggered just as before.
---
Fixes dns failover issue reported in LEDE (https://bugs.lede-project.org/index.php?do=details&task_id=841)
src/forward.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/forward.c b/src/forward.c
index 83f392d..0ce3612 100644
--- a/src/forward.c
+++ b/src/forward.c
@@ -790,7 +790,7 @@ void reply_query(int fd, int family, time_t now)
/* Note: if we send extra options in the EDNS0 header, we can't recreate
the query from the reply. */
if (RCODE(header) == REFUSED &&
- !option_bool(OPT_ORDER) &&
+ option_bool(OPT_ORDER) &&
forward->forwardall == 0 &&
!(forward->flags & FREC_HAS_EXTRADATA))
/* for broken servers, attempt to send to another one. */

[Simon Kelley]

Post by Hans Dedecker
If a DNS server replies REFUSED for a given DNS query in strict order mode
no failover to the next DNS server is triggered as the logic in reply_query
excluded strict order mode by mistake.

The above may well be true.

Post by Hans Dedecker
Also checking for not strict order mode makes the failover logic related
to REFUSED death code as it also checks for forwardall being 0 which can
only be the case for strict order mode.

but this is not true. In non-strict-order mode, the query gets forwarded
to a single server (forwardall == 0) and is the query gets resent from
the client after timeout, then it gets sent to all servers, and
forwardall != 0

Post by Hans Dedecker
Fix this by checking for strict order mode now so the failover logic in
case REFUSED is replied is triggered in case forwardall is 0 for a given
forward record. In case all servers mode is configured the fail over logic
won't be triggered just as before.

The patch now inhibits sending the query to all other servers when
strict-order is NOT set. I think it makes more sense to just delete the
option_bool(OPT_ORDER) condition completely.


Cheers,

Simon.

Post by Hans Dedecker
---
Fixes dns failover issue reported in LEDE (https://bugs.lede-project.org/index.php?do=details&task_id=841)
src/forward.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/forward.c b/src/forward.c
index 83f392d..0ce3612 100644
--- a/src/forward.c
+++ b/src/forward.c
@@ -790,7 +790,7 @@ void reply_query(int fd, int family, time_t now)
/* Note: if we send extra options in the EDNS0 header, we can't recreate
the query from the reply. */
if (RCODE(header) == REFUSED &&
- !option_bool(OPT_ORDER) &&
+ option_bool(OPT_ORDER) &&
forward->forwardall == 0 &&
!(forward->flags & FREC_HAS_EXTRADATA))
/* for broken servers, attempt to send to another one. */

[Hans Dedecker]

Post by Simon Kelley

Post by Hans Dedecker
If a DNS server replies REFUSED for a given DNS query in strict order mode
no failover to the next DNS server is triggered as the logic in reply_query
excluded strict order mode by mistake.

The above may well be true.

Post by Hans Dedecker
Also checking for not strict order mode makes the failover logic related
to REFUSED death code as it also checks for forwardall being 0 which can
only be the case for strict order mode.

but this is not true. In non-strict-order mode, the query gets forwarded
to a single server (forwardall == 0) and is the query gets resent from
the client after timeout, then it gets sent to all servers, and
forwardall != 0

Thanks for the explanation regarding the non strict order mode logic;
it was not completely clear to me when I made the patch how non strict
order mode behaved precisely. I will respin the patch based on this.

Post by Simon Kelley

Post by Hans Dedecker
Fix this by checking for strict order mode now so the failover logic in
case REFUSED is replied is triggered in case forwardall is 0 for a given
forward record. In case all servers mode is configured the fail over logic
won't be triggered just as before.

The patch now inhibits sending the query to all other servers when
strict-order is NOT set. I think it makes more sense to just delete the
option_bool(OPT_ORDER) condition completely.

The strict order mode for dnsmasq on routers is used quite a lot as
ISPs have often configured primary and secondary DNS servers in their
network. They want the primary DNS server to be contacted first; only
in case of timeout or other error condition like refused fallback to
the secondary DNS server(s) is requested.
Most ISPs don't like all DNS servers being contacted for a given
client query as they perceive it as dns traffic duplication; in the
world of routers strict order mode has still its use.


Hans

Post by Simon Kelley
Cheers,
Simon.

Post by Hans Dedecker
---
Fixes dns failover issue reported in LEDE (https://bugs.lede-project.org/index.php?do=details&task_id=841)
src/forward.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/forward.c b/src/forward.c
index 83f392d..0ce3612 100644
--- a/src/forward.c
+++ b/src/forward.c
@@ -790,7 +790,7 @@ void reply_query(int fd, int family, time_t now)
/* Note: if we send extra options in the EDNS0 header, we can't recreate
the query from the reply. */
if (RCODE(header) == REFUSED &&
- !option_bool(OPT_ORDER) &&
+ option_bool(OPT_ORDER) &&
forward->forwardall == 0 &&
!(forward->flags & FREC_HAS_EXTRADATA))
/* for broken servers, attempt to send to another one. */

[Simon Kelley]

Post by Hans Dedecker

Post by Simon Kelley

Post by Hans Dedecker
If a DNS server replies REFUSED for a given DNS query in strict order mode
no failover to the next DNS server is triggered as the logic in reply_query
excluded strict order mode by mistake.

The above may well be true.

Post by Hans Dedecker
Also checking for not strict order mode makes the failover logic related
to REFUSED death code as it also checks for forwardall being 0 which can
only be the case for strict order mode.

but this is not true. In non-strict-order mode, the query gets forwarded
to a single server (forwardall == 0) and is the query gets resent from
the client after timeout, then it gets sent to all servers, and
forwardall != 0

Thanks for the explanation regarding the non strict order mode logic;
it was not completely clear to me when I made the patch how non strict
order mode behaved precisely. I will respin the patch based on this.

Post by Simon Kelley

Post by Hans Dedecker
Fix this by checking for strict order mode now so the failover logic in
case REFUSED is replied is triggered in case forwardall is 0 for a given
forward record. In case all servers mode is configured the fail over logic
won't be triggered just as before.

The patch now inhibits sending the query to all other servers when
strict-order is NOT set. I think it makes more sense to just delete the
option_bool(OPT_ORDER) condition completely.

The strict order mode for dnsmasq on routers is used quite a lot as
ISPs have often configured primary and secondary DNS servers in their
network. They want the primary DNS server to be contacted first; only
in case of timeout or other error condition like refused fallback to
the secondary DNS server(s) is requested.
Most ISPs don't like all DNS servers being contacted for a given
client query as they perceive it as dns traffic duplication; in the
world of routers strict order mode has still its use.

I think you misunderstood my point, I don't want to remove the
--strict-order option, just to remove that line of code completely
(which is what your V2 patch does - I shall apply it right now.)


Cheers,

Simon.

Post by Hans Dedecker
Hans

Post by Simon Kelley
Cheers,
Simon.

Post by Hans Dedecker
---
Fixes dns failover issue reported in LEDE (https://bugs.lede-project.org/index.php?do=details&task_id=841)
src/forward.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/forward.c b/src/forward.c
index 83f392d..0ce3612 100644
--- a/src/forward.c
+++ b/src/forward.c
@@ -790,7 +790,7 @@ void reply_query(int fd, int family, time_t now)
/* Note: if we send extra options in the EDNS0 header, we can't recreate
the query from the reply. */
if (RCODE(header) == REFUSED &&
- !option_bool(OPT_ORDER) &&
+ option_bool(OPT_ORDER) &&
forward->forwardall == 0 &&
!(forward->flags & FREC_HAS_EXTRADATA))
/* for broken servers, attempt to send to another one. */