From d9e4446c9ddf9cd46765721854e27b0c1342c34b Mon Sep 17 00:00:00 2001 From: Joe Kralicky Date: Wed, 8 Jan 2025 03:32:02 +0000 Subject: [PATCH 1/5] add pomerium_otel tracer extension --- BUILD | 1 + .../early_header_mutation/trace_context/BUILD | 1 + .../trace_context/trace_context.cc | 25 ++++++-- .../trace_context/trace_context.h | 1 + source/extensions/tracers/pomerium_otel/BUILD | 47 ++++++++++++++ .../tracers/pomerium_otel/config.cc | 33 ++++++++++ .../extensions/tracers/pomerium_otel/config.h | 37 +++++++++++ .../tracers/pomerium_otel/pomerium_otel.proto | 19 ++++++ .../extensions/tracers/pomerium_otel/span.cc | 50 +++++++++++++++ .../extensions/tracers/pomerium_otel/span.h | 40 ++++++++++++ .../tracers/pomerium_otel/tracer_impl.cc | 64 +++++++++++++++++++ .../tracers/pomerium_otel/tracer_impl.h | 30 +++++++++ .../tracers/pomerium_otel/typeutils.h | 29 +++++++++ 13 files changed, 373 insertions(+), 4 deletions(-) create mode 100644 source/extensions/tracers/pomerium_otel/BUILD create mode 100644 source/extensions/tracers/pomerium_otel/config.cc create mode 100644 source/extensions/tracers/pomerium_otel/config.h create mode 100644 source/extensions/tracers/pomerium_otel/pomerium_otel.proto create mode 100644 source/extensions/tracers/pomerium_otel/span.cc create mode 100644 source/extensions/tracers/pomerium_otel/span.h create mode 100644 source/extensions/tracers/pomerium_otel/tracer_impl.cc create mode 100644 source/extensions/tracers/pomerium_otel/tracer_impl.h create mode 100644 source/extensions/tracers/pomerium_otel/typeutils.h diff --git a/BUILD b/BUILD index 059984e..e2d4cd9 100644 --- a/BUILD +++ b/BUILD @@ -13,6 +13,7 @@ envoy_cc_binary( deps = [ "//source/extensions/http/early_header_mutation/trace_context:pomerium_trace_context", "//source/extensions/request_id/uuidx:pomerium_uuidx", + "//source/extensions/tracers/pomerium_otel", "@envoy//source/exe:envoy_main_entry_lib", ], ) diff --git a/source/extensions/http/early_header_mutation/trace_context/BUILD b/source/extensions/http/early_header_mutation/trace_context/BUILD index b43849f..9f333ab 100644 --- a/source/extensions/http/early_header_mutation/trace_context/BUILD +++ b/source/extensions/http/early_header_mutation/trace_context/BUILD @@ -23,6 +23,7 @@ envoy_cc_library( ":trace_context_cc_proto", "@envoy//envoy/http:early_header_mutation_interface", "@envoy//envoy/registry", + "@envoy//source/common/common:base64_lib", "@envoy//source/common/common:logger_lib", "@envoy//source/common/http:header_mutation_lib", ], diff --git a/source/extensions/http/early_header_mutation/trace_context/trace_context.cc b/source/extensions/http/early_header_mutation/trace_context/trace_context.cc index bea8fb7..8aecb6c 100644 --- a/source/extensions/http/early_header_mutation/trace_context/trace_context.cc +++ b/source/extensions/http/early_header_mutation/trace_context/trace_context.cc @@ -1,5 +1,6 @@ #include "source/extensions/http/early_header_mutation/trace_context/trace_context.h" #include "source/common/common/logger.h" +#include "source/common/common/base64.h" namespace Envoy::Extensions::Http::EarlyHeaderMutation { @@ -37,7 +38,27 @@ bool TraceContext::mutate(Envoy::Http::RequestHeaderMap& headers, if (auto traceparent = values[0]->value().getStringView(); traceparent.size() == 55) { headers.setCopy(pomerium_external_parent_header, traceparent.substr(36, 16)); } + } else if (headers.getPathValue().starts_with("/oauth2/callback")) { + // oauth2 callback is a special case, we can't encode the traceparent in the usual way since + // the query parameters are standard and managed by oauth2 clients + const auto state = params.getFirstValue(Envoy::Http::LowerCaseString("state")); + if (state.has_value()) { + const std::string stateDecoded = + Base64Url::decode(StringUtil::removeTrailingCharacters(state.value(), '=')); + const std::vector segments = + absl::StrSplit(stateDecoded, absl::MaxSplits('|', 3)); + if (segments.size() == 4) { + const auto traceidDecoded = Base64Url::decode(segments[2]); + if (traceidDecoded.size() == 17) { // 16 byte trace ID + 1 byte flags + const auto traceID = traceidDecoded.substr(0, 16); + const auto flags = traceidDecoded.at(16); + headers.setCopy(pomerium_traceid_header, absl::BytesToHexString(traceID)); + headers.setCopy(pomerium_sampling_decision_header, (flags & 1) == 1 ? "1" : "0"); + } + } + } } + return true; } @@ -56,12 +77,8 @@ bool TraceContext::mutate(Envoy::Http::RequestHeaderMap& headers, auto flags = absl::HexStringToBytes(flags_hex).front(); if (flags & 1) { // sampled headers.setCopy(pomerium_sampling_decision_header, "1"); - ENVOY_LOG(debug, "pomerium_traceparent={}, forcing trace decision (on)", - pomerium_traceparent.value()); } else { // not sampled headers.setCopy(pomerium_sampling_decision_header, "0"); - ENVOY_LOG(debug, "pomerium_traceparent={}, forcing sampling decision (off)", - pomerium_traceparent.value()); } headers.setCopy(pomerium_traceparent_header, pomerium_traceparent.value()); diff --git a/source/extensions/http/early_header_mutation/trace_context/trace_context.h b/source/extensions/http/early_header_mutation/trace_context/trace_context.h index bcbeeb7..8f8ca89 100644 --- a/source/extensions/http/early_header_mutation/trace_context/trace_context.h +++ b/source/extensions/http/early_header_mutation/trace_context/trace_context.h @@ -20,6 +20,7 @@ class TraceContext : public Envoy::Http::EarlyHeaderMutation, Envoy::Http::LowerCaseString pomerium_sampling_decision_header{"x-pomerium-sampling-decision"}; Envoy::Http::LowerCaseString pomerium_traceparent_header{"x-pomerium-traceparent"}; Envoy::Http::LowerCaseString pomerium_tracestate_header{"x-pomerium-tracestate"}; + Envoy::Http::LowerCaseString pomerium_traceid_header{"x-pomerium-traceid"}; Envoy::Http::LowerCaseString pomerium_external_parent_header{"x-pomerium-external-parent-span"}; Envoy::Http::LowerCaseString traceparent_header{"traceparent"}; }; diff --git a/source/extensions/tracers/pomerium_otel/BUILD b/source/extensions/tracers/pomerium_otel/BUILD new file mode 100644 index 0000000..fac6586 --- /dev/null +++ b/source/extensions/tracers/pomerium_otel/BUILD @@ -0,0 +1,47 @@ +load( + "@envoy//bazel:envoy_build_system.bzl", + "envoy_cc_library", + "envoy_cc_test", +) + +package(default_visibility = ["//visibility:public"]) + +licenses(["notice"]) + +envoy_cc_library( + name = "pomerium_otel", + srcs = [ + "config.cc", + "span.cc", + "tracer_impl.cc", + ], + hdrs = [ + "config.h", + "span.h", + "tracer_impl.h", + "typeutils.h", + ], + repository = "@envoy", + deps = [ + ":pomerium_otel_cc_proto", + "@envoy//source/common/common:logger_lib", + "@envoy//source/common/common:utility_lib", + "@envoy//source/extensions/tracers/opentelemetry:opentelemetry_tracer_lib", + ], +) + +cc_proto_library( + name = "pomerium_otel_cc_proto", + deps = ["pomerium_otel_proto"], +) + +proto_library( + name = "pomerium_otel_proto", + srcs = ["pomerium_otel.proto"], + deps = [ + "@com_envoyproxy_protoc_gen_validate//validate:validate_proto", + "@com_github_cncf_xds//udpa/annotations:pkg", + "@envoy_api//envoy/annotations:pkg", + "@envoy_api//envoy/config/core/v3:pkg", + ], +) diff --git a/source/extensions/tracers/pomerium_otel/config.cc b/source/extensions/tracers/pomerium_otel/config.cc new file mode 100644 index 0000000..66b66f4 --- /dev/null +++ b/source/extensions/tracers/pomerium_otel/config.cc @@ -0,0 +1,33 @@ +#include "source/extensions/tracers/pomerium_otel/config.h" + +#include "envoy/config/trace/v3/opentelemetry.pb.h" +#include "envoy/config/trace/v3/opentelemetry.pb.validate.h" +#include "envoy/registry/registry.h" + +#include "source/common/common/logger.h" +#include "source/extensions/tracers/pomerium_otel/tracer_impl.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace OpenTelemetry { + +PomeriumOpenTelemetryTracerFactory::PomeriumOpenTelemetryTracerFactory() + : FactoryBase("envoy.tracers.pomerium_otel") {} + +Tracing::DriverSharedPtr PomeriumOpenTelemetryTracerFactory::createTracerDriverTyped( + const pomerium::extensions::OpenTelemetryConfig& proto_config, + Server::Configuration::TracerFactoryContext& context) { + envoy::config::trace::v3::OpenTelemetryConfig base = toBaseConfig(proto_config); + return std::make_shared(base, context); +} + +/** + * Static registration for the OpenTelemetry tracer. @see RegisterFactory. + */ +REGISTER_FACTORY(PomeriumOpenTelemetryTracerFactory, Server::Configuration::TracerFactory); + +} // namespace OpenTelemetry +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/tracers/pomerium_otel/config.h b/source/extensions/tracers/pomerium_otel/config.h new file mode 100644 index 0000000..f92f8fc --- /dev/null +++ b/source/extensions/tracers/pomerium_otel/config.h @@ -0,0 +1,37 @@ +#pragma once + +#include + +#include "source/extensions/tracers/pomerium_otel/typeutils.h" + +#include "envoy/config/trace/v3/opentelemetry.pb.h" +#include "envoy/config/trace/v3/opentelemetry.pb.validate.h" +#include "source/extensions/tracers/pomerium_otel/pomerium_otel.pb.h" + +#include "source/extensions/tracers/common/factory_base.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace OpenTelemetry { + +/** + * Config registration for the OpenTelemetry tracer. @see TracerFactory. + */ +class PomeriumOpenTelemetryTracerFactory + : Logger::Loggable, + public Common::FactoryBase { +public: + PomeriumOpenTelemetryTracerFactory(); + +private: + // FactoryBase + Tracing::DriverSharedPtr + createTracerDriverTyped(const pomerium::extensions::OpenTelemetryConfig& proto_config, + Server::Configuration::TracerFactoryContext& context) override; +}; + +} // namespace OpenTelemetry +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/tracers/pomerium_otel/pomerium_otel.proto b/source/extensions/tracers/pomerium_otel/pomerium_otel.proto new file mode 100644 index 0000000..022a718 --- /dev/null +++ b/source/extensions/tracers/pomerium_otel/pomerium_otel.proto @@ -0,0 +1,19 @@ +syntax = "proto3"; + +package pomerium.extensions; + +import "envoy/config/core/v3/extension.proto"; +import "envoy/config/core/v3/grpc_service.proto"; +import "envoy/config/core/v3/http_service.proto"; +import "udpa/annotations/migrate.proto"; +import "udpa/annotations/status.proto"; + +option (udpa.annotations.file_status).package_version_status = ACTIVE; + +message OpenTelemetryConfig { + envoy.config.core.v3.GrpcService grpc_service = 1 [(udpa.annotations.field_migrate).oneof_promotion = "otlp_exporter"]; + envoy.config.core.v3.HttpService http_service = 3 [(udpa.annotations.field_migrate).oneof_promotion = "otlp_exporter"]; + string service_name = 2; + repeated envoy.config.core.v3.TypedExtensionConfig resource_detectors = 4; + envoy.config.core.v3.TypedExtensionConfig sampler = 5; +} diff --git a/source/extensions/tracers/pomerium_otel/span.cc b/source/extensions/tracers/pomerium_otel/span.cc new file mode 100644 index 0000000..3203c07 --- /dev/null +++ b/source/extensions/tracers/pomerium_otel/span.cc @@ -0,0 +1,50 @@ +#include "source/extensions/tracers/pomerium_otel/span.h" + +namespace Envoy::Extensions::Tracers::OpenTelemetry { + +VariableNameSpan::VariableNameSpan(Tracing::SpanPtr&& base, StrSubstitutions substitutions) + : span_(dynamic_cast(base.release())), substitutions_(substitutions) { + RELEASE_ASSERT(!!span_, "bug: span type is not OpenTelemetry::Span*"); +} + +void VariableNameSpan::setTraceId(const absl::string_view& trace_id_hex) { + span_->setTraceId(trace_id_hex); +} + +void VariableNameSpan::setOperation(absl::string_view operation_name) { + span_->setOperation(absl::StrReplaceAll(operation_name, substitutions_)); +} + +void VariableNameSpan::setTag(absl::string_view name, absl::string_view value) { + span_->setTag(name, value); +}; + +void VariableNameSpan::log(SystemTime timestamp, const std::string& event) { + span_->log(timestamp, event); +}; + +void VariableNameSpan::finishSpan() { span_->finishSpan(); } + +void VariableNameSpan::injectContext(Envoy::Tracing::TraceContext& trace_context, + const Tracing::UpstreamContext& upstream) { + span_->injectContext(trace_context, upstream); +} + +Tracing::SpanPtr VariableNameSpan::spawnChild(const Tracing::Config& config, + const std::string& name, SystemTime start_time) { + return span_->spawnChild(config, name, start_time); +} + +void VariableNameSpan::setSampled(bool sampled) { span_->setSampled(sampled); }; + +std::string VariableNameSpan::getBaggage(absl::string_view key) { return span_->getBaggage(key); }; + +void VariableNameSpan::setBaggage(absl::string_view key, absl::string_view value) { + span_->setBaggage(key, value); +}; + +std::string VariableNameSpan::getTraceId() const { return span_->getTraceId(); } + +std::string VariableNameSpan::getSpanId() const { return span_->getSpanId(); } + +} // namespace Envoy::Extensions::Tracers::OpenTelemetry diff --git a/source/extensions/tracers/pomerium_otel/span.h b/source/extensions/tracers/pomerium_otel/span.h new file mode 100644 index 0000000..cec8eb4 --- /dev/null +++ b/source/extensions/tracers/pomerium_otel/span.h @@ -0,0 +1,40 @@ +#pragma once +#include "envoy/tracing/trace_driver.h" +#include "source/extensions/tracers/opentelemetry/opentelemetry_tracer_impl.h" +#include "absl/strings/str_replace.h" +#include "source/common/common/assert.h" + +namespace Envoy::Extensions::Tracers::OpenTelemetry { + +using BaseOtelSpan = ::Envoy::Extensions::Tracers::OpenTelemetry::Span; +using BaseOtelSpanPtr = std::unique_ptr; +using StrSubstitutions = std::vector>; + +class VariableNameSpan : public Tracing::Span { +public: + VariableNameSpan(Tracing::SpanPtr&& base, StrSubstitutions substitutions); + ~VariableNameSpan() override = default; + + void setTraceId(const absl::string_view& trace_id_hex); + + void setOperation(absl::string_view operation_name) override; + + void setTag(absl::string_view name, absl::string_view value) override; + void log(SystemTime timestamp, const std::string& event) override; + void finishSpan() override; + void injectContext(Envoy::Tracing::TraceContext& trace_context, + const Tracing::UpstreamContext& upstream) override; + Tracing::SpanPtr spawnChild(const Tracing::Config& config, const std::string& name, + SystemTime start_time) override; + void setSampled(bool sampled) override; + std::string getBaggage(absl::string_view key) override; + void setBaggage(absl::string_view key, absl::string_view value) override; + std::string getTraceId() const override; + std::string getSpanId() const override; + +private: + BaseOtelSpanPtr span_; + StrSubstitutions substitutions_; +}; + +} // namespace Envoy::Extensions::Tracers::OpenTelemetry \ No newline at end of file diff --git a/source/extensions/tracers/pomerium_otel/tracer_impl.cc b/source/extensions/tracers/pomerium_otel/tracer_impl.cc new file mode 100644 index 0000000..5374cf6 --- /dev/null +++ b/source/extensions/tracers/pomerium_otel/tracer_impl.cc @@ -0,0 +1,64 @@ + +#include "source/extensions/tracers/pomerium_otel/tracer_impl.h" +#include "source/extensions/tracers/pomerium_otel/span.h" +#include "source/common/tracing/trace_context_impl.h" +#include "source/common/common/logger.h" +#include "source/common/http/path_utility.h" +#include "absl/strings/str_replace.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace OpenTelemetry { + +const Tracing::TraceContextHandler& pomeriumTraceParentHeader() { + CONSTRUCT_ON_FIRST_USE(Tracing::TraceContextHandler, "x-pomerium-traceparent"); +} + +const Tracing::TraceContextHandler& pomeriumTraceIDHeader() { + CONSTRUCT_ON_FIRST_USE(Tracing::TraceContextHandler, "x-pomerium-traceid"); +} + +const Tracing::TraceContextHandler& pomeriumSamplingDecisionHeader() { + CONSTRUCT_ON_FIRST_USE(Tracing::TraceContextHandler, "x-pomerium-sampling-decision"); +} + +Tracing::SpanPtr PomeriumDriver::startSpan(const Tracing::Config& config, + Tracing::TraceContext& trace_context, + const StreamInfo::StreamInfo& stream_info, + const std::string& operation_name, + Tracing::Decision tracing_decision) { + + std::vector> substitutions{ + {"${path}", std::string(Envoy::Http::PathUtil::removeQueryAndFragment(trace_context.path()))}, + {"${host}", std::string(trace_context.host())}, + {"${method}", std::string(trace_context.method())}, + {"${protocol}", std::string(trace_context.protocol())}, + }; + auto span = new VariableNameSpan( + Driver::startSpan(config, trace_context, stream_info, operation_name, tracing_decision), + substitutions); + + if (auto tp = pomeriumTraceParentHeader().get(trace_context); + tp.has_value() && tp->size() == 55) { + auto new_trace_id = tp.value().substr(3, 32); + ENVOY_LOG(trace, "rewriting trace ID {} => {}", span->getTraceId(), new_trace_id); + span->setTraceId(new_trace_id); + } else if (auto tid = pomeriumTraceIDHeader().get(trace_context); + tid.has_value() && tid.value().size() == 32) { + auto new_trace_id = tid.value(); + ENVOY_LOG(trace, "rewriting trace ID (alt) {} => {}", span->getTraceId(), new_trace_id); + span->setTraceId(new_trace_id); + } + if (auto decision = pomeriumSamplingDecisionHeader().get(trace_context); decision.has_value()) { + ENVOY_LOG(trace, "forcing sampling decision: {}", decision.value()); + span->setSampled(decision.value() == "1"); + } + + return Tracing::SpanPtr(static_cast(span)); +} + +} // namespace OpenTelemetry +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy \ No newline at end of file diff --git a/source/extensions/tracers/pomerium_otel/tracer_impl.h b/source/extensions/tracers/pomerium_otel/tracer_impl.h new file mode 100644 index 0000000..db889df --- /dev/null +++ b/source/extensions/tracers/pomerium_otel/tracer_impl.h @@ -0,0 +1,30 @@ +#pragma once + +#include "envoy/tracing/trace_driver.h" +#include "source/extensions/tracers/opentelemetry/opentelemetry_tracer_impl.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace OpenTelemetry { + +/** + * OpenTelemetry tracing driver. + */ +class PomeriumDriver : public Driver { +public: + using Driver::Driver; + + // Tracing::Driver + Tracing::SpanPtr startSpan(const Tracing::Config& config, Tracing::TraceContext& trace_context, + const StreamInfo::StreamInfo& stream_info, + const std::string& operation_name, + Tracing::Decision tracing_decision) override; + +private: +}; + +} // namespace OpenTelemetry +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/tracers/pomerium_otel/typeutils.h b/source/extensions/tracers/pomerium_otel/typeutils.h new file mode 100644 index 0000000..0bdd185 --- /dev/null +++ b/source/extensions/tracers/pomerium_otel/typeutils.h @@ -0,0 +1,29 @@ +#pragma once + +#include "envoy/config/trace/v3/opentelemetry.pb.h" +#include "envoy/config/trace/v3/opentelemetry.pb.validate.h" +#include "source/extensions/tracers/pomerium_otel/pomerium_otel.pb.h" + +inline envoy::config::trace::v3::OpenTelemetryConfig +toBaseConfig(const ::pomerium::extensions::OpenTelemetryConfig& proto_config) { + envoy::config::trace::v3::OpenTelemetryConfig base; + if (proto_config.has_grpc_service()) { + *base.mutable_grpc_service() = proto_config.grpc_service(); + } + if (proto_config.has_http_service()) { + *base.mutable_http_service() = proto_config.http_service(); + } + base.set_service_name(proto_config.service_name()); + base.mutable_resource_detectors()->CopyFrom(proto_config.resource_detectors()); + if (proto_config.has_sampler()) { + *base.mutable_sampler() = proto_config.sampler(); + } + return base; +} + +namespace pomerium::extensions { +inline bool Validate(const ::pomerium::extensions::OpenTelemetryConfig& m, + pgv::ValidationMsg* err) { + return Validate(toBaseConfig(m), err); +} +} // namespace pomerium::extensions \ No newline at end of file From 2765bc820714df8038b17f7a13aa583ee56919d0 Mon Sep 17 00:00:00 2001 From: Joe Kralicky Date: Wed, 8 Jan 2025 20:09:02 +0000 Subject: [PATCH 2/5] code cleanup, upstream tracer lib visibility fix --- WORKSPACE | 1 + ...-opentelemetry-tracer-lib-visibility.patch | 10 ++++++ .../tracers/pomerium_otel/config.cc | 21 +++---------- .../extensions/tracers/pomerium_otel/config.h | 19 ++---------- .../extensions/tracers/pomerium_otel/span.cc | 2 ++ .../extensions/tracers/pomerium_otel/span.h | 2 -- .../tracers/pomerium_otel/tracer_impl.cc | 31 ++++++++++--------- .../tracers/pomerium_otel/tracer_impl.h | 14 ++------- .../tracers/pomerium_otel/typeutils.h | 13 ++++---- 9 files changed, 46 insertions(+), 67 deletions(-) create mode 100644 patches/0002-opentelemetry-tracer-lib-visibility.patch diff --git a/WORKSPACE b/WORKSPACE index c307376..aaddeb5 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -12,6 +12,7 @@ http_archive( patch_tool = "patch", patches = [ "//:patches/0001-fix-otel-grpc-trace-exporter.patch", + "//:patches/0002-opentelemetry-tracer-lib-visibility.patch", ], strip_prefix = "envoy-" + envoy_version, url = "https://github.com/envoyproxy/envoy/archive/refs/tags/v" + envoy_version + ".zip", diff --git a/patches/0002-opentelemetry-tracer-lib-visibility.patch b/patches/0002-opentelemetry-tracer-lib-visibility.patch new file mode 100644 index 0000000..81af13c --- /dev/null +++ b/patches/0002-opentelemetry-tracer-lib-visibility.patch @@ -0,0 +1,10 @@ +diff --git a/source/extensions/tracers/opentelemetry/BUILD b/source/extensions/tracers/opentelemetry/BUILD +index 0b6e75abea..0cac8092aa 100644 +--- a/source/extensions/tracers/opentelemetry/BUILD ++++ b/source/extensions/tracers/opentelemetry/BUILD +@@ -24,4 +24,5 @@ envoy_cc_extension( + + envoy_cc_library( ++ visibility = ["//visibility:public"], + name = "opentelemetry_tracer_lib", + srcs = [ \ No newline at end of file diff --git a/source/extensions/tracers/pomerium_otel/config.cc b/source/extensions/tracers/pomerium_otel/config.cc index 66b66f4..d53c2ad 100644 --- a/source/extensions/tracers/pomerium_otel/config.cc +++ b/source/extensions/tracers/pomerium_otel/config.cc @@ -1,16 +1,10 @@ #include "source/extensions/tracers/pomerium_otel/config.h" -#include "envoy/config/trace/v3/opentelemetry.pb.h" -#include "envoy/config/trace/v3/opentelemetry.pb.validate.h" #include "envoy/registry/registry.h" - -#include "source/common/common/logger.h" +#include "source/extensions/tracers/pomerium_otel/typeutils.h" #include "source/extensions/tracers/pomerium_otel/tracer_impl.h" -namespace Envoy { -namespace Extensions { -namespace Tracers { -namespace OpenTelemetry { +namespace Envoy::Extensions::Tracers::OpenTelemetry { PomeriumOpenTelemetryTracerFactory::PomeriumOpenTelemetryTracerFactory() : FactoryBase("envoy.tracers.pomerium_otel") {} @@ -18,16 +12,9 @@ PomeriumOpenTelemetryTracerFactory::PomeriumOpenTelemetryTracerFactory() Tracing::DriverSharedPtr PomeriumOpenTelemetryTracerFactory::createTracerDriverTyped( const pomerium::extensions::OpenTelemetryConfig& proto_config, Server::Configuration::TracerFactoryContext& context) { - envoy::config::trace::v3::OpenTelemetryConfig base = toBaseConfig(proto_config); - return std::make_shared(base, context); + return std::make_shared(toBaseConfig(proto_config), context); } -/** - * Static registration for the OpenTelemetry tracer. @see RegisterFactory. - */ REGISTER_FACTORY(PomeriumOpenTelemetryTracerFactory, Server::Configuration::TracerFactory); -} // namespace OpenTelemetry -} // namespace Tracers -} // namespace Extensions -} // namespace Envoy +} // namespace Envoy::Extensions::Tracers::OpenTelemetry diff --git a/source/extensions/tracers/pomerium_otel/config.h b/source/extensions/tracers/pomerium_otel/config.h index f92f8fc..a4f16e4 100644 --- a/source/extensions/tracers/pomerium_otel/config.h +++ b/source/extensions/tracers/pomerium_otel/config.h @@ -1,23 +1,11 @@ #pragma once -#include - -#include "source/extensions/tracers/pomerium_otel/typeutils.h" - -#include "envoy/config/trace/v3/opentelemetry.pb.h" -#include "envoy/config/trace/v3/opentelemetry.pb.validate.h" #include "source/extensions/tracers/pomerium_otel/pomerium_otel.pb.h" #include "source/extensions/tracers/common/factory_base.h" -namespace Envoy { -namespace Extensions { -namespace Tracers { -namespace OpenTelemetry { +namespace Envoy::Extensions::Tracers::OpenTelemetry { -/** - * Config registration for the OpenTelemetry tracer. @see TracerFactory. - */ class PomeriumOpenTelemetryTracerFactory : Logger::Loggable, public Common::FactoryBase { @@ -31,7 +19,4 @@ class PomeriumOpenTelemetryTracerFactory Server::Configuration::TracerFactoryContext& context) override; }; -} // namespace OpenTelemetry -} // namespace Tracers -} // namespace Extensions -} // namespace Envoy +} // namespace Envoy::Extensions::Tracers::OpenTelemetry diff --git a/source/extensions/tracers/pomerium_otel/span.cc b/source/extensions/tracers/pomerium_otel/span.cc index 3203c07..ea635ae 100644 --- a/source/extensions/tracers/pomerium_otel/span.cc +++ b/source/extensions/tracers/pomerium_otel/span.cc @@ -1,4 +1,6 @@ #include "source/extensions/tracers/pomerium_otel/span.h" +#include "absl/strings/str_replace.h" +#include "source/common/common/assert.h" namespace Envoy::Extensions::Tracers::OpenTelemetry { diff --git a/source/extensions/tracers/pomerium_otel/span.h b/source/extensions/tracers/pomerium_otel/span.h index cec8eb4..25ffed2 100644 --- a/source/extensions/tracers/pomerium_otel/span.h +++ b/source/extensions/tracers/pomerium_otel/span.h @@ -1,8 +1,6 @@ #pragma once #include "envoy/tracing/trace_driver.h" #include "source/extensions/tracers/opentelemetry/opentelemetry_tracer_impl.h" -#include "absl/strings/str_replace.h" -#include "source/common/common/assert.h" namespace Envoy::Extensions::Tracers::OpenTelemetry { diff --git a/source/extensions/tracers/pomerium_otel/tracer_impl.cc b/source/extensions/tracers/pomerium_otel/tracer_impl.cc index 5374cf6..2590ba9 100644 --- a/source/extensions/tracers/pomerium_otel/tracer_impl.cc +++ b/source/extensions/tracers/pomerium_otel/tracer_impl.cc @@ -1,15 +1,12 @@ #include "source/extensions/tracers/pomerium_otel/tracer_impl.h" + #include "source/extensions/tracers/pomerium_otel/span.h" #include "source/common/tracing/trace_context_impl.h" #include "source/common/common/logger.h" #include "source/common/http/path_utility.h" -#include "absl/strings/str_replace.h" -namespace Envoy { -namespace Extensions { -namespace Tracers { -namespace OpenTelemetry { +namespace Envoy::Extensions::Tracers::OpenTelemetry { const Tracing::TraceContextHandler& pomeriumTraceParentHeader() { CONSTRUCT_ON_FIRST_USE(Tracing::TraceContextHandler, "x-pomerium-traceparent"); @@ -23,13 +20,20 @@ const Tracing::TraceContextHandler& pomeriumSamplingDecisionHeader() { CONSTRUCT_ON_FIRST_USE(Tracing::TraceContextHandler, "x-pomerium-sampling-decision"); } +// workaround for OpenTelemetry::Driver having private inheritance to +// Logger::Loggable +static spdlog::logger& logger() { + static spdlog::logger& instance = Envoy::Logger::Registry::getLog(Logger::Id::tracing); + return instance; +} + Tracing::SpanPtr PomeriumDriver::startSpan(const Tracing::Config& config, Tracing::TraceContext& trace_context, const StreamInfo::StreamInfo& stream_info, const std::string& operation_name, Tracing::Decision tracing_decision) { - std::vector> substitutions{ + std::vector> name_substitutions{ {"${path}", std::string(Envoy::Http::PathUtil::removeQueryAndFragment(trace_context.path()))}, {"${host}", std::string(trace_context.host())}, {"${method}", std::string(trace_context.method())}, @@ -37,28 +41,27 @@ Tracing::SpanPtr PomeriumDriver::startSpan(const Tracing::Config& config, }; auto span = new VariableNameSpan( Driver::startSpan(config, trace_context, stream_info, operation_name, tracing_decision), - substitutions); + name_substitutions); if (auto tp = pomeriumTraceParentHeader().get(trace_context); tp.has_value() && tp->size() == 55) { auto new_trace_id = tp.value().substr(3, 32); - ENVOY_LOG(trace, "rewriting trace ID {} => {}", span->getTraceId(), new_trace_id); + ENVOY_LOG_TO_LOGGER(logger(), trace, "rewriting trace ID {} => {}", span->getTraceId(), + new_trace_id); span->setTraceId(new_trace_id); } else if (auto tid = pomeriumTraceIDHeader().get(trace_context); tid.has_value() && tid.value().size() == 32) { auto new_trace_id = tid.value(); - ENVOY_LOG(trace, "rewriting trace ID (alt) {} => {}", span->getTraceId(), new_trace_id); + ENVOY_LOG_TO_LOGGER(logger(), trace, "rewriting trace ID (alt) {} => {}", span->getTraceId(), + new_trace_id); span->setTraceId(new_trace_id); } if (auto decision = pomeriumSamplingDecisionHeader().get(trace_context); decision.has_value()) { - ENVOY_LOG(trace, "forcing sampling decision: {}", decision.value()); + ENVOY_LOG_TO_LOGGER(logger(), trace, "forcing sampling decision: {}", decision.value()); span->setSampled(decision.value() == "1"); } return Tracing::SpanPtr(static_cast(span)); } -} // namespace OpenTelemetry -} // namespace Tracers -} // namespace Extensions -} // namespace Envoy \ No newline at end of file +} // namespace Envoy::Extensions::Tracers::OpenTelemetry diff --git a/source/extensions/tracers/pomerium_otel/tracer_impl.h b/source/extensions/tracers/pomerium_otel/tracer_impl.h index db889df..3c1f807 100644 --- a/source/extensions/tracers/pomerium_otel/tracer_impl.h +++ b/source/extensions/tracers/pomerium_otel/tracer_impl.h @@ -3,13 +3,10 @@ #include "envoy/tracing/trace_driver.h" #include "source/extensions/tracers/opentelemetry/opentelemetry_tracer_impl.h" -namespace Envoy { -namespace Extensions { -namespace Tracers { -namespace OpenTelemetry { +namespace Envoy::Extensions::Tracers::OpenTelemetry { /** - * OpenTelemetry tracing driver. + * Pomerium custom OpenTelemetry tracing driver. */ class PomeriumDriver : public Driver { public: @@ -20,11 +17,6 @@ class PomeriumDriver : public Driver { const StreamInfo::StreamInfo& stream_info, const std::string& operation_name, Tracing::Decision tracing_decision) override; - -private: }; -} // namespace OpenTelemetry -} // namespace Tracers -} // namespace Extensions -} // namespace Envoy +} // namespace Envoy::Extensions::Tracers::OpenTelemetry \ No newline at end of file diff --git a/source/extensions/tracers/pomerium_otel/typeutils.h b/source/extensions/tracers/pomerium_otel/typeutils.h index 0bdd185..c54978a 100644 --- a/source/extensions/tracers/pomerium_otel/typeutils.h +++ b/source/extensions/tracers/pomerium_otel/typeutils.h @@ -4,9 +4,11 @@ #include "envoy/config/trace/v3/opentelemetry.pb.validate.h" #include "source/extensions/tracers/pomerium_otel/pomerium_otel.pb.h" -inline envoy::config::trace::v3::OpenTelemetryConfig -toBaseConfig(const ::pomerium::extensions::OpenTelemetryConfig& proto_config) { - envoy::config::trace::v3::OpenTelemetryConfig base; +namespace pomerium::extensions { + +inline ::envoy::config::trace::v3::OpenTelemetryConfig +toBaseConfig(const OpenTelemetryConfig& proto_config) { + ::envoy::config::trace::v3::OpenTelemetryConfig base; if (proto_config.has_grpc_service()) { *base.mutable_grpc_service() = proto_config.grpc_service(); } @@ -21,9 +23,8 @@ toBaseConfig(const ::pomerium::extensions::OpenTelemetryConfig& proto_config) { return base; } -namespace pomerium::extensions { -inline bool Validate(const ::pomerium::extensions::OpenTelemetryConfig& m, - pgv::ValidationMsg* err) { +inline bool Validate(const OpenTelemetryConfig& m, pgv::ValidationMsg* err) { return Validate(toBaseConfig(m), err); } + } // namespace pomerium::extensions \ No newline at end of file From 81cd8f7aa4f1d3192021359f6abbf3fb20d0e51e Mon Sep 17 00:00:00 2001 From: Joe Kralicky Date: Thu, 9 Jan 2025 01:05:29 +0000 Subject: [PATCH 3/5] add tests --- .gitignore | 3 +- .../early_header_mutation/trace_context/BUILD | 1 + .../trace_context/trace_context_test.cc | 28 +++- source/extensions/tracers/pomerium_otel/BUILD | 18 +++ .../tracers/pomerium_otel/config.cc | 1 - .../extensions/tracers/pomerium_otel/config.h | 1 + .../pomerium_otel/pomerium_otel_test.cc | 124 ++++++++++++++++++ .../extensions/tracers/pomerium_otel/span.cc | 3 + .../extensions/tracers/pomerium_otel/span.h | 2 + 9 files changed, 176 insertions(+), 5 deletions(-) create mode 100644 source/extensions/tracers/pomerium_otel/pomerium_otel_test.cc diff --git a/.gitignore b/.gitignore index 12d0d8a..4ec880e 100644 --- a/.gitignore +++ b/.gitignore @@ -3,4 +3,5 @@ clang.bazelrc user.bazelrc compile_commands.json .vscode -user.bazelrc \ No newline at end of file +.cache +user.bazelrc diff --git a/source/extensions/http/early_header_mutation/trace_context/BUILD b/source/extensions/http/early_header_mutation/trace_context/BUILD index 9f333ab..cefc89f 100644 --- a/source/extensions/http/early_header_mutation/trace_context/BUILD +++ b/source/extensions/http/early_header_mutation/trace_context/BUILD @@ -46,6 +46,7 @@ envoy_cc_test( deps = [ ":pomerium_trace_context", ":trace_context_cc_proto", + "@com_google_absl//absl/random", "@envoy//source/common/common:random_generator_lib", "@envoy//test/mocks/runtime:runtime_mocks", "@envoy//test/mocks/stream_info:stream_info_mocks", diff --git a/source/extensions/http/early_header_mutation/trace_context/trace_context_test.cc b/source/extensions/http/early_header_mutation/trace_context/trace_context_test.cc index 98163d5..fc3d2a9 100644 --- a/source/extensions/http/early_header_mutation/trace_context/trace_context_test.cc +++ b/source/extensions/http/early_header_mutation/trace_context/trace_context_test.cc @@ -2,6 +2,8 @@ #include "test/mocks/stream_info/mocks.h" #include "test/test_common/utility.h" +#include "source/common/common/base64.h" +#include "absl/random/random.h" #include "gtest/gtest.h" @@ -60,7 +62,7 @@ TEST(TraceContextTest, Mutate) { } { Envoy::Http::TestRequestHeaderMapImpl request_headers{ - {":path", fmt::format("/foo/bar?pomerium_traceparent=invalid")}, + {":path", "/foo/bar?pomerium_traceparent=invalid"}, }; EXPECT_TRUE(tc.mutate(request_headers, stream_info)); EXPECT_FALSE(request_headers.has("x-pomerium-sampling-decision")); @@ -68,7 +70,7 @@ TEST(TraceContextTest, Mutate) { } { Envoy::Http::TestRequestHeaderMapImpl request_headers{ - {":path", fmt::format("/foo/bar")}, + {":path", "/foo/bar"}, }; EXPECT_TRUE(tc.mutate(request_headers, stream_info)); EXPECT_FALSE(request_headers.has("x-pomerium-sampling-decision")); @@ -76,7 +78,7 @@ TEST(TraceContextTest, Mutate) { } { Envoy::Http::TestRequestHeaderMapImpl request_headers{ - {":path", fmt::format("/foo/bar?pomerium_traceparent=00-1-2-??")}, + {":path", "/foo/bar?pomerium_traceparent=00-1-2-??"}, }; EXPECT_TRUE(tc.mutate(request_headers, stream_info)); EXPECT_FALSE(request_headers.has("x-pomerium-sampling-decision")); @@ -90,6 +92,26 @@ TEST(TraceContextTest, Mutate) { EXPECT_EQ("2222222222222222", request_headers.get_("x-pomerium-external-parent-span")); EXPECT_FALSE(request_headers.has("x-pomerium-traceparent")); } + { + absl::BitGen bitgen; + const auto traceid_bytes = + absl::StrCat(absl::HexStringToBytes("11111111111111111111111111111111"), 1); + EXPECT_EQ(traceid_bytes.size(), 17); + const auto encoded_traceid = Base64Url::encode(traceid_bytes.c_str(), traceid_bytes.size()); + char random_bytes[64]; + for (int i = 0; i < 64; i++) { + random_bytes[i] = absl::Uniform(bitgen, -128, 127); + } + const auto state = absl::StrCat("foo|bar|", encoded_traceid, "|", random_bytes); + auto state_encoded = Base64Url::encode(state.c_str(), state.size()); + Base64::completePadding(state_encoded); // match go base64url encoding + Envoy::Http::TestRequestHeaderMapImpl request_headers{ + {":path", absl::StrCat("/oauth2/callback?code=xyz&state=", state_encoded)}, + }; + EXPECT_TRUE(tc.mutate(request_headers, stream_info)); + EXPECT_EQ("11111111111111111111111111111111", request_headers.get_("x-pomerium-traceid")); + EXPECT_EQ("1", request_headers.get_("x-pomerium-sampling-decision")); + } } } // namespace Envoy::Extensions::Http::EarlyHeaderMutation \ No newline at end of file diff --git a/source/extensions/tracers/pomerium_otel/BUILD b/source/extensions/tracers/pomerium_otel/BUILD index fac6586..704fde6 100644 --- a/source/extensions/tracers/pomerium_otel/BUILD +++ b/source/extensions/tracers/pomerium_otel/BUILD @@ -45,3 +45,21 @@ proto_library( "@envoy_api//envoy/config/core/v3:pkg", ], ) + +envoy_cc_test( + name = "pomerium_otel_test", + srcs = [ + "pomerium_otel_test.cc", + ], + repository = "@envoy", + deps = [ + ":pomerium_otel", + ":pomerium_otel_cc_proto", + "@envoy//test/mocks/server:tracer_factory_context_mocks", + "@envoy//test/mocks/stream_info:stream_info_mocks", + "@envoy//test/mocks/thread_local:thread_local_mocks", + "@envoy//test/mocks/tracing:tracing_mocks", + "@envoy//test/mocks/upstream:cluster_manager_mocks", + "@envoy//test/test_common:utility_lib", + ], +) diff --git a/source/extensions/tracers/pomerium_otel/config.cc b/source/extensions/tracers/pomerium_otel/config.cc index d53c2ad..5345b00 100644 --- a/source/extensions/tracers/pomerium_otel/config.cc +++ b/source/extensions/tracers/pomerium_otel/config.cc @@ -1,7 +1,6 @@ #include "source/extensions/tracers/pomerium_otel/config.h" #include "envoy/registry/registry.h" -#include "source/extensions/tracers/pomerium_otel/typeutils.h" #include "source/extensions/tracers/pomerium_otel/tracer_impl.h" namespace Envoy::Extensions::Tracers::OpenTelemetry { diff --git a/source/extensions/tracers/pomerium_otel/config.h b/source/extensions/tracers/pomerium_otel/config.h index a4f16e4..1842357 100644 --- a/source/extensions/tracers/pomerium_otel/config.h +++ b/source/extensions/tracers/pomerium_otel/config.h @@ -1,6 +1,7 @@ #pragma once #include "source/extensions/tracers/pomerium_otel/pomerium_otel.pb.h" +#include "source/extensions/tracers/pomerium_otel/typeutils.h" #include "source/extensions/tracers/common/factory_base.h" diff --git a/source/extensions/tracers/pomerium_otel/pomerium_otel_test.cc b/source/extensions/tracers/pomerium_otel/pomerium_otel_test.cc new file mode 100644 index 0000000..ff76f22 --- /dev/null +++ b/source/extensions/tracers/pomerium_otel/pomerium_otel_test.cc @@ -0,0 +1,124 @@ +#include "source/extensions/tracers/pomerium_otel/config.h" +#include "source/extensions/tracers/pomerium_otel/span.h" + +#include "source/common/tracing/http_tracer_impl.h" +#include "test/mocks/server/tracer_factory_context.h" +#include "test/mocks/stream_info/mocks.h" +#include "test/mocks/thread_local/mocks.h" +#include "test/mocks/tracing/mocks.h" +#include "test/mocks/upstream/cluster_manager.h" +#include "test/test_common/utility.h" +#include "tracer_impl.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +// constexpr auto traceid_unsampled = "00-11111111111111111111111111111111-2222222222222222-00"; +constexpr std::string_view traceid_sampled = + "00-11111111111111111111111111111111-2222222222222222-01"; + +namespace Envoy::Extensions::Tracers::OpenTelemetry { + +using testing::Return; + +class PomeriumOtelTest : public testing::Test { +public: + PomeriumOtelTest(); + +protected: + NiceMock config; + NiceMock stream_info; + NiceMock context; + NiceMock cluster_manager_; + + Tracing::TestTraceContextImpl trace_context{}; + std::unique_ptr driver; +}; + +PomeriumOtelTest::PomeriumOtelTest() { + cluster_manager_.initializeClusters({"fake-cluster"}, {}); + cluster_manager_.thread_local_cluster_.cluster_.info_->name_ = "fake-cluster"; + cluster_manager_.initializeThreadLocalClusters({"fake-cluster"}); + + const std::string yaml_string = R"EOF( + grpc_service: + envoy_grpc: + cluster_name: fake-cluster + timeout: 0.250s + service_name: test-service-name + )EOF"; + + envoy::config::trace::v3::OpenTelemetryConfig opentelemetry_config; + TestUtility::loadFromYaml(yaml_string, opentelemetry_config); + + trace_context = { + {":method", "GET"}, + {":protocol", "https://"}, + {":authority", "foo.example.com"}, + {":path", "/bar"}, + }; + driver = std::make_unique(opentelemetry_config, context); +} + +TEST_F(PomeriumOtelTest, VariableNameSpan) { + const std::string operation_name = "overwritten"; + Tracing::SpanPtr tracing_span = driver->startSpan( + config, trace_context, stream_info, operation_name, {Tracing::Reason::Sampling, true}); + + EXPECT_EQ(dynamic_cast(tracing_span.get())->name(), operation_name); + + const std::string new_operation_name = "${method} ${protocol}${host}${path}"; + tracing_span->setOperation(new_operation_name); + EXPECT_EQ(dynamic_cast(tracing_span.get())->name(), + "GET https://foo.example.com/bar"); +} + +TEST_F(PomeriumOtelTest, StartSpanWithNoTraceparent) { + NiceMock& mock_random_generator_ = + context.server_factory_context_.api_.random_; + ON_CALL(mock_random_generator_, random()).WillByDefault(Return(0xDEADBEEFDEADBEEF)); + + Tracing::SpanPtr tracing_span = driver->startSpan(config, trace_context, stream_info, "test", + {Tracing::Reason::Sampling, true}); + + EXPECT_EQ(tracing_span->getTraceId(), absl::StrCat(Hex::uint64ToHex(0xDEADBEEFDEADBEEF), + Hex::uint64ToHex(0xDEADBEEFDEADBEEF))); +} + +TEST_F(PomeriumOtelTest, StartSpanWithTraceparent) { + NiceMock& mock_random_generator_ = + context.server_factory_context_.api_.random_; + ON_CALL(mock_random_generator_, random()).WillByDefault(Return(0xDEADBEEFDEADBEEF)); + + trace_context.set("x-pomerium-traceparent", traceid_sampled); + Tracing::SpanPtr tracing_span = driver->startSpan(config, trace_context, stream_info, "test", + {Tracing::Reason::Sampling, true}); + EXPECT_EQ(tracing_span->getTraceId(), traceid_sampled.substr(3, 32)); +} + +TEST_F(PomeriumOtelTest, StartSpanWithTraceID) { + NiceMock& mock_random_generator_ = + context.server_factory_context_.api_.random_; + ON_CALL(mock_random_generator_, random()).WillByDefault(Return(0xDEADBEEFDEADBEEF)); + + trace_context.set("x-pomerium-traceid", traceid_sampled.substr(3, 32)); + Tracing::SpanPtr tracing_span = driver->startSpan(config, trace_context, stream_info, "test", + {Tracing::Reason::Sampling, true}); + EXPECT_EQ(tracing_span->getTraceId(), traceid_sampled.substr(3, 32)); +} + +TEST_F(PomeriumOtelTest, StartSpanWithSamplingDecisionOff) { + trace_context.set("x-pomerium-sampling-decision", "0"); + Tracing::SpanPtr tracing_span = driver->startSpan(config, trace_context, stream_info, "test", + {Tracing::Reason::Sampling, true}); + EXPECT_FALSE(dynamic_cast(tracing_span.get())->sampled()); +} + +TEST_F(PomeriumOtelTest, StartSpanWithSamplingDecisionOn) { + trace_context.set("x-pomerium-sampling-decision", "1"); + Tracing::SpanPtr tracing_span = driver->startSpan(config, trace_context, stream_info, "test", + {Tracing::Reason::NotTraceable, false}); + EXPECT_TRUE(dynamic_cast(tracing_span.get())->sampled()); +} + +} // namespace Envoy::Extensions::Tracers::OpenTelemetry diff --git a/source/extensions/tracers/pomerium_otel/span.cc b/source/extensions/tracers/pomerium_otel/span.cc index ea635ae..d5f7383 100644 --- a/source/extensions/tracers/pomerium_otel/span.cc +++ b/source/extensions/tracers/pomerium_otel/span.cc @@ -13,6 +13,9 @@ void VariableNameSpan::setTraceId(const absl::string_view& trace_id_hex) { span_->setTraceId(trace_id_hex); } +std::string VariableNameSpan::name() const { return span_->name(); } +bool VariableNameSpan::sampled() const { return span_->sampled(); } + void VariableNameSpan::setOperation(absl::string_view operation_name) { span_->setOperation(absl::StrReplaceAll(operation_name, substitutions_)); } diff --git a/source/extensions/tracers/pomerium_otel/span.h b/source/extensions/tracers/pomerium_otel/span.h index 25ffed2..b1c939b 100644 --- a/source/extensions/tracers/pomerium_otel/span.h +++ b/source/extensions/tracers/pomerium_otel/span.h @@ -14,6 +14,8 @@ class VariableNameSpan : public Tracing::Span { ~VariableNameSpan() override = default; void setTraceId(const absl::string_view& trace_id_hex); + std::string name() const; + bool sampled() const; void setOperation(absl::string_view operation_name) override; From 3ef735f26513cd5218cc0a9495911578c351cf22 Mon Sep 17 00:00:00 2001 From: Joe Kralicky Date: Thu, 9 Jan 2025 19:43:34 +0000 Subject: [PATCH 4/5] add .bazelignore --- .bazelignore | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 .bazelignore diff --git a/.bazelignore b/.bazelignore new file mode 100644 index 0000000..5a868a0 --- /dev/null +++ b/.bazelignore @@ -0,0 +1,4 @@ +bazel-bin +bazel-envoy-custom +bazel-out +bazel-testlogs From 912dec33b0780a272afa5f2238f57a35e2b49c4d Mon Sep 17 00:00:00 2001 From: Joe Kralicky Date: Thu, 9 Jan 2025 19:44:01 +0000 Subject: [PATCH 5/5] add more comments --- .../trace_context/trace_context.cc | 9 +++++++- .../pomerium_otel/pomerium_otel_test.cc | 11 +++++----- .../tracers/pomerium_otel/tracer_impl.cc | 22 +++++++++++++------ 3 files changed, 28 insertions(+), 14 deletions(-) diff --git a/source/extensions/http/early_header_mutation/trace_context/trace_context.cc b/source/extensions/http/early_header_mutation/trace_context/trace_context.cc index 8aecb6c..dbeab2d 100644 --- a/source/extensions/http/early_header_mutation/trace_context/trace_context.cc +++ b/source/extensions/http/early_header_mutation/trace_context/trace_context.cc @@ -24,7 +24,8 @@ bool TraceContext::mutate(Envoy::Http::RequestHeaderMap& headers, * * If the x-pomerium-traceparent header is present, the sampling decision will * be set in the x-pomerium-sampling-decision header. This header is then read - * by the uuidx extension to force the desired trace decision if necessary. + * by the uuidx extension to ensure consistency in request IDs, and by the + * pomerium_otel extension to force a sampling decision in newly created spans. */ headers.remove(pomerium_external_parent_header); @@ -43,8 +44,14 @@ bool TraceContext::mutate(Envoy::Http::RequestHeaderMap& headers, // the query parameters are standard and managed by oauth2 clients const auto state = params.getFirstValue(Envoy::Http::LowerCaseString("state")); if (state.has_value()) { + // The Pomerium state format looks like: + // nonce|timestamp|trace_id+flags|encrypted_data(redirect_url)+mac(nonce,ts) + // The trace ID segment can be empty if this request was not traced. If so, the delimiter + // is still present (e.g. the state will be "nonce|timestamp||encrypted_data"). const std::string stateDecoded = Base64Url::decode(StringUtil::removeTrailingCharacters(state.value(), '=')); + // The encrypted data is not base64-encoded like the other fields, so read only up to the + // third delimiter, instead of trying to split the entire string. const std::vector segments = absl::StrSplit(stateDecoded, absl::MaxSplits('|', 3)); if (segments.size() == 4) { diff --git a/source/extensions/tracers/pomerium_otel/pomerium_otel_test.cc b/source/extensions/tracers/pomerium_otel/pomerium_otel_test.cc index ff76f22..8bcac21 100644 --- a/source/extensions/tracers/pomerium_otel/pomerium_otel_test.cc +++ b/source/extensions/tracers/pomerium_otel/pomerium_otel_test.cc @@ -13,8 +13,7 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" -// constexpr auto traceid_unsampled = "00-11111111111111111111111111111111-2222222222222222-00"; -constexpr std::string_view traceid_sampled = +constexpr std::string_view example_traceid = "00-11111111111111111111111111111111-2222222222222222-01"; namespace Envoy::Extensions::Tracers::OpenTelemetry { @@ -90,10 +89,10 @@ TEST_F(PomeriumOtelTest, StartSpanWithTraceparent) { context.server_factory_context_.api_.random_; ON_CALL(mock_random_generator_, random()).WillByDefault(Return(0xDEADBEEFDEADBEEF)); - trace_context.set("x-pomerium-traceparent", traceid_sampled); + trace_context.set("x-pomerium-traceparent", example_traceid); Tracing::SpanPtr tracing_span = driver->startSpan(config, trace_context, stream_info, "test", {Tracing::Reason::Sampling, true}); - EXPECT_EQ(tracing_span->getTraceId(), traceid_sampled.substr(3, 32)); + EXPECT_EQ(tracing_span->getTraceId(), example_traceid.substr(3, 32)); } TEST_F(PomeriumOtelTest, StartSpanWithTraceID) { @@ -101,10 +100,10 @@ TEST_F(PomeriumOtelTest, StartSpanWithTraceID) { context.server_factory_context_.api_.random_; ON_CALL(mock_random_generator_, random()).WillByDefault(Return(0xDEADBEEFDEADBEEF)); - trace_context.set("x-pomerium-traceid", traceid_sampled.substr(3, 32)); + trace_context.set("x-pomerium-traceid", example_traceid.substr(3, 32)); Tracing::SpanPtr tracing_span = driver->startSpan(config, trace_context, stream_info, "test", {Tracing::Reason::Sampling, true}); - EXPECT_EQ(tracing_span->getTraceId(), traceid_sampled.substr(3, 32)); + EXPECT_EQ(tracing_span->getTraceId(), example_traceid.substr(3, 32)); } TEST_F(PomeriumOtelTest, StartSpanWithSamplingDecisionOff) { diff --git a/source/extensions/tracers/pomerium_otel/tracer_impl.cc b/source/extensions/tracers/pomerium_otel/tracer_impl.cc index 2590ba9..6083fae 100644 --- a/source/extensions/tracers/pomerium_otel/tracer_impl.cc +++ b/source/extensions/tracers/pomerium_otel/tracer_impl.cc @@ -43,22 +43,30 @@ Tracing::SpanPtr PomeriumDriver::startSpan(const Tracing::Config& config, Driver::startSpan(config, trace_context, stream_info, operation_name, tracing_decision), name_substitutions); + // a valid trace context is a 55-character string containing four hex-encoded segments separated + // by '-' characters (see https://www.w3.org/TR/trace-context/#trace-context-http-headers-format) if (auto tp = pomeriumTraceParentHeader().get(trace_context); tp.has_value() && tp->size() == 55) { - auto new_trace_id = tp.value().substr(3, 32); + // trace ID is the second segment in the context, and is 32 bytes long (16 bytes, hex encoded) + auto new_trace_id_hex = tp.value().substr(3, 32); ENVOY_LOG_TO_LOGGER(logger(), trace, "rewriting trace ID {} => {}", span->getTraceId(), - new_trace_id); - span->setTraceId(new_trace_id); + new_trace_id_hex); + span->setTraceId(new_trace_id_hex); } else if (auto tid = pomeriumTraceIDHeader().get(trace_context); tid.has_value() && tid.value().size() == 32) { - auto new_trace_id = tid.value(); + // alternate header containing only the trace ID, used when the complete trace context is not + // available (currently, when handling /oauth2/callback) + auto new_trace_id_hex = tid.value(); ENVOY_LOG_TO_LOGGER(logger(), trace, "rewriting trace ID (alt) {} => {}", span->getTraceId(), - new_trace_id); - span->setTraceId(new_trace_id); + new_trace_id_hex); + span->setTraceId(new_trace_id_hex); } + + // the sampling decision header will be set if either x-pomerium-traceparent or x-pomerium-traceid + // is also set. if (auto decision = pomeriumSamplingDecisionHeader().get(trace_context); decision.has_value()) { ENVOY_LOG_TO_LOGGER(logger(), trace, "forcing sampling decision: {}", decision.value()); - span->setSampled(decision.value() == "1"); + span->setSampled(decision.value() == "1"); // value will be "0" or "1" } return Tracing::SpanPtr(static_cast(span));