feat: convert grpc-status-details-bin in header to HTTP response body… by monkeyDluffy6017 · Pull Request #7639 · apache/apisix

monkeyDluffy6017 · 2022-08-10T09:28:13Z

Description

Convert grpc-status-details-bin in response header to response body, so that users could avoid parsing header to read error response.

Fixes #6996

Checklist

I have explained the need for this PR and the problem it solves
I have explained the changes or the new features added to this PR
I have added tests corresponding to this change
I have updated the documentation to reflect this change
I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

spacewander · 2022-08-10T11:26:26Z

apisix/plugins/grpc-transcode.lua

+            type        = "boolean",
+            default     = false
+        },
+        -- https://github.com/googleapis/googleapis/blob/master/google/rpc/status.proto#L46


Better to use permalink like https://github.com/api7/grpc-client-nginx-module/blob/a5a9d1fea46878eab2f5cf4646bb902591a1d7b1/config#L7

Do you mean link after api7?

You should use GitHub's permalink feature, not a link to the master.

spacewander · 2022-08-10T11:28:21Z

apisix/plugins/grpc-transcode/response.lua

+        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)


Should be wrapped with pcall

spacewander · 2022-08-10T11:29:30Z

apisix/plugins/grpc-transcode/response.lua

+
+local function handle_error_response(status_detail_type)
+    local headers = ngx.resp.get_headers()
+    local grpc_status = headers["grpc-status-details-bin"]


Use ngx.header will be better?

spacewander · 2022-08-10T11:35:22Z

docs/en/latest/plugins/grpc-transcode.md

 | 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  |


Better to use permalink

spacewander · 2022-08-10T11:35:50Z

docs/en/latest/plugins/grpc-transcode.md

 | 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 |


spacewander · 2022-08-10T11:36:29Z

t/grpc_server_example/main.go

+// 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{


Please use gofmt to format the code

spacewander · 2022-08-10T11:38:03Z

t/plugin/grpc-transcode3.t

+            ngx.say(body)
+        }
+    }
+--- request


These sections are defined at the top.

What do you mean? section 3 should be merge into section 1, or section 3 should be after section 1?

--- request is set in

apisix/t/plugin/grpc-transcode3.t

Line 27 in c4d5f2f

$block->set_value("request", "GET /t");

. There is no need to set it again.

spacewander · 2022-08-10T11:40:10Z

t/plugin/grpc-transcode3.t

+
+
+
+=== TEST 10: set routes (id: 1, show error details in body)


What is the difference between TEST 8~~9 and TEST 10~~11?

They are duplicated, deleted.

spacewander · 2022-08-10T13:01:43Z

t/plugin/grpc-transcode3.t

+
+
+
+=== TEST 4: set routes (id: 1, get error response from rpc)


We can merge TEST 3&4 into one, like the TEST 1?

spacewander · 2022-08-10T13:14:45Z

Don't forget to install the proto file:

apisix/Makefile

