ext_auth: add option to measure timeout when check request is created.

docs/root/version_history/current.rst

@@ -14,6 +14,8 @@ Minor Behavior Changes
 * build: the debug information will be generated separately to reduce target size and reduce compilation time when build in compilation mode `dbg` and `opt`. Users will need to build dwp file to debug with gdb.
 * compressor: always insert `Vary` headers for compressible resources even if it's decided not to compress a response due to incompatible `Accept-Encoding` value. The `Vary` header needs to be inserted to let a caching proxy in front of Envoy know that the requested resource still can be served with compression applied.
 * decompressor: headers-only requests were incorrectly not advertising accept-encoding when configured to do so. This is now fixed.
+* ext_authz filter: request timeout will now count from the time the check request is created, instead of when it becomes active. This makes sure that the timeout is enforced even if the ext_authz cluster's circuit breaker is engaged.
+  This behavior can be reverted by setting runtime feature ``envoy.reloadable_features.ext_authz_measure_timeout_on_check_created`` to false.
 * http: added :ref:`contains <envoy_api_msg_type.matcher.StringMatcher>` a new string matcher type which matches if the value of the string has the substring mentioned in contains matcher.
 * http: added :ref:`contains <envoy_api_msg_route.HeaderMatcher>` a new header matcher type which matches if the value of the header has the substring mentioned in contains matcher.
 * http: added :ref:`headers_to_add <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.ResponseMapper.headers_to_add>` to :ref:`local reply mapper <config_http_conn_man_local_reply>` to allow its users to add/append/override response HTTP headers to local replies.

include/envoy/grpc/async_client.h

@@ -184,6 +184,12 @@ class RawAsyncClient {
                                    absl::string_view method_name,
                                    RawAsyncStreamCallbacks& callbacks,
                                    const Http::AsyncClient::StreamOptions& options) PURE;
+
+  /**
+   * @return Event::Dispatcher& the dispatcher backing this client. May return nullptr for clients
+   * that do not use a dispatcher to perform their i/o.
+   */
+  virtual Event::Dispatcher* dispatcher() PURE;
 };
 
 using RawAsyncClientPtr = std::unique_ptr<RawAsyncClient>;

source/common/grpc/async_client_impl.cc

@@ -59,6 +59,10 @@ RawAsyncStream* AsyncClientImpl::startRaw(absl::string_view service_full_name,
   return active_streams_.front().get();
 }
 
+Event::Dispatcher* AsyncClientImpl::dispatcher() {
+  return &cm_.httpAsyncClientForCluster(remote_cluster_name_).dispatcher();
+}
+
 AsyncStreamImpl::AsyncStreamImpl(AsyncClientImpl& parent, absl::string_view service_full_name,
                                  absl::string_view method_name, RawAsyncStreamCallbacks& callbacks,
                                  const Http::AsyncClient::StreamOptions& options)

source/common/grpc/async_client_impl.h

