Pass Permanent Request Headers #213

christianvozar · 2016-08-10T22:35:17Z

Allows permanent request headers as-is and without Grpc-Metadata- prefix in metadata. This is particularly important if you are developing REST APIs where third-party developers will have expectations for headers such as Accept: and User-Agent:.

achew22 · 2016-08-13T17:24:14Z

runtime/context.go

+		return true
+	}
+	switch hdr {
+	case


Initially I was concerned that this would be slow because of a bunch of calls to strcmp. Turns out that golang builds a binary tree at compile time and uses that to search. Cool!

golang/go#10000

achew22 · 2016-08-13T18:27:23Z

@yugui What do you think of this?

christianvozar · 2016-08-15T15:45:51Z

Not sure why cla/google is not passing. I signed the CLA 4 days ago and still not passing.

yugui · 2016-08-16T05:41:45Z

@achew22, @christianvozar
I've started to realize that there are use cases which require HTTP headers forwarded, unlike our past assumption.
So I like the overall direction of this PR.

But I wonder if we can have better handling of several HTTP headers in the whitelist.

Content-Type, User-Agent, TE: They are a part of grpc wire protocol.. So they are not allowed as custom metadata names.
Connection: It does not make much sense to forward this field and it is prohibited by RFC 2616 14.10.
Upgrade, Accept, Accept-Encoding, Content-Length: It is possible to forward them but it would cause an issue if the upstream actually changes the behavior based on these headers. So they should be useless for the upstream service implementation.
Range, If-Range: It is possible to forward them but it does not make sense because they are Content-Type-dependent.

How about the following approach?

Limit the whitelist into

    "Accept-Charset",
    "Accept-Language",
    "Accept-Ranges",
    "Authorization",
    "Cache-Control",
    "Cookie",
    "Date",
    "Expect",
    "From",
    "If-Match",
    "If-Modified-Since",
    "If-None-Match",
    "If-Schedule-Tag-Match",
    "If-Unmodified-Since",
    "Max-Forwards",
    "Origin",
    "Pragma",
    "Referer",
    "Via",
    "Warning":

Allow users to add more items into the whitelist with options
Separately discuss why/how to forward User-Agent.
Optionally support more permanent headers and their forwarding.
- RFC 7239
- Decrement the count in Max-Forwards, which is required by RFC 2616 14.31
- ...

achew22 · 2016-08-16T06:33:02Z

I think that whitelist is very reasonable. Let's merge this. That closes bullet 1 and unblocks @christianvozar.

I would like to think a little bit more about allowing users to add more to the whitelist. It feels like there is a good way to provide that flexibility and also to ensure the server remains as fast as can be.

What do you think about proxying User-Agent as Grpc-Gateway-User-Agent? I don't think we should pass it through as the real User-Agent for the reasons you stated above but I think it would be reasonable to claim that in the header namespace.

As for more permanent headers, I think we should wait for there to be a use case that necessitates the enhancement. Those are valid concerns. Maybe we could open an issue preemptively that says "please comment here if you are running into this?"

yugui · 2016-08-16T08:08:06Z

What do you think about proxying User-Agent as Grpc-Gateway-User-Agent?

Sounds good to me. What do you think, @christianvozar?

christianvozar · 2016-08-17T01:25:57Z

The use case I am implementing is one where I am building an API using gRPC+JSON Gateway similar in design to one like Github's. One where a third-party developer utilizing the API might declare their User-Agent: as something specific utilized for debugging purposes I do not want the user to need to declare it as Grpc-Gateway-User-Agent in a request but if that is how it surfaces in the context metadata, that is fine by me. The same goes for Accept:, as this header is what I check for API version validation. (example: https://developer.github.com/v3/#current-version) If we want to pass Accept: to the context metadata as Grpc-Gateway-Accept: that is fine by me. Essentially, to the user of the REST gateway I want the appearance of a standard REST API without revealing the underlying usage of gRPC via prefixes.

So if we are all in agreement that @yugui 's whitelist is an appropriate one, that headers reserved for the gRPC wire protocol (User-Agent, et al.) should be passed into context metadata with a Grpc-Gateway- prefix, and that the headers that do not make sense as outlined by @yugui to forward such Connection or Upgrade should be dropped then I will whip together a commit and squash this all into one tidy PR.

@achew22 @yugui Gimme a +1 and I'll get it submitted asap.

yugui · 2016-08-17T02:21:33Z

@christianvozar Right. It does not mean that API users need to specify Grpc-Gateway-User-Agent. It just means that your backend gRPC service will receive Grpc-Gateway-User-Agent.

BTW, now I remember that the prefix Grpc- is reserved by grpc wire protocol. So can we use GrpcGateway- as an alternate prefix?

For versioning, the recommended way of implementation in gRPC is to define a different gRPC services or methods. And you can dispatch those versions in your custom http handler in your gateway.
In general, forwarding TE or Content-Type looks to be a layer violation. So I am against forwarding TE or Content-Type unless we have another concrete use case.

christianvozar · 2016-08-18T21:02:06Z

Sorry for the delayed response in getting you this. Beware NextProtos: []string{"h2"} in 1.7. Ha!

So for my use-case, as discussed previously, I am really only looking for most standard HTTP headers to be proxied to the gRPC server. That they have a prefix is totally fine, just so long as end-users/developers do not have to specify the prefix in their REST call. So to make life easier I prefix all standard HTTP Request Headers with GrpcGateway- to avoid gRPC reserved header

I did not want to put the ToLower in there but wanted to keep compatibility with bcaacb4.

Sample cURL: curl -X GET -k https://localhost:10000-H "Content-Type: application/json" -H "Accept: application/vnd.cvozar+json; version=1" -H "GrpcGateway-Sample: foo" -H "Authorization: token"

Sample Metadata Context: map[grpcgateway-authorization:[token] x-forwarded-for:[172.17.0.1] :authority:[localhost] grpcgateway-user-agent:[curl/7.43.0] grpcgateway-content-type:[application/json] x-forwarded-host:[localhost:10000] grpcgateway-accept:[application/vnd.cvozar+json; version=1]]

achew22 · 2016-08-19T01:57:16Z

runtime/context.go

+// permenant request headers maintained by IANA.
+// http://www.iana.org/assignments/message-headers/message-headers.xml
+func isPermanentHTTPHeader(hdr string) bool {
+	if hdr != "" && hdr[0] == ':' {


What permanent headers start with ":"?

None, solid point. I was under the impression (could be wrong here) that gRPC reserved headers start with : and this was part of a check. Happy to remove it.

christianvozar · 2016-08-22T17:05:04Z

@yugui || @achew22 Thoughts?

achew22 · 2016-09-05T01:47:03Z

@christianvozar it appears that there are tests failing on Travis. Can you take a look at those?

kalbasit · 2016-10-04T01:06:37Z

any updates on getting this merged?

achew22 · 2016-10-04T01:41:35Z

runtime/context_test.go

@@ -52,16 +53,16 @@ func TestAnnotateContext_ForwardsGrpcMetadata(t *testing.T) {
 	}
 	md, ok := metadata.FromContext(annotated)
 	if got, want := len(md), emptyForwardMetaCount+3; !ok || got != want {
-		t.Errorf("Expected %d metadata items in context; got %d", got, want)
+		t.Errorf("Expected %d metadata items in context; got %d", got, want, md)


md will never get printed. Nothing expects it in the string

tmc · 2016-11-04T00:47:49Z

Given the canonical MIME header key encoding we would likely want to make this 'Grpcgateway' as the header prefix. I don't love this but we should probably keep out of the way of 'Grpc-*'.

tmc · 2016-11-04T00:50:47Z

as this is it will be a breaking change as 'authorization' is passed as 'grpcgateway-authorization'

tmc · 2017-02-17T22:03:01Z

Closing as #252 addresses this

achew22 reviewed Aug 13, 2016
View reviewed changes

christianvozar force-pushed the feature/reserved-headers branch 2 times, most recently from eae2f5f to 815536c Compare August 18, 2016 20:51

achew22 reviewed Aug 19, 2016
View reviewed changes

christianvozar force-pushed the feature/reserved-headers branch from 815536c to 91c88f3 Compare August 19, 2016 16:14

christianvozar force-pushed the feature/reserved-headers branch 2 times, most recently from fc8c9c5 to 99d95a0 Compare September 14, 2016 20:33

Pass Permanent Request Headers

b2b57d2

christianvozar force-pushed the feature/reserved-headers branch from 99d95a0 to b2b57d2 Compare September 14, 2016 20:35

achew22 requested changes Oct 4, 2016

View reviewed changes

This was referenced Nov 4, 2016

Grpc-Metadata- functionality not matching documentation #245

Closed

Pass permanent HTTP request headers #252

Merged

tmc closed this Feb 17, 2017

lucas-natraj mentioned this pull request Mar 30, 2017

Why does not gateway forward headers as-is? #311

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass Permanent Request Headers #213

Pass Permanent Request Headers #213

christianvozar commented Aug 10, 2016

achew22 Aug 13, 2016

achew22 commented Aug 13, 2016

christianvozar commented Aug 15, 2016

yugui commented Aug 16, 2016 •

edited

Loading

achew22 commented Aug 16, 2016

yugui commented Aug 16, 2016

christianvozar commented Aug 17, 2016

yugui commented Aug 17, 2016

christianvozar commented Aug 18, 2016

achew22 Aug 19, 2016

christianvozar Aug 19, 2016

christianvozar commented Aug 22, 2016

achew22 commented Sep 5, 2016

kalbasit commented Oct 4, 2016

achew22 Oct 4, 2016

tmc commented Nov 4, 2016

tmc commented Nov 4, 2016

tmc commented Feb 17, 2017

Pass Permanent Request Headers #213

Pass Permanent Request Headers #213

Conversation

christianvozar commented Aug 10, 2016

achew22 Aug 13, 2016

Choose a reason for hiding this comment

achew22 commented Aug 13, 2016

christianvozar commented Aug 15, 2016

yugui commented Aug 16, 2016 • edited Loading

achew22 commented Aug 16, 2016

yugui commented Aug 16, 2016

christianvozar commented Aug 17, 2016

yugui commented Aug 17, 2016

christianvozar commented Aug 18, 2016

achew22 Aug 19, 2016

Choose a reason for hiding this comment

christianvozar Aug 19, 2016

Choose a reason for hiding this comment

christianvozar commented Aug 22, 2016

achew22 commented Sep 5, 2016

kalbasit commented Oct 4, 2016

achew22 Oct 4, 2016

Choose a reason for hiding this comment

tmc commented Nov 4, 2016

tmc commented Nov 4, 2016

tmc commented Feb 17, 2017

yugui commented Aug 16, 2016 •

edited

Loading