This is an automated email from the ASF dual-hosted git repository. cmcfarlen pushed a commit to branch 10.0.x in repository https://gitbox.apache.org/repos/asf/trafficserver.git
commit 6d55a20149dcfa9b2cf0803cda29bdab3ebf0639 Author: JosiahWI <[email protected]> AuthorDate: Tue Jul 23 11:37:45 2024 -0500 Reuse prepared cache write on redirected request (#11542) * Add AuTest to reproduce #9275 This reproduces a variant of #9275 where we fail on the cache lookup. If the cache lookup were to succeed, it should also fail on the cache write, so this one test should cover both bugs. * Reuse prepared cache write on redirected request Fixes #9275. When there is no parent proxy and ATS is following a redirect, it will do a cache lookup on the redirected request. If that lookup results in a cache miss, it will start to prepare a new cache write, which is wrong. The state machine intentionally leaves the write open so we can re-use the connection; this is possible to ascertain from the code and from comments to that effect. We should only open a new write if we don't already have one when following a redirect. (cherry picked from commit 875463c9645787d380b3215b414e906c6c38cd7b) --- include/proxy/http/HttpTransact.h | 1 + src/proxy/http/HttpSM.cc | 3 +- src/proxy/http/HttpTransact.cc | 31 +++-- .../redirect_to_same_origin_on_cache.replay.yaml | 65 ++++++++++ .../redirect_to_same_origin_on_cache.test.py | 140 +++++++++++++++++++++ 5 files changed, 231 insertions(+), 9 deletions(-) diff --git a/include/proxy/http/HttpTransact.h b/include/proxy/http/HttpTransact.h index 976999d8e6..00df25eefe 100644 --- a/include/proxy/http/HttpTransact.h +++ b/include/proxy/http/HttpTransact.h @@ -1023,6 +1023,7 @@ public: static void HandleCacheOpenReadHitFreshness(State *s); static void HandleCacheOpenReadHit(State *s); static void HandleCacheOpenReadMiss(State *s); + static void set_cache_prepare_write_action_for_new_request(State *s); static void build_response_from_cache(State *s, HTTPWarningCode warning_code); static void handle_cache_write_lock(State *s); static void HandleResponse(State *s); diff --git a/src/proxy/http/HttpSM.cc b/src/proxy/http/HttpSM.cc index 5fd1e7fd72..13b3b3c6f1 100644 --- a/src/proxy/http/HttpSM.cc +++ b/src/proxy/http/HttpSM.cc @@ -8049,9 +8049,8 @@ HttpSM::set_next_state() } case HttpTransact::SM_ACTION_CACHE_ISSUE_WRITE: { - ink_assert((cache_sm.cache_write_vc == nullptr) || t_state.redirect_info.redirect_in_process); + ink_assert(cache_sm.cache_write_vc == nullptr); HTTP_SM_SET_DEFAULT_HANDLER(&HttpSM::state_cache_open_write); - do_cache_prepare_write(); break; } diff --git a/src/proxy/http/HttpTransact.cc b/src/proxy/http/HttpTransact.cc index 8f30886d60..f3b700d767 100644 --- a/src/proxy/http/HttpTransact.cc +++ b/src/proxy/http/HttpTransact.cc @@ -2171,12 +2171,7 @@ HttpTransact::DecideCacheLookup(State *s) if (s->redirect_info.redirect_in_process) { // without calling out the CACHE_LOOKUP_COMPLETE_HOOK if (s->txn_conf->cache_http) { - if (s->cache_info.write_lock_state == CACHE_WL_FAIL) { - s->cache_info.action = CACHE_PREPARE_TO_WRITE; - s->cache_info.write_lock_state = HttpTransact::CACHE_WL_INIT; - } else if (s->cache_info.write_lock_state == CACHE_WL_SUCCESS) { - s->cache_info.action = CACHE_DO_WRITE; - } + HttpTransact::set_cache_prepare_write_action_for_new_request(s); } LookupSkipOpenServer(s); } else { @@ -3291,7 +3286,7 @@ HttpTransact::HandleCacheOpenReadMiss(State *s) } else if (s->api_server_response_no_store) { // plugin may have decided not to cache the response s->cache_info.action = CACHE_DO_NO_ACTION; } else { - s->cache_info.action = CACHE_PREPARE_TO_WRITE; + HttpTransact::set_cache_prepare_write_action_for_new_request(s); } /////////////////////////////////////////////////////////////// @@ -3346,6 +3341,28 @@ HttpTransact::HandleCacheOpenReadMiss(State *s) return; } +void +HttpTransact::set_cache_prepare_write_action_for_new_request(State *s) +{ + // This method must be called no more than one time per request. It should + // not be called for non-cacheable requests. + if (s->cache_info.write_lock_state == CACHE_WL_SUCCESS) { + // If and only if this is a redirected request, we may have already + // prepared a cache write (during the handling of the previous request + // which got the 3xx response) and can safely re-use it. Otherwise, we + // risk storing the response under the wrong cache key. This is a release + // assert because the correct behavior would be to prepare a new write, + // but we can't do that because we failed to release the lock. To recover + // we would have to tell the state machine to abort its write, and we + // don't have a state for that. + ink_release_assert(s->redirect_info.redirect_in_process); + s->cache_info.action = CACHE_DO_WRITE; + } else { + s->cache_info.action = CACHE_PREPARE_TO_WRITE; + s->cache_info.write_lock_state = HttpTransact::CACHE_WL_INIT; + } +} + /////////////////////////////////////////////////////////////////////////////// // Name : OriginServerRawOpen // Description: called for ssl tunneling diff --git a/tests/gold_tests/redirect/redirect_to_same_origin_on_cache.replay.yaml b/tests/gold_tests/redirect/redirect_to_same_origin_on_cache.replay.yaml new file mode 100644 index 0000000000..45ec9e03c7 --- /dev/null +++ b/tests/gold_tests/redirect/redirect_to_same_origin_on_cache.replay.yaml @@ -0,0 +1,65 @@ +# 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. + +meta: + version: "1.0" + +sessions: +- protocol: + - name: http + version: 1 + - name: tcp + - name: ip + + transactions: + - client-request: + method: "GET" + version: "1.1" + url: /a/path/resource + headers: + fields: + - [ Host, oof.com ] + - [ Connection, keep-alive ] + - [ Content-Length, 0 ] + + server-response: + status: 307 + headers: + fields: + - [ location, /a/new/path/resource ] + - [ content-length, 0 ] + + proxy-response: + status: 200 + - client-request: + method: "GET" + version: "1.1" + url: /a/new/path/resource + headers: + fields: + - [ Host, oof.com ] + - [ Connection, keep-alive ] + - [ Content-Length, 0 ] + + server-response: + status: 200 + reason: OK + headers: + fields: + - [ content-length, 16 ] + + proxy-response: + status: 200 diff --git a/tests/gold_tests/redirect/redirect_to_same_origin_on_cache.test.py b/tests/gold_tests/redirect/redirect_to_same_origin_on_cache.test.py new file mode 100644 index 0000000000..4ca60e1926 --- /dev/null +++ b/tests/gold_tests/redirect/redirect_to_same_origin_on_cache.test.py @@ -0,0 +1,140 @@ +# 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. + +import functools +from typing import Any, Callable, Dict, Optional + +from ports import get_port + +Test.Summary = 'Tests SNI port-based routing' + +TestParams = Dict[str, Any] + + +class TestRedirectToSameOriginOnCache: + """Configure a test for reproducing #9275.""" + + replay_filepath_one: str = "redirect_to_same_origin_on_cache.replay.yaml" + client_counter: int = 0 + server_counter: int = 0 + ts_counter: int = 0 + + def __init__(self, name: str, /, autorun) -> None: + """Initialize the test. + + :param name: The name of the test. + """ + self.name = name + self.autorun = autorun + + def _init_run(self) -> "TestRun": + """Initialize processes for the test run.""" + + tr = Test.AddTestRun(self.name) + server_one = TestRedirectToSameOriginOnCache.configure_server(tr, "oof.com") + self._configure_traffic_server(tr, server_one) + dns = TestRedirectToSameOriginOnCache.configure_dns(tr, server_one, self._dns_port) + + tr.Processes.Default.StartBefore(server_one) + tr.Processes.Default.StartBefore(dns) + tr.Processes.Default.StartBefore(self._ts) + + return { + "tr": tr, + "ts": self._ts, + "server_one": server_one, + "port_one": self._port_one, + } + + @classmethod + def runner(cls, name: str, autorun: bool = True) -> Optional[Callable]: + """Create a runner for a test case. + + :param autorun: Run the test case once it's set up. Default is True. + """ + test = cls(name, autorun=autorun)._prepare_test_case + return test + + def _prepare_test_case(self, func: Callable) -> Callable: + """Set up a test case and possibly run it. + + :param func: The test case to set up. + """ + functools.wraps(func) + test_params = self._init_run() + + def wrapper(*args, **kwargs) -> Any: + return func(test_params, *args, **kwargs) + + if self.autorun: + wrapper() + return wrapper + + @staticmethod + def configure_server(tr: "TestRun", domain: str): + server = tr.AddVerifierServerProcess( + f"server{TestRedirectToSameOriginOnCache.server_counter}.{domain}", + TestRedirectToSameOriginOnCache.replay_filepath_one, + other_args="--format \"{url}\"") + TestRedirectToSameOriginOnCache.server_counter += 1 + + return server + + @staticmethod + def configure_dns(tr: "TestRun", server_one: "Process", dns_port: int): + dns = tr.MakeDNServer("dns", port=dns_port, default="127.0.0.1") + return dns + + def _configure_traffic_server(self, tr: "TestRun", server_one: "Process"): + """Configure Traffic Server. + + :param tr: The TestRun object to associate the ts process with. + """ + ts = tr.MakeATSProcess( + f"ts-{TestRedirectToSameOriginOnCache.ts_counter}", select_ports=False, enable_tls=True, enable_cache=True) + TestRedirectToSameOriginOnCache.ts_counter += 1 + + self._port_one = get_port(ts, "PortOne") + self._dns_port = get_port(ts, "DNSPort") + ts.Disk.records_config.update( + { + 'proxy.config.http.server_ports': f"{self._port_one}", + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': "cache|dns|http|redirect|remap", + 'proxy.config.dns.nameservers': f"127.0.0.1:{self._dns_port}", + 'proxy.config.dns.resolv_conf': 'NULL', + 'proxy.config.http.redirect.actions': "self:follow", + 'proxy.config.http.number_of_redirections': 1, + }) + + ts.Disk.remap_config.AddLine(f"map oof.com http://oof.backend.com:{server_one.Variables.http_port}";) + + self._ts = ts + + +# Tests start. + + [email protected]("Redirect to same origin with cache") +def test1(params: TestParams) -> None: + client = params["tr"].AddVerifierClientProcess( + f"client0", + TestRedirectToSameOriginOnCache.replay_filepath_one, + http_ports=[params["port_one"]], + other_args="--format \"{url}\" --keys \"/a/path/resource\"") + + params["tr"].Processes.Default.ReturnCode = 0 + params["tr"].Processes.Default.Streams.stdout.Content += Testers.IncludesExpression("200 OK", "We should get the resource.")
