diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index 1e9581b70..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.Remote + 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 0bc632bdf..d15e24299 100644 --- a/internal/helm/chart/builder_remote.go +++ b/internal/helm/chart/builder_remote.go @@ -25,7 +25,6 @@ import ( "path/filepath" "github.com/Masterminds/semver/v3" - "github.com/fluxcd/source-controller/internal/helm/repository" helmchart "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/chartutil" "helm.sh/helm/v3/pkg/repo" @@ -35,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" ) -// Remote is a repository.ChartRepository or a repository.OCIChartRepository. -// It is used to download a chart from a remote Helm repository or OCI registry. -type Remote interface { - // GetChart returns a chart.Chart from the remote repository. - Get(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 Remote + remote repository.Downloader } // NewRemoteBuilder returns a Builder capable of building a Helm -// chart with a RemoteReference in the given repository.ChartRepository. -func NewRemoteBuilder(repository Remote) Builder { +// chart with a RemoteReference in the given repository.Downloader. +func NewRemoteBuilder(repository repository.Downloader) Builder { return &remoteChartBuilder{ remote: repository, } @@ -83,31 +74,12 @@ func (b *remoteChartBuilder) Build(_ context.Context, ref Reference, p string, o return nil, &BuildError{Reason: ErrChartReference, Err: err} } - var ( - res *bytes.Buffer - err error - ) - - result := &Build{} - switch b.remote.(type) { - case *repository.ChartRepository: - res, err = b.downloadFromRepository(b.remote.(*repository.ChartRepository), remoteRef, result, opts) - if err != nil { - return nil, &BuildError{Reason: ErrChartPull, Err: err} - } - if res == nil { - return result, nil - } - case *repository.OCIChartRepository: - res, err = b.downloadFromOCIRepository(b.remote.(*repository.OCIChartRepository), remoteRef, result, opts) - if err != nil { - return nil, &BuildError{Reason: ErrChartPull, Err: err} - } - if res == nil { - return result, nil - } - default: - return nil, &BuildError{Reason: ErrChartReference, Err: fmt.Errorf("unsupported remote type %T", b.remote)} + res, result, err := b.downloadFromRepository(b.remote, remoteRef, opts) + if err != nil { + return nil, &BuildError{Reason: ErrChartPull, Err: err} + } + if res == nil { + return result, nil } requiresPackaging := len(opts.GetValuesFiles()) != 0 || opts.VersionMetadata != "" @@ -152,66 +124,31 @@ func (b *remoteChartBuilder) Build(_ context.Context, ref Reference, p string, o return result, nil } -func (b *remoteChartBuilder) downloadFromOCIRepository(remote *repository.OCIChartRepository, remoteRef RemoteReference, buildResult *Build, opts BuildOptions) (*bytes.Buffer, error) { - cv, err := remote.Get(remoteRef.Name, remoteRef.Version) - if err != nil { - err = fmt.Errorf("failed to get chart version for remote reference: %w", err) - return nil, &BuildError{Reason: ErrChartPull, Err: err} - } - - result, shouldReturn, err := generateBuildResult(cv, opts) - if err != nil { - return nil, err - } - - if shouldReturn { - *buildResult = *result - return nil, nil - } - - // Download the package for the resolved version - res, err := remote.DownloadChart(cv) - if err != nil { - err = fmt.Errorf("failed to download chart for remote reference: %w", err) - return nil, &BuildError{Reason: ErrChartPull, Err: err} - } - - *buildResult = *result - - return res, nil -} - -func (b *remoteChartBuilder) downloadFromRepository(remote *repository.ChartRepository, remoteRef RemoteReference, buildResult *Build, opts BuildOptions) (*bytes.Buffer, error) { - if err := remote.StrategicallyLoadIndex(); err != nil { - err = fmt.Errorf("could not load repository index for remote chart reference: %w", err) - return nil, &BuildError{Reason: ErrChartPull, Err: err} - } - +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.Get(remoteRef.Name, remoteRef.Version) + cv, err := remote.GetChartVersion(remoteRef.Name, remoteRef.Version) if err != nil { err = fmt.Errorf("failed to get chart version for remote reference: %w", err) - return nil, &BuildError{Reason: ErrChartReference, Err: err} + return nil, nil, &BuildError{Reason: ErrChartReference, Err: err} } result, shouldReturn, err := generateBuildResult(cv, opts) if err != nil { - return nil, err + return nil, nil, err } - *buildResult = *result if shouldReturn { - return nil, nil + return nil, result, nil } // Download the package for the resolved version res, err := remote.DownloadChart(cv) if err != nil { err = fmt.Errorf("failed to download chart for remote reference: %w", err) - return nil, &BuildError{Reason: ErrChartPull, Err: err} + return nil, nil, &BuildError{Reason: ErrChartPull, Err: err} } - return res, nil + return res, result, nil } // generateBuildResult returns a Build object generated from the given chart version and build options. It also returns diff --git a/internal/helm/chart/dependency_manager.go b/internal/helm/chart/dependency_manager.go index 7206efff2..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.Get(dep.Name, dep.Version) + 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 c54888daa..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" @@ -150,10 +151,15 @@ func newChartRepository() *ChartRepository { } } -// Get returns the repo.ChartVersion for the given name, the version is expected +// GetChartVersion returns the repo.ChartVersion for the given name, the version is expected // to be a semver.Constraints compatible string. If version is empty, the latest // stable version will be returned and prerelease versions will be ignored. -func (r *ChartRepository) Get(name, ver string) (*repo.ChartVersion, error) { +func (r *ChartRepository) GetChartVersion(name, ver string) (*repo.ChartVersion, error) { + // See if we already have the index in cache or try to load it. + if err := r.StrategicallyLoadIndex(); err != nil { + return nil, err + } + r.RLock() defer r.RUnlock() @@ -471,6 +477,23 @@ func (r *ChartRepository) Unload() { r.Index = nil } +// Clear caches the index in memory before unloading it. +// It cleans up temporary files and directories created by the repository. +func (r *ChartRepository) Clear() error { + var errs []error + if err := r.CacheIndexInMemory(); err != nil { + errs = append(errs, err) + } + + r.Unload() + + if err := r.RemoveCache(); err != nil { + errs = append(errs, err) + } + + return kerrors.NewAggregate(errs) +} + // SetMemCache sets the cache to use for this repository. func (r *ChartRepository) SetMemCache(key string, c *cache.Cache, ttl time.Duration, rec RecordMetricsFunc) { r.IndexKey = key diff --git a/internal/helm/repository/chart_repository_test.go b/internal/helm/repository/chart_repository_test.go index 5bd8600f3..ef7f5c9c3 100644 --- a/internal/helm/repository/chart_repository_test.go +++ b/internal/helm/repository/chart_repository_test.go @@ -181,7 +181,7 @@ func TestChartRepository_Get(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - cv, err := r.Get(tt.chartName, tt.chartVersion) + cv, err := r.GetChartVersion(tt.chartName, tt.chartVersion) if tt.wantErr != "" { g.Expect(err).To(HaveOccurred()) g.Expect(err.Error()).To(ContainSubstring(tt.wantErr)) diff --git a/internal/helm/repository/oci_chart_repository.go b/internal/helm/repository/oci_chart_repository.go index e68a350d8..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. @@ -115,18 +126,18 @@ func NewOCIChartRepository(repositoryURL string, chartRepoOpts ...OCIChartReposi return r, nil } -// Get returns the repo.ChartVersion for the given name, the version is expected +// GetChartVersion returns the repo.ChartVersion for the given name, the version is expected // to be a semver.Constraints compatible string. If version is empty, the latest // stable version will be returned and prerelease versions will be ignored. // adapted from https://github.com/helm/helm/blob/49819b4ef782e80b0c7f78c30bd76b51ebb56dc8/pkg/downloader/chart_downloader.go#L162 -func (r *OCIChartRepository) Get(name, ver string) (*repo.ChartVersion, error) { +func (r *OCIChartRepository) GetChartVersion(name, ver string) (*repo.ChartVersion, error) { // Find chart versions matching the given name. // Either in an index file or from a registry. cpURL := r.URL 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/oci_chart_repository_test.go b/internal/helm/repository/oci_chart_repository_test.go index a41f2dd99..89e7b470e 100644 --- a/internal/helm/repository/oci_chart_repository_test.go +++ b/internal/helm/repository/oci_chart_repository_test.go @@ -183,7 +183,7 @@ func TestOCIChartRepository_Get(t *testing.T) { g.Expect(r).ToNot(BeNil()) chart := "podinfo" - cv, err := r.Get(chart, tc.version) + cv, err := r.GetChartVersion(chart, tc.version) if tc.expectedErr != "" { g.Expect(err).To(HaveOccurred()) g.Expect(err.Error()).To(Equal(tc.expectedErr)) 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) {