BUG/MEDIUM: filters: order the implicit filters around the explicit ones

In the legacy filter mode, flt_reorder_implicit_conf() reordered the
implicitly declared core filters but pinned the explicitly declared core
filters in their declaration position, treating them as segment
boundaries. An implicit filter bucketed after an explicit core filter
ended up in a wrong position: with an explicit "filter compression"
declaration followed by cache rules, the implicit cache-store filter
was placed after the compression filters, and the configuration checks
reported bogus errors:

    proxy 'li': unable to enable the compression filter on response
                before any cache filter.
    proxy 'li': unable to enable the compression filter before the
                cache 'c'.

while this configuration is valid: the implicit cache filter must be
placed at its canonical position, before the explicit compression
filters.

Explicitly declared core filters are now placed in the "any other
filter" class, like any explicitly declared filter: their manual
placement among the explicit filters is the user's responsibility, and
only the implicitly declared core filters are moved to their canonical
position. The placement policy is implemented in flt_order_class(),
which returns the "any other filter" class for an explicit core filter.

The order checks reporting conflicts between the compression,
decompression and cache filters are updated accordingly: an order
error is only reported when the filters involved are explicitly
declared. A filter that is automatically ordered never conflicts with
an explicitly declared one, since the reordering places them correctly
relative to each other.

No backport needed.

