You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2022/08/10 13:02:04 UTC
[GitHub] [apisix] spacewander commented on a diff in pull request #7639: feat: convert grpc-status-details-bin in header to HTTP response body…
spacewander commented on code in PR #7639:
URL: https://github.com/apache/apisix/pull/7639#discussion_r942337740
##########
apisix/plugins/grpc-transcode/response.lua:
##########
@@ -14,18 +14,77 @@
-- See the License for the specific language governing permissions and
-- limitations under the License.
--
-local util = require("apisix.plugins.grpc-transcode.util")
+local util = require("apisix.plugins.grpc-transcode.util")
+local grpc_proto = require("apisix.plugins.grpc-transcode.proto")
local core = require("apisix.core")
local pb = require("pb")
local ngx = ngx
local string = string
+local ngx_decode_base64 = ngx.decode_base64
+local ipairs = ipairs
+local pcall = pcall
-return function(ctx, proto, service, method, pb_option)
+
+local function handle_error_response(status_detail_type)
+ local headers = ngx.resp.get_headers()
+ local grpc_status = headers["grpc-status-details-bin"]
+ if grpc_status then
+ grpc_status = ngx_decode_base64(grpc_status)
+ if grpc_status == nil then
+ ngx.arg[1] = "grpc-status-details-bin is not base64 format"
+ return "grpc-status-details-bin is not base64 format"
+ end
+
+ local status_pb_state = grpc_proto.fetch_status_pb_state()
+ local old_pb_state = pb.state(status_pb_state)
+
+ local decoded_grpc_status = pb.decode("grpc.status.ErrorStatus", grpc_status)
Review Comment:
Should be wrapped with pcall
##########
apisix/plugins/grpc-transcode/response.lua:
##########
@@ -14,18 +14,77 @@
-- See the License for the specific language governing permissions and
-- limitations under the License.
--
-local util = require("apisix.plugins.grpc-transcode.util")
+local util = require("apisix.plugins.grpc-transcode.util")
+local grpc_proto = require("apisix.plugins.grpc-transcode.proto")
local core = require("apisix.core")
local pb = require("pb")
local ngx = ngx
local string = string
+local ngx_decode_base64 = ngx.decode_base64
+local ipairs = ipairs
+local pcall = pcall
-return function(ctx, proto, service, method, pb_option)
+
+local function handle_error_response(status_detail_type)
+ local headers = ngx.resp.get_headers()
+ local grpc_status = headers["grpc-status-details-bin"]
Review Comment:
Use ngx.header will be better?
##########
apisix/plugins/grpc-transcode.lua:
##########
@@ -72,6 +72,17 @@ local schema = {
"disable_hooks",
}
},
+ show_status_in_body = {
+ description = "show decoded grpc-status-details-bin in response body",
+ type = "boolean",
+ default = false
+ },
+ -- https://github.com/googleapis/googleapis/blob/master/google/rpc/status.proto#L46
Review Comment:
Better to use permalink like https://github.com/api7/grpc-client-nginx-module/blob/a5a9d1fea46878eab2f5cf4646bb902591a1d7b1/config#L7
##########
docs/en/latest/plugins/grpc-transcode.md:
##########
@@ -44,6 +44,9 @@ APISIX takes in an HTTP request, transcodes it and forwards it to a gRPC service
| method | string | True | | Method name of the gRPC service. |
| deadline | number | False | 0 | Deadline for the gRPC service in ms. |
| pb_option | array[string([pb_option_def](#options-for-pb_option))] | False | | protobuf options. |
+| show_status_in_body | boolean | False | False | Whether to display the parsed `grpc-status-details-bin` in the response body |
+| status_detail_type | string | False | | The message type corresponding to the [details](https://github.com/googleapis/googleapis/blob/master/google/rpc/status.proto#L46) part of `grpc-status-details-bin`, if not specified, this part will not be decoded |
Review Comment:
Better to use permalink
##########
docs/en/latest/plugins/grpc-transcode.md:
##########
@@ -44,6 +44,9 @@ APISIX takes in an HTTP request, transcodes it and forwards it to a gRPC service
| method | string | True | | Method name of the gRPC service. |
| deadline | number | False | 0 | Deadline for the gRPC service in ms. |
| pb_option | array[string([pb_option_def](#options-for-pb_option))] | False | | protobuf options. |
+| show_status_in_body | boolean | False | False | Whether to display the parsed `grpc-status-details-bin` in the response body |
Review Comment:
```suggestion
| show_status_in_body | boolean | False | false | Whether to display the parsed `grpc-status-details-bin` in the response body |
```
##########
t/plugin/grpc-transcode3.t:
##########
@@ -122,3 +122,447 @@ POST /grpctest
Content-Type: application/json
--- response_body chomp
{"message":"Hello world, name: Joe, age: 1, name: Jake, age: 2"}
+
+
+
+=== TEST 3: set proto (id: 1, get error response from rpc)
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+
+ local code, body = t('/apisix/admin/proto/1',
+ ngx.HTTP_PUT,
+ [[{
+ "content" : "syntax = \"proto3\";
+ package helloworld;
+ service Greeter {
+ rpc GetErrResp (HelloRequest) returns (HelloReply) {}
+ }
+ message HelloRequest {
+ string name = 1;
+ repeated string items = 2;
+ }
+ message HelloReply {
+ string message = 1;
+ repeated string items = 2;
+ }
+ message ErrorDetail {
+ int64 code = 1;
+ string message = 2;
+ string type = 3;
+ }"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
Review Comment:
These sections are defined at the top.
##########
t/grpc_server_example/main.go:
##########
@@ -95,6 +95,22 @@ func (s *server) SayHello(ctx context.Context, in *pb.HelloRequest) (*pb.HelloRe
}, nil
}
+// GetErrResp implements helloworld.GreeterServer
+func (s *server) GetErrResp(ctx context.Context, in *pb.HelloRequest) (*pb.HelloReply, error) {
+ st := status.New(codes.Unavailable, "Out of service")
+ st, err := st.WithDetails(&pb.ErrorDetail{
Review Comment:
Please use gofmt to format the code
##########
t/plugin/grpc-transcode3.t:
##########
@@ -122,3 +122,447 @@ POST /grpctest
Content-Type: application/json
--- response_body chomp
{"message":"Hello world, name: Joe, age: 1, name: Jake, age: 2"}
+
+
+
+=== TEST 3: set proto (id: 1, get error response from rpc)
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+
+ local code, body = t('/apisix/admin/proto/1',
+ ngx.HTTP_PUT,
+ [[{
+ "content" : "syntax = \"proto3\";
+ package helloworld;
+ service Greeter {
+ rpc GetErrResp (HelloRequest) returns (HelloReply) {}
+ }
+ message HelloRequest {
+ string name = 1;
+ repeated string items = 2;
+ }
+ message HelloReply {
+ string message = 1;
+ repeated string items = 2;
+ }
+ message ErrorDetail {
+ int64 code = 1;
+ string message = 2;
+ string type = 3;
+ }"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 4: set routes (id: 1, get error response from rpc)
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "methods": ["GET", "POST"],
+ "uri": "/grpctest",
+ "plugins": {
+ "grpc-transcode": {
+ "proto_id": "1",
+ "service": "helloworld.Greeter",
+ "method": "GetErrResp"
+ }
+ },
+ "upstream": {
+ "scheme": "grpc",
+ "type": "roundrobin",
+ "nodes": {
+ "127.0.0.1:50051": 1
+ }
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 5: hit route (error response in header)
+--- config
+ location /t {
+ content_by_lua_block {
+ local json = require("toolkit.json")
+ local t = require("lib.test_admin").test
+
+ local code, body, headers = t('/grpctest?name=world',
+ ngx.HTTP_GET
+ )
+
+ ngx.status = code
+
+ ngx.header['grpc-status'] = headers['grpc-status']
+ ngx.header['grpc-message'] = headers['grpc-message']
+ ngx.header['grpc-status-details-bin'] = headers['grpc-status-details-bin']
+
+ body = json.encode(body)
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_headers
+grpc-status: 14
+grpc-message: Out of service
+grpc-status-details-bin: CA4SDk91dCBvZiBzZXJ2aWNlGlcKKnR5cGUuZ29vZ2xlYXBpcy5jb20vaGVsbG93b3JsZC5FcnJvckRldGFpbBIpCAESHFRoZSBzZXJ2ZXIgaXMgb3V0IG9mIHNlcnZpY2UaB3NlcnZpY2U
+--- response_body_unlike eval
+qr/error/
+--- no_error_log
+[error]
+--- error_code: 503
+
+
+
+=== TEST 6: set routes (id: 1, show error response in body)
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "methods": ["GET", "POST"],
+ "uri": "/grpctest",
+ "plugins": {
+ "grpc-transcode": {
+ "proto_id": "1",
+ "service": "helloworld.Greeter",
+ "method": "GetErrResp",
+ "show_status_in_body": true
+ }
+ },
+ "upstream": {
+ "scheme": "grpc",
+ "type": "roundrobin",
+ "nodes": {
+ "127.0.0.1:50051": 1
+ }
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 7: hit route (show error status in body)
+--- config
+ location /t {
+ content_by_lua_block {
+ local json = require("toolkit.json")
+ local t = require("lib.test_admin").test
+
+ local code, body, headers = t('/grpctest?name=world',
+ ngx.HTTP_GET
+ )
+
+ ngx.status = code
+
+ ngx.header['grpc-status'] = headers['grpc-status']
+ ngx.header['grpc-message'] = headers['grpc-message']
+ ngx.header['grpc-status-details-bin'] = headers['grpc-status-details-bin']
+
+ body = json.decode(body)
+ body = json.encode(body)
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_headers
+grpc-status: 14
+grpc-message: Out of service
+grpc-status-details-bin: CA4SDk91dCBvZiBzZXJ2aWNlGlcKKnR5cGUuZ29vZ2xlYXBpcy5jb20vaGVsbG93b3JsZC5FcnJvckRldGFpbBIpCAESHFRoZSBzZXJ2ZXIgaXMgb3V0IG9mIHNlcnZpY2UaB3NlcnZpY2U
+--- response_body
+{"error":{"code":14,"details":[{"type_url":"type.googleapis.com/helloworld.ErrorDetail","value":"\b\u0001\u0012\u001cThe server is out of service\u001a\u0007service"}],"message":"Out of service"}}
+--- no_error_log
+[error]
+--- error_code: 503
+
+
+
+=== TEST 8: set routes (id: 1, show error details in body)
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "methods": ["GET", "POST"],
+ "uri": "/grpctest",
+ "plugins": {
+ "grpc-transcode": {
+ "proto_id": "1",
+ "service": "helloworld.Greeter",
+ "method": "GetErrResp",
+ "show_status_in_body": true,
+ "status_detail_type": "helloworld.ErrorDetail"
+ }
+ },
+ "upstream": {
+ "scheme": "grpc",
+ "type": "roundrobin",
+ "nodes": {
+ "127.0.0.1:50051": 1
+ }
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 9: hit route (show error details in body)
+--- config
+ location /t {
+ content_by_lua_block {
+ local json = require("toolkit.json")
+ local t = require("lib.test_admin").test
+
+ local code, body, headers = t('/grpctest?name=world',
+ ngx.HTTP_GET
+ )
+
+ ngx.status = code
+
+ ngx.header['grpc-status'] = headers['grpc-status']
+ ngx.header['grpc-message'] = headers['grpc-message']
+ ngx.header['grpc-status-details-bin'] = headers['grpc-status-details-bin']
+
+ body = json.decode(body)
+ body = json.encode(body)
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_headers
+grpc-status: 14
+grpc-message: Out of service
+grpc-status-details-bin: CA4SDk91dCBvZiBzZXJ2aWNlGlcKKnR5cGUuZ29vZ2xlYXBpcy5jb20vaGVsbG93b3JsZC5FcnJvckRldGFpbBIpCAESHFRoZSBzZXJ2ZXIgaXMgb3V0IG9mIHNlcnZpY2UaB3NlcnZpY2U
+--- response_body
+{"error":{"code":14,"details":[{"code":1,"message":"The server is out of service","type":"service"}],"message":"Out of service"}}
+--- no_error_log
+[error]
+--- error_code: 503
+
+
+
+=== TEST 10: set routes (id: 1, show error details in body)
Review Comment:
What is the difference between TEST 8~9 and TEST 10~11?
##########
t/plugin/grpc-transcode3.t:
##########
@@ -122,3 +122,447 @@ POST /grpctest
Content-Type: application/json
--- response_body chomp
{"message":"Hello world, name: Joe, age: 1, name: Jake, age: 2"}
+
+
+
+=== TEST 3: set proto (id: 1, get error response from rpc)
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+
+ local code, body = t('/apisix/admin/proto/1',
+ ngx.HTTP_PUT,
+ [[{
+ "content" : "syntax = \"proto3\";
+ package helloworld;
+ service Greeter {
+ rpc GetErrResp (HelloRequest) returns (HelloReply) {}
+ }
+ message HelloRequest {
+ string name = 1;
+ repeated string items = 2;
+ }
+ message HelloReply {
+ string message = 1;
+ repeated string items = 2;
+ }
+ message ErrorDetail {
+ int64 code = 1;
+ string message = 2;
+ string type = 3;
+ }"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 4: set routes (id: 1, get error response from rpc)
Review Comment:
We can merge TEST 3&4 into one, like the TEST 1?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org