BUG/MAJOR: compression/cache: Make it really works with these both filters

Caching the response with the compression enabled was totally broken. To fix the
problem, the compression must be done after caching the response. Otherwise it
needs to change the cache to store compressed and uncompressed objects for the
same ressource. So, because it is not possible for now, it is forbidden to
declare the compression filter before the cache one. To ease the configuration,
both can be implicitly declared (without "filter" keyword). The compression will
automatically be inserted after the cache.

Then, to make it works this way, the compression filter has been slighly
modified. Now, the response headers are updated after http-response rules
evaluations, instead of before. So, if the response contains a "Content-length"
header, it will be kept with the response stored in the cache. So this cached
response will be able to be served to clients not supporting the compression at
all.

doc/configuration.txt

@@ -17716,12 +17716,14 @@ filter compression
 
 The HTTP compression has been moved in a filter in HAProxy 1.7. "compression"
 keyword must still be used to enable and configure the HTTP compression. And
-when no other filter is used, it is enough. But it is mandatory to explicitly
+when no other filter is used, it is enough. When used with the cache enabled,
-use a filter line to enable the HTTP compression when two or more filters are
+it is also enough. In this case, the compression is always done after the
-used for the same listener/frontend/backend. This is important to know the
+response is stored in the cache. But it is mandatory to explicitly use a filter
-filters evaluation order.
+line to enable the HTTP compression when at least one filter other than the
+cache is used for the same listener/frontend/backend. This is important to know
+the filters evaluation order.
 
-See also : "compression"
+See also : "compression" and section 9.4 about the cache filter.
 
 
 9.3. Stream Processing Offload Engine (SPOE)
@@ -17767,12 +17769,14 @@ filter cache <name>
 The cache uses a filter to store cacheable responses. The HTTP rules
 "cache-store" and "cache-use" must be used to define how and when to use a
 cache. By default the correpsonding filter is implicitly defined. And when no
-other filter than cache is used, it is enough. But it is mandatory to
+other filters than cache or compression are used, it is enough. In such case,
-explicitly use a filter line to use a cache when two or more filters are used
+the compression filter is always evaluated after the cache filter. But it is
-for the same listener/frontend/backend. This is important to know the filters
+mandatory to explicitly use a filter line to use a cache when at least one
-evaluation order.
+filter other than the compression is used for the same
+listener/frontend/backend. This is important to know the filters evaluation
+order.
 
-See also : section 10 about cache.
+See also : section 9.2 about the compression filter and section 10 about cache.
 
 10. Cache
 ---------

src/cache.c

@@ -43,9 +43,7 @@
 					* messages (legacy implementation) */
 #define CACHE_F_HTX         0x00000002 /* The cache is used to store HTX messages */
 
-#define CACHE_FLT_F_IGNORE_CNT_ENC 0x00000001 /* Ignore 'Content-Encoding' header when response is cached
+#define CACHE_FLT_F_IMPLICIT_DECL  0x00000001 /* The cache filtre was implicitly declared (ie without
-					       * if compression is already started */
-#define CACHE_FLT_F_IMPLICIT_DECL  0x00000002 /* The cache filtre was implicitly declared (ie without
 					       * the filter keyword) */
 
 const char *cache_store_flt_id = "cache store filter";
@@ -161,7 +159,7 @@ cache_store_check(struct proxy *px, struct flt_conf *fconf)
 	struct cache_flt_conf *cconf = fconf->conf;
 	struct flt_conf *f;
 	struct cache *cache;
