From 3cfbcc059be82d9c0a8934d20fc0e72a2d290c24 Mon Sep 17 00:00:00 2001 From: Soule BA Date: Tue, 5 Jul 2022 16:52:33 +0200 Subject: [PATCH] Enable remote dependencies from OCI repositories If implemented, the source controller will be able to resolve charts dependencies from OCI repositories. The remote builder has been refactored as part of this work. Signed-off-by: Soule BA --- controllers/helmchart_controller.go | 128 +++++-- controllers/helmchart_controller_test.go | 14 +- controllers/helmrepository_controller_oci.go | 2 +- .../charts/helmchartwithdeps/Chart.yaml | 3 + internal/helm/chart/builder_local_test.go | 6 +- internal/helm/chart/builder_remote.go | 18 +- internal/helm/chart/dependency_manager.go | 71 ++-- .../helm/chart/dependency_manager_test.go | 350 +++++++++++++++--- internal/helm/registry/client.go | 18 +- internal/helm/repository/chart_repository.go | 8 +- .../helm/repository/oci_chart_repository.go | 32 +- internal/helm/repository/repository.go | 35 ++ internal/helm/repository/utils.go | 19 +- internal/helm/repository/utils_test.go | 10 + 14 files changed, 547 insertions(+), 167 deletions(-) create mode 100644 internal/helm/repository/repository.go diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index 2fa63ecf9..d2c1ad609 100644 --- a/controllers/helmchart_controller.go +++ b/controllers/helmchart_controller.go @@ -35,6 +35,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + kerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/uuid" kuberecorder "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" @@ -461,9 +462,10 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * loginOpts []helmreg.LoginOption ) + normalizedURL := repository.NormalizeURL(repo.Spec.URL) // Construct the Getter options from the HelmRepository data clientOpts := []helmgetter.Option{ - helmgetter.WithURL(repo.Spec.URL), + helmgetter.WithURL(normalizedURL), helmgetter.WithTimeout(repo.Spec.Timeout.Duration), helmgetter.WithPassCredentialsAll(repo.Spec.PassCredentials), } @@ -491,7 +493,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * } clientOpts = append(clientOpts, opts...) - tlsConfig, err = getter.TLSClientConfigFromSecret(*secret, repo.Spec.URL) + tlsConfig, err = getter.TLSClientConfigFromSecret(*secret, normalizedURL) if err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to create TLS client config with secret data: %w", err), @@ -503,7 +505,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * } // Build registryClient options from secret - loginOpt, err := registry.LoginOptionFromSecret(repo.Spec.URL, *secret) + loginOpt, err := registry.LoginOptionFromSecret(normalizedURL, *secret) if err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to configure Helm client with secret data: %w", err), @@ -518,11 +520,11 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * } // Initialize the chart repository - var chartRepo chart.Repository + var chartRepo repository.Downloader switch repo.Spec.Type { case sourcev1.HelmRepositoryTypeOCI: - if !helmreg.IsOCI(repo.Spec.URL) { - err := fmt.Errorf("invalid OCI registry URL: %s", repo.Spec.URL) + if !helmreg.IsOCI(normalizedURL) { + err := fmt.Errorf("invalid OCI registry URL: %s", normalizedURL) return chartRepoConfigErrorReturn(err, obj) } @@ -530,7 +532,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * // this is needed because otherwise the credentials are stored in ~/.docker/config.json. // TODO@souleb: remove this once the registry move to Oras v2 // or rework to enable reusing credentials to avoid the unneccessary handshake operations - registryClient, file, err := r.RegistryClientGenerator(loginOpts != nil) + registryClient, credentialsFile, err := r.RegistryClientGenerator(loginOpts != nil) if err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to construct Helm client: %w", err), @@ -540,9 +542,9 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * return sreconcile.ResultEmpty, e } - if file != "" { + if credentialsFile != "" { defer func() { - if err := os.Remove(file); err != nil { + if err := os.Remove(credentialsFile); err != nil { r.eventLogf(ctx, obj, corev1.EventTypeWarning, meta.FailedReason, "failed to delete temporary credentials file: %s", err) } @@ -551,7 +553,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * // Tell the chart repository to use the OCI client with the configured getter clientOpts = append(clientOpts, helmgetter.WithRegistryClient(registryClient)) - ociChartRepo, err := repository.NewOCIChartRepository(repo.Spec.URL, repository.WithOCIGetter(r.Getters), repository.WithOCIGetterOptions(clientOpts), repository.WithOCIRegistryClient(registryClient)) + ociChartRepo, err := repository.NewOCIChartRepository(normalizedURL, repository.WithOCIGetter(r.Getters), repository.WithOCIGetterOptions(clientOpts), repository.WithOCIRegistryClient(registryClient)) if err != nil { return chartRepoConfigErrorReturn(err, obj) } @@ -571,7 +573,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * } } default: - httpChartRepo, err := repository.NewChartRepository(repo.Spec.URL, r.Storage.LocalPath(*repo.GetArtifact()), r.Getters, tlsConfig, clientOpts, + httpChartRepo, err := repository.NewChartRepository(normalizedURL, r.Storage.LocalPath(*repo.GetArtifact()), r.Getters, tlsConfig, clientOpts, repository.WithMemoryCache(r.Storage.LocalPath(*repo.GetArtifact()), r.Cache, r.TTL, func(event string) { r.IncCacheEvents(event, obj.Name, obj.Namespace) })) @@ -684,9 +686,15 @@ func (r *HelmChartReconciler) buildFromTarballArtifact(ctx context.Context, obj // Setup dependency manager dm := chart.NewDependencyManager( - chart.WithRepositoryCallback(r.namespacedChartRepositoryCallback(ctx, obj.GetName(), obj.GetNamespace())), + chart.WithDownloaderCallback(r.namespacedChartRepositoryCallback(ctx, obj.GetName(), obj.GetNamespace())), ) - defer dm.Clear() + defer func() { + err := dm.Clear() + if err != nil { + r.eventLogf(ctx, obj, corev1.EventTypeWarning, meta.FailedReason, + "dependency manager cleanup error: %s", err) + } + }() // Configure builder options, including any previously cached chart opts := chart.BuildOptions{ @@ -913,12 +921,17 @@ func (r *HelmChartReconciler) garbageCollect(ctx context.Context, obj *sourcev1. return nil } -// namespacedChartRepositoryCallback returns a chart.GetChartRepositoryCallback scoped to the given namespace. -// The returned callback returns a repository.ChartRepository configured with the retrieved v1beta1.HelmRepository, +// namespacedChartRepositoryCallback returns a chart.GetChartDownloaderCallback scoped to the given namespace. +// The returned callback returns a repository.Downloader configured with the retrieved v1beta1.HelmRepository, // or a shim with defaults if no object could be found. -func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Context, name, namespace string) chart.GetChartRepositoryCallback { - return func(url string) (*repository.ChartRepository, error) { - var tlsConfig *tls.Config +// The callback returns an object with a state, so the caller has to do the necessary cleanup. +func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Context, name, namespace string) chart.GetChartDownloaderCallback { + return func(url string) (repository.Downloader, error) { + var ( + tlsConfig *tls.Config + loginOpts []helmreg.LoginOption + ) + normalizedURL := repository.NormalizeURL(url) repo, err := r.resolveDependencyRepository(ctx, url, namespace) if err != nil { // Return Kubernetes client errors, but ignore others @@ -933,7 +946,7 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont } } clientOpts := []helmgetter.Option{ - helmgetter.WithURL(repo.Spec.URL), + helmgetter.WithURL(normalizedURL), helmgetter.WithTimeout(repo.Spec.Timeout.Duration), helmgetter.WithPassCredentialsAll(repo.Spec.PassCredentials), } @@ -947,26 +960,77 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont } clientOpts = append(clientOpts, opts...) - tlsConfig, err = getter.TLSClientConfigFromSecret(*secret, repo.Spec.URL) + tlsConfig, err = getter.TLSClientConfigFromSecret(*secret, normalizedURL) if err != nil { return nil, fmt.Errorf("failed to create TLS client config for HelmRepository '%s': %w", repo.Name, err) } - } - chartRepo, err := repository.NewChartRepository(repo.Spec.URL, "", r.Getters, tlsConfig, clientOpts) - if err != nil { - return nil, err + // Build registryClient options from secret + loginOpt, err := registry.LoginOptionFromSecret(normalizedURL, *secret) + if err != nil { + return nil, fmt.Errorf("failed to create login options for HelmRepository '%s': %w", repo.Name, err) + } + + loginOpts = append([]helmreg.LoginOption{}, loginOpt) } - // Ensure that the cache key is the same as the artifact path - // otherwise don't enable caching. We don't want to cache indexes - // for repositories that are not reconciled by the source controller. - if repo.Status.Artifact != nil { - chartRepo.CachePath = r.Storage.LocalPath(*repo.GetArtifact()) - chartRepo.SetMemCache(r.Storage.LocalPath(*repo.GetArtifact()), r.Cache, r.TTL, func(event string) { - r.IncCacheEvents(event, name, namespace) - }) + var chartRepo repository.Downloader + if helmreg.IsOCI(normalizedURL) { + registryClient, credentialsFile, err := r.RegistryClientGenerator(loginOpts != nil) + if err != nil { + return nil, fmt.Errorf("failed to create registry client for HelmRepository '%s': %w", repo.Name, err) + } + + var errs []error + // Tell the chart repository to use the OCI client with the configured getter + clientOpts = append(clientOpts, helmgetter.WithRegistryClient(registryClient)) + ociChartRepo, err := repository.NewOCIChartRepository(normalizedURL, repository.WithOCIGetter(r.Getters), + repository.WithOCIGetterOptions(clientOpts), + repository.WithOCIRegistryClient(registryClient), + repository.WithCredentialsFile(credentialsFile)) + if err != nil { + errs = append(errs, fmt.Errorf("failed to create OCI chart repository for HelmRepository '%s': %w", repo.Name, err)) + // clean up the credentialsFile + if credentialsFile != "" { + if err := os.Remove(credentialsFile); err != nil { + errs = append(errs, err) + } + } + return nil, kerrors.NewAggregate(errs) + } + + // If login options are configured, use them to login to the registry + // The OCIGetter will later retrieve the stored credentials to pull the chart + if loginOpts != nil { + err = ociChartRepo.Login(loginOpts...) + if err != nil { + errs = append(errs, fmt.Errorf("failed to login to OCI chart repository for HelmRepository '%s': %w", repo.Name, err)) + // clean up the credentialsFile + errs = append(errs, ociChartRepo.Clear()) + return nil, kerrors.NewAggregate(errs) + } + } + + chartRepo = ociChartRepo + } else { + httpChartRepo, err := repository.NewChartRepository(normalizedURL, "", r.Getters, tlsConfig, clientOpts) + if err != nil { + return nil, err + } + + // Ensure that the cache key is the same as the artifact path + // otherwise don't enable caching. We don't want to cache indexes + // for repositories that are not reconciled by the source controller. + if repo.Status.Artifact != nil { + httpChartRepo.CachePath = r.Storage.LocalPath(*repo.GetArtifact()) + httpChartRepo.SetMemCache(r.Storage.LocalPath(*repo.GetArtifact()), r.Cache, r.TTL, func(event string) { + r.IncCacheEvents(event, name, namespace) + }) + } + + chartRepo = httpChartRepo } + return chartRepo, nil } } diff --git a/controllers/helmchart_controller_test.go b/controllers/helmchart_controller_test.go index b9bbe1725..56795f2be 100644 --- a/controllers/helmchart_controller_test.go +++ b/controllers/helmchart_controller_test.go @@ -411,9 +411,6 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) { })) }, }, - //{ - // name: "Error on transient build error", - //}, { name: "Stalling on persistent build error", source: &sourcev1.GitRepository{ @@ -1070,7 +1067,7 @@ func TestHelmChartReconciler_buildFromTarballArtifact(t *testing.T) { assertFunc: func(g *WithT, build chart.Build) { g.Expect(build.Name).To(Equal("helmchartwithdeps")) g.Expect(build.Version).To(Equal("0.1.0")) - g.Expect(build.ResolvedDependencies).To(Equal(3)) + g.Expect(build.ResolvedDependencies).To(Equal(4)) g.Expect(build.Path).To(BeARegularFile()) }, cleanFunc: func(g *WithT, build *chart.Build) { @@ -1178,10 +1175,11 @@ func TestHelmChartReconciler_buildFromTarballArtifact(t *testing.T) { g := NewWithT(t) r := &HelmChartReconciler{ - Client: fake.NewClientBuilder().Build(), - EventRecorder: record.NewFakeRecorder(32), - Storage: storage, - Getters: testGetters, + Client: fake.NewClientBuilder().Build(), + EventRecorder: record.NewFakeRecorder(32), + Storage: storage, + Getters: testGetters, + RegistryClientGenerator: registry.ClientGenerator, } obj := &sourcev1.HelmChart{ diff --git a/controllers/helmrepository_controller_oci.go b/controllers/helmrepository_controller_oci.go index ef084f224..70af64e04 100644 --- a/controllers/helmrepository_controller_oci.go +++ b/controllers/helmrepository_controller_oci.go @@ -326,7 +326,7 @@ func (r *HelmRepositoryOCIReconciler) reconcile(ctx context.Context, obj *v1beta if loginOpts != nil { err = chartRepo.Login(loginOpts...) if err != nil { - e := fmt.Errorf("failed to log into registry '%s': %w", obj.Spec.URL, err) + e := fmt.Errorf("failed to login to registry '%s': %w", obj.Spec.URL, err) conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.AuthenticationFailedReason, e.Error()) result, retErr = ctrl.Result{}, e return diff --git a/controllers/testdata/charts/helmchartwithdeps/Chart.yaml b/controllers/testdata/charts/helmchartwithdeps/Chart.yaml index 99dac50b9..0251612c0 100644 --- a/controllers/testdata/charts/helmchartwithdeps/Chart.yaml +++ b/controllers/testdata/charts/helmchartwithdeps/Chart.yaml @@ -31,3 +31,6 @@ dependencies: - name: grafana version: ">=5.7.0" repository: "https://grafana.github.io/helm-charts" + - name: podinfo + version: ">=6.1.*" + repository: "oci://ghcr.io/stefanprodan/charts" diff --git a/internal/helm/chart/builder_local_test.go b/internal/helm/chart/builder_local_test.go index 655b1709b..626dc072e 100644 --- a/internal/helm/chart/builder_local_test.go +++ b/internal/helm/chart/builder_local_test.go @@ -67,7 +67,7 @@ func TestLocalBuilder_Build(t *testing.T) { reference Reference buildOpts BuildOptions valuesFiles []helmchart.File - repositories map[string]*repository.ChartRepository + repositories map[string]repository.Downloader dependentChartPaths []string wantValues chartutil.Values wantVersion string @@ -146,7 +146,7 @@ fullnameOverride: "full-foo-name-override"`), { name: "chart with dependencies", reference: LocalReference{Path: "../testdata/charts/helmchartwithdeps"}, - repositories: map[string]*repository.ChartRepository{ + repositories: map[string]repository.Downloader{ "https://grafana.github.io/helm-charts/": mockRepo(), }, dependentChartPaths: []string{"./../testdata/charts/helmchart"}, @@ -165,7 +165,7 @@ fullnameOverride: "full-foo-name-override"`), { name: "v1 chart with dependencies", reference: LocalReference{Path: "../testdata/charts/helmchartwithdeps-v1"}, - repositories: map[string]*repository.ChartRepository{ + repositories: map[string]repository.Downloader{ "https://grafana.github.io/helm-charts/": mockRepo(), }, dependentChartPaths: []string{"../testdata/charts/helmchart-v1"}, diff --git a/internal/helm/chart/builder_remote.go b/internal/helm/chart/builder_remote.go index 31dd5be49..d15e24299 100644 --- a/internal/helm/chart/builder_remote.go +++ b/internal/helm/chart/builder_remote.go @@ -34,24 +34,16 @@ import ( "github.com/fluxcd/source-controller/internal/fs" "github.com/fluxcd/source-controller/internal/helm/chart/secureloader" + "github.com/fluxcd/source-controller/internal/helm/repository" ) -// Repository is a repository.ChartRepository or a repository.OCIChartRepository. -// It is used to download a chart from a remote Helm repository or OCI registry. -type Repository interface { - // GetChartVersion returns the repo.ChartVersion for the given name and version. - GetChartVersion(name, version string) (*repo.ChartVersion, error) - // GetChartVersion returns a chart.ChartVersion from the remote repository. - DownloadChart(chart *repo.ChartVersion) (*bytes.Buffer, error) -} - type remoteChartBuilder struct { - remote Repository + remote repository.Downloader } // NewRemoteBuilder returns a Builder capable of building a Helm -// chart with a RemoteReference in the given repository.ChartRepository. -func NewRemoteBuilder(repository Repository) Builder { +// chart with a RemoteReference in the given repository.Downloader. +func NewRemoteBuilder(repository repository.Downloader) Builder { return &remoteChartBuilder{ remote: repository, } @@ -132,7 +124,7 @@ func (b *remoteChartBuilder) Build(_ context.Context, ref Reference, p string, o return result, nil } -func (b *remoteChartBuilder) downloadFromRepository(remote Repository, remoteRef RemoteReference, opts BuildOptions) (*bytes.Buffer, *Build, error) { +func (b *remoteChartBuilder) downloadFromRepository(remote repository.Downloader, remoteRef RemoteReference, opts BuildOptions) (*bytes.Buffer, *Build, error) { // Get the current version for the RemoteReference cv, err := remote.GetChartVersion(remoteRef.Name, remoteRef.Version) if err != nil { diff --git a/internal/helm/chart/dependency_manager.go b/internal/helm/chart/dependency_manager.go index 6080f9862..83dcac762 100644 --- a/internal/helm/chart/dependency_manager.go +++ b/internal/helm/chart/dependency_manager.go @@ -30,26 +30,27 @@ import ( "golang.org/x/sync/errgroup" "golang.org/x/sync/semaphore" helmchart "helm.sh/helm/v3/pkg/chart" + "k8s.io/apimachinery/pkg/util/errors" "github.com/fluxcd/source-controller/internal/helm/chart/secureloader" "github.com/fluxcd/source-controller/internal/helm/repository" ) -// GetChartRepositoryCallback must return a repository.ChartRepository for the -// URL, or an error describing why it could not be returned. -type GetChartRepositoryCallback func(url string) (*repository.ChartRepository, error) +// GetChartDownloaderCallback must return a Downloader for the +// URL or an error describing why it could not be returned. +type GetChartDownloaderCallback func(url string) (repository.Downloader, error) // DependencyManager manages dependencies for a Helm chart. type DependencyManager struct { - // repositories contains a map of repository.ChartRepository objects + // downloaders contains a map of Downloader objects // indexed by their repository.NormalizeURL. // It is consulted as a lookup table for missing dependencies, based on // the (repository) URL the dependency refers to. - repositories map[string]*repository.ChartRepository + downloaders map[string]repository.Downloader - // getRepositoryCallback can be set to an on-demand GetChartRepositoryCallback - // whose returned result is cached to repositories. - getRepositoryCallback GetChartRepositoryCallback + // getChartDownloaderCallback can be set to an on-demand GetChartDownloaderCallback + // whose returned result is cached to downloaders. + getChartDownloaderCallback GetChartDownloaderCallback // concurrent is the number of concurrent chart-add operations during // Build. Defaults to 1 (non-concurrent). @@ -64,16 +65,16 @@ type DependencyManagerOption interface { applyToDependencyManager(dm *DependencyManager) } -type WithRepositories map[string]*repository.ChartRepository +type WithRepositories map[string]repository.Downloader func (o WithRepositories) applyToDependencyManager(dm *DependencyManager) { - dm.repositories = o + dm.downloaders = o } -type WithRepositoryCallback GetChartRepositoryCallback +type WithDownloaderCallback GetChartDownloaderCallback -func (o WithRepositoryCallback) applyToDependencyManager(dm *DependencyManager) { - dm.getRepositoryCallback = GetChartRepositoryCallback(o) +func (o WithDownloaderCallback) applyToDependencyManager(dm *DependencyManager) { + dm.getChartDownloaderCallback = GetChartDownloaderCallback(o) } type WithConcurrent int64 @@ -92,20 +93,14 @@ func NewDependencyManager(opts ...DependencyManagerOption) *DependencyManager { return dm } -// Clear iterates over the repositories, calling Unload and RemoveCache on all -// items. It returns a collection of (cache removal) errors. -func (dm *DependencyManager) Clear() []error { +// Clear iterates over the downloaders, calling Clear on all +// items. It returns an aggregate error of all Clear errors. +func (dm *DependencyManager) Clear() error { var errs []error - for _, v := range dm.repositories { - if err := v.CacheIndexInMemory(); err != nil { - errs = append(errs, err) - } - v.Unload() - if err := v.RemoveCache(); err != nil { - errs = append(errs, err) - } + for _, v := range dm.downloaders { + errs = append(errs, v.Clear()) } - return errs + return errors.NewAggregate(errs) } // Build compiles a set of missing dependencies from chart.Chart, and attempts to @@ -236,13 +231,9 @@ func (dm *DependencyManager) addRemoteDependency(chart *chartWithLock, dep *helm return err } - if err = repo.StrategicallyLoadIndex(); err != nil { - return fmt.Errorf("failed to load index for '%s': %w", dep.Name, err) - } - ver, err := repo.GetChartVersion(dep.Name, dep.Version) if err != nil { - return err + return fmt.Errorf("failed to get chart '%s' version '%s' from '%s': %w", dep.Name, dep.Version, dep.Repository, err) } res, err := repo.DownloadChart(ver) if err != nil { @@ -259,27 +250,29 @@ func (dm *DependencyManager) addRemoteDependency(chart *chartWithLock, dep *helm return nil } -// resolveRepository first attempts to resolve the url from the repositories, falling back -// to getRepositoryCallback if set. It returns the resolved Index, or an error. -func (dm *DependencyManager) resolveRepository(url string) (_ *repository.ChartRepository, err error) { +// resolveRepository first attempts to resolve the url from the downloaders, falling back +// to getDownloaderCallback if set. It returns the resolved Index, or an error. +func (dm *DependencyManager) resolveRepository(url string) (repo repository.Downloader, err error) { dm.mu.Lock() defer dm.mu.Unlock() nUrl := repository.NormalizeURL(url) - if _, ok := dm.repositories[nUrl]; !ok { - if dm.getRepositoryCallback == nil { + if _, ok := dm.downloaders[nUrl]; !ok { + if dm.getChartDownloaderCallback == nil { err = fmt.Errorf("no chart repository for URL '%s'", nUrl) return } - if dm.repositories == nil { - dm.repositories = map[string]*repository.ChartRepository{} + + if dm.downloaders == nil { + dm.downloaders = map[string]repository.Downloader{} } - if dm.repositories[nUrl], err = dm.getRepositoryCallback(nUrl); err != nil { + + if dm.downloaders[nUrl], err = dm.getChartDownloaderCallback(nUrl); err != nil { err = fmt.Errorf("failed to get chart repository for URL '%s': %w", nUrl, err) return } } - return dm.repositories[nUrl], nil + return dm.downloaders[nUrl], nil } // secureLocalChartPath returns the secure absolute path of a local dependency. diff --git a/internal/helm/chart/dependency_manager_test.go b/internal/helm/chart/dependency_manager_test.go index d3e5ee173..d6d871c4e 100644 --- a/internal/helm/chart/dependency_manager_test.go +++ b/internal/helm/chart/dependency_manager_test.go @@ -21,6 +21,7 @@ import ( "context" "errors" "fmt" + "net/url" "os" "path/filepath" "sync" @@ -29,12 +30,38 @@ import ( . "github.com/onsi/gomega" helmchart "helm.sh/helm/v3/pkg/chart" helmgetter "helm.sh/helm/v3/pkg/getter" + "helm.sh/helm/v3/pkg/registry" "helm.sh/helm/v3/pkg/repo" "github.com/fluxcd/source-controller/internal/helm/chart/secureloader" "github.com/fluxcd/source-controller/internal/helm/repository" ) +type mockTagsGetter struct { + tags map[string][]string +} + +func (m *mockTagsGetter) Tags(requestURL string) ([]string, error) { + u, err := url.Parse(requestURL) + if err != nil { + return nil, err + } + + name := filepath.Base(u.Path) + if tags, ok := m.tags[name]; ok { + return tags, nil + } + return nil, fmt.Errorf("no tags found for %s with requestURL %s", name, requestURL) +} + +func (m *mockTagsGetter) Login(_ string, _ ...registry.LoginOption) error { + return nil +} + +func (m *mockTagsGetter) Logout(_ string, _ ...registry.LogoutOption) error { + return nil +} + // mockGetter is a simple mocking getter.Getter implementation, returning // a byte response to any provided URL. type mockGetter struct { @@ -49,25 +76,42 @@ func (g *mockGetter) Get(_ string, _ ...helmgetter.Option) (*bytes.Buffer, error func TestDependencyManager_Clear(t *testing.T) { g := NewWithT(t) - repos := map[string]*repository.ChartRepository{ - "with index": { + file, err := os.CreateTemp("", "") + g.Expect(err).ToNot(HaveOccurred()) + ociRepoWithCreds, err := repository.NewOCIChartRepository("oci://example.com", repository.WithCredentialsFile(file.Name())) + g.Expect(err).ToNot(HaveOccurred()) + + downloaders := map[string]repository.Downloader{ + "with index": &repository.ChartRepository{ Index: repo.NewIndexFile(), RWMutex: &sync.RWMutex{}, }, - "cached cache path": { + "cached cache path": &repository.ChartRepository{ CachePath: "/invalid/path/resets", Cached: true, RWMutex: &sync.RWMutex{}, }, + "with credentials": ociRepoWithCreds, + "without credentials": &repository.OCIChartRepository{}, } - dm := NewDependencyManager(WithRepositories(repos)) + dm := NewDependencyManager(WithRepositories(downloaders)) g.Expect(dm.Clear()).To(BeNil()) - g.Expect(dm.repositories).To(HaveLen(len(repos))) - for _, v := range repos { - g.Expect(v.Index).To(BeNil()) - g.Expect(v.CachePath).To(BeEmpty()) - g.Expect(v.Cached).To(BeFalse()) + g.Expect(dm.downloaders).To(HaveLen(len(downloaders))) + for _, v := range downloaders { + switch v := v.(type) { + case *repository.ChartRepository: + g.Expect(v.Index).To(BeNil()) + g.Expect(v.CachePath).To(BeEmpty()) + g.Expect(v.Cached).To(BeFalse()) + case *repository.OCIChartRepository: + g.Expect(v.HasCredentials()).To(BeFalse()) + } + } + + if _, err := os.Stat(file.Name()); !errors.Is(err, os.ErrNotExist) { + err = os.Remove(file.Name()) + g.Expect(err).ToNot(HaveOccurred()) } } @@ -80,8 +124,22 @@ func TestDependencyManager_Build(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) g.Expect(chartGrafana).ToNot(BeEmpty()) - mockRepo := func() *repository.ChartRepository { - return &repository.ChartRepository{ + mockrepos := []repository.Downloader{ + &repository.OCIChartRepository{ + URL: url.URL{ + Scheme: "oci", + Host: "example.com", + }, + Client: &mockGetter{ + Response: chartGrafana, + }, + RegistryClient: &mockTagsGetter{ + tags: map[string][]string{ + "grafana": {"6.17.4"}, + }, + }, + }, + &repository.ChartRepository{ Client: &mockGetter{ Response: chartGrafana, }, @@ -99,15 +157,21 @@ func TestDependencyManager_Build(t *testing.T) { }, }, RWMutex: &sync.RWMutex{}, - } + }, + } + + for _, repo := range mockrepos { + build(t, repo) } +} +func build(t *testing.T, mockRepo repository.Downloader) { tests := []struct { name string baseDir string path string - repositories map[string]*repository.ChartRepository - getChartRepositoryCallback GetChartRepositoryCallback + downloaders map[string]repository.Downloader + getChartDownloaderCallback GetChartDownloaderCallback want int wantChartFunc func(g *WithT, c *helmchart.Chart) wantErr string @@ -140,10 +204,10 @@ func TestDependencyManager_Build(t *testing.T) { name: "build with dependencies using lock file", baseDir: "./../testdata/charts", path: "helmchartwithdeps", - repositories: map[string]*repository.ChartRepository{ - "https://grafana.github.io/helm-charts/": mockRepo(), + downloaders: map[string]repository.Downloader{ + "https://grafana.github.io/helm-charts/": mockRepo, }, - getChartRepositoryCallback: func(url string) (*repository.ChartRepository, error) { + getChartDownloaderCallback: func(url string) (repository.Downloader, error) { return &repository.ChartRepository{URL: "https://grafana.github.io/helm-charts/"}, nil }, wantChartFunc: func(g *WithT, c *helmchart.Chart) { @@ -170,8 +234,8 @@ func TestDependencyManager_Build(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) dm := NewDependencyManager( - WithRepositories(tt.repositories), - WithRepositoryCallback(tt.getChartRepositoryCallback), + WithRepositories(tt.downloaders), + WithDownloaderCallback(tt.getChartDownloaderCallback), ) absBaseDir, err := filepath.Abs(tt.baseDir) g.Expect(err).ToNot(HaveOccurred()) @@ -319,16 +383,16 @@ func TestDependencyManager_addRemoteDependency(t *testing.T) { g.Expect(chartB).ToNot(BeEmpty()) tests := []struct { - name string - repositories map[string]*repository.ChartRepository - dep *helmchart.Dependency - wantFunc func(g *WithT, c *helmchart.Chart) - wantErr string + name string + downloaders map[string]repository.Downloader + dep *helmchart.Dependency + wantFunc func(g *WithT, c *helmchart.Chart) + wantErr string }{ { name: "adds remote dependency", - repositories: map[string]*repository.ChartRepository{ - "https://example.com/": { + downloaders: map[string]repository.Downloader{ + "https://example.com/": &repository.ChartRepository{ Client: &mockGetter{ Response: chartB, }, @@ -357,8 +421,8 @@ func TestDependencyManager_addRemoteDependency(t *testing.T) { }, }, { - name: "resolve repository error", - repositories: map[string]*repository.ChartRepository{}, + name: "resolve repository error", + downloaders: map[string]repository.Downloader{}, dep: &helmchart.Dependency{ Repository: "https://example.com", }, @@ -366,8 +430,8 @@ func TestDependencyManager_addRemoteDependency(t *testing.T) { }, { name: "strategic load error", - repositories: map[string]*repository.ChartRepository{ - "https://example.com/": { + downloaders: map[string]repository.Downloader{ + "https://example.com/": &repository.ChartRepository{ CachePath: "/invalid/cache/path/foo", RWMutex: &sync.RWMutex{}, }, @@ -379,8 +443,8 @@ func TestDependencyManager_addRemoteDependency(t *testing.T) { }, { name: "repository get error", - repositories: map[string]*repository.ChartRepository{ - "https://example.com/": { + downloaders: map[string]repository.Downloader{ + "https://example.com/": &repository.ChartRepository{ Index: &repo.IndexFile{}, RWMutex: &sync.RWMutex{}, }, @@ -392,8 +456,8 @@ func TestDependencyManager_addRemoteDependency(t *testing.T) { }, { name: "repository version constraint error", - repositories: map[string]*repository.ChartRepository{ - "https://example.com/": { + downloaders: map[string]repository.Downloader{ + "https://example.com/": &repository.ChartRepository{ Index: &repo.IndexFile{ Entries: map[string]repo.ChartVersions{ chartName: { @@ -418,8 +482,8 @@ func TestDependencyManager_addRemoteDependency(t *testing.T) { }, { name: "repository chart download error", - repositories: map[string]*repository.ChartRepository{ - "https://example.com/": { + downloaders: map[string]repository.Downloader{ + "https://example.com/": &repository.ChartRepository{ Index: &repo.IndexFile{ Entries: map[string]repo.ChartVersions{ chartName: { @@ -444,8 +508,8 @@ func TestDependencyManager_addRemoteDependency(t *testing.T) { }, { name: "chart load error", - repositories: map[string]*repository.ChartRepository{ - "https://example.com/": { + downloaders: map[string]repository.Downloader{ + "https://example.com/": &repository.ChartRepository{ Client: &mockGetter{}, Index: &repo.IndexFile{ Entries: map[string]repo.ChartVersions{ @@ -476,7 +540,137 @@ func TestDependencyManager_addRemoteDependency(t *testing.T) { g := NewWithT(t) dm := &DependencyManager{ - repositories: tt.repositories, + downloaders: tt.downloaders, + } + chart := &helmchart.Chart{} + err := dm.addRemoteDependency(&chartWithLock{Chart: chart}, tt.dep) + if tt.wantErr != "" { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.wantErr)) + return + } + g.Expect(err).ToNot(HaveOccurred()) + if tt.wantFunc != nil { + tt.wantFunc(g, chart) + } + }) + } +} + +func TestDependencyManager_addRemoteOCIDependency(t *testing.T) { + g := NewWithT(t) + + chartB, err := os.ReadFile("../testdata/charts/helmchart-0.1.0.tgz") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(chartB).ToNot(BeEmpty()) + + tests := []struct { + name string + downloaders map[string]repository.Downloader + dep *helmchart.Dependency + wantFunc func(g *WithT, c *helmchart.Chart) + wantErr string + }{ + { + name: "adds remote oci dependency", + downloaders: map[string]repository.Downloader{ + "oci://example.com": &repository.OCIChartRepository{ + URL: url.URL{ + Scheme: "oci", + Host: "example.com", + }, + Client: &mockGetter{ + Response: chartB, + }, + RegistryClient: &mockTagsGetter{ + tags: map[string][]string{ + "helmchart": {"0.1.0"}, + }, + }, + }, + }, + dep: &helmchart.Dependency{ + Name: chartName, + Repository: "oci://example.com", + }, + wantFunc: func(g *WithT, c *helmchart.Chart) { + g.Expect(c.Dependencies()).To(HaveLen(1)) + }, + }, + { + name: "remote oci repository fetch tags error", + downloaders: map[string]repository.Downloader{ + "oci://example.com": &repository.OCIChartRepository{ + URL: url.URL{ + Scheme: "oci", + Host: "example.com", + }, + RegistryClient: &mockTagsGetter{ + tags: map[string][]string{}, + }, + }, + }, + dep: &helmchart.Dependency{ + Name: chartName, + Repository: "oci://example.com", + }, + wantErr: fmt.Sprintf("no tags found for %s", chartName), + }, + { + name: "remote oci repository version constraint error", + downloaders: map[string]repository.Downloader{ + "oci://example.com": &repository.OCIChartRepository{ + URL: url.URL{ + Scheme: "oci", + Host: "example.com", + }, + Client: &mockGetter{ + Response: chartB, + }, + RegistryClient: &mockTagsGetter{ + tags: map[string][]string{ + "helmchart": {"0.1.0"}, + }, + }, + }, + }, + dep: &helmchart.Dependency{ + Name: chartName, + Version: "0.2.0", + Repository: "oci://example.com", + }, + wantErr: "could not locate a version matching provided version string 0.2.0", + }, + { + name: "chart load error", + downloaders: map[string]repository.Downloader{ + "oci://example.com": &repository.OCIChartRepository{ + URL: url.URL{ + Scheme: "oci", + Host: "example.com", + }, + Client: &mockGetter{}, + RegistryClient: &mockTagsGetter{ + tags: map[string][]string{ + "helmchart": {"0.1.0"}, + }, + }, + }, + }, + dep: &helmchart.Dependency{ + Name: chartName, + Version: chartVersion, + Repository: "oci://example.com", + }, + wantErr: "failed to load downloaded archive of version '0.1.0'", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + dm := &DependencyManager{ + downloaders: tt.downloaders, } chart := &helmchart.Chart{} err := dm.addRemoteDependency(&chartWithLock{Chart: chart}, tt.dep) @@ -496,54 +690,98 @@ func TestDependencyManager_addRemoteDependency(t *testing.T) { func TestDependencyManager_resolveRepository(t *testing.T) { tests := []struct { name string - repositories map[string]*repository.ChartRepository - getChartRepositoryCallback GetChartRepositoryCallback + downloaders map[string]repository.Downloader + getChartDownloaderCallback GetChartDownloaderCallback url string - want *repository.ChartRepository - wantRepositories map[string]*repository.ChartRepository + want repository.Downloader + wantDownloaders map[string]repository.Downloader wantErr string }{ { - name: "resolves from repositories index", + name: "resolves from downloaders index", url: "https://example.com", - repositories: map[string]*repository.ChartRepository{ - "https://example.com/": {URL: "https://example.com"}, + downloaders: map[string]repository.Downloader{ + "https://example.com/": &repository.ChartRepository{URL: "https://example.com"}, }, want: &repository.ChartRepository{URL: "https://example.com"}, }, { name: "resolves from callback", url: "https://example.com", - getChartRepositoryCallback: func(url string) (*repository.ChartRepository, error) { + getChartDownloaderCallback: func(_ string) (repository.Downloader, error) { return &repository.ChartRepository{URL: "https://example.com"}, nil }, want: &repository.ChartRepository{URL: "https://example.com"}, - wantRepositories: map[string]*repository.ChartRepository{ - "https://example.com/": {URL: "https://example.com"}, + wantDownloaders: map[string]repository.Downloader{ + "https://example.com/": &repository.ChartRepository{URL: "https://example.com"}, }, }, { name: "error from callback", url: "https://example.com", - getChartRepositoryCallback: func(url string) (*repository.ChartRepository, error) { + getChartDownloaderCallback: func(_ string) (repository.Downloader, error) { return nil, errors.New("a very unique error") }, - wantErr: "a very unique error", - wantRepositories: map[string]*repository.ChartRepository{}, + wantErr: "a very unique error", + wantDownloaders: map[string]repository.Downloader{}, }, { name: "error on not found", url: "https://example.com", wantErr: "no chart repository for URL", }, + { + name: "resolves from oci repository", + url: "oci://example.com", + downloaders: map[string]repository.Downloader{ + "oci://example.com": &repository.OCIChartRepository{ + URL: url.URL{ + Scheme: "oci", + Host: "example.com", + }, + }, + }, + want: &repository.OCIChartRepository{ + URL: url.URL{ + Scheme: "oci", + Host: "example.com", + }, + }, + }, + { + name: "resolves oci repository from callback", + url: "oci://example.com", + getChartDownloaderCallback: func(_ string) (repository.Downloader, error) { + return &repository.OCIChartRepository{ + URL: url.URL{ + Scheme: "oci", + Host: "example.com"}, + }, nil + }, + want: &repository.OCIChartRepository{ + URL: url.URL{ + Scheme: "oci", + Host: "example.com", + }, + }, + + wantDownloaders: map[string]repository.Downloader{ + "oci://example.com": &repository.OCIChartRepository{ + URL: url.URL{ + Scheme: "oci", + Host: "example.com", + }, + }, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) dm := &DependencyManager{ - repositories: tt.repositories, - getRepositoryCallback: tt.getChartRepositoryCallback, + downloaders: tt.downloaders, + getChartDownloaderCallback: tt.getChartDownloaderCallback, } got, err := dm.resolveRepository(tt.url) @@ -556,8 +794,8 @@ func TestDependencyManager_resolveRepository(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) g.Expect(got).To(Equal(tt.want)) - if tt.wantRepositories != nil { - g.Expect(dm.repositories).To(Equal(tt.wantRepositories)) + if tt.wantDownloaders != nil { + g.Expect(dm.downloaders).To(Equal(tt.wantDownloaders)) } }) } diff --git a/internal/helm/registry/client.go b/internal/helm/registry/client.go index 9cb68a451..1247347ab 100644 --- a/internal/helm/registry/client.go +++ b/internal/helm/registry/client.go @@ -21,6 +21,7 @@ import ( "os" "helm.sh/helm/v3/pkg/registry" + "k8s.io/apimachinery/pkg/util/errors" ) // ClientGenerator generates a registry client and a temporary credential file. @@ -30,16 +31,25 @@ func ClientGenerator(isLogin bool) (*registry.Client, string, error) { if isLogin { // create a temporary file to store the credentials // this is needed because otherwise the credentials are stored in ~/.docker/config.json. - credentialFile, err := os.CreateTemp("", "credentials") + credentialsFile, err := os.CreateTemp("", "credentials") if err != nil { return nil, "", err } - rClient, err := registry.NewClient(registry.ClientOptWriter(io.Discard), registry.ClientOptCredentialsFile(credentialFile.Name())) + var errs []error + rClient, err := registry.NewClient(registry.ClientOptWriter(io.Discard), registry.ClientOptCredentialsFile(credentialsFile.Name())) if err != nil { - return nil, "", err + errs = append(errs, err) + // attempt to delete the temporary file + if credentialsFile != nil { + err := os.Remove(credentialsFile.Name()) + if err != nil { + errs = append(errs, err) + } + } + return nil, "", errors.NewAggregate(errs) } - return rClient, credentialFile.Name(), nil + return rClient, credentialsFile.Name(), nil } rClient, err := registry.NewClient(registry.ClientOptWriter(io.Discard)) diff --git a/internal/helm/repository/chart_repository.go b/internal/helm/repository/chart_repository.go index 5ff8206c2..282d49a5d 100644 --- a/internal/helm/repository/chart_repository.go +++ b/internal/helm/repository/chart_repository.go @@ -35,6 +35,7 @@ import ( "github.com/Masterminds/semver/v3" "helm.sh/helm/v3/pkg/getter" "helm.sh/helm/v3/pkg/repo" + kerrors "k8s.io/apimachinery/pkg/util/errors" "sigs.k8s.io/yaml" "github.com/fluxcd/pkg/version" @@ -476,9 +477,10 @@ func (r *ChartRepository) Unload() { r.Index = nil } -// Clear cache the index in memory before unloading it. +// Clear caches the index in memory before unloading it. // It cleans up temporary files and directories created by the repository. -func (r *ChartRepository) Clear() (errs []error) { +func (r *ChartRepository) Clear() error { + var errs []error if err := r.CacheIndexInMemory(); err != nil { errs = append(errs, err) } @@ -489,7 +491,7 @@ func (r *ChartRepository) Clear() (errs []error) { errs = append(errs, err) } - return + return kerrors.NewAggregate(errs) } // SetMemCache sets the cache to use for this repository. diff --git a/internal/helm/repository/oci_chart_repository.go b/internal/helm/repository/oci_chart_repository.go index 2dffe1b20..b9bb21312 100644 --- a/internal/helm/repository/oci_chart_repository.go +++ b/internal/helm/repository/oci_chart_repository.go @@ -21,6 +21,7 @@ import ( "crypto/tls" "fmt" "net/url" + "os" "path" "sort" "strings" @@ -60,6 +61,8 @@ type OCIChartRepository struct { // RegistryClient is a client to use while downloading tags or charts from a registry. RegistryClient RegistryClient + // credentialsFile is a temporary credentials file to use while downloading tags or charts from a registry. + credentialsFile string } // OCIChartRepositoryOption is a function that can be passed to NewOCIChartRepository @@ -94,6 +97,14 @@ func WithOCIGetterOptions(getterOpts []getter.Option) OCIChartRepositoryOption { } } +// WithCredentialsFile returns a ChartRepositoryOption that will set the credentials file +func WithCredentialsFile(credentialsFile string) OCIChartRepositoryOption { + return func(r *OCIChartRepository) error { + r.credentialsFile = credentialsFile + return nil + } +} + // NewOCIChartRepository constructs and returns a new ChartRepository with // the ChartRepository.Client configured to the getter.Getter for the // repository URL scheme. It returns an error on URL parsing failures. @@ -126,7 +137,7 @@ func (r *OCIChartRepository) GetChartVersion(name, ver string) (*repo.ChartVersi cpURL.Path = path.Join(cpURL.Path, name) cvs, err := r.getTags(cpURL.String()) if err != nil { - return nil, err + return nil, fmt.Errorf("could not get tags for %q: %s", name, err) } if len(cvs) == 0 { @@ -153,7 +164,7 @@ func (r *OCIChartRepository) getTags(ref string) ([]string, error) { // Retrieve list of repository tags tags, err := r.RegistryClient.Tags(strings.TrimPrefix(ref, fmt.Sprintf("%s://", registry.OCIScheme))) if err != nil { - return nil, err + return nil, fmt.Errorf("could not fetch tags for %q: %s", ref, err) } if len(tags) == 0 { return nil, fmt.Errorf("unable to locate any tags in provided repository: %s", ref) @@ -206,6 +217,23 @@ func (r *OCIChartRepository) Logout() error { return nil } +// HasCredentials returns true if the OCIChartRepository has credentials. +func (r *OCIChartRepository) HasCredentials() bool { + return r.credentialsFile != "" +} + +// Clear deletes the OCI registry credentials file. +func (r *OCIChartRepository) Clear() error { + // clean the credentials file if it exists + if r.credentialsFile != "" { + if err := os.Remove(r.credentialsFile); err != nil { + return err + } + } + r.credentialsFile = "" + return nil +} + // getLastMatchingVersionOrConstraint returns the last version that matches the given version string. // If the version string is empty, the highest available version is returned. func getLastMatchingVersionOrConstraint(cvs []string, ver string) (string, error) { diff --git a/internal/helm/repository/repository.go b/internal/helm/repository/repository.go new file mode 100644 index 000000000..4c8cb7ff8 --- /dev/null +++ b/internal/helm/repository/repository.go @@ -0,0 +1,35 @@ +/* +Copyright 2022 The Flux authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package repository + +import ( + "bytes" + + "helm.sh/helm/v3/pkg/repo" +) + +// Downloader is used to download a chart from a remote Helm repository or OCI Helm repository. +type Downloader interface { + // GetChartVersion returns the repo.ChartVersion for the given name and version + // from the remote Helm repository or OCI Helm repository. + GetChartVersion(name, version string) (*repo.ChartVersion, error) + // DownloadChart downloads a chart from the remote Helm repository or OCI Helm repository. + DownloadChart(chart *repo.ChartVersion) (*bytes.Buffer, error) + // Clear removes all temporary files created by the downloader, caching the files if the cache is configured, + // and calling garbage collector to remove unused files. + Clear() error +} diff --git a/internal/helm/repository/utils.go b/internal/helm/repository/utils.go index 1abc9dffb..f7f9e9193 100644 --- a/internal/helm/repository/utils.go +++ b/internal/helm/repository/utils.go @@ -18,13 +18,20 @@ package repository import ( "strings" + + helmreg "helm.sh/helm/v3/pkg/registry" ) -// NormalizeURL normalizes a ChartRepository URL by ensuring it ends with a -// single "/". -func NormalizeURL(url string) string { - if url != "" { - return strings.TrimRight(url, "/") + "/" +// NormalizeURL normalizes a ChartRepository URL by its scheme. +func NormalizeURL(repositoryURL string) string { + if repositoryURL == "" { + return "" + } + + if strings.Contains(repositoryURL, helmreg.OCIScheme) { + return strings.TrimRight(repositoryURL, "/") } - return url + + return strings.TrimRight(repositoryURL, "/") + "/" + } diff --git a/internal/helm/repository/utils_test.go b/internal/helm/repository/utils_test.go index bac683b46..3ee77606d 100644 --- a/internal/helm/repository/utils_test.go +++ b/internal/helm/repository/utils_test.go @@ -48,6 +48,16 @@ func TestNormalizeURL(t *testing.T) { url: "", want: "", }, + { + name: "oci with slash", + url: "oci://example.com/", + want: "oci://example.com", + }, + { + name: "oci double slash", + url: "oci://example.com//", + want: "oci://example.com", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) {