Hi Andrew, I've updated your patch quickly so Willy can integrate it. I've also updated the commit message to follow Lukas recommendations.
Baptiste On Tue, Oct 20, 2015 at 2:26 PM, Baptiste <[email protected]> wrote: > Hi Andrew, > > There is a bug repeated twice in your code. > In both dns_reset_resolution() and trigger_resolution(), you use > "resolution->resolver_family_priority" before it is positioned. This > may lead to using the last resolution->resolver_family_priority, which > may be different than the server one. > Please move the line "resolution->resolver_family_priority = > s->resolver_family_priority;" before using the value stored in it. > > Appart this, it looks good. > > Baptiste > > > On Tue, Oct 20, 2015 at 12:39 AM, Andrew Hayworth > <[email protected]> wrote: >> The ANY query type is weird, and some resolvers don't 'do the legwork' >> of resolving useful things like CNAMEs. Given that upstream resolver >> behavior is not always under the control of the HAProxy administrator, >> we should not use the ANY query type. Rather, we should use A or AAAA >> according to either the explicit preferences of the operator, or the >> implicit default (AAAA/IPv6). >> >> - Andrew Hayworth >> >> From 8ed172424cbd79197aacacd1fd89ddcfa46e213d Mon Sep 17 00:00:00 2001 >> From: Andrew Hayworth <[email protected]> >> Date: Mon, 19 Oct 2015 22:29:51 +0000 >> Subject: [PATCH] MEDIUM: dns: Don't use the ANY query type >> >> Basically, it's ill-defined and shouldn't really be used going forward. >> We can't guarantee that resolvers will do the 'legwork' for us and >> actually resolve CNAMES when we request the ANY query-type. Case in point >> (obfuscated, clearly): >> >> PRODUCTION! [email protected]:~$ >> dig @10.11.12.53 ANY api.somestartup.io >> >> ; <<>> DiG 9.8.4-rpz2+rl005.12-P1 <<>> @10.11.12.53 ANY api.somestartup.io >> ; (1 server found) >> ;; global options: +cmd >> ;; Got answer: >> ;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 62454 >> ;; flags: qr rd ra; QUERY: 1, ANSWER: 1, AUTHORITY: 4, ADDITIONAL: 0 >> >> ;; QUESTION SECTION: >> ;api.somestartup.io. IN ANY >> >> ;; ANSWER SECTION: >> api.somestartup.io. 20 IN CNAME >> api-somestartup-production.ap-southeast-2.elb.amazonaws.com. >> >> ;; AUTHORITY SECTION: >> somestartup.io. 166687 IN NS >> ns-1254.awsdns-28.org. >> somestartup.io. 166687 IN NS >> ns-1884.awsdns-43.co.uk. >> somestartup.io. 166687 IN NS ns-440.awsdns-55.com. >> somestartup.io. 166687 IN NS ns-577.awsdns-08.net. >> >> ;; Query time: 1 msec >> ;; SERVER: 10.11.12.53#53(10.11.12.53) >> ;; WHEN: Mon Oct 19 22:02:29 2015 >> ;; MSG SIZE rcvd: 242 >> >> HAProxy can't handle that response correctly. >> >> Rather than try to build in support for resolving CNAMEs presented >> without an A record in an answer section (which may be a valid >> improvement further on), this change just skips ANY record types >> altogether. A and AAAA are much more well-defined and predictable. >> >> Notably, this commit preserves the implicit "Prefer IPV6 behavior." >> --- >> include/types/dns.h | 3 ++- >> src/checks.c | 6 +++++- >> src/dns.c | 6 +++++- >> src/server.c | 18 +++++++----------- >> 4 files changed, 19 insertions(+), 14 deletions(-) >> >> diff --git a/include/types/dns.h b/include/types/dns.h >> index f8edb73..ea1a9f9 100644 >> --- a/include/types/dns.h >> +++ b/include/types/dns.h >> @@ -161,7 +161,8 @@ struct dns_resolution { >> unsigned int last_status_change; /* time of the latest DNS >> resolution status change */ >> int query_id; /* DNS query ID dedicated for this resolution */ >> struct eb32_node qid; /* ebtree query id */ >> - int query_type; /* query type to send. By default DNS_RTYPE_ANY */ >> + int query_type; >> + /* query type to send. By default DNS_RTYPE_A or DNS_RTYPE_AAAA >> depending on resolver_family_priority */ >> int status; /* status of the resolution being processed RSLV_STATUS_* */ >> int step; /* */ >> int try; /* current resolution try */ >> diff --git a/src/checks.c b/src/checks.c >> index ade2428..d3cd567 100644 >> --- a/src/checks.c >> +++ b/src/checks.c >> @@ -2214,7 +2214,11 @@ int trigger_resolution(struct server *s) >> resolution->query_id = query_id; >> resolution->qid.key = query_id; >> resolution->step = RSLV_STEP_RUNNING; >> - resolution->query_type = DNS_RTYPE_ANY; >> + if (resolution->resolver_family_priority == AF_INET) { >> + resolution->query_type = DNS_RTYPE_A; >> + } else { >> + resolution->query_type = DNS_RTYPE_AAAA; >> + } >> resolution->try = resolvers->resolve_retries; >> resolution->try_cname = 0; >> resolution->nb_responses = 0; >> diff --git a/src/dns.c b/src/dns.c >> index 7f71ac7..53b65ab 100644 >> --- a/src/dns.c >> +++ b/src/dns.c >> @@ -102,7 +102,11 @@ void dns_reset_resolution(struct dns_resolution >> *resolution) >> resolution->qid.key = 0; >> >> /* default values */ >> - resolution->query_type = DNS_RTYPE_ANY; >> + if (resolution->resolver_family_priority == AF_INET) { >> + resolution->query_type = DNS_RTYPE_A; >> + } else { >> + resolution->query_type = DNS_RTYPE_AAAA; >> + } >> >> /* the second resolution in the queue becomes the first one */ >> LIST_DEL(&resolution->list); >> diff --git a/src/server.c b/src/server.c >> index 8ddff00..33d6922 100644 >> --- a/src/server.c >> +++ b/src/server.c >> @@ -2692,21 +2692,13 @@ int snr_resolution_error_cb(struct >> dns_resolution *resolution, int error_code) >> case DNS_RESP_TRUNCATED: >> case DNS_RESP_ERROR: >> case DNS_RESP_NO_EXPECTED_RECORD: >> - qtype_any = resolution->query_type == DNS_RTYPE_ANY; >> res_preferred_afinet = resolution->resolver_family_priority == >> AF_INET && resolution->query_type == DNS_RTYPE_A; >> res_preferred_afinet6 = resolution->resolver_family_priority == >> AF_INET6 && resolution->query_type == DNS_RTYPE_AAAA; >> >> - if ((qtype_any || res_preferred_afinet || res_preferred_afinet6) >> + if ((res_preferred_afinet || res_preferred_afinet6) >> || (resolution->try > 0)) { >> /* let's change the query type */ >> - if (qtype_any) { >> - /* fallback from ANY to resolution preference */ >> - if (resolution->resolver_family_priority == AF_INET6) >> - resolution->query_type = DNS_RTYPE_AAAA; >> - else >> - resolution->query_type = DNS_RTYPE_A; >> - } >> - else if (res_preferred_afinet6) { >> + if (res_preferred_afinet6) { >> /* fallback from AAAA to A */ >> resolution->query_type = DNS_RTYPE_A; >> } >> @@ -2716,7 +2708,11 @@ int snr_resolution_error_cb(struct >> dns_resolution *resolution, int error_code) >> } >> else { >> resolution->try -= 1; >> - resolution->query_type = DNS_RTYPE_ANY; >> + if (resolution->resolver_family_priority == AF_INET) { >> + resolution->query_type = DNS_RTYPE_A; >> + } else { >> + resolution->query_type = DNS_RTYPE_AAAA; >> + } >> } >> >> dns_send_query(resolution); >> -- >> 2.1.3
From f16a2ed90f535b47fe73d0c80388d7767f96cbb5 Mon Sep 17 00:00:00 2001 From: Andrew Hayworth <[email protected]> Date: Mon, 19 Oct 2015 22:29:51 +0000 Subject: [PATCH] MEDIUM: dns: Don't use the ANY query type Basically, it's ill-defined and shouldn't really be used going forward. We can't guarantee that resolvers will do the 'legwork' for us and actually resolve CNAMES when we request the ANY query-type. Case in point (obfuscated, clearly): PRODUCTION! [email protected]:~$ dig @10.11.12.53 ANY api.somestartup.io ; <<>> DiG 9.8.4-rpz2+rl005.12-P1 <<>> @10.11.12.53 ANY api.somestartup.io ; (1 server found) ;; global options: +cmd ;; Got answer: ;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 62454 ;; flags: qr rd ra; QUERY: 1, ANSWER: 1, AUTHORITY: 4, ADDITIONAL: 0 ;; QUESTION SECTION: ;api.somestartup.io. IN ANY ;; ANSWER SECTION: api.somestartup.io. 20 IN CNAME api-somestartup-production.ap-southeast-2.elb.amazonaws.com. ;; AUTHORITY SECTION: somestartup.io. 166687 IN NS ns-1254.awsdns-28.org. somestartup.io. 166687 IN NS ns-1884.awsdns-43.co.uk. somestartup.io. 166687 IN NS ns-440.awsdns-55.com. somestartup.io. 166687 IN NS ns-577.awsdns-08.net. ;; Query time: 1 msec ;; SERVER: 10.11.12.53#53(10.11.12.53) ;; WHEN: Mon Oct 19 22:02:29 2015 ;; MSG SIZE rcvd: 242 HAProxy can't handle that response correctly. Rather than try to build in support for resolving CNAMEs presented without an A record in an answer section (which may be a valid improvement further on), this change just skips ANY record types altogether. A and AAAA are much more well-defined and predictable. Notably, this commit preserves the implicit "Prefer IPV6 behavior." Furthermore, ANY query type by default is a bad idea: (from Robin on HAProxy's ML): Using ANY queries for this kind of stuff is considered by most people to be a bad practice since besides all the things you named it can lead to incomplete responses. Basically a resolver is allowed to just return whatever it has in cache when it receives an ANY query instead of actually doing an ANY query at the authoritative nameserver. Thus if it only received queries for an A record before you do an ANY query you will not get an AAAA record even if it is actually available since the resolver doesn't have it in its cache. Even worse if before it only got MX queries, you won't get either A or AAAA --- include/types/dns.h | 3 ++- src/checks.c | 8 ++++++-- src/dns.c | 6 +++++- src/server.c | 20 ++++++++------------ 4 files changed, 21 insertions(+), 16 deletions(-) diff --git a/include/types/dns.h b/include/types/dns.h index f8edb73..ea1a9f9 100644 --- a/include/types/dns.h +++ b/include/types/dns.h @@ -161,7 +161,8 @@ struct dns_resolution { unsigned int last_status_change; /* time of the latest DNS resolution status change */ int query_id; /* DNS query ID dedicated for this resolution */ struct eb32_node qid; /* ebtree query id */ - int query_type; /* query type to send. By default DNS_RTYPE_ANY */ + int query_type; + /* query type to send. By default DNS_RTYPE_A or DNS_RTYPE_AAAA depending on resolver_family_priority */ int status; /* status of the resolution being processed RSLV_STATUS_* */ int step; /* */ int try; /* current resolution try */ diff --git a/src/checks.c b/src/checks.c index ade2428..997cf81 100644 --- a/src/checks.c +++ b/src/checks.c @@ -2214,11 +2214,15 @@ int trigger_resolution(struct server *s) resolution->query_id = query_id; resolution->qid.key = query_id; resolution->step = RSLV_STEP_RUNNING; - resolution->query_type = DNS_RTYPE_ANY; + resolution->resolver_family_priority = s->resolver_family_priority; + if (resolution->resolver_family_priority == AF_INET) { + resolution->query_type = DNS_RTYPE_A; + } else { + resolution->query_type = DNS_RTYPE_AAAA; + } resolution->try = resolvers->resolve_retries; resolution->try_cname = 0; resolution->nb_responses = 0; - resolution->resolver_family_priority = s->resolver_family_priority; eb32_insert(&resolvers->query_ids, &resolution->qid); dns_send_query(resolution); diff --git a/src/dns.c b/src/dns.c index 7f71ac7..53b65ab 100644 --- a/src/dns.c +++ b/src/dns.c @@ -102,7 +102,11 @@ void dns_reset_resolution(struct dns_resolution *resolution) resolution->qid.key = 0; /* default values */ - resolution->query_type = DNS_RTYPE_ANY; + if (resolution->resolver_family_priority == AF_INET) { + resolution->query_type = DNS_RTYPE_A; + } else { + resolution->query_type = DNS_RTYPE_AAAA; + } /* the second resolution in the queue becomes the first one */ LIST_DEL(&resolution->list); diff --git a/src/server.c b/src/server.c index 8ddff00..dcc5961 100644 --- a/src/server.c +++ b/src/server.c @@ -2668,7 +2668,7 @@ int snr_resolution_error_cb(struct dns_resolution *resolution, int error_code) { struct server *s; struct dns_resolvers *resolvers; - int qtype_any, res_preferred_afinet, res_preferred_afinet6; + int res_preferred_afinet, res_preferred_afinet6; /* shortcut to the server whose name is being resolved */ s = (struct server *)resolution->requester; @@ -2692,21 +2692,13 @@ int snr_resolution_error_cb(struct dns_resolution *resolution, int error_code) case DNS_RESP_TRUNCATED: case DNS_RESP_ERROR: case DNS_RESP_NO_EXPECTED_RECORD: - qtype_any = resolution->query_type == DNS_RTYPE_ANY; res_preferred_afinet = resolution->resolver_family_priority == AF_INET && resolution->query_type == DNS_RTYPE_A; res_preferred_afinet6 = resolution->resolver_family_priority == AF_INET6 && resolution->query_type == DNS_RTYPE_AAAA; - if ((qtype_any || res_preferred_afinet || res_preferred_afinet6) + if ((res_preferred_afinet || res_preferred_afinet6) || (resolution->try > 0)) { /* let's change the query type */ - if (qtype_any) { - /* fallback from ANY to resolution preference */ - if (resolution->resolver_family_priority == AF_INET6) - resolution->query_type = DNS_RTYPE_AAAA; - else - resolution->query_type = DNS_RTYPE_A; - } - else if (res_preferred_afinet6) { + if (res_preferred_afinet6) { /* fallback from AAAA to A */ resolution->query_type = DNS_RTYPE_A; } @@ -2716,7 +2708,11 @@ int snr_resolution_error_cb(struct dns_resolution *resolution, int error_code) } else { resolution->try -= 1; - resolution->query_type = DNS_RTYPE_ANY; + if (resolution->resolver_family_priority == AF_INET) { + resolution->query_type = DNS_RTYPE_A; + } else { + resolution->query_type = DNS_RTYPE_AAAA; + } } dns_send_query(resolution); -- 2.5.0
