Unexpected HTTP response codes for unimplemented GRPC methods #4392

hdevalence · 2024-05-15T19:58:47Z

Describe the bug

Skip reported unexpected HTTP response codes for unimplemented GRPC methods:

Another thing I noticed I think has to do with the problem I was running into with grpcurl. Instead of returning "Unimplemented" GRPC error for unimplemented queries, I get back an unknown error type

2024/05/15 15:05:12 rpc error: code = Unknown desc = unexpected HTTP status code received from server: 405 (Method Not Allowed); malformed header: missing HTTP content-type
exit status 1

Skip API uses the unimplemented error type to detect feature support in certain cases. We may need to handle this other error type specially since it seems like we are getting it instead of unimplemented.

They also reported that grpcurl didn't work, potentially because of problems with reflection.

This was against https://grpc.testnet.penumbra.zone; it's possible the custom load balancer setup there interferes with HTTP headers, we've had that issue in the past, but this seems more likely to be a problem in pd itself.

🔗 related work

The text was updated successfully, but these errors were encountered:

hdevalence · 2024-05-15T21:06:20Z

Link to gist with pd logs: https://gist.github.com/hdevalence/3cc8c9cef56463dc08cf4e303f316ba4

$ grpcurl -plaintext -vv localhost:8080 list
Failed to list services: rpc error: code = Unknown desc = unexpected HTTP status code received from server: 405 (Method Not Allowed); malformed header: missing HTTP content-type

conorsch · 2024-05-15T21:29:08Z

They also reported that grpcurl didn't work

Using grpcurl with a known-good query works fine:

❯ grpcurl grpc.testnet.penumbra.zone:443 penumbra.core.app.v1.QueryService.AppParameters  | jq .appParameters.chainId
"penumbra-testnet-deimos-8"

potentially because of problems with reflection.

Reflection is also supported, shown here via grpcurl:

❯ grpcurl grpc.testnet.penumbra.zone:443 list
grpc.reflection.v1alpha.ServerReflection
penumbra.cnidarium.v1.QueryService
penumbra.core.app.v1.QueryService
penumbra.core.component.auction.v1.QueryService
penumbra.core.component.community_pool.v1.QueryService
penumbra.core.component.compact_block.v1.QueryService
penumbra.core.component.dex.v1.QueryService
penumbra.core.component.dex.v1.SimulationService
penumbra.core.component.fee.v1.QueryService
penumbra.core.component.governance.v1.QueryService
penumbra.core.component.sct.v1.QueryService
penumbra.core.component.shielded_pool.v1.QueryService
penumbra.core.component.stake.v1.QueryService
penumbra.custody.v1.CustodyService
penumbra.tools.summoning.v1.CeremonyCoordinatorService
penumbra.util.tendermint_proxy.v1.TendermintProxyService
penumbra.view.v1.ViewService

Skip reported unexpected HTTP response codes for unimplemented GRPC methods:

That makes sense, as a side-effect of our merging multiple routers in the pd endpoint, so that we can provide a node status page in addition to the gRPC services. I'm not sure what the best compromise is for this situation.

conorsch · 2024-05-15T21:47:38Z

Ah, looks like I had an outdated grpcurl: I was using 1.8.7, and the results above were posted from that version. After upgrading my local version to 1.9.1, I can reproduce problems:

❯ grpcurl --version
grpcurl v1.9.1

❯ grpcurl -vv grpc.testnet.penumbra.zone:443 list
Failed to list services: rpc error: code = Unknown desc = unexpected HTTP status code received from server: 405 (Method Not Allowed); malformed header: missing HTTP content-type

conorsch · 2024-05-15T22:11:45Z

Looks like newer versions of grpcurl, the server reflection API moved from v1alpha to v1, as shown here: https://github.com/fullstorydev/grpcurl/pull/407/files We get gRPC server reflection support via the tonic-reflection crate, which is still using the v1alpha namespace: hyperium/tonic#1685 Can we do anything about that without an upstream patch and version bump? Let's find out.

hdevalence · 2024-05-15T22:13:18Z

Let's do an upstream patch and version bump, that seems like the best solution

cratelyn · 2024-05-15T22:47:59Z

i am opening an upstream patch now!

