From 15c1acc0e181c769da25c48f4bde3f35852fed4f Mon Sep 17 00:00:00 2001 From: Beyhan Veli Date: Wed, 8 Dec 2021 11:11:57 +0100 Subject: [PATCH 1/6] Try to improve code readability of var_kv --- director/template/var_kv.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/director/template/var_kv.go b/director/template/var_kv.go index 5ea43233d..4121664bb 100644 --- a/director/template/var_kv.go +++ b/director/template/var_kv.go @@ -14,31 +14,32 @@ type VarKV struct { func (a *VarKV) UnmarshalFlag(data string) error { pieces := strings.SplitN(data, "=", 2) + name, value := 0, 1 if len(pieces) != 2 { return bosherr.Errorf("Expected var '%s' to be in format 'name=value'", data) } - if len(pieces[0]) == 0 { + if len(pieces[name]) == 0 { return bosherr.Errorf("Expected var '%s' to specify non-empty name", data) } - if len(pieces[1]) == 0 { + if len(pieces[value]) == 0 { return bosherr.Errorf("Expected var '%s' to specify non-empty value", data) } var vars interface{} - err := yaml.Unmarshal([]byte(pieces[1]), &vars) + err := yaml.Unmarshal([]byte(pieces[value]), &vars) if err != nil { return bosherr.WrapErrorf(err, "Deserializing variables '%s'", data) } //yaml.Unmarshal returns a string if the input is not valid yaml. - //in that case, we pass through the string itself as the Unmarshal process strips newlines. if _, ok := vars.(string); ok { - *a = VarKV{Name: pieces[0], Value: pieces[1]} + //in that case, we pass through the initial flag value as the Unmarshal process strips newlines. + *a = VarKV{Name: pieces[name], Value: pieces[value]} } else { - *a = VarKV{Name: pieces[0], Value: vars} + *a = VarKV{Name: pieces[name], Value: vars} } return nil From dfe27885c8881b4cc4439621c6d1fffca8d1f154 Mon Sep 17 00:00:00 2001 From: Beyhan Veli Date: Thu, 9 Dec 2021 15:52:50 +0100 Subject: [PATCH 2/6] Make index vars constast and give better names --- director/template/var_kv.go | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/director/template/var_kv.go b/director/template/var_kv.go index 4121664bb..f7f690634 100644 --- a/director/template/var_kv.go +++ b/director/template/var_kv.go @@ -14,32 +14,33 @@ type VarKV struct { func (a *VarKV) UnmarshalFlag(data string) error { pieces := strings.SplitN(data, "=", 2) - name, value := 0, 1 + const nameIndex, valueIndex = 0, 1 if len(pieces) != 2 { return bosherr.Errorf("Expected var '%s' to be in format 'name=value'", data) } - if len(pieces[name]) == 0 { + if len(pieces[nameIndex]) == 0 { return bosherr.Errorf("Expected var '%s' to specify non-empty name", data) } - if len(pieces[value]) == 0 { + if len(pieces[valueIndex]) == 0 { return bosherr.Errorf("Expected var '%s' to specify non-empty value", data) } var vars interface{} - err := yaml.Unmarshal([]byte(pieces[value]), &vars) + err := yaml.Unmarshal([]byte(pieces[valueIndex]), &vars) if err != nil { return bosherr.WrapErrorf(err, "Deserializing variables '%s'", data) } //yaml.Unmarshal returns a string if the input is not valid yaml. if _, ok := vars.(string); ok { - //in that case, we pass through the initial flag value as the Unmarshal process strips newlines. - *a = VarKV{Name: pieces[name], Value: pieces[value]} + //in that case, we return the initial flag value as the Unmarshal process strips newlines. + *a = VarKV{Name: pieces[nameIndex], Value: pieces[valueIndex]} } else { - *a = VarKV{Name: pieces[name], Value: vars} + //otherwise, return the parsed YAML object + *a = VarKV{Name: pieces[nameIndex], Value: vars} } return nil From 132dfc4d5871e8dc140a4769e0e00555581b2dd4 Mon Sep 17 00:00:00 2001 From: Konstantin Kiess Date: Fri, 10 Dec 2021 13:02:27 +0100 Subject: [PATCH 3/6] Add fix for values containing wrapping quotes if we skip YAML.unmarshal on the value we may end up with superfluos quotes around the actual value. This is mostly for backwards compability as it may break scripts/manifest that would currently render fine: e.g.: ``` + releases: + - name: bosh + version: "'271.17.0+dev.1638991106'" ``` vs ``` + releases: + - name: bosh + version: 271.17.0+dev.1638885656 ``` Signed-off-by: Brian Cunnie --- director/template/var_kv.go | 6 ++++-- director/template/var_kv_test.go | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/director/template/var_kv.go b/director/template/var_kv.go index f7f690634..02470fa04 100644 --- a/director/template/var_kv.go +++ b/director/template/var_kv.go @@ -36,8 +36,10 @@ func (a *VarKV) UnmarshalFlag(data string) error { //yaml.Unmarshal returns a string if the input is not valid yaml. if _, ok := vars.(string); ok { - //in that case, we return the initial flag value as the Unmarshal process strips newlines. - *a = VarKV{Name: pieces[nameIndex], Value: pieces[valueIndex]} + //in that case, we return the initial flag value as the Unmarshal + //process strips newlines. Stripping the quotes is required to keep + //backwards compability (YAML unmarshal also removed wrapping quotes from the value). + *a = VarKV{Name: pieces[nameIndex], Value: strings.Trim(pieces[valueIndex], `"'`)} } else { //otherwise, return the parsed YAML object *a = VarKV{Name: pieces[nameIndex], Value: vars} diff --git a/director/template/var_kv_test.go b/director/template/var_kv_test.go index 003630d97..e41baac2e 100644 --- a/director/template/var_kv_test.go +++ b/director/template/var_kv_test.go @@ -23,6 +23,25 @@ var _ = Describe("VarKV", func() { Expect(arg).To(Equal(VarKV{Name: "name", Value: "val"})) }) + It("reverts to the old (incorrect) bevaviour and removes the wrapping double quotes if the value is a string", func() { + err := (&arg).UnmarshalFlag(`name="val"`) + Expect(err).ToNot(HaveOccurred()) + Expect(arg).To(Equal(VarKV{Name: "name", Value: "val"})) + }) + It("reverts to the old (incorrect) bevaviour and removes the wrapping single quotes if the value is a string", func() { + err := (&arg).UnmarshalFlag(`name='val'`) + Expect(err).ToNot(HaveOccurred()) + Expect(arg).To(Equal(VarKV{Name: "name", Value: "val"})) + }) + It("Trim only removes wrapping quotes", func() { + err := (&arg).UnmarshalFlag(`name="'val""val'"`) + Expect(err).ToNot(HaveOccurred()) + Expect(arg).To(Equal(VarKV{Name: "name", Value: `val""val`})) + err = (&arg).UnmarshalFlag(`name="val''val"`) + Expect(err).ToNot(HaveOccurred()) + Expect(arg).To(Equal(VarKV{Name: "name", Value: `val''val`})) + }) + }) It("sets name and value when value contains a `=`", func() { err := (&arg).UnmarshalFlag("public_key=ssh-rsa G4/+VHa1aw==") Expect(err).ToNot(HaveOccurred()) From 3612f57d33a8ae1ef7241da84e7b4baae7252277 Mon Sep 17 00:00:00 2001 From: Beyhan Veli Date: Tue, 14 Dec 2021 08:35:33 +0100 Subject: [PATCH 4/6] Fix syntax issue in var_kv_test --- director/template/var_kv_test.go | 38 +++++++++++++++++--------------- 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/director/template/var_kv_test.go b/director/template/var_kv_test.go index e41baac2e..392623997 100644 --- a/director/template/var_kv_test.go +++ b/director/template/var_kv_test.go @@ -23,25 +23,27 @@ var _ = Describe("VarKV", func() { Expect(arg).To(Equal(VarKV{Name: "name", Value: "val"})) }) - It("reverts to the old (incorrect) bevaviour and removes the wrapping double quotes if the value is a string", func() { - err := (&arg).UnmarshalFlag(`name="val"`) - Expect(err).ToNot(HaveOccurred()) - Expect(arg).To(Equal(VarKV{Name: "name", Value: "val"})) - }) - It("reverts to the old (incorrect) bevaviour and removes the wrapping single quotes if the value is a string", func() { - err := (&arg).UnmarshalFlag(`name='val'`) - Expect(err).ToNot(HaveOccurred()) - Expect(arg).To(Equal(VarKV{Name: "name", Value: "val"})) - }) - It("Trim only removes wrapping quotes", func() { - err := (&arg).UnmarshalFlag(`name="'val""val'"`) - Expect(err).ToNot(HaveOccurred()) - Expect(arg).To(Equal(VarKV{Name: "name", Value: `val""val`})) - err = (&arg).UnmarshalFlag(`name="val''val"`) - Expect(err).ToNot(HaveOccurred()) - Expect(arg).To(Equal(VarKV{Name: "name", Value: `val''val`})) - }) + It("reverts to the old (incorrect) bevaviour and removes the wrapping double quotes if the value is a string", func() { + err := (&arg).UnmarshalFlag(`name="val"`) + Expect(err).ToNot(HaveOccurred()) + Expect(arg).To(Equal(VarKV{Name: "name", Value: "val"})) }) + + It("reverts to the old (incorrect) bevaviour and removes the wrapping single quotes if the value is a string", func() { + err := (&arg).UnmarshalFlag(`name='val'`) + Expect(err).ToNot(HaveOccurred()) + Expect(arg).To(Equal(VarKV{Name: "name", Value: "val"})) + }) + + It("Trim only removes wrapping quotes", func() { + err := (&arg).UnmarshalFlag(`name="'val""val'"`) + Expect(err).ToNot(HaveOccurred()) + Expect(arg).To(Equal(VarKV{Name: "name", Value: `val""val`})) + err = (&arg).UnmarshalFlag(`name="val''val"`) + Expect(err).ToNot(HaveOccurred()) + Expect(arg).To(Equal(VarKV{Name: "name", Value: `val''val`})) + }) + It("sets name and value when value contains a `=`", func() { err := (&arg).UnmarshalFlag("public_key=ssh-rsa G4/+VHa1aw==") Expect(err).ToNot(HaveOccurred()) From 1e2cdc1fa8d23a2e54d11b70457bf3439754a271 Mon Sep 17 00:00:00 2001 From: Konstantin Kiess Date: Tue, 14 Dec 2021 11:55:08 +0100 Subject: [PATCH 5/6] Trim() will remove unmatched leading/trailing quotes e.g. if a the value is: `"'Password"'` we should return `"'Password"'` thanks @klakin-pivotal for spotting this. --- director/template/var_kv.go | 13 ++++++++++--- director/template/var_kv_test.go | 14 ++++++++++---- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/director/template/var_kv.go b/director/template/var_kv.go index 02470fa04..0ea0ff16a 100644 --- a/director/template/var_kv.go +++ b/director/template/var_kv.go @@ -12,6 +12,13 @@ type VarKV struct { Value interface{} } +func trimWrappingQuotes(s string) string { + if (s[0] == '"' && s[len(s)-1] == '"') || (s[0] == '\'' && s[len(s)-1] == '\'') { + s = s[1 : len(s)-1] + s = trimWrappingQuotes(s) + } + return s +} func (a *VarKV) UnmarshalFlag(data string) error { pieces := strings.SplitN(data, "=", 2) const nameIndex, valueIndex = 0, 1 @@ -30,16 +37,16 @@ func (a *VarKV) UnmarshalFlag(data string) error { var vars interface{} err := yaml.Unmarshal([]byte(pieces[valueIndex]), &vars) + // If for whatever reason YAML unmarshal fails, treat the second part of pieces as a string if err != nil { - return bosherr.WrapErrorf(err, "Deserializing variables '%s'", data) + vars = pieces[valueIndex] } - //yaml.Unmarshal returns a string if the input is not valid yaml. if _, ok := vars.(string); ok { //in that case, we return the initial flag value as the Unmarshal //process strips newlines. Stripping the quotes is required to keep //backwards compability (YAML unmarshal also removed wrapping quotes from the value). - *a = VarKV{Name: pieces[nameIndex], Value: strings.Trim(pieces[valueIndex], `"'`)} + *a = VarKV{Name: pieces[nameIndex], Value: trimWrappingQuotes(pieces[valueIndex])} } else { //otherwise, return the parsed YAML object *a = VarKV{Name: pieces[nameIndex], Value: vars} diff --git a/director/template/var_kv_test.go b/director/template/var_kv_test.go index 392623997..1f4ccc604 100644 --- a/director/template/var_kv_test.go +++ b/director/template/var_kv_test.go @@ -28,22 +28,28 @@ var _ = Describe("VarKV", func() { Expect(err).ToNot(HaveOccurred()) Expect(arg).To(Equal(VarKV{Name: "name", Value: "val"})) }) - It("reverts to the old (incorrect) bevaviour and removes the wrapping single quotes if the value is a string", func() { err := (&arg).UnmarshalFlag(`name='val'`) Expect(err).ToNot(HaveOccurred()) Expect(arg).To(Equal(VarKV{Name: "name", Value: "val"})) }) - - It("Trim only removes wrapping quotes", func() { + It("only removes wrapping quotes", func() { err := (&arg).UnmarshalFlag(`name="'val""val'"`) Expect(err).ToNot(HaveOccurred()) Expect(arg).To(Equal(VarKV{Name: "name", Value: `val""val`})) + err = (&arg).UnmarshalFlag(`name="val''val"`) Expect(err).ToNot(HaveOccurred()) Expect(arg).To(Equal(VarKV{Name: "name", Value: `val''val`})) - }) + err = (&arg).UnmarshalFlag(`name='"some-data'"`) + Expect(err).ToNot(HaveOccurred()) + Expect(arg).To(Equal(VarKV{Name: "name", Value: `'"some-data'"`})) + + err = (&arg).UnmarshalFlag(`name="'some-data"'`) + Expect(err).ToNot(HaveOccurred()) + Expect(arg).To(Equal(VarKV{Name: "name", Value: `"'some-data"'`})) + }) It("sets name and value when value contains a `=`", func() { err := (&arg).UnmarshalFlag("public_key=ssh-rsa G4/+VHa1aw==") Expect(err).ToNot(HaveOccurred()) From ab3ef1778fe87d5e2faae66f7f349e2e94e8687e Mon Sep 17 00:00:00 2001 From: Konstantin Kiess Date: Tue, 14 Dec 2021 12:46:04 +0100 Subject: [PATCH 6/6] no need to reset the var, even with an error --- director/template/var_kv.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/director/template/var_kv.go b/director/template/var_kv.go index 0ea0ff16a..6a3225b1b 100644 --- a/director/template/var_kv.go +++ b/director/template/var_kv.go @@ -37,12 +37,9 @@ func (a *VarKV) UnmarshalFlag(data string) error { var vars interface{} err := yaml.Unmarshal([]byte(pieces[valueIndex]), &vars) - // If for whatever reason YAML unmarshal fails, treat the second part of pieces as a string - if err != nil { - vars = pieces[valueIndex] - } + //If for whatever reason YAML unmarshal fails, treat the second part of pieces as a string //yaml.Unmarshal returns a string if the input is not valid yaml. - if _, ok := vars.(string); ok { + if _, ok := vars.(string); ok || err != nil { //in that case, we return the initial flag value as the Unmarshal //process strips newlines. Stripping the quotes is required to keep //backwards compability (YAML unmarshal also removed wrapping quotes from the value).