harmonize URL reference resolving

Fixes https://github.com/helm/helm/issues/11472

Signed-off-by: Philipp Stehle <philipp.stehle@sap.com>

pkg/downloader/chart_downloader.go

@@ -294,32 +294,13 @@ func (c *ChartDownloader) ResolveChartVersion(ref, version string) (*url.URL, er
 	}
 
 	// TODO: Seems that picking first URL is not fully correct
-	u, err = url.Parse(cv.URLs[0])
+	resolvedURL, err := repo.ResolveReferenceURL(rc.URL, cv.URLs[0])
+
 	if err != nil {
 		return u, errors.Errorf("invalid chart URL format: %s", ref)
 	}
 
-	// If the URL is relative (no scheme), prepend the chart repo's base URL
-	if !u.IsAbs() {
-		repoURL, err := url.Parse(rc.URL)
-		if err != nil {
-			return repoURL, err
-		}
-		q := repoURL.Query()
-		// We need a trailing slash for ResolveReference to work, but make sure there isn't already one
-		repoURL.RawPath = strings.TrimSuffix(repoURL.RawPath, "/") + "/"
-		repoURL.Path = strings.TrimSuffix(repoURL.Path, "/") + "/"
-		u = repoURL.ResolveReference(u)
-		u.RawQuery = q.Encode()
-		// TODO add user-agent
-		if _, err := getter.NewHTTPGetter(getter.WithURL(rc.URL)); err != nil {
-			return repoURL, err
-		}
-		return u, err
-	}
-
-	// TODO add user-agent
-	return u, nil
+	return url.Parse(resolvedURL)
 }
 
 // VerifyChart takes a path to a chart archive and a keyring, and verifies the chart.

pkg/repo/chartrepo.go

