Re: Don't add redundant Cache-Control

On Sun, Sep 23, 2007 at 05:24:05PM +0200, Krzysztof Oledzki wrote:
>
>
> On Sat, 22 Sep 2007, Willy Tarreau wrote:
>
> >Hi Krzysztof,
> >
> >On Sat, Sep 22, 2007 at 03:09:45AM +0200, Krzysztof Oledzki wrote:
> >>2. haproxy-cache-control-once.patch.gz
> >>
> >>This one is little more complicated.
> >>
> >>I noticed that haproxy, with "cookie (...) nocache" option, always adds
> >>"Cache-control: private" at the end of a header list received from this
> >>server:
> >>
> >>Cache-Control: no-cache
> >>Content-Length: 45102
> >>Content-Type: text/html
> >>Content-Location: http://www.newsweek.pl/index.htm
> >>Last-Modified: Sat, 22 Sep 2007 00:15:08 GMT
> >>Accept-Ranges: bytes
> >>ETag: "63a4aa2adfcc71:dde"
> >>Server: Microsoft-IIS/6.0
> >>X-Powered-By: ASP.NET
> >>Date: Sat, 22 Sep 2007 00:18:34 GMT
> >>Connection: close
> >>Set-Cookie: SERVERID=s6; path=/
> >>Cache-control: private
> >
> >That's strange for this one, I had in mind that it would only add it if
> >there was not already a "Cache-Control: no-cache". Or maybe I'm confusing
> >with the "check_cache" option. I don't remember.
> >
> >>or:
> >>
> >>Connection: close
> >>Date: Sat, 22 Sep 2007 01:03:24 GMT
> >>Server: Microsoft-IIS/6.0
> >>X-Powered-By: ASP.NET
> >>Content-Length: 7458
> >>Content-Type: text/html
> >>Set-Cookie: ASPSESSIONIDCSRCTSSB=HCCBGGACGBHDHMMKIOILPHNG; path=/
> >>Cache-control: private
> >>Set-Cookie: SERVERID=s5; path=/
> >>Cache-control: private
> >
> >Oh yes, seen like thatn it's plain stupid :-)
> >
> >>It may be just redundant (two "Cache-control: private"), but sometimes it
> >>may be quite confused as we may end with two different, more and less
> >>restricted directions (no-cache & private) and even quite conflicting
> >>directions (eg. public & private):
> >>
> >>So, I added and rearranged a code, so now haproxy adds a "Cache-control:
> >>private" header only when there is no the same (private) or more
> >>restrictive (no-cache) one. It was done in three steps:
> >>
> >>1. Use check_response_for_cacheability to check if response is
> >>not cacheable. I simply moved this call before http_header_add_tail2.
> >>
> >>2. Use TX_CACHEABLE (not TX_CACHE_COOK - apache <= 1.3.26) to check if we
> >>need to add a Cache-control header. If we add it, clear TX_CACHEABLE and
> >>TX_CACHE_COOK.
> >>
> >>3. Check cacheability not only with PR_O_CHK_CACHE but also with
> >>PR_O_COOK_NOC, so:
> >>
> >>- unlikely(t->be->options & PR_O_CHK_CACHE))
> >>+ (t->be->options &
> >>(PR_O_CHK_CACHE|PR_O_COOK_NOC)))
> >> txn->flags |= TX_CACHEABLE |
> >> TX_CACHE_COOK;
> >>
> >>I removed this unlikely since I believe that now it is not so unlikely.
> >>
> >>The patch is definitely not perfect, proxy should probably also remove
> >>"Cache-control: public". Unfortunately, I do not know the code good enough
> >>to do in myself, yet. ;)
> >>
> >>Anyway, I think that even now, it should be very useful.
> >
> >OK, thanks very much. I'll analyze it all to ensure that it does not change
> >any known corner case I may have in mind, but your description seems right.
>
> OK, attached a new version with two additional fixes:
>
> - s/Cache-control/Cache-Control/ (proper case)

There is no "proper case" in HTTP, as all headers are case insensitive. I did the same mistake in the past (and used "memcmp" to check for cache).

