Skip to content

Add SO_KEEPALIVE option for upstream connections. #614

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.

Sign up for GitHub

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

Closed
JonathanO wants to merge 6 commits into from
Closed

Add SO_KEEPALIVE option for upstream connections. #614

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9d38f06
Add SO_KEEPALIVE option for upstream connections.
JonathanO Apr 9, 2018
780f785
Fixes code review nits.
JonathanO Apr 10, 2018
b60ba8a
Added disable flag for keepalive option.
JonathanO Apr 10, 2018
83543bd
Moved TcpKeepalive setting into new message
JonathanO Apr 11, 2018
3580cf6
Fix field number conflict and bump next free.
JonathanO Apr 11, 2018
51d7f1a
Remove support for bootstrap config of keepalive
JonathanO Apr 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions docs/root/intro/version_history.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ Version history
:ref:`cluster specific <envoy_api_field_Cluster.upstream_bind_config>` options.
* sockets: added `IP_TRANSPARENT` socket option support for :ref:`listeners
<envoy_api_field_Listener.transparent>`.
* sockets: added `SO_KEEPALIVE` socket option for upstream connections via :ref:`cluster manager wide
<envoy_api_field_config.bootstrap.v2.ClusterManager.upstream_connection_options>` options, with override
:ref:`per cluster <envoy_api_field_Cluster.upstream_connection_options>`.
* tracing: the sampling decision is now delegated to the tracers, allowing the tracer to decide when and if
to use it. For example, if the :ref:`x-b3-sampled <config_http_conn_man_headers_x-b3-sampled>` header
is supplied with the client request, its value will override any sampling decision made by the Envoy proxy.
Expand Down
11 changes: 10 additions & 1 deletion envoy/api/v2/cds.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ service ClusterDiscoveryService {
// [#protodoc-title: Clusters]

// Configuration for a single upstream cluster.
// [#comment:next free field: 30]
// [#comment:next free field: 31]
message Cluster {
// Supplies the name of the cluster which must be unique across all clusters.
// The cluster name is used when emitting
Expand Down Expand Up @@ -429,6 +429,10 @@ message Cluster {

// Determines how Envoy selects the protocol used to speak to upstream hosts.
ClusterProtocolSelection protocol_selection = 26;

// Optional options for upstream connections.
// This overrides any upstream_connection_options specified in the bootstrap proto.
envoy.api.v2.UpstreamConnectionOptions upstream_connection_options = 30;
}

// An extensible structure containing the address Envoy should bind to when
Expand All @@ -437,3 +441,8 @@ message UpstreamBindConfig {
// The address Envoy should bind to when establishing upstream connections.
core.Address source_address = 1;
}

message UpstreamConnectionOptions {
Copy link
Copy Markdown
Member

@htuch htuch Apr 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I'd rename the message to just ConnectionOptions, since we might want to reuse this elsewhere, e.g. health check connections etc.

Copy link
Copy Markdown
Contributor Author

@JonathanO JonathanO Apr 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the health checks will inherit these options from the cluster already in my implementation.
I didn't want to call it ConnectionOptions, as I suspect we'll want a similar, but not identical, message for accept()ed connections.
Perhaps ClusterConnectionOptions?

// If set then set SO_KEEPALIVE on the socket to enable TCP Keepalives.
core.TcpKeepalive tcp_keepalive = 1;
}
17 changes: 17 additions & 0 deletions envoy/api/v2/core/address.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,23 @@ message SocketAddress {
bool ipv4_compat = 6;
}

message TcpKeepalive {
// Maximum number of keepalive probes to send without response before deciding
// the connection is dead. Default is to use the OS level configuration (unless
// overridden, Linux defaults to 9.)
google.protobuf.UInt32Value keepalive_probes = 1;
// The number of seconds a connection needs to be idle before keep-alive probes
// start being sent. Default is to use the OS level configuration (unless
// overridden, Linux defaults to 7200s (ie 2 hours.)
google.protobuf.UInt32Value keepalive_time = 2;
// The number of seconds between keep-alive probes. Default is to use the OS
// level configuration (unless overridden, Linux defaults to 75s.)
google.protobuf.UInt32Value keepalive_interval = 3;
// Flag to explicitly disable keepalive probes, allows overriding of settings
// inherited by a cluster from the cluster manager. Default is false.
bool disable = 4;
}

message BindConfig {
// The address to bind to when creating a socket.
SocketAddress source_address = 1
Expand Down
4 changes: 4 additions & 0 deletions envoy/config/bootstrap/v2/bootstrap.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,10 @@ message ClusterManager {
// <envoy_api_field_core.ApiConfigSource.api_type>` :ref:`GRPC
// <envoy_api_enum_value_core.ApiConfigSource.ApiType.GRPC>`.
envoy.api.v2.core.ApiConfigSource load_stats_config = 4;

// Optional options for upstream connections.
// This may be overridden on a per-cluster basis by upstream_connection_options in the cds_config.
envoy.api.v2.UpstreamConnectionOptions upstream_connection_options = 5;
Copy link
Copy Markdown
Member

@ggreenway ggreenway Apr 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is preferable to omit this. Envoy doesn't in general do hierarchical config, in the sense of setting an option at a higher level and overriding later.

This has been handled inconsistently (for instance bind config). But unless there's a compelling reason, please remove this (and "bool disable = 4" from the TcpKeepalive message).

Copy link
Copy Markdown
Contributor Author

@JonathanO JonathanO Apr 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I don't have a particularly good excuse for wanting this, and removing it will simplify the code a little, so I'll remove it.

}

// Envoy process watchdog configuration. When configured, this monitors for
Expand Down