@@ -25,7 +25,6 @@ import (
 	"log"
 	"net/url"
 	"os"
-	"path"
 	"path/filepath"
 	"strings"
 
@@ -116,14 +115,11 @@ func (r *ChartRepository) Load() error {
 
 // DownloadIndexFile fetches the index from a repository.
 func (r *ChartRepository) DownloadIndexFile() (string, error) {
-	parsedURL, err := url.Parse(r.Config.URL)
+	indexURL, err := ResolveReferenceURL(r.Config.URL, "index.yaml")
 	if err != nil {
 		return "", err
 	}
-	parsedURL.RawPath = path.Join(parsedURL.RawPath, "index.yaml")
-	parsedURL.Path = path.Join(parsedURL.Path, "index.yaml")
 
-	indexURL := parsedURL.String()
 	// TODO add user-agent
 	resp, err := r.Client.Get(indexURL,
 		getter.WithURL(r.Config.URL),
@@ -290,18 +286,27 @@ func FindChartInAuthAndTLSAndPassRepoURL(repoURL, username, password, chartName,
 // ResolveReferenceURL resolves refURL relative to baseURL.
 // If refURL is absolute, it simply returns refURL.
 func ResolveReferenceURL(baseURL, refURL string) (string, error) {
-	// We need a trailing slash for ResolveReference to work, but make sure there isn't already one
-	parsedBaseURL, err := url.Parse(strings.TrimSuffix(baseURL, "/") + "/")
+	parsedRefURL, err := url.Parse(refURL)
 	if err != nil {
-		return "", errors.Wrapf(err, "failed to parse %s as URL", baseURL)
+		return "", errors.Wrapf(err, "failed to parse %s as URL", refURL)
 	}
 
-	parsedRefURL, err := url.Parse(refURL)
+	if parsedRefURL.IsAbs() {
+		return refURL, nil
+	}
+
+	parsedBaseURL, err := url.Parse(baseURL)
 	if err != nil {
-		return "", errors.Wrapf(err, "failed to parse %s as URL", refURL)
+		return "", errors.Wrapf(err, "failed to parse %s as URL", baseURL)
 	}
 
-	return parsedBaseURL.ResolveReference(parsedRefURL).String(), nil
+	// We need a trailing slash for ResolveReference to work, but make sure there isn't already one
+	parsedBaseURL.RawPath = strings.TrimSuffix(parsedBaseURL.RawPath, "/") + "/"
+	parsedBaseURL.Path = strings.TrimSuffix(parsedBaseURL.Path, "/") + "/"
+
+	resolvedURL := parsedBaseURL.ResolveReference(parsedRefURL)
+	resolvedURL.RawQuery = parsedBaseURL.RawQuery
+	return resolvedURL.String(), nil
 }
 
 func (e *Entry) String() string {

pkg/repo/chartrepo_test.go

@@ -385,35 +385,21 @@ func TestErrorFindChartInRepoURL(t *testing.T) {
 }
 
 func TestResolveReferenceURL(t *testing.T) {
-	chartURL, err := ResolveReferenceURL("http://localhost:8123/charts/", "nginx-0.2.0.tgz")
-	if err != nil {
-		t.Errorf("%s", err)
-	}
-	if chartURL != "http://localhost:8123/charts/nginx-0.2.0.tgz" {
-		t.Errorf("%s", chartURL)
-	}
-
-	chartURL, err = ResolveReferenceURL("http://localhost:8123/charts-with-no-trailing-slash", "nginx-0.2.0.tgz")
-	if err != nil {
-		t.Errorf("%s", err)
-	}
-	if chartURL != "http://localhost:8123/charts-with-no-trailing-slash/nginx-0.2.0.tgz" {
-		t.Errorf("%s", chartURL)
-	}
-
-	chartURL, err = ResolveReferenceURL("http://localhost:8123", "https://charts.helm.sh/stable/nginx-0.2.0.tgz")
-	if err != nil {
-		t.Errorf("%s", err)
-	}
-	if chartURL != "https://charts.helm.sh/stable/nginx-0.2.0.tgz" {
-		t.Errorf("%s", chartURL)
-	}
-
-	chartURL, err = ResolveReferenceURL("http://localhost:8123/charts%2fwith%2fescaped%2fslash", "nginx-0.2.0.tgz")
-	if err != nil {
-		t.Errorf("%s", err)
-	}
-	if chartURL != "http://localhost:8123/charts%2fwith%2fescaped%2fslash/nginx-0.2.0.tgz" {
-		t.Errorf("%s", chartURL)
+	for _, tt := range []struct {
+		baseURL, refURL, chartURL string
+	}{
+		{"http://localhost:8123/charts/", "nginx-0.2.0.tgz", "http://localhost:8123/charts/nginx-0.2.0.tgz"},
+		{"http://localhost:8123/charts-with-no-trailing-slash", "nginx-0.2.0.tgz", "http://localhost:8123/charts-with-no-trailing-slash/nginx-0.2.0.tgz"},
+		{"http://localhost:8123", "https://charts.helm.sh/stable/nginx-0.2.0.tgz", "https://charts.helm.sh/stable/nginx-0.2.0.tgz"},
+		{"http://localhost:8123/charts%2fwith%2fescaped%2fslash", "nginx-0.2.0.tgz", "http://localhost:8123/charts%2fwith%2fescaped%2fslash/nginx-0.2.0.tgz"},
+		{"http://localhost:8123/charts?with=queryparameter", "nginx-0.2.0.tgz", "http://localhost:8123/charts/nginx-0.2.0.tgz?with=queryparameter"},
+	} {
+		chartURL, err := ResolveReferenceURL(tt.baseURL, tt.refURL)
+		if err != nil {
+			t.Errorf("unexpected error in ResolveReferenceURL(%q, %q): %s", tt.baseURL, tt.refURL, err)
+		}
+		if chartURL != tt.chartURL {
+			t.Errorf("expected ResolveReferenceURL(%q, %q) to equal %q, got %q", tt.baseURL, tt.refURL, tt.chartURL, chartURL)
+		}
 	}
 }