-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace custom Redis config struct with go-redis UniversalOptions (adds sentinel & cluster support) #4306
base: main
Are you sure you want to change the base?
Conversation
4431aa2
to
b09dbb1
Compare
Please sign your commits as per https://github.com/distribution/distribution/blob/main/CONTRIBUTING.md#contributing-code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with this change is, it's tying the code to a specific Go module, which I'm not entirely sure is a good idea.
b09dbb1
to
d8a1f01
Compare
@Jamstah done. p.s.: I figured out the test failure: The marshaller is trying to serialize the |
Also, it looks like |
d8a1f01
to
c6615dc
Compare
@milosgajdos, replicating the struct actually fixes the marshalling sooo 🤷♂️ fixed :-D |
c6615dc
to
f74a981
Compare
We're already using the go module to provide the client itself, so why not also use it to provide the config struct? If we ever change go module it would need rewriting anyway. I like the idea of matching our config to be more "standard". |
@Jamstah I agree. In that case the marshalling (or the test) needs some work though. I don't know how to exclude the functions that are referenced in those configs, any ideas? |
*ping after the holidays. I would love if we could figure out how to progress with this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are still missing doc updates here:
distribution/docs/content/about/configuration.md
Lines 953 to 1020 in 0d1792f
## `redis` | |
```yaml | |
redis: | |
addr: localhost:6379 | |
password: asecret | |
db: 0 | |
dialtimeout: 10ms | |
readtimeout: 10ms | |
writetimeout: 10ms | |
pool: | |
maxidle: 16 | |
maxactive: 64 | |
idletimeout: 300s | |
tls: | |
enabled: false | |
``` | |
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. | | |
Also, it looks like redis.tls.enabled had no effect previously? I don't see it used anywhere...
Yes, that was an oversight on my side when switching to the new module. There is an issue someone opened recently: #4284
My other comment is...there is now a boat load of new config items. What are they initialized to when omitted from the config file? Is not having a config value for each of them going to make the user confused? I guess what I'm trying to say is: what's the behaviour like when they're initialized to their default Go vals when omitted.
configuration/configuration.go
Outdated
|
||
// DB specifies the database to connect to on the redis instance. | ||
DB int `yaml:"db,omitempty"` | ||
Protocol int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does Protocol
stand for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Protocol 2 or 3. Use the version to negotiate RESP version with redis-server. Default is 3.
It seems to have been removed in the latest v9.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably would be useful to carry the comments from here https://github.com/redis/go-redis/blob/v9.1.0/options.go#L31-L139 into this struct as well in case questions like these come up again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not the UniversalOptions
struct though :-/
I carried over all the comments from that one.
Again, it's probably a lot smarter to simply use that struct directly. The indirection just adds a lot of confusion. If the go-redis struct is referenced directly you know there isn't any tomfoolery happening.
Here are the structs with all the options specified:
I'm starting to think that using the redis-go UniversalOptions struct directly is the better option here. It cuts down on maintenance and documentation. We can simply link out to e.g. https://pkg.go.dev/github.com/go-redis/redis/v9#UniversalOptions. All that is needed for that is figuring out how to get around the marshalling issue. |
I think that might be the best course of action should we decide to go forth with this. |
f74a981
to
2fa441f
Compare
@andsens can you sign your commit, please |
@milosgajdos ah crap, sry. I reverted to the previous version and forgot to amend. Will do when I get back to my work PC. In the meantime, what do we do about the failed marshalling of the config? |
There is some silly reflection stuff in |
Signed-off-by: Anders Ingemann <aim@orbit.online>
2fa441f
to
51037d3
Compare
*bump |
Yeah, the problem is The configuration marshaler naturally barfs its guts encountering them....this also complicates Registry's env var configuration -- which frankly I wouldn't mind ditching, but many are relying on passing config via env vars. I need to have a think about this... |
@@ -130,20 +131,14 @@ var configStruct = Configuration{ | |||
Enabled: true, | |||
}, | |||
}, | |||
Redis: Redis{ | |||
Addr: "localhost:6379", | |||
Redis: redis.UniversalOptions{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: this isn't Go fmt-ed
I have a potential workaround @andsens 🤔 We could create a wrapper struct called See this little piece of code I've hacked up for a concrete demonstration of this idea: Toggle me!package main
import (
"fmt"
"reflect"
"github.com/redis/go-redis/v9"
"gopkg.in/yaml.v2"
)
type Configuration struct {
// Registry config
Redis Redis `yaml:"redis"`
}
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[field.Name] = fieldValue.Interface()
}
return fields, nil
}
func main() {
config := Configuration{
Redis: Redis{
redis.UniversalOptions{
Addrs: []string{"localhost:6379"},
DB: 0,
Password: "password",
// Set other options as needed
},
},
// Set other configuration fields
}
data, err := yaml.Marshal(config)
if err != nil {
fmt.Println("Error marshaling config:", err)
return
}
fmt.Println(string(data))
} |
this is the diff of what it could look like @andsens ; this patch still doesn't pass tests but it no longer panics, so you'll have to massage the tests if you go with this...might need a bit more refactoring/thinking maybe 😄 Toggle me!diff --git configuration/configuration.go configuration/configuration.go
index 26216078..b38d21b8 100644
--- configuration/configuration.go
+++ configuration/configuration.go
@@ -175,7 +175,7 @@ type Configuration struct {
Notifications Notifications `yaml:"notifications,omitempty"`
// Redis configures the redis pool available to the registry webapp.
- Redis redis.UniversalOptions `yaml:"redis,omitempty"`
+ Redis Redis `yaml:"redis,omitempty"`
Health Health `yaml:"health,omitempty"`
Catalog Catalog `yaml:"catalog,omitempty"`
@@ -652,3 +652,28 @@ 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[field.Name] = fieldValue.Interface()
+ }
+
+ return fields, nil
+}
diff --git configuration/configuration_test.go configuration/configuration_test.go
index 3a1e0d09..5360b129 100644
--- configuration/configuration_test.go
+++ configuration/configuration_test.go
@@ -131,17 +131,19 @@ var configStruct = Configuration{
Enabled: true,
},
},
- Redis: 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,
+ Redis: Redis{
+ 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,
+ },
},
}
@@ -263,7 +265,7 @@ func (suite *ConfigSuite) TestParseSimple() {
func (suite *ConfigSuite) TestParseInmemory() {
suite.expectedConfig.Storage = Storage{"inmemory": Parameters{}}
suite.expectedConfig.Log.Fields = nil
- suite.expectedConfig.Redis = redis.UniversalOptions{}
+ suite.expectedConfig.Redis = Redis{}
config, err := Parse(bytes.NewReader([]byte(inmemoryConfigYamlV0_1)))
suite.Require().NoError(err)
@@ -283,7 +285,7 @@ func (suite *ConfigSuite) TestParseIncomplete() {
suite.expectedConfig.Auth = Auth{"silly": Parameters{"realm": "silly"}}
suite.expectedConfig.Notifications = Notifications{}
suite.expectedConfig.HTTP.Headers = nil
- suite.expectedConfig.Redis = redis.UniversalOptions{}
+ suite.expectedConfig.Redis = Redis{}
// Note: this also tests that REGISTRY_STORAGE and
// REGISTRY_STORAGE_FILESYSTEM_ROOTDIRECTORY can be used together
diff --git registry/handlers/app.go registry/handlers/app.go
index 373d28c1..e108dc2e 100644
--- registry/handlers/app.go
+++ registry/handlers/app.go
@@ -492,7 +492,7 @@ func (app *App) configureRedis(cfg *configuration.Configuration) {
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 {
@@ -518,8 +518,8 @@ 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);
+ }
+ return redis.NewUniversalClient(&cfg)
}
// configureLogHook prepares logging hook parameters. |
Brilliant! Thank you @milosgajdos! I'll see if I can find the time this week to apply it and get the tests to pass :-) |
@andsens how are you getting on with this PR? If you don't have any bandwidth I might look into it this week. |
@milosgajdos could you? |
So I tried taking this over and pushing a patch to your branch but don't have the perms, @andsens I think the issue is your branch is on an ORG repo rather than a PERSONAL repo so I can't push there...because "GH reasons" If you have time you could apply the patch below which works and fixes all the tests. TL;DR: I had to add an Toggle me!diff --git configuration/configuration.go configuration/configuration.go
index 26216078..884552da 100644
--- configuration/configuration.go
+++ configuration/configuration.go
@@ -175,7 +175,7 @@ type Configuration struct {
Notifications Notifications `yaml:"notifications,omitempty"`
// Redis configures the redis pool available to the registry webapp.
- Redis redis.UniversalOptions `yaml:"redis,omitempty"`
+ Redis Redis `yaml:"redis,omitempty"`
Health Health `yaml:"health,omitempty"`
Catalog Catalog `yaml:"catalog,omitempty"`
@@ -652,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 configuration/configuration_test.go configuration/configuration_test.go
index 3a1e0d09..b7018807 100644
--- configuration/configuration_test.go
+++ configuration/configuration_test.go
@@ -131,17 +131,19 @@ var configStruct = Configuration{
Enabled: true,
},
},
- Redis: 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,
+ Redis: Redis{
+ 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,
+ },
},
}
@@ -187,7 +189,7 @@ http:
redis:
addrs: [localhost:6379]
username: alice
- password: 123456
+ password: "123456"
db: 1
maxidleconns: 16
poolsize: 64
@@ -263,7 +265,7 @@ func (suite *ConfigSuite) TestParseSimple() {
func (suite *ConfigSuite) TestParseInmemory() {
suite.expectedConfig.Storage = Storage{"inmemory": Parameters{}}
suite.expectedConfig.Log.Fields = nil
- suite.expectedConfig.Redis = redis.UniversalOptions{}
+ suite.expectedConfig.Redis = Redis{}
config, err := Parse(bytes.NewReader([]byte(inmemoryConfigYamlV0_1)))
suite.Require().NoError(err)
@@ -283,7 +285,7 @@ func (suite *ConfigSuite) TestParseIncomplete() {
suite.expectedConfig.Auth = Auth{"silly": Parameters{"realm": "silly"}}
suite.expectedConfig.Notifications = Notifications{}
suite.expectedConfig.HTTP.Headers = nil
- suite.expectedConfig.Redis = redis.UniversalOptions{}
+ suite.expectedConfig.Redis = Redis{}
// Note: this also tests that REGISTRY_STORAGE and
// REGISTRY_STORAGE_FILESYSTEM_ROOTDIRECTORY can be used together
diff --git registry/handlers/app.go registry/handlers/app.go
index 373d28c1..e108dc2e 100644
--- registry/handlers/app.go
+++ registry/handlers/app.go
@@ -492,7 +492,7 @@ func (app *App) configureRedis(cfg *configuration.Configuration) {
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 {
@@ -518,8 +518,8 @@ 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);
+ }
+ return redis.NewUniversalClient(&cfg)
}
// configureLogHook prepares logging hook parameters.
If you grab that diff save it in a file like
That should get your tests and lining issues sorted 🤞 |
Less code more features! :-D
This PR replaces the custom
Redis
config struct withUniversalOptions
fromgo-redis
, which implicitly adds support for both redis sentinel and redis clustering.I have two failing tests that I can't quite figure out how to fix:
Associated stack trace
As v3 is still in alpha I was hoping that there would still be time for this to make it into the release.