TS-4399 TS-4400 Management API messes up proxy options

cmd/traffic_manager/traffic_manager.cc

@@ -27,6 +27,7 @@
 #include "ts/ink_sock.h"
 #include "ts/ink_args.h"
 #include "ts/ink_syslog.h"
+#include <vector>
 
 #include "WebMgmtUtils.h"
 #include "WebOverview.h"
@@ -445,6 +446,8 @@ main(int argc, const char **argv)
   int binding_version      = 0;
   BindingInstance *binding = NULL;
 
+  std::vector<char*> callback_proxy_options;
+
   ArgumentDescription argument_descriptions[] = {
     {"proxyOff", '-', "Disable proxy", "F", &proxy_off, NULL, NULL},
     {"aconfPort", '-', "Autoconf port", "I", &aconf_port_arg, "MGMT_ACONF_PORT", NULL},
@@ -611,31 +614,35 @@ main(int argc, const char **argv)
 
   /* Update cmd line overrides/environmental overrides/etc */
   if (tsArgs) { /* Passed command line args for proxy */
+    char* new_proxy_opts = new char[strlen(lmgmt->proxy_options) + strlen(tsArgs) + 1];
+    strcpy(new_proxy_opts, lmgmt->proxy_options);
+    strcat(new_proxy_opts, tsArgs);
     ats_free(lmgmt->proxy_options);
-    lmgmt->proxy_options = tsArgs;
-    mgmt_log("[main] Traffic Server Args: '%s'\n", lmgmt->proxy_options);
+    lmgmt->proxy_options = new_proxy_opts;
   }
 
-  // we must pass in bind_stdout and bind_stderr values to TS
-  // we do it so TS is able to create BaseLogFiles for each value
+  // TS needs to be started up with the same outputlog bindings each time,
+  // so we append the outputlog location to the persistent proxy options
+  //
+  // TS needs them to be able to create BaseLogFiles for each value
   if (*bind_stdout != 0) {
-    size_t l = strlen(lmgmt->proxy_options);
-    size_t n = 3                            /* " --" */
-               + sizeof(TM_OPT_BIND_STDOUT) /* nul accounted for here */
-               + 1                          /* space */
-               + strlen(bind_stdout);
-    lmgmt->proxy_options = static_cast<char *>(ats_realloc(lmgmt->proxy_options, n + l));
-    snprintf(lmgmt->proxy_options + l, n, " --%s %s", TM_OPT_BIND_STDOUT, bind_stdout);
+    const char *bind_stdout_opt = " --" TM_OPT_BIND_STDOUT " ";
+    char *new_proxy_opts = new char[strlen(lmgmt->proxy_options) + strlen(bind_stdout_opt) + strlen(bind_stdout) + 1];
+    strcpy(new_proxy_opts, lmgmt->proxy_options);
+    strcat(new_proxy_opts, bind_stdout_opt);
+    strcat(new_proxy_opts, bind_stdout);
+    delete lmgmt->proxy_options;
+    lmgmt->proxy_options = new_proxy_opts;
   }
 
   if (*bind_stderr != 0) {
-    size_t l = strlen(lmgmt->proxy_options);
-    size_t n = 3                            /* space dash dash */
-               + sizeof(TM_OPT_BIND_STDERR) /* nul accounted for here */
-               + 1                          /* space */
-               + strlen(bind_stderr);
-    lmgmt->proxy_options = static_cast<char *>(ats_realloc(lmgmt->proxy_options, n + l));
-    snprintf(lmgmt->proxy_options + l, n, " --%s %s", TM_OPT_BIND_STDERR, bind_stderr);
+    const char *bind_stderr_opt = " --" TM_OPT_BIND_STDERR " ";
+    char *new_proxy_opts = new char[strlen(lmgmt->proxy_options) + strlen(bind_stderr_opt) + strlen(bind_stderr) + 1];
+    strcpy(new_proxy_opts, lmgmt->proxy_options);
+    strcat(new_proxy_opts, bind_stderr_opt);
+    strcat(new_proxy_opts, bind_stderr);
+    delete lmgmt->proxy_options;
+    lmgmt->proxy_options = new_proxy_opts;
   }
 
   if (proxy_port) {
@@ -813,10 +820,11 @@ main(int argc, const char **argv)
       } else {
         sleep_time = 1;
       }
-      if (lmgmt->startProxy()) {
+      if (ProxyStateSet(TS_PROXY_ON, TS_CACHE_CLEAR_NONE) == TS_ERR_OKAY) {
         just_started = 0;
         sleep_time   = 0;
       } else {
+        mgmt_log("in ProxyStateSet else branch");
         just_started++;
       }
     } else { /* Give the proxy a chance to fire up */
@@ -844,6 +852,11 @@ main(int argc, const char **argv)
     }
   }
 
