diff --git a/include/proxy/http/HttpTransact.h b/include/proxy/http/HttpTransact.h index 976999d8e66..00df25eefe3 100644 --- a/include/proxy/http/HttpTransact.h +++ b/include/proxy/http/HttpTransact.h @@ -1023,6 +1023,7 @@ class HttpTransact 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 89364b4dc97..0257a648837 100644 --- a/src/proxy/http/HttpSM.cc +++ b/src/proxy/http/HttpSM.cc @@ -8043,9 +8043,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 8f30886d601..f3b700d7678 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 00000000000..45ec9e03c76 --- /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 00000000000..4ca60e1926d --- /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. + + +@TestRedirectToSameOriginOnCache.runner("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.")