Re: [PATCH] dns: Handle SRV record weights correctly

2018-01-09 Thread Willy Tarreau
On Tue, Jan 09, 2018 at 03:39:29PM +0100, Olivier Houchard wrote:
> Updated patch attached.

cool, now applied, thanks!
Willy



Re: [PATCH] dns: Handle SRV record weights correctly

2018-01-09 Thread Olivier Houchard
Hi,

On Tue, Jan 09, 2018 at 03:28:22PM +0100, Olivier Houchard wrote:
> Hi Willy,
> 
> On Tue, Jan 09, 2018 at 03:17:24PM +0100, Willy Tarreau wrote:
> > Hi Olivier,
> > 
> > On Mon, Jan 08, 2018 at 04:35:35PM +0100, Olivier Houchard wrote:
> > > Hi,
> > > 
> > > The attached patch attempts to map SRV record weight to haproxy weight 
> > > correctly,
> > > SRV weight goes from 0 to 65536 while haproxy uses 0 to 256, so we have to
> > > divide it by 256, and a SRV weight of 0 doesn't mean the server shouldn't 
> > > be
> > > used, so we use a minimum weight of 1.
> > 
> > From what I'm seeing in the code, it's 0..65535 for the SRV record. And
> > that allows us to simplify it and use the full range of the weight like
> > this :
> > 
> >hap_weight = srv_weight / 256 + 1;
> > 
> > => 0..255 return 1
> >1..511 return 2
> >...
> >65280..65535 return 256
> > 
> > What do you think ?
> > 
> > Willy
> > 
> 
> Sure, sounds good, for some reason I thought the max was 255, but it's
> actually 256, one day I'll learn how to read C.
> 

Updated patch attached.

Regards,

Olivier
>From 7043d8da312605b2876286c95a2c0c53d6bd43e5 Mon Sep 17 00:00:00 2001
From: Olivier Houchard 
Date: Mon, 8 Jan 2018 16:28:57 +0100
Subject: [PATCH] MINOR: dns: Handle SRV record weight correctly.

A SRV record weight can range from 0 to 65535, while haproxy weight goes
from 0 to 255, so we have to divide it by 256 before handing it to haproxy.
Also, a SRV record with a weight of 0 doesn't mean the server shouldn't be
used, so use a minimum weight of 1.

