From 0ad6375d1ada5f473d68574a33436d726555291d Mon Sep 17 00:00:00 2001 From: JosiahWI <41302989+JosiahWI@users.noreply.github.com> Date: Tue, 23 Jul 2024 11:37:45 -0500 Subject: [PATCH] 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) --- proxy/http/HttpSM.cc | 3 +- proxy/http/HttpTransact.cc | 31 +++- proxy/http/HttpTransact.h | 1 + ...direct_to_same_origin_on_cache.replay.yaml | 65 ++++++++ .../redirect_to_same_origin_on_cache.test.py | 140 ++++++++++++++++++ 5 files changed, 231 insertions(+), 9 deletions(-) create mode 100644 tests/gold_tests/redirect/redirect_to_same_origin_on_cache.replay.yaml create mode 100644 tests/gold_tests/redirect/redirect_to_same_origin_on_cache.test.py diff --git a/proxy/http/HttpSM.cc b/proxy/http/HttpSM.cc index c0ba82641e1..df13bab4755 100644 --- a/proxy/http/HttpSM.cc +++ b/proxy/http/HttpSM.cc @@ -7959,9 +7959,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/proxy/http/HttpTransact.cc b/proxy/http/HttpTransact.cc index 0ec9b7eb111..7f9f07e8548 100644 --- a/proxy/http/HttpTransact.cc +++ b/proxy/http/HttpTransact.cc @@ -2215,12 +2215,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 { @@ -3336,7 +3331,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); } /////////////////////////////////////////////////////////////// @@ -3391,6 +3386,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/proxy/http/HttpTransact.h b/proxy/http/HttpTransact.h index f3766c26ee9..9b82b5adf7f 100644 --- a/proxy/http/HttpTransact.h +++ b/proxy/http/HttpTransact.h @@ -965,6 +965,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/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.")