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

Notice: grpc-go will stop sending RPCs before HTTP/2 SETTINGS frame is received #64

Open
dfawley opened this issue Oct 26, 2018 · 19 comments · May be fixed by #66
Open

Notice: grpc-go will stop sending RPCs before HTTP/2 SETTINGS frame is received #64

dfawley opened this issue Oct 26, 2018 · 19 comments · May be fixed by #66

Comments

@dfawley
Copy link

dfawley commented Oct 26, 2018

This is a notice that grpc-go intends to change in a way that we know will break the way cmux currently works by default. This will bring grpc-go in line with grpc-java's behavior, and C/wrapped languages will be following suit as well. Details and justification for the change can be found in grpc/grpc#17006. grpc-go's migration plan is proposed in grpc/grpc-go#2406. Please feel free to comment in the appropriate PR/issue for questions or concerns about this. Apologies in advance for the breaking change.

jeanbza added a commit to jeanbza/grpc-go that referenced this issue Dec 20, 2018
This removes RequireHandshakeHybrid support and changes the default behavior
to RequireHandshakeOn. Dial calls will now block and wait for a successful
handshake before proceeding. Users relying on the old hybrid behavior (cmux
users) should consult soheilhy/cmux#64.

Also, several tests have been updated to take this into consideration by
sending settings frames.
jeanbza added a commit to jeanbza/grpc-go that referenced this issue Dec 20, 2018
This removes RequireHandshakeHybrid support and changes the default behavior
to RequireHandshakeOn. Dial calls will now block and wait for a successful
handshake before proceeding. Users relying on the old hybrid behavior (cmux
users) should consult soheilhy/cmux#64.

Also, several tests have been updated to take this into consideration by
sending settings frames.
jeanbza added a commit to jeanbza/grpc-go that referenced this issue Dec 21, 2018
This removes RequireHandshakeHybrid support and changes the default behavior
to RequireHandshakeOn. Dial calls will now block and wait for a successful
handshake before proceeding. Users relying on the old hybrid behavior (cmux
users) should consult soheilhy/cmux#64.

Also, several tests have been updated to take this into consideration by
sending settings frames.
chrislusf added a commit to seaweedfs/seaweedfs that referenced this issue Jan 18, 2019
jeanbza added a commit to jeanbza/grpc-go that referenced this issue Feb 8, 2019
This removes RequireHandshakeHybrid support and changes the default behavior
to RequireHandshakeOn. Dial calls will now block and wait for a successful
handshake before proceeding. Users relying on the old hybrid behavior (cmux
users) should consult soheilhy/cmux#64.

Also, several tests have been updated to take this into consideration by
sending settings frames.
jeanbza added a commit to jeanbza/grpc-go that referenced this issue Feb 8, 2019
This removes RequireHandshakeHybrid support and changes the default behavior
to RequireHandshakeOn. Dial calls will now block and wait for a successful
handshake before proceeding. Users relying on the old hybrid behavior (cmux
users) should consult soheilhy/cmux#64.

Also, several tests have been updated to take this into consideration by
sending settings frames.
@Iulian7
Copy link

Iulian7 commented Feb 8, 2019

Any ideas how to match grpc-go requests then?

@dfawley
Copy link
Author

dfawley commented Feb 8, 2019

@lulian7 - IIUC you should be able to use the workaround suggested here for Java gRPC clients: https://github.com/soheilhy/cmux#limitations

@Iulian7
Copy link

Iulian7 commented Feb 8, 2019

@dfawley Works. I missed that.

jeanbza added a commit to jeanbza/grpc-go that referenced this issue Feb 12, 2019
This removes RequireHandshakeHybrid support and changes the default behavior
to RequireHandshakeOn. Dial calls will now block and wait for a successful
handshake before proceeding. Users relying on the old hybrid behavior (cmux
users) should consult soheilhy/cmux#64.

Also, several tests have been updated to take this into consideration by
sending settings frames.
jeanbza added a commit to jeanbza/grpc-go that referenced this issue Feb 20, 2019
This removes RequireHandshakeHybrid support and changes the default behavior
to RequireHandshakeOn. Dial calls will now block and wait for a successful
handshake before proceeding. Users relying on the old hybrid behavior (cmux
users) should consult soheilhy/cmux#64.

