From ff42683731dbdb2c018b3b1167dcbabf65aa968f Mon Sep 17 00:00:00 2001 From: Kolbe Kegel Date: Thu, 21 Mar 2019 13:22:34 -0700 Subject: [PATCH 1/6] Stop server startup if unrecognized options are found in the config file --- config/config.go | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/config/config.go b/config/config.go index 2a3361ff90a70..a8500397d7e96 100644 --- a/config/config.go +++ b/config/config.go @@ -16,6 +16,7 @@ package config import ( "crypto/tls" "crypto/x509" + "fmt" "io/ioutil" "time" @@ -365,10 +366,22 @@ func GetGlobalConfig() *Config { // Load loads config options from a toml file. func (c *Config) Load(confFile string) error { - _, err := toml.DecodeFile(confFile, c) + metaData, err := toml.DecodeFile(confFile, c) if c.TokenLimit <= 0 { c.TokenLimit = 1000 } + + // If any items in confFile file are not mapped into the Config struct, issue + // an error and stop the server from starting. + undecoded := metaData.Undecoded() + if len(undecoded) > 0 { + undecodedItems := "" + for _, item := range undecoded { + undecodedItems += fmt.Sprintf(" %s \n", item.String()) + } + err = errors.Errorf("config file %s contained unknown configuration options:\n%s", confFile, undecodedItems) + } + return errors.Trace(err) } From d5c8f8afb23a236731c9e893bc8df8a5315f78c5 Mon Sep 17 00:00:00 2001 From: Kolbe Kegel Date: Thu, 21 Mar 2019 15:30:41 -0700 Subject: [PATCH 2/6] Added test for unrecognized options in a config file throwing an error --- config/config.go | 2 +- config/config_test.go | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/config/config.go b/config/config.go index a8500397d7e96..219175540b5eb 100644 --- a/config/config.go +++ b/config/config.go @@ -374,7 +374,7 @@ func (c *Config) Load(confFile string) error { // If any items in confFile file are not mapped into the Config struct, issue // an error and stop the server from starting. undecoded := metaData.Undecoded() - if len(undecoded) > 0 { + if len(undecoded) > 0 && err == nil { undecodedItems := "" for _, item := range undecoded { undecodedItems += fmt.Sprintf(" %s \n", item.String()) diff --git a/config/config_test.go b/config/config_test.go index 137c07572053f..bddb5b2dba4d5 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -44,6 +44,19 @@ func (s *testConfigSuite) TestConfig(c *C) { f, err := os.Create(configFile) c.Assert(err, IsNil) + + // Make sure the server refuses to start if there's an unrecognized configuration option + _, err = f.WriteString(` +unrecognized-option-test = true +`) + c.Assert(err, IsNil) + c.Assert(f.Sync(), IsNil) + + c.Assert(conf.Load(configFile), ErrorMatches, "(?:.|\n)*unknown configuration option(?:.|\n)*") + + f.Truncate(0) + f.Seek(0, 0) + _, err = f.WriteString(`[performance] [tikv-client] commit-timeout="41s" From 3238d1e50ebd4a09f6c83d31343fae49b1d5dd0d Mon Sep 17 00:00:00 2001 From: Kolbe Kegel Date: Fri, 22 Mar 2019 09:38:24 -0700 Subject: [PATCH 3/6] Added --config-check flag --- tidb-server/main.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tidb-server/main.go b/tidb-server/main.go index c12814fabe10c..84fa176b1f1df 100644 --- a/tidb-server/main.go +++ b/tidb-server/main.go @@ -63,6 +63,7 @@ import ( const ( nmVersion = "V" nmConfig = "config" + nmConfigCheck = "config-check" nmStore = "store" nmStorePath = "path" nmHost = "host" @@ -89,8 +90,9 @@ const ( ) var ( - version = flagBoolean(nmVersion, false, "print version information and exit") - configPath = flag.String(nmConfig, "", "config file path") + version = flagBoolean(nmVersion, false, "print version information and exit") + configPath = flag.String(nmConfig, "", "config file path") + configCheck = flagBoolean(nmConfigCheck, false, "check config file validity") // Base store = flag.String(nmStore, "mocktikv", "registered store name, [tikv, mocktikv]") @@ -142,6 +144,10 @@ func main() { loadConfig() overrideConfig() validateConfig() + if *configCheck { + fmt.Println("config check successful") + os.Exit(0) + } setGlobalVars() setupLog() setupTracing() // Should before createServer and after setup config. From 6266fd83de593a7861306fbb4390ac9e72916a08 Mon Sep 17 00:00:00 2001 From: Kolbe Kegel Date: Fri, 22 Mar 2019 10:20:03 -0700 Subject: [PATCH 4/6] Clarified behavior of new --config-check flag in --help output --- tidb-server/main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tidb-server/main.go b/tidb-server/main.go index 84fa176b1f1df..27ebc7ffb51da 100644 --- a/tidb-server/main.go +++ b/tidb-server/main.go @@ -92,7 +92,7 @@ const ( var ( version = flagBoolean(nmVersion, false, "print version information and exit") configPath = flag.String(nmConfig, "", "config file path") - configCheck = flagBoolean(nmConfigCheck, false, "check config file validity") + configCheck = flagBoolean(nmConfigCheck, false, "check config file validity and exit") // Base store = flag.String(nmStore, "mocktikv", "registered store name, [tikv, mocktikv]") From eb2a90a6748e24342500c37acf997b738466d820 Mon Sep 17 00:00:00 2001 From: Kolbe Kegel Date: Mon, 1 Apr 2019 22:11:34 -0700 Subject: [PATCH 5/6] Added new error type and some kind of circuitous handling to save and defer a warning from loadConfig until after logging has been set up. Virtually all of this should be removed after configStrict is made the default behavior of TiDB Server! --- config/config.go | 21 +++++++++++++++++---- tidb-server/main.go | 26 +++++++++++++++++++++----- 2 files changed, 38 insertions(+), 9 deletions(-) diff --git a/config/config.go b/config/config.go index ab8ab779cfd73..f5112120320d7 100644 --- a/config/config.go +++ b/config/config.go @@ -18,6 +18,7 @@ import ( "crypto/x509" "fmt" "io/ioutil" + "strings" "time" "github.com/BurntSushi/toml" @@ -109,6 +110,18 @@ type Security struct { ClusterSSLKey string `toml:"cluster-ssl-key" json:"cluster-ssl-key"` } +// The ErrConfigValidationFailed error is used so that external callers can do a type assertion +// to defer handling of this specific error when someone does not want strict type checking. +// This is needed only because logging hasn't been set up at the time we parse the config file. +// This should all be ripped out once strict config checking is made the default behavior. +type ErrConfigValidationFailed struct { + err string +} + +func (e *ErrConfigValidationFailed) Error() string { + return e.err +} + // ToTLSConfig generates tls's config based on security section of the config. func (s *Security) ToTLSConfig() (*tls.Config, error) { var tlsConfig *tls.Config @@ -383,14 +396,14 @@ func (c *Config) Load(confFile string) error { // an error and stop the server from starting. undecoded := metaData.Undecoded() if len(undecoded) > 0 && err == nil { - undecodedItems := "" + var undecodedItems []string for _, item := range undecoded { - undecodedItems += fmt.Sprintf(" %s \n", item.String()) + undecodedItems = append(undecodedItems, item.String()) } - err = errors.Errorf("config file %s contained unknown configuration options:\n%s", confFile, undecodedItems) + err = &ErrConfigValidationFailed{fmt.Sprintf("config file %s contained unknown configuration options: %s", confFile, strings.Join(undecodedItems, ", "))} } - return errors.Trace(err) + return err } // ToLogConfig converts *Log to *logutil.LogConfig. diff --git a/tidb-server/main.go b/tidb-server/main.go index 7d2866b5c6420..d8e1da3e1723d 100644 --- a/tidb-server/main.go +++ b/tidb-server/main.go @@ -64,6 +64,7 @@ const ( nmVersion = "V" nmConfig = "config" nmConfigCheck = "config-check" + nmConfigStrict = "config-strict" nmStore = "store" nmStorePath = "path" nmHost = "host" @@ -91,9 +92,10 @@ const ( ) var ( - version = flagBoolean(nmVersion, false, "print version information and exit") - configPath = flag.String(nmConfig, "", "config file path") - configCheck = flagBoolean(nmConfigCheck, false, "check config file validity and exit") + version = flagBoolean(nmVersion, false, "print version information and exit") + configPath = flag.String(nmConfig, "", "config file path") + configCheck = flagBoolean(nmConfigCheck, false, "check config file validity and exit") + configStrict = flagBoolean(nmConfigStrict, false, "enforce config file validity") // Base store = flag.String(nmStore, "mocktikv", "registered store name, [tikv, mocktikv]") @@ -143,7 +145,7 @@ func main() { } registerStores() registerMetrics() - loadConfig() + configWarning := loadConfig() overrideConfig() validateConfig() if *configCheck { @@ -152,6 +154,12 @@ func main() { } setGlobalVars() setupLog() + // If configStrict had been specified, and there had been an error, the server would already + // have exited by now. If configWarning is not an empty string, write it to the log now that + // it's been properly set up. + if configWarning != "" { + log.Warn(configWarning) + } setupTracing() // Should before createServer and after setup config. printInfo() setupBinlogClient() @@ -293,12 +301,20 @@ func flagBoolean(name string, defaultVal bool, usage string) *bool { return flag.Bool(name, defaultVal, usage) } -func loadConfig() { +func loadConfig() string { cfg = config.GetGlobalConfig() if *configPath != "" { err := cfg.Load(*configPath) + // This block is to accommodate an interim situation where strict config checking + // is not the default behavior of TiDB. The warning message must be deferred until + // logging has been set up. After strict config checking is the default behavior, + // This should all be removed. + if _, ok := err.(*config.ErrConfigValidationFailed); ok && !*configStrict { + return err.Error() + } terror.MustNil(err) } + return "" } func overrideConfig() { From bb0ec3d80e2a159cc53dc4df09dbaef3561c0c64 Mon Sep 17 00:00:00 2001 From: Kolbe Kegel Date: Tue, 2 Apr 2019 09:13:08 -0700 Subject: [PATCH 6/6] Added handling for case of --config-check=true --config-strict=false. Fixed test. --- config/config_test.go | 7 ++++--- tidb-server/main.go | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/config/config_test.go b/config/config_test.go index cb4831a046664..06110c2da9cc6 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -58,13 +58,14 @@ unrecognized-option-test = true f.Truncate(0) f.Seek(0, 0) - _, err = f.WriteString(`[performance] + _, err = f.WriteString(` +token-limit = 0 +[performance] [tikv-client] commit-timeout="41s" max-batch-size=128 - -token-limit = -1 `) + c.Assert(err, IsNil) c.Assert(f.Sync(), IsNil) diff --git a/tidb-server/main.go b/tidb-server/main.go index d8e1da3e1723d..cf295c4565543 100644 --- a/tidb-server/main.go +++ b/tidb-server/main.go @@ -309,7 +309,7 @@ func loadConfig() string { // is not the default behavior of TiDB. The warning message must be deferred until // logging has been set up. After strict config checking is the default behavior, // This should all be removed. - if _, ok := err.(*config.ErrConfigValidationFailed); ok && !*configStrict { + if _, ok := err.(*config.ErrConfigValidationFailed); ok && !*configCheck && !*configStrict { return err.Error() } terror.MustNil(err)