Merge pull request #1015 from somtochiama/helm-repo-url

Fix: Normalize helm repository url with query params properly

Author: hiddeco

controllers/helmchart_controller.go

@@ -509,7 +509,10 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 	ctxTimeout, cancel := context.WithTimeout(ctx, repo.Spec.Timeout.Duration)
 	defer cancel()
 
-	normalizedURL := repository.NormalizeURL(repo.Spec.URL)
+	normalizedURL, err := repository.NormalizeURL(repo.Spec.URL)
+	if err != nil {
+		return chartRepoConfigErrorReturn(err, obj)
+	}
 	// Construct the Getter options from the HelmRepository data
 	clientOpts := []helmgetter.Option{
 		helmgetter.WithURL(normalizedURL),
@@ -1021,7 +1024,10 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont
 			keychain      authn.Keychain
 		)
 
-		normalizedURL := repository.NormalizeURL(url)
+		normalizedURL, err := repository.NormalizeURL(url)
+		if err != nil {
+			return nil, err
+		}
 		obj, err := r.resolveDependencyRepository(ctx, url, namespace)
 		if err != nil {
 			// Return Kubernetes client errors, but ignore others
@@ -1201,8 +1207,8 @@ func (r *HelmChartReconciler) indexHelmRepositoryByURL(o client.Object) []string
 	if !ok {
 		panic(fmt.Sprintf("Expected a HelmRepository, got %T", o))
 	}
-	u := repository.NormalizeURL(repo.Spec.URL)
-	if u != "" {
+	u, err := repository.NormalizeURL(repo.Spec.URL)
+	if u != "" && err == nil {
 		return []string{u}
 	}
 	return nil

internal/helm/chart/dependency_manager.go

@@ -266,7 +266,10 @@ func (dm *DependencyManager) resolveRepository(url string) (repo repository.Down
 	dm.mu.Lock()
 	defer dm.mu.Unlock()
 
-	nUrl := repository.NormalizeURL(url)
+	nUrl, err := repository.NormalizeURL(url)
+	if err != nil {
+		return
+	}
 	err = repository.ValidateDepURL(nUrl)
 	if err != nil {
 		return

internal/helm/repository/chart_repository.go

@@ -27,7 +27,6 @@ import (
 	"os"
 	"path"
 	"sort"
-	"strings"
 	"sync"
 
 	"github.com/Masterminds/semver/v3"
@@ -271,31 +270,16 @@ func (r *ChartRepository) DownloadChart(chart *repo.ChartVersion) (*bytes.Buffer
 	//  always the correct one to pick, check for updates once in awhile.
 	//  Ref: https://github.com/helm/helm/blob/v3.3.0/pkg/downloader/chart_downloader.go#L241
 	ref := chart.URLs[0]
-	u, err := url.Parse(ref)
+	resolvedUrl, err := repo.ResolveReferenceURL(r.URL, ref)
 	if err != nil {
-		err = fmt.Errorf("invalid chart URL format '%s': %w", ref, err)
 		return nil, err
 	}
 
-	// Prepend the chart repository base URL if the URL is relative
-	if !u.IsAbs() {
-		repoURL, err := url.Parse(r.URL)
-		if err != nil {
-			err = fmt.Errorf("invalid chart repository URL format '%s': %w", r.URL, err)
-			return nil, err
-		}
-		q := repoURL.Query()
-		// Trailing slash is required for ResolveReference to work
-		repoURL.Path = strings.TrimSuffix(repoURL.Path, "/") + "/"
-		u = repoURL.ResolveReference(u)
-		u.RawQuery = q.Encode()
-	}
-
 	t := transport.NewOrIdle(r.tlsConfig)
 	clientOpts := append(r.Options, getter.WithTransport(t))
 	defer transport.Release(t)
 
-	return r.Client.Get(u.String(), clientOpts...)
+	return r.Client.Get(resolvedUrl, clientOpts...)
 }
 
 // CacheIndex attempts to write the index from the remote into a new temporary file

internal/helm/repository/utils.go

@@ -18,6 +18,7 @@ package repository
 
 import (
 	"fmt"
+	"net/url"
 	"strings"
 
 	helmreg "helm.sh/helm/v3/pkg/registry"
@@ -35,17 +36,22 @@ var (
 )
 
 // NormalizeURL normalizes a ChartRepository URL by its scheme.
-func NormalizeURL(repositoryURL string) string {
+func NormalizeURL(repositoryURL string) (string, error) {
 	if repositoryURL == "" {
-		return ""
+		return "", nil
 	}
-
-	if strings.Contains(repositoryURL, helmreg.OCIScheme) {
-		return strings.TrimRight(repositoryURL, "/")
+	u, err := url.Parse(repositoryURL)
+	if err != nil {
+		return "", err
 	}
 
-	return strings.TrimRight(repositoryURL, "/") + "/"
+	if u.Scheme == helmreg.OCIScheme {
+		u.Path = strings.TrimRight(u.Path, "/")
+		return u.String(), nil
+	}
 
+	u.Path = strings.TrimRight(u.Path, "/") + "/"
+	return u.String(), nil
 }
 
 // ValidateDepURL returns an error if the given depended repository URL declaration is not supported

internal/helm/repository/utils_test.go

@@ -24,9 +24,10 @@ import (
 
 func TestNormalizeURL(t *testing.T) {
 	tests := []struct {
-		name string
-		url  string
-		want string
+		name    string
+		url     string
+		want    string
+		wantErr bool
 	}{
 		{
 			name: "with slash",
@@ -43,11 +44,6 @@ func TestNormalizeURL(t *testing.T) {
 			url:  "http://example.com//",
 			want: "http://example.com/",
 		},
-		{
-			name: "empty",
-			url:  "",
-			want: "",
-		},
 		{
 			name: "oci with slash",
 			url:  "oci://example.com/",
@@ -58,12 +54,38 @@ func TestNormalizeURL(t *testing.T) {
 			url:  "oci://example.com//",
 			want: "oci://example.com",
 		},
+		{
+			name: "url with query",
+			url:  "http://example.com?st=pr",
+			want: "http://example.com/?st=pr",
+		},
+		{
+			name: "url with slash and query",
+			url:  "http://example.com/?st=pr",
+			want: "http://example.com/?st=pr",
+		},
+		{
+			name: "empty url",
+			url:  "",
+			want: "",
+		},
+		{
+			name:    "bad url",
+			url:     "://badurl.",
+			wantErr: true,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			g := NewWithT(t)
 
-			got := NormalizeURL(tt.url)
+			got, err := NormalizeURL(tt.url)
+			if tt.wantErr {
+				g.Expect(err).To(HaveOccurred())
+				return
+			}
+
+			g.Expect(err).To(Not(HaveOccurred()))
 			g.Expect(got).To(Equal(tt.want))
 		})
 	}