Grpc-web: Eliminate custom header x-grpc-web

[sgammon]

As far as I can tell, there is no actual functional purpose for the header x-grpc-web, aside from a vanity bump that it is possible to detect use of Improbable's gRPC web client.

Why is the header added? Of course there are significant problems with this approach. If gRPC web is being used cross-domain, the x-grpc-web header _must_ be included in the Access-Control-Allow-Headers response. And, naturally, the custom header triggers a CORS pre-flight which costs an entire HTTP round-trip, and is a head-of-line blocker for initial calls to the server.

So, here is my question:
1) Why was this header added in the first place?
2) Perhaps related to point 1, does this header _actually_ trigger any functionality? Is it triggered when using Improbable's server side tools, or all gRPC servers?
3) Why can't it be disabled, since the library works fine if the header is eliminated?

[johanbrandhorst]

I can't answer for #1, but I think I can answer the others;

1. No, it does not trigger any functionality in the proxy, the traffic is split based on the content-type header.
2. I think it could probably be disabled. It's not required by the spec (https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-WEB.md), though there's another header we might need to worry about instead: X-User-Agent.

My best guess is that the header is leftover from the olden days of before the spec. @marcuslongmuir probably knows.

[johanbrandhorst]

I suppose removing this header might constitute a breaking change, so I've labeled it as such. We're still in 0.x.y so it's not the end of the world.

[sgammon]

Thank you @johanbrandhorst for the quick response. On our end we manually remove the line in question before using the library. So we are well equipped to test if you'd like feedback.

Otherwise we will happily upgrade when this is merged.

[MarcusLongmuir]

This was originally added for our own internal usage and then not removed. As far as I can tell the only thing that would break if you removed the client's sending of this header would be the CORS handling.

Unfortunately that would mean that anyone using this package that upgraded their client before their server would be affected, unless we remove the server usage first.

Alternatively, if you just need the client to not send this we can put in an option (e.g. sendLegacyHeaders which defaults to true and sends the header.) and then later remove this option (and stop sending the header).

[johanbrandhorst]

Thanks for confirming Marcus, I'm tempted to say we shouldn't bother and just remove it with a major version bump (0.8.0). Any objections?

[jonnyreeves]

Works for me (although it would be 0.9.0)

On Wed, 6 Feb 2019, 17:39 Johan Brandhorst <[email protected] wrote:

Thanks for confirming Marcus, I'm tempted to say we shouldn't bother and
just remove it with a major version bump (0.8.0). Any objections?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/improbable-eng/grpc-web/issues/325#issuecomment-461115545,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAMN-YoAoSV4RMAQpULdwucNGXrWrHb6ks5vKxM5gaJpZM4ajter
.

[johanbrandhorst]

@sgammon would you be interested in contributing a PR to remove this?

[sgammon]

@MarcusLongmuir couldn't that code in grpcweb simply sniff the content type? i suppose i am at a loss here about why there needs to be some alternate protocol detection

[sgammon]

@johanbrandhorst sure thing

[sgammon]

@johanbrandhorst @jonnyreeves @MarcusLongmuir let me know what you think of PR #331

[jonny-improbable]

@sgammon commented inline on #331.

[jonny-improbable]

Did some more digging into this; found https://github.com/grpc/grpc-web/issues/85 which documents the historical context of the x-grpc-web header; given that context I am still happy for us to proceed with removing the header.

However, as partially discussed in #331 and in https://github.com/grpc/grpc-web/issues/85, we should replace it with an X-User-Agent: grpc-web-javascript/0.1 to comply the the grpcweb spec and therefore continue to trigger a CORS preflight on such requests.

[sgammon]

kewl, although, @jonny-improbable it would be really cool if that could be placed behind a flag, perhaps as suggested by @MarcusLongmuir

to avoid the pre-flight for those who have control of server systems along w/client-side, it would be nice to suppress any custom headers.

[sgammon]

fwiw we really appreciate this software, in fact, it's probably used a lot with the Go lib, which is why perhaps the custom CORS config isn't painful for others.

we've been using it with Endpoints Service Proxy etc, and it works great, but requires maintenance of a custom CORS config just for this lib. that's the only concern

[johanbrandhorst]

I whince a little at the idea of a flag to enable violation of the spec but I'm not going to insist against it. Thanks for your contributions so far @sgammon. Will you update the PR?

[jonny-improbable]

@sgammon, if I understand your use case correctly, and you just wish to
omit sending the x-grpc-web header from the client; you could create your
own custom Transport implementation, overriding the start(metadata) and
deleting the x-grpc-web key from the metadata object before calling the
super implementation - I can knock you up a gist if you like?

On Tue, 12 Feb 2019 at 18:31, Johan Brandhorst [e-mail]
wrote:

I whince a little at the idea of a flag to enable violation of the spec
but I'm not going to insist against it. Thanks for your contributions so
far @sgammon https://github.com/sgammon. Will you update the PR?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/improbable-eng/grpc-web/issues/325#issuecomment-462878327,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ANga8RR7nMR5B7frZTyEayJgtTUhCT50ks5vMwhugaJpZM4ajter
.

[sgammon]

@johanbrandhorst i would be happy to update the PR - i'll go ahead and push that change so that at least it's here

@jonny-improbable that would be a fine solution i suppose, thank you. a gist would be really welcome just to understand what would go into that kind of override.

[johanbrandhorst]

@sgammon if we cover the adding of the new header and removal of the old in the existing PR and then use your custom transport to override the sending of the new header I think that satisfies all parties?

[jonny-improbable]

@sgammon something like this should work.

[jonny-improbable]

Closing this issue in favor of #346; to summarise, our plan is to remove the x-grpc-web header from the client.