support "grpc-status-details-bin" (#334)

* support "grpc-status-details-bin" * Decode "grpc-status-details-bin" with base64 and parse into proto, and convert to json in response body. * Format fix. * Fix compiler error. * refactoring per comment. * Add test. * Fix test. * Remove unnecessary headers. * Revise per comment.
cloudendpoints · Jan 11, 2018 · c3d29d6 · c3d29d6
1 parent 8f2dfe4
commit c3d29d6
Show file tree

Hide file tree

Showing 8 changed files with 128 additions and 60 deletions.
diff --git a/include/api_manager/utils/status.h b/include/api_manager/utils/status.h
@@ -64,6 +64,13 @@ class Status final {
   // Constructs a Status object from a protobuf Status.
   static Status FromProto(const ::google::protobuf::util::Status& proto_status);
 
+  // Converts a |proto_status| of ::google::rpc::Status into a JSON string,
+  // and returns the JSON string in |result|. The options field is a OR'd set
+  // of the available JsonOptions. If the conversion failed, generates an error
+  // status and returns the error status in a JSON string.
+  static void StatusProtoToJson(const ::google::rpc::Status& proto_status,
+                                std::string* result, int options);
+
   // Pre-defined OK status.
   static const Status& OK;
 

diff --git a/src/api_manager/utils/status.cc b/src/api_manager/utils/status.cc
@@ -239,6 +239,23 @@ bool Status::operator==(const Status& x) const {
                 proto_status.error_message().ToString());
 }
 
+void Status::StatusProtoToJson(const ::google::rpc::Status& proto_status,
+                               std::string* result, int options) {
+  Status status = ProtoToJson(proto_status, result, options);
+  if (!status.ok()) {
+    // If translation failed, try outputting the json translation failure itself
+    // as a JSON error. This should only happen if one of the error details had
+    // an unresolvable type url.
+    ::google::rpc::Status proto = status.ToCanonicalProto();
+    status = ProtoToJson(proto, result, options);
+    if (!status.ok()) {
+      // This should never happen but just in case we do a non-json response.
+      *result = "Unable to generate error response: ";
+      result->append(status.message());
+    }
+  }
+}
+
 ::google::protobuf::util::Status Status::ToProto() const {
   ::google::protobuf::util::Status result(CanonicalCode(), message_);
   return result;
@@ -409,19 +426,7 @@ std::string Status::ToJson() const {
   ::google::rpc::Status proto = ToCanonicalProto();
   std::string result;
   int options = JsonOptions::PRETTY_PRINT | JsonOptions::OUTPUT_DEFAULTS;
-  Status status = ProtoToJson(proto, &result, options);
-  if (!status.ok()) {
-    // If translation failed, try outputting the json translation failure itself
-    // as a JSON error. This should only happen if one of the error details had
-    // an unresolvable type url.
-    proto = status.ToCanonicalProto();
-    status = ProtoToJson(proto, &result, options);
-    if (!status.ok()) {
-      // This should never happen but just in case we do a non-json response.
-      result = "Unable to generate error response: ";
-      result.append(status.message_);
-    }
-  }
+  Status::StatusProtoToJson(proto, &result, options);
   return result;
 }
 

diff --git a/src/nginx/error.cc b/src/nginx/error.cc
@@ -29,12 +29,13 @@
 //
 
 #include "src/nginx/error.h"
+#include "src/api_manager/utils/marshalling.h"
 #include "src/nginx/grpc_finish.h"
 #include "src/nginx/module.h"
 #include "src/nginx/util.h"
 
-using ::google::protobuf::util::error::Code;
 using ::google::api_manager::utils::Status;
+using ::google::protobuf::util::error::Code;
 
 namespace google {
 namespace api_manager {
@@ -207,8 +208,21 @@ ngx_int_t ngx_esp_error_body_filter(ngx_http_request_t *r, ngx_chain_t *in) {
         // Serialize error as JSON
         ngx_buf_t *body = nullptr;
         ngx_str_t json_error;
-        if (ngx_str_copy_from_std(r->pool, ctx->status.ToJson(), &json_error) !=
-            NGX_OK) {
+
+        // if there is grpc-status-detail-bin response header, generate error
+        // json body with that header.
+        if (ctx->grpc_status_details != nullptr) {
+          std::string status_details_in_json;
+          utils::Status::StatusProtoToJson(
+              *ctx->grpc_status_details, &status_details_in_json,
+              utils::JsonOptions::PRETTY_PRINT |
+                  utils::JsonOptions::OUTPUT_DEFAULTS);
+          if (ngx_str_copy_from_std(r->pool, status_details_in_json,
+                                    &json_error) != NGX_OK) {
+            return NGX_ERROR;
+          }
+        } else if (ngx_str_copy_from_std(r->pool, ctx->status.ToJson(),
+                                         &json_error) != NGX_OK) {
           return NGX_ERROR;
         }
 

diff --git a/src/nginx/module.h b/src/nginx/module.h
@@ -248,6 +248,9 @@ struct ngx_esp_request_ctx_s {
 
   // Nginx variable $backend_url. set from service config.
   ngx_str_t backend_url;
+
+  // Stores proto message of gRPC response header "grpc-status-details-bin".
+  std::unique_ptr<::google::rpc::Status> grpc_status_details;
 };
 
 static_assert(std::is_standard_layout<ngx_esp_request_ctx_t>::value,

diff --git a/src/nginx/t/transcoding_errors.t b/src/nginx/t/transcoding_errors.t
@@ -45,9 +45,9 @@ my $GrpcServerPort = ApiManager::pick_port();
 my $t = Test::Nginx->new()->has(qw/http proxy/)->plan(30);
 
 $t->write_file('service.pb.txt',
-  ApiManager::get_transcoding_test_service_config(
-    'endpoints-transcoding-test.cloudendpointsapis.com',
-    "http://127.0.0.1:${ServiceControlPort}"));
+    ApiManager::get_transcoding_test_service_config(
+        'endpoints-transcoding-test.cloudendpointsapis.com',
+        "http://127.0.0.1:${ServiceControlPort}"));
 
 ApiManager::write_file_expand($t, 'nginx.conf', <<EOF);
 %%TEST_GLOBALS%%
@@ -87,8 +87,8 @@ my $response = ApiManager::http_get($NginxPort,'/shelves?key=api-key-1');
 like($response, qr/HTTP\/1\.1 403 Forbidden/, 'Got a 403.');
 like($response, qr/Content-Type: application\/json/i, 'Content-type is application/json');
 like($response,
-  qr/API endpoints-transcoding-test.cloudendpointsapis.com is not enabled for the project./i,
-  "the message from service-control was propogated.");
+    qr/API endpoints-transcoding-test.cloudendpointsapis.com is not enabled for the project./i,
+    "the message from service-control was propogated.");
 
 
 # 2. The backend is not running yet. We should get 503.
@@ -116,18 +116,18 @@ is($t->waitforsocket("127.0.0.1:${GrpcServerPort}"), 1, "GRPC test server socket
 my $try = 0;
 my $max_tries = 5;
 while ($try < $max_tries) {
-  $response = ApiManager::http_get($NginxPort, '/shelves?key=test-api-key');
-  if ($response =~ qr/HTTP\/1.. 200 OK/) {
-    last;
-  } else {
-    # sleep 1 second before trying again
-    sleep(1);
-    $try++;
-  }
+    $response = ApiManager::http_get($NginxPort, '/shelves?key=test-api-key');
+    if ($response =~ qr/HTTP\/1.. 200 OK/) {
+        last;
+    } else {
+        # sleep 1 second before trying again
+        sleep(1);
+        $try++;
+    }
 }
 
 if ($try == $max_tries) {
-  fail('Failed to make a successful call after starting the backend.');
+    fail('Failed to make a successful call after starting the backend.');
 }
 
 ################################################################################
@@ -137,7 +137,8 @@ $response = ApiManager::http_get($NginxPort,'/shelves/100?key=api-key-3');
 
 like($response, qr/HTTP\/1\.1 404 Not Found/, 'Got a 404.');
 like($response, qr/Content-Type: application\/json/i, 'Content-type is application/json');
-like($response, qr/Shelf not found/i, "the message from the backend was propogated.");
+# This error message is from grpc-status-details-bin header from gRPC backend.
+like($response, qr/Cannot find shelf 100/i, "the message from the backend was propogated.");
 
 # 4. Calling a method that does not exist
 $response = ApiManager::http_get($NginxPort,'/non-existing-method?key=api-key-4');
@@ -208,20 +209,20 @@ $response = ApiManager::http_get($NginxPort,'/shelves/1');
 like($response, qr/HTTP\/1\.1 401 Unauthorized/, 'Got a 401.');
 like($response, qr/Content-Type: application\/json/i, 'Content-type is application/json');
 like($response, qr/Method doesn't allow unregistered callers/i,
-  'Got the "unregistered caller" message');
+    'Got the "unregistered caller" message');
 
 ################################################################################
 
 sub service_control {
-  my ($t, $port, $file) = @_;
+    my ($t, $port, $file) = @_;
 
-  my $server = HttpServer->new($port, $t->testdir() . '/' . $file)
-    or die "Can't create test server socket: $!\n";
+    my $server = HttpServer->new($port, $t->testdir() . '/' . $file)
+        or die "Can't create test server socket: $!\n";
 
-  local $SIG{PIPE} = 'IGNORE';
+    local $SIG{PIPE} = 'IGNORE';
 
-  # Failed check response
-  my $failed_check = ServiceControl::convert_proto(<<'EOF', 'check_response', 'binary');
+    # Failed check response
+    my $failed_check = ServiceControl::convert_proto(<<'EOF', 'check_response', 'binary');
 {
   "operationId": "ListShelves:7b3f4c4f-f29c-4391-b35e-0a676427fec8",
   "checkErrors": [
@@ -233,41 +234,41 @@ sub service_control {
 }
 EOF
 
-  my $first_check = 1;
+    my $first_check = 1;
 
-  $server->on_sub('POST', '/v1/services/endpoints-transcoding-test.cloudendpointsapis.com:check', sub {
-    my ($headers, $body, $client) = @_;
+    $server->on_sub('POST', '/v1/services/endpoints-transcoding-test.cloudendpointsapis.com:check', sub {
+            my ($headers, $body, $client) = @_;
 
-    # Fail the first check to simulate service-control error and succeed the rest
-    if ($first_check) {
-      print $client <<EOF . $failed_check;
+            # Fail the first check to simulate service-control error and succeed the rest
+            if ($first_check) {
+                print $client <<EOF . $failed_check;
 HTTP/1.1 200 OK
 Connection: close
 
 EOF
-      $first_check = 0;
-    } else {
-      print $client <<EOF;
+                $first_check = 0;
+            } else {
+                print $client <<EOF;
 HTTP/1.1 200 OK
 Content-Type: application/json
 Connection: close
 
 EOF
-    }
-  });
+            }
+        });
 
-  $server->on_sub('POST', '/v1/services/endpoints-transcoding-test.cloudendpointsapis.com:report', sub {
-    my ($headers, $body, $client) = @_;
-    print $client <<EOF;
+    $server->on_sub('POST', '/v1/services/endpoints-transcoding-test.cloudendpointsapis.com:report', sub {
+            my ($headers, $body, $client) = @_;
+            print $client <<EOF;
 HTTP/1.1 200 OK
 Content-Type: application/json
 Connection: close
 
 EOF
-  });
+        });
 
 
-  $server->run();
+    $server->run();
 }
 
-################################################################################
+################################################################################
diff --git a/src/nginx/transcoded_grpc_server_call.cc b/src/nginx/transcoded_grpc_server_call.cc
@@ -37,6 +37,8 @@
 #include "src/nginx/util.h"
 
 extern "C" {
+#include "src/core/lib/iomgr/exec_ctx.h"
+#include "src/core/lib/slice/b64.h"
 #include "src/http/v2/ngx_http_v2_module.h"
 }
 
@@ -46,7 +48,9 @@ namespace nginx {
 
 namespace {
 const ngx_str_t kContentTypeApplicationJson = ngx_string("application/json");
-}
+
+const std::string kGrpcStatusDetailsBin = "grpc-status-details-bin";
+}  // namespace
 
 NgxEspTranscodedGrpcServerCall::NgxEspTranscodedGrpcServerCall(
     ngx_http_request_t *r,
@@ -112,6 +116,30 @@ void NgxEspTranscodedGrpcServerCall::Finish(
   }
 
   if (!status.ok()) {
+    // If grpc response trailers have a "grpc-status-details-bin" header,
+    // use base64 to decode that value, parse it to proto and save it in status.
+    const auto &it = response_trailers.find(kGrpcStatusDetailsBin);
+    if (it != response_trailers.end() && !it->second.empty()) {
+      ngx_esp_request_ctx_t *ctx = ngx_http_esp_ensure_module_ctx(r_);
+      if (ctx) {
+        static grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT;
+        ::grpc::Slice value_slice(
+            grpc_base64_decode_with_len(&exec_ctx, it->second.c_str(),
+                                        it->second.length(), false),
+            ::grpc::Slice::STEAL_REF);
+        std::string binary_value(
+            reinterpret_cast<const char *>(value_slice.begin()),
+            value_slice.size());
+
+        ctx->grpc_status_details.reset(new ::google::rpc::Status);
+        if (!ctx->grpc_status_details->ParseFromString(binary_value)) {
+          ngx_log_error(
+              NGX_LOG_DEBUG, r_->connection->log, 0,
+              "Failed to parse grpc-status-details-bin header into proto.");
+          ctx->grpc_status_details.reset();
+        }
+      }
+    }
     HandleError(status);
     return;
   }

diff --git a/test/transcoding/BUILD b/test/transcoding/BUILD
@@ -48,6 +48,7 @@ cc_binary(
     deps = [
         ":bookstore",
         "//external:grpc++",
+        "//external:servicecontrol",
     ],
 )
 

diff --git a/test/transcoding/bookstore-server.cc b/test/transcoding/bookstore-server.cc
@@ -34,6 +34,7 @@
 #include "google/protobuf/util/json_util.h"
 #include "google/protobuf/util/type_resolver.h"
 #include "google/protobuf/util/type_resolver_util.h"
+#include "google/rpc/status.pb.h"
 #include "grpc++/grpc++.h"
 #include "test/transcoding/bookstore.grpc.pb.h"
 #include "test/transcoding/bookstore.pb.h"
@@ -157,7 +158,15 @@ class BookstoreServiceImpl : public Bookstore::Service {
       }
     }
 
-    return ::grpc::Status(grpc::NOT_FOUND, "Shelf not found");
+    ::google::rpc::Status detail_status;
+    detail_status.set_code(grpc::NOT_FOUND);
+    std::string error_details =
+        "Cannot find shelf " + std::to_string(request->shelf());
+    detail_status.set_message(error_details);
+    std::string detail_status_bin = detail_status.SerializeAsString();
+
+    return ::grpc::Status(grpc::NOT_FOUND, "Shelf not found",
+                          std::string(detail_status_bin));
   }
 
   ::grpc::Status DeleteShelf(::grpc::ServerContext*,
@@ -356,9 +365,9 @@ class BookstoreServiceImpl : public Bookstore::Service {
   std::multimap<int, Book> books_;
 };
 
-}  // namespace bookstore {
-}  // namespace examples {
-}  // namespace endpoints {
+}  // namespace bookstore
+}  // namespace examples
+}  // namespace endpoints
 
 int main(int argc, char** argv) {
   auto address = argc > 1 ? argv[1] : "localhost:8081";