You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2021/05/20 21:01:48 UTC
[GitHub] [trafficserver] bneradt opened a new pull request #7864: Do not invalidate cached resources upon error responses
bneradt opened a new pull request #7864:
URL: https://github.com/apache/trafficserver/pull/7864
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 fixes #7839
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] rob05c commented on a change in pull request #7864: Do not invalidate cached resources upon error responses
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7864:
URL: https://github.com/apache/trafficserver/pull/7864#discussion_r636979138
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -4485,7 +4487,21 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
}
} else if (s->cache_info.action == CACHE_DO_DELETE) {
- // do nothing
+ if (!cacheable && is_response_error_code(server_response_code) &&
+ (s->hdr_info.server_request.method_get_wksidx() == HTTP_WKSIDX_DELETE ||
Review comment:
Also, not part of this PR, but I believe `does_method_require_cache_copy_deletion` may actually be violating RFC7234 by not returning true for PATCH or unknown methods. I'll try to follow up on that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt merged pull request #7864: Do not invalidate cached resources upon error responses
Posted by GitBox <gi...@apache.org>.
bneradt merged pull request #7864:
URL: https://github.com/apache/trafficserver/pull/7864
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7864: Do not invalidate cached resources upon error responses
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7864:
URL: https://github.com/apache/trafficserver/pull/7864#discussion_r651904553
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -5896,6 +5932,17 @@ HttpTransact::is_cache_response_returnable(State *s)
SET_VIA_STRING(VIA_DETAIL_CACHE_LOOKUP, VIA_DETAIL_MISS_METHOD);
return false;
}
+ // If we are caching responses to POST, make sure that our cached resource
+ // has a method that matches the incoming requests's method. If not, then we
+ // cannot reply with the cached resource. That is, we cannot reply to an
+ // incoming GET request with a response to a previous POST request.
+ int const client_request_method = s->hdr_info.client_request.method_get_wksidx();
+ int const cached_request_method = s->cache_info.object_read->request_get()->method_get_wksidx();
+ if (s->http_config_param->cache_post_method == 1 && client_request_method != cached_request_method) {
Review comment:
Why check the config params? Are there cases where the item should be served even if the methods don't match?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a change in pull request #7864: Do not invalidate cached resources upon error responses
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #7864:
URL: https://github.com/apache/trafficserver/pull/7864#discussion_r651937484
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -2070,6 +2070,14 @@ HttpTransact::OSDNSLookup(State *s)
} else if (s->cache_lookup_result == CACHE_LOOKUP_MISS || s->cache_info.action == CACHE_DO_NO_ACTION) {
TRANSACT_RETURN(SM_ACTION_API_OS_DNS, HandleCacheOpenReadMiss);
// DNS lookup is done if the lookup failed and need to call Handle Cache Open Read Miss
+ } else if (s->cache_info.action == CACHE_PREPARE_TO_WRITE && s->http_config_param->cache_post_method == 1 &&
Review comment:
Logically it can be combined. But note that this is actually the third condition that returns `TRANSACT_RETURN(SM_ACTION_API_OS_DNS, HandleCacheOpenReadMiss);`. Since this block of conditions is deriving from state how it got here, and therefore what should be done from here, each else clause corresponds to a different historical path. As such, most of the blocks have an explanatory comment, which I've now added for my new condition. I prefer this over combining these three conditions into one big conditional statement because it makes the various state transitions more comprehensible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a change in pull request #7864: Do not invalidate cached resources upon error responses
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #7864:
URL: https://github.com/apache/trafficserver/pull/7864#discussion_r639122011
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -4485,7 +4487,21 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
}
} else if (s->cache_info.action == CACHE_DO_DELETE) {
- // do nothing
+ if (!cacheable && is_response_error_code(server_response_code) &&
+ (s->hdr_info.server_request.method_get_wksidx() == HTTP_WKSIDX_DELETE ||
Review comment:
Thanks again Rob! I appreciate your thoughtful review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7864: Do not invalidate cached resources upon error responses
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7864:
URL: https://github.com/apache/trafficserver/pull/7864#discussion_r651905229
##########
File path: proxy/http/HttpTransactHeaders.cc
##########
@@ -87,9 +87,24 @@ HttpTransactHeaders::is_this_method_supported(int the_scheme, int the_method)
bool
HttpTransactHeaders::is_method_safe(int method)
{
+ // See RFC 7231, section 4.2.1.
return (method == HTTP_WKSIDX_GET || method == HTTP_WKSIDX_OPTIONS || method == HTTP_WKSIDX_HEAD || method == HTTP_WKSIDX_TRACE);
}
+bool
+HttpTransactHeaders::is_method_unsafe(int method)
Review comment:
This seems overkill - it saves putting a "!" in an expression.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a change in pull request #7864: Do not invalidate cached resources upon error responses
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #7864:
URL: https://github.com/apache/trafficserver/pull/7864#discussion_r651939922
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -5896,6 +5932,17 @@ HttpTransact::is_cache_response_returnable(State *s)
SET_VIA_STRING(VIA_DETAIL_CACHE_LOOKUP, VIA_DETAIL_MISS_METHOD);
return false;
}
+ // If we are caching responses to POST, make sure that our cached resource
+ // has a method that matches the incoming requests's method. If not, then we
+ // cannot reply with the cached resource. That is, we cannot reply to an
+ // incoming GET request with a response to a previous POST request.
+ int const client_request_method = s->hdr_info.client_request.method_get_wksidx();
+ int const cached_request_method = s->cache_info.object_read->request_get()->method_get_wksidx();
+ if (s->http_config_param->cache_post_method == 1 && client_request_method != cached_request_method) {
Review comment:
Yeah, good point. There probably isn't a good reason to return a resource for a mismatched method. I added the config check because it reduced the scope of the impact of my change, reducing the risk of this patch. But I think it should be safe to remove.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #7864: Do not invalidate cached resources upon error responses
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #7864:
URL: https://github.com/apache/trafficserver/pull/7864#issuecomment-845518389
[approve ci autest]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] rob05c commented on a change in pull request #7864: Do not invalidate cached resources upon error responses
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7864:
URL: https://github.com/apache/trafficserver/pull/7864#discussion_r636474120
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -288,24 +288,26 @@ is_localhost(const char *name, int len)
return (len == (sizeof(local) - 1)) && (memcmp(name, local, len) == 0);
}
+/** Return whether the response code indicates an error response. */
inline static bool
-is_response_simple_code(HTTPStatus response_code)
+is_response_error_code(HTTPStatus response_code)
Review comment:
Is it theoretically valid for this to ever be different than `is_response_simple_code(response_code) || is_response_unavailable_code(response_code)` ?
If not, would it be better to make it that, to avoid duplication, one of them getting out of sync, etc?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a change in pull request #7864: Do not invalidate cached resources upon error responses
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #7864:
URL: https://github.com/apache/trafficserver/pull/7864#discussion_r636498675
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -288,24 +288,26 @@ is_localhost(const char *name, int len)
return (len == (sizeof(local) - 1)) && (memcmp(name, local, len) == 0);
}
+/** Return whether the response code indicates an error response. */
inline static bool
-is_response_simple_code(HTTPStatus response_code)
+is_response_error_code(HTTPStatus response_code)
Review comment:
Due to their simplicity, I'd prefer to keep the semantic value of having a function that names the intention of the condition.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] rob05c commented on a change in pull request #7864: Do not invalidate cached resources upon error responses
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7864:
URL: https://github.com/apache/trafficserver/pull/7864#discussion_r636478618
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -4485,7 +4487,21 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
}
} else if (s->cache_info.action == CACHE_DO_DELETE) {
- // do nothing
+ if (!cacheable && is_response_error_code(server_response_code) &&
+ (s->hdr_info.server_request.method_get_wksidx() == HTTP_WKSIDX_DELETE ||
Review comment:
I think this should omit safe methods, rather than include known unsafe ones. (GET, HEAD, OPTIONS, TRACE; rfc7231)
HTTP doesn't expressly forbid unknown methods, and methods can be added in a later spec, which would qualify as "a method whose safety is unknown."
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] rob05c commented on a change in pull request #7864: Do not invalidate cached resources upon error responses
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7864:
URL: https://github.com/apache/trafficserver/pull/7864#discussion_r636977969
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -4485,7 +4487,21 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
}
} else if (s->cache_info.action == CACHE_DO_DELETE) {
- // do nothing
+ if (!cacheable && is_response_error_code(server_response_code) &&
+ (s->hdr_info.server_request.method_get_wksidx() == HTTP_WKSIDX_DELETE ||
Review comment:
Putting a little more of our conversation here for posterity:
This code doesn't introduce spec violations, but it might not let us cache in all scenarios when we could.
Both the method and the codes _could_ be a little broader (including unknown methods, 1xx, 6xx+), but it's not violating anything as-is. And being conservative with behavior changes is probably a good thing.
But overall, I do think this is an improvement and not introducing spec violations.
I was also asked to verify the `is_response_*_code` changes aren't changing parent or strategy behavior. That part LGTM, behavior appears to be unchanged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] rob05c commented on a change in pull request #7864: Do not invalidate cached resources upon error responses
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7864:
URL: https://github.com/apache/trafficserver/pull/7864#discussion_r636481742
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -4485,7 +4487,21 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
}
} else if (s->cache_info.action == CACHE_DO_DELETE) {
- // do nothing
+ if (!cacheable && is_response_error_code(server_response_code) &&
+ (s->hdr_info.server_request.method_get_wksidx() == HTTP_WKSIDX_DELETE ||
Review comment:
`is_response_error_code` as-written will also return false for codes >= 600, and true for 1xx codes. No HTTP spec defines a 600+ code, but RFC7234 says "Here, a "non-error response" is one with a 2xx (Successful) or 3xx (Redirection) status code."
In this particular case, I think we should follow the spec rather than whatever the client, origin, ATS operator, or ATS programmer configured for general error codes, and check precisely `>= 200 && <= 300`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a change in pull request #7864: Do not invalidate cached resources upon error responses
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #7864:
URL: https://github.com/apache/trafficserver/pull/7864#discussion_r639122459
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -3078,7 +3086,8 @@ HttpTransact::build_response_from_cache(State *s, HTTPWarningCode warning_code)
// fall through
default:
SET_VIA_STRING(VIA_DETAIL_CACHE_LOOKUP, VIA_DETAIL_HIT_SERVED);
- if (s->method == HTTP_WKSIDX_GET || s->api_resp_cacheable == true) {
+ if (s->method == HTTP_WKSIDX_GET || (s->http_config_param->cache_post_method == 1 && s->method == HTTP_WKSIDX_POST) ||
+ s->api_resp_cacheable == true) {
Review comment:
This is the change that fixes caching of responses to POST requests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #7864: Do not invalidate cached resources upon error responses
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #7864:
URL: https://github.com/apache/trafficserver/pull/7864#issuecomment-854242535
I tested this in our production simulator and things looked fine. I didn't see any unexpected behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a change in pull request #7864: Do not invalidate cached resources upon error responses
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #7864:
URL: https://github.com/apache/trafficserver/pull/7864#discussion_r636528574
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -4485,7 +4487,21 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
}
} else if (s->cache_info.action == CACHE_DO_DELETE) {
- // do nothing
+ if (!cacheable && is_response_error_code(server_response_code) &&
+ (s->hdr_info.server_request.method_get_wksidx() == HTTP_WKSIDX_DELETE ||
Review comment:
Talking with Rob offline, we think it would be good to change out the `is_response_error_code` function for `is_response_success_code` and negate that instead. I'll do that and update the PR.
Thanks Rob!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #7864: Do not invalidate cached resources upon error responses
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #7864:
URL: https://github.com/apache/trafficserver/pull/7864#issuecomment-854242535
I tested this in our production simulator and things looked fine. I didn't see any unexpected behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7864: Do not invalidate cached resources upon error responses
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7864:
URL: https://github.com/apache/trafficserver/pull/7864#discussion_r651421311
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -2070,6 +2070,14 @@ HttpTransact::OSDNSLookup(State *s)
} else if (s->cache_lookup_result == CACHE_LOOKUP_MISS || s->cache_info.action == CACHE_DO_NO_ACTION) {
TRANSACT_RETURN(SM_ACTION_API_OS_DNS, HandleCacheOpenReadMiss);
// DNS lookup is done if the lookup failed and need to call Handle Cache Open Read Miss
+ } else if (s->cache_info.action == CACHE_PREPARE_TO_WRITE && s->http_config_param->cache_post_method == 1 &&
Review comment:
Why isn't this a logical or condition on the previous clause?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a change in pull request #7864: Do not invalidate cached resources upon error responses
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #7864:
URL: https://github.com/apache/trafficserver/pull/7864#discussion_r636495052
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -4485,7 +4487,21 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
}
} else if (s->cache_info.action == CACHE_DO_DELETE) {
- // do nothing
+ if (!cacheable && is_response_error_code(server_response_code) &&
+ (s->hdr_info.server_request.method_get_wksidx() == HTTP_WKSIDX_DELETE ||
Review comment:
Thank you for the review! These are good observations worth discussing.
Note that the logic this patch adds isn't applying deletion, it's undoing the deletion action in particular circumstances. So the way to apply a careful stance here is to be particular, not broad. Thus I think the concerns you raise are actually satisfied by this patch:
* The methods under consideration are those we know are unsafe, explicitly. If we don't know their safety, then the conditional block is not entered and the resource will be deleted. So for unknown methods, deletion will be preserved. (Or, I should say, if it isn't it's not the fault of this patch!)
* The response codes under consideration are those that are known to be error ones (400-599, inclusive). If they are anything else, then the deletion action will be kept. Thus for 2xx, 1xx, or any theoretical 6xx codes, deletion will be kept.
Related to this: I'm trying to keep the changed behavior narrow. An earlier revision of this patch during my development undid deletion too broadly and broke the splice plugin's assumptions about deletion. Changing caching behavior is a bit risky, so I'm trying to make the changed behavior judiciously narrow while still correct and addressing the linked issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org