From db74e313b3651588e59c671af45077714ac32cef Mon Sep 17 00:00:00 2001 From: tyxia <72890320+tyxia@users.noreply.github.com> Date: Thu, 2 Sep 2021 00:14:52 -0400 Subject: [PATCH] Remove the support for `hidden_envoy_deprecated_HTTP_JSON_V1` (#17806) - Removed the JsonV1Serializer and added exception for HTTP_JSON_V1version in Serializer creation. - Specified collector_endpoint_version in the test - Removed the default value for endpoint_, it is redundant here because in the zipkin.proto, it has PGV rule [(validate.rules).string = {min_len: 1}], so we always need to provide the config for this field. - Modified the ConstructBuffer test to create the buffer using ZipkinConfig::HTTP_JSON Risk Level: LOW Testing: CI Signed-off-by: Tianyu Xia --- api/envoy/config/trace/v3/zipkin.proto | 6 +- .../envoy/config/trace/v3/zipkin.proto | 6 +- .../extensions/tracers/zipkin/span_buffer.cc | 12 +- .../extensions/tracers/zipkin/span_buffer.h | 15 -- .../tracers/zipkin/zipkin_core_constants.h | 1 - .../tracers/zipkin/zipkin_tracer_impl.cc | 2 +- .../tracers/zipkin/zipkin_tracer_impl.h | 11 +- .../http_connection_manager/config_test.cc | 4 +- test/extensions/tracers/zipkin/config_test.cc | 2 +- .../tracers/zipkin/span_buffer_test.cc | 137 +++++++++--------- .../tracers/zipkin/zipkin_tracer_impl_test.cc | 15 +- 11 files changed, 94 insertions(+), 117 deletions(-) diff --git a/api/envoy/config/trace/v3/zipkin.proto b/api/envoy/config/trace/v3/zipkin.proto index 2c1026b8304a..0638d89315fa 100644 --- a/api/envoy/config/trace/v3/zipkin.proto +++ b/api/envoy/config/trace/v3/zipkin.proto @@ -50,8 +50,7 @@ message ZipkinConfig { string collector_cluster = 1 [(validate.rules).string = {min_len: 1}]; // The API endpoint of the Zipkin service where the spans will be sent. When - // using a standard Zipkin installation, the API endpoint is typically - // /api/v1/spans, which is the default value. + // using a standard Zipkin installation. string collector_endpoint = 2 [(validate.rules).string = {min_len: 1}]; // Determines whether a 128bit trace id will be used when creating a new @@ -62,8 +61,7 @@ message ZipkinConfig { // The default value is true. google.protobuf.BoolValue shared_span_context = 4; - // Determines the selected collector endpoint version. By default, the ``HTTP_JSON_V1`` will be - // used. + // Determines the selected collector endpoint version. CollectorEndpointVersion collector_endpoint_version = 5; // Optional hostname to use when sending spans to the collector_cluster. Useful for collectors diff --git a/generated_api_shadow/envoy/config/trace/v3/zipkin.proto b/generated_api_shadow/envoy/config/trace/v3/zipkin.proto index 61d3f0805fd3..42e46ed69c64 100644 --- a/generated_api_shadow/envoy/config/trace/v3/zipkin.proto +++ b/generated_api_shadow/envoy/config/trace/v3/zipkin.proto @@ -53,8 +53,7 @@ message ZipkinConfig { string collector_cluster = 1 [(validate.rules).string = {min_len: 1}]; // The API endpoint of the Zipkin service where the spans will be sent. When - // using a standard Zipkin installation, the API endpoint is typically - // /api/v1/spans, which is the default value. + // using a standard Zipkin installation. string collector_endpoint = 2 [(validate.rules).string = {min_len: 1}]; // Determines whether a 128bit trace id will be used when creating a new @@ -65,8 +64,7 @@ message ZipkinConfig { // The default value is true. google.protobuf.BoolValue shared_span_context = 4; - // Determines the selected collector endpoint version. By default, the ``HTTP_JSON_V1`` will be - // used. + // Determines the selected collector endpoint version. CollectorEndpointVersion collector_endpoint_version = 5; // Optional hostname to use when sending spans to the collector_cluster. Useful for collectors diff --git a/source/extensions/tracers/zipkin/span_buffer.cc b/source/extensions/tracers/zipkin/span_buffer.cc index e5ab41876cca..603a9129c0de 100644 --- a/source/extensions/tracers/zipkin/span_buffer.cc +++ b/source/extensions/tracers/zipkin/span_buffer.cc @@ -49,7 +49,9 @@ SerializerPtr SpanBuffer::makeSerializer( const bool shared_span_context) { switch (version) { case envoy::config::trace::v3::ZipkinConfig::hidden_envoy_deprecated_HTTP_JSON_V1: - return std::make_unique(); + throw EnvoyException( + "hidden_envoy_deprecated_HTTP_JSON_V1 has been deprecated. Please use a non-default " + "envoy::config::trace::v3::ZipkinConfig::CollectorEndpointVersion value."); case envoy::config::trace::v3::ZipkinConfig::HTTP_JSON: return std::make_unique(shared_span_context); case envoy::config::trace::v3::ZipkinConfig::HTTP_PROTO: @@ -59,14 +61,6 @@ SerializerPtr SpanBuffer::makeSerializer( } } -std::string JsonV1Serializer::serialize(const std::vector& zipkin_spans) { - const std::string serialized_elements = - absl::StrJoin(zipkin_spans, ",", [](std::string* element, const Span& zipkin_span) { - absl::StrAppend(element, zipkin_span.toJson()); - }); - return absl::StrCat("[", serialized_elements, "]"); -} - JsonV2Serializer::JsonV2Serializer(const bool shared_span_context) : shared_span_context_{shared_span_context} {} diff --git a/source/extensions/tracers/zipkin/span_buffer.h b/source/extensions/tracers/zipkin/span_buffer.h index 16927506ce5f..df1536044367 100644 --- a/source/extensions/tracers/zipkin/span_buffer.h +++ b/source/extensions/tracers/zipkin/span_buffer.h @@ -92,21 +92,6 @@ class SpanBuffer { using SpanBufferPtr = std::unique_ptr; -/** - * JsonV1Serializer implements Zipkin::Serializer that serializes list of Zipkin spans into JSON - * Zipkin v1 array. - */ -class JsonV1Serializer : public Serializer { -public: - JsonV1Serializer() = default; - - /** - * Serialize list of Zipkin spans into Zipkin v1 JSON array. - * @return std::string serialized pending spans as Zipkin v1 JSON array. - */ - std::string serialize(const std::vector& pending_spans) override; -}; - /** * JsonV2Serializer implements Zipkin::Serializer that serializes list of Zipkin spans into JSON * Zipkin v2 array. diff --git a/source/extensions/tracers/zipkin/zipkin_core_constants.h b/source/extensions/tracers/zipkin/zipkin_core_constants.h index 03e1ba63a40b..ee82dc36055a 100644 --- a/source/extensions/tracers/zipkin/zipkin_core_constants.h +++ b/source/extensions/tracers/zipkin/zipkin_core_constants.h @@ -37,7 +37,6 @@ constexpr char SERVER_ADDR[] = "sa"; constexpr char SAMPLED[] = "1"; constexpr char NOT_SAMPLED[] = "0"; -constexpr char DEFAULT_COLLECTOR_ENDPOINT[] = "/api/v1/spans"; constexpr bool DEFAULT_SHARED_SPAN_CONTEXT = true; } // namespace diff --git a/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc b/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc index aa59c59b9164..33e0327b26fc 100644 --- a/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc +++ b/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc @@ -87,7 +87,7 @@ Driver::Driver(const envoy::config::trace::v3::ZipkinConfig& zipkin_config, if (!zipkin_config.collector_endpoint().empty()) { collector.endpoint_ = zipkin_config.collector_endpoint(); } - // The current default version of collector_endpoint_version is HTTP_JSON_V1. + // The current default version of collector_endpoint_version is HTTP_JSON. collector.version_ = zipkin_config.collector_endpoint_version(); const bool trace_id_128bit = zipkin_config.trace_id_128bit(); diff --git a/source/extensions/tracers/zipkin/zipkin_tracer_impl.h b/source/extensions/tracers/zipkin/zipkin_tracer_impl.h index 59a5540361b3..a416c3268a4b 100644 --- a/source/extensions/tracers/zipkin/zipkin_tracer_impl.h +++ b/source/extensions/tracers/zipkin/zipkin_tracer_impl.h @@ -158,15 +158,12 @@ class Driver : public Tracing::Driver { * Information about the Zipkin collector. */ struct CollectorInfo { - // The Zipkin collector endpoint/path to receive the collected trace data. e.g. /api/v1/spans if - // HTTP_JSON_V1 or /api/v2/spans otherwise. - std::string endpoint_{DEFAULT_COLLECTOR_ENDPOINT}; + // The Zipkin collector endpoint/path to receive the collected trace data. + std::string endpoint_; // The version of the collector. This is related to endpoint's supported payload specification and - // transport. Currently it defaults to envoy::config::trace::v3::ZipkinConfig::HTTP_JSON_V1. In - // the future, we will throw when collector_endpoint_version is not specified. - envoy::config::trace::v3::ZipkinConfig::CollectorEndpointVersion version_{ - envoy::config::trace::v3::ZipkinConfig::hidden_envoy_deprecated_HTTP_JSON_V1}; + // transport. + envoy::config::trace::v3::ZipkinConfig::CollectorEndpointVersion version_; bool shared_span_context_{DEFAULT_SHARED_SPAN_CONTEXT}; }; diff --git a/test/extensions/filters/network/http_connection_manager/config_test.cc b/test/extensions/filters/network/http_connection_manager/config_test.cc index eb19cc514de3..1681eea7db60 100644 --- a/test/extensions/filters/network/http_connection_manager/config_test.cc +++ b/test/extensions/filters/network/http_connection_manager/config_test.cc @@ -423,7 +423,7 @@ stat_prefix: router typed_config: "@type": type.googleapis.com/envoy.config.trace.v3.ZipkinConfig collector_cluster: zipkin - collector_endpoint: "/api/v1/spans" + collector_endpoint: "/api/v2/spans" collector_endpoint_version: HTTP_JSON http_filters: - name: envoy.filters.http.router @@ -441,7 +441,7 @@ stat_prefix: router inlined_tracing_config.set_name("zipkin"); envoy::config::trace::v3::ZipkinConfig zipkin_config; zipkin_config.set_collector_cluster("zipkin"); - zipkin_config.set_collector_endpoint("/api/v1/spans"); + zipkin_config.set_collector_endpoint("/api/v2/spans"); zipkin_config.set_collector_endpoint_version(envoy::config::trace::v3::ZipkinConfig::HTTP_JSON); inlined_tracing_config.mutable_typed_config()->PackFrom(zipkin_config); diff --git a/test/extensions/tracers/zipkin/config_test.cc b/test/extensions/tracers/zipkin/config_test.cc index c3f9a71e46e6..6c95bf964b4e 100644 --- a/test/extensions/tracers/zipkin/config_test.cc +++ b/test/extensions/tracers/zipkin/config_test.cc @@ -29,7 +29,7 @@ TEST(ZipkinTracerConfigTest, ZipkinHttpTracer) { typed_config: "@type": type.googleapis.com/envoy.config.trace.v3.ZipkinConfig collector_cluster: fake_cluster - collector_endpoint: /api/v1/spans + collector_endpoint: /api/v2/spans collector_endpoint_version: HTTP_JSON )EOF"; diff --git a/test/extensions/tracers/zipkin/span_buffer_test.cc b/test/extensions/tracers/zipkin/span_buffer_test.cc index 9ccb9c7b3fc0..92c8a7960b86 100644 --- a/test/extensions/tracers/zipkin/span_buffer_test.cc +++ b/test/extensions/tracers/zipkin/span_buffer_test.cc @@ -157,68 +157,72 @@ TEST(ZipkinSpanBufferTest, TestSerializeTimestamp) { } TEST(ZipkinSpanBufferTest, ConstructBuffer) { - const std::string expected1 = - withDefaultTimestampAndDuration(R"([{"traceId":"0000000000000001",)" - R"("name":"",)" - R"("id":"0000000000000001",)" - R"("duration":DEFAULT_TEST_DURATION,)" - R"("annotations":[{"timestamp":ANNOTATION_TEST_TIMESTAMP,)" - R"("value":"cs",)" - R"("endpoint":{"ipv4":"1.2.3.4",)" - R"("port":8080,)" - R"("serviceName":"service1"}},)" - R"({"timestamp":ANNOTATION_TEST_TIMESTAMP,)" - R"("value":"sr",)" - R"("endpoint":{"ipv4":"1.2.3.4",)" - R"("port":8080,)" - R"("serviceName":"service1"}}],)" - R"("binaryAnnotations":[{"key":"response_size",)" - R"("value":"DEFAULT_TEST_DURATION"}]}])"); - - const std::string expected2 = - withDefaultTimestampAndDuration(R"([{"traceId":"0000000000000001",)" - R"("name":"",)" - R"("id":"0000000000000001",)" - R"("duration":DEFAULT_TEST_DURATION,)" - R"("annotations":[{"timestamp":ANNOTATION_TEST_TIMESTAMP,)" - R"("value":"cs",)" - R"("endpoint":{"ipv4":"1.2.3.4",)" - R"("port":8080,)" - R"("serviceName":"service1"}},)" - R"({"timestamp":ANNOTATION_TEST_TIMESTAMP,)" - R"("value":"sr",)" - R"("endpoint":{"ipv4":"1.2.3.4",)" - R"("port":8080,)" - R"("serviceName":"service1"}}],)" - R"("binaryAnnotations":[{"key":"response_size",)" - R"("value":"DEFAULT_TEST_DURATION"}]},)" - R"({"traceId":"0000000000000001",)" - R"("name":"",)" - R"("id":"0000000000000001",)" - R"("duration":DEFAULT_TEST_DURATION,)" - R"("annotations":[{"timestamp":ANNOTATION_TEST_TIMESTAMP,)" - R"("value":"cs",)" - R"("endpoint":{"ipv4":"1.2.3.4",)" - R"("port":8080,)" - R"("serviceName":"service1"}},)" - R"({"timestamp":ANNOTATION_TEST_TIMESTAMP,)" - R"("value":"sr",)" - R"("endpoint":{"ipv4":"1.2.3.4",)" - R"("port":8080,)" - R"("serviceName":"service1"}}],)" - R"("binaryAnnotations":[{"key":"response_size",)" - R"("value":"DEFAULT_TEST_DURATION"}]}])"); + const std::string expected = "[{" + R"("traceId":"0000000000000001",)" + R"("id":"0000000000000001",)" + R"("kind":"CLIENT",)" + R"("timestamp":ANNOTATION_TEST_TIMESTAMP,)" + R"("duration":DEFAULT_TEST_DURATION,)" + R"("localEndpoint":{)" + R"("serviceName":"service1",)" + R"("ipv4":"1.2.3.4",)" + R"("port":8080},)" + R"("tags":{)" + R"("response_size":"DEFAULT_TEST_DURATION"}},)" + R"({)" + R"("traceId":"0000000000000001",)" + R"("id":"0000000000000001",)" + R"("kind":"SERVER",)" + R"("timestamp":ANNOTATION_TEST_TIMESTAMP,)" + R"("duration":DEFAULT_TEST_DURATION,)" + R"("localEndpoint":{)" + R"("serviceName":"service1",)" + R"("ipv4":"1.2.3.4",)" + R"("port":8080},)" + R"("tags":{)" + R"("response_size":"DEFAULT_TEST_DURATION"},)" + R"("shared":true)" + "}]"; const bool shared = true; const bool delay_allocation = true; - SpanBuffer buffer1(envoy::config::trace::v3::ZipkinConfig::hidden_envoy_deprecated_HTTP_JSON_V1, - shared); - expectSerializedBuffer(buffer1, delay_allocation, {expected1, expected2}); + SpanBuffer buffer1(envoy::config::trace::v3::ZipkinConfig::HTTP_JSON, shared); + expectSerializedBuffer(buffer1, delay_allocation, {expected}); - // Prepare 3 slots, since we will add one more inside the `expectSerializedBuffer` function. - SpanBuffer buffer2(envoy::config::trace::v3::ZipkinConfig::hidden_envoy_deprecated_HTTP_JSON_V1, - shared, 3); - expectSerializedBuffer(buffer2, !delay_allocation, {expected1, expected2}); + // Prepare 2 slots, since we will add one more inside the `expectSerializedBuffer` function. + // SpanBuffer + SpanBuffer buffer2(envoy::config::trace::v3::ZipkinConfig::HTTP_JSON, shared, 2); + expectSerializedBuffer(buffer2, !delay_allocation, {expected}); + + const std::string expected2 = "[{" + R"("traceId":"0000000000000001",)" + R"("id":"0000000000000001",)" + R"("kind":"CLIENT",)" + R"("timestamp":ANNOTATION_TEST_TIMESTAMP,)" + R"("duration":DEFAULT_TEST_DURATION,)" + R"("localEndpoint":{)" + R"("serviceName":"service1",)" + R"("ipv4":"1.2.3.4",)" + R"("port":8080},)" + R"("tags":{)" + R"("response_size":"DEFAULT_TEST_DURATION"}},)" + R"({)" + R"("traceId":"0000000000000001",)" + R"("id":"0000000000000001",)" + R"("kind":"SERVER",)" + R"("timestamp":ANNOTATION_TEST_TIMESTAMP,)" + R"("duration":DEFAULT_TEST_DURATION,)" + R"("localEndpoint":{)" + R"("serviceName":"service1",)" + R"("ipv4":"1.2.3.4",)" + R"("port":8080},)" + R"("tags":{)" + R"("response_size":"DEFAULT_TEST_DURATION"},)" + "}]"; + + // Test the buffer construct when `shared_span_context` is set to false + SpanBuffer buffer3(envoy::config::trace::v3::ZipkinConfig::HTTP_JSON, !shared); + expectSerializedBuffer(buffer3, delay_allocation, {expected2}); } TEST(ZipkinSpanBufferTest, SerializeSpan) { @@ -455,16 +459,15 @@ TEST(ZipkinSpanBufferTest, TestSerializeTimestampInTheFuture) { Not(HasSubstr(R"("duration":2.584324295476870e+15)"))); EXPECT_THAT(bufferDeprecatedJsonV1.serialize(), Not(HasSubstr(R"("duration":"2584324295476870")"))); +} - SpanBuffer bufferJsonV2( - envoy::config::trace::v3::ZipkinConfig::hidden_envoy_deprecated_HTTP_JSON_V1, true, 2); - bufferJsonV2.addSpan(createSpan({"cs"}, IpType::V4)); - EXPECT_THAT(bufferJsonV2.serialize(), HasSubstr(R"("timestamp":1584324295476871)")); - EXPECT_THAT(bufferJsonV2.serialize(), Not(HasSubstr(R"("timestamp":1.58432429547687e+15)"))); - EXPECT_THAT(bufferJsonV2.serialize(), Not(HasSubstr(R"("timestamp":"1584324295476871")"))); - EXPECT_THAT(bufferJsonV2.serialize(), HasSubstr(R"("duration":2584324295476870)")); - EXPECT_THAT(bufferJsonV2.serialize(), Not(HasSubstr(R"("duration":2.584324295476870e+15)"))); - EXPECT_THAT(bufferJsonV2.serialize(), Not(HasSubstr(R"("duration":"2584324295476870")"))); +TEST(ZipkinSpanBufferTest, TestDeprecationOfHttpJsonV1) { + EXPECT_THROW_WITH_MESSAGE( + SpanBuffer buffer1( + envoy::config::trace::v3::ZipkinConfig::hidden_envoy_deprecated_HTTP_JSON_V1, false), + Envoy::EnvoyException, + "hidden_envoy_deprecated_HTTP_JSON_V1 has been deprecated. Please use a non-default " + "envoy::config::trace::v3::ZipkinConfig::CollectorEndpointVersion value."); } } // namespace diff --git a/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc b/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc index 059fb6063a0b..93cb4bafbfc2 100644 --- a/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc +++ b/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc @@ -65,7 +65,7 @@ class ZipkinDriverTest : public testing::Test { std::string yaml_string = fmt::format(R"EOF( collector_cluster: fake_cluster - collector_endpoint: /api/v1/spans + collector_endpoint: /api/v2/spans collector_endpoint_version: {} )EOF", version); @@ -99,7 +99,7 @@ class ZipkinDriverTest : public testing::Test { callback = &callbacks; const std::string& expected_hostname = !hostname.empty() ? hostname : "fake_cluster"; - EXPECT_EQ("/api/v1/spans", message->headers().getPathValue()); + EXPECT_EQ("/api/v2/spans", message->headers().getPathValue()); EXPECT_EQ(expected_hostname, message->headers().getHostValue()); EXPECT_EQ(content_type, message->headers().getContentTypeValue()); @@ -180,7 +180,8 @@ TEST_F(ZipkinDriverTest, InitializeDriver) { // Valid config but collector cluster doesn't exists. const std::string yaml_string = R"EOF( collector_cluster: fake_cluster - collector_endpoint: /api/v1/spans + collector_endpoint: /api/v2/spans + collector_endpoint_version: HTTP_JSON )EOF"; envoy::config::trace::v3::ZipkinConfig zipkin_config; TestUtility::loadFromYaml(yaml_string, zipkin_config); @@ -193,7 +194,8 @@ TEST_F(ZipkinDriverTest, InitializeDriver) { cm_.initializeClusters({"fake_cluster"}, {}); const std::string yaml_string = R"EOF( collector_cluster: fake_cluster - collector_endpoint: /api/v1/spans + collector_endpoint: /api/v2/spans + collector_endpoint_version: HTTP_JSON )EOF"; envoy::config::trace::v3::ZipkinConfig zipkin_config; TestUtility::loadFromYaml(yaml_string, zipkin_config); @@ -208,7 +210,8 @@ TEST_F(ZipkinDriverTest, AllowCollectorClusterToBeAddedViaApi) { const std::string yaml_string = R"EOF( collector_cluster: fake_cluster - collector_endpoint: /api/v1/spans + collector_endpoint: /api/v2/spans + collector_endpoint_version: HTTP_JSON )EOF"; envoy::config::trace::v3::ZipkinConfig zipkin_config; TestUtility::loadFromYaml(yaml_string, zipkin_config); @@ -242,7 +245,7 @@ TEST_F(ZipkinDriverTest, FlushOneSpanReportFailure) { const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { callback = &callbacks; - EXPECT_EQ("/api/v1/spans", message->headers().getPathValue()); + EXPECT_EQ("/api/v2/spans", message->headers().getPathValue()); EXPECT_EQ("fake_cluster", message->headers().getHostValue()); EXPECT_EQ("application/json", message->headers().getContentTypeValue());