From c3cddf7cd108c2869c0f6b67d803f7e3f91c92cc Mon Sep 17 00:00:00 2001 From: Yuval Kohavi Date: Tue, 24 Nov 2020 11:59:14 -0500 Subject: [PATCH] Revert "ext_auth: add option to measure timeout when check request is created. (#12778)" (#14152) This reverts commit dfa85e8ed4457cab6fac012fab79ecd7823e3b2a. Signed-off-by: Yuval Kohavi --- .../http/http_filters/ext_authz_filter.rst | 3 +- source/common/runtime/runtime_features.cc | 3 - .../filters/common/ext_authz/ext_authz.h | 29 +--- .../common/ext_authz/ext_authz_grpc_impl.cc | 33 +--- .../common/ext_authz/ext_authz_grpc_impl.h | 9 +- .../common/ext_authz/ext_authz_http_impl.cc | 34 +--- .../common/ext_authz/ext_authz_http_impl.h | 7 +- .../filters/http/ext_authz/config.cc | 1 - .../filters/http/ext_authz/ext_authz.cc | 9 +- .../filters/http/ext_authz/ext_authz.h | 3 - .../filters/network/ext_authz/ext_authz.cc | 8 +- .../filters/network/ext_authz/ext_authz.h | 1 - test/common/secret/sds_api_test.cc | 1 + .../extensions/filters/common/ext_authz/BUILD | 2 - .../ext_authz/ext_authz_grpc_impl_test.cc | 87 ++-------- .../ext_authz/ext_authz_http_impl_test.cc | 80 ++------- .../filters/common/ext_authz/mocks.h | 5 +- .../filters/common/ext_authz/test_common.h | 12 -- .../ext_authz/ext_authz_integration_test.cc | 87 +--------- .../filters/http/ext_authz/ext_authz_test.cc | 158 +++++++----------- .../network/ext_authz/ext_authz_fuzz_test.cc | 2 +- .../network/ext_authz/ext_authz_test.cc | 64 ++----- test/mocks/grpc/mocks.cc | 3 - test/mocks/grpc/mocks.h | 3 - 24 files changed, 119 insertions(+), 525 deletions(-) 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 269789a4be66..85162363a8a3 100644 --- a/docs/root/configuration/http/http_filters/ext_authz_filter.rst +++ b/docs/root/configuration/http/http_filters/ext_authz_filter.rst @@ -165,8 +165,7 @@ 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 (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 diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index fd9907cbef75..48189941e999 100644 --- a/source/common/runtime/runtime_features.cc +++ b/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() { diff --git a/source/extensions/filters/common/ext_authz/ext_authz.h b/source/extensions/filters/common/ext_authz/ext_authz.h index babfa52d0e7c..6011752fc3fe 100644 --- a/source/extensions/filters/common/ext_authz/ext_authz.h +++ b/source/extensions/filters/common/ext_authz/ext_authz.h @@ -6,14 +6,12 @@ #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/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; 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 5340af7d3eb3..f065706a4f82 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 @@ -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,7 +85,6 @@ 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&, @@ -107,23 +92,9 @@ 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 9db0831a1274..e9e7edfb4635 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,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,12 +62,9 @@ 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_{}; @@ -76,7 +72,6 @@ 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 e2e27ab6894d..824c2a6b6e87 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,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,23 +229,16 @@ 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; } @@ -255,7 +246,6 @@ 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; } @@ -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)); - callbacks_ = nullptr; -} - ResponsePtr RawHttpClientImpl::toResponse(Http::ResponseMessagePtr message) { const uint64_t status_code = Http::Utility::getResponseStatus(message->headers()); @@ -323,10 +301,9 @@ 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_); } @@ -334,7 +311,6 @@ ResponsePtr RawHttpClientImpl::toResponse(Http::ResponseMessagePtr message) { SuccessResponse denied{message->headers(), config_->clientHeaderMatchers(), config_->upstreamHeaderToAppendMatchers(), Response{CheckStatus::Denied, - ErrorKind::Other, Http::HeaderVector{}, Http::HeaderVector{}, Http::HeaderVector{}, 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 218a01f6c179..d197a9a34268 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 @@ -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 diff --git a/source/extensions/filters/http/ext_authz/config.cc b/source/extensions/filters/http/ext_authz/config.cc index 01c77e4705d7..22162671e73d 100644 --- a/source/extensions/filters/http/ext_authz/config.cc +++ b/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" diff --git a/source/extensions/filters/http/ext_authz/ext_authz.cc b/source/extensions/filters/http/ext_authz/ext_authz.cc index ba050edeaa24..18a5e1e0c885 100644 --- a/source/extensions/filters/http/ext_authz/ext_authz.cc +++ b/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(); diff --git a/source/extensions/filters/http/ext_authz/ext_authz.h b/source/extensions/filters/http/ext_authz/ext_authz.h index 6dd15619df42..db93fa3d2eea 100644 --- a/source/extensions/filters/http/ext_authz/ext_authz.h +++ b/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_; }; diff --git a/source/extensions/filters/network/ext_authz/ext_authz.cc b/source/extensions/filters/network/ext_authz/ext_authz.cc index 0ea178650d84..f19cd3bb0eb9 100644 --- a/source/extensions/filters/network/ext_authz/ext_authz.cc +++ b/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(); diff --git a/source/extensions/filters/network/ext_authz/ext_authz.h b/source/extensions/filters/network/ext_authz/ext_authz.h index 73fa9c0df5d8..89aa7b36892d 100644 --- a/source/extensions/filters/network/ext_authz/ext_authz.h +++ b/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) \ diff --git a/test/common/secret/sds_api_test.cc b/test/common/secret/sds_api_test.cc index 12386bbf535e..8e8908fa1296 100644 --- a/test/common/secret/sds_api_test.cc +++ b/test/common/secret/sds_api_test.cc @@ -14,6 +14,7 @@ #include "test/common/stats/stat_test_utility.h" #include "test/mocks/config/mocks.h" +#include "test/mocks/event/mocks.h" #include "test/mocks/filesystem/mocks.h" #include "test/mocks/grpc/mocks.h" #include "test/mocks/init/mocks.h" diff --git a/test/extensions/filters/common/ext_authz/BUILD b/test/extensions/filters/common/ext_authz/BUILD index 7c840b57ca09..ae47c399c533 100644 --- a/test/extensions/filters/common/ext_authz/BUILD +++ b/test/extensions/filters/common/ext_authz/BUILD @@ -33,7 +33,6 @@ 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/v3:pkg_cc_proto", "@envoy_api//envoy/type/v3:pkg_cc_proto", ], @@ -47,7 +46,6 @@ 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 6f5a4e042e3f..98bce5a24108 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 @@ -12,7 +12,6 @@ #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" @@ -54,12 +53,7 @@ class ExtAuthzGrpcClientTest : public testing::TestWithParam { "envoy.service.auth.{}.Authorization", api_version_), service_full_name); EXPECT_EQ("Check", method_name); - 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()); - } + EXPECT_EQ(timeout_->count(), options.timeout->count()); return &async_request_; })); } @@ -69,7 +63,6 @@ class ExtAuthzGrpcClientTest : public testing::TestWithParam { Grpc::MockAsyncRequest async_request_; GrpcClientImplPtr client_; MockRequestCallbacks request_callbacks_; - NiceMock dispatcher_; Tracing::MockSpan span_; NiceMock stream_info_; envoy::config::core::v3::ApiVersion api_version_; @@ -107,8 +100,7 @@ TEST_P(ExtAuthzGrpcClientTest, AuthorizationOk) { envoy::service::auth::v3::CheckRequest request; expectCallSend(request); - client_->check(request_callbacks_, dispatcher_, request, Tracing::NullSpan::instance(), - stream_info_); + client_->check(request_callbacks_, request, Tracing::NullSpan::instance(), stream_info_); Http::TestRequestHeaderMapImpl headers; client_->onCreateInitialMetadata(headers); @@ -132,8 +124,7 @@ TEST_P(ExtAuthzGrpcClientTest, AuthorizationOkWithAllAtributes) { envoy::service::auth::v3::CheckRequest request; expectCallSend(request); - client_->check(request_callbacks_, dispatcher_, request, Tracing::NullSpan::instance(), - stream_info_); + client_->check(request_callbacks_, request, Tracing::NullSpan::instance(), stream_info_); Http::TestRequestHeaderMapImpl headers; client_->onCreateInitialMetadata(headers); @@ -156,8 +147,7 @@ TEST_P(ExtAuthzGrpcClientTest, AuthorizationDenied) { envoy::service::auth::v3::CheckRequest request; expectCallSend(request); - client_->check(request_callbacks_, dispatcher_, request, Tracing::NullSpan::instance(), - stream_info_); + client_->check(request_callbacks_, request, Tracing::NullSpan::instance(), stream_info_); Http::TestRequestHeaderMapImpl headers; client_->onCreateInitialMetadata(headers); @@ -181,8 +171,7 @@ TEST_P(ExtAuthzGrpcClientTest, AuthorizationDeniedGrpcUnknownStatus) { envoy::service::auth::v3::CheckRequest request; expectCallSend(request); - client_->check(request_callbacks_, dispatcher_, request, Tracing::NullSpan::instance(), - stream_info_); + client_->check(request_callbacks_, request, Tracing::NullSpan::instance(), stream_info_); Http::TestRequestHeaderMapImpl headers; client_->onCreateInitialMetadata(headers); @@ -209,8 +198,7 @@ TEST_P(ExtAuthzGrpcClientTest, AuthorizationDeniedWithAllAttributes) { envoy::service::auth::v3::CheckRequest request; expectCallSend(request); - client_->check(request_callbacks_, dispatcher_, request, Tracing::NullSpan::instance(), - stream_info_); + client_->check(request_callbacks_, request, Tracing::NullSpan::instance(), stream_info_); Http::TestRequestHeaderMapImpl headers; client_->onCreateInitialMetadata(headers); @@ -228,8 +216,7 @@ TEST_P(ExtAuthzGrpcClientTest, UnknownError) { envoy::service::auth::v3::CheckRequest request; expectCallSend(request); - client_->check(request_callbacks_, dispatcher_, request, Tracing::NullSpan::instance(), - stream_info_); + client_->check(request_callbacks_, request, Tracing::NullSpan::instance(), stream_info_); EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo(AuthzErrorResponse(CheckStatus::Error)))); @@ -242,8 +229,7 @@ TEST_P(ExtAuthzGrpcClientTest, CancelledAuthorizationRequest) { envoy::service::auth::v3::CheckRequest request; EXPECT_CALL(*async_client_, sendRaw(_, _, _, _, _, _)).WillOnce(Return(&async_request_)); - client_->check(request_callbacks_, dispatcher_, request, Tracing::NullSpan::instance(), - stream_info_); + client_->check(request_callbacks_, request, Tracing::NullSpan::instance(), stream_info_); EXPECT_CALL(async_request_, cancel()); client_->cancel(); @@ -251,69 +237,17 @@ 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_, dispatcher_, request, Tracing::NullSpan::instance(), - stream_info_); + client_->check(request_callbacks_, 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()); @@ -342,8 +276,7 @@ TEST_P(ExtAuthzGrpcClientTest, AuthorizationOkWithDynamicMetadata) { envoy::service::auth::v3::CheckRequest request; expectCallSend(request); - client_->check(request_callbacks_, dispatcher_, request, Tracing::NullSpan::instance(), - stream_info_); + client_->check(request_callbacks_, 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 13e58e365a81..5b64f025b15b 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,7 +13,6 @@ #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" @@ -34,8 +33,6 @@ 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); } @@ -47,8 +44,7 @@ class ExtAuthzHttpClientTest : public testing::Test { .WillByDefault(ReturnRef(async_client_)); } - ClientConfigSharedPtr createConfig(const std::string& yaml = EMPTY_STRING, - uint32_t timeout = REQUEST_TIMEOUT, + ClientConfigSharedPtr createConfig(const std::string& yaml = EMPTY_STRING, uint32_t timeout = 250, const std::string& path_prefix = "/bar") { envoy::extensions::filters::http::ext_authz::v3::ExtAuthz proto_config{}; if (yaml.empty()) { @@ -99,6 +95,7 @@ class ExtAuthzHttpClientTest : public testing::Test { } else { TestUtility::loadFromYaml(yaml, proto_config); } + return std::make_shared(proto_config, timeout, path_prefix); } @@ -123,7 +120,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_, dispatcher_, request, parent_span_, stream_info_); + client_->check(request_callbacks_, request, parent_span_, stream_info_); EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo(AuthzOkResponse(authz_response)))); client_->onSuccess(async_request_, std::move(check_response)); @@ -137,7 +134,6 @@ 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_; @@ -289,7 +285,7 @@ TEST_F(ExtAuthzHttpClientTest, AuthorizationOk) { 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_, dispatcher_, request, parent_span_, stream_info_); + client_->check(request_callbacks_, request, parent_span_, stream_info_); EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo(AuthzOkResponse(authz_response)))); @@ -313,7 +309,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_, dispatcher_, request, parent_span_, stream_info_); + client_->check(request_callbacks_, 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"))); @@ -358,7 +354,7 @@ TEST_F(ExtAuthzHttpClientTest, AuthorizationOkWithAddedAuthzHeadersFromStreamInf EXPECT_CALL(stream_info, getRequestHeaders()).WillOnce(Return(&request_headers)); envoy::service::auth::v3::CheckRequest request; - client_->check(request_callbacks_, dispatcher_, request, parent_span_, stream_info); + client_->check(request_callbacks_, request, parent_span_, stream_info); EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo(AuthzOkResponse(authz_response)))); @@ -376,7 +372,7 @@ TEST_F(ExtAuthzHttpClientTest, AuthorizationOkWithAllowHeader) { envoy::service::auth::v3::CheckRequest request; EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo(AuthzOkResponse(authz_response)))); - client_->check(request_callbacks_, dispatcher_, request, parent_span_, stream_info_); + client_->check(request_callbacks_, request, parent_span_, stream_info_); const auto check_response_headers = TestCommon::makeHeaderValueOption({{":status", "200", false}, @@ -395,7 +391,7 @@ TEST_F(ExtAuthzHttpClientTest, AuthorizationOkWithAllowHeader) { // Response correctly. TEST_F(ExtAuthzHttpClientTest, AuthorizationOkWithHeadersToRemove) { envoy::service::auth::v3::CheckRequest request; - client_->check(request_callbacks_, dispatcher_, request, parent_span_, stream_info_); + client_->check(request_callbacks_, request, parent_span_, stream_info_); // When we call onSuccess() at the bottom of the test we expect that all the // headers-to-remove in that http response to have been correctly extracted @@ -425,7 +421,7 @@ TEST_F(ExtAuthzHttpClientTest, AuthorizationDenied) { auto check_response = TestCommon::makeMessageResponse(expected_headers); envoy::service::auth::v3::CheckRequest request; - client_->check(request_callbacks_, dispatcher_, request, parent_span_, stream_info_); + client_->check(request_callbacks_, 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"))); @@ -446,7 +442,7 @@ TEST_F(ExtAuthzHttpClientTest, AuthorizationDeniedWithAllAttributes) { CheckStatus::Denied, Http::Code::Unauthorized, expected_body, expected_headers); envoy::service::auth::v3::CheckRequest request; - client_->check(request_callbacks_, dispatcher_, request, parent_span_, stream_info_); + client_->check(request_callbacks_, request, parent_span_, stream_info_); EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo(AuthzDeniedResponse(authz_response)))); @@ -464,7 +460,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_, dispatcher_, request, parent_span_, stream_info_); + client_->check(request_callbacks_, request, parent_span_, stream_info_); EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo(AuthzDeniedResponse(authz_response)))); const auto check_response_headers = TestCommon::makeHeaderValueOption({{":method", "post", false}, @@ -479,7 +475,7 @@ TEST_F(ExtAuthzHttpClientTest, AuthorizationDeniedAndAllowedClientHeaders) { TEST_F(ExtAuthzHttpClientTest, AuthorizationRequestError) { envoy::service::auth::v3::CheckRequest request; - client_->check(request_callbacks_, dispatcher_, request, parent_span_, stream_info_); + client_->check(request_callbacks_, request, parent_span_, stream_info_); EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo(AuthzErrorResponse(CheckStatus::Error)))); @@ -492,7 +488,7 @@ TEST_F(ExtAuthzHttpClientTest, AuthorizationRequest5xxError) { Http::ResponseHeaderMapPtr{new Http::TestResponseHeaderMapImpl{{":status", "503"}}})); envoy::service::auth::v3::CheckRequest request; - client_->check(request_callbacks_, dispatcher_, request, parent_span_, stream_info_); + client_->check(request_callbacks_, request, parent_span_, stream_info_); EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo(AuthzErrorResponse(CheckStatus::Error)))); @@ -504,54 +500,10 @@ TEST_F(ExtAuthzHttpClientTest, CancelledAuthorizationRequest) { envoy::service::auth::v3::CheckRequest request; EXPECT_CALL(async_client_, send_(_, _, _)).WillOnce(Return(&async_request_)); - 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_); + client_->check(request_callbacks_, 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. @@ -562,8 +514,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_, dispatcher_, envoy::service::auth::v3::CheckRequest{}, - parent_span_, stream_info_); + client_->check(request_callbacks_, 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 682e2ce83fe4..900d64d7d0fd 100644 --- a/test/extensions/filters/common/ext_authz/mocks.h +++ b/test/extensions/filters/common/ext_authz/mocks.h @@ -23,9 +23,8 @@ class MockClient : public Client { // ExtAuthz::Client MOCK_METHOD(void, cancel, ()); MOCK_METHOD(void, check, - (RequestCallbacks & callbacks, Event::Dispatcher& dispatcher, - const envoy::service::auth::v3::CheckRequest& request, Tracing::Span& parent_span, - const StreamInfo::StreamInfo& stream_info)); + (RequestCallbacks & callbacks, 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 efbbc8e411ec..116973697fca 100644 --- a/test/extensions/filters/common/ext_authz/test_common.h +++ b/test/extensions/filters/common/ext_authz/test_common.h @@ -62,18 +62,6 @@ 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 385aee15ff89..b6847d475959 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,16 +23,6 @@ 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: @@ -44,8 +34,8 @@ class ExtAuthzGrpcIntegrationTest : public Grpc::VersionedGrpcClientIntegrationP addFakeUpstream(FakeHttpConnection::Type::HTTP2); } - void initializeConfig(bool with_timeout = false, bool disable_with_metadata = false) { - config_helper_.addConfigModifier([this, with_timeout, disable_with_metadata]( + void initializeConfig(bool disable_with_metadata = false) { + config_helper_.addConfigModifier([this, disable_with_metadata]( 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]); @@ -56,11 +46,6 @@ 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); if (disable_with_metadata) { @@ -374,31 +359,9 @@ 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, bool disable_with_metadata, const std::string& expected_status) { - initializeConfig(false, disable_with_metadata); + initializeConfig(disable_with_metadata); setDenyAtDisableRuntimeConfig(deny_at_disable, disable_with_metadata); setDownstreamProtocol(Http::CodecClient::Type::HTTP2); HttpIntegrationTest::initialize(); @@ -488,19 +451,14 @@ class ExtAuthzHttpIntegrationTest : public HttpIntegrationTest, } cleanupUpstreamAndDownstream(); } - void initializeConfig(bool with_timeout = false) { - config_helper_.addConfigModifier([this, with_timeout]( - envoy::config::bootstrap::v3::Bootstrap& bootstrap) { + + void initializeConfig() { + config_helper_.addConfigModifier([this](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_); @@ -564,27 +522,6 @@ 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_; @@ -659,12 +596,6 @@ TEST_P(ExtAuthzGrpcIntegrationTest, SendHeadersToAddAndToAppendToUpstream) { {"multiple", "multiple-second"}}); } -TEST_P(ExtAuthzGrpcIntegrationTest, CheckTimesOutLegacy) { expectCheckRequestTimedout(false); } - -TEST_P(ExtAuthzGrpcIntegrationTest, CheckTimesOutFromCheckCreation) { - expectCheckRequestTimedout(true); -} - TEST_P(ExtAuthzGrpcIntegrationTest, AllowAtDisable) { expectFilterDisableCheck(/*deny_at_disable=*/false, /*disable_with_metadata=*/false, "200"); } @@ -692,12 +623,6 @@ TEST_P(ExtAuthzHttpIntegrationTest, DefaultCaseSensitiveStringMatcher) { ASSERT_TRUE(header_entry.empty()); } -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 3456b0e57259..32dda5457d14 100644 --- a/test/extensions/filters/http/ext_authz/ext_authz_test.cc +++ b/test/extensions/filters/http/ext_authz/ext_authz_test.cc @@ -188,7 +188,7 @@ TEST_F(HttpFilterTest, StatsWithPrefix) { .value()); prepareCheck(); - EXPECT_CALL(*client_, check(_, _, _, _, _)) + EXPECT_CALL(*client_, check(_, _, _, _)) .WillOnce( WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; @@ -227,7 +227,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; @@ -246,51 +246,7 @@ 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. @@ -309,7 +265,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; @@ -348,7 +304,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; @@ -384,7 +340,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)); @@ -437,7 +393,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)); @@ -469,7 +425,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)); @@ -512,7 +468,7 @@ TEST_F(HttpFilterTest, RequestDataWithPartialMessageThenContinueDecoding) { EXPECT_CALL(connection_, localAddress()).WillOnce(ReturnRef(addr_)); // The check call should only be called once. - 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; @@ -565,7 +521,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)); @@ -591,8 +547,8 @@ TEST_F(HttpFilterTest, AuthWithRequestData) { prepareCheck(); envoy::service::auth::v3::CheckRequest check_request; - EXPECT_CALL(*client_, check(_, _, _, testing::A(), _)) - .WillOnce(WithArgs<0, 2>( + EXPECT_CALL(*client_, check(_, _, testing::A(), _)) + .WillOnce(WithArgs<0, 1>( Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks, const envoy::service::auth::v3::CheckRequest& check_param) -> void { request_callbacks_ = &callbacks; @@ -628,8 +584,8 @@ TEST_F(HttpFilterTest, AuthWithNonUtf8RequestData) { prepareCheck(); envoy::service::auth::v3::CheckRequest check_request; - EXPECT_CALL(*client_, check(_, _, _, testing::A(), _)) - .WillOnce(WithArgs<0, 2>( + EXPECT_CALL(*client_, check(_, _, testing::A(), _)) + .WillOnce(WithArgs<0, 1>( Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks, const envoy::service::auth::v3::CheckRequest& check_param) -> void { request_callbacks_ = &callbacks; @@ -666,7 +622,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; @@ -695,7 +651,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; @@ -723,7 +679,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; @@ -748,7 +704,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; @@ -782,7 +738,7 @@ TEST_F(HttpFilterTest, HeadersToRemoveRemovesHeadersExceptSpecialHeaders) { 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; @@ -835,7 +791,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; @@ -879,7 +835,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; @@ -920,7 +876,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; @@ -961,7 +917,7 @@ TEST_F(HttpFilterTest, ClearCacheRouteHeadersToRemoveOnly) { 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; @@ -1003,7 +959,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; @@ -1038,7 +994,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; @@ -1082,7 +1038,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)); @@ -1132,8 +1088,8 @@ TEST_F(HttpFilterTest, MetadataContext) { prepareCheck(); envoy::service::auth::v3::CheckRequest check_request; - EXPECT_CALL(*client_, check(_, _, _, _, _)) - .WillOnce(WithArgs<2>(Invoke([&](const envoy::service::auth::v3::CheckRequest& check_param) + EXPECT_CALL(*client_, check(_, _, _, _)) + .WillOnce(WithArgs<1>(Invoke([&](const envoy::service::auth::v3::CheckRequest& check_param) -> void { check_request = check_param; }))); filter_->decodeHeaders(request_headers_, false); @@ -1181,7 +1137,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)); } @@ -1207,7 +1163,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)); @@ -1239,7 +1195,7 @@ TEST_F(HttpFilterTest, MetadataDisabled) { ON_CALL(filter_callbacks_.stream_info_, dynamicMetadata()).WillByDefault(ReturnRef(metadata)); // 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)); } @@ -1272,7 +1228,7 @@ TEST_F(HttpFilterTest, MetadataEnabled) { prepareCheck(); // 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)); @@ -1316,7 +1272,7 @@ TEST_F(HttpFilterTest, FilterEnabledButMetadataDisabled) { ON_CALL(filter_callbacks_.stream_info_, dynamicMetadata()).WillByDefault(ReturnRef(metadata)); // 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)); } @@ -1359,7 +1315,7 @@ TEST_F(HttpFilterTest, FilterDisabledButMetadataEnabled) { ON_CALL(filter_callbacks_.stream_info_, dynamicMetadata()).WillByDefault(ReturnRef(metadata)); // 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)); } @@ -1404,7 +1360,7 @@ TEST_F(HttpFilterTest, FilterEnabledAndMetadataEnabled) { prepareCheck(); // 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)); @@ -1436,7 +1392,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)); @@ -1468,7 +1424,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)); } @@ -1505,8 +1461,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<2>(Invoke([&](const envoy::service::auth::v3::CheckRequest& check_param) + EXPECT_CALL(*client_, check(_, _, _, _)) + .WillOnce(WithArgs<1>(Invoke([&](const envoy::service::auth::v3::CheckRequest& check_param) -> void { check_request = check_param; }))); // Engage the filter so that check is called. @@ -1539,7 +1495,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)); @@ -1547,7 +1503,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)); } @@ -1583,14 +1539,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)); @@ -1612,7 +1568,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; @@ -1648,7 +1604,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)); @@ -1677,7 +1633,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)); @@ -1723,7 +1679,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)); @@ -1748,7 +1704,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)); @@ -1769,7 +1725,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; @@ -1801,7 +1757,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; @@ -1845,7 +1801,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; @@ -1903,7 +1859,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; @@ -1968,7 +1924,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; @@ -2010,7 +1966,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; @@ -2042,8 +1998,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<2>(Invoke([&](const envoy::service::auth::v3::CheckRequest& check_param) + EXPECT_CALL(*client_, check(_, _, _, _)) + .WillOnce(WithArgs<1>(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, @@ -2084,7 +2040,7 @@ TEST_P(HttpFilterTestParam, DisableRequestBodyBufferingOnRoute) { 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)); @@ -2094,7 +2050,7 @@ TEST_P(HttpFilterTestParam, DisableRequestBodyBufferingOnRoute) { 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::StopAllIterationAndWatermark, filter_->decodeHeaders(request_headers_, false)); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(data_, false)); 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 a78571199e7e..c2e816c748d5 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 e3dab5cdd2f2..39e875028e25 100644 --- a/test/extensions/filters/network/ext_authz/ext_authz_test.cc +++ b/test/extensions/filters/network/ext_authz/ext_authz_test.cc @@ -68,7 +68,7 @@ class ExtAuthzFilterTest : public testing::Test { void expectOKWithOnData() { 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; @@ -115,7 +115,6 @@ class ExtAuthzFilterTest : public testing::Test { EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.disabled").value()); 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()); @@ -179,7 +178,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; @@ -208,7 +207,6 @@ TEST_F(ExtAuthzFilterTest, DeniedWithOnData) { EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.disabled").value()); 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()); @@ -221,7 +219,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; @@ -241,7 +239,6 @@ TEST_F(ExtAuthzFilterTest, FailOpen) { EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.disabled").value()); 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()); @@ -256,7 +253,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; @@ -273,7 +270,6 @@ TEST_F(ExtAuthzFilterTest, FailClose) { EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.disabled").value()); 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()); @@ -288,7 +284,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; @@ -309,7 +305,6 @@ TEST_F(ExtAuthzFilterTest, DoNotCallCancelonRemoteClose) { EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.disabled").value()); 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()); @@ -324,7 +319,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; @@ -340,7 +335,6 @@ TEST_F(ExtAuthzFilterTest, VerifyCancelOnRemoteClose) { EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.disabled").value()); 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()); @@ -356,7 +350,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)); @@ -373,7 +367,6 @@ TEST_F(ExtAuthzFilterTest, ImmediateOK) { EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.disabled").value()); 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()); @@ -389,7 +382,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)); @@ -402,7 +395,6 @@ TEST_F(ExtAuthzFilterTest, ImmediateNOK) { EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.disabled").value()); 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()); @@ -418,7 +410,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)); @@ -435,41 +427,6 @@ TEST_F(ExtAuthzFilterTest, ImmediateErrorFailOpen) { EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.disabled").value()); 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) { - initialize(default_yaml_string_); - 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(0U, stats_store_.counter("ext_authz.name.disabled").value()); - 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()); @@ -495,14 +452,13 @@ TEST_F(ExtAuthzFilterTest, DisabledWithMetadata) { Buffer::OwnedImpl data("hello"); EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data, false)); - EXPECT_CALL(*client_, check(_, _, _, _, _)).Times(0); + EXPECT_CALL(*client_, check(_, _, _, _)).Times(0); EXPECT_CALL(filter_callbacks_.connection_, close(_)).Times(0); EXPECT_CALL(*client_, cancel()).Times(0); EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.disabled").value()); EXPECT_EQ(0U, 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()); diff --git a/test/mocks/grpc/mocks.cc b/test/mocks/grpc/mocks.cc index f5f62eb18112..bd7a07416f35 100644 --- a/test/mocks/grpc/mocks.cc +++ b/test/mocks/grpc/mocks.cc @@ -2,15 +2,12 @@ #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 c260c254e596..476ba677f945 100644 --- a/test/mocks/grpc/mocks.h +++ b/test/mocks/grpc/mocks.h @@ -11,7 +11,6 @@ #include "common/grpc/typed_async_client.h" -#include "test/mocks/event/mocks.h" #include "test/test_common/utility.h" #include "gmock/gmock.h" @@ -89,10 +88,8 @@ 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 {