+  // Delete the proxy options we saved for CoreAPI.
+  // This probably isn't needed b/c the process dies here...
+  for (auto it = callback_proxy_options.begin(); it < callback_proxy_options.end(); ++it)
+    free(*it);
+
   if (binding) {
     metrics_binding_destroy(*binding);
     delete binding;

mgmt/LocalManager.cc

@@ -239,8 +239,9 @@ LocalManager::LocalManager(bool proxy_on) : BaseManager(), run_proxy(proxy_on),
   process_server_timeout_msecs = REC_readInteger("proxy.config.lm.pserver_timeout_msecs", &found);
   proxy_name                   = REC_readString("proxy.config.proxy_name", &found);
   proxy_binary                 = REC_readString("proxy.config.proxy_binary", &found);
-  proxy_options                = REC_readString("proxy.config.proxy_binary_opts", &found);
   env_prep                     = REC_readString("proxy.config.env_prep", &found);
+  proxy_options                = new char[1];
+  strcpy(proxy_options, "");  // so later we can always `delete proxy_options` without worrying
 
   // Calculate proxy_binary from the absolute bin_path
   absolute_proxy_binary = Layout::relative_to(bindir, proxy_binary);
@@ -831,9 +832,13 @@ LocalManager::processEventQueue()
 /*
  * startProxy()
  *   Function fires up a proxy process.
+ *
+ * Args:
+ *   onetime_options: one time options that traffic_server should be started with (ie
+ *                    these options do not persist across reboots)
  */
 bool
-LocalManager::startProxy()
+LocalManager::startProxy(char *onetime_options)
 {
   if (proxy_launch_outstanding) {
     return false;
@@ -902,8 +907,11 @@ LocalManager::startProxy()
     Vec<char> real_proxy_options;
 
     real_proxy_options.append(proxy_options, strlen(proxy_options));
+    real_proxy_options.append(" ", strlen(" "));
+    real_proxy_options.append(onetime_options, strlen(onetime_options));
 
-    if (!strstr(proxy_options, MGMT_OPT)) { // Make sure we're starting the proxy in mgmt mode
+    // Make sure we're starting the proxy in mgmt mode
+    if (!strstr(proxy_options, MGMT_OPT) && !strstr(onetime_options, MGMT_OPT)) {
       real_proxy_options.append(" ", strlen(" "));
       real_proxy_options.append(MGMT_OPT, sizeof(MGMT_OPT) - 1);
     }

mgmt/LocalManager.h

@@ -74,7 +74,7 @@ class LocalManager : public BaseManager
   void signalAlarm(int alarm_id, const char *desc = NULL, const char *ip = NULL);
 
   void processEventQueue();
-  bool startProxy();
+  bool startProxy(char *onetime_options);
   void listenForProxy();
   void bindProxyPort(HttpProxyPort &);
   void closeProxyPorts();
@@ -108,7 +108,7 @@ class LocalManager : public BaseManager
   char *absolute_proxy_binary;
   char *proxy_name;
   char *proxy_binary;
-  char *proxy_options;
+  char *proxy_options;  // These options should persist across proxy reboots
   char *env_prep;
 
   int process_server_sockfd;

mgmt/api/CoreAPI.cc

@@ -41,6 +41,7 @@
 #include "ts/Diags.h"
 #include "ts/ink_hash_table.h"
 #include "ExpandingArray.h"
+#include <vector>
 //#include "I_AccCrypto.h"
 
 #include "CoreAPI.h"
@@ -53,6 +54,9 @@
 // global variable
 CallbackTable *local_event_callbacks;
 
+// Used to get any proxy options that traffic_manager might want to tack on
+std::function<void(std::vector<char*>&)> proxy_options_callback;
+
 extern FileManager *configFiles; // global in traffic_manager
 
 /*-------------------------------------------------------------------------
@@ -184,14 +188,11 @@ ProxyStateSet(TSProxyStateT state, TSCacheClearT clear)
       ink_strlcat(tsArgs, " -k", sizeof(tsArgs));
     }
 
-    if (strlen(tsArgs) > 0) { /* Passed command line args for proxy */
-      ats_free(lmgmt->proxy_options);
-      lmgmt->proxy_options = ats_strdup(tsArgs);
-      mgmt_log("[ProxyStateSet] Traffic Server Args: '%s'\n", lmgmt->proxy_options);
-    }
+    mgmt_log("[ProxyStateSet] Traffic Server Args: '%s %s'\n", lmgmt->proxy_options ? lmgmt->proxy_options : "", tsArgs);
 
     lmgmt->run_proxy = true;
     lmgmt->listenForProxy();
+    lmgmt->startProxy(tsArgs);
 
     do {
       mgmt_sleep_sec(1);