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
<andrew.haywo...@getbraintree.com> 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 <andrew.haywo...@getbraintree.com>
> 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! ahaywo...@secret-hostname.com:~$
> 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
