Repository: pingcap/tidb-binlog
Pull request #687 (2019-07-23 02:11:31)

Return error when loading some undefined configs from local
<!--
Thank you for contributing to TiDB! Please read TiDB's [CONTRIBUTING](https://github.com/pingcap/tidb/blob/master/CONTRIBUTING.md) document **BEFORE** filing this PR.
-->

### What problem does this PR solve? <!--add issue link with summary if exists-->
[TOOL-1403](https://internal.pingcap.net/jira/browse/TOOL-1403)
Referral PR: [TiDB#9855](https://github.com/pingcap/tidb/pull/9855/)
### What is changed and how it works?
Now func `configFromFile` in `config.go` of drainer, pump, arbiter, reparo will make sure that there is no undecoded args in `metaData`. This can help customers check their misspell in the config toml files. Relative tests have been added, too.

### Check List <!--REMOVE the items that are not applicable-->

Tests <!-- At least one of them must be included. -->

 - Unit test
 - Integration test

Code changes

Side effects

Related changes


kennytm: /run-all-tests

Patch:
diff --git a/arbiter/config.go b/arbiter/config.go
index 36644603..e2f2a1ea 100644
--- a/arbiter/config.go
+++ b/arbiter/config.go
@@ -19,10 +19,10 @@ import (
 	"fmt"
 	"os"
 
-	"github.com/BurntSushi/toml"
 	"github.com/pingcap/errors"
 	"github.com/pingcap/log"
 	"github.com/pingcap/tidb-binlog/pkg/flags"
+	"github.com/pingcap/tidb-binlog/pkg/util"
 	"github.com/pingcap/tidb-binlog/pkg/version"
 	"go.uber.org/zap"
 )
@@ -190,6 +190,5 @@ func (cfg *Config) adjustConfig() error {
 }
 
 func (cfg *Config) configFromFile(path string) error {
-	_, err := toml.DecodeFile(path, cfg)
-	return errors.Trace(err)
+	return util.StrictDecodeFile(path, "arbiter", cfg)
 }
diff --git a/drainer/config.go b/drainer/config.go
index 2f49d061..32704366 100644
--- a/drainer/config.go
+++ b/drainer/config.go
@@ -25,7 +25,6 @@ import (
 	"strings"
 	"time"
 
-	"github.com/BurntSushi/toml"
 	"github.com/pingcap/errors"
 	"github.com/pingcap/log"
 	"github.com/pingcap/parser/mysql"
@@ -223,8 +222,7 @@ func (c *SyncerConfig) adjustDoDBAndTable() {
 }
 
 func (cfg *Config) configFromFile(path string) error {
-	_, err := toml.DecodeFile(path, cfg)
-	return errors.Trace(err)
+	return util.StrictDecodeFile(path, "drainer", cfg)
 }
 
 // validate checks whether the configuration is valid
diff --git a/pkg/util/util.go b/pkg/util/util.go
index cfd901a4..8cee45ed 100644
--- a/pkg/util/util.go
+++ b/pkg/util/util.go
@@ -17,8 +17,10 @@ import (
 	"context"
 	"fmt"
 	"net"
+	"strings"
 	"time"
 
+	"github.com/BurntSushi/toml"
 	"github.com/pingcap/errors"
 	"github.com/pingcap/log"
 	"github.com/pingcap/parser/model"
@@ -178,6 +180,26 @@ func RetryContext(ctx context.Context, retryCount int, sleepTime time.Duration,
 	return err
 }
 
+// StrictDecodeFile decodes the toml file strictly. If any item in confFile file is not mapped
+// into the Config struct, issue an error and stop the server from starting.
+func StrictDecodeFile(path, component string, cfg interface{}) error {
+	metaData, err := toml.DecodeFile(path, cfg)
+	if err != nil {
+		return errors.Trace(err)
+	}
+
+	if undecoded := metaData.Undecoded(); len(undecoded) > 0 {
+		var undecodedItems []string
+		for _, item := range undecoded {
+			undecodedItems = append(undecodedItems, item.String())
+		}
+		err = errors.Errorf("component %s's config file %s contained unknown configuration options: %s",
+			component, path, strings.Join(undecodedItems, ", "))
+	}
+
+	return errors.Trace(err)
+}
+
 // TryUntilSuccess retries the given function until error is nil or the context is done,
 // waiting for `waitInterval` time between retries.
 func TryUntilSuccess(ctx context.Context, waitInterval time.Duration, errMsg string, fn func() error) error {
diff --git a/pump/config.go b/pump/config.go
index 987c7aec..b9f29fed 100644
--- a/pump/config.go
+++ b/pump/config.go
@@ -23,7 +23,6 @@ import (
 	"strings"
 	"time"
 
-	"github.com/BurntSushi/toml"
 	"github.com/pingcap/errors"
 	"github.com/pingcap/tidb-binlog/pkg/flags"
 	"github.com/pingcap/tidb-binlog/pkg/security"
@@ -175,8 +174,7 @@ func (cfg *Config) Parse(arguments []string) error {
 }
 
 func (cfg *Config) configFromFile(path string) error {
-	_, err := toml.DecodeFile(path, cfg)
-	return errors.Trace(err)
+	return util.StrictDecodeFile(path, "pump", cfg)
 }
 
 // validate checks whether the configuration is valid
diff --git a/reparo/config.go b/reparo/config.go
index 3de84f3d..72652a19 100644
--- a/reparo/config.go
+++ b/reparo/config.go
@@ -21,11 +21,11 @@ import (
 	"strings"
 	"time"
 
-	"github.com/BurntSushi/toml"
 	"github.com/pingcap/errors"
 	"github.com/pingcap/log"
 	"github.com/pingcap/tidb-binlog/pkg/filter"
 	"github.com/pingcap/tidb-binlog/pkg/flags"
+	"github.com/pingcap/tidb-binlog/pkg/util"
 	"github.com/pingcap/tidb-binlog/pkg/version"
 	"github.com/pingcap/tidb-binlog/reparo/syncer"
 	"github.com/pingcap/tidb/store/tikv/oracle"
@@ -168,8 +168,7 @@ func (c *Config) adjustDoDBAndTable() {
 }
 
 func (c *Config) configFromFile(path string) error {
-	_, err := toml.DecodeFile(path, c)
-	return errors.Trace(err)
+	return util.StrictDecodeFile(path, "reparo", c)
 }
 
 func (c *Config) validate() error {


Test patch:
diff --git a/arbiter/config_test.go b/arbiter/config_test.go
index 8b7907ae..56bc7ac2 100644
--- a/arbiter/config_test.go
+++ b/arbiter/config_test.go
@@ -14,11 +14,14 @@
 package arbiter
 
 import (
+	"bytes"
 	"fmt"
+	"io/ioutil"
 	"path"
 	"runtime"
 	"strings"
 
+	"github.com/BurntSushi/toml"
 	"github.com/pingcap/check"
 )
 
@@ -105,6 +108,38 @@ func (t *TestConfigSuite) TestParseConfig(c *check.C) {
 	c.Assert(strings.Contains(config.String(), listenAddr), check.IsTrue)
 }
 
+func (t *TestConfigSuite) TestParseConfigFileWithInvalidArgs(c *check.C) {
+	yc := struct {
+		LogLevel               string `toml:"log-level" json:"log-level"`
+		ListenAddr             string `toml:"addr" json:"addr"`
+		LogFile                string `toml:"log-file" json:"log-file"`
+		UnrecognizedOptionTest bool   `toml:"unrecognized-option-test" json:"unrecognized-option-test"`
+	}{
+		"debug",
+		"127.0.0.1:8251",
+		"/tmp/arbiter",
+		true,
+	}
+
+	var buf bytes.Buffer
+	e := toml.NewEncoder(&buf)
+	err := e.Encode(yc)
+	c.Assert(err, check.IsNil)
+
+	configFilename := path.Join(c.MkDir(), "arbiter_config_invalid.toml")
+	err = ioutil.WriteFile(configFilename, buf.Bytes(), 0644)
+	c.Assert(err, check.IsNil)
+
+	args := []string{
+		"--config",
+		configFilename,
+	}
+
+	cfg := NewConfig()
+	err = cfg.Parse(args)
+	c.Assert(err, check.ErrorMatches, ".*contained unknown configuration options: unrecognized-option-test.*")
+}
+
 func getTemplateConfigFilePath() string {
 	// we put the template config file in "cmd/arbiter/arbiter.toml"
 	_, filename, _, _ := runtime.Caller(0)
diff --git a/drainer/config_test.go b/drainer/config_test.go
index 5ae65dd8..4749b284 100644
--- a/drainer/config_test.go
+++ b/drainer/config_test.go
@@ -14,8 +14,12 @@
 package drainer
 
 import (
+	"bytes"
+	"io/ioutil"
+	"path"
 	"testing"
 
+	"github.com/BurntSushi/toml"
 	"github.com/coreos/etcd/integration"
 	. "github.com/pingcap/check"
 	"github.com/pingcap/parser/mysql"
@@ -118,3 +122,36 @@ func (t *testDrainerSuite) TestAdjustConfig(c *C) {
 	c.Assert(cfg.ListenAddr, Equals, "http://0.0.0.0:8257")
 	c.Assert(cfg.AdvertiseAddr, Equals, "http://192.168.15.12:8257")
 }
+
+func (t *testDrainerSuite) TestConfigParsingFileWithInvalidOptions(c *C) {
+	yc := struct {
+		DataDir                string `toml:"data-dir" json:"data-dir"`
+		ListenAddr             string `toml:"addr" json:"addr"`
+		AdvertiseAddr          string `toml:"advertise-addr" json:"advertise-addr"`
+		UnrecognizedOptionTest bool   `toml:"unrecognized-option-test" json:"unrecognized-option-test"`
+	}{
+		"data.drainer",
+		"192.168.15.10:8257",
+		"192.168.15.10:8257",
+		true,
+	}
+
+	var buf bytes.Buffer
+	e := toml.NewEncoder(&buf)
+	err := e.Encode(yc)
+	c.Assert(err, IsNil)
+
+	configFilename := path.Join(c.MkDir(), "drainer_config_invalid.toml")
+	err = ioutil.WriteFile(configFilename, buf.Bytes(), 0644)
+	c.Assert(err, IsNil)
+
+	args := []string{
+		"--config",
+		configFilename,
+		"-L", "debug",
+	}
+
+	cfg := NewConfig()
+	err = cfg.Parse(args)
+	c.Assert(err, ErrorMatches, ".*contained unknown configuration options: unrecognized-option-test.*")
+}
diff --git a/pump/config_test.go b/pump/config_test.go
index 2d571a81..76dc6a6d 100644
--- a/pump/config_test.go
+++ b/pump/config_test.go
@@ -17,6 +17,7 @@ import (
 	"bytes"
 	"io/ioutil"
 	"os"
+	"path"
 
 	"github.com/BurntSushi/toml"
 	. "github.com/pingcap/check"
@@ -75,6 +76,7 @@ func (s *testConfigSuite) TestConfigParsingEnvFlags(c *C) {
 	os.Setenv("PUMP_ADDR", "192.168.199.200:9000")
 	os.Setenv("PUMP_PD_URLS", "http://127.0.0.1:2379,http://localhost:2379")
 	os.Setenv("PUMP_DATA_DIR", "/tmp/pump")
+	defer os.Clearenv()
 
 	cfg := NewConfig()
 	mustSuccess(c, cfg.Parse(args))
@@ -84,7 +86,7 @@ func (s *testConfigSuite) TestConfigParsingEnvFlags(c *C) {
 func (s *testConfigSuite) TestConfigParsingFileFlags(c *C) {
 	yc := struct {
 		ListenAddr        string `toml:"addr" json:"addr"`
-		AdvertiseAddr     string `toml:"advertiser-addr" json:"advertise-addr"`
+		AdvertiseAddr     string `toml:"advertise-addr" json:"advertise-addr"`
 		EtcdURLs          string `toml:"pd-urls" json:"pd-urls"`
 		BinlogDir         string `toml:"data-dir" json:"data-dir"`
 		HeartbeatInterval uint   `toml:"heartbeat-interval" json:"heartbeat-interval"`
@@ -101,36 +103,60 @@ func (s *testConfigSuite) TestConfigParsingFileFlags(c *C) {
 	err := e.Encode(yc)
 	c.Assert(err, IsNil)
 
-	tmpfile := mustCreateCfgFile(c, buf.Bytes(), "pump_config")
-	defer os.Remove(tmpfile.Name())
+	configFilename := path.Join(c.MkDir(), "pump_config.toml")
+	err = ioutil.WriteFile(configFilename, buf.Bytes(), 0644)
+	c.Assert(err, IsNil)
 
 	args := []string{
 		"--config",
-		tmpfile.Name(),
+		configFilename,
 		"-L", "debug",
 	}
 
-	os.Clearenv()
 	cfg := NewConfig()
 	mustSuccess(c, cfg.Parse(args))
 	validateConfig(c, cfg)
 }
 
-func mustSuccess(c *C, err error) {
+func (s *testConfigSuite) TestConfigParsingFileWithInvalidArgs(c *C) {
+	yc := struct {
+		ListenAddr             string `toml:"addr" json:"addr"`
+		AdvertiseAddr          string `toml:"advertise-addr" json:"advertise-addr"`
+		EtcdURLs               string `toml:"pd-urls" json:"pd-urls"`
+		BinlogDir              string `toml:"data-dir" json:"data-dir"`
+		HeartbeatInterval      uint   `toml:"heartbeat-interval" json:"heartbeat-interval"`
+		UnrecognizedOptionTest bool   `toml:"unrecognized-option-test" json:"unrecognized-option-test"`
+	}{
+		"192.168.199.100:8260",
+		"192.168.199.100:8260",
+		"http://192.168.199.110:2379,http://hostname:2379",
+		"/tmp/pump",
+		1500,
+		true,
+	}
+
+	var buf bytes.Buffer
+	e := toml.NewEncoder(&buf)
+	err := e.Encode(yc)
 	c.Assert(err, IsNil)
-}
 
-func mustCreateCfgFile(c *C, b []byte, prefix string) *os.File {
-	tmpfile, err := ioutil.TempFile("", prefix)
-	mustSuccess(c, err)
+	configFilename := path.Join(c.MkDir(), "pump_config_invalid.toml")
+	err = ioutil.WriteFile(configFilename, buf.Bytes(), 0644)
+	c.Assert(err, IsNil)
 
-	_, err = tmpfile.Write(b)
-	mustSuccess(c, err)
+	args := []string{
+		"--config",
+		configFilename,
+		"-L", "debug",
+	}
 
-	err = tmpfile.Close()
-	mustSuccess(c, err)
+	cfg := NewConfig()
+	err = cfg.Parse(args)
+	c.Assert(err, ErrorMatches, ".*contained unknown configuration options: unrecognized-option-test.*")
+}
 
-	return tmpfile
+func mustSuccess(c *C, err error) {
+	c.Assert(err, IsNil)
 }
 
 func validateConfig(c *C, cfg *Config) {
diff --git a/reparo/config_test.go b/reparo/config_test.go
index 81efaa6a..e31c582d 100644
--- a/reparo/config_test.go
+++ b/reparo/config_test.go
@@ -14,10 +14,13 @@
 package reparo
 
 import (
+	"bytes"
 	"fmt"
+	"io/ioutil"
 	"path"
 	"runtime"
 
+	"github.com/BurntSushi/toml"
 	"github.com/pingcap/check"
 	"github.com/pingcap/tidb-binlog/pkg/filter"
 )
@@ -72,6 +75,46 @@ func (s *testConfigSuite) TestAdjustDoDBAndTable(c *check.C) {
 	c.Assert(config.DoDBs[1], check.Equals, "test2")
 }
 
+func (s *testConfigSuite) TestParseConfigFileWithInvalidArgs(c *check.C) {
+	yc := struct {
+		Dir                    string `toml:"data-dir" json:"data-dir"`
+		StartDatetime          string `toml:"start-datetime" json:"start-datetime"`
+		StopDatetime           string `toml:"stop-datetime" json:"stop-datetime"`
+		StartTSO               int64  `toml:"start-tso" json:"start-tso"`
+		StopTSO                int64  `toml:"stop-tso" json:"stop-tso"`
+		LogFile                string `toml:"log-file" json:"log-file"`
+		LogLevel               string `toml:"log-level" json:"log-level"`
+		UnrecognizedOptionTest bool   `toml:"unrecognized-option-test" json:"unrecognized-option-test"`
+	}{
+		"/tmp/reparo",
+		"",
+		"",
+		0,
+		0,
+		"tmp/reparo/reparo.log",
+		"debug",
+		true,
+	}
+
+	var buf bytes.Buffer
+	e := toml.NewEncoder(&buf)
+	err := e.Encode(yc)
+	c.Assert(err, check.IsNil)
+
+	configFilename := path.Join(c.MkDir(), "reparo_config_invalid.toml")
+	err = ioutil.WriteFile(configFilename, buf.Bytes(), 0644)
+	c.Assert(err, check.IsNil)
+
+	args := []string{
+		"--config",
+		configFilename,
+	}
+
+	cfg := NewConfig()
+	err = cfg.Parse(args)
+	c.Assert(err, check.ErrorMatches, ".*contained unknown configuration options: unrecognized-option-test.*")
+}
+
 func getTemplateConfigFilePath() string {
 	// we put the template config file in "cmd/reapro/reparo.toml"
 	_, filename, _, _ := runtime.Caller(0)