From 3d9d0c0888463b5f161da4465ebb84e3bada7b22 Mon Sep 17 00:00:00 2001
From: Henna Huang <henna@google.com>
Date: Tue, 18 Apr 2017 14:46:42 -0400
Subject: [PATCH 1/9] Verify socket passing in hot restart test

---
 docs/operations/cli.rst                    |  4 ++
 include/envoy/server/instance.h            |  5 ++
 include/envoy/server/options.h             |  5 ++
 source/common/json/json_loader.cc          | 13 ++++
 source/common/json/json_loader.h           |  2 +
 source/server/http/admin.cc                | 15 +++-
 source/server/http/admin.h                 |  1 +
 source/server/options_impl.cc              |  3 +
 source/server/options_impl.h               |  2 +
 source/server/server.cc                    |  8 +++
 source/server/server.h                     |  1 +
 test/integration/BUILD                     |  1 +
 test/integration/hotrestart_test.sh        | 22 +++++-
 test/integration/integration_admin_test.cc |  5 ++
 test/integration/server.h                  |  5 +-
 test/mocks/server/mocks.cc                 |  6 +-
 test/mocks/server/mocks.h                  |  5 +-
 test/server/options_impl_test.cc           |  3 +-
 tools/BUILD                                |  1 +
 tools/socket_passing.py                    | 83 ++++++++++++++++++++++
 20 files changed, 181 insertions(+), 9 deletions(-)
 create mode 100755 tools/socket_passing.py

diff --git a/docs/operations/cli.rst b/docs/operations/cli.rst
index 9dc1f8f6c2e43..08a5c6eed2b49 100644
--- a/docs/operations/cli.rst
+++ b/docs/operations/cli.rst
@@ -10,6 +10,10 @@ following are the command line options that Envoy supports.
 
   *(required)* The path to the :ref:`JSON configuration file <config>`.
 
+.. option:: -a <path string>, --admin-address-path <path string>
+
+  *(optional)* The output file path where the admin address will be written.
+
 .. option:: --base-id <integer>
 
   *(optional)* The base ID to use when allocating shared memory regions. Envoy uses shared memory
diff --git a/include/envoy/server/instance.h b/include/envoy/server/instance.h
index 92691a62111a6..a0f24fd5d74c1 100644
--- a/include/envoy/server/instance.h
+++ b/include/envoy/server/instance.h
@@ -183,6 +183,11 @@ class Instance {
    * @return information about the local environment the server is running in.
    */
   virtual const LocalInfo::LocalInfo& localInfo() PURE;
+
+  /**
+   * @return int the total number of listeners
+   */
+  virtual int numListeners() PURE;
 };
 
 } // Server
diff --git a/include/envoy/server/options.h b/include/envoy/server/options.h
index 11f0631273989..317d2da0c8c22 100644
--- a/include/envoy/server/options.h
+++ b/include/envoy/server/options.h
@@ -34,6 +34,11 @@ class Options {
    */
   virtual const std::string& configPath() PURE;
 
+  /**
+   * @return const std::string& the admin address output file.
+   */
+  virtual const std::string& adminAddressPath() PURE;
+
   /**
    * @return spdlog::level::level_enum the default log level for the server.
    */
diff --git a/source/common/json/json_loader.cc b/source/common/json/json_loader.cc
index b4f173d4f527d..4a325bdb56ecc 100644
--- a/source/common/json/json_loader.cc
+++ b/source/common/json/json_loader.cc
@@ -232,4 +232,17 @@ ObjectPtr Factory::LoadFromString(const std::string& json) {
   return ObjectPtr{new ObjectImplRoot(std::move(document))};
 }
 
+const std::string Factory::listAsJsonString(const std::list<std::string>& items) {
+  rapidjson::StringBuffer writer_string_buffer;
+  rapidjson::Writer<rapidjson::StringBuffer> writer(writer_string_buffer);
+
+  writer.StartArray();
+  for (const std::string& item : items) {
+    writer.String(item.c_str());
+  }
+  writer.EndArray();
+
+  return writer_string_buffer.GetString();
+}
+
 } // Json
diff --git a/source/common/json/json_loader.h b/source/common/json/json_loader.h
index 5e905d8b41822..66e16bb6720ea 100644
--- a/source/common/json/json_loader.h
+++ b/source/common/json/json_loader.h
@@ -15,6 +15,8 @@ class Factory {
    * Constructs a Json Object from a String.
    */
   static ObjectPtr LoadFromString(const std::string& json);
+
+  static const std::string listAsJsonString(const std::list<std::string>& items);
 };
 
 } // Json
diff --git a/source/server/http/admin.cc b/source/server/http/admin.cc
index d4d063aa87c28..0472ed09bd75f 100644
--- a/source/server/http/admin.cc
+++ b/source/server/http/admin.cc
@@ -19,6 +19,7 @@
 #include "common/http/header_map_impl.h"
 #include "common/http/headers.h"
 #include "common/http/http1/codec_impl.h"
+#include "common/json/json_loader.h"
 #include "common/network/listen_socket_impl.h"
 #include "common/profiler/profiler.h"
 #include "common/router/config_impl.h"
@@ -278,6 +279,17 @@ Http::Code AdminImpl::handlerQuitQuitQuit(const std::string&, Buffer::Instance&
   return Http::Code::OK;
 }
 
