This is an automated email from the ASF dual-hosted git repository.
jvanderzee pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new 875463c964 Reuse prepared cache write on redirected request (#11542)
875463c964 is described below
commit 875463c9645787d380b3215b414e906c6c38cd7b
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.
---
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 6dc467f52f..a691457c41 100644
--- a/src/proxy/http/HttpSM.cc
+++ b/src/proxy/http/HttpSM.cc
@@ -8041,9 +8041,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.")