@@ -33,6 +33,7 @@ class AsyncClientImpl final : public RawAsyncClient {
   RawAsyncStream* startRaw(absl::string_view service_full_name, absl::string_view method_name,
                            RawAsyncStreamCallbacks& callbacks,
                            const Http::AsyncClient::StreamOptions& options) override;
+  Event::Dispatcher* dispatcher() override;
 
 private:
   Upstream::ClusterManager& cm_;

source/common/grpc/google_async_client_impl.h

@@ -184,6 +184,8 @@ class GoogleAsyncClientImpl final : public RawAsyncClient, Logger::Loggable<Logg
   RawAsyncStream* startRaw(absl::string_view service_full_name, absl::string_view method_name,
                            RawAsyncStreamCallbacks& callbacks,
                            const Http::AsyncClient::StreamOptions& options) override;
+  // This does not use the worker thread's dispatcher, so we must return nullptr.
+  Event::Dispatcher* dispatcher() override { return nullptr; }
 
   TimeSource& timeSource() { return dispatcher_.timeSource(); }
   uint64_t perStreamBufferLimitBytes() const { return per_stream_buffer_limit_bytes_; }

source/common/grpc/typed_async_client.h

@@ -187,6 +187,8 @@ template <typename Request, typename Response> class AsyncClient /* : public Raw
         Internal::startUntyped(client_.get(), service_method, callbacks, options));
   }
 
+  Event::Dispatcher* dispatcher() { return client_->dispatcher(); }
+
   AsyncClient* operator->() { return this; }
   void operator=(RawAsyncClientPtr&& client) { client_ = std::move(client); }
   void reset() { client_.reset(); }

source/common/runtime/runtime_features.cc

@@ -68,6 +68,7 @@ constexpr const char* runtime_features[] = {
     "envoy.reloadable_features.enable_deprecated_v2_api_warning",
     "envoy.reloadable_features.enable_dns_cache_circuit_breakers",
     "envoy.reloadable_features.ext_authz_http_service_enable_case_sensitive_string_matcher",
+    "envoy.reloadable_features.ext_authz_measure_timeout_on_check_created",
     "envoy.reloadable_features.fix_upgrade_response",
     "envoy.reloadable_features.fix_wildcard_matching",
     "envoy.reloadable_features.fixed_connection_close",

source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc

@@ -17,11 +17,12 @@ namespace Filters {
 namespace Common {
 namespace ExtAuthz {
 
-GrpcClientImpl::GrpcClientImpl(Grpc::RawAsyncClientPtr&& async_client,
+GrpcClientImpl::GrpcClientImpl(Grpc::RawAsyncClientPtr&& async_client, bool internal_timeout,
                                const absl::optional<std::chrono::milliseconds>& timeout,
                                envoy::config::core::v3::ApiVersion transport_api_version,
                                bool use_alpha)
-    : async_client_(std::move(async_client)), timeout_(timeout),
+    : async_client_(std::move(async_client)), internal_timeout_(internal_timeout),
+      timeout_(timeout),
       service_method_(Grpc::VersionedMethods("envoy.service.auth.v3.Authorization.Check",
                                              "envoy.service.auth.v2.Authorization.Check",
                                              "envoy.service.auth.v2alpha.Authorization.Check")
@@ -34,6 +35,7 @@ void GrpcClientImpl::cancel() {
   ASSERT(callbacks_ != nullptr);
   request_->cancel();
   callbacks_ = nullptr;
+  timeout_timer_.reset();
 }
 
 void GrpcClientImpl::check(RequestCallbacks& callbacks,
@@ -42,9 +44,22 @@ void GrpcClientImpl::check(RequestCallbacks& callbacks,
   ASSERT(callbacks_ == nullptr);
   callbacks_ = &callbacks;
 
+  Http::AsyncClient::RequestOptions options;
+  if (internal_timeout_ && timeout_.has_value()) {
+    auto* dispatcher = async_client_->dispatcher();
+    if (dispatcher) {
+      timeout_timer_ = dispatcher->createTimer([this]() -> void { onTimeout(); });
+      timeout_timer_->enableTimer(timeout_.value());
+    }
+  }
+
+  // no internal timer, set the timeout on the request.
+  if (timeout_timer_ == nullptr) {
+    options.setTimeout(timeout_);
+  }
+
   ENVOY_LOG(trace, "Sending CheckRequest: {}", request.DebugString());
-  request_ = async_client_->send(service_method_, request, *this, parent_span,
-                                 Http::AsyncClient::RequestOptions().setTimeout(timeout_),
+  request_ = async_client_->send(service_method_, request, *this, parent_span, options,
                                  transport_api_version_);
 }
 
@@ -88,6 +103,17 @@ void GrpcClientImpl::onFailure(Grpc::Status::GrpcStatus status, const std::strin
   ENVOY_LOG(trace, "CheckRequest call failed with status: {}",
             Grpc::Utility::grpcStatusToString(status));
   ASSERT(status != Grpc::Status::WellKnownGrpcStatus::Ok);
+  respondFailure();
+}
+
+void GrpcClientImpl::onTimeout() {
+  ASSERT(request_ != nullptr);
+  request_->cancel();
+  // let the client know of failure:
+  respondFailure();
+}
+
+void GrpcClientImpl::respondFailure() {
   Response response{};
   response.status = CheckStatus::Error;
   response.status_code = Http::Code::Forbidden;

source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.h

@@ -44,7 +44,7 @@ class GrpcClientImpl : public Client,
                        public Logger::Loggable<Logger::Id::ext_authz> {
 public:
   // TODO(gsagula): remove `use_alpha` param when V2Alpha gets deprecated.
-  GrpcClientImpl(Grpc::RawAsyncClientPtr&& async_client,
+  GrpcClientImpl(Grpc::RawAsyncClientPtr&& async_client, bool internal_timeout,
                  const absl::optional<std::chrono::milliseconds>& timeout,
                  envoy::config::core::v3::ApiVersion transport_api_version, bool use_alpha);
   ~GrpcClientImpl() override;
@@ -62,16 +62,21 @@ class GrpcClientImpl : public Client,
                  Tracing::Span& span) override;
 
 private:
+  void onTimeout();
+  void respondFailure();
   void toAuthzResponseHeader(
       ResponsePtr& response,
       const Protobuf::RepeatedPtrField<envoy::config::core::v3::HeaderValueOption>& headers);
+
   Grpc::AsyncClient<envoy::service::auth::v3::CheckRequest, envoy::service::auth::v3::CheckResponse>
       async_client_;
   Grpc::AsyncRequest* request_{};
+  bool internal_timeout_{};
   absl::optional<std::chrono::milliseconds> timeout_;
   RequestCallbacks* callbacks_{};
   const Protobuf::MethodDescriptor& service_method_;
   const envoy::config::core::v3::ApiVersion transport_api_version_;
+  Event::TimerPtr timeout_timer_;
 };
 
 using GrpcClientImplPtr = std::unique_ptr<GrpcClientImpl>;

source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc

@@ -126,7 +126,7 @@ bool NotHeaderKeyMatcher::matches(absl::string_view key) const { return !matcher
 
 // Config
 ClientConfig::ClientConfig(const envoy::extensions::filters::http::ext_authz::v3::ExtAuthz& config,
-                           uint32_t timeout, absl::string_view path_prefix)
+                           bool internal_timeout, uint32_t timeout, absl::string_view path_prefix)
     : enable_case_sensitive_string_matcher_(Runtime::runtimeFeatureEnabled(
           "envoy.reloadable_features.ext_authz_http_service_enable_case_sensitive_string_matcher")),
       request_header_matchers_(
@@ -141,8 +141,8 @@ ClientConfig::ClientConfig(const envoy::extensions::filters::http::ext_authz::v3
       upstream_header_to_append_matchers_(toUpstreamMatchers(
           config.http_service().authorization_response().allowed_upstream_headers_to_append(),
           enable_case_sensitive_string_matcher_)),
-      cluster_name_(config.http_service().server_uri().cluster()), timeout_(timeout),
-      path_prefix_(path_prefix),
+      cluster_name_(config.http_service().server_uri().cluster()),
+      internal_timeout_(internal_timeout), timeout_(timeout), path_prefix_(path_prefix),
       tracing_name_(fmt::format("async {} egress", config.http_service().server_uri().cluster())),
       request_headers_parser_(Router::HeaderParser::configure(
           config.http_service().authorization_request().headers_to_add(), false)) {}
@@ -212,6 +212,7 @@ void RawHttpClientImpl::cancel() {
   ASSERT(callbacks_ != nullptr);
   request_->cancel();
   callbacks_ = nullptr;
+  timeout_timer_.reset();
 }
 
 // Client
@@ -266,12 +267,19 @@ void RawHttpClientImpl::check(RequestCallbacks& callbacks,
     callbacks_->onComplete(std::make_unique<Response>(errorResponse()));
     callbacks_ = nullptr;
   } else {
+    auto& async_client = cm_.httpAsyncClientForCluster(cluster);
     auto options = Http::AsyncClient::RequestOptions()
-                       .setTimeout(config_->timeout())
                        .setParentSpan(parent_span)
                        .setChildSpanName(config_->tracingName());
 
-    request_ = cm_.httpAsyncClientForCluster(cluster).send(std::move(message), *this, options);
+    if (config_->internalTimeout()) {
+      timeout_timer_ = async_client.dispatcher().createTimer([this]() -> void { onTimeout(); });
+      timeout_timer_->enableTimer(config_->timeout());
+    } else {
+      options.setTimeout(config_->timeout());
+    }
+
+    request_ = async_client.send(std::move(message), *this, options);
   }
 }
 
@@ -300,6 +308,15 @@ void RawHttpClientImpl::onBeforeFinalizeUpstreamSpan(
   }
 }
 
+void RawHttpClientImpl::onTimeout() {
+  ASSERT(request_ != nullptr);
+  request_->cancel();
+  // let the client know of failure:
+  ASSERT(callbacks_ != nullptr);
+  callbacks_->onComplete(std::make_unique<Response>(errorResponse()));
+  callbacks_ = nullptr;
+}
+
 ResponsePtr RawHttpClientImpl::toResponse(Http::ResponseMessagePtr message) {
   const uint64_t status_code = Http::Utility::getResponseStatus(message->headers());
 

source/extensions/filters/common/ext_authz/ext_authz_http_impl.h

@@ -63,7 +63,7 @@ class NotHeaderKeyMatcher : public Matcher {
 class ClientConfig {
 public:
   ClientConfig(const envoy::extensions::filters::http::ext_authz::v3::ExtAuthz& config,
-               uint32_t timeout, absl::string_view path_prefix);
+               bool internal_timeout, uint32_t timeout, absl::string_view path_prefix);
 
   /**
    * Returns the name of the authorization cluster.
@@ -116,6 +116,7 @@ class ClientConfig {
    * Returns the configured request header parser.
    */
   const Router::HeaderParser& requestHeaderParser() const { return *request_headers_parser_; }
+  bool internalTimeout() const { return internal_timeout_; }
 
 private:
   static MatcherSharedPtr
@@ -135,6 +136,7 @@ class ClientConfig {
   const MatcherSharedPtr upstream_header_to_append_matchers_;
   const Http::LowerCaseStrPairVector authorization_headers_to_add_;
   const std::string cluster_name_;
+  const bool internal_timeout_;
   const std::chrono::milliseconds timeout_;
   const std::string path_prefix_;
   const std::string tracing_name_;
@@ -170,12 +172,14 @@ class RawHttpClientImpl : public Client,
                                     const Http::ResponseHeaderMap* response_headers) override;
 
 private:
+  void onTimeout();
   ResponsePtr toResponse(Http::ResponseMessagePtr message);
 
   Upstream::ClusterManager& cm_;
   ClientConfigSharedPtr config_;
   Http::AsyncClient::Request* request_{};
   RequestCallbacks* callbacks_{};
+  Event::TimerPtr timeout_timer_;
 };
 
 } // namespace ExtAuthz

