Document client keepalive as being potentially dangerous

[evanj]

It is very easy to screw up gRPC's client keepalive feature. Any disagreement between the client and server results in RPCs failing at unpredictable intervals with code=Unknown desc=transport is closing. GRPC-go's own README calls this "hard to debug". Due to this, you must be extremely careful at how you order deploys when you change this configuration.

This should be at least better documented, and possibly should be redesigned. More details about the problems this can cause are in Additional Context below.

Describe the solution you'd like

1. Clearly document this in all API references and in generic documentation like https://github.com/grpc/grpc/blob/master/doc/keepalive.md and the RFC for this feature. They should all say something like "Prefer to configure keepalive on the server-side, and only configure client keepalive if you are absolutely sure you need it. Incorrectly configured client keepalive will cause RPCs to fail. You must deploy the server with the same configuration before you deploy the client. When disabling this feature, you must deploy the change in the opposite order"

2. Clients should log useful error messages when this occurs. For example, see my specific suggestions for the Go client: Make debugging client keepalive misconfigurations easier grpc-go#4266

3. Clients should all implement the "double the keepalive timeout" policy that is described in the client keepalives RFC. As far as I can tell, C and Java implements this, but I don't think Go does? This still isn't perfect. I had a client setting a 10 second keepalive timeout. The default server interval is 5 minutes. This means every client will have 5 connections closed until they get an acceptable keepalive timer. Each time, it will terminate all RPCs in flight.

4. It is possible this feature could be removed. Go enables TCP keepalives with a 15 second timer by default. The C gRPC implementation could attempt to do the same using SO_KEEPALIVE, and Java could do something similar. This would probably eliminate most WAN NAT/load balancer issues with zero configuration. If a more aggressive configuration is needed, the server could initiate the ping, using the existing server-side keepalive. The disadvantage to this is that clients may not learn a connection is dead for some time. However, robust GRPC applications need to set deadlines on every RPC anyway.

5. If client-side keepalive is really needed, then it could be redesigned to be less error prone. The ideal way, in my opinion, would be for it to be under server control, since the concern about "denial of service" described in the GRFC only really applies to servers. This would require the server to send some setting to the client about the permitted settings. Possibly GRPC could use some unused bits in the HTTP2 settings frame, or could define a special request where the client could ask the server for additional settings. This would resolve these misconfigurations, at the cost of substantial additional complexity.

Additional context

I just spent a day debugging an error caused when a client had an aggressive keepalive configuration, but the server was using the default configuration. This is a tricky error, because it is timing dependent: it only happens for really slow RPCs, or when the connection is fully active. If it is idle for long enough periods, then the connection does not collect the ping strikes.

Inside of a large organization, gRPC configuration like this tends to get copy/pasted from one service to a new one. When someone copies the configuration for a client, but not for the server, the resulting gRPC configuration breaks at somewhat unpredictable intervals. It will seem like it works fine under light testing, but will fail at unpredictable intervals in production. After I encountered this error, I searched internally for other uses, and I found multiple places where I believe this setting is being used incorrectly.

You can never make the server's configuration more restrictive (increase permitted time)

Deploying this feature correctly is extremely hard. Imagine you have a service in production, and decide you need client keepalives for some reason. There is only one safe order to deploy this configuration change:

1. Deploy the update server configuration everywhere.
2. Deploy the clients with the new setting.

If you discover clients are pinging too much, or you want to remove this configuration, you need to do it in the opposite order: you must deploy all clients first, then change the setting on the server. As a result, in complex situations with multiple clients, or clients where you don't control the deploy cycle, you can only ever make this more permissive on the server.

[yashykt]

Thanks for the detailed write-up and feedback!

Some quick comments -

Clearly document this in all API references and in generic documentation like https://github.com/grpc/grpc/blob/master/doc/keepalive.md and the RFC for this feature. They should all say something like "Prefer to configure keepalive on the server-side, and only configure client keepalive if you are absolutely sure you need it. Incorrectly configured client keepalive will cause RPCs to fail. You must deploy the server with the same configuration before you deploy the client. When disabling this feature, you must deploy the change in the opposite order"

https://github.com/grpc/grpc/blob/master/doc/keepalive.md is only meant for Core and not for Java and Go, but I do agree that the documentation can be improved.

Clients should log useful error messages when this occurs. For example, see my specific suggestions for the Go client: grpc/grpc-go#4266

Clients should all implement the "double the keepalive timeout" policy that is described in the client keepalives RFC. As far as I can tell, C and Java implements this, but I don't think Go does? This still isn't perfect. I had a client setting a 10 second keepalive timeout. The default server interval is 5 minutes. This means every client will have 5 connections closed until they get an acceptable keepalive timer. Each time, it will terminate all RPCs in flight.

@dfawley to verify Go's implementation

It is possible this feature could be removed. Go enables TCP keepalives with a 15 second timer by default. The C gRPC implementation could attempt to do the same using SO_KEEPALIVE, and Java could do something similar. This would probably eliminate most WAN NAT/load balancer issues with zero configuration. If a more aggressive configuration is needed, the server could initiate the ping, using the existing server-side keepalive. The disadvantage to this is that clients may not learn a connection is dead for some time. However, robust GRPC applications need to set deadlines on every RPC anyway.

The rationale for this is documented at https://github.com/grpc/proposal/blob/master/A8-client-side-keepalive.md#rationale. Also note that deadlines do not work for long-lived streaming RPCs.