cc: @conorsch https://github.com/cratelyn/tonic/tree/kate/update-tonic-reflection-to-v1

this fixes hyperium#1685. ### 🩹 changes * vendors the `v1` definition of [`reflection.proto`][proto]. * renames the (deprecated) `v1alpha` definition to `reflection_v1alpha.proto`. * `tonic_reflection::generated::grpc_reflection_v1` links to the generated Rust code (created by running `cargo run --package codegen`). * `tonic_reflection::generated::FILE_DESCRIPTOR_SET` is replaced by `tonic_reflection::generated::{FILE_DESCRIPTOR_SET_V1ALPHA, FILE_DESCRIPTOR_SET_V1}`. * `tonic_reflection::pb` now contains namespaced `tonic_reflection::pb::{v1alpha, v1}` submodules. (**NB: this is a breaking change to the public `tonic-reflection` API.**) * `tonic_reflection::server` is updated to use the generated `tonic_reflection::pb::v1` code. [proto]: https://github.com/grpc/grpc/blob/master/src/proto/grpc/reflection/v1/reflection.proto fixes: hyperium#1685 x-ref: penumbra-zone/penumbra#4392

this fixes hyperium#1685. in penumbra-zone/penumbra#4392, we observed that tonic servers do not properly support reflection when servicing a request sent by recent versions of [`grpcurl`], after [v1.8.8] began using `grpc.reflection.v1.ServerReflection`. (see fullstorydev/grpcurl#407) these lead to an error regarding an unexpected status code, like this: ``` ❯ grpcurl --version grpcurl v1.9.1 ❯ grpcurl -vv grpc.testnet.penumbra.zone:443 list Failed to list services: rpc error: code = Unknown desc = unexpected HTTP status code received from server: 405 (Method Not Allowed); malformed header: missing HTTP content-type ``` this adds the v1 reflection definition to `tonic-reflection`, which was observed as fixing these issues for our gRPC endpoint. ### 🩹 changes changes in this commet are as follows: * vendors the `v1` definition of [`reflection.proto`][proto]. * renames the (deprecated) `v1alpha` definition to `reflection_v1alpha.proto`. * `tonic_reflection::generated::grpc_reflection_v1` links to the generated Rust code (created by running `cargo run --package codegen`). * `tonic_reflection::generated::FILE_DESCRIPTOR_SET` is replaced by `tonic_reflection::generated::{FILE_DESCRIPTOR_SET_V1ALPHA, FILE_DESCRIPTOR_SET_V1}`. * `tonic_reflection::pb` now contains namespaced `tonic_reflection::pb::{v1alpha, v1}` submodules. (**NB: this is a breaking change to the public `tonic-reflection` API.**) * `tonic_reflection::server` is updated to use the generated `tonic_reflection::pb::v1` code. [v1.8.8]: https://github.com/fullstorydev/grpcurl/releases/tag/v1.8.8 [proto]: https://github.com/grpc/grpc/blob/master/src/proto/grpc/reflection/v1/reflection.proto [grpcurl]: https://github.com/fullstorydev/grpcurl --- fixes: hyperium#1685 x-ref: penumbra-zone/penumbra#4392 co-authored-by: Conor Schaefer <conor@penumbralabs.xyz>

cratelyn · 2024-05-16T00:13:21Z

i've opened hyperium/tonic/pull/1701. @conorsch and i confirmed fixes this issue by running pd locally and sending a list request from a new version of grpcurl.

once that lands, we will need to either upgrade our tonic dependency to a new release that includes that patch (NB: this will involve upgrading from 0.10 to 0.11), or we will need to backport that to the 0.10 release (that would involve a temporary fork of tonic-*). the former option is ideal, but will involve a bit of finagling to get ibc-types, ibc-proto, and our tonic dependencies all in agreement with one another.

in any case, i have a version of this patch based on the 0.11 and 0.10 releases here and here, respectively.

