Skip to content

Instantly share code, notes, and snippets.

@dio
Created November 6, 2022 10:50
Show Gist options
  • Save dio/e729ea0506e7ecc54fc3ca85cf5a086e to your computer and use it in GitHub Desktop.
Save dio/e729ea0506e7ecc54fc3ca85cf5a086e to your computer and use it in GitHub Desktop.
diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc
index 4b3cce5d32..8de74ef6f0 100644
--- a/source/common/runtime/runtime_features.cc
+++ b/source/common/runtime/runtime_features.cc
@@ -43,6 +43,7 @@ RUNTIME_GUARD(envoy_reloadable_features_delta_xds_subscription_state_tracking_fi
RUNTIME_GUARD(envoy_reloadable_features_do_not_await_headers_on_upstream_timeout_to_emit_stats);
RUNTIME_GUARD(envoy_reloadable_features_do_not_count_mapped_pages_as_free);
RUNTIME_GUARD(envoy_reloadable_features_enable_compression_bomb_protection);
+RUNTIME_GUARD(envoy_reloadable_features_ext_authz_http_service_5xx_is_denied);
RUNTIME_GUARD(envoy_reloadable_features_fix_hash_key);
RUNTIME_GUARD(envoy_reloadable_features_get_route_config_factory_by_type);
RUNTIME_GUARD(envoy_reloadable_features_http2_delay_keepalive_timeout);
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 3157136b43..a5d02c28dc 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
@@ -317,11 +317,14 @@ void RawHttpClientImpl::onBeforeFinalizeUpstreamSpan(
ResponsePtr RawHttpClientImpl::toResponse(Http::ResponseMessagePtr message) {
const uint64_t status_code = Http::Utility::getResponseStatus(message->headers());
- // Set an error status if the call to the authorization server returns any of the 5xx HTTP error
- // codes. A Forbidden response is sent to the client if the filter has not been configured with
- // failure_mode_allow.
- if (Http::CodeUtility::is5xx(status_code)) {
- return std::make_unique<Response>(errorResponse());
+ if (!Runtime::runtimeFeatureEnabled(
+ "envoy.reloadable_features.ext_authz_http_service_5xx_is_denied")) {
+ // Set an error status if the call to the authorization server returns any of the 5xx HTTP error
+ // codes. A Forbidden response is sent to the client if the filter has not been configured with
+ // failure_mode_allow.
+ if (Http::CodeUtility::is5xx(status_code)) {
+ return std::make_unique<Response>(errorResponse());
+ }
}
// Extract headers-to-remove from the storage header coming from the
diff --git a/test/extensions/filters/common/ext_authz/BUILD b/test/extensions/filters/common/ext_authz/BUILD
index ae47c399c5..a156bf0c24 100644
--- a/test/extensions/filters/common/ext_authz/BUILD
+++ b/test/extensions/filters/common/ext_authz/BUILD
@@ -46,6 +46,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_http_impl_test.cc b/test/extensions/filters/common/ext_authz/ext_authz_http_impl_test.cc
index cb66739a7f..f5cdf705bb 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
@@ -12,6 +12,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"
@@ -546,8 +547,13 @@ TEST_F(ExtAuthzHttpClientTest, AuthorizationRequestError) {
client_->onFailure(async_request_, Http::AsyncClient::FailureReason::Reset);
}
-// Test the client when a call to authorization server returns a 5xx error status.
-TEST_F(ExtAuthzHttpClientTest, AuthorizationRequest5xxError) {
+// Test the client when a call to authorization server returns a 5xx error status when
+// ext_authz_http_service_5xx_is_denied runtime flag is false.
+TEST_F(ExtAuthzHttpClientTest, AuthorizationRequest5xxErrorWhenRuntimeFlagFalse) {
+ TestScopedRuntime scoped_runtime;
+ scoped_runtime.mergeValues(
+ {{"envoy.reloadable_features.ext_authz_http_service_5xx_is_denied", "false"}});
+
Http::ResponseMessagePtr check_response(new Http::ResponseMessageImpl(
Http::ResponseHeaderMapPtr{new Http::TestResponseHeaderMapImpl{{":status", "503"}}}));
envoy::service::auth::v3::CheckRequest request;
@@ -559,6 +565,23 @@ TEST_F(ExtAuthzHttpClientTest, AuthorizationRequest5xxError) {
client_->onSuccess(async_request_, std::move(check_response));
}
+// Test the client when a call to authorization server returns a 5xx error status.
+TEST_F(ExtAuthzHttpClientTest, AuthorizationRequest5xxError) {
+ const auto expected_body = std::string{"test"};
+ const auto expected_headers = TestCommon::makeHeaderValueOption(
+ {{":status", "500", false}, {"foo", "bar", false}, {"x-foobar", "bar", false}});
+ const auto authz_response = TestCommon::makeAuthzResponse(
+ CheckStatus::Denied, Http::Code::InternalServerError, expected_body, expected_headers);
+
+ envoy::service::auth::v3::CheckRequest request;
+ client_->check(request_callbacks_, request, parent_span_, stream_info_);
+
+ EXPECT_CALL(request_callbacks_,
+ onComplete_(WhenDynamicCastTo<ResponsePtr&>(AuthzDeniedResponse(authz_response))));
+ client_->onSuccess(async_request_,
+ TestCommon::makeMessageResponse(expected_headers, expected_body));
+}
+
// Test the client when the request is canceled.
TEST_F(ExtAuthzHttpClientTest, CancelledAuthorizationRequest) {
envoy::service::auth::v3::CheckRequest request;
diff --git a/test/extensions/filters/http/ext_authz/BUILD b/test/extensions/filters/http/ext_authz/BUILD
index b963389346..e9c2ad75fd 100644
--- a/test/extensions/filters/http/ext_authz/BUILD
+++ b/test/extensions/filters/http/ext_authz/BUILD
@@ -29,6 +29,7 @@ envoy_extension_cc_test(
"//source/extensions/filters/common/ext_authz:ext_authz_grpc_lib",
"//source/extensions/filters/http/ext_authz",
"//test/extensions/filters/common/ext_authz:ext_authz_mocks",
+ "//test/extensions/filters/common/ext_authz:ext_authz_test_common",
"//test/mocks/http:http_mocks",
"//test/mocks/network:network_mocks",
"//test/mocks/runtime:runtime_mocks",
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 5f1d0fec16..ec85d46c20 100644
--- a/test/extensions/filters/http/ext_authz/ext_authz_test.cc
+++ b/test/extensions/filters/http/ext_authz/ext_authz_test.cc
@@ -20,6 +20,7 @@
#include "source/extensions/filters/http/ext_authz/ext_authz.h"
#include "test/extensions/filters/common/ext_authz/mocks.h"
+#include "test/extensions/filters/common/ext_authz/test_common.h"
#include "test/mocks/http/mocks.h"
#include "test/mocks/network/mocks.h"
#include "test/mocks/router/mocks.h"
@@ -350,6 +351,62 @@ TEST_F(HttpFilterTest, ErrorCustomStatusCode) {
EXPECT_EQ("ext_authz_error", decoder_filter_callbacks_.details());
}
+// Verifies that when HTTP service returns 5xx, it sends 5xx.
+TEST_F(HttpFilterTest, ErrorHttpService5xx) {
+ InSequence s;
+
+ initialize(R"EOF(
+ transport_api_version: V3
+ http_service:
+ server_uri:
+ uri: "ext_authz:9000"
+ cluster: "ext_authz"
+ timeout: 0.25s
+ )EOF");
+
+ const auto authz_response_body = std::string{"test"};
+ const auto authz_response_headers = Filters::Common::ExtAuthz::TestCommon::makeHeaderValueOption(
+ {{":status", "500", false}, {"foo", "bar", false}, {"x-foobar", "bar", false}});
+ Http::TestResponseHeaderMapImpl response_headers{
+ {":status", "500"},
+ // The "content-length" and "content-type" headers are here in the response headers since
+ // authz service sends a non-empty body response.
+ {"content-length", "4"},
+ {"content-type", "text/plain"},
+ {"foo", "bar"},
+ {"x-foobar", "bar"}};
+
+ ON_CALL(decoder_filter_callbacks_, connection())
+ .WillByDefault(Return(OptRef<const Network::Connection>{connection_}));
+ connection_.stream_info_.downstream_connection_info_provider_->setRemoteAddress(addr_);
+ connection_.stream_info_.downstream_connection_info_provider_->setLocalAddress(addr_);
+ EXPECT_CALL(*client_, check(_, _, _, _))
+ .WillOnce(
+ Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks,
+ const envoy::service::auth::v3::CheckRequest&, Tracing::Span&,
+ const StreamInfo::StreamInfo&) -> void { request_callbacks_ = &callbacks; }));
+ EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark,
+ filter_->decodeHeaders(request_headers_, false));
+ EXPECT_CALL(decoder_filter_callbacks_, continueDecoding()).Times(0);
+ EXPECT_CALL(decoder_filter_callbacks_,
+ encodeHeaders_(HeaderMapEqualRef(&response_headers), false))
+ .WillOnce(Invoke([&](const Http::ResponseHeaderMap& headers, bool) -> void {
+ EXPECT_EQ(headers.getStatusValue(),
+ std::to_string(enumToInt(Http::Code::InternalServerError)));
+ }));
+ const auto response = Filters::Common::ExtAuthz::TestCommon::makeAuthzResponse(
+ Filters::Common::ExtAuthz::CheckStatus::Denied, Http::Code::InternalServerError,
+ authz_response_body, authz_response_headers);
+
+ request_callbacks_->onComplete(std::make_unique<Filters::Common::ExtAuthz::Response>(response));
+ EXPECT_EQ(1U, decoder_filter_callbacks_.clusterInfo()
+ ->statsScope()
+ .counterFromString("ext_authz.denied")
+ .value());
+ EXPECT_EQ(1U, config_->stats().denied_.value());
+ EXPECT_EQ("ext_authz_denied", decoder_filter_callbacks_.details());
+}
+
// Test when failure_mode_allow is set and the response from the authorization service is Error that
// the request is allowed to continue.
TEST_F(HttpFilterTest, ErrorOpen) {
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment