Please sign your commits as per https://github.com/distribution/distribution/blob/main/CONTRIBUTING.md#contributing-code

@Jamstah done.
@milosgajdos, I understand the issue. I mean, we could just replicate the entire struct I guess? In practice the options struct seems to be quite stable and well thought out. The tests will catch any incompatible changes when bumping and then we can decide from there what to do about it, rather than doing a bunch of work that might not be necessary.

p.s.: I figured out the test failure: The marshaller is trying to serialize the Dialer function I believe, but I'm not familiar enough with the rest of the code to determine how to fix that.

Also, it looks like redis.tls.enabled had no effect previously? I don't see it used anywhere...

@milosgajdos, replicating the struct actually fixes the marshalling sooo 🤷‍♂️ fixed :-D

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.

Here are the structs with all the options specified:

• Simple mode: https://github.com/redis/go-redis/blob/v9.1.0/options.go#L31-L139
• Sentinel/failover mode: https://github.com/redis/go-redis/blob/v9.1.0/sentinel.go#L21-L81
• Cluster mode: https://github.com/redis/go-redis/blob/v9.1.0/cluster.go#L28-L87

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'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.

I think that might be the best course of action should we decide to go forth with this.

@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?
It's not used anywhere except during testing. I guess it's a useful feature for debugging.

There is some silly reflection stuff in configuration package to enable the ENV var shortcuts via cli...we should try fixing it

*bump
Is there any chance someone can help me with this? I don't have the expertise to get those two tests to pass.
We have been using this version in production for 2 months now, and it's been working like a charm. The failover on redis with sentinel happens seamlessly and through it all parts of distribution are now HA.

Yeah, the problem is UniversalOptions contain functions as fields 🫠 which is ....not great in...like...any Go code for that matter.

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...

