Revert "external authz: Interpret error grpc codes of the external au…

…thz server as failure to fix failure_mode_allowed feature (envoyproxy#34951)" (envoyproxy#35637) Fixes envoyproxy#35610 Fixes commit envoyproxy#34951 Signed-off-by: Yan Avlasov <[email protected]> Signed-off-by: asingh-g <[email protected]>
asingh-g · Aug 20, 2024 · ec960e2 · ec960e2
1 parent 53f6132
commit ec960e2
Show file tree

Hide file tree

Showing 7 changed files with 29 additions and 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
@@ -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
   // <config_http_filters_ext_authz_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
@@ -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
   // <config_http_filters_ext_authz_stats>`.

diff --git 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
-    <envoy_v3_api_field_extensions.filters.http.ext_authz.v3.ExtAuthz.failure_mode_allow>`
-    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
@@ -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
@@ -114,11 +114,7 @@ void GrpcClientImpl::onSuccess(std::unique_ptr<envoy::service::auth::v3::CheckRe
       const auto& ok_response = response->ok_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_ptr<envoy::service::auth::v3::CheckRe
       }
       authz_response->body = 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
@@ -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
@@ -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<envoy::service::auth::v3::CheckResponse>();
+  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<ResponsePtr&>(
+                                      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<envoy::service::auth::v3::CheckResponse>();
-  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<ResponsePtr&>(
-                                      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<envoy::service::auth::v3::CheckResponse>();
-  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<ResponsePtr&>(
-                                      AuthzResponseNoAttributes(authz_response))));
-
-  client_->onSuccess(std::move(check_response), span_);
-}
-
 } // namespace ExtAuthz
 } // namespace Common
 } // namespace Filters