From b63cbb33181f9a917c226c59f15e284b96d87bd8 Mon Sep 17 00:00:00 2001 From: Anders Ingemann Date: Tue, 19 Mar 2024 11:02:38 +0100 Subject: [PATCH] Replace custom Redis config struct with go-redis UniversalOptions Huge help from @milosgajdos who figured out how to do the entire marshalling/unmarshalling for the configs Signed-off-by: Anders Ingemann --- cmd/registry/config-cache.yml | 9 +- configuration/configuration.go | 161 ++++++++++++++++++++------ configuration/configuration_test.go | 38 +++--- docs/content/about/configuration.md | 82 ++++--------- registry/handlers/app.go | 31 ++--- registry/storage/cache/redis/redis.go | 4 +- tests/conf-e2e-cloud-storage.yml | 9 +- 7 files changed, 179 insertions(+), 155 deletions(-) diff --git a/cmd/registry/config-cache.yml b/cmd/registry/config-cache.yml index d648303d9..b553ad5bc 100644 --- a/cmd/registry/config-cache.yml +++ b/cmd/registry/config-cache.yml @@ -20,11 +20,10 @@ http: headers: X-Content-Type-Options: [nosniff] redis: - addr: localhost:6379 - pool: - maxidle: 16 - maxactive: 64 - idletimeout: 300s + addrs: [localhost:6379] + maxidleconns: 16 + poolsize: 64 + connmaxidletime: 300s dialtimeout: 10ms readtimeout: 10ms writetimeout: 10ms diff --git a/configuration/configuration.go b/configuration/configuration.go index 427081977..884552dab 100644 --- a/configuration/configuration.go +++ b/configuration/configuration.go @@ -8,6 +8,8 @@ import ( "reflect" "strings" "time" + + "github.com/redis/go-redis/v9" ) // Configuration is a versioned registry configuration, intended to be provided by a yaml file, and @@ -277,44 +279,6 @@ type FileChecker struct { Threshold int `yaml:"threshold,omitempty"` } -// Redis configures the redis pool available to the registry webapp. -type Redis struct { - // Addr specifies the redis instance available to the application. - Addr string `yaml:"addr,omitempty"` - - // Usernames can be used as a finer-grained permission control since the introduction of the redis 6.0. - Username string `yaml:"username,omitempty"` - - // Password string to use when making a connection. - Password string `yaml:"password,omitempty"` - - // DB specifies the database to connect to on the redis instance. - DB int `yaml:"db,omitempty"` - - // TLS configures settings for redis in-transit encryption - TLS struct { - Enabled bool `yaml:"enabled,omitempty"` - } `yaml:"tls,omitempty"` - - DialTimeout time.Duration `yaml:"dialtimeout,omitempty"` // timeout for connect - ReadTimeout time.Duration `yaml:"readtimeout,omitempty"` // timeout for reads of data - WriteTimeout time.Duration `yaml:"writetimeout,omitempty"` // timeout for writes of data - - // Pool configures the behavior of the redis connection pool. - Pool struct { - // MaxIdle sets the maximum number of idle connections. - MaxIdle int `yaml:"maxidle,omitempty"` - - // MaxActive sets the maximum number of connections that should be - // opened before blocking a connection request. - MaxActive int `yaml:"maxactive,omitempty"` - - // IdleTimeout sets the amount time to wait before closing - // inactive connections. - IdleTimeout time.Duration `yaml:"idletimeout,omitempty"` - } `yaml:"pool,omitempty"` -} - // HTTPChecker is a type of entry in the health section for checking HTTP URIs. type HTTPChecker struct { // Timeout is the duration to wait before timing out the HTTP request @@ -688,3 +652,124 @@ func Parse(rd io.Reader) (*Configuration, error) { return config, nil } + +type Redis struct { + redis.UniversalOptions +} + +func (c Redis) MarshalYAML() (interface{}, error) { + fields := make(map[string]interface{}) + + val := reflect.ValueOf(c.UniversalOptions) + typ := val.Type() + + for i := 0; i < val.NumField(); i++ { + field := typ.Field(i) + fieldValue := val.Field(i) + + // ignore imports and funcs + if field.PkgPath != "" || fieldValue.Kind() == reflect.Func { + continue + } + + fields[strings.ToLower(field.Name)] = fieldValue.Interface() + } + + return fields, nil +} + +func (c *Redis) UnmarshalYAML(unmarshal func(interface{}) error) error { + var fields map[string]interface{} + err := unmarshal(&fields) + if err != nil { + return err + } + + val := reflect.ValueOf(&c.UniversalOptions).Elem() + typ := val.Type() + + for i := 0; i < typ.NumField(); i++ { + field := typ.Field(i) + fieldName := strings.ToLower(field.Name) + + if value, ok := fields[fieldName]; ok { + fieldValue := val.Field(i) + if fieldValue.CanSet() { + switch field.Type { + case reflect.TypeOf(time.Duration(0)): + durationStr, ok := value.(string) + if !ok { + return fmt.Errorf("invalid duration value for field: %s", fieldName) + } + duration, err := time.ParseDuration(durationStr) + if err != nil { + return fmt.Errorf("failed to parse duration for field: %s, error: %v", fieldName, err) + } + fieldValue.Set(reflect.ValueOf(duration)) + default: + if err := setFieldValue(fieldValue, value); err != nil { + return fmt.Errorf("failed to set value for field: %s, error: %v", fieldName, err) + } + } + } + } + } + + return nil +} + +func setFieldValue(field reflect.Value, value interface{}) error { + if value == nil { + return nil + } + + switch field.Kind() { + case reflect.String: + stringValue, ok := value.(string) + if !ok { + return fmt.Errorf("failed to convert value to string") + } + field.SetString(stringValue) + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + intValue, ok := value.(int) + if !ok { + return fmt.Errorf("failed to convert value to integer") + } + field.SetInt(int64(intValue)) + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: + uintValue, ok := value.(uint) + if !ok { + return fmt.Errorf("failed to convert value to unsigned integer") + } + field.SetUint(uint64(uintValue)) + case reflect.Float32, reflect.Float64: + floatValue, ok := value.(float64) + if !ok { + return fmt.Errorf("failed to convert value to float") + } + field.SetFloat(floatValue) + case reflect.Bool: + boolValue, ok := value.(bool) + if !ok { + return fmt.Errorf("failed to convert value to boolean") + } + field.SetBool(boolValue) + case reflect.Slice: + slice := reflect.MakeSlice(field.Type(), 0, 0) + valueSlice, ok := value.([]interface{}) + if !ok { + return fmt.Errorf("failed to convert value to slice") + } + for _, item := range valueSlice { + sliceValue := reflect.New(field.Type().Elem()).Elem() + if err := setFieldValue(sliceValue, item); err != nil { + return err + } + slice = reflect.Append(slice, sliceValue) + } + field.Set(slice) + default: + return fmt.Errorf("unsupported field type: %v", field.Type()) + } + return nil +} diff --git a/configuration/configuration_test.go b/configuration/configuration_test.go index 2139f8f1a..b7018807c 100644 --- a/configuration/configuration_test.go +++ b/configuration/configuration_test.go @@ -8,6 +8,7 @@ import ( "testing" "time" + "github.com/redis/go-redis/v9" "github.com/stretchr/testify/suite" "gopkg.in/yaml.v2" ) @@ -131,22 +132,18 @@ var configStruct = Configuration{ }, }, Redis: Redis{ - Addr: "localhost:6379", - Username: "alice", - Password: "123456", - DB: 1, - Pool: struct { - MaxIdle int `yaml:"maxidle,omitempty"` - MaxActive int `yaml:"maxactive,omitempty"` - IdleTimeout time.Duration `yaml:"idletimeout,omitempty"` - }{ - MaxIdle: 16, - MaxActive: 64, - IdleTimeout: time.Second * 300, + redis.UniversalOptions{ + Addrs: []string{"localhost:6379"}, + Username: "alice", + Password: "123456", + DB: 1, + MaxIdleConns: 16, + PoolSize: 64, + ConnMaxIdleTime: time.Second * 300, + DialTimeout: time.Millisecond * 10, + ReadTimeout: time.Millisecond * 10, + WriteTimeout: time.Millisecond * 10, }, - DialTimeout: time.Millisecond * 10, - ReadTimeout: time.Millisecond * 10, - WriteTimeout: time.Millisecond * 10, }, } @@ -190,14 +187,13 @@ http: headers: X-Content-Type-Options: [nosniff] redis: - addr: localhost:6379 + addrs: [localhost:6379] username: alice - password: 123456 + password: "123456" db: 1 - pool: - maxidle: 16 - maxactive: 64 - idletimeout: 300s + maxidleconns: 16 + poolsize: 64 + connmaxidletime: 300s dialtimeout: 10ms readtimeout: 10ms writetimeout: 10ms diff --git a/docs/content/about/configuration.md b/docs/content/about/configuration.md index 482a40ca7..5ed5e22cd 100644 --- a/docs/content/about/configuration.md +++ b/docs/content/about/configuration.md @@ -241,16 +241,15 @@ notifications: actions: - pull redis: - addr: localhost:6379 + addrs: [localhost:6379] password: asecret db: 0 dialtimeout: 10ms readtimeout: 10ms writetimeout: 10ms - pool: - maxidle: 16 - maxactive: 64 - idletimeout: 300s + maxidleconns: 16 + poolsize: 64 + connmaxidletime: 300s tls: enabled: false health: @@ -952,72 +951,31 @@ The `events` structure configures the information provided in event notification ## `redis` +Declare parameters for constructing the `redis` connections. Registry instances +may use the Redis instance for several applications. Currently, it caches +information about immutable blobs. Most of the `redis` options control +how the registry connects to the `redis` instance. + +You should configure Redis with the **allkeys-lru** eviction policy, because the +registry does not set an expiration value on keys. + +Under the hood distribution uses [`go-redis`](https://redis.uptrace.dev/) for +redis connectivity and its [`UniversalOptions`](https://pkg.go.dev/github.com/redis/go-redis/v9#UniversalOptions) +struct. + ```yaml redis: - addr: localhost:6379 + addrs: [localhost:6379] password: asecret db: 0 dialtimeout: 10ms readtimeout: 10ms writetimeout: 10ms - pool: - maxidle: 16 - maxactive: 64 - idletimeout: 300s - tls: - enabled: false + maxidleconns: 16 + poolsize: 64 + connmaxidletime: 300s ``` -Declare parameters for constructing the `redis` connections. Registry instances -may use the Redis instance for several applications. Currently, it caches -information about immutable blobs. Most of the `redis` options control -how the registry connects to the `redis` instance. You can control the pool's -behavior with the [pool](#pool) subsection. Additionally, you can control -TLS connection settings with the [tls](#tls) subsection (in-transit encryption). - -You should configure Redis with the **allkeys-lru** eviction policy, because the -registry does not set an expiration value on keys. - -| Parameter | Required | Description | -|-----------|----------|-------------------------------------------------------| -| `addr` | yes | The address (host and port) of the Redis instance. | -| `password`| no | A password used to authenticate to the Redis instance.| -| `db` | no | The name of the database to use for each connection. | -| `dialtimeout` | no | The timeout for connecting to the Redis instance. | -| `readtimeout` | no | The timeout for reading from the Redis instance. | -| `writetimeout` | no | The timeout for writing to the Redis instance. | - -### `pool` - -```yaml -pool: - maxidle: 16 - maxactive: 64 - idletimeout: 300s -``` - -Use these settings to configure the behavior of the Redis connection pool. - -| Parameter | Required | Description | -|-----------|----------|-------------------------------------------------------| -| `maxidle` | no | The maximum number of idle connections in the pool. | -| `maxactive`| no | The maximum number of connections which can be open before blocking a connection request. | -| `idletimeout`| no | How long to wait before closing inactive connections. | - -### `tls` - -```yaml -tls: - enabled: false -``` - -Use these settings to configure Redis TLS. - -| Parameter | Required | Description | -|-----------|----------|-------------------------------------- | -| `enabled` | no | Whether or not to use TLS in-transit. | - - ## `health` ```yaml diff --git a/registry/handlers/app.go b/registry/handlers/app.go index 2983176b8..e108dc2ee 100644 --- a/registry/handlers/app.go +++ b/registry/handlers/app.go @@ -77,7 +77,7 @@ type App struct { source notifications.SourceRecord } - redis *redis.Client + redis redis.UniversalClient // isCache is true if this registry is configured as a pull through cache isCache bool @@ -487,12 +487,12 @@ func (app *App) configureEvents(configuration *configuration.Configuration) { } func (app *App) configureRedis(cfg *configuration.Configuration) { - if cfg.Redis.Addr == "" { + if len(cfg.Redis.Addrs) == 0 { dcontext.GetLogger(app).Infof("redis not configured") return } - app.redis = app.createPool(cfg.Redis) + app.redis = app.createPool(cfg.Redis.UniversalOptions) // Enable metrics instrumentation. if err := redisotel.InstrumentMetrics(app.redis); err != nil { @@ -514,25 +514,12 @@ func (app *App) configureRedis(cfg *configuration.Configuration) { })) } -func (app *App) createPool(cfg configuration.Redis) *redis.Client { - return redis.NewClient(&redis.Options{ - Addr: cfg.Addr, - OnConnect: func(ctx context.Context, cn *redis.Conn) error { - res := cn.Ping(ctx) - return res.Err() - }, - Username: cfg.Username, - Password: cfg.Password, - DB: cfg.DB, - MaxRetries: 3, - DialTimeout: cfg.DialTimeout, - ReadTimeout: cfg.ReadTimeout, - WriteTimeout: cfg.WriteTimeout, - PoolFIFO: false, - MaxIdleConns: cfg.Pool.MaxIdle, - PoolSize: cfg.Pool.MaxActive, - ConnMaxIdleTime: cfg.Pool.IdleTimeout, - }) +func (app *App) createPool(cfg redis.UniversalOptions) redis.UniversalClient { + cfg.OnConnect = func(ctx context.Context, cn *redis.Conn) error { + res := cn.Ping(ctx) + return res.Err() + } + return redis.NewUniversalClient(&cfg) } // configureLogHook prepares logging hook parameters. diff --git a/registry/storage/cache/redis/redis.go b/registry/storage/cache/redis/redis.go index d2596b7bc..baae8e1be 100644 --- a/registry/storage/cache/redis/redis.go +++ b/registry/storage/cache/redis/redis.go @@ -25,7 +25,7 @@ import ( // Note that there is no implied relationship between these two caches. The // layer may exist in one, both or none and the code must be written this way. type redisBlobDescriptorService struct { - pool *redis.Client + pool redis.UniversalClient // TODO(stevvooe): We use a pool because we don't have great control over // the cache lifecycle to manage connections. A new connection if fetched @@ -37,7 +37,7 @@ var _ distribution.BlobDescriptorService = &redisBlobDescriptorService{} // NewRedisBlobDescriptorCacheProvider returns a new redis-based // BlobDescriptorCacheProvider using the provided redis connection pool. -func NewRedisBlobDescriptorCacheProvider(pool *redis.Client) cache.BlobDescriptorCacheProvider { +func NewRedisBlobDescriptorCacheProvider(pool redis.UniversalClient) cache.BlobDescriptorCacheProvider { return metrics.NewPrometheusCacheProvider( &redisBlobDescriptorService{ pool: pool, diff --git a/tests/conf-e2e-cloud-storage.yml b/tests/conf-e2e-cloud-storage.yml index 63a8778c7..89e496c47 100644 --- a/tests/conf-e2e-cloud-storage.yml +++ b/tests/conf-e2e-cloud-storage.yml @@ -17,15 +17,14 @@ log: formatter: text level: debug redis: - addr: redis:6379 + addrs: [redis:6379] db: 0 dialtimeout: 5s readtimeout: 10ms writetimeout: 10ms - pool: - idletimeout: 60s - maxactive: 64 - maxidle: 16 + maxidleconns: 16 + poolsize: 64 + connmaxidletime: 300s storage: redirect: disable: true