master/worker/task add strictly config verification (pingcap#212)

* add task config verify * remove useless test config * add decodefile tests * remove useless config in dm-ansible
lichunzhu · Jul 24, 2019 · e37f9cc · e37f9cc
1 parent bb1c19b
commit e37f9cc
Show file tree

Hide file tree

Showing 37 changed files with 167 additions and 47 deletions.
diff --git a/dm/config/task.go b/dm/config/task.go
@@ -294,7 +294,7 @@ func (c *TaskConfig) DecodeFile(fpath string) error {
 		return errors.Annotatef(err, "read config file %v", fpath)
 	}
 
-	err = yaml.Unmarshal(bs, c)
+	err = yaml.UnmarshalStrict(bs, c)
 	if err != nil {
 		return errors.Trace(err)
 	}
@@ -304,7 +304,7 @@ func (c *TaskConfig) DecodeFile(fpath string) error {
 
 // Decode loads config from file data
 func (c *TaskConfig) Decode(data string) error {
-	err := yaml.Unmarshal([]byte(data), c)
+	err := yaml.UnmarshalStrict([]byte(data), c)
 	if err != nil {
 		return errors.Trace(err)
 	}

diff --git a/dm/config/task_test.go b/dm/config/task_test.go
@@ -0,0 +1,119 @@
+// Copyright 2019 PingCAP, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package config
+
+import (
+	. "github.com/pingcap/check"
+	"io/ioutil"
+	"path"
+)
+
+func (t *testConfig) TestInvalidTaskConfig(c *C) {
+	var errorTaskConfig1 = `---
+name: test
+task-mode: all
+is-sharding: true
+meta-schema: "dm_meta"
+remove-meta: false
+enable-heartbeat: true
+timezone: "Asia/Shanghai"
+ignore-checking-items: ["all"]
+
+target-database:
+  host: "127.0.0.1"
+  port: 4000
+  user: "root"
+  password: ""
+
+mysql-instances:
+  - source-id: "mysql-replica-01"
+    server-id: 101 
+    black-white-list:  "instance"
+    route-rules: ["sharding-route-rules-table", "sharding-route-rules-schema"]
+    column-mapping-rules: ["instance-1"]
+    mydumper-config-name: "global"
+    loader-config-name: "global"
+    syncer-config-name: "global"
+`
+	var errorTaskConfig2 = `---
+name: test
+name: test1
+task-mode: all
+is-sharding: true
+meta-schema: "dm_meta"
+remove-meta: false
+enable-heartbeat: true
+timezone: "Asia/Shanghai"
+ignore-checking-items: ["all"]
+
+target-database:
+  host: "127.0.0.1"
+  port: 4000
+  user: "root"
+  password: ""
+
+mysql-instances:
+  - source-id: "mysql-replica-01"
+    black-white-list:  "instance"
+    route-rules: ["sharding-route-rules-table", "sharding-route-rules-schema"]
+    column-mapping-rules: ["instance-1"]
+    mydumper-config-name: "global"
+    loader-config-name: "global"
+    syncer-config-name: "global"
+`
+	taskConfig := NewTaskConfig()
+	err := taskConfig.Decode(errorTaskConfig1)
+	// field server-id is not a member of TaskConfig
+	c.Check(err, NotNil)
+	c.Assert(err, ErrorMatches, "*line 19: field server-id not found in type config.MySQLInstance*")
+
+	err = taskConfig.Decode(errorTaskConfig2)
+	// field name duplicate
+	c.Check(err, NotNil)
+	c.Assert(err, ErrorMatches, "*line 3: field name already set in type config.TaskConfig*")
+
+	filepath := path.Join(c.MkDir(), "test_invalid_task.yaml")
+	configContent := []byte(`---
+aaa: xxx
+name: test
+task-mode: all
+is-sharding: true
+meta-schema: "dm_meta"
+remove-meta: false
+enable-heartbeat: true
+timezone: "Asia/Shanghai"
+ignore-checking-items: ["all"]
+`)
+	err = ioutil.WriteFile(filepath, configContent, 0644)
+	err = taskConfig.DecodeFile(filepath)
+	c.Assert(err, NotNil)
+	c.Assert(err, ErrorMatches, "*line 2: field aaa not found in type config.TaskConfig*")
+
+	filepath = path.Join(c.MkDir(), "test_invalid_task.yaml")
+	configContent = []byte(`---
+name: test
+task-mode: all
+task-mode: all
+is-sharding: true
+meta-schema: "dm_meta"
+remove-meta: false
+enable-heartbeat: true
+timezone: "Asia/Shanghai"
+ignore-checking-items: ["all"]
+`)
+	err = ioutil.WriteFile(filepath, configContent, 0644)
+	err = taskConfig.DecodeFile(filepath)
+	c.Assert(err, NotNil)
+	c.Assert(err, ErrorMatches, "*line 4: field task-mode already set in type config.TaskConfig*")
+}
diff --git a/dm/dm-ansible/roles/dm-worker/templates/dm-worker.toml.j2 b/dm/dm-ansible/roles/dm-worker/templates/dm-worker.toml.j2
@@ -3,7 +3,6 @@
 server-id = {{ server_id }}
 source-id = "{{ source_id }}"
 flavor = "{{ flavor }}"
-meta-file = "relay.meta"
 enable-gtid = {{ enable_gtid }}
 relay-binlog-name = "{{ relay_binlog_name }}"
 relay-binlog-gtid = "{{ relay_binlog_gtid }}"

diff --git a/dm/master/config.go b/dm/master/config.go
@@ -152,7 +152,15 @@ func (c *Config) Parse(arguments []string) error {
 
 // configFromFile loads config from file.
 func (c *Config) configFromFile(path string) error {
-	_, err := toml.DecodeFile(path, c)
+	metaData, err := toml.DecodeFile(path, c)
+	undecoded := metaData.Undecoded()
+	if len(undecoded) > 0 && err == nil {
+		var undecodedItems []string
+		for _, item := range undecoded {
+			undecodedItems = append(undecodedItems, item.String())
+		}
+		return errors.Errorf("master config contained unknown configuration options: %s", strings.Join(undecodedItems, ","))
+	}
 	return errors.Trace(err)
 }
 

diff --git a/dm/master/config_test.go b/dm/master/config_test.go
@@ -181,4 +181,17 @@ dm-worker = "172.16.10.72:8262"`)
 	cfg.ConfigFile = filepath
 	err = cfg.Reload()
 	c.Assert(err, check.NotNil)
+
+	filepath2 := path.Join(c.MkDir(), "test_invalid_config.toml")
+	// field still remain undecoded in config will cause verify failed
+	configContent2 := []byte(`
+master-addr = ":8261"
+aaa = "xxx"
+
+[[deploy]]
+dm-worker = "172.16.10.72:8262"`)
+	err = ioutil.WriteFile(filepath2, configContent2, 0644)
+	err = cfg.configFromFile(filepath2)
+	c.Assert(err, check.NotNil)
+	c.Assert(err, check.ErrorMatches, "*master config contained unknown configuration options: aaa*")
 }
diff --git a/dm/master/server_test.go b/dm/master/server_test.go
@@ -52,7 +52,6 @@ target-database:
 
 mysql-instances:
   - source-id: "mysql-replica-01"
-    server-id: 101
     black-white-list:  "instance"
     route-rules: ["sharding-route-rules-table", "sharding-route-rules-schema"]
     column-mapping-rules: ["instance-1"]
@@ -61,7 +60,6 @@ mysql-instances:
     syncer-config-name: "global"
 
   - source-id: "mysql-replica-02"
-    server-id: 102
     black-white-list:  "instance"
     route-rules: ["sharding-route-rules-table", "sharding-route-rules-schema"]
     column-mapping-rules: ["instance-2"]

diff --git a/dm/worker/config.go b/dm/worker/config.go
@@ -216,7 +216,16 @@ func (c *Config) verify() error {
 
 // configFromFile loads config from file.
 func (c *Config) configFromFile(path string) error {
-	_, err := toml.DecodeFile(path, c)
+	metaData, err := toml.DecodeFile(path, c)
+	undecoded := metaData.Undecoded()
+	if len(undecoded) > 0 && err == nil {
+		var undecodedItems []string
+		for _, item := range undecoded {
+			undecodedItems = append(undecodedItems, item.String())
+		}
+		return errors.Errorf("worker config contained unknown configuration options: %s", strings.Join(undecodedItems, ","))
+	}
+
 	if err != nil {
 		return errors.Trace(err)
 	}

diff --git a/dm/worker/config_test.go b/dm/worker/config_test.go
@@ -15,6 +15,7 @@ package worker
 
 import (
 	"fmt"
+	"io/ioutil"
 	"path"
 	"strings"
 
@@ -73,6 +74,19 @@ func (t *testServer) TestConfig(c *C) {
 	clone3, err := cfg.DecryptPassword()
 	c.Assert(err, IsNil)
 	c.Assert(clone3, DeepEquals, cfg)
+
+	// test invalid config
+	dir2 := c.MkDir()
+	configFile := path.Join(dir2, "dm-worker-invalid.toml")
+	configContent := []byte(`
+worker-addr = ":8262"
+aaa = "xxx"
+`)
+	err = ioutil.WriteFile(configFile, configContent, 0644)
+	c.Assert(err, IsNil)
+	err = cfg.configFromFile(configFile)
+	c.Assert(err, NotNil)
+	c.Assert(err, ErrorMatches, "*worker config contained unknown configuration options: aaa*")
 }
 
 func (t *testServer) TestConfigVerify(c *C) {

diff --git a/tests/all_mode/conf/dm-task.yaml b/tests/all_mode/conf/dm-task.yaml
@@ -17,14 +17,12 @@ target-database:
 
 mysql-instances:
   - source-id: "mysql-replica-01"
-    server-id: 101
     black-white-list:  "instance"
     mydumper-config-name: "global"
     loader-config-name: "global"
     syncer-config-name: "global"
 
   - source-id: "mysql-replica-02"
-    server-id: 102
     black-white-list:  "instance"
     mydumper-config-name: "global"
     loader-config-name: "global"

diff --git a/tests/all_mode/conf/dm-worker1.toml b/tests/all_mode/conf/dm-worker1.toml
@@ -3,7 +3,6 @@
 server-id = 101
 source-id = "mysql-replica-01"
 flavor = "mysql"
-meta-file = "relay.meta"
 enable-gtid = false
 relay-binlog-name = ""
 relay-binlog-gtid = ""

diff --git a/tests/all_mode/conf/dm-worker2.toml b/tests/all_mode/conf/dm-worker2.toml
@@ -3,7 +3,6 @@
 server-id = 102
 source-id = "mysql-replica-02"
 flavor = "mysql"
-meta-file = "relay.meta"
 enable-gtid = false
 relay-binlog-name = ""
 relay-binlog-gtid = ""

diff --git a/tests/dmctl_basic/conf/dm-task.yaml b/tests/dmctl_basic/conf/dm-task.yaml
@@ -15,7 +15,6 @@ target-database:
 
 mysql-instances:
   - source-id: "mysql-replica-01"
-    server-id: 101
     black-white-list:  "instance"
     route-rules: ["sharding-route-rules-table", "sharding-route-rules-schema"]
     column-mapping-rules: ["instance-1"]
@@ -24,7 +23,6 @@ mysql-instances:
     syncer-config-name: "global"
 
   - source-id: "mysql-replica-02"
-    server-id: 102
     black-white-list:  "instance"
     route-rules: ["sharding-route-rules-table", "sharding-route-rules-schema"]
     column-mapping-rules: ["instance-2"]

diff --git a/tests/dmctl_basic/conf/dm-worker1.toml b/tests/dmctl_basic/conf/dm-worker1.toml
@@ -3,7 +3,6 @@
 server-id = 101
 source-id = "mysql-replica-01"
 flavor = "mysql"
-meta-file = "relay.meta"
 enable-gtid = false
 relay-binlog-name = ""
 relay-binlog-gtid = ""

diff --git a/tests/dmctl_basic/conf/dm-worker2.toml b/tests/dmctl_basic/conf/dm-worker2.toml
@@ -3,7 +3,6 @@
 server-id = 102
 source-id = "mysql-replica-02"
 flavor = "mysql"
-meta-file = "relay.meta"
 enable-gtid = false
 relay-binlog-name = ""
 relay-binlog-gtid = ""

diff --git a/tests/incremental_mode/conf/dm-task.yaml b/tests/incremental_mode/conf/dm-task.yaml
@@ -17,7 +17,6 @@ target-database:
 
 mysql-instances:
   - source-id: "mysql-replica-01"
-    server-id: 101
     meta:
       binlog-name: binlog-name-placeholder-1
       binlog-pos: binlog-pos-placeholder-1
@@ -27,7 +26,6 @@ mysql-instances:
     syncer-config-name: "global"
 
   - source-id: "mysql-replica-02"
-    server-id: 102
     meta:
       binlog-name: binlog-name-placeholder-2
       binlog-pos: binlog-pos-placeholder-2

diff --git a/tests/incremental_mode/conf/dm-worker1.toml b/tests/incremental_mode/conf/dm-worker1.toml
@@ -3,7 +3,6 @@
 server-id = 101
 source-id = "mysql-replica-01"
 flavor = "mysql"
-meta-file = "relay.meta"
 enable-gtid = false
 relay-binlog-name = ""
 relay-binlog-gtid = ""

diff --git a/tests/incremental_mode/conf/dm-worker2.toml b/tests/incremental_mode/conf/dm-worker2.toml
@@ -3,7 +3,6 @@
 server-id = 102
 source-id = "mysql-replica-02"
 flavor = "mysql"
-meta-file = "relay.meta"
 enable-gtid = false
 relay-binlog-name = ""
 relay-binlog-gtid = ""

diff --git a/tests/load_interrupt/conf/dm-task.yaml b/tests/load_interrupt/conf/dm-task.yaml
@@ -15,14 +15,12 @@ target-database:
 
 mysql-instances:
   - source-id: "mysql-replica-01"
-    server-id: 101
     black-white-list:  "instance"
     mydumper-config-name: "global"
     loader-config-name: "global"
     syncer-config-name: "global"
 
   - source-id: "mysql-replica-02"
-    server-id: 102
     black-white-list:  "instance"
     mydumper-config-name: "global"
     loader-config-name: "global"

diff --git a/tests/load_interrupt/conf/dm-worker1.toml b/tests/load_interrupt/conf/dm-worker1.toml
@@ -3,7 +3,6 @@
 server-id = 101
 source-id = "mysql-replica-01"
 flavor = "mysql"
-meta-file = "relay.meta"
 enable-gtid = false
 relay-binlog-name = ""
 relay-binlog-gtid = ""

diff --git a/tests/load_interrupt/conf/dm-worker2.toml b/tests/load_interrupt/conf/dm-worker2.toml
@@ -3,7 +3,6 @@
 server-id = 102
 source-id = "mysql-replica-02"
 flavor = "mysql"
-meta-file = "relay.meta"
 enable-gtid = false
 relay-binlog-name = ""
 relay-binlog-gtid = ""

diff --git a/tests/online_ddl/conf/dm-task.yaml b/tests/online_ddl/conf/dm-task.yaml
@@ -16,7 +16,6 @@ target-database:
 
 mysql-instances:
   - source-id: "mysql-replica-01"
-    server-id: 101
     black-white-list:  "instance"
     route-rules: ["sharding-route-rules-table", "sharding-route-rules-schema"]
     column-mapping-rules: ["instance-1"]
@@ -25,7 +24,6 @@ mysql-instances:
     syncer-config-name: "global"
 
   - source-id: "mysql-replica-02"
-    server-id: 102
     meta:
       binlog-name: binlog.000001
       binlog-pos: 4

diff --git a/tests/online_ddl/conf/dm-worker1.toml b/tests/online_ddl/conf/dm-worker1.toml
@@ -3,7 +3,6 @@
 server-id = 101
 source-id = "mysql-replica-01"
 flavor = "mysql"
-meta-file = "relay.meta"
 enable-gtid = false
 relay-binlog-name = ""
 relay-binlog-gtid = ""