Support user configurable header forwarding & context metadata #336

tamalsaha · 2017-03-23T16:09:00Z

Fixes #311

@tmc / @yugui , if the general idea looks ok to you, I will add tests for this.

cc:@sadlil

tmc · 2017-03-23T18:21:29Z

@tamalsaha thanks for your contribution.

A couple notes:

Mutating a global to modify behavior should be avoided, I'd prefer passing some sort of Option.
It's not clear to me that we should supply a set of Matcher implementations.

tamalsaha · 2017-03-23T18:32:10Z

Thanks @tmc .

Mutating a global to modify behavior should be avoided, I'd prefer passing some sort of Option.

Do you have any suggestion about that? The only passed thing I can see if the Mux

grpc-gateway/protoc-gen-grpc-gateway/gengateway/template.go

Line 340 in 6863684

    
           func Register{{$svc.GetName}}Handler(ctx context.Context, mux *runtime.ServeMux, conn *grpc.ClientConn) error {

It's not clear to me that we should supply a set of Matcher implementations.

I have no options about this. I thought it could be handy.

tmc · 2017-03-23T18:41:04Z

I would prefer the Register methods taking a variadic options slice or the mux expanding a bit in responsibility.

tamalsaha · 2017-03-23T19:25:52Z

@tmc, I have looked into the code and template. It seems to me that extending ServeMux might be easier way. I can also turn my matchers into ServeMuxOption. So, mux sounds good? Then I will try to update this pr. Now, this will be a pretty big change.

tamalsaha · 2017-03-23T20:58:26Z

@tmc, I think it is ready for review. I have extended ServeMux to add the matchers.

tmc · 2017-03-23T21:42:18Z

$DAYJOB beckons but I can review this sometime soon. In the mean time you might take a look at somewhat related efforts in #323 and #305. Also, please sign the Google CLA. Thanks!

tamalsaha · 2017-03-23T21:53:21Z

@tmc, I have signed the CLA.

I think #323 is taken care by this pr.

nilium · 2017-03-24T01:07:56Z

runtime/matchers.go

+	return func(mux *ServeMux) {
+		mux.headerMatchers = append(mux.headerMatchers, func(key string) bool {
+			return strings.HasPrefix(strings.ToLower(key), h)
+		})


I think this could be handled without the ToLower by doing an EqualFold on the head of the key. For example, https://play.golang.org/p/lh_JIYnZ0s

I only mention this because EqualFold is a bit more involved than comparing two lowercase strings. This may not really matter, though, since I think the character set available to HTTP headers is fairly restrictive (i.e., ASCII). So, nitpicking a bit.

Thanks @nilium . I updated accordingly.

tamalsaha · 2017-03-24T10:43:14Z

@tmc, I have added support for context annotators in my pr. Looks like we need this too!

tamalsaha · 2017-03-28T05:02:07Z

@ilius, I have added support for adding metadata to context via Mux options. Does this solve your use-case?

ilius · 2017-03-28T06:12:52Z

@tamalsaha Not sure. Does it support returning error (in REST response) before reaching grpc client?

We need to read request cookie, set grpc context or return error, on each request
As I have done in #323

tamalsaha · 2017-03-28T06:23:58Z

I have few questions:

When you read cookie, do you need to send it via a direct key on Context or as part of metadata in Context? From looking at the metadata code, it seems to perform some encoding, decoding based on the data.
If your intention is to reject a call if it is missing Cookie? If so, why not just pass the cookie from http request to context metadata. Then in the grpc side, you can add an interceptor that will reject requests that are missing cookie. Does that work for you?

ilius · 2017-03-28T10:30:32Z

We use metadata for putting context item (JWT token), because otherwise it won't be transferred via grpc
We reject the call if the cookie is missing, or invalid (invalid signed JWT token)

tamalsaha · 2017-03-28T14:29:19Z

@ilius, in that case, you will be able to handle your use case using this pr, like below:

gwMux := runtime.NewServeMux(runtime.EqualFoldMatcher("Cookie"))

Then on the grpc side, you can check for JWT token in cookie and reject the request if it fails in a grpc interceptor. IMO, this is better than failing at the gateway layer for 2 reasons:

You keep all your application logic in one place instead of passing some to the gateway.
If/when applications can direct http/2 calls to the grpc service, it will also work if JWT token in passed via cookie.

tmc

@tamalsaha thanks for this! I have a few small comments but this is looking great.

tmc · 2017-04-17T04:49:47Z

runtime/matchers.go

+import "strings"
+
+// EqualMatcher performs a case-sensitive equality match for request metadata keys
+func EqualMatcher(s string) ServeMuxOption {


I would prefer a WithHeaderMatcher Option that accepts a func(string) bool to both support more scenarios but also to have a smaller api surface area.

Thinking further, perhaps the signature should be func(string) (string, bool) to allow transformations in addition to simple predicate filtering.

@tmc , I am not sure what you mean by WithHeaderMatcher option? Mind elaborating a bit?

I mean to something like:

type HeaderMatcherFunc func(string) (string, bool) func WithHeaderMatcher(fn HeaderMatcherFunc) ServeMuxOption { return func(s *ServeMux) { s.headerMatcher = fn } }

tmc · 2017-04-17T04:49:51Z

runtime/mux.go

@@ -36,12 +39,21 @@ func WithForwardResponseOption(forwardResponseOption func(context.Context, http.
 	}
 }

+// WithMetadata returns metadata to be passed with context.


this comment should elaborate a bit more

tmc · 2017-04-17T04:50:27Z

runtime/mux.go

 // NewServeMux returns a new ServeMux whose internal mapping is empty.
 func NewServeMux(opts ...ServeMuxOption) *ServeMux {
 	serveMux := &ServeMux{
 		handlers:               make(map[string][]handler),
 		forwardResponseOptions: make([]func(context.Context, http.ResponseWriter, proto.Message) error, 0),
 		marshalers:             makeMarshalerMIMERegistry(),
+		headerMatchers:         make([]func(string) bool, 0),
+		metadataAdders:          make([]func(context.Context, *http.Request) metadata.MD, 0),


nit: can we call this metadataAnnotators?

tamalsaha · 2017-04-17T22:46:50Z

@tmc, tests are passing.

tmc · 2017-04-17T22:47:49Z

I wonder if we should expose the direction to the header matcher?

tamalsaha · 2017-04-17T22:51:13Z

Like a IncomingHeaderMatcher, OutgoingHeaderMatcher ?

tmc · 2017-04-18T00:28:21Z

@tamalsaha I think those names are fine.

tmc · 2017-04-18T00:30:31Z

https://github.com/grpc/grpc-go/blob/master/Documentation/grpc-metadata.md FWIW

tamalsaha · 2017-04-18T03:39:39Z

Updated pr to add separate incoming/outgoing HeaderMatcher. IncomingHeaderMatcher is set to DefaultHeaderMatcher to maintain current behavior.

tmc · 2017-04-18T03:47:15Z

LGTM

ilius · 2017-04-18T05:41:38Z

@tamalsaha I don't understand. We have no access to cookie in our grpc server handlers. Only context and input(request). We must use context for storing token. Cookie is only for REST (http1). So we must copy the token from cookie to GRPC context when translating REST to GRPC.

tamalsaha · 2017-04-18T05:49:32Z

@ilius , if you want to forward the Cookie in http request to grpc, you can add an incomingHeaderMatcher for that.

runtime.NewServeMux(runtime.WithIncomingHeaderMatcher(func(h string) (string, bool) {
		if strings.EqualFold(h, "Cookie") {
			return h, true
		}
		return "", false
	}));

If you want to forward a specific key from Cookie, you can also do that by returning that in the if block.

ilius · 2017-04-18T05:56:41Z

And where to put this header in the grpc context?

tamalsaha · 2017-04-18T05:59:09Z

It will be automatically added to the Metadata in grpc context. See LN # 66 & # 95 https://github.com/grpc-ecosystem/grpc-gateway/blob/master/runtime/context.go#L66

ilius · 2017-04-18T09:32:46Z

Thanks

ilius · 2017-04-19T02:58:11Z

@tamalsaha Sorry, I think runtime.WithIncomingHeaderMatcher transforms headers, not cookies

tamalsaha · 2017-04-19T03:04:12Z

@ilius , I am not sure I understand you. Using runtime.WithIncomingHeaderMatcher, you can pass the full http request's Cookie header to grpc. You can also send a specific key from the Cookie, if you prefer that. And custom application specific logic for validation that Cookie/Cookie-key should be done in your grpc server (directly in the method or using a interceptor). Does that make sense?

ilius · 2017-04-19T03:37:03Z

Thanks, I get it now (the name mistakened me)

lingyuan2014 · 2017-05-15T22:57:29Z

This is awesome. Been waiting for 2+ months for this feature. Would like to see this in a new release. (It's kinda unfortunate this missed 1.2.2 by just one commit)

rogchap · 2022-03-04T03:08:17Z

For anyone reading this and looking for a way to get a *http.Cookie from the request:

In a normal REST request you would access Cookies via req.Cookies() or req.Cookie("my-cookie")

With all the suggestions above we are only mapping the request headers to the gRPC metadata (FYI: cookies are transmitted in the request header)

This means you have access to the raw cookie string in the metadata, not the http.Cookie struct.

There is no ParseCookie() function in the standard library 😞 see: golang/go#25194

So as a workaround you may need to do something like this (code not tested):

	md, _ := metadata.FromIncomingContext(ctx)
	rawCookies := md.Get("Grpc-Metadata-Cookie")
	dummyReq, _ := http.NewRequest("", "", nil)
	for _, rawCookie := range rawCookies {
		dummyReq.Header.Add("Cookie", rawCookie)
	}
	myCookie, _ := dummyReq.Cookie("my-cookie")

tamalsaha force-pushed the header-pass branch 3 times, most recently from f41c8ca to 15d0787 Compare March 23, 2017 18:21

tamalsaha force-pushed the header-pass branch from 5ce0d94 to eee52b6 Compare March 23, 2017 20:56

tamalsaha force-pushed the header-pass branch from eee52b6 to ba94bf4 Compare March 23, 2017 20:58

tamalsaha force-pushed the header-pass branch from ba94bf4 to bbaf9f6 Compare March 24, 2017 00:58

nilium reviewed Mar 24, 2017

View reviewed changes

tamalsaha force-pushed the header-pass branch from bbaf9f6 to 2668bc5 Compare March 24, 2017 01:23

tamalsaha changed the title ~~Allow user configurable header forwarding~~ Allow user configurable header forwarding & context annotation Mar 24, 2017

tamalsaha force-pushed the header-pass branch 2 times, most recently from 41d05d7 to 7af137c Compare March 24, 2017 12:02

tamalsaha changed the title ~~Allow user configurable header forwarding & context annotation~~ Support user configurable header forwarding & context metadata Mar 24, 2017

This was referenced Mar 30, 2017

Release 1.2.0 #340

Closed

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

Closed

tmc mentioned this pull request Apr 15, 2017

Release 1.3.0 #357

Closed

3 tasks

tmc reviewed Apr 17, 2017

View reviewed changes

tmc mentioned this pull request Apr 17, 2017

add support for adding context annotators, expose runtime.AddContextAnnotator #323

Closed

tamalsaha force-pushed the header-pass branch from 85468ba to 995b219 Compare April 17, 2017 22:41

Extend ServeMux to allow user configurable header forwarding.

fb9e6c9

tamalsaha force-pushed the header-pass branch from 995b219 to fb9e6c9 Compare April 18, 2017 03:38

tmc merged commit 893772d into grpc-ecosystem:master Apr 18, 2017

sadlil deleted the header-pass branch April 18, 2017 03:50

sadlil restored the header-pass branch April 18, 2017 03:50

tamalsaha deleted the header-pass branch April 18, 2017 05:50

tmc mentioned this pull request Apr 19, 2017

Allow whitelisting of particular HTTP headers to map to metadata. #253

Closed

tamalsaha mentioned this pull request Apr 20, 2017

docs: provide some common vendoring tips/pitfalls #365

Open

tamalsaha mentioned this pull request May 1, 2017

Return response headers from grpc server #374

Merged

tamalsaha mentioned this pull request May 11, 2017

Can't generate code with last version #384

Closed

achew22 mentioned this pull request May 29, 2017

Add method middleware via gateway-specific method annotations #305

Closed

feedmeapples mentioned this pull request Aug 15, 2021

Add AuthZ support temporalio/ui-server#42

Merged

Support user configurable header forwarding & context metadata #336

Support user configurable header forwarding & context metadata #336

Conversation

tamalsaha commented Mar 23, 2017 • edited Loading

tmc commented Mar 23, 2017

tamalsaha commented Mar 23, 2017 • edited Loading

tmc commented Mar 23, 2017 • edited Loading

tamalsaha commented Mar 23, 2017

tamalsaha commented Mar 23, 2017

tmc commented Mar 23, 2017 • edited Loading

tamalsaha commented Mar 23, 2017

nilium Mar 24, 2017 • edited Loading

Choose a reason for hiding this comment

tamalsaha Mar 24, 2017

Choose a reason for hiding this comment

tamalsaha commented Mar 24, 2017

tamalsaha commented Mar 28, 2017

ilius commented Mar 28, 2017 • edited Loading

tamalsaha commented Mar 28, 2017 • edited Loading

ilius commented Mar 28, 2017

tamalsaha commented Mar 28, 2017 • edited Loading

tmc left a comment

Choose a reason for hiding this comment

tmc Apr 17, 2017

Choose a reason for hiding this comment

tmc Apr 17, 2017

Choose a reason for hiding this comment

tamalsaha Apr 17, 2017

Choose a reason for hiding this comment

tmc Apr 17, 2017

Choose a reason for hiding this comment

tmc Apr 17, 2017

Choose a reason for hiding this comment

tmc Apr 17, 2017

Choose a reason for hiding this comment

tamalsaha commented Apr 17, 2017

tmc commented Apr 17, 2017

tamalsaha commented Apr 17, 2017

tmc commented Apr 18, 2017

tmc commented Apr 18, 2017

tamalsaha commented Apr 18, 2017 • edited Loading

tmc commented Apr 18, 2017

ilius commented Apr 18, 2017

tamalsaha commented Apr 18, 2017

ilius commented Apr 18, 2017

tamalsaha commented Apr 18, 2017

ilius commented Apr 18, 2017

ilius commented Apr 19, 2017

tamalsaha commented Apr 19, 2017 • edited Loading

ilius commented Apr 19, 2017

lingyuan2014 commented May 15, 2017

rogchap commented Mar 4, 2022

tamalsaha commented Mar 23, 2017 •

edited

Loading

tamalsaha commented Mar 23, 2017 •

edited

Loading

tmc commented Mar 23, 2017 •

edited

Loading

tmc commented Mar 23, 2017 •

edited

Loading

nilium Mar 24, 2017 •

edited

Loading

ilius commented Mar 28, 2017 •

edited

Loading

tamalsaha commented Mar 28, 2017 •

edited

Loading

tamalsaha commented Mar 28, 2017 •

edited

Loading

tamalsaha commented Apr 18, 2017 •

edited

Loading

tamalsaha commented Apr 19, 2017 •

edited

Loading