Also, several tests have been updated to take this into consideration by
sending settings frames.
jeanbza added a commit to grpc/grpc-go that referenced this issue Feb 27, 2019
This removes RequireHandshakeHybrid support and changes the default behavior
to RequireHandshakeOn. Dial calls will now block and wait for a successful
handshake before proceeding. Users relying on the old hybrid behavior (cmux
users) should consult soheilhy/cmux#64.

Also, several tests have been updated to take this into consideration by
sending settings frames.
@johanbrandhorst
Copy link

Testing with gRPC-Go 1.19.1 I can't get this to work even with

grpcL := mux.MatchWithWriters(cmux.HTTP2MatchHeaderFieldSendSettings("content-type", "application/grpc"))

Has anything changed in 1.19 to make this even harder? I tried debugging by sending non-matched entries to another listener and printing the output and this is what I got:

���c+��]
80�l

or as a byte slice:

[22 3 1 0 217 1 0 0 213 3 3 86 90 234 191 121 47 47 94 124 149 27 35 62]

I'm not sure what format this is. Any ideas? @menghanl @dfawley

@menghanl
Copy link

@johanbrandhorst No, this behavior didn't change in 1.19.
I also don't recognize the bytes...

Does it work with 1.18? Is the client a simple client?

@johanbrandhorst
Copy link

I haven't tried 1.18 yet, but yes it is a simple grpc-go client. I'll try 1.18 and below.

@johanbrandhorst
Copy link

Hm, everything is working as expected when serving gRPC without TLS. I guess this is a false alarm. What's the best way to use cmux and still encrypt the connection?

@johanbrandhorst
Copy link

johanbrandhorst commented May 21, 2019

For future reference;

I got the following working:

  1. Use tls.Listen to create the listener.
  2. Use (*grpc.Server).Serve without grpc.Creds.
  3. Use grpc.WithTransportCredentials as usual in the client.

srenatus added a commit to chef/automate that referenced this issue May 22, 2019
This was carried over from some ancient grpc-gateway example code[1]; and
we're not exposing the GRPC API that way right now.

Using (*grpc.Server).ServeHTTP has some issues:

1. it's known to be slow, very slow[2]
2. it's seems to be not working with go-grpc >= 1.19 [3]

Furthermore, we're exposing the proper GRPC server (the native go-grpc
one) on a different port. If decide to (finally!) expose our GRPC API,
that's the thing we should expose.

So, since we don't need this multiplexing business, we shouldn't keep
the code around. This is the cleanup.

[1]: https://github.com/philips/grpc-gateway-example/
[2]: grpc/grpc-go#586
[3]: soheilhy/cmux#64 (comment)

Signed-off-by: Stephan Renatus <srenatus@chef.io>
srenatus added a commit to chef/automate that referenced this issue May 22, 2019
This was carried over from some ancient grpc-gateway example code[1]; and
we're not exposing the GRPC API that way right now.

Using (*grpc.Server).ServeHTTP has some issues:

1. it's known to be slow, very slow[2]
2. it's seems to be not working with go-grpc >= 1.19 [3]

Furthermore, we're exposing the proper GRPC server (the native go-grpc
one) on a different port. If decide to (finally!) expose our GRPC API,
that's the thing we should expose.

So, since we don't need this multiplexing business, we shouldn't keep
the code around. This is the cleanup.

[1]: https://github.com/philips/grpc-gateway-example/
[2]: grpc/grpc-go#586
[3]: soheilhy/cmux#64 (comment)

Signed-off-by: Stephan Renatus <srenatus@chef.io>
srenatus added a commit to chef/automate that referenced this issue May 22, 2019
* automate-gateway: cleanup GRPC/HTTP multiplexing cruft

This was carried over from some ancient grpc-gateway example code[1]; and
we're not exposing the GRPC API that way right now.

Using (*grpc.Server).ServeHTTP has some issues:

1. it's known to be slow, very slow[2]
2. it's seems to be not working with go-grpc >= 1.19 [3]

Furthermore, we're exposing the proper GRPC server (the native go-grpc
one) on a different port. If decide to (finally!) expose our GRPC API,
that's the thing we should expose.

So, since we don't need this multiplexing business, we shouldn't keep
the code around. This is the cleanup.

[1]: https://github.com/philips/grpc-gateway-example/
[2]: grpc/grpc-go#586
[3]: soheilhy/cmux#64 (comment)

* [integration] automate-gateway: fix tests to use GRPC port

