Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore pclientd GRPC proxy functionality #3120

Closed
hdevalence opened this issue Sep 29, 2023 · 4 comments
Closed

Restore pclientd GRPC proxy functionality #3120

hdevalence opened this issue Sep 29, 2023 · 4 comments

Comments

@hdevalence
Copy link
Member

Is your feature request related to a problem? Please describe.

pclientd has an integrated GRPC proxy that allows pclientd to act as a single endpoint for client requests:

  • view service and custody service requests are processed locally;
  • chain state queries are proxied through to the GRPC endpoint pclientd connects to.

Unfortunately, the way this feature was originally implemented caused it to be broken by protobuf refactoring in #3091, which split up and renamed all of the query services.

The original (current) implementation created tuple structs wrapping a shared Channel, each implementing Tonic's NamedService trait, which uses a &'static str associated constant to name the service, e.g.,

impl NamedService for ObliviousQueryProxy {
    const NAME: &'static str = "penumbra.client.v1alpha1.ObliviousQueryService";
}

We have one of these for each proxied service, and then plug them into the Tonic server builder, which constructs a router that routes over all of them.

The problem with this approach is that each service to be proxied needs its own type and implementation and to be plugged in separately. So when we split up the queries, it breaks -- because it only proxies specific services -- and restoring functionality would mean creating many more services.

Describe the solution you'd like

Ideally, we'd have a proxy that would be generic over the service name, allowing us to have the following behavior:

  • if request matches one of the local services send it to them
  • otherwise check it against a filter list and if so proxy to remote endpoint
  • otherwise error out

Having a filter list is nice but non-essential, the important part is being able to configure the proxy as a generic fallback.

In the Tokio discord, @LucioFranco pointed to the Routes::into_router method as a way to solve the problem. This converts into an Axum router, which has a fallback method that can be configured however we want:

and that should work
so you just set your local services into the regular tonic router
call into_router and then filter on the path and either 404/Unimplemtned whatever or route it to your clients

To support a filter list, we could use a RegexSet as the configuration for whatever object we pass as the custom handler. Then our code could specify service filters like ["penumbra.core.component.*"] to proxy any kind of component query service.

Describe alternatives you've considered

If we run into obstacles with the above approach, we can always use copy-paste technology to extend the current solution across all of our current services. This is suboptimal but it's better than having broken functionality. Our InterchainTest integration relies on pclientd having this behavior, so we shouldn't release without some kind of fix (cc @conorsch).

@conorsch
Copy link
Contributor

If I run pclientd and pcli locally, using v0.61.0, I am indeed unable to perform simple operations like pcli view sync. The methods return an Unimplemented error. Notably those errors are slightly different for view mode of pclientd:

❯ /opt/penumbra/latest/pcli -n http://localhost:8081 view balance
Error: status: Unknown, message: "unable to fetch latest known block height from fullnode: status: Unimplemented, message: \"\", details: [], metadata: MetadataMap { headers: {\"date\": \"Fri, 29 Sep 2023 19:26:45 GMT\", \"content-type\": \"application/grpc\", \"content-length\": \"0\"} }", details: [], metadata: MetadataMap { headers: {"content-type": "application/grpc"} }

vs custody mode:

❯ /opt/penumbra/latest/pcli -n http://localhost:8081 view sync
Error: status: Unimplemented, message: "", details: [], metadata: MetadataMap { headers: {"date": "Fri, 29 Sep 2023 19:33:21 GMT", "content-type": "application/grpc", "content-length": "0"} }

How did you discover this error? I'm surprised that our CI hasn't failed on this yet, because we shipped the refactored protos as part of v0.61.0, and interchaintest has been updated to use those new import paths, and ostensibly the proto-blob -> pclientd -> pd exercise within that test suite is still passing.

@hdevalence
Copy link
Member Author

hdevalence commented Oct 1, 2023

I didn't do anything to discover the error, I just thought about it and realized it would be a problem.

I would also expect it to be a problem that InterchainTest would notice. I'm not sure what InterchainTest is actually running, but we don't exercise this code path in our own smoke tests.

@conorsch
Copy link
Contributor

conorsch commented Oct 13, 2023

Currently blocked by #3078, due to tonic upgrade requirements. We have a stopgap solution in #3171, so we're OK for now. Higher priority for Testnet 63 is #3093. @erwanor is currently working on tendermint-rs upgrades that should unblock this work.

@erwanor erwanor removed the P-blocked label Feb 2, 2024
@hdevalence
Copy link
Member Author

Closing as completed because of the stopgap situation. A real HTTP proxy would be cool and is in no way an important use of time.

@hdevalence hdevalence closed this as not planned Won't fix, can't repro, duplicate, stale Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Status: Next
Development

No branches or pull requests

3 participants