Line 267 in ccc43ea

    
           $(ENV_INSTALL) apisix/include/apisix/model/*.proto $(ENV_INST_LUADIR)/apisix/include/apisix/model/

…apache#6996)

…o-http-response-body

spacewander · 2022-08-12T06:20:57Z

Please make the CI pass, thanks!

monkeyDluffy6017 · 2022-08-12T06:24:57Z

Please make the CI pass, thanks!

I'm working on it.

…o-http-response-body

…se-body' of github.com:monkeyDluffy6017/apisix into covert-grpc-status-details-bin-in-header-to-http-response-body

spacewander · 2022-08-16T06:23:15Z

apisix/plugins/grpc-transcode/proto.lua

+        -- initialize protoc compiler
+        protoc.reload()
+        local status_protoc = protoc.new()
+        -- do not use loadfile here, it can not load proto file when use relative address


Suggested change

-- do not use loadfile here, it can not load proto file when use relative address

-- do not use loadfile here, it can not load the proto file when using a relative address

spacewander · 2022-08-16T06:26:11Z

apisix/plugins/grpc-transcode/response.lua

+        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"


We can exact the error message to a variable so we don't need to repeat them?

spacewander · 2022-08-16T06:26:58Z

apisix/plugins/grpc-transcode/response.lua

+        local status_pb_state = grpc_proto.fetch_status_pb_state()
+        local old_pb_state = pb.state(status_pb_state)
+
+        local ok, decoded_grpc_status = pcall(pb.decode, "grpc_status.ErrorStatus", grpc_status)


We should restore the pb_state here so the error won't pollute the pb_state

spacewander · 2022-08-16T06:27:44Z

docs/en/latest/plugins/grpc-transcode.md


+## Show `grpc-status-details-bin` in response body
+
+If the gRPC service returns an error, there may be a `grpc-status-details-bin` field in the return header describing the error, which you can decode and display in the return body.


Suggested change

If the gRPC service returns an error, there may be a `grpc-status-details-bin` field in the return header describing the error, which you can decode and display in the return body.

If the gRPC service returns an error, there may be a `grpc-status-details-bin` field in the response header describing the error, which you can decode and display in the response body.

spacewander · 2022-08-16T06:30:12Z

t/plugin/grpc-transcode3.t

+    }
+--- response_body
+passed
+--- no_error_log


The no_error_log is already set at the top of this file

spacewander · 2022-08-18T05:55:21Z

apisix/plugins/grpc-transcode/proto.lua

+        -- do not use loadfile here, it can not load the proto file when using a relative address
+        -- after luarocks install apisix
+        local ok, err = status_protoc:load(grpc_status_proto, "grpc_status.proto")
+        if not ok then


We should set the old_pb_state back when the error happened?

spacewander · 2022-08-18T05:58:51Z

t/plugin/plugin.t

-            for file_name in lfs.dir(ngx.config.prefix() .. "/../../apisix/plugins/") do
-                if string.match(file_name, ".lua$") then
-                    local expected = file_name:sub(1, #file_name - 4)
+            --local lfs = require("lfs")


Yes, forget to recovery, i have checked that there is no same problem

…o-http-response-body

github-actions · 2022-11-12T10:04:35Z

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@apisix.apache.org list. Thank you for your contributions.

…o-http-response-body

spacewander reviewed Aug 10, 2022

View reviewed changes

monkeyDluffy6017 and others added 3 commits August 10, 2022 22:15

feat: convert grpc-status-details-bin in header to HTTP response body (…

5caf726

…apache#6996)

change: minor tweaks.

cd98b50

test: change the proto uri

0b2999a

monkeyDluffy6017 force-pushed the covert-grpc-status-details-bin-in-header-to-http-response-body branch from a2e935d to 0b2999a Compare August 10, 2022 15:03

levy001 and others added 2 commits August 12, 2022 10:21

Merge branch 'master' into covert-grpc-status-details-bin-in-header-t…

ebaf9a1

…o-http-response-body

doc: fix doc lint error.

2627139

monkeyDluffy6017 and others added 14 commits August 12, 2022 14:34

change: minor tweaks.

cf81984

Merge branch 'master' into covert-grpc-status-details-bin-in-header-t…

3680e65

…o-http-response-body

change: minor tweaks.

eab7556

change: minor tweaks

14bb8fe

bugfix: recovery pb state after create a new one

082748a

test: remove redundant --- LAST

993f52f

change: for test

43ba525

change: for test

13d4556

change: use grpc_status.proto which is lua string

f1f4fa2

Merge branch 'master' into covert-grpc-status-details-bin-in-header-t…

ebb6ec2

…o-http-response-body

change: for test

c631ef0

change: use permalink

3721e7d

Merge branch 'covert-grpc-status-details-bin-in-header-to-http-respon…

da6ffef

…se-body' of github.com:monkeyDluffy6017/apisix into covert-grpc-status-details-bin-in-header-to-http-response-body

change: resolve code lint error

2787472

spacewander reviewed Aug 16, 2022

View reviewed changes

monkeyDluffy6017 added 2 commits August 16, 2022 15:06

change: resolve review comment

48871b7

change: optimize error log format

ac34232

monkeyDluffy6017 requested a review from spacewander August 18, 2022 04:39

spacewander reviewed Aug 18, 2022

View reviewed changes

spacewander mentioned this pull request Aug 18, 2022

fix: recovery old pb state after pubsub.new #7728

Merged

5 tasks

spacewander reviewed Aug 18, 2022

View reviewed changes

fix: recovery old pb state after return error

b7fd88b

monkeyDluffy6017 requested a review from spacewander August 19, 2022 02:12

spacewander approved these changes Aug 19, 2022

View reviewed changes

Merge branch 'master' into covert-grpc-status-details-bin-in-header-t…

cdfd619

…o-http-response-body

tzssangglass requested review from soulbird, tokers and tzssangglass September 13, 2022 06:55

github-actions bot added the stale label Nov 12, 2022

Merge branch 'master' into covert-grpc-status-details-bin-in-header-t…

61b185e

…o-http-response-body

tzssangglass approved these changes Nov 21, 2022

View reviewed changes

spacewander merged commit 02f9fa0 into apache:master Nov 21, 2022

	\| show_status_in_body \| boolean \| False \| False \| Whether to display the parsed `grpc-status-details-bin` in the response body \|
	\| show_status_in_body \| boolean \| False \| false \| Whether to display the parsed `grpc-status-details-bin` in the response body \|

	-- do not use loadfile here, it can not load proto file when use relative address
	-- do not use loadfile here, it can not load the proto file when using a relative address


		## Show `grpc-status-details-bin` in response body

		If the gRPC service returns an error, there may be a `grpc-status-details-bin` field in the return header describing the error, which you can decode and display in the return body.

Conversation

monkeyDluffy6017 commented Aug 10, 2022

Description

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacewander commented Aug 10, 2022

Uh oh!

spacewander commented Aug 12, 2022

Uh oh!

monkeyDluffy6017 commented Aug 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment