From 0cce1771fbecb422bf2fa5f7b74ace22b06bdb7f Mon Sep 17 00:00:00 2001 From: John Esmet Date: Wed, 28 Oct 2020 19:25:34 +0000 Subject: [PATCH] Add hostname to zipkin tracer config to support cases where the collector requires a certain hostname --- api/envoy/config/trace/v3/zipkin.proto | 6 +++- .../tracers/zipkin/v4alpha/zipkin.proto | 6 +++- .../envoy/config/trace/v3/zipkin.proto | 6 +++- .../tracers/zipkin/v4alpha/zipkin.proto | 6 +++- .../tracers/zipkin/zipkin_tracer_impl.cc | 4 ++- .../tracers/zipkin/zipkin_tracer_impl.h | 2 ++ .../tracers/zipkin/zipkin_tracer_impl_test.cc | 32 ++++++++++++++++--- 7 files changed, 52 insertions(+), 10 deletions(-) diff --git a/api/envoy/config/trace/v3/zipkin.proto b/api/envoy/config/trace/v3/zipkin.proto index 5c5349cdf1554..dbe9a2f89db7d 100644 --- a/api/envoy/config/trace/v3/zipkin.proto +++ b/api/envoy/config/trace/v3/zipkin.proto @@ -20,7 +20,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; // Configuration for the Zipkin tracer. // [#extension: envoy.tracers.zipkin] -// [#next-free-field: 6] +// [#next-free-field: 7] message ZipkinConfig { option (udpa.annotations.versioning).previous_message_type = "envoy.config.trace.v2.ZipkinConfig"; @@ -67,4 +67,8 @@ message ZipkinConfig { // Determines the selected collector endpoint version. By default, the ``HTTP_JSON_V1`` will be // used. CollectorEndpointVersion collector_endpoint_version = 5; + + // Optional hostname to use when sending spans to the collector_cluster. Useful for collectors + // that require a specific hostname. Defaults to `collector_cluster` above. + string collector_hostname = 6; } diff --git a/api/envoy/extensions/tracers/zipkin/v4alpha/zipkin.proto b/api/envoy/extensions/tracers/zipkin/v4alpha/zipkin.proto index 3abbcad2de15a..476f1fe708841 100644 --- a/api/envoy/extensions/tracers/zipkin/v4alpha/zipkin.proto +++ b/api/envoy/extensions/tracers/zipkin/v4alpha/zipkin.proto @@ -18,7 +18,7 @@ option (udpa.annotations.file_status).package_version_status = NEXT_MAJOR_VERSIO // Configuration for the Zipkin tracer. // [#extension: envoy.tracers.zipkin] -// [#next-free-field: 6] +// [#next-free-field: 7] message ZipkinConfig { option (udpa.annotations.versioning).previous_message_type = "envoy.config.trace.v3.ZipkinConfig"; @@ -65,4 +65,8 @@ message ZipkinConfig { // Determines the selected collector endpoint version. By default, the ``HTTP_JSON_V1`` will be // used. CollectorEndpointVersion collector_endpoint_version = 5; + + // Optional hostname to use when sending spans to the collector_cluster. Useful for collectors + // that require a specific hostname. Defaults to `collector_cluster` above. + string collector_hostname = 6; } diff --git a/generated_api_shadow/envoy/config/trace/v3/zipkin.proto b/generated_api_shadow/envoy/config/trace/v3/zipkin.proto index 10cf6d4ec3c48..f66b72e0a55dc 100644 --- a/generated_api_shadow/envoy/config/trace/v3/zipkin.proto +++ b/generated_api_shadow/envoy/config/trace/v3/zipkin.proto @@ -20,7 +20,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; // Configuration for the Zipkin tracer. // [#extension: envoy.tracers.zipkin] -// [#next-free-field: 6] +// [#next-free-field: 7] message ZipkinConfig { option (udpa.annotations.versioning).previous_message_type = "envoy.config.trace.v2.ZipkinConfig"; @@ -67,4 +67,8 @@ message ZipkinConfig { // Determines the selected collector endpoint version. By default, the ``HTTP_JSON_V1`` will be // used. CollectorEndpointVersion collector_endpoint_version = 5; + + // Optional hostname to use when sending spans to the collector_cluster. Useful for collectors + // that require a specific hostname. Defaults to `collector_cluster` above. + string collector_hostname = 6; } diff --git a/generated_api_shadow/envoy/extensions/tracers/zipkin/v4alpha/zipkin.proto b/generated_api_shadow/envoy/extensions/tracers/zipkin/v4alpha/zipkin.proto index 4207546ff7013..0b9374a79a84a 100644 --- a/generated_api_shadow/envoy/extensions/tracers/zipkin/v4alpha/zipkin.proto +++ b/generated_api_shadow/envoy/extensions/tracers/zipkin/v4alpha/zipkin.proto @@ -18,7 +18,7 @@ option (udpa.annotations.file_status).package_version_status = NEXT_MAJOR_VERSIO // Configuration for the Zipkin tracer. // [#extension: envoy.tracers.zipkin] -// [#next-free-field: 6] +// [#next-free-field: 7] message ZipkinConfig { option (udpa.annotations.versioning).previous_message_type = "envoy.config.trace.v3.ZipkinConfig"; @@ -65,4 +65,8 @@ message ZipkinConfig { // Determines the selected collector endpoint version. By default, the ``HTTP_JSON_V1`` will be // used. CollectorEndpointVersion collector_endpoint_version = 5; + + // Optional hostname to use when sending spans to the collector_cluster. Useful for collectors + // that require a specific hostname. Defaults to `collector_cluster` above. + string collector_hostname = 6; } diff --git a/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc b/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc index 25d1e60ee7990..5d8433dd36d4f 100644 --- a/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc +++ b/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc @@ -77,6 +77,8 @@ Driver::Driver(const envoy::config::trace::v3::ZipkinConfig& zipkin_config, Config::Utility::checkCluster(TracerNames::get().Zipkin, zipkin_config.collector_cluster(), cm_, /* allow_added_via_api */ true); cluster_ = zipkin_config.collector_cluster(); + hostname_ = !zipkin_config.collector_hostname().empty() ? + zipkin_config.collector_hostname() : zipkin_config.collector_cluster(); CollectorInfo collector; if (!zipkin_config.collector_endpoint().empty()) { @@ -177,7 +179,7 @@ void ReporterImpl::flushSpans() { Http::RequestMessagePtr message = std::make_unique(); message->headers().setReferenceMethod(Http::Headers::get().MethodValues.Post); message->headers().setPath(collector_.endpoint_); - message->headers().setHost(driver_.cluster()); + message->headers().setHost(driver_.hostname()); message->headers().setReferenceContentType( collector_.version_ == envoy::config::trace::v3::ZipkinConfig::HTTP_PROTO ? Http::Headers::get().ContentTypeValues.Protobuf diff --git a/source/extensions/tracers/zipkin/zipkin_tracer_impl.h b/source/extensions/tracers/zipkin/zipkin_tracer_impl.h index def08e83e50cd..c223d95a22665 100644 --- a/source/extensions/tracers/zipkin/zipkin_tracer_impl.h +++ b/source/extensions/tracers/zipkin/zipkin_tracer_impl.h @@ -119,6 +119,7 @@ class Driver : public Tracing::Driver { // Getters to return the ZipkinDriver's key members. Upstream::ClusterManager& clusterManager() { return cm_; } const std::string& cluster() { return cluster_; } + const std::string& hostname() { return hostname_; } Runtime::Loader& runtime() { return runtime_; } ZipkinTracerStats& tracerStats() { return tracer_stats_; } @@ -135,6 +136,7 @@ class Driver : public Tracing::Driver { Upstream::ClusterManager& cm_; std::string cluster_; + std::string hostname_; ZipkinTracerStats tracer_stats_; ThreadLocal::SlotPtr tls_; Runtime::Loader& runtime_; diff --git a/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc b/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc index 4cbc21c3bc772..0af1365697923 100644 --- a/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc +++ b/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc @@ -61,23 +61,31 @@ class ZipkinDriverTest : public testing::Test { random_, time_source_); } - void setupValidDriver(const std::string& version) { + void setupValidDriverWithHostname(const std::string& version, const std::string& hostname) { EXPECT_CALL(cm_, get(Eq("fake_cluster"))).WillRepeatedly(Return(&cm_.thread_local_cluster_)); - const std::string yaml_string = fmt::format(R"EOF( + std::string yaml_string = fmt::format(R"EOF( collector_cluster: fake_cluster collector_endpoint: /api/v1/spans collector_endpoint_version: {} )EOF", version); + if (!hostname.empty()) { + yaml_string = yaml_string + fmt::format(R"EOF( + collector_hostname: {} + )EOF", + hostname); + } + envoy::config::trace::v3::ZipkinConfig zipkin_config; TestUtility::loadFromYaml(yaml_string, zipkin_config); setup(zipkin_config, true); } - void expectValidFlushSeveralSpans(const std::string& version, const std::string& content_type) { - setupValidDriver(version); + void expectValidFlushSeveralSpansWithHostname(const std::string& version, const std::string& content_type, + const std::string& hostname) { + setupValidDriverWithHostname(version, hostname); Http::MockAsyncClientRequest request(&cm_.async_client_); Http::AsyncClient::Callbacks* callback; @@ -90,8 +98,9 @@ class ZipkinDriverTest : public testing::Test { const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { callback = &callbacks; + const std::string& expected_hostname = !hostname.empty() ? hostname : "fake_cluster"; EXPECT_EQ("/api/v1/spans", message->headers().getPathValue()); - EXPECT_EQ("fake_cluster", message->headers().getHostValue()); + EXPECT_EQ(expected_hostname, message->headers().getHostValue()); EXPECT_EQ(content_type, message->headers().getContentTypeValue()); return &request; @@ -127,6 +136,14 @@ class ZipkinDriverTest : public testing::Test { EXPECT_EQ(1U, stats_.counter("tracing.zipkin.reports_failed").value()); } + void setupValidDriver(const std::string& version) { + setupValidDriverWithHostname(version, ""); + } + + void expectValidFlushSeveralSpans(const std::string& version, const std::string& content_type) { + expectValidFlushSeveralSpansWithHostname(version, content_type, ""); + } + // TODO(#4160): Currently time_system_ is initialized from DangerousDeprecatedTestTime, which uses // real time, not mock-time. When that is switched to use mock-time instead, I think // generateRandom64() may not be as random as we want, and we'll need to inject entropy @@ -217,6 +234,11 @@ TEST_F(ZipkinDriverTest, FlushSeveralSpansHttpJson) { expectValidFlushSeveralSpans("HTTP_JSON", "application/json"); } +TEST_F(ZipkinDriverTest, FlushSeveralSpansHttpJsonWithHostname) { + expectValidFlushSeveralSpansWithHostname("HTTP_JSON", "application/json", + "zipkin.fakedomain.io"); +} + TEST_F(ZipkinDriverTest, FlushSeveralSpansHttpProto) { expectValidFlushSeveralSpans("HTTP_PROTO", "application/x-protobuf"); }