Issue 605 grpc host matching#632
Conversation
|
|
|
This would be very useful for me. |
|
Triggering a rebuild ... will merge if tests come back clean. We have Travis setup again. |
|
@tommyalatalo tests pass and I've looked over the code. Would you be willing to add some documentation updates to describe the metadata options? |
|
Would be good to have the metadata doco updated to get a better understanding. cheers |
|
doc is updated, please check if that is enough. |
There was a problem hiding this comment.
I know it is outside the scope of the review right now but this can be more concisely written as:
| } | |
| kv := map[string]string{"dstHost":"www.example.com"} | |
| if _, ok := kv["dstHost"]; !ok { | |
| fmt.Print("fail") | |
| } | |
| fmt.Println(kv["dstHost"]) |
There was a problem hiding this comment.
The print "fmt.Println("hosts", md)" is only for debug purpose, it shouldn't be enabled/included in production. Otherwise each request will trigger this print, cause the performance issue.
ishworg
left a comment
ishworg
left a comment
There was a problem hiding this comment.
nice work bud. I have commented few small things.
|
We hit the 4mb limit also. Any chance we can get this merged in and a release cut? Thanks! |
|
@rocwenlinux can you squash this? If so I'll merge this and slate it for 1.6 |
…ng 'dsthost' in metadata in grpc client side.;set max message size for grpc proxy server and client;
@nathanejohnson squashed. |
nathanejohnson
left a comment
nathanejohnson
left a comment
There was a problem hiding this comment.
Can we make max message sizes configurable?
| @@ -161,6 +161,8 @@ func newGrpcProxy(cfg *config.Config, tlscfg *tls.Config) []grpc.ServerOption { | |||
| grpc.UnknownServiceHandler(handler), | |||
| grpc.StreamInterceptor(proxyInterceptor.Stream), | |||
| grpc.StatsHandler(statsHandler), | |||
There was a problem hiding this comment.
It would be great to have these values be configurable. Could you add these to config/config.go in the Config struct, maybe as GRPCMaxRxMsgSize and GRPCMaxTxSize , and then setting sane default values in config/default.go ? Perhaps setting the current default of 4MB is still sane here, 1gb seems like it might be excessive for many people? And if it's configurable, this allows people to relax the limits.
|
|
||
| func (p *grpcConnectionPool) newConnection(ctx context.Context, target *route.Target) (*grpc.ClientConn, error) { | ||
| opts := []grpc.DialOption{ | ||
| grpc.WithDefaultCallOptions(grpc.CallCustomCodec(grpc_proxy.Codec())), |
There was a problem hiding this comment.
See above about the configurable limits
|
I'm going to merge this and do a subsequent PR to make these limits configurable. Some good work in here and I'm guessing the original authors have moved on. |
…able add configurable grpc message sizes to #632
8 participants
We have implemented grpc host matching by using client-side metadata which is used to match hosts. If the specified metadata host is not found, the request will be routed according to the current default behavior, i.e. to any host serving the grpc service requested.
In doing this we also found that the GRPC max message size is hardcoded in fabio to the default 4mb, we increased this to 1Gb.