Adding http to grpc status code mapping

[juanmolle]

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

Related issue @https://github.com/proxy-wasm/proxy-wasm-rust-sdk/issues/148

[google-cla[bot]]

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]

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

[PiotrSikora]

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

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

[juanmolle]

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]

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]

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]

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]

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

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!