ezelkow1 commented on code in PR #13776:
URL: https://github.com/apache/trafficserver/pull/13776#discussion_r4162548629
##########
plugins/compress/compress.cc:
##########
@@ -811,6 +810,190 @@ add_vary_header_to_client_response(TSHttpTxn txnp)
TSHandleMLocRelease(resp_buf, TS_NULL_MLOC, resp_loc);
}
+// A 304 carries the origin's strong ETag, which the cache would merge over
the weakened ETag of a
+// stored compressed copy. Keep it weak when both name the same entity and the
stored copy is
+// encoded; an identity copy's ETag is the origin's own, so it may
legitimately become strong.
+static void
+keep_cached_etag_weak(TSHttpTxn txnp)
+{
+ TSMBuffer srv_buf;
+ TSMLoc srv_loc;
+
+ if (TS_SUCCESS != TSHttpTxnServerRespGet(txnp, &srv_buf, &srv_loc)) {
+ return;
+ }
+ ts::PostScript srv_defer([&]() -> void { TSHandleMLocRelease(srv_buf,
TS_NULL_MLOC, srv_loc); });
+
+ if (TSHttpHdrStatusGet(srv_buf, srv_loc) != TS_HTTP_STATUS_NOT_MODIFIED) {
+ return;
+ }
+
+ TSMBuffer cached_buf;
+ TSMLoc cached_loc;
+
+ if (TS_SUCCESS != TSHttpTxnCachedRespGet(txnp, &cached_buf, &cached_loc)) {
+ return;
+ }
+ ts::PostScript cached_defer([&]() -> void { TSHandleMLocRelease(cached_buf,
TS_NULL_MLOC, cached_loc); });
+
+ TSMLoc cached_ce = TSMimeHdrFieldFind(cached_buf, cached_loc,
TS_MIME_FIELD_CONTENT_ENCODING, TS_MIME_LEN_CONTENT_ENCODING);
+
+ if (cached_ce == TS_NULL_MLOC) {
+ return;
+ }
+ TSHandleMLocRelease(cached_buf, cached_loc, cached_ce);
+
+ TSMLoc srv_etag = TSMimeHdrFieldFind(srv_buf, srv_loc, TS_MIME_FIELD_ETAG,
TS_MIME_LEN_ETAG);
+
+ if (srv_etag == TS_NULL_MLOC) {
+ return;
+ }
+ ts::PostScript srv_etag_defer([&]() -> void { TSHandleMLocRelease(srv_buf,
srv_loc, srv_etag); });
+
+ TSMLoc cached_etag = TSMimeHdrFieldFind(cached_buf, cached_loc,
TS_MIME_FIELD_ETAG, TS_MIME_LEN_ETAG);
+
+ if (cached_etag == TS_NULL_MLOC) {
+ return;
+ }
+ ts::PostScript cached_etag_defer([&]() -> void {
TSHandleMLocRelease(cached_buf, cached_loc, cached_etag); });
+
+ int srv_len;
+ const char *srv_str = TSMimeHdrFieldValueStringGet(srv_buf, srv_loc,
srv_etag, -1, &srv_len);
+ int cached_len;
+ const char *cached_str = TSMimeHdrFieldValueStringGet(cached_buf,
cached_loc, cached_etag, -1, &cached_len);
+ std::string_view srv_value{srv_str, static_cast<size_t>(srv_len)};
+ std::string_view cached_value{cached_str, static_cast<size_t>(cached_len)};
+
+ if (cached_value.starts_with("W/") && cached_value.substr(2) == srv_value) {
+ TSMimeHdrFieldValueStringSet(srv_buf, srv_loc, srv_etag, -1,
cached_value.data(), cached_value.size());
+ }
+}
+
+static bool
+client_accepts_compression(TSMBuffer req_buf, TSMLoc req_loc,
HostConfiguration *hc)
+{
+ TSMLoc ae = TSMimeHdrFieldFind(req_buf, req_loc,
TS_MIME_FIELD_ACCEPT_ENCODING, TS_MIME_LEN_ACCEPT_ENCODING);
+
+ if (ae == TS_NULL_MLOC) {
+ return false;
+ }
+ ts::PostScript ae_defer([&]() -> void { TSHandleMLocRelease(req_buf,
req_loc, ae); });
+
+ int const algorithms = hc->compression_algorithms();
+ int const nvalues = TSMimeHdrFieldValuesCount(req_buf, req_loc, ae);
+
+ for (int i = 0; i < nvalues; i++) {
+ int len;
+ const char *value = TSMimeHdrFieldValueStringGet(req_buf, req_loc, ae, i,
&len);
+
+ if (value == nullptr) {
+ continue;
+ }
+ if ((strncasecmp(value, "zstd", sizeof("zstd") - 1) == 0 && (algorithms &
ALGORITHM_ZSTD)) ||
+ (strncasecmp(value, "br", sizeof("br") - 1) == 0 && (algorithms &
ALGORITHM_BROTLI)) ||
+ (strncasecmp(value, "deflate", sizeof("deflate") - 1) == 0 &&
(algorithms & ALGORITHM_DEFLATE)) ||
+ (strncasecmp(value, "gzip", sizeof("gzip") - 1) == 0 && (algorithms &
ALGORITHM_GZIP))) {
+ return true;
+ }
+ }
+
+ return false;
+}
+
+// No transform runs on a bodyless 304, so it goes out with whatever ETag the
origin or the cached
+// identity copy has. A 304 must carry the ETag its 200 would (RFC 9110
15.4.5), so when the client
+// validated with the weak form of a strong 304 ETag, it holds a compressed
copy: answer in kind.
+static void
+weaken_client_not_modified_etag(TSHttpTxn txnp, HostConfiguration *hc)
+{
+ TSMBuffer resp_buf;
+ TSMLoc resp_loc;
+
+ if (TS_SUCCESS != TSHttpTxnClientRespGet(txnp, &resp_buf, &resp_loc)) {
+ return;
+ }
+ ts::PostScript resp_defer([&]() -> void { TSHandleMLocRelease(resp_buf,
TS_NULL_MLOC, resp_loc); });
+
+ if (TSHttpHdrStatusGet(resp_buf, resp_loc) != TS_HTTP_STATUS_NOT_MODIFIED) {
+ return;
+ }
+
+ TSMLoc resp_etag = TSMimeHdrFieldFind(resp_buf, resp_loc,
TS_MIME_FIELD_ETAG, TS_MIME_LEN_ETAG);
+
+ if (resp_etag == TS_NULL_MLOC) {
+ return;
+ }
+ ts::PostScript resp_etag_defer([&]() -> void { TSHandleMLocRelease(resp_buf,
resp_loc, resp_etag); });
+
+ int resp_len;
+ const char *resp_str = TSMimeHdrFieldValueStringGet(resp_buf, resp_loc,
resp_etag, -1, &resp_len);
+ std::string_view resp_value{resp_str, static_cast<size_t>(resp_len)};
+
+ if (resp_value.starts_with("W/") || resp_value.starts_with("w/")) {
+ return;
+ }
+
+ TSMBuffer req_buf;
+ TSMLoc req_loc;
+
+ if (TS_SUCCESS != TSHttpTxnClientReqGet(txnp, &req_buf, &req_loc)) {
+ return;
+ }
+ ts::PostScript req_defer([&]() -> void { TSHandleMLocRelease(req_buf,
TS_NULL_MLOC, req_loc); });
+
+ if (!client_accepts_compression(req_buf, req_loc, hc)) {
+ return;
+ }
+
+ TSMLoc inm = TSMimeHdrFieldFind(req_buf, req_loc,
TS_MIME_FIELD_IF_NONE_MATCH, TS_MIME_LEN_IF_NONE_MATCH);
+
+ if (inm == TS_NULL_MLOC) {
+ return;
+ }
+ ts::PostScript inm_defer([&]() -> void { TSHandleMLocRelease(req_buf,
req_loc, inm); });
+
+ int const nvalues = TSMimeHdrFieldValuesCount(req_buf, req_loc, inm);
+
+ for (int i = 0; i < nvalues; i++) {
+ int len;
+ const char *str = TSMimeHdrFieldValueStringGet(req_buf, req_loc, inm, i,
&len);
+
+ if (str == nullptr) {
+ continue;
+ }
+
+ std::string_view tag{str, static_cast<size_t>(len)};
+
+ if (tag.starts_with("W/") && tag.substr(2) == resp_value) {
+ TSMimeHdrFieldValueStringSet(resp_buf, resp_loc, resp_etag, -1,
tag.data(), tag.size());
Review Comment:
Agreed on both points. The 304 hook is now registered only where
`transformable()` decides the response would be compressed (origin 200, cached
identity copy, or the cached copy behind an origin 304), and it weakens any
outgoing strong-ETag 304 regardless of how the client wrote its validator.
`cache-false-strong-conditional` now expects `W/"c1"`, and a non-compressible
`image/png` case keeps its strong tag. For the one path with no representation
to judge (a forwarded conditional with nothing stored, e.g. `Authorization`), a
weak `If-None-Match` is dropped upstream along with `If-Modified-Since`, so the
origin returns a 200 and ATS answers the client's conditional from it. That
costs the same full fetch these requests made before this PR, when `"x-df"`
never matched. A strong validator on that path is still forwarded as-is.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