-	int ignore = 0;
+	int comp = 0;
 
 	/* resolve the cache name to a ptr in the filter config */
 	list_for_each_entry(cache, &caches, list) {
@@ -186,36 +184,36 @@ cache_store_check(struct proxy *px, struct flt_conf *fconf)
 	 * points on the cache filter configuration. */
 
 	/* Check all filters for proxy <px> to know if the compression is
-	 * enabled and if it is before or after the cache. When the compression
+	 * enabled and if it is after the cache. When the compression is before
-	 * is before the cache, nothing special is done. The response is stored
+	 * the cache, an error is returned. Also check if the cache filter must
-	 * compressed in the cache. When the compression is after the cache, the
+	 * be explicitly declaired or not. */
-	 * 'Content-encoding' header must be ignored because the response will
-	 * be stored uncompressed. The compression will be done on the cached
-	 * response too. Also check if the cache filter must be explicitly
-	 * declaired or not. */
 	list_for_each_entry(f, &px->filter_configs, list) {
 		if (f == fconf) {
-			ignore = 1;
+			/* The compression filter must be evaluated after the cache. */
-			continue;
+			if (comp) {
+				ha_alert("config: %s '%s': unable to enable the compression filter before "
+					 "the cache '%s'.\n", proxy_type_str(px), px->id, cache->id);
+				return 1;
+			}
 		}
-
+		else if (f->id == http_comp_flt_id) {
-		if ((f->id != fconf->id) && (cconf->flags & CACHE_FLT_F_IMPLICIT_DECL)) {
-			ha_alert("config: %s '%s': require an explicit filter declaration "
-				 "to use the cache '%s'.\n", proxy_type_str(px), px->id, cache->id);
-			return 1;
-		}
-
-		if (f->id == http_comp_flt_id) {
 			if (!(px->options2 & PR_O2_USE_HTX)) {
 				ha_alert("config: %s '%s' : compression and cache filters cannot be "
 					 "both enabled on non HTX proxy.\n",
 					 proxy_type_str(px), px->id);
 				return 1;
 			}
-			if (ignore)
+			comp = 1;
-				cconf->flags |= CACHE_FLT_F_IGNORE_CNT_ENC;
-			break;
 		}
+		else if ((f->id != fconf->id) && (cconf->flags & CACHE_FLT_F_IMPLICIT_DECL)) {
+			/* Implicit declaration is only allowed with the
+			 * compression. For other filters, an implicit
+			 * declaration is required. */
+			ha_alert("config: %s '%s': require an explicit filter declaration "
+				 "to use the cache '%s'.\n", proxy_type_str(px), px->id, cache->id);
+			return 1;
+		}
+
 	}
 	return 0;
 }
@@ -675,21 +673,6 @@ enum act_return http_action_store_cache(struct act_rule *rule, struct proxy *px,
 			enum htx_blk_type type = htx_get_blk_type(blk);
 			uint32_t sz = htx_get_blksz(blk);
 
-			/* Check if we need to skip 'Content-encoding' header or not */
-			if ((msg->flags & HTTP_MSGF_COMPRESSING) && /* Compression in progress */
-			    (cconf->flags & CACHE_FLT_F_IGNORE_CNT_ENC) &&  /* Compression before the cache */
-			    (type == HTX_BLK_HDR)) {
-				struct ist n = htx_get_blk_name(htx, blk);
-				struct ist v = htx_get_blk_value(htx, blk);
-
-				if (isteq(n, ist("content-encoding")))
-					continue;
-				if (!(msg->flags & HTTP_MSGF_TE_CHNK) &&
-				    isteq(n, ist("transfer-encoding")) &&
-				    isteqi(v, ist("chunked")))
-				    continue;
-			}
-
 			chunk_memcat(&trash, (char *)&blk->info, sizeof(blk->info));
 			if (type == HTX_BLK_EOH)
 				break;

src/flt_http_comp.c

@@ -59,6 +59,9 @@ static int select_compression_request_header(struct comp_state *st,
 static int select_compression_response_header(struct comp_state *st,
 					      struct stream *s,
 					      struct http_msg *msg);
+static int set_compression_response_header(struct comp_state *st,
+					   struct stream *s,
+					   struct http_msg *msg);
 
 static int htx_compression_buffer_init(struct htx *htx, struct buffer *out);
 static int htx_compression_buffer_add_data(struct comp_state *st, const char *data, size_t len,
@@ -161,6 +164,8 @@ comp_http_headers(struct stream *s, struct filter *filter, struct http_msg *msg)
 		/* Response headers have already been checked in
 		 * comp_http_post_analyze callback. */
 		if (st->comp_algo) {
+			if (!set_compression_response_header(st, s, msg))
+				goto end;
 			register_data_filter(s, msg->chn, filter);
 			if (!IS_HTX_STRM(s))
 				st->hdrs_len = s->txn->rsp.sov;
@@ -456,6 +461,102 @@ comp_http_end(struct stream *s, struct filter *filter,
 	return 1;
 }
 /***********************************************************************/
+static int
+http_set_comp_reshdr(struct comp_state *st, struct stream *s, struct http_msg *msg)
+{
+	struct http_txn *txn = s->txn;
+
+	/*
+	 * Add Content-Encoding header when it's not identity encoding.
+         * RFC 2616 : Identity encoding: This content-coding is used only in the
+	 * Accept-Encoding header, and SHOULD NOT be used in the Content-Encoding
+	 * header.
+	 */
+	if (st->comp_algo->cfg_name_len != 8 || memcmp(st->comp_algo->cfg_name, "identity", 8) != 0) {
+		trash.data = 18;
+		memcpy(trash.area, "Content-Encoding: ", trash.data);
+		memcpy(trash.area + trash.data, st->comp_algo->ua_name,
+		       st->comp_algo->ua_name_len);
+		trash.data += st->comp_algo->ua_name_len;
+		trash.area[trash.data] = '\0';
+		if (http_header_add_tail2(msg, &txn->hdr_idx, trash.area, trash.data) < 0)
+			goto error;
+	}
+
+	/* remove Content-Length header */
+	if (msg->flags & HTTP_MSGF_CNT_LEN) {
+		struct hdr_ctx ctx;
+
+		ctx.idx = 0;
+		while (http_find_header2("Content-Length", 14, ci_head(&s->res), &txn->hdr_idx, &ctx))
+			http_remove_header2(msg, &txn->hdr_idx, &ctx);
+	}
+
+	/* add Transfer-Encoding header */
+	if (!(msg->flags & HTTP_MSGF_TE_CHNK)) {
+		if (http_header_add_tail2(msg, &txn->hdr_idx, "Transfer-Encoding: chunked", 26) < 0)
+			goto error;
+	}
+
+
+	return 1;
+
+  error:
+	st->comp_algo->end(&st->comp_ctx);
+	st->comp_algo = NULL;
+	return 0;
+}
+
+static int
+htx_set_comp_reshdr(struct comp_state *st, struct stream *s, struct http_msg *msg)
+{
+	struct htx *htx = htxbuf(&msg->chn->buf);
+
+	/*
+	 * Add Content-Encoding header when it's not identity encoding.
+	 * RFC 2616 : Identity encoding: This content-coding is used only in the
+	 * Accept-Encoding header, and SHOULD NOT be used in the Content-Encoding
+	 * header.
+	 */
+	if (st->comp_algo->cfg_name_len != 8 || memcmp(st->comp_algo->cfg_name, "identity", 8) != 0) {
+		struct ist v = ist2(st->comp_algo->ua_name, st->comp_algo->ua_name_len);
+
+		if (!http_add_header(htx, ist("Content-Encoding"), v))
+			goto error;
+	}
+
+	/* remove Content-Length header */
+	if (msg->flags & HTTP_MSGF_CNT_LEN) {
+		struct http_hdr_ctx ctx;
+
+		ctx.blk = NULL;
+		while (http_find_header(htx, ist("Content-Length"), &ctx, 1))
+			http_remove_header(htx, &ctx);
+	}
+
+	/* add "Transfer-Encoding: chunked" header */
+	if (!(msg->flags & HTTP_MSGF_TE_CHNK)) {
+		if (!http_add_header(htx, ist("Transfer-Encoding"), ist("chunked")))
+			goto error;
+	}
+
+	return 1;
+
+  error:
+	st->comp_algo->end(&st->comp_ctx);
+	st->comp_algo = NULL;
+	return 0;
+}
+
+static int
+set_compression_response_header(struct comp_state *st, struct stream *s, struct http_msg *msg)
+{
+	if (IS_HTX_STRM(s))
+		return htx_set_comp_reshdr(st, s, msg);
+	else
+		return http_set_comp_reshdr(st, s, msg);
+}
+
 /*
  * Selects a compression algorithm depending on the client request.
  */
@@ -780,32 +881,6 @@ http_select_comp_reshdr(struct comp_state *st, struct stream *s, struct http_msg
 	/* initialize compression */
 	if (st->comp_algo->init(&st->comp_ctx, global.tune.comp_maxlevel) < 0)
 		goto fail;
-
-	/* remove Content-Length header */
-	ctx.idx = 0;
-	if ((msg->flags & HTTP_MSGF_CNT_LEN) && http_find_header2("Content-Length", 14, ci_head(c), &txn->hdr_idx, &ctx))
-		http_remove_header2(msg, &txn->hdr_idx, &ctx);
-
-	/* add Transfer-Encoding header */
-	if (!(msg->flags & HTTP_MSGF_TE_CHNK))
-		http_header_add_tail2(&txn->rsp, &txn->hdr_idx, "Transfer-Encoding: chunked", 26);
-
-	/*
-	 * Add Content-Encoding header when it's not identity encoding.
-         * RFC 2616 : Identity encoding: This content-coding is used only in the
-	 * Accept-Encoding header, and SHOULD NOT be used in the Content-Encoding
-	 * header.
-	 */
-	if (st->comp_algo->cfg_name_len != 8 || memcmp(st->comp_algo->cfg_name, "identity", 8) != 0) {
-		trash.data = 18;
-		memcpy(trash.area, "Content-Encoding: ", trash.data);
-		memcpy(trash.area + trash.data, st->comp_algo->ua_name,
-		       st->comp_algo->ua_name_len);
-		trash.data += st->comp_algo->ua_name_len;
-		trash.area[trash.data] = '\0';
-		http_header_add_tail2(&txn->rsp, &txn->hdr_idx, trash.area,
-				      trash.data);
-	}
 	msg->flags |= HTTP_MSGF_COMPRESSING;
 	return 1;
 
@@ -898,34 +973,6 @@ htx_select_comp_reshdr(struct comp_state *st, struct stream *s, struct http_msg
 	/* initialize compression */
 	if (st->comp_algo->init(&st->comp_ctx, global.tune.comp_maxlevel) < 0)
 		goto fail;
-
-	/*
-	 * Add Content-Encoding header when it's not identity encoding.
-         * RFC 2616 : Identity encoding: This content-coding is used only in the
-	 * Accept-Encoding header, and SHOULD NOT be used in the Content-Encoding
-	 * header.
-	 */
-	if (st->comp_algo->cfg_name_len != 8 || memcmp(st->comp_algo->cfg_name, "identity", 8) != 0) {
-		struct ist v = ist2(st->comp_algo->ua_name, st->comp_algo->ua_name_len);
-
-		if (!http_add_header(htx, ist("Content-Encoding"), v))
-			goto deinit_comp_ctx;
-	}
-
-	/* remove Content-Length header */
-	if (msg->flags & HTTP_MSGF_CNT_LEN) {
-		ctx.blk = NULL;
-
-		while (http_find_header(htx, ist("Content-Length"), &ctx, 1))
-			http_remove_header(htx, &ctx);
-	}
-
-	/* add "Transfer-Encoding: chunked" header */
-	if (!(msg->flags & HTTP_MSGF_TE_CHNK)) {
-		if (!http_add_header(htx, ist("Transfer-Encoding"), ist("chunked")))
-			goto deinit_comp_ctx;
-	}
-
 	msg->flags |= HTTP_MSGF_COMPRESSING;
 	return 1;
 
@@ -1304,6 +1351,8 @@ int
 check_implicit_http_comp_flt(struct proxy *proxy)
 {
 	struct flt_conf *fconf;
+	int explicit = 0;
+	int comp = 0;
 	int err = 0;
 
 	if (proxy->comp == NULL)
@@ -1311,14 +1360,31 @@ check_implicit_http_comp_flt(struct proxy *proxy)
 	if (!LIST_ISEMPTY(&proxy->filter_configs)) {
 		list_for_each_entry(fconf, &proxy->filter_configs, list) {
 			if (fconf->id == http_comp_flt_id)
-				goto end;
+				comp = 1;
+			else if (fconf->id == cache_store_flt_id) {
+				if (comp) {
+					ha_alert("config: %s '%s': unable to enable the compression filter "
+						 "before any cache filter.\n",
+						 proxy_type_str(proxy), proxy->id);
+					err++;
+					goto end;
+				}
+			}
+			else
+				explicit = 1;
 		}
-		ha_alert("config: %s '%s': require an explicit filter declaration to use HTTP compression\n",
+	}
-			 proxy_type_str(proxy), proxy->id);
+	if (comp)
+		goto end;
+	else if (explicit) {
+		ha_alert("config: %s '%s': require an explicit filter declaration to use "
+			 "HTTP compression\n", proxy_type_str(proxy), proxy->id);
 		err++;
 		goto end;
 	}
 
+	/* Implicit declaration of the compression filter is always the last
+	 * one */
 	fconf = calloc(1, sizeof(*fconf));
 	if (!fconf) {
 		ha_alert("config: %s '%s': out of memory\n",