If client-side keepalive is really needed, then it could be redesigned to be less error prone. The ideal way, in my opinion, would be for it to be under server control, since the concern about "denial of service" described in the GRFC only really applies to servers. This would require the server to send some setting to the client about the permitted settings. Possibly GRPC could use some unused bits in the HTTP2 settings frame, or could define a special request where the client could ask the server for additional settings. This would resolve these misconfigurations, at the cost of substantial additional complexity.

@ejona86 (Also for general comments over keepalive design)

[dfawley]

@dfawley to verify Go's implementation

We implement keepalive scaling in Go:

https://github.com/grpc/grpc-go/blob/21976fa3e38a266811384409bc8b25437cc1ff1d/clientconn.go#L1121

[evanj]

Oh thanks! I must have missed the Go implementation, sorry about that (now that I see it: I'm not sure how? I thought I searched the code for "too_many_pings" which should have turned that up!

I will admit my familiarity is "inside data center" applications, so this is a bit outside my area of expertise to understand how WAN client application may need to use this. However, it seems to me that TCP keepalives should be sufficient to keep connections alive (e.g. Google Cloud's 10 minute NAT timeout documents using TCP keepalives to avoid this problem), although they may take some time to detect failed connections (like on the order of 3 minutes or something with Go's default keepalive settings). Maybe a flood of TCP keepalive packets can also overwhelm a server, but I suspect the operating system is probably pretty efficient at responding to ACKs.

[ejona86]

It definitely seems Go should improve the error message. That would go a long way.

Go enables TCP keepalives with a 15 second timer by default.

@evanj, this is news to me. Could you provide a reference to that?

It is possible this feature could be removed.

Doubtful. Java can enable keepalive, but can't configure it in many situations so it is left with potentially 2 hour time.

Last I saw, Go's keepalive implementation was not all that great for noticing a broken connection, because it uses the same period for the initial keepalive and for retries (so if you set it to 5 minutes, it'd take 55-60 to kill a broken connection, which isn't that different from many defaults yet is more costly). If the network provides a RST in response it can be helpful, but far too many networks don't 😢. It would have been much better if they didn't touch the reattempt delay and just let you set the initial keepalive time.

If client-side keepalive is really needed

It really is needed on client-side. Server-side keepalive only helps prevent a connection from getting broken. It does nothing to help the client discover the connection was broken.

This would require the server to send some setting to the client about the permitted settings. Possibly GRPC could use some unused bits in the HTTP2 settings frame, or could define a special request where the client could ask the server for additional settings. This would resolve these misconfigurations, at the cost of substantial additional complexity.

It'd be possible for a new SETTINGS option to be defined to inform the client the maximum poll frequency of pings it should use. But it'd take the time of actually getting that defined. If that were defined, the implementation seems trivial. Unfortunately, I do worry about getting something like that defined because it must inherently be fuzzy because there's many different usages of PING and even for keepalive multiple styles of implementation and restrictions.

[dfawley]

It definitely seems Go should improve the error message. That would go a long way.

I believe this would be improved with the fix to grpc/grpc-go#4163. A user has submitted a PR to address it, but they haven't updated the PR in a long time, unfortunately. (Our stale bot would have closed it already, but it's not working correctly right now.)

Go enables TCP keepalives with a 15 second timer by default.
@evanj, this is news to me. Could you provide a reference to that?

https://golang.org/pkg/net/#Dialer says:

// KeepAlive specifies the interval between keep-alive
// probes for an active network connection.
// If zero, keep-alive probes are sent with a default value
// (currently 15 seconds), if supported by the protocol and operating
// system. Network protocols or operating systems that do
// not support keep-alives ignore this field.
// If negative, keep-alive probes are disabled.

[ejona86]

:-/ We want keepalive on, but with the OS default. The documentation changed in golang/go@cbf90b0 and the default-on behavior was in golang/go@5bd7e9c. This is quite disappointing, and may be enough for us to argue with go team to improve the keepalive configuration. I mean, I'm happy they are enabling keepalive by default, but the "use same value for time and period without any control over number of attempts" leads to this silly aggressiveness of 15 seconds. We normally want to enable keepalive but then use the OS's default, because some people will tune their OS to 1 minute keepalive time or similar. I guess not any more...

[evanj]

Interesting. It turns out that the C implementation will currently only send 2 keep alive pings. This is apparently a known issue due to "communication with certain proxies which restrict how often pings can be sent by clients" ... which is exactly the problem, from gRPC itself, as I discovered (see #25722). So just setting the client keepalive time is insufficient for this to actually work to keep connections alive at the moment.

This again suggests all these keepalive options at least need to be documented with a warning note that you will actually need to configure multiple settings together, and if you do it incorrectly it will either not work, or will cause your connections to be dropped.

[yashykt]

Thanks for the feedback, @evanj ! #25730 updates the doc

[dfawley]

I don't understand this.

If the problem is "certain proxies which restrict how often pings can be sent by clients", then what do you intend to change when you say "We intend to deprecate this channel arg as soon as we can fix this behavior"? What behavior will you fix?

[yashykt]

The idea is that we could either convince the folks working on said proxies to not have such restrictions or Core could do things similar to how Go operates right now where it sends a window update frame before it sends a BDP ping. (Note that this is mostly an offhand comment, and I might be forgetting some nuances, but we want to get rid of this arg since it bottlenecks the keepalive ping)