Skip to content

Commit 218af57

Browse files
darkowlzzdarkowlzz
authored
Merge pull request #1516 from bb-Ricardo/main
Fix Helm index validation for Artifactory
2 parents 58b4e6d + a65f6fd commit 218af57

7 files changed

+256
-4
lines changed

‎internal/helm/chart/builder_remote_test.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,7 @@ entries:
9999
- https://example.com/grafana.tgz
100100
description: string
101101
version: 6.17.4
102+
name: grafana
102103
`)
103104

104105
mockGetter := &mockIndexChartGetter{

‎internal/helm/repository/chart_repository.go

Lines changed: 31 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ import (
2828
"os"
2929
"path"
3030
"sort"
31+
"strings"
3132
"sync"
3233

3334
"github.com/Masterminds/semver/v3"
@@ -86,18 +87,24 @@ func IndexFromBytes(b []byte) (*repo.IndexFile, error) {
8687
return nil, repo.ErrNoAPIVersion
8788
}
8889

89-
for _, cvs := range i.Entries {
90+
for name, cvs := range i.Entries {
9091
for idx := len(cvs) - 1; idx >= 0; idx-- {
9192
if cvs[idx] == nil {
9293
continue
9394
}
95+
// When metadata section missing, initialize with no data
96+
if cvs[idx].Metadata == nil {
97+
cvs[idx].Metadata = &chart.Metadata{}
98+
}
9499
if cvs[idx].APIVersion == "" {
95100
cvs[idx].APIVersion = chart.APIVersionV1
96101
}
97-
if err := cvs[idx].Validate(); err != nil {
102+
if err := cvs[idx].Validate(); ignoreSkippableChartValidationError(err) != nil {
98103
cvs = append(cvs[:idx], cvs[idx+1:]...)
99104
}
100105
}
106+
// adjust slice to only contain a set of valid versions
107+
i.Entries[name] = cvs
101108
}
102109

103110
i.SortEntries()
@@ -501,3 +508,25 @@ func jsonOrYamlUnmarshal(b []byte, i interface{}) error {
501508
}
502509
return yaml.UnmarshalStrict(b, i)
503510
}
511+
512+
// ignoreSkippableChartValidationError inspect the given error and returns nil if
513+
// the error isn't important for index loading
514+
//
515+
// In particular, charts may introduce validations that don't impact repository indexes
516+
// And repository indexes may be generated by older/non-complient software, which doesn't
517+
// conform to all validations.
518+
//
519+
// this code is taken from https://github.com/helm/helm/blob/v3.15.2/pkg/repo/index.go#L402
520+
func ignoreSkippableChartValidationError(err error) error {
521+
verr, ok := err.(chart.ValidationError)
522+
if !ok {
523+
return err
524+
}
525+
526+
// https://github.com/helm/helm/issues/12748 (JFrog repository strips alias field from index)
527+
if strings.HasPrefix(verr.Error(), "validation: more than one dependency with name or alias") {
528+
return nil
529+
}
530+
531+
return err
532+
}

‎internal/helm/repository/chart_repository_test.go

Lines changed: 146 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -672,7 +672,7 @@ func verifyLocalIndex(t *testing.T, i *repo.IndexFile) {
672672
g := NewWithT(t)
673673

674674
g.Expect(i.Entries).ToNot(BeNil())
675-
g.Expect(i.Entries).To(HaveLen(3), "expected 3 entries in index file")
675+
g.Expect(i.Entries).To(HaveLen(4), "expected 4 entries in index file")
676676

677677
alpine, ok := i.Entries["alpine"]
678678
g.Expect(ok).To(BeTrue(), "expected 'alpine' entry to exist")
@@ -682,6 +682,10 @@ func verifyLocalIndex(t *testing.T, i *repo.IndexFile) {
682682
g.Expect(ok).To(BeTrue(), "expected 'nginx' entry to exist")
683683
g.Expect(nginx).To(HaveLen(2), "'nginx' should have 2 entries")
684684

685+
broken, ok := i.Entries["xChartWithDuplicateDependenciesAndMissingAlias"]
686+
g.Expect(ok).To(BeTrue(), "expected 'xChartWithDuplicateDependenciesAndMissingAlias' entry to exist")
687+
g.Expect(broken).To(HaveLen(1), "'xChartWithDuplicateDependenciesAndMissingAlias' should have 1 entries")
688+
685689
expects := []*repo.ChartVersion{
686690
{
687691
Metadata: &chart.Metadata{
@@ -723,8 +727,24 @@ func verifyLocalIndex(t *testing.T, i *repo.IndexFile) {
723727
},
724728
Digest: "sha256:1234567890abcdef",
725729
},
730+
{
731+
Metadata: &chart.Metadata{
732+
Name: "xChartWithDuplicateDependenciesAndMissingAlias",
733+
Description: "string",
734+
Version: "1.2.3",
735+
Keywords: []string{"broken", "still accepted"},
736+
Home: "https://example.com/something",
737+
Dependencies: []*chart.Dependency{
738+
{Name: "kube-rbac-proxy", Version: "0.9.1"},
739+
},
740+
},
741+
URLs: []string{
742+
"https://kubernetes-charts.storage.googleapis.com/nginx-1.2.3.tgz",
743+
},
744+
Digest: "sha256:1234567890abcdef",
745+
},
726746
}
727-
tests := []*repo.ChartVersion{alpine[0], nginx[0], nginx[1]}
747+
tests := []*repo.ChartVersion{alpine[0], nginx[0], nginx[1], broken[0]}
728748

729749
for i, tt := range tests {
730750
expect := expects[i]
@@ -735,5 +755,129 @@ func verifyLocalIndex(t *testing.T, i *repo.IndexFile) {
735755
g.Expect(tt.Home).To(Equal(expect.Home))
736756
g.Expect(tt.URLs).To(ContainElements(expect.URLs))
737757
g.Expect(tt.Keywords).To(ContainElements(expect.Keywords))
758+
g.Expect(tt.Dependencies).To(ContainElements(expect.Dependencies))
759+
}
760+
}
761+
762+
// This code is taken from https://github.com/helm/helm/blob/v3.15.2/pkg/repo/index_test.go#L601
763+
// and refers to: https://github.com/helm/helm/issues/12748
764+
func TestIgnoreSkippableChartValidationError(t *testing.T) {
765+
type TestCase struct {
766+
Input error
767+
ErrorSkipped bool
768+
}
769+
testCases := map[string]TestCase{
770+
"nil": {
771+
Input: nil,
772+
},
773+
"generic_error": {
774+
Input: fmt.Errorf("foo"),
775+
},
776+
"non_skipped_validation_error": {
777+
Input: chart.ValidationError("chart.metadata.type must be application or library"),
778+
},
779+
"skipped_validation_error": {
780+
Input: chart.ValidationErrorf("more than one dependency with name or alias %q", "foo"),
781+
ErrorSkipped: true,
782+
},
783+
}
784+
785+
for name, tc := range testCases {
786+
t.Run(name, func(t *testing.T) {
787+
result := ignoreSkippableChartValidationError(tc.Input)
788+
789+
if tc.Input == nil {
790+
if result != nil {
791+
t.Error("expected nil result for nil input")
792+
}
793+
return
794+
}
795+
796+
if tc.ErrorSkipped {
797+
if result != nil {
798+
t.Error("expected nil result for skipped error")
799+
}
800+
return
801+
}
802+
803+
if tc.Input != result {
804+
t.Error("expected the result equal to input")
805+
}
806+
807+
})
808+
}
809+
}
810+
811+
var indexWithFirstVersionInvalid = `
812+
apiVersion: v1
813+
entries:
814+
nginx:
815+
- urls:
816+
- https://charts.helm.sh/stable/alpine-1.0.0.tgz
817+
- http://storage2.googleapis.com/kubernetes-charts/alpine-1.0.0.tgz
818+
name: nginx
819+
version: 0..1.0
820+
description: string
821+
home: https://github.com/something
822+
digest: "sha256:1234567890abcdef"
823+
- urls:
824+
- https://charts.helm.sh/stable/nginx-0.2.0.tgz
825+
name: nginx
826+
description: string
827+
version: 0.2.0
828+
home: https://github.com/something/else
829+
digest: "sha256:1234567890abcdef"
830+
`
831+
var indexWithLastVersionInvalid = `
832+
apiVersion: v1
833+
entries:
834+
nginx:
835+
- urls:
836+
- https://charts.helm.sh/stable/nginx-0.2.0.tgz
837+
name: nginx
838+
description: string
839+
version: 0.2.0
840+
home: https://github.com/something/else
841+
digest: "sha256:1234567890abcdef"
842+
- urls:
843+
- https://charts.helm.sh/stable/alpine-1.0.0.tgz
844+
- http://storage2.googleapis.com/kubernetes-charts/alpine-1.0.0.tgz
845+
name: nginx
846+
version: 0..1.0
847+
description: string
848+
home: https://github.com/something
849+
digest: "sha256:1234567890abcdef"
850+
`
851+
852+
func TestIndexFromBytes_InvalidEntries(t *testing.T) {
853+
tests := []struct {
854+
source string
855+
data string
856+
}{
857+
{
858+
source: "indexWithFirstVersionInvalid",
859+
data: indexWithFirstVersionInvalid,
860+
},
861+
{
862+
source: "indexWithLastVersionInvalid",
863+
data: indexWithLastVersionInvalid,
864+
},
865+
}
866+
for _, tc := range tests {
867+
t.Run(tc.source, func(t *testing.T) {
868+
idx, err := IndexFromBytes([]byte(tc.data))
869+
if err != nil {
870+
t.Fatalf("unexpected error: %s", err)
871+
}
872+
cvs := idx.Entries["nginx"]
873+
if len(cvs) == 0 {
874+
t.Error("expected one chart version not to be filtered out")
875+
}
876+
for _, v := range cvs {
877+
if v.Version == "0..1.0" {
878+
t.Error("malformed version was not filtered out")
879+
}
880+
}
881+
})
738882
}
739883
}

‎internal/helm/testdata/chartmuseum-index.json

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,36 @@
7777
"created": "0001-01-01T00:00:00Z",
7878
"digest": "sha256:1234567890abcdef"
7979
}
80+
],
81+
"xChartWithDuplicateDependenciesAndMissingAlias": [
82+
{
83+
"name": "xChartWithDuplicateDependenciesAndMissingAlias",
84+
"home": "https://example.com/something",
85+
"version": "1.2.3",
86+
"description": "string",
87+
"keywords": [
88+
"broken",
89+
"still accepted"
90+
],
91+
"apiVersion": "v1",
92+
"dependencies": [
93+
{
94+
"name": "kube-rbac-proxy",
95+
"version": "0.9.1",
96+
"repository": ""
97+
},
98+
{
99+
"name": "kube-rbac-proxy",
100+
"version": "0.9.1",
101+
"repository": ""
102+
}
103+
],
104+
"urls": [
105+
"https://kubernetes-charts.storage.googleapis.com/nginx-1.2.3.tgz"
106+
],
107+
"created": "0001-01-01T00:00:00Z",
108+
"digest": "sha256:1234567890abcdef"
109+
}
80110
]
81111
}
82112
}

‎internal/helm/testdata/chartmuseum-index.yaml

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,3 +48,19 @@ entries:
4848
- small
4949
- sumtin
5050
digest: "sha256:1234567890abcdef"
51+
xChartWithDuplicateDependenciesAndMissingAlias:
52+
- name: xChartWithDuplicateDependenciesAndMissingAlias
53+
description: string
54+
version: 1.2.3
55+
home: https://example.com/something
56+
keywords:
57+
- broken
58+
- still accepted
59+
urls:
60+
- https://kubernetes-charts.storage.googleapis.com/nginx-1.2.3.tgz
61+
digest: "sha256:1234567890abcdef"
62+
dependencies:
63+
- name: kube-rbac-proxy
64+
version: "0.9.1"
65+
- name: kube-rbac-proxy
66+
version: "0.9.1"

‎internal/helm/testdata/local-index-unordered.yaml

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,3 +46,19 @@ entries:
4646
- small
4747
- sumtin
4848
digest: "sha256:1234567890abcdef"
49+
xChartWithDuplicateDependenciesAndMissingAlias:
50+
- name: xChartWithDuplicateDependenciesAndMissingAlias
51+
description: string
52+
version: 1.2.3
53+
home: https://example.com/something
54+
keywords:
55+
- broken
56+
- still accepted
57+
urls:
58+
- https://kubernetes-charts.storage.googleapis.com/nginx-1.2.3.tgz
59+
digest: "sha256:1234567890abcdef"
60+
dependencies:
61+
- name: kube-rbac-proxy
62+
version: "0.9.1"
63+
- name: kube-rbac-proxy
64+
version: "0.9.1"

‎internal/helm/testdata/local-index.yaml

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,3 +46,19 @@ entries:
4646
- small
4747
- sumtin
4848
digest: "sha256:1234567890abcdef"
49+
xChartWithDuplicateDependenciesAndMissingAlias:
50+
- name: xChartWithDuplicateDependenciesAndMissingAlias
51+
description: string
52+
version: 1.2.3
53+
home: https://example.com/something
54+
keywords:
55+
- broken
56+
- still accepted
57+
urls:
58+
- https://kubernetes-charts.storage.googleapis.com/nginx-1.2.3.tgz
59+
digest: "sha256:1234567890abcdef"
60+
dependencies:
61+
- name: kube-rbac-proxy
62+
version: "0.9.1"
63+
- name: kube-rbac-proxy
64+
version: "0.9.1"

0 commit comments

Comments
 (0)