Fail the config validation if an empty node is set at root level (#3080)

Signed-off-by: Marco Pracucci <[email protected]>

Author: pracucci

CHANGELOG.md

@@ -45,6 +45,7 @@
 * [ENHANCEMENT] Add config validation to the experimental Alertmanager API. Invalid configs are no longer accepted. #3053
 * [ENHANCEMENT] Add "integration" as a label for `cortex_alertmanager_notifications_total` and `cortex_alertmanager_notifications_failed_total` metrics. #3056
 * [ENHANCEMENT] Add `cortex_ruler_config_last_reload_successful` and `cortex_ruler_config_last_reload_successful_seconds` to check status of users rule manager. #3056
+* [ENHANCEMENT] The configuration validation now fails if an empty YAML node has been set for a root YAML config property. #3080
 * [ENHANCEMENT] Memcached dial() calls now have an optional circuit-breaker to avoid hammering a broken cache #3051
 * [ENHANCEMENT] `-ruler.evaluation-delay-duration` is now overridable as a per-tenant limit, `ruler_evaluation_delay_duration`. #3098
 * [ENHANCEMENT] Add TLS support to etcd client. #3102

cmd/cortex/main.go

@@ -106,6 +106,16 @@ func main() {
 		}
 	}
 
+	// Validate the config once both the config file has been loaded
+	// and CLI flags parsed.
+	err = cfg.Validate(util.Logger)
+	if err != nil {
+		fmt.Fprintf(os.Stderr, "error validating config: %v\n", err)
+		if !testMode {
+			os.Exit(1)
+		}
+	}
+
 	// Continue on if -modules flag is given. Code handling the
 	// -modules flag will not start cortex.
 	if testMode && !cfg.ListModules {
@@ -118,13 +128,6 @@ func main() {
 	}
 
 	util.InitLogger(&cfg.Server)
-	// Validate the config once both the config file has been loaded
-	// and CLI flags parsed.
-	err = cfg.Validate(util.Logger)
-	if err != nil {
-		fmt.Printf("error validating config: %v\n", err)
-		os.Exit(1)
-	}
 
 	// Allocate a block of memory to alter GC behaviour. See https://github.com/golang/go/issues/23044
 	ballast := make([]byte, ballastBytes)

cmd/cortex/main_test.go

@@ -75,6 +75,11 @@ func TestFlagParsing(t *testing.T) {
 			stderrExcluded: "ingester\n",
 		},
 
+		"root level configuration option specified as an empty node in YAML": {
+			yaml:          "querier:",
+			stderrMessage: "the Querier configuration in YAML has been specified as an empty YAML node",
+		},
+
 		// we cannot test the happy path, as cortex would then fully start
 	} {
 		t.Run(name, func(t *testing.T) {

pkg/chunk/encoding/factory.go

@@ -22,7 +22,7 @@ var (
 // RegisterFlags registers configuration settings.
 func (Config) RegisterFlags(f *flag.FlagSet) {
 	f.Var(&DefaultEncoding, "ingester.chunk-encoding", "Encoding version to use for chunks.")
-	flag.IntVar(&bigchunkSizeCapBytes, "store.bigchunk-size-cap-bytes", bigchunkSizeCapBytes, "When using bigchunk encoding, start a new bigchunk if over this size (0 = unlimited)")
+	f.IntVar(&bigchunkSizeCapBytes, "store.bigchunk-size-cap-bytes", bigchunkSizeCapBytes, "When using bigchunk encoding, start a new bigchunk if over this size (0 = unlimited)")
 }
 
 // Validate errors out if the encoding is set to Delta.

pkg/cortex/cortex.go

@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"net/http"
 	"os"
+	"reflect"
 
 	"github.com/go-kit/kit/log"
 	"github.com/go-kit/kit/log/level"
@@ -43,6 +44,7 @@ import (
 	"github.com/cortexproject/cortex/pkg/storegateway"
 	"github.com/cortexproject/cortex/pkg/util"
 	"github.com/cortexproject/cortex/pkg/util/fakeauth"
+	"github.com/cortexproject/cortex/pkg/util/flagext"
 	"github.com/cortexproject/cortex/pkg/util/grpc/healthcheck"
 	"github.com/cortexproject/cortex/pkg/util/modules"
 	"github.com/cortexproject/cortex/pkg/util/runtimeconfig"
@@ -143,12 +145,16 @@ func (c *Config) RegisterFlags(f *flag.FlagSet) {
 	c.MemberlistKV.RegisterFlags(f, "")
 
 	// These don't seem to have a home.
-	flag.IntVar(&chunk_util.QueryParallelism, "querier.query-parallelism", 100, "Max subqueries run in parallel per higher-level query.")
+	f.IntVar(&chunk_util.QueryParallelism, "querier.query-parallelism", 100, "Max subqueries run in parallel per higher-level query.")
 }
 
 // Validate the cortex config and returns an error if the validation
 // doesn't pass
 func (c *Config) Validate(log log.Logger) error {
+	if err := c.validateYAMLEmptyNodes(); err != nil {
+		return err
+	}
+
 	if err := c.Schema.Validate(); err != nil {
 		return errors.Wrap(err, "invalid schema config")
 	}
@@ -199,6 +205,33 @@ func (c *Config) Validate(log log.Logger) error {
 	return nil
 }
 
+// validateYAMLEmptyNodes ensure that no empty node has been specified in the YAML config file.
+// When an empty node is defined in YAML, the YAML parser sets the whole struct to its zero value
+// and so we loose all default values. It's very difficult to detect this case for the user, so we
+// try to prevent it (on the root level) with this custom validation.
+func (c *Config) validateYAMLEmptyNodes() error {
+	defaults := Config{}
+	flagext.DefaultValues(&defaults)
+
+	defStruct := reflect.ValueOf(defaults)
+	cfgStruct := reflect.ValueOf(*c)
+
+	// We expect all structs are the exact same. This check should never fail.
+	if cfgStruct.NumField() != defStruct.NumField() {
+		return errors.New("unable to validate configuration because of mismatching internal config data structure")
+	}
+
+	for i := 0; i < cfgStruct.NumField(); i++ {
+		// If the struct has been reset due to empty YAML value and the zero struct value
+		// doesn't match the default one, then we should warn the user about the issue.
+		if cfgStruct.Field(i).Kind() == reflect.Struct && cfgStruct.Field(i).IsZero() && !defStruct.Field(i).IsZero() {
+			return fmt.Errorf("the %s configuration in YAML has been specified as an empty YAML node", cfgStruct.Type().Field(i).Name)
+		}
+	}
+
+	return nil
+}
+
 // Cortex is the root datastructure for Cortex.
 type Cortex struct {
 	Cfg Config