From 53e45edc9cd7b50be0a9153c525bc26d7bdd4943 Mon Sep 17 00:00:00 2001 From: Joe Kralicky Date: Thu, 27 Jul 2023 12:10:17 -0400 Subject: [PATCH] Improve yaml empty node validation by inspecting the ast directly instead of checking the struct after unmarshaling --- cmd/cortex/main.go | 42 +++++++++++++++++++++++++++++++++++++++++ cmd/cortex/main_test.go | 17 ++++++++++++++++- pkg/cortex/cortex.go | 32 ------------------------------- 3 files changed, 58 insertions(+), 33 deletions(-) diff --git a/cmd/cortex/main.go b/cmd/cortex/main.go index 824c913daf..af486ed1c2 100644 --- a/cmd/cortex/main.go +++ b/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}. diff --git a/cmd/cortex/main_test.go b/cmd/cortex/main_test.go index 5c71949129..c8ff391d79 100644 --- a/cmd/cortex/main_test.go +++ b/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": { diff --git a/pkg/cortex/cortex.go b/pkg/cortex/cortex.go index 83496d3565..128e550bbf 100644 --- a/pkg/cortex/cortex.go +++ b/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)