Revert "ext_auth: add option to measure timeout when check request is…

… created. (#12778)" (#14152)

This reverts commit dfa85e8.

Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>

docs/root/configuration/http/http_filters/ext_authz_filter.rst

@@ -165,8 +165,7 @@ The HTTP filter outputs statistics in the *cluster.<route target cluster>.ext_au
   :widths: 1, 1, 2
 
   ok, Counter, Total responses from the filter.
-  error, Counter, Total errors (including timeouts) contacting the external service.
-  timeout, Counter, Total timeouts contacting the external service (only counted when timeout is measured when check request is created).
+  error, Counter, Total errors contacting the external service.
   denied, Counter, Total responses from the authorizations service that were to deny the traffic.
   disabled, Counter, Total requests that are allowed without calling external services due to the filter is disabled.
   failure_mode_allowed, Counter, "Total requests that were error(s) but were allowed through because

source/common/runtime/runtime_features.cc

@@ -111,9 +111,6 @@ constexpr const char* disabled_runtime_features[] = {
     "envoy.reloadable_features.upstream_http2_flood_checks",
     // Sentinel and test flag.
     "envoy.reloadable_features.test_feature_false",
-    // gRPC Timeout header is missing (#13580)
-    "envoy.reloadable_features.ext_authz_measure_timeout_on_check_created",
-
 };
 
 RuntimeFeatures::RuntimeFeatures() {

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

@@ -6,14 +6,12 @@
 #include <vector>
 
 #include "envoy/common/pure.h"
-#include "envoy/event/dispatcher.h"
 #include "envoy/http/codes.h"
 #include "envoy/service/auth/v3/external_auth.pb.h"
 #include "envoy/stream_info/stream_info.h"
 #include "envoy/tracing/http_tracer.h"
 
 #include "common/http/headers.h"
-#include "common/runtime/runtime_features.h"
 #include "common/singleton/const_singleton.h"
 
 namespace Envoy {
@@ -61,27 +59,12 @@ enum class CheckStatus {
   Denied
 };
 
-/**
- * Possible error kind for Error status..
- */
-enum class ErrorKind {
-  // Other error.
-  Other,
-  // The request timed out. This will only be set if the timeout is measure when the check request
-  // was created.
-  Timedout,
-};
-
 /**
  * Authorization response object for a RequestCallback.
  */
 struct Response {
   // Call status.
   CheckStatus status;
-
-  // In case status is Error, this will contain the kind of error that occurred.
-  ErrorKind error_kind{ErrorKind::Other};
-
   // A set of HTTP headers returned by the authorization server, that will be optionally appended
   // to the request to the upstream server.
   Http::HeaderVector headers_to_append;
@@ -134,23 +117,13 @@ class Client {
    * passed request parameters to make a permit/deny decision.
    * @param callback supplies the completion callbacks.
    *        NOTE: The callback may happen within the calling stack.
-   * @param dispatcher is the dispatcher of the current thread.
    * @param request is the proto message with the attributes of the specific payload.
    * @param parent_span source for generating an egress child span as part of the trace.
    * @param stream_info supplies the client's stream info.
    */
-  virtual void check(RequestCallbacks& callback, Event::Dispatcher& dispatcher,
+  virtual void check(RequestCallbacks& callback,
                      const envoy::service::auth::v3::CheckRequest& request,
                      Tracing::Span& parent_span, const StreamInfo::StreamInfo& stream_info) PURE;
-
-protected:
-  /**
-   * @return should we start the request time out when the check request is created.
-   */
-  static bool timeoutStartsAtCheckCreation() {
-    return Runtime::runtimeFeatureEnabled(
-        "envoy.reloadable_features.ext_authz_measure_timeout_on_check_created");
-  }
 };
 
 using ClientPtr = std::unique_ptr<Client>;

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

@@ -31,29 +31,15 @@ void GrpcClientImpl::cancel() {
   ASSERT(callbacks_ != nullptr);
   request_->cancel();
   callbacks_ = nullptr;
-  timeout_timer_.reset();
 }
 
-void GrpcClientImpl::check(RequestCallbacks& callbacks, Event::Dispatcher& dispatcher,
+void GrpcClientImpl::check(RequestCallbacks& callbacks,
                            const envoy::service::auth::v3::CheckRequest& request,
                            Tracing::Span& parent_span, const StreamInfo::StreamInfo& stream_info) {
   ASSERT(callbacks_ == nullptr);
   callbacks_ = &callbacks;
-
   Http::AsyncClient::RequestOptions options;
-  if (timeout_.has_value()) {
-    if (timeoutStartsAtCheckCreation()) {
-      // TODO(yuval-k): We currently use dispatcher based timeout even if the underlying client is
-      // Google gRPC client, which has its own timeout mechanism. We may want to change that in the
-      // future if the implementations converge.
-      timeout_timer_ = dispatcher.createTimer([this]() -> void { onTimeout(); });
-      timeout_timer_->enableTimer(timeout_.value());
-    } else {
-      // not starting timer on check creation, set the timeout on the request.
-      options.setTimeout(timeout_);
-    }
-  }
-
+  options.setTimeout(timeout_);
   options.setParentContext(Http::AsyncClient::ParentContext{&stream_info});
 
   ENVOY_LOG(trace, "Sending CheckRequest: {}", request.DebugString());
@@ -99,31 +85,16 @@ void GrpcClientImpl::onSuccess(std::unique_ptr<envoy::service::auth::v3::CheckRe
 
   callbacks_->onComplete(std::move(authz_response));
   callbacks_ = nullptr;
-  timeout_timer_.reset();
 }
 
 void GrpcClientImpl::onFailure(Grpc::Status::GrpcStatus status, const std::string&,
                                Tracing::Span&) {
   ENVOY_LOG(trace, "CheckRequest call failed with status: {}",
             Grpc::Utility::grpcStatusToString(status));
   ASSERT(status != Grpc::Status::WellKnownGrpcStatus::Ok);
-  timeout_timer_.reset();
-  respondFailure(ErrorKind::Other);
-}
-
-void GrpcClientImpl::onTimeout() {
-  ENVOY_LOG(trace, "CheckRequest timed-out");
-  ASSERT(request_ != nullptr);
-  request_->cancel();
-  // let the client know of failure:
-  respondFailure(ErrorKind::Timedout);
-}
-
-void GrpcClientImpl::respondFailure(ErrorKind kind) {
   Response response{};
   response.status = CheckStatus::Error;
   response.status_code = Http::Code::Forbidden;
-  response.error_kind = kind;
   callbacks_->onComplete(std::make_unique<Response>(response));
   callbacks_ = nullptr;
 }

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

@@ -51,9 +51,8 @@ class GrpcClientImpl : public Client,
 
   // ExtAuthz::Client
   void cancel() override;
-  void check(RequestCallbacks& callbacks, Event::Dispatcher& dispatcher,
-             const envoy::service::auth::v3::CheckRequest& request, Tracing::Span& parent_span,
-             const StreamInfo::StreamInfo& stream_info) override;
+  void check(RequestCallbacks& callbacks, const envoy::service::auth::v3::CheckRequest& request,
+             Tracing::Span& parent_span, const StreamInfo::StreamInfo& stream_info) override;
 
   // Grpc::AsyncRequestCallbacks
   void onCreateInitialMetadata(Http::RequestHeaderMap&) override {}
@@ -63,20 +62,16 @@ class GrpcClientImpl : public Client,
                  Tracing::Span& span) override;
 
 private:
-  void onTimeout();
-  void respondFailure(Filters::Common::ExtAuthz::ErrorKind kind);
   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_{};
   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

@@ -32,7 +32,6 @@ const Http::HeaderMap& lengthZeroHeader() {
 // Static response used for creating authorization ERROR responses.
 const Response& errorResponse() {
   CONSTRUCT_ON_FIRST_USE(Response, Response{CheckStatus::Error,
-                                            ErrorKind::Other,
                                             Http::HeaderVector{},
                                             Http::HeaderVector{},
                                             Http::HeaderVector{},
@@ -176,11 +175,10 @@ void RawHttpClientImpl::cancel() {
   ASSERT(callbacks_ != nullptr);
   request_->cancel();
   callbacks_ = nullptr;
-  timeout_timer_.reset();
 }
 
 // Client
-void RawHttpClientImpl::check(RequestCallbacks& callbacks, Event::Dispatcher& dispatcher,
+void RawHttpClientImpl::check(RequestCallbacks& callbacks,
                               const envoy::service::auth::v3::CheckRequest& request,
                               Tracing::Span& parent_span,
                               const StreamInfo::StreamInfo& stream_info) {
@@ -231,31 +229,23 @@ void RawHttpClientImpl::check(RequestCallbacks& callbacks, Event::Dispatcher& di
     callbacks_ = nullptr;
   } else {
     auto options = Http::AsyncClient::RequestOptions()
+                       .setTimeout(config_->timeout())
                        .setParentSpan(parent_span)
                        .setChildSpanName(config_->tracingName());
 
-    if (timeoutStartsAtCheckCreation()) {
-      timeout_timer_ = dispatcher.createTimer([this]() -> void { onTimeout(); });
-      timeout_timer_->enableTimer(config_->timeout());
-    } else {
-      options.setTimeout(config_->timeout());
-    }
-
     request_ = cm_.httpAsyncClientForCluster(cluster).send(std::move(message), *this, options);
   }
 }
 
 void RawHttpClientImpl::onSuccess(const Http::AsyncClient::Request&,
                                   Http::ResponseMessagePtr&& message) {
-  timeout_timer_.reset();
   callbacks_->onComplete(toResponse(std::move(message)));
   callbacks_ = nullptr;
 }
 
 void RawHttpClientImpl::onFailure(const Http::AsyncClient::Request&,
                                   Http::AsyncClient::FailureReason reason) {
   ASSERT(reason == Http::AsyncClient::FailureReason::Reset);
-  timeout_timer_.reset();
   callbacks_->onComplete(std::make_unique<Response>(errorResponse()));
   callbacks_ = nullptr;
 }
@@ -272,18 +262,6 @@ void RawHttpClientImpl::onBeforeFinalizeUpstreamSpan(
   }
 }
 
-void RawHttpClientImpl::onTimeout() {
-  ENVOY_LOG(trace, "CheckRequest timed-out");
-  ASSERT(request_ != nullptr);
-  request_->cancel();
-  // let the client know of failure:
-  ASSERT(callbacks_ != nullptr);
-  Response response = errorResponse();
-  response.error_kind = ErrorKind::Timedout;
-  callbacks_->onComplete(std::make_unique<Response>(response));
-  callbacks_ = nullptr;
-}
-
 ResponsePtr RawHttpClientImpl::toResponse(Http::ResponseMessagePtr message) {
   const uint64_t status_code = Http::Utility::getResponseStatus(message->headers());
 
@@ -323,18 +301,16 @@ ResponsePtr RawHttpClientImpl::toResponse(Http::ResponseMessagePtr message) {
   if (status_code == enumToInt(Http::Code::OK)) {
     SuccessResponse ok{message->headers(), config_->upstreamHeaderMatchers(),
                        config_->upstreamHeaderToAppendMatchers(),
-                       Response{CheckStatus::OK, ErrorKind::Other, Http::HeaderVector{},
-                                Http::HeaderVector{}, Http::HeaderVector{},
-                                std::move(headers_to_remove), EMPTY_STRING, Http::Code::OK,
-                                ProtobufWkt::Struct{}}};
+                       Response{CheckStatus::OK, Http::HeaderVector{}, Http::HeaderVector{},
+                                Http::HeaderVector{}, std::move(headers_to_remove), EMPTY_STRING,
+                                Http::Code::OK, ProtobufWkt::Struct{}}};
     return std::move(ok.response_);
   }
 
   // Create a Denied authorization response.
   SuccessResponse denied{message->headers(), config_->clientHeaderMatchers(),
                          config_->upstreamHeaderToAppendMatchers(),
                          Response{CheckStatus::Denied,
-                                  ErrorKind::Other,
                                   Http::HeaderVector{},
                                   Http::HeaderVector{},
                                   Http::HeaderVector{},

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

@@ -154,9 +154,8 @@ class RawHttpClientImpl : public Client,
 
   // ExtAuthz::Client
   void cancel() override;
-  void check(RequestCallbacks& callbacks, Event::Dispatcher& dispatcher,
-             const envoy::service::auth::v3::CheckRequest& request, Tracing::Span& parent_span,
-             const StreamInfo::StreamInfo& stream_info) override;
+  void check(RequestCallbacks& callbacks, const envoy::service::auth::v3::CheckRequest& request,
+             Tracing::Span& parent_span, const StreamInfo::StreamInfo& stream_info) override;
 
   // Http::AsyncClient::Callbacks
   void onSuccess(const Http::AsyncClient::Request&, Http::ResponseMessagePtr&& message) override;
@@ -166,14 +165,12 @@ 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

@@ -10,7 +10,6 @@
 
 #include "common/grpc/google_async_client_cache.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"

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

@@ -72,8 +72,7 @@ void Filter::initiateCall(const Http::RequestHeaderMap& headers,
                                                // going to invoke check call.
   cluster_ = callbacks_->clusterInfo();
   initiating_call_ = true;
-  client_->check(*this, callbacks_->dispatcher(), check_request_, callbacks_->activeSpan(),
-                 callbacks_->streamInfo());
+  client_->check(*this, check_request_, callbacks_->activeSpan(), callbacks_->streamInfo());
   initiating_call_ = false;
 }
 
@@ -281,14 +280,8 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) {
   case CheckStatus::Error: {
     if (cluster_) {
       config_->incCounter(cluster_->statsScope(), config_->ext_authz_error_);
-      if (response->error_kind == Filters::Common::ExtAuthz::ErrorKind::Timedout) {
-        config_->incCounter(cluster_->statsScope(), config_->ext_authz_timeout_);
-      }
     }
     stats_.error_.inc();
-    if (response->error_kind == Filters::Common::ExtAuthz::ErrorKind::Timedout) {
-      stats_.timeout_.inc();
-    }
     if (config_->failureModeAllow()) {
       ENVOY_STREAM_LOG(trace, "ext_authz filter allowed the request with error", *callbacks_);
       stats_.failure_mode_allowed_.inc();

source/extensions/filters/http/ext_authz/ext_authz.h

@@ -37,7 +37,6 @@ namespace ExtAuthz {
   COUNTER(ok)                                                                                      \
   COUNTER(denied)                                                                                  \
   COUNTER(error)                                                                                   \
-  COUNTER(timeout)                                                                                 \
   COUNTER(disabled)                                                                                \
   COUNTER(failure_mode_allowed)
 
@@ -83,7 +82,6 @@ class FilterConfig {
         ext_authz_ok_(pool_.add(createPoolStatName(config.stat_prefix(), "ok"))),
         ext_authz_denied_(pool_.add(createPoolStatName(config.stat_prefix(), "denied"))),
         ext_authz_error_(pool_.add(createPoolStatName(config.stat_prefix(), "error"))),
-        ext_authz_timeout_(pool_.add(createPoolStatName(config.stat_prefix(), "timeout"))),
         ext_authz_failure_mode_allowed_(
             pool_.add(createPoolStatName(config.stat_prefix(), "failure_mode_allowed"))) {}
 
@@ -183,7 +181,6 @@ class FilterConfig {
   const Stats::StatName ext_authz_ok_;
   const Stats::StatName ext_authz_denied_;
   const Stats::StatName ext_authz_error_;
-  const Stats::StatName ext_authz_timeout_;
   const Stats::StatName ext_authz_failure_mode_allowed_;
 };
 

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

@@ -30,9 +30,8 @@ void Filter::callCheck() {
   config_->stats().total_.inc();
 
   calling_check_ = true;
-  auto& connection = filter_callbacks_->connection();
-  client_->check(*this, connection.dispatcher(), check_request_, Tracing::NullSpan::instance(),
-                 connection.streamInfo());
+  client_->check(*this, check_request_, Tracing::NullSpan::instance(),
+                 filter_callbacks_->connection().streamInfo());
   calling_check_ = false;
 }
 
@@ -78,9 +77,6 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) {
     break;
   case Filters::Common::ExtAuthz::CheckStatus::Error:
     config_->stats().error_.inc();
-    if (response->error_kind == Filters::Common::ExtAuthz::ErrorKind::Timedout) {
-      config_->stats().timeout_.inc();
-    }
     break;
   case Filters::Common::ExtAuthz::CheckStatus::Denied:
     config_->stats().denied_.inc();

source/extensions/filters/network/ext_authz/ext_authz.h

@@ -31,7 +31,6 @@ namespace ExtAuthz {
   COUNTER(cx_closed)                                                                               \
   COUNTER(denied)                                                                                  \
   COUNTER(error)                                                                                   \
-  COUNTER(timeout)                                                                                 \
   COUNTER(failure_mode_allowed)                                                                    \
   COUNTER(ok)                                                                                      \
   COUNTER(total)                                                                                   \