> - accpet no-cache as ~TX_CACHEABLE & ~TX_CACHE_COOK, oryginal haproxy
> code only accepted 'no-cache="set-cookie' as ~TX_CACHE_COOK.

To be honnest, I have to check the original code an intended behaviour. The reason was to block responses with a cacheable cookie, and return a "502 Bad Gateway" instead.

> So now:
>
> Cache-Control: no-cache
> Content-Length: 44972
> Content-Type: text/html
> Content-Location: (...)
> Last-Modified: Sun, 23 Sep 2007 15:20:16 GMT
> Accept-Ranges: bytes
> ETag: "6086cd3ef5fdc71:dde"
> Server: Microsoft-IIS/6.0
> X-Powered-By: ASP.NET
> Date: Sun, 23 Sep 2007 15:21:48 GMT
> Connection: close
> Set-Cookie: SERVERID=s6; path=/

Looks clean at first glance. I should merge it once I understand all the implications on the cookie cacheability checks.

Thanks,
Willy

>
>
> Best regards,
>
> Krzysztof Ol?dzki

> diff -Nur haproxy-1.3.12.2-orig/src/proto_http.c haproxy-1.3.12.2-dev/src/proto_http.c
> --- haproxy-1.3.12.2-orig/src/proto_http.c 2007-09-20 08:47:17.000000000 +0200
> +++ haproxy-1.3.12.2-dev/src/proto_http.c 2007-09-23 17:04:40.000000000 +0200
> @@ -2866,7 +2866,7 @@
> * Cache-Control or Expires header fields."
> */
> if (likely(txn->meth != HTTP_METH_POST) &&
> - unlikely(t->be->options & PR_O_CHK_CACHE))
> + (t->be->options & (PR_O_CHK_CACHE|PR_O_COOK_NOC)))
> txn->flags |= TX_CACHEABLE | TX_CACHE_COOK;
> break;
> default:
> @@ -3003,8 +3003,15 @@
> */
> manage_server_side_cookies(t, rep);
>
> +
> /*
> - * 5: add server cookie in the response if needed
> + * 5: check for cache-control or pragma headers.
> + */
> + check_response_for_cacheability(t, rep);
> +
> +
> + /*
> + * 6: add server cookie in the response if needed
> */
> if ((t->srv) && !(t->flags & SN_DIRECT) && (t->be->options & PR_O_COOK_INS) &&
> (!(t->be->options & PR_O_COOK_POST) || (txn->meth == HTTP_METH_POST))) {
> @@ -3029,21 +3036,18 @@
> * Some caches understand the correct form: 'no-cache="set-cookie"', but
> * others don't (eg: apache <= 1.3.26). So we use 'private' instead.
> */
> - if (t->be->options & PR_O_COOK_NOC) {
> + if ((t->be->options & PR_O_COOK_NOC) && (txn->flags & TX_CACHEABLE)) {
> +
> + txn->flags &= ~TX_CACHEABLE & ~TX_CACHE_COOK;
> +
> if (unlikely(http_header_add_tail2(rep, &txn->rsp, &txn->hdr_idx,
> - "Cache-control: private", 22)) < 0)
> + "Cache-Control: private", 22)) < 0)
> goto return_bad_resp;
> }
> }
>
>
> /*
> - * 6: check for cache-control or pragma headers.
> - */
> - check_response_for_cacheability(t, rep);
> -
> -
> - /*
> * 7: check if result will be cacheable with a cookie.
> * We'll block the response if security checks have caught
> * nasty things such as a cacheable cookie.
> @@ -5116,6 +5120,7 @@
>
> /* OK, so we know that either p2 points to the end of string or to a comma */
> if (((p2 - p1 == 7) && strncasecmp(p1, "private", 7) == 0) ||
> + ((p2 - p1 == 8) && strncasecmp(p1, "no-cache", 8) == 0) ||
> ((p2 - p1 == 8) && strncasecmp(p1, "no-store", 8) == 0) ||
> ((p2 - p1 == 9) && strncasecmp(p1, "max-age=0", 9) == 0) ||
> ((p2 - p1 == 10) && strncasecmp(p1, "s-maxage=0", 10) == 0)) {
Received on 2007/09/29 12:47