Just a few questions and minor comments below : On Fri, Aug 04, 2017 at 06:49:43PM +0200, Olivier Houchard wrote: > This also adds support for SRV records. To use them, simply use a SRV label > instead of a hostname on the server line, ie : > server s1 _http._tcp.example.com resolvers dns check > server s2 _http._tcp.example.com resolvers dns check > > When this is done, haproxy will first resolve _http._tcp.example.com, and then > give the hostname (as well as port and weight) to each available server, that > will then do a regular DNS resolution to get the IP.
What makes the distinction between an SRV record and a real hostname here ? Just the leading underscore, or the plain "_http." maybe ? I'm not expecting any problem with this given that the underscore is not allowed as a regular hostname character (except under windows). But at least this will deserve a mention in the doc where the server's address is described, so that anyone experiencing trouble could spot this easily. > From 1b408464590fea38d8a45b2b7fed5c615465a858 Mon Sep 17 00:00:00 2001 > From: Olivier Houchard <[email protected]> > Date: Thu, 6 Jul 2017 18:46:47 +0200 > Subject: [PATCH 1/4] MINOR: dns: Cache previous DNS answers. > > As DNS servers may not return all IPs in one answer, we want to cache the > previous entries. Those entries are removed when considered obsolete, which > happens when the IP hasn't been returned by the DNS server for a time > defined in the "hold obsolete" parameter of the resolver section. The default > is 30s. > --- > doc/configuration.txt | 7 +- > include/proto/server.h | 2 +- > include/types/dns.h | 9 +- > src/cfgparse.c | 5 +- > src/dns.c | 247 > ++++++++++++++++++++++++++++--------------------- > src/server.c | 28 ++++-- > 6 files changed, 175 insertions(+), 123 deletions(-) > > diff --git a/doc/configuration.txt b/doc/configuration.txt > index bfeb3ce0..f4674387 100644 > --- a/doc/configuration.txt > +++ b/doc/configuration.txt > @@ -11693,6 +11693,10 @@ For example, with 2 name servers configured in a > resolvers section: > - first response is truncated and second one is a NX Domain, then HAProxy > stops resolution. > > +As a DNS server may not answer all the IPs in one DNS request, haproxy keeps > +a cache of previous answers, an answer will be considered obsolete after > +"hold obsolete" seconds without the IP returned. > + > > resolvers <resolvers id> > Creates a new name server list labelled <resolvers id> > @@ -11709,7 +11713,7 @@ hold <status> <period> > Defines <period> during which the last name resolution should be kept based > on last resolution <status> > <status> : last name resolution status. Acceptable values are "nx", > - "other", "refused", "timeout", "valid". > + "other", "refused", "timeout", "valid", "obsolete". > <period> : interval between two successive name resolution when the last > answer was in <status>. It follows the HAProxy time format. > <period> is in milliseconds by default. > @@ -11756,6 +11760,7 @@ timeout <event> <time> > hold nx 30s > hold timeout 30s > hold valid 10s > + hold obsolete 30s > > > 6. HTTP header manipulation > diff --git a/include/proto/server.h b/include/proto/server.h > index 43e4e425..c4f8e1d5 100644 > --- a/include/proto/server.h > +++ b/include/proto/server.h > @@ -52,7 +52,7 @@ int srv_init_addr(void); > struct server *cli_find_server(struct appctx *appctx, char *arg); > > /* functions related to server name resolution */ > -int snr_update_srv_status(struct server *s); > +int snr_update_srv_status(struct server *s, int has_no_ip); > int snr_resolution_cb(struct dns_requester *requester, struct dns_nameserver > *nameserver); > int snr_resolution_error_cb(struct dns_requester *requester, int error_code); > struct server *snr_check_ip_callback(struct server *srv, void *ip, unsigned > char *ip_family); > diff --git a/include/types/dns.h b/include/types/dns.h > index 7a19aa37..12c11552 100644 > --- a/include/types/dns.h > +++ b/include/types/dns.h > @@ -113,7 +113,7 @@ struct dns_query_item { > /* NOTE: big endian structure */ > struct dns_answer_item { > struct list list; > - char *name; /* answer name > + char name[DNS_MAX_NAME_SIZE]; /* answer name Do you have an estimate of the worst case increase of memory usage incurred by using the max name size for every name component ? I understand that it might not be possible to use a shared area anymore for all entries once you start to deal with obsolescence, but it's just to get an idea of what the worst DNS response could have as impact. > @@ -124,7 +124,8 @@ struct dns_answer_item { > int16_t port; /* SRV type port */ > int16_t data_len; /* number of bytes in target > below */ > struct sockaddr address; /* IPv4 or IPv6, network format > */ > - char *target; /* Response data: SRV or CNAME > type target */ > + char target[DNS_MAX_NAME_SIZE]; /* Response data: SRV or CNAME > type target */ Same here. > @@ -1011,30 +1027,29 @@ int dns_validate_dns_response(unsigned char *resp, > unsigned char *bufend, struct > } > > /* now parsing response records */ > - LIST_INIT(&dns_p->answer_list); > nb_saved_records = 0; > - for (dns_answer_record_id = 0; dns_answer_record_id < > dns_p->header.ancount; dns_answer_record_id++) { > + for (int i = 0; i < dns_p->header.ancount; i++) { "for (int i ...)" will not build here, it's only C99 and we don't enforce this (and personally I hate this form as it's the only exception I know of where a variable declaration is used outside of the scope it's declared in. > @@ -1046,43 +1061,50 @@ int dns_validate_dns_response(unsigned char *resp, > unsigned char *bufend, struct > > } > > - dns_answer_record->name = chunk_newstr(dns_response_buffer); > - if (dns_answer_record->name == NULL) > - return DNS_RESP_INVALID; > - > - ret = chunk_strncat(dns_response_buffer, tmpname, len); > - if (ret == 0) > - return DNS_RESP_INVALID; > + memcpy(dns_answer_record->name, tmpname, len); > + dns_answer_record->name[len] = 0; I'm just realizing that when I merge the minimalist indirect string manipulation functions I'm using for HTTP/2, it will simplify such code, as dealing all the time with memcpy() and memcmp() for strings is a pain. It will also give me ideas of what to improve there before it represents a perceptible benefit. We'll probably see this for 1.9. > From a2e3bba11a5c1a3c69e9f33c566d7c285c9121f2 Mon Sep 17 00:00:00 2001 > From: Olivier Houchard <[email protected]> > Date: Fri, 4 Aug 2017 18:35:36 +0200 > Subject: [PATCH 3/4] MINOR: dns: Handle SRV records. > > Make it so for each server, instead of specifying a hostname, one can use > a SRV label. > When doing so, haproxy will first resolve the SRV label, then use the > resulting hostnames, as well as port and weight (priority is ignored right > now), to each server using the SRV label. > It is resolved periodically, and any server disappearing from the SRV records > will be removed, and any server appearing will be added, assuming there're > free servers in haproxy. > --- > include/proto/dns.h | 1 + > include/proto/server.h | 1 + > include/types/dns.h | 16 +++ > include/types/proxy.h | 1 + > include/types/server.h | 1 + > src/cfgparse.c | 22 +++- > src/dns.c | 313 > ++++++++++++++++++++++++++++++++++++++++++------- > src/proxy.c | 1 + > src/server.c | 96 ++++++++++++--- > 9 files changed, 395 insertions(+), 57 deletions(-) > (...) > diff --git a/src/dns.c b/src/dns.c > index 0ce63c91..2931752a 100644 > --- a/src/dns.c > +++ b/src/dns.c > @@ -478,14 +497,104 @@ void dns_resolve_recv(struct dgram_conn *dgram) > break; (...) > + /* Check if a server already uses that hostname > */ > + for (srv = srvrq->proxy->srv; srv != NULL; srv > = srv->next) { > + if (srv->srvrq == srvrq && > + item1->data_len == > srv->hostname_dn_len && > + !memcmp(srv->hostname_dn, > item1->target, item1->data_len) && > + srv->svc_port == item1->port) { > + if (srv->uweight != > item1->weight) { > + char weight[9]; > + > + snprintf(weight, > sizeof(weight), > + "%d", > item1->weight); > + > server_parse_weight_change_request(srv, weight); Next time, you can use ultoa() as a replacement for snprintf(str, size, "%d"), which also ensures you'll use a large enough string for the largest supported integer types, there's also U2A() which uses a rotating buffer and which can be used multiple times in a single function call. > + reader += sizeof(uint16_t); > + memcpy(&dns_answer_record->weight, > + reader, sizeof(uint16_t)); > + dns_answer_record->weight = > ntohs(dns_answer_record->weight); I guess there's an alignment issue there, am I right ? I think we'll have to add some easier conversion functions, like unaligned reads using something like this to make such manipulation less of a pain : static inline uint16_t readu16(const void *p) { union { const void *p; uint16_t u16; } __attribute__((packed)) u = { .p = p }; return u.u16; } static inline uint16_t readbe16(const void *p) { return ((unsigned char *)p)[0] << 8 + ((unsigned char *)p)[1]; } The the code above simply becomes : reader += sizeof(uint16_t); dns_answer_record->weight = readbe16(reader); and for cases where native-endian word is needed instead : reader += sizeof(uint16_t); dns_answer_record->weight = readu16(reader); > @@ -1635,6 +1801,27 @@ int dns_build_query(int query_id, int query_type, char > *hostname_dn, int hostnam > return ptr - buf; > } > > +/* Turn a domain name label into a string */ > +void dns_dn_label_to_str(char *dn, char *str, int dn_len) > +{ > + int remain_size = 0; > + > + for (int i = 0; i < dn_len; i++) { Note, "for (int i" here as well. > From 899413f3a965feeaa96ffb5b284b504dbdb01fa1 Mon Sep 17 00:00:00 2001 > From: Olivier Houchard <[email protected]> > Date: Fri, 4 Aug 2017 18:39:01 +0200 > Subject: [PATCH 4/4] MINOR: check: Fix checks when using SRV records. > > When started, a server may not yet have an associated protocol, so don't > bother trying to run the checks until it is there. > --- > src/checks.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/checks.c b/src/checks.c > index 7938b873..fc92a243 100644 > --- a/src/checks.c > +++ b/src/checks.c > @@ -1557,7 +1557,7 @@ static int connect_conn_chk(struct task *t) > } > > ret = SF_ERR_INTERNAL; > - if (proto->connect) > + if (proto && proto->connect) > ret = proto->connect(conn, check->type, quickack ? 2 : 0); > if (s->check.send_proxy && !(check->state & CHK_ST_AGENT)) { > conn->send_proxy_ofs = 1; Do you think this one might be triggered before SRV records ? If so we'd need to tag it for backporting. If you're unsure we should also backport it, of course :-) Thanks! Willy
