Oh wonderful - something's come up that would have blocked me from working on this until next week, so thank you very much for updating it for me!
On Tue, Oct 20, 2015 at 3:07 PM, Baptiste <[email protected]> wrote: > 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 -- - Andrew Hayworth
