Merge pull request #1516 from bb-Ricardo/main

Fix Helm index validation for Artifactory

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{

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
+}

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")
+				}
+			}
+		})
 	}
 }

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"
+      }
     ]
   }
 }

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"

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"

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"