This should probably be backported to 1.8.
---
 include/types/dns.h |  2 +-
 src/dns.c   | 19 ---
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/include/types/dns.h b/include/types/dns.h
index b1f068a61..9b1d08df7 100644
--- a/include/types/dns.h
+++ b/include/types/dns.h
@@ -143,7 +143,7 @@ struct dns_answer_item {
int16_t class; /* query class */
int32_t ttl;   /* response TTL */
int16_t priority;  /* SRV type priority */
-   int16_t weight;/* SRV type weight */
+   uint16_tweight;/* SRV type weight */
int16_t port;  /* SRV type port */
int16_t data_len;  /* number of bytes in target 
below */
struct sockaddr address;   /* IPv4 or IPv6, network 
format */
diff --git a/src/dns.c b/src/dns.c
index fceef2e48..a957710ed 100644
--- a/src/dns.c
+++ b/src/dns.c
@@ -522,10 +522,16 @@ static void dns_check_dns_response(struct dns_resolution 
*res)
if (srv->srvrq == srvrq && srv->svc_port == 
item->port &&
item->data_len == srv->hostname_dn_len &&
!memcmp(srv->hostname_dn, item->target, 
item->data_len)) {
-   if (srv->uweight != item->weight) {
+   int ha_weight;
+
+   /* Make sure weight is at least 1, so
+* that the server will be used.
+*/
+   ha_weight = item->weight / 256 + 1;
+   if (srv->uweight != ha_weight) {
char weight[9];
 
-   snprintf(weight, 
sizeof(weight), "%d", item->weight);
+   snprintf(weight, 
sizeof(weight), "%d", ha_weight);

server_parse_weight_change_request(srv, weight);
}
HA_SPIN_UNLOCK(SERVER_LOCK, &srv->lock);
@@ -547,6 +553,7 @@ static void dns_check_dns_response(struct dns_resolution 
*res)
if (srv) {
const char *msg = NULL;
char weight[9];
+   int ha_weight;
char hostname[DNS_MAX_NAME_SIZE];
 
if (dns_dn_label_to_str(item->target, 
item->data_len+1,
@@ -563,7 +570,13 @@ static void dns_check_dns_response(struct dns_resolution 
*res)
if ((srv->check.state & CHK_ST_CONFIGURED) &&
!(srv->flags & SRV_F_CHECKPORT))
srv->check.port = item->port;
-   snprintf(weight, sizeof(weight), "%d", 
item->weight);
+
+   /* Make sure weight is at least 1, so
+* that the server will be used.
+*/
+   ha_weight = item->weight / 256 

Re: [PATCH] dns: Handle SRV record weights correctly

2018-01-09 Thread Willy Tarreau
On Tue, Jan 09, 2018 at 03:28:22PM +0100, Olivier Houchard wrote:
> Hi Willy,
> 
> On Tue, Jan 09, 2018 at 03:17:24PM +0100, Willy Tarreau wrote:
> > Hi Olivier,
> > 
> > On Mon, Jan 08, 2018 at 04:35:35PM +0100, Olivier Houchard wrote:
> > > Hi,
> > > 
> > > The attached patch attempts to map SRV record weight to haproxy weight 
> > > correctly,
> > > SRV weight goes from 0 to 65536 while haproxy uses 0 to 256, so we have to
> > > divide it by 256, and a SRV weight of 0 doesn't mean the server shouldn't 
> > > be
> > > used, so we use a minimum weight of 1.
> > 
> > From what I'm seeing in the code, it's 0..65535 for the SRV record. And
> > that allows us to simplify it and use the full range of the weight like
> > this :
> > 
> >hap_weight = srv_weight / 256 + 1;
> > 
> > => 0..255 return 1
> >1..511 return 2
> >...
> >65280..65535 return 256
> > 
> > What do you think ?
> > 
> > Willy
> > 
> 
> Sure, sounds good, for some reason I thought the max was 255, but it's
> actually 256, one day I'll learn how to read C.

I can only recommend you this one, which I hope will also help me write
less bugs in haproxy once I finish it :

http://www.c-for-dummies.com/

Willy



Re: [PATCH] dns: Handle SRV record weights correctly

2018-01-09 Thread Olivier Houchard
Hi Willy,

On Tue, Jan 09, 2018 at 03:17:24PM +0100, Willy Tarreau wrote:
> Hi Olivier,
> 
> On Mon, Jan 08, 2018 at 04:35:35PM +0100, Olivier Houchard wrote:
> > Hi,
> > 
> > The attached patch attempts to map SRV record weight to haproxy weight 
> > correctly,
> > SRV weight goes from 0 to 65536 while haproxy uses 0 to 256, so we have to
> > divide it by 256, and a SRV weight of 0 doesn't mean the server shouldn't be
> > used, so we use a minimum weight of 1.
> 
> From what I'm seeing in the code, it's 0..65535 for the SRV record. And
> that allows us to simplify it and use the full range of the weight like
> this :
> 
>hap_weight = srv_weight / 256 + 1;
> 
> => 0..255 return 1
>1..511 return 2
>...
>65280..65535 return 256
> 
> What do you think ?
> 
> Willy
> 

Sure, sounds good, for some reason I thought the max was 255, but it's
actually 256, one day I'll learn how to read C.

Regards,

Olivier



Re: [PATCH] dns: Handle SRV record weights correctly

2018-01-09 Thread Willy Tarreau
Hi Olivier,

On Mon, Jan 08, 2018 at 04:35:35PM +0100, Olivier Houchard wrote:
> Hi,
> 
> The attached patch attempts to map SRV record weight to haproxy weight 
> correctly,
> SRV weight goes from 0 to 65536 while haproxy uses 0 to 256, so we have to
> divide it by 256, and a SRV weight of 0 doesn't mean the server shouldn't be
> used, so we use a minimum weight of 1.

>From what I'm seeing in the code, it's 0..65535 for the SRV record. And
that allows us to simplify it and use the full range of the weight like
this :

   hap_weight = srv_weight / 256 + 1;

=> 0..255 return 1
   1..511 return 2
   ...
   65280..65535 return 256

What do you think ?

Willy