From be9054628af2786cd25cf30980629129872624dc Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Wed, 25 Aug 2021 16:57:11 +0200 Subject: [PATCH] Ensure extra config options are properly initialized YAML is decoded by the structure of the YAML source document, not the Go destination data structure. Therefore, the old code did not always call UnmarshalYAML() on all sub-structs. Therefore, defaults were not always set but zero values were used, resulting in all kind of strange behavior. This commit changes the code so that it no longer relies on individual UnmarshalYAML() functions to set the defaults for each sub-struct but instead just sets all of them when creating the surrounding Config instance. It also moves the config validation to separate Validate() functions. --- pkg/config/config.go | 20 ++++++++++++++++++++ pkg/config/database.go | 5 +++++ pkg/config/redis.go | 5 +++++ pkg/icingadb/db.go | 15 ++------------- pkg/icingaredis/client.go | 16 ++-------------- 5 files changed, 34 insertions(+), 27 deletions(-) diff --git a/pkg/config/config.go b/pkg/config/config.go index 3ace47d4..93d26a96 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -1,6 +1,7 @@ package config import ( + "github.com/creasty/defaults" "github.com/jessevdk/go-flags" "github.com/pkg/errors" "gopkg.in/yaml.v3" @@ -13,6 +14,17 @@ type Config struct { Redis Redis `yaml:"redis"` } +// Validate checks constraints in the supplied configuration and returns an error if they are violated. +func (c *Config) Validate() error { + if err := c.Database.Validate(); err != nil { + return err + } + if err := c.Redis.Validate(); err != nil { + return err + } + return nil +} + // Flags defines CLI flags. type Flags struct { // Version decides whether to just print the version and exit. @@ -32,10 +44,18 @@ func FromYAMLFile(name string) (*Config, error) { c := &Config{} d := yaml.NewDecoder(f) + if err := defaults.Set(c); err != nil { + return nil, errors.Wrap(err, "can't set config defaults") + } + if err := d.Decode(c); err != nil { return nil, errors.Wrap(err, "can't parse YAML file "+name) } + if err := c.Validate(); err != nil { + return nil, errors.Wrap(err, "invalid configuration") + } + return c, nil } diff --git a/pkg/config/database.go b/pkg/config/database.go index 0f57f579..30cdec19 100644 --- a/pkg/config/database.go +++ b/pkg/config/database.go @@ -49,3 +49,8 @@ func (d *Database) Open(logger *zap.SugaredLogger) (*icingadb.DB, error) { return icingadb.NewDb(db, logger, &d.Options), nil } + +// Validate checks constraints in the supplied database configuration and returns an error if they are violated. +func (d *Database) Validate() error { + return d.Options.Validate() +} diff --git a/pkg/config/redis.go b/pkg/config/redis.go index f11412b5..76872aae 100644 --- a/pkg/config/redis.go +++ b/pkg/config/redis.go @@ -74,3 +74,8 @@ func dialWithLogging(logger *zap.SugaredLogger) func(context.Context, string, st return } } + +// Validate checks constraints in the supplied Redis configuration and returns an error if they are violated. +func (r *Redis) Validate() error { + return r.Options.Validate() +} diff --git a/pkg/icingadb/db.go b/pkg/icingadb/db.go index 51ff8b9e..161dbd6f 100644 --- a/pkg/icingadb/db.go +++ b/pkg/icingadb/db.go @@ -4,7 +4,6 @@ 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" @@ -56,24 +55,14 @@ 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) - } - +// Validate checks constraints in the supplied database options and returns an error if they are violated. +func (o *Options) Validate() error { 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") } - return nil } diff --git a/pkg/icingaredis/client.go b/pkg/icingaredis/client.go index a56cb105..308dffec 100644 --- a/pkg/icingaredis/client.go +++ b/pkg/icingaredis/client.go @@ -2,9 +2,7 @@ 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" @@ -34,17 +32,8 @@ 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) - } - +// Validate checks constraints in the supplied Redis options and returns an error if they are violated. +func (o *Options) Validate() error { if o.Timeout == 0 { return errors.New("timeout cannot be 0. Configure a value greater than zero, or use -1 for no timeout") } @@ -57,7 +46,6 @@ func (o *Options) UnmarshalYAML(unmarshal func(interface{}) error) error { if o.HScanCount < 1 { return errors.New("hscan_count must be at least 1") } - return nil }