Improve yaml empty node validation by inspecting the ast directly instead of checking the struct after unmarshaling

Signed-off-by: Joe Kralicky <joe.kralicky@suse.com>

Author: kralicky

cmd/cortex/main.go

@@ -18,6 +18,7 @@ import (
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/common/version"
 	"gopkg.in/yaml.v2"
+	yamlv3 "gopkg.in/yaml.v3"
 
 	"github.com/cortexproject/cortex/pkg/cortex"
 	"github.com/cortexproject/cortex/pkg/tracing"
@@ -250,6 +251,10 @@ func LoadConfig(filename string, expandENV bool, cfg *cortex.Config) error {
 		return errors.Wrap(err, "Error parsing config file")
 	}
 
+	if err := validateYAMLEmptyNodes(buf); err != nil {
+		return err
+	}
+
 	return nil
 }
 
@@ -262,6 +267,43 @@ func DumpYaml(cfg *cortex.Config) {
 	}
 }
 
+// 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 validateYAMLEmptyNodes(buf []byte) error {
+	var document yamlv3.Node
+	if err := yamlv3.Unmarshal(buf, &document); err != nil {
+		return err
+	}
+	if document.Kind != yamlv3.DocumentNode {
+		return errors.New("invalid YAML document")
+	}
+	for _, node := range document.Content {
+		if node.Kind != yamlv3.MappingNode || len(node.Content) < 2 || node.Content[0].Kind != yamlv3.ScalarNode {
+			continue
+		}
+		foundEmptyNode := false
+		switch node.Content[1].Kind {
+		case yamlv3.MappingNode:
+			if len(node.Content[1].Content) == 0 {
+				// "key: {}"
+				foundEmptyNode = true
+			}
+		case yamlv3.ScalarNode:
+			if node.Content[1].Tag == "!!null" {
+				// "key: null" or "key:"
+				foundEmptyNode = true
+			}
+		}
+		if foundEmptyNode {
+			return fmt.Errorf("the %s configuration in YAML has been specified as an empty YAML node", node.Content[0].Value)
+		}
+		return nil
+	}
+	return nil
+}
+
 // expandEnv replaces ${var} or $var in config according to the values of the current environment variables.
 // The replacement is case-sensitive. References to undefined variables are replaced by the empty string.
 // A default value can be given by using the form ${var:default value}.

cmd/cortex/main_test.go

@@ -76,7 +76,22 @@ func TestFlagParsing(t *testing.T) {
 
 		"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",
+			stderrMessage: "the querier configuration in YAML has been specified as an empty YAML node",
+		},
+
+		"root level configuration option specified as an empty object node in YAML": {
+			yaml:          "querier: {}",
+			stderrMessage: "the querier configuration in YAML has been specified as an empty YAML node",
+		},
+
+		"root level configuration option specified as a null node in YAML": {
+			yaml:          "querier: null",
+			stderrMessage: "the querier configuration in YAML has been specified as an empty YAML node",
+		},
+
+		"root level configuration option specified with all zero values": {
+			yaml:           "flusher: { exit_after_flush: false }",
+			stderrExcluded: "empty YAML node",
 		},
 
 		"version": {

pkg/cortex/cortex.go

@@ -7,7 +7,6 @@ import (
 	"fmt"
 	"net/http"
 	"os"
-	"reflect"
 	"strings"
 
 	"github.com/go-kit/log"
@@ -174,10 +173,6 @@ func (c *Config) RegisterFlags(f *flag.FlagSet) {
 // 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 c.HTTPPrefix != "" && !strings.HasPrefix(c.HTTPPrefix, "/") {
 		return errInvalidHTTPPrefix
 	}
@@ -236,33 +231,6 @@ func (c *Config) isModuleEnabled(m string) bool {
 	return util.StringsContain(c.Target, m)
 }
 
-// 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
-}
-
 func (c *Config) registerServerFlagsWithChangedDefaultValues(fs *flag.FlagSet) {
 	throwaway := flag.NewFlagSet("throwaway", flag.PanicOnError)