remote_cache: Fix redis (#17483)

* wip: fix remote cache for redis
connstr parsing and non-negative expires for #17377
TODO: finish parse, check zero case, find out why negative duration in the first place

* finish parse.
Still TODO, find out negative value, and decide if would be better to make database specific entries in the .ini file

* update ini files

* remove accidental uncomment in defaults.ini

* auth_proxy: expiration non-negative so expiration is not in the past

* fix test, revert neg in redis

* review: use errutil

conf/defaults.ini

@@ -116,9 +116,9 @@ type = database
 
 
 # cache connectionstring options
 # cache connectionstring options
 # database: will use Grafana primary database.
 # database: will use Grafana primary database.
-# redis: config like redis server e.g. `addr=127.0.0.1:6379,pool_size=100,db=grafana`
+# redis: config like redis server e.g. `addr=127.0.0.1:6379,pool_size=100,db=0`. Only addr is required.
 # memcache: 127.0.0.1:11211
 # memcache: 127.0.0.1:11211
-connstr =
+;connstr =
 
 
 #################################### Data proxy ###########################
 #################################### Data proxy ###########################
 [dataproxy]
 [dataproxy]

conf/sample.ini

@@ -112,7 +112,7 @@
 
 
 # cache connectionstring options
 # cache connectionstring options
 # database: will use Grafana primary database.
 # database: will use Grafana primary database.
-# redis: config like redis server e.g. `addr=127.0.0.1:6379,pool_size=100,db=grafana`
+# redis: config like redis server e.g. `addr=127.0.0.1:6379,pool_size=100,db=0`. Only addr is required.
 # memcache: 127.0.0.1:11211
 # memcache: 127.0.0.1:11211
 ;connstr =
 ;connstr =
 
 

pkg/infra/remotecache/redis_storage.go

 package remotecache
 package remotecache
 
 
 import (
 import (
+	"fmt"
+	"strconv"
+	"strings"
 	"time"
 	"time"
 
 
 	"github.com/grafana/grafana/pkg/setting"
 	"github.com/grafana/grafana/pkg/setting"
+	"github.com/grafana/grafana/pkg/util/errutil"
 	redis "gopkg.in/redis.v2"
 	redis "gopkg.in/redis.v2"
 )
 )
 
 
@@ -13,12 +17,47 @@ type redisStorage struct {
 	c *redis.Client
 	c *redis.Client
 }
 }
 
 
-func newRedisStorage(opts *setting.RemoteCacheOptions) *redisStorage {
-	opt := &redis.Options{
-		Network: "tcp",
-		Addr:    opts.ConnStr,
+// parseRedisConnStr parses k=v pairs in csv and builds a redis Options object
+func parseRedisConnStr(connStr string) (*redis.Options, error) {
+	keyValueCSV := strings.Split(connStr, ",")
+	options := &redis.Options{Network: "tcp"}
+	for _, rawKeyValue := range keyValueCSV {
+		keyValueTuple := strings.Split(rawKeyValue, "=")
+		if len(keyValueTuple) != 2 {
+			return nil, fmt.Errorf("incorrect redis connection string format detected for '%v', format is key=value,key=value", rawKeyValue)
+		}
+		connKey := keyValueTuple[0]
+		connVal := keyValueTuple[1]
+		switch connKey {
+		case "addr":
+			options.Addr = connVal
+		case "password":
+			options.Password = connVal
+		case "db":
+			i, err := strconv.ParseInt(connVal, 10, 64)
+			if err != nil {
+				return nil, errutil.Wrap("value for db in redis connection string must be a number", err)
+			}
+			options.DB = i
+		case "pool_size":
+			i, err := strconv.Atoi(connVal)
+			if err != nil {
+				return nil, errutil.Wrap("value for pool_size in redis connection string must be a number", err)
+			}
+			options.PoolSize = i
+		default:
+			return nil, fmt.Errorf("unrecorgnized option '%v' in redis connection string", connVal)
+		}
 	}
 	}
-	return &redisStorage{c: redis.NewClient(opt)}
+	return options, nil
+}
+
+func newRedisStorage(opts *setting.RemoteCacheOptions) (*redisStorage, error) {
+	opt, err := parseRedisConnStr(opts.ConnStr)
+	if err != nil {
+		return nil, err
+	}
+	return &redisStorage{c: redis.NewClient(opt)}, nil
 }
 }
 
 
 // Set sets value to given key in session.
 // Set sets value to given key in session.
@@ -28,7 +67,6 @@ func (s *redisStorage) Set(key string, val interface{}, expires time.Duration) e
 	if err != nil {
 	if err != nil {
 		return err
 		return err
 	}
 	}
-
 	status := s.c.SetEx(key, expires, string(value))
 	status := s.c.SetEx(key, expires, string(value))
 	return status.Err()
 	return status.Err()
 }
 }

pkg/infra/remotecache/redis_storage_integration_test.go

@@ -10,7 +10,7 @@ import (
 
 
 func TestRedisCacheStorage(t *testing.T) {
 func TestRedisCacheStorage(t *testing.T) {
 
 
-	opts := &setting.RemoteCacheOptions{Name: redisCacheType, ConnStr: "localhost:6379"}
+	opts := &setting.RemoteCacheOptions{Name: redisCacheType, ConnStr: "addr=localhost:6379"}
 	client := createTestClient(t, opts, nil)
 	client := createTestClient(t, opts, nil)
 	runTestsForClient(t, client)
 	runTestsForClient(t, client)
 }
 }

pkg/infra/remotecache/remotecache.go

@@ -91,7 +91,7 @@ func (ds *RemoteCache) Run(ctx context.Context) error {
 
 
 func createClient(opts *setting.RemoteCacheOptions, sqlstore *sqlstore.SqlStore) (CacheStorage, error) {
 func createClient(opts *setting.RemoteCacheOptions, sqlstore *sqlstore.SqlStore) (CacheStorage, error) {
 	if opts.Name == redisCacheType {
 	if opts.Name == redisCacheType {
-		return newRedisStorage(opts), nil
+		return newRedisStorage(opts)
 	}
 	}
 
 
 	if opts.Name == memcachedCacheType {
 	if opts.Name == memcachedCacheType {