BUG/MINOR: resolvers: Restore round-robin selection on records in DNS answers

Since the commit dcb696cd3 ("MEDIUM: resolvers: hash the records before
inserting them into the tree"), When several records are found in a DNS
answer, the round robin selection over these records is no longer performed.

Indeed, before a list of records was used. To ensure each records was
selected one after the other, at each selection, the first record of the
list was moved at the end. When this list was replaced bu a tree, the same
mechanism was preserved. However, the record is indexed using its key, a
hash of the record. So its position never changes. When it is removed and
reinserted in the tree, its position remains the same. When we walk though
the tree, starting from the root, the records are always evaluated in the
same order. So, even if there are several records in a DNS answer, the same
IP address is always selected.

It is quite easy to trigger the issue with a do-resolv action.

To fix the issue, the node to perform the next selection is now saved. So
instead of restarting from the root each time, we can restart from the next
node of the previous call.

Thanks to Damien Claisse for the issue analysis and for the reproducer.

This patch should fix the issue #3116. It must be backported as far as 2.6.

diff --git a/include/haproxy/resolvers-t.h b/include/haproxy/resolvers-t.h
index 914b375301382cc4fc7366813fe0d1593377af5c..7c411b3db2e33919c3a94c4d43412f7488499981 100644 (file)
--- a/include/haproxy/resolvers-t.h
+++ b/include/haproxy/resolvers-t.h
@@ -133,6 +133,7 @@ struct resolv_answer_item {
 struct resolv_response {
        struct dns_header header;
        struct eb_root    answer_tree;
+       struct eb32_node *next; /* node to start eval on the next lookup to perform a round robin selection on entries (may be NULL) */
        /* authority ignored for now */
 };
 

diff --git a/src/resolvers.c b/src/resolvers.c
index 7753422a9a9798a672ed1018ad5985a29039b6f6..a42ae9133738184b2570a0193042e0d7d727d793 100644 (file)
--- a/src/resolvers.c
+++ b/src/resolvers.c
@@ -1614,7 +1614,7 @@ int resolv_get_ip_from_response(struct resolv_response *r_res,
                              struct server *owner)
 {
        struct resolv_answer_item *record, *found_record = NULL;
-       struct eb32_node *eb32;
+       struct eb32_node *eb32, *end;
        int family_priority;
        int currentip_found;
        unsigned char *newip4, *newip6;
@@ -1647,11 +1647,16 @@ int resolv_get_ip_from_response(struct resolv_response *r_res,
         *  1 - current ip.
         * The result with the biggest score is returned.
         */
-
-       for (eb32 = eb32_first(&r_res->answer_tree); eb32 != NULL;  eb32 = eb32_next(eb32)) {
+       eb32 = (!r_res->next) ? eb32_first(&r_res->answer_tree) : r_res->next;
+       end = r_res->next;
+       r_res->next = eb32_next(eb32); /* get node for the next lookup */
+       do {
                void *ip;
                unsigned char ip_type;
 
+               if (eb32 == NULL)
+                       eb32 = eb32_first(&r_res->answer_tree);
+
                record = eb32_entry(eb32, typeof(*record), link);
                if (record->type == DNS_RTYPE_A && (resolv_active_families() & RSLV_ACCEPT_IPV4)) {
                        ip_type = AF_INET;
@@ -1662,7 +1667,7 @@ int resolv_get_ip_from_response(struct resolv_response *r_res,
                        ip = &record->data.in6.sin6_addr;
                }
                else
-                       continue;
+                       goto next;
                score = 0;
 
                /* Check for preferred ip protocol. */
@@ -1674,7 +1679,7 @@ int resolv_get_ip_from_response(struct resolv_response *r_res,
 
                        /* Compare only the same addresses class. */
                        if (resolv_opts->pref_net[j].family != ip_type)
-                               continue;
+                               goto next;
 
                        if ((ip_type == AF_INET &&
                             in_net_ipv4(ip,
@@ -1698,7 +1703,7 @@ int resolv_get_ip_from_response(struct resolv_response *r_res,
 
                        list_for_each_entry(srv, &record->attached_servers, ip_rec_item) {
                                if (srv == owner)
-                                       continue;
+                                       goto next;
                                if (srv->proxy == owner->proxy) {
                                        already_used = 1;
                                        break;
@@ -1706,7 +1711,7 @@ int resolv_get_ip_from_response(struct resolv_response *r_res,
                        }
                        if (already_used) {
                                if (!allowed_duplicated_ip) {
-                                       continue;
+                                       goto next;
                                }
                        }
                        else {
@@ -1748,7 +1753,9 @@ int resolv_get_ip_from_response(struct resolv_response *r_res,
                        }
                        max_score = score;
                }
-       } /* list for each record entries */
+         next:
+               eb32 = eb32_next(eb32);
+       } while (eb32 != end); /* list for each record entries */
 
        /* No IP found in the response */
        if (!newip4 && !newip6)
@@ -1794,15 +1801,6 @@ int resolv_get_ip_from_response(struct resolv_response *r_res,
                LIST_APPEND(&found_record->attached_servers, &owner->ip_rec_item);
        }
 
-       eb32 = eb32_first(&r_res->answer_tree);
-       if (eb32) {
-               /* Move the first record to the end of the list, for internal
-                * round robin.
-                */
-               eb32_delete(eb32);
-               eb32_insert(&r_res->answer_tree, eb32);
-       }
-
        return (currentip_found ? RSLV_UPD_NO : RSLV_UPD_SRVIP_NOT_FOUND);
 }
 
@@ -1973,6 +1971,7 @@ static struct resolv_resolution *resolv_pick_resolution(struct resolvers *resolv
 
                LIST_INIT(&res->requesters);
                res->response.answer_tree = EB_ROOT;
+               res->response.next = NULL;
 
                res->prefered_query_type = query_type;
                res->query_type          = query_type;