Adding http to grpc status code mapping #211

juanmolle · 2023-10-24T18:29:18Z

Instead of sending unrelated -1 grps status code map the http code into a representative grpc code.

Related issue @#148

google-cla · 2023-10-24T18:29:23Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

juanmolle · 2023-10-24T19:04:59Z

src/hostcalls.rs

@@ -703,6 +703,44 @@ extern "C" {
    ) -> Status;
 }

+#[allow(dead_code)]
+enum GrpcStatucCode {


should this enum be moved to types?

PiotrSikora · 2023-10-24T19:35:43Z

src/hostcalls.rs

@@ -718,7 +756,7 @@ pub fn send_http_response(
            body.map_or(0, |body| body.len()),
            serialized_headers.as_ptr(),
            serialized_headers.len(),
-            -1,
+            http_to_grpc_status_code(status_code),


I don't believe this is correct. It appear to be always sending gRPC status code, even for non-gRPC responses.

that's true. If I create a wasm that just create a local reply and I make a call with grpcurl, the error I get is the code -1, then current implementation is always sending an status code and envoy is forwarding to the client. How do you think we should fix this scenario?

Interesting, there is already a mapping in envoy (not the same I have created)
https://github.com/envoyproxy/envoy/blob/main/source/common/grpc/status.cc#L6

and I guess from envoy doc, will use this value to override that
https://github.com/envoyproxy/envoy/blob/main/envoy/http/filter.h#L579

What seams to be happening is that in the wasm context the value sent from this function (-1). is directly sent here
https://github.com/envoyproxy/envoy/blob/main/source/extensions/common/wasm/context.cc#L1672

and set always a value to the optional, and does not let envoy to automatically map to the corresponding grpc code from http code, because any value sent will override that

probably it would be cleaner if we can instruct envoy to interpret -1 as null (because 0 is a valid status code) to forward that an an empty optional and let envoy do it's magic., saying that, I still think that sending a meaning value could be better than sending -1.

Let me shared the test I have done, first a call without the change, and after that calls with a wasm filter that do just basic auth, the first request show the -1 (4294967295) currently sent by the wasm without the change

Old wasm

jolle@jolle-ltmrmbv InstallFromScratch % grpcurl -d '{"greeting": "pepe"}' --plaintext -protoset grpc/hello.proto-descriptor_grpc localhost:8080 hello.HelloService.SayHello
ERROR:
Code: Internal
Message: transport: malformed grpc-status: strconv.ParseInt: parsing "4294967295": value out of range

New Wasm rejecting the call no auth header

% grpcurl -d '{"greeting": "pepe"}' --plaintext -protoset grpc/hello.proto-descriptor_grpc localhost:8080 hello.HelloService.SayHello
ERROR:
Code: Unauthenticated
Message: {"error":"Registered authentication is set to HTTP basic authentication but there was no security context on the session."}

New wasm with invalid user:pass

% grpcurl -expand-headers -H 'Authorization: Basic YXNkOmFzZA==' -d '{"greeting": "pepe"}' --plaintext -protoset grpc/hello.proto-descriptor_grpc localhost:8080 hello.HelloService.SayHello
ERROR:
Code: Unauthenticated
Message: {"error":"Authentication Attempt Failed"}

Server rejected the query (proto not corresponding with server)

% grpcurl -expand-headers -H 'Authorization: Basic YWRtaW46dGVzdA==' -d '{"greeting": "pepe"}' --plaintext -protoset grpc/hello.proto-descriptor_grpc localhost:8080 hello.HelloService.SayHello
ERROR:
Code: Unimplemented
Message: unknown service hello.HelloService

OK

% grpcurl -expand-headers -H 'Authorization: Basic YWRtaW46dGVzdA==' -d '{"greeting": "pepe"}' --plaintext -protoset grpc/hello.proto-descriptor_grpc localhost:8080 hello.HelloService.SayHello
{
"reply": "hello pepe"
}

Signed-off-by: Juan Manuel Ollé <[email protected]>

juanmolle · 2023-11-03T18:33:46Z

@PiotrSikora any thought about the explanation and use cases I have described?

PiotrSikora · 2023-11-07T21:54:12Z

@PiotrSikora any thought about the explanation and use cases I have described?

This seems to be a real issue that needs to be fixed, but the solution is not to emit any grpc-status code when not requested.

Emitting grpc-status code when sending a plain HTTP (i.e. non-gRPC) response is trading one incorrect behavior for another.

I suspect there was a regression on the Envoy side, since I cannot imagine we were emitting grpc-status = -1 from the beginning. @mpwarres could you please take a look?

PiotrSikora · 2023-11-09T06:31:10Z

Thinking about this a bit more, maybe adding send_grpc_response where users can provide grpc-status would be a better solution?

juanmolle · 2023-11-09T21:17:43Z

not sure adding a new method, many samples does not know in the wasm if the request comes from a grpc client. for example native ext_authz does not include a grpc status code but works with grpcurl. due to the fact that envoy does not support optionals, the other option is changing the signature, but unfortunately will change the interface and I guess there are no default values nor overload like there are in c++. I still thinking for viable options

PiotrSikora · 2023-11-14T06:53:44Z

not sure adding a new method, many samples does not know in the wasm if the request comes from a grpc client.

I guess the question is what problem are you trying to solve?

Based on the PR, I assumed you wanted to emit custom gRPC responses with correct grpc-status.

However, your response suggests that you want to use grpcurl tool to work with plain HTTP (i.e. non-gRPC) responses? While emitting grpc-status isn't great, it's a legitimate HTTP header, so it's presence shouldn't be breaking anything for non-gRPC responses.

Also, per gRPC spec, the only valid HTTP response code is 200 OK, so the grpc-status for non-200 status codes shouldn't be interpreted anyway.

juanmolle · 2023-11-14T12:55:20Z

guess the question is what problem are you trying to solve?

When a grpc client try to connect to grpc server and a wasm filter is in the filter chain that replay with unautorized, the wasm replay with -1 that is an unknown grpc status code, no letting the clien know what had happened

'''
Code: Internal
Message: transport: malformed grpc-status: strconv.ParseInt: parsing "4294967295": value out of range
'''

This is not happening with native filters like ext_authz, because the filter sent null as optional value and envoy translate the http unauthorized code into the equivalent grpc status

But I get your point. I will go to envoy again to in witch condition the grpc status code is infer from the http status code.

just to be in the same page the correct fix should be in envoy wasm filter to allows the grpc code be optional, but that request an api change that I guess will to be easy to do. In addition forcing the grpc status code to be always -1 is incorrect too and I though a situation in the middle when instead for sending invalid -1 was better.

The other not elegant workaround is to translate the -1 into empty optional in the wasm filter, but the multiple conversions between int32 -> uint32 -> uint64 make even less elegant. ( and I don't really know if all arch will behave equal)

PiotrSikora · 2023-11-14T18:43:38Z

When a grpc client try to connect to grpc server and a wasm filter is in the filter chain that replay with unautorized, the wasm replay with -1 that is an unknown grpc status code, no letting the clien know what had happened

''' Code: Internal Message: transport: malformed grpc-status: strconv.ParseInt: parsing "4294967295": value out of range '''

Right, but the correct response in that case is 200 OK with grpc-status: UNAUTHENTICATED or grpc-status: PERMISSION_DENIED, not 401 Unauthorized.

And since the existing ABI doesn't support trailers in HTTP callouts, the only way to emit that from the Rust SDK would be to add send_grpc_response, as mentioned earlier.

This is not happening with native filters like ext_authz, because the filter sent null as optional value and envoy translate the http unauthorized code into the equivalent grpc status

But I get your point. I will go to envoy again to in witch condition the grpc status code is infer from the http status code.

just to be in the same page the correct fix should be in envoy wasm filter to allows the grpc code be optional, but that request an api change that I guess will to be easy to do. In addition forcing the grpc status code to be always -1 is incorrect too and I though a situation in the middle when instead for sending invalid -1 was better.

The other not elegant workaround is to translate the -1 into empty optional in the wasm filter, but the multiple conversions between int32 -> uint32 -> uint64 make even less elegant. ( and I don't really know if all arch will behave equal)

That's an issue in Envoy integration, and not Rust SDK. Also, I don't believe it requires any API changes, since Envoy's host integration can simply map -1 (which is invalid value) to std::nullopt. cc @mpwarres

FWIW, the mapping from this PR is not that bad, since we're sending garbage HTTP header already, but if you want to send gRPC responses, then you need to add send_grpc_response that allows users to set gRPC status.

juanmolle · 2024-04-29T15:48:32Z

@PiotrSikora, Sorry I lost your last comment

That's an issue in Envoy integration, and not Rust SDK. Also, I don't believe it requires any API changes, since Envoy's host integration can simply map -1 (which is invalid value) to std::nullopt. cc @mpwarres

It makes sense to me and let envoy replay automatically, If you agree I could make envoy change to do it.

PiotrSikora · 2024-04-29T16:45:30Z

It makes sense to me and let envoy replay automatically, If you agree I could make envoy change to do it.

Please do, thank you!

Notably, it fixes an issue with always injecting invalid "grpc-status" HTTP response header when sending local HTTP response to gRPC clients. Fixes proxy-wasm#211. Signed-off-by: Piotr Sikora <[email protected]>

Notably, it fixes an issue with always injecting invalid "grpc-status" HTTP response header when sending local HTTP response to gRPC clients. Fixes #211. Signed-off-by: Piotr Sikora <[email protected]>

Notably, it fixes an issue with always injecting invalid "grpc-status" HTTP response header when sending local HTTP response to gRPC clients. Fixes proxy-wasm#211. Signed-off-by: Piotr Sikora <[email protected]>

juanmolle requested a review from PiotrSikora as a code owner October 24, 2023 18:29

juanmolle force-pushed the map_http_status_code_into_grpc_status_code branch from 3ce82e6 to c9f33d0 Compare October 24, 2023 18:33

juanmolle commented Oct 24, 2023

View reviewed changes

PiotrSikora reviewed Oct 24, 2023

View reviewed changes

juanmolle requested a review from PiotrSikora October 25, 2023 11:56

juanmolle added 2 commits October 27, 2023 13:21

Adding http to grpc status code mapping

300a0d5

Signed-off-by: Juan Manuel Ollé <[email protected]>

Fix format

5b5a697

Signed-off-by: Juan Manuel Ollé <[email protected]>

juanmolle force-pushed the map_http_status_code_into_grpc_status_code branch from 96fb807 to 5b5a697 Compare October 27, 2023 16:21

juanmolle closed this Apr 30, 2024

PiotrSikora mentioned this pull request Jul 21, 2024

Update Envoy to v1.31. #251

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding http to grpc status code mapping #211

Adding http to grpc status code mapping #211

juanmolle commented Oct 24, 2023

google-cla bot commented Oct 24, 2023

juanmolle Oct 24, 2023

PiotrSikora Oct 24, 2023

juanmolle Oct 24, 2023 •

edited

Loading

juanmolle Oct 24, 2023 •

edited

Loading

juanmolle Oct 25, 2023 •

edited

Loading

juanmolle commented Nov 3, 2023

PiotrSikora commented Nov 7, 2023 •

edited

Loading

PiotrSikora commented Nov 9, 2023

juanmolle commented Nov 9, 2023

PiotrSikora commented Nov 14, 2023 •

edited

Loading

juanmolle commented Nov 14, 2023

PiotrSikora commented Nov 14, 2023 •

edited

Loading

juanmolle commented Apr 29, 2024

PiotrSikora commented Apr 29, 2024

Adding http to grpc status code mapping #211

Adding http to grpc status code mapping #211

Conversation

juanmolle commented Oct 24, 2023

google-cla bot commented Oct 24, 2023

juanmolle Oct 24, 2023

Choose a reason for hiding this comment

PiotrSikora Oct 24, 2023

Choose a reason for hiding this comment

juanmolle Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

juanmolle Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

juanmolle Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

juanmolle commented Nov 3, 2023

PiotrSikora commented Nov 7, 2023 • edited Loading

PiotrSikora commented Nov 9, 2023

juanmolle commented Nov 9, 2023

PiotrSikora commented Nov 14, 2023 • edited Loading

juanmolle commented Nov 14, 2023

PiotrSikora commented Nov 14, 2023 • edited Loading

juanmolle commented Apr 29, 2024

PiotrSikora commented Apr 29, 2024

juanmolle Oct 24, 2023 •

edited

Loading

juanmolle Oct 24, 2023 •

edited

Loading

juanmolle Oct 25, 2023 •

edited

Loading

PiotrSikora commented Nov 7, 2023 •

edited

Loading

PiotrSikora commented Nov 14, 2023 •

edited

Loading

PiotrSikora commented Nov 14, 2023 •

edited

Loading