From 48c4a5b7364d8334dc2eff1c256e4a0206fcae08 Mon Sep 17 00:00:00 2001
From: Derek Menteer <105233703+hashi-derek@users.noreply.github.com>
Date: Tue, 24 Oct 2023 08:05:31 -0500
Subject: [PATCH] Add grpc keepalive configuration. (#19339)

Prior to the introduction of this configuration, grpc keepalive messages were
sent after 2 hours of inactivity on the stream. This posed issues in various
scenarios where the server-side xds connection balancing was unaware that envoy
instances were uncleanly killed / force-closed, since the connections would
only be cleaned up after ~5 minutes of TCP timeouts occurred. Setting this
config to a 30 second interval with a 20 second timeout ensures that at most,
it should take up to 50 seconds for a dead xds connection to be closed.
---
 .changelog/19339.txt                                |  4 ++++
 agent/agent.go                                      |  9 +++++++++
 agent/config/builder.go                             |  2 ++
 agent/config/config.go                              |  8 +++++---
 agent/config/default.go                             |  2 ++
 agent/config/runtime.go                             | 13 +++++++++++++
 agent/config/runtime_test.go                        |  2 ++
 .../testdata/TestRuntimeConfig_Sanitize.golden      |  2 ++
 agent/config/testdata/full-config.hcl               |  2 ++
 agent/config/testdata/full-config.json              |  4 +++-
 agent/consul/server_test.go                         |  3 ++-
 agent/grpc-external/server.go                       |  9 ++++++++-
 agent/grpc-external/stats_test.go                   |  3 ++-
 agent/rpc/peering/service_test.go                   |  3 ++-
 website/content/docs/agent/config/config-files.mdx  |  4 ++++
 15 files changed, 62 insertions(+), 8 deletions(-)
 create mode 100644 .changelog/19339.txt

diff --git a/.changelog/19339.txt b/.changelog/19339.txt
new file mode 100644
index 0000000000..884fb4a3bd
--- /dev/null
+++ b/.changelog/19339.txt
@@ -0,0 +1,4 @@
+```release-note:bug
+connect: Fix bug where uncleanly closed xDS connections would influence connection balancing for too long and prevent envoy instances from starting. Two new configuration fields
+`performance.grpc_keepalive_timeout` and `performance.grpc_keepalive_interval` now exist to allow for configuration on how often these dead connections will be cleaned up.
+```
diff --git a/agent/agent.go b/agent/agent.go
index 06f538bf96..9277443777 100644
--- a/agent/agent.go
+++ b/agent/agent.go
@@ -34,6 +34,7 @@ import (
 	"golang.org/x/net/http2"
 	"golang.org/x/net/http2/h2c"
 	"google.golang.org/grpc"
+	"google.golang.org/grpc/keepalive"
 
 	"github.com/hashicorp/consul/acl"
 	"github.com/hashicorp/consul/acl/resolver"
@@ -722,6 +723,10 @@ func (a *Agent) Start(ctx context.Context) error {
 			metrics.Default(),
 			a.tlsConfigurator,
 			incomingRPCLimiter,
+			keepalive.ServerParameters{
+				Time:    a.config.GRPCKeepaliveInterval,
+				Timeout: a.config.GRPCKeepaliveTimeout,
+			},
 		)
 
 		var pt *proxytracker.ProxyTracker
@@ -757,6 +762,10 @@ func (a *Agent) Start(ctx context.Context) error {
 			metrics.Default(),
 			a.tlsConfigurator,
 			rpcRate.NullRequestLimitsHandler(),
+			keepalive.ServerParameters{
+				Time:    a.config.GRPCKeepaliveInterval,
+				Timeout: a.config.GRPCKeepaliveTimeout,
+			},
 		)
 
 		client, err := consul.NewClient(consulCfg, a.baseDeps.Deps)
diff --git a/agent/config/builder.go b/agent/config/builder.go
index c7fdcb7dd7..9d55696d8d 100644
--- a/agent/config/builder.go
+++ b/agent/config/builder.go
@@ -1019,6 +1019,8 @@ func (b *builder) build() (rt RuntimeConfig, err error) {
 		GRPCPort:                   grpcPort,
 		GRPCTLSAddrs:               grpcTlsAddrs,
 		GRPCTLSPort:                grpcTlsPort,
+		GRPCKeepaliveInterval:      b.durationValWithDefaultMin("performance.grpc_keepalive_interval", c.Performance.GRPCKeepaliveInterval, 30*time.Second, time.Second),
+		GRPCKeepaliveTimeout:       b.durationValWithDefaultMin("performance.grpc_keepalive_timeout", c.Performance.GRPCKeepaliveTimeout, 20*time.Second, time.Second),
 		HTTPMaxConnsPerClient:      intVal(c.Limits.HTTPMaxConnsPerClient),
 		HTTPSHandshakeTimeout:      b.durationVal("limits.https_handshake_timeout", c.Limits.HTTPSHandshakeTimeout),
 		KVMaxValueSize:             uint64Val(c.Limits.KVMaxValueSize),
diff --git a/agent/config/config.go b/agent/config/config.go
index 816f7ae85f..eb82397d71 100644
--- a/agent/config/config.go
+++ b/agent/config/config.go
@@ -673,9 +673,11 @@ type HTTPConfig struct {
 }
 
 type Performance struct {
-	LeaveDrainTime *string `mapstructure:"leave_drain_time"`
-	RaftMultiplier *int    `mapstructure:"raft_multiplier"` // todo(fs): validate as uint
-	RPCHoldTimeout *string `mapstructure:"rpc_hold_timeout"`
+	LeaveDrainTime        *string `mapstructure:"leave_drain_time"`
+	RaftMultiplier        *int    `mapstructure:"raft_multiplier"` // todo(fs): validate as uint
+	RPCHoldTimeout        *string `mapstructure:"rpc_hold_timeout"`
+	GRPCKeepaliveInterval *string `mapstructure:"grpc_keepalive_interval"`
+	GRPCKeepaliveTimeout  *string `mapstructure:"grpc_keepalive_timeout"`
 }
 
 type Telemetry struct {
diff --git a/agent/config/default.go b/agent/config/default.go
index cb71d4eede..f07a8bdf46 100644
--- a/agent/config/default.go
+++ b/agent/config/default.go
@@ -118,6 +118,8 @@ func DefaultSource() Source {
 			leave_drain_time = "5s"
 			raft_multiplier = ` + strconv.Itoa(int(consul.DefaultRaftMultiplier)) + `
 			rpc_hold_timeout = "7s"
+			grpc_keepalive_interval = "30s"
+			grpc_keepalive_timeout = "20s"
 		}
 		ports = {
 			dns = 8600
diff --git a/agent/config/runtime.go b/agent/config/runtime.go
index 18278b08b1..954aa7198e 100644
--- a/agent/config/runtime.go
+++ b/agent/config/runtime.go
@@ -717,6 +717,19 @@ type RuntimeConfig struct {
 	// hcl: client_addr = string addresses { grpc_tls = string } ports { grpc_tls = int }
 	GRPCTLSAddrs []net.Addr
 
+	// GRPCKeepaliveInterval determines how frequently an HTTP2 keepalive will be broadcast
+	// whenever a GRPC connection is idle. This helps detect xds connections that have died.
+	//
+	// Since the xds load balancing between servers relies on knowing how many connections
+	// are active, this configuration ensures that they are routinely detected / cleaned up
+	// on an interval.
+	GRPCKeepaliveInterval time.Duration
+
+	// GRPCKeepaliveTimeout specifies how long a GRPC client has to reply to the keepalive
+	// messages spawned from GRPCKeepaliveInterval. If a client does not reply in this amount of
+	// time, the connection will be closed by the server.
+	GRPCKeepaliveTimeout time.Duration
+
 	// HTTPAddrs contains the list of TCP addresses and UNIX sockets the HTTP
 	// server will bind to. If the HTTP endpoint is disabled (ports.http <= 0)
 	// the list is empty.
diff --git a/agent/config/runtime_test.go b/agent/config/runtime_test.go
index e7772c94a9..13497b04fc 100644
--- a/agent/config/runtime_test.go
+++ b/agent/config/runtime_test.go
@@ -6560,6 +6560,8 @@ func TestLoad_FullConfig(t *testing.T) {
 		GRPCAddrs:             []net.Addr{tcpAddr("32.31.61.91:4881")},
 		GRPCTLSPort:           5201,
 		GRPCTLSAddrs:          []net.Addr{tcpAddr("23.14.88.19:5201")},
+		GRPCKeepaliveInterval: 33 * time.Second,
+		GRPCKeepaliveTimeout:  22 * time.Second,
 		HTTPAddrs:             []net.Addr{tcpAddr("83.39.91.39:7999")},
 		HTTPBlockEndpoints:    []string{"RBvAFcGD", "fWOWFznh"},
 		AllowWriteHTTPFrom:    []*net.IPNet{cidr("127.0.0.0/8"), cidr("22.33.44.55/32"), cidr("0.0.0.0/0")},
diff --git a/agent/config/testdata/TestRuntimeConfig_Sanitize.golden b/agent/config/testdata/TestRuntimeConfig_Sanitize.golden
index b82baea535..56397ad94d 100644
--- a/agent/config/testdata/TestRuntimeConfig_Sanitize.golden
+++ b/agent/config/testdata/TestRuntimeConfig_Sanitize.golden
@@ -210,6 +210,8 @@
     "GRPCPort": 0,
     "GRPCTLSAddrs": [],
     "GRPCTLSPort": 0,
+    "GRPCKeepaliveInterval": "0s",
+    "GRPCKeepaliveTimeout": "0s",
     "GossipLANGossipInterval": "0s",
     "GossipLANGossipNodes": 0,
     "GossipLANProbeInterval": "0s",
diff --git a/agent/config/testdata/full-config.hcl b/agent/config/testdata/full-config.hcl
index 1c1fb0158a..b008ad3db6 100644
--- a/agent/config/testdata/full-config.hcl
+++ b/agent/config/testdata/full-config.hcl
@@ -335,6 +335,8 @@ performance {
     leave_drain_time = "8265s"
     raft_multiplier = 5
     rpc_hold_timeout = "15707s"
+    grpc_keepalive_interval = "33s"
+    grpc_keepalive_timeout = "22s"
 }
 pid_file = "43xN80Km"
 ports {
diff --git a/agent/config/testdata/full-config.json b/agent/config/testdata/full-config.json
index cd407d3e5d..f3514e6323 100644
--- a/agent/config/testdata/full-config.json
+++ b/agent/config/testdata/full-config.json
@@ -383,7 +383,9 @@
   "performance": {
     "leave_drain_time": "8265s",
     "raft_multiplier": 5,
-    "rpc_hold_timeout": "15707s"
+    "rpc_hold_timeout": "15707s",
+    "grpc_keepalive_interval": "33s",
+    "grpc_keepalive_timeout": "22s"
   },
   "pid_file": "43xN80Km",
   "ports": {
diff --git a/agent/consul/server_test.go b/agent/consul/server_test.go
index 6cf396efe4..95fa102d4a 100644
--- a/agent/consul/server_test.go
+++ b/agent/consul/server_test.go
@@ -28,6 +28,7 @@ import (
 	"github.com/stretchr/testify/require"
 	"golang.org/x/time/rate"
 	"google.golang.org/grpc"
+	"google.golang.org/grpc/keepalive"
 
 	"github.com/hashicorp/consul-net-rpc/net/rpc"
 
@@ -339,7 +340,7 @@ func newServerWithDeps(t *testing.T, c *Config, deps Deps) (*Server, error) {
 			oldNotify()
 		}
 	}
-	grpcServer := external.NewServer(deps.Logger.Named("grpc.external"), nil, deps.TLSConfigurator, rpcRate.NullRequestLimitsHandler())
+	grpcServer := external.NewServer(deps.Logger.Named("grpc.external"), nil, deps.TLSConfigurator, rpcRate.NullRequestLimitsHandler(), keepalive.ServerParameters{})
 	proxyUpdater := proxytracker.NewProxyTracker(proxytracker.ProxyTrackerConfig{})
 	srv, err := NewServer(c, deps, grpcServer, nil, deps.Logger, proxyUpdater)
 	if err != nil {
diff --git a/agent/grpc-external/server.go b/agent/grpc-external/server.go
index 6090a8f31a..30af8f2e6e 100644
--- a/agent/grpc-external/server.go
+++ b/agent/grpc-external/server.go
@@ -27,7 +27,13 @@ var (
 
 // NewServer constructs a gRPC server for the external gRPC port, to which
 // handlers can be registered.
-func NewServer(logger agentmiddleware.Logger, metricsObj *metrics.Metrics, tls *tlsutil.Configurator, limiter rate.RequestLimitsHandler) *grpc.Server {
+func NewServer(
+	logger agentmiddleware.Logger,
+	metricsObj *metrics.Metrics,
+	tls *tlsutil.Configurator,
+	limiter rate.RequestLimitsHandler,
+	keepaliveParams keepalive.ServerParameters,
+) *grpc.Server {
 	if metricsObj == nil {
 		metricsObj = metrics.Default()
 	}
@@ -56,6 +62,7 @@ func NewServer(logger agentmiddleware.Logger, metricsObj *metrics.Metrics, tls *
 		grpc.StatsHandler(agentmiddleware.NewStatsHandler(metricsObj, metricsLabels)),
 		middleware.WithUnaryServerChain(unaryInterceptors...),
 		middleware.WithStreamServerChain(streamInterceptors...),
+		grpc.KeepaliveParams(keepaliveParams),
 		grpc.KeepaliveEnforcementPolicy(keepalive.EnforcementPolicy{
 			// This must be less than the keealive.ClientParameters Time setting, otherwise
 			// the server will disconnect the client for sending too many keepalive pings.
diff --git a/agent/grpc-external/stats_test.go b/agent/grpc-external/stats_test.go
index 798c900148..3bd5c777cd 100644
--- a/agent/grpc-external/stats_test.go
+++ b/agent/grpc-external/stats_test.go
@@ -14,6 +14,7 @@ import (
 	"github.com/stretchr/testify/require"
 	"golang.org/x/sync/errgroup"
 	"google.golang.org/grpc"
+	"google.golang.org/grpc/keepalive"
 
 	"github.com/hashicorp/go-hclog"
 
@@ -27,7 +28,7 @@ import (
 func TestServer_EmitsStats(t *testing.T) {
 	sink, metricsObj := testutil.NewFakeSink(t)
 
-	srv := NewServer(hclog.Default(), metricsObj, nil, rate.NullRequestLimitsHandler())
+	srv := NewServer(hclog.Default(), metricsObj, nil, rate.NullRequestLimitsHandler(), keepalive.ServerParameters{})
 
 	testservice.RegisterSimpleServer(srv, &testservice.Simple{})
 
diff --git a/agent/rpc/peering/service_test.go b/agent/rpc/peering/service_test.go
index 8fde278c8b..b2b2075157 100644
--- a/agent/rpc/peering/service_test.go
+++ b/agent/rpc/peering/service_test.go
@@ -23,6 +23,7 @@ import (
 	"github.com/stretchr/testify/require"
 	gogrpc "google.golang.org/grpc"
 	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/keepalive"
 	"google.golang.org/grpc/metadata"
 	grpcstatus "google.golang.org/grpc/status"
 	"google.golang.org/protobuf/proto"
@@ -1818,7 +1819,7 @@ func newTestServer(t *testing.T, cb func(conf *consul.Config)) testingServer {
 	conf.ACLResolverSettings.EnterpriseMeta = *conf.AgentEnterpriseMeta()
 
 	deps := newDefaultDeps(t, conf)
-	externalGRPCServer := external.NewServer(deps.Logger, nil, deps.TLSConfigurator, rate.NullRequestLimitsHandler())
+	externalGRPCServer := external.NewServer(deps.Logger, nil, deps.TLSConfigurator, rate.NullRequestLimitsHandler(), keepalive.ServerParameters{})
 
 	server, err := consul.NewServer(conf, deps, externalGRPCServer, nil, deps.Logger, nil)
 	require.NoError(t, err)
diff --git a/website/content/docs/agent/config/config-files.mdx b/website/content/docs/agent/config/config-files.mdx
index 869d6f990b..651a7f6411 100644
--- a/website/content/docs/agent/config/config-files.mdx
+++ b/website/content/docs/agent/config/config-files.mdx
@@ -606,6 +606,10 @@ Refer to the [formatting specification](https://golang.org/pkg/time/#ParseDurati
     This was added in Consul 1.0. Must be a duration value such as 10s. Defaults
     to 7s.
 
+  - `grpc_keepalive_interval` - A duration that determines the frequency that Consul servers send keep-alive messages to inactive gRPC clients. Configure this setting to modify how quickly Consul detects and removes improperly closed xDS or peering connections. Default is `30s`.
+
+  - `grpc_keepalive_timeout` - A duration that determines how long a Consul server waits for a reply to a keep-alive message. If the server does not receive a reply before the end of the duration, Consul flags the gRPC connection as unhealthy and forcibly removes it. Defaults to `20s`.
+
 - `pid_file` Equivalent to the [`-pid-file` command line flag](/consul/docs/agent/config/cli-flags#_pid_file).
 
 - `ports` This is a nested object that allows setting the bind ports for the following keys: