Re: [PATCHES] Major DNS changes

[Willy Tarreau]

Hi Baptiste,

On Wed, May 24, 2017 at 11:34:31AM +0200, Baptiste wrote:
> Over the last few weeks, I entirely reworked the internal resolver of
> HAProxy to make it more flexible.
> The main driver for this is to add more features related to DNS use-cases
> (SRV records, scale in / scale out a backend, DNS converter, etc...) and
> also to make it more efficient internaly and more friendly with DNS servers
> ;)
(...)

I have a few comments :

1) The following construct to copy single characters :
memcpy([len], "#", 1);
len += 1;

is better done this way :

str[len++] = '#';

I'm not changing it, it's not in a performance critical path.

2) patch 8 introduces a one-byte overflow in dns_cache_key() :
+   if (len + hostname_dn_len > size)
+   return NULL;
+   memcpy([len], hostname_dn, hostname_dn_len);
+   len += hostname_dn_len;

Here len may be equal to size.

+   str[len] = '\0';

and here it's overwritten.

+   return buf;

I'm changing it this way :

+   if (len + hostname_dn_len + 1 > size) // +1 for trailing zero
+   return NULL;

3) in patch 10, there's a strange construct :

+ return_DNS_UPD_SRVIP_NOT_FOUND:
+   list_for_each_entry(record, _p->answer_list, list) {
+   /* move the first record to the end of the list, for internal 
round robin */
+   if (record) {
+   LIST_DEL(>list);
+   LIST_ADDQ(_p->answer_list, >list);
+   break;
+   }
+   }
+   return DNS_UPD_SRVIP_NOT_FOUND;

I *think* you did this only to be sure to have a first element, but
that's not appropriate. A few points to keep in mind :
  - list_for_each_entry() must not be used in cases where the element
is deleted. list_for_each_entry_safe() must be used for this. It
just happens to work in your case because you quit after the first
change, but this is very bug-prone.

  - "if (record)" will always be true since it's the item carrying the
list you stopped on.

So I'm not totally sure about the intent there. I'm leaving it as-is
for now but I'd prefer it if you could send an update for this one. If
you just want to delete the first record and put it at the end only
when it exists, you can for example do this :

if (!LIST_ISEMPTY(_p->answer_list)) {
struct list *first = dns_p->anwser_list->n;
LIST_DEL(first);
LIST_ADDQ(_p->answer_list, first);
}

4) in patch 11, dns_dump_config() is not used anymore and looks like
debugging code as it dumps pointers. Once all the changes are done,
don't forget either to kill it, or to enclose it between #ifdef if 
you prefer to keep it around.

By the way, I don't want to be nit-picking, but :

+   /* generates a query id */
+   i = 0;
+   do {
+   query_id = dns_rnd16();
+   /* we do try only 100 times to find a free query id */
+   if (i++ > 100) {
(...)

That's 101 times and not 100 :-)

Otherwise at first glance it looks good, I've merged your series.

thanks!
Willy

Re: [PATCHES] Major DNS changes

[Aleksandar Lazic]

Hi Baptiste.

Baptiste  have written on Mon, 29 May 2017 11:14:14
+0200:

> Hi Aleksandar,
> 
> I have take a look into the code and have just some questions about
> > calloc in [PATCH 03/11] & [PATCH 07/11]
> >
> > In the function dns_alloc_resolution is calloc used, would the use
> > of haproxy pools bring any benefit?
> >
> >  
> it may help a bit from memory usage point of view.
> I planned to use them, but since I was blocking other devs, I used the
> quickest and safest way for now.
> Let say I'll improve this for HAProxy 1.9, unless I find some time
> before 1.8 is ready.

Thanks for info.

> > Do I have understand the code right that you have per resolver a
> > cache and this resolver belongs to a backend/frontend.
> >
> >  
> A cache belong to a "resolvers" section. Many servers (for now) or any
> requester (later) can use the same "resolvers", hence benefit from the
> cached responses.
>
> > So in case several backends have the same server names all resolver
> > requests this server individual.
> > There is no 'global dns cache' for all or I missed something.
> >
> >  
> No.
> Take a look at this configuration:
> 
>resolvers mydns
> ...
>frontend ft
> ...
> 
>backend b1
> server s1 myappsrv1.domain.com:80 resolvers mydns   # no need
> "check" anymore :)
> 
>backend b2
> server s1 myappsrv1.domain.com:80 resolvers mydns   # no need
> "check" anymore :)
> 
> Both b1/s1 and b2/s1 points to the same resolvers and one will
> benefit from the cached response of the other one.
> 
> There is still one point though, the cache is per resolvers, but (for
> now) based on family preference. Soon, Olivier or I will improve this
> by enforcing the resolvers to perform both A and  queries and
> cache both response and let the requester pick-up the one he wants.
> For now, the cache only stores the response of the latest query...

