From a911e3164d5355dcd3cad33dd81dbb8ed39debc0 Mon Sep 17 00:00:00 2001 From: bneradt Date: Tue, 3 Dec 2019 16:23:14 +0000 Subject: [PATCH] Fixing TS_HTTP_REQUEST_BUFFER_READ_COMPLETE_HOOK enum value. The value for TS_HTTP_REQUEST_BUFFER_READ_COMPLETE_HOOK was out of sync with the corresponding event, TS_EVENT_HTTP_REQUEST_BUFFER_READ_COMPLETE. This caused the handler for TS_HTTP_REQUEST_BUFFER_READ_COMPLETE_HOOK to never be invoked with the TS_EVENT_HTTP_REQUEST_BUFFER_READ_COMPLETE event. This patch fixes this problem and adds comments explaining the otherwise implicit but very important requirement that these values correspond as they do. The naming of the event and hook were off too. A hook with name TS_HTTP_X_HOOK should have an event of name TS_EVENT_HTTP_X, but these got out of sync somehow for request buffer read complete. This adjusts the event name appropriately. --- .../c-api/request_buffer/request_buffer.c | 2 +- include/ts/apidefs.h.in | 54 +++++--- proxy/http/HttpDebugNames.cc | 4 +- src/traffic_server/InkAPITest.cc | 25 +++- .../gold_tests/pluginTest/test_hooks/200.gold | 28 ++++ .../pluginTest/test_hooks/body_buffer.test.py | 129 ++++++++++++++++++ tests/tools/plugins/request_buffer.c | 4 +- 7 files changed, 220 insertions(+), 26 deletions(-) create mode 100644 tests/gold_tests/pluginTest/test_hooks/200.gold create mode 100644 tests/gold_tests/pluginTest/test_hooks/body_buffer.test.py diff --git a/example/plugins/c-api/request_buffer/request_buffer.c b/example/plugins/c-api/request_buffer/request_buffer.c index 69ab0c6269d..2214cda5243 100644 --- a/example/plugins/c-api/request_buffer/request_buffer.c +++ b/example/plugins/c-api/request_buffer/request_buffer.c @@ -67,7 +67,7 @@ request_buffer_plugin(TSCont contp, TSEvent event, void *edata) { TSDebug(PLUGIN_NAME, "request_buffer_plugin starting, event[%d]", event); TSHttpTxn txnp = (TSHttpTxn)(edata); - if (event == TS_EVENT_HTTP_REQUEST_BUFFER_COMPLETE) { + if (event == TS_EVENT_HTTP_REQUEST_BUFFER_READ_COMPLETE) { int len = 0; char *body = request_body_get(txnp, &len); TSDebug(PLUGIN_NAME, "request_buffer_plugin gets the request body with length[%d]", len); diff --git a/include/ts/apidefs.h.in b/include/ts/apidefs.h.in index a33cc192d78..320869f1f18 100644 --- a/include/ts/apidefs.h.in +++ b/include/ts/apidefs.h.in @@ -261,6 +261,14 @@ typedef enum { TS_HTTP_LAST_HOOK _must_ be the last element. Only right place to insert a new element is just before TS_HTTP_LAST_HOOK. + @note The TS_HTTP hooks below have to be in the same order as their + corresponding TS_EVENT counterparts. We use this in calculating the + corresponding event from a hook ID by summing + TS_EVENT_HTTP_READ_REQUEST_HDR with the hook ID (see the invoke call in + HttpSM::state_api_callout). For example, the following expression must + be true: + + TS_EVENT_HTTP_TXN_CLOSE == TS_EVENT_HTTP_READ_REQUEST_HDR + TS_HTTP_TXN_CLOSE_HOOK */ typedef enum { TS_HTTP_READ_REQUEST_HDR_HOOK, @@ -279,7 +287,14 @@ typedef enum { TS_HTTP_CACHE_LOOKUP_COMPLETE_HOOK, TS_HTTP_PRE_REMAP_HOOK, TS_HTTP_POST_REMAP_HOOK, + TS_HTTP_REQUEST_BUFFER_READ_COMPLETE_HOOK, TS_HTTP_RESPONSE_CLIENT_HOOK, + + // NOTE: + // If adding any TS_HTTP hooks, be sure to understand the note above in the + // doxygen comment about the ordering of these enum values with respect to + // their corresponding TS_EVENT values. + // Putting the SSL hooks in the same enum space // So both sets of hooks can be set by the same Hook function TS_SSL_FIRST_HOOK, @@ -297,7 +312,6 @@ typedef enum { TS_VCONN_OUTBOUND_START_HOOK, TS_VCONN_OUTBOUND_CLOSE_HOOK, TS_SSL_LAST_HOOK = TS_VCONN_OUTBOUND_CLOSE_HOOK, - TS_HTTP_REQUEST_BUFFER_READ_COMPLETE_HOOK, TS_HTTP_LAST_HOOK } TSHttpHookID; @@ -459,25 +473,25 @@ typedef enum { TS_EVENT_AIO_DONE = 3900, - TS_EVENT_HTTP_CONTINUE = 60000, - TS_EVENT_HTTP_ERROR = 60001, - TS_EVENT_HTTP_READ_REQUEST_HDR = 60002, - TS_EVENT_HTTP_OS_DNS = 60003, - TS_EVENT_HTTP_SEND_REQUEST_HDR = 60004, - TS_EVENT_HTTP_READ_CACHE_HDR = 60005, - TS_EVENT_HTTP_READ_RESPONSE_HDR = 60006, - TS_EVENT_HTTP_SEND_RESPONSE_HDR = 60007, - TS_EVENT_HTTP_REQUEST_TRANSFORM = 60008, - TS_EVENT_HTTP_RESPONSE_TRANSFORM = 60009, - TS_EVENT_HTTP_SELECT_ALT = 60010, - TS_EVENT_HTTP_TXN_START = 60011, - TS_EVENT_HTTP_TXN_CLOSE = 60012, - TS_EVENT_HTTP_SSN_START = 60013, - TS_EVENT_HTTP_SSN_CLOSE = 60014, - TS_EVENT_HTTP_CACHE_LOOKUP_COMPLETE = 60015, - TS_EVENT_HTTP_PRE_REMAP = 60016, - TS_EVENT_HTTP_POST_REMAP = 60017, - TS_EVENT_HTTP_REQUEST_BUFFER_COMPLETE = 60018, + TS_EVENT_HTTP_CONTINUE = 60000, + TS_EVENT_HTTP_ERROR = 60001, + TS_EVENT_HTTP_READ_REQUEST_HDR = 60002, + TS_EVENT_HTTP_OS_DNS = 60003, + TS_EVENT_HTTP_SEND_REQUEST_HDR = 60004, + TS_EVENT_HTTP_READ_CACHE_HDR = 60005, + TS_EVENT_HTTP_READ_RESPONSE_HDR = 60006, + TS_EVENT_HTTP_SEND_RESPONSE_HDR = 60007, + TS_EVENT_HTTP_REQUEST_TRANSFORM = 60008, + TS_EVENT_HTTP_RESPONSE_TRANSFORM = 60009, + TS_EVENT_HTTP_SELECT_ALT = 60010, + TS_EVENT_HTTP_TXN_START = 60011, + TS_EVENT_HTTP_TXN_CLOSE = 60012, + TS_EVENT_HTTP_SSN_START = 60013, + TS_EVENT_HTTP_SSN_CLOSE = 60014, + TS_EVENT_HTTP_CACHE_LOOKUP_COMPLETE = 60015, + TS_EVENT_HTTP_PRE_REMAP = 60016, + TS_EVENT_HTTP_POST_REMAP = 60017, + TS_EVENT_HTTP_REQUEST_BUFFER_READ_COMPLETE = 60018, TS_EVENT_LIFECYCLE_PORTS_INITIALIZED = 60100, TS_EVENT_LIFECYCLE_PORTS_READY = 60101, diff --git a/proxy/http/HttpDebugNames.cc b/proxy/http/HttpDebugNames.cc index f99e5b1c135..3f691e6546c 100644 --- a/proxy/http/HttpDebugNames.cc +++ b/proxy/http/HttpDebugNames.cc @@ -355,8 +355,8 @@ HttpDebugNames::get_event_name(int event) return "TS_EVENT_VCONN_CLOSE"; case TS_EVENT_LIFECYCLE_MSG: return "TS_EVENT_LIFECYCLE_MSG"; - case TS_EVENT_HTTP_REQUEST_BUFFER_COMPLETE: - return "TS_EVENT_HTTP_REQUEST_BUFFER_COMPLETE"; + case TS_EVENT_HTTP_REQUEST_BUFFER_READ_COMPLETE: + return "TS_EVENT_HTTP_REQUEST_BUFFER_READ_COMPLETE"; case TS_EVENT_MGMT_UPDATE: return "TS_EVENT_MGMT_UPDATE"; case TS_EVENT_INTERNAL_60200: diff --git a/src/traffic_server/InkAPITest.cc b/src/traffic_server/InkAPITest.cc index 685da749bb1..513ad42f4b7 100644 --- a/src/traffic_server/InkAPITest.cc +++ b/src/traffic_server/InkAPITest.cc @@ -83,6 +83,29 @@ #define ERROR_BODY "TESTING ERROR PAGE" #define TRANSFORM_APPEND_STRING "This is a transformed response" +////////////////////////////////////////////////////////////////////////////// +// These static asserts verify the hook vs event value relationship described +// in the doxygen comment for TSHttpHookID. +////////////////////////////////////////////////////////////////////////////// +static_assert(TS_EVENT_HTTP_READ_REQUEST_HDR == TS_EVENT_HTTP_READ_REQUEST_HDR + TS_HTTP_READ_REQUEST_HDR_HOOK); +static_assert(TS_EVENT_HTTP_OS_DNS == TS_EVENT_HTTP_READ_REQUEST_HDR + TS_HTTP_OS_DNS_HOOK); +static_assert(TS_EVENT_HTTP_SEND_REQUEST_HDR == TS_EVENT_HTTP_READ_REQUEST_HDR + TS_HTTP_SEND_REQUEST_HDR_HOOK); +static_assert(TS_EVENT_HTTP_READ_CACHE_HDR == TS_EVENT_HTTP_READ_REQUEST_HDR + TS_HTTP_READ_CACHE_HDR_HOOK); +static_assert(TS_EVENT_HTTP_READ_RESPONSE_HDR == TS_EVENT_HTTP_READ_REQUEST_HDR + TS_HTTP_READ_RESPONSE_HDR_HOOK); +static_assert(TS_EVENT_HTTP_SEND_RESPONSE_HDR == TS_EVENT_HTTP_READ_REQUEST_HDR + TS_HTTP_SEND_RESPONSE_HDR_HOOK); +static_assert(TS_EVENT_HTTP_REQUEST_TRANSFORM == TS_EVENT_HTTP_READ_REQUEST_HDR + TS_HTTP_REQUEST_TRANSFORM_HOOK); +static_assert(TS_EVENT_HTTP_RESPONSE_TRANSFORM == TS_EVENT_HTTP_READ_REQUEST_HDR + TS_HTTP_RESPONSE_TRANSFORM_HOOK); +static_assert(TS_EVENT_HTTP_SELECT_ALT == TS_EVENT_HTTP_READ_REQUEST_HDR + TS_HTTP_SELECT_ALT_HOOK); +static_assert(TS_EVENT_HTTP_TXN_START == TS_EVENT_HTTP_READ_REQUEST_HDR + TS_HTTP_TXN_START_HOOK); +static_assert(TS_EVENT_HTTP_TXN_CLOSE == TS_EVENT_HTTP_READ_REQUEST_HDR + TS_HTTP_TXN_CLOSE_HOOK); +static_assert(TS_EVENT_HTTP_SSN_START == TS_EVENT_HTTP_READ_REQUEST_HDR + TS_HTTP_SSN_START_HOOK); +static_assert(TS_EVENT_HTTP_SSN_CLOSE == TS_EVENT_HTTP_READ_REQUEST_HDR + TS_HTTP_SSN_CLOSE_HOOK); +static_assert(TS_EVENT_HTTP_CACHE_LOOKUP_COMPLETE == TS_EVENT_HTTP_READ_REQUEST_HDR + TS_HTTP_CACHE_LOOKUP_COMPLETE_HOOK); +static_assert(TS_EVENT_HTTP_PRE_REMAP == TS_EVENT_HTTP_READ_REQUEST_HDR + TS_HTTP_PRE_REMAP_HOOK); +static_assert(TS_EVENT_HTTP_POST_REMAP == TS_EVENT_HTTP_READ_REQUEST_HDR + TS_HTTP_POST_REMAP_HOOK); +static_assert(TS_EVENT_HTTP_REQUEST_BUFFER_READ_COMPLETE == + TS_EVENT_HTTP_READ_REQUEST_HDR + TS_HTTP_REQUEST_BUFFER_READ_COMPLETE_HOOK); + ////////////////////////////////////////////////////////////////////////////// // STRUCTURES ////////////////////////////////////////////////////////////////////////////// @@ -6603,6 +6626,7 @@ typedef enum { ORIG_TS_HTTP_CACHE_LOOKUP_COMPLETE_HOOK, ORIG_TS_HTTP_PRE_REMAP_HOOK, ORIG_TS_HTTP_POST_REMAP_HOOK, + ORIG_TS_HTTP_REQUEST_BUFFER_READ_COMPLETE_HOOK, ORIG_TS_HTTP_RESPONSE_CLIENT_HOOK, ORIG_TS_SSL_FIRST_HOOK, ORIG_TS_VCONN_START_HOOK = ORIG_TS_SSL_FIRST_HOOK, @@ -6616,7 +6640,6 @@ typedef enum { ORIG_TS_VCONN_OUTBOUND_START_HOOK, ORIG_TS_VCONN_OUTBOUND_CLOSE_HOOK, ORIG_TS_SSL_LAST_HOOK = ORIG_TS_VCONN_OUTBOUND_CLOSE_HOOK, - ORIG_TS_HTTP_REQUEST_BUFFER_READ_COMPLETE_HOOK, ORIG_TS_HTTP_LAST_HOOK } ORIG_TSHttpHookID; diff --git a/tests/gold_tests/pluginTest/test_hooks/200.gold b/tests/gold_tests/pluginTest/test_hooks/200.gold new file mode 100644 index 00000000000..a8875735ebb --- /dev/null +++ b/tests/gold_tests/pluginTest/test_hooks/200.gold @@ -0,0 +1,28 @@ +`` +> POST /contentlength HTTP/1.1`` +> Host:`` +> User-Agent: curl/`` +> Accept: */*`` +> Content-Length: 22`` +> Content-Type: application/`` +`` +< HTTP/1.1 200 OK`` +< Server: ATS/`` +< Content-Length: 23`` +< Date:`` +< Age:`` +`` +> POST /chunked`` +> Host:`` +> User-Agent: curl/`` +> Accept: */*`` +> Transfer-Encoding: chunked`` +> Content-Type: application/`` +`` +< HTTP/1.1 200 OK`` +< Server: ATS/`` +< Date: `` +< Age: 0`` +< Transfer-Encoding: chunked`` +< Connection: keep-alive`` +`` diff --git a/tests/gold_tests/pluginTest/test_hooks/body_buffer.test.py b/tests/gold_tests/pluginTest/test_hooks/body_buffer.test.py new file mode 100644 index 00000000000..7e84e8914a4 --- /dev/null +++ b/tests/gold_tests/pluginTest/test_hooks/body_buffer.test.py @@ -0,0 +1,129 @@ +''' +Verify HTTP body buffering. +''' +# 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 os + + +def int_to_hex_string(int_value): + ''' + Convert the given int value to a hex string with no '0x' prefix. + + >>> int_to_hex_string(0) + '0' + >>> int_to_hex_string(1) + '1' + >>> int_to_hex_string(10) + 'r' + >>> int_to_hex_string(16) + '0' + >>> int_to_hex_string(17) + 'f1' + ''' + if type(int_value) != int: + raise ValueError("Input should be an int type.") + return hex(int_value).split('x')[1] + + +class BodyBufferTest: + def __init__(cls, description): + Test.Summary = description + cls._origin_max_connections = 3 + cls.setupOriginServer() + cls.setupTS() + + def setupOriginServer(self): + self._server = Test.MakeOriginServer("server") + self.content_length_request_body = "content-length request" + request_header = {"headers": "POST /contentlength HTTP/1.1\r\n" + "Host: www.example.com\r\n" + "Content-Length: {}\r\n\r\n".format( + len(self.content_length_request_body)), + "timestamp": "1469733493.993", + "body": self.content_length_request_body} + content_length_response_body = "content-length response" + response_header = {"headers": "HTTP/1.1 200 OK\r\n" + "Server: microserver\r\n" + "Content-Length: {}\r\n\r\n" + "Connection: close\r\n\r\n".format( + len(content_length_response_body)), + "timestamp": "1469733493.993", + "body": content_length_response_body} + self._server.addResponse("sessionlog.json", request_header, response_header) + + self.chunked_request_body = "chunked request" + self.encoded_chunked_request = "{0}\r\n{1}\r\n0\r\n\r\n".format( + int_to_hex_string(len(self.chunked_request_body)), self.chunked_request_body) + request_header2 = {"headers": "POST /chunked HTTP/1.1\r\n" + "Transfer-Encoding: chunked\r\n" + "Host: www.example.com\r\n" + "Connection: keep-alive\r\n\r\n", + "timestamp": "1469733493.993", + "body": self.encoded_chunked_request} + self.chunked_response_body = "chunked response" + self.encoded_chunked_response = "{0}\r\n{1}\r\n0\r\n\r\n".format( + int_to_hex_string(len(self.chunked_response_body)), self.chunked_response_body) + response_header2 = {"headers": "HTTP/1.1 200 OK\r\n" + "Transfer-Encoding: chunked\r\n" + "Server: microserver\r\n" + "Connection: close\r\n\r\n", + "timestamp": "1469733493.993", + "body": self.encoded_chunked_response} + self._server.addResponse("sessionlog.json", request_header2, response_header2) + + def setupTS(self): + self._ts = Test.MakeATSProcess("ts", select_ports=False) + self._ts.Disk.remap_config.AddLine( + 'map / http://127.0.0.1:{0}'.format(self._server.Variables.Port) + ) + Test.PreparePlugin(os.path.join(Test.Variables.AtsTestToolsDir, 'plugins', 'request_buffer.c'), self._ts) + self._ts.Disk.records_config.update({ + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'request_buffer', + 'proxy.config.http.per_server.connection.max': self._origin_max_connections, + # Disable queueing when connection reaches limit + 'proxy.config.http.per_server.connection.queue_size': 0, + }) + + self._ts.Streams.stderr = Testers.ContainsExpression( + "request_buffer_plugin gets the request body with length\[{0}\]: \[{1}\]".format( + len(self.content_length_request_body), self.content_length_request_body), + "Verify that the plugin parsed the content-length request body data.") + self._ts.Streams.stderr += Testers.ContainsExpression( + "request_buffer_plugin gets the request body with length\[{0}\]: \[".format( + len(self.encoded_chunked_request)), + "Verify that the plugin parsed the chunked request body.") + self._ts.Streams.stderr += Testers.ContainsExpression( + "^{}".format(self.chunked_request_body), + "Verify that the plugin parsed the chunked request body data.") + + def run(self): + tr = Test.AddTestRun() + # Send both a Content-Length request and a chunked-encoded request. + tr.Processes.Default.Command = ( + 'curl -v http://127.0.0.1:{0}/contentlength -d "{1}" ; ' + 'curl -v http://127.0.0.1:{0}/chunked -H "Transfer-Encoding: chunked" -d "{2}"'.format( + self._ts.Variables.port, self.content_length_request_body, self.chunked_request_body)) + tr.Processes.Default.ReturnCode = 0 + tr.Processes.Default.StartBefore(self._server) + tr.Processes.Default.StartBefore(Test.Processes.ts) + tr.Processes.Default.Streams.stderr = "200.gold" + + +bodyBufferTest = BodyBufferTest("Test request body buffering.") +bodyBufferTest.run() diff --git a/tests/tools/plugins/request_buffer.c b/tests/tools/plugins/request_buffer.c index 7e6df14413c..eae6b9fe224 100644 --- a/tests/tools/plugins/request_buffer.c +++ b/tests/tools/plugins/request_buffer.c @@ -65,10 +65,10 @@ request_buffer_plugin(TSCont contp, TSEvent event, void *edata) { TSDebug(PLUGIN_NAME, "request_buffer_plugin starting, event[%d]", event); TSHttpTxn txnp = (TSHttpTxn)(edata); - if (event == TS_EVENT_HTTP_REQUEST_BUFFER_COMPLETE) { + if (event == TS_EVENT_HTTP_REQUEST_BUFFER_READ_COMPLETE) { int len = 0; char *body = request_body_get(txnp, &len); - TSDebug(PLUGIN_NAME, "request_buffer_plugin gets the request body with length[%d]", len); + TSDebug(PLUGIN_NAME, "request_buffer_plugin gets the request body with length[%d]: [%s]", len, body); TSfree(body); TSContDestroy(contp); }