Respect Vary rules when ignore_accept_encoding_mismatch is set to 2 (default value)

[JakeChampion]

I believe the previous implementation was a bug because it was treating any non-zero proxy.config.http.cache.ignore_accept_encoding_mismatch value as an indication to "ignore the mismatch", but the the documented default value of 2 is meant to honor Vary: Accept-Encoding

.. ts:cv:: CONFIG proxy.config.http.cache.ignore_accept_encoding_mismatch INT 2
:reloadable:
:overridable:

When enabled with a value of 1, |TS| serves documents from cache with a
Content-Encoding: header even if it does not match the Accept-Encoding:
header of the request. If set to 2 (default), this logic only happens in the absence of a
Vary header in the cached response (which is the recommended and safe use).

.. note::

This option should only be enabled with 1 if you're having
problems with caching and you origin server doesn't set the Vary
header. Alternatively, if the origin is incorrectly setting
Vary: Accept-Encoding or doesn't respond with 406 (Not Acceptable)
you can also enable this configuration with a 1.

previously, the default value 2 which should still respect Vary: Accept-Encoding when the origin supplies it, ended up collapsing distinct encoding variants into a single cache alternate, meaning requests with Accept-Encoding: br could get a response cached for gzip, which the tests confirmed

changing the check to only be for when the value is set to 1 looks to be the behaviour we do want, and the original test assertions now pass

Fixes #7458

[JosiahWI]

Looks like the file needs a newline added (at the end). I did not notice whether that was the case before you applied my suggestion. Did the restored AuTests pass prior to applying my suggestion?

[JakeChampion]

Looks like the file needs a newline added (at the end). I did not notice whether that was the case before you applied my suggestion. Did the restored AuTests pass prior to applying my suggestion?

yes, they all passed before the suugestion was applied

[JosiahWI]

   Run: 0-tr: Failed
     Test : Checking that True == _isRunningAfter - Passed
        Reason: Returned Value: True == _isRunningAfter
     Starting TestRun 0-tr : No Issues found - Passed
        Reason: Started!
     Process: Default: Failed
       Setting up : Copying '/home/jenkins/workspace/Github_Builds/autest/src/build/proxy-verifier-v2.12.0/linux-amd64' to '/tmp/sandbox/normalized_ae_match_vary_cache/client/bin' - Passed
       Setting up : Copying 'replays/normalized_ae_varied_transactions.replay.yaml' to '/tmp/sandbox/normalized_ae_match_vary_cache/client/normalized_ae_varied_transactions.replay.yaml' - Passed
       Setting up : Copying '/home/jenkins/workspace/Github_Builds/autest/src/tests/gold_tests/autest-site/../../tools/proxy-verifier/ssl/client.pem' to '/tmp/sandbox/normalized_ae_match_vary_cache/client/client.pem' - Passed
       Setting up : Copying '/home/jenkins/workspace/Github_Builds/autest/src/tests/gold_tests/autest-site/../../tools/proxy-verifier/ssl/ca.pem' to '/tmp/sandbox/normalized_ae_match_vary_cache/client/ca.pem' - Passed
       Test : Checking that ReturnCode == 0 - Failed
          Reason: Returned Value 1 != 0
       Time-Out : Process finishes within expected time - Passed
          Reason: Returned value: 2.515380620956421 < 600.0
       file /tmp/sandbox/normalized_ae_match_vary_cache/_output/0-tr-Default/stream.stdout.txt : There should be no Proxy Verifier violation errors. - Failed
          Reason: Contents of /tmp/sandbox/normalized_ae_match_vary_cache/_output/0-tr-Default/stream.stdout.txt contains expression: "Violation|Invalid status"
             Details:
               [INFO]: Equals Violation: Different. Key: "06", Field Name: "x-cache", Correct Value: "miss", Actual Value: "hit-fresh" : 27
               [INFO]: Equals Violation: Different. Key: "06", Field Name: "x-response-identifier", Correct Value: "Br-Accept-Encoding", Actual Value: "Gzip-Accept-Encoding" : 28
               [INFO]: Equals Violation: Different. Key: "36", Field Name: "x-cache", Correct Value: "miss", Actual Value: "hit-fresh" : 65
               [INFO]: Equals Violation: Different. Key: "36", Field Name: "x-response-identifier", Correct Value: "Br-Gzip-Accept-Encoding", Actual Value: "Gzip-Accept-Encoding" : 66
               [INFO]: Equals Violation: Different. Key: "39", Field Name: "x-response-identifier", Correct Value: "Br-Gzip-Accept-Encoding", Actual Value: "Gzip-Accept-Encoding" : 72

