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

Implement tower::Layer for tonic_web::Config #1119

Merged
merged 4 commits into from
Oct 28, 2022

Conversation

slinkydeveloper
Copy link
Contributor

@slinkydeveloper
Copy link
Contributor Author

Perhaps one question @LucioFranco, should we document it somewhere in the rustdocs of the method tonic_web::config or of the struct tonic_web::config::Config that this trait is implemented? Or what rustdoc already generates in the trait implementation section is enough for you?

Comment on lines 77 to 83
let svc = tonic_web::config()
.allow_origins(vec![allowed_origin])
.enable(TestServer::new(Svc));
let tonic_web_config = tonic_web::config().allow_origins(vec![allowed_origin]);

let _ = tokio::spawn(async move {
Server::builder()
.accept_http1(true)
.add_service(svc)
.layer(tonic_web_config)
.add_service(TestServer::new(Svc))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I think the idea of adding a layer impl makes sense it feels like the code is actually a bit harder to understand at a glance. So I am inclined to continue to force users through the enabled api. I am curious to hear what @davidpdrsn and @neoeinstein thoughts might be.

@slinkydeveloper
Copy link
Contributor Author

@LucioFranco in my case, I'm not using tonic::Server and I'm implementating manually the underlying service, reusing tonic only for decoding/encoding the grpc/grpc-web protocol. In this kind of use cases, supporting tower layers is really handy.

Somehow related to this (but I can open another issue to discuss it), I found out that the CORS code in tonic-web is doing more or less the same of https://docs.rs/tower-http/latest/tower_http/cors/index.html. I was wondering if it makes sense to just expose a GrpcWeb layer that transcodes grpc-web to grpc, and let the tower-http cors implementation handle the cors preflight. So one could either pick the "transcoding layer" manually, or can use tonic_web::enable to get the cors layer from tower-http composed with the transcoding layer. WDYT?

@LucioFranco
Copy link
Member

@slinkydeveloper that seems like an extremely valid argument for the layer stuff. Maybe we should create a to_layer fn that produces a WebLayer that is a bit more clear of a distinction. I believe this should work for your use case as well?

Yeah, I think that cors idea is great, we originally had it in here before the one in tower-http was a thing, so more than happy to remove code.

Thanks!

@slinkydeveloper
Copy link
Contributor Author

slinkydeveloper commented Oct 20, 2022

@LucioFranco to_layer sounds good to me, although I have in mind something different (which involves breaking changes):

  • WebLayer is exposed directly, it has no parameters (doesn't need them), has a new method, and it implements Layer, generating GrpcWeb<S> services
  • tonic_web::enable composes Cors layer from tonic-http and WebLayer. If a user wants more control over the cors layer (like me), they can just instantiate manually WebLayer and compose it with whatever configured cors layer they want
  • Config gets removed, there is no need for that anymore because all the fields are used to implement cors

WDYT?

I can split this work in two or three separate PRs as well (perhaps keeping Config as deprecated?).

@LucioFranco
Copy link
Member

Yeah, that sounds like a great plan, smaller separate PRs will make my life much easier! This all sounds good to me. Thank you!

@slinkydeveloper
Copy link
Contributor Author

@LucioFranco I think this PR as it is should be fine now. I'm working on the second PR to reimplement cors using tower_http::cors

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM just one question

tonic-web/src/layer.rs Show resolved Hide resolved
Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perfect! Thanks!

@LucioFranco LucioFranco merged commit 40536dc into hyperium:master Oct 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement tower::Layer for tonic_web::Config Remove NamedService bound for tonic_web::enable
2 participants