So apparently these HAD used the multiplexing code I've deleted. Well,
they don't have to. Changed the hardcoded port to automate-gateway's
default grpc port.

Signed-off-by: Stephan Renatus <srenatus@chef.io>
@glerchundi
Copy link

@johanbrandhorst I finally took some time to sketch a code snippet to make our use case work. It did, but I don't know if it's going to work after reading this commit (as we didn't upgraded to that grpc-go version yet). I want to expose grpc-gateway + pure grpc in the same port without security; It's a microservice never exposed directly to the outside so it's ok. But I don't want to commit to a solution that cannot be extended in the future by adopting an approach like BeyondCorp. This means that our microservice should be able to be exposed publicly by, obviously, using TLS. The mentioned code allows addition TLS over the whole cmux but I don't know if you encountered further problems with this approach?

@johanbrandhorst
Copy link

There should be no reason you can't do TLS termination before any packets reach the service.

@glerchundi
Copy link

glerchundi commented Jul 1, 2019

Correct, thanks for confirming. Although we found another issue with the combo grpc-gateway, grpc, port-sharing, cmux and persistent connections kept by a reverse proxy (envoy in this case). It seems that cmux is not very useful if we're behind a proxy which keeps persistent connection without doing any differentiation between the underlying protocol being used.

Take this sequence as an example:
As stated by cmux, it matches the connection when it's accepted, so it's possible this to happen:

  1. Someone makes a gRPC call
  2. Envoy creates a new http connection
  3. cmux sees it's a http2 connection and the content-type is gRPC it then creates a new connection against grpc.Serve
  4. grpc.Serve replies to the gRPC request succesfully. While Envoy decides not to close the fresh connection for performance reasons
  5. Someone makes a REST call
  6. Envoy decides to use the same connection created before
  7. As the connection was previously matched grpc.Serve is called again and it fails as it's not a gRPC call

WDYT @johanbrandhorst?

PS.: I don't really know which is the right forum for this question as there are lots of little components taking part in the overall problem...

@johanbrandhorst
Copy link

This is probably not the place, no. Maybe raise an issue against Envoy?

@glerchundi
Copy link

glerchundi commented Jul 2, 2019

This is probably not the place, no. Maybe raise an issue against Envoy?

Already asked in Envoy and it seems like nothing could be done there as it keeps one and only connection pool for everything no matter it's plain or gRPC.

UPDATE: It can be done but cannot because we're using Contour to manage Envoy.

@mpuncel wrote:

Envoy uses the same connection pool for gRPC and non-gRPC. You could probably accomplish what you want with 2 different clusters and set up a routing rule that matches on gRPC to go to 1 and everything else goes to the other

@johanbrandhorst
Copy link

There's a question to be asked about why you're using cmux behind a proxy like envoy anyway. Envoy can do the traffic splitting for you, so why not just remove the cmux?

@glerchundi
Copy link

This is probably not the place, no. Maybe raise an issue against Envoy?

There are several reasons:

  • We want to have one and only one container for both (gRPC and gRPC-Gateway) as it simplifies everything from management/operation to understanding of how the microservice works by the team.
  • The operational cost for having multiple ports is bigger than having only one.
  • Although Envoy supports multiplexing connections based on more advanced L7 rules (like header content based ones), Heptio VMware Contour doesn't support it yet. Which means that currently it's not even possible.

Did you have this problem previously @johanbrandhorst? How do you currently operate services that publish both gRPC + REST in your environment?

Thanks!

@johanbrandhorst
Copy link

I think we can move this discussion to the #grpc channel on Gophers slack. We've taken up this issue for long enough.

csweichel pushed a commit to csweichel/jaeger that referenced this issue Aug 23, 2019
csweichel pushed a commit to csweichel/jaeger that referenced this issue Aug 23, 2019
breaking change: grpc/grpc-go#2406
workaround described in:
- soheilhy/cmux#64
- https://github.com/soheilhy/cmux#limitations

Signed-off-by: Christian Weichel <christian.weichel@typefox.io>
csweichel pushed a commit to csweichel/jaeger that referenced this issue Aug 25, 2019
breaking change: grpc/grpc-go#2406
workaround described in:
- soheilhy/cmux#64
- https://github.com/soheilhy/cmux#limitations

Signed-off-by: Christian Weichel <christian.weichel@typefox.io>
yurishkuro pushed a commit to yurishkuro/jaeger that referenced this issue Aug 25, 2019
breaking change: grpc/grpc-go#2406
workaround described in:
- soheilhy/cmux#64
- https://github.com/soheilhy/cmux#limitations

Signed-off-by: Christian Weichel <christian.weichel@typefox.io>
pavolloffay pushed a commit to jaegertracing/jaeger that referenced this issue Sep 2, 2019
* Add unit test for gRPC over cmux

Signed-off-by: Yuri Shkuro <ys@uber.com>

* Fix tests

Signed-off-by: Yuri Shkuro <ys@uber.com>

* Fix gRPC query service cmux

breaking change: grpc/grpc-go#2406
workaround described in:
- soheilhy/cmux#64
- https://github.com/soheilhy/cmux#limitations

Signed-off-by: Christian Weichel <christian.weichel@typefox.io>

* Fix asertions

Signed-off-by: Yuri Shkuro <ys@uber.com>

* Use DialContext

Signed-off-by: Yuri Shkuro <ys@uber.com>

* Clean-up timeouts

Signed-off-by: Yuri Shkuro <ys@uber.com>
bookmoons pushed a commit to bookmoons/jaeger that referenced this issue Sep 10, 2019
* Add unit test for gRPC over cmux

Signed-off-by: Yuri Shkuro <ys@uber.com>

* Fix tests

Signed-off-by: Yuri Shkuro <ys@uber.com>

* Fix gRPC query service cmux

breaking change: grpc/grpc-go#2406
workaround described in:
- soheilhy/cmux#64
- https://github.com/soheilhy/cmux#limitations

Signed-off-by: Christian Weichel <christian.weichel@typefox.io>

* Fix asertions

Signed-off-by: Yuri Shkuro <ys@uber.com>

* Use DialContext

Signed-off-by: Yuri Shkuro <ys@uber.com>

* Clean-up timeouts

Signed-off-by: Yuri Shkuro <ys@uber.com>
@ghost
Copy link

ghost commented Oct 7, 2019

TLS: net/http uses a type assertion to identify TLS connections; since cmux's lookahead-implementing connection wraps the underlying TLS connection, this type assertion fails. Because of that, you can serve HTTPS using cmux but http.Request.TLS would not be set in your handlers.

do I understand this correctly if I assume that I can still serve http2 and http1(grpc server+ grpc gateway server) via cmux on the same port and have TLS for the clients and I only have to enable tls on the listener that I provide to cmux and disable tls for the servers the cmux is routing to? ie. I still can serve secure grpc and grpc gateway via cmux since both servers are linked to the cmux only via memory and not network, hence no security issue with tls "termination" on cmux level?

@johanbrandhorst
Copy link

That is correct, TLS is terminated by cmux, but it's still in-memory, so it's about the same level of security.

@delwaterman
Copy link

Ran into this issue. Can we at least update the documentation for this?

johanbrandhorst added a commit to johanbrandhorst/cmux that referenced this issue Jan 10, 2022
gRPC-Go has been waiting for the settings frame for a while now (soheilhy#64),
and having the old handler in the example is confusing users (soheilhy#67).
Use the new handler in the example and remove the "Limitations" from the README.
johanbrandhorst added a commit to johanbrandhorst/cmux that referenced this issue Jan 10, 2022
gRPC-Go has been waiting for the settings frame for a while now (soheilhy#64),
and having the old handler in the example is confusing users (soheilhy#67).
Use the new handler in the example and remove the "Limitations" from the README.
outdoorSpirit added a commit to outdoorSpirit/Go-Jag that referenced this issue May 3, 2024
* Add unit test for gRPC over cmux

Signed-off-by: Yuri Shkuro <ys@uber.com>

* Fix tests

Signed-off-by: Yuri Shkuro <ys@uber.com>

* Fix gRPC query service cmux

breaking change: grpc/grpc-go#2406
workaround described in:
- soheilhy/cmux#64
- https://github.com/soheilhy/cmux#limitations

Signed-off-by: Christian Weichel <christian.weichel@typefox.io>

* Fix asertions

Signed-off-by: Yuri Shkuro <ys@uber.com>

* Use DialContext

Signed-off-by: Yuri Shkuro <ys@uber.com>

* Clean-up timeouts

Signed-off-by: Yuri Shkuro <ys@uber.com>
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

Successfully merging a pull request may close this issue.

6 participants