From a65f6fda92f6b97d958fb35e87591ef6c8fb8cfd Mon Sep 17 00:00:00 2001 From: "ricardo.bartels@telekom.de" Date: Fri, 19 Jul 2024 09:06:27 +0200 Subject: [PATCH] mitigate issue with chart validation in Helm 3.14 #1515 Signed-off-by: ricardo.bartels@telekom.de --- internal/helm/chart/builder_remote_test.go | 1 + internal/helm/repository/chart_repository.go | 33 +++- .../helm/repository/chart_repository_test.go | 148 +++++++++++++++++- internal/helm/testdata/chartmuseum-index.json | 30 ++++ internal/helm/testdata/chartmuseum-index.yaml | 16 ++ .../helm/testdata/local-index-unordered.yaml | 16 ++ internal/helm/testdata/local-index.yaml | 16 ++ 7 files changed, 256 insertions(+), 4 deletions(-) diff --git a/internal/helm/chart/builder_remote_test.go b/internal/helm/chart/builder_remote_test.go index d43966dc3..ebe31ae3a 100644 --- a/internal/helm/chart/builder_remote_test.go +++ b/internal/helm/chart/builder_remote_test.go @@ -99,6 +99,7 @@ entries: - https://example.com/grafana.tgz description: string version: 6.17.4 + name: grafana `) mockGetter := &mockIndexChartGetter{ diff --git a/internal/helm/repository/chart_repository.go b/internal/helm/repository/chart_repository.go index bb279713e..9837224f4 100644 --- a/internal/helm/repository/chart_repository.go +++ b/internal/helm/repository/chart_repository.go @@ -28,6 +28,7 @@ import ( "os" "path" "sort" + "strings" "sync" "github.com/Masterminds/semver/v3" @@ -86,18 +87,24 @@ func IndexFromBytes(b []byte) (*repo.IndexFile, error) { return nil, repo.ErrNoAPIVersion } - for _, cvs := range i.Entries { + for name, cvs := range i.Entries { for idx := len(cvs) - 1; idx >= 0; idx-- { if cvs[idx] == nil { continue } + // When metadata section missing, initialize with no data + if cvs[idx].Metadata == nil { + cvs[idx].Metadata = &chart.Metadata{} + } if cvs[idx].APIVersion == "" { cvs[idx].APIVersion = chart.APIVersionV1 } - if err := cvs[idx].Validate(); err != nil { + if err := cvs[idx].Validate(); ignoreSkippableChartValidationError(err) != nil { cvs = append(cvs[:idx], cvs[idx+1:]...) } } + // adjust slice to only contain a set of valid versions + i.Entries[name] = cvs } i.SortEntries() @@ -501,3 +508,25 @@ func jsonOrYamlUnmarshal(b []byte, i interface{}) error { } return yaml.UnmarshalStrict(b, i) } + +// ignoreSkippableChartValidationError inspect the given error and returns nil if +// the error isn't important for index loading +// +// In particular, charts may introduce validations that don't impact repository indexes +// And repository indexes may be generated by older/non-complient software, which doesn't +// conform to all validations. +// +// this code is taken from https://github.com/helm/helm/blob/v3.15.2/pkg/repo/index.go#L402 +func ignoreSkippableChartValidationError(err error) error { + verr, ok := err.(chart.ValidationError) + if !ok { + return err + } + + // https://github.com/helm/helm/issues/12748 (JFrog repository strips alias field from index) + if strings.HasPrefix(verr.Error(), "validation: more than one dependency with name or alias") { + return nil + } + + return err +} diff --git a/internal/helm/repository/chart_repository_test.go b/internal/helm/repository/chart_repository_test.go index 1fcf5682e..1b2f1c0fb 100644 --- a/internal/helm/repository/chart_repository_test.go +++ b/internal/helm/repository/chart_repository_test.go @@ -672,7 +672,7 @@ func verifyLocalIndex(t *testing.T, i *repo.IndexFile) { g := NewWithT(t) g.Expect(i.Entries).ToNot(BeNil()) - g.Expect(i.Entries).To(HaveLen(3), "expected 3 entries in index file") + g.Expect(i.Entries).To(HaveLen(4), "expected 4 entries in index file") alpine, ok := i.Entries["alpine"] g.Expect(ok).To(BeTrue(), "expected 'alpine' entry to exist") @@ -682,6 +682,10 @@ func verifyLocalIndex(t *testing.T, i *repo.IndexFile) { g.Expect(ok).To(BeTrue(), "expected 'nginx' entry to exist") g.Expect(nginx).To(HaveLen(2), "'nginx' should have 2 entries") + broken, ok := i.Entries["xChartWithDuplicateDependenciesAndMissingAlias"] + g.Expect(ok).To(BeTrue(), "expected 'xChartWithDuplicateDependenciesAndMissingAlias' entry to exist") + g.Expect(broken).To(HaveLen(1), "'xChartWithDuplicateDependenciesAndMissingAlias' should have 1 entries") + expects := []*repo.ChartVersion{ { Metadata: &chart.Metadata{ @@ -723,8 +727,24 @@ func verifyLocalIndex(t *testing.T, i *repo.IndexFile) { }, Digest: "sha256:1234567890abcdef", }, + { + Metadata: &chart.Metadata{ + Name: "xChartWithDuplicateDependenciesAndMissingAlias", + Description: "string", + Version: "1.2.3", + Keywords: []string{"broken", "still accepted"}, + Home: "https://example.com/something", + Dependencies: []*chart.Dependency{ + {Name: "kube-rbac-proxy", Version: "0.9.1"}, + }, + }, + URLs: []string{ + "https://kubernetes-charts.storage.googleapis.com/nginx-1.2.3.tgz", + }, + Digest: "sha256:1234567890abcdef", + }, } - tests := []*repo.ChartVersion{alpine[0], nginx[0], nginx[1]} + tests := []*repo.ChartVersion{alpine[0], nginx[0], nginx[1], broken[0]} for i, tt := range tests { expect := expects[i] @@ -735,5 +755,129 @@ func verifyLocalIndex(t *testing.T, i *repo.IndexFile) { g.Expect(tt.Home).To(Equal(expect.Home)) g.Expect(tt.URLs).To(ContainElements(expect.URLs)) g.Expect(tt.Keywords).To(ContainElements(expect.Keywords)) + g.Expect(tt.Dependencies).To(ContainElements(expect.Dependencies)) + } +} + +// This code is taken from https://github.com/helm/helm/blob/v3.15.2/pkg/repo/index_test.go#L601 +// and refers to: https://github.com/helm/helm/issues/12748 +func TestIgnoreSkippableChartValidationError(t *testing.T) { + type TestCase struct { + Input error + ErrorSkipped bool + } + testCases := map[string]TestCase{ + "nil": { + Input: nil, + }, + "generic_error": { + Input: fmt.Errorf("foo"), + }, + "non_skipped_validation_error": { + Input: chart.ValidationError("chart.metadata.type must be application or library"), + }, + "skipped_validation_error": { + Input: chart.ValidationErrorf("more than one dependency with name or alias %q", "foo"), + ErrorSkipped: true, + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + result := ignoreSkippableChartValidationError(tc.Input) + + if tc.Input == nil { + if result != nil { + t.Error("expected nil result for nil input") + } + return + } + + if tc.ErrorSkipped { + if result != nil { + t.Error("expected nil result for skipped error") + } + return + } + + if tc.Input != result { + t.Error("expected the result equal to input") + } + + }) + } +} + +var indexWithFirstVersionInvalid = ` +apiVersion: v1 +entries: + nginx: + - urls: + - https://charts.helm.sh/stable/alpine-1.0.0.tgz + - http://storage2.googleapis.com/kubernetes-charts/alpine-1.0.0.tgz + name: nginx + version: 0..1.0 + description: string + home: https://github.com/something + digest: "sha256:1234567890abcdef" + - urls: + - https://charts.helm.sh/stable/nginx-0.2.0.tgz + name: nginx + description: string + version: 0.2.0 + home: https://github.com/something/else + digest: "sha256:1234567890abcdef" +` +var indexWithLastVersionInvalid = ` +apiVersion: v1 +entries: + nginx: + - urls: + - https://charts.helm.sh/stable/nginx-0.2.0.tgz + name: nginx + description: string + version: 0.2.0 + home: https://github.com/something/else + digest: "sha256:1234567890abcdef" + - urls: + - https://charts.helm.sh/stable/alpine-1.0.0.tgz + - http://storage2.googleapis.com/kubernetes-charts/alpine-1.0.0.tgz + name: nginx + version: 0..1.0 + description: string + home: https://github.com/something + digest: "sha256:1234567890abcdef" +` + +func TestIndexFromBytes_InvalidEntries(t *testing.T) { + tests := []struct { + source string + data string + }{ + { + source: "indexWithFirstVersionInvalid", + data: indexWithFirstVersionInvalid, + }, + { + source: "indexWithLastVersionInvalid", + data: indexWithLastVersionInvalid, + }, + } + for _, tc := range tests { + t.Run(tc.source, func(t *testing.T) { + idx, err := IndexFromBytes([]byte(tc.data)) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + cvs := idx.Entries["nginx"] + if len(cvs) == 0 { + t.Error("expected one chart version not to be filtered out") + } + for _, v := range cvs { + if v.Version == "0..1.0" { + t.Error("malformed version was not filtered out") + } + } + }) } } diff --git a/internal/helm/testdata/chartmuseum-index.json b/internal/helm/testdata/chartmuseum-index.json index 745617e30..15ba3e704 100644 --- a/internal/helm/testdata/chartmuseum-index.json +++ b/internal/helm/testdata/chartmuseum-index.json @@ -77,6 +77,36 @@ "created": "0001-01-01T00:00:00Z", "digest": "sha256:1234567890abcdef" } + ], + "xChartWithDuplicateDependenciesAndMissingAlias": [ + { + "name": "xChartWithDuplicateDependenciesAndMissingAlias", + "home": "https://example.com/something", + "version": "1.2.3", + "description": "string", + "keywords": [ + "broken", + "still accepted" + ], + "apiVersion": "v1", + "dependencies": [ + { + "name": "kube-rbac-proxy", + "version": "0.9.1", + "repository": "" + }, + { + "name": "kube-rbac-proxy", + "version": "0.9.1", + "repository": "" + } + ], + "urls": [ + "https://kubernetes-charts.storage.googleapis.com/nginx-1.2.3.tgz" + ], + "created": "0001-01-01T00:00:00Z", + "digest": "sha256:1234567890abcdef" + } ] } } diff --git a/internal/helm/testdata/chartmuseum-index.yaml b/internal/helm/testdata/chartmuseum-index.yaml index 3077596f4..ab00c1807 100644 --- a/internal/helm/testdata/chartmuseum-index.yaml +++ b/internal/helm/testdata/chartmuseum-index.yaml @@ -48,3 +48,19 @@ entries: - small - sumtin digest: "sha256:1234567890abcdef" + xChartWithDuplicateDependenciesAndMissingAlias: + - name: xChartWithDuplicateDependenciesAndMissingAlias + description: string + version: 1.2.3 + home: https://example.com/something + keywords: + - broken + - still accepted + urls: + - https://kubernetes-charts.storage.googleapis.com/nginx-1.2.3.tgz + digest: "sha256:1234567890abcdef" + dependencies: + - name: kube-rbac-proxy + version: "0.9.1" + - name: kube-rbac-proxy + version: "0.9.1" diff --git a/internal/helm/testdata/local-index-unordered.yaml b/internal/helm/testdata/local-index-unordered.yaml index 7482baaae..91ad62f1e 100644 --- a/internal/helm/testdata/local-index-unordered.yaml +++ b/internal/helm/testdata/local-index-unordered.yaml @@ -46,3 +46,19 @@ entries: - small - sumtin digest: "sha256:1234567890abcdef" + xChartWithDuplicateDependenciesAndMissingAlias: + - name: xChartWithDuplicateDependenciesAndMissingAlias + description: string + version: 1.2.3 + home: https://example.com/something + keywords: + - broken + - still accepted + urls: + - https://kubernetes-charts.storage.googleapis.com/nginx-1.2.3.tgz + digest: "sha256:1234567890abcdef" + dependencies: + - name: kube-rbac-proxy + version: "0.9.1" + - name: kube-rbac-proxy + version: "0.9.1" diff --git a/internal/helm/testdata/local-index.yaml b/internal/helm/testdata/local-index.yaml index e680d2a3e..56c0ac2c3 100644 --- a/internal/helm/testdata/local-index.yaml +++ b/internal/helm/testdata/local-index.yaml @@ -46,3 +46,19 @@ entries: - small - sumtin digest: "sha256:1234567890abcdef" + xChartWithDuplicateDependenciesAndMissingAlias: + - name: xChartWithDuplicateDependenciesAndMissingAlias + description: string + version: 1.2.3 + home: https://example.com/something + keywords: + - broken + - still accepted + urls: + - https://kubernetes-charts.storage.googleapis.com/nginx-1.2.3.tgz + digest: "sha256:1234567890abcdef" + dependencies: + - name: kube-rbac-proxy + version: "0.9.1" + - name: kube-rbac-proxy + version: "0.9.1"