this fixes hyperium#1685. in penumbra-zone/penumbra#4392, we observed that tonic servers do not properly support reflection when servicing a request sent by recent versions of [`grpcurl`], after [v1.8.8] began using `grpc.reflection.v1.ServerReflection`. (see fullstorydev/grpcurl#407) these lead to an error regarding an unexpected status code, like this: ``` ❯ grpcurl --version grpcurl v1.9.1 ❯ grpcurl -vv grpc.testnet.penumbra.zone:443 list Failed to list services: rpc error: code = Unknown desc = unexpected HTTP status code received from server: 405 (Method Not Allowed); malformed header: missing HTTP content-type ``` this adds the v1 reflection definition to `tonic-reflection`, which was observed as fixing these issues for our gRPC endpoint. ### 🩹 changes changes in this commet are as follows: * vendors the `v1` definition of [`reflection.proto`][proto]. * renames the (deprecated) `v1alpha` definition to `reflection_v1alpha.proto`. * `tonic_reflection::generated::grpc_reflection_v1` links to the generated Rust code (created by running `cargo run --package codegen`). * `tonic_reflection::generated::FILE_DESCRIPTOR_SET` is replaced by `tonic_reflection::generated::{FILE_DESCRIPTOR_SET_V1ALPHA, FILE_DESCRIPTOR_SET_V1}`. * `tonic_reflection::pb` now contains namespaced `tonic_reflection::pb::{v1alpha, v1}` submodules. (**NB: this is a breaking change to the public `tonic-reflection` API.**) * `tonic_reflection::server` is updated to use the generated `tonic_reflection::pb::v1` code. [v1.8.8]: https://github.com/fullstorydev/grpcurl/releases/tag/v1.8.8 [proto]: https://github.com/grpc/grpc/blob/master/src/proto/grpc/reflection/v1/reflection.proto [grpcurl]: https://github.com/fullstorydev/grpcurl --- fixes: hyperium#1685 x-ref: penumbra-zone/penumbra#4392 co-authored-by: Conor Schaefer <conor@penumbralabs.xyz>

this fixes hyperium#1685. in penumbra-zone/penumbra#4392, we observed that tonic servers do not properly support reflection when servicing a request sent by recent versions of [`grpcurl`], after [v1.8.8] began using `grpc.reflection.v1.ServerReflection`. (see fullstorydev/grpcurl#407) these lead to an error regarding an unexpected status code, like this: ``` ❯ grpcurl --version grpcurl v1.9.1 ❯ grpcurl -vv grpc.testnet.penumbra.zone:443 list Failed to list services: rpc error: code = Unknown desc = unexpected HTTP status code received from server: 405 (Method Not Allowed); malformed header: missing HTTP content-type ``` this adds the v1 reflection definition to `tonic-reflection`, which was observed as fixing these issues for our gRPC endpoint. ### 🩹 changes changes in this commet are as follows: * vendors the `v1` definition of [`reflection.proto`][proto]. * renames the (deprecated) `v1alpha` definition to `reflection_v1alpha.proto`. * `tonic_reflection::generated::grpc_reflection_v1` links to the generated Rust code (created by running `cargo run --package codegen`). * `tonic_reflection::generated::FILE_DESCRIPTOR_SET` is replaced by `tonic_reflection::generated::{FILE_DESCRIPTOR_SET_V1ALPHA, FILE_DESCRIPTOR_SET_V1}`. * `tonic_reflection::pb` now contains namespaced `tonic_reflection::pb::{v1alpha, v1}` submodules. (**NB: this is a breaking change to the public `tonic-reflection` API.**) * `tonic_reflection::server` is updated to use the generated `tonic_reflection::pb::v1` code. * `HttpsUriWithoutTlsSupport` is now gated behind the `transport` feature flag, because it is not used otherwise. [v1.8.8]: https://github.com/fullstorydev/grpcurl/releases/tag/v1.8.8 [proto]: https://github.com/grpc/grpc/blob/master/src/proto/grpc/reflection/v1/reflection.proto [grpcurl]: https://github.com/fullstorydev/grpcurl --- fixes: hyperium#1685 x-ref: penumbra-zone/penumbra#4392 co-authored-by: Conor Schaefer <conor@penumbralabs.xyz>