@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?
I have actually made the time for it now, but after trying to get unmarshalling to work so the tests pass I have to admit that I have way too little experience with Go to understand how that entire show is run.
I can see that we also need to introduce some defaults. Lifting them from options.go in go-redis seems like the most sensible thing to do (a bit annoying they don't have it for the UniversalOptions variant).

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 UnmarshalYAML func that handles unmarshalling because we have a nested imported struct there...so...can't easily add yaml struct tags...anyways, the patch is below

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 foo.patch and then do

git apply foo.patch

That should get your tests and lining issues sorted 🤞

@milosgajdos done! Thank you so much for helping out.
I have also updated the documentation to reflect the changes.
Though, the tls options are still a big questionmark. They have the same issue regarding function fields. At least you might be able to enable it I think and use CAs installed on the host for verifying the chain (not tested).

Though, the tls options are still a big questionmark. They have the same issue regarding function fields.

Arrgh, I see. Hmm, maybe we can "hack" around it and simply introduce an additional TLS substruct and then create the tls.Config and pass it to the options instance...let me think about this...We're getting there! 😄

I've finally added support for custom TLS config in Redis @andsens 🤞

PTAL @Jamstah @squizzi @thaJeztah

Awesome work @milosgajdos! Looking forward to ditching our fork and running the official server again :-D

Some very quick thoughts (overall this looks like a nice PR);

• This PR makes redis-go part of the distribution project's public API; the type is directly embedded in the config type
• Maybe that's not problematic; most common use would be to configure the binary, and YAML doesn't care what type is used
• But a quick search shows that there's definitely consumers of the go types for these; https://grep.app/search?q=%22github.com/distribution/distribution/v3/configuration%22

When setting these options through the Go API, users are currently required to (as we do in our tests) instance a redis.UniversalOptions to set the config. This also means that if distribution would update its dependency to go-redis/v10, it would be a breaking change, and from a go module (SemVer) perspective, would require distribution/distribution to do a major version update (v4).

Thinking out loud here;

We can could consider creating an alias for redis.UniversalOptions;

type RedisOptions = redis.UniversalOptions

Consumers of the config package can use that alias to construct a config (and we can recommend them to do so); IF we would have to update the dependency to go-redis/v10, and if that update would not introduce a breaking change in the UniversalOptions struct, the alias would take care of keeping our module backward compatible; users of the config package would continue to use config.Redis (which now just is an alias to a different (v10) module).

On a slight note; Redis.UniversalOptions is a bit of a mouthful; wondering if we should create a shorter name for it;

type RedisOptions = redis.UniversalOptions

type Redis struct {
	Options RedisOptions `yaml:",inline"`
	TLS                    struct {
		Certificate string   `yaml:"certificate,omitempty"`
		Key         string   `yaml:"key,omitempty"`
		ClientCAs   []string `yaml:"clientcas,omitempty"`
	} `yaml:"tls,omitempty"`
}

Finally; perhaps we can define a TLSOptions type instead of the struct; I think that would make it easier to construct a config from code;

type RedisOptions = redis.UniversalOptions

type TLSOptions struct {
	Certificate string   `yaml:"certificate,omitempty"`
	Key         string   `yaml:"key,omitempty"`
	ClientCAs   []string `yaml:"clientcas,omitempty"`
}

type Redis struct {
	Options RedisOptions `yaml:",inline"`
	TLS     TLSOptions   `yaml:"tls,omitempty"`
}

Thanks for the feedback, @thaJeztah . This (your suggestions) makes a lot of sense indeed if we want to maintain b/w compact, which I agree we should strive for as much as possible.

The only thing I'm wondering about is the TLSOptions type. It'd have to be RedisTLSOptions IF we were to rip it out into a dedicated type. Mostly due to the following reasons:

• consistency sake (there is an anonymous struct pattern established in the configuration package, see the TLS inside the HTTP config:

  distribution/configuration/configuration.go

  Lines 96 to 136 in 5f804a9

  	TLS struct {
  	// Certificate specifies the path to an x509 certificate file to
  	// be used for TLS.
  	Certificate string `yaml:"certificate,omitempty"`
  	// Key specifies the path to the x509 key file, which should
  	// contain the private portion for the file specified in
  	// Certificate.
  	Key string `yaml:"key,omitempty"`
  	// Specifies the CA certs for client authentication
  	// A file may contain multiple CA certificates encoded as PEM
  	ClientCAs []string `yaml:"clientcas,omitempty"`
  	// Specifies the lowest TLS version allowed
  	MinimumTLS string `yaml:"minimumtls,omitempty"`
  	// Specifies a list of cipher suites allowed
  	CipherSuites []string `yaml:"ciphersuites,omitempty"`
  	// LetsEncrypt is used to configuration setting up TLS through
  	// Let's Encrypt instead of manually specifying certificate and
  	// key. If a TLS certificate is specified, the Let's Encrypt
  	// section will not be used.
  	LetsEncrypt struct {
  	// CacheFile specifies cache file to use for lets encrypt
  	// certificates and keys.
  	CacheFile string `yaml:"cachefile,omitempty"`
  	// Email is the email to use during Let's Encrypt registration
  	Email string `yaml:"email,omitempty"`
  	// Hosts specifies the hosts which are allowed to obtain Let's
  	// Encrypt certificates.
  	Hosts []string `yaml:"hosts,omitempty"`
  	// DirectoryURL points to the CA directory endpoint.
  	// If empty, LetsEncrypt is used.
  	DirectoryURL string `yaml:"directoryurl,omitempty"`
  	} `yaml:"letsencrypt,omitempty"`
  	} `yaml:"tls,omitempty"`

  ; we can't easily replace that because it contains other things we dot necessarily need in Redis config per see)
• It could be consumed as configuration.TLSOptions which makes it seem more generic than it is (see the mention of HTTP.TLS earlier); unless, I suppose, we "massage" that TLS config somehow 🤔

But all in all Im fully onboard with the notion of a type alias. I think it makes a lot of sense, indeed. Thanks for bringing this up!

PTAL @thaJeztah

Seems like a sensible change to me, good spot on giving us options for backwards compatibility

🥳 awesome!
Thanks for all the work you put into this @milosgajdos.