thanks. got the point.

> Baptiste
> 
> 
> >  
> > > Please give it a try and report any issues you may spot :)
> > >
> > > Baptiste  
> >
> > Regards
> > Aleks
> >  
> 

Re: [PATCHES] Major DNS changes

[Baptiste]

Hi Aleksandar,

I have take a look into the code and have just some questions about
> calloc in [PATCH 03/11] & [PATCH 07/11]
>
> In the function dns_alloc_resolution is calloc used, would the use of
> haproxy pools bring any benefit?
>
>
it may help a bit from memory usage point of view.
I planned to use them, but since I was blocking other devs, I used the
quickest and safest way for now.
Let say I'll improve this for HAProxy 1.9, unless I find some time before
1.8 is ready.



> [PATCH 08/11]
> +static int dns_cache_size = 1024;   /* arbitrary DNS cache size */
>
> For the future maybe there could be a glolbal.tune.dns-cache-size or
> something similar.
>

Ah yes!
I take the point :)


>
> Do I have understand the code right that you have per resolver a cache
> and this resolver belongs to a backend/frontend.
>
>
A cache belong to a "resolvers" section. Many servers (for now) or any
requester (later) can use the same "resolvers", hence benefit from the
cached responses.



> So in case several backends have the same server names all resolver
> requests this server individual.
> There is no 'global dns cache' for all or I missed something.
>
>
No.
Take a look at this configuration:

   resolvers mydns
...
   frontend ft
...

   backend b1
server s1 myappsrv1.domain.com:80 resolvers mydns   # no need "check"
anymore :)

   backend b2
server s1 myappsrv1.domain.com:80 resolvers mydns   # no need "check"
anymore :)

Both b1/s1 and b2/s1 points to the same resolvers and one will benefit from
the cached response of the other one.

There is still one point though, the cache is per resolvers, but (for now)
based on family preference. Soon, Olivier or I will improve this by
enforcing the resolvers to perform both A and  queries and cache both
response and let the requester pick-up the one he wants.
For now, the cache only stores the response of the latest query...

Baptiste


>
> > Please give it a try and report any issues you may spot :)
> >
> > Baptiste
>
> Regards
> Aleks
>

Re: [PATCHES] Major DNS changes

[Aleksandar Lazic]

Hi Baptiste.

Baptiste have written on Wed, 24 May 2017 11:34:31 +0200:

> Hi all,
> 
> Over the last few weeks, I entirely reworked the internal resolver of
> HAProxy to make it more flexible.
> The main driver for this is to add more features related to DNS
> use-cases (SRV records, scale in / scale out a backend, DNS
> converter, etc...) and also to make it more efficient internaly and
> more friendly with DNS servers ;)
>
> With this in mind, I performed the following changes:
> - DNS tasks are now autonomous, they are not triggered by the health
> check any more, this means we can enable DNS resolution without
> enabling health monitoring on a server
> - DNS responses are now kept in memory, when they are not an error...
> so many servers using the same hostname resolution will use a
> response in the cache if it is fresh enough
> - full "anonymisation" of the requesters. Up to now, DNS code was
> though for servers only (well layers were pretty well defined). I
> added an abstraction layer between the resolution and the requester,
> so the requester could be of any HAProxy internal type (backend,
> bind, ...).
> - management of requester through queues: 2 queues are available: the
> run queue and the wait queue. If a requester needs a resolution, he
> will register himself to the run queue, and if he is the first one,
> then the resolution is triggered
> - a pool of resolution is now linked to a resolvers section. Memory is
> allocated at configuration parsing time. If the pool is too small for
> your configuration, an error is returned.
> 
> I still have a few minor things to work on, but since HAProxy Tech
> guys needs my code to move forward on their contribution, I'm
> publishing it right now.

Wow great work.

I have take a look into the code and have just some questions about
calloc in [PATCH 03/11] & [PATCH 07/11]

In the function dns_alloc_resolution is calloc used, would the use of
haproxy pools bring any benefit?

[PATCH 08/11]
+static int dns_cache_size = 1024;   /* arbitrary DNS cache size */

For the future maybe there could be a glolbal.tune.dns-cache-size or
something similar.

Do I have understand the code right that you have per resolver a cache
and this resolver belongs to a backend/frontend.

So in case several backends have the same server names all resolver
requests this server individual.
There is no 'global dns cache' for all or I missed something.


> Please give it a try and report any issues you may spot :)
> 
> Baptiste

Regards
Aleks