From 289400d3aec3794dc310942fb81d0b3423843753 Mon Sep 17 00:00:00 2001 From: Timofey Kirillov Date: Fri, 1 Jul 2022 19:22:42 +0300 Subject: [PATCH] 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 --- pkg/secret/yaml_encoder_test.go | 9 +- pkg/secret/yaml_helpers.go | 43 ++++- pkg/secret/yaml_helpers_test.go | 277 ++++++++++++++++++++++++++++++++ 3 files changed, 325 insertions(+), 4 deletions(-) diff --git a/pkg/secret/yaml_encoder_test.go b/pkg/secret/yaml_encoder_test.go index 1cacb72d33..e9527dc8fb 100644 --- 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 index cc903a3d2e..2290b997fe 100644 --- a/pkg/secret/yaml_helpers.go +++ b/pkg/secret/yaml_helpers.go @@ -41,6 +41,9 @@ 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 } @@ -48,7 +51,12 @@ func MergeEncodedYamlNode(oldConfig, newConfig, oldEncodedConfig, newEncodedConf 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 index d72bb72e97..29f5d93525 100644 --- 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 `), }), )