diff --git a/src/cache.c b/src/cache.c
index 81284897476bf0889cd423412e762b5473bd67de..834bb3720082ef3154516222bfe265e4ecd1e3c7 100644 (file)
--- a/src/cache.c
+++ b/src/cache.c
@@ -708,14 +708,22 @@ static int cache_store_check_order(struct flt_conf *f, void *data)
        if (f->id == http_comp_req_flt_id)
                return 0;
        if (f->id == http_comp_res_flt_id) {
-               ctx->comp_seen = 1;
+               /* Only report an explicitly declared compression filter
+                * placed before the cache: the implicit ones are
+                * automatically ordered after it.
+                */
+               if (f->flags & FLT_CFG_FL_EXPLICIT)
+                       ctx->comp_seen = 1;
                return 0;
        }
        if (f->id == decomp_req_flt_id)
                return 0;
        else if (f->id == decomp_res_flt_id) {
-               /* The decomp-res filter must be evaluated after the cache. */
-               if (ctx->cache_seen) {
+               /* Only report an explicitly declared decomp-res filter placed
+                * after an explicitly declared cache: the implicit filters
+                * are automatically ordered.
+                */
+               if ((f->flags & FLT_CFG_FL_EXPLICIT) && (ctx->fconf->flags & FLT_CFG_FL_EXPLICIT) && ctx->cache_seen) {
                        ha_alert("config: %s '%s': unable to enable the decomp-res filter after "
                                 "the cache '%s'.\n", proxy_type_str(ctx->px), ctx->px->id, ctx->cache->id);
                        return 1;

diff --git a/src/filters.c b/src/filters.c
index a48a822c3b103e3b534a81a0981efc390b67b298..11188feb6b12039d77a25a42e965c8e93b13814b 100644 (file)
--- a/src/filters.c
+++ b/src/filters.c
@@ -716,8 +716,12 @@ static int flt_is_core_flt(const struct flt_conf *fconf)
  *   response: fcgi > decomp-res > cache-store > any other filter >
  *             decomp-req > http-comp-res
  *
- * Explicitly declared core filters are never moved and act as segment
- * boundaries (see flt_reorder_implicit_conf()).
+ * Explicitly declared core filters are placed in the "any other filter"
+ * class, like any explicitly declared filter: their manual placement among
+ * the explicit filters is the user's responsibility, and a conflicting
+ * order between explicitly declared filters is reported by their checks.
+ * Only the implicitly declared core filters are moved to their canonical
+ * position.
  */
 static int flt_order_class(const struct flt_conf *fconf, unsigned int side)
 {
@@ -730,6 +734,17 @@ static int flt_order_class(const struct flt_conf *fconf, unsigned int side)
        int is_fcgi = (fconf->id == fcgi_flt_id);
 #endif
 
+       if ((fconf->flags & FLT_CFG_FL_EXPLICIT) && flt_is_core_flt(fconf)) {
+               switch (side) {
+               case FLT_SIDE_REQ:
+                       return 1; /* any other filter */
+               case FLT_SIDE_RES:
+                       return 3; /* any other filter */
+               default:
+                       return 2; /* any other filter */
+               }
+       }
+
        switch (side) {
        case FLT_SIDE_REQ:
                if (is_decomp_req)
@@ -805,10 +820,12 @@ static void flt_flush_order_segs(struct list *out, struct list seg[], int nb_seg
  * px->filters_res_configs) are used to fill the stream's request and response
  * filter lists.
  *
- * Explicitly declared core filters are never moved, they keep their declaration
- * order. They delimit segments of filters inside which the implicitly declared
- * core filters are sorted, so that an implicit filter never crosses an
- * explicitly declared one.
+ * Explicitly declared core filters are placed in the "any other filter"
+ * class, like any explicitly declared filter: their manual placement
+ * between the explicit filters is the user's responsibility, and the
+ * conflicting orders between explicitly declared filters are reported by
+ * their checks, which walk the lists before the reordering. Only the
+ * implicitly declared core filters are moved to their canonical position.
  */
 static void flt_reorder_implicit_conf(struct proxy *px)
 {
@@ -821,47 +838,24 @@ static void flt_reorder_implicit_conf(struct proxy *px)
                        LIST_INIT(&seg[s][i]);
        }
 
-       /* First, the global order, with the 'list' member. */
-       LIST_INIT(&out);
-       list_for_each_entry_safe(fconf, back, &px->filter_configs, list) {
-               if ((fconf->flags & FLT_CFG_FL_EXPLICIT) && flt_is_core_flt(fconf)) {
-                       /* Explicitly declared core filter: flush the current
-                        * segment before it and keep the filter in place.
-                        */
-                       flt_flush_order_segs(&out, seg[0], 6);
-                       LIST_DEL_INIT(&fconf->list);
-                       LIST_APPEND(&out, &fconf->list);
-                       continue;
-               }
-               LIST_DEL_INIT(&fconf->list);
-               LIST_APPEND(&seg[0][flt_order_class(fconf, FLT_SIDE_GLOBAL)], &fconf->list);
-       }
-       flt_flush_order_segs(&out, seg[0], 6);
-       LIST_SPLICE(&px->filter_configs, &out);
-
-       /* Then the request and response orders, from the final global order,
-        * with the 'by_req' and 'by_res' members. Both lists are rebuilt from
-        * scratch: reset their heads, all the links were initialized by
-        * flt_conf_add()/flt_conf_insert() or above.
+       /* Rebuild the three orders in one pass over the declaration order:
+        * bucket the 'list', 'by_req' and 'by_res' members of every filter
+        * config by its class, then flush the buckets in class order. Within
+        * a class, the configs keep their declaration order.
         */
+       LIST_INIT(&out);
        LIST_INIT(&px->filters_req_configs);
        LIST_INIT(&px->filters_res_configs);
        list_for_each_entry_safe(fconf, back, &px->filter_configs, list) {
-               if ((fconf->flags & FLT_CFG_FL_EXPLICIT) && flt_is_core_flt(fconf)) {
-                       /* Explicitly declared core filter: flush the current
-                        * segments before it and keep the filter in place.
-                        */
-                       flt_flush_order_segs(&px->filters_req_configs, seg[1], 6);
-                       flt_flush_order_segs(&px->filters_res_configs, seg[2], 6);
-                       LIST_APPEND(&px->filters_req_configs, &fconf->by_req);
-                       LIST_APPEND(&px->filters_res_configs, &fconf->by_res);
-                       continue;
-               }
+               LIST_DEL_INIT(&fconf->list);
+               LIST_APPEND(&seg[0][flt_order_class(fconf, FLT_SIDE_GLOBAL)], &fconf->list);
                LIST_APPEND(&seg[1][flt_order_class(fconf, FLT_SIDE_REQ)], &fconf->by_req);
                LIST_APPEND(&seg[2][flt_order_class(fconf, FLT_SIDE_RES)], &fconf->by_res);
        }
+       flt_flush_order_segs(&out, seg[0], 6);
        flt_flush_order_segs(&px->filters_req_configs, seg[1], 6);
        flt_flush_order_segs(&px->filters_res_configs, seg[2], 6);
+       LIST_SPLICE(&px->filter_configs, &out);
 }
 
 /*

diff --git a/src/flt_decomp.c b/src/flt_decomp.c
index 8c1823439e9eefd78c4e4d49e8859f34a153926d..56de068d4b3fd1339d6eb948f0fd0416e8852671 100644 (file)
--- a/src/flt_decomp.c
+++ b/src/flt_decomp.c
@@ -1343,6 +1343,7 @@ struct decomp_check_ctx {
        int decomp_req_seen;
        int decomp_res_seen;
        int cache_seen;
+       unsigned int cache_flags;
 };
 
 /* Checks the filter <f> against the decomp filter being checked. It is used
@@ -1361,15 +1362,21 @@ static int decomp_check_order(struct flt_conf *f, void *data)
        }
        else if (flt_use_legacy_filter(ctx->px)) {
                if ((ctx->px->cap & PR_CAP_BE) && f->id == decomp_res_flt_id) {
-                       if (ctx->cache_seen) {
+                       /* Only report an explicitly declared cache filter
+                        * placed before decomp-res: the implicit one is
+                        * automatically ordered before it.
+                        */
+                       if (ctx->cache_seen && (ctx->cache_flags & FLT_CFG_FL_EXPLICIT)) {
                                ha_alert("config: %s '%s': cache filter declared before a decompression filter on the response is invalid.\n",
                                         proxy_type_str(ctx->px), ctx->px->id);
                                return 1;
                        }
                        ctx->decomp_res_seen = 1;
                }
-               if (f->id == cache_store_flt_id)
+               if (f->id == cache_store_flt_id) {
                        ctx->cache_seen = 1;
+                       ctx->cache_flags = f->flags;
+               }
        }
        return 0;
 }

diff --git a/src/flt_http_comp.c b/src/flt_http_comp.c
index 9649dc5afd4cfef820b9fbc2e43d82d9d0e14ce3..b0cfd77ed02a08e3b77297d03870e211fdddc0ae 100644 (file)
--- a/src/flt_http_comp.c
+++ b/src/flt_http_comp.c
@@ -1312,7 +1312,7 @@ static int http_comp_check_order(struct flt_conf *f, void *data)
                return 0;
        }
        if (flt_use_legacy_filter(ctx->px) && f->id == cache_store_flt_id &&
-           ctx->comp_res_seen) {
+           (f->flags & FLT_CFG_FL_EXPLICIT) && ctx->comp_res_seen) {
                ha_alert("config: %s '%s': unable to enable the compression filter on response "
                         "before any cache filter.\n",
                         proxy_type_str(ctx->px), ctx->px->id);