Repository: helm/helm
Pull request #11524 (2022-11-09 09:39:45)

harmonize URL reference resolving
**What this PR does / why we need it**:

Fixes https://github.com/helm/helm/issues/11472

TL;DR

Currently helm behaves differently when doing:

```bash
helm repo add test 'https://example.com?query'
helm pull test/test
```

versus

```bash
helm pull --repo 'https://example.com?query' test
```

(the later case drops the `?query` while the former preserves it)

This PR makes sure both cases use the same code for url "reference resolving" to ensure consistent behavior even in the future.

I also refactored the `TestResolveReferenceURL` test into a table test.

**Special notes for your reviewer**:

**If applicable**:
- [ ] this PR contains documentation
- [x] this PR contains unit tests
- [x] this PR has been tested for backwards compatibility

On backwards compatibility; In theory someone could rely on helm dropping the query parameters.


Patch:
diff --git a/pkg/downloader/chart_downloader.go b/pkg/downloader/chart_downloader.go
index c532759b5..29a9d64c2 100644
--- a/pkg/downloader/chart_downloader.go
+++ b/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.
diff --git a/pkg/repo/chartrepo.go b/pkg/repo/chartrepo.go
index fce947e4c..cab7b4680 100644
--- a/pkg/repo/chartrepo.go
+++ b/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 {


Test patch:
diff --git a/pkg/repo/chartrepo_test.go b/pkg/repo/chartrepo_test.go
index 3dae90391..790c16203 100644
--- a/pkg/repo/chartrepo_test.go
+++ b/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)
+		}
 	}
 }