Repository: helm/helm
Pull request #13119 (2024-06-16 13:36:36)

fix(concurrency): Use channel for repoFailList errors in updateCharts
**What this PR does / why we need it**:
This PR addresses a concurrency issue in the updateCharts function where multiple goroutines accessed and modified the `repoFailList` slice and `out` without synchronization. This could lead to data races.



idsulik: Hi @mattfarina ,

I noticed that this pull request has been pending for a while. Is there anything more needed from my side? Please let me know if there are any updates or additional changes required.

Thanks!
idsulik: Hello @mattfarina, just a gentle reminder about this PR created months ago. If possible, could it be reviewed before the next Helm release? I’d appreciate any feedback to help get it ready in time. Thank you!
idsulik: @gjenkins8 hi! Could you please check the PR?
sabre1041: @idsulik can you resolve the conflicts so that this enhancement can be integrated?

Patch:
diff --git a/pkg/cmd/repo_update.go b/pkg/cmd/repo_update.go
index 6590d9872..12de2bdaa 100644
--- a/pkg/cmd/repo_update.go
+++ b/pkg/cmd/repo_update.go
@@ -111,20 +111,30 @@ func (o *repoUpdateOptions) run(out io.Writer) error {
 func updateCharts(repos []*repo.ChartRepository, out io.Writer) error {
 	fmt.Fprintln(out, "Hang tight while we grab the latest from your chart repositories...")
 	var wg sync.WaitGroup
-	var repoFailList []string
+	failRepoURLChan := make(chan string, len(repos))
+
 	for _, re := range repos {
 		wg.Add(1)
 		go func(re *repo.ChartRepository) {
 			defer wg.Done()
 			if _, err := re.DownloadIndexFile(); err != nil {
 				fmt.Fprintf(out, "...Unable to get an update from the %q chart repository (%s):\n\t%s\n", re.Config.Name, re.Config.URL, err)
-				repoFailList = append(repoFailList, re.Config.URL)
+				failRepoURLChan <- re.Config.URL
 			} else {
 				fmt.Fprintf(out, "...Successfully got an update from the %q chart repository\n", re.Config.Name)
 			}
 		}(re)
 	}
-	wg.Wait()
+
+	go func() {
+		wg.Wait()
+		close(failRepoURLChan)
+	}()
+
+	var repoFailList []string
+	for url := range failRepoURLChan {
+		repoFailList = append(repoFailList, url)
+	}
 
 	if len(repoFailList) > 0 {
 		return fmt.Errorf("Failed to update the following repositories: %s",


Test patch:
diff --git a/pkg/cmd/repo_update_test.go b/pkg/cmd/repo_update_test.go
index 6fc4c8f4b..aa8f52beb 100644
--- a/pkg/cmd/repo_update_test.go
+++ b/pkg/cmd/repo_update_test.go
@@ -172,7 +172,14 @@ func TestUpdateChartsFailWithError(t *testing.T) {
 	defer ts.Stop()
 
 	var invalidURL = ts.URL() + "55"
-	r, err := repo.NewChartRepository(&repo.Entry{
+	r1, err := repo.NewChartRepository(&repo.Entry{
+		Name: "charts",
+		URL:  invalidURL,
+	}, getter.All(settings))
+	if err != nil {
+		t.Error(err)
+	}
+	r2, err := repo.NewChartRepository(&repo.Entry{
 		Name: "charts",
 		URL:  invalidURL,
 	}, getter.All(settings))
@@ -181,7 +188,7 @@ func TestUpdateChartsFailWithError(t *testing.T) {
 	}
 
 	b := bytes.NewBuffer(nil)
-	err = updateCharts([]*repo.ChartRepository{r}, b)
+	err = updateCharts([]*repo.ChartRepository{r1, r2}, b)
 	if err == nil {
 		t.Error("Repo update should return error because update of repository fails and 'fail-on-repo-update-fail' flag set")
 		return