Merge pull request #2063 from ogulcanaydogan/fix/654-descriptive-helm-repo-error

Add the repository URL to the Helm index parse failure message

Author: stefanprodan

internal/controller/helmrepository_controller.go

@@ -488,7 +488,7 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patc
 	// Load the cached repository index to ensure it passes validation.
 	if err := chartRepo.LoadFromPath(); err != nil {
 		e := serror.NewGeneric(
-			fmt.Errorf("failed to load Helm repository from index YAML: %w", err),
+			fmt.Errorf("failed to load Helm repository from index YAML at %s: %w", redactURL(obj.Spec.URL), err),
 			sourcev1.IndexationFailedReason,
 		)
 		conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e)
@@ -732,3 +732,13 @@ func (r *HelmRepositoryReconciler) migrationToStatic(ctx context.Context, sp *pa
 
 	return ctrl.Result{}, nil
 }
+
+// redactURL returns the given URL with the password of its user info
+// replaced, so it can be added to conditions and events.
+func redactURL(repoURL string) string {
+	u, err := url.Parse(repoURL)
+	if err != nil {
+		return "<invalid URL>"
+	}
+	return u.Redacted()
+}

internal/controller/helmrepository_controller_test.go

@@ -1157,12 +1157,39 @@ func TestHelmRepositoryReconciler_reconcileSource_cachedIndexFile(t *testing.T)
 		got, err := newReconciler(g, c).reconcileSource(context.TODO(), patch.NewSerialPatcher(obj, c), obj, &meta.Artifact{}, &chartRepo)
 
 		g.Expect(err).To(HaveOccurred())
-		g.Expect(err.Error()).To(ContainSubstring("failed to load Helm repository from index YAML"))
+		g.Expect(err.Error()).To(ContainSubstring("failed to load Helm repository from index YAML at " + server.URL() + ":"))
 		g.Expect(got).To(Equal(sreconcile.ResultEmpty))
 		g.Expect(conditions.IsTrue(obj, sourcev1.FetchFailedCondition)).To(BeTrue())
+		g.Expect(conditions.GetMessage(obj, sourcev1.FetchFailedCondition)).To(ContainSubstring("failed to load Helm repository from index YAML at " + server.URL() + ":"))
 		g.Expect(chartIndexFiles(g, dir)).To(BeEmpty())
 	})
 
+	t.Run("invalid index message redacts credentials in the URL", func(t *testing.T) {
+		g := NewWithT(t)
+		dir := t.TempDir()
+		t.Setenv("TMPDIR", dir)
+
+		server := startServer(g, []byte("{}\n"))
+		defer server.Stop()
+		defer os.RemoveAll(server.Root())
+
+		c := fakeclient.NewClientBuilder().
+			WithScheme(testEnv.GetScheme()).
+			WithStatusSubresource(&sourcev1.HelmRepository{}).
+			Build()
+		obj := newObject(strings.Replace(server.URL(), "http://", "http://user:s3cr3t@", 1))
+		g.Expect(c.Create(context.TODO(), obj)).To(Succeed())
+
+		var chartRepo repository.ChartRepository
+		_, err := newReconciler(g, c).reconcileSource(context.TODO(), patch.NewSerialPatcher(obj, c), obj, &meta.Artifact{}, &chartRepo)
+
+		g.Expect(err).To(HaveOccurred())
+		g.Expect(err.Error()).To(ContainSubstring("failed to load Helm repository from index YAML at"))
+		g.Expect(err.Error()).To(ContainSubstring("user:xxxxx@"))
+		g.Expect(err.Error()).ToNot(ContainSubstring("s3cr3t"))
+		g.Expect(conditions.GetMessage(obj, sourcev1.FetchFailedCondition)).ToNot(ContainSubstring("s3cr3t"))
+	})
+
 	t.Run("status patch failure removes cached file", func(t *testing.T) {
 		g := NewWithT(t)
 		dir := t.TempDir()