this fixes hyperium#1685. in penumbra-zone/penumbra#4392, we observed that tonic servers do not properly support reflection when servicing a request sent by recent versions of [`grpcurl`], after [v1.8.8] began using `grpc.reflection.v1.ServerReflection`. (see fullstorydev/grpcurl#407) these lead to an error regarding an unexpected status code, like this: ``` ❯ grpcurl --version grpcurl v1.9.1 ❯ grpcurl -vv grpc.testnet.penumbra.zone:443 list Failed to list services: rpc error: code = Unknown desc = unexpected HTTP status code received from server: 405 (Method Not Allowed); malformed header: missing HTTP content-type ``` this adds the v1 reflection definition to `tonic-reflection`, which was observed as fixing these issues for our gRPC endpoint. ### 🩹 changes changes in this commet are as follows: * vendors the `v1` definition of [`reflection.proto`][proto]. * renames the (deprecated) `v1alpha` definition to `reflection_v1alpha.proto`. * `tonic_reflection::generated::grpc_reflection_v1` links to the generated Rust code (created by running `cargo run --package codegen`). * `tonic_reflection::generated::FILE_DESCRIPTOR_SET` is replaced by `tonic_reflection::generated::{FILE_DESCRIPTOR_SET_V1ALPHA, FILE_DESCRIPTOR_SET_V1}`. * `tonic_reflection::pb` now contains namespaced `tonic_reflection::pb::{v1alpha, v1}` submodules. (**NB: this is a breaking change to the public `tonic-reflection` API.**) * `tonic_reflection::server` is updated to use the generated `tonic_reflection::pb::v1` code. * `HttpsUriWithoutTlsSupport` is now gated behind the `tls` feature flag, because it is not used otherwise. [v1.8.8]: https://github.com/fullstorydev/grpcurl/releases/tag/v1.8.8 [proto]: https://github.com/grpc/grpc/blob/master/src/proto/grpc/reflection/v1/reflection.proto [grpcurl]: https://github.com/fullstorydev/grpcurl --- fixes: hyperium#1685 x-ref: penumbra-zone/penumbra#4392 co-authored-by: Conor Schaefer <conor@penumbralabs.xyz>

@conorsch

@conorsch

Refs #4392. We want to ensure that server reflection remains working, despite changes to the tonic dependencies (#4400) and proto compiling logic #4422. While the most effective test is exercising all these protos regularly, we currently lack solid coverage, so this superficial integration test is a sanity-check to save time versus building locally and inspecting the output manually.

this fixes hyperium#1685. in penumbra-zone/penumbra#4392, we observed that tonic servers do not properly support reflection when servicing a request sent by recent versions of [`grpcurl`], after [v1.8.8] began using `grpc.reflection.v1.ServerReflection`. (see fullstorydev/grpcurl#407) these lead to an error regarding an unexpected status code, like this: ``` ❯ grpcurl --version grpcurl v1.9.1 ❯ grpcurl -vv grpc.testnet.penumbra.zone:443 list Failed to list services: rpc error: code = Unknown desc = unexpected HTTP status code received from server: 405 (Method Not Allowed); malformed header: missing HTTP content-type ``` this adds the v1 reflection definition to `tonic-reflection`, which was observed as fixing these issues for our gRPC endpoint. ### 🩹 changes changes in this commet are as follows: * vendors the `v1` definition of [`reflection.proto`][proto]. * renames the (deprecated) `v1alpha` definition to `reflection_v1alpha.proto`. * `tonic_reflection::generated::grpc_reflection_v1` links to the generated Rust code (created by running `cargo run --package codegen`). * `tonic_reflection::generated::FILE_DESCRIPTOR_SET` is replaced by `tonic_reflection::generated::{FILE_DESCRIPTOR_SET_V1ALPHA, FILE_DESCRIPTOR_SET_V1}`. * `tonic_reflection::pb` now contains namespaced `tonic_reflection::pb::{v1alpha, v1}` submodules. (**NB: this is a breaking change to the public `tonic-reflection` API.**) * `tonic_reflection::server` is updated to use the generated `tonic_reflection::pb::v1` code. [v1.8.8]: https://github.com/fullstorydev/grpcurl/releases/tag/v1.8.8 [proto]: https://github.com/grpc/grpc/blob/master/src/proto/grpc/reflection/v1/reflection.proto [grpcurl]: https://github.com/fullstorydev/grpcurl --- fixes: hyperium#1685 x-ref: penumbra-zone/penumbra#4392 co-authored-by: Conor Schaefer <conor@penumbralabs.xyz>

