From 4d78ff50a63d9d9b648bfd4f1e070a9dee86dbc0 Mon Sep 17 00:00:00 2001 From: l8huang Date: Sun, 25 Aug 2019 14:36:35 -0700 Subject: [PATCH 01/16] eds: avoid send too many ClusterLoadAssignment requests (#7976) During initializing secondary clusters, for each initialized cluster, a ClusterLoadAssignment request is sent to istio pilot with the cluster's name appended into request's resource_names list. With a huge number of clusters(e.g 10k clusters), this behavior slows down Envoy's initialization and consumes ton of memory. This change pauses ADS mux for ClusterLoadAssignment to avoid that. Risk Level: Medium Testing: tiny change, no test case added Fixes #7955 Signed-off-by: lhuang8 --- source/common/config/grpc_mux_impl.h | 2 +- source/common/upstream/cds_api_impl.cc | 3 + .../common/upstream/cluster_manager_impl.cc | 36 +++++++---- source/common/upstream/cluster_manager_impl.h | 7 ++- .../upstream/cluster_manager_impl_test.cc | 59 ++++++++++++++++++- 5 files changed, 92 insertions(+), 15 deletions(-) diff --git a/source/common/config/grpc_mux_impl.h b/source/common/config/grpc_mux_impl.h index d1aa49459053..ae245c48d7fd 100644 --- a/source/common/config/grpc_mux_impl.h +++ b/source/common/config/grpc_mux_impl.h @@ -125,7 +125,7 @@ class NullGrpcMuxImpl : public GrpcMux { } void pause(const std::string&) override {} void resume(const std::string&) override {} - bool paused(const std::string&) const override { NOT_REACHED_GCOVR_EXCL_LINE; } + bool paused(const std::string&) const override { return false; } }; } // namespace Config diff --git a/source/common/upstream/cds_api_impl.cc b/source/common/upstream/cds_api_impl.cc index 3ec334cf9b52..41f7ec4faca6 100644 --- a/source/common/upstream/cds_api_impl.cc +++ b/source/common/upstream/cds_api_impl.cc @@ -59,6 +59,9 @@ void CdsApiImpl::onConfigUpdate( cm_.adsMux().pause(Config::TypeUrl::get().ClusterLoadAssignment); Cleanup eds_resume([this] { cm_.adsMux().resume(Config::TypeUrl::get().ClusterLoadAssignment); }); + ENVOY_LOG(info, "cds: add {} cluster(s), remove {} cluster(s)", added_resources.size(), + removed_resources.size()); + std::vector exception_msgs; std::unordered_set cluster_names; bool any_applied = false; diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 1dd78f2dabfa..cad4bd02d679 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -103,6 +103,19 @@ void ClusterManagerInitHelper::removeCluster(Cluster& cluster) { maybeFinishInitialize(); } +void ClusterManagerInitHelper::initializeSecondaryClusters() { + started_secondary_initialize_ = true; + // Cluster::initialize() method can modify the list of secondary_init_clusters_ to remove + // the item currently being initialized, so we eschew range-based-for and do this complicated + // dance to increment the iterator before calling initialize. + for (auto iter = secondary_init_clusters_.begin(); iter != secondary_init_clusters_.end();) { + Cluster* cluster = *iter; + ++iter; + ENVOY_LOG(debug, "initializing secondary cluster {}", cluster->info()->name()); + cluster->initialize([cluster, this] { onClusterInit(*cluster); }); + } +} + void ClusterManagerInitHelper::maybeFinishInitialize() { // Do not do anything if we are still doing the initial static load or if we are waiting for // CDS initialize. @@ -121,15 +134,16 @@ void ClusterManagerInitHelper::maybeFinishInitialize() { if (!secondary_init_clusters_.empty()) { if (!started_secondary_initialize_) { ENVOY_LOG(info, "cm init: initializing secondary clusters"); - started_secondary_initialize_ = true; - // Cluster::initialize() method can modify the list of secondary_init_clusters_ to remove - // the item currently being initialized, so we eschew range-based-for and do this complicated - // dance to increment the iterator before calling initialize. - for (auto iter = secondary_init_clusters_.begin(); iter != secondary_init_clusters_.end();) { - Cluster* cluster = *iter; - ++iter; - ENVOY_LOG(debug, "initializing secondary cluster {}", cluster->info()->name()); - cluster->initialize([cluster, this] { onClusterInit(*cluster); }); + // If the first CDS response doesn't have any primary cluster, ClusterLoadAssignment + // should be already paused by CdsApiImpl::onConfigUpdate(). Need to check that to + // avoid double pause ClusterLoadAssignment. + if (cm_.adsMux().paused(Config::TypeUrl::get().ClusterLoadAssignment)) { + initializeSecondaryClusters(); + } else { + cm_.adsMux().pause(Config::TypeUrl::get().ClusterLoadAssignment); + Cleanup eds_resume( + [this] { cm_.adsMux().resume(Config::TypeUrl::get().ClusterLoadAssignment); }); + initializeSecondaryClusters(); } } @@ -188,7 +202,7 @@ ClusterManagerImpl::ClusterManagerImpl( : factory_(factory), runtime_(runtime), stats_(stats), tls_(tls.allocateSlot()), random_(random), bind_config_(bootstrap.cluster_manager().upstream_bind_config()), local_info_(local_info), cm_stats_(generateStats(stats)), - init_helper_([this](Cluster& cluster) { onClusterInit(cluster); }), + init_helper_(*this, [this](Cluster& cluster) { onClusterInit(cluster); }), config_tracker_entry_( admin.getConfigTracker().add("clusters", [this] { return dumpClusterConfigs(); })), time_source_(main_thread_dispatcher.timeSource()), dispatcher_(main_thread_dispatcher), @@ -496,7 +510,7 @@ bool ClusterManagerImpl::addOrUpdateCluster(const envoy::api::v2::Cluster& clust loadCluster(cluster, version_info, true, use_active_map ? active_clusters_ : warming_clusters_); if (use_active_map) { - ENVOY_LOG(info, "add/update cluster {} during init", cluster_name); + ENVOY_LOG(debug, "add/update cluster {} during init", cluster_name); auto& cluster_entry = active_clusters_.at(cluster_name); createOrUpdateThreadLocalCluster(*cluster_entry); init_helper_.addCluster(*cluster_entry->cluster_); diff --git a/source/common/upstream/cluster_manager_impl.h b/source/common/upstream/cluster_manager_impl.h index 6b8f205ed9b2..e45e9752372f 100644 --- a/source/common/upstream/cluster_manager_impl.h +++ b/source/common/upstream/cluster_manager_impl.h @@ -100,8 +100,9 @@ class ClusterManagerInitHelper : Logger::Loggable { * @param per_cluster_init_callback supplies the callback to call when a cluster has itself * initialized. The cluster manager can use this for post-init processing. */ - ClusterManagerInitHelper(const std::function& per_cluster_init_callback) - : per_cluster_init_callback_(per_cluster_init_callback) {} + ClusterManagerInitHelper(ClusterManager& cm, + const std::function& per_cluster_init_callback) + : cm_(cm), per_cluster_init_callback_(per_cluster_init_callback) {} enum class State { // Initial state. During this state all static clusters are loaded. Any phase 1 clusters @@ -128,9 +129,11 @@ class ClusterManagerInitHelper : Logger::Loggable { State state() const { return state_; } private: + void initializeSecondaryClusters(); void maybeFinishInitialize(); void onClusterInit(Cluster& cluster); + ClusterManager& cm_; std::function per_cluster_init_callback_; CdsApi* cds_{}; std::function initialized_callback_; diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index d8a85b0e13a2..f9632b12da76 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -42,6 +42,8 @@ #include "gtest/gtest.h" using testing::_; +using testing::ByRef; +using testing::Eq; using testing::InSequence; using testing::Invoke; using testing::Mock; @@ -2752,7 +2754,8 @@ class ClusterManagerInitHelperTest : public testing::Test { public: MOCK_METHOD1(onClusterInit, void(Cluster& cluster)); - ClusterManagerInitHelper init_helper_{[this](Cluster& cluster) { onClusterInit(cluster); }}; + NiceMock cm_; + ClusterManagerInitHelper init_helper_{cm_, [this](Cluster& cluster) { onClusterInit(cluster); }}; }; TEST_F(ClusterManagerInitHelperTest, ImmediateInitialize) { @@ -2824,6 +2827,60 @@ TEST_F(ClusterManagerInitHelperTest, UpdateAlreadyInitialized) { cluster2.initialize_callback_(); } +// If secondary clusters initialization triggered outside of CdsApiImpl::onConfigUpdate()'s +// callback flows, sending ClusterLoadAssignment should not be paused before calling +// ClusterManagerInitHelper::maybeFinishInitialize(). This case tests that +// ClusterLoadAssignment request is paused and resumed properly. +TEST_F(ClusterManagerInitHelperTest, InitSecondaryWithoutEdsPaused) { + InSequence s; + + ReadyWatcher cm_initialized; + init_helper_.setInitializedCb([&]() -> void { cm_initialized.ready(); }); + + NiceMock cluster1; + ON_CALL(cluster1, initializePhase()).WillByDefault(Return(Cluster::InitializePhase::Secondary)); + init_helper_.addCluster(cluster1); + + const auto& type_url = Config::TypeUrl::get().ClusterLoadAssignment; + EXPECT_CALL(cm_.ads_mux_, paused(Eq(ByRef(type_url)))).WillRepeatedly(Return(false)); + EXPECT_CALL(cm_.ads_mux_, pause(Eq(ByRef(type_url)))); + EXPECT_CALL(cluster1, initialize(_)); + EXPECT_CALL(cm_.ads_mux_, resume(Eq(ByRef(type_url)))); + + init_helper_.onStaticLoadComplete(); + + EXPECT_CALL(*this, onClusterInit(Ref(cluster1))); + EXPECT_CALL(cm_initialized, ready()); + cluster1.initialize_callback_(); +} + +// If secondary clusters initialization triggered inside of CdsApiImpl::onConfigUpdate()'s +// callback flows, that's, the CDS response didn't have any primary cluster, sending +// ClusterLoadAssignment should be already paused by CdsApiImpl::onConfigUpdate(). +// This case tests that ClusterLoadAssignment request isn't paused again. +TEST_F(ClusterManagerInitHelperTest, InitSecondaryWithEdsPaused) { + InSequence s; + + ReadyWatcher cm_initialized; + init_helper_.setInitializedCb([&]() -> void { cm_initialized.ready(); }); + + NiceMock cluster1; + ON_CALL(cluster1, initializePhase()).WillByDefault(Return(Cluster::InitializePhase::Secondary)); + init_helper_.addCluster(cluster1); + + const auto& type_url = Config::TypeUrl::get().ClusterLoadAssignment; + EXPECT_CALL(cm_.ads_mux_, paused(Eq(ByRef(type_url)))).WillRepeatedly(Return(true)); + EXPECT_CALL(cm_.ads_mux_, pause(Eq(ByRef(type_url)))).Times(0); + EXPECT_CALL(cluster1, initialize(_)); + EXPECT_CALL(cm_.ads_mux_, resume(Eq(ByRef(type_url)))).Times(0); + + init_helper_.onStaticLoadComplete(); + + EXPECT_CALL(*this, onClusterInit(Ref(cluster1))); + EXPECT_CALL(cm_initialized, ready()); + cluster1.initialize_callback_(); +} + TEST_F(ClusterManagerInitHelperTest, AddSecondaryAfterSecondaryInit) { InSequence s; From b0aca3045192bc88e7f0b5f94f2fb310285d1bfc Mon Sep 17 00:00:00 2001 From: Yikun Jiang Date: Mon, 26 Aug 2019 10:42:27 -0500 Subject: [PATCH 02/16] Set the bazel verison to 0.28.1 explicitly (#8037) In https://github.com/theopenlab/openlab-zuul-jobs/pull/622 , the OpenLab add the ability to set the bazel to specific version explicitly. This patch add the bazel role for the envoy job. Signed-off-by: Yikun Jiang --- .zuul/playbooks/envoy-build/run.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.zuul/playbooks/envoy-build/run.yaml b/.zuul/playbooks/envoy-build/run.yaml index 1ca8f832e08a..0087029e4a4f 100644 --- a/.zuul/playbooks/envoy-build/run.yaml +++ b/.zuul/playbooks/envoy-build/run.yaml @@ -3,6 +3,8 @@ roles: - role: config-gcc gcc_version: 7 + - role: config-bazel + bazel_version: 0.28.1 tasks: - name: Build envoy shell: From fc32b645a60a1cf70f889702cd8f24576be6db67 Mon Sep 17 00:00:00 2001 From: Henry Yang <4411287+HenryYYang@users.noreply.github.com> Date: Mon, 26 Aug 2019 09:10:53 -0700 Subject: [PATCH 03/16] Read_policy is not set correctly. (#8034) Add more integration test and additional checks in the unit tests. Signed-off-by: Henry Yang --- .../network/common/redis/client_impl.cc | 6 +-- .../network/redis_proxy/conn_pool_impl.cc | 4 +- .../redis/redis_cluster_integration_test.cc | 49 +++++++++++++++++-- .../redis_proxy/conn_pool_impl_test.cc | 18 +++++-- 4 files changed, 64 insertions(+), 13 deletions(-) diff --git a/source/extensions/filters/network/common/redis/client_impl.cc b/source/extensions/filters/network/common/redis/client_impl.cc index a2610417a873..6f941d223ca0 100644 --- a/source/extensions/filters/network/common/redis/client_impl.cc +++ b/source/extensions/filters/network/common/redis/client_impl.cc @@ -31,14 +31,14 @@ ConfigImpl::ConfigImpl( break; case envoy::config::filter::network::redis_proxy::v2:: RedisProxy_ConnPoolSettings_ReadPolicy_REPLICA: - read_policy_ = ReadPolicy::PreferMaster; + read_policy_ = ReadPolicy::Replica; break; case envoy::config::filter::network::redis_proxy::v2:: RedisProxy_ConnPoolSettings_ReadPolicy_PREFER_REPLICA: - read_policy_ = ReadPolicy::PreferMaster; + read_policy_ = ReadPolicy::PreferReplica; break; case envoy::config::filter::network::redis_proxy::v2::RedisProxy_ConnPoolSettings_ReadPolicy_ANY: - read_policy_ = ReadPolicy::PreferMaster; + read_policy_ = ReadPolicy::Any; break; default: NOT_REACHED_GCOVR_EXCL_LINE; diff --git a/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc b/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc index 2e1414e0252c..a097924ea734 100644 --- a/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc +++ b/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc @@ -225,8 +225,8 @@ InstanceImpl::ThreadLocalPool::makeRequest(const std::string& key, } const bool use_crc16 = is_redis_cluster_; - Clusters::Redis::RedisLoadBalancerContextImpl lb_context(key, parent_.config_.enableHashtagging(), - use_crc16, request); + Clusters::Redis::RedisLoadBalancerContextImpl lb_context( + key, parent_.config_.enableHashtagging(), use_crc16, request, parent_.config_.readPolicy()); Upstream::HostConstSharedPtr host = cluster_->loadBalancer().chooseHost(&lb_context); if (!host) { return nullptr; diff --git a/test/extensions/clusters/redis/redis_cluster_integration_test.cc b/test/extensions/clusters/redis/redis_cluster_integration_test.cc index 000928ef2441..bb1bfbd53d98 100644 --- a/test/extensions/clusters/redis/redis_cluster_integration_test.cc +++ b/test/extensions/clusters/redis/redis_cluster_integration_test.cc @@ -15,8 +15,7 @@ namespace { // This is a basic redis_proxy configuration with a single host // in the cluster. The load balancing policy must be set // to random for proper test operation. - -const std::string& testConfig() { +const std::string& listenerConfig() { CONSTRUCT_ON_FIRST_USE(std::string, R"EOF( admin: access_log_path: /dev/null @@ -40,7 +39,11 @@ const std::string& testConfig() { catch_all_route: cluster: cluster_0 settings: - op_timeout: 5s + op_timeout: 5s)EOF"); +} + +const std::string& clusterConfig() { + CONSTRUCT_ON_FIRST_USE(std::string, R"EOF( clusters: - name: cluster_0 lb_policy: CLUSTER_PROVIDED @@ -58,6 +61,16 @@ const std::string& testConfig() { )EOF"); } +const std::string& testConfig() { + CONSTRUCT_ON_FIRST_USE(std::string, listenerConfig() + clusterConfig()); +} + +const std::string& testConfigWithReadPolicy() { + CONSTRUCT_ON_FIRST_USE(std::string, listenerConfig() + R"EOF( + read_policy: REPLICA +)EOF" + clusterConfig()); +} + // This is the basic redis_proxy configuration with an upstream // authentication password specified. @@ -278,6 +291,13 @@ class RedisClusterWithAuthIntegrationTest : public RedisClusterIntegrationTest { : RedisClusterIntegrationTest(config, num_upstreams) {} }; +class RedisClusterWithReadPolicyIntegrationTest : public RedisClusterIntegrationTest { +public: + RedisClusterWithReadPolicyIntegrationTest(const std::string& config = testConfigWithReadPolicy(), + int num_upstreams = 3) + : RedisClusterIntegrationTest(config, num_upstreams) {} +}; + INSTANTIATE_TEST_SUITE_P(IpVersions, RedisClusterIntegrationTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), TestUtility::ipTestParamsToString); @@ -333,6 +353,29 @@ TEST_P(RedisClusterIntegrationTest, TwoSlot) { simpleRequestAndResponse(1, makeBulkStringArray({"get", "foo"}), "$3\r\nbar\r\n"); } +// This test sends simple "set foo" and "get foo" command from a fake +// downstream client through the proxy to a fake upstream +// Redis cluster with a single slot with master and replica. +// The envoy proxy is set with read_policy to read from replica, the expected result +// is that the set command will be sent to the master and the get command will be sent +// to the replica + +TEST_P(RedisClusterWithReadPolicyIntegrationTest, SingleSlotMasterReplicaReadReplica) { + random_index_ = 0; + + on_server_init_function_ = [this]() { + std::string cluster_slot_response = singleSlotMasterReplica( + fake_upstreams_[0]->localAddress()->ip(), fake_upstreams_[1]->localAddress()->ip()); + expectCallClusterSlot(random_index_, cluster_slot_response); + }; + + initialize(); + + // foo hashes to slot 12182 which has master node in upstream 0 and replica in upstream 1 + simpleRequestAndResponse(0, makeBulkStringArray({"set", "foo", "bar"}), ":1\r\n"); + simpleRequestAndResponse(1, makeBulkStringArray({"get", "foo"}), "$3\r\nbar\r\n"); +} + // This test sends a simple "get foo" command from a fake // downstream client through the proxy to a fake upstream // Redis cluster with a single slot with master and replica. diff --git a/test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc b/test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc index 22161d0e00fb..9dd249ec6002 100644 --- a/test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc +++ b/test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc @@ -161,7 +161,8 @@ class RedisConnPoolImplTest : public testing::Test, public Common::Redis::Client void testReadPolicy( envoy::config::filter::network::redis_proxy::v2::RedisProxy::ConnPoolSettings::ReadPolicy - read_policy) { + read_policy, + NetworkFilters::Common::Redis::Client::ReadPolicy expected_read_policy) { InSequence s; read_policy_ = read_policy; @@ -178,6 +179,9 @@ class RedisConnPoolImplTest : public testing::Test, public Common::Redis::Client EXPECT_EQ(context->computeHashKey().value(), MurmurHash::murmurHash2_64("hash_key")); EXPECT_EQ(context->metadataMatchCriteria(), nullptr); EXPECT_EQ(context->downstreamConnection(), nullptr); + auto redis_context = + dynamic_cast(context); + EXPECT_EQ(redis_context->readPolicy(), expected_read_policy); return cm_.thread_local_cluster_.lb_.host_; })); EXPECT_CALL(*this, create_(_)).WillOnce(Return(client)); @@ -279,13 +283,17 @@ TEST_F(RedisConnPoolImplTest, BasicWithAuthPassword) { TEST_F(RedisConnPoolImplTest, BasicWithReadPolicy) { testReadPolicy(envoy::config::filter::network::redis_proxy::v2:: - RedisProxy_ConnPoolSettings_ReadPolicy_PREFER_MASTER); + RedisProxy_ConnPoolSettings_ReadPolicy_PREFER_MASTER, + NetworkFilters::Common::Redis::Client::ReadPolicy::PreferMaster); testReadPolicy(envoy::config::filter::network::redis_proxy::v2:: - RedisProxy_ConnPoolSettings_ReadPolicy_REPLICA); + RedisProxy_ConnPoolSettings_ReadPolicy_REPLICA, + NetworkFilters::Common::Redis::Client::ReadPolicy::Replica); testReadPolicy(envoy::config::filter::network::redis_proxy::v2:: - RedisProxy_ConnPoolSettings_ReadPolicy_PREFER_REPLICA); + RedisProxy_ConnPoolSettings_ReadPolicy_PREFER_REPLICA, + NetworkFilters::Common::Redis::Client::ReadPolicy::PreferReplica); testReadPolicy( - envoy::config::filter::network::redis_proxy::v2::RedisProxy_ConnPoolSettings_ReadPolicy_ANY); + envoy::config::filter::network::redis_proxy::v2::RedisProxy_ConnPoolSettings_ReadPolicy_ANY, + NetworkFilters::Common::Redis::Client::ReadPolicy::Any); }; TEST_F(RedisConnPoolImplTest, Hashtagging) { From 5c2b34bdefb8ef6d3bd8426d289128847c0768b2 Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Mon, 26 Aug 2019 11:03:45 -0700 Subject: [PATCH 04/16] admin: fix /server_info hot restart version (#8022) Signed-off-by: Matt Klein --- api/envoy/admin/v2alpha/server_info.proto | 6 ++++-- source/server/http/admin.cc | 1 + test/server/http/admin_test.cc | 2 ++ test/server/options_impl_test.cc | 5 +++-- 4 files changed, 10 insertions(+), 4 deletions(-) diff --git a/api/envoy/admin/v2alpha/server_info.proto b/api/envoy/admin/v2alpha/server_info.proto index 7389af5b0860..78cc6fa7020a 100644 --- a/api/envoy/admin/v2alpha/server_info.proto +++ b/api/envoy/admin/v2alpha/server_info.proto @@ -36,6 +36,9 @@ message ServerInfo { // Uptime since the start of the first epoch. google.protobuf.Duration uptime_all_epochs = 4; + // Hot restart version. + string hot_restart_version = 5; + // Command line options the server is currently running with. CommandLineOptions command_line_options = 6; } @@ -82,8 +85,7 @@ message CommandLineOptions { // See :option:`--log-path` for details. string log_path = 11; - // See :option:`--hot-restart-version` for details. - bool hot_restart_version = 12; + reserved 12; // See :option:`--service-cluster` for details. string service_cluster = 13; diff --git a/source/server/http/admin.cc b/source/server/http/admin.cc index 5faacc1eb76d..71fd23d1118c 100644 --- a/source/server/http/admin.cc +++ b/source/server/http/admin.cc @@ -719,6 +719,7 @@ Http::Code AdminImpl::handlerServerInfo(absl::string_view, Http::HeaderMap& head time_t current_time = time(nullptr); envoy::admin::v2alpha::ServerInfo server_info; server_info.set_version(VersionInfo::version()); + server_info.set_hot_restart_version(server_.hotRestart().version()); server_info.set_state( Utility::serverState(server_.initManager().state(), server_.healthCheckFailed())); diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index 2c9e933ed881..24dbed7b348e 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -1239,6 +1239,7 @@ TEST_P(AdminInstanceTest, GetRequest) { })); NiceMock initManager; ON_CALL(server_, initManager()).WillByDefault(ReturnRef(initManager)); + ON_CALL(server_.hot_restart_, version()).WillByDefault(Return("foo_version")); { Http::HeaderMapImpl response_headers; @@ -1254,6 +1255,7 @@ TEST_P(AdminInstanceTest, GetRequest) { // values such as timestamps + Envoy version are tricky to test for. TestUtility::loadFromJson(body, server_info_proto); EXPECT_EQ(server_info_proto.state(), envoy::admin::v2alpha::ServerInfo::LIVE); + EXPECT_EQ(server_info_proto.hot_restart_version(), "foo_version"); EXPECT_EQ(server_info_proto.command_line_options().restart_epoch(), 2); EXPECT_EQ(server_info_proto.command_line_options().service_cluster(), "cluster"); } diff --git a/test/server/options_impl_test.cc b/test/server/options_impl_test.cc index bc4cc16f7fa7..4372585c4b48 100644 --- a/test/server/options_impl_test.cc +++ b/test/server/options_impl_test.cc @@ -245,14 +245,15 @@ TEST_F(OptionsImplTest, OptionsAreInSyncWithProto) { Server::CommandLineOptionsPtr command_line_options = options->toCommandLineOptions(); // Failure of this condition indicates that the server_info proto is not in sync with the options. // If an option is added/removed, please update server_info proto as well to keep it in sync. - // Currently the following 5 options are not defined in proto, hence the count differs by 5. + // Currently the following 7 options are not defined in proto, hence the count differs by 7. // 1. version - default TCLAP argument. // 2. help - default TCLAP argument. // 3. ignore_rest - default TCLAP argument. // 4. use-libevent-buffers - short-term override for rollout of new buffer implementation. // 5. allow-unknown-fields - deprecated alias of allow-unknown-static-fields. // 6. use-fake-symbol-table - short-term override for rollout of real symbol-table implementation. - EXPECT_EQ(options->count() - 6, command_line_options->GetDescriptor()->field_count()); + // 7. hot restart version - print the hot restart version and exit. + EXPECT_EQ(options->count() - 7, command_line_options->GetDescriptor()->field_count()); } TEST_F(OptionsImplTest, BadCliOption) { From fdd0e016301c07f2254edbbbabceecbd57ddb6a2 Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Mon, 26 Aug 2019 14:12:01 -0400 Subject: [PATCH 05/16] test: adding debug hints for integration test config failures (#8038) Risk Level: n/a (test only) Testing: manual Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk --- test/integration/integration.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/integration/integration.cc b/test/integration/integration.cc index f8fe1910abf1..595c78f1e24b 100644 --- a/test/integration/integration.cc +++ b/test/integration/integration.cc @@ -442,7 +442,8 @@ void BaseIntegrationTest::createGeneratedApiTestServer(const std::string& bootst if (!allow_lds_rejection) { RELEASE_ASSERT(test_server_->counter(rejected) == nullptr || test_server_->counter(rejected)->value() == 0, - "Lds update failed"); + "Lds update failed. For details, run test with -l trace and look for " + "\"Error adding/updating listener(s)\" in the logs."); } time_system_.sleep(std::chrono::milliseconds(10)); } From d39bd813a156989369597d69efd4e50489cfeebb Mon Sep 17 00:00:00 2001 From: danzh Date: Mon, 26 Aug 2019 18:10:46 -0400 Subject: [PATCH 06/16] udp_listener: refactor ActiveUdpListener creation (#7884) Signed-off-by: Dan Zhang --- api/envoy/api/v2/BUILD | 2 + api/envoy/api/v2/lds.proto | 11 +- api/envoy/api/v2/listener/BUILD | 17 +++ .../api/v2/listener/udp_listener_config.proto | 31 ++++++ docs/build.sh | 1 + docs/root/api-v2/listeners/listeners.rst | 1 + include/envoy/network/connection_handler.h | 50 +++++++++ include/envoy/network/listener.h | 7 ++ include/envoy/server/BUILD | 6 + .../envoy/server/active_udp_listener_config.h | 29 +++++ source/server/BUILD | 21 ++++ .../server/active_raw_udp_listener_config.cc | 26 +++++ .../server/active_raw_udp_listener_config.h | 31 ++++++ source/server/connection_handler_impl.cc | 91 +++++++--------- source/server/connection_handler_impl.h | 103 +++++++++--------- source/server/http/admin.h | 3 + source/server/listener_manager_impl.cc | 12 ++ source/server/listener_manager_impl.h | 4 + source/server/well_known_names.h | 21 ++++ .../proxy_protocol/proxy_protocol_test.cc | 2 + test/integration/BUILD | 1 + test/integration/fake_upstream.h | 4 + test/mocks/network/mocks.h | 1 + test/server/BUILD | 2 + test/server/connection_handler_test.cc | 11 ++ 25 files changed, 386 insertions(+), 102 deletions(-) create mode 100644 api/envoy/api/v2/listener/udp_listener_config.proto create mode 100644 include/envoy/server/active_udp_listener_config.h create mode 100644 source/server/active_raw_udp_listener_config.cc create mode 100644 source/server/active_raw_udp_listener_config.h create mode 100644 source/server/well_known_names.h diff --git a/api/envoy/api/v2/BUILD b/api/envoy/api/v2/BUILD index 48ddb2e13025..b86ca2a788bf 100644 --- a/api/envoy/api/v2/BUILD +++ b/api/envoy/api/v2/BUILD @@ -109,6 +109,7 @@ api_proto_library_internal( "//envoy/api/v2/core:address", "//envoy/api/v2/core:base", "//envoy/api/v2/listener", + "//envoy/api/v2/listener:udp_listener_config", ], ) @@ -120,6 +121,7 @@ api_go_grpc_library( "//envoy/api/v2/core:address_go_proto", "//envoy/api/v2/core:base_go_proto", "//envoy/api/v2/listener:listener_go_proto", + "//envoy/api/v2/listener:udp_listener_config_go_proto", ], ) diff --git a/api/envoy/api/v2/lds.proto b/api/envoy/api/v2/lds.proto index 195401341c96..643ac146213b 100644 --- a/api/envoy/api/v2/lds.proto +++ b/api/envoy/api/v2/lds.proto @@ -12,6 +12,7 @@ import "envoy/api/v2/core/address.proto"; import "envoy/api/v2/core/base.proto"; import "envoy/api/v2/discovery.proto"; import "envoy/api/v2/listener/listener.proto"; +import "envoy/api/v2/listener/udp_listener_config.proto"; import "google/api/annotations.proto"; import "google/protobuf/duration.proto"; @@ -44,7 +45,7 @@ service ListenerDiscoveryService { } } -// [#comment:next free field: 18] +// [#comment:next free field: 19] message Listener { // The unique name by which this listener is known. If no name is provided, // Envoy will allocate an internal UUID for the listener. If the listener is to be dynamically @@ -194,4 +195,12 @@ message Listener { // Specifies the intended direction of the traffic relative to the local Envoy. core.TrafficDirection traffic_direction = 16; + + // If the protocol in the listener socket address in :ref:`protocol + // ` is :ref:'UDP + // `, this field specifies the actual udp listener to create, + // i.e. :ref:`udp_listener_name + // ` = "raw_udp_listener" for + // creating a packet-oriented UDP listener. If not present, treat it as "raw_udp_listener". + listener.UdpListenerConfig udp_listener_config = 18; } diff --git a/api/envoy/api/v2/listener/BUILD b/api/envoy/api/v2/listener/BUILD index 9eb0c0ec982f..e539c4b8c090 100644 --- a/api/envoy/api/v2/listener/BUILD +++ b/api/envoy/api/v2/listener/BUILD @@ -22,3 +22,20 @@ api_go_proto_library( "//envoy/api/v2/core:base_go_proto", ], ) + +api_proto_library_internal( + name = "udp_listener_config", + srcs = ["udp_listener_config.proto"], + visibility = ["//envoy/api/v2:friends"], + deps = [ + "//envoy/api/v2/core:base", + ], +) + +api_go_proto_library( + name = "udp_listener_config", + proto = ":udp_listener_config", + deps = [ + "//envoy/api/v2/core:base_go_proto", + ], +) diff --git a/api/envoy/api/v2/listener/udp_listener_config.proto b/api/envoy/api/v2/listener/udp_listener_config.proto new file mode 100644 index 000000000000..88a2a35d3cfc --- /dev/null +++ b/api/envoy/api/v2/listener/udp_listener_config.proto @@ -0,0 +1,31 @@ +syntax = "proto3"; + +package envoy.api.v2.listener; + +option java_outer_classname = "ListenerProto"; +option java_multiple_files = true; +option java_package = "io.envoyproxy.envoy.api.v2.listener"; +option go_package = "listener"; +option csharp_namespace = "Envoy.Api.V2.ListenerNS"; +option ruby_package = "Envoy::Api::V2::ListenerNS"; + +import "google/protobuf/struct.proto"; +import "google/protobuf/any.proto"; + +// [#protodoc-title: Udp Listener Config] +// Listener :ref:`configuration overview ` + +message UdpListenerConfig { + // Used to look up UDP listener factory, matches "raw_udp_listener" or + // "quic_listener" to create a specific udp listener. + // If not specified, treat as "raw_udp_listener". + string udp_listener_name = 1; + + // Used to create a specific listener factory. To some factory, e.g. + // "raw_udp_listener", config is not needed. + oneof config_type { + google.protobuf.Struct config = 2; + + google.protobuf.Any typed_config = 3; + } +} diff --git a/docs/build.sh b/docs/build.sh index 7b89527bb24c..1b8cac8b06fa 100755 --- a/docs/build.sh +++ b/docs/build.sh @@ -82,6 +82,7 @@ PROTO_RST=" /envoy/api/v2/srds/envoy/api/v2/srds.proto.rst /envoy/api/v2/lds/envoy/api/v2/lds.proto.rst /envoy/api/v2/listener/listener/envoy/api/v2/listener/listener.proto.rst + /envoy/api/v2/listener/udp_listener_config/envoy/api/v2/listener/udp_listener_config.proto.rst /envoy/api/v2/ratelimit/ratelimit/envoy/api/v2/ratelimit/ratelimit.proto.rst /envoy/config/accesslog/v2/als/envoy/config/accesslog/v2/als.proto.rst /envoy/config/accesslog/v2/file/envoy/config/accesslog/v2/file.proto.rst diff --git a/docs/root/api-v2/listeners/listeners.rst b/docs/root/api-v2/listeners/listeners.rst index d933ccd32d66..6ed0279da7de 100644 --- a/docs/root/api-v2/listeners/listeners.rst +++ b/docs/root/api-v2/listeners/listeners.rst @@ -7,3 +7,4 @@ Listeners ../api/v2/lds.proto ../api/v2/listener/listener.proto + ../api/v2/listener/udp_listener_config.proto diff --git a/include/envoy/network/connection_handler.h b/include/envoy/network/connection_handler.h index 9a36aed1cc49..6c24a814db5f 100644 --- a/include/envoy/network/connection_handler.h +++ b/include/envoy/network/connection_handler.h @@ -68,9 +68,59 @@ class ConnectionHandler { * after they have been temporarily disabled. */ virtual void enableListeners() PURE; + + /** + * Used by ConnectionHandler to manage listeners. + */ + class ActiveListener { + public: + virtual ~ActiveListener() = default; + + /** + * @return the tag value as configured. + */ + virtual uint64_t listenerTag() PURE; + /** + * @return the actual Listener object. + */ + virtual Listener* listener() PURE; + /** + * Destroy the actual Listener it wraps. + */ + virtual void destroy() PURE; + }; + + using ActiveListenerPtr = std::unique_ptr; }; using ConnectionHandlerPtr = std::unique_ptr; +/** + * A registered factory interface to create different kinds of + * ActiveUdpListener. + */ +class ActiveUdpListenerFactory { +public: + virtual ~ActiveUdpListenerFactory() = default; + + /** + * Creates an ActiveUdpListener object and a corresponding UdpListener + * according to given config. + * @param parent is the owner of the created ActiveListener objects. + * @param dispatcher is used to create actual UDP listener. + * @param logger might not need to be passed in. + * TODO(danzh): investigate if possible to use statically defined logger in ActiveUdpListener + * implementation instead. + * @param config provides information needed to create ActiveUdpListener and + * UdpListener objects. + * @return the ActiveUdpListener created. + */ + virtual ConnectionHandler::ActiveListenerPtr + createActiveUdpListener(ConnectionHandler& parent, Event::Dispatcher& disptacher, + spdlog::logger& logger, Network::ListenerConfig& config) const PURE; +}; + +using ActiveUdpListenerFactoryPtr = std::unique_ptr; + } // namespace Network } // namespace Envoy diff --git a/include/envoy/network/listener.h b/include/envoy/network/listener.h index 53b06b01be65..451a76508581 100644 --- a/include/envoy/network/listener.h +++ b/include/envoy/network/listener.h @@ -14,6 +14,7 @@ namespace Envoy { namespace Network { class UdpListenerFilterManager; +class ActiveUdpListenerFactory; /** * A configuration for an individual listener. @@ -90,6 +91,12 @@ class ListenerConfig { * @return const std::string& the listener's name. */ virtual const std::string& name() const PURE; + + /** + * @return factory pointer if listening on UDP socket, otherwise return + * nullptr. + */ + virtual const ActiveUdpListenerFactory* udpListenerFactory() PURE; }; /** diff --git a/include/envoy/server/BUILD b/include/envoy/server/BUILD index 591868dde496..db070132fbcf 100644 --- a/include/envoy/server/BUILD +++ b/include/envoy/server/BUILD @@ -256,3 +256,9 @@ envoy_cc_library( "@envoy_api//envoy/config/trace/v2:trace_cc", ], ) + +envoy_cc_library( + name = "active_udp_listener_config_interface", + hdrs = ["active_udp_listener_config.h"], + deps = ["//include/envoy/network:connection_handler_interface"], +) diff --git a/include/envoy/server/active_udp_listener_config.h b/include/envoy/server/active_udp_listener_config.h new file mode 100644 index 000000000000..810d25add389 --- /dev/null +++ b/include/envoy/server/active_udp_listener_config.h @@ -0,0 +1,29 @@ +#pragma once + +#include "envoy/network/connection_handler.h" + +namespace Envoy { +namespace Server { + +/** + * Interface to create udp listener according to + * envoy::api::v2::listener::UdpListenerConfig.udp_listener_name. + */ +class ActiveUdpListenerConfigFactory { +public: + virtual ~ActiveUdpListenerConfigFactory() = default; + + /** + * Create an ActiveUdpListenerFactory object according to given message. + */ + virtual Network::ActiveUdpListenerFactoryPtr + createActiveUdpListenerFactory(const Protobuf::Message& message) PURE; + + /** + * Used to identify which udp listener to create: quic or raw udp. + */ + virtual std::string name() PURE; +}; + +} // namespace Server +} // namespace Envoy diff --git a/source/server/BUILD b/source/server/BUILD index bd9c72b56e63..5750846d1dca 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -64,6 +64,7 @@ envoy_cc_library( "//include/envoy/network:filter_interface", "//include/envoy/network:listen_socket_interface", "//include/envoy/network:listener_interface", + "//include/envoy/server:active_udp_listener_config_interface", "//include/envoy/server:listener_manager_interface", "//include/envoy/stats:timespan", "//source/common/common:linked_object", @@ -260,6 +261,8 @@ envoy_cc_library( ":filter_chain_manager_lib", ":lds_api_lib", ":transport_socket_config_lib", + ":well_known_names_lib", + "//include/envoy/server:active_udp_listener_config_interface", "//include/envoy/server:filter_config_interface", "//include/envoy/server:listener_manager_interface", "//include/envoy/server:transport_socket_config_interface", @@ -441,3 +444,21 @@ envoy_cc_library( "//include/envoy/server:transport_socket_config_interface", ], ) + +envoy_cc_library( + name = "well_known_names_lib", + hdrs = ["well_known_names.h"], + deps = ["//source/common/singleton:const_singleton"], +) + +envoy_cc_library( + name = "active_raw_udp_listener_config", + srcs = ["active_raw_udp_listener_config.cc"], + hdrs = ["active_raw_udp_listener_config.h"], + deps = [ + ":connection_handler_lib", + ":well_known_names_lib", + "//include/envoy/registry", + "//include/envoy/server:active_udp_listener_config_interface", + ], +) diff --git a/source/server/active_raw_udp_listener_config.cc b/source/server/active_raw_udp_listener_config.cc new file mode 100644 index 000000000000..f60074dd4ea0 --- /dev/null +++ b/source/server/active_raw_udp_listener_config.cc @@ -0,0 +1,26 @@ +#include "server/active_raw_udp_listener_config.h" + +#include "server/connection_handler_impl.h" +#include "server/well_known_names.h" + +namespace Envoy { +namespace Server { + +Network::ConnectionHandler::ActiveListenerPtr ActiveRawUdpListenerFactory::createActiveUdpListener( + Network::ConnectionHandler& /*parent*/, Event::Dispatcher& dispatcher, + spdlog::logger& /*logger*/, Network::ListenerConfig& config) const { + return std::make_unique(dispatcher, config); +} + +Network::ActiveUdpListenerFactoryPtr +ActiveRawUdpListenerConfigFactory::createActiveUdpListenerFactory( + const Protobuf::Message& /*message*/) { + return std::make_unique(); +} + +std::string ActiveRawUdpListenerConfigFactory::name() { return UdpListenerNames::get().RawUdp; } + +REGISTER_FACTORY(ActiveRawUdpListenerConfigFactory, Server::ActiveUdpListenerConfigFactory); + +} // namespace Server +} // namespace Envoy diff --git a/source/server/active_raw_udp_listener_config.h b/source/server/active_raw_udp_listener_config.h new file mode 100644 index 000000000000..157ff28f6b41 --- /dev/null +++ b/source/server/active_raw_udp_listener_config.h @@ -0,0 +1,31 @@ +#pragma once + +#include "envoy/network/connection_handler.h" +#include "envoy/registry/registry.h" +#include "envoy/server/active_udp_listener_config.h" + +namespace Envoy { +namespace Server { + +class ActiveRawUdpListenerFactory : public Network::ActiveUdpListenerFactory { +public: + Network::ConnectionHandler::ActiveListenerPtr + createActiveUdpListener(Network::ConnectionHandler& parent, Event::Dispatcher& disptacher, + spdlog::logger& logger, Network::ListenerConfig& config) const override; +}; + +// This class uses a protobuf config to create a UDP listener factory which +// creates a Server::ConnectionHandlerImpl::ActiveUdpListener. +// This is the default UDP listener if not specified in config. +class ActiveRawUdpListenerConfigFactory : public ActiveUdpListenerConfigFactory { +public: + Network::ActiveUdpListenerFactoryPtr + createActiveUdpListenerFactory(const Protobuf::Message&) override; + + std::string name() override; +}; + +DECLARE_FACTORY(ActiveRawUdpListenerConfigFactory); + +} // namespace Server +} // namespace Envoy diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc index b7e08a31441b..31fb7e64f2d0 100644 --- a/source/server/connection_handler_impl.cc +++ b/source/server/connection_handler_impl.cc @@ -18,28 +18,27 @@ ConnectionHandlerImpl::ConnectionHandlerImpl(spdlog::logger& logger, Event::Disp : logger_(logger), dispatcher_(dispatcher), disable_listeners_(false) {} void ConnectionHandlerImpl::addListener(Network::ListenerConfig& config) { - ActiveListenerBasePtr listener; + Network::ConnectionHandler::ActiveListenerPtr listener; Network::Address::SocketType socket_type = config.socket().socketType(); if (socket_type == Network::Address::SocketType::Stream) { - ActiveTcpListenerPtr tcp(new ActiveTcpListener(*this, config)); - listener = std::move(tcp); + listener = std::make_unique(*this, config); } else { ASSERT(socket_type == Network::Address::SocketType::Datagram, "Only datagram/stream listener supported"); - ActiveUdpListenerPtr udp(new ActiveUdpListener(*this, config)); - listener = std::move(udp); + listener = + config.udpListenerFactory()->createActiveUdpListener(*this, dispatcher_, logger_, config); } if (disable_listeners_) { - listener->listener_->disable(); + listener->listener()->disable(); } listeners_.emplace_back(config.socket().localAddress(), std::move(listener)); } void ConnectionHandlerImpl::removeListeners(uint64_t listener_tag) { for (auto listener = listeners_.begin(); listener != listeners_.end();) { - if (listener->second->listener_tag_ == listener_tag) { + if (listener->second->listenerTag() == listener_tag) { listener = listeners_.erase(listener); } else { ++listener; @@ -49,29 +48,29 @@ void ConnectionHandlerImpl::removeListeners(uint64_t listener_tag) { void ConnectionHandlerImpl::stopListeners(uint64_t listener_tag) { for (auto& listener : listeners_) { - if (listener.second->listener_tag_ == listener_tag) { - listener.second->listener_.reset(); + if (listener.second->listenerTag() == listener_tag) { + listener.second->destroy(); } } } void ConnectionHandlerImpl::stopListeners() { for (auto& listener : listeners_) { - listener.second->listener_.reset(); + listener.second->destroy(); } } void ConnectionHandlerImpl::disableListeners() { disable_listeners_ = true; for (auto& listener : listeners_) { - listener.second->listener_->disable(); + listener.second->listener()->disable(); } } void ConnectionHandlerImpl::enableListeners() { disable_listeners_ = false; for (auto& listener : listeners_) { - listener.second->listener_->enable(); + listener.second->listener()->enable(); } } @@ -84,11 +83,9 @@ void ConnectionHandlerImpl::ActiveTcpListener::removeConnection(ActiveConnection parent_.num_connections_--; } -ConnectionHandlerImpl::ActiveListenerBase::ActiveListenerBase(ConnectionHandlerImpl& parent, - Network::ListenerPtr&& listener, - Network::ListenerConfig& config) - : parent_(parent), listener_(std::move(listener)), - stats_(generateStats(config.listenerScope())), +ConnectionHandlerImpl::ActiveListenerImplBase::ActiveListenerImplBase( + Network::ListenerPtr&& listener, Network::ListenerConfig& config) + : listener_(std::move(listener)), stats_(generateStats(config.listenerScope())), listener_filters_timeout_(config.listenerFiltersTimeout()), continue_on_listener_filters_timeout_(config.continueOnListenerFiltersTimeout()), listener_tag_(config.listenerTag()), config_(config) {} @@ -104,7 +101,7 @@ ConnectionHandlerImpl::ActiveTcpListener::ActiveTcpListener(ConnectionHandlerImp ConnectionHandlerImpl::ActiveTcpListener::ActiveTcpListener(ConnectionHandlerImpl& parent, Network::ListenerPtr&& listener, Network::ListenerConfig& config) - : ConnectionHandlerImpl::ActiveListenerBase(parent, std::move(listener), config) {} + : ConnectionHandlerImpl::ActiveListenerImplBase(std::move(listener), config), parent_(parent) {} ConnectionHandlerImpl::ActiveTcpListener::~ActiveTcpListener() { // Purge sockets that have not progressed to connections. This should only happen when @@ -123,22 +120,22 @@ ConnectionHandlerImpl::ActiveTcpListener::~ActiveTcpListener() { Network::Listener* ConnectionHandlerImpl::findListenerByAddress(const Network::Address::Instance& address) { - ActiveListenerBase* listener = findActiveListenerByAddress(address); - return listener ? listener->listener_.get() : nullptr; + Network::ConnectionHandler::ActiveListener* listener = findActiveListenerByAddress(address); + return listener ? listener->listener() : nullptr; } -ConnectionHandlerImpl::ActiveListenerBase* +Network::ConnectionHandler::ActiveListener* ConnectionHandlerImpl::findActiveListenerByAddress(const Network::Address::Instance& address) { // This is a linear operation, may need to add a map to improve performance. // However, linear performance might be adequate since the number of listeners is small. // We do not return stopped listeners. - auto listener_it = std::find_if( - listeners_.begin(), listeners_.end(), - [&address]( - const std::pair& p) { - return p.second->listener_ != nullptr && p.first->type() == Network::Address::Type::Ip && - *(p.first) == address; - }); + auto listener_it = + std::find_if(listeners_.begin(), listeners_.end(), + [&address](const std::pair& p) { + return p.second->listener() != nullptr && + p.first->type() == Network::Address::Type::Ip && *(p.first) == address; + }); // If there is exact address match, return the corresponding listener. if (listener_it != listeners_.end()) { @@ -150,9 +147,9 @@ ConnectionHandlerImpl::findActiveListenerByAddress(const Network::Address::Insta // TODO(wattli): consolidate with previous search for more efficiency. listener_it = std::find_if( listeners_.begin(), listeners_.end(), - [&address]( - const std::pair& p) { - return p.second->listener_ != nullptr && p.first->type() == Network::Address::Type::Ip && + [&address](const std::pair& p) { + return p.second->listener() != nullptr && p.first->type() == Network::Address::Type::Ip && p.first->ip()->port() == address.ip()->port() && p.first->ip()->isAnyAddress(); }); return (listener_it != listeners_.end()) ? listener_it->second.get() : nullptr; @@ -210,7 +207,7 @@ void ConnectionHandlerImpl::ActiveSocket::continueFilterChain(bool success) { void ConnectionHandlerImpl::ActiveSocket::newConnection() { // Check if the socket may need to be redirected to another listener. - ActiveListenerBase* new_listener = nullptr; + ConnectionHandler::ActiveListener* new_listener = nullptr; if (hand_off_restored_destination_connections_ && socket_->localAddressRestored()) { // Find a listener associated with the original destination address. @@ -318,15 +315,12 @@ ListenerStats ConnectionHandlerImpl::generateStats(Stats::Scope& scope) { return {ALL_LISTENER_STATS(POOL_COUNTER(scope), POOL_GAUGE(scope), POOL_HISTOGRAM(scope))}; } -ConnectionHandlerImpl::ActiveUdpListener::ActiveUdpListener(ConnectionHandlerImpl& parent, - Network::ListenerConfig& config) - : ActiveUdpListener(parent, parent.dispatcher_.createUdpListener(config.socket(), *this), - config) {} +ActiveUdpListener::ActiveUdpListener(Event::Dispatcher& dispatcher, Network::ListenerConfig& config) + : ActiveUdpListener(dispatcher.createUdpListener(config.socket(), *this), config) {} -ConnectionHandlerImpl::ActiveUdpListener::ActiveUdpListener(ConnectionHandlerImpl& parent, - Network::ListenerPtr&& listener, - Network::ListenerConfig& config) - : ConnectionHandlerImpl::ActiveListenerBase(parent, std::move(listener), config), +ActiveUdpListener::ActiveUdpListener(Network::ListenerPtr&& listener, + Network::ListenerConfig& config) + : ConnectionHandlerImpl::ActiveListenerImplBase(std::move(listener), config), udp_listener_(dynamic_cast(listener_.get())), read_filter_(nullptr) { // TODO(sumukhs): Try to avoid dynamic_cast by coming up with a better interface design ASSERT(udp_listener_ != nullptr, ""); @@ -342,32 +336,27 @@ ConnectionHandlerImpl::ActiveUdpListener::ActiveUdpListener(ConnectionHandlerImp } } -void ConnectionHandlerImpl::ActiveUdpListener::onData(Network::UdpRecvData& data) { - read_filter_->onData(data); -} +void ActiveUdpListener::onData(Network::UdpRecvData& data) { read_filter_->onData(data); } -void ConnectionHandlerImpl::ActiveUdpListener::onWriteReady(const Network::Socket&) { +void ActiveUdpListener::onWriteReady(const Network::Socket&) { // TODO(sumukhs): This is not used now. When write filters are implemented, this is a // trigger to invoke the on write ready API on the filters which is when they can write // data } -void ConnectionHandlerImpl::ActiveUdpListener::onReceiveError( - const Network::UdpListenerCallbacks::ErrorCode&, Api::IoError::IoErrorCode) { +void ActiveUdpListener::onReceiveError(const Network::UdpListenerCallbacks::ErrorCode&, + Api::IoError::IoErrorCode) { // TODO(sumukhs): Determine what to do on receive error. // Would the filters need to know on error? Can't foresee a scenario where they // would take an action } -void ConnectionHandlerImpl::ActiveUdpListener::addReadFilter( - Network::UdpListenerReadFilterPtr&& filter) { +void ActiveUdpListener::addReadFilter(Network::UdpListenerReadFilterPtr&& filter) { ASSERT(read_filter_ == nullptr, "Cannot add a 2nd UDP read filter"); read_filter_ = std::move(filter); } -Network::UdpListener& ConnectionHandlerImpl::ActiveUdpListener::udpListener() { - return *udp_listener_; -} +Network::UdpListener& ActiveUdpListener::udpListener() { return *udp_listener_; } } // namespace Server } // namespace Envoy diff --git a/source/server/connection_handler_impl.h b/source/server/connection_handler_impl.h index 7ca0088abe61..4c1354e29870 100644 --- a/source/server/connection_handler_impl.h +++ b/source/server/connection_handler_impl.h @@ -12,6 +12,7 @@ #include "envoy/network/filter.h" #include "envoy/network/listen_socket.h" #include "envoy/network/listener.h" +#include "envoy/server/active_udp_listener_config.h" #include "envoy/server/listener_manager.h" #include "envoy/stats/scope.h" #include "envoy/stats/timespan.h" @@ -59,33 +60,21 @@ class ConnectionHandlerImpl : public Network::ConnectionHandler, NonCopyable { Network::Listener* findListenerByAddress(const Network::Address::Instance& address) override; -private: - struct ActiveListenerBase; - using ActiveListenerBasePtr = std::unique_ptr; - - struct ActiveTcpListener; - using ActiveTcpListenerPtr = std::unique_ptr; - - struct ActiveUdpListener; - using ActiveUdpListenerPtr = std::unique_ptr; - - ActiveListenerBase* findActiveListenerByAddress(const Network::Address::Instance& address); - - struct ActiveConnection; - using ActiveConnectionPtr = std::unique_ptr; - struct ActiveSocket; - using ActiveSocketPtr = std::unique_ptr; + Network::ConnectionHandler::ActiveListener* + findActiveListenerByAddress(const Network::Address::Instance& address); /** * Wrapper for an active listener owned by this handler. */ - struct ActiveListenerBase { - ActiveListenerBase(ConnectionHandlerImpl& parent, Network::ListenerPtr&& listener, - Network::ListenerConfig& config); + class ActiveListenerImplBase : public Network::ConnectionHandler::ActiveListener { + public: + ActiveListenerImplBase(Network::ListenerPtr&& listener, Network::ListenerConfig& config); - virtual ~ActiveListenerBase() = default; + // Network::ConnectionHandler::ActiveListener. + uint64_t listenerTag() override { return listener_tag_; } + Network::Listener* listener() override { return listener_.get(); } + void destroy() override { listener_.reset(); } - ConnectionHandlerImpl& parent_; Network::ListenerPtr listener_; ListenerStats stats_; const std::chrono::milliseconds listener_filters_timeout_; @@ -94,38 +83,19 @@ class ConnectionHandlerImpl : public Network::ConnectionHandler, NonCopyable { Network::ListenerConfig& config_; }; - /** - * Wrapper for an active udp listener owned by this handler. - */ - struct ActiveUdpListener : public Network::UdpListenerCallbacks, - public ActiveListenerBase, - public Network::UdpListenerFilterManager, - public Network::UdpReadFilterCallbacks { - ActiveUdpListener(ConnectionHandlerImpl& parent, Network::ListenerConfig& config); - - ActiveUdpListener(ConnectionHandlerImpl& parent, Network::ListenerPtr&& listener, - Network::ListenerConfig& config); - - // Network::UdpListenerCallbacks - void onData(Network::UdpRecvData& data) override; - void onWriteReady(const Network::Socket& socket) override; - void onReceiveError(const Network::UdpListenerCallbacks::ErrorCode& error_code, - Api::IoError::IoErrorCode err) override; - - // Network::UdpListenerFilterManager - void addReadFilter(Network::UdpListenerReadFilterPtr&& filter) override; - - // Network::UdpReadFilterCallbacks - Network::UdpListener& udpListener() override; - - Network::UdpListener* udp_listener_; - Network::UdpListenerReadFilterPtr read_filter_; - }; +private: + class ActiveTcpListener; + using ActiveTcpListenerPtr = std::unique_ptr; + struct ActiveConnection; + using ActiveConnectionPtr = std::unique_ptr; + struct ActiveSocket; + using ActiveSocketPtr = std::unique_ptr; /** * Wrapper for an active tcp listener owned by this handler. */ - struct ActiveTcpListener : public Network::ListenerCallbacks, public ActiveListenerBase { + class ActiveTcpListener : public Network::ListenerCallbacks, public ActiveListenerImplBase { + public: ActiveTcpListener(ConnectionHandlerImpl& parent, Network::ListenerConfig& config); ActiveTcpListener(ConnectionHandlerImpl& parent, Network::ListenerPtr&& listener, @@ -149,6 +119,7 @@ class ConnectionHandlerImpl : public Network::ConnectionHandler, NonCopyable { */ void newConnection(Network::ConnectionSocketPtr&& socket); + ConnectionHandlerImpl& parent_; std::list sockets_; std::list connections_; }; @@ -225,10 +196,42 @@ class ConnectionHandlerImpl : public Network::ConnectionHandler, NonCopyable { spdlog::logger& logger_; Event::Dispatcher& dispatcher_; - std::list> listeners_; + std::list> + listeners_; std::atomic num_connections_{}; bool disable_listeners_; }; +/** + * Wrapper for an active udp listener owned by this handler. + * TODO(danzh): rename to ActiveRawUdpListener. + */ +class ActiveUdpListener : public Network::UdpListenerCallbacks, + public ConnectionHandlerImpl::ActiveListenerImplBase, + public Network::UdpListenerFilterManager, + public Network::UdpReadFilterCallbacks { +public: + ActiveUdpListener(Event::Dispatcher& dispatcher, Network::ListenerConfig& config); + + ActiveUdpListener(Network::ListenerPtr&& listener, Network::ListenerConfig& config); + + // Network::UdpListenerCallbacks + void onData(Network::UdpRecvData& data) override; + void onWriteReady(const Network::Socket& socket) override; + void onReceiveError(const Network::UdpListenerCallbacks::ErrorCode& error_code, + Api::IoError::IoErrorCode err) override; + + // Network::UdpListenerFilterManager + void addReadFilter(Network::UdpListenerReadFilterPtr&& filter) override; + + // Network::UdpReadFilterCallbacks + Network::UdpListener& udpListener() override; + +private: + Network::UdpListener* udp_listener_; + Network::UdpListenerReadFilterPtr read_filter_; +}; + } // namespace Server } // namespace Envoy diff --git a/source/server/http/admin.h b/source/server/http/admin.h index 09bf1ecedf4c..ac01902068a7 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -320,6 +320,9 @@ class AdminImpl : public Admin, Stats::Scope& listenerScope() override { return *scope_; } uint64_t listenerTag() const override { return 0; } const std::string& name() const override { return name_; } + const Network::ActiveUdpListenerFactory* udpListenerFactory() override { + NOT_REACHED_GCOVR_EXCL_LINE; + } AdminImpl& parent_; const std::string name_; diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index 0f2ec74ee62b..1b305d3157fd 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -4,6 +4,7 @@ #include "envoy/admin/v2alpha/config_dump.pb.h" #include "envoy/registry/registry.h" +#include "envoy/server/active_udp_listener_config.h" #include "envoy/server/transport_socket_config.h" #include "envoy/stats/scope.h" @@ -23,6 +24,7 @@ #include "server/drain_manager_impl.h" #include "server/filter_chain_manager_impl.h" #include "server/transport_socket_config_impl.h" +#include "server/well_known_names.h" #include "extensions/filters/listener/well_known_names.h" #include "extensions/transport_sockets/well_known_names.h" @@ -226,6 +228,16 @@ ListenerImpl::ListenerImpl(const envoy::api::v2::Listener& config, const std::st addListenSocketOptions(Network::SocketOptionFactory::buildIpPacketInfoOptions()); // Needed to return receive buffer overflown indicator. addListenSocketOptions(Network::SocketOptionFactory::buildRxQueueOverFlowOptions()); + std::string listener_name = + config.has_udp_listener_config() ? config.udp_listener_config().udp_listener_name() : ""; + if (listener_name.empty()) { + listener_name = UdpListenerNames::get().RawUdp; + } + udp_listener_factory_ = + Config::Utility::getAndCheckFactory(listener_name) + .createActiveUdpListenerFactory(config.has_udp_listener_config() + ? config.udp_listener_config() + : envoy::api::v2::listener::UdpListenerConfig()); } if (!config.listener_filters().empty()) { diff --git a/source/server/listener_manager_impl.h b/source/server/listener_manager_impl.h index 1197ac82a9be..b634a1bf6537 100644 --- a/source/server/listener_manager_impl.h +++ b/source/server/listener_manager_impl.h @@ -279,6 +279,9 @@ class ListenerImpl : public Network::ListenerConfig, Stats::Scope& listenerScope() override { return *listener_scope_; } uint64_t listenerTag() const override { return listener_tag_; } const std::string& name() const override { return name_; } + const Network::ActiveUdpListenerFactory* udpListenerFactory() override { + return udp_listener_factory_.get(); + } // Server::Configuration::ListenerFactoryContext AccessLog::AccessLogManager& accessLogManager() override { @@ -379,6 +382,7 @@ class ListenerImpl : public Network::ListenerConfig, Network::Socket::OptionsSharedPtr listen_socket_options_; const std::chrono::milliseconds listener_filters_timeout_; const bool continue_on_listener_filters_timeout_; + Network::ActiveUdpListenerFactoryPtr udp_listener_factory_; // to access ListenerManagerImpl::factory_. friend class ListenerFilterChainFactoryBuilder; }; diff --git a/source/server/well_known_names.h b/source/server/well_known_names.h new file mode 100644 index 000000000000..23d202d996ae --- /dev/null +++ b/source/server/well_known_names.h @@ -0,0 +1,21 @@ +#pragma once + +#include + +#include "common/singleton/const_singleton.h" + +namespace Envoy { +namespace Server { + +/** + * Well-known active UDP listener names. + */ +class UdpListenerNameValues { +public: + const std::string RawUdp = "raw_udp_listener"; +}; + +using UdpListenerNames = ConstSingleton; + +} // namespace Server +} // namespace Envoy diff --git a/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc b/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc index 2f37b861df43..cdc225696c3d 100644 --- a/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc +++ b/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc @@ -77,6 +77,7 @@ class ProxyProtocolTest : public testing::TestWithParam, Stats::Scope& listenerScope() override { return parent_.stats_store_; } uint64_t listenerTag() const override { return 0; } const std::string& name() const override { return name_; } + Network::ActiveUdpListenerFactory* udpListenerFactory() override { + return udp_listener_factory_.get(); + } FakeUpstream& parent_; + Network::ActiveUdpListenerFactoryPtr udp_listener_factory_; std::string name_; }; diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index e63d3f22957b..33931209fffe 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -311,6 +311,7 @@ class MockListenerConfig : public ListenerConfig { MOCK_METHOD0(listenerScope, Stats::Scope&()); MOCK_CONST_METHOD0(listenerTag, uint64_t()); MOCK_CONST_METHOD0(name, const std::string&()); + MOCK_METHOD0(udpListenerFactory, const Network::ActiveUdpListenerFactory*()); testing::NiceMock filter_chain_factory_; testing::NiceMock socket_; diff --git a/test/server/BUILD b/test/server/BUILD index 6f2893539e87..7ffe0fcfd36b 100644 --- a/test/server/BUILD +++ b/test/server/BUILD @@ -53,6 +53,7 @@ envoy_cc_test( "//source/common/common:utility_lib", "//source/common/network:address_lib", "//source/common/stats:stats_lib", + "//source/server:active_raw_udp_listener_config", "//source/server:connection_handler_lib", "//test/mocks/network:network_mocks", "//test/mocks/server:server_mocks", @@ -180,6 +181,7 @@ envoy_cc_test( "//source/extensions/transport_sockets/raw_buffer:config", "//source/extensions/transport_sockets/tls:config", "//source/extensions/transport_sockets/tls:ssl_socket_lib", + "//source/server:active_raw_udp_listener_config", "//source/server:listener_manager_lib", "//test/mocks/network:network_mocks", "//test/mocks/server:server_mocks", diff --git a/test/server/connection_handler_test.cc b/test/server/connection_handler_test.cc index 59d14ffb5e2f..092ff2780649 100644 --- a/test/server/connection_handler_test.cc +++ b/test/server/connection_handler_test.cc @@ -1,3 +1,4 @@ +#include "envoy/server/active_udp_listener_config.h" #include "envoy/stats/scope.h" #include "common/common/utility.h" @@ -44,6 +45,12 @@ class ConnectionHandlerTest : public testing::Test, protected Logger::Loggable(listener_name) + .createActiveUdpListenerFactory(dummy); EXPECT_CALL(socket_, socketType()).WillOnce(Return(socket_type)); } @@ -66,6 +73,9 @@ class ConnectionHandlerTest : public testing::Test, protected Logger::Loggable udp_listener_factory_; }; using TestListenerPtr = std::unique_ptr; From 8bdebbfb936c533a822df9e96fd37be2cbdbab94 Mon Sep 17 00:00:00 2001 From: Lizan Zhou Date: Mon, 26 Aug 2019 16:07:39 -0700 Subject: [PATCH 07/16] accesslog: implement TCP gRPC access logger (#7941) Description: Initial implementation for TCP gRPC access logger. Risk Level: Low (extension only) Testing: integration test Docs Changes: Added Release Notes: Added Signed-off-by: Lizan Zhou --- api/envoy/config/accesslog/v2/als.proto | 1 - .../filter/accesslog/v2/accesslog.proto | 3 + api/envoy/data/accesslog/v2/accesslog.proto | 13 +- api/envoy/service/accesslog/v2/als.proto | 2 - docs/root/intro/version_history.rst | 1 + source/extensions/access_loggers/grpc/BUILD | 40 +++- .../access_loggers/grpc/config_utils.cc | 25 +++ .../access_loggers/grpc/config_utils.h | 18 ++ .../grpc/grpc_access_log_impl.cc | 18 +- .../grpc/grpc_access_log_impl.h | 23 ++- .../grpc/grpc_access_log_proto_descriptors.cc | 4 +- .../grpc/grpc_access_log_proto_descriptors.h | 4 +- .../grpc/grpc_access_log_utils.cc | 3 +- .../access_loggers/grpc/http_config.cc | 22 +- .../access_loggers/grpc/http_config.h | 2 - .../grpc/http_grpc_access_log_impl.cc | 4 +- .../access_loggers/grpc/tcp_config.cc | 51 +++++ .../access_loggers/grpc/tcp_config.h | 29 +++ .../grpc/tcp_grpc_access_log_impl.cc | 48 +++++ .../grpc/tcp_grpc_access_log_impl.h | 60 ++++++ .../access_loggers/well_known_names.h | 2 + source/extensions/extensions_build_config.bzl | 1 + test/extensions/access_loggers/grpc/BUILD | 17 ++ .../grpc/grpc_access_log_impl_test.cc | 15 +- .../grpc/http_grpc_access_log_impl_test.cc | 21 +- .../tcp_grpc_access_log_integration_test.cc | 189 ++++++++++++++++++ 26 files changed, 564 insertions(+), 52 deletions(-) create mode 100644 source/extensions/access_loggers/grpc/config_utils.cc create mode 100644 source/extensions/access_loggers/grpc/config_utils.h create mode 100644 source/extensions/access_loggers/grpc/tcp_config.cc create mode 100644 source/extensions/access_loggers/grpc/tcp_config.h create mode 100644 source/extensions/access_loggers/grpc/tcp_grpc_access_log_impl.cc create mode 100644 source/extensions/access_loggers/grpc/tcp_grpc_access_log_impl.h create mode 100644 test/extensions/access_loggers/grpc/tcp_grpc_access_log_integration_test.cc diff --git a/api/envoy/config/accesslog/v2/als.proto b/api/envoy/config/accesslog/v2/als.proto index a7291e4e9780..9d83ebfcfb91 100644 --- a/api/envoy/config/accesslog/v2/als.proto +++ b/api/envoy/config/accesslog/v2/als.proto @@ -38,7 +38,6 @@ message HttpGrpcAccessLogConfig { // Configuration for the built-in *envoy.tcp_grpc_access_log* type. This configuration will // populate *StreamAccessLogsMessage.tcp_logs*. -// [#not-implemented-hide:] message TcpGrpcAccessLogConfig { CommonGrpcAccessLogConfig common_config = 1 [(validate.rules).message.required = true]; } diff --git a/api/envoy/config/filter/accesslog/v2/accesslog.proto b/api/envoy/config/filter/accesslog/v2/accesslog.proto index 66d901c2d413..76fc4baf80c7 100644 --- a/api/envoy/config/filter/accesslog/v2/accesslog.proto +++ b/api/envoy/config/filter/accesslog/v2/accesslog.proto @@ -24,6 +24,7 @@ message AccessLog { // // #. "envoy.file_access_log" // #. "envoy.http_grpc_access_log" + // #. "envoy.tcp_grpc_access_log" string name = 1; // Filter which is used to determine if the access log needs to be written. @@ -36,6 +37,8 @@ message AccessLog { // ` // #. "envoy.http_grpc_access_log": :ref:`HttpGrpcAccessLogConfig // ` + // #. "envoy.tcp_grpc_access_log": :ref:`TcpGrpcAccessLogConfig + // ` oneof config_type { google.protobuf.Struct config = 3; diff --git a/api/envoy/data/accesslog/v2/accesslog.proto b/api/envoy/data/accesslog/v2/accesslog.proto index 1396c729f88e..7309d4a362a6 100644 --- a/api/envoy/data/accesslog/v2/accesslog.proto +++ b/api/envoy/data/accesslog/v2/accesslog.proto @@ -28,10 +28,12 @@ option (gogoproto.stable_marshaler_all) = true; // Fields describing *upstream* interaction will explicitly include ``upstream`` // in their name. -// [#not-implemented-hide:] message TCPAccessLogEntry { // Common properties shared by all Envoy access logs. AccessLogCommon common_properties = 1; + + // Properties of the TCP connection. + ConnectionProperties connection_properties = 2; } message HTTPAccessLogEntry { @@ -54,6 +56,15 @@ message HTTPAccessLogEntry { HTTPResponseProperties response = 4; } +// Defines fields for a connection +message ConnectionProperties { + // Number of bytes received from downstream. + uint64 received_bytes = 1; + + // Number of bytes sent to downstream. + uint64 sent_bytes = 2; +} + // Defines fields that are shared by all Envoy access logs. message AccessLogCommon { // [#not-implemented-hide:] diff --git a/api/envoy/service/accesslog/v2/als.proto b/api/envoy/service/accesslog/v2/als.proto index 1ee6ccd0094c..52788e0659c6 100644 --- a/api/envoy/service/accesslog/v2/als.proto +++ b/api/envoy/service/accesslog/v2/als.proto @@ -53,7 +53,6 @@ message StreamAccessLogsMessage { [(validate.rules).repeated .min_items = 1]; } - // [#not-implemented-hide:] // Wrapper for batches of TCP access log entries. message TCPAccessLogEntries { repeated envoy.data.accesslog.v2.TCPAccessLogEntry log_entry = 1 @@ -67,7 +66,6 @@ message StreamAccessLogsMessage { HTTPAccessLogEntries http_logs = 2; - // [#not-implemented-hide:] TCPAccessLogEntries tcp_logs = 3; } } diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index caf541e796d5..63fb953c060a 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -4,6 +4,7 @@ Version history 1.12.0 (pending) ================ * access log: added :ref:`buffering ` and :ref:`periodical flushing ` support to gRPC access logger. Defaults to 16KB buffer and flushing every 1 second. +* access log: gRPC Access Log Service (ALS) support added for :ref:`TCP access logs `. * admin: added ability to configure listener :ref:`socket options `. * admin: added config dump support for Secret Discovery Service :ref:`SecretConfigDump `. * api: added ::ref:`set_node_on_first_message_only ` option to omit the node identifier from the subsequent discovery requests on the same stream. diff --git a/source/extensions/access_loggers/grpc/BUILD b/source/extensions/access_loggers/grpc/BUILD index e0e705952699..e7ad7b299588 100644 --- a/source/extensions/access_loggers/grpc/BUILD +++ b/source/extensions/access_loggers/grpc/BUILD @@ -11,6 +11,18 @@ load( envoy_package() +envoy_cc_library( + name = "config_utils", + srcs = ["config_utils.cc"], + hdrs = ["config_utils.h"], + deps = [ + ":grpc_access_log_lib", + "//include/envoy/registry", + "//include/envoy/server:filter_config_interface", + "//include/envoy/singleton:instance_interface", + ], +) + envoy_cc_library( name = "grpc_access_log_lib", srcs = ["grpc_access_log_impl.cc"], @@ -18,7 +30,6 @@ envoy_cc_library( deps = [ "//include/envoy/grpc:async_client_interface", "//include/envoy/grpc:async_client_manager_interface", - "//include/envoy/singleton:instance_interface", "//include/envoy/thread_local:thread_local_interface", "//include/envoy/upstream:cluster_manager_interface", "//include/envoy/upstream:upstream_interface", @@ -54,6 +65,16 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "tcp_grpc_access_log_lib", + srcs = ["tcp_grpc_access_log_impl.cc"], + hdrs = ["tcp_grpc_access_log_impl.h"], + deps = [ + ":grpc_access_log_lib", + ":grpc_access_log_utils", + ], +) + envoy_cc_library( name = "grpc_access_log_proto_descriptors_lib", srcs = ["grpc_access_log_proto_descriptors.cc"], @@ -70,7 +91,7 @@ envoy_cc_library( srcs = ["http_config.cc"], hdrs = ["http_config.h"], deps = [ - "//include/envoy/registry", + ":config_utils", "//include/envoy/server:access_log_config_interface", "//source/common/common:assert_lib", "//source/common/protobuf", @@ -79,3 +100,18 @@ envoy_cc_library( "//source/extensions/access_loggers/grpc:http_grpc_access_log_lib", ], ) + +envoy_cc_library( + name = "tcp_config", + srcs = ["tcp_config.cc"], + hdrs = ["tcp_config.h"], + deps = [ + ":config_utils", + "//include/envoy/server:access_log_config_interface", + "//source/common/common:assert_lib", + "//source/common/protobuf", + "//source/extensions/access_loggers:well_known_names", + "//source/extensions/access_loggers/grpc:grpc_access_log_proto_descriptors_lib", + "//source/extensions/access_loggers/grpc:tcp_grpc_access_log_lib", + ], +) diff --git a/source/extensions/access_loggers/grpc/config_utils.cc b/source/extensions/access_loggers/grpc/config_utils.cc new file mode 100644 index 000000000000..5d2a648a0f5d --- /dev/null +++ b/source/extensions/access_loggers/grpc/config_utils.cc @@ -0,0 +1,25 @@ +#include "extensions/access_loggers/grpc/config_utils.h" + +#include "envoy/singleton/manager.h" + +namespace Envoy { +namespace Extensions { +namespace AccessLoggers { +namespace GrpcCommon { + +// Singleton registration via macro defined in envoy/singleton/manager.h +SINGLETON_MANAGER_REGISTRATION(grpc_access_logger_cache); + +std::shared_ptr +getGrpcAccessLoggerCacheSingleton(Server::Configuration::FactoryContext& context) { + return context.singletonManager().getTyped( + SINGLETON_MANAGER_REGISTERED_NAME(grpc_access_logger_cache), [&context] { + return std::make_shared( + context.clusterManager().grpcAsyncClientManager(), context.scope(), + context.threadLocal(), context.localInfo()); + }); +} +} // namespace GrpcCommon +} // namespace AccessLoggers +} // namespace Extensions +} // namespace Envoy \ No newline at end of file diff --git a/source/extensions/access_loggers/grpc/config_utils.h b/source/extensions/access_loggers/grpc/config_utils.h new file mode 100644 index 000000000000..f95b53f6a790 --- /dev/null +++ b/source/extensions/access_loggers/grpc/config_utils.h @@ -0,0 +1,18 @@ +#pragma once + +#include "envoy/server/filter_config.h" + +#include "extensions/access_loggers/grpc/grpc_access_log_impl.h" + +namespace Envoy { +namespace Extensions { +namespace AccessLoggers { +namespace GrpcCommon { + +GrpcAccessLoggerCacheSharedPtr +getGrpcAccessLoggerCacheSingleton(Server::Configuration::FactoryContext& context); + +} // namespace GrpcCommon +} // namespace AccessLoggers +} // namespace Extensions +} // namespace Envoy \ No newline at end of file diff --git a/source/extensions/access_loggers/grpc/grpc_access_log_impl.cc b/source/extensions/access_loggers/grpc/grpc_access_log_impl.cc index 38020326b7da..962e3a68084d 100644 --- a/source/extensions/access_loggers/grpc/grpc_access_log_impl.cc +++ b/source/extensions/access_loggers/grpc/grpc_access_log_impl.cc @@ -38,14 +38,22 @@ GrpcAccessLoggerImpl::GrpcAccessLoggerImpl(Grpc::RawAsyncClientPtr&& client, std void GrpcAccessLoggerImpl::log(envoy::data::accesslog::v2::HTTPAccessLogEntry&& entry) { approximate_message_size_bytes_ += entry.ByteSizeLong(); - message_.mutable_http_logs()->add_log_entry()->Swap(&entry); + message_.mutable_http_logs()->mutable_log_entry()->Add(std::move(entry)); + if (approximate_message_size_bytes_ >= buffer_size_bytes_) { + flush(); + } +} + +void GrpcAccessLoggerImpl::log(envoy::data::accesslog::v2::TCPAccessLogEntry&& entry) { + approximate_message_size_bytes_ += entry.ByteSizeLong(); + message_.mutable_tcp_logs()->mutable_log_entry()->Add(std::move(entry)); if (approximate_message_size_bytes_ >= buffer_size_bytes_) { flush(); } } void GrpcAccessLoggerImpl::flush() { - if (!message_.has_http_logs()) { + if (!message_.has_http_logs() && !message_.has_tcp_logs()) { // Nothing to flush. return; } @@ -88,11 +96,11 @@ GrpcAccessLoggerCacheImpl::GrpcAccessLoggerCacheImpl(Grpc::AsyncClientManager& a } GrpcAccessLoggerSharedPtr GrpcAccessLoggerCacheImpl::getOrCreateLogger( - const envoy::config::accesslog::v2::CommonGrpcAccessLogConfig& config) { + const envoy::config::accesslog::v2::CommonGrpcAccessLogConfig& config, + GrpcAccessLoggerType logger_type) { // TODO(euroelessar): Consider cleaning up loggers. auto& cache = tls_slot_->getTyped(); - // TODO(lizan): Include logger type in the hash - const std::size_t cache_key = MessageUtil::hash(config); + const auto cache_key = std::make_pair(MessageUtil::hash(config), logger_type); const auto it = cache.access_loggers_.find(cache_key); if (it != cache.access_loggers_.end()) { return it->second; diff --git a/source/extensions/access_loggers/grpc/grpc_access_log_impl.h b/source/extensions/access_loggers/grpc/grpc_access_log_impl.h index 71745adc54d1..8c254e47bca0 100644 --- a/source/extensions/access_loggers/grpc/grpc_access_log_impl.h +++ b/source/extensions/access_loggers/grpc/grpc_access_log_impl.h @@ -36,10 +36,18 @@ class GrpcAccessLogger { * @param entry supplies the access log to send. */ virtual void log(envoy::data::accesslog::v2::HTTPAccessLogEntry&& entry) PURE; + + /** + * Log tcp access entry. + * @param entry supplies the access log to send. + */ + virtual void log(envoy::data::accesslog::v2::TCPAccessLogEntry&& entry) PURE; }; using GrpcAccessLoggerSharedPtr = std::shared_ptr; +enum class GrpcAccessLoggerType { TCP, HTTP }; + /** * Interface for an access logger cache. The cache deals with threading and de-duplicates loggers * for the same configuration. @@ -54,7 +62,8 @@ class GrpcAccessLoggerCache { * @return GrpcAccessLoggerSharedPtr ready for logging requests. */ virtual GrpcAccessLoggerSharedPtr - getOrCreateLogger(const ::envoy::config::accesslog::v2::CommonGrpcAccessLogConfig& config) PURE; + getOrCreateLogger(const ::envoy::config::accesslog::v2::CommonGrpcAccessLogConfig& config, + GrpcAccessLoggerType logger_type) PURE; }; using GrpcAccessLoggerCacheSharedPtr = std::shared_ptr; @@ -66,7 +75,9 @@ class GrpcAccessLoggerImpl : public GrpcAccessLogger { uint64_t buffer_size_bytes, Event::Dispatcher& dispatcher, const LocalInfo::LocalInfo& local_info); + // Extensions::AccessLoggers::GrpcCommon::GrpcAccessLogger void log(envoy::data::accesslog::v2::HTTPAccessLogEntry&& entry) override; + void log(envoy::data::accesslog::v2::TCPAccessLogEntry&& entry) override; private: struct LocalStream @@ -106,8 +117,9 @@ class GrpcAccessLoggerCacheImpl : public Singleton::Instance, public GrpcAccessL ThreadLocal::SlotAllocator& tls, const LocalInfo::LocalInfo& local_info); - GrpcAccessLoggerSharedPtr getOrCreateLogger( - const ::envoy::config::accesslog::v2::CommonGrpcAccessLogConfig& config) override; + GrpcAccessLoggerSharedPtr + getOrCreateLogger(const ::envoy::config::accesslog::v2::CommonGrpcAccessLogConfig& config, + GrpcAccessLoggerType logger_type) override; private: /** @@ -117,8 +129,9 @@ class GrpcAccessLoggerCacheImpl : public Singleton::Instance, public GrpcAccessL ThreadLocalCache(Event::Dispatcher& dispatcher) : dispatcher_(dispatcher) {} Event::Dispatcher& dispatcher_; - // Access loggers indexed by the hash of logger's configuration. - absl::flat_hash_map access_loggers_; + // Access loggers indexed by the hash of logger's configuration and logger type. + absl::flat_hash_map, GrpcAccessLoggerSharedPtr> + access_loggers_; }; Grpc::AsyncClientManager& async_client_manager_; diff --git a/source/extensions/access_loggers/grpc/grpc_access_log_proto_descriptors.cc b/source/extensions/access_loggers/grpc/grpc_access_log_proto_descriptors.cc index 3b038045ee89..6936800be0ff 100644 --- a/source/extensions/access_loggers/grpc/grpc_access_log_proto_descriptors.cc +++ b/source/extensions/access_loggers/grpc/grpc_access_log_proto_descriptors.cc @@ -9,7 +9,7 @@ namespace Envoy { namespace Extensions { namespace AccessLoggers { -namespace HttpGrpc { +namespace GrpcCommon { void validateProtoDescriptors() { const auto method = "envoy.service.accesslog.v2.AccessLogService.StreamAccessLogs"; @@ -17,7 +17,7 @@ void validateProtoDescriptors() { RELEASE_ASSERT(Protobuf::DescriptorPool::generated_pool()->FindMethodByName(method) != nullptr, ""); }; -} // namespace HttpGrpc +} // namespace GrpcCommon } // namespace AccessLoggers } // namespace Extensions } // namespace Envoy diff --git a/source/extensions/access_loggers/grpc/grpc_access_log_proto_descriptors.h b/source/extensions/access_loggers/grpc/grpc_access_log_proto_descriptors.h index 62b70a387a7b..988723cfa2da 100644 --- a/source/extensions/access_loggers/grpc/grpc_access_log_proto_descriptors.h +++ b/source/extensions/access_loggers/grpc/grpc_access_log_proto_descriptors.h @@ -3,12 +3,12 @@ namespace Envoy { namespace Extensions { namespace AccessLoggers { -namespace HttpGrpc { +namespace GrpcCommon { // This function validates that the method descriptors for gRPC services and type descriptors that // are referenced in Any messages are available in the descriptor pool. void validateProtoDescriptors(); -} // namespace HttpGrpc +} // namespace GrpcCommon } // namespace AccessLoggers } // namespace Extensions } // namespace Envoy diff --git a/source/extensions/access_loggers/grpc/grpc_access_log_utils.cc b/source/extensions/access_loggers/grpc/grpc_access_log_utils.cc index 6bab3fd1e274..66cf3c0a7489 100644 --- a/source/extensions/access_loggers/grpc/grpc_access_log_utils.cc +++ b/source/extensions/access_loggers/grpc/grpc_access_log_utils.cc @@ -116,6 +116,7 @@ void Utility::responseFlagsToAccessLogResponseFlags( void Utility::extractCommonAccessLogProperties( envoy::data::accesslog::v2::AccessLogCommon& common_access_log, const StreamInfo::StreamInfo& stream_info) { + // TODO(mattklein123): Populate sample_rate field. if (stream_info.downstreamRemoteAddress() != nullptr) { Network::Utility::addressToProtobufAddress( *stream_info.downstreamRemoteAddress(), @@ -229,4 +230,4 @@ void Utility::extractCommonAccessLogProperties( } // namespace GrpcCommon } // namespace AccessLoggers } // namespace Extensions -} // namespace Envoy \ No newline at end of file +} // namespace Envoy diff --git a/source/extensions/access_loggers/grpc/http_config.cc b/source/extensions/access_loggers/grpc/http_config.cc index 1047beaa579a..e8f6992600cd 100644 --- a/source/extensions/access_loggers/grpc/http_config.cc +++ b/source/extensions/access_loggers/grpc/http_config.cc @@ -10,6 +10,7 @@ #include "common/grpc/async_client_impl.h" #include "common/protobuf/protobuf.h" +#include "extensions/access_loggers/grpc/config_utils.h" #include "extensions/access_loggers/grpc/grpc_access_log_proto_descriptors.h" #include "extensions/access_loggers/grpc/http_grpc_access_log_impl.h" #include "extensions/access_loggers/well_known_names.h" @@ -19,32 +20,23 @@ namespace Extensions { namespace AccessLoggers { namespace HttpGrpc { -// Singleton registration via macro defined in envoy/singleton/manager.h -SINGLETON_MANAGER_REGISTRATION(grpc_access_logger_cache); - AccessLog::InstanceSharedPtr HttpGrpcAccessLogFactory::createAccessLogInstance(const Protobuf::Message& config, AccessLog::FilterPtr&& filter, Server::Configuration::FactoryContext& context) { - validateProtoDescriptors(); + GrpcCommon::validateProtoDescriptors(); const auto& proto_config = MessageUtil::downcastAndValidate< const envoy::config::accesslog::v2::HttpGrpcAccessLogConfig&>( config, context.messageValidationVisitor()); - std::shared_ptr grpc_access_logger_cache = - context.singletonManager().getTyped( - SINGLETON_MANAGER_REGISTERED_NAME(grpc_access_logger_cache), [&context] { - return std::make_shared( - context.clusterManager().grpcAsyncClientManager(), context.scope(), - context.threadLocal(), context.localInfo()); - }); - - return std::make_shared(std::move(filter), proto_config, context.threadLocal(), - grpc_access_logger_cache); + + return std::make_shared( + std::move(filter), proto_config, context.threadLocal(), + GrpcCommon::getGrpcAccessLoggerCacheSingleton(context)); } ProtobufTypes::MessagePtr HttpGrpcAccessLogFactory::createEmptyConfigProto() { - return ProtobufTypes::MessagePtr{new envoy::config::accesslog::v2::HttpGrpcAccessLogConfig()}; + return std::make_unique(); } std::string HttpGrpcAccessLogFactory::name() const { return AccessLogNames::get().HttpGrpc; } diff --git a/source/extensions/access_loggers/grpc/http_config.h b/source/extensions/access_loggers/grpc/http_config.h index 9e046ac39218..c88a3a5ac62d 100644 --- a/source/extensions/access_loggers/grpc/http_config.h +++ b/source/extensions/access_loggers/grpc/http_config.h @@ -23,8 +23,6 @@ class HttpGrpcAccessLogFactory : public Server::Configuration::AccessLogInstance std::string name() const override; }; -// TODO(mattklein123): Add TCP access log. - } // namespace HttpGrpc } // namespace AccessLoggers } // namespace Extensions diff --git a/source/extensions/access_loggers/grpc/http_grpc_access_log_impl.cc b/source/extensions/access_loggers/grpc/http_grpc_access_log_impl.cc index 07b91a0a894a..e5bad26a3efa 100644 --- a/source/extensions/access_loggers/grpc/http_grpc_access_log_impl.cc +++ b/source/extensions/access_loggers/grpc/http_grpc_access_log_impl.cc @@ -34,8 +34,8 @@ HttpGrpcAccessLog::HttpGrpcAccessLog(AccessLog::FilterPtr&& filter, } tls_slot_->set([this](Event::Dispatcher&) { - return std::make_shared( - access_logger_cache_->getOrCreateLogger(config_.common_config())); + return std::make_shared(access_logger_cache_->getOrCreateLogger( + config_.common_config(), GrpcCommon::GrpcAccessLoggerType::HTTP)); }); } diff --git a/source/extensions/access_loggers/grpc/tcp_config.cc b/source/extensions/access_loggers/grpc/tcp_config.cc new file mode 100644 index 000000000000..b8c053ae0c44 --- /dev/null +++ b/source/extensions/access_loggers/grpc/tcp_config.cc @@ -0,0 +1,51 @@ +#include "extensions/access_loggers/grpc/tcp_config.h" + +#include "envoy/config/accesslog/v2/als.pb.validate.h" +#include "envoy/config/filter/accesslog/v2/accesslog.pb.validate.h" +#include "envoy/registry/registry.h" +#include "envoy/server/filter_config.h" + +#include "common/common/assert.h" +#include "common/common/macros.h" +#include "common/grpc/async_client_impl.h" +#include "common/protobuf/protobuf.h" + +#include "extensions/access_loggers/grpc/config_utils.h" +#include "extensions/access_loggers/grpc/grpc_access_log_proto_descriptors.h" +#include "extensions/access_loggers/grpc/tcp_grpc_access_log_impl.h" +#include "extensions/access_loggers/well_known_names.h" + +namespace Envoy { +namespace Extensions { +namespace AccessLoggers { +namespace TcpGrpc { + +AccessLog::InstanceSharedPtr +TcpGrpcAccessLogFactory::createAccessLogInstance(const Protobuf::Message& config, + AccessLog::FilterPtr&& filter, + Server::Configuration::FactoryContext& context) { + GrpcCommon::validateProtoDescriptors(); + + const auto& proto_config = + MessageUtil::downcastAndValidate( + config, context.messageValidationVisitor()); + + return std::make_shared(std::move(filter), proto_config, context.threadLocal(), + GrpcCommon::getGrpcAccessLoggerCacheSingleton(context)); +} + +ProtobufTypes::MessagePtr TcpGrpcAccessLogFactory::createEmptyConfigProto() { + return std::make_unique(); +} + +std::string TcpGrpcAccessLogFactory::name() const { return AccessLogNames::get().TcpGrpc; } + +/** + * Static registration for the TCP gRPC access log. @see RegisterFactory. + */ +REGISTER_FACTORY(TcpGrpcAccessLogFactory, Server::Configuration::AccessLogInstanceFactory); + +} // namespace TcpGrpc +} // namespace AccessLoggers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/access_loggers/grpc/tcp_config.h b/source/extensions/access_loggers/grpc/tcp_config.h new file mode 100644 index 000000000000..39bc986146b9 --- /dev/null +++ b/source/extensions/access_loggers/grpc/tcp_config.h @@ -0,0 +1,29 @@ +#pragma once + +#include + +#include "envoy/server/access_log_config.h" + +namespace Envoy { +namespace Extensions { +namespace AccessLoggers { +namespace TcpGrpc { + +/** + * Config registration for the TCP gRPC access log. @see AccessLogInstanceFactory. + */ +class TcpGrpcAccessLogFactory : public Server::Configuration::AccessLogInstanceFactory { +public: + AccessLog::InstanceSharedPtr + createAccessLogInstance(const Protobuf::Message& config, AccessLog::FilterPtr&& filter, + Server::Configuration::FactoryContext& context) override; + + ProtobufTypes::MessagePtr createEmptyConfigProto() override; + + std::string name() const override; +}; + +} // namespace TcpGrpc +} // namespace AccessLoggers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/access_loggers/grpc/tcp_grpc_access_log_impl.cc b/source/extensions/access_loggers/grpc/tcp_grpc_access_log_impl.cc new file mode 100644 index 000000000000..c6c8a1bb5a58 --- /dev/null +++ b/source/extensions/access_loggers/grpc/tcp_grpc_access_log_impl.cc @@ -0,0 +1,48 @@ +#include "extensions/access_loggers/grpc/tcp_grpc_access_log_impl.h" + +#include "common/common/assert.h" +#include "common/network/utility.h" +#include "common/stream_info/utility.h" + +#include "extensions/access_loggers/grpc/grpc_access_log_utils.h" + +namespace Envoy { +namespace Extensions { +namespace AccessLoggers { +namespace TcpGrpc { + +TcpGrpcAccessLog::ThreadLocalLogger::ThreadLocalLogger(GrpcCommon::GrpcAccessLoggerSharedPtr logger) + : logger_(std::move(logger)) {} + +TcpGrpcAccessLog::TcpGrpcAccessLog(AccessLog::FilterPtr&& filter, + envoy::config::accesslog::v2::TcpGrpcAccessLogConfig config, + ThreadLocal::SlotAllocator& tls, + GrpcCommon::GrpcAccessLoggerCacheSharedPtr access_logger_cache) + : Common::ImplBase(std::move(filter)), config_(std::move(config)), + tls_slot_(tls.allocateSlot()), access_logger_cache_(std::move(access_logger_cache)) { + tls_slot_->set([this](Event::Dispatcher&) { + return std::make_shared(access_logger_cache_->getOrCreateLogger( + config_.common_config(), GrpcCommon::GrpcAccessLoggerType::TCP)); + }); +} + +void TcpGrpcAccessLog::emitLog(const Http::HeaderMap&, const Http::HeaderMap&, + const Http::HeaderMap&, const StreamInfo::StreamInfo& stream_info) { + // Common log properties. + envoy::data::accesslog::v2::TCPAccessLogEntry log_entry; + GrpcCommon::Utility::extractCommonAccessLogProperties(*log_entry.mutable_common_properties(), + stream_info); + + envoy::data::accesslog::v2::ConnectionProperties& connection_properties = + *log_entry.mutable_connection_properties(); + connection_properties.set_received_bytes(stream_info.bytesReceived()); + connection_properties.set_sent_bytes(stream_info.bytesSent()); + + // request_properties->set_request_body_bytes(stream_info.bytesReceived()); + tls_slot_->getTyped().logger_->log(std::move(log_entry)); +} + +} // namespace TcpGrpc +} // namespace AccessLoggers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/access_loggers/grpc/tcp_grpc_access_log_impl.h b/source/extensions/access_loggers/grpc/tcp_grpc_access_log_impl.h new file mode 100644 index 000000000000..115c8a467719 --- /dev/null +++ b/source/extensions/access_loggers/grpc/tcp_grpc_access_log_impl.h @@ -0,0 +1,60 @@ +#pragma once + +#include +#include + +#include "envoy/config/accesslog/v2/als.pb.h" +#include "envoy/config/filter/accesslog/v2/accesslog.pb.h" +#include "envoy/grpc/async_client.h" +#include "envoy/grpc/async_client_manager.h" +#include "envoy/local_info/local_info.h" +#include "envoy/service/accesslog/v2/als.pb.h" +#include "envoy/singleton/instance.h" +#include "envoy/thread_local/thread_local.h" + +#include "common/grpc/typed_async_client.h" + +#include "extensions/access_loggers/common/access_log_base.h" +#include "extensions/access_loggers/grpc/grpc_access_log_impl.h" + +namespace Envoy { +namespace Extensions { +namespace AccessLoggers { +namespace TcpGrpc { + +// TODO(mattklein123): Stats + +/** + * Access log Instance that streams TCP logs over gRPC. + */ +class TcpGrpcAccessLog : public Common::ImplBase { +public: + TcpGrpcAccessLog(AccessLog::FilterPtr&& filter, + envoy::config::accesslog::v2::TcpGrpcAccessLogConfig config, + ThreadLocal::SlotAllocator& tls, + GrpcCommon::GrpcAccessLoggerCacheSharedPtr access_logger_cache); + +private: + /** + * Per-thread cached logger. + */ + struct ThreadLocalLogger : public ThreadLocal::ThreadLocalObject { + ThreadLocalLogger(GrpcCommon::GrpcAccessLoggerSharedPtr logger); + + const GrpcCommon::GrpcAccessLoggerSharedPtr logger_; + }; + + // Common::ImplBase + void emitLog(const Http::HeaderMap& request_headers, const Http::HeaderMap& response_headers, + const Http::HeaderMap& response_trailers, + const StreamInfo::StreamInfo& stream_info) override; + + const envoy::config::accesslog::v2::TcpGrpcAccessLogConfig config_; + const ThreadLocal::SlotPtr tls_slot_; + const GrpcCommon::GrpcAccessLoggerCacheSharedPtr access_logger_cache_; +}; + +} // namespace TcpGrpc +} // namespace AccessLoggers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/access_loggers/well_known_names.h b/source/extensions/access_loggers/well_known_names.h index 7c5e7576c1df..dc55f536bfad 100644 --- a/source/extensions/access_loggers/well_known_names.h +++ b/source/extensions/access_loggers/well_known_names.h @@ -18,6 +18,8 @@ class AccessLogNameValues { const std::string File = "envoy.file_access_log"; // HTTP gRPC access log const std::string HttpGrpc = "envoy.http_grpc_access_log"; + // TCP gRPC access log + const std::string TcpGrpc = "envoy.tcp_grpc_access_log"; }; using AccessLogNames = ConstSingleton; diff --git a/source/extensions/extensions_build_config.bzl b/source/extensions/extensions_build_config.bzl index f8b1ccc5158f..5366f7ba55fb 100644 --- a/source/extensions/extensions_build_config.bzl +++ b/source/extensions/extensions_build_config.bzl @@ -6,6 +6,7 @@ EXTENSIONS = { "envoy.access_loggers.file": "//source/extensions/access_loggers/file:config", "envoy.access_loggers.http_grpc": "//source/extensions/access_loggers/grpc:http_config", + "envoy.access_loggers.tcp_grpc": "//source/extensions/access_loggers/grpc:tcp_config", # # Clusters diff --git a/test/extensions/access_loggers/grpc/BUILD b/test/extensions/access_loggers/grpc/BUILD index 9e2c7b46240f..652fc010d578 100644 --- a/test/extensions/access_loggers/grpc/BUILD +++ b/test/extensions/access_loggers/grpc/BUILD @@ -77,3 +77,20 @@ envoy_extension_cc_test( "//test/test_common:utility_lib", ], ) + +envoy_extension_cc_test( + name = "tcp_grpc_access_log_integration_test", + srcs = ["tcp_grpc_access_log_integration_test.cc"], + extension_name = "envoy.access_loggers.http_grpc", + deps = [ + "//source/common/buffer:zero_copy_input_stream_lib", + "//source/common/grpc:codec_lib", + "//source/common/grpc:common_lib", + "//source/extensions/access_loggers/grpc:http_config", + "//source/extensions/access_loggers/grpc:tcp_config", + "//source/extensions/filters/network/tcp_proxy:config", + "//test/common/grpc:grpc_client_integration_lib", + "//test/integration:http_integration_lib", + "//test/test_common:utility_lib", + ], +) diff --git a/test/extensions/access_loggers/grpc/grpc_access_log_impl_test.cc b/test/extensions/access_loggers/grpc/grpc_access_log_impl_test.cc index 83ba234f7973..07875b008ddf 100644 --- a/test/extensions/access_loggers/grpc/grpc_access_log_impl_test.cc +++ b/test/extensions/access_loggers/grpc/grpc_access_log_impl_test.cc @@ -271,21 +271,26 @@ TEST_F(GrpcAccessLoggerCacheImplTest, Deduplication) { config.mutable_grpc_service()->mutable_envoy_grpc()->set_cluster_name("cluster-1"); expectClientCreation(); - GrpcAccessLoggerSharedPtr logger1 = logger_cache_->getOrCreateLogger(config); - EXPECT_EQ(logger1, logger_cache_->getOrCreateLogger(config)); + GrpcAccessLoggerSharedPtr logger1 = + logger_cache_->getOrCreateLogger(config, GrpcAccessLoggerType::HTTP); + EXPECT_EQ(logger1, logger_cache_->getOrCreateLogger(config, GrpcAccessLoggerType::HTTP)); + + // Do not deduplicate different types of logger + expectClientCreation(); + EXPECT_NE(logger1, logger_cache_->getOrCreateLogger(config, GrpcAccessLoggerType::TCP)); // Changing log name leads to another logger. config.set_log_name("log-2"); expectClientCreation(); - EXPECT_NE(logger1, logger_cache_->getOrCreateLogger(config)); + EXPECT_NE(logger1, logger_cache_->getOrCreateLogger(config, GrpcAccessLoggerType::HTTP)); config.set_log_name("log-1"); - EXPECT_EQ(logger1, logger_cache_->getOrCreateLogger(config)); + EXPECT_EQ(logger1, logger_cache_->getOrCreateLogger(config, GrpcAccessLoggerType::HTTP)); // Changing cluster name leads to another logger. config.mutable_grpc_service()->mutable_envoy_grpc()->set_cluster_name("cluster-2"); expectClientCreation(); - EXPECT_NE(logger1, logger_cache_->getOrCreateLogger(config)); + EXPECT_NE(logger1, logger_cache_->getOrCreateLogger(config, GrpcAccessLoggerType::HTTP)); } } // namespace diff --git a/test/extensions/access_loggers/grpc/http_grpc_access_log_impl_test.cc b/test/extensions/access_loggers/grpc/http_grpc_access_log_impl_test.cc index 22e1f7cd59c0..893d23d61243 100644 --- a/test/extensions/access_loggers/grpc/http_grpc_access_log_impl_test.cc +++ b/test/extensions/access_loggers/grpc/http_grpc_access_log_impl_test.cc @@ -14,6 +14,7 @@ using namespace std::chrono_literals; using testing::_; +using testing::An; using testing::InSequence; using testing::Invoke; using testing::NiceMock; @@ -25,18 +26,22 @@ namespace AccessLoggers { namespace HttpGrpc { namespace { +using envoy::data::accesslog::v2::HTTPAccessLogEntry; + class MockGrpcAccessLogger : public GrpcCommon::GrpcAccessLogger { public: // GrpcAccessLogger - MOCK_METHOD1(log, void(envoy::data::accesslog::v2::HTTPAccessLogEntry&& entry)); + MOCK_METHOD1(log, void(HTTPAccessLogEntry&& entry)); + MOCK_METHOD1(log, void(envoy::data::accesslog::v2::TCPAccessLogEntry&& entry)); }; class MockGrpcAccessLoggerCache : public GrpcCommon::GrpcAccessLoggerCache { public: // GrpcAccessLoggerCache - MOCK_METHOD1(getOrCreateLogger, + MOCK_METHOD2(getOrCreateLogger, GrpcCommon::GrpcAccessLoggerSharedPtr( - const ::envoy::config::accesslog::v2::CommonGrpcAccessLogConfig& config)); + const ::envoy::config::accesslog::v2::CommonGrpcAccessLogConfig& config, + GrpcCommon::GrpcAccessLoggerType logger_type)); }; class HttpGrpcAccessLogTest : public testing::Test { @@ -44,9 +49,11 @@ class HttpGrpcAccessLogTest : public testing::Test { void init() { ON_CALL(*filter_, evaluate(_, _, _, _)).WillByDefault(Return(true)); config_.mutable_common_config()->set_log_name("hello_log"); - EXPECT_CALL(*logger_cache_, getOrCreateLogger(_)) - .WillOnce([this](const ::envoy::config::accesslog::v2::CommonGrpcAccessLogConfig& config) { + EXPECT_CALL(*logger_cache_, getOrCreateLogger(_, _)) + .WillOnce([this](const ::envoy::config::accesslog::v2::CommonGrpcAccessLogConfig& config, + GrpcCommon::GrpcAccessLoggerType logger_type) { EXPECT_EQ(config.DebugString(), config_.common_config().DebugString()); + EXPECT_EQ(GrpcCommon::GrpcAccessLoggerType::HTTP, logger_type); return logger_; }); access_log_ = std::make_unique(AccessLog::FilterPtr{filter_}, config_, tls_, @@ -58,9 +65,9 @@ class HttpGrpcAccessLogTest : public testing::Test { init(); } - envoy::data::accesslog::v2::HTTPAccessLogEntry expected_log_entry; + HTTPAccessLogEntry expected_log_entry; TestUtility::loadFromYaml(expected_log_entry_yaml, expected_log_entry); - EXPECT_CALL(*logger_, log(_)) + EXPECT_CALL(*logger_, log(An())) .WillOnce( Invoke([expected_log_entry](envoy::data::accesslog::v2::HTTPAccessLogEntry&& entry) { EXPECT_EQ(entry.DebugString(), expected_log_entry.DebugString()); diff --git a/test/extensions/access_loggers/grpc/tcp_grpc_access_log_integration_test.cc b/test/extensions/access_loggers/grpc/tcp_grpc_access_log_integration_test.cc new file mode 100644 index 000000000000..8ad6bbe7bb68 --- /dev/null +++ b/test/extensions/access_loggers/grpc/tcp_grpc_access_log_integration_test.cc @@ -0,0 +1,189 @@ +#include "envoy/config/accesslog/v2/als.pb.h" +#include "envoy/config/filter/network/tcp_proxy/v2/tcp_proxy.pb.validate.h" +#include "envoy/service/accesslog/v2/als.pb.h" + +#include "common/buffer/zero_copy_input_stream_impl.h" +#include "common/common/version.h" +#include "common/grpc/codec.h" +#include "common/grpc/common.h" + +#include "test/common/grpc/grpc_client_integration.h" +#include "test/integration/http_integration.h" +#include "test/test_common/utility.h" + +#include "gtest/gtest.h" + +using testing::AssertionResult; + +namespace Envoy { +namespace { + +void clearPort(envoy::api::v2::core::Address& address) { + address.mutable_socket_address()->clear_port_specifier(); +} + +class TcpGrpcAccessLogIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, + public BaseIntegrationTest { +public: + TcpGrpcAccessLogIntegrationTest() + : BaseIntegrationTest(ipVersion(), ConfigHelper::TCP_PROXY_CONFIG) { + enable_half_close_ = true; + } + + ~TcpGrpcAccessLogIntegrationTest() override { + test_server_.reset(); + fake_upstreams_.clear(); + } + + void createUpstreams() override { + BaseIntegrationTest::createUpstreams(); + fake_upstreams_.emplace_back( + new FakeUpstream(0, FakeHttpConnection::Type::HTTP2, version_, timeSystem())); + } + + void initialize() override { + config_helper_.renameListener("tcp_proxy"); + config_helper_.addConfigModifier([](envoy::config::bootstrap::v2::Bootstrap& bootstrap) { + auto* accesslog_cluster = bootstrap.mutable_static_resources()->add_clusters(); + accesslog_cluster->MergeFrom(bootstrap.static_resources().clusters()[0]); + accesslog_cluster->set_name("accesslog"); + accesslog_cluster->mutable_http2_protocol_options(); + }); + + config_helper_.addConfigModifier([this](envoy::config::bootstrap::v2::Bootstrap& bootstrap) { + auto* listener = bootstrap.mutable_static_resources()->mutable_listeners(0); + auto* filter_chain = listener->mutable_filter_chains(0); + auto* config_blob = filter_chain->mutable_filters(0)->mutable_config(); + + envoy::config::filter::network::tcp_proxy::v2::TcpProxy tcp_proxy_config; + TestUtility::jsonConvert(*config_blob, tcp_proxy_config); + + auto* access_log = tcp_proxy_config.add_access_log(); + access_log->set_name("envoy.tcp_grpc_access_log"); + envoy::config::accesslog::v2::TcpGrpcAccessLogConfig access_log_config; + auto* common_config = access_log_config.mutable_common_config(); + common_config->set_log_name("foo"); + setGrpcService(*common_config->mutable_grpc_service(), "accesslog", + fake_upstreams_.back()->localAddress()); + TestUtility::jsonConvert(access_log_config, *access_log->mutable_config()); + + TestUtility::jsonConvert(tcp_proxy_config, *config_blob); + }); + BaseIntegrationTest::initialize(); + } + + ABSL_MUST_USE_RESULT + AssertionResult waitForAccessLogConnection() { + return fake_upstreams_[1]->waitForHttpConnection(*dispatcher_, fake_access_log_connection_); + } + + ABSL_MUST_USE_RESULT + AssertionResult waitForAccessLogStream() { + return fake_access_log_connection_->waitForNewStream(*dispatcher_, access_log_request_); + } + + ABSL_MUST_USE_RESULT + AssertionResult waitForAccessLogRequest(const std::string& expected_request_msg_yaml) { + envoy::service::accesslog::v2::StreamAccessLogsMessage request_msg; + VERIFY_ASSERTION(access_log_request_->waitForGrpcMessage(*dispatcher_, request_msg)); + EXPECT_EQ("POST", access_log_request_->headers().Method()->value().getStringView()); + EXPECT_EQ("/envoy.service.accesslog.v2.AccessLogService/StreamAccessLogs", + access_log_request_->headers().Path()->value().getStringView()); + EXPECT_EQ("application/grpc", + access_log_request_->headers().ContentType()->value().getStringView()); + + envoy::service::accesslog::v2::StreamAccessLogsMessage expected_request_msg; + TestUtility::loadFromYaml(expected_request_msg_yaml, expected_request_msg); + + // Clear fields which are not deterministic. + auto* log_entry = request_msg.mutable_tcp_logs()->mutable_log_entry(0); + clearPort(*log_entry->mutable_common_properties()->mutable_downstream_remote_address()); + clearPort(*log_entry->mutable_common_properties()->mutable_downstream_local_address()); + clearPort(*log_entry->mutable_common_properties()->mutable_upstream_remote_address()); + clearPort(*log_entry->mutable_common_properties()->mutable_upstream_local_address()); + log_entry->mutable_common_properties()->clear_start_time(); + log_entry->mutable_common_properties()->clear_time_to_last_rx_byte(); + log_entry->mutable_common_properties()->clear_time_to_first_downstream_tx_byte(); + log_entry->mutable_common_properties()->clear_time_to_last_downstream_tx_byte(); + EXPECT_EQ(request_msg.DebugString(), expected_request_msg.DebugString()); + + return AssertionSuccess(); + } + + void cleanup() { + if (fake_access_log_connection_ != nullptr) { + AssertionResult result = fake_access_log_connection_->close(); + RELEASE_ASSERT(result, result.message()); + result = fake_access_log_connection_->waitForDisconnect(); + RELEASE_ASSERT(result, result.message()); + fake_access_log_connection_ = nullptr; + } + } + + FakeHttpConnectionPtr fake_access_log_connection_; + FakeStreamPtr access_log_request_; +}; + +INSTANTIATE_TEST_SUITE_P(IpVersionsCientType, TcpGrpcAccessLogIntegrationTest, + GRPC_CLIENT_INTEGRATION_PARAMS); + +// Test a basic full access logging flow. +TEST_P(TcpGrpcAccessLogIntegrationTest, BasicAccessLogFlow) { + initialize(); + + IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("tcp_proxy")); + FakeRawConnectionPtr fake_upstream_connection; + ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); + + ASSERT_TRUE(fake_upstream_connection->write("hello")); + tcp_client->waitForData("hello"); + tcp_client->write("bar", false); + + ASSERT_TRUE(fake_upstream_connection->write("", true)); + tcp_client->waitForHalfClose(); + tcp_client->write("", true); + ASSERT_TRUE(fake_upstream_connection->waitForHalfClose()); + ASSERT_TRUE(fake_upstream_connection->waitForDisconnect()); + + ASSERT_TRUE(waitForAccessLogConnection()); + ASSERT_TRUE(waitForAccessLogStream()); + ASSERT_TRUE(waitForAccessLogRequest( + fmt::format(R"EOF( +identifier: + node: + id: node_name + cluster: cluster_name + locality: + zone: zone_name + build_version: {} + log_name: foo +tcp_logs: + log_entry: + common_properties: + downstream_remote_address: + socket_address: + address: {} + downstream_local_address: + socket_address: + address: {} + upstream_remote_address: + socket_address: + address: {} + upstream_local_address: + socket_address: + address: {} + upstream_cluster: cluster_0 + connection_properties: + received_bytes: 3 + sent_bytes: 5 +)EOF", + VersionInfo::version(), Network::Test::getLoopbackAddressString(ipVersion()), + Network::Test::getLoopbackAddressString(ipVersion()), + Network::Test::getLoopbackAddressString(ipVersion()), + Network::Test::getLoopbackAddressString(ipVersion())))); + + cleanup(); +} + +} // namespace +} // namespace Envoy From 816d6f14fd65329a8511351e74b385fafa3160a8 Mon Sep 17 00:00:00 2001 From: easy Date: Tue, 27 Aug 2019 13:15:52 +1000 Subject: [PATCH 08/16] tracing: add OpenCensus agent exporter support to OpenCensus driver. (#8023) Signed-off-by: Emil Mikulic --- api/bazel/repository_locations.bzl | 8 ++++---- api/envoy/config/trace/v2/trace.proto | 8 ++++++++ bazel/repositories.bzl | 4 ++++ bazel/repository_locations.bzl | 8 ++++---- source/extensions/tracers/opencensus/BUILD | 1 + .../tracers/opencensus/opencensus_tracer_impl.cc | 6 ++++++ test/extensions/tracers/opencensus/config_test.cc | 2 ++ 7 files changed, 29 insertions(+), 8 deletions(-) diff --git a/api/bazel/repository_locations.bzl b/api/bazel/repository_locations.bzl index 7b0bfeb07539..2febf71148b7 100644 --- a/api/bazel/repository_locations.bzl +++ b/api/bazel/repository_locations.bzl @@ -4,8 +4,8 @@ BAZEL_SKYLIB_SHA256 = "2ef429f5d7ce7111263289644d233707dba35e39696377ebab8b0bc70 GOGOPROTO_RELEASE = "1.2.1" GOGOPROTO_SHA256 = "99e423905ba8921e86817607a5294ffeedb66fdd4a85efce5eb2848f715fdb3a" -OPENCENSUS_PROTO_RELEASE = "0.2.1" -OPENCENSUS_PROTO_SHA256 = "bfcefa6093fc2ecdf5c9effea86e6982d0d6f9a5178b17fcf73a62e0f3fb43d0" +OPENCENSUS_PROTO_GIT_SHA = "5cec5ea58c3efa81fa808f2bd38ce182da9ee731" # Jul 25, 2019 +OPENCENSUS_PROTO_SHA256 = "faeb93f293ff715b0cb530d273901c0e2e99277b9ed1c0a0326bca9ec5774ad2" PGV_GIT_SHA = "2feaabb13a5d697b80fcb938c0ce37b24c9381ee" # Jul 26, 2018 PGV_SHA256 = "ddefe3dcbb25d68a2e5dfea67d19c060959c2aecc782802bd4c1a5811d44dd45" @@ -54,8 +54,8 @@ REPOSITORY_LOCATIONS = dict( ), opencensus_proto = dict( sha256 = OPENCENSUS_PROTO_SHA256, - strip_prefix = "opencensus-proto-" + OPENCENSUS_PROTO_RELEASE + "/src", - urls = ["https://github.com/census-instrumentation/opencensus-proto/archive/v" + OPENCENSUS_PROTO_RELEASE + ".tar.gz"], + strip_prefix = "opencensus-proto-" + OPENCENSUS_PROTO_GIT_SHA + "/src", + urls = ["https://github.com/census-instrumentation/opencensus-proto/archive/" + OPENCENSUS_PROTO_GIT_SHA + ".tar.gz"], ), kafka_source = dict( sha256 = KAFKA_SOURCE_SHA, diff --git a/api/envoy/config/trace/v2/trace.proto b/api/envoy/config/trace/v2/trace.proto index e4e4dec64e95..9da6ef4313d8 100644 --- a/api/envoy/config/trace/v2/trace.proto +++ b/api/envoy/config/trace/v2/trace.proto @@ -136,6 +136,14 @@ message OpenCensusConfig { // The URL to Zipkin, e.g. "http://127.0.0.1:9411/api/v2/spans" string zipkin_url = 6; + // Enables the OpenCensus Agent exporter if set to true. The address must also + // be set. + bool ocagent_exporter_enabled = 11; + + // The address of the OpenCensus Agent, if its exporter is enabled, in gRPC + // format: https://github.com/grpc/grpc/blob/master/doc/naming.md + string ocagent_address = 12; + reserved 7; // Formerly zipkin_service_name. enum TraceContext { diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index 319d374286bd..15dbd7cb0e8c 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -549,6 +549,10 @@ def _io_opencensus_cpp(): name = "opencensus_trace_trace_context", actual = "@io_opencensus_cpp//opencensus/trace:trace_context", ) + native.bind( + name = "opencensus_exporter_ocagent", + actual = "@io_opencensus_cpp//opencensus/exporters/trace/ocagent:ocagent_exporter", + ) native.bind( name = "opencensus_exporter_stdout", actual = "@io_opencensus_cpp//opencensus/exporters/trace/stdout:stdout_exporter", diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 7c2c5ad23e62..2abff095453a 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -232,10 +232,10 @@ REPOSITORY_LOCATIONS = dict( urls = ["https://files.pythonhosted.org/packages/b3/b2/238e2590826bfdd113244a40d9d3eb26918bd798fc187e2360a8367068db/six-1.10.0.tar.gz"], ), io_opencensus_cpp = dict( - sha256 = "8d6016e47c2e19e7acbadb6f905b8c422748c64299d71101ac8f28151677e195", - strip_prefix = "opencensus-cpp-cad0d03ff3474cf14389fc249e16847ab7b6895f", - # 2019-07-31 - urls = ["https://github.com/census-instrumentation/opencensus-cpp/archive/cad0d03ff3474cf14389fc249e16847ab7b6895f.tar.gz"], + sha256 = "b5fcc36a994a4ecb6e53c901e33579ed1ac238cff9975807db760918a15f43ff", + strip_prefix = "opencensus-cpp-8058a1b8efe6a63bd18673abc51223917d12d45d", + # 2019-08-22 + urls = ["https://github.com/census-instrumentation/opencensus-cpp/archive/8058a1b8efe6a63bd18673abc51223917d12d45d.tar.gz"], ), com_github_curl = dict( sha256 = "4376ac72b95572fb6c4fbffefb97c7ea0dd083e1974c0e44cd7e49396f454839", diff --git a/source/extensions/tracers/opencensus/BUILD b/source/extensions/tracers/opencensus/BUILD index c318395cad66..0e7c9085fcab 100644 --- a/source/extensions/tracers/opencensus/BUILD +++ b/source/extensions/tracers/opencensus/BUILD @@ -32,6 +32,7 @@ envoy_cc_library( "opencensus_trace_cloud_trace_context", "opencensus_trace_grpc_trace_bin", "opencensus_trace_trace_context", + "opencensus_exporter_ocagent", "opencensus_exporter_stdout", "opencensus_exporter_stackdriver", "opencensus_exporter_zipkin", diff --git a/source/extensions/tracers/opencensus/opencensus_tracer_impl.cc b/source/extensions/tracers/opencensus/opencensus_tracer_impl.cc index 9be9d3484868..a5562d20c81f 100644 --- a/source/extensions/tracers/opencensus/opencensus_tracer_impl.cc +++ b/source/extensions/tracers/opencensus/opencensus_tracer_impl.cc @@ -8,6 +8,7 @@ #include "absl/strings/str_cat.h" #include "google/devtools/cloudtrace/v2/tracing.grpc.pb.h" +#include "opencensus/exporters/trace/ocagent/ocagent_exporter.h" #include "opencensus/exporters/trace/stackdriver/stackdriver_exporter.h" #include "opencensus/exporters/trace/stdout/stdout_exporter.h" #include "opencensus/exporters/trace/zipkin/zipkin_exporter.h" @@ -257,6 +258,11 @@ Driver::Driver(const envoy::config::trace::v2::OpenCensusConfig& oc_config, opts.service_name = local_info_.clusterName(); ::opencensus::exporters::trace::ZipkinExporter::Register(opts); } + if (oc_config.ocagent_exporter_enabled()) { + ::opencensus::exporters::trace::OcAgentOptions opts; + opts.address = oc_config.ocagent_address(); + ::opencensus::exporters::trace::OcAgentExporter::Register(std::move(opts)); + } } void Driver::applyTraceConfig(const opencensus::proto::trace::v1::TraceConfig& config) { diff --git a/test/extensions/tracers/opencensus/config_test.cc b/test/extensions/tracers/opencensus/config_test.cc index 686f3e5b5813..95cd8767f86b 100644 --- a/test/extensions/tracers/opencensus/config_test.cc +++ b/test/extensions/tracers/opencensus/config_test.cc @@ -50,6 +50,8 @@ TEST(OpenCensusTracerConfigTest, OpenCensusHttpTracerWithTypedConfig) { stackdriver_project_id: test_project_id zipkin_exporter_enabled: true zipkin_url: http://127.0.0.1:9411/api/v2/spans + ocagent_exporter_enabled: true + ocagent_address: 127.0.0.1:55678 incoming_trace_context: b3 incoming_trace_context: trace_context incoming_trace_context: grpc_trace_bin From e1cd4cca46179562cf80de7f81774f4dc6354085 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Tue, 27 Aug 2019 17:07:55 +0200 Subject: [PATCH 09/16] Exporting platform_impl_lib headers (#8045) This allows consuming projects using repository overlaying to disambiguate overlapping include paths when it comes to platform_impl.h by going through envoy/external/... Addendum to #8005 Risk Level: Low Testing: N/A Signed-off-by: Otto van der Schaaf --- source/exe/BUILD | 29 ++++++++++++++++++++++------ source/exe/platform_impl.h | 19 ++++++++++++++++++ source/exe/posix/platform_impl.cc | 12 ++++++++++++ source/exe/posix/platform_impl.h | 18 ----------------- source/exe/win32/platform_impl.cc | 26 +++++++++++++++++++++++++ source/exe/win32/platform_impl.h | 32 ------------------------------- 6 files changed, 80 insertions(+), 56 deletions(-) create mode 100644 source/exe/platform_impl.h create mode 100644 source/exe/posix/platform_impl.cc delete mode 100644 source/exe/posix/platform_impl.h create mode 100644 source/exe/win32/platform_impl.cc delete mode 100644 source/exe/win32/platform_impl.h diff --git a/source/exe/BUILD b/source/exe/BUILD index d48f99ce33b8..7ed716a317a6 100644 --- a/source/exe/BUILD +++ b/source/exe/BUILD @@ -57,7 +57,8 @@ envoy_cc_library( ], deps = [ ":envoy_main_common_lib", - ] + envoy_cc_platform_dep("platform_impl_lib"), + ":platform_impl_lib", + ], ) envoy_cc_library( @@ -66,6 +67,7 @@ envoy_cc_library( hdrs = ["main_common.h"], deps = [ ":envoy_common_lib", + ":platform_impl_lib", ":process_wide_lib", "//source/common/api:os_sys_calls_lib", "//source/common/common:compiler_requirements_lib", @@ -80,7 +82,7 @@ envoy_cc_library( "//source/common/signal:sigaction_lib", ":terminate_handler_lib", ], - }) + envoy_cc_platform_dep("platform_impl_lib"), + }), ) envoy_cc_library( @@ -96,11 +98,26 @@ envoy_cc_library( ] + envoy_google_grpc_external_deps(), ) +envoy_cc_library( + name = "platform_impl_lib", + deps = [":platform_header_lib"] + + envoy_cc_platform_dep("platform_impl_lib"), +) + +envoy_cc_library( + name = "platform_header_lib", + hdrs = ["platform_impl.h"], + deps = [ + "//include/envoy/filesystem:filesystem_interface", + "//include/envoy/thread:thread_interface", + ], +) + envoy_cc_posix_library( name = "platform_impl_lib", - hdrs = ["posix/platform_impl.h"], - strip_include_prefix = "posix", + srcs = ["posix/platform_impl.cc"], deps = [ + ":platform_header_lib", "//source/common/common:thread_lib", "//source/common/filesystem:filesystem_lib", ], @@ -108,9 +125,9 @@ envoy_cc_posix_library( envoy_cc_win32_library( name = "platform_impl_lib", - hdrs = ["win32/platform_impl.h"], - strip_include_prefix = "win32", + srcs = ["win32/platform_impl.cc"], deps = [ + ":platform_header_lib", "//source/common/common:assert_lib", "//source/common/common:thread_lib", "//source/common/filesystem:filesystem_lib", diff --git a/source/exe/platform_impl.h b/source/exe/platform_impl.h new file mode 100644 index 000000000000..c52152c8fe6d --- /dev/null +++ b/source/exe/platform_impl.h @@ -0,0 +1,19 @@ +#pragma once + +#include "envoy/filesystem/filesystem.h" +#include "envoy/thread/thread.h" + +namespace Envoy { + +class PlatformImpl { +public: + PlatformImpl(); + Thread::ThreadFactory& threadFactory() { return *thread_factory_; } + Filesystem::Instance& fileSystem() { return *file_system_; } + +private: + std::unique_ptr thread_factory_; + std::unique_ptr file_system_; +}; + +} // namespace Envoy diff --git a/source/exe/posix/platform_impl.cc b/source/exe/posix/platform_impl.cc new file mode 100644 index 000000000000..f664071815ee --- /dev/null +++ b/source/exe/posix/platform_impl.cc @@ -0,0 +1,12 @@ +#include "common/common/thread_impl.h" +#include "common/filesystem/filesystem_impl.h" + +#include "exe/platform_impl.h" + +namespace Envoy { + +PlatformImpl::PlatformImpl() + : thread_factory_(std::make_unique()), + file_system_(std::make_unique()) {} + +} // namespace Envoy diff --git a/source/exe/posix/platform_impl.h b/source/exe/posix/platform_impl.h deleted file mode 100644 index 45fbd7340779..000000000000 --- a/source/exe/posix/platform_impl.h +++ /dev/null @@ -1,18 +0,0 @@ -#pragma once - -#include "common/common/thread_impl.h" -#include "common/filesystem/filesystem_impl.h" - -namespace Envoy { - -class PlatformImpl { -public: - Thread::ThreadFactory& threadFactory() { return thread_factory_; } - Filesystem::Instance& fileSystem() { return file_system_; } - -private: - Thread::ThreadFactoryImplPosix thread_factory_; - Filesystem::InstanceImplPosix file_system_; -}; - -} // namespace Envoy diff --git a/source/exe/win32/platform_impl.cc b/source/exe/win32/platform_impl.cc new file mode 100644 index 000000000000..860a209a8e05 --- /dev/null +++ b/source/exe/win32/platform_impl.cc @@ -0,0 +1,26 @@ +#include "common/common/assert.h" +#include "common/common/thread_impl.h" +#include "common/filesystem/filesystem_impl.h" + +#include "exe/platform_impl.h" + +// clang-format off +#include +// clang-format on + +namespace Envoy { + +PlatformImpl::PlatformImpl() + : thread_factory_(std::make_unique()), + file_system_(std::make_unique()) { + const WORD wVersionRequested = MAKEWORD(2, 2); + WSADATA wsaData; + const int rc = ::WSAStartup(wVersionRequested, &wsaData); + RELEASE_ASSERT(rc == 0, "WSAStartup failed with error"); +} + +~PlatformImpl() { ::WSACleanup(); } + +}; // namespace Envoy + +} // namespace Envoy diff --git a/source/exe/win32/platform_impl.h b/source/exe/win32/platform_impl.h deleted file mode 100644 index ffb239dd7ebb..000000000000 --- a/source/exe/win32/platform_impl.h +++ /dev/null @@ -1,32 +0,0 @@ -#pragma once - -#include "common/common/assert.h" -#include "common/common/thread_impl.h" -#include "common/filesystem/filesystem_impl.h" - -// clang-format off -#include -// clang-format on - -namespace Envoy { - -class PlatformImpl { -public: - PlatformImpl() { - const WORD wVersionRequested = MAKEWORD(2, 2); - WSADATA wsaData; - const int rc = ::WSAStartup(wVersionRequested, &wsaData); - RELEASE_ASSERT(rc == 0, "WSAStartup failed with error"); - } - - ~PlatformImpl() { ::WSACleanup(); } - - Thread::ThreadFactory& threadFactory() { return thread_factory_; } - Filesystem::Instance& fileSystem() { return file_system_; } - -private: - Thread::ThreadFactoryImplWin32 thread_factory_; - Filesystem::InstanceImplWin32 file_system_; -}; - -} // namespace Envoy From 854e80072f9f602c49b9633e0af3b4689cd3cbbf Mon Sep 17 00:00:00 2001 From: Stephan Zuercher Date: Tue, 27 Aug 2019 10:32:29 -0700 Subject: [PATCH 10/16] access_log: minimal log file error handling (#7938) Rather than ASSERT for a reasonably common error condition (e.g. disk full) record a stat that indicates log file writing failed. Also fixes a test race condition. Risk Level: low Testing: added stats checks Docs Changes: documented new stat Release Notes: updated Signed-off-by: Stephan Zuercher --- .../observability/statistics.rst | 7 +- docs/root/intro/version_history.rst | 1 + .../access_log/access_log_manager_impl.cc | 8 +- .../access_log/access_log_manager_impl.h | 7 +- .../access_log_manager_impl_test.cc | 96 +++++++++++++++++-- test/exe/main_common_test.cc | 8 +- 6 files changed, 107 insertions(+), 20 deletions(-) diff --git a/docs/root/configuration/observability/statistics.rst b/docs/root/configuration/observability/statistics.rst index 0042051e0acb..376263f42bb4 100644 --- a/docs/root/configuration/observability/statistics.rst +++ b/docs/root/configuration/observability/statistics.rst @@ -16,7 +16,7 @@ Server related statistics are rooted at *server.* with following statistics: uptime, Gauge, Current server uptime in seconds concurrency, Gauge, Number of worker threads - memory_allocated, Gauge, Current amount of allocated memory in bytes. Total of both new and old Envoy processes on hot restart. + memory_allocated, Gauge, Current amount of allocated memory in bytes. Total of both new and old Envoy processes on hot restart. memory_heap_size, Gauge, Current reserved heap size in bytes. New Envoy process heap size on hot restart. live, Gauge, "1 if the server is not currently draining, 0 otherwise" state, Gauge, Current :ref:`State ` of the Server. @@ -30,6 +30,8 @@ Server related statistics are rooted at *server.* with following statistics: static_unknown_fields, Counter, Number of messages in static configuration with unknown fields dynamic_unknown_fields, Counter, Number of messages in dynamic configuration with unknown fields +.. _filesystem_stats: + File system ----------- @@ -40,7 +42,8 @@ Statistics related to file system are emitted in the *filesystem.* namespace. :widths: 1, 1, 2 write_buffered, Counter, Total number of times file data is moved to Envoy's internal flush buffer - write_completed, Counter, Total number of times a file was written + write_completed, Counter, Total number of times a file was successfully written + write_failed, Counter, Total number of times an error occurred during a file write operation flushed_by_timer, Counter, Total number of times internal flush buffers are written to a file due to flush timeout reopen_failed, Counter, Total number of times a file was failed to be opened write_total_buffered, Gauge, Current total size of internal flush buffer in bytes diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 63fb953c060a..5152e89fdc65 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -5,6 +5,7 @@ Version history ================ * access log: added :ref:`buffering ` and :ref:`periodical flushing ` support to gRPC access logger. Defaults to 16KB buffer and flushing every 1 second. * access log: gRPC Access Log Service (ALS) support added for :ref:`TCP access logs `. +* access log: reintroduce :ref:`filesystem ` stats and added the `write_failed` counter to track failed log writes * admin: added ability to configure listener :ref:`socket options `. * admin: added config dump support for Secret Discovery Service :ref:`SecretConfigDump `. * api: added ::ref:`set_node_on_first_message_only ` option to omit the node identifier from the subsequent discovery requests on the same stream. diff --git a/source/common/access_log/access_log_manager_impl.cc b/source/common/access_log/access_log_manager_impl.cc index cf5a98daaf47..fd576b9b00e1 100644 --- a/source/common/access_log/access_log_manager_impl.cc +++ b/source/common/access_log/access_log_manager_impl.cc @@ -100,8 +100,12 @@ void AccessLogFileImpl::doWrite(Buffer::Instance& buffer) { for (const Buffer::RawSlice& slice : slices) { absl::string_view data(static_cast(slice.mem_), slice.len_); const Api::IoCallSizeResult result = file_->write(data); - ASSERT(result.rc_ == static_cast(slice.len_)); - stats_.write_completed_.inc(); + if (result.ok() && result.rc_ == static_cast(slice.len_)) { + stats_.write_completed_.inc(); + } else { + // Probably disk full. + stats_.write_failed_.inc(); + } } } diff --git a/source/common/access_log/access_log_manager_impl.h b/source/common/access_log/access_log_manager_impl.h index 03efbf2d030b..64c9c2594b49 100644 --- a/source/common/access_log/access_log_manager_impl.h +++ b/source/common/access_log/access_log_manager_impl.h @@ -20,6 +20,7 @@ namespace Envoy { COUNTER(reopen_failed) \ COUNTER(write_buffered) \ COUNTER(write_completed) \ + COUNTER(write_failed) \ GAUGE(write_total_buffered, Accumulate) struct AccessLogFileStats { @@ -34,9 +35,9 @@ class AccessLogManagerImpl : public AccessLogManager { Event::Dispatcher& dispatcher, Thread::BasicLockable& lock, Stats::Store& stats_store) : file_flush_interval_msec_(file_flush_interval_msec), api_(api), dispatcher_(dispatcher), - lock_(lock), file_stats_{ACCESS_LOG_FILE_STATS( - POOL_COUNTER_PREFIX(stats_store, "access_log_file."), - POOL_GAUGE_PREFIX(stats_store, "access_log_file."))} {} + lock_(lock), file_stats_{ + ACCESS_LOG_FILE_STATS(POOL_COUNTER_PREFIX(stats_store, "filesystem."), + POOL_GAUGE_PREFIX(stats_store, "filesystem."))} {} // AccessLog::AccessLogManager void reopen() override; diff --git a/test/common/access_log/access_log_manager_impl_test.cc b/test/common/access_log/access_log_manager_impl_test.cc index d8d9446d856f..3b28ee6526a2 100644 --- a/test/common/access_log/access_log_manager_impl_test.cc +++ b/test/common/access_log/access_log_manager_impl_test.cc @@ -8,6 +8,8 @@ #include "test/mocks/api/mocks.h" #include "test/mocks/event/mocks.h" #include "test/mocks/filesystem/mocks.h" +#include "test/test_common/test_time.h" +#include "test/test_common/utility.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -37,6 +39,14 @@ class AccessLogManagerImplTest : public testing::Test { EXPECT_CALL(api_, threadFactory()).WillRepeatedly(ReturnRef(thread_factory_)); } + void waitForCounterEq(const std::string& name, uint64_t value) { + TestUtility::waitForCounterEq(store_, name, value, time_system_); + } + + void waitForGaugeEq(const std::string& name, uint64_t value) { + TestUtility::waitForGaugeEq(store_, name, value, time_system_); + } + NiceMock api_; NiceMock file_system_; NiceMock* file_; @@ -46,6 +56,7 @@ class AccessLogManagerImplTest : public testing::Test { NiceMock dispatcher_; Thread::MutexBasicLockable lock_; AccessLogManagerImpl access_log_manager_; + Event::TestRealTimeSystem time_system_; }; TEST_F(AccessLogManagerImplTest, BadFile) { @@ -74,9 +85,18 @@ TEST_F(AccessLogManagerImplTest, flushToLogFilePeriodically) { EXPECT_CALL(*file_, open_(_)).WillOnce(Return(ByMove(Filesystem::resultSuccess(true)))); AccessLogFileSharedPtr log_file = access_log_manager_.createAccessLog("foo"); + EXPECT_EQ(0UL, store_.counter("filesystem.write_failed").value()); + EXPECT_EQ(0UL, store_.counter("filesystem.write_completed").value()); + EXPECT_EQ(0UL, store_.counter("filesystem.flushed_by_timer").value()); + EXPECT_EQ(0UL, store_.counter("filesystem.write_buffered").value()); + EXPECT_CALL(*timer, enableTimer(timeout_40ms_, _)); EXPECT_CALL(*file_, write_(_)) - .WillOnce(Invoke([](absl::string_view data) -> Api::IoCallSizeResult { + .WillOnce(Invoke([&](absl::string_view data) -> Api::IoCallSizeResult { + EXPECT_EQ( + 4UL, + store_.gauge("filesystem.write_total_buffered", Stats::Gauge::ImportMode::Accumulate) + .value()); EXPECT_EQ(0, data.compare("test")); return Filesystem::resultSuccess(static_cast(data.length())); })); @@ -90,14 +110,25 @@ TEST_F(AccessLogManagerImplTest, flushToLogFilePeriodically) { } } + waitForCounterEq("filesystem.write_completed", 1); + EXPECT_EQ(1UL, store_.counter("filesystem.write_buffered").value()); + EXPECT_EQ(0UL, store_.counter("filesystem.flushed_by_timer").value()); + waitForGaugeEq("filesystem.write_total_buffered", 0); + EXPECT_CALL(*file_, write_(_)) - .WillOnce(Invoke([](absl::string_view data) -> Api::IoCallSizeResult { + .WillOnce(Invoke([&](absl::string_view data) -> Api::IoCallSizeResult { + EXPECT_EQ( + 5UL, + store_.gauge("filesystem.write_total_buffered", Stats::Gauge::ImportMode::Accumulate) + .value()); EXPECT_EQ(0, data.compare("test2")); return Filesystem::resultSuccess(static_cast(data.length())); })); - // make sure timer is re-enabled on callback call log_file->write("test2"); + EXPECT_EQ(2UL, store_.counter("filesystem.write_buffered").value()); + + // make sure timer is re-enabled on callback call EXPECT_CALL(*timer, enableTimer(timeout_40ms_, _)); timer->invokeCallback(); @@ -107,6 +138,13 @@ TEST_F(AccessLogManagerImplTest, flushToLogFilePeriodically) { file_->write_event_.wait(file_->write_mutex_); } } + + waitForCounterEq("filesystem.write_completed", 2); + EXPECT_EQ(0UL, store_.counter("filesystem.write_failed").value()); + EXPECT_EQ(1UL, store_.counter("filesystem.flushed_by_timer").value()); + EXPECT_EQ(2UL, store_.counter("filesystem.write_buffered").value()); + waitForGaugeEq("filesystem.write_total_buffered", 0); + EXPECT_CALL(*file_, close_()).WillOnce(Return(ByMove(Filesystem::resultSuccess(true)))); } @@ -116,21 +154,24 @@ TEST_F(AccessLogManagerImplTest, flushToLogFileOnDemand) { EXPECT_CALL(*file_, open_(_)).WillOnce(Return(ByMove(Filesystem::resultSuccess(true)))); AccessLogFileSharedPtr log_file = access_log_manager_.createAccessLog("foo"); + EXPECT_EQ(0UL, store_.counter("filesystem.flushed_by_timer").value()); + EXPECT_CALL(*timer, enableTimer(timeout_40ms_, _)); - // The first write to a given file will start the flush thread, which can flush - // immediately (race on whether it will or not). So do a write and flush to - // get that state out of the way, then test that small writes don't trigger a flush. + // The first write to a given file will start the flush thread. Because AccessManagerImpl::write + // holds the write_lock_ when the thread is started, the thread will flush on its first loop, once + // it obtains the write_lock_. Perform a write to get all that out of the way. EXPECT_CALL(*file_, write_(_)) .WillOnce(Invoke([](absl::string_view data) -> Api::IoCallSizeResult { return Filesystem::resultSuccess(static_cast(data.length())); })); log_file->write("prime-it"); - log_file->flush(); uint32_t expected_writes = 1; { Thread::LockGuard lock(file_->write_mutex_); - EXPECT_EQ(expected_writes, file_->num_writes_); + while (file_->num_writes_ != expected_writes) { + file_->write_event_.wait(file_->write_mutex_); + } } EXPECT_CALL(*file_, write_(_)) @@ -150,9 +191,14 @@ TEST_F(AccessLogManagerImplTest, flushToLogFileOnDemand) { expected_writes++; { Thread::LockGuard lock(file_->write_mutex_); - EXPECT_EQ(expected_writes, file_->num_writes_); + while (file_->num_writes_ != expected_writes) { + file_->write_event_.wait(file_->write_mutex_); + } } + waitForCounterEq("filesystem.write_completed", 2); + EXPECT_EQ(0UL, store_.counter("filesystem.flushed_by_timer").value()); + EXPECT_CALL(*file_, write_(_)) .WillOnce(Invoke([](absl::string_view data) -> Api::IoCallSizeResult { EXPECT_EQ(0, data.compare("test2")); @@ -174,6 +220,36 @@ TEST_F(AccessLogManagerImplTest, flushToLogFileOnDemand) { EXPECT_CALL(*file_, close_()).WillOnce(Return(ByMove(Filesystem::resultSuccess(true)))); } +TEST_F(AccessLogManagerImplTest, flushCountsIOErrors) { + NiceMock* timer = new NiceMock(&dispatcher_); + + EXPECT_CALL(*file_, open_(_)).WillOnce(Return(ByMove(Filesystem::resultSuccess(true)))); + AccessLogFileSharedPtr log_file = access_log_manager_.createAccessLog("foo"); + + EXPECT_EQ(0UL, store_.counter("filesystem.write_failed").value()); + + EXPECT_CALL(*timer, enableTimer(timeout_40ms_, _)); + EXPECT_CALL(*file_, write_(_)) + .WillOnce(Invoke([](absl::string_view data) -> Api::IoCallSizeResult { + EXPECT_EQ(0, data.compare("test")); + return Filesystem::resultFailure(2UL, ENOSPC); + })); + + log_file->write("test"); + + { + Thread::LockGuard lock(file_->write_mutex_); + while (file_->num_writes_ != 1) { + file_->write_event_.wait(file_->write_mutex_); + } + } + + waitForCounterEq("filesystem.write_failed", 1); + EXPECT_EQ(0UL, store_.counter("filesystem.write_completed").value()); + + EXPECT_CALL(*file_, close_()).WillOnce(Return(ByMove(Filesystem::resultSuccess(true)))); +} + TEST_F(AccessLogManagerImplTest, reopenFile) { NiceMock* timer = new NiceMock(&dispatcher_); @@ -274,6 +350,8 @@ TEST_F(AccessLogManagerImplTest, reopenThrows) { // write call should not cause any exceptions log_file->write("random data"); timer->invokeCallback(); + + waitForCounterEq("filesystem.reopen_failed", 1); } TEST_F(AccessLogManagerImplTest, bigDataChunkShouldBeFlushedWithoutTimer) { diff --git a/test/exe/main_common_test.cc b/test/exe/main_common_test.cc index 4dadcb31875f..7af5aee32c6a 100644 --- a/test/exe/main_common_test.cc +++ b/test/exe/main_common_test.cc @@ -240,7 +240,7 @@ class AdminRequestTest : public MainCommonTest { TEST_P(AdminRequestTest, AdminRequestGetStatsAndQuit) { startEnvoy(); started_.WaitForNotification(); - EXPECT_THAT(adminRequest("/stats", "GET"), HasSubstr("access_log_file.reopen_failed")); + EXPECT_THAT(adminRequest("/stats", "GET"), HasSubstr("filesystem.reopen_failed")); adminRequest("/quitquitquit", "POST"); EXPECT_TRUE(waitForEnvoyToExit()); } @@ -253,7 +253,7 @@ TEST_P(AdminRequestTest, AdminRequestGetStatsAndKill) { // TODO(htuch): Remove when https://github.com/libevent/libevent/issues/779 is // fixed, started_ will then become our real synchronization point. waitForEnvoyRun(); - EXPECT_THAT(adminRequest("/stats", "GET"), HasSubstr("access_log_file.reopen_failed")); + EXPECT_THAT(adminRequest("/stats", "GET"), HasSubstr("filesystem.reopen_failed")); kill(getpid(), SIGTERM); EXPECT_TRUE(waitForEnvoyToExit()); } @@ -266,7 +266,7 @@ TEST_P(AdminRequestTest, AdminRequestGetStatsAndCtrlC) { // TODO(htuch): Remove when https://github.com/libevent/libevent/issues/779 is // fixed, started_ will then become our real synchronization point. waitForEnvoyRun(); - EXPECT_THAT(adminRequest("/stats", "GET"), HasSubstr("access_log_file.reopen_failed")); + EXPECT_THAT(adminRequest("/stats", "GET"), HasSubstr("filesystem.reopen_failed")); kill(getpid(), SIGINT); EXPECT_TRUE(waitForEnvoyToExit()); } @@ -335,7 +335,7 @@ TEST_P(AdminRequestTest, AdminRequestBeforeRun) { EXPECT_TRUE(admin_handler_was_called); // This just checks that some stat output was reported. We could pick any stat. - EXPECT_THAT(out, HasSubstr("access_log_file.reopen_failed")); + EXPECT_THAT(out, HasSubstr("filesystem.reopen_failed")); } // Class to track whether an object has been destroyed, which it does by bumping an atomic. From 1fc6c6e7cb15697e7cc8b99b7a39969f265130bd Mon Sep 17 00:00:00 2001 From: Caleb Gilmour Date: Wed, 28 Aug 2019 05:34:37 +1200 Subject: [PATCH 11/16] tracing: add grpc-status and grpc-message to spans (#7996) Signed-off-by: Caleb Gilmour --- .../arch_overview/observability/tracing.rst | 9 +- docs/root/intro/version_history.rst | 1 + source/common/http/conn_manager_impl.cc | 5 +- source/common/tracing/http_tracer_impl.cc | 26 +++ source/common/tracing/http_tracer_impl.h | 3 + test/common/tracing/http_tracer_impl_test.cc | 148 ++++++++++++++++-- 6 files changed, 176 insertions(+), 16 deletions(-) diff --git a/docs/root/intro/arch_overview/observability/tracing.rst b/docs/root/intro/arch_overview/observability/tracing.rst index bd8016424d9b..24072465e26b 100644 --- a/docs/root/intro/arch_overview/observability/tracing.rst +++ b/docs/root/intro/arch_overview/observability/tracing.rst @@ -93,9 +93,12 @@ associated with it. Each span generated by Envoy contains the following data: * Originating host set via :option:`--service-node`. * Downstream cluster set via the :ref:`config_http_conn_man_headers_downstream-service-cluster` header. -* HTTP URL. -* HTTP method. -* HTTP response code. +* HTTP request URL, method, protocol and user-agent. +* Additional HTTP request headers set via :ref:`request_headers_for_tags + ` +* HTTP response status code. +* GRPC response status and message (if available). +* An error tag when HTTP status is 5xx or GRPC status is not "OK" * Tracing system-specific metadata. The span also includes a name (or operation) which by default is defined as the host of the invoked diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 5152e89fdc65..147c65f5ce60 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -46,6 +46,7 @@ Version history * router check tool: add comprehensive coverage reporting. * tls: added verification of IP address SAN fields in certificates against configured SANs in the certificate validation context. +* tracing: added tags for gRPC response status and meesage. * upstream: added network filter chains to upstream connections, see :ref:`filters`. * upstream: use p2c to select hosts for least-requests load balancers if all host weights are the same, even in cases where weights are not equal to 1. * upstream: added :ref:`an option ` that allows draining HTTP, TCP connection pools on cluster membership change. diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index e5d7fe942155..580ba1e6c365 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -502,8 +502,9 @@ ConnectionManagerImpl::ActiveStream::~ActiveStream() { } if (active_span_) { - Tracing::HttpTracerUtility::finalizeSpan(*active_span_, request_headers_.get(), stream_info_, - *this); + Tracing::HttpTracerUtility::finalizeSpan(*active_span_, request_headers_.get(), + response_headers_.get(), response_trailers_.get(), + stream_info_, *this); } if (state_.successful_upgrade_) { connection_manager_.stats_.named_.downstream_cx_upgrades_active_.dec(); diff --git a/source/common/tracing/http_tracer_impl.cc b/source/common/tracing/http_tracer_impl.cc index 46ff74ac58a1..004e41c84a86 100644 --- a/source/common/tracing/http_tracer_impl.cc +++ b/source/common/tracing/http_tracer_impl.cc @@ -7,6 +7,7 @@ #include "common/common/fmt.h" #include "common/common/macros.h" #include "common/common/utility.h" +#include "common/grpc/common.h" #include "common/http/codes.h" #include "common/http/header_map_impl.h" #include "common/http/headers.h" @@ -81,6 +82,22 @@ Decision HttpTracerUtility::isTracing(const StreamInfo::StreamInfo& stream_info, NOT_REACHED_GCOVR_EXCL_LINE; } +static void addGrpcTags(Span& span, const Http::HeaderMap& headers) { + const Http::HeaderEntry* grpc_status_header = headers.GrpcStatus(); + if (grpc_status_header) { + span.setTag(Tracing::Tags::get().GrpcStatusCode, grpc_status_header->value().getStringView()); + } + const Http::HeaderEntry* grpc_message_header = headers.GrpcMessage(); + if (grpc_message_header) { + span.setTag(Tracing::Tags::get().GrpcMessage, grpc_message_header->value().getStringView()); + } + absl::optional grpc_status_code = Grpc::Common::getGrpcStatus(headers); + // Set error tag when status is not OK. + if (grpc_status_code && grpc_status_code.value() != Grpc::Status::GrpcStatus::Ok) { + span.setTag(Tracing::Tags::get().Error, Tracing::Tags::get().True); + } +} + static void annotateVerbose(Span& span, const StreamInfo::StreamInfo& stream_info) { const auto start_time = stream_info.startTime(); if (stream_info.lastDownstreamRxByteReceived()) { @@ -121,6 +138,8 @@ static void annotateVerbose(Span& span, const StreamInfo::StreamInfo& stream_inf } void HttpTracerUtility::finalizeSpan(Span& span, const Http::HeaderMap* request_headers, + const Http::HeaderMap* response_headers, + const Http::HeaderMap* response_trailers, const StreamInfo::StreamInfo& stream_info, const Config& tracing_config) { // Pre response data. @@ -163,6 +182,13 @@ void HttpTracerUtility::finalizeSpan(Span& span, const Http::HeaderMap* request_ span.setTag(Tracing::Tags::get().ResponseFlags, StreamInfo::ResponseFlagUtils::toShortString(stream_info)); + // GRPC data. + if (response_trailers && response_trailers->GrpcStatus() != nullptr) { + addGrpcTags(span, *response_trailers); + } else if (response_headers && response_headers->GrpcStatus() != nullptr) { + addGrpcTags(span, *response_headers); + } + if (tracing_config.verbose()) { annotateVerbose(span, stream_info); } diff --git a/source/common/tracing/http_tracer_impl.h b/source/common/tracing/http_tracer_impl.h index 50a782146043..ec421736eab4 100644 --- a/source/common/tracing/http_tracer_impl.h +++ b/source/common/tracing/http_tracer_impl.h @@ -41,6 +41,7 @@ class TracingTagValues { // Non-standard tag names. const std::string DownstreamCluster = "downstream_cluster"; const std::string GrpcStatusCode = "grpc.status_code"; + const std::string GrpcMessage = "grpc.message"; const std::string GuidXClientTraceId = "guid:x-client-trace-id"; const std::string GuidXRequestId = "guid:x-request-id"; const std::string HttpProtocol = "http.protocol"; @@ -101,6 +102,8 @@ class HttpTracerUtility { * 2) Finish active span. */ static void finalizeSpan(Span& span, const Http::HeaderMap* request_headers, + const Http::HeaderMap* response_headers, + const Http::HeaderMap* response_trailers, const StreamInfo::StreamInfo& stream_info, const Config& tracing_config); static const std::string IngressOperation; diff --git a/test/common/tracing/http_tracer_impl_test.cc b/test/common/tracing/http_tracer_impl_test.cc index 4aeee65fc6ac..4e1b991507f3 100644 --- a/test/common/tracing/http_tracer_impl_test.cc +++ b/test/common/tracing/http_tracer_impl_test.cc @@ -122,12 +122,14 @@ TEST(HttpConnManFinalizerImpl, OriginalAndLongPath) { {"x-envoy-original-path", path}, {":method", "GET"}, {"x-forwarded-proto", "http"}}; + Http::TestHeaderMapImpl response_headers; + Http::TestHeaderMapImpl response_trailers; NiceMock stream_info; absl::optional protocol = Http::Protocol::Http2; EXPECT_CALL(stream_info, bytesReceived()).WillOnce(Return(10)); EXPECT_CALL(stream_info, bytesSent()).WillOnce(Return(11)); - EXPECT_CALL(stream_info, protocol()).WillOnce(ReturnPointee(&protocol)); + EXPECT_CALL(stream_info, protocol()).WillRepeatedly(ReturnPointee(&protocol)); absl::optional response_code; EXPECT_CALL(stream_info, responseCode()).WillRepeatedly(ReturnPointee(&response_code)); @@ -137,7 +139,8 @@ TEST(HttpConnManFinalizerImpl, OriginalAndLongPath) { EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpProtocol), Eq("HTTP/2"))); NiceMock config; - HttpTracerUtility::finalizeSpan(span, &request_headers, stream_info, config); + HttpTracerUtility::finalizeSpan(span, &request_headers, &response_headers, &response_trailers, + stream_info, config); } TEST(HttpConnManFinalizerImpl, NoGeneratedId) { @@ -148,12 +151,14 @@ TEST(HttpConnManFinalizerImpl, NoGeneratedId) { Http::TestHeaderMapImpl request_headers{ {"x-envoy-original-path", path}, {":method", "GET"}, {"x-forwarded-proto", "http"}}; + Http::TestHeaderMapImpl response_headers; + Http::TestHeaderMapImpl response_trailers; NiceMock stream_info; absl::optional protocol = Http::Protocol::Http2; EXPECT_CALL(stream_info, bytesReceived()).WillOnce(Return(10)); EXPECT_CALL(stream_info, bytesSent()).WillOnce(Return(11)); - EXPECT_CALL(stream_info, protocol()).WillOnce(ReturnPointee(&protocol)); + EXPECT_CALL(stream_info, protocol()).WillRepeatedly(ReturnPointee(&protocol)); absl::optional response_code; EXPECT_CALL(stream_info, responseCode()).WillRepeatedly(ReturnPointee(&response_code)); @@ -163,7 +168,8 @@ TEST(HttpConnManFinalizerImpl, NoGeneratedId) { EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpProtocol), Eq("HTTP/2"))); NiceMock config; - HttpTracerUtility::finalizeSpan(span, &request_headers, stream_info, config); + HttpTracerUtility::finalizeSpan(span, &request_headers, &response_headers, &response_trailers, + stream_info, config); } TEST(HttpConnManFinalizerImpl, NullRequestHeaders) { @@ -184,7 +190,7 @@ TEST(HttpConnManFinalizerImpl, NullRequestHeaders) { EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().UpstreamCluster), _)).Times(0); NiceMock config; - HttpTracerUtility::finalizeSpan(span, nullptr, stream_info, config); + HttpTracerUtility::finalizeSpan(span, nullptr, nullptr, nullptr, stream_info, config); } TEST(HttpConnManFinalizerImpl, StreamInfoLogs) { @@ -222,7 +228,7 @@ TEST(HttpConnManFinalizerImpl, StreamInfoLogs) { NiceMock config; EXPECT_CALL(config, verbose).WillOnce(Return(true)); - HttpTracerUtility::finalizeSpan(span, nullptr, stream_info, config); + HttpTracerUtility::finalizeSpan(span, nullptr, nullptr, nullptr, stream_info, config); } TEST(HttpConnManFinalizerImpl, UpstreamClusterTagSet) { @@ -244,7 +250,7 @@ TEST(HttpConnManFinalizerImpl, UpstreamClusterTagSet) { EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().RequestSize), Eq("10"))); NiceMock config; - HttpTracerUtility::finalizeSpan(span, nullptr, stream_info, config); + HttpTracerUtility::finalizeSpan(span, nullptr, nullptr, nullptr, stream_info, config); } TEST(HttpConnManFinalizerImpl, SpanOptionalHeaders) { @@ -254,11 +260,13 @@ TEST(HttpConnManFinalizerImpl, SpanOptionalHeaders) { {":path", "/test"}, {":method", "GET"}, {"x-forwarded-proto", "https"}}; + Http::TestHeaderMapImpl response_headers; + Http::TestHeaderMapImpl response_trailers; NiceMock stream_info; absl::optional protocol = Http::Protocol::Http10; EXPECT_CALL(stream_info, bytesReceived()).WillOnce(Return(10)); - EXPECT_CALL(stream_info, protocol()).WillOnce(ReturnPointee(&protocol)); + EXPECT_CALL(stream_info, protocol()).WillRepeatedly(ReturnPointee(&protocol)); const std::string service_node = "i-453"; // Check that span is populated correctly. @@ -282,7 +290,8 @@ TEST(HttpConnManFinalizerImpl, SpanOptionalHeaders) { EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().UpstreamCluster), _)).Times(0); NiceMock config; - HttpTracerUtility::finalizeSpan(span, &request_headers, stream_info, config); + HttpTracerUtility::finalizeSpan(span, &request_headers, &response_headers, &response_trailers, + stream_info, config); } TEST(HttpConnManFinalizerImpl, SpanPopulatedFailureResponse) { @@ -291,6 +300,8 @@ TEST(HttpConnManFinalizerImpl, SpanPopulatedFailureResponse) { {":path", "/test"}, {":method", "GET"}, {"x-forwarded-proto", "http"}}; + Http::TestHeaderMapImpl response_headers; + Http::TestHeaderMapImpl response_trailers; NiceMock stream_info; request_headers.insertHost().value(std::string("api")); @@ -299,7 +310,7 @@ TEST(HttpConnManFinalizerImpl, SpanPopulatedFailureResponse) { request_headers.insertClientTraceId().value(std::string("client_trace_id")); absl::optional protocol = Http::Protocol::Http10; - EXPECT_CALL(stream_info, protocol()).WillOnce(ReturnPointee(&protocol)); + EXPECT_CALL(stream_info, protocol()).WillRepeatedly(ReturnPointee(&protocol)); EXPECT_CALL(stream_info, bytesReceived()).WillOnce(Return(10)); const std::string service_node = "i-453"; @@ -339,7 +350,118 @@ TEST(HttpConnManFinalizerImpl, SpanPopulatedFailureResponse) { EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().ResponseFlags), Eq("UT"))); EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().UpstreamCluster), _)).Times(0); - HttpTracerUtility::finalizeSpan(span, &request_headers, stream_info, config); + HttpTracerUtility::finalizeSpan(span, &request_headers, &response_headers, &response_trailers, + stream_info, config); +} + +TEST(HttpConnManFinalizerImpl, GrpcOkStatus) { + const std::string path_prefix = "http://"; + NiceMock span; + + Http::TestHeaderMapImpl request_headers{{":method", "POST"}, + {":scheme", "http"}, + {":path", "/pb.Foo/Bar"}, + {":authority", "example.com:80"}, + {"content-type", "application/grpc"}, + {"te", "trailers"}}; + + Http::TestHeaderMapImpl response_headers{{":status", "200"}, + {"content-type", "application/grpc"}}; + Http::TestHeaderMapImpl response_trailers{{"grpc-status", "0"}, {"grpc-message", ""}}; + NiceMock stream_info; + + absl::optional protocol = Http::Protocol::Http2; + absl::optional response_code(200); + EXPECT_CALL(stream_info, responseCode()).WillRepeatedly(ReturnPointee(&response_code)); + EXPECT_CALL(stream_info, bytesReceived()).WillOnce(Return(10)); + EXPECT_CALL(stream_info, bytesSent()).WillOnce(Return(11)); + EXPECT_CALL(stream_info, protocol()).WillRepeatedly(ReturnPointee(&protocol)); + + EXPECT_CALL(span, setTag(_, _)).Times(testing::AnyNumber()); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpMethod), Eq("POST"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpProtocol), Eq("HTTP/2"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpStatusCode), Eq("200"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GrpcStatusCode), Eq("0"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GrpcMessage), Eq(""))); + + NiceMock config; + HttpTracerUtility::finalizeSpan(span, &request_headers, &response_headers, &response_trailers, + stream_info, config); +} + +TEST(HttpConnManFinalizerImpl, GrpcErrorTag) { + const std::string path_prefix = "http://"; + NiceMock span; + + Http::TestHeaderMapImpl request_headers{{":method", "POST"}, + {":scheme", "http"}, + {":path", "/pb.Foo/Bar"}, + {":authority", "example.com:80"}, + {"content-type", "application/grpc"}, + {"te", "trailers"}}; + + Http::TestHeaderMapImpl response_headers{{":status", "200"}, + {"content-type", "application/grpc"}}; + Http::TestHeaderMapImpl response_trailers{{"grpc-status", "7"}, + {"grpc-message", "permission denied"}}; + NiceMock stream_info; + + absl::optional protocol = Http::Protocol::Http2; + absl::optional response_code(200); + EXPECT_CALL(stream_info, responseCode()).WillRepeatedly(ReturnPointee(&response_code)); + EXPECT_CALL(stream_info, bytesReceived()).WillOnce(Return(10)); + EXPECT_CALL(stream_info, bytesSent()).WillOnce(Return(11)); + EXPECT_CALL(stream_info, protocol()).WillRepeatedly(ReturnPointee(&protocol)); + + EXPECT_CALL(span, setTag(_, _)).Times(testing::AnyNumber()); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().Error), Eq(Tracing::Tags::get().True))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpMethod), Eq("POST"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpProtocol), Eq("HTTP/2"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpStatusCode), Eq("200"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GrpcStatusCode), Eq("7"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GrpcMessage), Eq("permission denied"))); + + NiceMock config; + HttpTracerUtility::finalizeSpan(span, &request_headers, &response_headers, &response_trailers, + stream_info, config); +} + +TEST(HttpConnManFinalizerImpl, GrpcTrailersOnly) { + const std::string path_prefix = "http://"; + NiceMock span; + + Http::TestHeaderMapImpl request_headers{{":method", "POST"}, + {":scheme", "http"}, + {":path", "/pb.Foo/Bar"}, + {":authority", "example.com:80"}, + {"content-type", "application/grpc"}, + {"te", "trailers"}}; + + Http::TestHeaderMapImpl response_headers{{":status", "200"}, + {"content-type", "application/grpc"}, + {"grpc-status", "7"}, + {"grpc-message", "permission denied"}}; + Http::TestHeaderMapImpl response_trailers; + NiceMock stream_info; + + absl::optional protocol = Http::Protocol::Http2; + absl::optional response_code(200); + EXPECT_CALL(stream_info, responseCode()).WillRepeatedly(ReturnPointee(&response_code)); + EXPECT_CALL(stream_info, bytesReceived()).WillOnce(Return(10)); + EXPECT_CALL(stream_info, bytesSent()).WillOnce(Return(11)); + EXPECT_CALL(stream_info, protocol()).WillRepeatedly(ReturnPointee(&protocol)); + + EXPECT_CALL(span, setTag(_, _)).Times(testing::AnyNumber()); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().Error), Eq(Tracing::Tags::get().True))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpMethod), Eq("POST"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpProtocol), Eq("HTTP/2"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpStatusCode), Eq("200"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GrpcStatusCode), Eq("7"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GrpcMessage), Eq("permission denied"))); + + NiceMock config; + HttpTracerUtility::finalizeSpan(span, &request_headers, &response_headers, &response_trailers, + stream_info, config); } TEST(HttpTracerUtilityTest, operationTypeToString) { @@ -352,6 +474,8 @@ TEST(HttpNullTracerTest, BasicFunctionality) { MockConfig config; StreamInfo::MockStreamInfo stream_info; Http::TestHeaderMapImpl request_headers; + Http::TestHeaderMapImpl response_headers; + Http::TestHeaderMapImpl response_trailers; SpanPtr span_ptr = null_tracer.startSpan(config, request_headers, stream_info, {Reason::Sampling, true}); @@ -374,6 +498,8 @@ class HttpTracerImplTest : public testing::Test { Http::TestHeaderMapImpl request_headers_{ {":path", "/"}, {":method", "GET"}, {"x-request-id", "foo"}, {":authority", "test"}}; + Http::TestHeaderMapImpl response_headers; + Http::TestHeaderMapImpl response_trailers; StreamInfo::MockStreamInfo stream_info_; NiceMock local_info_; MockConfig config_; From 0a3fc6a091a768ba868ff4dd94cf526af4ffc0ff Mon Sep 17 00:00:00 2001 From: asraa Date: Tue, 27 Aug 2019 13:38:40 -0400 Subject: [PATCH 12/16] fuzz: add bounds to statsh flush interval (#8043) Add PGV bounds to the stats flush interval (greater than 1ms and less than 5000ms) to prevent Envoy from hanging from too small of a flush time. Risk Level: Low Testing: Corpus Entry added Fixes OSS-Fuzz issue https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=16300 Signed-off-by: Asra Ali --- api/envoy/config/bootstrap/v2/bootstrap.proto | 9 ++++++++- ...-testcase-server_fuzz_test-5734693923717120 | 18 ++++++++++++++++++ test/server/server_fuzz_test.cc | 2 ++ 3 files changed, 28 insertions(+), 1 deletion(-) create mode 100644 test/server/server_corpus/clusterfuzz-testcase-server_fuzz_test-5734693923717120 diff --git a/api/envoy/config/bootstrap/v2/bootstrap.proto b/api/envoy/config/bootstrap/v2/bootstrap.proto index 477aeac5a141..9e3d9fe1cdb1 100644 --- a/api/envoy/config/bootstrap/v2/bootstrap.proto +++ b/api/envoy/config/bootstrap/v2/bootstrap.proto @@ -99,7 +99,14 @@ message Bootstrap { // performance reasons Envoy latches counters and only flushes counters and // gauges at a periodic interval. If not specified the default is 5000ms (5 // seconds). - google.protobuf.Duration stats_flush_interval = 7 [(gogoproto.stdduration) = true]; + // Duration must be at least 1ms and at most 5 min. + google.protobuf.Duration stats_flush_interval = 7 [ + (validate.rules).duration = { + lt: {seconds: 300}, + gte: {nanos: 1000000} + }, + (gogoproto.stdduration) = true + ]; // Optional watchdog configuration. Watchdog watchdog = 8; diff --git a/test/server/server_corpus/clusterfuzz-testcase-server_fuzz_test-5734693923717120 b/test/server/server_corpus/clusterfuzz-testcase-server_fuzz_test-5734693923717120 new file mode 100644 index 000000000000..187e397539d2 --- /dev/null +++ b/test/server/server_corpus/clusterfuzz-testcase-server_fuzz_test-5734693923717120 @@ -0,0 +1,18 @@ +static_resources { + clusters { + name: "@" + connect_timeout { + nanos: 250000000 + } + common_lb_config { + zone_aware_lb_config { + min_cluster_size { + value: 38 + } + } + } + } +} +stats_flush_interval { + nanos: 32256 +} diff --git a/test/server/server_fuzz_test.cc b/test/server/server_fuzz_test.cc index 2ee39d2f8d88..b47f1aed9622 100644 --- a/test/server/server_fuzz_test.cc +++ b/test/server/server_fuzz_test.cc @@ -1,5 +1,7 @@ #include +#include "envoy/config/bootstrap/v2/bootstrap.pb.validate.h" + #include "common/network/address_impl.h" #include "common/thread_local/thread_local_impl.h" From 44634d812f3c6bf7c9b9e3a827e650ca59b6e25c Mon Sep 17 00:00:00 2001 From: Luke Shumaker Date: Tue, 27 Aug 2019 17:17:31 -0400 Subject: [PATCH 13/16] Improve tools/stack_decode.py (#8041) Adjust tools/stack_decode.py to more obviously be Python 2 (not 3), and to work on stack traces that don't include the symbol names. Risk Level: Low Testing: Manually tested on a stack trace that one of our users sent us Signed-off-by: Luke Shumaker --- tools/stack_decode.py | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/tools/stack_decode.py b/tools/stack_decode.py index 3e3c7bd87e7a..cc22bfd82a39 100755 --- a/tools/stack_decode.py +++ b/tools/stack_decode.py @@ -1,4 +1,4 @@ -#!/usr/bin/env python +#!/usr/bin/env python3 # Call addr2line as needed to resolve addresses in a stack trace. The addresses # will be replaced if they can be resolved into file and line numbers. The @@ -11,7 +11,6 @@ # In each case this script will add file and line information to any backtrace log # lines found and echo back all non-Backtrace lines untouched. -from __future__ import print_function import collections import re import subprocess @@ -24,10 +23,14 @@ # any nonmatching lines unmodified. End when EOF received. def decode_stacktrace_log(object_file, input_source): traces = {} - # Match something like [backtrace] - # bazel-out/local-dbg/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:84] + # Match something like: + # [backtrace] [bazel-out/local-dbg/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:84] backtrace_marker = "\[backtrace\] [^\s]+" - stackaddr_re = re.compile("%s #\d+: .* \[(0x[0-9a-fA-F]+)\]$" % backtrace_marker) + # Match something like: + # ${backtrace_marker} #10: SYMBOL [0xADDR] + # or: + # ${backtrace_marker} #10: [0xADDR] + stackaddr_re = re.compile("%s #\d+:(?: .*)? \[(0x[0-9a-fA-F]+)\]$" % backtrace_marker) try: while True: @@ -53,7 +56,7 @@ def decode_stacktrace_log(object_file, input_source): # # Returns list of result lines def run_addr2line(obj_file, addr_to_resolve): - return subprocess.check_output(["addr2line", "-Cpie", obj_file, addr_to_resolve]) + return subprocess.check_output(["addr2line", "-Cpie", obj_file, addr_to_resolve]).decode('utf-8') # Because of how bazel compiles, addr2line reports file names that begin with @@ -69,7 +72,10 @@ def trim_proc_cwd(file_and_line_number): decode_stacktrace_log(sys.argv[2], sys.stdin) sys.exit(0) elif len(sys.argv) > 1: - rununder = subprocess.Popen(sys.argv[1:], stdout=subprocess.PIPE, stderr=subprocess.STDOUT) + rununder = subprocess.Popen(sys.argv[1:], + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + text=True) decode_stacktrace_log(sys.argv[1], rununder.stdout) rununder.wait() sys.exit(rununder.returncode) # Pass back test pass/fail result From 0006efc5a04dbd0a2381f25b09c77ba536330588 Mon Sep 17 00:00:00 2001 From: Lizan Zhou Date: Tue, 27 Aug 2019 16:35:55 -0700 Subject: [PATCH 14/16] build: tell googletest to use absl stacktrace (#8047) Description: https://github.com/google/googletest/blob/d7003576dd133856432e2e07340f45926242cc3a/BUILD.bazel#L42 Risk Level: Low (test only) Testing: CI Docs Changes: Release Notes: Signed-off-by: Lizan Zhou --- .bazelrc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.bazelrc b/.bazelrc index 01d32bdd3fd3..6b73d8942498 100644 --- a/.bazelrc +++ b/.bazelrc @@ -20,6 +20,9 @@ build --action_env=BAZEL_LINKOPTS=-lm:-static-libgcc build --host_javabase=@bazel_tools//tools/jdk:remote_jdk11 build --javabase=@bazel_tools//tools/jdk:remote_jdk11 +# We already have absl in the build, define absl=1 to tell googletest to use absl for backtrace. +build --define absl=1 + # Pass PATH, CC and CXX variables from the environment. build --action_env=CC build --action_env=CXX @@ -147,4 +150,4 @@ build:asan-fuzzer --define=FUZZING_ENGINE=libfuzzer build:asan-fuzzer --copt=-DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION build:asan-fuzzer --copt=-fsanitize-coverage=trace-pc-guard # Remove UBSAN halt_on_error to avoid crashing on protobuf errors. -build:asan-fuzzer --test_env=UBSAN_OPTIONS=print_stacktrace=1 \ No newline at end of file +build:asan-fuzzer --test_env=UBSAN_OPTIONS=print_stacktrace=1 From 6dd0ee161ab97e494125b1c937f7135ed0d0594f Mon Sep 17 00:00:00 2001 From: scheler Date: Tue, 27 Aug 2019 16:40:04 -0700 Subject: [PATCH 15/16] Update references to local scripts to enable using build container for filter repos (#7907) Description: This change enables using run_envoy_docker.sh to build envoy-filter-example Risk Level: Low Testing: Manually tested building envoy-filter-example using: envoy/ci/run_envoy_docker.sh './ci/do_ci.sh build' Docs Changes: N/A Release Notes: N/A Signed-off-by: Santosh Kumar Cheler --- ci/envoy_build_sha.sh | 2 +- ci/run_envoy_docker.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ci/envoy_build_sha.sh b/ci/envoy_build_sha.sh index 709617afe9b6..bdc6fefe409d 100644 --- a/ci/envoy_build_sha.sh +++ b/ci/envoy_build_sha.sh @@ -1,2 +1,2 @@ -ENVOY_BUILD_SHA=$(grep envoyproxy/envoy-build .circleci/config.yml | sed -e 's#.*envoyproxy/envoy-build:\(.*\)#\1#' | uniq) +ENVOY_BUILD_SHA=$(grep envoyproxy/envoy-build $(dirname $0)/../.circleci/config.yml | sed -e 's#.*envoyproxy/envoy-build:\(.*\)#\1#' | uniq) [[ $(wc -l <<< "${ENVOY_BUILD_SHA}" | awk '{$1=$1};1') == 1 ]] || (echo ".circleci/config.yml hashes are inconsistent!" && exit 1) diff --git a/ci/run_envoy_docker.sh b/ci/run_envoy_docker.sh index a93802b26173..ede21c5de859 100755 --- a/ci/run_envoy_docker.sh +++ b/ci/run_envoy_docker.sh @@ -2,7 +2,7 @@ set -e -. ci/envoy_build_sha.sh +. $(dirname $0)/envoy_build_sha.sh # We run as root and later drop permissions. This is required to setup the USER # in useradd below, which is need for correct Python execution in the Docker From d99e7f6a8ddebe6cfbe0afd0cf9056137d9ee6ae Mon Sep 17 00:00:00 2001 From: Dmitry Rozhkov Date: Wed, 28 Aug 2019 02:41:20 +0300 Subject: [PATCH 16/16] bazel: patch gRPC to fix Envoy builds with glibc v2.30 (#7971) Description: the latest glibc (v2.30) declares its own `gettid()` function (see [0]) and this creates a naming conflict in gRPC which has a function with the same name. Apply to gRPC [a patch](https://github.com/grpc/grpc/pull/18950) which renames `gettid()` to `sys_gettid()`. [0] https://sourceware.org/git/?p=glibc.git;a=commit;h=1d0fc213824eaa2a8f8c4385daaa698ee8fb7c92 Risk Level: low Testing: unit tests Docs Changes: n/a Release Notes: n/a Signed-off-by: Dmitry Rozhkov --- bazel/grpc-rename-gettid.patch | 78 ++++++++++++++++++++++++++++++++++ bazel/repositories.bzl | 2 + 2 files changed, 80 insertions(+) create mode 100644 bazel/grpc-rename-gettid.patch diff --git a/bazel/grpc-rename-gettid.patch b/bazel/grpc-rename-gettid.patch new file mode 100644 index 000000000000..90bd9115893f --- /dev/null +++ b/bazel/grpc-rename-gettid.patch @@ -0,0 +1,78 @@ +From d1d017390b799c59d6fdf7b8afa6136d218bdd61 Mon Sep 17 00:00:00 2001 +From: Benjamin Peterson +Date: Fri, 3 May 2019 08:11:00 -0700 +Subject: [PATCH] Rename gettid() functions. + +glibc 2.30 will declare its own gettid; see https://sourceware.org/git/?p=glibc.git;a=commit;h=1d0fc213824eaa2a8f8c4385daaa698ee8fb7c92. Rename the grpc versions to avoid naming conflicts. +--- + src/core/lib/gpr/log_linux.cc | 4 ++-- + src/core/lib/gpr/log_posix.cc | 4 ++-- + src/core/lib/iomgr/ev_epollex_linux.cc | 4 ++-- + 3 files changed, 6 insertions(+), 6 deletions(-) + +diff --git a/src/core/lib/gpr/log_linux.cc b/src/core/lib/gpr/log_linux.cc +index 561276f0c20..8b597b4cf2f 100644 +--- a/src/core/lib/gpr/log_linux.cc ++++ b/src/core/lib/gpr/log_linux.cc +@@ -40,7 +40,7 @@ + #include + #include + +-static long gettid(void) { return syscall(__NR_gettid); } ++static long sys_gettid(void) { return syscall(__NR_gettid); } + + void gpr_log(const char* file, int line, gpr_log_severity severity, + const char* format, ...) { +@@ -70,7 +70,7 @@ void gpr_default_log(gpr_log_func_args* args) { + gpr_timespec now = gpr_now(GPR_CLOCK_REALTIME); + struct tm tm; + static __thread long tid = 0; +- if (tid == 0) tid = gettid(); ++ if (tid == 0) tid = sys_gettid(); + + timer = static_cast(now.tv_sec); + final_slash = strrchr(args->file, '/'); +diff --git a/src/core/lib/gpr/log_posix.cc b/src/core/lib/gpr/log_posix.cc +index b6edc14ab6b..2f7c6ce3760 100644 +--- a/src/core/lib/gpr/log_posix.cc ++++ b/src/core/lib/gpr/log_posix.cc +@@ -31,7 +31,7 @@ + #include + #include + +-static intptr_t gettid(void) { return (intptr_t)pthread_self(); } ++static intptr_t sys_gettid(void) { return (intptr_t)pthread_self(); } + + void gpr_log(const char* file, int line, gpr_log_severity severity, + const char* format, ...) { +@@ -86,7 +86,7 @@ void gpr_default_log(gpr_log_func_args* args) { + char* prefix; + gpr_asprintf(&prefix, "%s%s.%09d %7" PRIdPTR " %s:%d]", + gpr_log_severity_string(args->severity), time_buffer, +- (int)(now.tv_nsec), gettid(), display_file, args->line); ++ (int)(now.tv_nsec), sys_gettid(), display_file, args->line); + + fprintf(stderr, "%-70s %s\n", prefix, args->message); + gpr_free(prefix); +diff --git a/src/core/lib/iomgr/ev_epollex_linux.cc b/src/core/lib/iomgr/ev_epollex_linux.cc +index 08116b3ab53..76f59844312 100644 +--- a/src/core/lib/iomgr/ev_epollex_linux.cc ++++ b/src/core/lib/iomgr/ev_epollex_linux.cc +@@ -1102,7 +1102,7 @@ static void end_worker(grpc_pollset* pollset, grpc_pollset_worker* worker, + } + + #ifndef NDEBUG +-static long gettid(void) { return syscall(__NR_gettid); } ++static long sys_gettid(void) { return syscall(__NR_gettid); } + #endif + + /* pollset->mu lock must be held by the caller before calling this. +@@ -1122,7 +1122,7 @@ static grpc_error* pollset_work(grpc_pollset* pollset, + #define WORKER_PTR (&worker) + #endif + #ifndef NDEBUG +- WORKER_PTR->originator = gettid(); ++ WORKER_PTR->originator = sys_gettid(); + #endif + if (GRPC_TRACE_FLAG_ENABLED(grpc_polling_trace)) { + gpr_log(GPR_INFO, diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index 15dbd7cb0e8c..b80d6f0bfe22 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -617,6 +617,8 @@ def _com_github_grpc_grpc(): "@envoy//bazel:grpc-protoinfo-2.patch", # Pre-integration of https://github.com/grpc/grpc/pull/19860 "@envoy//bazel:grpc-protoinfo-3.patch", + # Pre-integration of https://github.com/grpc/grpc/pull/18950 + "@envoy//bazel:grpc-rename-gettid.patch", ], patch_args = ["-p1"], )