From 845171d5d77b7084ed9d87f74cfd5cf228f5fe7a Mon Sep 17 00:00:00 2001 From: BhavikaSharma Date: Wed, 18 Oct 2023 13:57:21 -0700 Subject: [PATCH] fix: Add ENV variable to configure GRPC Keep Alive Time (#15656) (#15806) * Add ENV variables to configure GRPC Keep Alive Time Signed-off-by: Bhavika Sharma * Retrigger CI pipeline Signed-off-by: Bhavika Sharma * Resolve conflict with master Signed-off-by: Bhavika Sharma * Update docs/user-guide/environment-variables.md Co-authored-by: Ishita Sequeira <46771830+ishitasequeira@users.noreply.github.com> Signed-off-by: BhavikaSharma --------- Signed-off-by: Bhavika Sharma Signed-off-by: BhavikaSharma Co-authored-by: Ishita Sequeira <46771830+ishitasequeira@users.noreply.github.com> --- cmpserver/server.go | 2 +- common/common.go | 23 ++++++++++-- common/common_test.go | 46 ++++++++++++++++++++++++ docs/user-guide/environment-variables.md | 1 + pkg/apiclient/grpcproxy.go | 2 +- reposerver/server.go | 2 +- server/server.go | 2 +- util/grpc/grpc.go | 2 +- 8 files changed, 72 insertions(+), 8 deletions(-) create mode 100644 common/common_test.go diff --git a/cmpserver/server.go b/cmpserver/server.go index bbb493f6b1d66..1d07e531394d3 100644 --- a/cmpserver/server.go +++ b/cmpserver/server.go @@ -65,7 +65,7 @@ func NewServer(initConstants plugin.CMPServerInitConstants) (*ArgoCDCMPServer, e grpc.MaxSendMsgSize(apiclient.MaxGRPCMessageSize), grpc.KeepaliveEnforcementPolicy( keepalive.EnforcementPolicy{ - MinTime: common.GRPCKeepAliveEnforcementMinimum, + MinTime: common.GetGRPCKeepAliveEnforcementMinimum(), }, ), } diff --git a/common/common.go b/common/common.go index d7c2d24738b58..5faedc2e344c4 100644 --- a/common/common.go +++ b/common/common.go @@ -258,6 +258,8 @@ const ( EnvRedisName = "ARGOCD_REDIS_NAME" // EnvRedisHaProxyName is the name of the Argo CD Redis HA proxy component, as specified by the value under the LabelKeyAppName label key. EnvRedisHaProxyName = "ARGOCD_REDIS_HAPROXY_NAME" + // EnvGRPCKeepAliveMin defines the GRPCKeepAliveEnforcementMinimum, used in the grpc.KeepaliveEnforcementPolicy. Expects a "Duration" format (e.g. 10s). + EnvGRPCKeepAliveMin = "ARGOCD_GRPC_KEEP_ALIVE_MIN" ) // Config Management Plugin related constants @@ -351,11 +353,26 @@ const ( // gRPC settings const ( - GRPCKeepAliveEnforcementMinimum = 10 * time.Second - // GRPCKeepAliveTime is 2x enforcement minimum to ensure network jitter does not introduce ENHANCE_YOUR_CALM errors - GRPCKeepAliveTime = 2 * GRPCKeepAliveEnforcementMinimum + defaultGRPCKeepAliveEnforcementMinimum = 10 * time.Second ) +func GetGRPCKeepAliveEnforcementMinimum() time.Duration { + if GRPCKeepAliveMinStr := os.Getenv(EnvGRPCKeepAliveMin); GRPCKeepAliveMinStr != "" { + GRPCKeepAliveMin, err := time.ParseDuration(GRPCKeepAliveMinStr) + if err != nil { + logrus.Warnf("invalid env var value for %s: cannot parse: %s. Default value %s will be used.", EnvGRPCKeepAliveMin, err, defaultGRPCKeepAliveEnforcementMinimum) + return defaultGRPCKeepAliveEnforcementMinimum + } + return GRPCKeepAliveMin + } + return defaultGRPCKeepAliveEnforcementMinimum +} + +func GetGRPCKeepAliveTime() time.Duration { + // GRPCKeepAliveTime is 2x enforcement minimum to ensure network jitter does not introduce ENHANCE_YOUR_CALM errors + return 2 * GetGRPCKeepAliveEnforcementMinimum() +} + // Security severity logging const ( SecurityField = "security" diff --git a/common/common_test.go b/common/common_test.go new file mode 100644 index 0000000000000..5632c1e7a78cc --- /dev/null +++ b/common/common_test.go @@ -0,0 +1,46 @@ +package common + +import ( + "fmt" + "os" + "testing" + "time" + + "github.com/stretchr/testify/assert" +) + +// Test env var not set for EnvGRPCKeepAliveMin +func Test_GRPCKeepAliveMinNotSet(t *testing.T) { + grpcKeepAliveMin := GetGRPCKeepAliveEnforcementMinimum() + grpcKeepAliveExpectedMin := defaultGRPCKeepAliveEnforcementMinimum + assert.Equal(t, grpcKeepAliveExpectedMin, grpcKeepAliveMin) + + grpcKeepAliveTime := GetGRPCKeepAliveTime() + assert.Equal(t, 2*grpcKeepAliveExpectedMin, grpcKeepAliveTime) +} + +// Test valid env var set for EnvGRPCKeepAliveMin +func Test_GRPCKeepAliveMinIsSet(t *testing.T) { + numSeconds := 15 + os.Setenv(EnvGRPCKeepAliveMin, fmt.Sprintf("%ds", numSeconds)) + + grpcKeepAliveMin := GetGRPCKeepAliveEnforcementMinimum() + grpcKeepAliveExpectedMin := time.Duration(numSeconds) * time.Second + assert.Equal(t, grpcKeepAliveExpectedMin, grpcKeepAliveMin) + + grpcKeepAliveTime := GetGRPCKeepAliveTime() + assert.Equal(t, 2*grpcKeepAliveExpectedMin, grpcKeepAliveTime) +} + +// Test invalid env var set for EnvGRPCKeepAliveMin +func Test_GRPCKeepAliveMinIncorrectlySet(t *testing.T) { + numSeconds := 15 + os.Setenv(EnvGRPCKeepAliveMin, fmt.Sprintf("%d", numSeconds)) + + grpcKeepAliveMin := GetGRPCKeepAliveEnforcementMinimum() + grpcKeepAliveExpectedMin := defaultGRPCKeepAliveEnforcementMinimum + assert.Equal(t, grpcKeepAliveExpectedMin, grpcKeepAliveMin) + + grpcKeepAliveTime := GetGRPCKeepAliveTime() + assert.Equal(t, 2*grpcKeepAliveExpectedMin, grpcKeepAliveTime) +} diff --git a/docs/user-guide/environment-variables.md b/docs/user-guide/environment-variables.md index 238db85b5c718..c990073363f04 100644 --- a/docs/user-guide/environment-variables.md +++ b/docs/user-guide/environment-variables.md @@ -12,3 +12,4 @@ The following environment variables can be used with `argocd` CLI: | `ARGOCD_APPLICATION_CONTROLLER_NAME` | the Argo CD Application Controller name (default "argocd-application-controller") | | `ARGOCD_REDIS_NAME` | the Argo CD Redis name (default "argocd-redis") | | `ARGOCD_REDIS_HAPROXY_NAME` | the Argo CD Redis HA Proxy name (default "argocd-redis-ha-haproxy") | +| `ARGOCD_GRPC_KEEP_ALIVE_MIN` | defines the GRPCKeepAliveEnforcementMinimum, used in the grpc.KeepaliveEnforcementPolicy. Expects a "Duration" format (default `10s`). | diff --git a/pkg/apiclient/grpcproxy.go b/pkg/apiclient/grpcproxy.go index dcbb3b296833e..28af7b62783df 100644 --- a/pkg/apiclient/grpcproxy.go +++ b/pkg/apiclient/grpcproxy.go @@ -116,7 +116,7 @@ func (c *client) startGRPCProxy() (*grpc.Server, net.Listener, error) { grpc.ForceServerCodec(&noopCodec{}), grpc.KeepaliveEnforcementPolicy( keepalive.EnforcementPolicy{ - MinTime: common.GRPCKeepAliveEnforcementMinimum, + MinTime: common.GetGRPCKeepAliveEnforcementMinimum(), }, ), grpc.UnknownServiceHandler(func(srv interface{}, stream grpc.ServerStream) error { diff --git a/reposerver/server.go b/reposerver/server.go index 9576604751dfc..007b7136e41ed 100644 --- a/reposerver/server.go +++ b/reposerver/server.go @@ -90,7 +90,7 @@ func NewServer(metricsServer *metrics.MetricsServer, cache *reposervercache.Cach grpc.MaxSendMsgSize(apiclient.MaxGRPCMessageSize), grpc.KeepaliveEnforcementPolicy( keepalive.EnforcementPolicy{ - MinTime: common.GRPCKeepAliveEnforcementMinimum, + MinTime: common.GetGRPCKeepAliveEnforcementMinimum(), }, ), } diff --git a/server/server.go b/server/server.go index a0fc5327e985c..b4225dfdc51b8 100644 --- a/server/server.go +++ b/server/server.go @@ -731,7 +731,7 @@ func (a *ArgoCDServer) newGRPCServer() (*grpc.Server, application.AppResourceTre grpc.ConnectionTimeout(300 * time.Second), grpc.KeepaliveEnforcementPolicy( keepalive.EnforcementPolicy{ - MinTime: common.GRPCKeepAliveEnforcementMinimum, + MinTime: common.GetGRPCKeepAliveEnforcementMinimum(), }, ), } diff --git a/util/grpc/grpc.go b/util/grpc/grpc.go index 323d78398a8ce..1741727a5d604 100644 --- a/util/grpc/grpc.go +++ b/util/grpc/grpc.go @@ -88,7 +88,7 @@ func BlockingDial(ctx context.Context, network, address string, creds credential grpc.FailOnNonTempDialError(true), grpc.WithContextDialer(dialer), grpc.WithTransportCredentials(insecure.NewCredentials()), // we are handling TLS, so tell grpc not to - grpc.WithKeepaliveParams(keepalive.ClientParameters{Time: common.GRPCKeepAliveTime}), + grpc.WithKeepaliveParams(keepalive.ClientParameters{Time: common.GetGRPCKeepAliveTime()}), ) conn, err := grpc.DialContext(ctx, address, opts...) var res interface{}