+Http::Code AdminImpl::handlerListenerAddresses(const std::string&, Buffer::Instance& response) {
+  std::list<std::string> listeners;
+
+  for (int i = 0; i < server_.numListeners(); ++i) {
+    listeners.push_back(server_.getListenSocketByIndex(i)->localAddress()->asString());
+  }
+  response.add(fmt::format("{}", Json::Factory::listAsJsonString(listeners)));
+
+  return Http::Code::OK;
+}
+
 Http::Code AdminImpl::handlerCerts(const std::string&, Buffer::Instance& response) {
   // This set is used to track distinct certificates. We may have multiple listeners, upstreams, etc
   // using the same cert.
@@ -337,7 +349,8 @@ AdminImpl::AdminImpl(const std::string& access_log_path, const std::string& prof
           {"/reset_counters", "reset all counters to zero", MAKE_HANDLER(handlerResetCounters)},
           {"/server_info", "print server version/status information",
            MAKE_HANDLER(handlerServerInfo)},
-          {"/stats", "print server stats", MAKE_HANDLER(handlerStats)}} {
+          {"/stats", "print server stats", MAKE_HANDLER(handlerStats)},
+          {"/listeners", "print listener addresses", MAKE_HANDLER(handlerListenerAddresses)}} {
 
   access_logs_.emplace_back(new Http::AccessLog::InstanceImpl(
       access_log_path, {}, Http::AccessLog::AccessLogFormatUtils::defaultAccessLogFormatter(),
diff --git a/source/server/http/admin.h b/source/server/http/admin.h
index d83713c4b13a7..a553fc2b003ed 100644
--- a/source/server/http/admin.h
+++ b/source/server/http/admin.h
@@ -114,6 +114,7 @@ class AdminImpl : public Admin,
   Http::Code handlerServerInfo(const std::string& url, Buffer::Instance& response);
   Http::Code handlerStats(const std::string& url, Buffer::Instance& response);
   Http::Code handlerQuitQuitQuit(const std::string& url, Buffer::Instance& response);
+  Http::Code handlerListenerAddresses(const std::string& url, Buffer::Instance& response);
 
   Server::Instance& server_;
   std::list<Http::AccessLog::InstanceSharedPtr> access_logs_;
diff --git a/source/server/options_impl.cc b/source/server/options_impl.cc
index 696200d5e8124..2ebed240717e5 100644
--- a/source/server/options_impl.cc
+++ b/source/server/options_impl.cc
@@ -23,6 +23,8 @@ OptionsImpl::OptionsImpl(int argc, char** argv, const std::string& hot_restart_v
                                         std::thread::hardware_concurrency(), "uint32_t", cmd);
   TCLAP::ValueArg<std::string> config_path("c", "config-path", "Path to configuration file", false,
                                            "", "string", cmd);
+  TCLAP::ValueArg<std::string> admin_address_path("a", "admin-address-path", "Admin address path",
+                                                  false, "", "string", cmd);
   TCLAP::ValueArg<std::string> log_level("l", "log-level", log_levels_string, false,
                                          spdlog::level::level_names[default_log_level], "string",
                                          cmd);
@@ -68,6 +70,7 @@ OptionsImpl::OptionsImpl(int argc, char** argv, const std::string& hot_restart_v
   base_id_ = base_id.getValue() * 10;
   concurrency_ = concurrency.getValue();
   config_path_ = config_path.getValue();
+  admin_address_path_ = admin_address_path.getValue();
   restart_epoch_ = restart_epoch.getValue();
   service_cluster_ = service_cluster.getValue();
   service_node_ = service_node.getValue();
diff --git a/source/server/options_impl.h b/source/server/options_impl.h
index 0832ce4093692..3c9d9f1f10421 100644
--- a/source/server/options_impl.h
+++ b/source/server/options_impl.h
@@ -18,6 +18,7 @@ class OptionsImpl : public Server::Options {
   uint64_t baseId() override { return base_id_; }
   uint32_t concurrency() override { return concurrency_; }
   const std::string& configPath() override { return config_path_; }
+  const std::string& adminAddressPath() override { return admin_address_path_; }
   std::chrono::seconds drainTime() override { return drain_time_; }
   spdlog::level::level_enum logLevel() override { return log_level_; }
   std::chrono::seconds parentShutdownTime() override { return parent_shutdown_time_; }
@@ -28,6 +29,7 @@ class OptionsImpl : public Server::Options {
   uint64_t base_id_;
   uint32_t concurrency_;
   std::string config_path_;
+  std::string admin_address_path_;
   spdlog::level::level_enum log_level_;
   uint64_t restart_epoch_;
   std::string service_cluster_;
diff --git a/source/server/server.cc b/source/server/server.cc
index 187b5089fa730..8ba4be057c423 100644
--- a/source/server/server.cc
+++ b/source/server/server.cc
@@ -184,6 +184,14 @@ void InstanceImpl::initialize(Options& options, TestHooks& hooks,
   admin_.reset(new AdminImpl(initial_config.admin().accessLogPath(),
                              initial_config.admin().profilePath(), initial_config.admin().address(),
                              *this));
+
+  if (options.adminAddressPath().length() > 0) {
+    std::ofstream admin_address_file(options.adminAddressPath());
+    ASSERT(admin_address_file.is_open());
+    admin_address_file << admin_->mutable_socket().localAddress()->asString();
+    admin_address_file.close();
+  }
+
   admin_scope_ = stats_store_.createScope("listener.admin.");
   handler_.addListener(*admin_, admin_->mutable_socket(), *admin_scope_,
                        Network::ListenerOptions::listenerOptionsWithBindToPort());
diff --git a/source/server/server.h b/source/server/server.h
index 6f2d06bba116f..6f045854ef316 100644
--- a/source/server/server.h
+++ b/source/server/server.h
@@ -132,6 +132,7 @@ class InstanceImpl : Logger::Loggable<Logger::Id::main>, public Instance {
   Tracing::HttpTracer& httpTracer() override;
   ThreadLocal::Instance& threadLocal() override { return thread_local_; }
   const LocalInfo::LocalInfo& localInfo() override { return local_info_; }
+  int numListeners() override { return config_->listeners().size(); }
 
 private:
   void flushStats();
diff --git a/test/integration/BUILD b/test/integration/BUILD
index 87ffcce1bd288..c9a7f0c627788 100644
--- a/test/integration/BUILD
+++ b/test/integration/BUILD
@@ -10,6 +10,7 @@ envoy_sh_test(
         "//test/common/runtime:filesystem_setup.sh",
         "//test/common/runtime:filesystem_test_data",
         "//test/config/integration:server_config_files",
+        "//tools:socket_passing.py",
     ],
     # Need to disable test execution sandboxing for this test, since it uses shmem.
     # TODO(htuch): When most folks are using a Bazel with the recent
diff --git a/test/integration/hotrestart_test.sh b/test/integration/hotrestart_test.sh
index 2ae3282f55522..5c2a61fa23c7b 100755
--- a/test/integration/hotrestart_test.sh
+++ b/test/integration/hotrestart_test.sh
@@ -14,12 +14,18 @@ cat test/config/integration/server.json |
 
 # Now start the real server, hot restart it twice, and shut it all down as a basic hot restart
 # sanity test.
+ADMIN_ADDRESS_PATH_0="${TEST_TMPDIR}"/admin_address_file_0.pid
 echo "Starting epoch 0"
 "${ENVOY_BIN}" -c "${HOT_RESTART_JSON}" \
-    --restart-epoch 0 --base-id 1 --service-cluster cluster --service-node node &
+    --restart-epoch 0 --base-id 1 --service-cluster cluster --service-node node \
+    -a "${ADMIN_ADDRESS_PATH_0}" &
 
 FIRST_SERVER_PID=$!
 sleep 3
+
+echo "Replacing listener addresses"
+"tools/socket_passing.py" "-o" "${HOT_RESTART_JSON}" "-a" "${ADMIN_ADDRESS_PATH_0}"
+
 # Send SIGUSR1 signal to the first server, this should not kill it. Also send SIGHUP which should
 # get eaten.
 echo "Sending SIGUSR1/SIGHUP to first server"
@@ -27,21 +33,31 @@ kill -SIGUSR1 ${FIRST_SERVER_PID}
 kill -SIGHUP ${FIRST_SERVER_PID}
 sleep 3
 
+ADMIN_ADDRESS_PATH_1="${TEST_TMPDIR}"/admin_address_file_1.pid
 echo "Starting epoch 1"
 "${ENVOY_BIN}" -c "${HOT_RESTART_JSON}" \
-    --restart-epoch 1 --base-id 1 --service-cluster cluster --service-node node &
+    --restart-epoch 1 --base-id 1 --service-cluster cluster --service-node node \
+    -a "${ADMIN_ADDRESS_PATH_1}" &
 
 SECOND_SERVER_PID=$!
 # Wait for stat flushing
 sleep 7
 
+echo "Checking that listener addresses have not changed"
+"tools/socket_passing.py" "-o" "${HOT_RESTART_JSON}" "-a" "${ADMIN_ADDRESS_PATH_1}" "-n"
+
+ADMIN_ADDRESS_PATH_2="${TEST_TMPDIR}"/admin_address_file_2.pid
 echo "Starting epoch 2"
 "${ENVOY_BIN}" -c "${HOT_RESTART_JSON}" \
-    --restart-epoch 2 --base-id 1 --service-cluster cluster --service-node node &
+    --restart-epoch 2 --base-id 1 --service-cluster cluster --service-node node \
+    -a "${ADMIN_ADDRESS_PATH_2}" &
 
 THIRD_SERVER_PID=$!
 sleep 3
 
+echo "Checking that listener addresses have not changed"
+"tools/socket_passing.py" "-o" "${HOT_RESTART_JSON}" "-a" "${ADMIN_ADDRESS_PATH_2}" "-n"
+
 # First server should already be gone.
 echo "Waiting for epoch 0"
 wait ${FIRST_SERVER_PID}
diff --git a/test/integration/integration_admin_test.cc b/test/integration/integration_admin_test.cc
index bf1ce5719d1a7..7fc34f2092368 100644
--- a/test/integration/integration_admin_test.cc
+++ b/test/integration/integration_admin_test.cc
@@ -119,6 +119,11 @@ TEST_F(IntegrationTest, Admin) {
                                                 Http::CodecClient::Type::HTTP1);
   EXPECT_TRUE(response->complete());
   EXPECT_STREQ("200", response->headers().Status()->value().c_str());
+
+  response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/listeners", "",
+                                                Http::CodecClient::Type::HTTP1);
+  EXPECT_TRUE(response->complete());
+  EXPECT_STREQ("200", response->headers().Status()->value().c_str());
 }
 
 // Successful call to startProfiler requires tcmalloc.
diff --git a/test/integration/server.h b/test/integration/server.h
index d256f002fe681..cc8ef8b2e1a62 100644
--- a/test/integration/server.h
+++ b/test/integration/server.h
@@ -19,12 +19,14 @@ namespace Server {
  */
 class TestOptionsImpl : public Options {
 public:
-  TestOptionsImpl(const std::string& config_path) : config_path_(config_path) {}
+  TestOptionsImpl(const std::string& config_path)
+      : config_path_(config_path), admin_address_path_("") {}
 
   // Server::Options
   uint64_t baseId() override { return 0; }
   uint32_t concurrency() override { return 1; }
   const std::string& configPath() override { return config_path_; }
+  const std::string& adminAddressPath() override { return admin_address_path_; }
   std::chrono::seconds drainTime() override { return std::chrono::seconds(0); }
   spdlog::level::level_enum logLevel() override { NOT_IMPLEMENTED; }
   std::chrono::seconds parentShutdownTime() override { return std::chrono::seconds(0); }
@@ -35,6 +37,7 @@ class TestOptionsImpl : public Options {
 
 private:
   const std::string config_path_;
+  const std::string admin_address_path_;
 };
 
 class TestDrainManager : public DrainManager {
diff --git a/test/mocks/server/mocks.cc b/test/mocks/server/mocks.cc
index 7d604dcc80afe..5482d696ba193 100644
--- a/test/mocks/server/mocks.cc
+++ b/test/mocks/server/mocks.cc
@@ -8,8 +8,10 @@ using testing::SaveArg;
 
 namespace Server {
 
-MockOptions::MockOptions(const std::string& path) : path_(path) {
-  ON_CALL(*this, configPath()).WillByDefault(ReturnRef(path_));
+MockOptions::MockOptions(const std::string& config_path)
+    : config_path_(config_path), admin_address_path_("") {
+  ON_CALL(*this, configPath()).WillByDefault(ReturnRef(config_path_));
+  ON_CALL(*this, adminAddressPath()).WillByDefault(ReturnRef(admin_address_path_));
 }
 MockOptions::~MockOptions() {}
 
diff --git a/test/mocks/server/mocks.h b/test/mocks/server/mocks.h
index 1d2840b30a4f0..b1f1c81f0d0ba 100644
--- a/test/mocks/server/mocks.h
+++ b/test/mocks/server/mocks.h
@@ -36,13 +36,15 @@ class MockOptions : public Options {
   MOCK_METHOD0(baseId, uint64_t());
   MOCK_METHOD0(concurrency, uint32_t());
   MOCK_METHOD0(configPath, const std::string&());
+  MOCK_METHOD0(adminAddressPath, const std::string&());
   MOCK_METHOD0(drainTime, std::chrono::seconds());
   MOCK_METHOD0(logLevel, spdlog::level::level_enum());
   MOCK_METHOD0(parentShutdownTime, std::chrono::seconds());
   MOCK_METHOD0(restartEpoch, uint64_t());
   MOCK_METHOD0(fileFlushIntervalMsec, std::chrono::milliseconds());
 
-  std::string path_;
+  std::string config_path_;
+  std::string admin_address_path_;
 };
 
 class MockAdmin : public Admin {
@@ -123,6 +125,7 @@ class MockInstance : public Instance {
   MOCK_METHOD0(httpTracer, Tracing::HttpTracer&());
   MOCK_METHOD0(threadLocal, ThreadLocal::Instance&());
   MOCK_METHOD0(localInfo, const LocalInfo::LocalInfo&());
+  MOCK_METHOD0(numListeners, int());
 
   testing::NiceMock<ThreadLocal::MockInstance> thread_local_;
   Stats::IsolatedStoreImpl stats_store_;
diff --git a/test/server/options_impl_test.cc b/test/server/options_impl_test.cc
index 2d7ad481fa138..67f1c3286bdcc 100644
--- a/test/server/options_impl_test.cc
+++ b/test/server/options_impl_test.cc
@@ -20,11 +20,12 @@ TEST(OptionsImplDeathTest, HotRestartVersion) {
 
 TEST(OptionsImplTest, All) {
   std::unique_ptr<OptionsImpl> options = createOptionsImpl(
-      "envoy --concurrency 2 -c hello --restart-epoch 1 -l info "
+      "envoy --concurrency 2 -c hello -a path --restart-epoch 1 -l info "
       "--service-cluster cluster --service-node node --service-zone zone "
       "--file-flush-interval-msec 9000 --drain-time-s 60 --parent-shutdown-time-s 90");
   EXPECT_EQ(2U, options->concurrency());
   EXPECT_EQ("hello", options->configPath());
+  EXPECT_EQ("path", options->adminAddressPath());
   EXPECT_EQ(1U, options->restartEpoch());
   EXPECT_EQ(spdlog::level::info, options->logLevel());
   EXPECT_EQ("cluster", options->serviceClusterName());
diff --git a/tools/BUILD b/tools/BUILD
index 21666fb51b44f..74536baa18a2f 100644
--- a/tools/BUILD
+++ b/tools/BUILD
@@ -3,4 +3,5 @@ package(default_visibility = ["//visibility:public"])
 exports_files([
     "gen_git_sha.sh",
     "git_sha_rewriter.py",
+    "socket_passing.py",
 ])
diff --git a/tools/socket_passing.py b/tools/socket_passing.py
new file mode 100755
index 0000000000000..1db55ea087a67
--- /dev/null
+++ b/tools/socket_passing.py
@@ -0,0 +1,83 @@
+#!/usr/bin/env python
+
+# This tool is a helper script that queries the admin address for all listener
+# addresses after envoy startup. The script can then be used to update an
+# exisiting json config file with updated listener addresses. This script is
+# currently called in the hot restart integration test to update listener
+# addresses bound to port 0 in the intial json config file. With the
+# -n or -no_port_change option, this script does not update an exisiting json
+# config file but checks that the listener addresses after envoy startup
+# match the listener addresses in the initial json config file.
+
+from collections import OrderedDict
+
+import argparse
+import json
+import os.path
+import httplib
+import sys
+import time
+
+def CheckNoChange(original_listeners, updated_listeners):
+  for updated, original in zip(updated_listeners, original_listeners):
+      if original['address'] != "tcp://" + updated:
+        return False
+  return True
+
+def ReplaceListenerAddresses(original_json, admin_address, no_port_change):
+  # Get original listener addresses
+  with open(original_json, 'r') as original_json_file:
+    parsed_json = json.load(original_json_file, object_pairs_hook=OrderedDict)
+  original_listeners = parsed_json['listeners']
+
+  admin_conn = httplib.HTTPConnection(admin_address)
+  admin_conn.request("GET", "/listeners")
+  admin_response = admin_conn.getresponse()
+
+  if not admin_response.status == 200:
+    admin_conn.close()
+    return False
+
+  updated_listeners = json.loads(admin_response.read())
+  admin_conn.close()
+
+  if no_port_change:
+    return CheckNoChange(original_listeners, updated_listeners)
+  else:
+    for updated, original in zip(updated_listeners, original_listeners):
+      original['address'] = "tcp://" + updated
+    with open(original_json, 'w') as outfile:
+      json.dump(OrderedDict(parsed_json), outfile, indent=2)
+
+  return True
+
+if __name__ == '__main__':
+  parser = argparse.ArgumentParser(description='Replace listener addressses in json file.')
+  parser.add_argument('-o', '--original_json', type=str, required=True,
+                      help='Original json file')
+  parser.add_argument('-a', '--admin_address_path', type=str, required=True,
+                      help='Admin address path')
+  parser.add_argument('-n', '--no_port_change', action='store_true', default=False,
+                      help='Check that listener port addresses have not changed.');
+  args = parser.parse_args()
+  original_json = args.original_json
+  admin_address_path = args.admin_address_path
+  no_port_change = args.no_port_change
+
+  # Read admin address from file
+  counter = 0;
+  while not os.path.exists(admin_address_path):
+    time.sleep(1)
+    counter += 1
+    if counter > 20:
+      break
+
+  if not os.path.exists(admin_address_path):
+    sys.exit(1)
+
+  with open(admin_address_path, 'r') as admin_address_file:
+    admin_address = admin_address_file.read()
+
+  result = ReplaceListenerAddresses(original_json, admin_address, no_port_change)
+  if not result:
+    sys.exit(1)

From f0e57fb8c2ddfbfbe1079118bff652bce03b5288 Mon Sep 17 00:00:00 2001
From: Henna Huang <henna@google.com>
Date: Thu, 20 Apr 2017 16:50:56 -0400
Subject: [PATCH 2/9] Add std libraries

---
 source/common/json/json_loader.h | 1 +
 source/server/server.h           | 1 +
 2 files changed, 2 insertions(+)

diff --git a/source/common/json/json_loader.h b/source/common/json/json_loader.h
index d1c0541db0bb8..461371137f43c 100644
--- a/source/common/json/json_loader.h
+++ b/source/common/json/json_loader.h
@@ -1,5 +1,6 @@
 #pragma once
 
+#include <list>
 #include <string>
 
 #include "envoy/json/json_object.h"
diff --git a/source/server/server.h b/source/server/server.h
index ba19db8a23792..52b84c1961761 100644
--- a/source/server/server.h
+++ b/source/server/server.h
@@ -2,6 +2,7 @@
 
 #include <chrono>
 #include <cstdint>
+#include <fstream>
 #include <functional>
 #include <list>
 #include <memory>

From 11067f543a0928697a977b5e3ad4259b5a252e55 Mon Sep 17 00:00:00 2001
From: Henna Huang <henna@google.com>
Date: Sun, 23 Apr 2017 22:21:23 -0400
Subject: [PATCH 3/9] Response to comments

---
 docs/operations/cli.rst                    |  2 +-
 include/envoy/server/instance.h            |  5 --
 source/common/json/json_loader.h           |  1 +
 source/server/http/admin.cc                | 20 ++++++--
 source/server/http/admin.h                 |  4 +-
 source/server/server.cc                    | 11 +---
 source/server/server.h                     |  2 -
 test/common/json/json_loader_test.cc       | 16 ++++++
 test/integration/hotrestart_test.sh        | 28 ++++++----
 test/integration/integration_admin_test.cc | 11 ++++
 test/mocks/server/mocks.h                  |  1 -
 test/server/http/BUILD                     |  1 +
 test/server/http/admin_test.cc             | 41 +++++++++++++--
 tools/socket_passing.py                    | 60 +++++++++++-----------
 14 files changed, 135 insertions(+), 68 deletions(-)

diff --git a/docs/operations/cli.rst b/docs/operations/cli.rst
index 08a5c6eed2b49..044d852e2a5b5 100644
--- a/docs/operations/cli.rst
+++ b/docs/operations/cli.rst
@@ -12,7 +12,7 @@ following are the command line options that Envoy supports.
 
 .. option:: -a <path string>, --admin-address-path <path string>
 
-  *(optional)* The output file path where the admin address will be written.
+  *(optional)* The output file path where the admin address and port will be written.
 
 .. option:: --base-id <integer>
 
diff --git a/include/envoy/server/instance.h b/include/envoy/server/instance.h
index 7d5e501841576..25df0b5a68907 100644
--- a/include/envoy/server/instance.h
+++ b/include/envoy/server/instance.h
@@ -187,11 +187,6 @@ class Instance {
    * @return information about the local environment the server is running in.
    */
   virtual const LocalInfo::LocalInfo& localInfo() PURE;
-
-  /**
-   * @return int the total number of listeners
-   */
-  virtual int numListeners() PURE;
 };
 
 } // Server
diff --git a/source/common/json/json_loader.h b/source/common/json/json_loader.h
index 461371137f43c..0c812f9fb8740 100644
--- a/source/common/json/json_loader.h
+++ b/source/common/json/json_loader.h
@@ -9,6 +9,7 @@ namespace Json {
 
 class Factory {
 public:
+  // TODO(hennna): Cleanup function names - i.e. s/LoadFromFile/loadFromFile/.
   /*
    * Constructs a Json Object from a File.
    */
diff --git a/source/server/http/admin.cc b/source/server/http/admin.cc
index dce9058ad4a65..343bb16228078 100644
--- a/source/server/http/admin.cc
+++ b/source/server/http/admin.cc
@@ -287,12 +287,12 @@ Http::Code AdminImpl::handlerQuitQuitQuit(const std::string&, Buffer::Instance&
 
 Http::Code AdminImpl::handlerListenerAddresses(const std::string&, Buffer::Instance& response) {
   std::list<std::string> listeners;
-
-  for (int i = 0; i < server_.numListeners(); ++i) {
-    listeners.push_back(server_.getListenSocketByIndex(i)->localAddress()->asString());
+  int listener_index = 0;
+  while (server_.getListenSocketByIndex(listener_index) != nullptr) {
+    listeners.push_back(server_.getListenSocketByIndex(listener_index)->localAddress()->asString());
+    ++listener_index;
   }
-  response.add(fmt::format("{}", Json::Factory::listAsJsonString(listeners)));
-
+  response.add(Json::Factory::listAsJsonString(listeners));
   return Http::Code::OK;
 }
 
@@ -334,6 +334,7 @@ AdminImpl::NullRouteConfigProvider::NullRouteConfigProvider()
     : config_(new Router::NullConfigImpl()) {}
 
 AdminImpl::AdminImpl(const std::string& access_log_path, const std::string& profile_path,
+                     const std::string& address_out_path,
                      Network::Address::InstanceConstSharedPtr address, Server::Instance& server)
     : server_(server), profile_path_(profile_path),
       socket_(new Network::TcpListenSocket(address, true)),
@@ -358,6 +359,15 @@ AdminImpl::AdminImpl(const std::string& access_log_path, const std::string& prof
           {"/stats", "print server stats", MAKE_HANDLER(handlerStats)},
           {"/listeners", "print listener addresses", MAKE_HANDLER(handlerListenerAddresses)}} {
 
+  if (address_out_path.length() > 0) {
+    std::ofstream address_out_file(address_out_path);
+    if (!address_out_file) {
+      log().critical("cannot open admin address output file {} for writing.", address_out_path);
+    } else {
+      address_out_file << socket_->localAddress()->asString();
+    }
+  }
+
   access_logs_.emplace_back(new Http::AccessLog::InstanceImpl(
       access_log_path, {}, Http::AccessLog::AccessLogFormatUtils::defaultAccessLogFormatter(),
       server.accessLogManager()));
diff --git a/source/server/http/admin.h b/source/server/http/admin.h
index 8c44d2140abb9..ed64b27dd0845 100644
--- a/source/server/http/admin.h
+++ b/source/server/http/admin.h
@@ -1,6 +1,7 @@
 #pragma once
 
 #include <chrono>
+#include <fstream>
 #include <list>
 #include <string>
 
@@ -29,7 +30,8 @@ class AdminImpl : public Admin,
                   Logger::Loggable<Logger::Id::admin> {
 public:
   AdminImpl(const std::string& access_log_path, const std::string& profiler_path,
-            Network::Address::InstanceConstSharedPtr address, Server::Instance& server);
+            const std::string& address_out_path, Network::Address::InstanceConstSharedPtr address,
+            Server::Instance& server);
 
   Http::Code runCallback(const std::string& path, Buffer::Instance& response);
   const Network::ListenSocket& socket() override { return *socket_; }
diff --git a/source/server/server.cc b/source/server/server.cc
index 6766cfd44b214..04887279524fb 100644
--- a/source/server/server.cc
+++ b/source/server/server.cc
@@ -189,15 +189,8 @@ void InstanceImpl::initialize(Options& options, TestHooks& hooks,
   drain_manager_->startParentShutdownSequence();
   original_start_time_ = info.original_start_time_;
   admin_.reset(new AdminImpl(initial_config.admin().accessLogPath(),
-                             initial_config.admin().profilePath(), initial_config.admin().address(),
-                             *this));
-
-  if (options.adminAddressPath().length() > 0) {
-    std::ofstream admin_address_file(options.adminAddressPath());
-    ASSERT(admin_address_file.is_open());
-    admin_address_file << admin_->mutable_socket().localAddress()->asString();
-    admin_address_file.close();
-  }
+                             initial_config.admin().profilePath(), options.adminAddressPath(),
+                             initial_config.admin().address(), *this));
 
   admin_scope_ = stats_store_.createScope("listener.admin.");
   handler_.addListener(*admin_, admin_->mutable_socket(), *admin_scope_,
diff --git a/source/server/server.h b/source/server/server.h
index 52b84c1961761..2e4b378de5c2f 100644
--- a/source/server/server.h
+++ b/source/server/server.h
@@ -2,7 +2,6 @@
 
 #include <chrono>
 #include <cstdint>
-#include <fstream>
 #include <functional>
 #include <list>
 #include <memory>
@@ -140,7 +139,6 @@ class InstanceImpl : Logger::Loggable<Logger::Id::main>, public Instance {
   Tracing::HttpTracer& httpTracer() override;
   ThreadLocal::Instance& threadLocal() override { return thread_local_; }
   const LocalInfo::LocalInfo& localInfo() override { return local_info_; }
-  int numListeners() override { return config_->listeners().size(); }
 
 private:
   void flushStats();
diff --git a/test/common/json/json_loader_test.cc b/test/common/json/json_loader_test.cc
index 3af55a5dbf86e..24a9bb3477b41 100644
--- a/test/common/json/json_loader_test.cc
+++ b/test/common/json/json_loader_test.cc
@@ -227,4 +227,20 @@ TEST(JsonLoaderTest, AsString) {
   });
 }
 
+TEST(JsonLoaderTest, ListAsString) {
+  std::list<std::string> list1 = {};
+  EXPECT_STREQ("[]", Json::Factory::listAsJsonString(list1).c_str());
+
+  std::list<std::string> list2 = {"one"};
+  EXPECT_STREQ("[\"one\"]", Json::Factory::listAsJsonString(list2).c_str());
+
+  std::list<std::string> list3 = {"one", "two", "three", "four"};
+  EXPECT_STREQ("[\"one\",\"two\",\"three\",\"four\"]",
+               Json::Factory::listAsJsonString(list3).c_str());
+
+  std::list<std::string> list4 = {"127.0.0.1:46465", "127.0.0.1:52211", "127.0.0.1:58941"};
+  EXPECT_STREQ("[\"127.0.0.1:46465\",\"127.0.0.1:52211\",\"127.0.0.1:58941\"]",
+               Json::Factory::listAsJsonString(list4).c_str());
+}
+
 } // Json
diff --git a/test/integration/hotrestart_test.sh b/test/integration/hotrestart_test.sh
index 5c2a61fa23c7b..3956b5632e90d 100755
--- a/test/integration/hotrestart_test.sh
+++ b/test/integration/hotrestart_test.sh
@@ -14,8 +14,8 @@ cat test/config/integration/server.json |
 
 # Now start the real server, hot restart it twice, and shut it all down as a basic hot restart
 # sanity test.
-ADMIN_ADDRESS_PATH_0="${TEST_TMPDIR}"/admin_address_file_0.pid
 echo "Starting epoch 0"
+ADMIN_ADDRESS_PATH_0="${TEST_TMPDIR}"/admin_0.address
 "${ENVOY_BIN}" -c "${HOT_RESTART_JSON}" \
     --restart-epoch 0 --base-id 1 --service-cluster cluster --service-node node \
     -a "${ADMIN_ADDRESS_PATH_0}" &
@@ -23,8 +23,10 @@ echo "Starting epoch 0"
 FIRST_SERVER_PID=$!
 sleep 3
 
-echo "Replacing listener addresses"
-"tools/socket_passing.py" "-o" "${HOT_RESTART_JSON}" "-a" "${ADMIN_ADDRESS_PATH_0}"
+echo "Updating original config json listener addresses"
+UPDATED_HOT_RESTART_JSON="${TEST_TMPDIR}"/hot_restart_updated.json
+tools/socket_passing.py "-o" "${HOT_RESTART_JSON}" "-a" "${ADMIN_ADDRESS_PATH_0}" \
+  "-u" "${UPDATED_HOT_RESTART_JSON}"
 
 # Send SIGUSR1 signal to the first server, this should not kill it. Also send SIGHUP which should
 # get eaten.
@@ -33,9 +35,9 @@ kill -SIGUSR1 ${FIRST_SERVER_PID}
 kill -SIGHUP ${FIRST_SERVER_PID}
 sleep 3
 
-ADMIN_ADDRESS_PATH_1="${TEST_TMPDIR}"/admin_address_file_1.pid
 echo "Starting epoch 1"
-"${ENVOY_BIN}" -c "${HOT_RESTART_JSON}" \
+ADMIN_ADDRESS_PATH_1="${TEST_TMPDIR}"/admin_1.address
+"${ENVOY_BIN}" -c "${UPDATED_HOT_RESTART_JSON}" \
     --restart-epoch 1 --base-id 1 --service-cluster cluster --service-node node \
     -a "${ADMIN_ADDRESS_PATH_1}" &
 
@@ -44,11 +46,15 @@ SECOND_SERVER_PID=$!
 sleep 7
 
 echo "Checking that listener addresses have not changed"
-"tools/socket_passing.py" "-o" "${HOT_RESTART_JSON}" "-a" "${ADMIN_ADDRESS_PATH_1}" "-n"
+HOT_RESTART_JSON_1="${TEST_TMPDIR}"/hot_restart_1.json
+tools/socket_passing.py "-o" "${UPDATED_HOT_RESTART_JSON}" "-a" "${ADMIN_ADDRESS_PATH_1}" \
+  "-u" "${HOT_RESTART_JSON_1}"
+CONFIG_DIFF=$(diff -Z "${UPDATED_HOT_RESTART_JSON}" "${HOT_RESTART_JSON_1}")
+[[ "${CONFIG_DIFF}" == "" ]]
 
-ADMIN_ADDRESS_PATH_2="${TEST_TMPDIR}"/admin_address_file_2.pid
+ADMIN_ADDRESS_PATH_2="${TEST_TMPDIR}"/admin_2.address
 echo "Starting epoch 2"
-"${ENVOY_BIN}" -c "${HOT_RESTART_JSON}" \
+"${ENVOY_BIN}" -c "${UPDATED_HOT_RESTART_JSON}" \
     --restart-epoch 2 --base-id 1 --service-cluster cluster --service-node node \
     -a "${ADMIN_ADDRESS_PATH_2}" &
 
@@ -56,7 +62,11 @@ THIRD_SERVER_PID=$!
 sleep 3
 
 echo "Checking that listener addresses have not changed"
-"tools/socket_passing.py" "-o" "${HOT_RESTART_JSON}" "-a" "${ADMIN_ADDRESS_PATH_2}" "-n"
+HOT_RESTART_JSON_2="${TEST_TMPDIR}"/hot_restart_2.json
+tools/socket_passing.py "-o" "${UPDATED_HOT_RESTART_JSON}" "-a" "${ADMIN_ADDRESS_PATH_2}" \
+  "-u" "${HOT_RESTART_JSON_2}"
+CONFIG_DIFF=$(diff -Z "${UPDATED_HOT_RESTART_JSON}" "${HOT_RESTART_JSON_2}")
+[[ "${CONFIG_DIFF}" == "" ]]
 
 # First server should already be gone.
 echo "Waiting for epoch 0"
diff --git a/test/integration/integration_admin_test.cc b/test/integration/integration_admin_test.cc
index e5b12c3a021a8..f3b2ed9927ef0 100644
--- a/test/integration/integration_admin_test.cc
+++ b/test/integration/integration_admin_test.cc
@@ -2,6 +2,8 @@
 
 #include "envoy/http/header_map.h"
 
+#include "common/json/json_loader.h"
+
 #include "test/integration/integration_test.h"
 #include "test/integration/utility.h"
 
@@ -126,8 +128,17 @@ TEST_F(IntegrationTest, Admin) {
 
   response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/listeners", "",
                                                 Http::CodecClient::Type::HTTP1);
+  std::list<std::string> listener_addresses;
+  int listener_index = 0;
+  while (test_server_->server().getListenSocketByIndex(listener_index) != nullptr) {
+    listener_addresses.push_back(
+        test_server_->server().getListenSocketByIndex(listener_index)->localAddress()->asString());
+    ++listener_index;
+  }
   EXPECT_TRUE(response->complete());
   EXPECT_STREQ("200", response->headers().Status()->value().c_str());
+  EXPECT_STREQ(Json::Factory::listAsJsonString(listener_addresses).c_str(),
+               response->body().c_str());
 }
 
 // Successful call to startProfiler requires tcmalloc.
diff --git a/test/mocks/server/mocks.h b/test/mocks/server/mocks.h
index 3e194122f52ca..918bf4901d4fa 100644
--- a/test/mocks/server/mocks.h
+++ b/test/mocks/server/mocks.h
@@ -135,7 +135,6 @@ class MockInstance : public Instance {
   MOCK_METHOD0(httpTracer, Tracing::HttpTracer&());
   MOCK_METHOD0(threadLocal, ThreadLocal::Instance&());
   MOCK_METHOD0(localInfo, const LocalInfo::LocalInfo&());
-  MOCK_METHOD0(numListeners, int());
 
   testing::NiceMock<ThreadLocal::MockInstance> thread_local_;
   Stats::IsolatedStoreImpl stats_store_;
diff --git a/test/server/http/BUILD b/test/server/http/BUILD
index ffdba3b0395a9..13eb0685a059e 100644
--- a/test/server/http/BUILD
+++ b/test/server/http/BUILD
@@ -11,6 +11,7 @@ envoy_cc_test(
         "//source/server/http:admin_lib",
         "//test/mocks/server:server_mocks",
         "//test/test_common:environment_lib",
+        "//test/test_common:network_utility_lib",
         "//test/test_common:utility_lib",
     ],
 )
diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc
index cb66bc0f3c21f..7c5b8e4aa9262 100644
--- a/test/server/http/admin_test.cc
+++ b/test/server/http/admin_test.cc
@@ -5,6 +5,7 @@
 
 #include "test/mocks/server/mocks.h"
 #include "test/test_common/environment.h"
+#include "test/test_common/network_utility.h"
 #include "test/test_common/printers.h"
 #include "test/test_common/utility.h"
 
@@ -21,6 +22,7 @@ class AdminFilterTest : public testing::Test {
   // TODO(mattklein123): Switch to mocks and do not bind to a real port.
   AdminFilterTest()
       : admin_("/dev/null", TestEnvironment::temporaryPath("envoy.prof"),
+               TestEnvironment::temporaryPath("admin.address"),
                Network::Utility::resolveUrl("tcp://127.0.0.1:9002"), server_),
         filter_(admin_), request_headers_{{":path", "/"}} {
     filter_.setDecoderFilterCallbacks(callbacks_);
@@ -53,12 +55,26 @@ TEST_F(AdminFilterTest, Trailers) {
   filter_.decodeTrailers(request_headers_);
 }
 
+class AdminInstanceTest : public testing::Test {
+public:
+  AdminInstanceTest()
+      : address_out_path_(TestEnvironment::temporaryPath("admin.address")),
+        cpu_profile_path_(TestEnvironment::temporaryPath("envoy.prof")),
+        admin_("/dev/null", cpu_profile_path_, address_out_path_,
+               Network::Test::getSomeLoopbackAddress(Network::Address::IpVersion::v4), server_) {}
+
+  std::string address_out_path_;
+  std::string cpu_profile_path_;
+  NiceMock<MockInstance> server_;
+  AdminImpl admin_;
+};
+
 // Can only get code coverage of AdminImpl::handlerCpuProfiler stopProfiler with
 // a real profiler linked in (successful call to startProfiler). startProfiler
 // requies tcmalloc.
 #ifdef TCMALLOC
 
-TEST_F(AdminFilterTest, AdminProfiler) {
+TEST_F(AdminInstanceTest, AdminProfiler) {
   Buffer::OwnedImpl data;
   admin_.runCallback("/cpuprofiler?enable=y", data);
   EXPECT_TRUE(Profiler::Cpu::profilerEnabled());
@@ -68,13 +84,28 @@ TEST_F(AdminFilterTest, AdminProfiler) {
 
 #endif
 
-TEST_F(AdminFilterTest, AdminBadProfiler) {
+TEST_F(AdminInstanceTest, AdminBadProfiler) {
   Buffer::OwnedImpl data;
-  AdminImpl admin_bad_profile_path("/dev/null",
-                                   TestEnvironment::temporaryPath("some/unlikely/bad/path.prof"),
-                                   Network::Utility::resolveUrl("tcp://127.0.0.1:9002"), server_);
+  AdminImpl admin_bad_profile_path(
+      "/dev/null", TestEnvironment::temporaryPath("some/unlikely/bad/path.prof"), "",
+      Network::Test::getSomeLoopbackAddress(Network::Address::IpVersion::v4), server_);
   admin_bad_profile_path.runCallback("/cpuprofiler?enable=y", data);
   EXPECT_FALSE(Profiler::Cpu::profilerEnabled());
 }
 
+TEST_F(AdminInstanceTest, WriteAddressToFile) {
+  std::ifstream address_file(address_out_path_);
+  std::string address_from_file;
+  std::getline(address_file, address_from_file);
+  EXPECT_STREQ(admin_.socket().localAddress()->asString().c_str(), address_from_file.c_str());
+}
+
+TEST_F(AdminInstanceTest, AdminBadAddressOutPath) {
+  std::string bad_path =
+      TestEnvironment::temporaryPath("some/unlikely/bad/path/admin.address");
+  AdminImpl admin_bad_address_out_path(
+      "/dev/null", cpu_profile_path_, bad_path,
+      Network::Test::getSomeLoopbackAddress(Network::Address::IpVersion::v4), server_);
+  EXPECT_FALSE(std::ifstream(bad_path));
+}
 } // namespace Server
diff --git a/tools/socket_passing.py b/tools/socket_passing.py
index 1db55ea087a67..e89615531346c 100755
--- a/tools/socket_passing.py
+++ b/tools/socket_passing.py
@@ -12,64 +12,63 @@
 from collections import OrderedDict
 
 import argparse
+import httplib
 import json
 import os.path
-import httplib
 import sys
 import time
 
-def CheckNoChange(original_listeners, updated_listeners):
-  for updated, original in zip(updated_listeners, original_listeners):
-      if original['address'] != "tcp://" + updated:
-        return False
-  return True
+ADMIN_FILE_TIMEOUT_SECS = 20
 
-def ReplaceListenerAddresses(original_json, admin_address, no_port_change):
+def ReplaceListenerAddresses(original_json, admin_address, updated_json):
   # Get original listener addresses
   with open(original_json, 'r') as original_json_file:
+    # Import original config file in order to get a deterministic output. This
+    # allows us to diff the original config file and the updated config file
+    # output from this script to check for any changes.
     parsed_json = json.load(original_json_file, object_pairs_hook=OrderedDict)
   original_listeners = parsed_json['listeners']
 
-  admin_conn = httplib.HTTPConnection(admin_address)
-  admin_conn.request("GET", "/listeners")
-  admin_response = admin_conn.getresponse()
-
-  if not admin_response.status == 200:
-    admin_conn.close()
+  sys.stdout.write('Admin address is ' + admin_address + '\n')
+  try:
+    admin_conn = httplib.HTTPConnection(admin_address)
+    admin_conn.request('GET', '/listeners')
+    admin_response = admin_conn.getresponse()
+    if not admin_response.status == 200:
+      return False
+    discovered_listeners = json.loads(admin_response.read())
+  except:
+    sys.stderr.write('Cannot connect to admin.\n')
     return False
-
-  updated_listeners = json.loads(admin_response.read())
-  admin_conn.close()
-
-  if no_port_change:
-    return CheckNoChange(original_listeners, updated_listeners)
   else:
-    for updated, original in zip(updated_listeners, original_listeners):
-      original['address'] = "tcp://" + updated
-    with open(original_json, 'w') as outfile:
+    if not len(discovered) == len(original):
+      return False
+    for discovered, original in zip(discovered_listeners, original_listeners):
+      original['address'] = 'tcp://' + discovered
+    with open(updated_json, 'w') as outfile:
       json.dump(OrderedDict(parsed_json), outfile, indent=2)
+  finally:
+    admin_conn.close()
 
   return True
 
 if __name__ == '__main__':
   parser = argparse.ArgumentParser(description='Replace listener addressses in json file.')
   parser.add_argument('-o', '--original_json', type=str, required=True,
-                      help='Original json file')
+                      help='Path of the original config json file')
   parser.add_argument('-a', '--admin_address_path', type=str, required=True,
-                      help='Admin address path')
-  parser.add_argument('-n', '--no_port_change', action='store_true', default=False,
-                      help='Check that listener port addresses have not changed.');
+                      help='Path of the admin address file')
+  parser.add_argument('-u', '--updated_json', type=str, required=True,
+                      help='Path to output updated json config file')
   args = parser.parse_args()
-  original_json = args.original_json
   admin_address_path = args.admin_address_path
-  no_port_change = args.no_port_change
 
   # Read admin address from file
   counter = 0;
   while not os.path.exists(admin_address_path):
     time.sleep(1)
     counter += 1
-    if counter > 20:
+    if counter > ADMIN_FILE_TIMEOUT_SECS:
       break
 
   if not os.path.exists(admin_address_path):
@@ -78,6 +77,7 @@ def ReplaceListenerAddresses(original_json, admin_address, no_port_change):
   with open(admin_address_path, 'r') as admin_address_file:
     admin_address = admin_address_file.read()
 
-  result = ReplaceListenerAddresses(original_json, admin_address, no_port_change)
+  result = ReplaceListenerAddresses(args.original_json, admin_address, args.updated_json)
+
   if not result:
     sys.exit(1)

From 6aca5098dfa6bd627ef4cec61ae11dcd0da5e766 Mon Sep 17 00:00:00 2001
From: Henna Huang <henna@google.com>
Date: Sun, 23 Apr 2017 22:46:45 -0400
Subject: [PATCH 4/9] Fixes

---
 test/server/http/admin_test.cc | 4 +---
 tools/socket_passing.py        | 2 +-
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc
index 57f82272e6a62..0e33868b5ce4c 100644
--- a/test/server/http/admin_test.cc
+++ b/test/server/http/admin_test.cc
@@ -86,7 +86,6 @@ TEST_F(AdminInstanceTest, AdminProfiler) {
 
 TEST_F(AdminInstanceTest, AdminBadProfiler) {
   Buffer::OwnedImpl data;
-<<<<<<< HEAD
   AdminImpl admin_bad_profile_path(
       "/dev/null", TestEnvironment::temporaryPath("some/unlikely/bad/path.prof"), "",
       Network::Test::getSomeLoopbackAddress(Network::Address::IpVersion::v4), server_);
@@ -102,8 +101,7 @@ TEST_F(AdminInstanceTest, WriteAddressToFile) {
 }
 
 TEST_F(AdminInstanceTest, AdminBadAddressOutPath) {
-  std::string bad_path =
-      TestEnvironment::temporaryPath("some/unlikely/bad/path/admin.address");
+  std::string bad_path = TestEnvironment::temporaryPath("some/unlikely/bad/path/admin.address");
   AdminImpl admin_bad_address_out_path(
       "/dev/null", cpu_profile_path_, bad_path,
       Network::Test::getSomeLoopbackAddress(Network::Address::IpVersion::v4), server_);
diff --git a/tools/socket_passing.py b/tools/socket_passing.py
index e89615531346c..5a78acd112b3b 100755
--- a/tools/socket_passing.py
+++ b/tools/socket_passing.py
@@ -41,7 +41,7 @@ def ReplaceListenerAddresses(original_json, admin_address, updated_json):
     sys.stderr.write('Cannot connect to admin.\n')
     return False
   else:
-    if not len(discovered) == len(original):
+    if not len(discovered_listeners) == len(original_listeners):
       return False
     for discovered, original in zip(discovered_listeners, original_listeners):
       original['address'] = 'tcp://' + discovered

From ad79a1470754262a812c2cf5999195d2981fcc12 Mon Sep 17 00:00:00 2001
From: Henna Huang <henna@google.com>
Date: Mon, 24 Apr 2017 18:42:23 -0400
Subject: [PATCH 5/9] Response to comments

---
 source/server/http/admin.cc                |  9 ++++----
 source/server/http/admin.h                 |  2 +-
 test/common/json/json_loader_test.cc       | 26 +++++++++++++++++-----
 test/integration/BUILD                     |  2 +-
 test/integration/hotrestart_test.sh        | 10 ++++-----
 test/integration/integration_admin_test.cc | 17 +++++++-------
 tools/BUILD                                | 10 ++++++++-
 tools/socket_passing.py                    | 18 +++++++--------
 8 files changed, 57 insertions(+), 37 deletions(-)

diff --git a/source/server/http/admin.cc b/source/server/http/admin.cc
index 9dc443a35feee..9b5303dcb8178 100644
--- a/source/server/http/admin.cc
+++ b/source/server/http/admin.cc
@@ -285,12 +285,11 @@ Http::Code AdminImpl::handlerQuitQuitQuit(const std::string&, Buffer::Instance&
   return Http::Code::OK;
 }
 
-Http::Code AdminImpl::handlerListenerAddresses(const std::string&, Buffer::Instance& response) {
+Http::Code AdminImpl::handlerListenerInfo(const std::string&, Buffer::Instance& response) {
   std::list<std::string> listeners;
   int listener_index = 0;
-  while (server_.getListenSocketByIndex(listener_index) != nullptr) {
-    listeners.push_back(server_.getListenSocketByIndex(listener_index)->localAddress()->asString());
-    ++listener_index;
+  while (auto listen_socket = server_.getListenSocketByIndex(listener_index++)) {
+    listeners.push_back(listen_socket->localAddress()->asString());
   }
   response.add(Json::Factory::listAsJsonString(listeners));
   return Http::Code::OK;
@@ -357,7 +356,7 @@ AdminImpl::AdminImpl(const std::string& access_log_path, const std::string& prof
           {"/server_info", "print server version/status information",
            MAKE_HANDLER(handlerServerInfo)},
           {"/stats", "print server stats", MAKE_HANDLER(handlerStats)},
-          {"/listeners", "print listener addresses", MAKE_HANDLER(handlerListenerAddresses)}} {
+          {"/listeners", "print listener addresses", MAKE_HANDLER(handlerListenerInfo)}} {
 
   if (address_out_path.length() > 0) {
     std::ofstream address_out_file(address_out_path);
diff --git a/source/server/http/admin.h b/source/server/http/admin.h
index ed64b27dd0845..a15dd36e67ee3 100644
--- a/source/server/http/admin.h
+++ b/source/server/http/admin.h
@@ -120,7 +120,7 @@ class AdminImpl : public Admin,
   Http::Code handlerServerInfo(const std::string& url, Buffer::Instance& response);
   Http::Code handlerStats(const std::string& url, Buffer::Instance& response);
   Http::Code handlerQuitQuitQuit(const std::string& url, Buffer::Instance& response);
-  Http::Code handlerListenerAddresses(const std::string& url, Buffer::Instance& response);
+  Http::Code handlerListenerInfo(const std::string& url, Buffer::Instance& response);
 
   Server::Instance& server_;
   std::list<Http::AccessLog::InstanceSharedPtr> access_logs_;
diff --git a/test/common/json/json_loader_test.cc b/test/common/json/json_loader_test.cc
index 24a9bb3477b41..052eeec84c13d 100644
--- a/test/common/json/json_loader_test.cc
+++ b/test/common/json/json_loader_test.cc
@@ -229,18 +229,32 @@ TEST(JsonLoaderTest, AsString) {
 
 TEST(JsonLoaderTest, ListAsString) {
   std::list<std::string> list1 = {};
-  EXPECT_STREQ("[]", Json::Factory::listAsJsonString(list1).c_str());
+  std::vector<Json::ObjectPtr> output1 =
+      Json::Factory::LoadFromString(Json::Factory::listAsJsonString(list1))->asObjectArray();
+  EXPECT_TRUE(output1.empty());
 
   std::list<std::string> list2 = {"one"};
-  EXPECT_STREQ("[\"one\"]", Json::Factory::listAsJsonString(list2).c_str());
+  std::vector<Json::ObjectPtr> output2 =
+      Json::Factory::LoadFromString(Json::Factory::listAsJsonString(list2))->asObjectArray();
+  EXPECT_EQ(output2.size(), 1);
+  EXPECT_STREQ(output2[0]->asString().c_str(), "one");
 
   std::list<std::string> list3 = {"one", "two", "three", "four"};
-  EXPECT_STREQ("[\"one\",\"two\",\"three\",\"four\"]",
-               Json::Factory::listAsJsonString(list3).c_str());
+  std::vector<Json::ObjectPtr> output3 =
+      Json::Factory::LoadFromString(Json::Factory::listAsJsonString(list3))->asObjectArray();
+  EXPECT_EQ(output3.size(), 4);
+  EXPECT_STREQ(output3[0]->asString().c_str(), "one");
+  EXPECT_STREQ(output3[1]->asString().c_str(), "two");
+  EXPECT_STREQ(output3[2]->asString().c_str(), "three");
+  EXPECT_STREQ(output3[3]->asString().c_str(), "four");
 
   std::list<std::string> list4 = {"127.0.0.1:46465", "127.0.0.1:52211", "127.0.0.1:58941"};
-  EXPECT_STREQ("[\"127.0.0.1:46465\",\"127.0.0.1:52211\",\"127.0.0.1:58941\"]",
-               Json::Factory::listAsJsonString(list4).c_str());
+  std::vector<Json::ObjectPtr> output4 =
+      Json::Factory::LoadFromString(Json::Factory::listAsJsonString(list4))->asObjectArray();
+  EXPECT_EQ(output4.size(), 3);
+  EXPECT_STREQ(output4[0]->asString().c_str(), "127.0.0.1:46465");
+  EXPECT_STREQ(output4[1]->asString().c_str(), "127.0.0.1:52211");
+  EXPECT_STREQ(output4[2]->asString().c_str(), "127.0.0.1:58941");
 }
 
 } // Json
diff --git a/test/integration/BUILD b/test/integration/BUILD
index c9a7f0c627788..421bc16267796 100644
--- a/test/integration/BUILD
+++ b/test/integration/BUILD
@@ -10,7 +10,7 @@ envoy_sh_test(
         "//test/common/runtime:filesystem_setup.sh",
         "//test/common/runtime:filesystem_test_data",
         "//test/config/integration:server_config_files",
-        "//tools:socket_passing.py",
+        "//tools:socket_passing",
     ],
     # Need to disable test execution sandboxing for this test, since it uses shmem.
     # TODO(htuch): When most folks are using a Bazel with the recent
diff --git a/test/integration/hotrestart_test.sh b/test/integration/hotrestart_test.sh
index 489a1217f5680..6510899e6dc51 100755
--- a/test/integration/hotrestart_test.sh
+++ b/test/integration/hotrestart_test.sh
@@ -26,7 +26,7 @@ sleep 3
 
 echo "Updating original config json listener addresses"
 UPDATED_HOT_RESTART_JSON="${TEST_TMPDIR}"/hot_restart_updated.json
-tools/socket_passing.py "-o" "${HOT_RESTART_JSON}" "-a" "${ADMIN_ADDRESS_PATH_0}" \
+"${TEST_RUNDIR}"/tools/socket_passing "-o" "${HOT_RESTART_JSON}" "-a" "${ADMIN_ADDRESS_PATH_0}" \
   "-u" "${UPDATED_HOT_RESTART_JSON}"
 
 # Send SIGUSR1 signal to the first server, this should not kill it. Also send SIGHUP which should
@@ -48,10 +48,10 @@ sleep 7
 
 echo "Checking that listener addresses have not changed"
 HOT_RESTART_JSON_1="${TEST_TMPDIR}"/hot_restart_1.json
-tools/socket_passing.py "-o" "${UPDATED_HOT_RESTART_JSON}" "-a" "${ADMIN_ADDRESS_PATH_1}" \
+"${TEST_RUNDIR}"/tools/socket_passing "-o" "${UPDATED_HOT_RESTART_JSON}" "-a" "${ADMIN_ADDRESS_PATH_1}" \
   "-u" "${HOT_RESTART_JSON_1}"
 CONFIG_DIFF=$(diff -Z "${UPDATED_HOT_RESTART_JSON}" "${HOT_RESTART_JSON_1}")
-[[ "${CONFIG_DIFF}" == "" ]]
+[[ -z "${CONFIG_DIFF}" ]]
 
 ADMIN_ADDRESS_PATH_2="${TEST_TMPDIR}"/admin_2.address
 echo "Starting epoch 2"
@@ -64,10 +64,10 @@ sleep 3
 
 echo "Checking that listener addresses have not changed"
 HOT_RESTART_JSON_2="${TEST_TMPDIR}"/hot_restart_2.json
-tools/socket_passing.py "-o" "${UPDATED_HOT_RESTART_JSON}" "-a" "${ADMIN_ADDRESS_PATH_2}" \
+"${TEST_RUNDIR}"/tools/socket_passing "-o" "${UPDATED_HOT_RESTART_JSON}" "-a" "${ADMIN_ADDRESS_PATH_2}" \
   "-u" "${HOT_RESTART_JSON_2}"
 CONFIG_DIFF=$(diff -Z "${UPDATED_HOT_RESTART_JSON}" "${HOT_RESTART_JSON_2}")
-[[ "${CONFIG_DIFF}" == "" ]]
+[[ -z "${CONFIG_DIFF}" ]]
 
 # First server should already be gone.
 echo "Waiting for epoch 0"
diff --git a/test/integration/integration_admin_test.cc b/test/integration/integration_admin_test.cc
index b75d3636c31a1..8ab7c017f8cb2 100644
--- a/test/integration/integration_admin_test.cc
+++ b/test/integration/integration_admin_test.cc
@@ -127,17 +127,16 @@ TEST_F(IntegrationTest, Admin) {
 
   response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/listeners", "",
                                                 Http::CodecClient::Type::HTTP1);
-  std::list<std::string> listener_addresses;
-  int listener_index = 0;
-  while (test_server_->server().getListenSocketByIndex(listener_index) != nullptr) {
-    listener_addresses.push_back(
-        test_server_->server().getListenSocketByIndex(listener_index)->localAddress()->asString());
-    ++listener_index;
-  }
   EXPECT_TRUE(response->complete());
   EXPECT_STREQ("200", response->headers().Status()->value().c_str());
-  EXPECT_STREQ(Json::Factory::listAsJsonString(listener_addresses).c_str(),
-               response->body().c_str());
+
+  std::vector<Json::ObjectPtr> listener_info =
+      Json::Factory::LoadFromString(response->body())->asObjectArray();
+  for (std::size_t index = 0; index < listener_info.size(); index++) {
+    EXPECT_STREQ(
+        test_server_->server().getListenSocketByIndex(index)->localAddress()->asString().c_str(),
+        listener_info[index]->asString().c_str());
+  }
 }
 
 // Successful call to startProfiler requires tcmalloc.
diff --git a/tools/BUILD b/tools/BUILD
index 74536baa18a2f..b68dcbf320878 100644
--- a/tools/BUILD
+++ b/tools/BUILD
@@ -1,7 +1,15 @@
 package(default_visibility = ["//visibility:public"])
 
+load("//bazel:envoy_build_system.bzl", "envoy_py_test_binary")
+
 exports_files([
     "gen_git_sha.sh",
     "git_sha_rewriter.py",
-    "socket_passing.py",
 ])
+
+envoy_py_test_binary(
+    name = "socket_passing",
+    srcs = [
+        "socket_passing.py",
+    ],
+)
diff --git a/tools/socket_passing.py b/tools/socket_passing.py
index 5a78acd112b3b..9a87ffe0063b5 100755
--- a/tools/socket_passing.py
+++ b/tools/socket_passing.py
@@ -1,13 +1,11 @@
 #!/usr/bin/env python
 
 # This tool is a helper script that queries the admin address for all listener
-# addresses after envoy startup. The script can then be used to update an
-# exisiting json config file with updated listener addresses. This script is
-# currently called in the hot restart integration test to update listener
-# addresses bound to port 0 in the intial json config file. With the
-# -n or -no_port_change option, this script does not update an exisiting json
-# config file but checks that the listener addresses after envoy startup
-# match the listener addresses in the initial json config file.
+# addresses after envoy startup. (The admin adress is written out to a file by
+# setting the -a flag in the envoy binary.) The script then outputs a new json
+# config file with updated listener addresses. This script is currently called
+# in the hot restart integration test to update listener addresses bound to
+# port 0 in the intial json config file.
 
 from collections import OrderedDict
 
@@ -18,6 +16,8 @@
 import sys
 import time
 
+# Seconds to wait for the admin address output file to appear. The script exits
+# if the file is not found.
 ADMIN_FILE_TIMEOUT_SECS = 20
 
 def ReplaceListenerAddresses(original_json, admin_address, updated_json):
@@ -37,8 +37,8 @@ def ReplaceListenerAddresses(original_json, admin_address, updated_json):
     if not admin_response.status == 200:
       return False
     discovered_listeners = json.loads(admin_response.read())
-  except:
-    sys.stderr.write('Cannot connect to admin.\n')
+  except Exception as e:
+    sys.stderr.write('Cannot connect to admin: %s\n' % e)
     return False
   else:
     if not len(discovered_listeners) == len(original_listeners):

From 1c04a2bae983a3f561b0f1e67eeb44be8919223e Mon Sep 17 00:00:00 2001
From: Henna Huang <henna@google.com>
Date: Tue, 25 Apr 2017 11:22:48 -0400
Subject: [PATCH 6/9] Response to comments

---
 source/server/http/admin.cc                |  1 +
 source/server/http/admin.h                 |  1 -
 test/common/json/json_loader_test.cc       | 22 +++++++++++-----------
 test/integration/hotrestart_test.sh        |  6 +++---
 test/integration/integration_admin_test.cc |  5 ++---
 test/server/http/admin_test.cc             |  4 +++-
 tools/BUILD                                |  3 +--
 tools/socket_passing.py                    | 12 ++++++------
 8 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/source/server/http/admin.cc b/source/server/http/admin.cc
index 9b5303dcb8178..dc89ef6a327e9 100644
--- a/source/server/http/admin.cc
+++ b/source/server/http/admin.cc
@@ -1,6 +1,7 @@
 #include "server/http/admin.h"
 
 #include <cstdint>
+#include <fstream>
 #include <string>
 #include <unordered_set>
 
diff --git a/source/server/http/admin.h b/source/server/http/admin.h
index a15dd36e67ee3..c5757eeb63a80 100644
--- a/source/server/http/admin.h
+++ b/source/server/http/admin.h
@@ -1,7 +1,6 @@
 #pragma once
 
 #include <chrono>
-#include <fstream>
 #include <list>
 #include <string>
 
diff --git a/test/common/json/json_loader_test.cc b/test/common/json/json_loader_test.cc
index 052eeec84c13d..31ddf437f4dbc 100644
--- a/test/common/json/json_loader_test.cc
+++ b/test/common/json/json_loader_test.cc
@@ -236,25 +236,25 @@ TEST(JsonLoaderTest, ListAsString) {
   std::list<std::string> list2 = {"one"};
   std::vector<Json::ObjectPtr> output2 =
       Json::Factory::LoadFromString(Json::Factory::listAsJsonString(list2))->asObjectArray();
-  EXPECT_EQ(output2.size(), 1);
-  EXPECT_STREQ(output2[0]->asString().c_str(), "one");
+  EXPECT_EQ(1, output2.size());
+  EXPECT_EQ("one", output2[0]->asString());
 
   std::list<std::string> list3 = {"one", "two", "three", "four"};
   std::vector<Json::ObjectPtr> output3 =
       Json::Factory::LoadFromString(Json::Factory::listAsJsonString(list3))->asObjectArray();
-  EXPECT_EQ(output3.size(), 4);
-  EXPECT_STREQ(output3[0]->asString().c_str(), "one");
-  EXPECT_STREQ(output3[1]->asString().c_str(), "two");
-  EXPECT_STREQ(output3[2]->asString().c_str(), "three");
-  EXPECT_STREQ(output3[3]->asString().c_str(), "four");
+  EXPECT_EQ(4, output3.size());
+  EXPECT_EQ("one", output3[0]->asString());
+  EXPECT_EQ("two", output3[1]->asString());
+  EXPECT_EQ("three", output3[2]->asString());
+  EXPECT_EQ("four", output3[3]->asString());
 
   std::list<std::string> list4 = {"127.0.0.1:46465", "127.0.0.1:52211", "127.0.0.1:58941"};
   std::vector<Json::ObjectPtr> output4 =
       Json::Factory::LoadFromString(Json::Factory::listAsJsonString(list4))->asObjectArray();
-  EXPECT_EQ(output4.size(), 3);
-  EXPECT_STREQ(output4[0]->asString().c_str(), "127.0.0.1:46465");
-  EXPECT_STREQ(output4[1]->asString().c_str(), "127.0.0.1:52211");
-  EXPECT_STREQ(output4[2]->asString().c_str(), "127.0.0.1:58941");
+  EXPECT_EQ(3, output4.size());
+  EXPECT_EQ("127.0.0.1:46465", output4[0]->asString());
+  EXPECT_EQ("127.0.0.1:52211", output4[1]->asString());
+  EXPECT_EQ("127.0.0.1:58941", output4[2]->asString());
 }
 
 } // Json
diff --git a/test/integration/hotrestart_test.sh b/test/integration/hotrestart_test.sh
index 6510899e6dc51..18fa813f48ba5 100755
--- a/test/integration/hotrestart_test.sh
+++ b/test/integration/hotrestart_test.sh
@@ -26,7 +26,7 @@ sleep 3
 
 echo "Updating original config json listener addresses"
 UPDATED_HOT_RESTART_JSON="${TEST_TMPDIR}"/hot_restart_updated.json
-"${TEST_RUNDIR}"/tools/socket_passing "-o" "${HOT_RESTART_JSON}" "-a" "${ADMIN_ADDRESS_PATH_0}" \
+"${TEST_RUNDIR}"/tools/socket_passing.py "-o" "${HOT_RESTART_JSON}" "-a" "${ADMIN_ADDRESS_PATH_0}" \
   "-u" "${UPDATED_HOT_RESTART_JSON}"
 
 # Send SIGUSR1 signal to the first server, this should not kill it. Also send SIGHUP which should
@@ -48,7 +48,7 @@ sleep 7
 
 echo "Checking that listener addresses have not changed"
 HOT_RESTART_JSON_1="${TEST_TMPDIR}"/hot_restart_1.json
-"${TEST_RUNDIR}"/tools/socket_passing "-o" "${UPDATED_HOT_RESTART_JSON}" "-a" "${ADMIN_ADDRESS_PATH_1}" \
+"${TEST_RUNDIR}"/tools/socket_passing.py "-o" "${UPDATED_HOT_RESTART_JSON}" "-a" "${ADMIN_ADDRESS_PATH_1}" \
   "-u" "${HOT_RESTART_JSON_1}"
 CONFIG_DIFF=$(diff -Z "${UPDATED_HOT_RESTART_JSON}" "${HOT_RESTART_JSON_1}")
 [[ -z "${CONFIG_DIFF}" ]]
@@ -64,7 +64,7 @@ sleep 3
 
 echo "Checking that listener addresses have not changed"
 HOT_RESTART_JSON_2="${TEST_TMPDIR}"/hot_restart_2.json
-"${TEST_RUNDIR}"/tools/socket_passing "-o" "${UPDATED_HOT_RESTART_JSON}" "-a" "${ADMIN_ADDRESS_PATH_2}" \
+"${TEST_RUNDIR}"/tools/socket_passing.py "-o" "${UPDATED_HOT_RESTART_JSON}" "-a" "${ADMIN_ADDRESS_PATH_2}" \
   "-u" "${HOT_RESTART_JSON_2}"
 CONFIG_DIFF=$(diff -Z "${UPDATED_HOT_RESTART_JSON}" "${HOT_RESTART_JSON_2}")
 [[ -z "${CONFIG_DIFF}" ]]
diff --git a/test/integration/integration_admin_test.cc b/test/integration/integration_admin_test.cc
index 8ab7c017f8cb2..1811075c4784e 100644
--- a/test/integration/integration_admin_test.cc
+++ b/test/integration/integration_admin_test.cc
@@ -133,9 +133,8 @@ TEST_F(IntegrationTest, Admin) {
   std::vector<Json::ObjectPtr> listener_info =
       Json::Factory::LoadFromString(response->body())->asObjectArray();
   for (std::size_t index = 0; index < listener_info.size(); index++) {
-    EXPECT_STREQ(
-        test_server_->server().getListenSocketByIndex(index)->localAddress()->asString().c_str(),
-        listener_info[index]->asString().c_str());
+    EXPECT_EQ(test_server_->server().getListenSocketByIndex(index)->localAddress()->asString(),
+              listener_info[index]->asString());
   }
 }
 
diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc
index 0e33868b5ce4c..94a9f18aad199 100644
--- a/test/server/http/admin_test.cc
+++ b/test/server/http/admin_test.cc
@@ -1,3 +1,5 @@
+#include <fstream>
+
 #include "common/http/message_impl.h"
 #include "common/profiler/profiler.h"
 
@@ -97,7 +99,7 @@ TEST_F(AdminInstanceTest, WriteAddressToFile) {
   std::ifstream address_file(address_out_path_);
   std::string address_from_file;
   std::getline(address_file, address_from_file);
-  EXPECT_STREQ(admin_.socket().localAddress()->asString().c_str(), address_from_file.c_str());
+  EXPECT_EQ(admin_.socket().localAddress()->asString(), address_from_file);
 }
 
 TEST_F(AdminInstanceTest, AdminBadAddressOutPath) {
diff --git a/tools/BUILD b/tools/BUILD
index 79a0d5ef9dbb3..ef893b865bb94 100644
--- a/tools/BUILD
+++ b/tools/BUILD
@@ -1,12 +1,11 @@
 load(
     "//bazel:envoy_build_system.bzl",
+    "envoy_py_test_binary",
     "envoy_package",
 )
 
 envoy_package()
 
-load("//bazel:envoy_build_system.bzl", "envoy_py_test_binary")
-
 exports_files([
     "gen_git_sha.sh",
     "git_sha_rewriter.py",
diff --git a/tools/socket_passing.py b/tools/socket_passing.py
index 9a87ffe0063b5..86e5e2d3b47f0 100755
--- a/tools/socket_passing.py
+++ b/tools/socket_passing.py
@@ -17,10 +17,10 @@
 import time
 
 # Seconds to wait for the admin address output file to appear. The script exits
-# if the file is not found.
+# with failure if the file is not found.
 ADMIN_FILE_TIMEOUT_SECS = 20
 
-def ReplaceListenerAddresses(original_json, admin_address, updated_json):
+def GenerateNewConfig(original_json, admin_address, updated_json):
   # Get original listener addresses
   with open(original_json, 'r') as original_json_file:
     # Import original config file in order to get a deterministic output. This
@@ -41,12 +41,12 @@ def ReplaceListenerAddresses(original_json, admin_address, updated_json):
     sys.stderr.write('Cannot connect to admin: %s\n' % e)
     return False
   else:
-    if not len(discovered_listeners) == len(original_listeners):
+    if len(discovered_listeners) != len(original_listeners):
       return False
     for discovered, original in zip(discovered_listeners, original_listeners):
       original['address'] = 'tcp://' + discovered
     with open(updated_json, 'w') as outfile:
-      json.dump(OrderedDict(parsed_json), outfile, indent=2)
+      json.dump(OrderedDict(parsed_json), outfile, indent=2, separators=(',',':'))
   finally:
     admin_conn.close()
 
@@ -77,7 +77,7 @@ def ReplaceListenerAddresses(original_json, admin_address, updated_json):
   with open(admin_address_path, 'r') as admin_address_file:
     admin_address = admin_address_file.read()
 
-  result = ReplaceListenerAddresses(args.original_json, admin_address, args.updated_json)
+  success = GenerateNewConfig(args.original_json, admin_address, args.updated_json)
 
-  if not result:
+  if not success:
     sys.exit(1)

From 8b7d3d566386fd3098f64fd8ace828354f746890 Mon Sep 17 00:00:00 2001
From: Henna Huang <henna@google.com>
Date: Tue, 25 Apr 2017 12:46:28 -0400
Subject: [PATCH 7/9] Fix

---
 tools/BUILD | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/BUILD b/tools/BUILD
index ef893b865bb94..d516bf1c4ea1f 100644
--- a/tools/BUILD
+++ b/tools/BUILD
@@ -1,7 +1,7 @@
 load(
     "//bazel:envoy_build_system.bzl",
-    "envoy_py_test_binary",
     "envoy_package",
+    "envoy_py_test_binary",
 )
 
 envoy_package()

From e6e4b591e8b6c6bb4deabaeb026323c9d5dcbfc8 Mon Sep 17 00:00:00 2001
From: Henna Huang <henna@google.com>
Date: Thu, 27 Apr 2017 12:44:38 -0400
Subject: [PATCH 8/9] Fix asan heap-use-after-free

---
 test/common/json/json_loader_test.cc       | 63 ++++++++++++----------
 test/integration/integration_admin_test.cc |  4 +-
 2 files changed, 38 insertions(+), 29 deletions(-)

diff --git a/test/common/json/json_loader_test.cc b/test/common/json/json_loader_test.cc
index 31ddf437f4dbc..b6716b75cfb0c 100644
--- a/test/common/json/json_loader_test.cc
+++ b/test/common/json/json_loader_test.cc
@@ -96,6 +96,7 @@ TEST(JsonLoaderTest, Basic) {
 
     ObjectPtr config = Factory::LoadFromString(json);
     EXPECT_EQ(2U, config->getObjectArray("descriptors")[0]->asObjectArray().size());
+
     EXPECT_EQ(1U, config->getObjectArray("descriptors")[1]->asObjectArray().size());
   }
 
@@ -228,33 +229,41 @@ TEST(JsonLoaderTest, AsString) {
 }
 
 TEST(JsonLoaderTest, ListAsString) {
-  std::list<std::string> list1 = {};
-  std::vector<Json::ObjectPtr> output1 =
-      Json::Factory::LoadFromString(Json::Factory::listAsJsonString(list1))->asObjectArray();
-  EXPECT_TRUE(output1.empty());
-
-  std::list<std::string> list2 = {"one"};
-  std::vector<Json::ObjectPtr> output2 =
-      Json::Factory::LoadFromString(Json::Factory::listAsJsonString(list2))->asObjectArray();
-  EXPECT_EQ(1, output2.size());
-  EXPECT_EQ("one", output2[0]->asString());
-
-  std::list<std::string> list3 = {"one", "two", "three", "four"};
-  std::vector<Json::ObjectPtr> output3 =
-      Json::Factory::LoadFromString(Json::Factory::listAsJsonString(list3))->asObjectArray();
-  EXPECT_EQ(4, output3.size());
-  EXPECT_EQ("one", output3[0]->asString());
-  EXPECT_EQ("two", output3[1]->asString());
-  EXPECT_EQ("three", output3[2]->asString());
-  EXPECT_EQ("four", output3[3]->asString());
-
-  std::list<std::string> list4 = {"127.0.0.1:46465", "127.0.0.1:52211", "127.0.0.1:58941"};
-  std::vector<Json::ObjectPtr> output4 =
-      Json::Factory::LoadFromString(Json::Factory::listAsJsonString(list4))->asObjectArray();
-  EXPECT_EQ(3, output4.size());
-  EXPECT_EQ("127.0.0.1:46465", output4[0]->asString());
-  EXPECT_EQ("127.0.0.1:52211", output4[1]->asString());
-  EXPECT_EQ("127.0.0.1:58941", output4[2]->asString());
+  {
+    std::list<std::string> list = {};
+    Json::ObjectPtr json = Json::Factory::LoadFromString(Json::Factory::listAsJsonString(list));
+    std::vector<Json::ObjectPtr> output = json->asObjectArray();
+    EXPECT_TRUE(output.empty());
+  }
+
+  {
+    std::list<std::string> list = {"one"};
+    Json::ObjectPtr json = Json::Factory::LoadFromString(Json::Factory::listAsJsonString(list));
+    std::vector<Json::ObjectPtr> output = json->asObjectArray();
+    EXPECT_EQ(1, output.size());
+    EXPECT_EQ("one", output[0]->asString());
+  }
+
+  {
+    std::list<std::string> list = {"one", "two", "three", "four"};
+    Json::ObjectPtr json = Json::Factory::LoadFromString(Json::Factory::listAsJsonString(list));
+    std::vector<Json::ObjectPtr> output = json->asObjectArray();
+    EXPECT_EQ(4, output.size());
+    EXPECT_EQ("one", output[0]->asString());
+    EXPECT_EQ("two", output[1]->asString());
+    EXPECT_EQ("three", output[2]->asString());
+    EXPECT_EQ("four", output[3]->asString());
+  }
+
+  {
+    std::list<std::string> list = {"127.0.0.1:46465", "127.0.0.1:52211", "127.0.0.1:58941"};
+    Json::ObjectPtr json = Json::Factory::LoadFromString(Json::Factory::listAsJsonString(list));
+    std::vector<Json::ObjectPtr> output = json->asObjectArray();
+    EXPECT_EQ(3, output.size());
+    EXPECT_EQ("127.0.0.1:46465", output[0]->asString());
+    EXPECT_EQ("127.0.0.1:52211", output[1]->asString());
+    EXPECT_EQ("127.0.0.1:58941", output[2]->asString());
+  }
 }
 
 } // Json
diff --git a/test/integration/integration_admin_test.cc b/test/integration/integration_admin_test.cc
index 1811075c4784e..a175d5a3a3477 100644
--- a/test/integration/integration_admin_test.cc
+++ b/test/integration/integration_admin_test.cc
@@ -130,8 +130,8 @@ TEST_F(IntegrationTest, Admin) {
   EXPECT_TRUE(response->complete());
   EXPECT_STREQ("200", response->headers().Status()->value().c_str());
 
-  std::vector<Json::ObjectPtr> listener_info =
-      Json::Factory::LoadFromString(response->body())->asObjectArray();
+  Json::ObjectPtr json = Json::Factory::LoadFromString(response->body());
+  std::vector<Json::ObjectPtr> listener_info = json->asObjectArray();
   for (std::size_t index = 0; index < listener_info.size(); index++) {
     EXPECT_EQ(test_server_->server().getListenSocketByIndex(index)->localAddress()->asString(),
               listener_info[index]->asString());

From 1620325478eaa9817160573564694494dbe70c6b Mon Sep 17 00:00:00 2001
From: Henna Huang <henna@google.com>
Date: Thu, 27 Apr 2017 15:05:02 -0400
Subject: [PATCH 9/9] Respone to comments

---
 docs/operations/cli.rst             | 2 +-
 source/server/http/admin.cc         | 2 +-
 source/server/options_impl.cc       | 2 +-
 test/integration/hotrestart_test.sh | 6 +++---
 test/integration/server.h           | 3 +--
 test/server/options_impl_test.cc    | 3 ++-
 6 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/docs/operations/cli.rst b/docs/operations/cli.rst
index 044d852e2a5b5..fd88f987826be 100644
--- a/docs/operations/cli.rst
+++ b/docs/operations/cli.rst
@@ -10,7 +10,7 @@ following are the command line options that Envoy supports.
 
   *(required)* The path to the :ref:`JSON configuration file <config>`.
 
-.. option:: -a <path string>, --admin-address-path <path string>
+.. option:: --admin-address-path <path string>
 
   *(optional)* The output file path where the admin address and port will be written.
 
diff --git a/source/server/http/admin.cc b/source/server/http/admin.cc
index dc89ef6a327e9..6c9b9c83df074 100644
--- a/source/server/http/admin.cc
+++ b/source/server/http/admin.cc
@@ -359,7 +359,7 @@ AdminImpl::AdminImpl(const std::string& access_log_path, const std::string& prof
           {"/stats", "print server stats", MAKE_HANDLER(handlerStats)},
           {"/listeners", "print listener addresses", MAKE_HANDLER(handlerListenerInfo)}} {
 
-  if (address_out_path.length() > 0) {
+  if (!address_out_path.empty()) {
     std::ofstream address_out_file(address_out_path);
     if (!address_out_file) {
       log().critical("cannot open admin address output file {} for writing.", address_out_path);
diff --git a/source/server/options_impl.cc b/source/server/options_impl.cc
index c62e0b7075dfe..29206b5c57a27 100644
--- a/source/server/options_impl.cc
+++ b/source/server/options_impl.cc
@@ -29,7 +29,7 @@ OptionsImpl::OptionsImpl(int argc, char** argv, const std::string& hot_restart_v
                                         std::thread::hardware_concurrency(), "uint32_t", cmd);
   TCLAP::ValueArg<std::string> config_path("c", "config-path", "Path to configuration file", false,
                                            "", "string", cmd);
-  TCLAP::ValueArg<std::string> admin_address_path("a", "admin-address-path", "Admin address path",
+  TCLAP::ValueArg<std::string> admin_address_path("", "admin-address-path", "Admin address path",
                                                   false, "", "string", cmd);
   TCLAP::ValueArg<std::string> log_level("l", "log-level", log_levels_string, false,
                                          spdlog::level::level_names[default_log_level], "string",
diff --git a/test/integration/hotrestart_test.sh b/test/integration/hotrestart_test.sh
index 18fa813f48ba5..e86637cfb7d4d 100755
--- a/test/integration/hotrestart_test.sh
+++ b/test/integration/hotrestart_test.sh
@@ -19,7 +19,7 @@ echo "Starting epoch 0"
 ADMIN_ADDRESS_PATH_0="${TEST_TMPDIR}"/admin_0.address
 "${ENVOY_BIN}" -c "${HOT_RESTART_JSON}" \
     --restart-epoch 0 --base-id 1 --service-cluster cluster --service-node node \
-    -a "${ADMIN_ADDRESS_PATH_0}" &
+    --admin-address-path "${ADMIN_ADDRESS_PATH_0}" &
 
 FIRST_SERVER_PID=$!
 sleep 3
@@ -40,7 +40,7 @@ echo "Starting epoch 1"
 ADMIN_ADDRESS_PATH_1="${TEST_TMPDIR}"/admin_1.address
 "${ENVOY_BIN}" -c "${UPDATED_HOT_RESTART_JSON}" \
     --restart-epoch 1 --base-id 1 --service-cluster cluster --service-node node \
-    -a "${ADMIN_ADDRESS_PATH_1}" &
+    --admin-address-path "${ADMIN_ADDRESS_PATH_1}" &
 
 SECOND_SERVER_PID=$!
 # Wait for stat flushing
@@ -57,7 +57,7 @@ ADMIN_ADDRESS_PATH_2="${TEST_TMPDIR}"/admin_2.address
 echo "Starting epoch 2"
 "${ENVOY_BIN}" -c "${UPDATED_HOT_RESTART_JSON}" \
     --restart-epoch 2 --base-id 1 --service-cluster cluster --service-node node \
-    -a "${ADMIN_ADDRESS_PATH_2}" &
+    --admin-address-path "${ADMIN_ADDRESS_PATH_2}" &
 
 THIRD_SERVER_PID=$!
 sleep 3
diff --git a/test/integration/server.h b/test/integration/server.h
index e36feded38444..d8b4a135f1d2b 100644
--- a/test/integration/server.h
+++ b/test/integration/server.h
@@ -28,8 +28,7 @@ namespace Server {
  */
 class TestOptionsImpl : public Options {
 public:
-  TestOptionsImpl(const std::string& config_path)
-      : config_path_(config_path), admin_address_path_("") {}
+  TestOptionsImpl(const std::string& config_path) : config_path_(config_path) {}
 
   // Server::Options
   uint64_t baseId() override { return 0; }
diff --git a/test/server/options_impl_test.cc b/test/server/options_impl_test.cc
index a6783fbfac6a4..d54021140779f 100644
--- a/test/server/options_impl_test.cc
+++ b/test/server/options_impl_test.cc
@@ -28,7 +28,7 @@ TEST(OptionsImplDeathTest, HotRestartVersion) {
 
 TEST(OptionsImplTest, All) {
   std::unique_ptr<OptionsImpl> options = createOptionsImpl(
-      "envoy --concurrency 2 -c hello -a path --restart-epoch 1 -l info "
+      "envoy --concurrency 2 -c hello --admin-address-path path --restart-epoch 1 -l info "
       "--service-cluster cluster --service-node node --service-zone zone "
       "--file-flush-interval-msec 9000 --drain-time-s 60 --parent-shutdown-time-s 90");
   EXPECT_EQ(2U, options->concurrency());
@@ -48,4 +48,5 @@ TEST(OptionsImplTest, DefaultParams) {
   std::unique_ptr<OptionsImpl> options = createOptionsImpl("envoy -c hello");
   EXPECT_EQ(std::chrono::seconds(600), options->drainTime());
   EXPECT_EQ(std::chrono::seconds(900), options->parentShutdownTime());
+  EXPECT_EQ("", options->adminAddressPath());
 }