From 412dee67f1fb6712223835929e5228a1da37424d Mon Sep 17 00:00:00 2001 From: Waldemar Quevedo Date: Sun, 13 Aug 2023 19:36:57 -0700 Subject: [PATCH 1/2] config: allow empty configs, but prevent bad configs - Adds reporting the line with the bad key position that makes the config invalid. - Fixes a few tests with trailing braces which were being handled as keys and ignored before. Signed-off-by: Waldemar Quevedo --- conf/parse.go | 8 +- conf/parse_test.go | 180 +++++++++++++++++++++++++++++++++++----- server/leafnode_test.go | 8 +- 3 files changed, 166 insertions(+), 30 deletions(-) diff --git a/conf/parse.go b/conf/parse.go index 4d2a20adee..05713409c6 100644 --- a/conf/parse.go +++ b/conf/parse.go @@ -137,18 +137,20 @@ func parse(data, fp string, pedantic bool) (p *parser, err error) { } p.pushContext(p.mapping) + var prevItem itemType for { it := p.next() if it.typ == itemEOF { + if prevItem == itemKey { + return nil, fmt.Errorf("config is invalid (%s:%d:%d)", fp, it.line, it.pos) + } break } + prevItem = it.typ if err := p.processItem(it, fp); err != nil { return nil, err } } - if len(p.mapping) == 0 { - return nil, fmt.Errorf("config has no values or is empty") - } return p, nil } diff --git a/conf/parse_test.go b/conf/parse_test.go index 3fc7c9275f..a6d5d2e85e 100644 --- a/conf/parse_test.go +++ b/conf/parse_test.go @@ -3,6 +3,7 @@ package conf import ( "fmt" "os" + "path/filepath" "reflect" "strings" "testing" @@ -404,29 +405,162 @@ func TestParserNoInfiniteLoop(t *testing.T) { } } -func TestParseWithNoValues(t *testing.T) { - for _, test := range []string{ - ``, - `aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa`, - ` aaaaaaaaaaaaaaaaaaaaaaaaaaa`, - ` aaaaaaaaaaaaaaaaaaaaaaaaaaa `, - ` - # just comments with no values - # is also invalid. - `, - ` - # with comments and no spaces to create key values - # is also an invalid config. - aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa - `, - ` - a,a,a,a,a,a,a,a,a,a,a - `, +func TestParseWithNoValuesAreInvalid(t *testing.T) { + for _, test := range []struct { + name string + conf string + err string + }{ + { + "invalid key without values", + `aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa`, + "config is invalid (:1:41)", + }, + { + "invalid untrimmed key without values", + ` aaaaaaaaaaaaaaaaaaaaaaaaaaa`, + "config is invalid (:1:41)", + }, + { + "invalid untrimmed key without values", + ` aaaaaaaaaaaaaaaaaaaaaaaaaaa `, + "config is invalid (:1:41)", + }, + { + "invalid keys after comments", + ` + # with comments and no spaces to create key values + # is also an invalid config. + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + `, + "config is invalid (:5:25)", + }, + { + "comma separated without values are invalid", + ` + a,a,a,a,a,a,a,a,a,a,a + `, + "config is invalid (:3:25)", + }, + { + // trailing brackets accidentally can become keys, these are also invalid. + "trailing brackets after config", + ` + accounts { users = [{}]} + } + `, + "config is invalid (:4:25)", + }, } { - if _, err := parse(test, "", true); err == nil { - t.Fatal("expected an error") - } else if !strings.Contains(err.Error(), "config has no values or is empty") { - t.Fatal("expected invalid conf error") - } + t.Run(test.name, func(t *testing.T) { + if _, err := parse(test.conf, "", true); err == nil { + t.Error("expected an error") + } else if !strings.Contains(err.Error(), test.err) { + t.Errorf("expected invalid conf error, got: %v", err) + } + }) + } +} + +func TestParseWithNoValuesEmptyConfigsAreValid(t *testing.T) { + for _, test := range []struct { + name string + conf string + }{ + { + "empty conf", + "", + }, + { + "empty conf with line breaks", + ` + + + `, + }, + { + "just comments with no values", + ` + # just comments with no values + # is still valid. + `, + }, + } { + t.Run(test.name, func(t *testing.T) { + if _, err := parse(test.conf, "", true); err != nil { + t.Errorf("unexpected error: %v", err) + } + }) + } +} + +func TestParseWithNoValuesIncludes(t *testing.T) { + for _, test := range []struct { + input string + includes map[string]string + err string + linepos string + }{ + { + `# includes + accounts { + foo { include 'foo.conf'} + bar { users = [{user = "bar"}] } + quux { include 'quux.conf'} + } + `, + map[string]string{ + "foo.conf": ``, + "quux.conf": `?????????????`, + }, + "error parsing include file 'quux.conf', config is invalid", + "quux.conf:1:1", + }, + { + `# includes + accounts { + foo { include 'foo.conf'} + bar { include 'bar.conf'} + quux { include 'quux.conf'} + } + `, + map[string]string{ + "foo.conf": ``, // Empty configs are ok + "bar.conf": `AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA`, + "quux.conf": ` + # just some comments, + # and no key values also ok. + `, + }, + "error parsing include file 'bar.conf', config is invalid", + "bar.conf:1:34", + }, + } { + t.Run("", func(t *testing.T) { + sdir := t.TempDir() + f, err := os.CreateTemp(sdir, "nats.conf-") + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(f.Name(), []byte(test.input), 066); err != nil { + t.Error(err) + } + if test.includes != nil { + for includeFile, contents := range test.includes { + inf, err := os.Create(filepath.Join(sdir, includeFile)) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(inf.Name(), []byte(contents), 066); err != nil { + t.Error(err) + } + } + } + if _, err := parse(test.input, f.Name(), true); err == nil { + t.Error("expected an error") + } else if !strings.Contains(err.Error(), test.err) || !strings.Contains(err.Error(), test.linepos) { + t.Errorf("expected invalid conf error, got: %v", err) + } + }) } } diff --git a/server/leafnode_test.go b/server/leafnode_test.go index 10caf8cbde..e6577fd373 100644 --- a/server/leafnode_test.go +++ b/server/leafnode_test.go @@ -2540,7 +2540,7 @@ func TestLeafNodeOperatorBadCfg(t *testing.T) { cfg: ` port: -1 authorization { - users = [{user: "u", password: "p"}]} + users = [{user: "u", password: "p"}] }`, }, { @@ -3876,9 +3876,9 @@ func TestLeafNodeInterestPropagationDaisychain(t *testing.T) { aTmpl := ` port: %d leafnodes { - port: %d - } - }` + port: %d + } + ` confA := createConfFile(t, []byte(fmt.Sprintf(aTmpl, -1, -1))) sA, _ := RunServerWithConfig(confA) From 3a20f665358b22f421165d19470901d411624415 Mon Sep 17 00:00:00 2001 From: Waldemar Quevedo Date: Sun, 13 Aug 2023 23:59:50 -0700 Subject: [PATCH 2/2] config: parsed empty config only show warnings Signed-off-by: Waldemar Quevedo --- server/config_check_test.go | 19 +++++++++++++++++++ server/opts.go | 3 +++ 2 files changed, 22 insertions(+) diff --git a/server/config_check_test.go b/server/config_check_test.go index 0ba2c77f3b..0d7187d3c7 100644 --- a/server/config_check_test.go +++ b/server/config_check_test.go @@ -1579,6 +1579,23 @@ func TestConfigCheck(t *testing.T) { errorLine: 5, errorPos: 6, }, + { + name: "show warnings on empty configs without values", + config: ``, + warningErr: errors.New(`config has no values or is empty`), + errorLine: 0, + errorPos: 0, + reason: "", + }, + { + name: "show warnings on empty configs without values and only comments", + config: `# Valid file but has no usable values. + `, + warningErr: errors.New(`config has no values or is empty`), + errorLine: 0, + errorPos: 0, + reason: "", + }, } checkConfig := func(config string) error { @@ -1620,6 +1637,8 @@ func TestConfigCheck(t *testing.T) { if test.reason != "" { msg += ": " + test.reason } + } else if test.warningErr != nil { + msg = expectedErr.Error() } else { msg = test.reason } diff --git a/server/opts.go b/server/opts.go index a2b231aa1f..1cb531dae0 100644 --- a/server/opts.go +++ b/server/opts.go @@ -744,6 +744,9 @@ func (o *Options) ProcessConfigFile(configFile string) error { // Collect all errors and warnings and report them all together. errors := make([]error, 0) warnings := make([]error, 0) + if len(m) == 0 { + warnings = append(warnings, fmt.Errorf("%s: config has no values or is empty", configFile)) + } // First check whether a system account has been defined, // as that is a condition for other features to be enabled.