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

Suggest not to forward hop-to-htop http headers to grpc #48

Open
lispc opened this issue Dec 29, 2020 · 3 comments
Open

Suggest not to forward hop-to-htop http headers to grpc #48

lispc opened this issue Dec 29, 2020 · 3 comments

Comments

@lispc
Copy link

lispc commented Dec 29, 2020

in the current code

const convertHeaders = (headers, grpc) => {
, every http header is added to grpc metadata.
While in the official grpc-gateway, only header starting with 'Grpc-Metadata-' will be mapped to gRPC metadata.
https://github.com/grpc-ecosystem/grpc-gateway/blob/26104fd424bf45f44d498149b071ef657d604f2b/runtime/mux.go#L70

@konsumer
Copy link
Owner

Personally, I don't see the value of not passing the headers. There are lots of situations where you might want access to other standard headers, that don't have the prefix. I have used them for authentication and it's good to have access in situations where you can't change the incoming headers (like in the case of AWS lambdas, for example) and I like having access to them. What do you get from them being blocked? I'd accept a PR, if you want to add an option (with current behavior as default) to block headers that don't have a prefix.

in the official grpc-gateway

I think both of our projects don't really have a standard to follow, or have to work like the other. My gateway was created a bit after theirs, but is definitely it's own thing, and works totally different. For example, that one requires that you generate all the code up-front, whereas mine is on-the-fly, which has a small startup cost, but is easier to prototype with. Mine exposes express middleware. I think you'll find all kinds of things that differ like this, and that's ok.

@lispc
Copy link
Author

lispc commented Dec 30, 2020

HTTP headers can be divided into two categories: end-to-end header and hop-to-hop header. 1 2 3. Hop-to-hop headers are meaningful only for a single transport-level connection. So generally proxies will not pass hop-to-hop headers, such as 4.

Why I encounter this problem:
My senario: webpack-dev-server ( node-http-proxy ) -> grpc-dynamic-gateway -> tonic grpc server. Node-http-proxy adds 'connection: close' to every request 5 , and grpc-dynamic-gateway forwards this header to tonic grpc server, and tonic grpc server rejects every request with 'connection: close' request header.

So I think not forwarding hop-to-htop headers, at least the most used 'connection', may be a good idea.

@lispc lispc changed the title Suggest not to forward every http header to grpc Suggest not to forward hop-to-htop http headers to grpc Dec 30, 2020
@konsumer
Copy link
Owner

So I think not forwarding hop-to-htop headers, at least the most used 'connection', may be a good idea.

Hmm. As I mentioned there are lots of use-cases where you might want other headers, or to not have to change the names. I like to keep things simple, so I'd rather not have a lookup table of end-to-end vs hop-to-hop headers to maintain, if I can help it. When I have used this tool in my own projects, I wanted it to be a very thin layer, so I could just inject it into another express-based app, and for the most part I think it accomplishes that goal. I'd put any special logic at the grpc level, instead of taking it on to the gateway. I'm still not convinced it's a good idea, but I'm not against it either. An issue I had with grpc-ecosystem/grpc-gateway is that I had to restructure my headers, which isn't always possible (like the lambda example I gave) or doing so would make things less standard (like Authorize for JWT header.) That said, i'm open to PRs for config options to support more people's use-cases, as long as the basic behavior stays the same, and it doesn't add too much complication.

It seems like your use-case could be solved in several ways:

  • PR for an additional config option, here. You could even incorporate a simple callback that returns true/false for a "good" header to pass (with a default of header => true) and then you could support the grpc-ecosystem/grpc-gateway style, or just pass all headers that are not connection: close or any other scheme, defined as a simple function in user-code, like using those tables from hapi you linked to. if it were me, I'd probly write it to use await, so I could even do (optional) async checks. I think it's like a 2 or 3 line change, and it wouldn't complicate other people's stuff or have any side-effects.
  • You could just use grpc-ecosystem/grpc-gateway it doesn't need to all be in node. Your server is in rust, anyway, so it might be fine to use go, too. If your grpc spec doesn't change too often, go is great at pre-compiling for a bunch of targets (for non-go people) so you could have a nice fast little gateway, tailor-made for your service.
  • You could seek to PR node-http-proxy to not add connection: close to every request, or at least have the option to not do that.
  • You could seek to PR tonic to not ignore close requests. I'd have to do some testing to be sure, but I'm not sure that other grpc server-solutions do this. It's not part of the grpc spec.

For the last 2, there might even be a config option or something. I'm not sure, as I don't use these.

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

No branches or pull requests

2 participants