Willy, read the cover letter of this thread before ignoring the first patch, just because this one has a higher version number to avoid mistakes.
Apply with `git am --scissors` to automatically cut the commit message. -- >8 -- Make HAProxy set the `Vary: Accept-Encoding` response header if the server response would normally be compressed based off the current configuration. Specifically make sure to: 1. Disregard the *request* headers ... 2. Disregard the current compression rate and other temporary conditions ... ... when determining whether the `Vary` header must be set. The *response* headers generated by the upstream are allowed to be taken into account. This patch *does not* though. Once compression is enabled the `Vary` header will be set for *all* upstream responses. A small issue remains: The User-Agent is not added to the `Vary` header, despite being relevant to the response. Adding the User-Agent header would make responses effectively uncacheable and it's unlikely to see a Mozilla/4 in the wild in 2019. Add a reg-test to ensure this behaviour. --- reg-tests/compression/vary.vtc | 187 +++++++++++++++++++++++++++++++++ src/flt_http_comp.c | 30 +++++- 2 files changed, 213 insertions(+), 4 deletions(-) create mode 100644 reg-tests/compression/vary.vtc diff --git a/reg-tests/compression/vary.vtc b/reg-tests/compression/vary.vtc new file mode 100644 index 000000000..2f5d82d73 --- /dev/null +++ b/reg-tests/compression/vary.vtc @@ -0,0 +1,187 @@ +varnishtest "Compression sets Vary header" + +#REQUIRE_VERSION=1.9 +#REQUIRE_OPTION=ZLIB|SLZ + +feature ignore_unknown_macro + +server s1 { + rxreq + expect req.url == "/plain/accept-encoding-gzip" + expect req.http.accept-encoding == "gzip" + txresp \ + -hdr "Content-Type: text/plain" \ + -bodylen 100 + + rxreq + expect req.url == "/plain/accept-encoding-invalid" + expect req.http.accept-encoding == "invalid" + txresp \ + -hdr "Content-Type: text/plain" \ + -bodylen 100 + + rxreq + expect req.url == "/plain/accept-encoding-null" + expect req.http.accept-encoding == "<undef>" + txresp \ + -hdr "Content-Type: text/plain" \ + -bodylen 100 + + rxreq + expect req.url == "/html/accept-encoding-gzip" + expect req.http.accept-encoding == "gzip" + txresp \ + -hdr "Content-Type: text/html" \ + -bodylen 100 + + rxreq + expect req.url == "/html/accept-encoding-invalid" + expect req.http.accept-encoding == "invalid" + txresp \ + -hdr "Content-Type: text/html" \ + -bodylen 100 + + + rxreq + expect req.url == "/html/accept-encoding-null" + expect req.http.accept-encoding == "<undef>" + txresp \ + -hdr "Content-Type: text/html" \ + -bodylen 100 + + rxreq + expect req.url == "/dup-etag/accept-encoding-gzip" + expect req.http.accept-encoding == "gzip" + txresp \ + -hdr "Content-Type: text/plain" \ + -hdr "ETag: \"123\"" \ + -hdr "ETag: \"123\"" \ + -bodylen 100 +} -repeat 2 -start + + +haproxy h1 -conf { + defaults + mode http + ${no-htx} option http-use-htx + timeout connect 1s + timeout client 1s + timeout server 1s + + frontend fe-gzip + bind "fd@${fe_gzip}" + default_backend be-gzip + + backend be-gzip + compression algo gzip + compression type text/plain + server www ${s1_addr}:${s1_port} + + frontend fe-nothing + bind "fd@${fe_nothing}" + default_backend be-nothing + + backend be-nothing + server www ${s1_addr}:${s1_port} +} -start + +client c1 -connect ${h1_fe_gzip_sock} { + txreq -url "/plain/accept-encoding-gzip" \ + -hdr "Accept-Encoding: gzip" + rxresp + expect resp.status == 200 + expect resp.http.content-encoding == "gzip" + expect resp.http.vary == "Accept-Encoding" + gunzip + expect resp.bodylen == 100 + + txreq -url "/plain/accept-encoding-invalid" \ + -hdr "Accept-Encoding: invalid" + rxresp + expect resp.status == 200 + expect resp.http.vary == "Accept-Encoding" + expect resp.bodylen == 100 + + txreq -url "/plain/accept-encoding-null" + rxresp + expect resp.status == 200 + expect resp.http.vary == "Accept-Encoding" + expect resp.bodylen == 100 + + txreq -url "/html/accept-encoding-gzip" \ + -hdr "Accept-Encoding: gzip" + rxresp + expect resp.status == 200 + expect resp.http.vary == "Accept-Encoding" + expect resp.bodylen == 100 + + txreq -url "/html/accept-encoding-invalid" \ + -hdr "Accept-Encoding: invalid" + rxresp + expect resp.status == 200 + expect resp.http.vary == "Accept-Encoding" + expect resp.bodylen == 100 + + txreq -url "/html/accept-encoding-null" + rxresp + expect resp.status == 200 + expect resp.http.vary == "Accept-Encoding" + expect resp.bodylen == 100 + + txreq -url "/dup-etag/accept-encoding-gzip" \ + -hdr "Accept-Encoding: gzip" + rxresp + expect resp.status == 200 + expect resp.http.vary == "Accept-Encoding" + expect resp.bodylen == 100 +} -run + +# This Client duplicates c1, against the "nothing" frontend, ensuring no Vary header is ever set. +client c2 -connect ${h1_fe_nothing_sock} { + txreq -url "/plain/accept-encoding-gzip" \ + -hdr "Accept-Encoding: gzip" + rxresp + expect resp.status == 200 + expect resp.http.vary == "<undef>" + expect resp.bodylen == 100 + + txreq -url "/plain/accept-encoding-invalid" \ + -hdr "Accept-Encoding: invalid" + rxresp + expect resp.status == 200 + expect resp.http.vary == "<undef>" + expect resp.bodylen == 100 + + txreq -url "/plain/accept-encoding-null" + rxresp + expect resp.status == 200 + expect resp.http.vary == "<undef>" + expect resp.bodylen == 100 + + txreq -url "/html/accept-encoding-gzip" \ + -hdr "Accept-Encoding: gzip" + rxresp + expect resp.status == 200 + expect resp.http.vary == "<undef>" + expect resp.bodylen == 100 + + txreq -url "/html/accept-encoding-invalid" \ + -hdr "Accept-Encoding: invalid" + rxresp + expect resp.status == 200 + expect resp.http.vary == "<undef>" + expect resp.bodylen == 100 + + txreq -url "/html/accept-encoding-null" + rxresp + expect resp.status == 200 + expect resp.http.vary == "<undef>" + expect resp.bodylen == 100 + + txreq -url "/dup-etag/accept-encoding-gzip" \ + -hdr "Accept-Encoding: gzip" + rxresp + expect resp.status == 200 + expect resp.http.vary == "<undef>" + expect resp.bodylen == 100 +} -run diff --git a/src/flt_http_comp.c b/src/flt_http_comp.c index b04dcd145..d513257b7 100644 --- a/src/flt_http_comp.c +++ b/src/flt_http_comp.c @@ -160,11 +160,11 @@ comp_http_headers(struct stream *s, struct filter *filter, struct http_msg *msg) if (!(msg->chn->flags & CF_ISRESP)) select_compression_request_header(st, s, msg); else { + if (!set_compression_response_header(st, s, msg)) + goto end; /* 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; @@ -476,6 +476,16 @@ http_set_comp_reshdr(struct comp_state *st, struct stream *s, struct http_msg *m struct http_txn *txn = s->txn; struct hdr_ctx ctx; + if (http_header_add_tail2(msg, &txn->hdr_idx, "Vary: Accept-Encoding", 21) < 0) + goto error; + + /* + * Break if no compression is actually happening. + */ + if (!st->comp_algo) { + return 1; + } + /* * Add Content-Encoding header when it's not identity encoding. * RFC 2616 : Identity encoding: This content-coding is used only in the @@ -526,7 +536,8 @@ http_set_comp_reshdr(struct comp_state *st, struct stream *s, struct http_msg *m return 1; error: - st->comp_algo->end(&st->comp_ctx); + if (st->comp_algo) + st->comp_algo->end(&st->comp_ctx); st->comp_algo = NULL; return 0; } @@ -537,6 +548,16 @@ htx_set_comp_reshdr(struct comp_state *st, struct stream *s, struct http_msg *ms struct htx *htx = htxbuf(&msg->chn->buf); struct http_hdr_ctx ctx; + if (!http_add_header(htx, ist("Vary"), ist("Accept-Encoding"))) + goto error; + + /* + * Break if no compression is actually happening. + */ + if (!st->comp_algo) { + return 1; + } + /* * Add Content-Encoding header when it's not identity encoding. * RFC 2616 : Identity encoding: This content-coding is used only in the @@ -580,7 +601,8 @@ htx_set_comp_reshdr(struct comp_state *st, struct stream *s, struct http_msg *ms return 1; error: - st->comp_algo->end(&st->comp_ctx); + if (st->comp_algo) + st->comp_algo->end(&st->comp_ctx); st->comp_algo = NULL; return 0; } -- 2.21.0