Make sure, when plugins cause a GET request to fail, there is always a response body.

plugins/xdebug/Cleanup.h → include/tscpp/api/Cleanup.h

@@ -124,9 +124,13 @@ class TxnAuxDataMgrBase
 
 using TxnAuxMgrData = TxnAuxDataMgrBase::MgrData;
 
-// Class to manage auxiliary data for a transaction.  If an instance is created for the transaction, the instance
-// will be deleted on the TXN_CLOSE transaction hook (which is always triggered for all transactions).
-// The TxnAuxData class must have a public default constructor.
+// Class to manage auxiliary data for a transaction.  If an instance of auxiliary data is created for the
+// transaction, the instance will be deleted on the TXN_CLOSE transaction hook (which is always triggered
+// for all transactions).  The TxnAuxData class must have a public default constructor.
+//
+// This class and the base class are pseudo-namespaces, have only static member functions (no data members).
+// The manager data cannot simply be in the base class, because the (static member) continuation function that
+// runs on the TXN_CLOSE hook would not be able to access it.
 //
 template <class TxnAuxData, TxnAuxMgrData &MDRef> class TxnAuxDataMgr : private TxnAuxDataMgrBase
 {

plugins/xdebug/xdebug.cc

@@ -31,11 +31,11 @@
 #include <unistd.h>
 
 #include <ts/ts.h>
-#include "ts/experimental.h"
-#include "tscore/ink_defs.h"
-#include "tscpp/util/PostScript.h"
-#include "tscpp/util/TextView.h"
-#include "Cleanup.h"
+#include <ts/experimental.h>
+#include <tscore/ink_defs.h>
+#include <tscpp/util/PostScript.h>
+#include <tscpp/util/TextView.h>
+#include <tscpp/api/Cleanup.h>
 
 namespace
 {

proxy/http/HttpTransact.cc

@@ -1743,8 +1743,7 @@ HttpTransact::HandleApiErrorJump(State *s)
   **/
   if (s->http_return_code && s->http_return_code >= HTTP_STATUS_BAD_REQUEST) {
     const char *reason = http_hdr_reason_lookup(s->http_return_code);
-    ;
-    build_response(s, &s->hdr_info.client_response, s->client_info.http_version, s->http_return_code, reason ? reason : "Error");
+    build_error_response(s, s->http_return_code, reason ? reason : "Error", nullptr);
   } else {
     build_response(s, &s->hdr_info.client_response, s->client_info.http_version, HTTP_STATUS_INTERNAL_SERVER_ERROR, "INKApi Error");
   }

tests/Makefile.am

@@ -34,6 +34,7 @@ include gold_tests/bigobj/Makefile.inc
 include gold_tests/continuations/plugins/Makefile.inc
 include gold_tests/chunked_encoding/Makefile.inc
 include gold_tests/pluginTest/tsapi/Makefile.inc
+include gold_tests/pluginTest/pi_set_err_status/Makefile.inc
 include gold_tests/timeout/Makefile.inc
 include gold_tests/tls/Makefile.inc
 include tools/plugins/Makefile.inc

tests/gold_tests/pluginTest/pi_set_err_status/Makefile.inc

@@ -0,0 +1,19 @@
+#  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.
+
+noinst_LTLIBRARIES += gold_tests/pluginTest/pi_set_err_status/pi_set_err_status.la
+gold_tests_pluginTest_pi_set_err_status_pi_set_err_status_la_SOURCES = \
+  gold_tests/pluginTest/pi_set_err_status/pi_set_err_status.cc

tests/gold_tests/pluginTest/pi_set_err_status/pi_set_err_status.cc

@@ -0,0 +1,197 @@
+/*
+ * 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.
+ */
+
+// This is used to contrast how the code would look if the proposed C++ wrapper
+// for TS C API calls to access components of HTTP Messages.
+//
+#define USE_HTTP_MSG_COMP 0
+
+#include <string>
+
+#include <ts/ts.h>
+#include <tscpp/util/TextView.h>
+#if USE_HTTP_MSG_COMP
+#include <tscpp/api/HttpMsgComp.h>
+#endif
+#include <tscpp/api/Cleanup.h>
+
+#define PINAME "pi_set_err_status"
+
+namespace
+{
+char const PIName[] = PINAME;
+
+struct AuxData {
+  TSEvent error_event{TS_EVENT_NONE};
+  int http_status_code{-1};
+  std::string resp_body;
+
+  TSEvent last_event{TS_EVENT_NONE};
+
+  ~AuxData()
+  {
+    TSReleaseAssert(TS_EVENT_NONE != error_event);
+    TSReleaseAssert(TS_EVENT_HTTP_SEND_RESPONSE_HDR == last_event);
+  }
+};
+
+atscppapi::TxnAuxMgrData md;
+using AuxDataMgr = atscppapi::TxnAuxDataMgr<AuxData, md>;
+
+int
+contFunc(TSCont, TSEvent event, void *edata)
+{
+  TSDebug(PIName, "event=%u", unsigned(event));
+
+  TSEvent reenable_event{TS_EVENT_HTTP_CONTINUE};
+  auto txn{static_cast<TSHttpTxn>(edata)};
+  {
+    AuxData &d{AuxDataMgr::data(txn)};
+
+    switch (event) {
+    case TS_EVENT_HTTP_READ_REQUEST_HDR: {
+      TSReleaseAssert(TS_EVENT_NONE == d.last_event);
+
+#if USE_HTTP_MSG_COMP
+      atscppapi::TxnClientReq req{txn};
+      TSReleaseAssert(req.hasMsg());
+      atscppapi::MimeField fld{req, "X-Test-Data"};
+      TSReleaseAssert(fld.valid());
+      ts::TextView test_data{fld.valuesGet()};
+#else
+      TSMBuffer msgBuffer;
+      TSMLoc bufLoc;
+      TSReleaseAssert(TS_SUCCESS == TSHttpTxnClientReqGet(txn, &msgBuffer, &bufLoc));
+      char const fldName[] = "X-Test-Data";
+      TSMLoc fldLoc        = TSMimeHdrFieldFind(msgBuffer, bufLoc, fldName, sizeof(fldName) - 1);
+      TSReleaseAssert(TS_NULL_MLOC != fldLoc);
+      int fldValLen;
+      const char *fldValArr = TSMimeHdrFieldValueStringGet(msgBuffer, bufLoc, fldLoc, -1, &fldValLen);
+      TSReleaseAssert(fldValArr && (fldValLen > 0));
+      ts::TextView test_data(fldValArr, fldValLen);
+#endif
+
+      ts::TextView hook_name{test_data.take_prefix_at('/')};
+      TSDebug(PIName, "hook_name=%.*s", int(hook_name.size()), hook_name.data());
+      if ("READ_REQUEST_HDR" == hook_name) {
+        d.error_event = TS_EVENT_HTTP_READ_REQUEST_HDR;
+
+      } else if ("PRE_REMAP" == hook_name) {
+        d.error_event = TS_EVENT_HTTP_PRE_REMAP;
+
+      } else if ("POST_REMAP" == hook_name) {
+        d.error_event = TS_EVENT_HTTP_POST_REMAP;
+
+      } else if ("CACHE_LOOKUP_COMPLETE" == hook_name) {
+        d.error_event = TS_EVENT_HTTP_CACHE_LOOKUP_COMPLETE;
+
+      } else if ("SEND_RESPONSE_HDR" == hook_name) {
+        d.error_event = TS_EVENT_HTTP_SEND_RESPONSE_HDR;
+
+      } else {
+        TSReleaseAssert(false);
+      }
+
+      d.http_status_code = int(svtoi(test_data.take_prefix_at('/')));
+      d.resp_body        = test_data;
+
+      TSDebug(PIName, "hook_name=%.*s status=%d body=%s", int(hook_name.size()), hook_name.data(), d.http_status_code,
+              d.resp_body.c_str());
+
+#if !USE_HTTP_MSG_COMP
+      // Only releasing mloc for the field because the release for the message mloc does nothing.
+      //
+      TSReleaseAssert(TSHandleMLocRelease(msgBuffer, bufLoc, fldLoc) == TS_SUCCESS);
+#endif
+    } break;
+
+    case TS_EVENT_HTTP_PRE_REMAP: {
+      TSReleaseAssert(TS_EVENT_HTTP_READ_REQUEST_HDR == d.last_event);
+    } break;
+
+    case TS_EVENT_HTTP_POST_REMAP: {
+      TSReleaseAssert(TS_EVENT_HTTP_PRE_REMAP == d.last_event);
+    } break;
+
+    case TS_EVENT_HTTP_CACHE_LOOKUP_COMPLETE: {
+      TSReleaseAssert(TS_EVENT_HTTP_POST_REMAP == d.last_event);
+    } break;
+
+    case TS_EVENT_HTTP_SEND_RESPONSE_HDR: {
+      if (TS_EVENT_HTTP_SEND_RESPONSE_HDR == d.error_event) {
+        TSEvent expected_last =
+          TS_EVENT_HTTP_SEND_RESPONSE_HDR == d.error_event ? TS_EVENT_HTTP_CACHE_LOOKUP_COMPLETE : d.error_event;
+        TSReleaseAssert(expected_last == d.last_event);
+      }
+    } break;
+
+    default:
+      TSReleaseAssert(false);
+    }
+
+    if (event == d.error_event) {
+      reenable_event = TS_EVENT_HTTP_ERROR;
+      TSHttpTxnStatusSet(txn, TSHttpStatus(d.http_status_code));
+      if (!d.resp_body.empty()) {
+        TSHttpTxnErrorBodySet(txn, TSstrdup(d.resp_body.c_str()), d.resp_body.size(), nullptr);
+      }
+    }
+
+    d.last_event = event;
+  }
+
+  TSHttpTxnReenable(txn, reenable_event);
+
+  return 0;
+}
+
+} // end anonymous namespace
+
+void
+TSPluginInit(int n_arg, char const *arg[])
+{
+  TSDebug(PIName, "initializing plugin");
+
+  TSPluginRegistrationInfo info;
+
+  info.plugin_name   = const_cast<char *>(PIName);
+  info.vendor_name   = const_cast<char *>("Apache");
+  info.support_email = const_cast<char *>("dev-subscribe@trafficserver.apache.com");
+
+  if (TSPluginRegister(&info) != TS_SUCCESS) {
+    TSError(PINAME ": Plugin registration failed.");
+    return;
+  } else {
+    TSDebug(PIName, "Plugin registration succeeded.");
+  }
+
+  AuxDataMgr::init(PIName);
+
+  if (n_arg != 1) {
+    TSError(PINAME ": global initialization failed, no plugin arguments allowed");
+    return;
+  }
+
+  TSCont contp = TSContCreate(contFunc, nullptr);
+  TSReleaseAssert(contp != nullptr);
+  TSHttpHookAdd(TS_HTTP_READ_REQUEST_HDR_HOOK, contp);
+  TSHttpHookAdd(TS_HTTP_PRE_REMAP_HOOK, contp);
+  TSHttpHookAdd(TS_HTTP_POST_REMAP_HOOK, contp);
+  TSHttpHookAdd(TS_HTTP_CACHE_LOOKUP_COMPLETE_HOOK, contp);
+  TSHttpHookAdd(TS_HTTP_SEND_RESPONSE_HDR_HOOK, contp);
+}

tests/gold_tests/pluginTest/pi_set_err_status/pi_set_err_status.test.py

@@ -0,0 +1,93 @@
+#  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
+
+
+Test.Summary = '''
+Test forcing error responses on various TXN hooks.
+'''
+
+Test.ContinueOnFail = True
+
+plugin_name = "pi_set_err_status"
+
+# Disable the cache to make sure each request is forwarded to the origin
+# server.
+ts = Test.MakeATSProcess("ts", enable_tls=True, enable_cache=False)
+
+dns = Test.MakeDNServer("dns")
+
+dns.addRecords(records={"test-host.com": ["127.0.0.1"]})
+
+ts.Disk.records_config.update({
+    'proxy.config.url_remap.remap_required': 0,
+    'proxy.config.dns.nameservers': f'127.0.0.1:{dns.Variables.Port}',
+    'proxy.config.dns.resolv_conf': 'NULL',
+    'proxy.config.diags.debug.enabled': 1,
+    'proxy.config.diags.debug.tags': f'http|{plugin_name}',
+})
+
+rp = os.path.join(Test.Variables.AtsBuildGoldTestsDir, 'pluginTest', plugin_name, '.libs', plugin_name + '.so')
+ts.Setup.Copy(rp, ts.Env['PROXY_CONFIG_PLUGIN_PLUGIN_DIR'])
+
+ts.Disk.plugin_config.AddLine(plugin_name + '.so')
+
+ts.Setup.Copy('run_curl.sh')
+
+tr = Test.AddTestRun()
+tr.Processes.Default.StartBefore(ts)
+tr.Processes.Default.StartBefore(dns)
+# Make run_curl.sh executable.
+tr.Processes.Default.Command = "bash -c 'chmod +x run_curl.sh'"
+tr.Processes.Default.ReturnCode = 0
+
+
+def addRuns(http_status):
+    def add2Runs(hook_name, http_status):
+        tr = Test.AddTestRun()
+        tr.Processes.Default.Command = (
+            f'./run_curl.sh --verbose --ipv4 --header "X-Test-Data: {hook_name}/{http_status}" ' +
+            f'--header "Host: test-host.com" http://localhost:{ts.Variables.port}/'
+        )
+        tr.Processes.Default.ReturnCode = 0
+
+        tr = Test.AddTestRun()
+        tr.Processes.Default.Command = (
+            f'./run_curl.sh --verbose --ipv4 --header "X-Test-Data: {hook_name}/{http_status}/'
+            f'body for hook {hook_name}, status {http_status}" ' +
+            f'--header "Host: test-host.com" http://localhost:{ts.Variables.port}/'
+        )
+        tr.Processes.Default.ReturnCode = 0
+
+    add2Runs("READ_REQUEST_HDR", http_status)
+    add2Runs("PRE_REMAP", http_status)
+    add2Runs("POST_REMAP", http_status)
+    add2Runs("CACHE_LOOKUP_COMPLETE", http_status)
+    add2Runs("SEND_RESPONSE_HDR", http_status)
+
+
+addRuns(400)
+addRuns(403)
+addRuns(451)
+
+tr = Test.AddTestRun()
+tr.Processes.Default.Command = 'echo check output'
+tr.Processes.Default.ReturnCode = 0
+f = tr.Disk.File('stdout')
+f.Content = 'stdout.gold'
+f = tr.Disk.File('stderr')
+f.Content = 'stderr.gold'

tests/gold_tests/pluginTest/pi_set_err_status/run_curl.sh

@@ -0,0 +1,26 @@
+#!/bin/env bash
+
+#  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.
+
+curl "$@" >> stdout 2> stderr1
+printf '\n=====\n' >> stdout
+# Get in/out HTTP header lines that are consistent.
+tr -d '\r' < stderr1 | sed 's/\([<>]\)/\
+\1/g' | grep '^[<>]' | \
+grep -iv -e '^< Date: ' -e '^< Server: ' -e '^< Connection: ' -e '> User-Agent: ' >> stderr
+printf '=====\n' >> stderr
+rm stderr1