source/extensions/filters/http/ext_authz/config.cc

@@ -9,6 +9,7 @@
 #include "envoy/registry/registry.h"
 
 #include "common/protobuf/utility.h"
+#include "common/runtime/runtime_features.h"
 
 #include "extensions/filters/common/ext_authz/ext_authz_grpc_impl.h"
 #include "extensions/filters/common/ext_authz/ext_authz_http_impl.h"
@@ -27,13 +28,16 @@ Http::FilterFactoryCb ExtAuthzFilterConfig::createFilterFactoryFromProtoTyped(
                                      context.runtime(), context.httpContext(), stats_prefix);
   Http::FilterFactoryCb callback;
 
+  bool internal_timeout = Runtime::runtimeFeatureEnabled(
+      "envoy.reloadable_features.ext_authz_measure_timeout_on_check_created");
+
   if (proto_config.has_http_service()) {
     // Raw HTTP client.
     const uint32_t timeout_ms = PROTOBUF_GET_MS_OR_DEFAULT(proto_config.http_service().server_uri(),
                                                            timeout, DefaultTimeout);
     const auto client_config =
         std::make_shared<Extensions::Filters::Common::ExtAuthz::ClientConfig>(
-            proto_config, timeout_ms, proto_config.http_service().path_prefix());
+            proto_config, internal_timeout, timeout_ms, proto_config.http_service().path_prefix());
     callback = [filter_config, client_config,
                 &context](Http::FilterChainFactoryCallbacks& callbacks) {
       auto client = std::make_unique<Extensions::Filters::Common::ExtAuthz::RawHttpClientImpl>(
@@ -42,18 +46,19 @@ Http::FilterFactoryCb ExtAuthzFilterConfig::createFilterFactoryFromProtoTyped(
           std::make_shared<Filter>(filter_config, std::move(client))});
     };
   } else {
+
     // gRPC client.
     const uint32_t timeout_ms =
         PROTOBUF_GET_MS_OR_DEFAULT(proto_config.grpc_service(), timeout, DefaultTimeout);
     callback = [grpc_service = proto_config.grpc_service(), &context, filter_config, timeout_ms,
                 transport_api_version = proto_config.transport_api_version(),
-                use_alpha = proto_config.hidden_envoy_deprecated_use_alpha()](
-                   Http::FilterChainFactoryCallbacks& callbacks) {
+                use_alpha = proto_config.hidden_envoy_deprecated_use_alpha(),
+                internal_timeout](Http::FilterChainFactoryCallbacks& callbacks) {
       const auto async_client_factory =
           context.clusterManager().grpcAsyncClientManager().factoryForGrpcService(
               grpc_service, context.scope(), true);
       auto client = std::make_unique<Filters::Common::ExtAuthz::GrpcClientImpl>(
-          async_client_factory->create(), std::chrono::milliseconds(timeout_ms),
+          async_client_factory->create(), internal_timeout, std::chrono::milliseconds(timeout_ms),
           transport_api_version, use_alpha);
       callbacks.addStreamDecoderFilter(Http::StreamDecoderFilterSharedPtr{
           std::make_shared<Filter>(filter_config, std::move(client))});

source/extensions/filters/network/ext_authz/config.cc

@@ -34,7 +34,7 @@ Network::FilterFactoryCb ExtAuthzConfigFactory::createFilterFactoryFromProtoType
             grpc_service, context.scope(), true);
 
     auto client = std::make_unique<Filters::Common::ExtAuthz::GrpcClientImpl>(
-        async_client_factory->create(), std::chrono::milliseconds(timeout_ms),
+        async_client_factory->create(), false, std::chrono::milliseconds(timeout_ms),
         transport_api_version, false);
     filter_manager.addReadFilter(Network::ReadFilterSharedPtr{
         std::make_shared<Filter>(ext_authz_config, std::move(client))});