added tracing capability to ext-authz http client (envoyproxy#8142)

This PR adds tracing support to the HTTP client in the external authorization filter. So far, only gRPC client was able to trace requests from the filter to an authorization service. Risk Level: Low Testing: yes Docs Changes: N/A Release Notes: Added Fixes envoyproxy#6520 Signed-off-by: Gabriel Linden Sagula <[email protected]>
KNWBIN · Sep 17, 2019 · 2f5f947 · 2f5f947
1 parent 99004b8
commit 2f5f947
Show file tree

Hide file tree

Showing 10 changed files with 169 additions and 41 deletions.
diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst
@@ -22,6 +22,7 @@ Version history
 * config: changed the default value of :ref:`initial_fetch_timeout <envoy_api_field_core.ConfigSource.initial_fetch_timeout>` from 0s to 15s. This is a change in behaviour in the sense that Envoy will move to the next initialization phase, even if the first config is not delivered in 15s. Refer to :ref:`initialization process <arch_overview_initialization>` for more details.
 * config: added stat :ref:`init_fetch_timeout <config_cluster_manager_cds>`.
 * ext_authz: added :ref:`configurable ability <envoy_api_field_config.filter.http.ext_authz.v2.ExtAuthz.metadata_context_namespaces>` to send dynamic metadata to the `ext_authz` service.
+* ext_authz: added tracing to the HTTP client.
 * fault: added overrides for default runtime keys in :ref:`HTTPFault <envoy_api_msg_config.filter.http.fault.v2.HTTPFault>` filter.
 * grpc: added :ref:`AWS IAM grpc credentials extension <envoy_api_file_envoy/config/grpc_credential/v2alpha/aws_iam.proto>` for AWS-managed xDS.
 * grpc-json: added support for :ref:`ignoring unknown query parameters<envoy_api_field_config.filter.http.transcoder.v2.GrpcJsonTranscoder.ignore_unknown_query_parameters>`.

diff --git a/source/extensions/filters/common/ext_authz/BUILD b/source/extensions/filters/common/ext_authz/BUILD
@@ -63,6 +63,7 @@ envoy_cc_library(
         "//source/common/common:minimal_logger_lib",
         "//source/common/http:async_client_lib",
         "//source/common/http:codes_lib",
+        "//source/common/tracing:http_tracer_lib",
         "@envoy_api//envoy/config/filter/http/ext_authz/v2:ext_authz_cc",
     ],
 )

diff --git a/source/extensions/filters/common/ext_authz/ext_authz.h b/source/extensions/filters/common/ext_authz/ext_authz.h
@@ -10,12 +10,26 @@
 #include "envoy/service/auth/v2/external_auth.pb.h"
 #include "envoy/tracing/http_tracer.h"
 
