Do not invalidate cached resources upon error responses to unsafe methods#7999
Merged
bneradt merged 1 commit intoapache:masterfrom Jun 30, 2021
Merged
Do not invalidate cached resources upon error responses to unsafe methods#7999bneradt merged 1 commit intoapache:masterfrom
bneradt merged 1 commit intoapache:masterfrom
Conversation
bneradt
requested review from
SolidWallOfCode,
bryancall and
zwoop
as code owners
shukitchan
requested changes
Jun 27, 2021
Contributor
shukitchan
left a comment
shukitchan
left a comment
There was a problem hiding this comment.
I think we need to change https://github.com/apache/trafficserver/blob/master/plugins/esi/esi.cc#L1346 as well.
The "post" request will be intercepted to update the cache. Changing it to PUT will make it not go through those code.
randall
changed the title
…hods Before this change, cached resources would be deleted when the server replied to a request with an unsafe method (POST, PUT, etc.) with an error response code. This patch changes that behavior so that it is only deleted when the response has a successful response code. This seeks to comply more narrowly with the wording of RFC 7234, section-4.4: A cache MUST invalidate the effective request URI (Section 5.5 of [RFC7230]) when it receives a non-error response to a request with a method whose safety is unknown. Note that we were technically in compliance with this before since we always invalidated, regardless of the error response code. This, however, was too broad in that we invalidated when we didn't need to. Now we will only invalidate when the response code indicates a successful response to the request with an unsafe method. This also fixes our feature that caches responses to POST requests. --- This change is a reprisal of commit c75c797. One of the ESI plugin features, called packed-node, updates the cached resource for ESI includes with a "packed" version which offers more efficient parsing. The caching of this packed version is achieved via an internal POST to the cache, effectively replacing the resource with the packed version. This commit, however, breaks this mechanism because the updated resource is associated with a POST request, which differed from the subsequent GET request for the resource. This difference in method, per the changes made to HttpTransact for caching of POST requests, now results in the cached resource no longer matching the requst, which causes the ESI plugin to not find it, resulting eventually in assertions being tripped. Talking with Shu Kit, the lead developer of the ESI plugin, he observes that the packed node feature isn't used, has never been implemented to work completely, and our documentation advises users to avoid it. This being the case, he suggests we simply comment out this test. That is what this commit does, therefore.
bneradt
force-pushed
the
fix_unsafe_caching_post
branch
from
35d2a8d to
831d242
Compare
Contributor
Author
Thank you, @shukitchan , for these observations. Updating this code again causes issues with the ESI plugin. I've now updated this PR to follow your initial advice and simply not exercise the packed-node feature. |
shukitchan
approved these changes
Jun 30, 2021
Contributor
shukitchan
left a comment
shukitchan
left a comment
There was a problem hiding this comment.
Looks fine with the commented out test for ESI packed node feature
ezelkow1
approved these changes
Jun 30, 2021
moonchen
pushed a commit
to moonchen/trafficserver
that referenced
this pull request
Mar 7, 2022
* asf/master: (763 commits) rate_limit: Add a global hook to rate limit concurrent connections based on SNI (apache#8021) Fix uri_signing unit test for out of source builds (apache#8040) tests: Add conditions for BoringSSL and OpenSSL (apache#8045) change debug tags and make sure sni is printed on certain logs (apache#7673) Doc build in CI: build English docs with -W (apache#8039) When loading async SSL configuration file fails, log SSL error (apache#8036) Doc build: treat warnings as errors only by default (apache#8038) For test async_engine, export all symbols (apache#8037) Fix the server cert reload (apache#8030) Treat Sphinx doc build warnings as errors. (apache#8033) Stablize trace curl test in good_request_after_bad (apache#8032) Doc: Update documentation to build cleanly in Sphinx 3. Require Sphinx 3 or better. (apache#7978) Docs: Fix pre-formatting for ratelimit plugin (apache#7986) Make it slightly harder to dump private keys to logs (apache#8029) tls_bad_alpn: Add an openssl version skip check (apache#8026) per thread jemalloc arena for MADV_DONTDUMP (apache#7501) Adds a new rm-destination, this lets you specify either QUERY or PATH, and be able to drop them from the incoming request (apache#8025) Fix HPACK eviction iterator manipulation (apache#8004) Do not invalidate cached resources upon error responses to unsafe methods (apache#7999) Cleanup SSLUtils (apache#8007) ...
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Before this change, cached resources would be deleted when the server
replied to a request with an unsafe method (POST, PUT, etc.) with an
error response code. This patch changes that behavior so that it is only
deleted when the response has a successful response code.
This seeks to comply more narrowly with the wording of RFC 7234,
section-4.4:
A cache MUST invalidate the effective request URI (Section 5.5 of
[RFC7230]) when it receives a non-error response to a request with a
method whose safety is unknown.
Note that we were technically in compliance with this before since we
always invalidated, regardless of the error response code. This,
however, was too broad in that we invalidated when we didn't need to.
Now we will only invalidate when the response code indicates a
successful response to the request with an unsafe method.
This also fixes our feature that caches responses to POST requests.
This change is a reprisal of commit
c75c797. One of the ESI plugin
features, called packed-node, updates the cached resource for ESI
includes with a "packed" version which offers more efficient parsing.
The caching of this packed version is achieved via an internal POST to
the cache, effectively replacing the resource with the packed version.
This commit, however, breaks this mechanism because the updated resource
is associated with a POST request, which differed from the subsequent
GET request for the resource. This difference in method, per the changes
made to HttpTransact for caching of POST requests, now results in the
cached resource no longer matching the requst, which causes the ESI
plugin to not find it, resulting eventually in assertions being tripped.
Talking with Shu Kit, the lead developer of the ESI plugin, he observes
that the packed node feature isn't used, has never been implemented to
work completely, and our documentation advises users to avoid it. This
being the case, he suggests we simply comment out this test. That is
what this commit does, therefore.