Re: [Dnsmasq-discuss] [PATCH] fix dns failover when dns server returns REFUSED
On 26/06/17 14:17, Hans Dedecker wrote: > On Sun, Jun 25, 2017 at 12:15 AM, Simon Kelley > wrote: >> On 14/06/17 14:46, Hans Dedecker wrote: >>> 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. >>> >>> 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. >>> >>> 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. > > > Hans >> >> >> Cheers, >> >> Simon. >> >> >>> Signed-off-by: Hans Dedecker >>> Signed-off-by: Mi Feng >>> --- >>> >>> 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. */ >>> >> > signature.asc Description: OpenPGP digital signature ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] [PATCH] fix dns failover when dns server returns REFUSED
On Sun, Jun 25, 2017 at 12:15 AM, Simon Kelley wrote: > On 14/06/17 14:46, Hans Dedecker wrote: >> 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. >> >> 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. >> >> 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 > > > Cheers, > > Simon. > > >> Signed-off-by: Hans Dedecker >> Signed-off-by: Mi Feng >> --- >> >> 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. */ >> > ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] [PATCH] fix dns failover when dns server returns REFUSED
On 14/06/17 14:46, Hans Dedecker wrote: > 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. > > 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 > > 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. > Signed-off-by: Hans Dedecker > Signed-off-by: Mi Feng > --- > > 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. */ > ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] [PATCH] fix dns failover when dns server returns REFUSED
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 On 14/06/17 14:46, Hans Dedecker wrote: 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 Signed-off-by: Mi Feng --- 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. */ ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
[Dnsmasq-discuss] [PATCH] fix dns failover when dns server returns REFUSED
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 Signed-off-by: Mi Feng --- 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. */ -- 1.9.1 ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
