Skip to content

Conversation

@bneradt
Copy link
Contributor

@bneradt bneradt commented Dec 2, 2021

ATS initially experiences cache lookups as a HIT for resources that we
ultimately cannot use due to a need for authentication or if the methods
for the incoming request and that of the request for the cached response
don't match. Our TSHttpTxnCacheLookupStatusGet plugin interface returned
the initial HIT status instead of marking these as MISSES. This patch
changes the status to return a MISS in these circumstances.

Closes #8539

ATS initially experiences cache lookups as a HIT for resources that we
ultimately cannot use due to a need for authentication or if the methods
for the incoming request and that of the request for the cached response
don't match. Our TSHttpTxnCacheLookupStatusGet plugin interface returned
the initial HIT status instead of marking these as MISSES. This patch
changes the status to return a MISS in these circumstances.

Closes apache#8539
@bneradt
Copy link
Contributor Author

bneradt commented Dec 7, 2021

@SolidWallOfCode volunteered to take a look at this.

@SolidWallOfCode
Copy link
Member

Why have a plugin for checking the cache status rather than a custom log?

@bneradt
Copy link
Contributor Author

bneradt commented Jan 18, 2022

Why have a plugin for checking the cache status rather than a custom log?

Thank you for taking a look at this PR.

The issue this addresses (see the link) is for the compress plugin. Namely, this code block:

case TS_EVENT_HTTP_CACHE_LOOKUP_COMPLETE: {
int obj_status;
if (TS_ERROR != TSHttpTxnCacheLookupStatusGet(txnp, &obj_status) && (TS_CACHE_LOOKUP_HIT_FRESH == obj_status)) {
if (hc != nullptr) {
info("handling compression of cached object");
if (transformable(txnp, false, hc, &compress_type, &algorithms)) {
compress_transform_add(txnp, hc, compress_type, algorithms);

The plugin is not using the status for logging. Rather, the plugin grabs the cache status via TSHttpTxnCacheLookupStatusGet and conditionally performs logic if ATS is serving the object out of cache. Without this patch, the TS_CACHE_LOOKUP_HIT_FRESH is provided to the plugin even when the object is deemed to be something it cannot in fact reply to the client with (such as the request is a POST method but the cached object is a response to a GET request). In this case, it's a bug to call such cached objects a hit for these requests and the plugin is negatively impacted by this. This patch corrects the status in such cases to a miss.

@bneradt bneradt merged commit 080e236 into apache:master Jan 18, 2022
@bneradt bneradt deleted the fix_cache_status_on_post branch January 18, 2022 15:32
zwoop pushed a commit that referenced this pull request Jan 18, 2022
ATS initially experiences cache lookups as a HIT for resources that we
ultimately cannot use due to a need for authentication or if the methods
for the incoming request and that of the request for the cached response
don't match. Our TSHttpTxnCacheLookupStatusGet plugin interface returned
the initial HIT status instead of marking these as MISSES. This patch
changes the status to return a MISS in these circumstances.

Closes #8539

(cherry picked from commit 080e236)
@zwoop
Copy link
Contributor

zwoop commented Jan 18, 2022

Cherry-picked to v9.2.x

@zwoop zwoop modified the milestones: 10.0.0, 9.2.0 Jan 18, 2022
bneradt added a commit to bneradt/trafficserver that referenced this pull request Jan 19, 2022
bneradt added a commit to bneradt/trafficserver that referenced this pull request Jan 31, 2022
Fixing apache#8539 is easy to do in a common case, but it causing issues in
corner cases. For now I'm going to revert the fixes for this and circle
back around later to see whether this can be solved in a different way.

This reverts the following commits:

Revert "Commenting TSHttpTxnCacheLookupStatusGet need_to_revalidate (apache#8621)"
This reverts commit 4678ae3

Revert "TSHttpTxnCacheLookupStatusGet: call need_to_revalidate (apache#8617)"
This reverts commit 235c44a.

Revert "TSHttpTxnCacheLookupStatusGet: handle cannot respond cases (apache#8545)"
This reverts commit 080e236.
@bneradt bneradt mentioned this pull request Jan 31, 2022
bneradt added a commit that referenced this pull request Jan 31, 2022
Fixing #8539 is easy to do in a common case, but it causing issues in
corner cases. For now I'm going to revert the fixes for this and circle
back around later to see whether this can be solved in a different way.

This reverts the following commits:

Revert "Commenting TSHttpTxnCacheLookupStatusGet need_to_revalidate (#8621)"
This reverts commit 4678ae3

Revert "TSHttpTxnCacheLookupStatusGet: call need_to_revalidate (#8617)"
This reverts commit 235c44a.

Revert "TSHttpTxnCacheLookupStatusGet: handle cannot respond cases (#8545)"
This reverts commit 080e236.
@bneradt bneradt added the Revert label Jan 31, 2022
@bneradt
Copy link
Contributor Author

bneradt commented Jan 31, 2022

Adding the 9.2.x project as this will be reverted in 9.2.x when this is cherry-picked back:
#8637

zwoop pushed a commit that referenced this pull request Feb 2, 2022
Fixing #8539 is easy to do in a common case, but it causing issues in
corner cases. For now I'm going to revert the fixes for this and circle
back around later to see whether this can be solved in a different way.

This reverts the following commits:

Revert "Commenting TSHttpTxnCacheLookupStatusGet need_to_revalidate (#8621)"
This reverts commit 4678ae3

Revert "TSHttpTxnCacheLookupStatusGet: call need_to_revalidate (#8617)"
This reverts commit 235c44a.

Revert "TSHttpTxnCacheLookupStatusGet: handle cannot respond cases (#8545)"
This reverts commit 080e236.

(cherry picked from commit 2f1bd0f)
@zwoop zwoop removed this from the 9.2.0 milestone Feb 2, 2022
moonchen pushed a commit to moonchen/trafficserver that referenced this pull request Mar 17, 2022
* asf/9.2.x:
  Updated ChangeLog
  TSHttpTxnCacheLookupStatusGet: handle cannot respond cases (apache#8545)
  Update to Proxy Verifier version v2.3.0 (apache#8608)
  Don't use Http1ClientTransaction as an event handler (apache#8609)
  Eliminate erroneous self-loop error on transparent mode (apache#8586)
  Clean up of next hop HostRecord class. (apache#8585)
  Propagate accept options to HTTP/2 (apache#8594)
  add --with-mimalloc option (apache#8233)
  Fix transparent mode documentation (apache#8593)
  Docs: Slack instead of irc (apache#8599)
  LogFilter: fix NULL termination check (apache#8603)
  Fixes a scoping bug that leads to "sticky" weights (apache#8606)
@zwoop zwoop added this to the 10.0.0 milestone Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CACHE_LOOKUP_HIT_FRESH status for POST request is not good

3 participants