Skip to content

cache_range_requests plugin: don't require 206 Partial Content reason string #8488

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
traeak merged 3 commits into from
Nov 9, 2021
Merged

cache_range_requests plugin: don't require 206 Partial Content reason string #8488

traeak merged 3 commits into from
Nov 9, 2021

Conversation

@traeak
Copy link
Contributor

@traeak traeak commented Nov 2, 2021 •
edited
Loading

The cache_range_requests plugin expects the parent to have the reason set to "Partial Content" (case insensiitve) in addition to the expecting a 206 status.

Instead of checking for "Partial Content" in the header, this fix tracks origin status for uncached behavior.

For incoming client range request:

  • For cached assets, the plugin always converts 200 to 206 (cachekey has the range).
  • For fetched assets, only cache and retag client status to 206 IF origin returned a 206.

Additional changes:

  • Ensure TS_NULL_MLOC is always used with TSMLoc
  • Refactor some logical expression scopes to ensure TSHandleMLocRelease is appropriately called (potential for memory leaks in fail conditions).

closes #8481

@ezelkow1
Copy link
Member

ezelkow1 commented Nov 2, 2021

[approve ci]

@traeak traeak force-pushed the crr_set_reason branch 2 times, most recently from 32f5342 to 7590ef3 Compare November 2, 2021 15:37
@mlibbey
Copy link
Contributor

mlibbey commented Nov 2, 2021

Since the reason text in the status codes are RFC recommendations (https://datatracker.ietf.org/doc/html/rfc7231#section-6.1 -- The reason phrases listed here are only recommendations
-- they can be replaced by local equivalents without affecting the protocol), wouldn't it be better just to ignore the reason code entirely?

@traeak
Copy link
Contributor Author

traeak commented Nov 2, 2021 •
edited
Loading

