Remove deprecated flag allow-envoy-deprecated-v2-api

README.md

@@ -43,8 +43,7 @@ bazel build -c opt //:nighthawk
 ```
 USAGE:
 
-bazel-bin/nighthawk_client  [--allow-envoy-deprecated-v2-api]
-[--latency-response-header-name <string>]
+bazel-bin/nighthawk_client  [--latency-response-header-name <string>]
 [--stats-flush-interval <uint32_t>]
 [--stats-sinks <string>] ...
 [--no-duration] [--simple-warmup]
@@ -84,9 +83,6 @@ format>
 
 Where:
 
---allow-envoy-deprecated-v2-api
-DEPRECATED, not supported anymore.
-
 --latency-response-header-name <string>
 Set an optional header name that will be returned in responses, whose
 values will be tracked in a latency histogram if set. Can be used in

api/client/options.proto

@@ -220,9 +220,9 @@ message CommandLineOptions {
   // "emit_previous_request_delta_in_response_header" to record elapsed time between request
   // arrivals.
   google.protobuf.StringValue latency_response_header_name = 36;
-  // Set to allow usage of the v2 api. (Not recommended, support will stop in Q1 2021).
-  google.protobuf.BoolValue allow_envoy_deprecated_v2_api = 38 [deprecated = true];
   // Provide an execution starting date and time. Optional, any value specified must be in the
   // future.
   google.protobuf.Timestamp scheduled_start = 105;
+
+  reserved 38; // deprecated
 }

include/nighthawk/client/options.h

@@ -75,7 +75,6 @@ class Options {
   virtual std::vector<envoy::config::metrics::v3::StatsSink> statsSinks() const PURE;
   virtual uint32_t statsFlushInterval() const PURE;
   virtual std::string responseHeaderWithLatencyInput() const PURE;
-  virtual bool allowEnvoyDeprecatedV2Api() const PURE;
 
   virtual absl::optional<Envoy::SystemTime> scheduled_start() const PURE;
   /**

source/client/options_impl.cc

@@ -315,9 +315,6 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv) {
       "Default: \"\"",
       false, "", "string", cmd);
 
-  TCLAP::SwitchArg allow_envoy_deprecated_v2_api("", "allow-envoy-deprecated-v2-api",
-                                                 "DEPRECATED, not supported anymore.", cmd);
-
   Utility::parseCommand(cmd, argc, argv);
 
   // --duration and --no-duration are mutually exclusive
@@ -450,7 +447,6 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv) {
   }
   TCLAP_SET_IF_SPECIFIED(stats_flush_interval, stats_flush_interval_);
   TCLAP_SET_IF_SPECIFIED(latency_response_header_name, latency_response_header_name_);
-  TCLAP_SET_IF_SPECIFIED(allow_envoy_deprecated_v2_api, allow_envoy_deprecated_v2_api_);
 
   // CLI-specific tests.
   // TODO(oschaaf): as per mergconflicts's remark, it would be nice to aggregate
@@ -656,8 +652,6 @@ OptionsImpl::OptionsImpl(const nighthawk::client::CommandLineOptions& options) {
   std::copy(options.labels().begin(), options.labels().end(), std::back_inserter(labels_));
   latency_response_header_name_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT(
       options, latency_response_header_name, latency_response_header_name_);
-  allow_envoy_deprecated_v2_api_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT(
-      options, allow_envoy_deprecated_v2_api, allow_envoy_deprecated_v2_api_);
   if (options.has_scheduled_start()) {
     const auto elapsed_since_epoch = std::chrono::duration_cast<std::chrono::microseconds>(
         std::chrono::nanoseconds(options.scheduled_start().nanos()) +
@@ -841,8 +835,6 @@ CommandLineOptionsPtr OptionsImpl::toCommandLineOptionsInternal() const {
   command_line_options->mutable_stats_flush_interval()->set_value(stats_flush_interval_);
   command_line_options->mutable_latency_response_header_name()->set_value(
       latency_response_header_name_);
-  command_line_options->mutable_allow_envoy_deprecated_v2_api()->set_value(
-      allow_envoy_deprecated_v2_api_);
   if (scheduled_start_.has_value()) {
     *(command_line_options->mutable_scheduled_start()) =
         Envoy::ProtobufUtil::TimeUtil::NanosecondsToTimestamp(

source/client/options_impl.h

@@ -93,7 +93,6 @@ class OptionsImpl : public Options, public Envoy::Logger::Loggable<Envoy::Logger
   std::string responseHeaderWithLatencyInput() const override {
     return latency_response_header_name_;
   };
-  bool allowEnvoyDeprecatedV2Api() const override { return allow_envoy_deprecated_v2_api_; }
   absl::optional<Envoy::SystemTime> scheduled_start() const override { return scheduled_start_; }
 
 private:
@@ -151,7 +150,6 @@ class OptionsImpl : public Options, public Envoy::Logger::Loggable<Envoy::Logger
   std::vector<envoy::config::metrics::v3::StatsSink> stats_sinks_;
   uint32_t stats_flush_interval_{5};
   std::string latency_response_header_name_;
-  bool allow_envoy_deprecated_v2_api_{false};
   absl::optional<Envoy::SystemTime> scheduled_start_;
 };
 

source/client/process_impl.cc

@@ -299,30 +299,10 @@ ProcessImpl::mergeWorkerStatistics(const std::vector<ClientWorkerPtr>& workers)
   return merged_statistics;
 }
 
-void ProcessImpl::allowEnvoyDeprecatedV2Api(envoy::config::bootstrap::v3::Bootstrap& bootstrap) {
-  auto* admin_layer = bootstrap.mutable_layered_runtime()->add_layers();
-  admin_layer->set_name("admin layer");
-  admin_layer->mutable_admin_layer();
-  envoy::config::bootstrap::v3::RuntimeLayer* runtime_layer =
-      bootstrap.mutable_layered_runtime()->add_layers();
-  runtime_layer->set_name("static_layer");
-  Envoy::ProtobufWkt::Value proto_true;
-  proto_true.set_string_value("true");
-  (*runtime_layer->mutable_static_layer()
-        ->mutable_fields())["envoy.reloadable_features.enable_deprecated_v2_api"] = proto_true;
-  (*runtime_layer->mutable_static_layer()
-        ->mutable_fields())["envoy.reloadable_features.allow_prefetch"] = proto_true;
-}
-
 void ProcessImpl::createBootstrapConfiguration(envoy::config::bootstrap::v3::Bootstrap& bootstrap,
                                                const std::vector<UriPtr>& uris,
                                                const UriPtr& request_source_uri,
-                                               int number_of_clusters,
-                                               bool allow_envoy_deprecated_v2_api) const {
-  if (allow_envoy_deprecated_v2_api) {
-    allowEnvoyDeprecatedV2Api(bootstrap);
-  }
-
+                                               int number_of_clusters) const {
   for (int i = 0; i < number_of_clusters; i++) {
     auto* cluster = bootstrap.mutable_static_resources()->add_clusters();
     RELEASE_ASSERT(!uris.empty(), "illegal configuration with zero endpoints");
@@ -509,8 +489,7 @@ bool ProcessImpl::runInternal(OutputCollector& collector, const std::vector<UriP
     int number_of_workers = determineConcurrency();
     shutdown_ = false;
     envoy::config::bootstrap::v3::Bootstrap bootstrap;
-    createBootstrapConfiguration(bootstrap, uris, request_source_uri, number_of_workers,
-                                 options_.allowEnvoyDeprecatedV2Api());
+    createBootstrapConfiguration(bootstrap, uris, request_source_uri, number_of_workers);
     // Needs to happen as early as possible (before createWorkers()) in the instantiation to preempt
     // the objects that require stats.
     if (!options_.statsSinks().empty()) {
@@ -522,7 +501,7 @@ bool ProcessImpl::runInternal(OutputCollector& collector, const std::vector<UriP
     store_root_.initializeThreading(*dispatcher_, tls_);
     runtime_singleton_ = std::make_unique<Envoy::Runtime::ScopedLoaderSingleton>(
         Envoy::Runtime::LoaderPtr{new Envoy::Runtime::LoaderImpl(
-            *dispatcher_, tls_, bootstrap.layered_runtime(), *local_info_, store_root_, generator_,
+            *dispatcher_, tls_, {}, *local_info_, store_root_, generator_,
             Envoy::ProtobufMessage::getStrictValidationVisitor(), *api_)});
     ssl_context_manager_ =
         std::make_unique<Envoy::Extensions::TransportSockets::Tls::ContextManagerImpl>(

source/client/process_impl.h

@@ -86,12 +86,6 @@ class ProcessImpl : public Process, public Envoy::Logger::Loggable<Envoy::Logger
 
   bool requestExecutionCancellation() override;
 
-  /**
-   * @param bootstrap The bootstrap that should have it's runtime configuration
-   * modified to allow for api v2 usage.
-   */
-  static void allowEnvoyDeprecatedV2Api(envoy::config::bootstrap::v3::Bootstrap& bootstrap);
-
 private:
   /**
    * @brief Creates a cluster for usage by a remote request source.
@@ -107,8 +101,7 @@ class ProcessImpl : public Process, public Envoy::Logger::Loggable<Envoy::Logger
                                   const Uri& uri) const;
   void createBootstrapConfiguration(envoy::config::bootstrap::v3::Bootstrap& bootstrap,
                                     const std::vector<UriPtr>& uris,
-                                    const UriPtr& request_source_uri, int number_of_workers,
-                                    bool allow_envoy_deprecated_v2_api) const;
+                                    const UriPtr& request_source_uri, int number_of_workers) const;
   void maybeCreateTracingDriver(const envoy::config::trace::v3::Tracing& configuration);
   void configureComponentLogLevels(spdlog::level::level_enum level);
   /**

test/integration/BUILD

@@ -13,7 +13,6 @@ py_library(
     name = "integration_test_base",
     data = [
         "configurations/nighthawk_http_origin.yaml",
-        "configurations/nighthawk_http_origin_envoy_deprecated_v2_api.yaml",
         "configurations/nighthawk_https_origin.yaml",
         "configurations/nighthawk_track_timings.yaml",
         "configurations/sni_origin.yaml",

test/integration/integration_test_fixtures.py

@@ -67,7 +67,7 @@ class IntegrationTestBase():
       about the currently executing test case.
   """
 
-  def __init__(self, request, server_config, backend_count=1, bootstrap_version_arg=None):
+  def __init__(self, request, server_config, backend_count=1):
     """Initialize the IntegrationTestBase instance.
 
     Args:
@@ -76,7 +76,6 @@ def __init__(self, request, server_config, backend_count=1, bootstrap_version_ar
         about the currently executing test case.
       server_config: path to the server configuration
       backend_count: number of Nighthawk Test Server backends to run, to allow testing MultiTarget mode
-      bootstrap_version_arg: An optional int, specify a bootstrap cli argument value for the test server binary. If None is specified, no bootstrap cli argment will be passed.
     """
     super(IntegrationTestBase, self).__init__()
     self.request = request
@@ -97,7 +96,6 @@ def __init__(self, request, server_config, backend_count=1, bootstrap_version_ar
     self._test_servers = []
     self._backend_count = backend_count
     self._test_id = ""
-    self._bootstrap_version_arg = bootstrap_version_arg
 
   # TODO(oschaaf): For the NH test server, add a way to let it determine a port by itself and pull that
   # out.
@@ -165,8 +163,7 @@ def _tryStartTestServers(self):
                                         self.ip_version,
                                         self.request,
                                         parameters=self.parameters,
-                                        tag=self.tag,
-                                        bootstrap_version_arg=self._bootstrap_version_arg)
+                                        tag=self.tag)
       if not test_server.start():
         return False
       self._test_servers.append(test_server)
@@ -322,25 +319,6 @@ def getTestServerRootUri(self):
     return super(HttpIntegrationTestBase, self).getTestServerRootUri(False)
 
 
-class HttpIntegrationTestBaseWithEnvoyDeprecatedV2Bootstrap(IntegrationTestBase):
-  """Base for running plain http tests against the Nighthawk test server.
-
-  NOTE: any script that consumes derivations of this, needs to also explicitly
-  import server_config, to avoid errors caused by the server_config not being found
-  by pytest.
-  """
-
-  def __init__(self, request, server_config):
-    """See base class."""
-    super(HttpIntegrationTestBaseWithEnvoyDeprecatedV2Bootstrap,
-          self).__init__(request, server_config, bootstrap_version_arg=2)
-
-  def getTestServerRootUri(self):
-    """See base class."""
-    return super(HttpIntegrationTestBaseWithEnvoyDeprecatedV2Bootstrap,
-                 self).getTestServerRootUri(False)
-
-
 class MultiServerHttpIntegrationTestBase(IntegrationTestBase):
   """Base for running plain http tests against multiple Nighthawk test servers."""
 

test/integration/nighthawk_test_server.py

@@ -113,16 +113,8 @@ class TestServerBase(object):
     tmpdir: String, indicates the location used to store outputs like logs.
   """
 
-  def __init__(self,
-               server_binary_path,
-               config_template_path,
-               server_ip,
-               ip_version,
-               request,
-               server_binary_config_path_arg,
-               parameters,
-               tag,
-               bootstrap_version_arg=None):
+  def __init__(self, server_binary_path, config_template_path, server_ip, ip_version, request,
+               server_binary_config_path_arg, parameters, tag):
     """Initialize a TestServerBase instance.
 
     Args:
@@ -134,7 +126,6 @@ def __init__(self,
         server_binary_config_path_arg (str): Specify the name of the CLI argument the test server binary uses to accept a configuration path.
         parameters (dict): Supply to provide configuration template parameter replacement values.
         tag (str): Supply to get recognizeable output locations.
-        bootstrap_version_arg (int, optional): specify a bootstrap cli argument value for the test server binary.
     """
     assert ip_version != IpVersion.UNKNOWN
     self.ip_version = ip_version
@@ -154,7 +145,6 @@ def __init__(self,
     self._parameterized_config_path = ""
     self._instance_id = str(random.randint(1, 1024 * 1024 * 1024))
     self._server_binary_config_path_arg = server_binary_config_path_arg
-    self._bootstrap_version_arg = bootstrap_version_arg
     self._prepareForExecution()
     self._request = request
 
@@ -195,8 +185,6 @@ def _serverThreadRunner(self):
         self._parameterized_config_path, "-l", "debug", "--base-id", self._instance_id,
         "--admin-address-path", self._admin_address_path, "--concurrency", "1"
     ]
-    if self._bootstrap_version_arg is not None:
-      args = args + ["--bootstrap-version", str(self._bootstrap_version_arg)]
 
     logging.info("Test server popen() args: %s" % str.join(" ", args))
     self._server_process = subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
@@ -313,8 +301,7 @@ def __init__(self,
                ip_version,
                request,
                parameters=dict(),
-               tag="",
-               bootstrap_version_arg=None):
+               tag=""):
     """Initialize a NighthawkTestServer instance.
 
     Args:
@@ -325,17 +312,9 @@ def __init__(self,
         request: The pytest `request` fixture used to determin information about the currently executed test.
         parameters (dictionary, optional): Directionary with replacement values for substition purposes in the server configuration template. Defaults to dict().
         tag (str, optional): Tags. Supply this to get recognizeable output locations. Defaults to "".
-        bootstrap_version_arg (String, optional): Specify a cli argument value for --bootstrap-version when running the server.
     """
-    super(NighthawkTestServer, self).__init__(server_binary_path,
-                                              config_template_path,
-                                              server_ip,
-                                              ip_version,
-                                              request,
-                                              "--config-path",
-                                              parameters,
-                                              tag,
-                                              bootstrap_version_arg=bootstrap_version_arg)
+    super(NighthawkTestServer, self).__init__(server_binary_path, config_template_path, server_ip,
+                                              ip_version, request, "--config-path", parameters, tag)
 
   def getCliVersionString(self):
     """Get the version string as written to the output by the CLI."""

test/integration/test_integration_basics.py

@@ -65,19 +65,6 @@ def test_http_h1(http_test_server_fixture):
   asserts.assertEqual(len(counters), 12)
 
 
-# TODO(oschaaf): This ought to work after the Envoy update.
-def DISABLED_test_nighthawk_client_v2_api_breaks_by_default(http_test_server_fixture):
-  """Test that the v2 api breaks us when it's not explicitly requested."""
-  _, _ = http_test_server_fixture.runNighthawkClient([
-      http_test_server_fixture.getTestServerRootUri(), "--duration", "100",
-      "--termination-predicate", "benchmark.pool_connection_failure:0", "--failure-predicate",
-      "foo:1", "--transport-socket",
-      "{name:\"envoy.transport_sockets.tls\",typed_config:{\"@type\":\"type.googleapis.com/envoy.api.v2.auth.UpstreamTlsContext\",\"common_tls_context\":{}}}"
-  ],
-                                                     expect_failure=True,
-                                                     as_json=False)
-
-
 def _mini_stress_test(fixture, args):
   # run a test with more rps then we can handle, and a very small client-side queue.
   # we should observe both lots of successfull requests as well as time spend in blocking mode.,

test/mocks/client/mock_options.h

@@ -57,7 +57,6 @@ class MockOptions : public Options {
   MOCK_CONST_METHOD0(statsSinks, std::vector<envoy::config::metrics::v3::StatsSink>());
   MOCK_CONST_METHOD0(statsFlushInterval, uint32_t());
   MOCK_CONST_METHOD0(responseHeaderWithLatencyInput, std::string());
-  MOCK_CONST_METHOD0(allowEnvoyDeprecatedV2Api, bool());
   MOCK_CONST_METHOD0(scheduled_start, absl::optional<Envoy::SystemTime>());
 };