From f9e12d9df734d8cc95edf1f521df5b362b377b2e Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Wed, 4 Aug 2021 09:38:14 +0200 Subject: [PATCH 1/7] Move method --- pkg/config/redis.go | 48 ++++++++++++++++++++++----------------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/pkg/config/redis.go b/pkg/config/redis.go index 717f5624..7f0d00eb 100644 --- a/pkg/config/redis.go +++ b/pkg/config/redis.go @@ -42,6 +42,30 @@ func (r *Redis) NewClient(logger *zap.SugaredLogger) (*icingaredis.Client, error return icingaredis.NewClient(c, logger, &r.Options), nil } +// UnmarshalYAML implements the yaml.Unmarshaler interface. +func (r *Redis) UnmarshalYAML(unmarshal func(interface{}) error) error { + if err := defaults.Set(r); err != nil { + return errors.Wrapf(err, "can't set defaults %#v", r) + } + // Prevent recursion. + type self Redis + if err := unmarshal((*self)(r)); err != nil { + return internal.CantUnmarshalYAML(err, r) + } + + if r.MaxHMGetConnections < 1 { + return errors.New("max_hmget_connections must be at least 1") + } + if r.HMGetCount < 1 { + return errors.New("hmget_count must be at least 1") + } + if r.HScanCount < 1 { + return errors.New("hscan_count must be at least 1") + } + + return nil +} + // dialWithLogging returns a Redis Dialer with logging capabilities. func dialWithLogging(logger *zap.SugaredLogger) func(context.Context, string, string) (net.Conn, error) { // dial behaves like net.Dialer#DialContext, but re-tries on syscall.ECONNREFUSED. @@ -76,27 +100,3 @@ func dialWithLogging(logger *zap.SugaredLogger) func(context.Context, string, st return } } - -// UnmarshalYAML implements the yaml.Unmarshaler interface. -func (r *Redis) UnmarshalYAML(unmarshal func(interface{}) error) error { - if err := defaults.Set(r); err != nil { - return errors.Wrapf(err, "can't set defaults %#v", r) - } - // Prevent recursion. - type self Redis - if err := unmarshal((*self)(r)); err != nil { - return internal.CantUnmarshalYAML(err, r) - } - - if r.MaxHMGetConnections < 1 { - return errors.New("max_hmget_connections must be at least 1") - } - if r.HMGetCount < 1 { - return errors.New("hmget_count must be at least 1") - } - if r.HScanCount < 1 { - return errors.New("hscan_count must be at least 1") - } - - return nil -} From 559b27cd8b91f423624c4573c7f6ff5d20314657 Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Wed, 4 Aug 2021 09:40:22 +0200 Subject: [PATCH 2/7] Don't inline Redis options There is now the options key to separate required and optional configuration. Before, both were mixed. --- pkg/config/redis.go | 18 ++++-------------- pkg/icingaredis/client.go | 26 ++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 14 deletions(-) diff --git a/pkg/config/redis.go b/pkg/config/redis.go index 7f0d00eb..5c87ff08 100644 --- a/pkg/config/redis.go +++ b/pkg/config/redis.go @@ -19,9 +19,9 @@ import ( // Redis defines Redis client configuration. type Redis struct { - Address string `yaml:"address"` - Password string `yaml:"password"` - icingaredis.Options `yaml:",inline"` + Address string `yaml:"address"` + Password string `yaml:"password"` + Options icingaredis.Options `yaml:"options"` } // NewClient prepares Redis client configuration, @@ -32,7 +32,7 @@ func (r *Redis) NewClient(logger *zap.SugaredLogger) (*icingaredis.Client, error Dialer: dialWithLogging(logger), Password: r.Password, DB: 0, // Use default DB, - ReadTimeout: r.Timeout, + ReadTimeout: r.Options.Timeout, }) opts := c.Options() @@ -53,16 +53,6 @@ func (r *Redis) UnmarshalYAML(unmarshal func(interface{}) error) error { return internal.CantUnmarshalYAML(err, r) } - if r.MaxHMGetConnections < 1 { - return errors.New("max_hmget_connections must be at least 1") - } - if r.HMGetCount < 1 { - return errors.New("hmget_count must be at least 1") - } - if r.HScanCount < 1 { - return errors.New("hscan_count must be at least 1") - } - return nil } diff --git a/pkg/icingaredis/client.go b/pkg/icingaredis/client.go index 01a765d7..dd05be22 100644 --- a/pkg/icingaredis/client.go +++ b/pkg/icingaredis/client.go @@ -2,7 +2,9 @@ package icingaredis import ( "context" + "github.com/creasty/defaults" "github.com/go-redis/redis/v8" + "github.com/icinga/icingadb/internal" "github.com/icinga/icingadb/pkg/com" "github.com/icinga/icingadb/pkg/common" "github.com/icinga/icingadb/pkg/contracts" @@ -32,6 +34,30 @@ type Options struct { HScanCount int `yaml:"hscan_count" default:"4096"` } +// UnmarshalYAML implements the yaml.Unmarshaler interface. +func (o *Options) UnmarshalYAML(unmarshal func(interface{}) error) error { + if err := defaults.Set(o); err != nil { + return errors.Wrapf(err, "can't set defaults %#v", o) + } + // Prevent recursion. + type self Options + if err := unmarshal((*self)(o)); err != nil { + return internal.CantUnmarshalYAML(err, o) + } + + if o.MaxHMGetConnections < 1 { + return errors.New("max_hmget_connections must be at least 1") + } + if o.HMGetCount < 1 { + return errors.New("hmget_count must be at least 1") + } + if o.HScanCount < 1 { + return errors.New("hscan_count must be at least 1") + } + + return nil +} + // NewClient returns a new icingaredis.Client wrapper for a pre-existing *redis.Client. func NewClient(client *redis.Client, logger *zap.SugaredLogger, options *Options) *Client { return &Client{Client: client, logger: logger, options: options} From 8927e942f16484ffe923a1c7f978cd179571afd5 Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Wed, 4 Aug 2021 09:53:55 +0200 Subject: [PATCH 3/7] Remove UnmarshalYAML Config options are no longer inlined. --- pkg/config/redis.go | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/pkg/config/redis.go b/pkg/config/redis.go index 5c87ff08..f11412b5 100644 --- a/pkg/config/redis.go +++ b/pkg/config/redis.go @@ -2,9 +2,7 @@ package config import ( "context" - "github.com/creasty/defaults" "github.com/go-redis/redis/v8" - "github.com/icinga/icingadb/internal" "github.com/icinga/icingadb/pkg/backoff" "github.com/icinga/icingadb/pkg/icingaredis" "github.com/icinga/icingadb/pkg/retry" @@ -42,20 +40,6 @@ func (r *Redis) NewClient(logger *zap.SugaredLogger) (*icingaredis.Client, error return icingaredis.NewClient(c, logger, &r.Options), nil } -// UnmarshalYAML implements the yaml.Unmarshaler interface. -func (r *Redis) UnmarshalYAML(unmarshal func(interface{}) error) error { - if err := defaults.Set(r); err != nil { - return errors.Wrapf(err, "can't set defaults %#v", r) - } - // Prevent recursion. - type self Redis - if err := unmarshal((*self)(r)); err != nil { - return internal.CantUnmarshalYAML(err, r) - } - - return nil -} - // dialWithLogging returns a Redis Dialer with logging capabilities. func dialWithLogging(logger *zap.SugaredLogger) func(context.Context, string, string) (net.Conn, error) { // dial behaves like net.Dialer#DialContext, but re-tries on syscall.ECONNREFUSED. From 1c386c9c2fae8b9b3870156b7f4cbb845c764964 Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Wed, 4 Aug 2021 09:56:22 +0200 Subject: [PATCH 4/7] Don't inline Database options There is now the options key to separate required and optional configuration. Before, both were mixed. --- pkg/config/database.go | 20 ++++++++------------ pkg/icingadb/db.go | 19 +++++++++++++++++++ 2 files changed, 27 insertions(+), 12 deletions(-) diff --git a/pkg/config/database.go b/pkg/config/database.go index 5ef8c3dc..dd79a731 100644 --- a/pkg/config/database.go +++ b/pkg/config/database.go @@ -18,12 +18,12 @@ var registerDriverOnce sync.Once // Database defines database client configuration. type Database struct { - Host string `yaml:"host"` - Port int `yaml:"port"` - Database string `yaml:"database"` - User string `yaml:"user"` - Password string `yaml:"password"` - icingadb.Options `yaml:",inline"` + Host string `yaml:"host"` + Port int `yaml:"port"` + Database string `yaml:"database"` + User string `yaml:"user"` + Password string `yaml:"password"` + Options icingadb.Options `yaml:"options"` } // Open prepares the DSN string and driver configuration, @@ -42,8 +42,8 @@ func (d *Database) Open(logger *zap.SugaredLogger) (*icingadb.DB, error) { return nil, errors.Wrap(err, "can't open database") } - db.SetMaxIdleConns(d.MaxConnections / 3) - db.SetMaxOpenConns(d.MaxConnections) + db.SetMaxIdleConns(d.Options.MaxConnections / 3) + db.SetMaxOpenConns(d.Options.MaxConnections) db.Mapper = reflectx.NewMapperFunc("db", func(s string) string { return utils.Key(s, '_') @@ -63,9 +63,5 @@ func (d *Database) UnmarshalYAML(unmarshal func(interface{}) error) error { return internal.CantUnmarshalYAML(err, d) } - if d.MaxConnectionsPerTable < 1 { - return errors.New("max_connections_per_table must be at least 1") - } - return nil } diff --git a/pkg/icingadb/db.go b/pkg/icingadb/db.go index e471d6dc..caaea880 100644 --- a/pkg/icingadb/db.go +++ b/pkg/icingadb/db.go @@ -4,6 +4,7 @@ import ( "context" "database/sql/driver" "fmt" + "github.com/creasty/defaults" "github.com/go-sql-driver/mysql" "github.com/icinga/icingadb/internal" "github.com/icinga/icingadb/pkg/backoff" @@ -55,6 +56,24 @@ type Options struct { MaxRowsPerTransaction int `yaml:"MaxRowsPerTransaction" default:"8192"` } +// UnmarshalYAML implements the yaml.Unmarshaler interface. +func (o *Options) UnmarshalYAML(unmarshal func(interface{}) error) error { + if err := defaults.Set(o); err != nil { + return errors.Wrap(err, "can't set default database config") + } + // Prevent recursion. + type self Options + if err := unmarshal((*self)(o)); err != nil { + return internal.CantUnmarshalYAML(err, o) + } + + if o.MaxConnectionsPerTable < 1 { + return errors.New("max_connections_per_table must be at least 1") + } + + return nil +} + // NewDb returns a new icingadb.DB wrapper for a pre-existing *sqlx.DB. func NewDb(db *sqlx.DB, logger *zap.SugaredLogger, options *Options) *DB { return &DB{ From 50473bca70cb8f73ada5d121f9a5724a5853368c Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Wed, 4 Aug 2021 09:56:42 +0200 Subject: [PATCH 5/7] Remove UnmarshalYAML Config options are no longer inlined. --- pkg/config/database.go | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/pkg/config/database.go b/pkg/config/database.go index dd79a731..0f57f579 100644 --- a/pkg/config/database.go +++ b/pkg/config/database.go @@ -2,8 +2,6 @@ package config import ( "fmt" - "github.com/creasty/defaults" - "github.com/icinga/icingadb/internal" "github.com/icinga/icingadb/pkg/driver" "github.com/icinga/icingadb/pkg/icingadb" "github.com/icinga/icingadb/pkg/utils" @@ -51,17 +49,3 @@ func (d *Database) Open(logger *zap.SugaredLogger) (*icingadb.DB, error) { return icingadb.NewDb(db, logger, &d.Options), nil } - -// UnmarshalYAML implements the yaml.Unmarshaler interface. -func (d *Database) UnmarshalYAML(unmarshal func(interface{}) error) error { - if err := defaults.Set(d); err != nil { - return errors.Wrap(err, "can't set default database config") - } - // Prevent recursion. - type self Database - if err := unmarshal((*self)(d)); err != nil { - return internal.CantUnmarshalYAML(err, d) - } - - return nil -} From 8232000524f4fe20d18f0694cd75d0b4ff7fa710 Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Tue, 10 Aug 2021 08:55:24 +0200 Subject: [PATCH 6/7] Don't allow 0 for max_connections database option 0 stands for deactivate, which makes no sense here. --- pkg/icingadb/db.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/icingadb/db.go b/pkg/icingadb/db.go index caaea880..51ff8b9e 100644 --- a/pkg/icingadb/db.go +++ b/pkg/icingadb/db.go @@ -67,6 +67,9 @@ func (o *Options) UnmarshalYAML(unmarshal func(interface{}) error) error { return internal.CantUnmarshalYAML(err, o) } + if o.MaxConnections == 0 { + return errors.New("max_connections cannot be 0. Configure a value greater than zero, or use -1 for no connection limit") + } if o.MaxConnectionsPerTable < 1 { return errors.New("max_connections_per_table must be at least 1") } From fbbb9bfacdab3434da645deec84c5358123d7a00 Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Tue, 10 Aug 2021 09:29:27 +0200 Subject: [PATCH 7/7] Don't allow 0 for timeout redis option 0 stands for deactivate, which makes no sense here. --- pkg/icingaredis/client.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/icingaredis/client.go b/pkg/icingaredis/client.go index dd05be22..a56cb105 100644 --- a/pkg/icingaredis/client.go +++ b/pkg/icingaredis/client.go @@ -45,6 +45,9 @@ func (o *Options) UnmarshalYAML(unmarshal func(interface{}) error) error { return internal.CantUnmarshalYAML(err, o) } + if o.Timeout == 0 { + return errors.New("timeout cannot be 0. Configure a value greater than zero, or use -1 for no timeout") + } if o.MaxHMGetConnections < 1 { return errors.New("max_hmget_connections must be at least 1") }