From b402e546bc950979d27566325013b4bff0d2a38e Mon Sep 17 00:00:00 2001 From: Soule BA Date: Sun, 5 Jun 2022 23:13:08 +0200 Subject: [PATCH 1/2] Refactor repository logic Signed-off-by: Soule BA --- controllers/helmchart_controller.go | 2 +- internal/helm/chart/builder_remote.go | 93 ++++--------------- internal/helm/chart/dependency_manager.go | 2 +- internal/helm/repository/chart_repository.go | 25 ++++- .../helm/repository/chart_repository_test.go | 2 +- .../helm/repository/oci_chart_repository.go | 4 +- .../repository/oci_chart_repository_test.go | 2 +- 7 files changed, 48 insertions(+), 82 deletions(-) diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index 1e9581b7..2fa63ecf 100644 --- a/controllers/helmchart_controller.go +++ b/controllers/helmchart_controller.go @@ -518,7 +518,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * } // Initialize the chart repository - var chartRepo chart.Remote + var chartRepo chart.Repository switch repo.Spec.Type { case sourcev1.HelmRepositoryTypeOCI: if !helmreg.IsOCI(repo.Spec.URL) { diff --git a/internal/helm/chart/builder_remote.go b/internal/helm/chart/builder_remote.go index 0bc632bd..31dd5be4 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" @@ -37,22 +36,22 @@ import ( "github.com/fluxcd/source-controller/internal/helm/chart/secureloader" ) -// Remote is a repository.ChartRepository or a repository.OCIChartRepository. +// 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 Remote interface { - // GetChart returns a chart.Chart from the remote repository. - Get(name, version string) (*repo.ChartVersion, error) +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 Remote + remote Repository } // NewRemoteBuilder returns a Builder capable of building a Helm // chart with a RemoteReference in the given repository.ChartRepository. -func NewRemoteBuilder(repository Remote) Builder { +func NewRemoteBuilder(repository Repository) Builder { return &remoteChartBuilder{ remote: repository, } @@ -83,31 +82,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 +132,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, 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 7206efff..6080f986 100644 --- a/internal/helm/chart/dependency_manager.go +++ b/internal/helm/chart/dependency_manager.go @@ -240,7 +240,7 @@ func (dm *DependencyManager) addRemoteDependency(chart *chartWithLock, dep *helm 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 } diff --git a/internal/helm/repository/chart_repository.go b/internal/helm/repository/chart_repository.go index c54888da..5ff8206c 100644 --- a/internal/helm/repository/chart_repository.go +++ b/internal/helm/repository/chart_repository.go @@ -150,10 +150,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 +476,22 @@ func (r *ChartRepository) Unload() { r.Index = nil } +// Clear cache the index in memory before unloading it. +// It cleans up temporary files and directories created by the repository. +func (r *ChartRepository) Clear() (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 +} + // 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 5bd8600f..ef7f5c9c 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 e68a350d..2dffe1b2 100644 --- a/internal/helm/repository/oci_chart_repository.go +++ b/internal/helm/repository/oci_chart_repository.go @@ -115,11 +115,11 @@ 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 diff --git a/internal/helm/repository/oci_chart_repository_test.go b/internal/helm/repository/oci_chart_repository_test.go index a41f2dd9..89e7b470 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)) From 361b975bf46c19f4dd8783dec183c344fe17e5f9 Mon Sep 17 00:00:00 2001 From: Soule BA Date: Tue, 5 Jul 2022 16:52:33 +0200 Subject: [PATCH 2/2] 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 2fa63ecf..d2c1ad60 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) } + + // 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) } - chartRepo, err := repository.NewChartRepository(repo.Spec.URL, "", r.Getters, tlsConfig, clientOpts) - if err != nil { - return nil, err + 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 } - // 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) - }) - } return chartRepo, nil } } diff --git a/controllers/helmchart_controller_test.go b/controllers/helmchart_controller_test.go index b9bbe172..56795f2b 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 ef084f22..70af64e0 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 99dac50b..0251612c 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 655b1709..626dc072 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 31dd5be4..d15e2429 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 6080f986..83dcac76 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 d3e5ee17..d6d871c4 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 9cb68a45..1247347a 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 5ff8206c..282d49a5 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 2dffe1b2..b9bb2131 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 00000000..4c8cb7ff --- /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 1abc9dff..f7f9e919 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 "" } - return url + + if strings.Contains(repositoryURL, helmreg.OCIScheme) { + return strings.TrimRight(repositoryURL, "/") + } + + return strings.TrimRight(repositoryURL, "/") + "/" + } diff --git a/internal/helm/repository/utils_test.go b/internal/helm/repository/utils_test.go index bac683b4..3ee77606 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) {