From 6b851646ae8f72764d677f44198a6c760e72019c Mon Sep 17 00:00:00 2001 From: Marco Magdy Date: Tue, 1 Oct 2019 17:00:45 -0700 Subject: [PATCH 01/19] Skeleton of a tracer for AWS X-Ray Signed-off-by: Marco Magdy --- CODEOWNERS | 2 + api/envoy/config/trace/v2/trace.proto | 15 +++ source/extensions/extensions_build_config.bzl | 1 + source/extensions/tracers/well_known_names.h | 2 + source/extensions/tracers/xray/BUILD | 45 ++++++++ source/extensions/tracers/xray/config.cc | 46 ++++++++ source/extensions/tracers/xray/config.h | 31 ++++++ .../tracers/xray/sampling_strategy.h | 57 ++++++++++ source/extensions/tracers/xray/tracer.h | 34 ++++++ source/extensions/tracers/xray/util.cc | 49 +++++++++ source/extensions/tracers/xray/util.h | 27 +++++ .../tracers/xray/xray_configuration.h | 25 +++++ .../tracers/xray/xray_tracer_impl.cc | 44 ++++++++ .../tracers/xray/xray_tracer_impl.h | 33 ++++++ test/extensions/tracers/xray/BUILD | 24 ++++ test/extensions/tracers/xray/util_test.cc | 103 ++++++++++++++++++ 16 files changed, 538 insertions(+) create mode 100644 source/extensions/tracers/xray/BUILD create mode 100644 source/extensions/tracers/xray/config.cc create mode 100644 source/extensions/tracers/xray/config.h create mode 100644 source/extensions/tracers/xray/sampling_strategy.h create mode 100644 source/extensions/tracers/xray/tracer.h create mode 100644 source/extensions/tracers/xray/util.cc create mode 100644 source/extensions/tracers/xray/util.h create mode 100644 source/extensions/tracers/xray/xray_configuration.h create mode 100644 source/extensions/tracers/xray/xray_tracer_impl.cc create mode 100644 source/extensions/tracers/xray/xray_tracer_impl.h create mode 100644 test/extensions/tracers/xray/BUILD create mode 100644 test/extensions/tracers/xray/util_test.cc diff --git a/CODEOWNERS b/CODEOWNERS index 62327d48ac91..7c795a1a8720 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -32,6 +32,8 @@ extensions/filters/common/original_src @snowp @klarose /*/extensions/filters/network/sni_cluster @rshriram @lizan # tracers.datadog extension /*/extensions/tracers/datadog @cgilmour @palazzem +# tracers.xray extension +/*/extensions/tracers/xray @marcomagdy @lavignes # mysql_proxy extension /*/extensions/filters/network/mysql_proxy @rshriram @venilnoronha @mattklein123 # quic extension diff --git a/api/envoy/config/trace/v2/trace.proto b/api/envoy/config/trace/v2/trace.proto index 1fad6beb2ad3..5fe4a604ef28 100644 --- a/api/envoy/config/trace/v2/trace.proto +++ b/api/envoy/config/trace/v2/trace.proto @@ -34,6 +34,7 @@ message Tracing { // - *envoy.dynamic.ot* // - *envoy.tracers.datadog* // - *envoy.tracers.opencensus* + // - *envoy.tracers.xray* string name = 1 [(validate.rules).string = {min_bytes: 1}]; // Trace driver specific configuration which depends on the driver being instantiated. @@ -44,6 +45,7 @@ message Tracing { // - :ref:`DynamicOtConfig ` // - :ref:`DatadogConfig ` // - :ref:`OpenCensusConfig ` + // - :ref:`XRayConfig ` oneof config_type { google.protobuf.Struct config = 2; @@ -197,6 +199,19 @@ message OpenCensusConfig { repeated TraceContext outgoing_trace_context = 9; } +// Configuration for AWS X-Ray tracer. +message XRayConfig { + // The endpoint of the X-Ray Daemon where the spans will be sent. Since by default daemon + // listens to localhost:2000, so the default value is 127.0.0.1:2000. + string daemon_endpoint = 1 [(validate.rules).string.min_bytes = 1]; + + // The custom name to name a X-Ray segment. By default will use cluster name. + string segment_name = 2; + + // The location of custom sampling rule json file. + string json_file = 3; +} + // Configuration structure. message TraceServiceConfig { // The upstream gRPC cluster that hosts the metrics service. diff --git a/source/extensions/extensions_build_config.bzl b/source/extensions/extensions_build_config.bzl index fd78ee5b7811..7240b0468ee8 100644 --- a/source/extensions/extensions_build_config.bzl +++ b/source/extensions/extensions_build_config.bzl @@ -124,6 +124,7 @@ EXTENSIONS = { "envoy.tracers.datadog": "//source/extensions/tracers/datadog:config", "envoy.tracers.zipkin": "//source/extensions/tracers/zipkin:config", "envoy.tracers.opencensus": "//source/extensions/tracers/opencensus:config", + "envoy.tracers.xray": "//source/extensions/tracers/xray:config", # # Transport sockets diff --git a/source/extensions/tracers/well_known_names.h b/source/extensions/tracers/well_known_names.h index a756df8de089..8b48fd0aeb54 100644 --- a/source/extensions/tracers/well_known_names.h +++ b/source/extensions/tracers/well_known_names.h @@ -23,6 +23,8 @@ class TracerNameValues { const std::string Datadog = "envoy.tracers.datadog"; // OpenCensus tracer const std::string OpenCensus = "envoy.tracers.opencensus"; + // AWS XRay tracer + const std::string XRay = "envoy.tracers.xray"; }; using TracerNames = ConstSingleton; diff --git a/source/extensions/tracers/xray/BUILD b/source/extensions/tracers/xray/BUILD new file mode 100644 index 000000000000..192d382e972a --- /dev/null +++ b/source/extensions/tracers/xray/BUILD @@ -0,0 +1,45 @@ +licenses(["notice"]) # Apache 2 + +# Trace driver for AWS X-Ray. + +load( + "//bazel:envoy_build_system.bzl", + "envoy_cc_library", + "envoy_package", +) + +envoy_package() + +envoy_cc_library( + name = "xray_lib", + srcs = [ + "util.cc", + "xray_tracer_impl.cc", + ], + hdrs = [ + "sampling_strategy.h", + "tracer.h", + "util.h", + "xray_configuration.h", + "xray_tracer_impl.h", + ], + deps = [ + "//include/envoy/common:time_interface", + "//include/envoy/server:instance_interface", + "//include/envoy/tracing:http_tracer_interface", + "//source/common/http:header_map_lib", + "//source/common/json:json_loader_lib", + "//source/common/tracing:http_tracer_lib", + ], +) + +envoy_cc_library( + name = "config", + srcs = ["config.cc"], + hdrs = ["config.h"], + deps = [ + ":xray_lib", + "//source/extensions/tracers:well_known_names", + "//source/extensions/tracers/common:factory_base_lib", + ], +) diff --git a/source/extensions/tracers/xray/config.cc b/source/extensions/tracers/xray/config.cc new file mode 100644 index 000000000000..1b5243869298 --- /dev/null +++ b/source/extensions/tracers/xray/config.cc @@ -0,0 +1,46 @@ +#include "extensions/tracers/xray/config.h" + +#include + +#include "envoy/registry/registry.h" + +#include "common/common/utility.h" +#include "common/tracing/http_tracer_impl.h" + +#include "extensions/tracers/well_known_names.h" +#include "extensions/tracers/xray/xray_tracer_impl.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace XRay { + +XRayTracerFactory::XRayTracerFactory() : FactoryBase(TracerNames::get().XRay) {} + +Tracing::HttpTracerPtr +XRayTracerFactory::createHttpTracerTyped(const envoy::config::trace::v2::XRayConfig& proto_config, + Server::Instance& server) { + std::string sampling_rule_json; + try { + sampling_rules_json = server.api().fileSystem().fileReadToEnd(proto_config.json_file()); + } catch (EnvoyException& e) { + ENVOY_LOG(error, "Could not read custom sampling rule json file: {}, because of {}.", + proto_config.json_file(), e.what()); + } + + XRayConfiguration xconfig(proto_config.daemon_endpoint, proto_config.segment_name, + sampling_rules_json); + auto xray_driver = std::make_unique(xconfig, server); + + return std::make_unique(std::move(xray_driver), server.localInfo()); +} + +/** + * Static registration for the XRay tracer. @see RegisterFactory. + */ +REGISTER_FACTORY(XRayTracerFactory, Server::Configuration::TracerFactory); + +} // namespace XRay +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/tracers/xray/config.h b/source/extensions/tracers/xray/config.h new file mode 100644 index 000000000000..a02b590d9cf4 --- /dev/null +++ b/source/extensions/tracers/xray/config.h @@ -0,0 +1,31 @@ +#pragma once + +#include "envoy/config/trace/v2/trace.pb.validate.h" + +#include "common/common/logger.h" + +#include "extensions/tracers/common/factory_base.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace XRay { + +/** + * Config registration for the XRay tracer. @see TracerFactory. + */ +class XRayTracerFactory : public Common::FactoryBase, + Logger::Loggable { +public: + XRayTracerFactory(); + +private: + Tracing::HttpTracerPtr + createHttpTracerTyped(const envoy::config::trace::v2::XRayConfig& proto_config, + Server::Instance& server) override; +}; + +} // namespace XRay +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/tracers/xray/sampling_strategy.h b/source/extensions/tracers/xray/sampling_strategy.h new file mode 100644 index 000000000000..14f47e34a609 --- /dev/null +++ b/source/extensions/tracers/xray/sampling_strategy.h @@ -0,0 +1,57 @@ +#pragma once + +#include +#include + +#include "envoy/common/pure.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace XRay { + +struct SamplingRequest { + /** + * Creates a new SamplingRequest + * + * @param host_name The host name the request. + * @param http_method The http method of the request e.g. GET, POST, etc. + * @param http_url The path part of the URL of the request. + * @param service The name of the service (user specified) + * @param service_type The type of the service (user specified) + */ + SamplingRequest(absl::string_view host_name, absl::string_view http_method, + absl::string_view http_url) + : host_(host_name), http_method_(http_method), http_url_(http_url) {} + + const std::string host_; + const std::string http_method_; + const std::string http_url_; +}; + +/** + * Strategy provides an interface for implementing trace sampling strategies. + */ +class SamplingStrategy { +public: + explicit SamplingStrategy(uint64_t rng_seed) : rng_(rng_seed) {} + virtual ~SamplingStrategy() = default; + + /** + * sampleRequest determines if the given request should be traced or not. + */ + virtual bool sampleRequest(const SamplingRequest& sampling_request) { + (void)sampling_request; // unused for now + return rng_() % 100 == 42; + } + +private: + std::mt19937 rng_; +}; + +using SamplingStrategyPtr = std::unique_ptr; + +} // namespace XRay +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/tracers/xray/tracer.h b/source/extensions/tracers/xray/tracer.h new file mode 100644 index 000000000000..d583b53d3cd0 --- /dev/null +++ b/source/extensions/tracers/xray/tracer.h @@ -0,0 +1,34 @@ +#pragma once + +#include + +#include "envoy/common/time.h" + +#include "extensions/tracers/xray/sampling_strategy.h" + +#include "absl/strings/string_view.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace XRay { + +class Tracer { +public: + Tracer(absl::string_view segment_name, TimeSource& time_source) + : segment_name_(segment_name), time_source_(time_source) {} + + /** + * Starts a tracing span for XRay + */ + Tracing::SpanPtr startSpan() { return nullptr; } + +private: + const std::string segment_name_; + TimeSource& time_source_; +}; + +} // namespace XRay +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/tracers/xray/util.cc b/source/extensions/tracers/xray/util.cc new file mode 100644 index 000000000000..c90a8f5250df --- /dev/null +++ b/source/extensions/tracers/xray/util.cc @@ -0,0 +1,49 @@ +#include "extensions/tracers/xray/util.h" + +#include "absl/strings/ascii.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace XRay { + +bool WildcardMatch(absl::string_view pattern, absl::string_view input) { + if (pattern.empty()) { + return input.empty(); + } + + // Check the special case of a single * pattern, as it's common. + constexpr char glob = '*'; + if (pattern.size() == 1 && pattern[0] == glob) { + return true; + } + + size_t i = 0, p = 0, iStar = input.size(), pStar = 0; + while (i < input.size()) { + if (p < pattern.size() && absl::ascii_tolower(input[i]) == absl::ascii_tolower(pattern[p])) { + ++i; + ++p; + } else if (p < pattern.size() && '?' == pattern[p]) { + ++i; + ++p; + } else if (p < pattern.size() && pattern[p] == glob) { + iStar = i; + pStar = p++; + } else if (iStar != input.size()) { + i = ++iStar; + p = pStar + 1; + } else + return false; + } + + while (p < pattern.size() && pattern[p] == glob) { + ++p; + } + + return p == pattern.size() && i == input.size(); +} + +} // namespace XRay +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/tracers/xray/util.h b/source/extensions/tracers/xray/util.h new file mode 100644 index 000000000000..4ed8ef2dcaca --- /dev/null +++ b/source/extensions/tracers/xray/util.h @@ -0,0 +1,27 @@ +#pragma once +#include + +#include "absl/strings/string_view.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace XRay { + +/** + * Performs a case-insensitive wild-card match against the input string. + * This method works with pseudo-regex chars; specifically ? and * + * + * An asterisk (*) represents any combination of characters. + * A question mark (?) represents any single character. + * + * @param pattern The regex-like pattern to compare with. + * @param text The string to compare against the pattern. + * @return whether the text matches the pattern. + */ + +bool WildcardMatch(absl::string_view pattern, absl::string_view input); +} // namespace XRay +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/tracers/xray/xray_configuration.h b/source/extensions/tracers/xray/xray_configuration.h new file mode 100644 index 000000000000..c35c3f111fe0 --- /dev/null +++ b/source/extensions/tracers/xray/xray_configuration.h @@ -0,0 +1,25 @@ +#pragma once +#include + +#include "absl/strings/string_view.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace XRay { + +struct XRayConfiguration { + XRayConfiguration(absl::string_view daemon_endpoint, absl::string_view segment_name, + absl::string_view sampling_rules) + : daemon_endpoint_(daemon_endpoint), segment_name_(segment_name), + sampling_rules_(sampling_rules) {} + + const std::string daemon_endpoint_; + const std::string segment_name_; + const std::string sampling_rules_; +}; + +} // namespace XRay +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/tracers/xray/xray_tracer_impl.cc b/source/extensions/tracers/xray/xray_tracer_impl.cc new file mode 100644 index 000000000000..ecbcc8c39228 --- /dev/null +++ b/source/extensions/tracers/xray/xray_tracer_impl.cc @@ -0,0 +1,44 @@ +#include "extensions/tracers/xray/xray_tracer_impl.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace XRay { + +static const char DEFAULT_DAEMON_ENDPOINT[] = "127.0.0.1:2000"; +static const char ENV_APPMESH_NODE_NAME[] = "APPMESH_VIRTUAL_NODE_NAME"; +Driver::Driver(const XRayConfiguration& config, Server::Instance& server) : xray_config_(config) { + + const std::string daemon_endpoint = + config.daemon_endpoint_.empty() ? DEFAULT_DAEMON_ENDPOINT : config.daemon_endpoint_; + + ENVOY_LOG(debug, "send X-Ray generated segments to daemon address on {}", daemon_endpoint); + std::string span_name = + config.segment_name_.empty() ? server.localInfo().clusterName() : config.segment_name_; + + sampling_strategy_ = std::make_unique(server.random().random()); + tracer_.emplace(span_name, server.timeSource()); +} + +Tracing::SpanPtr Driver::startSpan(const Tracing::Config& config, Http::HeaderMap& request_headers, + const std::string& operation_name, SystemTime start_time, + const Tracing::Decision tracing_decision) { + + (void)config; + (void)operation_name; + (void)start_time; + (void)tracing_decision; + const SamplingRequest request{request_headers.Host()->value().getStringView(), + request_headers.Method()->value().getStringView(), + request_headers.Path()->value().getStringView()}; + + if (!sampling_strategy_->sampleRequest(request)) { + return nullptr; + } + + return tracer_->startSpan(); +} +} // namespace XRay +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/tracers/xray/xray_tracer_impl.h b/source/extensions/tracers/xray/xray_tracer_impl.h new file mode 100644 index 000000000000..c1c4dd84d785 --- /dev/null +++ b/source/extensions/tracers/xray/xray_tracer_impl.h @@ -0,0 +1,33 @@ +#pragma once + +#include "envoy/server/instance.h" +#include "envoy/tracing/http_tracer.h" + +#include "common/tracing/http_tracer_impl.h" + +#include "extensions/tracers/xray/tracer.h" +#include "extensions/tracers/xray/xray_configuration.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace XRay { + +class Driver : public Tracing::Driver, Logger::Loggable { +public: + Driver(const XRay::XRayConfiguration& config, Server::Instance& server); + + Tracing::SpanPtr startSpan(const Tracing::Config& config, Http::HeaderMap& request_headers, + const std::string& operation_name, SystemTime start_time, + const Tracing::Decision tracing_decision) override; + +private: + XRayConfiguration xray_config_; + SamplingStrategyPtr sampling_strategy_; + absl::optional tracer_; // optional<> for delayed construction +}; + +} // namespace XRay +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/test/extensions/tracers/xray/BUILD b/test/extensions/tracers/xray/BUILD new file mode 100644 index 000000000000..ca373d08d328 --- /dev/null +++ b/test/extensions/tracers/xray/BUILD @@ -0,0 +1,24 @@ +licenses(["notice"]) # Apache 2 + +load( + "//bazel:envoy_build_system.bzl", + "envoy_cc_test_library", + "envoy_package", +) +load( + "//test/extensions:extensions_build_system.bzl", + "envoy_extension_cc_test", +) + +envoy_package() + +envoy_extension_cc_test( + name = "xray_test", + srcs = [ + "util_test.cc", + ], + extension_name = "envoy.tracers.xray", + deps = [ + "//source/extensions/tracers/xray:xray_lib", + ], +) diff --git a/test/extensions/tracers/xray/util_test.cc b/test/extensions/tracers/xray/util_test.cc new file mode 100644 index 000000000000..4b5ae1e664dd --- /dev/null +++ b/test/extensions/tracers/xray/util_test.cc @@ -0,0 +1,103 @@ +#include "extensions/tracers/xray/util.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace XRay { + +TEST(XRayUtilTest, WildcardMatchingInvalidArgs) { + const std::string pattern = ""; + const std::string text = "whatever"; + ASSERT_FALSE(WildcardMatch(pattern, text)); +} + +TEST(XRayUtilTest, WildcardMatchExactPositive) { + const std::string pattern = "foo"; + const std::string text = "foo"; + ASSERT_TRUE(WildcardMatch(pattern, text)); +} + +TEST(XRayUtilTest, WildcardMatchExactNegative) { + const std::string pattern = "foo"; + const std::string text = "bar"; + ASSERT_FALSE(WildcardMatch(pattern, text)); +} + +TEST(XRayUtilTest, singleWildcardPositive) { + const std::string pattern = "fo?"; + const std::string text = "foo"; + ASSERT_TRUE(WildcardMatch(pattern, text)); +} + +TEST(XRayUtilTest, singleWildcardNegative) { + const std::string pattern = "f?o"; + const std::string text = "boo"; + ASSERT_FALSE(WildcardMatch(pattern, text)); +} + +TEST(XRayUtilTest, multipleWildcardPositive) { + const std::string pattern = "?o?"; + const std::string text = "foo"; + ASSERT_TRUE(WildcardMatch(pattern, text)); +} + +TEST(XRayUtilTest, multipleWildcardNegative) { + const std::string pattern = "f??"; + const std::string text = "boo"; + ASSERT_FALSE(WildcardMatch(pattern, text)); +} + +TEST(XRayUtilTest, globPositive) { + const std::string pattern = "*oo"; + const std::string text = "foo"; + ASSERT_TRUE(WildcardMatch(pattern, text)); +} + +TEST(XRayUtilTest, globPositiveZeroOrMore) { + const std::string pattern = "f?o*ba*"; + const std::string text = "foobazbar"; + ASSERT_TRUE(WildcardMatch(pattern, text)); +} + +TEST(XRayUtilTest, globNegativeZeroOrMore) { + const std::string pattern = "foo*"; + const std::string text = "fo0"; + ASSERT_FALSE(WildcardMatch(pattern, text)); +} + +TEST(XRayUtilTest, globNegative) { + const std::string pattern = "fo*"; + const std::string text = "boo"; + ASSERT_FALSE(WildcardMatch(pattern, text)); +} + +TEST(XRayUtilTest, globAndSinglePositive) { + const std::string pattern = "*o?"; + const std::string text = "foo"; + ASSERT_TRUE(WildcardMatch(pattern, text)); +} + +TEST(XRayUtilTest, globAndSingleNegative) { + const std::string pattern = "f?*"; + const std::string text = "boo"; + ASSERT_FALSE(WildcardMatch(pattern, text)); +} + +TEST(XRayUtilTest, pureWildcard) { + const std::string pattern = "*"; + const std::string text = "foo"; + ASSERT_TRUE(WildcardMatch(pattern, text)); +} + +TEST(XRayUtilTest, exactMatch) { + const std::string pattern = "6543210"; + const std::string text = "6543210"; + ASSERT_TRUE(WildcardMatch(pattern, text)); +} + +} // namespace XRay +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy From bbadcb4c7c790587dfff4d1dcf0e3dfc5b88c1ef Mon Sep 17 00:00:00 2001 From: Marco Magdy Date: Mon, 7 Oct 2019 23:30:52 +0000 Subject: [PATCH 02/19] Fix proto format Signed-off-by: Marco Magdy --- api/envoy/config/trace/v2/trace.proto | 2 +- api/envoy/config/trace/v3alpha/trace.proto | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/api/envoy/config/trace/v2/trace.proto b/api/envoy/config/trace/v2/trace.proto index 5fe4a604ef28..19854886b50f 100644 --- a/api/envoy/config/trace/v2/trace.proto +++ b/api/envoy/config/trace/v2/trace.proto @@ -203,7 +203,7 @@ message OpenCensusConfig { message XRayConfig { // The endpoint of the X-Ray Daemon where the spans will be sent. Since by default daemon // listens to localhost:2000, so the default value is 127.0.0.1:2000. - string daemon_endpoint = 1 [(validate.rules).string.min_bytes = 1]; + string daemon_endpoint = 1 [(validate.rules).string = {min_bytes: 1}]; // The custom name to name a X-Ray segment. By default will use cluster name. string segment_name = 2; diff --git a/api/envoy/config/trace/v3alpha/trace.proto b/api/envoy/config/trace/v3alpha/trace.proto index 238a6ad6bdb8..42af6c2b702a 100644 --- a/api/envoy/config/trace/v3alpha/trace.proto +++ b/api/envoy/config/trace/v3alpha/trace.proto @@ -34,6 +34,7 @@ message Tracing { // - *envoy.dynamic.ot* // - *envoy.tracers.datadog* // - *envoy.tracers.opencensus* + // - *envoy.tracers.xray* string name = 1 [(validate.rules).string = {min_bytes: 1}]; // Trace driver specific configuration which depends on the driver being instantiated. @@ -44,6 +45,7 @@ message Tracing { // - :ref:`DynamicOtConfig ` // - :ref:`DatadogConfig ` // - :ref:`OpenCensusConfig ` + // - :ref:`XRayConfig ` oneof config_type { google.protobuf.Struct config = 2; @@ -197,6 +199,19 @@ message OpenCensusConfig { repeated TraceContext outgoing_trace_context = 9; } +// Configuration for AWS X-Ray tracer. +message XRayConfig { + // The endpoint of the X-Ray Daemon where the spans will be sent. Since by default daemon + // listens to localhost:2000, so the default value is 127.0.0.1:2000. + string daemon_endpoint = 1 [(validate.rules).string = {min_bytes: 1}]; + + // The custom name to name a X-Ray segment. By default will use cluster name. + string segment_name = 2; + + // The location of custom sampling rule json file. + string json_file = 3; +} + // Configuration structure. message TraceServiceConfig { // The upstream gRPC cluster that hosts the metrics service. From b17c374d57b6efaccdfd24cac2efca12d42b4c37 Mon Sep 17 00:00:00 2001 From: Marco Magdy Date: Tue, 8 Oct 2019 03:41:28 +0000 Subject: [PATCH 03/19] Not sure why gcc did not catch all this Signed-off-by: Marco Magdy --- source/extensions/tracers/xray/config.cc | 4 ++-- source/extensions/tracers/xray/tracer.h | 4 +++- source/extensions/tracers/xray/util.cc | 3 ++- source/extensions/tracers/xray/xray_tracer_impl.cc | 1 - test/extensions/tracers/xray/util_test.cc | 2 +- 5 files changed, 8 insertions(+), 6 deletions(-) diff --git a/source/extensions/tracers/xray/config.cc b/source/extensions/tracers/xray/config.cc index 1b5243869298..34ea92f723de 100644 --- a/source/extensions/tracers/xray/config.cc +++ b/source/extensions/tracers/xray/config.cc @@ -20,7 +20,7 @@ XRayTracerFactory::XRayTracerFactory() : FactoryBase(TracerNames::get().XRay) {} Tracing::HttpTracerPtr XRayTracerFactory::createHttpTracerTyped(const envoy::config::trace::v2::XRayConfig& proto_config, Server::Instance& server) { - std::string sampling_rule_json; + std::string sampling_rules_json; try { sampling_rules_json = server.api().fileSystem().fileReadToEnd(proto_config.json_file()); } catch (EnvoyException& e) { @@ -28,7 +28,7 @@ XRayTracerFactory::createHttpTracerTyped(const envoy::config::trace::v2::XRayCon proto_config.json_file(), e.what()); } - XRayConfiguration xconfig(proto_config.daemon_endpoint, proto_config.segment_name, + XRayConfiguration xconfig(proto_config.daemon_endpoint(), proto_config.segment_name(), sampling_rules_json); auto xray_driver = std::make_unique(xconfig, server); diff --git a/source/extensions/tracers/xray/tracer.h b/source/extensions/tracers/xray/tracer.h index d583b53d3cd0..b97a2455e56e 100644 --- a/source/extensions/tracers/xray/tracer.h +++ b/source/extensions/tracers/xray/tracer.h @@ -16,7 +16,9 @@ namespace XRay { class Tracer { public: Tracer(absl::string_view segment_name, TimeSource& time_source) - : segment_name_(segment_name), time_source_(time_source) {} + : segment_name_(segment_name), time_source_(time_source) { + (void)time_source_; + } /** * Starts a tracing span for XRay diff --git a/source/extensions/tracers/xray/util.cc b/source/extensions/tracers/xray/util.cc index c90a8f5250df..bf42e9575c3c 100644 --- a/source/extensions/tracers/xray/util.cc +++ b/source/extensions/tracers/xray/util.cc @@ -32,8 +32,9 @@ bool WildcardMatch(absl::string_view pattern, absl::string_view input) { } else if (iStar != input.size()) { i = ++iStar; p = pStar + 1; - } else + } else { return false; + } } while (p < pattern.size() && pattern[p] == glob) { diff --git a/source/extensions/tracers/xray/xray_tracer_impl.cc b/source/extensions/tracers/xray/xray_tracer_impl.cc index ecbcc8c39228..61a434084a9d 100644 --- a/source/extensions/tracers/xray/xray_tracer_impl.cc +++ b/source/extensions/tracers/xray/xray_tracer_impl.cc @@ -6,7 +6,6 @@ namespace Tracers { namespace XRay { static const char DEFAULT_DAEMON_ENDPOINT[] = "127.0.0.1:2000"; -static const char ENV_APPMESH_NODE_NAME[] = "APPMESH_VIRTUAL_NODE_NAME"; Driver::Driver(const XRayConfiguration& config, Server::Instance& server) : xray_config_(config) { const std::string daemon_endpoint = diff --git a/test/extensions/tracers/xray/util_test.cc b/test/extensions/tracers/xray/util_test.cc index 4b5ae1e664dd..3013bb357481 100644 --- a/test/extensions/tracers/xray/util_test.cc +++ b/test/extensions/tracers/xray/util_test.cc @@ -8,7 +8,7 @@ namespace Tracers { namespace XRay { TEST(XRayUtilTest, WildcardMatchingInvalidArgs) { - const std::string pattern = ""; + const std::string pattern; const std::string text = "whatever"; ASSERT_FALSE(WildcardMatch(pattern, text)); } From fa410ccd78d3d76f2015890165b2be7b587530aa Mon Sep 17 00:00:00 2001 From: Marco Magdy Date: Tue, 8 Oct 2019 17:44:17 +0000 Subject: [PATCH 04/19] Fix clang-tidy errors Signed-off-by: Marco Magdy --- source/extensions/tracers/xray/sampling_strategy.h | 2 ++ source/extensions/tracers/xray/tracer.h | 1 + 2 files changed, 3 insertions(+) diff --git a/source/extensions/tracers/xray/sampling_strategy.h b/source/extensions/tracers/xray/sampling_strategy.h index 14f47e34a609..814577a620dd 100644 --- a/source/extensions/tracers/xray/sampling_strategy.h +++ b/source/extensions/tracers/xray/sampling_strategy.h @@ -5,6 +5,8 @@ #include "envoy/common/pure.h" +#include "absl/strings/string_view.h" + namespace Envoy { namespace Extensions { namespace Tracers { diff --git a/source/extensions/tracers/xray/tracer.h b/source/extensions/tracers/xray/tracer.h index b97a2455e56e..f2ca3f0fded4 100644 --- a/source/extensions/tracers/xray/tracer.h +++ b/source/extensions/tracers/xray/tracer.h @@ -3,6 +3,7 @@ #include #include "envoy/common/time.h" +#include "envoy/tracing/http_tracer.h" #include "extensions/tracers/xray/sampling_strategy.h" From 7be86ba77d7d51f20a5f95eda8ff8ec11c2ebe08 Mon Sep 17 00:00:00 2001 From: Marco Magdy Date: Tue, 8 Oct 2019 20:31:25 +0000 Subject: [PATCH 05/19] review feedback Signed-off-by: Marco Magdy --- source/extensions/tracers/xray/util.cc | 2 +- source/extensions/tracers/xray/util.h | 2 +- .../tracers/xray/xray_tracer_impl.cc | 4 +- test/extensions/tracers/xray/util_test.cc | 113 ++++++------------ 4 files changed, 42 insertions(+), 79 deletions(-) diff --git a/source/extensions/tracers/xray/util.cc b/source/extensions/tracers/xray/util.cc index bf42e9575c3c..2025db1062c2 100644 --- a/source/extensions/tracers/xray/util.cc +++ b/source/extensions/tracers/xray/util.cc @@ -7,7 +7,7 @@ namespace Extensions { namespace Tracers { namespace XRay { -bool WildcardMatch(absl::string_view pattern, absl::string_view input) { +bool wildcardMatch(absl::string_view pattern, absl::string_view input) { if (pattern.empty()) { return input.empty(); } diff --git a/source/extensions/tracers/xray/util.h b/source/extensions/tracers/xray/util.h index 4ed8ef2dcaca..762caf6df34d 100644 --- a/source/extensions/tracers/xray/util.h +++ b/source/extensions/tracers/xray/util.h @@ -20,7 +20,7 @@ namespace XRay { * @return whether the text matches the pattern. */ -bool WildcardMatch(absl::string_view pattern, absl::string_view input); +bool wildcardMatch(absl::string_view pattern, absl::string_view input); } // namespace XRay } // namespace Tracers } // namespace Extensions diff --git a/source/extensions/tracers/xray/xray_tracer_impl.cc b/source/extensions/tracers/xray/xray_tracer_impl.cc index 61a434084a9d..431139b1ee63 100644 --- a/source/extensions/tracers/xray/xray_tracer_impl.cc +++ b/source/extensions/tracers/xray/xray_tracer_impl.cc @@ -5,11 +5,11 @@ namespace Extensions { namespace Tracers { namespace XRay { -static const char DEFAULT_DAEMON_ENDPOINT[] = "127.0.0.1:2000"; +static const char DefaultDaemonEndpoint[] = "127.0.0.1:2000"; Driver::Driver(const XRayConfiguration& config, Server::Instance& server) : xray_config_(config) { const std::string daemon_endpoint = - config.daemon_endpoint_.empty() ? DEFAULT_DAEMON_ENDPOINT : config.daemon_endpoint_; + config.daemon_endpoint_.empty() ? DefaultDaemonEndpoint : config.daemon_endpoint_; ENVOY_LOG(debug, "send X-Ray generated segments to daemon address on {}", daemon_endpoint); std::string span_name = diff --git a/test/extensions/tracers/xray/util_test.cc b/test/extensions/tracers/xray/util_test.cc index 3013bb357481..7f0e5ec7cf44 100644 --- a/test/extensions/tracers/xray/util_test.cc +++ b/test/extensions/tracers/xray/util_test.cc @@ -7,96 +7,59 @@ namespace Extensions { namespace Tracers { namespace XRay { -TEST(XRayUtilTest, WildcardMatchingInvalidArgs) { - const std::string pattern; - const std::string text = "whatever"; - ASSERT_FALSE(WildcardMatch(pattern, text)); +TEST(XRayWildcardTest, matchingEmpty) { + ASSERT_TRUE(wildcardMatch("", "")); + ASSERT_FALSE(wildcardMatch("", "42")); + ASSERT_TRUE(wildcardMatch("*", "")); + ASSERT_FALSE(wildcardMatch("?", "")); } -TEST(XRayUtilTest, WildcardMatchExactPositive) { - const std::string pattern = "foo"; - const std::string text = "foo"; - ASSERT_TRUE(WildcardMatch(pattern, text)); +TEST(XRayWildcardTest, matchIdentityCaseInsensitive) { + ASSERT_TRUE(wildcardMatch("foo", "foo")); + ASSERT_TRUE(wildcardMatch("foo", "FOO")); + ASSERT_TRUE(wildcardMatch("foo", "Foo")); + ASSERT_TRUE(wildcardMatch("6543210", "6543210")); } -TEST(XRayUtilTest, WildcardMatchExactNegative) { - const std::string pattern = "foo"; - const std::string text = "bar"; - ASSERT_FALSE(WildcardMatch(pattern, text)); +TEST(XRayWildcardTest, matchIdentityExtra) { + ASSERT_FALSE(wildcardMatch("foo", "foob")); + ASSERT_FALSE(wildcardMatch("foo", "xfoo")); + ASSERT_FALSE(wildcardMatch("foo", "bar")); } -TEST(XRayUtilTest, singleWildcardPositive) { - const std::string pattern = "fo?"; - const std::string text = "foo"; - ASSERT_TRUE(WildcardMatch(pattern, text)); +TEST(XRayWildcardTest, singleWildcard) { + ASSERT_FALSE(wildcardMatch("f?o", "boo")); + ASSERT_TRUE(wildcardMatch("fo?", "foo")); } -TEST(XRayUtilTest, singleWildcardNegative) { - const std::string pattern = "f?o"; - const std::string text = "boo"; - ASSERT_FALSE(WildcardMatch(pattern, text)); +TEST(XRayWildcardTest, multipleWildcards) { + ASSERT_FALSE(wildcardMatch("f??", "boo")); + ASSERT_TRUE(wildcardMatch("he??o", "Hello")); + ASSERT_TRUE(wildcardMatch("?o?", "foo")); } -TEST(XRayUtilTest, multipleWildcardPositive) { - const std::string pattern = "?o?"; - const std::string text = "foo"; - ASSERT_TRUE(WildcardMatch(pattern, text)); +TEST(XRayWildcardTest, globMatch) { + ASSERT_TRUE(wildcardMatch("f?o*ba*", "foobazbar")); + ASSERT_TRUE(wildcardMatch("*oo", "foo")); + ASSERT_TRUE(wildcardMatch("*o?", "foo")); + ASSERT_TRUE(wildcardMatch("mis*spell", "mistily spell")); + ASSERT_TRUE(wildcardMatch("mis*spell", "misspell")); } -TEST(XRayUtilTest, multipleWildcardNegative) { - const std::string pattern = "f??"; - const std::string text = "boo"; - ASSERT_FALSE(WildcardMatch(pattern, text)); +TEST(XRayWildcardTest, globMismatch) { + ASSERT_FALSE(wildcardMatch("foo*", "fo0")); + ASSERT_FALSE(wildcardMatch("fo*obar", "foobaz")); + ASSERT_FALSE(wildcardMatch("mis*spell", "mispell")); + ASSERT_FALSE(wildcardMatch("f?*", "boo")); } -TEST(XRayUtilTest, globPositive) { - const std::string pattern = "*oo"; - const std::string text = "foo"; - ASSERT_TRUE(WildcardMatch(pattern, text)); +TEST(XRayWildcardTest, onlyGlob) { + ASSERT_TRUE(wildcardMatch("*", "foo")); + ASSERT_TRUE(wildcardMatch("*", "anything")); + ASSERT_TRUE(wildcardMatch("*", "12354")); + ASSERT_TRUE(wildcardMatch("*", "UPPERCASE")); + ASSERT_TRUE(wildcardMatch("*", "miXEDcaSe")); } - -TEST(XRayUtilTest, globPositiveZeroOrMore) { - const std::string pattern = "f?o*ba*"; - const std::string text = "foobazbar"; - ASSERT_TRUE(WildcardMatch(pattern, text)); -} - -TEST(XRayUtilTest, globNegativeZeroOrMore) { - const std::string pattern = "foo*"; - const std::string text = "fo0"; - ASSERT_FALSE(WildcardMatch(pattern, text)); -} - -TEST(XRayUtilTest, globNegative) { - const std::string pattern = "fo*"; - const std::string text = "boo"; - ASSERT_FALSE(WildcardMatch(pattern, text)); -} - -TEST(XRayUtilTest, globAndSinglePositive) { - const std::string pattern = "*o?"; - const std::string text = "foo"; - ASSERT_TRUE(WildcardMatch(pattern, text)); -} - -TEST(XRayUtilTest, globAndSingleNegative) { - const std::string pattern = "f?*"; - const std::string text = "boo"; - ASSERT_FALSE(WildcardMatch(pattern, text)); -} - -TEST(XRayUtilTest, pureWildcard) { - const std::string pattern = "*"; - const std::string text = "foo"; - ASSERT_TRUE(WildcardMatch(pattern, text)); -} - -TEST(XRayUtilTest, exactMatch) { - const std::string pattern = "6543210"; - const std::string text = "6543210"; - ASSERT_TRUE(WildcardMatch(pattern, text)); -} - } // namespace XRay } // namespace Tracers } // namespace Extensions From 746415314fe6fca6981497840de5dd02176ca3b9 Mon Sep 17 00:00:00 2001 From: Marco Magdy Date: Tue, 8 Oct 2019 21:03:35 +0000 Subject: [PATCH 06/19] Add a TODO and remove blank line Signed-off-by: Marco Magdy --- source/extensions/tracers/xray/util.cc | 1 + source/extensions/tracers/xray/util.h | 1 - test/extensions/tracers/xray/util_test.cc | 2 +- 3 files changed, 2 insertions(+), 2 deletions(-) diff --git a/source/extensions/tracers/xray/util.cc b/source/extensions/tracers/xray/util.cc index 2025db1062c2..960bcfc69715 100644 --- a/source/extensions/tracers/xray/util.cc +++ b/source/extensions/tracers/xray/util.cc @@ -7,6 +7,7 @@ namespace Extensions { namespace Tracers { namespace XRay { +// TODO(magmarco): fuzz test this function bool wildcardMatch(absl::string_view pattern, absl::string_view input) { if (pattern.empty()) { return input.empty(); diff --git a/source/extensions/tracers/xray/util.h b/source/extensions/tracers/xray/util.h index 762caf6df34d..76a3ec5fc75e 100644 --- a/source/extensions/tracers/xray/util.h +++ b/source/extensions/tracers/xray/util.h @@ -19,7 +19,6 @@ namespace XRay { * @param text The string to compare against the pattern. * @return whether the text matches the pattern. */ - bool wildcardMatch(absl::string_view pattern, absl::string_view input); } // namespace XRay } // namespace Tracers diff --git a/test/extensions/tracers/xray/util_test.cc b/test/extensions/tracers/xray/util_test.cc index 7f0e5ec7cf44..e85afd6ed43d 100644 --- a/test/extensions/tracers/xray/util_test.cc +++ b/test/extensions/tracers/xray/util_test.cc @@ -49,7 +49,7 @@ TEST(XRayWildcardTest, globMatch) { TEST(XRayWildcardTest, globMismatch) { ASSERT_FALSE(wildcardMatch("foo*", "fo0")); ASSERT_FALSE(wildcardMatch("fo*obar", "foobaz")); - ASSERT_FALSE(wildcardMatch("mis*spell", "mispell")); + ASSERT_FALSE(wildcardMatch("mis*spellx", "mispellx")); ASSERT_FALSE(wildcardMatch("f?*", "boo")); } From 276b079c0a415b74705736d382974fd53c6592f2 Mon Sep 17 00:00:00 2001 From: Marco Magdy Date: Tue, 8 Oct 2019 22:19:27 +0000 Subject: [PATCH 07/19] more test cases for wildcard matching Signed-off-by: Marco Magdy --- test/extensions/tracers/xray/util_test.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/extensions/tracers/xray/util_test.cc b/test/extensions/tracers/xray/util_test.cc index e85afd6ed43d..0fef2806855e 100644 --- a/test/extensions/tracers/xray/util_test.cc +++ b/test/extensions/tracers/xray/util_test.cc @@ -60,6 +60,12 @@ TEST(XRayWildcardTest, onlyGlob) { ASSERT_TRUE(wildcardMatch("*", "UPPERCASE")); ASSERT_TRUE(wildcardMatch("*", "miXEDcaSe")); } + +TEST(XRayWildcardTest, LengthAtLeastTwo) { + EXPECT_FALSE(wildcardMatch("??*", "a")); + EXPECT_TRUE(wildcardMatch("??*", "aa")); + EXPECT_TRUE(wildcardMatch("??*", "aaa")); +} } // namespace XRay } // namespace Tracers } // namespace Extensions From 7ad797c8cba01c39bbd13a900df0dcbd3be92e67 Mon Sep 17 00:00:00 2001 From: Marco Magdy Date: Wed, 9 Oct 2019 23:11:44 +0000 Subject: [PATCH 08/19] Use DataSource instead of string filename Signed-off-by: Marco Magdy --- api/envoy/config/trace/v2/trace.proto | 3 ++- api/envoy/config/trace/v3alpha/trace.proto | 3 ++- source/extensions/tracers/xray/config.cc | 5 +++-- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/api/envoy/config/trace/v2/trace.proto b/api/envoy/config/trace/v2/trace.proto index 19854886b50f..357ccf846bec 100644 --- a/api/envoy/config/trace/v2/trace.proto +++ b/api/envoy/config/trace/v2/trace.proto @@ -6,6 +6,7 @@ option java_outer_classname = "TraceProto"; option java_multiple_files = true; option java_package = "io.envoyproxy.envoy.config.trace.v2"; +import "envoy/api/v2/core/base.proto"; import "envoy/api/v2/core/grpc_service.proto"; import "google/protobuf/any.proto"; @@ -209,7 +210,7 @@ message XRayConfig { string segment_name = 2; // The location of custom sampling rule json file. - string json_file = 3; + api.v2.core.DataSource sampling_rule_manifest = 3; } // Configuration structure. diff --git a/api/envoy/config/trace/v3alpha/trace.proto b/api/envoy/config/trace/v3alpha/trace.proto index 42af6c2b702a..096b0c52fa3f 100644 --- a/api/envoy/config/trace/v3alpha/trace.proto +++ b/api/envoy/config/trace/v3alpha/trace.proto @@ -6,6 +6,7 @@ option java_outer_classname = "TraceProto"; option java_multiple_files = true; option java_package = "io.envoyproxy.envoy.config.trace.v3alpha"; +import "envoy/api/v3alpha/core/base.proto"; import "envoy/api/v3alpha/core/grpc_service.proto"; import "google/protobuf/any.proto"; @@ -209,7 +210,7 @@ message XRayConfig { string segment_name = 2; // The location of custom sampling rule json file. - string json_file = 3; + api.v3alpha.core.DataSource sampling_rule_manifest = 3; } // Configuration structure. diff --git a/source/extensions/tracers/xray/config.cc b/source/extensions/tracers/xray/config.cc index 34ea92f723de..21e8dc1e4db0 100644 --- a/source/extensions/tracers/xray/config.cc +++ b/source/extensions/tracers/xray/config.cc @@ -22,10 +22,11 @@ XRayTracerFactory::createHttpTracerTyped(const envoy::config::trace::v2::XRayCon Server::Instance& server) { std::string sampling_rules_json; try { - sampling_rules_json = server.api().fileSystem().fileReadToEnd(proto_config.json_file()); + sampling_rules_json = + server.api().fileSystem().fileReadToEnd(proto_config.sampling_rule_manifest().filename()); } catch (EnvoyException& e) { ENVOY_LOG(error, "Could not read custom sampling rule json file: {}, because of {}.", - proto_config.json_file(), e.what()); + proto_config.sampling_rule_manifest().filename(), e.what()); } XRayConfiguration xconfig(proto_config.daemon_endpoint(), proto_config.segment_name(), From ab92985ca93772b7913bf994471f94559d534bfc Mon Sep 17 00:00:00 2001 From: Marco Magdy Date: Thu, 10 Oct 2019 21:13:01 +0000 Subject: [PATCH 09/19] Add tests for config loading Signed-off-by: Marco Magdy --- test/extensions/tracers/xray/BUILD | 12 +++ test/extensions/tracers/xray/config_test.cc | 79 +++++++++++++++++++ test/extensions/tracers/zipkin/config_test.cc | 2 + 3 files changed, 93 insertions(+) create mode 100644 test/extensions/tracers/xray/config_test.cc diff --git a/test/extensions/tracers/xray/BUILD b/test/extensions/tracers/xray/BUILD index ca373d08d328..96f4436584e5 100644 --- a/test/extensions/tracers/xray/BUILD +++ b/test/extensions/tracers/xray/BUILD @@ -22,3 +22,15 @@ envoy_extension_cc_test( "//source/extensions/tracers/xray:xray_lib", ], ) + +envoy_extension_cc_test( + name = "config_test", + srcs = ["config_test.cc"], + extension_name = "envoy.tracers.xray", + deps = [ + "//source/extensions/tracers/xray:config", + "//test/mocks/server:server_mocks", + "//test/test_common:environment_lib", + "//test/test_common:utility_lib", + ], +) diff --git a/test/extensions/tracers/xray/config_test.cc b/test/extensions/tracers/xray/config_test.cc new file mode 100644 index 000000000000..27a57485f649 --- /dev/null +++ b/test/extensions/tracers/xray/config_test.cc @@ -0,0 +1,79 @@ +#include "envoy/registry/registry.h" + +#include "extensions/tracers/xray/config.h" + +#include "test/mocks/server/mocks.h" +#include "test/test_common/environment.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +using ::testing::Eq; +using ::testing::Throw; + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace XRay { +namespace { + +TEST(XRayTracerConfigTest, XRayHttpTracerWithTypedConfig) { + NiceMock server; + + const std::string yaml_string = R"EOF( + http: + name: envoy.tracers.xray + typed_config: + "@type": type.googleapis.com/envoy.config.trace.v2.XRayConfig + daemon_endpoint: 127.0.0.1 + segment_name: AwsAppMesh + sampling_rule_manifest: + filename: "rules.json")EOF"; + + envoy::config::trace::v2::Tracing configuration; + TestUtility::loadFromYaml(yaml_string, configuration); + + XRayTracerFactory factory; + auto message = Config::Utility::translateToFactoryConfig( + configuration.http(), ProtobufMessage::getStrictValidationVisitor(), factory); + Tracing::HttpTracerPtr xray_tracer = factory.createHttpTracer(*message, server); + ASSERT_NE(nullptr, xray_tracer); +} + +TEST(XRayTracerConfigTest, XRayHttpTracerWithInvalidFileName) { + NiceMock server; + NiceMock api; + NiceMock file_system; + + // fake invalid file + EXPECT_CALL(file_system, fileReadToEnd("rules.json")) + .WillRepeatedly(Throw(EnvoyException("failed to open file."))); + EXPECT_CALL(api, fileSystem()).WillRepeatedly(ReturnRef(file_system)); + EXPECT_CALL(server, api()).WillRepeatedly(ReturnRef(api)); + + const std::string yaml_string = R"EOF( + http: + name: envoy.tracers.xray + typed_config: + "@type": type.googleapis.com/envoy.config.trace.v2.XRayConfig + daemon_endpoint: 127.0.0.1 + segment_name: AwsAppMesh + sampling_rule_manifest: + filename: "rules.json")EOF"; + + envoy::config::trace::v2::Tracing configuration; + TestUtility::loadFromYaml(yaml_string, configuration); + + XRayTracerFactory factory; + auto message = Config::Utility::translateToFactoryConfig( + configuration.http(), ProtobufMessage::getStrictValidationVisitor(), factory); + + Tracing::HttpTracerPtr xray_tracer = factory.createHttpTracer(*message, server); + ASSERT_NE(nullptr, xray_tracer); +} + +} // namespace +} // namespace XRay +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/test/extensions/tracers/zipkin/config_test.cc b/test/extensions/tracers/zipkin/config_test.cc index 14841b034e17..750596655291 100644 --- a/test/extensions/tracers/zipkin/config_test.cc +++ b/test/extensions/tracers/zipkin/config_test.cc @@ -17,6 +17,7 @@ namespace { TEST(ZipkinTracerConfigTest, ZipkinHttpTracer) { NiceMock server; + EXPECT_CALL(server.cluster_manager_, get(Eq("fake_cluster"))) .WillRepeatedly(Return(&server.cluster_manager_.thread_local_cluster_)); @@ -41,6 +42,7 @@ TEST(ZipkinTracerConfigTest, ZipkinHttpTracer) { TEST(ZipkinTracerConfigTest, ZipkinHttpTracerWithTypedConfig) { NiceMock server; + EXPECT_CALL(server.cluster_manager_, get(Eq("fake_cluster"))) .WillRepeatedly(Return(&server.cluster_manager_.thread_local_cluster_)); From 553166a38cbcf9211eca6bd08771cc16c6493c22 Mon Sep 17 00:00:00 2001 From: Marco Magdy Date: Thu, 10 Oct 2019 23:15:41 +0000 Subject: [PATCH 10/19] Address feedback Signed-off-by: Marco Magdy --- api/envoy/config/trace/v2/trace.proto | 1 + .../extensions/tracers/xray/sampling_strategy.h | 2 +- source/extensions/tracers/xray/tracer.h | 2 +- source/extensions/tracers/xray/util.cc | 12 ++++++------ source/extensions/tracers/xray/util.h | 1 + test/extensions/tracers/xray/util_test.cc | 16 ++++++++-------- 6 files changed, 18 insertions(+), 16 deletions(-) diff --git a/api/envoy/config/trace/v2/trace.proto b/api/envoy/config/trace/v2/trace.proto index 357ccf846bec..2e0703ab6554 100644 --- a/api/envoy/config/trace/v2/trace.proto +++ b/api/envoy/config/trace/v2/trace.proto @@ -200,6 +200,7 @@ message OpenCensusConfig { repeated TraceContext outgoing_trace_context = 9; } +// [#not-implemented-hide:] // Configuration for AWS X-Ray tracer. message XRayConfig { // The endpoint of the X-Ray Daemon where the spans will be sent. Since by default daemon diff --git a/source/extensions/tracers/xray/sampling_strategy.h b/source/extensions/tracers/xray/sampling_strategy.h index 814577a620dd..0fad22c39dce 100644 --- a/source/extensions/tracers/xray/sampling_strategy.h +++ b/source/extensions/tracers/xray/sampling_strategy.h @@ -43,7 +43,7 @@ class SamplingStrategy { * sampleRequest determines if the given request should be traced or not. */ virtual bool sampleRequest(const SamplingRequest& sampling_request) { - (void)sampling_request; // unused for now + UNREFERENCED_PARAMETER(sampling_request); // unused for now return rng_() % 100 == 42; } diff --git a/source/extensions/tracers/xray/tracer.h b/source/extensions/tracers/xray/tracer.h index f2ca3f0fded4..b201b858a558 100644 --- a/source/extensions/tracers/xray/tracer.h +++ b/source/extensions/tracers/xray/tracer.h @@ -18,7 +18,7 @@ class Tracer { public: Tracer(absl::string_view segment_name, TimeSource& time_source) : segment_name_(segment_name), time_source_(time_source) { - (void)time_source_; + UNREFERENCED_PARAMETER(time_source_); } /** diff --git a/source/extensions/tracers/xray/util.cc b/source/extensions/tracers/xray/util.cc index 960bcfc69715..870d8af326aa 100644 --- a/source/extensions/tracers/xray/util.cc +++ b/source/extensions/tracers/xray/util.cc @@ -19,7 +19,7 @@ bool wildcardMatch(absl::string_view pattern, absl::string_view input) { return true; } - size_t i = 0, p = 0, iStar = input.size(), pStar = 0; + size_t i = 0, p = 0, i_star = input.size(), p_star = 0; while (i < input.size()) { if (p < pattern.size() && absl::ascii_tolower(input[i]) == absl::ascii_tolower(pattern[p])) { ++i; @@ -28,11 +28,11 @@ bool wildcardMatch(absl::string_view pattern, absl::string_view input) { ++i; ++p; } else if (p < pattern.size() && pattern[p] == glob) { - iStar = i; - pStar = p++; - } else if (iStar != input.size()) { - i = ++iStar; - p = pStar + 1; + i_star = i; + p_star = p++; + } else if (i_star != input.size()) { + i = ++i_star; + p = p_star + 1; } else { return false; } diff --git a/source/extensions/tracers/xray/util.h b/source/extensions/tracers/xray/util.h index 76a3ec5fc75e..be2559bdd502 100644 --- a/source/extensions/tracers/xray/util.h +++ b/source/extensions/tracers/xray/util.h @@ -20,6 +20,7 @@ namespace XRay { * @return whether the text matches the pattern. */ bool wildcardMatch(absl::string_view pattern, absl::string_view input); + } // namespace XRay } // namespace Tracers } // namespace Extensions diff --git a/test/extensions/tracers/xray/util_test.cc b/test/extensions/tracers/xray/util_test.cc index 0fef2806855e..e78be430d0bb 100644 --- a/test/extensions/tracers/xray/util_test.cc +++ b/test/extensions/tracers/xray/util_test.cc @@ -7,38 +7,38 @@ namespace Extensions { namespace Tracers { namespace XRay { -TEST(XRayWildcardTest, matchingEmpty) { +TEST(XRayWildcardTest, MatchingEmpty) { ASSERT_TRUE(wildcardMatch("", "")); ASSERT_FALSE(wildcardMatch("", "42")); ASSERT_TRUE(wildcardMatch("*", "")); ASSERT_FALSE(wildcardMatch("?", "")); } -TEST(XRayWildcardTest, matchIdentityCaseInsensitive) { +TEST(XRayWildcardTest, MatchIdentityCaseInsensitive) { ASSERT_TRUE(wildcardMatch("foo", "foo")); ASSERT_TRUE(wildcardMatch("foo", "FOO")); ASSERT_TRUE(wildcardMatch("foo", "Foo")); ASSERT_TRUE(wildcardMatch("6543210", "6543210")); } -TEST(XRayWildcardTest, matchIdentityExtra) { +TEST(XRayWildcardTest, MatchIdentityExtra) { ASSERT_FALSE(wildcardMatch("foo", "foob")); ASSERT_FALSE(wildcardMatch("foo", "xfoo")); ASSERT_FALSE(wildcardMatch("foo", "bar")); } -TEST(XRayWildcardTest, singleWildcard) { +TEST(XRayWildcardTest, SingleWildcard) { ASSERT_FALSE(wildcardMatch("f?o", "boo")); ASSERT_TRUE(wildcardMatch("fo?", "foo")); } -TEST(XRayWildcardTest, multipleWildcards) { +TEST(XRayWildcardTest, MultipleWildcards) { ASSERT_FALSE(wildcardMatch("f??", "boo")); ASSERT_TRUE(wildcardMatch("he??o", "Hello")); ASSERT_TRUE(wildcardMatch("?o?", "foo")); } -TEST(XRayWildcardTest, globMatch) { +TEST(XRayWildcardTest, GlobMatch) { ASSERT_TRUE(wildcardMatch("f?o*ba*", "foobazbar")); ASSERT_TRUE(wildcardMatch("*oo", "foo")); ASSERT_TRUE(wildcardMatch("*o?", "foo")); @@ -46,14 +46,14 @@ TEST(XRayWildcardTest, globMatch) { ASSERT_TRUE(wildcardMatch("mis*spell", "misspell")); } -TEST(XRayWildcardTest, globMismatch) { +TEST(XRayWildcardTest, GlobMismatch) { ASSERT_FALSE(wildcardMatch("foo*", "fo0")); ASSERT_FALSE(wildcardMatch("fo*obar", "foobaz")); ASSERT_FALSE(wildcardMatch("mis*spellx", "mispellx")); ASSERT_FALSE(wildcardMatch("f?*", "boo")); } -TEST(XRayWildcardTest, onlyGlob) { +TEST(XRayWildcardTest, OnlyGlob) { ASSERT_TRUE(wildcardMatch("*", "foo")); ASSERT_TRUE(wildcardMatch("*", "anything")); ASSERT_TRUE(wildcardMatch("*", "12354")); From 49323b5b9b4de982d0aaabc3aee9bd505f1fc91b Mon Sep 17 00:00:00 2001 From: Marco Magdy Date: Fri, 11 Oct 2019 15:31:35 +0000 Subject: [PATCH 11/19] clang tidy stuff Signed-off-by: Marco Magdy --- api/envoy/config/trace/v3alpha/trace.proto | 1 + source/extensions/tracers/xray/sampling_strategy.h | 1 + test/extensions/tracers/xray/config_test.cc | 1 - 3 files changed, 2 insertions(+), 1 deletion(-) diff --git a/api/envoy/config/trace/v3alpha/trace.proto b/api/envoy/config/trace/v3alpha/trace.proto index 096b0c52fa3f..dcc7f099971b 100644 --- a/api/envoy/config/trace/v3alpha/trace.proto +++ b/api/envoy/config/trace/v3alpha/trace.proto @@ -200,6 +200,7 @@ message OpenCensusConfig { repeated TraceContext outgoing_trace_context = 9; } +// [#not-implemented-hide:] // Configuration for AWS X-Ray tracer. message XRayConfig { // The endpoint of the X-Ray Daemon where the spans will be sent. Since by default daemon diff --git a/source/extensions/tracers/xray/sampling_strategy.h b/source/extensions/tracers/xray/sampling_strategy.h index 0fad22c39dce..dc0f4c5e5d6e 100644 --- a/source/extensions/tracers/xray/sampling_strategy.h +++ b/source/extensions/tracers/xray/sampling_strategy.h @@ -3,6 +3,7 @@ #include #include +#include "envoy/common/macros.h" #include "envoy/common/pure.h" #include "absl/strings/string_view.h" diff --git a/test/extensions/tracers/xray/config_test.cc b/test/extensions/tracers/xray/config_test.cc index 27a57485f649..91dcd5903a0d 100644 --- a/test/extensions/tracers/xray/config_test.cc +++ b/test/extensions/tracers/xray/config_test.cc @@ -8,7 +8,6 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" -using ::testing::Eq; using ::testing::Throw; namespace Envoy { From f21aa1c7362356028b8ab666eb8799ac94dd0d48 Mon Sep 17 00:00:00 2001 From: Marco Magdy Date: Fri, 11 Oct 2019 15:59:13 +0000 Subject: [PATCH 12/19] crossing my fingers Signed-off-by: Marco Magdy --- source/extensions/tracers/xray/BUILD | 1 + source/extensions/tracers/xray/sampling_strategy.h | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/source/extensions/tracers/xray/BUILD b/source/extensions/tracers/xray/BUILD index 192d382e972a..b581dfa8383e 100644 --- a/source/extensions/tracers/xray/BUILD +++ b/source/extensions/tracers/xray/BUILD @@ -27,6 +27,7 @@ envoy_cc_library( "//include/envoy/common:time_interface", "//include/envoy/server:instance_interface", "//include/envoy/tracing:http_tracer_interface", + "//source/common/common:macros", "//source/common/http:header_map_lib", "//source/common/json:json_loader_lib", "//source/common/tracing:http_tracer_lib", diff --git a/source/extensions/tracers/xray/sampling_strategy.h b/source/extensions/tracers/xray/sampling_strategy.h index dc0f4c5e5d6e..5fed69492e63 100644 --- a/source/extensions/tracers/xray/sampling_strategy.h +++ b/source/extensions/tracers/xray/sampling_strategy.h @@ -3,9 +3,10 @@ #include #include -#include "envoy/common/macros.h" #include "envoy/common/pure.h" +#include "common/common/macros.h" + #include "absl/strings/string_view.h" namespace Envoy { From d4242f5d30e76a738ea4e81be3ffb32ac3044bde Mon Sep 17 00:00:00 2001 From: Marco Magdy Date: Fri, 11 Oct 2019 16:55:27 +0000 Subject: [PATCH 13/19] Docs - comment out references to xray Signed-off-by: Marco Magdy --- api/envoy/config/trace/v2/trace.proto | 3 ++- api/envoy/config/trace/v3alpha/trace.proto | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/api/envoy/config/trace/v2/trace.proto b/api/envoy/config/trace/v2/trace.proto index 2e0703ab6554..01b29c173838 100644 --- a/api/envoy/config/trace/v2/trace.proto +++ b/api/envoy/config/trace/v2/trace.proto @@ -46,7 +46,8 @@ message Tracing { // - :ref:`DynamicOtConfig ` // - :ref:`DatadogConfig ` // - :ref:`OpenCensusConfig ` - // - :ref:`XRayConfig ` + // [#comment: TODO(marco) when XRay is implemented, uncomment the following; - :ref:`XRayConfig + // `] oneof config_type { google.protobuf.Struct config = 2; diff --git a/api/envoy/config/trace/v3alpha/trace.proto b/api/envoy/config/trace/v3alpha/trace.proto index dcc7f099971b..37de63439968 100644 --- a/api/envoy/config/trace/v3alpha/trace.proto +++ b/api/envoy/config/trace/v3alpha/trace.proto @@ -46,7 +46,8 @@ message Tracing { // - :ref:`DynamicOtConfig ` // - :ref:`DatadogConfig ` // - :ref:`OpenCensusConfig ` - // - :ref:`XRayConfig ` + // [#comment: TODO(marco) when XRay is implemented, uncomment the following; - :ref:`XRayConfig + // `] oneof config_type { google.protobuf.Struct config = 2; From e87a0e0ef06eccb8164a6dbf7925dbf7517ef30c Mon Sep 17 00:00:00 2001 From: Marco Magdy Date: Mon, 14 Oct 2019 11:28:51 -0700 Subject: [PATCH 14/19] Fuzz test the wildcard matcher Signed-off-by: Marco Magdy --- source/extensions/tracers/xray/util.cc | 1 - test/extensions/tracers/xray/BUILD | 10 +++++++++ test/extensions/tracers/xray/fuzz_test.cc | 22 +++++++++++++++++++ .../xray/wildcard_matcher_corpus/example | 1 + 4 files changed, 33 insertions(+), 1 deletion(-) create mode 100644 test/extensions/tracers/xray/fuzz_test.cc create mode 100644 test/extensions/tracers/xray/wildcard_matcher_corpus/example diff --git a/source/extensions/tracers/xray/util.cc b/source/extensions/tracers/xray/util.cc index 870d8af326aa..f3310255c7bc 100644 --- a/source/extensions/tracers/xray/util.cc +++ b/source/extensions/tracers/xray/util.cc @@ -7,7 +7,6 @@ namespace Extensions { namespace Tracers { namespace XRay { -// TODO(magmarco): fuzz test this function bool wildcardMatch(absl::string_view pattern, absl::string_view input) { if (pattern.empty()) { return input.empty(); diff --git a/test/extensions/tracers/xray/BUILD b/test/extensions/tracers/xray/BUILD index 96f4436584e5..b4ae1dbd65d6 100644 --- a/test/extensions/tracers/xray/BUILD +++ b/test/extensions/tracers/xray/BUILD @@ -2,6 +2,7 @@ licenses(["notice"]) # Apache 2 load( "//bazel:envoy_build_system.bzl", + "envoy_cc_fuzz_test", "envoy_cc_test_library", "envoy_package", ) @@ -34,3 +35,12 @@ envoy_extension_cc_test( "//test/test_common:utility_lib", ], ) + +envoy_cc_fuzz_test( + name = "xray_fuzz_test", + srcs = ["fuzz_test.cc"], + corpus = "wildcard_matcher_corpus", + deps = [ + "//source/extensions/tracers/xray:xray_lib", + ], +) diff --git a/test/extensions/tracers/xray/fuzz_test.cc b/test/extensions/tracers/xray/fuzz_test.cc new file mode 100644 index 000000000000..7d0103665f1d --- /dev/null +++ b/test/extensions/tracers/xray/fuzz_test.cc @@ -0,0 +1,22 @@ +#include "extensions/tracers/xray/util.h" + +#include "test/fuzz/fuzz_runner.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace XRay { + +DEFINE_FUZZER(const uint8_t* buf, size_t len) { + absl::string_view sv(reinterpret_cast(buf), len); + wildcardMatch("*", sv); + wildcardMatch("?", sv); + wildcardMatch("*??", sv); + wildcardMatch("??*", sv); + wildcardMatch(sv, "hello, world"); +} + +} // namespace XRay +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/test/extensions/tracers/xray/wildcard_matcher_corpus/example b/test/extensions/tracers/xray/wildcard_matcher_corpus/example new file mode 100644 index 000000000000..1de83e62599d --- /dev/null +++ b/test/extensions/tracers/xray/wildcard_matcher_corpus/example @@ -0,0 +1 @@ +/match/on/th?s/*/route* From 4abd18052ade429b57564cdb9a80e104d7fb5523 Mon Sep 17 00:00:00 2001 From: Marco Magdy Date: Mon, 14 Oct 2019 13:32:17 -0700 Subject: [PATCH 15/19] Use DataSource API Signed-off-by: Marco Magdy --- source/extensions/tracers/xray/BUILD | 1 + source/extensions/tracers/xray/config.cc | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/source/extensions/tracers/xray/BUILD b/source/extensions/tracers/xray/BUILD index b581dfa8383e..4306e557f25c 100644 --- a/source/extensions/tracers/xray/BUILD +++ b/source/extensions/tracers/xray/BUILD @@ -40,6 +40,7 @@ envoy_cc_library( hdrs = ["config.h"], deps = [ ":xray_lib", + "//source/common/config:datasource_lib", "//source/extensions/tracers:well_known_names", "//source/extensions/tracers/common:factory_base_lib", ], diff --git a/source/extensions/tracers/xray/config.cc b/source/extensions/tracers/xray/config.cc index 21e8dc1e4db0..d1d9be528aba 100644 --- a/source/extensions/tracers/xray/config.cc +++ b/source/extensions/tracers/xray/config.cc @@ -5,6 +5,7 @@ #include "envoy/registry/registry.h" #include "common/common/utility.h" +#include "common/config/datasource.h" #include "common/tracing/http_tracer_impl.h" #include "extensions/tracers/well_known_names.h" @@ -23,7 +24,7 @@ XRayTracerFactory::createHttpTracerTyped(const envoy::config::trace::v2::XRayCon std::string sampling_rules_json; try { sampling_rules_json = - server.api().fileSystem().fileReadToEnd(proto_config.sampling_rule_manifest().filename()); + Config::DataSource::read(proto_config.sampling_rule_manifest(), true, server.api()); } catch (EnvoyException& e) { ENVOY_LOG(error, "Could not read custom sampling rule json file: {}, because of {}.", proto_config.sampling_rule_manifest().filename(), e.what()); From c081d68c99be15e29b79f5b02a2c59ee405ba207 Mon Sep 17 00:00:00 2001 From: Marco Magdy Date: Mon, 14 Oct 2019 15:15:14 -0700 Subject: [PATCH 16/19] Updated error message Signed-off-by: Marco Magdy --- source/extensions/tracers/xray/config.cc | 3 +-- source/extensions/tracers/xray/xray_tracer_impl.cc | 10 ++++++---- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/source/extensions/tracers/xray/config.cc b/source/extensions/tracers/xray/config.cc index d1d9be528aba..e19705bd9b8a 100644 --- a/source/extensions/tracers/xray/config.cc +++ b/source/extensions/tracers/xray/config.cc @@ -26,8 +26,7 @@ XRayTracerFactory::createHttpTracerTyped(const envoy::config::trace::v2::XRayCon sampling_rules_json = Config::DataSource::read(proto_config.sampling_rule_manifest(), true, server.api()); } catch (EnvoyException& e) { - ENVOY_LOG(error, "Could not read custom sampling rule json file: {}, because of {}.", - proto_config.sampling_rule_manifest().filename(), e.what()); + ENVOY_LOG(error, "Failed to read sampling rules manifest because of {}.", e.what()); } XRayConfiguration xconfig(proto_config.daemon_endpoint(), proto_config.segment_name(), diff --git a/source/extensions/tracers/xray/xray_tracer_impl.cc b/source/extensions/tracers/xray/xray_tracer_impl.cc index 431139b1ee63..841eb1d0428d 100644 --- a/source/extensions/tracers/xray/xray_tracer_impl.cc +++ b/source/extensions/tracers/xray/xray_tracer_impl.cc @@ -1,5 +1,7 @@ #include "extensions/tracers/xray/xray_tracer_impl.h" +#include "common/common/macros.h" + namespace Envoy { namespace Extensions { namespace Tracers { @@ -23,10 +25,10 @@ Tracing::SpanPtr Driver::startSpan(const Tracing::Config& config, Http::HeaderMa const std::string& operation_name, SystemTime start_time, const Tracing::Decision tracing_decision) { - (void)config; - (void)operation_name; - (void)start_time; - (void)tracing_decision; + UNREFERENCED_PARAMETER(config); + UNREFERENCED_PARAMETER(operation_name); + UNREFERENCED_PARAMETER(start_time); + UNREFERENCED_PARAMETER(tracing_decision); const SamplingRequest request{request_headers.Host()->value().getStringView(), request_headers.Method()->value().getStringView(), request_headers.Path()->value().getStringView()}; From 2a5f7e2072a56500c4cf5e644e146d43ed08114d Mon Sep 17 00:00:00 2001 From: Marco Magdy Date: Tue, 15 Oct 2019 15:17:25 -0700 Subject: [PATCH 17/19] Added comments to the wildcard matching function Signed-off-by: Marco Magdy --- source/extensions/tracers/xray/util.cc | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/source/extensions/tracers/xray/util.cc b/source/extensions/tracers/xray/util.cc index f3310255c7bc..65b052d48300 100644 --- a/source/extensions/tracers/xray/util.cc +++ b/source/extensions/tracers/xray/util.cc @@ -7,6 +7,22 @@ namespace Extensions { namespace Tracers { namespace XRay { +// AWS X-Ray has two sources of sampling rules (wildcard patterns): +// +// 1- The manifest file (static config) +// 2- The X-Ray Service - periodically polled for new rules. (dynamic config) +// +// X-Ray will inspect every single request and try to match it against the set of rules it has to +// decide whether or not a given request should be sampled. That means this wildcard matching +// routine is on the hot path. I've spent a great deal of time to make this function optimal and not +// allocate. +// Using regex matching here has many downsides and it would require us to: +// 1- escape/lint the user input pattern to avoid messing up its meaning (think '.' character is a +// valid regex and URL character) 2- compile the regex and store it with every corresponding part of +// the rule +// +// Those two steps would add significant overhead to the tracer. Meanwhile, the following function +// has a comprehensive test suite and fuzz tests. bool wildcardMatch(absl::string_view pattern, absl::string_view input) { if (pattern.empty()) { return input.empty(); From b16f0fb541d0d34df9730a59d6f439e8dfc6ec0c Mon Sep 17 00:00:00 2001 From: Marco Magdy Date: Wed, 16 Oct 2019 16:41:16 -0700 Subject: [PATCH 18/19] more tests Signed-off-by: Marco Magdy --- test/extensions/tracers/xray/fuzz_test.cc | 17 +++++++++++------ test/extensions/tracers/xray/util_test.cc | 1 + 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/test/extensions/tracers/xray/fuzz_test.cc b/test/extensions/tracers/xray/fuzz_test.cc index 7d0103665f1d..0e39d1ae6b07 100644 --- a/test/extensions/tracers/xray/fuzz_test.cc +++ b/test/extensions/tracers/xray/fuzz_test.cc @@ -8,12 +8,17 @@ namespace Tracers { namespace XRay { DEFINE_FUZZER(const uint8_t* buf, size_t len) { - absl::string_view sv(reinterpret_cast(buf), len); - wildcardMatch("*", sv); - wildcardMatch("?", sv); - wildcardMatch("*??", sv); - wildcardMatch("??*", sv); - wildcardMatch(sv, "hello, world"); + absl::string_view pattern, input; + if (len > 1) { + pattern = absl::string_view(reinterpret_cast(buf), len / 2); + input = absl::string_view(reinterpret_cast(buf + len / 2), len - len / 2); + wildcardMatch(pattern, input); + } else { // buf is a single byte, use it for both pattern and input + absl::string_view sv(reinterpret_cast(buf), len); + wildcardMatch(sv, "hello"); + wildcardMatch("*", sv); + wildcardMatch("?", sv); + } } } // namespace XRay diff --git a/test/extensions/tracers/xray/util_test.cc b/test/extensions/tracers/xray/util_test.cc index e78be430d0bb..9a5622dd4653 100644 --- a/test/extensions/tracers/xray/util_test.cc +++ b/test/extensions/tracers/xray/util_test.cc @@ -59,6 +59,7 @@ TEST(XRayWildcardTest, OnlyGlob) { ASSERT_TRUE(wildcardMatch("*", "12354")); ASSERT_TRUE(wildcardMatch("*", "UPPERCASE")); ASSERT_TRUE(wildcardMatch("*", "miXEDcaSe")); + ASSERT_TRUE(wildcardMatch("*******", "Envoy")); } TEST(XRayWildcardTest, LengthAtLeastTwo) { From 5b9cb8993d5a14c9f9a47bd74831a5f633698a4e Mon Sep 17 00:00:00 2001 From: Marco Magdy Date: Wed, 16 Oct 2019 16:46:36 -0700 Subject: [PATCH 19/19] Added TODO Signed-off-by: Marco Magdy --- test/extensions/tracers/xray/fuzz_test.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/extensions/tracers/xray/fuzz_test.cc b/test/extensions/tracers/xray/fuzz_test.cc index 0e39d1ae6b07..cbec8697ea8b 100644 --- a/test/extensions/tracers/xray/fuzz_test.cc +++ b/test/extensions/tracers/xray/fuzz_test.cc @@ -7,6 +7,8 @@ namespace Extensions { namespace Tracers { namespace XRay { +// TODO(@marcomagdy): @htuch suggests to compare results with re2 (after replacing * with .* and ? +// with '.' and doing proper regex escaping) DEFINE_FUZZER(const uint8_t* buf, size_t len) { absl::string_view pattern, input; if (len > 1) {