Change default values for two GRPC setting we have to set so the queriers can connect to a frontend or scheduler (#4435)

* change gprc server defaults

Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>

* add docs and changelog

Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>

CHANGELOG.md

@@ -1,5 +1,6 @@
 ## Main
 * [4400](https://github.com/grafana/loki/pull/4400) **trevorwhitney**: Config: automatically apply memberlist config too all rings when provided
+* [4435](https://github.com/grafana/loki/pull/4435) **trevorwhitney**: Change default values for two GRPC settings so querier can connect to frontend/scheduler
 * [4443](https://github.com/grafana/loki/pull/4443) **DylanGuedes**: Loki: Change how push API checks for contentType
 
 # 2.3.0 (2021/08/06)

docs/sources/upgrading/_index.md

@@ -61,6 +61,27 @@ ruler:
         host: consul.namespace.svc.cluster.local:8500
 ```
 
+#### Changed defaults for some GRPC server settings
+* [4435](https://github.com/grafana/loki/pull/4435) **trevorwhitney**: Change default values for two GRPC settings so querier can connect to frontend/scheduler
+
+This changes two default values, `grpc_server_min_time_between_pings` and `grpc_server_ping_without_stream_allowed` used by the GRPC server.
+
+*Previous Values*:
+```
+server:
+  grpc_server_min_time_between_pings: '5m'
+  grpc_server_ping_without_stream_allowed: false
+```
+
+*New Values*:
+```
+server:
+  grpc_server_min_time_between_pings: '10s'
+  grpc_server_ping_without_stream_allowed: true
+```
+
+Please manually provide the values of `5m` and `true` (respectively) in your config if you rely on those values.
+
 -_add changes here which are unreleased_
 
 

pkg/loki/loki.go

@@ -87,7 +87,7 @@ func (c *Config) RegisterFlags(f *flag.FlagSet) {
 		"The alias 'all' can be used in the list to load a number of core modules and will enable single-binary mode. ")
 	f.BoolVar(&c.AuthEnabled, "auth.enabled", true, "Set to false to disable auth.")
 
-	c.Server.RegisterFlags(f)
+	c.registerServerFlagsWithChangedDefaultValues(f)
 	c.Distributor.RegisterFlags(f)
 	c.Querier.RegisterFlags(f)
 	c.IngesterClient.RegisterFlags(f)
@@ -108,6 +108,27 @@ func (c *Config) RegisterFlags(f *flag.FlagSet) {
 	c.QueryScheduler.RegisterFlags(f)
 }
 
+func (c *Config) registerServerFlagsWithChangedDefaultValues(fs *flag.FlagSet) {
+	throwaway := flag.NewFlagSet("throwaway", flag.PanicOnError)
+
+	// Register to throwaway flags first. Default values are remembered during registration and cannot be changed,
+	// but we can take values from throwaway flag set and reregister into supplied flags with new default values.
+	c.Server.RegisterFlags(throwaway)
+
+	throwaway.VisitAll(func(f *flag.Flag) {
+		// Ignore errors when setting new values. We have a test to verify that it works.
+		switch f.Name {
+		case "server.grpc.keepalive.min-time-between-pings":
+			_ = f.Value.Set("10s")
+
+		case "server.grpc.keepalive.ping-without-stream-allowed":
+			_ = f.Value.Set("true")
+		}
+
+		fs.Var(f.Value, f.Name, f.Usage)
+	})
+}
+
 // Clone takes advantage of pass-by-value semantics to return a distinct *Config.
 // This is primarily used to parse a different flag set without mutating the original *Config.
 func (c *Config) Clone() flagext.Registerer {

pkg/loki/loki_test.go

@@ -0,0 +1,58 @@
+package loki
+
+import (
+	"bytes"
+	"flag"
+	"io"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestFlagDefaults(t *testing.T) {
+	c := Config{}
+
+	f := flag.NewFlagSet("test", flag.PanicOnError)
+	c.RegisterFlags(f)
+
+	buf := bytes.Buffer{}
+
+	f.SetOutput(&buf)
+	f.PrintDefaults()
+
+	const delim = '\n'
+
+	minTimeChecked := false
+	pingWithoutStreamChecked := false
+	for {
+		line, err := buf.ReadString(delim)
+		if err == io.EOF {
+			break
+		}
+
+		require.NoError(t, err)
+
+		if strings.Contains(line, "-server.grpc.keepalive.min-time-between-pings") {
+			nextLine, err := buf.ReadString(delim)
+			require.NoError(t, err)
+			assert.Contains(t, nextLine, "(default 10s)")
+			minTimeChecked = true
+		}
+
+		if strings.Contains(line, "-server.grpc.keepalive.ping-without-stream-allowed") {
+			nextLine, err := buf.ReadString(delim)
+			require.NoError(t, err)
+			assert.Contains(t, nextLine, "(default true)")
+			pingWithoutStreamChecked = true
+		}
+	}
+
+	require.True(t, minTimeChecked)
+	require.True(t, pingWithoutStreamChecked)
+
+	require.Equal(t, true, c.Server.GRPCServerPingWithoutStreamAllowed)
+	require.Equal(t, 10*time.Second, c.Server.GRPCServerMinTimeBetweenPings)
+}