Since the reason text in the status codes are RFC recommendations (https://datatracker.ietf.org/doc/html/rfc7231#section-6.1 -- The reason phrases listed here are only recommendations -- they can be replaced by local equivalents without affecting the protocol), wouldn't it be better just to ignore the reason code entirely?

That's why the PR is still in draft. If the plugin tries to pass a legit 200 down from a server response it shouldn't retag it 206 while sending it to the client... should have a solution here soon.

@traeak traeak changed the title cache_range_requests plugin: hard set reason string for handled 206 response cache_range_requests plugin: Ignore reason string with 206 Nov 2, 2021
@traeak traeak changed the title cache_range_requests plugin: Ignore reason string with 206 cache_range_requests plugin: don't require 206 Partial Content reason string Nov 2, 2021
@traeak
Copy link
Contributor Author

traeak commented Nov 3, 2021

[approve ci autest]

@traeak traeak marked this pull request as ready for review November 3, 2021 18:21
@traeak traeak requested a review from bryancall as a code owner November 3, 2021 18:21
@traeak traeak added the Plugins label Nov 4, 2021
@traeak traeak added this to the 10.0.0 milestone Nov 4, 2021
@bryancall bryancall requested a review from ywkaras November 9, 2021 00:20
ywkaras
ywkaras reviewed Nov 9, 2021
View reviewed changes
ywkaras
ywkaras reviewed Nov 9, 2021
View reviewed changes
@traeak traeak merged commit 3fed79a into apache:master Nov 9, 2021
elsloo added a commit to elsloo/trafficserver that referenced this pull request May 5, 2022
@elsloo
Fixes issues with the CRR plugin introduced in apache#8488
acb60d3 
* Fixes an issue that leads to an incorrect assumption about the origin status code on cache hit

* Fixes the content revalidation case, as original implementation did not recognize the 304
elsloo added a commit to elsloo/trafficserver that referenced this pull request May 5, 2022
@elsloo
Fixes issues with the CRR plugin introduced in apache#8488
13460ab 
* Fixes an issue that leads to an incorrect assumption about the origin status code on cache hit

* Fixes the content revalidation case, as original implementation did not recognize the 304
elsloo added a commit to elsloo/trafficserver that referenced this pull request May 5, 2022
@elsloo
Fixes issues with the CRR plugin introduced in apache#8488
a2a9a67 
* Fixes an issue that leads to an incorrect assumption about the origin status code on cache hit

* Fixes the content revalidation case, as original implementation did not recognize the 304
@elsloo elsloo mentioned this pull request May 5, 2022
Merged
traeak pushed a commit that referenced this pull request May 10, 2022
@elsloo
Fixes issues with the CRR plugin introduced in #8488 (#8828)
aedb7fb 
* Fixes an issue that leads to an incorrect assumption about the origin status code on cache hit

* Fixes the content revalidation case, as original implementation did not recognize the 304
@bryancall bryancall mentioned this pull request Jun 6, 2022
Merged
zwoop pushed a commit that referenced this pull request Jun 7, 2022
@traeak @zwoop
cache_range_requests plugin: don't require 206 Partial Content reason…
20f4496 
… string (#8488)

* cache_range_requests plugin: don't rely on 'Partial Content' reason

* crr: remove unnecessary cast in TSContCreate

* crr: move txndata* declaration to interior scope

(cherry picked from commit 3fed79a)
@zwoop
Copy link
Contributor

zwoop commented Jun 7, 2022

Cherry-picked to v9.2.x

@zwoop zwoop modified the milestones: 10.0.0, 9.2.0 Jun 7, 2022
zwoop pushed a commit that referenced this pull request Jun 8, 2022
@elsloo @zwoop
Fixes issues with the CRR plugin introduced in #8488 (#8828)
0d3c9a6 
* Fixes an issue that leads to an incorrect assumption about the origin status code on cache hit

* Fixes the content revalidation case, as original implementation did not recognize the 304

(cherry picked from commit aedb7fb)
@traeak traeak deleted the crr_set_reason branch January 13, 2023 18:32
masaori335 pushed a commit to masaori335/trafficserver that referenced this pull request Feb 21, 2023
@zwoop
Merge remote-tracking branch 'asf/9.2.x' into 9.2.x
cd6a6cc 
* asf/9.2.x:
  Updated ChangeLog
  Add proxy.process.hostdb.total_serve_stale (apache#8873)
  Allow for long Http* error.log lines (apache#8855)
  mkdfa.c is not being used and doesn't compile with gcc 12.1.1 (apache#8838)
  Add compatibility define when building with OpenSSL3 (apache#8837)
  Make post-early-return Au test more robust. (apache#8832)
  Add support for caching complete responses to the cache range requests plugin (apache#8816)
  Fixes issues with the CRR plugin introduced in apache#8488 (apache#8828)
  slice and cache_range_requests: allow header override (apache#8666) (apache#8898)
  Removed references to the throttle option from the slice plugin. (apache#8373) (apache#8897)
  cache_range_requests plugin: don't require 206 Partial Content reason string (apache#8488)
  Improve option processing in cache promote (apache#8501)
  Change parent_select Init func to constructor (apache#8853)
  Fix "is is" typos. (apache#8866)
  Eliminate duplicate words. (apache#8870)
  money_trace: allow custom header, change span-id gen, opt to create if none (apache#8655)
  Update HostDBContinuation timeout handling to clear pending queue. (apache#8480)
  Upgrade to Proxy Verifier 2.4.0. (apache#8884)
  Change ats_scoped_obj to std::unique_ptr . (apache#8882)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ywkaras ywkaras ywkaras approved these changes

@bryancall bryancall Awaiting requested review from bryancall

Assignees

@traeak traeak

Labels

Plugins

Projects

None yet

Milestone

9.2.0

Development

Successfully merging this pull request may close these issues.

Wrong response on range request

5 participants

@traeak @ezelkow1 @mlibbey @zwoop @ywkaras