Refs #4392. We want to ensure that server reflection remains working, despite changes to the tonic dependencies (#4400) and proto compiling logic #4422. While the most effective test is exercising all these protos regularly, we currently lack solid coverage, so this superficial integration test is a sanity-check to save time versus building locally and inspecting the output manually.

this fixes hyperium#1685. in penumbra-zone/penumbra#4392, we observed that tonic servers do not properly support reflection when servicing a request sent by recent versions of [`grpcurl`], after [v1.8.8] began using `grpc.reflection.v1.ServerReflection`. (see fullstorydev/grpcurl#407) these lead to an error regarding an unexpected status code, like this: ``` ❯ grpcurl --version grpcurl v1.9.1 ❯ grpcurl -vv grpc.testnet.penumbra.zone:443 list Failed to list services: rpc error: code = Unknown desc = unexpected HTTP status code received from server: 405 (Method Not Allowed); malformed header: missing HTTP content-type ``` this adds the v1 reflection definition to `tonic-reflection`, which was observed as fixing these issues for our gRPC endpoint. ### 🩹 changes changes in this commet are as follows: * vendors the `v1` definition of [`reflection.proto`][proto]. * renames the (deprecated) `v1alpha` definition to `reflection_v1alpha.proto`. * `tonic_reflection::generated::grpc_reflection_v1` links to the generated Rust code (created by running `cargo run --package codegen`). * `tonic_reflection::generated::FILE_DESCRIPTOR_SET` is replaced by `tonic_reflection::generated::{FILE_DESCRIPTOR_SET_V1ALPHA, FILE_DESCRIPTOR_SET_V1}`. * `tonic_reflection::pb` now contains namespaced `tonic_reflection::pb::{v1alpha, v1}` submodules. (**NB: this is a breaking change to the public `tonic-reflection` API.**) * `tonic_reflection::server` is updated to use the generated `tonic_reflection::pb::v1` code. [v1.8.8]: https://github.com/fullstorydev/grpcurl/releases/tag/v1.8.8 [proto]: https://github.com/grpc/grpc/blob/master/src/proto/grpc/reflection/v1/reflection.proto [grpcurl]: https://github.com/fullstorydev/grpcurl --- fixes: hyperium#1685 x-ref: penumbra-zone/penumbra#4392 co-authored-by: Conor Schaefer <conor@penumbralabs.xyz>

Refs #4392. We want to ensure that server reflection remains working, despite changes to the tonic dependencies (#4400) and proto compiling logic #4422. While the most effective test is exercising all these protos regularly, we currently lack solid coverage, so this superficial integration test is a sanity-check to save time versus building locally and inspecting the output manually.

this fixes #1685. in penumbra-zone/penumbra#4392, we observed that tonic servers do not properly support reflection when servicing a request sent by recent versions of [`grpcurl`], after [v1.8.8] began using `grpc.reflection.v1.ServerReflection`. (see fullstorydev/grpcurl#407) these lead to an error regarding an unexpected status code, like this: ``` ❯ grpcurl --version grpcurl v1.9.1 ❯ grpcurl -vv grpc.testnet.penumbra.zone:443 list Failed to list services: rpc error: code = Unknown desc = unexpected HTTP status code received from server: 405 (Method Not Allowed); malformed header: missing HTTP content-type ``` this adds the v1 reflection definition to `tonic-reflection`, which was observed as fixing these issues for our gRPC endpoint. ### 🩹 changes changes in this commet are as follows: * vendors the `v1` definition of [`reflection.proto`][proto]. * renames the (deprecated) `v1alpha` definition to `reflection_v1alpha.proto`. * `tonic_reflection::generated::grpc_reflection_v1` links to the generated Rust code (created by running `cargo run --package codegen`). * `tonic_reflection::generated::FILE_DESCRIPTOR_SET` is replaced by `tonic_reflection::generated::{FILE_DESCRIPTOR_SET_V1ALPHA, FILE_DESCRIPTOR_SET_V1}`. * `tonic_reflection::pb` now contains namespaced `tonic_reflection::pb::{v1alpha, v1}` submodules. (**NB: this is a breaking change to the public `tonic-reflection` API.**) * `tonic_reflection::server` is updated to use the generated `tonic_reflection::pb::v1` code. [v1.8.8]: https://github.com/fullstorydev/grpcurl/releases/tag/v1.8.8 [proto]: https://github.com/grpc/grpc/blob/master/src/proto/grpc/reflection/v1/reflection.proto [grpcurl]: https://github.com/fullstorydev/grpcurl --- fixes: #1685 x-ref: penumbra-zone/penumbra#4392 Co-authored-by: Conor Schaefer <conor@penumbralabs.xyz>

