Fix retry logic for TSHttpTxnServerAddrSet (issue #12611)

doc/developer-guide/api/functions/TSHttpTxnServerAddrSet.en.rst

@@ -51,10 +51,10 @@ Notes
 =====
 
 If |TS| is configured to retry connections to origin servers and
-:func:`TSHttpTxnServerAddrGet` has been called, |TS| will return
-to TS_HTTP_OS_DNS_HOOK so to let the plugin set a different server
-address. Plugins should be prepared for TS_HTTP_OS_DNS_HOOK and any
-subsequent hooks to be called multiple times.
+:func:`TSHttpTxnServerAddrSet` has been called, |TS| will return
+to TS_HTTP_OS_DNS_HOOK once to let the plugin set a different server
+address. Plugins should be prepared for TS_HTTP_OS_DNS_HOOK to be
+called one additional time on connection failure.
 
 Once a plugin calls :func:`TSHttpTxnServerAddrGet` any prior DNS
 resolution results are lost. The plugin should use

include/proxy/http/HttpTransact.h

@@ -683,6 +683,7 @@ class HttpTransact
     bool api_server_request_body_set  = false;
     bool api_req_cacheable            = false;
     bool api_resp_cacheable           = false;
+    bool api_server_addr_set_retried  = false;
     bool reverse_proxy                = false;
     bool url_remap_success            = false;
     bool api_skip_all_remapping       = false;

src/proxy/http/HttpTransact.cc

@@ -3770,6 +3770,18 @@ HttpTransact::handle_response_from_server(State *s)
         // families - that is locked in by the client source address.
         ats_force_order_by_family(s->current.server->dst_addr.family(), s->my_txn_conf().host_res_data.order);
         return CallOSDNSLookup(s);
+      } else if (ResolveInfo::OS_Addr::USE_API == s->dns_info.os_addr_style && !s->api_server_addr_set_retried) {
+        // Plugin set the server address via TSHttpTxnServerAddrSet(). Clear resolution
+        // state to allow the OS_DNS hook to be called again, giving the plugin a chance
+        // to set a different server address for retry (issue #12611).
+        // Only retry once to avoid infinite loops if the plugin keeps setting failing addresses.
+        s->api_server_addr_set_retried = true;
+        s->dns_info.resolved_p         = false;
+        s->dns_info.os_addr_style      = ResolveInfo::OS_Addr::TRY_DEFAULT;
+        // Clear the server request so it can be rebuilt for the new destination
+        s->hdr_info.server_request.destroy();
+        TxnDbg(dbg_ctl_http_trans, "Retrying with plugin-set address, returning to OS_DNS hook");
+        return CallOSDNSLookup(s);
       } else {
         if ((s->txn_conf->connect_attempts_rr_retries > 0) &&
             ((s->current.retry_attempts.get() + 1) % s->txn_conf->connect_attempts_rr_retries == 0)) {

tests/gold_tests/pluginTest/tsapi/CMakeLists.txt

@@ -20,3 +20,4 @@ add_autest_plugin(test_TSHttpTxnServerAddrSet test_TSHttpTxnServerAddrSet.cc)
 add_autest_plugin(test_TSHttpSsnInfo test_TSHttpSsnInfo.cc)
 add_autest_plugin(test_TSVConnPPInfo test_TSVConnPPInfo.cc)
 add_autest_plugin(test_TSHttpTxnVerifiedAddr test_TSHttpTxnVerifiedAddr.cc)
+add_autest_plugin(test_TSHttpTxnServerAddrSet_retry test_TSHttpTxnServerAddrSet_retry.cc)

tests/gold_tests/pluginTest/tsapi/test_TSHttpTxnServerAddrSet_retry.cc

@@ -0,0 +1,169 @@
+/*
+ * 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.
+ */
+
+/**
+ * Test plugin to reproduce issue #12611
+ *
+ * This plugin sets server addresses via TSHttpTxnServerAddrSet() in the OS_DNS hook.
+ * On first call, it sets a non-routable address that will fail to connect.
+ * On retry (if OS_DNS is called again), it sets a working address.
+ *
+ * BUG: On master, the OS_DNS hook is NOT called again on retry, so the connection
+ * keeps failing with the bad address.
+ */
+
+#include <ts/ts.h>
+
+#include <arpa/inet.h>
+#include <netinet/in.h>
+#include <sys/socket.h>
+
+#include <cstdint>
+#include <cstdlib>
+
+namespace
+{
+
+DbgCtl dbg_ctl{"test_TSHttpTxnServerAddrSet_retry"};
+
+// Transaction argument index for per-transaction call count
+int g_txn_arg_idx = -1;
+
+/** Get the OS_DNS call count for this transaction */
+int
+get_txn_call_count(TSHttpTxn txnp)
+{
+  return static_cast<int>(reinterpret_cast<intptr_t>(TSUserArgGet(txnp, g_txn_arg_idx)));
+}
+
+/** Set the OS_DNS call count for this transaction */
+void
+set_txn_call_count(TSHttpTxn txnp, int count)
+{
+  TSUserArgSet(txnp, g_txn_arg_idx, reinterpret_cast<void *>(static_cast<intptr_t>(count)));
+}
+
+/** Handler for OS_DNS hook */
+int
+handle_os_dns(TSCont /* cont */, TSEvent event, void *edata)
+{
+  if (event != TS_EVENT_HTTP_OS_DNS) {
+    TSError("[TSHttpTxnServerAddrSet_retry] Unexpected event in OS_DNS handler: %d", event);
+    return TS_ERROR;
+  }
+
+  TSHttpTxn txnp = (TSHttpTxn)edata;
+
+  // Increment per-transaction call count
+  int call_count = get_txn_call_count(txnp) + 1;
+  set_txn_call_count(txnp, call_count);
+
+  Dbg(dbg_ctl, "OS_DNS hook called, count=%d", call_count);
+  TSError("[TSHttpTxnServerAddrSet_retry] OS_DNS hook called, count=%d", call_count);
+
+  // First call: set a bad address (192.0.2.1 is TEST-NET-1, non-routable)
+  // Second call: set a good address (localhost)
+  const char *addr_str;
+  int         port;
+
+  if (call_count == 1) {
+    addr_str = "192.0.2.1"; // Non-routable - will fail
+    port     = 80;
+    TSError("[TSHttpTxnServerAddrSet_retry] Attempt 1: Setting BAD address %s:%d (will fail)", addr_str, port);
+  } else {
+    addr_str = "127.0.0.1"; // Localhost - should work
+    port     = 8080;
+    TSError("[TSHttpTxnServerAddrSet_retry] Attempt %d: Setting GOOD address %s:%d (should work)", call_count, addr_str, port);
+  }
+
+  // Create sockaddr
+  struct sockaddr_in sa;
+  sa.sin_family = AF_INET;
+  sa.sin_port   = htons(port);
+  if (inet_pton(AF_INET, addr_str, &sa.sin_addr) != 1) {
+    TSError("[TSHttpTxnServerAddrSet_retry] Invalid address %s", addr_str);
+    TSHttpTxnReenable(txnp, TS_EVENT_HTTP_ERROR);
+    return TS_ERROR;
+  }
+
+  // Set the server address
+  if (TSHttpTxnServerAddrSet(txnp, reinterpret_cast<struct sockaddr const *>(&sa)) != TS_SUCCESS) {
+    TSError("[TSHttpTxnServerAddrSet_retry] Failed to set server address to %s:%d", addr_str, port);
+    TSHttpTxnReenable(txnp, TS_EVENT_HTTP_ERROR);
+    return TS_ERROR;
+  }
+
+  Dbg(dbg_ctl, "Set server address to %s:%d", addr_str, port);
+
+  TSHttpTxnReenable(txnp, TS_EVENT_HTTP_CONTINUE);
+  return TS_SUCCESS;
+}
+
+/** Handler for TXN_CLOSE hook - report results */
+int
+handle_txn_close(TSCont /* cont */, TSEvent event, void *edata)
+{
+  if (event != TS_EVENT_HTTP_TXN_CLOSE) {
+    TSError("[TSHttpTxnServerAddrSet_retry] Unexpected event in TXN_CLOSE handler: %d", event);
+    return TS_ERROR;
+  }
+
+  TSHttpTxn txnp = (TSHttpTxn)edata;
+
+  // Get the per-transaction call count
+  int call_count = get_txn_call_count(txnp);
+
+  TSError("[TSHttpTxnServerAddrSet_retry] Transaction closing. OS_DNS was called %d time(s)", call_count);
+
+  if (call_count == 1) {
+    TSError("[TSHttpTxnServerAddrSet_retry] *** BUG CONFIRMED: OS_DNS hook was only called ONCE. "
+            "Plugin could not retry with different address. This is issue #12611. ***");
+  } else if (call_count >= 2) {
+    TSError("[TSHttpTxnServerAddrSet_retry] SUCCESS: OS_DNS hook was called %d times. "
+            "Plugin was able to retry with different address.",
+            call_count);
+  }
+
+  TSHttpTxnReenable(txnp, TS_EVENT_HTTP_CONTINUE);
+  return TS_SUCCESS;
+}
+
+} // anonymous namespace
+
+void
+TSPluginInit(int /* argc */, char const * /* argv */[])
+{
+  Dbg(dbg_ctl, "Initializing plugin to reproduce issue #12611");
+  TSError("[TSHttpTxnServerAddrSet_retry] Plugin initialized - will test TSHttpTxnServerAddrSet retry behavior");
+
+  TSPluginRegistrationInfo info;
+  info.plugin_name   = "test_TSHttpTxnServerAddrSet_retry";
+  info.vendor_name   = "Apache Software Foundation";
+  info.support_email = "dev@trafficserver.apache.org";
+  TSReleaseAssert(TSPluginRegister(&info) == TS_SUCCESS);
+
+  // Reserve a transaction argument slot for per-transaction call count
+  TSReleaseAssert(TSUserArgIndexReserve(TS_USER_ARGS_TXN, "test_TSHttpTxnServerAddrSet_retry", "OS_DNS call count",
+                                        &g_txn_arg_idx) == TS_SUCCESS);
+
+  TSCont os_dns_cont = TSContCreate(handle_os_dns, nullptr);
+  TSHttpHookAdd(TS_HTTP_OS_DNS_HOOK, os_dns_cont);
+
+  TSCont close_cont = TSContCreate(handle_txn_close, nullptr);
+  TSHttpHookAdd(TS_HTTP_TXN_CLOSE_HOOK, close_cont);
+}

tests/gold_tests/pluginTest/tsapi/test_TSHttpTxnServerAddrSet_retry.test.py

@@ -0,0 +1,87 @@
+#  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.
+'''
+Test to reproduce issue #12611 - retry fails when TSHttpTxnServerAddrSet is used
+'''
+
+import os
+from ports import get_port
+
+Test.Summary = '''
+Reproduce issue #12611: OS_DNS hook is not called again on retry when using TSHttpTxnServerAddrSet
+'''
+
+plugin_name = "test_TSHttpTxnServerAddrSet_retry"
+
+
+class TestIssue12611:
+    """Reproduce issue #12611: retry fails when TSHttpTxnServerAddrSet is used."""
+
+    def _configure_dns(self, tr: 'TestRun') -> 'Process':
+        """Configure the DNS process for a TestRun."""
+        self._dns = tr.MakeDNServer("dns", default=['127.0.0.1'])
+        return self._dns
+
+    def _configure_traffic_server(self, tr: 'TestRun') -> 'Process':
+        """Configure the Traffic Server process for a TestRun."""
+        self._ts = tr.MakeATSProcess("ts", enable_cache=False)
+        ts = self._ts
+
+        plugin_path = os.path.join(Test.Variables.AtsBuildGoldTestsDir, 'pluginTest', 'tsapi', '.libs', f'{plugin_name}.so')
+        ts.Setup.Copy(plugin_path, ts.Env['PROXY_CONFIG_PLUGIN_PLUGIN_DIR'])
+        ts.Disk.plugin_config.AddLine(f"{plugin_path}")
+
+        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|test_TSHttpTxnServerAddrSet_retry',
+                # Enable retries so we can see if OS_DNS is called multiple times
+                'proxy.config.http.connect_attempts_max_retries': 3,
+                'proxy.config.http.connect_attempts_timeout': 1,
+            })
+
+        # Remap to a nonexisting server - the plugin will set addresses
+        bogus_port = get_port(ts, "bogus_port")
+        ts.Disk.remap_config.AddLine(f'map / http://non.existent.server.com:{bogus_port}')
+
+    def run(self):
+        """Configure the TestRun."""
+        tr = Test.AddTestRun("Reproduce issue #12611")
+        self._configure_dns(tr)
+        self._configure_traffic_server(tr)
+
+        # Make a simple request - it should fail since first address is non-routable
+        # but the plugin should log whether OS_DNS was called multiple times
+        tr.Processes.Default.Command = f'curl -vs --connect-timeout 5 http://127.0.0.1:{self._ts.Variables.port}/ -o /dev/null 2>&1; true'
+        tr.Processes.Default.ReturnCode = 0
+
+        tr.Processes.Default.StartBefore(self._dns)
+        tr.Processes.Default.StartBefore(self._ts)
+
+        # Override the default diags.log error check - we use TSError() for test output
+        # Using '=' instead of '+=' replaces the default "no errors" check
+        self._ts.Disk.diags_log.Content = Testers.ContainsExpression("OS_DNS hook called, count=1", "First OS_DNS call logged")
+
+        # This message indicates the fix works - OS_DNS was called multiple times
+        # Test will FAIL on master (bug exists), PASS after fix is applied
+        self._ts.Disk.diags_log.Content += Testers.ContainsExpression(
+            "SUCCESS: OS_DNS hook was called", "Plugin was able to retry with different address")
+
+
+test = TestIssue12611()
+test.run()