From b046fdce822aa7e0406aadbbed0d1dde4a17e138 Mon Sep 17 00:00:00 2001 From: Brian Date: Sun, 6 Nov 2022 01:58:58 +0000 Subject: [PATCH] per_server.connection.max: CONNECT requests We were putting servers that hit the per_server.connection.max limit on the dead server list if the client were connecting to them via HTTP CONNECT requests. This fixes the handling of these so the server is not put on the dead server list. --- proxy/http/HttpTransact.h | 21 +++-- .../gold/two_503_congested.gold | 9 ++ .../per_server_connection_max.test.py | 88 +++++++++++++++++-- 3 files changed, 107 insertions(+), 11 deletions(-) create mode 100644 tests/gold_tests/origin_connection/gold/two_503_congested.gold diff --git a/proxy/http/HttpTransact.h b/proxy/http/HttpTransact.h index 6fbe0f36599..db30be7cf64 100644 --- a/proxy/http/HttpTransact.h +++ b/proxy/http/HttpTransact.h @@ -722,10 +722,15 @@ class HttpTransact int64_t internal_msg_buffer_size = 0; // out int64_t internal_msg_buffer_fast_allocator_size = -1; - int scheme = -1; // out - int next_hop_scheme = scheme; // out - int orig_scheme = scheme; // pre-mapped scheme - int method = 0; + int scheme = -1; // out + int next_hop_scheme = scheme; // out + int orig_scheme = scheme; // pre-mapped scheme + int method = 0; + + /// The errno associated with a failed connect attempt. + /// + /// This is used for logging and (in some code paths) for determing HTTP + /// response reason phrases. int cause_of_death_errno = -UNKNOWN_INTERNAL_ERROR; // in ink_time_t client_request_time = UNDEFINED_TIME; // internal @@ -918,7 +923,13 @@ class HttpTransact void set_connect_fail(int e) { - if (e == EIO || this->current.server->connect_result == EIO) { + if (e == EUSERS) { + // EUSERS is used when the number of connections exceeds the configured + // limit. Since this is not a network connectivity issue with the + // server, we should not mark it as such. Otherwise we will incorrectly + // mark the server as down. + this->current.server->connect_result = 0; + } else if (e == EIO || this->current.server->connect_result == EIO) { this->current.server->connect_result = e; } if (e != EIO) { diff --git a/tests/gold_tests/origin_connection/gold/two_503_congested.gold b/tests/gold_tests/origin_connection/gold/two_503_congested.gold new file mode 100644 index 00000000000..ff5b5495cef --- /dev/null +++ b/tests/gold_tests/origin_connection/gold/two_503_congested.gold @@ -0,0 +1,9 @@ +`` +> CONNECT`` +`` +< HTTP/1.1 503 Origin server congested +`` +> CONNECT`` +`` +< HTTP/1.1 503 Origin server congested +`` diff --git a/tests/gold_tests/origin_connection/per_server_connection_max.test.py b/tests/gold_tests/origin_connection/per_server_connection_max.test.py index d1d4a4da614..380fc928f09 100644 --- a/tests/gold_tests/origin_connection/per_server_connection_max.test.py +++ b/tests/gold_tests/origin_connection/per_server_connection_max.test.py @@ -33,22 +33,23 @@ def __init__(self) -> None: self._configure_server() self._configure_trafficserver() - def _configure_dns(self): + def _configure_dns(self) -> None: """Configure a nameserver for the test.""" - self._nameserver = Test.MakeDNServer("dns", default='127.0.0.1') + self._dns = Test.MakeDNServer("dns1", default='127.0.0.1') def _configure_server(self) -> None: """Configure the server to be used in the test.""" - self._server = Test.MakeVerifierServerProcess('server', self._replay_file) + self._server = Test.MakeVerifierServerProcess('server1', self._replay_file) def _configure_trafficserver(self) -> None: """Configure Traffic Server to be used in the test.""" - self._ts = Test.MakeATSProcess("ts") + self._ts = Test.MakeATSProcess("ts1") self._ts.Disk.remap_config.AddLine( f'map / http://127.0.0.1:{self._server.Variables.http_port}' ) self._ts.Disk.records_config.update({ - 'proxy.config.dns.nameservers': f"127.0.0.1:{self._nameserver.Variables.Port}", + 'proxy.config.dns.nameservers': f"127.0.0.1:{self._dns.Variables.Port}", + 'proxy.config.dns.resolv_conf': 'NULL', 'proxy.config.diags.debug.enabled': 1, 'proxy.config.diags.debug.tags': 'http', 'proxy.config.http.per_server.connection.max': self._origin_max_connections, @@ -61,7 +62,7 @@ def run(self) -> None: """Configure the TestRun.""" tr = Test.AddTestRun( 'Verify we enforce proxy.config.http.per_server.connection.max') - tr.Processes.Default.StartBefore(self._nameserver) + tr.Processes.Default.StartBefore(self._dns) tr.Processes.Default.StartBefore(self._server) tr.Processes.Default.StartBefore(self._ts) @@ -71,4 +72,79 @@ def run(self) -> None: http_ports=[self._ts.Variables.port]) +class ConnectMethodTest: + """Test our max origin connection behavior with CONNECT traffic.""" + + _client_counter: int = 0 + _origin_max_connections: int = 3 + + def __init__(self) -> None: + """Configure the server processes in preparation for the TestRun.""" + self._configure_dns() + self._configure_origin_server() + self._configure_trafficserver() + + def _configure_dns(self) -> None: + """Configure a nameserver for the test.""" + self._dns = Test.MakeDNServer("dns2", default='127.0.0.1') + + def _configure_origin_server(self) -> None: + """Configure the httpbin origin server.""" + self._server = Test.MakeHttpBinServer("server2") + + def _configure_trafficserver(self) -> None: + self._ts = Test.MakeATSProcess("ts2") + + self._ts.Disk.records_config.update({ + 'proxy.config.dns.nameservers': f"127.0.0.1:{self._dns.Variables.Port}", + 'proxy.config.dns.resolv_conf': 'NULL', + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'http|dns|hostdb', + 'proxy.config.http.server_ports': f"{self._ts.Variables.port}", + 'proxy.config.http.connect_ports': f"{self._server.Variables.Port}", + + 'proxy.config.http.per_server.connection.max': self._origin_max_connections, + }) + + self._ts.Disk.remap_config.AddLines([ + f"map http://foo.com/ http://www.this.origin.com:{self._server.Variables.Port}/", + ]) + + def _configure_client_with_slow_response(self, tr) -> 'Test.Process': + """Configure a client to perform a CONNECT request with a slow response from the server.""" + p = tr.Processes.Process(f'slow_client_{self._client_counter}') + self._client_counter += 1 + p.Command = f"curl -v --fail -s -p -x 127.0.0.1:{self._ts.Variables.port} 'http://foo.com/delay/2'" + return p + + def run(self) -> None: + """Verify per_server.connection.max with CONNECT traffic.""" + tr = Test.AddTestRun() + tr.Processes.Default.StartBefore(self._dns) + tr.Processes.Default.StartBefore(self._server) + tr.Processes.Default.StartBefore(self._ts) + + slow0 = self._configure_client_with_slow_response(tr) + slow1 = self._configure_client_with_slow_response(tr) + slow2 = self._configure_client_with_slow_response(tr) + + tr.Processes.Default.StartBefore(slow0) + tr.Processes.Default.StartBefore(slow1) + tr.Processes.Default.StartBefore(slow2) + + # With those three slow transactions going on in the background, do a + # couple quick transactions and make sure they both reply with a 503 + # response. + tr.Processes.Default.Command = ( + f"sleep 1; curl -v --fail -s -p -x 127.0.0.1:{self._ts.Variables.port} 'http://foo.com/get'" + f"--next -v --fail -s -p -x 127.0.0.1:{self._ts.Variables.port} 'http://foo.com/get'" + ) + # Curl will have a 22 exit code if it receives a 5XX response (and we + # expect a 503). + tr.Processes.Default.ReturnCode = 22 + tr.Processes.Default.Streams.stderr = "gold/two_503_congested.gold" + tr.Processes.Default.TimeOut = 3 + + PerServerConnectionMaxTest().run() +ConnectMethodTest().run()