Adds a version of grpcui to the local nix dev env. Includes logic to build the go binary from source, via nix, because the released version of grpcui upstream still uses the old v1alpha reflection API, which is incompatible with current versions of `pd`. Refs #4392.

hdevalence assigned cratelyn May 15, 2024

github-project-automation bot added this to Penumbra May 15, 2024

github-project-automation bot moved this to Backlog in Penumbra May 15, 2024

github-actions bot added the needs-refinement unclear, incomplete, or stub issue that needs work label May 15, 2024

cratelyn assigned conorsch May 15, 2024

cratelyn moved this from Backlog to In progress in Penumbra May 15, 2024

cratelyn added this to the Sprint 6 milestone May 15, 2024

cratelyn added A-node Area: System design and implementation for node software C-bug Category: a bug labels May 15, 2024

aubrika added _P-V1 Priority: slated for V1 release _P-high High priority labels May 15, 2024

cratelyn mentioned this issue May 16, 2024

tonic-reflection: 🆕 use v1 reflection protobuffer definitions hyperium/tonic#1701

Merged

cratelyn mentioned this issue May 16, 2024

chore: 🧃 upgrade to tonic@0.11.0 #4400

Open

cratelyn mentioned this issue May 20, 2024

tonic-reflection: 🆕 use v1 reflection protobuffer definitions cratelyn/tonic#1

Closed

aubrika modified the milestones: Sprint 6, Sprint 7 May 20, 2024

This was referenced May 20, 2024

tonic-reflection: 🆕 use v1 reflection protobuffer definitions penumbra-zone/tonic#1

Closed

ci: 🔨 fix v0.10.2 build penumbra-zone/tonic#2

Closed

cratelyn mentioned this issue May 20, 2024

tonic: 🍸 patch tonic dependencies #4424

Merged

1 task

conorsch mentioned this issue May 21, 2024

Include vendored proto definitions in the proto_descriptor file #4425

Merged

1 task

conorsch closed this as completed in #4424 May 21, 2024

github-project-automation bot moved this from In progress to Done in Penumbra May 21, 2024

conorsch mentioned this issue May 21, 2024

test(pd): add integration test for grpc reflection #4426

Merged

1 task

conorsch mentioned this issue Jul 31, 2024

feat(dev): add grpcui to nix dev env #4781

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unexpected HTTP response codes for unimplemented GRPC methods #4392

Unexpected HTTP response codes for unimplemented GRPC methods #4392

hdevalence commented May 15, 2024 •

edited by cratelyn

Loading

hdevalence commented May 15, 2024

conorsch commented May 15, 2024

conorsch commented May 15, 2024

conorsch commented May 15, 2024

hdevalence commented May 15, 2024

cratelyn commented May 15, 2024 •

edited

Loading

cratelyn commented May 16, 2024 •

edited

Loading

Unexpected HTTP response codes for unimplemented GRPC methods #4392

Unexpected HTTP response codes for unimplemented GRPC methods #4392

Comments

hdevalence commented May 15, 2024 • edited by cratelyn Loading

🔗 related work

hdevalence commented May 15, 2024

conorsch commented May 15, 2024

conorsch commented May 15, 2024

conorsch commented May 15, 2024

hdevalence commented May 15, 2024

cratelyn commented May 15, 2024 • edited Loading

cratelyn commented May 16, 2024 • edited Loading

hdevalence commented May 15, 2024 •

edited by cratelyn

Loading

cratelyn commented May 15, 2024 •

edited

Loading

cratelyn commented May 16, 2024 •

edited

Loading