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
