MINOR: map: mapfile ordering also matters for tree-based match types

Willy made me realize that tree-based matching may also suffer from
out-of-order mapfile loading, as opposed to what's being said in
b546bb6d ("BUG/MINOR: map: list-based matching potential ordering
regression") and the associated REGTEST.

Indeed, in case of duplicated keys, we want to be sure that only the key
that was first seen in the file will be returned (as long as it is not
removed). The above fix is still valid, and the list-based match regtest
will also prevent regressions for tree-based match since mapfile loading
logic is currently match-type agnostic.

But let's clarify that by making both the code comment and the regtest
more precise.

diff --git a/reg-tests/http-rules/map_ordering.map b/reg-tests/http-rules/map_ordering.map
index dcd952955c0d81a9dcd04d8ff764fdb103f25302..dc9ac71bd08f160ce96b0ac820e0b97f4932106e 100644 (file)
--- a/reg-tests/http-rules/map_ordering.map
+++ b/reg-tests/http-rules/map_ordering.map
@@ -2,3 +2,5 @@
 first.domain.tld first
 domain.tld domain
 second.domain.tld second
+# This entry is used to test duplicate behavior (ie: tree-based match)
+first.domain.tld first_dup

diff --git a/reg-tests/http-rules/map_ordering.vtc b/reg-tests/http-rules/map_ordering.vtc
index 40da465cae27433d1f30692f4a9dfdce5686c313..923d19fa5837401fb3c9c01fa32dba8f98fc262d 100644 (file)
--- a/reg-tests/http-rules/map_ordering.vtc
+++ b/reg-tests/http-rules/map_ordering.vtc
@@ -1,4 +1,4 @@
-varnishtest "Test list-based matching types ordering"
+varnishtest "Ensure mapfile ordering is preserved when loading the file"
 feature cmd "$HAPROXY_PROGRAM -cc 'version_atleast(2.5-dev0)'"
 feature ignore_unknown_macro
 
@@ -14,9 +14,13 @@ haproxy h1 -conf {
 
     # check list ordering using map_dom (list-based match)
     http-request return hdr dom %[req.hdr(Host),lower,map_dom(${testdir}/map_ordering.map)] if { url_beg /dom }
+
+    # check tree ordering using map_str (tree-based match) and duplicated keys
+    http-request return hdr str %[req.hdr(Host),lower,map_str(${testdir}/map_ordering.map)] if { url_beg /str }
+
 } -start
 
-# Check map ordering
+# Check map ordering for list-based matching types
 client c1 -connect ${h1_fe1_sock} {
     # first.domain.tld is above domain.tld so it should match first
     txreq -url "/dom" -hdr "Host: first.domain.tld"
@@ -30,3 +34,13 @@ client c1 -connect ${h1_fe1_sock} {
     expect resp.status == 200
     expect resp.http.dom == "domain"
 } -run
+
+# Check map ordering for tree-based matching types (check that the matching
+# key is the first one seen in the file)
+client c2 -connect ${h1_fe1_sock} {
+    # first.domain.tld is first mapped to "first" in the mapfile
+    txreq -url "/str" -hdr "Host: first.domain.tld"
+    rxresp
+    expect resp.status == 200
+    expect resp.http.str == "first"
+} -run

diff --git a/src/pattern.c b/src/pattern.c
index 57ef92320bb11ff788e3c626c45f8eafa8409220..f07223f7c349b76632b16bee5b4ac73b9b173590 100644 (file)
--- a/src/pattern.c
+++ b/src/pattern.c
@@ -2512,7 +2512,10 @@ int pattern_read_from_file(struct pattern_head *head, unsigned int refflags,
 
        /* Load reference content in the pattern expression.
         * We need to load elements in the same order they were seen in the
-        * file as list-based matching types may rely on it.
+        * file. Indeed, some list-based matching types may rely on it as the
+        * list is positional, and for tree-based matching, even if the tree is
+        * content-based in case of duplicated keys we only want the first key
+        * in the file to be considered.
         */
        list_for_each_entry(elt, &ref->head, list) {
                if (!pat_ref_push(elt, expr, patflags, err)) {