From 62dcf260d24e426a78720d597347a32c19819efc Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Wed, 10 Jul 2024 12:05:31 -0500 Subject: [PATCH 1/2] 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. --- ...direct_to_same_origin_on_cache.replay.yaml | 65 ++++++++ .../redirect_to_same_origin_on_cache.test.py | 140 ++++++++++++++++++ 2 files changed, 205 insertions(+) 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/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.") From ce094297b5cbd23fc64a906e43cf04d417ccdcc1 Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Fri, 12 Jul 2024 13:08:52 -0500 Subject: [PATCH 2/2] 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. --- include/proxy/http/HttpTransact.h | 1 + src/proxy/http/HttpSM.cc | 3 +-- src/proxy/http/HttpTransact.cc | 31 ++++++++++++++++++++++++------- 3 files changed, 26 insertions(+), 9 deletions(-) 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