From 0b71f15a62516566a0aeb28ea8e9a43ce4d4c7cd Mon Sep 17 00:00:00 2001 From: aadesh Date: Mon, 13 Feb 2023 21:09:25 -0500 Subject: [PATCH] [chart/loader] use strict yaml unmarshaling for chart files This updates the helm chart loader to use strict yaml unmarshaling (UnmarshalStrict) instead of the non-strict mode (Unmarshal). The main benefit of this is to prevent duplicate keys within the Values.yaml file which seems like that would never be intentionally done by the user. Right now if there are duplicate keys within Values.yaml, the last one will be used and the other ones will be silently ignored. Signed-off-by: Aadesh Patel --- pkg/chart/loader/load.go | 10 +++---- pkg/chart/loader/load_test.go | 11 +++++++ .../frobnitz_with_duplicate_keys/.helmignore | 1 + .../frobnitz_with_duplicate_keys/Chart.lock | 8 ++++++ .../frobnitz_with_duplicate_keys/Chart.yaml | 27 ++++++++++++++++++ .../frobnitz_with_duplicate_keys/INSTALL.txt | 1 + .../frobnitz_with_duplicate_keys/LICENSE | 1 + .../frobnitz_with_duplicate_keys/README.md | 11 +++++++ .../charts/_ignore_me | 1 + .../charts/alpine/Chart.yaml | 5 ++++ .../charts/alpine/README.md | 9 ++++++ .../charts/alpine/charts/mast1/Chart.yaml | 5 ++++ .../charts/alpine/charts/mast1/values.yaml | 4 +++ .../charts/alpine/charts/mast2-0.1.0.tgz | Bin 0 -> 252 bytes .../charts/alpine/templates/alpine-pod.yaml | 14 +++++++++ .../charts/alpine/values.yaml | 2 ++ .../charts/mariner-4.3.2.tgz | Bin 0 -> 967 bytes .../docs/README.md | 1 + .../frobnitz_with_duplicate_keys/icon.svg | 8 ++++++ .../ignore/me.txt | 0 .../templates/template.tpl | 1 + .../frobnitz_with_duplicate_keys/values.yaml | 9 ++++++ 22 files changed, 124 insertions(+), 5 deletions(-) create mode 100644 pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/.helmignore create mode 100644 pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/Chart.lock create mode 100644 pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/Chart.yaml create mode 100644 pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/INSTALL.txt create mode 100644 pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/LICENSE create mode 100644 pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/README.md create mode 100644 pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/_ignore_me create mode 100644 pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/alpine/Chart.yaml create mode 100644 pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/alpine/README.md create mode 100644 pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/alpine/charts/mast1/Chart.yaml create mode 100644 pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/alpine/charts/mast1/values.yaml create mode 100644 pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/alpine/charts/mast2-0.1.0.tgz create mode 100644 pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/alpine/templates/alpine-pod.yaml create mode 100644 pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/alpine/values.yaml create mode 100644 pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/mariner-4.3.2.tgz create mode 100644 pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/docs/README.md create mode 100644 pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/icon.svg create mode 100644 pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/ignore/me.txt create mode 100644 pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/templates/template.tpl create mode 100644 pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/values.yaml diff --git a/pkg/chart/loader/load.go b/pkg/chart/loader/load.go index 7cc8878a8..de4e95040 100644 --- a/pkg/chart/loader/load.go +++ b/pkg/chart/loader/load.go @@ -81,7 +81,7 @@ func LoadFiles(files []*BufferedFile) (*chart.Chart, error) { if c.Metadata == nil { c.Metadata = new(chart.Metadata) } - if err := yaml.Unmarshal(f.Data, c.Metadata); err != nil { + if err := yaml.UnmarshalStrict(f.Data, c.Metadata); err != nil { return c, errors.Wrap(err, "cannot load Chart.yaml") } // NOTE(bacongobbler): while the chart specification says that APIVersion must be set, @@ -99,12 +99,12 @@ func LoadFiles(files []*BufferedFile) (*chart.Chart, error) { continue case f.Name == "Chart.lock": c.Lock = new(chart.Lock) - if err := yaml.Unmarshal(f.Data, &c.Lock); err != nil { + if err := yaml.UnmarshalStrict(f.Data, &c.Lock); err != nil { return c, errors.Wrap(err, "cannot load Chart.lock") } case f.Name == "values.yaml": c.Values = make(map[string]interface{}) - if err := yaml.Unmarshal(f.Data, &c.Values); err != nil { + if err := yaml.UnmarshalStrict(f.Data, &c.Values); err != nil { return c, errors.Wrap(err, "cannot load values.yaml") } case f.Name == "values.schema.json": @@ -119,7 +119,7 @@ func LoadFiles(files []*BufferedFile) (*chart.Chart, error) { if c.Metadata.APIVersion != chart.APIVersionV1 { log.Printf("Warning: Dependencies are handled in Chart.yaml since apiVersion \"v2\". We recommend migrating dependencies to Chart.yaml.") } - if err := yaml.Unmarshal(f.Data, c.Metadata); err != nil { + if err := yaml.UnmarshalStrict(f.Data, c.Metadata); err != nil { return c, errors.Wrap(err, "cannot load requirements.yaml") } if c.Metadata.APIVersion == chart.APIVersionV1 { @@ -128,7 +128,7 @@ func LoadFiles(files []*BufferedFile) (*chart.Chart, error) { // Deprecated: requirements.lock is deprecated use Chart.lock. case f.Name == "requirements.lock": c.Lock = new(chart.Lock) - if err := yaml.Unmarshal(f.Data, &c.Lock); err != nil { + if err := yaml.UnmarshalStrict(f.Data, &c.Lock); err != nil { return c, errors.Wrap(err, "cannot load requirements.lock") } if c.Metadata == nil { diff --git a/pkg/chart/loader/load_test.go b/pkg/chart/loader/load_test.go index a737098b4..27f25644f 100644 --- a/pkg/chart/loader/load_test.go +++ b/pkg/chart/loader/load_test.go @@ -489,6 +489,17 @@ func TestLoadInvalidArchive(t *testing.T) { } } +func TestLoadWithDuplicateKeysInValuesFile(t *testing.T) { + l, err := Loader("testdata/frobnitz_with_duplicate_keys") + if err != nil { + t.Fatalf("Failed to load testdata: %s", err) + } + + if _, err = l.Load(); err == nil { + t.Errorf("packages with a value yaml file with duplicate keys should not load") + } +} + func verifyChart(t *testing.T, c *chart.Chart) { t.Helper() if c.Name() == "" { diff --git a/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/.helmignore b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/.helmignore new file mode 100644 index 000000000..9973a57b8 --- /dev/null +++ b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/.helmignore @@ -0,0 +1 @@ +ignore/ diff --git a/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/Chart.lock b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/Chart.lock new file mode 100644 index 000000000..6fcc2ed9f --- /dev/null +++ b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/Chart.lock @@ -0,0 +1,8 @@ +dependencies: + - name: alpine + version: "0.1.0" + repository: https://example.com/charts + - name: mariner + version: "4.3.2" + repository: https://example.com/charts +digest: invalid diff --git a/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/Chart.yaml b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/Chart.yaml new file mode 100644 index 000000000..fcd4a4a37 --- /dev/null +++ b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/Chart.yaml @@ -0,0 +1,27 @@ +apiVersion: v1 +name: frobnitz +description: This is a frobnitz. +version: "1.2.3" +keywords: + - frobnitz + - sprocket + - dodad +maintainers: + - name: The Helm Team + email: helm@example.com + - name: Someone Else + email: nobody@example.com +sources: + - https://example.com/foo/bar +home: http://example.com +icon: https://example.com/64x64.png +annotations: + extrakey: extravalue + anotherkey: anothervalue +dependencies: + - name: alpine + version: "0.1.0" + repository: https://example.com/charts + - name: mariner + version: "4.3.2" + repository: https://example.com/charts diff --git a/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/INSTALL.txt b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/INSTALL.txt new file mode 100644 index 000000000..2010438c2 --- /dev/null +++ b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/INSTALL.txt @@ -0,0 +1 @@ +This is an install document. The client may display this. diff --git a/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/LICENSE b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/LICENSE new file mode 100644 index 000000000..6121943b1 --- /dev/null +++ b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/LICENSE @@ -0,0 +1 @@ +LICENSE placeholder. diff --git a/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/README.md b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/README.md new file mode 100644 index 000000000..8cf4cc3d7 --- /dev/null +++ b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/README.md @@ -0,0 +1,11 @@ +# Frobnitz + +This is an example chart. + +## Usage + +This is an example. It has no usage. + +## Development + +For developer info, see the top-level repository. diff --git a/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/_ignore_me b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/_ignore_me new file mode 100644 index 000000000..2cecca682 --- /dev/null +++ b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/_ignore_me @@ -0,0 +1 @@ +This should be ignored by the loader, but may be included in a chart. diff --git a/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/alpine/Chart.yaml b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/alpine/Chart.yaml new file mode 100644 index 000000000..79e0d65db --- /dev/null +++ b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/alpine/Chart.yaml @@ -0,0 +1,5 @@ +apiVersion: v1 +name: alpine +description: Deploy a basic Alpine Linux pod +version: 0.1.0 +home: https://helm.sh/helm diff --git a/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/alpine/README.md b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/alpine/README.md new file mode 100644 index 000000000..b30b949dd --- /dev/null +++ b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/alpine/README.md @@ -0,0 +1,9 @@ +This example was generated using the command `helm create alpine`. + +The `templates/` directory contains a very simple pod resource with a +couple of parameters. + +The `values.toml` file contains the default values for the +`alpine-pod.yaml` template. + +You can install this example using `helm install ./alpine`. diff --git a/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/alpine/charts/mast1/Chart.yaml b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/alpine/charts/mast1/Chart.yaml new file mode 100644 index 000000000..1c9dd5fa4 --- /dev/null +++ b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/alpine/charts/mast1/Chart.yaml @@ -0,0 +1,5 @@ +apiVersion: v1 +name: mast1 +description: A Helm chart for Kubernetes +version: 0.1.0 +home: "" diff --git a/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/alpine/charts/mast1/values.yaml b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/alpine/charts/mast1/values.yaml new file mode 100644 index 000000000..42c39c262 --- /dev/null +++ b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/alpine/charts/mast1/values.yaml @@ -0,0 +1,4 @@ +# Default values for mast1. +# This is a YAML-formatted file. +# Declare name/value pairs to be passed into your templates. +# name = "value" diff --git a/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/alpine/charts/mast2-0.1.0.tgz b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/alpine/charts/mast2-0.1.0.tgz new file mode 100644 index 0000000000000000000000000000000000000000..61cb62051110b55f3d08213dc81dcf0b1c2d8e53 GIT binary patch literal 252 zcmVDc zVQyr3R8em|NM&qo0PNJUs=_c72H?(liabH@pWIst-7YSIyL+rhEHrIN(t?QZE=F{y zgNRfS&$pa5Ly`mMk2OB%pV`*9knW7FlL-Joo@KED7*{C$d;N~z z37$S{+}wvSU9}|VtF|fRpv9Ve>8dWo|9?5B+RE}Y9CFh-x#(Bq8Vck^V=NUiPLCKa z8z5CF#JgK!4>;$4Fm+FUst4d+{(+nP|0&J+e}(;l^U4@w-{=?s0RR8vgVbLD3;+OM Cs&R<` literal 0 HcmV?d00001 diff --git a/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/alpine/templates/alpine-pod.yaml b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/alpine/templates/alpine-pod.yaml new file mode 100644 index 000000000..21ae20aad --- /dev/null +++ b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/alpine/templates/alpine-pod.yaml @@ -0,0 +1,14 @@ +apiVersion: v1 +kind: Pod +metadata: + name: {{.Release.Name}}-{{.Chart.Name}} + labels: + app.kubernetes.io/managed-by: {{.Release.Service}} + app.kubernetes.io/name: {{.Chart.Name}} + helm.sh/chart: "{{.Chart.Name}}-{{.Chart.Version}}" +spec: + restartPolicy: {{default "Never" .restart_policy}} + containers: + - name: waiter + image: "alpine:3.9" + command: ["/bin/sleep","9000"] diff --git a/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/alpine/values.yaml b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/alpine/values.yaml new file mode 100644 index 000000000..6c2aab7ba --- /dev/null +++ b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/alpine/values.yaml @@ -0,0 +1,2 @@ +# The pod name +name: "my-alpine" diff --git a/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/mariner-4.3.2.tgz b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/charts/mariner-4.3.2.tgz new file mode 100644 index 0000000000000000000000000000000000000000..3190136b050e62c628b3c817fd963ac9dc4a9e25 GIT binary patch literal 967 zcmV;&133I2iwFR+9h6)E1MQb_y%a`Bd>#= zhbqf2w!b6}wZ9@rvIhtwzm?~C&+QLm+G2!l%`$_aUgS(@pdjdX3a%E}VXVc7`)dg( zL%IRN2}c1D3xoMi2w@WuWOMZ?5i&3FelBVyq_Ce_8fREdP%NDf<&d!wu3{&VUQNs{N%vK$Ha~k^gB2!0bO7r0ic0 zbqCp*X#j?=|H@GNONsuE)&Idbh>2MX(Z}|+=@*sLod*wS?A8Ugp#mMPuMO0NnZmos9_rr z3xpDL+otj~lRh?B4hHFTl+d5-8NBXmUXB~EyF^bx7m*+!*g>obczvGF|8xkWsHN8; z%#+wiWP{=2pC*98@$VNzzsll&G#D7&11-;D>HT0x|DV2?6}a~*p46@R|2l??e_8dX z@Bb>D3t~VC_*wjq2Dy!6J-_5ME%%J+xmsbK5a#qO>ZV?Wt`d|TDdrB^B&LqFr@lYdUA zs&4-JAg3&uc4dA0gv*bXU9QePa9^8wR{HovA)j@)JOAIkak0Jl)aKqA_3XLM#?H=V z-{tlG=xy^os=1Z><53;&+C4=zp|%rwv!)UyY=%k_t%Y|wNRQl`C6N_Z&S;S+~wb1=)2Uh_4I81`y>DO zoLPSt=btB&x{CUK`0DA&){efW_O36RxnsYZNT0r;JbTLR{H4M3C$8(Cee==aQ~Gbq p$`5v3?NB{4-j0XQHf literal 0 HcmV?d00001 diff --git a/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/docs/README.md b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/docs/README.md new file mode 100644 index 000000000..d40747caf --- /dev/null +++ b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/docs/README.md @@ -0,0 +1 @@ +This is a placeholder for documentation. diff --git a/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/icon.svg b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/icon.svg new file mode 100644 index 000000000..892130606 --- /dev/null +++ b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/icon.svg @@ -0,0 +1,8 @@ + + + Example icon + + + diff --git a/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/ignore/me.txt b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/ignore/me.txt new file mode 100644 index 000000000..e69de29bb diff --git a/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/templates/template.tpl b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/templates/template.tpl new file mode 100644 index 000000000..c651ee6a0 --- /dev/null +++ b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/templates/template.tpl @@ -0,0 +1 @@ +Hello {{.Name | default "world"}} diff --git a/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/values.yaml b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/values.yaml new file mode 100644 index 000000000..8f4a081f3 --- /dev/null +++ b/pkg/chart/loader/testdata/frobnitz_with_duplicate_keys/values.yaml @@ -0,0 +1,9 @@ +# A values file contains configuration. + +name: "Some Name" + +section: + name: "Name in a section" + +section: + name: "Duplicate section name"