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 2020/12/18 05:59:28 UTC
[GitHub] [trafficserver] shinya1020 opened a new pull request #7401: Fix unnecessary addition of Expires header
shinya1020 opened a new pull request #7401:
URL: https://github.com/apache/trafficserver/pull/7401
TrafficServer accidentally adds Expires header generated by negative_caching_lifetime to non-negative responses. Due to the addition of Expires header, the expiration of non-negative cache that has neither Cache-Control nor Expires is determined by `proxy.config.http.negative_caching_lifetime` . In this case, the expiration should be determined by heuristic expiration (between `proxy.config.http.cache.heuristic_min_lifetime` and `proxy.config.http.cache.heuristic_max_lifetime` ).
Also, this breaks the response to a conditional request.
This is caused by https://github.com/apache/trafficserver/commit/8eb68266167d8f8b3fa3a00ca9f6b7889e8ec101 .
# How to reproduce
## settings
### records.config
```
CONFIG proxy.config.http.negative_caching_enabled INT 1
CONFIG proxy.config.http.negative_caching_lifetime INT 10
CONFIG proxy.config.http.cache.required_headers INT 0
CONFIG proxy.config.http.insert_response_via_str INT 2
```
### remap.config
```
map / https://httpbin.org/
```
## unnecessary addition of Expires header
1. purge cache just in case
```
$ curl -X PURGE -D - -s -o /dev/null 'http://localhost:8080/get'
```
2. send request
```
$ curl -D - -s -o /dev/null 'http://localhost:8080/get' | grep -E '(Date|Expires|Via|Age)'
Date: Fri, 18 Dec 2020 00:46:09 GMT
Age: 0
Via: https/1.1 traffic_server (ApacheTrafficServer/10.0.0 [cMsSfW])
```
3. send request within 10 seconds from step 2
```
$ curl -D - -s -o /dev/null 'http://localhost:8080/get' | grep -E '(Date|Expires|Via|Age)'
Date: Fri, 18 Dec 2020 00:46:09 GMT
Expires: Fri, 18 Dec 2020 00:46:19 GMT
Age: 7
Via: http/1.1 traffic_server (ApacheTrafficServer/10.0.0 [cHs f ])
```
The cached response should not contain Expires header.
4. send request after 10 seconds from step 2
```
$ curl -D - -s -o /dev/null 'http://localhost:8080/get' | grep -E '(Date|Expires|Via|Age)'
Date: Fri, 18 Dec 2020 00:46:20 GMT
Age: 2
Via: https/1.1 traffic_server (ApacheTrafficServer/10.0.0 [cSsSfU])
```
## invalid response to conditional request
1. purge cache just in case
```
$ curl -X PURGE -D - -s -o /dev/null 'http://localhost:8080/response-headers?Last-Modified=Fri%2C+18+Dec+2020+00%3A00%3A00+GMT'
```
2. send request
```
$ curl -D - -s -o /dev/null -H 'If-Modified-Since: Fri, 18 Dec 2020 00:00:00 GMT' 'http://localhost:8080/response-headers?Last-Modified=Fri%2C+18+Dec+2020+00%3A00%3A00+GMT' | grep -E '(HTTP|Via)'
HTTP/1.1 200 OK
Via: https/1.1 traffic_server (ApacheTrafficServer/10.0.0 [cMsSfW])
```
In this case, the response code should be `304 Not Modified` .
----------------------------------------------------------------
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] masaori335 commented on a change in pull request #7401: Fix unnecessary addition of Expires header
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #7401:
URL: https://github.com/apache/trafficserver/pull/7401#discussion_r546531334
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -4464,7 +4464,7 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
// before issuing a 304
if (s->cache_info.action == CACHE_DO_WRITE || s->cache_info.action == CACHE_DO_NO_ACTION ||
s->cache_info.action == CACHE_DO_REPLACE) {
- if (s->is_cacheable_and_negative_caching_is_enabled) {
+ if (s->is_cacheable_and_negative_caching_is_enabled && is_negative_caching_appropriate(s)) {
Review comment:
I worry about `s->is_cacheable_and_negative_caching_is_enabled` is not set correctly in line 4405. Prior to the 8eb68266167d8f8b3fa3a00ca9f6b7889e8ec101, when this function set `s->is_cacheable_and_negative_caching_is_enabled` (renamed from `s-> negative_caching`), it called `is_negative_caching_appropriate(s)`. Should we roll it back?
```
4405 - s->negative_caching = is_negative_caching_appropriate(s) && cacheable;
4405 + s->is_cacheable_and_negative_caching_is_enabled = cacheable && s->txn_conf->negative_caching_enabled;
```
https://github.com/apache/trafficserver/commit/8eb68266167d8f8b3fa3a00ca9f6b7889e8ec101#diff-7f286e5169bc2d28c13f1ceaf3bb482b36cdb0743fbac4040fd4cd5a9c0897c1L4405-R4405
/cc @bneradt
----------------------------------------------------------------
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] shinya1020 commented on a change in pull request #7401: Fix unnecessary addition of Expires header
Posted by GitBox <gi...@apache.org>.
shinya1020 commented on a change in pull request #7401:
URL: https://github.com/apache/trafficserver/pull/7401#discussion_r547600314
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -4464,7 +4464,7 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
// before issuing a 304
if (s->cache_info.action == CACHE_DO_WRITE || s->cache_info.action == CACHE_DO_NO_ACTION ||
s->cache_info.action == CACHE_DO_REPLACE) {
- if (s->is_cacheable_and_negative_caching_is_enabled) {
+ if (s->is_cacheable_and_negative_caching_is_enabled && is_negative_caching_appropriate(s)) {
Review comment:
@bneradt Thank you for your suggestions. I confirmed that Expires header issue no longer occurs on your branch.
----------------------------------------------------------------
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] zwoop commented on pull request #7401: Fix unnecessary addition of Expires header
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7401:
URL: https://github.com/apache/trafficserver/pull/7401#issuecomment-756358936
Cherry-picked to 8.1.x
Cherry-picked to v9.0.x
----------------------------------------------------------------
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] masaori335 commented on a change in pull request #7401: Fix unnecessary addition of Expires header
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #7401:
URL: https://github.com/apache/trafficserver/pull/7401#discussion_r547549875
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -4464,7 +4464,7 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
// before issuing a 304
if (s->cache_info.action == CACHE_DO_WRITE || s->cache_info.action == CACHE_DO_NO_ACTION ||
s->cache_info.action == CACHE_DO_REPLACE) {
- if (s->is_cacheable_and_negative_caching_is_enabled) {
+ if (s->is_cacheable_and_negative_caching_is_enabled && is_negative_caching_appropriate(s)) {
Review comment:
I verified the suggested change fixes the leak 👍
----------------------------------------------------------------
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] masaori335 merged pull request #7401: Fix unnecessary addition of Expires header
Posted by GitBox <gi...@apache.org>.
masaori335 merged pull request #7401:
URL: https://github.com/apache/trafficserver/pull/7401
----------------------------------------------------------------
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] zwoop commented on pull request #7401: Fix unnecessary addition of Expires header
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7401:
URL: https://github.com/apache/trafficserver/pull/7401#issuecomment-756390472
Well, pooey, I don't think this patch is appropriate for 8.1.x. If this fix is needed there, I think it needs a different PR. At least not without #7361 (thanks @randall ) but not sure we should pull that in ?
----------------------------------------------------------------
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] masaori335 commented on pull request #7401: Fix unnecessary addition of Expires header
Posted by GitBox <gi...@apache.org>.
masaori335 commented on pull request #7401:
URL: https://github.com/apache/trafficserver/pull/7401#issuecomment-747898703
[approve ci] [add to whitelist]
----------------------------------------------------------------
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 #7401: Fix unnecessary addition of Expires header
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #7401:
URL: https://github.com/apache/trafficserver/pull/7401#discussion_r547561420
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -4464,7 +4464,7 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
// before issuing a 304
if (s->cache_info.action == CACHE_DO_WRITE || s->cache_info.action == CACHE_DO_NO_ACTION ||
s->cache_info.action == CACHE_DO_REPLACE) {
- if (s->is_cacheable_and_negative_caching_is_enabled) {
+ if (s->is_cacheable_and_negative_caching_is_enabled && is_negative_caching_appropriate(s)) {
Review comment:
Great! Thank you @masaori335 for the quick confirmation. I also ran the negative-caching AuTest with this change on master and verified that the original negative caching fix still works as expected.
@shinya1020 : Can you please try out this change on your branch and verify it addresses the Expires header issue you were seeing? If so, I suggest we update this PR and get this merged in.
----------------------------------------------------------------
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] masaori335 commented on a change in pull request #7401: Fix unnecessary addition of Expires header
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #7401:
URL: https://github.com/apache/trafficserver/pull/7401#discussion_r546531797
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -4464,7 +4464,7 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
// before issuing a 304
if (s->cache_info.action == CACHE_DO_WRITE || s->cache_info.action == CACHE_DO_NO_ACTION ||
s->cache_info.action == CACHE_DO_REPLACE) {
- if (s->is_cacheable_and_negative_caching_is_enabled) {
+ if (s->is_cacheable_and_negative_caching_is_enabled && is_negative_caching_appropriate(s)) {
Review comment:
FWIW, this change fixed a leak which I reported on https://github.com/apache/trafficserver/issues/7380#issuecomment-748701998.
----------------------------------------------------------------
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] masaori335 removed a comment on pull request #7401: Fix unnecessary addition of Expires header
Posted by GitBox <gi...@apache.org>.
masaori335 removed a comment on pull request #7401:
URL: https://github.com/apache/trafficserver/pull/7401#issuecomment-747898703
[approve ci] [add to whitelist]
----------------------------------------------------------------
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] masaori335 commented on a change in pull request #7401: Fix unnecessary addition of Expires header
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #7401:
URL: https://github.com/apache/trafficserver/pull/7401#discussion_r546531797
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -4464,7 +4464,7 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
// before issuing a 304
if (s->cache_info.action == CACHE_DO_WRITE || s->cache_info.action == CACHE_DO_NO_ACTION ||
s->cache_info.action == CACHE_DO_REPLACE) {
- if (s->is_cacheable_and_negative_caching_is_enabled) {
+ if (s->is_cacheable_and_negative_caching_is_enabled && is_negative_caching_appropriate(s)) {
Review comment:
FWIW, this change ~~fixed~~ hide a leak which I reported on https://github.com/apache/trafficserver/issues/7380#issuecomment-748701998.
----------------------------------------------------------------
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 #7401: Fix unnecessary addition of Expires header
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #7401:
URL: https://github.com/apache/trafficserver/pull/7401#discussion_r547504018
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -4464,7 +4464,7 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
// before issuing a 304
if (s->cache_info.action == CACHE_DO_WRITE || s->cache_info.action == CACHE_DO_NO_ACTION ||
s->cache_info.action == CACHE_DO_REPLACE) {
- if (s->is_cacheable_and_negative_caching_is_enabled) {
+ if (s->is_cacheable_and_negative_caching_is_enabled && is_negative_caching_appropriate(s)) {
Review comment:
See my thoughts here:
https://github.com/apache/trafficserver/issues/7380#issuecomment-749740797
> Prior to the 8eb6826, when this function set s->is_cacheable_and_negative_caching_is_enabled (renamed from s-> negative_caching), it called is_negative_caching_appropriate(s).
To add more detail to this: `is_negative_caching_appropriate()` was called before and after 8eb6826. Before 8eb6826, `cacheable` meant something pretty complicated and unintuitive if negative caching was enabled. If negative caching was **disabled**, `cacheable` meant simply that, all things considered (status code, cache-control header, etc.), the response looks cacheable. If negative caching was **enabled**, however, `cacheable` meant that the response looks cacheable in all respects other than response code considerations. This had the intention that `s->negative_caching` would be inspected to see whether it was cacheable with negative response status codes. The intention was explicitly marked by this comment:
https://github.com/apache/trafficserver/blob/f3647619921ada5214b78d6ddb594a10fbc99a15/proxy/http/HttpTransact.cc#L6354
However, this made `cacheable` unintuitive for negative caching and this resulted in the negative caching bug in which all negative responses, not just the specified ones, got cached.
To fix the negative caching bug, I updated the `is_response_cacheable()` function, from which `cacheable` is populated, to take into account negative caching. Therefore `cacheable` now simply is true if the response looks cacheable (with or without negative caching enabled, as the case may be), and it is false otherwise. And `is_cacheable_and_negative_caching_is_enabled` is set using the `cacheable` value.
Thus the problem isn't that `is_cacheable_and_negative_caching_is_enabled` isn't set with `is_negative_caching_appropriate()` in consideration, the problem is that it is logically OR'd with the other features of the response which makes it cacheable. Thus `is_cacheable_and_negative_caching_is_enabled` means literally what it's name indicates: it will be true if both negative caching is enabled and the response is cacheable (either because it is some otherwise cacheable response, like a 200, or if it is a negative response that was marked for negative caching).
The solution, I believe, will be to do what @shinya1020 is doing in this diff, but apply it more broadly to the value of `is_cacheable_and_negative_caching_is_enabled`. Thus I suggest that we initialize `is_cacheable_and_negative_caching_is_enabled` like so (`s->txn_conf->negative_caching_enabled` is taken into account in `is_negative_caching_appropriate()` so it doesn't need to be repeated on initialization):
s->is_cacheable_and_negative_caching_is_enabled = cacheable && is_negative_caching_appropriate(s);
And, with this change, we should rename the variable. I suggest: `is_cacheable_due_to_negative_caching_configuration`
This should resolve both the leak in #7380 and the issue of the Expires header.
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -4464,7 +4464,7 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
// before issuing a 304
if (s->cache_info.action == CACHE_DO_WRITE || s->cache_info.action == CACHE_DO_NO_ACTION ||
s->cache_info.action == CACHE_DO_REPLACE) {
- if (s->is_cacheable_and_negative_caching_is_enabled) {
+ if (s->is_cacheable_and_negative_caching_is_enabled && is_negative_caching_appropriate(s)) {
Review comment:
In addition to renaming `is_cacheable_due_to_negative_caching_configuration` we should update the comment here:
https://github.com/apache/trafficserver/blob/8eb68266167d8f8b3fa3a00ca9f6b7889e8ec101/proxy/http/HttpTransact.h#L741-L756
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -4464,7 +4464,7 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
// before issuing a 304
if (s->cache_info.action == CACHE_DO_WRITE || s->cache_info.action == CACHE_DO_NO_ACTION ||
s->cache_info.action == CACHE_DO_REPLACE) {
- if (s->is_cacheable_and_negative_caching_is_enabled) {
+ if (s->is_cacheable_and_negative_caching_is_enabled && is_negative_caching_appropriate(s)) {
Review comment:
In case it's helpful, I did these suggested changes on the following branch:
https://github.com/bneradt/trafficserver/tree/fix_negative_caching_leak
@masaori335 and @shinya1020 : can you please try things out with this branch to see whether it addresses the header and leak issues? If so, we can update this PR with that change. Or I can create a new PR if you prefer. Just let me know.
Thanks,
Brian
----------------------------------------------------------------
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] zwoop commented on pull request #7401: Fix unnecessary addition of Expires header
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7401:
URL: https://github.com/apache/trafficserver/pull/7401#issuecomment-756444923
Ok, never mind, @masaori335 is smarter than us all. Going to restore this again to 8.1.x.
----------------------------------------------------------------
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] shinya1020 commented on a change in pull request #7401: Fix unnecessary addition of Expires header
Posted by GitBox <gi...@apache.org>.
shinya1020 commented on a change in pull request #7401:
URL: https://github.com/apache/trafficserver/pull/7401#discussion_r547601646
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -4464,7 +4464,7 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
// before issuing a 304
if (s->cache_info.action == CACHE_DO_WRITE || s->cache_info.action == CACHE_DO_NO_ACTION ||
s->cache_info.action == CACHE_DO_REPLACE) {
- if (s->is_cacheable_and_negative_caching_is_enabled) {
+ if (s->is_cacheable_and_negative_caching_is_enabled && is_negative_caching_appropriate(s)) {
Review comment:
I've updated this PR to @bneradt patch.
----------------------------------------------------------------
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