diff --git a/README.md b/README.md index 6adbd6a67..47927a487 100644 --- a/README.md +++ b/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 ] +bazel-bin/nighthawk_client [--latency-response-header-name ] [--stats-flush-interval ] [--stats-sinks ] ... [--no-duration] [--simple-warmup] @@ -84,9 +83,6 @@ format> Where: ---allow-envoy-deprecated-v2-api -DEPRECATED, not supported anymore. - --latency-response-header-name 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 diff --git a/api/client/options.proto b/api/client/options.proto index 3274dd015..041083217 100644 --- a/api/client/options.proto +++ b/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 } diff --git a/include/nighthawk/client/options.h b/include/nighthawk/client/options.h index a04292a86..0671ef039 100644 --- a/include/nighthawk/client/options.h +++ b/include/nighthawk/client/options.h @@ -75,7 +75,6 @@ class Options { virtual std::vector statsSinks() const PURE; virtual uint32_t statsFlushInterval() const PURE; virtual std::string responseHeaderWithLatencyInput() const PURE; - virtual bool allowEnvoyDeprecatedV2Api() const PURE; virtual absl::optional scheduled_start() const PURE; /** diff --git a/source/client/options_impl.cc b/source/client/options_impl.cc index 6cf10275b..e54cd5d77 100644 --- a/source/client/options_impl.cc +++ b/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::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( diff --git a/source/client/options_impl.h b/source/client/options_impl.h index b84d80d3e..7132aba01 100644 --- a/source/client/options_impl.h +++ b/source/client/options_impl.h @@ -93,7 +93,6 @@ class OptionsImpl : public Options, public Envoy::Logger::Loggable scheduled_start() const override { return scheduled_start_; } private: @@ -151,7 +150,6 @@ class OptionsImpl : public Options, public Envoy::Logger::Loggable stats_sinks_; uint32_t stats_flush_interval_{5}; std::string latency_response_header_name_; - bool allow_envoy_deprecated_v2_api_{false}; absl::optional scheduled_start_; }; diff --git a/source/client/process_impl.cc b/source/client/process_impl.cc index 4864a5b6d..69318a662 100644 --- a/source/client/process_impl.cc +++ b/source/client/process_impl.cc @@ -299,30 +299,10 @@ ProcessImpl::mergeWorkerStatistics(const std::vector& 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& 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( 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( diff --git a/source/client/process_impl.h b/source/client/process_impl.h index 5936007fc..81684815b 100644 --- a/source/client/process_impl.h +++ b/source/client/process_impl.h @@ -86,12 +86,6 @@ class ProcessImpl : public Process, public Envoy::Logger::Loggable& 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); /** diff --git a/test/integration/BUILD b/test/integration/BUILD index c9cd5ad68..1290e6090 100644 --- a/test/integration/BUILD +++ b/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", diff --git a/test/integration/configurations/nighthawk_http_origin_envoy_deprecated_v2_api.yaml b/test/integration/configurations/nighthawk_http_origin_envoy_deprecated_v2_api.yaml deleted file mode 100644 index 5e07954c7..000000000 --- a/test/integration/configurations/nighthawk_http_origin_envoy_deprecated_v2_api.yaml +++ /dev/null @@ -1,39 +0,0 @@ -# This file is intentionally using the v2 api: it is used to test support for that. -admin: - access_log_path: $tmpdir/nighthawk-test-server-admin-access.log - profile_path: $tmpdir/nighthawk-test-server.prof - address: - socket_address: { address: $server_ip, port_value: 0 } -static_resources: - listeners: - - address: - socket_address: - address: $server_ip - port_value: 0 - filter_chains: - - filters: - - name: envoy.http_connection_manager - config: - generate_request_id: false - codec_type: auto - stat_prefix: ingress_http - route_config: - name: local_route - virtual_hosts: - - name: service - domains: - - "*" - http_filters: - - name: test-server - config: - response_body_size: 10 - v3_response_headers: - - { header: { key: "x-nh", value: "1"}} - - name: envoy.router - config: - dynamic_stats: false -layered_runtime: - layers: - - name: static_layer - static_layer: - envoy.reloadable_features.enable_deprecated_v2_api: true diff --git a/test/integration/integration_test_fixtures.py b/test/integration/integration_test_fixtures.py index dacacd3b0..a9c931240 100644 --- a/test/integration/integration_test_fixtures.py +++ b/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.""" diff --git a/test/integration/nighthawk_test_server.py b/test/integration/nighthawk_test_server.py index 71653eeb7..f073980de 100644 --- a/test/integration/nighthawk_test_server.py +++ b/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.""" diff --git a/test/integration/test_integration_basics.py b/test/integration/test_integration_basics.py index affeb796b..78b07fdc2 100644 --- a/test/integration/test_integration_basics.py +++ b/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., diff --git a/test/mocks/client/mock_options.h b/test/mocks/client/mock_options.h index a6e85d42c..04fc35ec0 100644 --- a/test/mocks/client/mock_options.h +++ b/test/mocks/client/mock_options.h @@ -57,7 +57,6 @@ class MockOptions : public Options { MOCK_CONST_METHOD0(statsSinks, std::vector()); MOCK_CONST_METHOD0(statsFlushInterval, uint32_t()); MOCK_CONST_METHOD0(responseHeaderWithLatencyInput, std::string()); - MOCK_CONST_METHOD0(allowEnvoyDeprecatedV2Api, bool()); MOCK_CONST_METHOD0(scheduled_start, absl::optional()); }; diff --git a/test/options_test.cc b/test/options_test.cc index a1f6c143b..d65604b67 100644 --- a/test/options_test.cc +++ b/test/options_test.cc @@ -118,7 +118,7 @@ TEST_F(OptionsImplTest, AlmostAll) { "--experimental-h2-use-multiple-connections " "--experimental-h1-connection-reuse-strategy lru --label label1 --label label2 {} " "--simple-warmup --stats-sinks {} --stats-sinks {} --stats-flush-interval 10 " - "--latency-response-header-name zz --allow-envoy-deprecated-v2-api", + "--latency-response-header-name zz", client_name_, "{name:\"envoy.transport_sockets.tls\"," "typed_config:{\"@type\":\"type.googleapis.com/" @@ -193,7 +193,6 @@ TEST_F(OptionsImplTest, AlmostAll) { "183412668: \"envoy.config.metrics.v2.StatsSink\"\n", options->statsSinks()[1].DebugString()); EXPECT_EQ("zz", options->responseHeaderWithLatencyInput()); - EXPECT_TRUE(options->allowEnvoyDeprecatedV2Api()); // Check that our conversion to CommandLineOptionsPtr makes sense. CommandLineOptionsPtr cmd = options->toCommandLineOptions(); @@ -252,8 +251,6 @@ TEST_F(OptionsImplTest, AlmostAll) { EXPECT_TRUE(util(cmd->stats_sinks(0), options->statsSinks()[0])); EXPECT_TRUE(util(cmd->stats_sinks(1), options->statsSinks()[1])); EXPECT_EQ(cmd->latency_response_header_name().value(), options->responseHeaderWithLatencyInput()); - ASSERT_TRUE(cmd->has_allow_envoy_deprecated_v2_api()); - EXPECT_EQ(cmd->allow_envoy_deprecated_v2_api().value(), options->allowEnvoyDeprecatedV2Api()); // TODO(#433) Here and below, replace comparisons once we choose a proto diff. OptionsImpl options_from_proto(*cmd); std::string s1 = Envoy::MessageUtil::getYamlStringFromMessage( @@ -597,22 +594,6 @@ TEST_F(OptionsImplTest, PrefetchConnectionsFlag) { MalformedArgvException, "Couldn't find match for argument"); } -TEST_F(OptionsImplTest, AllowEnvoyDeprecatedV2ApiFlag) { - EXPECT_FALSE(TestUtility::createOptionsImpl(fmt::format("{} {}", client_name_, good_test_uri_)) - ->allowEnvoyDeprecatedV2Api()); - EXPECT_TRUE(TestUtility::createOptionsImpl(fmt::format("{} --allow-envoy-deprecated-v2-api {}", - client_name_, good_test_uri_)) - ->allowEnvoyDeprecatedV2Api()); - EXPECT_THROW_WITH_REGEX( - TestUtility::createOptionsImpl( - fmt::format("{} --allow-envoy-deprecated-v2-api 0 {}", client_name_, good_test_uri_)), - MalformedArgvException, "Couldn't find match for argument"); - EXPECT_THROW_WITH_REGEX( - TestUtility::createOptionsImpl( - fmt::format("{} --allow-envoy-deprecated-v2-api true {}", client_name_, good_test_uri_)), - MalformedArgvException, "Couldn't find match for argument"); -} - // Test --concurrency, which is a bit special. It's an int option, which also accepts 'auto' as // a value. We need to implement some stuff ourselves to get this to work, hence we don't run it // through the OptionsImplIntTest. diff --git a/test/process_test.cc b/test/process_test.cc index 4296ef770..7d1191790 100644 --- a/test/process_test.cc +++ b/test/process_test.cc @@ -9,7 +9,6 @@ #include "external/envoy/test/test_common/registry.h" #include "external/envoy/test/test_common/simulated_time_system.h" #include "external/envoy/test/test_common/utility.h" -#include "external/envoy_api/envoy/config/bootstrap/v3/bootstrap.pb.h" #include "common/uri_impl.h" @@ -181,38 +180,6 @@ TEST_P(ProcessTest, NoFlushWhenCancelExecutionBeforeLoadTestBegin) { EXPECT_EQ(numFlushes, 0); } -TEST(RuntimeConfiguration, allowEnvoyDeprecatedV2Api) { - envoy::config::bootstrap::v3::Bootstrap bootstrap; - EXPECT_EQ(bootstrap.DebugString(), ""); - ProcessImpl::allowEnvoyDeprecatedV2Api(bootstrap); - std::cerr << bootstrap.DebugString() << std::endl; - EXPECT_EQ(bootstrap.DebugString(), R"EOF(layered_runtime { - layers { - name: "admin layer" - admin_layer { - } - } - layers { - name: "static_layer" - static_layer { - fields { - key: "envoy.reloadable_features.allow_prefetch" - value { - string_value: "true" - } - } - fields { - key: "envoy.reloadable_features.enable_deprecated_v2_api" - value { - string_value: "true" - } - } - } - } -} -)EOF"); -} - /** * Fixture for executing the Nighthawk process with simulated time. */