+#include "common/singleton/const_singleton.h"
+
 namespace Envoy {
 namespace Extensions {
 namespace Filters {
 namespace Common {
 namespace ExtAuthz {
 
+/**
+ * Constant values used for tracing metadata.
+ */
+struct TracingContantValues {
+  const std::string TraceStatus = "ext_authz_status";
+  const std::string TraceUnauthz = "ext_authz_unauthorized";
+  const std::string TraceOk = "ext_authz_ok";
+  const std::string HttpStatus = "ext_authz_http_status";
+};
+
+using TracingConstants = ConstSingleton<TracingContantValues>;
+
 /**
  * Possible async results for a check call.
  */

diff --git a/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc b/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc
@@ -45,13 +45,13 @@ void GrpcClientImpl::onSuccess(std::unique_ptr<envoy::service::auth::v2::CheckRe
                                Tracing::Span& span) {
   ResponsePtr authz_response = std::make_unique<Response>(Response{});
   if (response->status().code() == Grpc::Status::GrpcStatus::Ok) {
-    span.setTag(Constants::get().TraceStatus, Constants::get().TraceOk);
+    span.setTag(TracingConstants::get().TraceStatus, TracingConstants::get().TraceOk);
     authz_response->status = CheckStatus::OK;
     if (response->has_ok_response()) {
       toAuthzResponseHeader(authz_response, response->ok_response().headers());
     }
   } else {
-    span.setTag(Constants::get().TraceStatus, Constants::get().TraceUnauthz);
+    span.setTag(TracingConstants::get().TraceStatus, TracingConstants::get().TraceUnauthz);
     authz_response->status = CheckStatus::Denied;
     if (response->has_denied_response()) {
       toAuthzResponseHeader(authz_response, response->denied_response().headers());

diff --git a/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.h b/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.h
@@ -17,7 +17,6 @@
 #include "envoy/upstream/cluster_manager.h"
 
 #include "common/grpc/typed_async_client.h"
-#include "common/singleton/const_singleton.h"
 
 #include "extensions/filters/common/ext_authz/check_request_utils.h"
 #include "extensions/filters/common/ext_authz/ext_authz.h"
@@ -30,14 +29,6 @@ namespace ExtAuthz {
 
 using ExtAuthzAsyncCallbacks = Grpc::AsyncRequestCallbacks<envoy::service::auth::v2::CheckResponse>;
 
-struct ConstantValues {
-  const std::string TraceStatus = "ext_authz_status";
-  const std::string TraceUnauthz = "ext_authz_unauthorized";
-  const std::string TraceOk = "ext_authz_ok";
-};
-
-using Constants = ConstSingleton<ConstantValues>;
-
 /*
  * This client implementation is used when the Ext_Authz filter needs to communicate with an gRPC
  * authorization server. Unlike the HTTP client, the gRPC allows the server to define response

diff --git a/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc b/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc
@@ -1,6 +1,7 @@
 #include "extensions/filters/common/ext_authz/ext_authz_http_impl.h"
 
 #include "common/common/enum_to_int.h"
+#include "common/common/fmt.h"
 #include "common/http/async_client_impl.h"
 #include "common/http/codes.h"
 
@@ -88,7 +89,8 @@ ClientConfig::ClientConfig(const envoy::config::filter::http::ext_authz::v2::Ext
       authorization_headers_to_add_(
           toHeadersAdd(config.http_service().authorization_request().headers_to_add())),
       cluster_name_(config.http_service().server_uri().cluster()), timeout_(timeout),
-      path_prefix_(path_prefix) {}
+      path_prefix_(path_prefix),
+      tracing_name_(fmt::format("async {} egress", config.http_service().server_uri().cluster())) {}
 
 MatcherSharedPtr
 ClientConfig::toRequestMatchers(const envoy::type::matcher::ListStringMatcher& list) {
@@ -150,23 +152,35 @@ Http::LowerCaseStrPairVector ClientConfig::toHeadersAdd(
   return header_vec;
 }
 
-RawHttpClientImpl::RawHttpClientImpl(Upstream::ClusterManager& cm, ClientConfigSharedPtr config)
-    : cm_(cm), config_(config) {}
+RawHttpClientImpl::RawHttpClientImpl(Upstream::ClusterManager& cm, ClientConfigSharedPtr config,
+                                     TimeSource& time_source)
+    : cm_(cm), config_(config), time_source_(time_source) {}
 
-RawHttpClientImpl::~RawHttpClientImpl() { ASSERT(!callbacks_); }
+RawHttpClientImpl::~RawHttpClientImpl() {
+  ASSERT(callbacks_ == nullptr);
+  ASSERT(span_ == nullptr);
+}
 
 void RawHttpClientImpl::cancel() {
   ASSERT(callbacks_ != nullptr);
+  ASSERT(span_ != nullptr);
+  span_->setTag(Tracing::Tags::get().Status, Tracing::Tags::get().Canceled);
+  span_->finishSpan();
   request_->cancel();
   callbacks_ = nullptr;
+  span_ = nullptr;
 }
 
 // Client
 void RawHttpClientImpl::check(RequestCallbacks& callbacks,
                               const envoy::service::auth::v2::CheckRequest& request,
-                              Tracing::Span&) {
+                              Tracing::Span& parent_span) {
   ASSERT(callbacks_ == nullptr);
+  ASSERT(span_ == nullptr);
   callbacks_ = &callbacks;
+  span_ = parent_span.spawnChild(Tracing::EgressConfig::get(), config_->tracingName(),
+                                 time_source_.systemTime());
+  span_->setTag(Tracing::Tags::get().UpstreamCluster, config_->cluster());
 
   Http::HeaderMapPtr headers;
   const uint64_t request_length = request.attributes().request().http().body().size();
@@ -210,7 +224,10 @@ void RawHttpClientImpl::check(RequestCallbacks& callbacks,
     // TODO(dio): Add stats and tracing related to this.
     ENVOY_LOG(debug, "ext_authz cluster '{}' does not exist", cluster);
     callbacks_->onComplete(std::make_unique<Response>(errorResponse()));
+    span_->setTag(Tracing::Tags::get().Error, Tracing::Tags::get().True);
+    span_->finishSpan();
     callbacks_ = nullptr;
+    span_ = nullptr;
   } else {
     request_ = cm_.httpAsyncClientForCluster(cluster).send(
         std::move(message), *this,
@@ -220,13 +237,18 @@ void RawHttpClientImpl::check(RequestCallbacks& callbacks,
 
 void RawHttpClientImpl::onSuccess(Http::MessagePtr&& message) {
   callbacks_->onComplete(toResponse(std::move(message)));
+  span_->finishSpan();
   callbacks_ = nullptr;
+  span_ = nullptr;
 }
 
 void RawHttpClientImpl::onFailure(Http::AsyncClient::FailureReason reason) {
   ASSERT(reason == Http::AsyncClient::FailureReason::Reset);
   callbacks_->onComplete(std::make_unique<Response>(errorResponse()));
+  span_->setTag(Tracing::Tags::get().Error, Tracing::Tags::get().True);
+  span_->finishSpan();
   callbacks_ = nullptr;
+  span_ = nullptr;
 }
 
 ResponsePtr RawHttpClientImpl::toResponse(Http::MessagePtr message) {
@@ -235,9 +257,13 @@ ResponsePtr RawHttpClientImpl::toResponse(Http::MessagePtr message) {
   uint64_t status_code{};
   if (!absl::SimpleAtoi(message->headers().Status()->value().getStringView(), &status_code)) {
     ENVOY_LOG(warn, "ext_authz HTTP client failed to parse the HTTP status code.");
+    span_->setTag(Tracing::Tags::get().Error, Tracing::Tags::get().True);
     return std::make_unique<Response>(errorResponse());
   }
 
+  span_->setTag(TracingConstants::get().HttpStatus,
+                Http::CodeUtility::toString(static_cast<Http::Code>(status_code)));
+
   // Set an error status if the call to the authorization server returns any of the 5xx HTTP error
   // codes. A Forbidden response is sent to the client if the filter has not been configured with
   // failure_mode_allow.
@@ -250,13 +276,15 @@ ResponsePtr RawHttpClientImpl::toResponse(Http::MessagePtr message) {
     SuccessResponse ok{message->headers(), config_->upstreamHeaderMatchers(),
                        Response{CheckStatus::OK, Http::HeaderVector{}, Http::HeaderVector{},
                                 EMPTY_STRING, Http::Code::OK}};
+    span_->setTag(TracingConstants::get().TraceStatus, TracingConstants::get().TraceOk);
     return std::move(ok.response_);
   }
 
   // Create a Denied authorization response.
   SuccessResponse denied{message->headers(), config_->clientHeaderMatchers(),
                          Response{CheckStatus::Denied, Http::HeaderVector{}, Http::HeaderVector{},
                                   message->bodyAsString(), static_cast<Http::Code>(status_code)}};
+  span_->setTag(TracingConstants::get().TraceStatus, TracingConstants::get().TraceUnauthz);
   return std::move(denied.response_);
 }
 

diff --git a/source/extensions/filters/common/ext_authz/ext_authz_http_impl.h b/source/extensions/filters/common/ext_authz/ext_authz_http_impl.h
@@ -1,6 +1,7 @@
 #pragma once
 
 #include "envoy/config/filter/http/ext_authz/v2/ext_authz.pb.h"
+#include "envoy/tracing/http_tracer.h"
 #include "envoy/upstream/cluster_manager.h"
 
 #include "common/common/logger.h"
@@ -88,16 +89,21 @@ class ClientConfig {
   const MatcherSharedPtr& clientHeaderMatchers() const { return client_header_matchers_; }
 
   /**
-   *  Returns a list of matchers used for selecting the authorization response headers that
+   * Returns a list of matchers used for selecting the authorization response headers that
    * should be send to an the upstream server.
    */
   const MatcherSharedPtr& upstreamHeaderMatchers() const { return upstream_header_matchers_; }
 
   /**
-   * @return List of headers that will be add to the authorization request.
+   * Returns a list of headers that will be add to the authorization request.
    */
   const Http::LowerCaseStrPairVector& headersToAdd() const { return authorization_headers_to_add_; }
 
+  /**
+   * Returns the name used for tracing.
+   */
+  const std::string& tracingName() { return tracing_name_; }
+
 private:
   static MatcherSharedPtr toRequestMatchers(const envoy::type::matcher::ListStringMatcher& matcher);
   static MatcherSharedPtr toClientMatchers(const envoy::type::matcher::ListStringMatcher& matcher);
@@ -113,6 +119,7 @@ class ClientConfig {
   const std::string cluster_name_;
   const std::chrono::milliseconds timeout_;
   const std::string path_prefix_;
+  const std::string tracing_name_;
 };
 
 using ClientConfigSharedPtr = std::shared_ptr<ClientConfig>;
@@ -128,7 +135,8 @@ class RawHttpClientImpl : public Client,
                           public Http::AsyncClient::Callbacks,
                           Logger::Loggable<Logger::Id::config> {
 public:
-  explicit RawHttpClientImpl(Upstream::ClusterManager& cm, ClientConfigSharedPtr config);
+  explicit RawHttpClientImpl(Upstream::ClusterManager& cm, ClientConfigSharedPtr config,
+                             TimeSource& time_source);
   ~RawHttpClientImpl() override;
 
   // ExtAuthz::Client
@@ -146,6 +154,8 @@ class RawHttpClientImpl : public Client,
   ClientConfigSharedPtr config_;
   Http::AsyncClient::Request* request_{};
   RequestCallbacks* callbacks_{};
+  TimeSource& time_source_;
+  Tracing::SpanPtr span_;
 };
 
 } // namespace ExtAuthz

diff --git a/source/extensions/filters/http/ext_authz/config.cc b/source/extensions/filters/http/ext_authz/config.cc
@@ -34,7 +34,7 @@ Http::FilterFactoryCb ExtAuthzFilterConfig::createFilterFactoryFromProtoTyped(
     callback = [filter_config, client_config,
                 &context](Http::FilterChainFactoryCallbacks& callbacks) {
       auto client = std::make_unique<Extensions::Filters::Common::ExtAuthz::RawHttpClientImpl>(
-          context.clusterManager(), client_config);
+          context.clusterManager(), client_config, context.timeSource());
       callbacks.addStreamDecoderFilter(Http::StreamDecoderFilterSharedPtr{
           std::make_shared<Filter>(filter_config, std::move(client))});
     };