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 <[email protected]>
envoyproxy · Sep 2, 2021 · db74e31 · db74e31
1 parent 4aab7fa
commit db74e31
Show file tree

Hide file tree

Showing 11 changed files with 94 additions and 117 deletions.
diff --git 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
diff --git 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<JsonV1Serializer>();
+    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<JsonV2Serializer>(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<Span>& 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
@@ -92,21 +92,6 @@ class SpanBuffer {
 
 using SpanBufferPtr = std::unique_ptr<SpanBuffer>;
 
-/**
- * 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<Span>& 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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());