[tonic-web] Fix CORS trait issue when using tonic_web::enable

[mdrach]

Motivation

The grpc-web README example doesn't compile due to an unimplemented trait in tower_http::cors::Cors.
It seems related to this PR, which added the tower_http Cors layer wrapper to tonic_web::enable.

The error

error[E0277]: the trait bound `tower_http::cors::Cors<GrpcWebService<GreeterServer<MyGreeter>>>: NamedService` is not satisfied
   --> examples/src/grpc-web/server.rs:44:22
    |
44  |         .add_service(tonic_web::enable(greeter))
    |          ----------- ^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `NamedService` is not implemented for `tower_http::cors::Cors<GrpcWebService<GreeterServer<MyGreeter>>>`
    |          |
    |          required by a bound introduced by this call
    |
    = help: the following other types implement trait `NamedService`:
              GreeterServer<T>
              GrpcWebService<S>
              tonic::service::interceptor::InterceptedService<S, F>
              tower::util::either::Either<S, T>
note: required by a bound in `Server::<L>::add_service`
   --> /home/ubuntu/repos/tonic/tonic/src/transport/server/mod.rs:347:15
    |
347 |             + NamedService
    |               ^^^^^^^^^^^^ required by this bound in `Server::<L>::add_service`

For more information about this error, try `rustc --explain E0277`.

Reproducing the error

You can repro the issue by cloning the tonic repo and modifying the code in this example.

...
    Server::builder()
        .accept_http1(true)
-        .layer(GrpcWebLayer::new())
-        .add_service(greeter)
+        .add_service(tonic_web::enable(greeter))
        .serve(addr)
        .await?;
...

Then run cargo run -p examples --bin grpc-web-server

Solution

Added NamedService impl for tower_http::cors::Cors<S>. Open to feedback on where exactly this should live.

Also made the grpc-web server example consistent with the README. It now uses the tonic_web::enable helper which enables CORS support by default. As stated in this issue, CORS support is needed for a browser client to talk to a tonic server without a proxy. I imagine this is what the typical tonic-web user wants.

After making this change, a grpc-web client running in my browser is able to talk to my tonic-web server without issue.

[LucioFranco]

I think the readme is outdated so I updated it here #1292 since this should be the new way to do it rather than the old way.

[mdrach]

I think the readme is outdated so I updated it here #1292 since this should be the new way to do it rather than the old way.

Do we want to enable CORS by default in the common example, though? This will make using tonic-web a lot easier to get started with for newcomers.

[LucioFranco]

Do we want to enable CORS by default in the common example, though? This will make using tonic-web a lot easier to get started with for newcomers.

I believe we should be pointing people to use tower_http::Cors, if I remember correctly from the change PR.

[nrot]

Hello! Is there anything I can do to speed up the process?

[dragonnn]

I am trying to get this patch working on a cloned version but I am still getting error:

error[E0277]: the trait bound `tower_http::cors::Cors<GrpcWebService<models::proto::users::user_service_server::UserServiceServer<UserGreeter>>>: NamedService` is not satisfied
   --> server/src/grpc/mod.rs:77:22
    |
77  |         .add_service(tonic_web::enable(user_gretter))
    |          ----------- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `NamedService` is not implemented for `tower_http::cors::Cors<GrpcWebService<models::proto::users::user_service_server::UserServiceServer<UserGreeter>>>`
    |          |
    |          required by a bound introduced by this call
    |
    = help: the following other types implement trait `NamedService`:
              Cors<S>
              GrpcWebService<S>
              InterceptedService<S, F>
              console_api::instrument::instrument_server::InstrumentServer<T>
              console_api::trace::trace_server::TraceServer<T>
              models::proto::defined_alarms::defined_alarms_service_server::DefinedAlarmsServiceServer<T>
              models::proto::devices::devices_service_server::DevicesServiceServer<T>
              models::proto::generic_devices::generic_devices_service_server::GenericDevicesServiceServer<T>
            and 8 others
note: required by a bound in `tonic::transport::Server::<L>::add_service`

I am pretty sure that is applied... I am doing something wrong? It shows that Cors<S> does have that type implemented... and GrpcWebService. When I build without this patch Cors<S> is gone. So what I am missing here?

And that service works fine without tonic_web::enable

[LucioFranco]

Check the readme https://github.com/hyperium/tonic/tree/master/tonic-web#enabling-tonic-services and the examples for how to configure it.

[dragonnn]

So we shouldn't use tonic_web::enable at all?
They why it is for? I think it should be removed in this case.

[mdrach]

Check the readme https://github.com/hyperium/tonic/tree/master/tonic-web#enabling-tonic-services and the examples for how to configure it.

@LucioFranco mind adding an example with how configure tonic_web with CORS?

My understanding is that this PR broke CORS support by not implementing the trait on tower_http::cors::CorsLayer. This PR fixes this.

Using the new builder syntax doesn't affect the underlying issue. This also fails with the trait issue, for example.

    let cors = CorsLayer::new()...;

    Server::builder()
        .accept_http1(true)
        .layer(GrpcWebLayer::new())
        .layer(cors)
        .add_service(greeter)

[mdrach]

Thanks for the example @LucioFranco this works for me: #1320 and allows me to configure the CORS layer to my needs.

This default should work well for anyone getting started.

    const DEFAULT_MAX_AGE: Duration = Duration::from_secs(24 * 60 * 60);
    const DEFAULT_EXPOSED_HEADERS: [&str; 3] =
        ["grpc-status", "grpc-message", "grpc-status-details-bin"];
    const DEFAULT_ALLOW_HEADERS: [&str; 4] =
        ["x-grpc-web", "content-type", "x-user-agent", "grpc-timeout"];

    let cors_layer = CorsLayer::new()
        .allow_origin(AllowOrigin::mirror_request())
        .allow_credentials(true)
        .max_age(DEFAULT_MAX_AGE)
        .expose_headers(
            DEFAULT_EXPOSED_HEADERS
                .iter()
                .cloned()
                .map(HeaderName::from_static)
                .collect::<Vec<HeaderName>>(),
        )
        .allow_headers(
            DEFAULT_ALLOW_HEADERS
                .iter()
                .cloned()
                .map(HeaderName::from_static)
                .collect::<Vec<HeaderName>>(),
        );
//...
// from example https://github.com/hyperium/tonic/pull/1320
    Server::builder()
        // GrpcWeb is over http1 so we must enable it.
        .accept_http1(true)
        .accept_http1(true)
        // from above
        .layer(cors_layer)
        // Apply the tonic-web layer to convert
        // http1 requests into something that
        // the core tonic code can understand.
        .layer(GrpcWebLayer::new())
        .layer(GrpcWebLayer::new())
        .add_service(greeter)
        .add_service(greeter)
        .serve(addr)
        .serve(addr)

[LucioFranco]

I messed up and I thought we were removing enable but turns out there was a mistake in my review before I've placed a fix here #1325 and the previous versions of this should work.