From b50e4daf01ca854d44050bd648c69628a64d3a4e Mon Sep 17 00:00:00 2001 From: Yingrong Zhao <22300958+VinozzZ@users.noreply.github.com> Date: Tue, 20 Aug 2024 12:51:39 -0400 Subject: [PATCH 1/6] add ClusterHosts config option in RedisPeerManagement --- config/config.go | 4 ++++ config/config_test.go | 16 ++++++++++++++++ config/file_config.go | 8 ++++++++ config/metadata/configMeta.yaml | 13 +++++++++++++ config/mock.go | 7 +++++++ pubsub/pubsub_goredis.go | 12 +++++++----- 6 files changed, 55 insertions(+), 5 deletions(-) diff --git a/config/config.go b/config/config.go index b567716aff..416f620694 100644 --- a/config/config.go +++ b/config/config.go @@ -66,6 +66,10 @@ type Config interface { // management. GetRedisHost() string + // GetRedisClusterHosts returns the list of addresses of a Redis cluster, used for + // managing peer cluster membership. + GetRedisClusterHosts() []string + // GetRedisUsername returns the username of a Redis instance to use for peer // management. GetRedisUsername() string diff --git a/config/config_test.go b/config/config_test.go index 455af2e344..929f3e10ef 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -12,6 +12,7 @@ import ( "github.com/honeycombio/refinery/internal/configwatcher" "github.com/honeycombio/refinery/pubsub" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "gopkg.in/yaml.v3" ) @@ -454,6 +455,21 @@ func TestDryRun(t *testing.T) { } } +func TestRedisClusterHosts(t *testing.T) { + clusterHosts := []string{"localhost:7001", "localhost:7002"} + cm := makeYAML("General.ConfigurationVersion", 2, "RedisPeerManagement.ClusterHosts", clusterHosts) + rm := makeYAML("ConfigVersion", 2) + config, rules := createTempConfigs(t, cm, rm) + defer os.Remove(rules) + defer os.Remove(config) + c, err := getConfig([]string{"--no-validate", "--config", config, "--rules_config", rules}) + assert.NoError(t, err) + + d := c.GetRedisClusterHosts() + require.NotNil(t, d) + require.EqualValues(t, clusterHosts, d) +} + func TestMaxAlloc(t *testing.T) { cm := makeYAML("General.ConfigurationVersion", 2, "Collection.CacheCapacity", 1000, "Collection.MaxAlloc", 17179869184) rm := makeYAML("ConfigVersion", 2) diff --git a/config/file_config.go b/config/file_config.go index 0882ee13f7..2307d2aefb 100644 --- a/config/file_config.go +++ b/config/file_config.go @@ -285,6 +285,7 @@ type PeerManagementConfig struct { type RedisPeerManagementConfig struct { Host string `yaml:"Host" cmdenv:"RedisHost"` + ClusterHosts []string `yaml:ClusterHosts` Username string `yaml:"Username" cmdenv:"RedisUsername"` Password string `yaml:"Password" cmdenv:"RedisPassword"` AuthCode string `yaml:"AuthCode" cmdenv:"RedisAuthCode"` @@ -653,6 +654,13 @@ func (f *fileConfig) GetRedisHost() string { return f.mainConfig.RedisPeerManagement.Host } +func (f *fileConfig) GetRedisClusterHosts() []string { + f.mux.RLock() + defer f.mux.RUnlock() + + return f.mainConfig.RedisPeerManagement.ClusterHosts +} + func (f *fileConfig) GetRedisUsername() string { f.mux.RLock() defer f.mux.RUnlock() diff --git a/config/metadata/configMeta.yaml b/config/metadata/configMeta.yaml index dfcb19acf0..73a2f61c02 100644 --- a/config/metadata/configMeta.yaml +++ b/config/metadata/configMeta.yaml @@ -987,6 +987,19 @@ groups: description: > Must be in the form `host:port`. + - name: ClusterHosts + type: stringarray + valuetype: stringarray + example: "- localhost:6379" + validations: + - type: elementType + arg: hostport + reload: false + summary: is a list of host and port pairs for the instances in a Redis Cluster, used for managing peer cluster membership. + description: > + This configuration enables Refinery to connect to a Redis cluster. Each entry in the list should follow the format `host:port`. + If `ClusterHosts` is specified, the `Host` setting will be ignored. + - name: Username v1group: PeerManagement v1name: RedisUsername diff --git a/config/mock.go b/config/mock.go index b8b6b81919..5e6349f0fe 100644 --- a/config/mock.go +++ b/config/mock.go @@ -27,6 +27,7 @@ type MockConfig struct { GetLoggerLevelVal Level GetPeersVal []string GetRedisHostVal string + GetRedisClusterHostsVal []string GetRedisUsernameVal string GetRedisPasswordVal string GetRedisAuthCodeVal string @@ -216,6 +217,12 @@ func (m *MockConfig) GetRedisHost() string { return m.GetRedisHostVal } +func (m *MockConfig) GetRedisClusterHosts() []string { + m.Mux.RLock() + defer m.Mux.RUnlock() + + return m.GetRedisClusterHostsVal +} func (m *MockConfig) GetRedisUsername() string { m.Mux.RLock() defer m.Mux.RUnlock() diff --git a/pubsub/pubsub_goredis.go b/pubsub/pubsub_goredis.go index ba96cfdb20..e948af3134 100644 --- a/pubsub/pubsub_goredis.go +++ b/pubsub/pubsub_goredis.go @@ -3,7 +3,6 @@ package pubsub import ( "context" "crypto/tls" - "strings" "sync" "go.opentelemetry.io/otel/trace" @@ -58,14 +57,17 @@ func (ps *GoRedisPubSub) Start() error { if ps.Config != nil { host := ps.Config.GetRedisHost() + hosts := []string{host} + clusterHosts := ps.Config.GetRedisClusterHosts() + // if we have a cluster host, use that instead of the regular host + if len(clusterHosts) > 0 { + hosts = clusterHosts + } + username := ps.Config.GetRedisUsername() pw := ps.Config.GetRedisPassword() authcode = ps.Config.GetRedisAuthCode() - // we may have multiple hosts, separated by commas, so split them up and - // use them as the addrs for the client (if there are multiples, it will - // create a cluster client) - hosts := strings.Split(host, ",") options.Addrs = hosts options.Username = username options.Password = pw From 020a45c5c8e172c7f916b749b2c7bafb5de96184 Mon Sep 17 00:00:00 2001 From: Yingrong Zhao <22300958+VinozzZ@users.noreply.github.com> Date: Wed, 21 Aug 2024 10:50:27 -0400 Subject: [PATCH 2/6] fix yaml tag --- config/config_test.go | 8 +++++++- config/file_config.go | 2 +- config/metadata/configMeta.yaml | 2 ++ 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/config/config_test.go b/config/config_test.go index 929f3e10ef..67ac761570 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -457,7 +457,13 @@ func TestDryRun(t *testing.T) { func TestRedisClusterHosts(t *testing.T) { clusterHosts := []string{"localhost:7001", "localhost:7002"} - cm := makeYAML("General.ConfigurationVersion", 2, "RedisPeerManagement.ClusterHosts", clusterHosts) + cm := makeYAML( + "General.ConfigurationVersion", 2, + "PeerManagement.Type", "redis", + "RedisPeerManagement.ClusterHosts", clusterHosts, + "RedisPeerManagement.Prefix", "test", + "RedisPeerManagement.Database", 9, + ) rm := makeYAML("ConfigVersion", 2) config, rules := createTempConfigs(t, cm, rm) defer os.Remove(rules) diff --git a/config/file_config.go b/config/file_config.go index 2307d2aefb..abe252d7ae 100644 --- a/config/file_config.go +++ b/config/file_config.go @@ -285,7 +285,7 @@ type PeerManagementConfig struct { type RedisPeerManagementConfig struct { Host string `yaml:"Host" cmdenv:"RedisHost"` - ClusterHosts []string `yaml:ClusterHosts` + ClusterHosts []string `yaml:"ClusterHosts"` Username string `yaml:"Username" cmdenv:"RedisUsername"` Password string `yaml:"Password" cmdenv:"RedisPassword"` AuthCode string `yaml:"AuthCode" cmdenv:"RedisAuthCode"` diff --git a/config/metadata/configMeta.yaml b/config/metadata/configMeta.yaml index 73a2f61c02..a8e1a6e0f8 100644 --- a/config/metadata/configMeta.yaml +++ b/config/metadata/configMeta.yaml @@ -991,6 +991,7 @@ groups: type: stringarray valuetype: stringarray example: "- localhost:6379" + firstversion: v2.8 validations: - type: elementType arg: hostport @@ -1266,6 +1267,7 @@ groups: - name: ShutdownDelay type: duration valuetype: nondefault + firstversion: v2.8 default: 15s reload: true summary: controls the maximum time Refinery can use while draining traces at shutdown. From c6f170a9c9f24f8e78ce57793ea94f6c0b56d984 Mon Sep 17 00:00:00 2001 From: Yingrong Zhao <22300958+VinozzZ@users.noreply.github.com> Date: Wed, 21 Aug 2024 11:42:37 -0400 Subject: [PATCH 3/6] use ClusterClient when ClusterHosts is set --- config/metadata/configMeta.yaml | 3 ++- pubsub/pubsub_goredis.go | 14 +++++++++++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/config/metadata/configMeta.yaml b/config/metadata/configMeta.yaml index a8e1a6e0f8..0ab2c4d078 100644 --- a/config/metadata/configMeta.yaml +++ b/config/metadata/configMeta.yaml @@ -998,7 +998,8 @@ groups: reload: false summary: is a list of host and port pairs for the instances in a Redis Cluster, used for managing peer cluster membership. description: > - This configuration enables Refinery to connect to a Redis cluster. Each entry in the list should follow the format `host:port`. + This configuration enables Refinery to connect to a Redis deployment setup in Cluster Mode. + Each entry in the list should follow the format `host:port`. If `ClusterHosts` is specified, the `Host` setting will be ignored. - name: Username diff --git a/pubsub/pubsub_goredis.go b/pubsub/pubsub_goredis.go index e948af3134..c1cefec864 100644 --- a/pubsub/pubsub_goredis.go +++ b/pubsub/pubsub_goredis.go @@ -55,6 +55,7 @@ func (ps *GoRedisPubSub) Start() error { ps.Metrics.Register("redis_pubsub_published", "counter") ps.Metrics.Register("redis_pubsub_received", "counter") + var clusterModeEnabled bool if ps.Config != nil { host := ps.Config.GetRedisHost() hosts := []string{host} @@ -62,6 +63,7 @@ func (ps *GoRedisPubSub) Start() error { // if we have a cluster host, use that instead of the regular host if len(clusterHosts) > 0 { hosts = clusterHosts + clusterModeEnabled = true } username := ps.Config.GetRedisUsername() @@ -81,7 +83,17 @@ func (ps *GoRedisPubSub) Start() error { } } - client := redis.NewUniversalClient(options) + var client redis.UniversalClient + if clusterModeEnabled { + client = redis.NewClusterClient(&redis.ClusterOptions{ + Addrs: options.Addrs, + Username: options.Username, + Password: options.Password, + TLSConfig: options.TLSConfig, + }) + } else { + client = redis.NewUniversalClient(options) + } // if an authcode was provided, use it to authenticate the connection if authcode != "" { From d7c355e83aff1d30f9ae86ef674ab8937d07ac98 Mon Sep 17 00:00:00 2001 From: Yingrong Zhao <22300958+VinozzZ@users.noreply.github.com> Date: Wed, 21 Aug 2024 12:04:20 -0400 Subject: [PATCH 4/6] use a single getter for RedisPeerManagementConfig --- config/config.go | 34 +------------------- config/config_test.go | 14 ++++---- config/file_config.go | 7 ++++ config/mock.go | 69 ++-------------------------------------- pubsub/pubsub_goredis.go | 44 +++++++++++-------------- 5 files changed, 37 insertions(+), 131 deletions(-) diff --git a/config/config.go b/config/config.go index 416f620694..1fa599829a 100644 --- a/config/config.go +++ b/config/config.go @@ -62,39 +62,7 @@ type Config interface { GetPeerManagementType() string - // GetRedisHost returns the address of a Redis instance to use for peer - // management. - GetRedisHost() string - - // GetRedisClusterHosts returns the list of addresses of a Redis cluster, used for - // managing peer cluster membership. - GetRedisClusterHosts() []string - - // GetRedisUsername returns the username of a Redis instance to use for peer - // management. - GetRedisUsername() string - - // GetRedisPassword returns the password of a Redis instance to use for peer - // management. - GetRedisPassword() string - - // GetRedisAuthCode returns the AUTH string to use for connecting to a Redis - // instance to use for peer management - GetRedisAuthCode() string - - // GetRedisPrefix returns the prefix string used in the keys for peer - // management. - GetRedisPrefix() string - - // GetRedisDatabase returns the ID of the Redis database to use for peer management. - GetRedisDatabase() int - - // GetUseTLS returns true when TLS must be enabled to dial the Redis instance to - // use for peer management. - GetUseTLS() bool - - // UseTLSInsecure returns true when certificate checks are disabled - GetUseTLSInsecure() bool + GetRedisPeerManagement() RedisPeerManagementConfig // GetHoneycombAPI returns the base URL (protocol, hostname, and port) of // the upstream Honeycomb API server diff --git a/config/config_test.go b/config/config_test.go index 67ac761570..248f152e46 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -91,7 +91,7 @@ func TestRedisHostEnvVar(t *testing.T) { c, err := getConfig([]string{"--no-validate", "--config", "../config.yaml", "--rules_config", "../rules.yaml"}) assert.NoError(t, err) - if d := c.GetRedisHost(); d != host { + if d := c.GetRedisPeerManagement().Host; d != host { t.Error("received", d, "expected", host) } } @@ -104,7 +104,7 @@ func TestRedisUsernameEnvVar(t *testing.T) { c, err := getConfig([]string{"--no-validate", "--config", "../config.yaml", "--rules_config", "../rules.yaml"}) assert.NoError(t, err) - if d := c.GetRedisUsername(); d != username { + if d := c.GetRedisPeerManagement().Username; d != username { t.Error("received", d, "expected", username) } } @@ -117,7 +117,7 @@ func TestRedisPasswordEnvVar(t *testing.T) { c, err := getConfig([]string{"--no-validate", "--config", "../config.yaml", "--rules_config", "../rules.yaml"}) assert.NoError(t, err) - if d := c.GetRedisPassword(); d != password { + if d := c.GetRedisPeerManagement().Password; d != password { t.Error("received", d, "expected", password) } } @@ -130,7 +130,7 @@ func TestRedisAuthCodeEnvVar(t *testing.T) { c, err := getConfig([]string{"--no-validate", "--config", "../config.yaml", "--rules_config", "../rules.yaml"}) assert.NoError(t, err) - if d := c.GetRedisAuthCode(); d != authCode { + if d := c.GetRedisPeerManagement().AuthCode; d != authCode { t.Error("received", d, "expected", authCode) } } @@ -404,11 +404,11 @@ func TestPeerManagementType(t *testing.T) { t.Error("received", d, "expected", "redis") } - if s := c.GetRedisPrefix(); s != "testPrefix" { + if s := c.GetRedisPeerManagement().Prefix; s != "testPrefix" { t.Error("received", s, "expected", "testPrefix") } - if db := c.GetRedisDatabase(); db != 9 { + if db := c.GetRedisPeerManagement().Database; db != 9 { t.Error("received", db, "expected", 9) } } @@ -471,7 +471,7 @@ func TestRedisClusterHosts(t *testing.T) { c, err := getConfig([]string{"--no-validate", "--config", config, "--rules_config", rules}) assert.NoError(t, err) - d := c.GetRedisClusterHosts() + d := c.GetRedisPeerManagement().ClusterHosts require.NotNil(t, d) require.EqualValues(t, clusterHosts, d) } diff --git a/config/file_config.go b/config/file_config.go index abe252d7ae..c4463f574a 100644 --- a/config/file_config.go +++ b/config/file_config.go @@ -647,6 +647,13 @@ func (f *fileConfig) GetPeers() []string { return f.mainConfig.PeerManagement.Peers } +func (f *fileConfig) GetRedisPeerManagement() RedisPeerManagementConfig { + f.mux.RLock() + defer f.mux.RUnlock() + + return f.mainConfig.RedisPeerManagement +} + func (f *fileConfig) GetRedisHost() string { f.mux.RLock() defer f.mux.RUnlock() diff --git a/config/mock.go b/config/mock.go index 5e6349f0fe..34497a3a10 100644 --- a/config/mock.go +++ b/config/mock.go @@ -26,15 +26,7 @@ type MockConfig struct { GetStdoutLoggerConfigVal StdoutLoggerConfig GetLoggerLevelVal Level GetPeersVal []string - GetRedisHostVal string - GetRedisClusterHostsVal []string - GetRedisUsernameVal string - GetRedisPasswordVal string - GetRedisAuthCodeVal string - GetRedisDatabaseVal int - GetRedisPrefixVal string - GetUseTLSVal bool - GetUseTLSInsecureVal bool + GetRedisPeerManagementVal RedisPeerManagementConfig GetSamplerTypeName string GetSamplerTypeVal interface{} GetMetricsTypeVal string @@ -210,66 +202,11 @@ func (m *MockConfig) GetPeers() []string { return m.GetPeersVal } -func (m *MockConfig) GetRedisHost() string { +func (m *MockConfig) GetRedisPeerManagement() RedisPeerManagementConfig { m.Mux.RLock() defer m.Mux.RUnlock() - return m.GetRedisHostVal -} - -func (m *MockConfig) GetRedisClusterHosts() []string { - m.Mux.RLock() - defer m.Mux.RUnlock() - - return m.GetRedisClusterHostsVal -} -func (m *MockConfig) GetRedisUsername() string { - m.Mux.RLock() - defer m.Mux.RUnlock() - - return m.GetRedisUsernameVal -} - -func (m *MockConfig) GetRedisPassword() string { - m.Mux.RLock() - defer m.Mux.RUnlock() - - return m.GetRedisPasswordVal -} - -func (m *MockConfig) GetRedisAuthCode() string { - m.Mux.RLock() - defer m.Mux.RUnlock() - - return m.GetRedisAuthCodeVal -} - -func (m *MockConfig) GetRedisPrefix() string { - m.Mux.RLock() - defer m.Mux.RUnlock() - - return m.GetRedisPrefixVal -} - -func (m *MockConfig) GetRedisDatabase() int { - m.Mux.RLock() - defer m.Mux.RUnlock() - - return m.GetRedisDatabaseVal -} - -func (m *MockConfig) GetUseTLS() bool { - m.Mux.RLock() - defer m.Mux.RUnlock() - - return m.GetUseTLSVal -} - -func (m *MockConfig) GetUseTLSInsecure() bool { - m.Mux.RLock() - defer m.Mux.RUnlock() - - return m.GetUseTLSInsecureVal + return m.GetRedisPeerManagementVal } func (m *MockConfig) GetGeneralConfig() GeneralConfig { diff --git a/pubsub/pubsub_goredis.go b/pubsub/pubsub_goredis.go index c1cefec864..e27603eaee 100644 --- a/pubsub/pubsub_goredis.go +++ b/pubsub/pubsub_goredis.go @@ -49,48 +49,39 @@ type GoRedisSubscription struct { var _ Subscription = (*GoRedisSubscription)(nil) func (ps *GoRedisPubSub) Start() error { - options := &redis.UniversalOptions{} - authcode := "" + options := new(redis.UniversalOptions) + var ( + authcode string + clusterModeEnabled bool + ) - ps.Metrics.Register("redis_pubsub_published", "counter") - ps.Metrics.Register("redis_pubsub_received", "counter") - - var clusterModeEnabled bool if ps.Config != nil { - host := ps.Config.GetRedisHost() - hosts := []string{host} - clusterHosts := ps.Config.GetRedisClusterHosts() + redisCfg := ps.Config.GetRedisPeerManagement() + hosts := []string{redisCfg.Host} // if we have a cluster host, use that instead of the regular host - if len(clusterHosts) > 0 { - hosts = clusterHosts + if len(redisCfg.ClusterHosts) > 0 { + hosts = redisCfg.ClusterHosts clusterModeEnabled = true } - username := ps.Config.GetRedisUsername() - pw := ps.Config.GetRedisPassword() - authcode = ps.Config.GetRedisAuthCode() + authcode = redisCfg.AuthCode options.Addrs = hosts - options.Username = username - options.Password = pw - options.DB = ps.Config.GetRedisDatabase() + options.Username = redisCfg.Username + options.Password = redisCfg.Password + options.DB = redisCfg.Database - if ps.Config.GetUseTLS() { + if redisCfg.UseTLS { options.TLSConfig = &tls.Config{ MinVersion: tls.VersionTLS12, - InsecureSkipVerify: ps.Config.GetUseTLSInsecure(), + InsecureSkipVerify: redisCfg.UseTLSInsecure, } } } var client redis.UniversalClient if clusterModeEnabled { - client = redis.NewClusterClient(&redis.ClusterOptions{ - Addrs: options.Addrs, - Username: options.Username, - Password: options.Password, - TLSConfig: options.TLSConfig, - }) + client = redis.NewClusterClient(options.Cluster()) } else { client = redis.NewUniversalClient(options) } @@ -104,6 +95,9 @@ func (ps *GoRedisPubSub) Start() error { } } + ps.Metrics.Register("redis_pubsub_published", "counter") + ps.Metrics.Register("redis_pubsub_received", "counter") + ps.client = client ps.subs = make([]*GoRedisSubscription, 0) return nil From 248cbe9558883b89d1da426dc442c92b6ec316b2 Mon Sep 17 00:00:00 2001 From: Yingrong Zhao <22300958+VinozzZ@users.noreply.github.com> Date: Wed, 21 Aug 2024 16:37:53 -0400 Subject: [PATCH 5/6] add environment variable for redis cluster hosts --- config/cmdenv.go | 61 ++++++++++++++++++++++++++++++++++++------- config/cmdenv_test.go | 45 ++++++++++++++++++++----------- config/file_config.go | 2 +- 3 files changed, 83 insertions(+), 25 deletions(-) diff --git a/config/cmdenv.go b/config/cmdenv.go index e39a3ed8c5..f60e7d3754 100644 --- a/config/cmdenv.go +++ b/config/cmdenv.go @@ -32,6 +32,7 @@ type CmdEnv struct { PeerListenAddr string `long:"peer-listen-address" env:"REFINERY_PEER_LISTEN_ADDRESS" description:"Peer listen address for communication between Refinery instances"` GRPCListenAddr string `long:"grpc-listen-address" env:"REFINERY_GRPC_LISTEN_ADDRESS" description:"gRPC listen address for OTLP traffic"` RedisHost string `long:"redis-host" env:"REFINERY_REDIS_HOST" description:"Redis host address"` + RedisClusterHosts []string `long:"redis-cluster-hosts" env:"REFINERY_REDIS_CLUSTER_HOSTS" env-delim:"," description:"Redis cluster host addresses"` RedisUsername string `long:"redis-username" env:"REFINERY_REDIS_USERNAME" description:"Redis username"` RedisPassword string `long:"redis-password" env:"REFINERY_REDIS_PASSWORD" description:"Redis password"` RedisAuthCode string `long:"redis-auth-code" env:"REFINERY_REDIS_AUTH_CODE" description:"Redis AUTH code"` @@ -50,6 +51,8 @@ type CmdEnv struct { NoValidate bool `long:"no-validate" description:"Do not attempt to validate the configuration files. Makes --validate meaningless."` WriteConfig string `long:"write-config" description:"After applying defaults, environment variables, and command line values, write the loaded configuration to the specified file as YAML and exit."` WriteRules string `long:"write-rules" description:"After applying defaults, write the loaded rules to the specified file as YAML and exit."` + + structType reflect.Type } func NewCmdEnvOptions(args []string) (*CmdEnv, error) { @@ -70,12 +73,29 @@ func NewCmdEnvOptions(args []string) (*CmdEnv, error) { } } + v := reflect.ValueOf(*opts) + switch v.Kind() { + case reflect.Struct: + opts.structType = v.Type() + } + return opts, nil } +type EnvField struct { + value reflect.Value + delimiter string +} + // GetField returns the reflect.Value for the field with the given name in the CmdEnvOptions struct. -func (c *CmdEnv) GetField(name string) reflect.Value { - return reflect.ValueOf(c).Elem().FieldByName(name) +func (c *CmdEnv) GetField(name string) EnvField { + field, _ := c.structType.FieldByName(name) + delim := field.Tag.Get("env-delim") + + return EnvField{ + value: reflect.ValueOf(c).Elem().FieldByName(name), + delimiter: delim, + } } // ApplyTags uses reflection to apply the values from the CmdEnv struct to the @@ -89,7 +109,7 @@ func (c *CmdEnv) ApplyTags(s reflect.Value) error { } type getFielder interface { - GetField(name string) reflect.Value + GetField(name string) EnvField } // applyCmdEnvTags is a helper function that applies the values from the given @@ -106,8 +126,8 @@ func applyCmdEnvTags(s reflect.Value, fielder getFielder) error { if tags := fieldType.Tag.Get("cmdenv"); tags != "" { // this field has a cmdenv tag, so try all its values for _, tag := range strings.Split(tags, ",") { - value := fielder.GetField(tag) - if !value.IsValid() { + f := fielder.GetField(tag) + if !f.value.IsValid() { // if you get this error, you didn't specify cmdenv tags // correctly -- its value must be the name of a field in the struct return fmt.Errorf("programming error -- invalid field name: %s", tag) @@ -117,14 +137,37 @@ func applyCmdEnvTags(s reflect.Value, fielder getFielder) error { } // don't overwrite values that are already set - if !value.IsZero() { + if !f.value.IsZero() { // ensure that the types match - if fieldType.Type != value.Type() { + if fieldType.Type != f.value.Type() { return fmt.Errorf("programming error -- types don't match for field: %s (%v and %v)", - fieldType.Name, fieldType.Type, value.Type()) + fieldType.Name, fieldType.Type, f.value.Type()) + } + + if f.value.Kind() == reflect.Slice { + if f.delimiter == "" { + return fmt.Errorf("programming error -- missing delimiter for slice field: %s", fieldType.Name) + } + + rawValue, ok := f.value.Index(0).Interface().(string) + if !ok { + return fmt.Errorf("programming error -- slice field must be a string: %s", fieldType.Name) + } + + // split the value on the delimiter + values := strings.Split(rawValue, f.delimiter) + // create a new slice of the same type as the field + slice := reflect.MakeSlice(field.Type(), len(values), len(values)) + // iterate over the values and set them + for i, v := range values { + slice.Index(i).SetString(v) + } + // set the field + field.Set(slice) + break } // now we can set it - field.Set(value) + field.Set(f.value) // and we're done with this field break } diff --git a/config/cmdenv_test.go b/config/cmdenv_test.go index e137183703..54d1c2b91f 100644 --- a/config/cmdenv_test.go +++ b/config/cmdenv_test.go @@ -6,22 +6,37 @@ import ( ) type TestFielder struct { - S string - S2 string - I int - F float64 + S string + S2 string + I int + F float64 + STRS []string `env-delim:","` } // implement getFielder -func (t *TestFielder) GetField(name string) reflect.Value { - return reflect.ValueOf(t).Elem().FieldByName(name) +func (t *TestFielder) GetField(name string) EnvField { + v := reflect.ValueOf(*t) + var structType reflect.Type + switch v.Kind() { + case reflect.Struct: + structType = v.Type() + } + + field, _ := structType.FieldByName(name) + delim := field.Tag.Get("env-delim") + + return EnvField{ + value: reflect.ValueOf(t).Elem().FieldByName(name), + delimiter: delim, + } } type TestConfig struct { - St string `cmdenv:"S"` - It int `cmdenv:"I"` - Fl float64 `cmdenv:"F"` - No string + St string `cmdenv:"S"` + It int `cmdenv:"I"` + Fl float64 `cmdenv:"F"` + No string + Strs []string `cmdenv:"STRS"` } type FallbackConfig struct { @@ -43,11 +58,11 @@ func TestApplyCmdEnvTags(t *testing.T) { want any wantErr bool }{ - {"normal", &TestFielder{"foo", "bar", 1, 2.3}, &TestConfig{}, &TestConfig{"foo", 1, 2.3, ""}, false}, - {"bad", &TestFielder{"foo", "bar", 1, 2.3}, &BadTestConfig1{}, &BadTestConfig1{}, true}, - {"type mismatch", &TestFielder{"foo", "bar", 1, 2.3}, &BadTestConfig2{17}, &BadTestConfig2{17}, true}, - {"fallback1", &TestFielder{"foo", "bar", 1, 2.3}, &FallbackConfig{}, &FallbackConfig{"foo"}, false}, - {"fallback2", &TestFielder{"", "bar", 1, 2.3}, &FallbackConfig{}, &FallbackConfig{"bar"}, false}, + {"normal", &TestFielder{"foo", "bar", 1, 2.3, []string{"test,test1"}}, &TestConfig{}, &TestConfig{"foo", 1, 2.3, "", []string{"test", "test1"}}, false}, + {"bad", &TestFielder{"foo", "bar", 1, 2.3, []string{}}, &BadTestConfig1{}, &BadTestConfig1{}, true}, + {"type mismatch", &TestFielder{"foo", "bar", 1, 2.3, []string{}}, &BadTestConfig2{17}, &BadTestConfig2{17}, true}, + {"fallback1", &TestFielder{"foo", "bar", 1, 2.3, []string{}}, &FallbackConfig{}, &FallbackConfig{"foo"}, false}, + {"fallback2", &TestFielder{"", "bar", 1, 2.3, []string{}}, &FallbackConfig{}, &FallbackConfig{"bar"}, false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/config/file_config.go b/config/file_config.go index c4463f574a..aea5fd6c65 100644 --- a/config/file_config.go +++ b/config/file_config.go @@ -285,7 +285,7 @@ type PeerManagementConfig struct { type RedisPeerManagementConfig struct { Host string `yaml:"Host" cmdenv:"RedisHost"` - ClusterHosts []string `yaml:"ClusterHosts"` + ClusterHosts []string `yaml:"ClusterHosts" cmdenv:"RedisClusterHosts"` Username string `yaml:"Username" cmdenv:"RedisUsername"` Password string `yaml:"Password" cmdenv:"RedisPassword"` AuthCode string `yaml:"AuthCode" cmdenv:"RedisAuthCode"` From f1c57dcd01478b1d290ebd7b03b3efd6cf952fad Mon Sep 17 00:00:00 2001 From: Yingrong Zhao <22300958+VinozzZ@users.noreply.github.com> Date: Thu, 22 Aug 2024 14:30:50 -0400 Subject: [PATCH 6/6] simplify code --- config/cmdenv.go | 51 ++++++++++++++++++------------------------- config/cmdenv_test.go | 21 +++++++----------- 2 files changed, 29 insertions(+), 43 deletions(-) diff --git a/config/cmdenv.go b/config/cmdenv.go index f60e7d3754..0cd18f48f7 100644 --- a/config/cmdenv.go +++ b/config/cmdenv.go @@ -51,8 +51,6 @@ type CmdEnv struct { NoValidate bool `long:"no-validate" description:"Do not attempt to validate the configuration files. Makes --validate meaningless."` WriteConfig string `long:"write-config" description:"After applying defaults, environment variables, and command line values, write the loaded configuration to the specified file as YAML and exit."` WriteRules string `long:"write-rules" description:"After applying defaults, write the loaded rules to the specified file as YAML and exit."` - - structType reflect.Type } func NewCmdEnvOptions(args []string) (*CmdEnv, error) { @@ -73,29 +71,20 @@ func NewCmdEnvOptions(args []string) (*CmdEnv, error) { } } - v := reflect.ValueOf(*opts) - switch v.Kind() { - case reflect.Struct: - opts.structType = v.Type() - } - return opts, nil } -type EnvField struct { - value reflect.Value - delimiter string -} - // GetField returns the reflect.Value for the field with the given name in the CmdEnvOptions struct. -func (c *CmdEnv) GetField(name string) EnvField { - field, _ := c.structType.FieldByName(name) - delim := field.Tag.Get("env-delim") +func (c *CmdEnv) GetField(name string) reflect.Value { + return reflect.ValueOf(c).Elem().FieldByName(name) +} - return EnvField{ - value: reflect.ValueOf(c).Elem().FieldByName(name), - delimiter: delim, +func (c *CmdEnv) GetDelimiter(name string) string { + field, ok := reflect.TypeOf(c).Elem().FieldByName(name) + if !ok { + return "" } + return field.Tag.Get("env-delim") } // ApplyTags uses reflection to apply the values from the CmdEnv struct to the @@ -109,7 +98,8 @@ func (c *CmdEnv) ApplyTags(s reflect.Value) error { } type getFielder interface { - GetField(name string) EnvField + GetField(name string) reflect.Value + GetDelimiter(name string) string } // applyCmdEnvTags is a helper function that applies the values from the given @@ -126,8 +116,8 @@ func applyCmdEnvTags(s reflect.Value, fielder getFielder) error { if tags := fieldType.Tag.Get("cmdenv"); tags != "" { // this field has a cmdenv tag, so try all its values for _, tag := range strings.Split(tags, ",") { - f := fielder.GetField(tag) - if !f.value.IsValid() { + value := fielder.GetField(tag) + if !value.IsValid() { // if you get this error, you didn't specify cmdenv tags // correctly -- its value must be the name of a field in the struct return fmt.Errorf("programming error -- invalid field name: %s", tag) @@ -137,25 +127,26 @@ func applyCmdEnvTags(s reflect.Value, fielder getFielder) error { } // don't overwrite values that are already set - if !f.value.IsZero() { + if !value.IsZero() { // ensure that the types match - if fieldType.Type != f.value.Type() { + if fieldType.Type != value.Type() { return fmt.Errorf("programming error -- types don't match for field: %s (%v and %v)", - fieldType.Name, fieldType.Type, f.value.Type()) + fieldType.Name, fieldType.Type, value.Type()) } - if f.value.Kind() == reflect.Slice { - if f.delimiter == "" { + if value.Kind() == reflect.Slice { + delimiter := fielder.GetDelimiter(tag) + if delimiter == "" { return fmt.Errorf("programming error -- missing delimiter for slice field: %s", fieldType.Name) } - rawValue, ok := f.value.Index(0).Interface().(string) + rawValue, ok := value.Index(0).Interface().(string) if !ok { return fmt.Errorf("programming error -- slice field must be a string: %s", fieldType.Name) } // split the value on the delimiter - values := strings.Split(rawValue, f.delimiter) + values := strings.Split(rawValue, delimiter) // create a new slice of the same type as the field slice := reflect.MakeSlice(field.Type(), len(values), len(values)) // iterate over the values and set them @@ -167,7 +158,7 @@ func applyCmdEnvTags(s reflect.Value, fielder getFielder) error { break } // now we can set it - field.Set(f.value) + field.Set(value) // and we're done with this field break } diff --git a/config/cmdenv_test.go b/config/cmdenv_test.go index 54d1c2b91f..a68054e3d0 100644 --- a/config/cmdenv_test.go +++ b/config/cmdenv_test.go @@ -14,21 +14,16 @@ type TestFielder struct { } // implement getFielder -func (t *TestFielder) GetField(name string) EnvField { - v := reflect.ValueOf(*t) - var structType reflect.Type - switch v.Kind() { - case reflect.Struct: - structType = v.Type() - } - - field, _ := structType.FieldByName(name) - delim := field.Tag.Get("env-delim") +func (t *TestFielder) GetField(name string) reflect.Value { + return reflect.ValueOf(t).Elem().FieldByName(name) +} - return EnvField{ - value: reflect.ValueOf(t).Elem().FieldByName(name), - delimiter: delim, +func (t *TestFielder) GetDelimiter(name string) string { + field, ok := reflect.TypeOf(t).Elem().FieldByName(name) + if !ok { + return "" } + return field.Tag.Get("env-delim") } type TestConfig struct {