Fix the unmatched request handling by set the grpc status in payload. (…

…#223) * mst fix e2e with hardcode. Signed-off-by: Jianfei Hu <[email protected]> * wip organize the convert code. Signed-off-by: Jianfei Hu <[email protected]> * update example to use localhost Signed-off-by: Jianfei Hu <[email protected]> * make format. Signed-off-by: Jianfei Hu <[email protected]>
istio-ecosystem · Jul 28, 2022 · efe8187 · efe8187
1 parent 28404a8
commit efe8187
Show file tree

Hide file tree

Showing 4 changed files with 67 additions and 38 deletions.
diff --git a/bookinfo-example/authservice/templates/config.yaml b/bookinfo-example/authservice/templates/config.yaml
@@ -20,9 +20,14 @@ data:
       "listen_port": "10003",
       "log_level": "trace",
       "threads": 8,
+      "allow_unmatched_requests": "false",
       "chains": [
         {
           "name": "idp_filter_chain",
+          "match": {
+            "header": ":authority",
+            "prefix": "localhost",
+          },
           "filters": [
           {
             "oidc":

diff --git a/src/service/BUILD b/src/service/BUILD
@@ -35,6 +35,7 @@ authsvc_cc_library(
         "@com_github_gabime_spdlog//:spdlog",
         "@com_github_grpc_grpc//:grpc++",
         "@envoy//source/common/config:version_converter_lib",
+        # "@envoy//envoy/grpc:status",
         "@envoy_api//envoy/service/auth/v2:pkg_cc_grpc",
         "@envoy_api//envoy/service/auth/v3:pkg_cc_grpc",
     ],

diff --git a/src/service/async_service_impl.cc b/src/service/async_service_impl.cc
@@ -18,6 +18,33 @@ namespace {
 constexpr uint16_t kHealthCheckServerPort = 10004;
 }
 
+::grpc::Status convertGrpcStatus(const google::rpc::Code status) {
+  // See src/filters/filter.h:filter::Process for a description of how
+  // status codes should be handled
+  switch (status) {
+    case google::rpc::Code::OK:                 // The request was successful
+    case google::rpc::Code::UNAUTHENTICATED:    // A filter indicated the
+                                                // request had no
+                                                // authentication but was
+                                                // processed correctly.
+    case google::rpc::Code::PERMISSION_DENIED:  // A filter indicated
+      // insufficient permissions
+      // for the authenticated
+      // requester but was processed
+      // correctly.
+      return ::grpc::Status::OK;
+    case google::rpc::Code::INVALID_ARGUMENT:  // The request was not well
+      // formed. Indicate a
+      // processing error to the
+      // caller.
+      return ::grpc::Status(::grpc::StatusCode::INVALID_ARGUMENT,
+                            "invalid request");
+    default:  // All other errors are treated as internal processing
+              // failures.
+      return ::grpc::Status(::grpc::StatusCode::INTERNAL, "internal error");
+  }
+}
+
 ProcessingStateV2::ProcessingStateV2(
     ProcessingStateFactory &parent,
     envoy::service::auth::v2::Authorization::AsyncService &service)

diff --git a/src/service/async_service_impl.h b/src/service/async_service_impl.h
@@ -12,6 +12,7 @@
 #include "common/config/version_converter.h"
 #include "config/config.pb.h"
 #include "envoy/common/exception.h"
+// #include "envoy/grpc/status.h"
 #include "envoy/service/auth/v2/external_auth.grpc.pb.h"
 #include "envoy/service/auth/v3/external_auth.grpc.pb.h"
 #include "src/common/http/http.h"
@@ -22,6 +23,8 @@
 namespace authservice {
 namespace service {
 
+::grpc::Status convertGrpcStatus(const google::rpc::Code status);
+
 template <class RequestType, class ResponseType>
 ::grpc::Status Check(
     const RequestType &request, ResponseType &response,
@@ -59,14 +62,6 @@ ::grpc::Status Check(
       return ::grpc::Status::OK;
     }
 
-    // TODO(incfly): Clean up trigger rule after checking the current Istio
-    // ExtAuthz API is sufficient.
-    const auto default_response_code =
-        allow_unmatched_requests
-            ? grpc::Status::OK
-            : grpc::Status(grpc::StatusCode::PERMISSION_DENIED,
-                           "permission denied");
-
     // Find a configured processing chain.
     for (auto &chain : chains) {
       if (chain->Matches(&request_v3)) {
@@ -80,7 +75,6 @@ ::grpc::Status Check(
         // Create a new instance of a processor.
         auto processor = chain->New();
         auto status = processor->Process(&request_v3, &response_v3, ioc, yield);
-
         // response v2/v3 conversion layer
         if constexpr (std::is_same_v<
                           ResponseType,
@@ -97,42 +91,44 @@ ::grpc::Status Check(
                                   ::envoy::service::auth::v3::CheckResponse>) {
           response = response_v3;
         }
-
-        // See src/filters/filter.h:filter::Process for a description of how
-        // status codes should be handled
-        switch (status) {
-          case google::rpc::Code::OK:  // The request was successful
-          case google::rpc::Code::UNAUTHENTICATED:    // A filter indicated the
-                                                      // request had no
-                                                      // authentication but was
-                                                      // processed correctly.
-          case google::rpc::Code::PERMISSION_DENIED:  // A filter indicated
-            // insufficient permissions
-            // for the authenticated
-            // requester but was processed
-            // correctly.
-            return ::grpc::Status::OK;
-          case google::rpc::Code::INVALID_ARGUMENT:  // The request was not well
-            // formed. Indicate a
-            // processing error to the
-            // caller.
-            return ::grpc::Status(::grpc::StatusCode::INVALID_ARGUMENT,
-                                  "invalid request");
-          default:  // All other errors are treated as internal processing
-                    // failures.
-            return ::grpc::Status(::grpc::StatusCode::INTERNAL,
-                                  "internal error");
-        }
+        return convertGrpcStatus(status);
       }
     }
+    // No matching filter chain found.
+
+    // TODO(incfly): Clean up trigger rule after checking the current Istio
+    // ExtAuthz API is sufficient.
+    auto default_response_code =
+        grpc::Status(grpc::StatusCode::PERMISSION_DENIED, "permission denied");
+    google::rpc::Code code = google::rpc::Code::PERMISSION_DENIED;
+    if (allow_unmatched_requests) {
+      default_response_code = grpc::Status::OK;
+      code = google::rpc::Code::OK;
+    }
+    envoy::service::auth::v2::CheckResponse response_v2;
+    envoy::service::auth::v3::CheckResponse response_v3;
+    response_v2.mutable_status()->set_code(code);
+    response_v3.mutable_status()->set_code(code);
+    if constexpr (std::is_same_v<ResponseType,
+                                 ::envoy::service::auth::v2::CheckResponse>) {
+      response = response_v2;
+    } else if (std::is_same_v<ResponseType,
+                              ::envoy::service::auth::v3::CheckResponse>) {
+      response = response_v3;
+    }
+
+    if constexpr (std::is_same_v<ResponseType,
+                                 ::envoy::service::auth::v3::CheckResponse>) {
+      response = response_v3;
+    }
 
-    // No matching filter chain found. Allow request to continue.
     spdlog::debug(
-        "{}: no matching filter chain for request to {}://{}{}, respond with: "
+        "{}: no matching filter chain for request to "
+        "{}://{}{}, allow_unmatched_requests {}, respond with: "
         "{}",
         __func__, request.attributes().request().http().scheme(),
         request.attributes().request().http().host(),
-        request.attributes().request().http().path(),
+        request.attributes().request().http().path(), allow_unmatched_requests,
         default_response_code.error_code());
     return default_response_code;
   } catch (const std::exception &exception) {