Invalid helm charts are not fully excluded in helm search result #12744
Description
I have an umbrella helm chart with two dependencies with the same sub-chart, identified by alias. However, my helm repository (powered by JFrog) fails to record the alias in its metadata. Helm 3.14.0 starts to enforce on this kind of metadata (via #9176), so my umbrella helm charts become "invalid" now, and helm reported below error message:
index.go:366: skipping loading invalid entry for chart "some-chart" "some-version" from <dir_name>helm-index.yaml: validation: more than one dependency with name or alias "sub-chart-name"
While the repository metadata needs to record aliases to make it compatible with new helm rules, on the other hand, I find helm may return some of these invalid helm charts back in the search result. After my troubleshooting, I located below code as the root cause:
pkg/repo/index.go
// loadIndex loads an index file and does minimal validity checking.
//
// The source parameter is only used for logging.
// This will fail if API Version is not set (ErrNoAPIVersion) or if the unmarshal fails.
func loadIndex(data []byte, source string) (*IndexFile, error) {
i := &IndexFile{}
if len(data) == 0 {
return i, ErrEmptyIndexYaml
}
if err := jsonOrYamlUnmarshal(data, i); err != nil {
return i, err
}
for name, cvs := range i.Entries {
for idx := len(cvs) - 1; idx >= 0; idx-- {
if cvs[idx] == nil {
log.Printf("skipping loading invalid entry for chart %q from %s: empty entry", name, source)
continue
}
if cvs[idx].APIVersion == "" {
cvs[idx].APIVersion = chart.APIVersionV1
}
if err := cvs[idx].Validate(); err != nil {
log.Printf("skipping loading invalid entry for chart %q %q from %s: %s", name, cvs[idx].Version, source, err)
cvs = append(cvs[:idx], cvs[idx+1:]...)
}
}
}
i.SortEntries()
if i.APIVersion == "" {
return i, ErrNoAPIVersion
}
return i, nil
}
To be specific, it is this line: cvs = append(cvs[:idx], cvs[idx+1:]...)
This slice operation removed items from cvs, ideally, it should remove corresponding items from the raw i.Entries as well. But that's not always the case:
- if the last item in the entries is kept, then the result is what we expect: every invalid helm charts are excluded;
- if the last item is to be excluded, it will still in the i.Entries, and confuses the search result.
I would like to use below code (can get the result via Go Playground) to show the difference:
package main
import "fmt"
func remove_err(map1 map[string][]string) {
fmt.Printf("Before removal, map1 = %s\n", map1)
for _, value := range map1 {
for idx := len(value) - 1; idx >= 0; idx-- {
if value[idx] == "err" {
value = append(value[:idx], value[idx+1:]...)
}
}
fmt.Printf("After removal, the new value is now: %s\n", value)
}
fmt.Printf("After remove_err():\t%s\n", map1)
}
func main() {
map1 := map[string][]string{}
map1["test-key"] = []string{"err", "something", "err", "err", "keep-value1", "keep-value2"}
remove_err(map1)
map1["test-key"] = []string{"err", "keep-value1", "err", "keep-value2", "err", "err"}
remove_err(map1)
}
Output:
Before removal, map1 = map[test-key:[err something err err keep-value1 keep-value2]]
After removal, the new value is now: [something keep-value1 keep-value2]
After remove_err(): map[test-key:[something keep-value1 keep-value2 keep-value2 keep-value2 keep-value2]]
Before removal, map1 = map[test-key:[err keep-value1 err keep-value2 err err]]
After removal, the new value is now: [keep-value1 keep-value2]
After remove_err(): map[test-key:[keep-value1 keep-value2 keep-value2 keep-value2 err err]]
The above output tells us: the slice operation will affect the original entries, but only the values than the whole array. Instead, the array length will keep the same size. If the slice operation removes the last item, the original array will keep exactly the same after this operation.
The case that invalid helm chart located at the end of the entries appeared in the search result is exactly caused by this fact. The case that valid helm chart located at the end of the entries will duplicate as many times as the removed number of invalid helm charts. From search efficiency perspective, keep the original long array is less efficient than excluding all invalid charts.
My proposal for this bug is to assign the cvs back explicitly, so that the original entries are shortened as well.
package main
import "fmt"
func remove_err(map1 map[string][]string, assign_back bool) {
fmt.Printf("Before removal, map1 = %s\n", map1)
for key, value := range map1 {
for idx := len(value) - 1; idx >= 0; idx-- {
if value[idx] == "err" {
value = append(value[:idx], value[idx+1:]...)
}
}
fmt.Printf("After removal, the new value is now: %s\n", value)
if assign_back {
map1[key] = value
}
}
fmt.Printf("After remove_err(assign_back=%t):\t%s\n", assign_back, map1)
}
func remove_array(arr1 []string) {
fmt.Printf("Before removal, arr1 = %s\n", arr1)
for idx := len(arr1) - 1; idx >= 0; idx-- {
if arr1[idx] == "err" {
arr1 = append(arr1[:idx], arr1[idx+1:]...)
}
}
fmt.Printf("After removal, the new array is now: %s\n", arr1)
}
func main() {
map1 := map[string][]string{}
map1["test-key"] = []string{"err", "something", "err", "err", "keep-value1", "keep-value2"}
remove_err(map1, false)
map1["test-key"] = []string{"err", "something", "err", "err", "keep-value1", "keep-value2"}
remove_err(map1, true)
map1["test-key"] = []string{"err", "keep-value1", "err", "keep-value2", "err", "err"}
remove_err(map1, false)
map1["test-key"] = []string{"err", "keep-value1", "err", "keep-value2", "err", "err"}
remove_err(map1, true)
map1["test-key"] = []string{"err", "keep-value1", "err", "keep-value2", "err", "err"}
remove_array(map1["test-key"])
fmt.Printf("After remove_arr():\t%s\n", map1)
}
Output:
Before removal, map1 = map[test-key:[err something err err keep-value1 keep-value2]]
After removal, the new value is now: [something keep-value1 keep-value2]
After remove_err(assign_back=false): map[test-key:[something keep-value1 keep-value2 keep-value2 keep-value2 keep-value2]]
Before removal, map1 = map[test-key:[err something err err keep-value1 keep-value2]]
After removal, the new value is now: [something keep-value1 keep-value2]
After remove_err(assign_back=true): map[test-key:[something keep-value1 keep-value2]]
Before removal, map1 = map[test-key:[err keep-value1 err keep-value2 err err]]
After removal, the new value is now: [keep-value1 keep-value2]
After remove_err(assign_back=false): map[test-key:[keep-value1 keep-value2 keep-value2 keep-value2 err err]]
Before removal, map1 = map[test-key:[err keep-value1 err keep-value2 err err]]
After removal, the new value is now: [keep-value1 keep-value2]
After remove_err(assign_back=true): map[test-key:[keep-value1 keep-value2]]
Before removal, arr1 = [err keep-value1 err keep-value2 err err]
After removal, the new array is now: [keep-value1 keep-value2]
After remove_arr(): map[test-key:[keep-value1 keep-value2 keep-value2 keep-value2 err err]]
Environment information:
Output of helm version:
version.BuildInfo{Version:"v3.14.0", GitCommit:"3fc9f4b2638e76f26739cd77c7017139be81d0ea", GitTreeState:"clean", GoVersion:"go1.21.5"}
Output of kubectl version:
Client Version: v1.29.1
Kustomize Version: v5.0.4-0.20230601165947-6ce0bf390ce3
Server Version: v1.23.6
WARNING: version difference between client (1.29) and server (1.23) exceeds the supported minor version skew of +/-1
Cloud Provider/Platform (AKS, GKE, Minikube etc.): not related.