From 6f0384c50ef7ce1cdf840c8f1010eaf0bc4252e4 Mon Sep 17 00:00:00 2001 From: Somtochi Onyekwere Date: Tue, 31 Jan 2023 19:51:18 +0100 Subject: [PATCH] Normalize path in url Signed-off-by: Somtochi Onyekwere --- controllers/helmchart_controller.go | 14 +++++-- internal/helm/chart/dependency_manager.go | 5 ++- internal/helm/repository/chart_repository.go | 20 +--------- internal/helm/repository/utils.go | 18 ++++++--- internal/helm/repository/utils_test.go | 40 +++++++++++++++----- 5 files changed, 59 insertions(+), 38 deletions(-) diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index a6119225e..a3f05ce25 100644 --- a/controllers/helmchart_controller.go +++ b/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 diff --git a/internal/helm/chart/dependency_manager.go b/internal/helm/chart/dependency_manager.go index 4465931c7..97b1534a4 100644 --- a/internal/helm/chart/dependency_manager.go +++ b/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 diff --git a/internal/helm/repository/chart_repository.go b/internal/helm/repository/chart_repository.go index 34781e9ac..8071df242 100644 --- a/internal/helm/repository/chart_repository.go +++ b/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 diff --git a/internal/helm/repository/utils.go b/internal/helm/repository/utils.go index 5d5ab2548..2b0b8ed8d 100644 --- a/internal/helm/repository/utils.go +++ b/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 diff --git a/internal/helm/repository/utils_test.go b/internal/helm/repository/utils_test.go index 3ee77606d..c9a022758 100644 --- a/internal/helm/repository/utils_test.go +++ b/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)) }) }