fix(secrets): panic and incorrect behaviour during secrets edit

Problem reproduced when adding, removing or changin order of map keys. Or when removing and changing order of array elements. Signed-off-by: Timofey Kirillov <timofey.kirillov@flant.com>
werf · Jul 1, 2022 · 289400d · 289400d
1 parent 2e404a9
commit 289400d
Show file tree

Hide file tree

Showing 3 changed files with 325 additions and 4 deletions.
diff --git a/pkg/secret/yaml_encoder_test.go b/pkg/secret/yaml_encoder_test.go
@@ -28,7 +28,14 @@ var _ = Describe("YamlEncoder", func() {
 			Expect(strings.TrimSpace(originalData)).To(Equal(strings.TrimSpace(string(resultData))), fmt.Sprintf("\n[EXPECTED]\n%q\n[GOT]\n%q\n", originalData, resultData))
 		},
 
-		Entry("simple yaml", `a: one`),
+		Entry("simple yaml", `
+a: one
+bbb:
+  url: http://service:3441/bbb
+postgresql:
+  password:
+    _default: "1234"
+`),
 
 		Entry("yaml with anchors", `
 common_values: &common-values

diff --git a/pkg/secret/yaml_helpers.go b/pkg/secret/yaml_helpers.go
@@ -41,14 +41,22 @@ func MergeEncodedYaml(oldData, newData, oldEncodedData, newEncodedData []byte) (
 }
 
 func MergeEncodedYamlNode(oldConfig, newConfig, oldEncodedConfig, newEncodedConfig *yaml_v3.Node) (*yaml_v3.Node, error) {
+	if oldConfig == nil {
+		return newEncodedConfig, nil
+	}
 	if newConfig.Kind != oldConfig.Kind {
 		return newEncodedConfig, nil
 	}
 
 	switch newEncodedConfig.Kind {
 	case yaml_v3.DocumentNode:
 		for pos := 0; pos < len(newEncodedConfig.Content); pos += 1 {
-			newValueNode, err := MergeEncodedYamlNode(oldConfig.Content[pos], newConfig.Content[pos], oldEncodedConfig.Content[pos], newEncodedConfig.Content[pos])
+			newEncodedValue := newEncodedConfig.Content[pos]
+			newValue := newConfig.Content[pos]
+			oldEncodedValue := getSubNodeByIndex(oldEncodedConfig, pos)
+			oldValue := getSubNodeByIndex(oldConfig, pos)
+
+			newValueNode, err := MergeEncodedYamlNode(oldValue, newValue, oldEncodedValue, newEncodedValue)
 			if err != nil {
 				return nil, fmt.Errorf("unable to process document key %d: %w", pos, err)
 			}
@@ -57,7 +65,14 @@ func MergeEncodedYamlNode(oldConfig, newConfig, oldEncodedConfig, newEncodedConf
 
 	case yaml_v3.MappingNode:
 		for pos := 0; pos < len(newEncodedConfig.Content); pos += 2 {
-			newValueNode, err := MergeEncodedYamlNode(oldConfig.Content[pos+1], newConfig.Content[pos+1], oldEncodedConfig.Content[pos+1], newEncodedConfig.Content[pos+1])
+			newKey := newEncodedConfig.Content[pos]
+
+			newEncodedValue := newEncodedConfig.Content[pos+1]
+			newValue := newConfig.Content[pos+1]
+			oldEncodedValue := getSubNodeByKey(oldEncodedConfig, newKey.Value)
+			oldValue := getSubNodeByKey(oldConfig, newKey.Value)
+
+			newValueNode, err := MergeEncodedYamlNode(oldValue, newValue, oldEncodedValue, newEncodedValue)
 			if err != nil {
 				return nil, fmt.Errorf("unable to process map key %q: %w", newEncodedConfig.Content[pos].Value, err)
 			}
@@ -66,7 +81,12 @@ func MergeEncodedYamlNode(oldConfig, newConfig, oldEncodedConfig, newEncodedConf
 
 	case yaml_v3.SequenceNode:
 		for pos := 0; pos < len(newEncodedConfig.Content); pos += 1 {
-			newValueNode, err := MergeEncodedYamlNode(oldConfig.Content[pos], newConfig.Content[pos], oldEncodedConfig.Content[pos], newEncodedConfig.Content[pos])
+			newEncodedValue := newEncodedConfig.Content[pos]
+			newValue := newConfig.Content[pos]
+			oldEncodedValue := getSubNodeByIndex(oldEncodedConfig, pos)
+			oldValue := getSubNodeByIndex(oldConfig, pos)
+
+			newValueNode, err := MergeEncodedYamlNode(oldValue, newValue, oldEncodedValue, newEncodedValue)
 			if err != nil {
 				return nil, fmt.Errorf("unable to process array key %d: %w", pos, err)
 			}
@@ -89,3 +109,20 @@ func MergeEncodedYamlNode(oldConfig, newConfig, oldEncodedConfig, newEncodedConf
 
 	return newEncodedConfig, nil
 }
+
+func getSubNodeByIndex(node *yaml_v3.Node, ind int) *yaml_v3.Node {
+	if ind < len(node.Content) {
+		return node.Content[ind]
+	}
+	return nil
+}
+
+func getSubNodeByKey(node *yaml_v3.Node, rawKey string) *yaml_v3.Node {
+	for i := 0; i < len(node.Content); i += 2 {
+		k, v := node.Content[i], node.Content[i+1]
+		if k.Value == rawKey {
+			return v
+		}
+	}
+	return nil
+}
diff --git a/pkg/secret/yaml_helpers_test.go b/pkg/secret/yaml_helpers_test.go
@@ -199,6 +199,283 @@ values:
   key4: value4-changed-encoded1
   # include common list
   key5: *common-list
+`),
+		}),
+
+		Entry("added elements into map and array", MergeEncodedYamlTest{
+			OldData: []byte(`
+database:
+  user: vasya
+  password: gfhjkm
+hosts:
+  - name: local1
+    address: a1
+  - name: local2
+    address: a2
+`),
+			OldEncodedData: []byte(`
+database:
+  user: enc1
+  password: enc2
+hosts:
+  - name: enc3
+    address: enc4
+  - name: enc5
+    address: enc6
+`),
+			NewData: []byte(`
+database:
+  user: vasya
+  password: gfhjkm
+  admin: petya
+  adminPassword:
+    _default: "1234"
+    production: "#@$213sldfzjxXFLKJSdf1233s"
+hosts:
+  - name: local1
+    address: a1
+    options:
+      timeout: 5s
+  - name: local2
+    address: a2
+`),
+			NewEncodedData: []byte(`
+database:
+  user: enc1-1
+  password: enc2-1
+  admin: enc3-1
+  adminPassword:
+    _default: enc4-1
+    production: enc5-1
+hosts:
+  - name: enc6-1
+    address: enc7-1
+    options:
+      timeout: enc8-1
+  - name: enc9-1
+    address: enc10-1
+`),
+			ExpectedResult: []byte(`
+database:
+  user: enc1
+  password: enc2
+  admin: enc3-1
+  adminPassword:
+    _default: enc4-1
+    production: enc5-1
+hosts:
+  - name: enc3
+    address: enc4
+    options:
+      timeout: enc8-1
+  - name: enc5
+    address: enc6
+`),
+		}),
+
+		Entry("removed elements from map and array", MergeEncodedYamlTest{
+			OldData: []byte(`
+database:
+  user: vasya
+  password: gfhjkm
+  admin: petya
+  adminPassword:
+    _default: "1234"
+    production: "#@$213sldfzjxXFLKJSdf1233s"
+hosts:
+  - name: local1
+    address: a1
+    options:
+      timeout: 5s
+  - name: local2
+    address: a2
+`),
+			OldEncodedData: []byte(`
+database:
+  user: enc1
+  password: enc2
+  admin: enc3
+  adminPassword:
+    _default: enc4
+    production: enc5
+hosts:
+  - name: enc6
+    address: enc7
+    options:
+      timeout: enc8
+  - name: enc9
+    address: enc10
+`),
+			NewData: []byte(`
+database:
+  user: vasya
+  adminPassword:
+    _default: "1234"
+    production: "#@$213sldfzjxXFLKJSdf1233s"
+hosts:
+  - name: local2
+    address: a2
+`),
+			NewEncodedData: []byte(`
+database:
+  user: enc1-1
+  adminPassword:
+    _default: enc2-1
+    production: enc3-1
+hosts:
+  - name: enc4-1
+    address: enc5-1
+`),
+			ExpectedResult: []byte(`
+database:
+  user: enc1
+  adminPassword:
+    _default: enc4
+    production: enc5
+hosts:
+  - name: enc4-1
+    address: enc5-1
+`),
+		}),
+
+		Entry("no changes in array", MergeEncodedYamlTest{
+			OldData: []byte(`
+arr:
+  - k1: v1
+    k2: v2
+  - k1: v3
+    k2: v4
+  - k1: v5
+    k2: v6
+`),
+			OldEncodedData: []byte(`
+arr:
+  - k1: enc1
+    k2: enc2
+  - k1: enc3
+    k2: enc4
+  - k1: enc5
+    k2: enc6
+`),
+			NewData: []byte(`
+arr:
+  - k1: v1
+    k2: v2
+  - k1: v3
+    k2: v4
+  - k1: v5
+    k2: v6
+`),
+			NewEncodedData: []byte(`
+arr:
+  - k1: enc1-1
+    k2: enc2-1
+  - k1: enc3-1
+    k2: enc4-1
+  - k1: enc5-1
+    k2: enc6-1
+`),
+			ExpectedResult: []byte(`
+arr:
+  - k1: enc1
+    k2: enc2
+  - k1: enc3
+    k2: enc4
+  - k1: enc5
+    k2: enc6
+`),
+		}),
+
+		Entry("random crazy mindless changes: change order of array elements, swap map keys with different values, change order of map values", MergeEncodedYamlTest{
+			OldData: []byte(`
+database:
+  user: vasya
+  password: gfhjkm
+  admin: petya
+  adminPassword:
+    _default: "1234"
+    production: "#@$213sldfzjxXFLKJSdf1233s"
+hosts:
+  - name: local0
+    address: a0
+  - name: local1
+    address: a1
+    options:
+      timeout: 5s
+  - name: local2
+    address: a2
+`),
+			OldEncodedData: []byte(`
+database:
+  user: enc1
+  password: enc2
+  admin: enc3
+  adminPassword:
+    _default: enc4
+    production: enc5
+hosts:
+  - name: enc6
+    address: enc7
+  - name: enc8
+    address: enc9
+    options:
+      timeout: enc10
+  - name: enc11
+    address: enc12
+`),
+			NewData: []byte(`
+database:
+  password: gfhjkm
+  user: vasya
+  adminPassword: petya
+  admin:
+    _default: "1234"
+    production: "#@$213sldfzjxXFLKJSdf1233s"
+hosts:
+  - name: local0
+    address: new-addr
+  - name: local2
+    address: a2
+  - name: local1
+    address: a1
+    options:
+      timeout: 5s
+`),
+			NewEncodedData: []byte(`
+database:
+  password: enc1-1
+  user: enc2-1
+  adminPassword: enc3-1
+  admin:
+    _default: enc4-1
+    production: enc5-1
+hosts:
+  - name: enc6-1
+    address: enc7-1
+  - name: enc8-1
+    address: enc9-1
+  - name: enc10-1
+    address: enc11-1
+    options:
+      timeout: enc12-1
+`),
+			ExpectedResult: []byte(`
+database:
+  password: enc2
+  user: enc1
+  adminPassword: enc3-1
+  admin:
+    _default: enc4-1
+    production: enc5-1
+hosts:
+  - name: enc6
+    address: enc7-1
+  - name: enc8-1
+    address: enc9-1
+  - name: enc10-1
+    address: enc11-1
+    options:
+      timeout: enc12-1
 `),
 		}),
 	)