diff --git a/doc/admin-guide/files/records.yaml.en.rst b/doc/admin-guide/files/records.yaml.en.rst index 5d75b68bafc..09c143372ad 100644 --- a/doc/admin-guide/files/records.yaml.en.rst +++ b/doc/admin-guide/files/records.yaml.en.rst @@ -2795,11 +2795,39 @@ Cache Control using the newest key generation. This might be temporarily necessary if a large cache was created by the previous version of ATS but the new version changed the way cache keys are generated. If this is turned on, - a metric called `proxy.process.http.cache.compat_key_reads` will be + a metric called ``proxy.process.http.cache.compat_key_reads`` will be incremented any time the compat cache lookup successfully finds the object. You can monitor this metric and know when its safe to turn this feature off as the cache wraps around. + Three costs come with enabling this. Every cache miss performs a second + lookup, so a tier with a low hit ratio roughly doubles its cache lookup + load for the duration. Every ``DELETE`` and ``PURGE`` issues a second + remove under the previous key, because a migrated object exists under both + keys until the old copy ages out. And an object found under the previous + key is not revalidated when it goes stale, because a ``304`` cannot be + applied to it: the write that would carry the update is a create under the + new key rather than an update of the old one. A stale one is treated as a + miss instead, so the origin returns the full response, which is stored + under the new key, and the copy under the previous key is left to age out. + The client's own conditional headers are not forwarded on that request, + even when :ts:cv:`proxy.config.http.cache.when_to_revalidate` is ``4``, + since a ``304`` from the origin would leave the object unmigrated. The + client still receives a ``304`` if its conditions match the full response. + Each object pays this once, but on a large cache the aggregate is a + bandwidth event worth sizing before enabling the setting in production. + + For the same reason a plugin cannot modify such an object in place. + ``TSHttpTxnUpdateCachedObject`` still returns ``TS_SUCCESS``, but when the + update is later prepared it is refused the same way an invalid update is, + and the transaction is answered with a ``500`` response rather than the + cached object. + + Objects whose path contains a ``;`` are unaffected. The previous algorithm + hashed the path and the deprecated ``;params`` segment as separate + components, which produces the same string the current algorithm produces + for such a path, so no compatibility lookup is issued for them. + .. ts:cv:: CONFIG proxy.config.http.cache.range.lookup INT 1 :overridable: diff --git a/include/proxy/hdrs/URL.h b/include/proxy/hdrs/URL.h index e8aeee0978d..eebf2011df0 100644 --- a/include/proxy/hdrs/URL.h +++ b/include/proxy/hdrs/URL.h @@ -28,6 +28,7 @@ #include "proxy/hdrs/HdrHeap.h" #include "tscore/CryptoHash.h" #include "proxy/hdrs/MIME.h" +#include #include #include "tscore/ink_apidefs.h" @@ -101,6 +102,13 @@ class URLImpl : public HdrHeapObjImpl void set_type_code(unsigned int typecode); std::string_view get_params() const noexcept; void set_params(HdrHeap *heap, std::string_view value, bool copy_string); + /** Whether the path carries a deprecated ";params" segment. + + ATS 9.2 and earlier split "/path;params" into separate path and params + components. That parsing was removed, so the segment now stays inside the + path. Reproducing a 9.2 cache key has to know which form it is looking at. + */ + bool has_path_params() const noexcept; std::string_view get_query() const noexcept; void set_query(HdrHeap *heap, std::string_view value, bool copy_string); std::string_view get_fragment() const noexcept; @@ -261,6 +269,7 @@ class URL : public HdrHeapSDKHandle char *string_get_buf(char *dstbuf, int dsbuf_size, int *length = nullptr) const; void hash_get(CryptoHash *hash, bool ignore_query = false, cache_generation_t generation = -1) const; void hash_get92(CryptoHash *hash, bool ignore_query = false, cache_generation_t generation = -1) const; + bool has_path_params() const noexcept; void host_hash_get(CryptoHash *hash) const; std::string_view scheme_get() const noexcept; @@ -488,6 +497,22 @@ URL::hash_get92(CryptoHash *hash, bool ignore_query, cache_generation_t generati url_CryptoHash_get_92(m_url_impl, hash, ignore_query, generation); } +/*------------------------------------------------------------------------- + -------------------------------------------------------------------------*/ + +inline bool +URLImpl::has_path_params() const noexcept +{ + return m_ptr_path != nullptr && memchr(m_ptr_path, ';', m_len_path) != nullptr; +} + +inline bool +URL::has_path_params() const noexcept +{ + ink_assert(valid()); + return m_url_impl->has_path_params(); +} + /*------------------------------------------------------------------------- -------------------------------------------------------------------------*/ diff --git a/include/proxy/http/HttpSM.h b/include/proxy/http/HttpSM.h index c2128eeca20..9945273c768 100644 --- a/include/proxy/http/HttpSM.h +++ b/include/proxy/http/HttpSM.h @@ -35,8 +35,6 @@ #include #include -#include "tscore/ink_platform.h" -#include "iocore/eventsystem/EventSystem.h" #include "proxy/http/HttpCacheSM.h" #include "proxy/http/HttpTransact.h" #include "proxy/http/HttpUserAgent.h" @@ -45,7 +43,6 @@ #include "proxy/http/HttpTunnel.h" #include "api/InkAPIInternal.h" #include "proxy/ProxyTransaction.h" -#include "proxy/hdrs/HdrUtils.h" // inknet #include "proxy/http/PreWarmManager.h" @@ -182,6 +179,28 @@ enum class CompatibilityCacheLookup { COMPAT_CACHE_LAST, }; +/// Policy for an object found under the previous (9.2) cache key. Shared by the +/// HTTP state machine, its cache sub-machine and HttpTransact, which is why it +/// is not a member of any of them. +namespace CompatCacheKey +{ +/// Whether this lookup addresses the cache with the previous (9.2) key. +inline bool +is_legacy(CompatibilityCacheLookup lookup) +{ + return lookup == CompatibilityCacheLookup::COMPAT_CACHE_LOOKUP_92; +} + +/// The object info to hand to a cache write, which a compatibility read must not +/// carry: it belongs to the legacy key and would turn the write into an update +/// of a vector the canonical key does not have. +inline CacheHTTPInfo * +write_info(CompatibilityCacheLookup lookup, CacheHTTPInfo *object_read_info) +{ + return is_legacy(lookup) ? nullptr : object_read_info; +} +} // namespace CompatCacheKey + class HttpSM : public Continuation, public PluginUserArgs { friend class HttpTransact; @@ -406,6 +425,7 @@ class HttpSM : public Continuation, public PluginUserArgs void do_hostdb_lookup(); void do_hostdb_reverse_lookup(); + URL *cache_lookup_url(); void do_cache_lookup_and_read(); void do_http_server_open(bool raw = false, bool only_direct = false); bool apply_ip_allow_filter(); @@ -421,6 +441,7 @@ class HttpSM : public Continuation, public PluginUserArgs void do_cache_prepare_update(); void do_cache_prepare_action(HttpCacheSM *c_sm, CacheHTTPInfo *object_read_info, bool retry, bool allow_multiple = false); void do_cache_delete_all_alts(); + void do_cache_delete_compat_alts(); void do_auth_callout(); int do_api_callout(); int do_api_callout_internal(); diff --git a/src/proxy/hdrs/URL.cc b/src/proxy/hdrs/URL.cc index b0d0e5329b3..89aca8623cd 100644 --- a/src/proxy/hdrs/URL.cc +++ b/src/proxy/hdrs/URL.cc @@ -1720,6 +1720,11 @@ memcpy_tolower(char *d, const char *s, int n) // fast path for CryptoHash, HTTP, no user/password/params/query, // no buffer overflow, no unescaping needed +// +// NOTE: this emits the ";" path/params separator, which matches +// url_CryptoHash_get_general_92() but not url_CryptoHash_get_general(). It is +// currently unreachable because url_hash_method is 0; enabling it would make +// canonical keys collide with 9.2 keys. static inline void url_CryptoHash_get_fast(const URLImpl *url, CryptoContext &ctx, CryptoHash *hash, cache_generation_t generation) @@ -1908,8 +1913,16 @@ url_CryptoHash_get_general_92(const URLImpl *url, CryptoContext &ctx, CryptoHash ends[7] = strs[7] + 1; ends[8] = strs[8] + url->m_len_path; - strs[9] = ";"; - strs[10] = url->m_ptr_params; + // ATS 9.2 split "/path;params" into separate path and params components and + // hashed them as path + ";" + params. That parsing was removed, so ";params" + // now stays inside the path and already spells the same byte sequence. Adding + // the separator again would append a ";" that 9.2 never emitted, so only add + // it when the path does not carry one. The params component itself is always + // empty now; it is left out rather than read back as an empty string. + bool const path_has_params = url->has_path_params(); + + strs[9] = path_has_params ? nullptr : ";"; + strs[10] = nullptr; strs[11] = "?"; // Special case for the query paramters, allowing us to ignore them if requested @@ -1921,8 +1934,8 @@ url_CryptoHash_get_general_92(const URLImpl *url, CryptoContext &ctx, CryptoHash ends[12] = nullptr; } - ends[9] = strs[9] + 1; - ends[10] = strs[10] + url->m_len_params; + ends[9] = path_has_params ? nullptr : strs[9] + 1; + ends[10] = nullptr; ends[11] = strs[11] + 1; p = buffer; @@ -1979,7 +1992,7 @@ void url_CryptoHash_get_92(const URLImpl *url, CryptoHash *hash, bool ignore_query, cache_generation_t generation) { URLHashContext ctx; - if ((url_hash_method != 0) && (url->m_url_type == URLType::HTTP) && + if ((url_hash_method != 0) && (url->m_url_type == URLType::HTTP) && !url->has_path_params() && ((url->m_len_user + url->m_len_password + url->m_len_params + (ignore_query ? 0 : url->m_len_query)) == 0) && (10u + url->m_len_scheme + url->m_len_host + url->m_len_path < BUFSIZE) && (memchr(url->m_ptr_host, '%', url->m_len_host) == nullptr) && (memchr(url->m_ptr_path, '%', url->m_len_path) == nullptr)) { diff --git a/src/proxy/hdrs/unit_tests/test_URL.cc b/src/proxy/hdrs/unit_tests/test_URL.cc index f1963c9039f..3f3219eab69 100644 --- a/src/proxy/hdrs/unit_tests/test_URL.cc +++ b/src/proxy/hdrs/unit_tests/test_URL.cc @@ -19,6 +19,8 @@ */ #include +#include +#include #include #include @@ -852,6 +854,106 @@ TEST_CASE("UrlPathGet", "[url][path_get]") } } +// ATS 9.2 hashed "path" ";" "params" as separate cache-key components, always +// emitting the separator. The params component was removed from the parser, so +// ";params" now lives inside the path and the separator is already there. That +// makes the 9.2 key expressible through the current algorithm: it is the +// current key of the same URL with exactly one ";" between path and query. +namespace +{ +/// A failing REQUIRE unwinds out of these helpers, so the heap is released by +/// scope exit rather than by a call that the unwind would skip. +struct HdrHeapDeleter { + void + operator()(HdrHeap *heap) const + { + heap->destroy(); + } +}; + +using HdrHeapPtr = std::unique_ptr; + +CryptoHash +hash92(char const *text) +{ + HdrHeapPtr heap{new_HdrHeap()}; + URL url; + + url.create(heap.get()); + REQUIRE(url.parse(text, strlen(text)) == ParseResult::DONE); + + CryptoHash hash; + + url.hash_get92(&hash); + + return hash; +} + +CryptoHash +hash_current(char const *text) +{ + HdrHeapPtr heap{new_HdrHeap()}; + URL url; + + url.create(heap.get()); + REQUIRE(url.parse(text, strlen(text)) == ParseResult::DONE); + + CryptoHash hash; + + url.hash_get(&hash); + + return hash; +} + +bool +has_params(char const *text) +{ + HdrHeapPtr heap{new_HdrHeap()}; + URL url; + + url.create(heap.get()); + REQUIRE(url.parse(text, strlen(text)) == ParseResult::DONE); + + return url.has_path_params(); +} +} // namespace + +TEST_CASE("UrlHashGet92 reproduces the 9.2 cache key", "[url][hash_get92]") +{ + SECTION("a path without params gains the 9.2 separator") + { + CHECK(hash92("http://foo.test/path") == hash_current("http://foo.test/path;")); + CHECK(hash92("http://foo.test/path?q=1") == hash_current("http://foo.test/path;?q=1")); + // A ';' inside the query is not a params segment: 9.2 stopped the params + // component at '?', so the path still gained a separator of its own. + CHECK(hash92("http://foo.test/a?b;c") == hash_current("http://foo.test/a;?b;c")); + } + + SECTION("a path carrying params already spells the 9.2 key") + { + CHECK(hash92("http://foo.test/a;b=1") == hash_current("http://foo.test/a;b=1")); + CHECK(hash92("http://foo.test/a;b;c") == hash_current("http://foo.test/a;b;c")); + CHECK(hash92("http://foo.test/a;b=1?q=1") == hash_current("http://foo.test/a;b=1?q=1")); + } + + SECTION("the keys diverge only when the path has no params segment") + { + // This divergence is the whole reason the compatibility lookup exists. + CHECK(hash92("http://foo.test/path") != hash_current("http://foo.test/path")); + CHECK(hash92("http://foo.test/path?q=1") != hash_current("http://foo.test/path?q=1")); + // ... and when it converges there is nothing for a second lookup to find. + CHECK(hash92("http://foo.test/a;b=1") == hash_current("http://foo.test/a;b=1")); + } + + SECTION("has_path_params identifies the converging case") + { + CHECK(!has_params("http://foo.test/path")); + CHECK(!has_params("http://foo.test/a?b;c")); + CHECK(has_params("http://foo.test/a;b=1")); + CHECK(has_params("http://foo.test/a;b=1?q=1")); + } +} + // URL getters must not construct std::string_view from a nullptr pointer // (which is UB). Parts that are not present in the URL should return an // empty string_view with data() == nullptr. diff --git a/src/proxy/http/HttpCacheSM.cc b/src/proxy/http/HttpCacheSM.cc index a6fe5e0cf91..8055ca9b6ce 100644 --- a/src/proxy/http/HttpCacheSM.cc +++ b/src/proxy/http/HttpCacheSM.cc @@ -294,7 +294,8 @@ HttpCacheSM::state_cache_open_write(int event, void *data) // than or equal to the max number of open write retries ink_assert(!write_retry_done()); - open_write(&cache_key, lookup_url, read_request_hdr, master_sm->t_state.cache_info.object_read, + open_write(&cache_key, lookup_url, read_request_hdr, + CompatCacheKey::write_info(master_sm->compatibility_cache_lookup, master_sm->t_state.cache_info.object_read), static_cast( (master_sm->t_state.cache_control.pin_in_cache_for < 0) ? 0 : master_sm->t_state.cache_control.pin_in_cache_for), retry_write, false); diff --git a/src/proxy/http/HttpSM.cc b/src/proxy/http/HttpSM.cc index b7a123c6f62..7fd1cf68252 100644 --- a/src/proxy/http/HttpSM.cc +++ b/src/proxy/http/HttpSM.cc @@ -23,6 +23,7 @@ */ #include "proxy/http/HttpConfig.h" +#include "proxy/hdrs/HdrUtils.h" #include "tscore/ink_hrtime.h" #include "tscore/ink_time.h" #include "tsutil/Metrics.h" @@ -2766,13 +2767,19 @@ HttpSM::state_cache_open_read(int event, void *data) if (cache_sm.get_last_error() == -ECACHE_DOC_BUSY) { t_state.cache_lookup_result = HttpTransact::CacheLookupResult_t::DOC_BUSY; } else { + // A path that carries its own ";params" segment already hashes to the 9.2 + // key, so a compatibility lookup would just repeat the lookup that missed. if (t_state.http_config_param->cache_try_compat_key_read && - compatibility_cache_lookup == CompatibilityCacheLookup::COMPAT_CACHE_LOOKUP_NORMAL) { + compatibility_cache_lookup == CompatibilityCacheLookup::COMPAT_CACHE_LOOKUP_NORMAL && + !cache_lookup_url()->has_path_params()) { // do the retry compatibility_cache_lookup = CompatibilityCacheLookup::COMPAT_CACHE_LOOKUP_92; do_cache_lookup_and_read(); return 0; } + // Nothing was found under either key. Leaving the flag set would make + // the rest of the transaction act as though a legacy object were in play. + compatibility_cache_lookup = CompatibilityCacheLookup::COMPAT_CACHE_LOOKUP_NORMAL; t_state.cache_lookup_result = HttpTransact::CacheLookupResult_t::MISS; } @@ -5279,6 +5286,28 @@ HttpSM::do_range_setup_if_necessary() } } +// The URL this transaction looks up in the cache, and that every delete which +// has to match that lookup uses too. +// +// hdr_info.client_request's URL always names the *current* request: +// redirect_request() rewrites it in place, so after a redirect follow it is the +// Location target. cache_info.lookup_url is set once, before the first lookup +// (HttpTransact::DecideCacheLookup), and names the *original* request. Whether +// it also moves with a redirect is an accident of how it was set: with +// pristine_host_hdr off it aliases client_request's URL and follows it, with +// pristine_host_hdr on it is a private copy and stays put. It is therefore not +// a reliable way to reach the redirected URL, which is why the redirect case +// below reads client_request instead. +URL * +HttpSM::cache_lookup_url() +{ + // Follow the redirect unless configured to keep the original cache key. + if (t_state.redirect_info.redirect_in_process && !t_state.txn_conf->redirect_use_orig_cache_key) { + return t_state.hdr_info.client_request.url_get(); + } + return t_state.cache_info.lookup_url; +} + void HttpSM::do_cache_lookup_and_read() { @@ -5292,23 +5321,19 @@ HttpSM::do_cache_lookup_and_read() Metrics::Counter::increment(http_rsb.cache_lookups); ATS_PROBE1(milestone_cache_open_read_begin, sm_id); - milestones[TS_MILESTONE_CACHE_OPEN_READ_BEGIN] = ink_get_hrtime(); - t_state.cache_lookup_result = HttpTransact::CacheLookupResult_t::NONE; - t_state.cache_info.lookup_count++; - // YTS Team, yamsat Plugin - // Changed the lookup_url to c_url which enables even - // the new redirect url to perform a CACHE_LOOKUP - URL *c_url; - if (t_state.redirect_info.redirect_in_process && !t_state.txn_conf->redirect_use_orig_cache_key) { - c_url = t_state.hdr_info.client_request.url_get(); - } else { - c_url = t_state.cache_info.lookup_url; + // A compatibility retry continues the lookup that just missed rather than + // starting a new one, so it keeps the original begin time and count. + if (!CompatCacheKey::is_legacy(compatibility_cache_lookup)) { + milestones[TS_MILESTONE_CACHE_OPEN_READ_BEGIN] = ink_get_hrtime(); + t_state.cache_info.lookup_count++; } + t_state.cache_lookup_result = HttpTransact::CacheLookupResult_t::NONE; + URL *c_url = cache_lookup_url(); SMDbg(dbg_ctl_http_seq, "Issuing cache lookup for URL %s", c_url->string_get(&t_state.arena)); HttpCacheKey key; - if (compatibility_cache_lookup == CompatibilityCacheLookup::COMPAT_CACHE_LOOKUP_92) { + if (CompatCacheKey::is_legacy(compatibility_cache_lookup)) { Cache::generate_key92(&key, c_url, t_state.txn_conf->cache_ignore_query, t_state.txn_conf->cache_generation_number); } else { Cache::generate_key(&key, c_url, t_state.txn_conf->cache_ignore_query, t_state.txn_conf->cache_generation_number); @@ -5336,11 +5361,48 @@ HttpSM::do_cache_delete_all_alts() // Do not delete a non-existent object. ink_assert(t_state.cache_info.object_read); - SMDbg(dbg_ctl_http_seq, "Issuing cache delete for %s", t_state.cache_info.lookup_url->string_get_ref()); + // Address the object that was looked up. A redirect follow can look up a + // different URL than cache_info.lookup_url, which is set once and does not + // track the redirect when the pristine host header is maintained. + URL *url = cache_lookup_url(); + + SMDbg(dbg_ctl_http_seq, "Issuing cache delete for %s", url->string_get_ref()); + + HttpCacheKey key; + Cache::generate_key(&key, url, t_state.txn_conf->cache_ignore_query, t_state.txn_conf->cache_generation_number); + cacheProcessor.remove(nullptr, &key); + + // A migration leaves the legacy copy in place, so the object can live under + // both keys. Removing only one of them would let the other be served after + // the purge. + do_cache_delete_compat_alts(); +} + +// Remove the object stored under the legacy key. +// +// Only for the cases that abort the canonical-key write, where nothing is left +// depending on it, and for deletes, which have to reach both keys. A successful +// migration deliberately leaves the legacy copy alone: VC_EVENT_WRITE_COMPLETE +// means the tunnel handed the last byte to the cache VC, not that the object +// reached disk, so deleting on that signal loses the object outright whenever +// the write later fails. The copy ages out on its own, and it stops being read +// as soon as the canonical key resolves, so compat_key_reads still decays to +// zero. +void +HttpSM::do_cache_delete_compat_alts() +{ + // Same URL the lookup used; see do_cache_delete_all_alts(). + URL *url = cache_lookup_url(); + + // A path that carries its own ";params" segment hashes to the same key + // under both schemes, so the canonical delete already reached it. + if (!t_state.http_config_param->cache_try_compat_key_read || url->has_path_params()) { + return; + } + SMDbg(dbg_ctl_http_seq, "Issuing compatibility cache delete for %s", url->string_get_ref()); HttpCacheKey key; - Cache::generate_key(&key, t_state.cache_info.lookup_url, t_state.txn_conf->cache_ignore_query, - t_state.txn_conf->cache_generation_number); + Cache::generate_key92(&key, url, t_state.txn_conf->cache_ignore_query, t_state.txn_conf->cache_generation_number); cacheProcessor.remove(nullptr, &key); } @@ -5365,10 +5427,15 @@ HttpSM::do_cache_prepare_write_transform() void HttpSM::do_cache_prepare_update() { + // An object found under the 9.2 key cannot be updated in place: the write + // would be a create on the current key, and the cache turns a header-only + // close of a create into an abort. Refuse the way an invalid update is + // refused rather than let the plugin's change vanish. if (t_state.cache_info.object_read != nullptr && t_state.cache_info.object_read->valid() && t_state.cache_info.object_store.valid() && t_state.cache_info.object_store.response_get() != nullptr && t_state.cache_info.object_store.response_get()->valid() && - t_state.hdr_info.client_request.method_get_wksidx() == HTTP_WKSIDX_GET) { + t_state.hdr_info.client_request.method_get_wksidx() == HTTP_WKSIDX_GET && + !CompatCacheKey::is_legacy(compatibility_cache_lookup)) { t_state.cache_info.object_store.request_set(t_state.cache_info.object_read->request_get()); // t_state.cache_info.object_read = NULL; // cache_sm.close_read(); @@ -5419,8 +5486,15 @@ HttpSM::do_cache_prepare_action(HttpCacheSM *c_sm, CacheHTTPInfo *object_read_in HttpCacheKey key; Cache::generate_key(&key, s_url, t_state.txn_conf->cache_ignore_query, t_state.txn_conf->cache_generation_number); + // A compatibility read returns an object stored under the legacy key. Passing + // that object to a write using the canonical key turns the write into an + // update, but the canonical-key vector does not contain the legacy alternate. + // Cache::open_write then fails with ECACHE_NO_DOC instead of creating the + // migrated object. Create a new canonical-key object for compatibility reads. + CacheHTTPInfo *write_object_read_info = CompatCacheKey::write_info(compatibility_cache_lookup, object_read_info); + pending_action = - c_sm->open_write(&key, s_url, &t_state.hdr_info.cache_request, object_read_info, + c_sm->open_write(&key, s_url, &t_state.hdr_info.cache_request, write_object_read_info, static_cast((t_state.cache_control.pin_in_cache_for < 0) ? 0 : t_state.cache_control.pin_in_cache_for), retry, allow_multiple); } @@ -6818,6 +6892,9 @@ HttpSM::perform_cache_write_action() case HttpTransact::CacheAction_t::DELETE: { // Write close deletes the old alternate cache_sm.close_write(); + // That reached only one of the two keys the object can live under while + // the compatibility lookup is enabled. + do_cache_delete_compat_alts(); cache_sm.close_read(); t_state.cache_info.write_lock_state = HttpTransact::CacheWriteLock_t::INIT; break; @@ -8394,6 +8471,10 @@ HttpSM::set_next_state() case HttpTransact::StateMachineAction_t::CACHE_LOOKUP: { HTTP_SM_SET_DEFAULT_HANDLER(&HttpSM::state_cache_open_read); + // Every lookup starts from the canonical key. A redirect follow or a read + // retry looks up a different object than the one a previous compatibility + // lookup found, so the flag must not carry over. + compatibility_cache_lookup = CompatibilityCacheLookup::COMPAT_CACHE_LOOKUP_NORMAL; do_cache_lookup_and_read(); break; } diff --git a/src/proxy/http/HttpTransact.cc b/src/proxy/http/HttpTransact.cc index 8a3c601092a..bd9b0d56afe 100644 --- a/src/proxy/http/HttpTransact.cc +++ b/src/proxy/http/HttpTransact.cc @@ -2724,6 +2724,22 @@ HttpTransact::HandleCacheOpenReadHitFreshness(State *s) s->cache_lookup_result = HttpTransact::CacheLookupResult_t::HIT_STALE; } + // An object found under the 9.2 key cannot be revalidated: the write that + // would apply a 304 is a create on the current key, not an update of the + // legacy vector, so the cache discards it. Treat a stale one as a miss. The + // full response then lands under the current key through the ordinary miss + // path and the legacy copy ages out. Only reads migrate; the methods that + // invalidate take the delete path, which reaches both keys. + if (s->cache_lookup_result == HttpTransact::CacheLookupResult_t::HIT_STALE && s->state_machine != nullptr && + CompatCacheKey::is_legacy(s->state_machine->compatibility_cache_lookup) && + (s->method == HTTP_WKSIDX_GET || s->method == HTTP_WKSIDX_HEAD)) { + TxnDbg(dbg_ctl_http_seq, "Stale under the compatibility key, treating as a miss"); + s->cache_info.object_read = nullptr; + s->cache_lookup_result = HttpTransact::CacheLookupResult_t::MISS; + s->cache_lookup_complete_deferred = false; + TRANSACT_RETURN(StateMachineAction_t::API_CACHE_LOOKUP_COMPLETE, HttpTransact::HandleCacheOpenReadMiss); + } + ink_assert(s->cache_lookup_result != HttpTransact::CacheLookupResult_t::MISS); if (s->cache_lookup_result == HttpTransact::CacheLookupResult_t::HIT_STALE) { SET_VIA_STRING(VIA_DETAIL_CACHE_LOOKUP, VIA_DETAIL_MISS_EXPIRED); @@ -8267,6 +8283,17 @@ HttpTransact::build_request(State *s, HTTPHdr *base_request, HTTPHdr *outgoing_r // instead of the normal non-conditional request. TxnDbg(dbg_ctl_http_trans, "request not like cacheable and conditional headers not removed"); } + + // A stale object under the 9.2 key is fetched as a miss so the full + // response can be stored under the current key. A 304 would leave it + // unmigrated, so drop the client's conditionals even in the two cases + // above that keep them. They are still matched against the full response, + // so the client gets its 304. A miss that will not be written keeps them. + if (s->cache_lookup_result == CacheLookupResult_t::MISS && s->cache_info.action != CacheAction_t::NO_ACTION && + s->state_machine != nullptr && CompatCacheKey::is_legacy(s->state_machine->compatibility_cache_lookup)) { + TxnDbg(dbg_ctl_http_trans, "legacy key object fetched as a miss, conditional headers removed"); + HttpTransactHeaders::remove_conditional_headers(outgoing_request); + } } if (s->hdr_info.client_request.m_100_continue_sent) { diff --git a/tests/gold_tests/cache/compat-cache-key-client-conditional.test.py b/tests/gold_tests/cache/compat-cache-key-client-conditional.test.py new file mode 100644 index 00000000000..c632e721de5 --- /dev/null +++ b/tests/gold_tests/cache/compat-cache-key-client-conditional.test.py @@ -0,0 +1,24 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +Test.Summary = ''' +Verify that a stale compatibility hit, which is fetched as a miss, strips the +client's own conditional headers in the configurations where build_request() +forwards them, so the origin cannot answer with a 304 that would leave the +object unmigrated. A miss that will not be written, such as a HEAD, keeps them. +''' + +Test.ATSReplayTest(replay_file="replay/compat-cache-key-client-conditional.replay.yaml") diff --git a/tests/gold_tests/cache/compat-cache-key.test.py b/tests/gold_tests/cache/compat-cache-key.test.py new file mode 100644 index 00000000000..080c739b861 --- /dev/null +++ b/tests/gold_tests/cache/compat-cache-key.test.py @@ -0,0 +1,24 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +Test.Summary = ''' +Verify proxy.config.http.cache.try_compat_key_read: objects stored under the +ATS 9.2 cache key are served while fresh, treated as a miss once stale so the +full response is stored under the current key, and left in place under the +legacy key to age out. Deletes reach both keys. +''' + +Test.ATSReplayTest(replay_file="replay/compat-cache-key.replay.yaml") diff --git a/tests/gold_tests/cache/replay/compat-cache-key-client-conditional.replay.yaml b/tests/gold_tests/cache/replay/compat-cache-key-client-conditional.replay.yaml new file mode 100644 index 00000000000..4e7ad943d05 --- /dev/null +++ b/tests/gold_tests/cache/replay/compat-cache-key-client-conditional.replay.yaml @@ -0,0 +1,227 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + + +# +# A compatibility-key object cannot consume a 304, so once stale it is fetched +# from the origin as a miss and the full response is stored under the current +# key. build_request() already removes the client's conditionals for a request +# it expects to cache, but it deliberately forwards them in two cases: when the +# request does not look cacheable, and when +# proxy.config.http.cache.when_to_revalidate is 4. In those the origin could +# still answer 304, which would leave the object unmigrated, so build_request() +# drops them whenever the response is going to be stored. A miss that will not +# be written, such as a HEAD, keeps them: there is nothing to migrate. +# +# This exercises the when_to_revalidate case, which is a global switch and so +# needs a TS of its own rather than sharing one with compat-cache-key. +# +# As in compat-cache-key.replay.yaml, "/cc;" addresses the same key ATS 9.2 +# produced for "/cc", which is how a legacy object gets into the cache. +# + +meta: + version: "1.0" + +autest: + description: 'Verify client-supplied conditional headers on a stale compatibility hit' + + dns: + name: 'dns-compat-cache-key-client-conditional' + + server: + name: 'proxy-verifier-server' + + client: + name: 'proxy-verifier-client' + + ats: + name: 'ts-for-proxy-verifier' + process_config: + enable_cache: true + + records_config: + proxy.config.diags.debug.enabled: 1 + proxy.config.diags.debug.tags: 'http|http_cache' + proxy.config.http.insert_age_in_response: 0 + proxy.config.http.cache.try_compat_key_read: 1 + # Forward the client's conditionals rather than stripping them. + proxy.config.http.cache.when_to_revalidate: 4 + + remap_config: + - from: "http://example.com/" + to: "http://backend.example.com:{SERVER_HTTP_PORT}/" + + metric_checks: + # Two compatibility hits: the stale object fetched as a miss in test 2 + # and the stale HEAD in test 5. + - metric: proxy.process.http.cache.compat_key_reads + value: 2 + +sessions: +- transactions: + + # + # Test 1: Prime the cache under the legacy key. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /cc; + headers: + fields: + - [ uuid, 1-prime-legacy-key ] + - [ Host, example.com ] + + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 16 ] + - [ Cache-Control, max-age=1 ] + - [ ETag, '"legacy-object"' ] + + proxy-response: + status: 200 + + # + # Test 2: The client's own conditional is stripped for the migration. + # + # The object is stale, so this compatibility hit is fetched as a miss. The + # client sent an ETag of its own and when_to_revalidate 4 would forward it, + # which would let the origin answer 304 -- a response this transaction cannot + # apply to a legacy-key object. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /cc + headers: + fields: + - [ uuid, 2-client-conditional-stripped ] + - [ Host, example.com ] + - [ If-None-Match, '"something-the-client-has"' ] + # Outlive the max-age=1 above so the object is stale. + delay: 2s + + proxy-request: + headers: + fields: + - [ If-None-Match, { as: absent } ] + - [ If-Modified-Since, { as: absent } ] + + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 16 ] + - [ Cache-Control, max-age=300 ] + - [ ETag, '"migrated-object"' ] + + proxy-response: + status: 200 + + # + # Test 3: The object really did migrate to the current key. + # + # Had the origin been allowed to answer 304, nothing would have been stored + # under the current key and this would go back to the origin instead. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /cc + headers: + fields: + - [ uuid, 3-migrated-to-current-key ] + - [ Host, example.com ] + delay: 100ms + + proxy-request: + expect: absent + + server-response: + status: 400 + reason: "Bad Request" + headers: + fields: + - [ Content-Length, 0 ] + + proxy-response: + status: 200 + + # + # Test 4-5: a HEAD keeps the client's conditional. + # + # A stale legacy object is fetched as a miss for HEAD too, but a HEAD response + # is never written to the cache, so there is nothing to migrate and no reason + # to give up the 304. The conditional headers are forwarded as they are. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /head; + headers: + fields: + - [ uuid, 4-prime-head-target ] + - [ Host, example.com ] + delay: 100ms + + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 16 ] + - [ Cache-Control, max-age=1 ] + - [ ETag, '"head-object"' ] + + proxy-response: + status: 200 + + - client-request: + method: "HEAD" + version: "1.1" + scheme: "http" + url: /head + headers: + fields: + - [ uuid, 5-head-keeps-conditional ] + - [ Host, example.com ] + - [ If-None-Match, '"head-object"' ] + # Outlive the max-age=1 above so the object is stale. + delay: 2s + + proxy-request: + headers: + fields: + - [ If-None-Match, { value: '"head-object"', as: equal } ] + + server-response: + status: 304 + reason: "Not Modified" + headers: + fields: + - [ ETag, '"head-object"' ] + + proxy-response: + status: 304 diff --git a/tests/gold_tests/cache/replay/compat-cache-key.replay.yaml b/tests/gold_tests/cache/replay/compat-cache-key.replay.yaml new file mode 100644 index 00000000000..99082cce5e5 --- /dev/null +++ b/tests/gold_tests/cache/replay/compat-cache-key.replay.yaml @@ -0,0 +1,462 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# +# ATS 9.2 hashed a cache key as path + ";" + params. The params segment was +# removed from the URL parser, so ";params" now lives inside the path and the +# separator is no longer added -- which is why every 9.2 object misses after an +# upgrade and why proxy.config.http.cache.try_compat_key_read exists. +# +# That same equivalence gives this test a way to create a 9.2-key object using +# only the running proxy: the 9.2 key of "/obj" is byte-identical to the current +# key of "/obj;". Priming the cache with "/obj;" therefore leaves an object that +# only a compatibility lookup for "/obj" can find. +# + +meta: + version: "1.0" + +autest: + description: 'Verify 92x compatibility cache lookups serve and migrate to the current key' + + dns: + name: 'dns-compat-cache-key' + + server: + name: 'proxy-verifier-server' + + client: + name: 'proxy-verifier-client' + + ats: + name: 'ts-for-proxy-verifier' + process_config: + enable_cache: true + + records_config: + proxy.config.diags.debug.enabled: 1 + proxy.config.diags.debug.tags: 'http|http_cache|url_cachekey' + proxy.config.http.insert_age_in_response: 0 + proxy.config.http.cache.try_compat_key_read: 1 + # Honour Cache-Control in the client request, which is how these + # transactions force revalidation without waiting for an object to age. + proxy.config.http.cache.ignore_client_cc_max_age: 0 + + remap_config: + - from: "http://example.com/" + to: "http://backend.example.com:{SERVER_HTTP_PORT}/" + + metric_checks: + # Four compatibility hits: the fresh serve, the stale revalidation that + # migrates the object, the PUT in test 8 and the migration in test 10. + - metric: proxy.process.http.cache.compat_key_reads + value: 4 + +sessions: +- transactions: + + # + # Test 1: Prime the cache under the legacy key. + # + # "/migrate;" hashes to exactly the key ATS 9.2 produced for "/migrate", so + # after this transaction the cache holds what looks like a 9.2 object. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /migrate; + headers: + fields: + - [ uuid, 1-prime-legacy-key ] + - [ Host, example.com ] + + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 16 ] + - [ Cache-Control, max-age=300 ] + - [ ETag, '"legacy-object"' ] + + proxy-response: + status: 200 + + # + # Test 2: A fresh compatibility hit is served from cache. + # + # The canonical lookup for "/migrate" misses and the compatibility lookup + # finds the object primed above. The origin must not be contacted. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /migrate + headers: + fields: + - [ uuid, 2-compat-hit-fresh ] + - [ Host, example.com ] + delay: 100ms + + proxy-request: + expect: absent + + server-response: + status: 400 + reason: "Bad Request" + headers: + fields: + - [ Content-Length, 0 ] + + proxy-response: + status: 200 + + # + # Test 3: A stale compatibility hit is treated as a miss. + # + # A 304 cannot be applied to a legacy-key object, because the write that would + # carry it is a create on the canonical key rather than an update of the + # legacy vector. ATS therefore does not revalidate it at all: the request goes + # to the origin as an ordinary miss, so no conditional headers appear even + # though the cached object has an ETag, and the full response is stored under + # the canonical key. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /migrate + headers: + fields: + - [ uuid, 3-compat-migrate ] + - [ Host, example.com ] + # Ask for a revalidation rather than sleeping until the object ages out. + # Freshness is decided before any of the compatibility handling, so the + # path under test is the same either way. + - [ Cache-Control, max-age=0 ] + delay: 100ms + + proxy-request: + headers: + fields: + - [ If-None-Match, { as: absent } ] + - [ If-Modified-Since, { as: absent } ] + + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 16 ] + - [ Cache-Control, max-age=300 ] + - [ ETag, '"migrated-object"' ] + + proxy-response: + status: 200 + + # + # Test 4: The migrated object is now a canonical-key hit. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /migrate + headers: + fields: + - [ uuid, 4-canonical-hit ] + - [ Host, example.com ] + delay: 100ms + + proxy-request: + expect: absent + + server-response: + status: 400 + reason: "Bad Request" + headers: + fields: + - [ Content-Length, 0 ] + + proxy-response: + status: 200 + + # + # Test 5: The legacy copy is left in place. + # + # "/migrate;" addresses the legacy key directly. Migration does not delete it: + # nothing at this layer reports that the new object reached disk, so removing + # it here would lose the object whenever that write later failed. It ages out + # instead, and stops being read as soon as the current key resolves -- which + # is what lets compat_key_reads decay to zero. + # + # Still being in the cache is observable: the object is stale by now, so it + # revalidates conditionally with the ETag it was stored with. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /migrate; + headers: + fields: + - [ uuid, 5-legacy-key-retained ] + - [ Host, example.com ] + # Force the revalidation that reveals the stored ETag. + - [ Cache-Control, max-age=0 ] + delay: 100ms + + proxy-request: + headers: + fields: + - [ If-None-Match, { value: '"legacy-object"', as: equal } ] + + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 16 ] + - [ Cache-Control, max-age=300 ] + + proxy-response: + status: 200 + + # + # Test 6: A path that already carries ";params" gets no compatibility lookup. + # + # Its current key and its 9.2 key are the same string, so a second lookup + # would only repeat the one that just missed. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /keep;jsessionid=abc + headers: + fields: + - [ uuid, 6-params-path-miss ] + - [ Host, example.com ] + delay: 100ms + + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 16 ] + - [ Cache-Control, max-age=300 ] + + proxy-response: + status: 200 + + # + # Test 7: ... and it still caches normally under the current key. + # + # Neither of these two transactions takes a compatibility path, so the + # running compat_key_reads count is still 2 after them. The remaining two + # of the four asserted in metric_checks come from tests 8 and 10. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /keep;jsessionid=abc + headers: + fields: + - [ uuid, 7-params-path-hit ] + - [ Host, example.com ] + delay: 100ms + + proxy-request: + expect: absent + + server-response: + status: 400 + reason: "Bad Request" + headers: + fields: + - [ Content-Length, 0 ] + + proxy-response: + status: 200 + + # + # Test 8: a PUT keeps the client's precondition. + # + # Only GET and HEAD are treated as a miss when stale; a PUT takes the delete + # path with its request untouched. If-Match is the client's concurrency guard: + # dropping it would let the origin overwrite whatever is there now rather than + # only the version the client saw. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /put; + headers: + fields: + - [ uuid, 8a-prime-put-target ] + - [ Host, example.com ] + delay: 100ms + + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 16 ] + - [ Cache-Control, max-age=300 ] + - [ ETag, '"v1"' ] + + proxy-response: + status: 200 + + - client-request: + method: "PUT" + version: "1.1" + scheme: "http" + url: /put + headers: + fields: + - [ uuid, 8b-put-keeps-if-match ] + - [ Host, example.com ] + - [ Content-Length, 0 ] + - [ If-Match, '"v1"' ] + delay: 100ms + + proxy-request: + headers: + fields: + - [ If-Match, { value: '"v1"', as: equal } ] + + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 0 ] + + proxy-response: + status: 200 + + # + # Test 9-11: a purge has to reach both keys. + # + # After a migration the object exists under both the current and the legacy + # key. Removing only the one the purge looked up would leave the other to be + # served afterwards, which for a purge is the whole point of the operation. + # + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /purge; + headers: + fields: + - [ uuid, 9-prime-purge-target ] + - [ Host, example.com ] + delay: 100ms + + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 16 ] + - [ Cache-Control, max-age=300 ] + - [ ETag, '"legacy-purge"' ] + + proxy-response: + status: 200 + + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /purge + headers: + fields: + - [ uuid, 10-migrate-purge-target ] + - [ Host, example.com ] + - [ Cache-Control, max-age=0 ] + delay: 100ms + + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 16 ] + - [ Cache-Control, max-age=300 ] + - [ ETag, '"migrated-purge"' ] + + proxy-response: + status: 200 + + - client-request: + method: "PURGE" + version: "1.1" + scheme: "http" + url: /purge + headers: + fields: + - [ uuid, 11a-purge ] + - [ Host, example.com ] + - [ Content-Length, 0 ] + delay: 100ms + + # A purge is answered from the cache and never reaches the origin. + proxy-request: + expect: absent + + server-response: + status: 400 + reason: "Bad Request" + headers: + fields: + - [ Content-Length, 0 ] + + proxy-response: + status: 200 + + - client-request: + method: "GET" + version: "1.1" + scheme: "http" + url: /purge + headers: + fields: + - [ uuid, 11b-purged-content-is-gone ] + - [ Host, example.com ] + delay: 100ms + + # Reaching the origin at all is the assertion: a legacy copy that survived + # the purge is still fresh, so it would have been served from cache and the + # origin would never have been contacted. + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 16 ] + - [ Cache-Control, max-age=300 ] + - [ ETag, '"refetched"' ] + + proxy-response: + status: 200