From ec960e2c413b6e1065c83f52ef8f017e11cdc63b Mon Sep 17 00:00:00 2001 From: yanavlasov Date: Fri, 9 Aug 2024 15:47:14 -0400 Subject: [PATCH] Revert "external authz: Interpret error grpc codes of the external authz server as failure to fix failure_mode_allowed feature (#34951)" (#35637) Fixes #35610 Fixes commit #34951 Signed-off-by: Yan Avlasov Signed-off-by: asingh-g --- .../filter/http/ext_authz/v2/ext_authz.proto | 5 +- .../filters/http/ext_authz/v3/ext_authz.proto | 5 +- changelogs/current.yaml | 6 -- source/common/runtime/runtime_features.cc | 2 - .../common/ext_authz/ext_authz_grpc_impl.cc | 12 +-- .../extensions/filters/common/ext_authz/BUILD | 1 - .../ext_authz/ext_authz_grpc_impl_test.cc | 91 +++++-------------- 7 files changed, 29 insertions(+), 93 deletions(-) diff --git a/api/envoy/config/filter/http/ext_authz/v2/ext_authz.proto b/api/envoy/config/filter/http/ext_authz/v2/ext_authz.proto index c55938fc3cbe..c79dd4a24bc9 100644 --- a/api/envoy/config/filter/http/ext_authz/v2/ext_authz.proto +++ b/api/envoy/config/filter/http/ext_authz/v2/ext_authz.proto @@ -40,12 +40,11 @@ message ExtAuthz { // // 1. When set to true, the filter will *accept* client request even if the communication with // the authorization service has failed, or if the authorization service has returned a HTTP 5xx - // error. In case with GRPC authorization service, only PermissionDenied (7) and Unauthenticated (16) - // status codes will *reject* client requests. And other GRPC statuses will *accept* client requests. + // error. // // 2. When set to false, ext-authz will *reject* client requests and return a *Forbidden* // response if the communication with the authorization service has failed, or if the - // authorization service has returned a HTTP 5xx error or any non-Ok GRPC status. + // authorization service has returned a HTTP 5xx error. // // Note that errors can be *always* tracked in the :ref:`stats // `. diff --git a/api/envoy/extensions/filters/http/ext_authz/v3/ext_authz.proto b/api/envoy/extensions/filters/http/ext_authz/v3/ext_authz.proto index 357dd5fc645a..3b6a67cc9074 100644 --- a/api/envoy/extensions/filters/http/ext_authz/v3/ext_authz.proto +++ b/api/envoy/extensions/filters/http/ext_authz/v3/ext_authz.proto @@ -57,12 +57,11 @@ message ExtAuthz { // // 1. When set to true, the filter will ``accept`` client request even if the communication with // the authorization service has failed, or if the authorization service has returned a HTTP 5xx - // error. In case with GRPC authorization service, only PermissionDenied (7) and Unauthenticated (16) - // status codes will ``reject`` client requests. And other GRPC statuses will ``accept`` client requests. + // error. // // 2. When set to false, ext-authz will ``reject`` client requests and return a ``Forbidden`` // response if the communication with the authorization service has failed, or if the - // authorization service has returned a HTTP 5xx error or any non-Ok GRPC status. + // authorization service has returned a HTTP 5xx error. // // Note that errors can be ``always`` tracked in the :ref:`stats // `. diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 048032ab3a56..59a6740dc7da 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -54,12 +54,6 @@ bug_fixes: - area: c-ares change: | Applying a C-ares patch to fix DNS resoultion by the Google gRPC library. -- area: ext_authz - change: | - Fixed fail-open behavior of the :ref:`failure_mode_allow config option - ` - when a grpc external authz server is used. - The behavior can be enabled by ``envoy_reloadable_features_process_ext_authz_grpc_error_codes_as_errors``. - area: websocket change: | Fixed a bug where the websocket upgrade filter would not take into account per-filter configs. diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 1e1c3820519a..6771624e0426 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -69,8 +69,6 @@ RUNTIME_GUARD(envoy_reloadable_features_no_extension_lookup_by_name); RUNTIME_GUARD(envoy_reloadable_features_no_timer_based_rate_limit_token_bucket); RUNTIME_GUARD(envoy_reloadable_features_original_dst_rely_on_idle_timeout); RUNTIME_GUARD(envoy_reloadable_features_prefer_ipv6_dns_on_macos); -// Fixes fail-open behaviour of failure_mode_allow for external authz grpc servers. -RUNTIME_GUARD(envoy_reloadable_features_process_ext_authz_grpc_error_codes_as_errors); RUNTIME_GUARD(envoy_reloadable_features_proxy_104); RUNTIME_GUARD(envoy_reloadable_features_proxy_status_mapping_more_core_response_flags); RUNTIME_GUARD(envoy_reloadable_features_quic_receive_ecn); 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 22b72b803224..dab5d014d109 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 @@ -114,11 +114,7 @@ void GrpcClientImpl::onSuccess(std::unique_ptrok_response(); copyOkResponseMutations(authz_response, ok_response); } - } else if (response->status().code() == Grpc::Status::WellKnownGrpcStatus::PermissionDenied || - response->status().code() == Grpc::Status::WellKnownGrpcStatus::Unauthenticated || - !Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.process_ext_authz_grpc_error_codes_as_errors")) { - // The request was explicitly forbidden by the external authz server. + } else { span.setTag(TracingConstants::get().TraceStatus, TracingConstants::get().TraceUnauthz); authz_response->status = CheckStatus::Denied; @@ -133,12 +129,6 @@ void GrpcClientImpl::onSuccess(std::unique_ptrbody = response->denied_response().body(); } - } else { - // Unexpected response from external authz server is interpreted as failure - ENVOY_LOG(trace, "CheckRequest call failed with status: {}", - Grpc::Utility::grpcStatusToString(response->status().code())); - authz_response->status = CheckStatus::Error; - authz_response->status_code = Http::Code::Forbidden; } // OkHttpResponse.dynamic_metadata is deprecated. Until OkHttpResponse.dynamic_metadata is diff --git a/test/extensions/filters/common/ext_authz/BUILD b/test/extensions/filters/common/ext_authz/BUILD index 02e8da4a7e29..a9c84c57d34a 100644 --- a/test/extensions/filters/common/ext_authz/BUILD +++ b/test/extensions/filters/common/ext_authz/BUILD @@ -36,7 +36,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", ], 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 cec883ca7506..a7832ddeda2a 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 @@ -11,7 +11,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" @@ -180,6 +179,30 @@ TEST_F(ExtAuthzGrpcClientTest, AuthorizationDenied) { client_->onSuccess(std::move(check_response), span_); } +// Test the client when a gRPC status code unknown is received from the authorization server. +TEST_F(ExtAuthzGrpcClientTest, AuthorizationDeniedGrpcUnknownStatus) { + initialize(); + + auto check_response = std::make_unique(); + auto status = check_response->mutable_status(); + status->set_code(Grpc::Status::WellKnownGrpcStatus::Unknown); + auto authz_response = Response{}; + authz_response.status = CheckStatus::Denied; + + envoy::service::auth::v3::CheckRequest request; + expectCallSend(request); + client_->check(request_callbacks_, request, Tracing::NullSpan::instance(), stream_info_); + + Http::TestRequestHeaderMapImpl headers; + client_->onCreateInitialMetadata(headers); + EXPECT_EQ(nullptr, headers.RequestId()); + EXPECT_CALL(span_, setTag(Eq("ext_authz_status"), Eq("ext_authz_unauthorized"))); + EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo( + AuthzResponseNoAttributes(authz_response)))); + + client_->onSuccess(std::move(check_response), span_); +} + // Test the client when a denied response with additional HTTP attributes is received. TEST_F(ExtAuthzGrpcClientTest, AuthorizationDeniedWithAllAttributes) { initialize(); @@ -414,72 +437,6 @@ TEST_F(ExtAuthzGrpcClientTest, AuthorizationOkWithAppendActions) { span_); } -// Test the client when an error code response is received. -TEST_F(ExtAuthzGrpcClientTest, AuthorizationReturnsErrorOnBadGrpcCode) { - initialize(); - - auto check_response = std::make_unique(); - auto status = check_response->mutable_status(); - - ProtobufWkt::Struct expected_dynamic_metadata; - auto* metadata_fields = expected_dynamic_metadata.mutable_fields(); - (*metadata_fields)["foo"] = ValueUtil::stringValue("ok"); - (*metadata_fields)["bar"] = ValueUtil::numberValue(1); - - // The expected dynamic metadata is set to the outer check response, hence regardless the - // check_response's http_response value (either OkHttpResponse or DeniedHttpResponse) the dynamic - // metadata is set to be equal to the check response's dynamic metadata. - check_response->mutable_dynamic_metadata()->MergeFrom(expected_dynamic_metadata); - - status->set_code(Grpc::Status::WellKnownGrpcStatus::Unavailable); - - // This is the expected authz response. - auto authz_response = Response{}; - authz_response.status = CheckStatus::Error; - - authz_response.dynamic_metadata = expected_dynamic_metadata; - - envoy::service::auth::v3::CheckRequest request; - expectCallSend(request); - client_->check(request_callbacks_, request, Tracing::NullSpan::instance(), stream_info_); - - Http::TestRequestHeaderMapImpl headers; - client_->onCreateInitialMetadata(headers); - - EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo( - AuthzResponseNoAttributes(authz_response)))); - client_->onSuccess(std::move(check_response), span_); -} - -// Test the client when an error code response is received and -// process_ext_authz_grpc_error_codes_as_errors is false. -TEST_F(ExtAuthzGrpcClientTest, AuthorizationReturnsDeniedOnUnknownGrpcCodeWhenFeatureFlagDisabled) { - TestScopedRuntime scoped_runtime; - scoped_runtime.mergeValues( - {{"envoy.reloadable_features.process_ext_authz_grpc_error_codes_as_errors", "false"}}); - - initialize(); - - auto check_response = std::make_unique(); - auto status = check_response->mutable_status(); - status->set_code(Grpc::Status::WellKnownGrpcStatus::Unknown); - auto authz_response = Response{}; - authz_response.status = CheckStatus::Denied; - - envoy::service::auth::v3::CheckRequest request; - expectCallSend(request); - client_->check(request_callbacks_, request, Tracing::NullSpan::instance(), stream_info_); - - Http::TestRequestHeaderMapImpl headers; - client_->onCreateInitialMetadata(headers); - EXPECT_EQ(nullptr, headers.RequestId()); - EXPECT_CALL(span_, setTag(Eq("ext_authz_status"), Eq("ext_authz_unauthorized"))); - EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo( - AuthzResponseNoAttributes(authz_response)))); - - client_->onSuccess(std::move(check_response), span_); -} - } // namespace ExtAuthz } // namespace Common } // namespace Filters