You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by zwoop <gi...@git.apache.org> on 2016/06/27 19:25:23 UTC
[GitHub] trafficserver pull request #749: TS-4506 Should not remove Expires/Last-Modi...
GitHub user zwoop opened a pull request:
https://github.com/apache/trafficserver/pull/749
TS-4506 Should not remove Expires/Last-Modifed on 304 response
When generating a 304 page, we used to remove Expires and Last-Modified
always. This change makes sure we never touch Expires, and only remove
the Last-Modified header if the is an ETag in the response. This is per
the specs in
https://tools.ietf.org/html/rfc7232#page-18
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zwoop/trafficserver TS-4506
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/749.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #749
----
commit fb4f565f4437f9a18d6e8b049cbd45586e83b543
Author: Leif Hedstrom <zw...@apache.org>
Date: 2016-06-03T21:35:30Z
TS-4506 Should not remove Expires/Last-Modifed on 304 response
When generating a 304 page, we used to remove Expires and Last-Modified
always. This change makes sure we never touch Expires, and only remove
the Last-Modified header if the is an ETag in the response. This is per
the specs in
https://tools.ietf.org/html/rfc7232#page-18
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #749: TS-4506 Don't remove Expires/Last-Modifed on 304 r...
Posted by masaori335 <gi...@git.apache.org>.
Github user masaori335 commented on the issue:
https://github.com/apache/trafficserver/pull/749
Do we need some changes in `HttpTransact::build_response()` too?
https://github.com/apache/trafficserver/blob/master/proxy/http/HttpTransact.cc#L7945-L7975.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #749: TS-4506 Should not remove Expires/Last-Modifed on ...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/749
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/252/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #749: TS-4506 Don't remove Expires/Last-Modifed o...
Posted by bryancall <gi...@git.apache.org>.
Github user bryancall commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/749#discussion_r69032683
--- Diff: proxy/http/HttpTransact.cc ---
@@ -8199,13 +8199,15 @@ HttpTransact::build_error_response(State *s, HTTPStatus status_code, const char
if (has_ua_msie)
s->hdr_info.client_response.value_set(MIME_FIELD_PROXY_CONNECTION, MIME_LEN_PROXY_CONNECTION, "close", 5);
}
- // Add a bunch of headers to make sure that caches between
- // the Traffic Server and the client do not cache the error
- // page.
+ // Make sure that caches between the Traffic Server and the client do not cache the error.
+ // ToDo: Not sure this matches expectation in RFC 7223.
s->hdr_info.client_response.value_set(MIME_FIELD_CACHE_CONTROL, MIME_LEN_CACHE_CONTROL, "no-store", 8);
- // Make sure there are no Expires and Last-Modified headers.
- s->hdr_info.client_response.field_delete(MIME_FIELD_EXPIRES, MIME_LEN_EXPIRES);
- s->hdr_info.client_response.field_delete(MIME_FIELD_LAST_MODIFIED, MIME_LEN_LAST_MODIFIED);
+
+ // Remove the Last-Modified header, but only if there is an ETag header. See sec 4.1 in
+ // https://tools.ietf.org/html/rfc7232#page-18
--- End diff --
I think it would be better to link to the section and not the page:
https://tools.ietf.org/html/rfc7232#section-4.1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #749: TS-4506 Don't remove Expires/Last-Modifed on 304 r...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/749
@masaori335 HttpTransact::build_response() seems correct, doesn't it? But, seeing that, also makes me think that the place where we modify Cache-Control: (see your comment above) might actually be wrong?
Wdyt?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #749: TS-4506 Don't remove Expires/Last-Modifed o...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop closed the pull request at:
https://github.com/apache/trafficserver/pull/749
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #749: TS-4506 Don't remove Expires/Last-Modifed o...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/749#discussion_r68776903
--- Diff: proxy/http/HttpTransact.cc ---
@@ -8199,13 +8199,15 @@ HttpTransact::build_error_response(State *s, HTTPStatus status_code, const char
if (has_ua_msie)
s->hdr_info.client_response.value_set(MIME_FIELD_PROXY_CONNECTION, MIME_LEN_PROXY_CONNECTION, "close", 5);
}
- // Add a bunch of headers to make sure that caches between
- // the Traffic Server and the client do not cache the error
- // page.
+ // Make sure that caches between the Traffic Server and the client do not cache the error.
+ // ToDo: Not sure this matches expectation in RFC 7223.
--- End diff --
Well, I'm open to suggestion here. It seems, as per the RFC, that we probably shouldn't modify the Cache-Control header here? That seems inline with the code build_response() function too, right ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #749: TS-4506 Don't remove Expires/Last-Modifed on 304 r...
Posted by mlibbey <gi...@git.apache.org>.
Github user mlibbey commented on the issue:
https://github.com/apache/trafficserver/pull/749
The RFC portion didn't mandate removing Last-Modified. ("Since the goal of a 304 response is to minimize information transfer when the recipient already has one or more cached representations, a sender SHOULD NOT generate representation metadata other than the above listed fields unless said metadata exists for the purpose of guiding cache updates (e.g., Last-Modified might be useful if the response does not have an ETag field)." I've seen one case where others were expecting it to be their regardless of the ETag (because another large commercial CDN does it that way).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #749: TS-4506 Don't remove Expires/Last-Modifed on 304 r...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/749
I updated the PR as per William's post on the Jira. I had a bunch of typos and errors here, I hope this fixes those issues.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #749: TS-4506 Don't remove Expires/Last-Modifed on 304 r...
Posted by bryancall <gi...@git.apache.org>.
Github user bryancall commented on the issue:
https://github.com/apache/trafficserver/pull/749
It should be conditional on the 304 in the build_error_response and also added to build_response. Maybe pull the logic into a method and call it from both.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #749: TS-4506 Don't remove Expires/Last-Modifed on 304 r...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/749
Yeah, I need to work on this some more, not sure exactly yet what path to take yet. We have some options :). I'd like to keep this open, so we retain the feedback.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #749: TS-4506 Should not remove Expires/Last-Modifed on ...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/749
@bryancall @masaori335 Please review.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #749: TS-4506 Should not remove Expires/Last-Modifed on ...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/749
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/358/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #749: TS-4506 Don't remove Expires/Last-Modifed o...
Posted by bryancall <gi...@git.apache.org>.
Github user bryancall commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/749#discussion_r69032591
--- Diff: proxy/http/HttpTransact.cc ---
@@ -8199,13 +8199,15 @@ HttpTransact::build_error_response(State *s, HTTPStatus status_code, const char
if (has_ua_msie)
s->hdr_info.client_response.value_set(MIME_FIELD_PROXY_CONNECTION, MIME_LEN_PROXY_CONNECTION, "close", 5);
}
- // Add a bunch of headers to make sure that caches between
- // the Traffic Server and the client do not cache the error
- // page.
+ // Make sure that caches between the Traffic Server and the client do not cache the error.
+ // ToDo: Not sure this matches expectation in RFC 7223.
--- End diff --
It should be RFC7232 not RFC7223, like @masaori335 said.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #749: TS-4506 Don't remove Expires/Last-Modifed on 304 r...
Posted by wbardwel <gi...@git.apache.org>.
Github user wbardwel commented on the issue:
https://github.com/apache/trafficserver/pull/749
I assumed that the Cache-Control: no-store stuff was because that function is not mostly used for 304s...it is mostly used for 5xx errors, it might need to branch to handle different status codes differently.
build_response() seems to no try to pass along Last-Modified headers, only ETag which seems slightly wrong...(but doesn't have the other issues that this is about)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #749: TS-4506 Don't remove Expires/Last-Modifed on 304 r...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/749
Another option would be to just not remove the Last-Modified here, but unclear what exactly implications that has. I guess I really ought to look through all possible status codes, and see if the current logic makes sense everywhere :-/. Why can't anything be simple?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #749: TS-4506 Don't remove Expires/Last-Modifed on 304 r...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/749
So, do we want to change it such that this isn't used for 304's? Or do we want to make it conditional in here for 304's? Maybe build_error_response() should avoid setting or modifying headers at all?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---