diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index a3bd7d6a8..f9cd8190d 100644 --- a/controllers/helmchart_controller.go +++ b/controllers/helmchart_controller.go @@ -35,7 +35,6 @@ 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" @@ -463,10 +462,9 @@ 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(normalizedURL), + helmgetter.WithURL(repo.Spec.URL), helmgetter.WithTimeout(repo.Spec.Timeout.Duration), helmgetter.WithPassCredentialsAll(repo.Spec.PassCredentials), } @@ -494,7 +492,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * } clientOpts = append(clientOpts, opts...) - tlsConfig, err = getter.TLSClientConfigFromSecret(*secret, normalizedURL) + tlsConfig, err = getter.TLSClientConfigFromSecret(*secret, repo.Spec.URL) if err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to create TLS client config with secret data: %w", err), @@ -506,7 +504,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * } // Build registryClient options from secret - loginOpt, err := registry.LoginOptionFromSecret(normalizedURL, *secret) + loginOpt, err := registry.LoginOptionFromSecret(repo.Spec.URL, *secret) if err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to configure Helm client with secret data: %w", err), @@ -521,11 +519,11 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * } // Initialize the chart repository - var chartRepo repository.Downloader + var chartRepo chart.Remote switch repo.Spec.Type { case sourcev1.HelmRepositoryTypeOCI: - if !helmreg.IsOCI(normalizedURL) { - err := fmt.Errorf("invalid OCI registry URL: %s", normalizedURL) + if !helmreg.IsOCI(repo.Spec.URL) { + err := fmt.Errorf("invalid OCI registry URL: %s", repo.Spec.URL) return chartRepoConfigErrorReturn(err, obj) } @@ -533,7 +531,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, credentialsFile, err := r.RegistryClientGenerator(loginOpts != nil) + registryClient, file, err := r.RegistryClientGenerator(loginOpts != nil) if err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to construct Helm client: %w", err), @@ -543,9 +541,9 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * return sreconcile.ResultEmpty, e } - if credentialsFile != "" { + if file != "" { defer func() { - if err := os.Remove(credentialsFile); err != nil { + if err := os.Remove(file); err != nil { r.eventLogf(ctx, obj, corev1.EventTypeWarning, meta.FailedReason, "failed to delete temporary credentials file: %s", err) } @@ -554,7 +552,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(normalizedURL, repository.WithOCIGetter(r.Getters), repository.WithOCIGetterOptions(clientOpts), repository.WithOCIRegistryClient(registryClient)) + ociChartRepo, err := repository.NewOCIChartRepository(repo.Spec.URL, repository.WithOCIGetter(r.Getters), repository.WithOCIGetterOptions(clientOpts), repository.WithOCIRegistryClient(registryClient)) if err != nil { return chartRepoConfigErrorReturn(err, obj) } @@ -574,7 +572,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * } } default: - httpChartRepo, err := repository.NewChartRepository(normalizedURL, r.Storage.LocalPath(*repo.GetArtifact()), r.Getters, tlsConfig, clientOpts, + httpChartRepo, err := repository.NewChartRepository(repo.Spec.URL, 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) })) @@ -687,15 +685,9 @@ func (r *HelmChartReconciler) buildFromTarballArtifact(ctx context.Context, obj // Setup dependency manager dm := chart.NewDependencyManager( - chart.WithDownloaderCallback(r.namespacedChartRepositoryCallback(ctx, obj.GetName(), obj.GetNamespace())), + chart.WithRepositoryCallback(r.namespacedChartRepositoryCallback(ctx, obj.GetName(), obj.GetNamespace())), ) - defer func() { - err := dm.Clear() - if err != nil { - r.eventLogf(ctx, obj, corev1.EventTypeWarning, meta.FailedReason, - "dependency manager cleanup error: %s", err) - } - }() + defer dm.Clear() // Configure builder options, including any previously cached chart opts := chart.BuildOptions{ @@ -922,17 +914,12 @@ func (r *HelmChartReconciler) garbageCollect(ctx context.Context, obj *sourcev1. return nil } -// namespacedChartRepositoryCallback returns a chart.GetChartDownloaderCallback scoped to the given namespace. -// The returned callback returns a repository.Downloader configured with the retrieved v1beta1.HelmRepository, +// namespacedChartRepositoryCallback returns a chart.GetChartRepositoryCallback scoped to the given namespace. +// The returned callback returns a repository.ChartRepository configured with the retrieved v1beta1.HelmRepository, // or a shim with defaults if no object could be found. -// 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) +func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Context, name, namespace string) chart.GetChartRepositoryCallback { + return func(url string) (*repository.ChartRepository, error) { + var tlsConfig *tls.Config repo, err := r.resolveDependencyRepository(ctx, url, namespace) if err != nil { // Return Kubernetes client errors, but ignore others @@ -947,7 +934,7 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont } } clientOpts := []helmgetter.Option{ - helmgetter.WithURL(normalizedURL), + helmgetter.WithURL(repo.Spec.URL), helmgetter.WithTimeout(repo.Spec.Timeout.Duration), helmgetter.WithPassCredentialsAll(repo.Spec.PassCredentials), } @@ -961,77 +948,26 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont } clientOpts = append(clientOpts, opts...) - tlsConfig, err = getter.TLSClientConfigFromSecret(*secret, normalizedURL) + tlsConfig, err = getter.TLSClientConfigFromSecret(*secret, repo.Spec.URL) 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) } - 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 + chartRepo, err := repository.NewChartRepository(repo.Spec.URL, "", 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 { + 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 a98059704..53e000e93 100644 --- a/controllers/helmchart_controller_test.go +++ b/controllers/helmchart_controller_test.go @@ -411,6 +411,9 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) { })) }, }, + //{ + // name: "Error on transient build error", + //}, { name: "Stalling on persistent build error", source: &sourcev1.GitRepository{ @@ -1217,7 +1220,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(4)) + g.Expect(build.ResolvedDependencies).To(Equal(3)) g.Expect(build.Path).To(BeARegularFile()) }, cleanFunc: func(g *WithT, build *chart.Build) { @@ -1325,11 +1328,10 @@ func TestHelmChartReconciler_buildFromTarballArtifact(t *testing.T) { g := NewWithT(t) r := &HelmChartReconciler{ - Client: fake.NewClientBuilder().Build(), - EventRecorder: record.NewFakeRecorder(32), - Storage: storage, - Getters: testGetters, - RegistryClientGenerator: registry.ClientGenerator, + Client: fake.NewClientBuilder().Build(), + EventRecorder: record.NewFakeRecorder(32), + Storage: storage, + Getters: testGetters, } obj := &sourcev1.HelmChart{ diff --git a/controllers/helmrepository_controller_oci.go b/controllers/helmrepository_controller_oci.go index 70af64e04..ef084f224 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 login to registry '%s': %w", obj.Spec.URL, err) + e := fmt.Errorf("failed to log into 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 0251612c0..99dac50b9 100644 --- a/controllers/testdata/charts/helmchartwithdeps/Chart.yaml +++ b/controllers/testdata/charts/helmchartwithdeps/Chart.yaml @@ -31,6 +31,3 @@ 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 626dc072e..655b1709b 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.Downloader + repositories map[string]*repository.ChartRepository 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.Downloader{ + repositories: map[string]*repository.ChartRepository{ "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.Downloader{ + repositories: map[string]*repository.ChartRepository{ "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 d15e24299..0bc632bdf 100644 --- a/internal/helm/chart/builder_remote.go +++ b/internal/helm/chart/builder_remote.go @@ -25,6 +25,7 @@ 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" @@ -34,16 +35,24 @@ 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 repository.Downloader + remote Remote } // NewRemoteBuilder returns a Builder capable of building a Helm -// chart with a RemoteReference in the given repository.Downloader. -func NewRemoteBuilder(repository repository.Downloader) Builder { +// chart with a RemoteReference in the given repository.ChartRepository. +func NewRemoteBuilder(repository Remote) Builder { return &remoteChartBuilder{ remote: repository, } @@ -74,12 +83,31 @@ func (b *remoteChartBuilder) Build(_ context.Context, ref Reference, p string, o return nil, &BuildError{Reason: ErrChartReference, Err: err} } - 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 + 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)} } requiresPackaging := len(opts.GetValuesFiles()) != 0 || opts.VersionMetadata != "" @@ -124,31 +152,66 @@ func (b *remoteChartBuilder) Build(_ context.Context, ref Reference, p string, o return result, nil } -func (b *remoteChartBuilder) downloadFromRepository(remote repository.Downloader, remoteRef RemoteReference, opts BuildOptions) (*bytes.Buffer, *Build, error) { +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} + } + // Get the current version for the RemoteReference - cv, err := remote.GetChartVersion(remoteRef.Name, remoteRef.Version) + 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, nil, &BuildError{Reason: ErrChartReference, Err: err} + return nil, &BuildError{Reason: ErrChartReference, Err: err} } result, shouldReturn, err := generateBuildResult(cv, opts) if err != nil { - return nil, nil, err + return nil, err } + *buildResult = *result if shouldReturn { - return nil, result, nil + 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, nil, &BuildError{Reason: ErrChartPull, Err: err} + return nil, &BuildError{Reason: ErrChartPull, Err: err} } - return res, result, nil + return res, 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 1fbe6328c..83bda46e4 100644 --- a/internal/helm/chart/dependency_manager.go +++ b/internal/helm/chart/dependency_manager.go @@ -30,27 +30,26 @@ 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" ) -// 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) +// 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) // DependencyManager manages dependencies for a Helm chart. type DependencyManager struct { - // downloaders contains a map of Downloader objects + // repositories contains a map of repository.ChartRepository 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. - downloaders map[string]repository.Downloader + repositories map[string]*repository.ChartRepository - // getChartDownloaderCallback can be set to an on-demand GetChartDownloaderCallback - // whose returned result is cached to downloaders. - getChartDownloaderCallback GetChartDownloaderCallback + // getRepositoryCallback can be set to an on-demand GetChartRepositoryCallback + // whose returned result is cached to repositories. + getRepositoryCallback GetChartRepositoryCallback // concurrent is the number of concurrent chart-add operations during // Build. Defaults to 1 (non-concurrent). @@ -65,16 +64,16 @@ type DependencyManagerOption interface { applyToDependencyManager(dm *DependencyManager) } -type WithRepositories map[string]repository.Downloader +type WithRepositories map[string]*repository.ChartRepository func (o WithRepositories) applyToDependencyManager(dm *DependencyManager) { - dm.downloaders = o + dm.repositories = o } -type WithDownloaderCallback GetChartDownloaderCallback +type WithRepositoryCallback GetChartRepositoryCallback -func (o WithDownloaderCallback) applyToDependencyManager(dm *DependencyManager) { - dm.getChartDownloaderCallback = GetChartDownloaderCallback(o) +func (o WithRepositoryCallback) applyToDependencyManager(dm *DependencyManager) { + dm.getRepositoryCallback = GetChartRepositoryCallback(o) } type WithConcurrent int64 @@ -93,16 +92,20 @@ func NewDependencyManager(opts ...DependencyManagerOption) *DependencyManager { return dm } -// Clear iterates over the downloaders, calling Clear on all -// items. It returns an aggregate error of all Clear errors. -func (dm *DependencyManager) Clear() error { +// 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 { var errs []error - for _, v := range dm.downloaders { - if v != nil { - errs = append(errs, v.Clear()) + 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) } } - return errors.NewAggregate(errs) + return errs } // Build compiles a set of missing dependencies from chart.Chart, and attempts to @@ -233,9 +236,13 @@ func (dm *DependencyManager) addRemoteDependency(chart *chartWithLock, dep *helm return err } - ver, err := repo.GetChartVersion(dep.Name, dep.Version) + 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) if err != nil { - return fmt.Errorf("failed to get chart '%s' version '%s' from '%s': %w", dep.Name, dep.Version, dep.Repository, err) + return err } res, err := repo.DownloadChart(ver) if err != nil { @@ -252,9 +259,9 @@ func (dm *DependencyManager) addRemoteDependency(chart *chartWithLock, dep *helm return nil } -// 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) { +// 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) { dm.mu.Lock() defer dm.mu.Unlock() @@ -263,22 +270,20 @@ func (dm *DependencyManager) resolveRepository(url string) (repo repository.Down if err != nil { return } - if _, ok := dm.downloaders[nUrl]; !ok { - if dm.getChartDownloaderCallback == nil { + if _, ok := dm.repositories[nUrl]; !ok { + if dm.getRepositoryCallback == nil { err = fmt.Errorf("no chart repository for URL '%s'", nUrl) return } - - if dm.downloaders == nil { - dm.downloaders = map[string]repository.Downloader{} + if dm.repositories == nil { + dm.repositories = map[string]*repository.ChartRepository{} } - - if dm.downloaders[nUrl], err = dm.getChartDownloaderCallback(nUrl); err != nil { + if dm.repositories[nUrl], err = dm.getRepositoryCallback(nUrl); err != nil { err = fmt.Errorf("failed to get chart repository for URL '%s': %w", nUrl, err) return } } - return dm.downloaders[nUrl], nil + return dm.repositories[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 8a66c9797..4f142e07f 100644 --- a/internal/helm/chart/dependency_manager_test.go +++ b/internal/helm/chart/dependency_manager_test.go @@ -21,7 +21,6 @@ import ( "context" "errors" "fmt" - "net/url" "os" "path/filepath" "sync" @@ -30,38 +29,12 @@ 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 { @@ -76,43 +49,25 @@ func (g *mockGetter) Get(_ string, _ ...helmgetter.Option) (*bytes.Buffer, error func TestDependencyManager_Clear(t *testing.T) { g := NewWithT(t) - 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{ + repos := map[string]*repository.ChartRepository{ + "with index": { Index: repo.NewIndexFile(), RWMutex: &sync.RWMutex{}, }, - "cached cache path": &repository.ChartRepository{ + "cached cache path": { CachePath: "/invalid/path/resets", Cached: true, RWMutex: &sync.RWMutex{}, }, - "with credentials": ociRepoWithCreds, - "without credentials": &repository.OCIChartRepository{}, - "nil downloader": nil, } - dm := NewDependencyManager(WithRepositories(downloaders)) + dm := NewDependencyManager(WithRepositories(repos)) g.Expect(dm.Clear()).To(BeNil()) - 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()) + 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()) } } @@ -125,22 +80,8 @@ func TestDependencyManager_Build(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) g.Expect(chartGrafana).ToNot(BeEmpty()) - 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{ + mockRepo := func() *repository.ChartRepository { + return &repository.ChartRepository{ Client: &mockGetter{ Response: chartGrafana, }, @@ -155,24 +96,19 @@ func TestDependencyManager_Build(t *testing.T) { URLs: []string{"https://example.com/grafana.tgz"}, }, }, + "nil downloader": nil, }, }, 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 - downloaders map[string]repository.Downloader - getChartDownloaderCallback GetChartDownloaderCallback + repositories map[string]*repository.ChartRepository + getChartRepositoryCallback GetChartRepositoryCallback want int wantChartFunc func(g *WithT, c *helmchart.Chart) wantErr string @@ -205,10 +141,10 @@ func build(t *testing.T, mockRepo repository.Downloader) { name: "build with dependencies using lock file", baseDir: "./../testdata/charts", path: "helmchartwithdeps", - downloaders: map[string]repository.Downloader{ - "https://grafana.github.io/helm-charts/": mockRepo, + repositories: map[string]*repository.ChartRepository{ + "https://grafana.github.io/helm-charts/": mockRepo(), }, - getChartDownloaderCallback: func(url string) (repository.Downloader, error) { + getChartRepositoryCallback: func(url string) (*repository.ChartRepository, error) { return &repository.ChartRepository{URL: "https://grafana.github.io/helm-charts/"}, nil }, wantChartFunc: func(g *WithT, c *helmchart.Chart) { @@ -235,8 +171,8 @@ func build(t *testing.T, mockRepo repository.Downloader) { g.Expect(err).ToNot(HaveOccurred()) dm := NewDependencyManager( - WithRepositories(tt.downloaders), - WithDownloaderCallback(tt.getChartDownloaderCallback), + WithRepositories(tt.repositories), + WithRepositoryCallback(tt.getChartRepositoryCallback), ) absBaseDir, err := filepath.Abs(tt.baseDir) g.Expect(err).ToNot(HaveOccurred()) @@ -384,16 +320,16 @@ func TestDependencyManager_addRemoteDependency(t *testing.T) { 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 string + repositories map[string]*repository.ChartRepository + dep *helmchart.Dependency + wantFunc func(g *WithT, c *helmchart.Chart) + wantErr string }{ { name: "adds remote dependency", - downloaders: map[string]repository.Downloader{ - "https://example.com/": &repository.ChartRepository{ + repositories: map[string]*repository.ChartRepository{ + "https://example.com/": { Client: &mockGetter{ Response: chartB, }, @@ -422,25 +358,26 @@ func TestDependencyManager_addRemoteDependency(t *testing.T) { }, }, { - name: "resolve repository error", - downloaders: map[string]repository.Downloader{}, + name: "resolve repository error", + repositories: map[string]*repository.ChartRepository{}, dep: &helmchart.Dependency{ Repository: "https://example.com", }, wantErr: "no chart repository for URL", }, - { - name: "resolve aliased repository error", - downloaders: map[string]repository.Downloader{}, - dep: &helmchart.Dependency{ - Repository: "@fantastic-charts", - }, - wantErr: "aliased repository dependency is not supported", - }, + // Commented due to 770 being reverted + // { + // name: "resolve aliased repository error", + // downloaders: map[string]repository.Downloader{}, + // dep: &helmchart.Dependency{ + // Repository: "@fantastic-charts", + // }, + // wantErr: "aliased repository dependency is not supported", + // }, { name: "strategic load error", - downloaders: map[string]repository.Downloader{ - "https://example.com/": &repository.ChartRepository{ + repositories: map[string]*repository.ChartRepository{ + "https://example.com/": { CachePath: "/invalid/cache/path/foo", RWMutex: &sync.RWMutex{}, }, @@ -452,8 +389,8 @@ func TestDependencyManager_addRemoteDependency(t *testing.T) { }, { name: "repository get error", - downloaders: map[string]repository.Downloader{ - "https://example.com/": &repository.ChartRepository{ + repositories: map[string]*repository.ChartRepository{ + "https://example.com/": { Index: &repo.IndexFile{}, RWMutex: &sync.RWMutex{}, }, @@ -465,8 +402,8 @@ func TestDependencyManager_addRemoteDependency(t *testing.T) { }, { name: "repository version constraint error", - downloaders: map[string]repository.Downloader{ - "https://example.com/": &repository.ChartRepository{ + repositories: map[string]*repository.ChartRepository{ + "https://example.com/": { Index: &repo.IndexFile{ Entries: map[string]repo.ChartVersions{ chartName: { @@ -491,8 +428,8 @@ func TestDependencyManager_addRemoteDependency(t *testing.T) { }, { name: "repository chart download error", - downloaders: map[string]repository.Downloader{ - "https://example.com/": &repository.ChartRepository{ + repositories: map[string]*repository.ChartRepository{ + "https://example.com/": { Index: &repo.IndexFile{ Entries: map[string]repo.ChartVersions{ chartName: { @@ -517,8 +454,8 @@ func TestDependencyManager_addRemoteDependency(t *testing.T) { }, { name: "chart load error", - downloaders: map[string]repository.Downloader{ - "https://example.com/": &repository.ChartRepository{ + repositories: map[string]*repository.ChartRepository{ + "https://example.com/": { Client: &mockGetter{}, Index: &repo.IndexFile{ Entries: map[string]repo.ChartVersions{ @@ -549,137 +486,7 @@ func TestDependencyManager_addRemoteDependency(t *testing.T) { g := NewWithT(t) dm := &DependencyManager{ - 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, + repositories: tt.repositories, } chart := &helmchart.Chart{} err := dm.addRemoteDependency(&chartWithLock{Chart: chart}, tt.dep) @@ -699,98 +506,54 @@ func TestDependencyManager_addRemoteOCIDependency(t *testing.T) { func TestDependencyManager_resolveRepository(t *testing.T) { tests := []struct { name string - downloaders map[string]repository.Downloader - getChartDownloaderCallback GetChartDownloaderCallback + repositories map[string]*repository.ChartRepository + getChartRepositoryCallback GetChartRepositoryCallback url string - want repository.Downloader - wantDownloaders map[string]repository.Downloader + want *repository.ChartRepository + wantRepositories map[string]*repository.ChartRepository wantErr string }{ { - name: "resolves from downloaders index", + name: "resolves from repositories index", url: "https://example.com", - downloaders: map[string]repository.Downloader{ - "https://example.com/": &repository.ChartRepository{URL: "https://example.com"}, + repositories: map[string]*repository.ChartRepository{ + "https://example.com/": {URL: "https://example.com"}, }, want: &repository.ChartRepository{URL: "https://example.com"}, }, { name: "resolves from callback", url: "https://example.com", - getChartDownloaderCallback: func(_ string) (repository.Downloader, error) { + getChartRepositoryCallback: func(url string) (*repository.ChartRepository, error) { return &repository.ChartRepository{URL: "https://example.com"}, nil }, want: &repository.ChartRepository{URL: "https://example.com"}, - wantDownloaders: map[string]repository.Downloader{ - "https://example.com/": &repository.ChartRepository{URL: "https://example.com"}, + wantRepositories: map[string]*repository.ChartRepository{ + "https://example.com/": {URL: "https://example.com"}, }, }, { name: "error from callback", url: "https://example.com", - getChartDownloaderCallback: func(_ string) (repository.Downloader, error) { + getChartRepositoryCallback: func(url string) (*repository.ChartRepository, error) { return nil, errors.New("a very unique error") }, - wantErr: "a very unique error", - wantDownloaders: map[string]repository.Downloader{}, + wantErr: "a very unique error", + wantRepositories: map[string]*repository.ChartRepository{}, }, { 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{ - downloaders: tt.downloaders, - getChartDownloaderCallback: tt.getChartDownloaderCallback, + repositories: tt.repositories, + getRepositoryCallback: tt.getChartRepositoryCallback, } got, err := dm.resolveRepository(tt.url) @@ -803,8 +566,8 @@ func TestDependencyManager_resolveRepository(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) g.Expect(got).To(Equal(tt.want)) - if tt.wantDownloaders != nil { - g.Expect(dm.downloaders).To(Equal(tt.wantDownloaders)) + if tt.wantRepositories != nil { + g.Expect(dm.repositories).To(Equal(tt.wantRepositories)) } }) } diff --git a/internal/helm/registry/client.go b/internal/helm/registry/client.go index 1247347ab..9cb68a451 100644 --- a/internal/helm/registry/client.go +++ b/internal/helm/registry/client.go @@ -21,7 +21,6 @@ import ( "os" "helm.sh/helm/v3/pkg/registry" - "k8s.io/apimachinery/pkg/util/errors" ) // ClientGenerator generates a registry client and a temporary credential file. @@ -31,25 +30,16 @@ 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. - credentialsFile, err := os.CreateTemp("", "credentials") + credentialFile, err := os.CreateTemp("", "credentials") if err != nil { return nil, "", err } - var errs []error - rClient, err := registry.NewClient(registry.ClientOptWriter(io.Discard), registry.ClientOptCredentialsFile(credentialsFile.Name())) + rClient, err := registry.NewClient(registry.ClientOptWriter(io.Discard), registry.ClientOptCredentialsFile(credentialFile.Name())) if err != nil { - 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 nil, "", err } - return rClient, credentialsFile.Name(), nil + return rClient, credentialFile.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 282d49a5d..c54888daa 100644 --- a/internal/helm/repository/chart_repository.go +++ b/internal/helm/repository/chart_repository.go @@ -35,7 +35,6 @@ 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" @@ -151,15 +150,10 @@ func newChartRepository() *ChartRepository { } } -// GetChartVersion returns the repo.ChartVersion for the given name, the version is expected +// Get 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) 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 - } - +func (r *ChartRepository) Get(name, ver string) (*repo.ChartVersion, error) { r.RLock() defer r.RUnlock() @@ -477,23 +471,6 @@ 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 ef7f5c9c3..5bd8600f3 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.GetChartVersion(tt.chartName, tt.chartVersion) + cv, err := r.Get(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 b9bb21312..e68a350d8 100644 --- a/internal/helm/repository/oci_chart_repository.go +++ b/internal/helm/repository/oci_chart_repository.go @@ -21,7 +21,6 @@ import ( "crypto/tls" "fmt" "net/url" - "os" "path" "sort" "strings" @@ -61,8 +60,6 @@ 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 @@ -97,14 +94,6 @@ 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,18 +115,18 @@ func NewOCIChartRepository(repositoryURL string, chartRepoOpts ...OCIChartReposi return r, nil } -// GetChartVersion returns the repo.ChartVersion for the given name, the version is expected +// Get 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) GetChartVersion(name, ver string) (*repo.ChartVersion, error) { +func (r *OCIChartRepository) Get(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, fmt.Errorf("could not get tags for %q: %s", name, err) + return nil, err } if len(cvs) == 0 { @@ -164,7 +153,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, fmt.Errorf("could not fetch tags for %q: %s", ref, err) + return nil, err } if len(tags) == 0 { return nil, fmt.Errorf("unable to locate any tags in provided repository: %s", ref) @@ -217,23 +206,6 @@ 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 89e7b470e..a41f2dd99 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.GetChartVersion(chart, tc.version) + cv, err := r.Get(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 deleted file mode 100644 index 4c8cb7ff8..000000000 --- a/internal/helm/repository/repository.go +++ /dev/null @@ -1,35 +0,0 @@ -/* -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 5d5ab2548..a5a33b581 100644 --- a/internal/helm/repository/utils.go +++ b/internal/helm/repository/utils.go @@ -39,13 +39,10 @@ func NormalizeURL(repositoryURL string) string { if repositoryURL == "" { return "" } - if strings.Contains(repositoryURL, helmreg.OCIScheme) { return strings.TrimRight(repositoryURL, "/") } - return strings.TrimRight(repositoryURL, "/") + "/" - } // ValidateDepURL returns an error if the given depended repository URL declaration is not supported diff --git a/internal/helm/repository/utils_test.go b/internal/helm/repository/utils_test.go index 3ee77606d..bac683b46 100644 --- a/internal/helm/repository/utils_test.go +++ b/internal/helm/repository/utils_test.go @@ -48,16 +48,6 @@ 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) {