Looks like the tests no longer pass after the suggestion was applied.

[JosiahWI]

I would like to have explanations for why the test cases are correct and why the implementation should be this way. It looks to me like found_match will not ever be set to true, and therefore a quality of -1.0 will always be returned from the new part of the code. It's good that it passes the tests, but this implementation doesn't make sense, does it? I believe you can replace all of the new code with return -1.0 and the tests will still pass.

(And assuming the tests are correct, I would rather have return -1.0 than an algorithm that does all the work to find a minimum and then doesn't do anything with it.)

[JakeChampion]

@JosiahWI I think the actual issue ended up being a bug in how we were deciding when to disable the vary mis-match for accept-encoding

[JosiahWI]

I am suspicious of all the tests in this AuTest (including ones you have not touched) that reply with the empty-encoding alternate in response to requests for a deflate encoded resource. I think that is nonconforming to the RFCs and needs to be fixed. I would appreciate if @maskit or someone could double check my work on that.

Other than that general concern, I have some comments pertaining directly to the changes in this PR. Please take a look.

[JosiahWI]

The Rocky CI ran into this again (I've seen it before):

146/146 Test  #26: test_cache_CacheAggregateWriteBuffer ...***Timeout 1500.11 sec
Randomness seeded to: 57145569
===============================================================================
All tests passed (2 assertions in 2 test cases)

=================================================================
==7089==ERROR: AddressSanitizer: heap-use-after-free on address 0x6220000039c8 at pc 0x000000c81eeb bp 0x7fbfc60e1aa0 sp 0x7fbfc60e1a90
READ of size 8 at 0x6220000039c8 thread T1 ([ET_NET 0])

[JakeChampion]

I am suspicious of all the tests in this AuTest (including ones you have not touched) that reply with the empty-encoding alternate in response to requests for a deflate encoded resource. I think that is nonconforming to the RFCs and needs to be fixed. I would appreciate if @maskit or someone could double check my work on that.

Other than that general concern, I have some comments pertaining directly to the changes in this PR. Please take a look.

I believe those tests are correct because of https://www.rfc-editor.org/rfc/rfc9110.html#section-12.5.3-10.2 which states

If the representation has no content coding, then it is acceptable by default unless specifically excluded by the Accept-Encoding header field stating either "identity;q=0" or "*;q=0" without a more specific entry for "identity".

we are not sending either "identity;q=0" or "*;q=0" - so ATS can send back the identity response it has

[JosiahWI]

I am suspicious of all the tests in this AuTest (including ones you have not touched) that reply with the empty-encoding alternate in response to requests for a deflate encoded resource. I think that is nonconforming to the RFCs and needs to be fixed. I would appreciate if @maskit or someone could double check my work on that.
Other than that general concern, I have some comments pertaining directly to the changes in this PR. Please take a look.

I believe those tests are correct because of https://www.rfc-editor.org/rfc/rfc9110.html#section-12.5.3-10.2 which states

If the representation has no content coding, then it is acceptable by default unless specifically excluded by the Accept-Encoding header field stating either "identity;q=0" or "*;q=0" without a more specific entry for "identity".

we are not sending either "identity;q=0" or "*;q=0" - so ATS can send back the identity response it has

Whoops, I was looking at the obsoleted RFCs, thanks you for the correction! I think it is still non-conforming for a proxy to reply out-of-cache in this scenario, however: https://www.rfc-editor.org/rfc/rfc9111#section-4.1-1.

We could argue about wording, but I think it will be more profitable to talk about the overall meaning of the RFCs. First let me go over my understanding of the background, just so I know we're on the same page about the setup. Suppose that ATS receives a request to ae-0.com/path with Accept-Encoding: as in the AuTests here, and that no alternate for that resource has been cached. ATS will request the resource from the origin server, get a response with Vary: Accept-Encoding and cache it. Now, suppose that ATS receives another request for ae-0.com/path with Accept-Encoding: deflate. Should ATS reply out of cache? The user agent must be capable of processing a response with no content-encoding, so functionally everything should be fine if we reply out of cache. Here's the issue: why did the user agent send the Accept-Encoding: deflate header? The only reason to send that header is to let us know we can compress the content to save bandwidth, which is desirable. Section 12.5.3 of RFC 9110 provides evidence for this understanding, by suggesting that this optimization is so valuable for transferring large content that a server might even want to inform a client that it ought to consider accepting compressed content. ATS, by replying out of cache with uncompressed content, has ignored the possibility that the origin does have deflate compression. Up to this point, there is no problem with current ATS behavior, because we see that in fact ATS will not immediately reply out of cache, but will go to the origin server instead to see if it has an alternate with deflate encoding.

The real issue I see with the current behavior is that ATS, having received a response (404 Not Found in the case of the AuTest) from the origin for Accept-Encoding: deflate, decides to instead serve the 200 OK response. Accept-Encoding: deflate does not match Accept-Encoding: by the rules in RFC 9111 (which encompasses the rules ATS uses for normalizing the header). Is it acceptable for a proxy to reply from cache with a response that is different than the origin's response to the request, which it also knows about? The server is not constrained by Vary, so it may choose which encoding to send. The proxy is bound to obey the Vary header and therefore does not have the flexibility that the origin server has.

This has been fixed in the case 1 deflate test in this PR. I am not sure why the similar tests for cases 2, 3, and 4 still pass.

[JakeChampion]

@JosiahWI I believe cases 2,3, and 4 are also correct because we are normalising the request's accept-encoding header, and deflate is not one that we keep during that process. perhaps we need to update the autests to assert that no server-response is to be expected at all because it is a cache hit?

[JosiahWI]

This comment from #7458 seems to suggest that the normalize_ae option is not intended to affect the selection of an alternate from cache: #7458 (comment). I wonder if there are any other rules for normalization that do affect alternate selection from cache...

[JosiahWI]

@JakeChampion You are right, I had missed that ATS cleared the Accept-Encoding: field in some cases with normalize_ae set to a nonzero value. In those cases, it is correct to match the previous Accept-Encoding: response, as you said.

Assuming that normalize_ae does only affect the request to origin and not which alternate is served from cache, is it correct to serve the cached document after getting the 404 from the origin? My intuition says that the 404 should replace the existing 200 in cache, but I haven't checked the RFC, so I may be wrong about that.

Ideally, I think it would be good to serve directly from cache and skip the request to origin (unless the response is stale), as you suggested. (In other words, the Accept-Encoding header should be normalized before looking for alternates, not after).

[JosiahWI]

I'm curious why this was necessary.

[JakeChampion]

for some reason this autest for me would exit before the server started up - it looked to be due to a port conflict, which i solved with this hard-coding, this wasn't meant to be pushed up though, it was meant to be local only for me, apologies

[JosiahWI]

Bummer. These local AuTest issues can greatly impede ease of development but are not always easy to debug.

[JosiahWI]

I don't know if we needed quite that many assertions to be convinced, but it's certainly very convincing. :)

This looks good (assuming the assertions pass; even if they don't, we can back that out and get in the fix here if you want). Maybe @bneradt could double check the port thing since he is more familiar with AuTests, but I'm comfortable putting this in master.

[JakeChampion]

Apologies @JosiahWI - I did a rebase to tidy up the commits and it looks to have lost your approval

[JosiahWI]

I'll wait for CI to pass (it looked like there were problems that may have been related to port selection in some AuTests. 🤔) and approve again. We do squash on merge which will tidy up the commit history.

[JakeChampion]

I'll wait for CI to pass (it looked like there were problems that may have been related to port selection in some AuTests. 🤔) and approve again. We do squash on merge which will tidy up the commit history.

Looks like it all passed on ci now, yay. I think one of the next things I will look into is the flakey port selection in the autests because I do see that very often when running them locally as well.

[maskit]

Looks like you guys figured it out.

There is cryptic code that was added when the setting value 2 was introduced (70815db). I bet the line fixed on this PR was supposed to be checked in a similar way on the commit, but it's too cryptic IMO. I like the explicit comparison better 👍

trafficserver/src/iocore/cache/HttpTransactCache.cc

Lines 313 to 332 in 577cbed

	// vary_skip_mask is used as a bitmask, 0b01 or 0b11 depending on the presence of Vary.
	// This allows us to AND each of the four configs against it; Table:
	//
	// Conf Mask Conf Mask Conf Mask
	// ---- ---- ---- ---- ---- ----
	// 00 & 01 == false 01 & 01 == true 10 & 01 == false
	// 00 & 11 == false 01 & 11 == true 10 & 11 == true
	//
	// A true value means the check for that config can be skipped. Note: from a users
	// perspective, the configs are simply 0, 1 or 2.
	unsigned int vary_skip_mask = obj_origin_server_response->presence(MIME_PRESENCE_VARY) ? 1 : 3;
	// Make debug output happy
	q[1] = (q[2] = (q[3] = -2.0));
	// This content_field is used for a couple of headers, so get it first
	content_field = obj_origin_server_response->field_find(static_cast<std::string_view>(MIME_FIELD_CONTENT_TYPE));
	// Accept: header
	if (http_config_param->get_ignore_accept_mismatch() & vary_skip_mask) {