diff --git a/docs/root/configuration/http/http_filters/ext_authz_filter.rst b/docs/root/configuration/http/http_filters/ext_authz_filter.rst index b66ef07cb464d..bd16c35bcc459 100644 --- a/docs/root/configuration/http/http_filters/ext_authz_filter.rst +++ b/docs/root/configuration/http/http_filters/ext_authz_filter.rst @@ -133,7 +133,8 @@ The HTTP filter outputs statistics in the *cluster..ext_au :widths: 1, 1, 2 ok, Counter, Total responses from the filter. - error, Counter, Total errors contacting the external service. + 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). denied, Counter, Total responses from the authorizations service that were to deny the traffic. failure_mode_allowed, Counter, "Total requests that were error(s) but were allowed through because of failure_mode_allow set to true." diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 3aac2f958f239..60c90b68cc7ab 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -15,6 +15,9 @@ 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. When enabled, a new `ext_authz.timeout` stat is counted when timeout occurs. See :ref:`stats + // `. * http: added :ref:`contains ` a new string matcher type which matches if the value of the string has the substring mentioned in contains matcher. * http: added :ref:`contains ` 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 ` to :ref:`local reply mapper ` to allow its users to add/append/override response HTTP headers to local replies. diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 9070056802837..598f3284aeb5c 100644 --- a/source/common/runtime/runtime_features.cc +++ b/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", diff --git a/source/extensions/filters/common/ext_authz/ext_authz.h b/source/extensions/filters/common/ext_authz/ext_authz.h index ba34d2e8a9fcb..b67c34c526d6c 100644 --- a/source/extensions/filters/common/ext_authz/ext_authz.h +++ b/source/extensions/filters/common/ext_authz/ext_authz.h @@ -6,11 +6,13 @@ #include #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/runtime/runtime_features.h" #include "common/singleton/const_singleton.h" namespace Envoy { @@ -43,12 +45,27 @@ 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; @@ -98,13 +115,23 @@ 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, + virtual void check(RequestCallbacks& callback, Event::Dispatcher& dispatcher, 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; 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 index 2f720ca26f8ab..28d5d62629b4d 100644 --- a/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc +++ b/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc @@ -34,17 +34,31 @@ void GrpcClientImpl::cancel() { ASSERT(callbacks_ != nullptr); request_->cancel(); callbacks_ = nullptr; + timeout_timer_.reset(); } -void GrpcClientImpl::check(RequestCallbacks& callbacks, +void GrpcClientImpl::check(RequestCallbacks& callbacks, Event::Dispatcher& dispatcher, const envoy::service::auth::v3::CheckRequest& request, Tracing::Span& parent_span, const StreamInfo::StreamInfo&) { 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 it's 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_); + } + } + 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_); } @@ -81,6 +95,7 @@ void GrpcClientImpl::onSuccess(std::unique_ptronComplete(std::move(authz_response)); callbacks_ = nullptr; + timeout_timer_.reset(); } void GrpcClientImpl::onFailure(Grpc::Status::GrpcStatus status, const std::string&, @@ -88,9 +103,23 @@ 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); + 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)); callbacks_ = nullptr; } 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 index da1ed1d2ebf1e..626a17eb2bb70 100644 --- a/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.h +++ b/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.h @@ -51,8 +51,9 @@ class GrpcClientImpl : public Client, // ExtAuthz::Client void cancel() override; - void check(RequestCallbacks& callbacks, const envoy::service::auth::v3::CheckRequest& request, - Tracing::Span& parent_span, const StreamInfo::StreamInfo& stream_info) 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; // Grpc::AsyncRequestCallbacks void onCreateInitialMetadata(Http::RequestHeaderMap&) override {} @@ -62,9 +63,12 @@ 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& headers); + Grpc::AsyncClient async_client_; Grpc::AsyncRequest* request_{}; @@ -72,6 +76,7 @@ class GrpcClientImpl : public Client, 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; 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 index fe7207bdfbd32..77d253ba77c9c 100644 --- a/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc +++ b/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc @@ -32,9 +32,9 @@ const Http::HeaderMap& lengthZeroHeader() { // Static response used for creating authorization ERROR responses. const Response& errorResponse() { CONSTRUCT_ON_FIRST_USE(Response, - Response{CheckStatus::Error, Http::HeaderVector{}, Http::HeaderVector{}, - Http::HeaderVector{}, EMPTY_STRING, Http::Code::Forbidden, - ProtobufWkt::Struct{}}); + Response{CheckStatus::Error, ErrorKind::Other, Http::HeaderVector{}, + Http::HeaderVector{}, Http::HeaderVector{}, EMPTY_STRING, + Http::Code::Forbidden, ProtobufWkt::Struct{}}); } // SuccessResponse used for creating either DENIED or OK authorization responses. @@ -212,10 +212,11 @@ void RawHttpClientImpl::cancel() { ASSERT(callbacks_ != nullptr); request_->cancel(); callbacks_ = nullptr; + timeout_timer_.reset(); } // Client -void RawHttpClientImpl::check(RequestCallbacks& callbacks, +void RawHttpClientImpl::check(RequestCallbacks& callbacks, Event::Dispatcher& dispatcher, const envoy::service::auth::v3::CheckRequest& request, Tracing::Span& parent_span, const StreamInfo::StreamInfo& stream_info) { @@ -267,16 +268,23 @@ void RawHttpClientImpl::check(RequestCallbacks& callbacks, 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; } @@ -284,6 +292,7 @@ void RawHttpClientImpl::onSuccess(const Http::AsyncClient::Request&, 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(errorResponse())); callbacks_ = nullptr; } @@ -300,6 +309,18 @@ 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)); + callbacks_ = nullptr; +} + ResponsePtr RawHttpClientImpl::toResponse(Http::ResponseMessagePtr message) { const uint64_t status_code = Http::Utility::getResponseStatus(message->headers()); @@ -314,18 +335,19 @@ ResponsePtr RawHttpClientImpl::toResponse(Http::ResponseMessagePtr message) { if (status_code == enumToInt(Http::Code::OK)) { SuccessResponse ok{message->headers(), config_->upstreamHeaderMatchers(), config_->upstreamHeaderToAppendMatchers(), - Response{CheckStatus::OK, Http::HeaderVector{}, Http::HeaderVector{}, - Http::HeaderVector{}, EMPTY_STRING, Http::Code::OK, - ProtobufWkt::Struct{}}}; + Response{CheckStatus::OK, ErrorKind::Other, Http::HeaderVector{}, + Http::HeaderVector{}, Http::HeaderVector{}, 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, Http::HeaderVector{}, Http::HeaderVector{}, - Http::HeaderVector{}, message->bodyAsString(), - static_cast(status_code), ProtobufWkt::Struct{}}}; + Response{CheckStatus::Denied, ErrorKind::Other, Http::HeaderVector{}, + Http::HeaderVector{}, Http::HeaderVector{}, + message->bodyAsString(), static_cast(status_code), + ProtobufWkt::Struct{}}}; 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 index 8f5abd684379f..5f16fbc6890c8 100644 --- a/source/extensions/filters/common/ext_authz/ext_authz_http_impl.h +++ b/source/extensions/filters/common/ext_authz/ext_authz_http_impl.h @@ -159,8 +159,9 @@ class RawHttpClientImpl : public Client, // ExtAuthz::Client void cancel() override; - void check(RequestCallbacks& callbacks, const envoy::service::auth::v3::CheckRequest& request, - Tracing::Span& parent_span, const StreamInfo::StreamInfo& stream_info) 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; // Http::AsyncClient::Callbacks void onSuccess(const Http::AsyncClient::Request&, Http::ResponseMessagePtr&& message) override; @@ -170,12 +171,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 diff --git a/source/extensions/filters/http/ext_authz/config.cc b/source/extensions/filters/http/ext_authz/config.cc index f5808ee7fdf5a..bf789d0ca94f4 100644 --- a/source/extensions/filters/http/ext_authz/config.cc +++ b/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" diff --git a/source/extensions/filters/http/ext_authz/ext_authz.cc b/source/extensions/filters/http/ext_authz/ext_authz.cc index 156cdf3d11afc..a3ddccc497901 100644 --- a/source/extensions/filters/http/ext_authz/ext_authz.cc +++ b/source/extensions/filters/http/ext_authz/ext_authz.cc @@ -69,7 +69,8 @@ void Filter::initiateCall(const Http::RequestHeaderMap& headers, // going to invoke check call. cluster_ = callbacks_->clusterInfo(); initiating_call_ = true; - client_->check(*this, check_request_, callbacks_->activeSpan(), callbacks_->streamInfo()); + client_->check(*this, callbacks_->dispatcher(), check_request_, callbacks_->activeSpan(), + callbacks_->streamInfo()); initiating_call_ = false; } @@ -258,8 +259,14 @@ 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(); diff --git a/source/extensions/filters/http/ext_authz/ext_authz.h b/source/extensions/filters/http/ext_authz/ext_authz.h index 9a24436c7c183..55e35d24709c7 100644 --- a/source/extensions/filters/http/ext_authz/ext_authz.h +++ b/source/extensions/filters/http/ext_authz/ext_authz.h @@ -38,6 +38,7 @@ namespace ExtAuthz { COUNTER(ok) \ COUNTER(denied) \ COUNTER(error) \ + COUNTER(timeout) \ COUNTER(failure_mode_allowed) /** @@ -76,6 +77,7 @@ class FilterConfig { stats_(generateStats(stats_prefix, scope)), ext_authz_ok_(pool_.add("ext_authz.ok")), ext_authz_denied_(pool_.add("ext_authz.denied")), ext_authz_error_(pool_.add("ext_authz.error")), + ext_authz_timeout_(pool_.add("ext_authz.timeout")), ext_authz_failure_mode_allowed_(pool_.add("ext_authz.failure_mode_allowed")) {} bool allowPartialMessage() const { return allow_partial_message_; } @@ -154,6 +156,7 @@ 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_; }; diff --git a/source/extensions/filters/network/ext_authz/ext_authz.cc b/source/extensions/filters/network/ext_authz/ext_authz.cc index 97feb62e0d22d..bf0fa8fef1543 100644 --- a/source/extensions/filters/network/ext_authz/ext_authz.cc +++ b/source/extensions/filters/network/ext_authz/ext_authz.cc @@ -30,8 +30,9 @@ void Filter::callCheck() { config_->stats().total_.inc(); calling_check_ = true; - client_->check(*this, check_request_, Tracing::NullSpan::instance(), - filter_callbacks_->connection().streamInfo()); + auto& connection = filter_callbacks_->connection(); + client_->check(*this, connection.dispatcher(), check_request_, Tracing::NullSpan::instance(), + connection.streamInfo()); calling_check_ = false; } @@ -72,6 +73,9 @@ 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(); diff --git a/source/extensions/filters/network/ext_authz/ext_authz.h b/source/extensions/filters/network/ext_authz/ext_authz.h index 9fa49f0f3bb7a..18e30ba197f69 100644 --- a/source/extensions/filters/network/ext_authz/ext_authz.h +++ b/source/extensions/filters/network/ext_authz/ext_authz.h @@ -29,6 +29,7 @@ namespace ExtAuthz { COUNTER(cx_closed) \ COUNTER(denied) \ COUNTER(error) \ + COUNTER(timeout) \ COUNTER(failure_mode_allowed) \ COUNTER(ok) \ COUNTER(total) \ diff --git a/test/extensions/filters/common/ext_authz/BUILD b/test/extensions/filters/common/ext_authz/BUILD index 472c9a54f8c56..8ee2d19ecb892 100644 --- a/test/extensions/filters/common/ext_authz/BUILD +++ b/test/extensions/filters/common/ext_authz/BUILD @@ -32,6 +32,7 @@ envoy_cc_test( "//source/extensions/filters/common/ext_authz:ext_authz_grpc_lib", "//test/extensions/filters/common/ext_authz:ext_authz_test_common", "//test/mocks/tracing:tracing_mocks", + "//test/test_common:test_runtime_lib", "@envoy_api//envoy/service/auth/v2alpha:pkg_cc_proto", "@envoy_api//envoy/service/auth/v3:pkg_cc_proto", "@envoy_api//envoy/type/v3:pkg_cc_proto", @@ -46,6 +47,7 @@ envoy_cc_test( "//test/extensions/filters/common/ext_authz:ext_authz_test_common", "//test/mocks/stream_info:stream_info_mocks", "//test/mocks/upstream:cluster_manager_mocks", + "//test/test_common:test_runtime_lib", "@envoy_api//envoy/extensions/filters/http/ext_authz/v3:pkg_cc_proto", "@envoy_api//envoy/service/auth/v3:pkg_cc_proto", ], diff --git a/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc b/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc index 46dbfdd4cfe2f..1e289f6df7164 100644 --- a/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc +++ b/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc @@ -13,6 +13,7 @@ #include "test/mocks/grpc/mocks.h" #include "test/mocks/stream_info/mocks.h" #include "test/mocks/tracing/mocks.h" +#include "test/test_common/test_runtime.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -55,7 +56,12 @@ class ExtAuthzGrpcClientTest : public testing::TestWithParam { "envoy.service.auth.{}.Authorization", api_version_, use_alpha_), service_full_name); EXPECT_EQ("Check", method_name); - EXPECT_EQ(timeout_->count(), options.timeout->count()); + if (Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.ext_authz_measure_timeout_on_check_created")) { + EXPECT_FALSE(options.timeout.has_value()); + } else { + EXPECT_EQ(timeout_->count(), options.timeout->count()); + } return &async_request_; })); } @@ -65,6 +71,7 @@ class ExtAuthzGrpcClientTest : public testing::TestWithParam { Grpc::MockAsyncRequest async_request_; GrpcClientImplPtr client_; MockRequestCallbacks request_callbacks_; + NiceMock dispatcher_; Tracing::MockSpan span_; bool use_alpha_{}; NiceMock stream_info_; @@ -102,9 +109,15 @@ TEST_P(ExtAuthzGrpcClientTest, AuthorizationOk) { authz_response.dynamic_metadata = expected_dynamic_metadata; + NiceMock* timer = new NiceMock(&dispatcher_); + EXPECT_CALL(*timer, enableTimer(timeout_.value(), _)); + bool timer_destroyed = false; + timer->timer_destroyed_ = &timer_destroyed; + envoy::service::auth::v3::CheckRequest request; expectCallSend(request); - client_->check(request_callbacks_, request, Tracing::NullSpan::instance(), stream_info_); + client_->check(request_callbacks_, dispatcher_, request, Tracing::NullSpan::instance(), + stream_info_); Http::TestRequestHeaderMapImpl headers; client_->onCreateInitialMetadata(headers); @@ -113,6 +126,8 @@ TEST_P(ExtAuthzGrpcClientTest, AuthorizationOk) { EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo( AuthzResponseNoAttributes(authz_response)))); client_->onSuccess(std::move(check_response), span_); + // make sure the internal timeout timer is destroyed + EXPECT_EQ(timer_destroyed, true); } // Test the client when an ok response is received. @@ -128,7 +143,8 @@ TEST_P(ExtAuthzGrpcClientTest, AuthorizationOkWithAllAtributes) { envoy::service::auth::v3::CheckRequest request; expectCallSend(request); - client_->check(request_callbacks_, request, Tracing::NullSpan::instance(), stream_info_); + client_->check(request_callbacks_, dispatcher_, request, Tracing::NullSpan::instance(), + stream_info_); Http::TestRequestHeaderMapImpl headers; client_->onCreateInitialMetadata(headers); @@ -151,7 +167,8 @@ TEST_P(ExtAuthzGrpcClientTest, AuthorizationDenied) { envoy::service::auth::v3::CheckRequest request; expectCallSend(request); - client_->check(request_callbacks_, request, Tracing::NullSpan::instance(), stream_info_); + client_->check(request_callbacks_, dispatcher_, request, Tracing::NullSpan::instance(), + stream_info_); Http::TestRequestHeaderMapImpl headers; client_->onCreateInitialMetadata(headers); @@ -175,7 +192,8 @@ TEST_P(ExtAuthzGrpcClientTest, AuthorizationDeniedGrpcUnknownStatus) { envoy::service::auth::v3::CheckRequest request; expectCallSend(request); - client_->check(request_callbacks_, request, Tracing::NullSpan::instance(), stream_info_); + client_->check(request_callbacks_, dispatcher_, request, Tracing::NullSpan::instance(), + stream_info_); Http::TestRequestHeaderMapImpl headers; client_->onCreateInitialMetadata(headers); @@ -202,7 +220,8 @@ TEST_P(ExtAuthzGrpcClientTest, AuthorizationDeniedWithAllAttributes) { envoy::service::auth::v3::CheckRequest request; expectCallSend(request); - client_->check(request_callbacks_, request, Tracing::NullSpan::instance(), stream_info_); + client_->check(request_callbacks_, dispatcher_, request, Tracing::NullSpan::instance(), + stream_info_); Http::TestRequestHeaderMapImpl headers; client_->onCreateInitialMetadata(headers); @@ -218,13 +237,22 @@ TEST_P(ExtAuthzGrpcClientTest, AuthorizationDeniedWithAllAttributes) { TEST_P(ExtAuthzGrpcClientTest, UnknownError) { initialize(GetParam()); + NiceMock* timer = new NiceMock(&dispatcher_); + EXPECT_CALL(*timer, enableTimer(timeout_.value(), _)); + bool timer_destroyed = false; + timer->timer_destroyed_ = &timer_destroyed; + envoy::service::auth::v3::CheckRequest request; expectCallSend(request); - client_->check(request_callbacks_, request, Tracing::NullSpan::instance(), stream_info_); + client_->check(request_callbacks_, dispatcher_, request, Tracing::NullSpan::instance(), + stream_info_); EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo(AuthzErrorResponse(CheckStatus::Error)))); client_->onFailure(Grpc::Status::Unknown, "", span_); + + // make sure the internal timeout timer is destroyed + EXPECT_EQ(timer_destroyed, true); } // Test the client when the request is canceled. @@ -233,7 +261,8 @@ TEST_P(ExtAuthzGrpcClientTest, CancelledAuthorizationRequest) { envoy::service::auth::v3::CheckRequest request; EXPECT_CALL(*async_client_, sendRaw(_, _, _, _, _, _)).WillOnce(Return(&async_request_)); - client_->check(request_callbacks_, request, Tracing::NullSpan::instance(), stream_info_); + client_->check(request_callbacks_, dispatcher_, request, Tracing::NullSpan::instance(), + stream_info_); EXPECT_CALL(async_request_, cancel()); client_->cancel(); @@ -241,17 +270,69 @@ TEST_P(ExtAuthzGrpcClientTest, CancelledAuthorizationRequest) { // Test the client when the request times out. TEST_P(ExtAuthzGrpcClientTest, AuthorizationRequestTimeout) { + TestScopedRuntime scoped_runtime; + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.ext_authz_measure_timeout_on_check_created", "false"}}); initialize(GetParam()); envoy::service::auth::v3::CheckRequest request; expectCallSend(request); - client_->check(request_callbacks_, request, Tracing::NullSpan::instance(), stream_info_); + client_->check(request_callbacks_, dispatcher_, request, Tracing::NullSpan::instance(), + stream_info_); EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo(AuthzErrorResponse(CheckStatus::Error)))); client_->onFailure(Grpc::Status::DeadlineExceeded, "", span_); } +// Test the client when the request times out on an internal timeout. +TEST_P(ExtAuthzGrpcClientTest, AuthorizationInternalRequestTimeout) { + initialize(GetParam()); + TestScopedRuntime scoped_runtime; + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.ext_authz_measure_timeout_on_check_created", "true"}}); + + NiceMock* timer = new NiceMock(&dispatcher_); + EXPECT_CALL(*timer, enableTimer(timeout_.value(), _)); + + envoy::service::auth::v3::CheckRequest request; + expectCallSend(request); + + client_->check(request_callbacks_, dispatcher_, request, Tracing::NullSpan::instance(), + stream_info_); + + EXPECT_CALL(async_request_, cancel()); + EXPECT_CALL(request_callbacks_, + onComplete_(WhenDynamicCastTo(AuthzTimedoutResponse()))); + timer->invokeCallback(); +} + +// Test when the client is cancelled with internal timeout. +TEST_P(ExtAuthzGrpcClientTest, AuthorizationInternalRequestTimeoutCancelled) { + TestScopedRuntime scoped_runtime; + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.ext_authz_measure_timeout_on_check_created", "true"}}); + + initialize(GetParam()); + + NiceMock* timer = new NiceMock(&dispatcher_); + EXPECT_CALL(*timer, enableTimer(timeout_.value(), _)); + + envoy::service::auth::v3::CheckRequest request; + expectCallSend(request); + + client_->check(request_callbacks_, dispatcher_, request, Tracing::NullSpan::instance(), + stream_info_); + + EXPECT_CALL(async_request_, cancel()); + EXPECT_CALL(request_callbacks_, onComplete_(_)).Times(0); + // make sure cancel resets the timer: + bool timer_destroyed = false; + timer->timer_destroyed_ = &timer_destroyed; + client_->cancel(); + EXPECT_EQ(timer_destroyed, true); +} + // Test the client when an OK response is received with dynamic metadata in that OK response. TEST_P(ExtAuthzGrpcClientTest, AuthorizationOkWithDynamicMetadata) { initialize(GetParam()); @@ -280,7 +361,8 @@ TEST_P(ExtAuthzGrpcClientTest, AuthorizationOkWithDynamicMetadata) { envoy::service::auth::v3::CheckRequest request; expectCallSend(request); - client_->check(request_callbacks_, request, Tracing::NullSpan::instance(), stream_info_); + client_->check(request_callbacks_, dispatcher_, request, Tracing::NullSpan::instance(), + stream_info_); Http::TestRequestHeaderMapImpl headers; client_->onCreateInitialMetadata(headers); diff --git a/test/extensions/filters/common/ext_authz/ext_authz_http_impl_test.cc b/test/extensions/filters/common/ext_authz/ext_authz_http_impl_test.cc index 0695251a0735d..78397b3260885 100644 --- a/test/extensions/filters/common/ext_authz/ext_authz_http_impl_test.cc +++ b/test/extensions/filters/common/ext_authz/ext_authz_http_impl_test.cc @@ -13,6 +13,7 @@ #include "test/extensions/filters/common/ext_authz/test_common.h" #include "test/mocks/stream_info/mocks.h" #include "test/mocks/upstream/cluster_manager.h" +#include "test/test_common/test_runtime.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -33,6 +34,8 @@ namespace Common { namespace ExtAuthz { namespace { +constexpr uint32_t REQUEST_TIMEOUT{250}; + class ExtAuthzHttpClientTest : public testing::Test { public: ExtAuthzHttpClientTest() : async_request_{&async_client_} { initialize(EMPTY_STRING); } @@ -44,7 +47,8 @@ class ExtAuthzHttpClientTest : public testing::Test { .WillByDefault(ReturnRef(async_client_)); } - ClientConfigSharedPtr createConfig(const std::string& yaml = EMPTY_STRING, uint32_t timeout = 250, + ClientConfigSharedPtr createConfig(const std::string& yaml = EMPTY_STRING, + uint32_t timeout = REQUEST_TIMEOUT, const std::string& path_prefix = "/bar") { envoy::extensions::filters::http::ext_authz::v3::ExtAuthz proto_config{}; if (yaml.empty()) { @@ -95,7 +99,6 @@ class ExtAuthzHttpClientTest : public testing::Test { } else { TestUtility::loadFromYaml(yaml, proto_config); } - return std::make_shared(proto_config, timeout, path_prefix); } @@ -120,7 +123,7 @@ class ExtAuthzHttpClientTest : public testing::Test { const auto authz_response = TestCommon::makeAuthzResponse(CheckStatus::OK); auto check_response = TestCommon::makeMessageResponse(expected_headers); - client_->check(request_callbacks_, request, parent_span_, stream_info_); + client_->check(request_callbacks_, dispatcher_, request, parent_span_, stream_info_); EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo(AuthzOkResponse(authz_response)))); client_->onSuccess(async_request_, std::move(check_response)); @@ -134,6 +137,7 @@ class ExtAuthzHttpClientTest : public testing::Test { ClientConfigSharedPtr config_; std::unique_ptr client_; MockRequestCallbacks request_callbacks_; + NiceMock dispatcher_; Tracing::MockSpan parent_span_; Tracing::MockSpan child_span_; NiceMock stream_info_; @@ -281,15 +285,22 @@ TEST_F(ExtAuthzHttpClientTest, AllowedRequestHeadersPrefix) { // Verify client response when authorization server returns a 200 OK. TEST_F(ExtAuthzHttpClientTest, AuthorizationOk) { + NiceMock* timer = new NiceMock(&dispatcher_); + EXPECT_CALL(*timer, enableTimer(_, _)); + bool timer_destroyed = false; + timer->timer_destroyed_ = &timer_destroyed; + const auto expected_headers = TestCommon::makeHeaderValueOption({{":status", "200", false}}); const auto authz_response = TestCommon::makeAuthzResponse(CheckStatus::OK); auto check_response = TestCommon::makeMessageResponse(expected_headers); envoy::service::auth::v3::CheckRequest request; - client_->check(request_callbacks_, request, parent_span_, stream_info_); + client_->check(request_callbacks_, dispatcher_, request, parent_span_, stream_info_); EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo(AuthzOkResponse(authz_response)))); client_->onSuccess(async_request_, std::move(check_response)); + // make sure the internal timeout timer is destroyed + EXPECT_EQ(timer_destroyed, true); } using HeaderValuePair = std::pair; @@ -309,7 +320,7 @@ TEST_F(ExtAuthzHttpClientTest, AuthorizationOkWithAddedAuthzHeaders) { const HeaderValuePair header2{"x-authz-header2", "value"}; EXPECT_CALL(async_client_, send_(AllOf(ContainsPairAsHeader(header1), ContainsPairAsHeader(header2)), _, _)); - client_->check(request_callbacks_, request, parent_span_, stream_info_); + client_->check(request_callbacks_, dispatcher_, request, parent_span_, stream_info_); // Check for child span tagging when the request is allowed. EXPECT_CALL(child_span_, setTag(Eq("ext_authz_http_status"), Eq("OK"))); @@ -354,7 +365,7 @@ TEST_F(ExtAuthzHttpClientTest, AuthorizationOkWithAddedAuthzHeadersFromStreamInf EXPECT_CALL(stream_info, getRequestHeaders()).WillOnce(Return(&request_headers)); envoy::service::auth::v3::CheckRequest request; - client_->check(request_callbacks_, request, parent_span_, stream_info); + client_->check(request_callbacks_, dispatcher_, request, parent_span_, stream_info); EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo(AuthzOkResponse(authz_response)))); @@ -372,7 +383,7 @@ TEST_F(ExtAuthzHttpClientTest, AuthorizationOkWithAllowHeader) { envoy::service::auth::v3::CheckRequest request; EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo(AuthzOkResponse(authz_response)))); - client_->check(request_callbacks_, request, parent_span_, stream_info_); + client_->check(request_callbacks_, dispatcher_, request, parent_span_, stream_info_); const auto check_response_headers = TestCommon::makeHeaderValueOption({{":status", "200", false}, @@ -395,7 +406,7 @@ TEST_F(ExtAuthzHttpClientTest, AuthorizationDenied) { auto check_response = TestCommon::makeMessageResponse(expected_headers); envoy::service::auth::v3::CheckRequest request; - client_->check(request_callbacks_, request, parent_span_, stream_info_); + client_->check(request_callbacks_, dispatcher_, request, parent_span_, stream_info_); // Check for child span tagging when the request is denied. EXPECT_CALL(child_span_, setTag(Eq("ext_authz_http_status"), Eq("Forbidden"))); @@ -416,7 +427,7 @@ TEST_F(ExtAuthzHttpClientTest, AuthorizationDeniedWithAllAttributes) { CheckStatus::Denied, Http::Code::Unauthorized, expected_body, expected_headers); envoy::service::auth::v3::CheckRequest request; - client_->check(request_callbacks_, request, parent_span_, stream_info_); + client_->check(request_callbacks_, dispatcher_, request, parent_span_, stream_info_); EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo(AuthzDeniedResponse(authz_response)))); @@ -434,7 +445,7 @@ TEST_F(ExtAuthzHttpClientTest, AuthorizationDeniedAndAllowedClientHeaders) { {{"x-foo", "bar", false}, {":status", "401", false}, {"foo", "bar", false}})); envoy::service::auth::v3::CheckRequest request; - client_->check(request_callbacks_, request, parent_span_, stream_info_); + client_->check(request_callbacks_, dispatcher_, request, parent_span_, stream_info_); EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo(AuthzDeniedResponse(authz_response)))); const auto check_response_headers = TestCommon::makeHeaderValueOption({{":method", "post", false}, @@ -447,13 +458,20 @@ TEST_F(ExtAuthzHttpClientTest, AuthorizationDeniedAndAllowedClientHeaders) { // Test the client when an unknown error occurs. TEST_F(ExtAuthzHttpClientTest, AuthorizationRequestError) { + NiceMock* timer = new NiceMock(&dispatcher_); + EXPECT_CALL(*timer, enableTimer(_, _)); + bool timer_destroyed = false; + timer->timer_destroyed_ = &timer_destroyed; + envoy::service::auth::v3::CheckRequest request; - client_->check(request_callbacks_, request, parent_span_, stream_info_); + client_->check(request_callbacks_, dispatcher_, request, parent_span_, stream_info_); EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo(AuthzErrorResponse(CheckStatus::Error)))); client_->onFailure(async_request_, Http::AsyncClient::FailureReason::Reset); + // make sure the internal timeout timer is destroyed + // EXPECT_EQ(timer_destroyed, true); } // Test the client when a call to authorization server returns a 5xx error status. @@ -462,7 +480,7 @@ TEST_F(ExtAuthzHttpClientTest, AuthorizationRequest5xxError) { Http::ResponseHeaderMapPtr{new Http::TestResponseHeaderMapImpl{{":status", "503"}}})); envoy::service::auth::v3::CheckRequest request; - client_->check(request_callbacks_, request, parent_span_, stream_info_); + client_->check(request_callbacks_, dispatcher_, request, parent_span_, stream_info_); EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo(AuthzErrorResponse(CheckStatus::Error)))); @@ -474,10 +492,54 @@ TEST_F(ExtAuthzHttpClientTest, CancelledAuthorizationRequest) { envoy::service::auth::v3::CheckRequest request; EXPECT_CALL(async_client_, send_(_, _, _)).WillOnce(Return(&async_request_)); - client_->check(request_callbacks_, request, parent_span_, stream_info_); + client_->check(request_callbacks_, dispatcher_, request, parent_span_, stream_info_); + + EXPECT_CALL(async_request_, cancel()); + client_->cancel(); +} + +// Test the client when the request times out on an internal timeout. +TEST_F(ExtAuthzHttpClientTest, AuthorizationInternalRequestTimeout) { + TestScopedRuntime scoped_runtime; + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.ext_authz_measure_timeout_on_check_created", "true"}}); + + initialize(""); + envoy::service::auth::v3::CheckRequest request; + + NiceMock* timer = new NiceMock(&dispatcher_); + EXPECT_CALL(*timer, enableTimer(std::chrono::milliseconds(REQUEST_TIMEOUT), _)); + + EXPECT_CALL(async_client_, send_(_, _, _)).WillOnce(Return(&async_request_)); + client_->check(request_callbacks_, dispatcher_, request, parent_span_, stream_info_); + + EXPECT_CALL(async_request_, cancel()); + EXPECT_CALL(request_callbacks_, + onComplete_(WhenDynamicCastTo(AuthzTimedoutResponse()))); + timer->invokeCallback(); +} + +// Test when the client is cancelled with internal timeout. +TEST_F(ExtAuthzHttpClientTest, AuthorizationInternalRequestTimeoutCancelled) { + TestScopedRuntime scoped_runtime; + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.ext_authz_measure_timeout_on_check_created", "true"}}); + + initialize(""); + envoy::service::auth::v3::CheckRequest request; + + NiceMock* timer = new NiceMock(&dispatcher_); + EXPECT_CALL(*timer, enableTimer(std::chrono::milliseconds(REQUEST_TIMEOUT), _)); + + EXPECT_CALL(async_client_, send_(_, _, _)).WillOnce(Return(&async_request_)); + client_->check(request_callbacks_, dispatcher_, request, parent_span_, stream_info_); + // make sure cancel resets the timer: EXPECT_CALL(async_request_, cancel()); + bool timer_destroyed = false; + timer->timer_destroyed_ = &timer_destroyed; client_->cancel(); + EXPECT_EQ(timer_destroyed, true); } // Test the client when the configured cluster is missing/removed. @@ -488,8 +550,8 @@ TEST_F(ExtAuthzHttpClientTest, NoCluster) { EXPECT_CALL(cm_, httpAsyncClientForCluster("ext_authz")).Times(0); EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo(AuthzErrorResponse(CheckStatus::Error)))); - client_->check(request_callbacks_, envoy::service::auth::v3::CheckRequest{}, parent_span_, - stream_info_); + client_->check(request_callbacks_, dispatcher_, envoy::service::auth::v3::CheckRequest{}, + parent_span_, stream_info_); } } // namespace diff --git a/test/extensions/filters/common/ext_authz/mocks.h b/test/extensions/filters/common/ext_authz/mocks.h index 900d64d7d0fdf..682e2ce83fe4a 100644 --- a/test/extensions/filters/common/ext_authz/mocks.h +++ b/test/extensions/filters/common/ext_authz/mocks.h @@ -23,8 +23,9 @@ class MockClient : public Client { // ExtAuthz::Client MOCK_METHOD(void, cancel, ()); MOCK_METHOD(void, check, - (RequestCallbacks & callbacks, const envoy::service::auth::v3::CheckRequest& request, - Tracing::Span& parent_span, const StreamInfo::StreamInfo& stream_info)); + (RequestCallbacks & callbacks, Event::Dispatcher& dispatcher, + const envoy::service::auth::v3::CheckRequest& request, Tracing::Span& parent_span, + const StreamInfo::StreamInfo& stream_info)); }; class MockRequestCallbacks : public RequestCallbacks { diff --git a/test/extensions/filters/common/ext_authz/test_common.h b/test/extensions/filters/common/ext_authz/test_common.h index 106bf0f6c979e..b8a8563979678 100644 --- a/test/extensions/filters/common/ext_authz/test_common.h +++ b/test/extensions/filters/common/ext_authz/test_common.h @@ -60,6 +60,18 @@ MATCHER_P(AuthzErrorResponse, status, "") { return arg->status == status; } +MATCHER(AuthzTimedoutResponse, "") { + // These fields should be always empty when the status is a timeout error. + if (!arg->headers_to_add.empty() || !arg->headers_to_append.empty() || !arg->body.empty()) { + return false; + } + // HTTP status code should be always set to Forbidden. + if (arg->status_code != Http::Code::Forbidden) { + return false; + } + return arg->status == CheckStatus::Error && arg->error_kind == ErrorKind::Timedout; +} + MATCHER_P(AuthzResponseNoAttributes, response, "") { const bool equal_status = arg->status == response.status; const bool equal_metadata = diff --git a/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc b/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc index 4127deffc9730..2ea650285913c 100644 --- a/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc +++ b/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc @@ -23,6 +23,16 @@ namespace Envoy { using Headers = std::vector>; +void setMeasureTimeoutOnCheckCreated(ConfigHelper& config_helper, bool timeout_on_check) { + if (timeout_on_check) { + config_helper.addRuntimeOverride( + "envoy.reloadable_features.ext_authz_measure_timeout_on_check_created", "true"); + } else { + config_helper.addRuntimeOverride( + "envoy.reloadable_features.ext_authz_measure_timeout_on_check_created", "false"); + } +} + class ExtAuthzGrpcIntegrationTest : public Grpc::VersionedGrpcClientIntegrationParamTest, public HttpIntegrationTest { public: @@ -35,8 +45,9 @@ class ExtAuthzGrpcIntegrationTest : public Grpc::VersionedGrpcClientIntegrationP new FakeUpstream(0, FakeHttpConnection::Type::HTTP2, version_, timeSystem())); } - void initializeConfig() { - config_helper_.addConfigModifier([this](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { + void initializeConfig(bool with_timeout = false) { + config_helper_.addConfigModifier([this, with_timeout]( + envoy::config::bootstrap::v3::Bootstrap& bootstrap) { auto* ext_authz_cluster = bootstrap.mutable_static_resources()->add_clusters(); ext_authz_cluster->MergeFrom(bootstrap.static_resources().clusters()[0]); ext_authz_cluster->set_name("ext_authz"); @@ -46,6 +57,11 @@ class ExtAuthzGrpcIntegrationTest : public Grpc::VersionedGrpcClientIntegrationP setGrpcService(*proto_config_.mutable_grpc_service(), "ext_authz", fake_upstreams_.back()->localAddress()); + if (with_timeout) { + proto_config_.mutable_grpc_service()->mutable_timeout()->CopyFrom( + Protobuf::util::TimeUtil::MillisecondsToDuration(1)); + } + proto_config_.mutable_filter_enabled()->set_runtime_key("envoy.ext_authz.enable"); proto_config_.mutable_filter_enabled()->mutable_default_value()->set_numerator(100); proto_config_.mutable_deny_at_disable()->set_runtime_key("envoy.ext_authz.deny_at_disable"); @@ -333,6 +349,28 @@ class ExtAuthzGrpcIntegrationTest : public Grpc::VersionedGrpcClientIntegrationP cleanup(); } + void initiateAndWait() { + initiateClientConnection(4); + response_->waitForEndStream(); + } + + void expectCheckRequestTimedout(bool timeout_on_check) { + setMeasureTimeoutOnCheckCreated(this->config_helper_, timeout_on_check); + initializeConfig(true); + setDownstreamProtocol(Http::CodecClient::Type::HTTP2); + HttpIntegrationTest::initialize(); + initiateAndWait(); + if (timeout_on_check) { + uint32_t timeouts = test_server_->counter("http.config_test.ext_authz.timeout")->value(); + EXPECT_EQ(1U, timeouts); + } + + EXPECT_TRUE(response_->complete()); + EXPECT_EQ("403", response_->headers().getStatusValue()); + + cleanup(); + } + void expectFilterDisableCheck(bool deny_at_disable, const std::string& expected_status) { initializeConfig(); setDenyAtDisableRuntimeConfig(deny_at_disable); @@ -422,20 +460,29 @@ class ExtAuthzHttpIntegrationTest : public HttpIntegrationTest, } cleanupUpstreamAndDownstream(); } - - void setupWithDisabledCaseSensitiveStringMatcher(bool disable_case_sensitive_matcher) { - config_helper_.addConfigModifier([this](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { + void initializeConfig(bool with_timeout = false) { + config_helper_.addConfigModifier([this, with_timeout]( + envoy::config::bootstrap::v3::Bootstrap& bootstrap) { auto* ext_authz_cluster = bootstrap.mutable_static_resources()->add_clusters(); ext_authz_cluster->MergeFrom(bootstrap.static_resources().clusters()[0]); ext_authz_cluster->set_name("ext_authz"); TestUtility::loadFromYaml(default_config_, proto_config_); - + if (with_timeout) { + proto_config_.mutable_http_service()->mutable_server_uri()->mutable_timeout()->CopyFrom( + Protobuf::util::TimeUtil::MillisecondsToDuration(1)); + proto_config_.clear_failure_mode_allow(); + } envoy::config::listener::v3::Filter ext_authz_filter; ext_authz_filter.set_name(Extensions::HttpFilters::HttpFilterNames::get().ExtAuthorization); ext_authz_filter.mutable_typed_config()->PackFrom(proto_config_); + config_helper_.addFilter(MessageUtil::getJsonStringFromMessage(ext_authz_filter)); }); + } + + void setupWithDisabledCaseSensitiveStringMatcher(bool disable_case_sensitive_matcher) { + initializeConfig(); if (disable_case_sensitive_matcher) { disableCaseSensitiveStringMatcher(); @@ -482,6 +529,27 @@ class ExtAuthzHttpIntegrationTest : public HttpIntegrationTest, cleanup(); } + void initiateAndWait() { + initiateClientConnection(); + response_->waitForEndStream(); + } + + void expectCheckRequestTimedout(bool timeout_on_check) { + setMeasureTimeoutOnCheckCreated(this->config_helper_, timeout_on_check); + initializeConfig(true); + HttpIntegrationTest::initialize(); + + initiateAndWait(); + if (timeout_on_check) { + uint32_t timeouts = test_server_->counter("http.config_test.ext_authz.timeout")->value(); + EXPECT_EQ(1U, timeouts); + } + + EXPECT_TRUE(response_->complete()); + EXPECT_EQ("403", response_->headers().getStatusValue()); + cleanup(); + } + envoy::extensions::filters::http::ext_authz::v3::ExtAuthz proto_config_{}; FakeHttpConnectionPtr fake_ext_authz_connection_; FakeStreamPtr ext_authz_request_; @@ -555,6 +623,12 @@ TEST_P(ExtAuthzGrpcIntegrationTest, SendHeadersToAddAndToAppendToUpstream) { {"multiple", "multiple-second"}}); } +TEST_P(ExtAuthzGrpcIntegrationTest, CheckTimesOutLegacy) { expectCheckRequestTimedout(false); } + +TEST_P(ExtAuthzGrpcIntegrationTest, CheckTimesOutFromCheckCreation) { + expectCheckRequestTimedout(true); +} + TEST_P(ExtAuthzGrpcIntegrationTest, AllowAtDisable) { expectFilterDisableCheck(false, "200"); } TEST_P(ExtAuthzGrpcIntegrationTest, DenyAtDisable) { expectFilterDisableCheck(true, "403"); } @@ -580,6 +654,12 @@ TEST_P(ExtAuthzHttpIntegrationTest, DisableCaseSensitiveStringMatcher) { EXPECT_EQ(case_sensitive_header_value_, header_entry->value().getStringView()); } +TEST_P(ExtAuthzHttpIntegrationTest, CheckTimesOutLegacy) { expectCheckRequestTimedout(false); } + +TEST_P(ExtAuthzHttpIntegrationTest, CheckTimesOutFromCheckCreation) { + expectCheckRequestTimedout(true); +} + class ExtAuthzLocalReplyIntegrationTest : public HttpIntegrationTest, public TestWithParam { public: diff --git a/test/extensions/filters/http/ext_authz/ext_authz_test.cc b/test/extensions/filters/http/ext_authz/ext_authz_test.cc index 38939b19a714b..a1ced1acd3453 100644 --- a/test/extensions/filters/http/ext_authz/ext_authz_test.cc +++ b/test/extensions/filters/http/ext_authz/ext_authz_test.cc @@ -179,7 +179,7 @@ TEST_F(HttpFilterTest, ErrorFailClose) { ON_CALL(filter_callbacks_, connection()).WillByDefault(Return(&connection_)); EXPECT_CALL(connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); EXPECT_CALL(connection_, localAddress()).WillOnce(ReturnRef(addr_)); - EXPECT_CALL(*client_, check(_, _, _, _)) + EXPECT_CALL(*client_, check(_, _, _, _, _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; @@ -198,7 +198,51 @@ TEST_F(HttpFilterTest, ErrorFailClose) { EXPECT_EQ( 1U, filter_callbacks_.clusterInfo()->statsScope().counterFromString("ext_authz.error").value()); + EXPECT_EQ( + 0U, + filter_callbacks_.clusterInfo()->statsScope().counterFromString("ext_authz.timeout").value()); + EXPECT_EQ(1U, config_->stats().error_.value()); + EXPECT_EQ(0U, config_->stats().timeout_.value()); +} + +// Test when when a timeout error occurs, the correct stat is incremented. +TEST_F(HttpFilterTest, TimeoutError) { + InSequence s; + initialize(R"EOF( + grpc_service: + envoy_grpc: + cluster_name: "ext_authz_server" + failure_mode_allow: false + )EOF"); + + ON_CALL(filter_callbacks_, connection()).WillByDefault(Return(&connection_)); + EXPECT_CALL(connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); + EXPECT_CALL(connection_, localAddress()).WillOnce(ReturnRef(addr_)); + EXPECT_CALL(*client_, check(_, _, _, _, _)) + .WillOnce( + WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { + request_callbacks_ = &callbacks; + }))); + EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark, + filter_->decodeHeaders(request_headers_, false)); + EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); + EXPECT_CALL(filter_callbacks_, encodeHeaders_(_, true)) + .WillOnce(Invoke([&](const Http::ResponseHeaderMap& headers, bool) -> void { + EXPECT_EQ(headers.getStatusValue(), std::to_string(enumToInt(Http::Code::Forbidden))); + })); + + Filters::Common::ExtAuthz::Response response{}; + response.status = Filters::Common::ExtAuthz::CheckStatus::Error; + response.error_kind = Filters::Common::ExtAuthz::ErrorKind::Timedout; + request_callbacks_->onComplete(std::make_unique(response)); + EXPECT_EQ( + 1U, + filter_callbacks_.clusterInfo()->statsScope().counterFromString("ext_authz.error").value()); + EXPECT_EQ( + 1U, + filter_callbacks_.clusterInfo()->statsScope().counterFromString("ext_authz.timeout").value()); EXPECT_EQ(1U, config_->stats().error_.value()); + EXPECT_EQ(1U, config_->stats().timeout_.value()); } // Verifies that the filter responds with a configurable HTTP status when an network error occurs. @@ -217,7 +261,7 @@ TEST_F(HttpFilterTest, ErrorCustomStatusCode) { ON_CALL(filter_callbacks_, connection()).WillByDefault(Return(&connection_)); EXPECT_CALL(connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); EXPECT_CALL(connection_, localAddress()).WillOnce(ReturnRef(addr_)); - EXPECT_CALL(*client_, check(_, _, _, _)) + EXPECT_CALL(*client_, check(_, _, _, _, _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; @@ -256,7 +300,7 @@ TEST_F(HttpFilterTest, ErrorOpen) { ON_CALL(filter_callbacks_, connection()).WillByDefault(Return(&connection_)); EXPECT_CALL(connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); EXPECT_CALL(connection_, localAddress()).WillOnce(ReturnRef(addr_)); - EXPECT_CALL(*client_, check(_, _, _, _)) + EXPECT_CALL(*client_, check(_, _, _, _, _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; @@ -292,7 +336,7 @@ TEST_F(HttpFilterTest, ImmediateErrorOpen) { Filters::Common::ExtAuthz::Response response{}; response.status = Filters::Common::ExtAuthz::CheckStatus::Error; - EXPECT_CALL(*client_, check(_, _, _, _)) + EXPECT_CALL(*client_, check(_, _, _, _, _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { callbacks.onComplete(std::make_unique(response)); @@ -345,7 +389,7 @@ TEST_F(HttpFilterTest, RequestDataIsTooLarge) { EXPECT_CALL(filter_callbacks_, setDecoderBufferLimit(_)).Times(1); EXPECT_CALL(connection_, remoteAddress()).Times(0); EXPECT_CALL(connection_, localAddress()).Times(0); - EXPECT_CALL(*client_, check(_, _, _, _)).Times(0); + EXPECT_CALL(*client_, check(_, _, _, _, _)).Times(0); EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, false)); @@ -377,7 +421,7 @@ TEST_F(HttpFilterTest, RequestDataWithPartialMessage) { EXPECT_CALL(filter_callbacks_, setDecoderBufferLimit(_)).Times(0); EXPECT_CALL(connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); EXPECT_CALL(connection_, localAddress()).WillOnce(ReturnRef(addr_)); - EXPECT_CALL(*client_, check(_, _, _, _)).Times(1); + EXPECT_CALL(*client_, check(_, _, _, _, _)).Times(1); EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, false)); @@ -417,7 +461,7 @@ TEST_F(HttpFilterTest, RequestDataWithSmallBuffer) { EXPECT_CALL(filter_callbacks_, setDecoderBufferLimit(_)).Times(0); EXPECT_CALL(connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); EXPECT_CALL(connection_, localAddress()).WillOnce(ReturnRef(addr_)); - EXPECT_CALL(*client_, check(_, _, _, _)).Times(1); + EXPECT_CALL(*client_, check(_, _, _, _, _)).Times(1); EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, false)); @@ -441,7 +485,7 @@ TEST_F(HttpFilterTest, AuthWithRequestData) { prepareCheck(); - EXPECT_CALL(*client_, check(_, _, testing::A(), _)) + EXPECT_CALL(*client_, check(_, _, _, testing::A(), _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; @@ -469,7 +513,7 @@ TEST_F(HttpFilterTest, HeaderOnlyRequest) { prepareCheck(); - EXPECT_CALL(*client_, check(_, _, testing::A(), _)) + EXPECT_CALL(*client_, check(_, _, _, testing::A(), _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; @@ -498,7 +542,7 @@ TEST_F(HttpFilterTest, UpgradeWebsocketRequest) { request_headers_.addCopy(Http::Headers::get().Upgrade, Http::Headers::get().UpgradeValues.WebSocket); - EXPECT_CALL(*client_, check(_, _, testing::A(), _)) + EXPECT_CALL(*client_, check(_, _, _, testing::A(), _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; @@ -526,7 +570,7 @@ TEST_F(HttpFilterTest, H2UpgradeRequest) { request_headers_.addCopy(Http::Headers::get().Protocol, Http::Headers::get().ProtocolStrings.Http2String); - EXPECT_CALL(*client_, check(_, _, testing::A(), _)) + EXPECT_CALL(*client_, check(_, _, _, testing::A(), _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; @@ -551,7 +595,7 @@ TEST_F(HttpFilterTest, HeaderOnlyRequestWithStream) { prepareCheck(); - EXPECT_CALL(*client_, check(_, _, testing::A(), _)) + EXPECT_CALL(*client_, check(_, _, _, testing::A(), _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; @@ -579,7 +623,7 @@ TEST_F(HttpFilterTest, ClearCache) { prepareCheck(); - EXPECT_CALL(*client_, check(_, _, testing::A(), _)) + EXPECT_CALL(*client_, check(_, _, _, testing::A(), _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; @@ -620,7 +664,7 @@ TEST_F(HttpFilterTest, ClearCacheRouteHeadersToAppendOnly) { prepareCheck(); - EXPECT_CALL(*client_, check(_, _, testing::A(), _)) + EXPECT_CALL(*client_, check(_, _, _, testing::A(), _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; @@ -660,7 +704,7 @@ TEST_F(HttpFilterTest, ClearCacheRouteHeadersToAddOnly) { prepareCheck(); - EXPECT_CALL(*client_, check(_, _, testing::A(), _)) + EXPECT_CALL(*client_, check(_, _, _, testing::A(), _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; @@ -699,7 +743,7 @@ TEST_F(HttpFilterTest, NoClearCacheRoute) { prepareCheck(); - EXPECT_CALL(*client_, check(_, _, testing::A(), _)) + EXPECT_CALL(*client_, check(_, _, _, testing::A(), _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; @@ -734,7 +778,7 @@ TEST_F(HttpFilterTest, NoClearCacheRouteConfig) { prepareCheck(); - EXPECT_CALL(*client_, check(_, _, testing::A(), _)) + EXPECT_CALL(*client_, check(_, _, _, testing::A(), _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; @@ -778,7 +822,7 @@ TEST_F(HttpFilterTest, NoClearCacheRouteDeniedResponse) { response.headers_to_set = Http::HeaderVector{{Http::LowerCaseString{"foo"}, "bar"}}; auto response_ptr = std::make_unique(response); - EXPECT_CALL(*client_, check(_, _, testing::A(), _)) + EXPECT_CALL(*client_, check(_, _, _, testing::A(), _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { callbacks.onComplete(std::move(response_ptr)); @@ -828,8 +872,8 @@ TEST_F(HttpFilterTest, MetadataContext) { prepareCheck(); envoy::service::auth::v3::CheckRequest check_request; - EXPECT_CALL(*client_, check(_, _, _, _)) - .WillOnce(WithArgs<1>(Invoke([&](const envoy::service::auth::v3::CheckRequest& check_param) + EXPECT_CALL(*client_, check(_, _, _, _, _)) + .WillOnce(WithArgs<2>(Invoke([&](const envoy::service::auth::v3::CheckRequest& check_param) -> void { check_request = check_param; }))); filter_->decodeHeaders(request_headers_, false); @@ -877,7 +921,7 @@ TEST_F(HttpFilterTest, FilterDisabled) { .WillByDefault(Return(false)); // Make sure check is not called. - EXPECT_CALL(*client_, check(_, _, _, _)).Times(0); + EXPECT_CALL(*client_, check(_, _, _, _, _)).Times(0); // Engage the filter. EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false)); } @@ -903,7 +947,7 @@ TEST_F(HttpFilterTest, FilterEnabled) { .WillByDefault(Return(true)); // Make sure check is called once. - EXPECT_CALL(*client_, check(_, _, _, _)).Times(1); + EXPECT_CALL(*client_, check(_, _, _, _, _)).Times(1); // Engage the filter. EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark, filter_->decodeHeaders(request_headers_, false)); @@ -935,7 +979,7 @@ TEST_F(HttpFilterTest, FilterDenyAtDisable) { .WillByDefault(Return(true)); // Make sure check is not called. - EXPECT_CALL(*client_, check(_, _, _, _)).Times(0); + EXPECT_CALL(*client_, check(_, _, _, _, _)).Times(0); // Engage the filter. EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, false)); @@ -967,7 +1011,7 @@ TEST_F(HttpFilterTest, FilterAllowAtDisable) { .WillByDefault(Return(false)); // Make sure check is not called. - EXPECT_CALL(*client_, check(_, _, _, _)).Times(0); + EXPECT_CALL(*client_, check(_, _, _, _, _)).Times(0); // Engage the filter. EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false)); } @@ -1004,8 +1048,8 @@ TEST_P(HttpFilterTestParam, ContextExtensions) { // Save the check request from the check call. envoy::service::auth::v3::CheckRequest check_request; - EXPECT_CALL(*client_, check(_, _, _, _)) - .WillOnce(WithArgs<1>(Invoke([&](const envoy::service::auth::v3::CheckRequest& check_param) + EXPECT_CALL(*client_, check(_, _, _, _, _)) + .WillOnce(WithArgs<2>(Invoke([&](const envoy::service::auth::v3::CheckRequest& check_param) -> void { check_request = check_param; }))); // Engage the filter so that check is called. @@ -1038,7 +1082,7 @@ TEST_P(HttpFilterTestParam, DisabledOnRoute) { // baseline: make sure that when not disabled, check is called test_disable(false); - EXPECT_CALL(*client_, check(_, _, testing::A(), _)).Times(1); + EXPECT_CALL(*client_, check(_, _, _, testing::A(), _)).Times(1); // Engage the filter. EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark, filter_->decodeHeaders(request_headers_, false)); @@ -1046,7 +1090,7 @@ TEST_P(HttpFilterTestParam, DisabledOnRoute) { // test that disabling works test_disable(true); // Make sure check is not called. - EXPECT_CALL(*client_, check(_, _, _, _)).Times(0); + EXPECT_CALL(*client_, check(_, _, _, _, _)).Times(0); // Engage the filter. EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false)); } @@ -1082,14 +1126,14 @@ TEST_P(HttpFilterTestParam, DisabledOnRouteWithRequestBody) { EXPECT_CALL(filter_callbacks_, setDecoderBufferLimit(_)).Times(1); EXPECT_CALL(connection_, remoteAddress()).Times(0); EXPECT_CALL(connection_, localAddress()).Times(0); - EXPECT_CALL(*client_, check(_, _, _, _)).Times(0); + EXPECT_CALL(*client_, check(_, _, _, _, _)).Times(0); EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, false)); EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_->decodeData(data_, false)); // To test that disabling the filter works. test_disable(true); - EXPECT_CALL(*client_, check(_, _, _, _)).Times(0); + EXPECT_CALL(*client_, check(_, _, _, _, _)).Times(0); // Make sure that setDecoderBufferLimit is skipped. EXPECT_CALL(filter_callbacks_, setDecoderBufferLimit(_)).Times(0); EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false)); @@ -1111,7 +1155,7 @@ TEST_P(HttpFilterTestParam, OkResponse) { prepareCheck(); - EXPECT_CALL(*client_, check(_, _, testing::A(), _)) + EXPECT_CALL(*client_, check(_, _, _, testing::A(), _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; @@ -1147,7 +1191,7 @@ TEST_P(HttpFilterTestParam, ImmediateOkResponse) { Filters::Common::ExtAuthz::Response response{}; response.status = Filters::Common::ExtAuthz::CheckStatus::OK; - EXPECT_CALL(*client_, check(_, _, _, _)) + EXPECT_CALL(*client_, check(_, _, _, _, _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { callbacks.onComplete(std::make_unique(response)); @@ -1176,7 +1220,7 @@ TEST_P(HttpFilterTestParam, ImmediateDeniedResponseWithHttpAttributes) { auto response_ptr = std::make_unique(response); - EXPECT_CALL(*client_, check(_, _, _, _)) + EXPECT_CALL(*client_, check(_, _, _, _, _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { callbacks.onComplete(std::move(response_ptr)); @@ -1217,7 +1261,7 @@ TEST_P(HttpFilterTestParam, ImmediateOkResponseWithHttpAttributes) { auto response_ptr = std::make_unique(response); - EXPECT_CALL(*client_, check(_, _, _, _)) + EXPECT_CALL(*client_, check(_, _, _, _, _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { callbacks.onComplete(std::move(response_ptr)); @@ -1241,7 +1285,7 @@ TEST_P(HttpFilterTestParam, ImmediateDeniedResponse) { Filters::Common::ExtAuthz::Response response{}; response.status = Filters::Common::ExtAuthz::CheckStatus::Denied; - EXPECT_CALL(*client_, check(_, _, _, _)) + EXPECT_CALL(*client_, check(_, _, _, _, _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { callbacks.onComplete(std::make_unique(response)); @@ -1262,7 +1306,7 @@ TEST_P(HttpFilterTestParam, DeniedResponseWith401) { InSequence s; prepareCheck(); - EXPECT_CALL(*client_, check(_, _, _, _)) + EXPECT_CALL(*client_, check(_, _, _, _, _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; @@ -1294,7 +1338,7 @@ TEST_P(HttpFilterTestParam, DeniedResponseWith403) { InSequence s; prepareCheck(); - EXPECT_CALL(*client_, check(_, _, _, _)) + EXPECT_CALL(*client_, check(_, _, _, _, _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; @@ -1338,7 +1382,7 @@ TEST_P(HttpFilterTestParam, DestroyResponseBeforeSendLocalReply) { std::make_unique(response); prepareCheck(); - EXPECT_CALL(*client_, check(_, _, _, _)) + EXPECT_CALL(*client_, check(_, _, _, _, _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; @@ -1396,7 +1440,7 @@ TEST_P(HttpFilterTestParam, OverrideEncodingHeaders) { std::make_unique(response); prepareCheck(); - EXPECT_CALL(*client_, check(_, _, _, _)) + EXPECT_CALL(*client_, check(_, _, _, _, _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; @@ -1460,7 +1504,7 @@ TEST_F(HttpFilterTest, EmitDynamicMetadata) { prepareCheck(); - EXPECT_CALL(*client_, check(_, _, testing::A(), _)) + EXPECT_CALL(*client_, check(_, _, _, testing::A(), _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; @@ -1502,7 +1546,7 @@ TEST_P(HttpFilterTestParam, ResetDuringCall) { InSequence s; prepareCheck(); - EXPECT_CALL(*client_, check(_, _, _, _)) + EXPECT_CALL(*client_, check(_, _, _, _, _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; @@ -1534,8 +1578,8 @@ TEST_P(HttpFilterTestParam, NoCluster) { // Save the check request from the check call. envoy::service::auth::v3::CheckRequest check_request; - EXPECT_CALL(*client_, check(_, _, _, _)) - .WillOnce(WithArgs<1>(Invoke([&](const envoy::service::auth::v3::CheckRequest& check_param) + EXPECT_CALL(*client_, check(_, _, _, _, _)) + .WillOnce(WithArgs<2>(Invoke([&](const envoy::service::auth::v3::CheckRequest& check_param) -> void { check_request = check_param; }))); // Make sure that filter chain is not continued and the call has been invoked. EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark, diff --git a/test/extensions/filters/network/ext_authz/ext_authz_fuzz_test.cc b/test/extensions/filters/network/ext_authz/ext_authz_fuzz_test.cc index c2e816c748d51..a78571199e7e6 100644 --- a/test/extensions/filters/network/ext_authz/ext_authz_fuzz_test.cc +++ b/test/extensions/filters/network/ext_authz/ext_authz_fuzz_test.cc @@ -83,7 +83,7 @@ DEFINE_PROTO_FUZZER(const envoy::extensions::filters::network::ext_authz::ExtAut case envoy::extensions::filters::network::ext_authz::Action::kOnData: { // Optional input field to set default authorization check result for the following "onData()" if (action.on_data().has_result()) { - ON_CALL(*client, check(_, _, _, _)) + ON_CALL(*client, check(_, _, _, _, _)) .WillByDefault(WithArgs<0>( Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { callbacks.onComplete(makeAuthzResponse( diff --git a/test/extensions/filters/network/ext_authz/ext_authz_test.cc b/test/extensions/filters/network/ext_authz/ext_authz_test.cc index 9f50f576ac42e..b8fd75767e215 100644 --- a/test/extensions/filters/network/ext_authz/ext_authz_test.cc +++ b/test/extensions/filters/network/ext_authz/ext_authz_test.cc @@ -102,7 +102,7 @@ stat_prefix: name TEST_F(ExtAuthzFilterTest, OKWithOnData) { EXPECT_CALL(filter_callbacks_.connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); EXPECT_CALL(filter_callbacks_.connection_, localAddress()).WillOnce(ReturnRef(addr_)); - EXPECT_CALL(*client_, check(_, _, testing::A(), _)) + EXPECT_CALL(*client_, check(_, _, _, testing::A(), _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; @@ -147,6 +147,7 @@ TEST_F(ExtAuthzFilterTest, OKWithOnData) { EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.error").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.timeout").value()); EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.failure_mode_allowed").value()); EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.denied").value()); EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.ok").value()); @@ -158,7 +159,7 @@ TEST_F(ExtAuthzFilterTest, DeniedWithOnData) { EXPECT_CALL(filter_callbacks_.connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); EXPECT_CALL(filter_callbacks_.connection_, localAddress()).WillOnce(ReturnRef(addr_)); - EXPECT_CALL(*client_, check(_, _, _, _)) + EXPECT_CALL(*client_, check(_, _, _, _, _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; @@ -186,6 +187,7 @@ TEST_F(ExtAuthzFilterTest, DeniedWithOnData) { EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.error").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.timeout").value()); EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.failure_mode_allowed").value()); EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.denied").value()); EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.ok").value()); @@ -197,7 +199,7 @@ TEST_F(ExtAuthzFilterTest, FailOpen) { EXPECT_CALL(filter_callbacks_.connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); EXPECT_CALL(filter_callbacks_.connection_, localAddress()).WillOnce(ReturnRef(addr_)); - EXPECT_CALL(*client_, check(_, _, _, _)) + EXPECT_CALL(*client_, check(_, _, _, _, _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; @@ -216,6 +218,7 @@ TEST_F(ExtAuthzFilterTest, FailOpen) { EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.error").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.timeout").value()); EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.failure_mode_allowed").value()); EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.denied").value()); EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.ok").value()); @@ -229,7 +232,7 @@ TEST_F(ExtAuthzFilterTest, FailClose) { EXPECT_CALL(filter_callbacks_.connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); EXPECT_CALL(filter_callbacks_.connection_, localAddress()).WillOnce(ReturnRef(addr_)); - EXPECT_CALL(*client_, check(_, _, _, _)) + EXPECT_CALL(*client_, check(_, _, _, _, _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; @@ -245,6 +248,7 @@ TEST_F(ExtAuthzFilterTest, FailClose) { EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.error").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.timeout").value()); EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.failure_mode_allowed").value()); EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.denied").value()); EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.ok").value()); @@ -258,7 +262,7 @@ TEST_F(ExtAuthzFilterTest, DoNotCallCancelonRemoteClose) { EXPECT_CALL(filter_callbacks_.connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); EXPECT_CALL(filter_callbacks_.connection_, localAddress()).WillOnce(ReturnRef(addr_)); - EXPECT_CALL(*client_, check(_, _, _, _)) + EXPECT_CALL(*client_, check(_, _, _, _, _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; @@ -278,6 +282,7 @@ TEST_F(ExtAuthzFilterTest, DoNotCallCancelonRemoteClose) { EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.error").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.timeout").value()); EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.failure_mode_allowed").value()); EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.denied").value()); EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.ok").value()); @@ -291,7 +296,7 @@ TEST_F(ExtAuthzFilterTest, VerifyCancelOnRemoteClose) { EXPECT_CALL(filter_callbacks_.connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); EXPECT_CALL(filter_callbacks_.connection_, localAddress()).WillOnce(ReturnRef(addr_)); - EXPECT_CALL(*client_, check(_, _, _, _)) + EXPECT_CALL(*client_, check(_, _, _, _, _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; @@ -306,6 +311,7 @@ TEST_F(ExtAuthzFilterTest, VerifyCancelOnRemoteClose) { EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.error").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.timeout").value()); EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.failure_mode_allowed").value()); EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.denied").value()); EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.ok").value()); @@ -320,7 +326,7 @@ TEST_F(ExtAuthzFilterTest, ImmediateOK) { EXPECT_CALL(filter_callbacks_.connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); EXPECT_CALL(filter_callbacks_.connection_, localAddress()).WillOnce(ReturnRef(addr_)); EXPECT_CALL(filter_callbacks_, continueReading()).Times(0); - EXPECT_CALL(*client_, check(_, _, _, _)) + EXPECT_CALL(*client_, check(_, _, _, _, _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { callbacks.onComplete(makeAuthzResponse(Filters::Common::ExtAuthz::CheckStatus::OK)); @@ -336,6 +342,7 @@ TEST_F(ExtAuthzFilterTest, ImmediateOK) { EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.error").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.timeout").value()); EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.failure_mode_allowed").value()); EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.denied").value()); EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.ok").value()); @@ -350,7 +357,7 @@ TEST_F(ExtAuthzFilterTest, ImmediateNOK) { EXPECT_CALL(filter_callbacks_.connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); EXPECT_CALL(filter_callbacks_.connection_, localAddress()).WillOnce(ReturnRef(addr_)); EXPECT_CALL(filter_callbacks_, continueReading()).Times(0); - EXPECT_CALL(*client_, check(_, _, _, _)) + EXPECT_CALL(*client_, check(_, _, _, _, _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { callbacks.onComplete(makeAuthzResponse(Filters::Common::ExtAuthz::CheckStatus::Denied)); @@ -362,6 +369,7 @@ TEST_F(ExtAuthzFilterTest, ImmediateNOK) { EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.error").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.timeout").value()); EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.failure_mode_allowed").value()); EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.denied").value()); EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.ok").value()); @@ -376,7 +384,7 @@ TEST_F(ExtAuthzFilterTest, ImmediateErrorFailOpen) { EXPECT_CALL(filter_callbacks_.connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); EXPECT_CALL(filter_callbacks_.connection_, localAddress()).WillOnce(ReturnRef(addr_)); EXPECT_CALL(filter_callbacks_, continueReading()).Times(0); - EXPECT_CALL(*client_, check(_, _, _, _)) + EXPECT_CALL(*client_, check(_, _, _, _, _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { callbacks.onComplete(makeAuthzResponse(Filters::Common::ExtAuthz::CheckStatus::Error)); @@ -392,6 +400,39 @@ TEST_F(ExtAuthzFilterTest, ImmediateErrorFailOpen) { EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.error").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.timeout").value()); + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.failure_mode_allowed").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.denied").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.ok").value()); + EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.cx_closed").value()); +} + +// Test to verify that timeout the proper stat is incremented. +TEST_F(ExtAuthzFilterTest, TimeoutError) { + InSequence s; + + EXPECT_CALL(filter_callbacks_.connection_, remoteAddress()).WillOnce(ReturnRef(addr_)); + EXPECT_CALL(filter_callbacks_.connection_, localAddress()).WillOnce(ReturnRef(addr_)); + EXPECT_CALL(filter_callbacks_, continueReading()).Times(0); + EXPECT_CALL(*client_, check(_, _, _, _, _)) + .WillOnce( + WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { + auto resp = makeAuthzResponse(Filters::Common::ExtAuthz::CheckStatus::Error); + resp->error_kind = Filters::Common::ExtAuthz::ErrorKind::Timedout; + callbacks.onComplete(std::move(resp)); + }))); + + EXPECT_EQ(Network::FilterStatus::Continue, filter_->onNewConnection()); + Buffer::OwnedImpl data("hello"); + EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data, false)); + EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data, false)); + + EXPECT_CALL(*client_, cancel()).Times(0); + filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose); + + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value()); + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.error").value()); + EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.timeout").value()); EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.failure_mode_allowed").value()); EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.denied").value()); EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.ok").value()); diff --git a/test/mocks/event/mocks.cc b/test/mocks/event/mocks.cc index d263c3ad68bca..215e7cafbdb4a 100644 --- a/test/mocks/event/mocks.cc +++ b/test/mocks/event/mocks.cc @@ -49,7 +49,11 @@ MockTimer::MockTimer(MockDispatcher* dispatcher) : MockTimer() { .RetiresOnSaturation(); } -MockTimer::~MockTimer() = default; +MockTimer::~MockTimer() { + if (timer_destroyed_) { + *timer_destroyed_ = true; + } +} MockSchedulableCallback::~MockSchedulableCallback() = default; diff --git a/test/mocks/event/mocks.h b/test/mocks/event/mocks.h index 605005fe471a8..c4f9d6c911fee 100644 --- a/test/mocks/event/mocks.h +++ b/test/mocks/event/mocks.h @@ -172,6 +172,9 @@ class MockTimer : public Timer { bool enabled_{}; Event::TimerCb callback_; + + // If not nullptr, will be set on dtor. This can help to verify that the timer was destroyed. + bool* timer_destroyed_{}; }; class MockSchedulableCallback : public SchedulableCallback { diff --git a/test/mocks/grpc/mocks.cc b/test/mocks/grpc/mocks.cc index bd7a07416f35f..f5f62eb181126 100644 --- a/test/mocks/grpc/mocks.cc +++ b/test/mocks/grpc/mocks.cc @@ -2,12 +2,15 @@ #include "test/mocks/http/mocks.h" +using testing::Return; + namespace Envoy { namespace Grpc { MockAsyncClient::MockAsyncClient() { async_request_ = std::make_unique>(); ON_CALL(*this, sendRaw(_, _, _, _, _, _)).WillByDefault(Return(async_request_.get())); + ON_CALL(*this, dispatcher()).WillByDefault(Return(&dispatcher_)); } MockAsyncClient::~MockAsyncClient() = default; diff --git a/test/mocks/grpc/mocks.h b/test/mocks/grpc/mocks.h index 476ba677f9453..c260c254e5964 100644 --- a/test/mocks/grpc/mocks.h +++ b/test/mocks/grpc/mocks.h @@ -11,6 +11,7 @@ #include "common/grpc/typed_async_client.h" +#include "test/mocks/event/mocks.h" #include "test/test_common/utility.h" #include "gmock/gmock.h" @@ -88,8 +89,10 @@ class MockAsyncClient : public RawAsyncClient { (absl::string_view service_full_name, absl::string_view method_name, RawAsyncStreamCallbacks& callbacks, const Http::AsyncClient::StreamOptions& options)); + MOCK_METHOD(Event::Dispatcher*, dispatcher, ()); std::unique_ptr> async_request_; + Event::MockDispatcher dispatcher_; }; class MockAsyncClientFactory : public AsyncClientFactory {