Do we handle 304 responses correctly?

[nigoroll]

I'm wondering if there aren't some potential problems unrelated to this patch that we have uncovered. The fact that Varnish will enter 304 revalidation logic even though we never asked for it (didn't send out any IMS/INM headers) is worrying to me. I wonder if we shouldn't keep track somehow of whether to expect a 304, and error out if we still got a 304. Could prevent a future gotcha at least.

Originally posted by @mbgrydeland in #4013 (comment)

[walid-git]

We already error out when we get an unexpected 304 response from the backend :
https://github.com/varnishcache/varnish-cache/blob/master/bin/varnishd/cache/cache_fetch.c#L375-L378

	if (bo->stale_oc != NULL &&
	    ObjCheckFlag(bo->wrk, bo->stale_oc, OF_IMSCAND)) {
	    ....
	} else if (!bo->uncacheable) {
		/*
		 * Backend sent unallowed 304
		 */
		VSLb(bo->vsl, SLT_Error,
		    "304 response but not conditional fetch");
		bo->htc->doclose = SC_RX_BAD;
		vbf_cleanup(bo);
		return (-1);
	}

[mbgrydeland]

I guess the discrepancy to investigate is that OF_IMSCAND doesn't track whether we sent IMS/INM on the fetch, only if we could've done that. And especially for zero-length objects, it is possible to not send IMS/INM, but still execute 304 handling if the backend responded with 304 anyways.

[dridi]

I'm of the opinion that if we can't see it this problem does not exist, so may I suggest starting with a new addition to vmod_debug?

.. NB: keep enum in sync with obj_attr.h
$Function INT obj_attr_size($Enum {LEN, VXID, FLAGS, GZIPBITS, LASTMODIFIED, VARY, HEADERS, ESIDATA, GZIPED, CHGCE, IMSCAND, ESIPROC})
$Restrict vcl_deliver, vcl_backend_refresh, vcl_backend_response

Give the size of an attribute or zero if it is missing for:

- ``obj`` in ``vcl_deliver``
- ``obj_stale`` in ``vcl_backend_refresh``
- ``beresp`` in ``vcl_backend_response``

This way it can also be used in boolean expressions. Once we have that we can add more test cases, or more check to existing test cases. And since we want to solve this before #3994 we would initially skip the obj_stale inspection.

Does it make sense?

[bsdphk]

My view is that the IMS header indicates to VCL that conditional fetch is available, but nothing prevents VCL from replacing it with If-None-Match or another "better" conditional, for whatever value of "better" the VCL writer prefers.

The only dubious case that the bereq has no conditionals and a buggy backend sends 304, something I do not see as a high priority for us.

I'm not even convinced that all conditionals, also private ones, are mandated to have "If-*" as a prefix, is that written down somewhere ?