diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index 64193fef4..be7871ab2 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" @@ -460,9 +461,10 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * loginOpts []helmreg.LoginOption ) + normalizedURL := strings.TrimSuffix(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), } @@ -490,7 +492,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), @@ -502,7 +504,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), @@ -517,11 +519,11 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * } // Initialize the chart repository - var chartRepo chart.Repository + var chartRepo chart.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) } @@ -550,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(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) } @@ -570,7 +572,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) })) @@ -683,7 +685,7 @@ 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() @@ -912,12 +914,17 @@ func (r *HelmChartReconciler) garbageCollect(ctx context.Context, obj *sourcev1. return nil } -// namespacedChartRepositoryCallback returns a chart.GetChartRepositoryCallback scoped to the given namespace. +// namespacedChartRepositoryCallback returns a chart.GetChartDownloaderCallback 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. -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) (chart.CleanDownloader, error) { + var ( + tlsConfig *tls.Config + loginOpts []helmreg.LoginOption + ) + normalizedURL := strings.TrimSuffix(url, "/") repo, err := r.resolveDependencyRepository(ctx, url, namespace) if err != nil { // Return Kubernetes client errors, but ignore others @@ -932,7 +939,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), } @@ -946,26 +953,72 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont } clientOpts = append(clientOpts, opts...) - tlsConfig, err = getter.TLSClientConfigFromSecret(*secret, repo.Spec.URL) + tlsConfig, err = getter.TLSClientConfigFromSecret(*secret, normalizedURL) if err != nil { return nil, fmt.Errorf("failed to create TLS client config for HelmRepository '%s': %w", repo.Name, err) } - } - chartRepo, err := repository.NewChartRepository(repo.Spec.URL, "", r.Getters, tlsConfig, clientOpts) - if err != nil { - return nil, err + // Build registryClient options from secret + loginOpt, err := registry.LoginOptionFromSecret(normalizedURL, *secret) + if err != nil { + return nil, fmt.Errorf("failed to create login options for HelmRepository '%s': %w", repo.Name, err) + } + + loginOpts = append([]helmreg.LoginOption{}, loginOpt) } - // Ensure that the cache key is the same as the artifact path - // otherwise don't enable caching. We don't want to cache indexes - // for repositories that are not reconciled by the source controller. - if repo.Status.Artifact != nil { - chartRepo.CachePath = r.Storage.LocalPath(*repo.GetArtifact()) - chartRepo.SetMemCache(r.Storage.LocalPath(*repo.GetArtifact()), r.Cache, r.TTL, func(event string) { - r.IncCacheEvents(event, name, namespace) - }) + var chartRepo chart.CleanDownloader + if helmreg.IsOCI(normalizedURL) { + registryClient, file, 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.WithCredentialFile(file)) + if err != nil { + // clean up the file + if err := os.Remove(file); err != nil { + errs = append(errs, err) + } + errs = append(errs, fmt.Errorf("failed to create OCI chart repository for HelmRepository '%s': %w", repo.Name, 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 { + return nil, fmt.Errorf("failed to login to OCI chart repository for HelmRepository '%s': %w", repo.Name, err) + } + } + + chartRepo = ociChartRepo + } else { + httpChartRepo, err := repository.NewChartRepository(normalizedURL, "", r.Getters, tlsConfig, clientOpts) + if err != nil { + return nil, err + } + + // Ensure that the cache key is the same as the artifact path + // otherwise don't enable caching. We don't want to cache indexes + // for repositories that are not reconciled by the source controller. + if repo.Status.Artifact != nil { + httpChartRepo.CachePath = r.Storage.LocalPath(*repo.GetArtifact()) + httpChartRepo.SetMemCache(r.Storage.LocalPath(*repo.GetArtifact()), r.Cache, r.TTL, func(event string) { + r.IncCacheEvents(event, name, namespace) + }) + } + + chartRepo = httpChartRepo } + return chartRepo, nil } } diff --git a/controllers/helmchart_controller_test.go b/controllers/helmchart_controller_test.go index b9bbe1725..69c46bb5a 100644 --- a/controllers/helmchart_controller_test.go +++ b/controllers/helmchart_controller_test.go @@ -1070,7 +1070,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 +1178,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/testdata/charts/helmchartwithdeps/Chart.yaml b/controllers/testdata/charts/helmchartwithdeps/Chart.yaml index 99dac50b9..0251612c0 100644 --- a/controllers/testdata/charts/helmchartwithdeps/Chart.yaml +++ b/controllers/testdata/charts/helmchartwithdeps/Chart.yaml @@ -31,3 +31,6 @@ dependencies: - name: grafana version: ">=5.7.0" repository: "https://grafana.github.io/helm-charts" + - name: podinfo + version: ">=6.1.*" + repository: "oci://ghcr.io/stefanprodan/charts" diff --git a/internal/helm/chart/builder_local_test.go b/internal/helm/chart/builder_local_test.go index 655b1709b..b6b596f01 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]CleanDownloader 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]CleanDownloader{ "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]CleanDownloader{ "https://grafana.github.io/helm-charts/": mockRepo(), }, dependentChartPaths: []string{"../testdata/charts/helmchart-v1"}, diff --git a/internal/helm/chart/builder_remote.go b/internal/helm/chart/builder_remote.go index 31dd5be49..cc05113ff 100644 --- a/internal/helm/chart/builder_remote.go +++ b/internal/helm/chart/builder_remote.go @@ -36,22 +36,22 @@ import ( "github.com/fluxcd/source-controller/internal/helm/chart/secureloader" ) -// 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. +// Downloader is used to download a chart from a remote Helm repository or OCI registry. +type Downloader interface { + // GetChartVersion returns the repo.ChartVersion for the given name and version + // from the remote repository.ChartRepository. GetChartVersion(name, version string) (*repo.ChartVersion, error) - // GetChartVersion returns a chart.ChartVersion from the remote repository. + // DownloadChart downloads a chart from the remote Helm repository or OCI registry. DownloadChart(chart *repo.ChartVersion) (*bytes.Buffer, error) } type remoteChartBuilder struct { - remote Repository + remote 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 Downloader. +func NewRemoteBuilder(repository Downloader) Builder { return &remoteChartBuilder{ remote: repository, } @@ -132,7 +132,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 Downloader, remoteRef RemoteReference, opts BuildOptions) (*bytes.Buffer, *Build, error) { // Get the current version for the RemoteReference cv, err := remote.GetChartVersion(remoteRef.Name, remoteRef.Version) if err != nil { diff --git a/internal/helm/chart/dependency_manager.go b/internal/helm/chart/dependency_manager.go index 6080f9862..ce2323ec2 100644 --- a/internal/helm/chart/dependency_manager.go +++ b/internal/helm/chart/dependency_manager.go @@ -35,21 +35,29 @@ import ( "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) +// CleanDownloader is a Downloader that cleans temporary files created by the downloader, +// caching the files if the cache is configured, +// and calling garbage collector to remove unused files. +type CleanDownloader interface { + Clean() []error + Downloader +} + +// GetChartDownloaderCallback must return a Downloader for the +// URL or an error describing why it could not be returned. +type GetChartDownloaderCallback func(url string) (CleanDownloader, 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]CleanDownloader - // 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 +72,16 @@ type DependencyManagerOption interface { applyToDependencyManager(dm *DependencyManager) } -type WithRepositories map[string]*repository.ChartRepository +type WithRepositories map[string]CleanDownloader 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,18 +100,13 @@ 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. +// Clear iterates over the downloaders, calling Clean on all +// items. +// It returns a collection of 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.Clean()...) } return errs } @@ -236,13 +239,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 version '%s' from '%s': %w", dep.Version, dep.Repository, err) } res, err := repo.DownloadChart(ver) if err != nil { @@ -259,27 +258,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) (_ 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]CleanDownloader{} } - if dm.repositories[nUrl], err = dm.getRepositoryCallback(nUrl); err != nil { + + if dm.downloaders[nUrl], err = dm.getChartDownloaderCallback(nUrl); err != nil { err = fmt.Errorf("failed to get chart repository for URL '%s': %w", nUrl, err) return } } - return dm.repositories[nUrl], nil + return dm.downloaders[nUrl], nil } // secureLocalChartPath returns the secure absolute path of a local dependency. diff --git a/internal/helm/chart/dependency_manager_test.go b/internal/helm/chart/dependency_manager_test.go index d3e5ee173..2b8b11568 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", name) +} + +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,27 @@ 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": { + downloaders := map[string]CleanDownloader{ + "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{}, }, } - 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 { + repo, ok := v.(*repository.ChartRepository) + g.Expect(ok).To(BeTrue()) + g.Expect(repo.Index).To(BeNil()) + g.Expect(repo.CachePath).To(BeEmpty()) + g.Expect(repo.Cached).To(BeFalse()) } } @@ -106,8 +135,8 @@ func TestDependencyManager_Build(t *testing.T) { name string baseDir string path string - repositories map[string]*repository.ChartRepository - getChartRepositoryCallback GetChartRepositoryCallback + downloaders map[string]CleanDownloader + getChartDownloaderCallback GetChartDownloaderCallback want int wantChartFunc func(g *WithT, c *helmchart.Chart) wantErr string @@ -140,10 +169,10 @@ func TestDependencyManager_Build(t *testing.T) { name: "build with dependencies using lock file", baseDir: "./../testdata/charts", path: "helmchartwithdeps", - repositories: map[string]*repository.ChartRepository{ + downloaders: map[string]CleanDownloader{ "https://grafana.github.io/helm-charts/": mockRepo(), }, - getChartRepositoryCallback: func(url string) (*repository.ChartRepository, error) { + getChartDownloaderCallback: func(url string) (CleanDownloader, error) { return &repository.ChartRepository{URL: "https://grafana.github.io/helm-charts/"}, nil }, wantChartFunc: func(g *WithT, c *helmchart.Chart) { @@ -170,8 +199,125 @@ 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()) + got, err := dm.Build(context.TODO(), LocalReference{WorkDir: absBaseDir, Path: tt.path}, chart) + + if tt.wantErr != "" { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.wantErr)) + g.Expect(got).To(BeZero()) + return + } + + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(got).To(Equal(tt.want)) + if tt.wantChartFunc != nil { + tt.wantChartFunc(g, chart) + } + }) + } +} + +func TestDependencyManager_OCIBuild(t *testing.T) { + g := NewWithT(t) + + // Mock chart used as grafana chart in the test below. The cached repository + // takes care of the actual grafana related details in the chart index. + chartGrafana, err := os.ReadFile("./../testdata/charts/helmchart-0.1.0.tgz") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(chartGrafana).ToNot(BeEmpty()) + + mockRepo := func() *repository.OCIChartRepository { + return &repository.OCIChartRepository{ + URL: url.URL{ + Scheme: "oci", + Host: "example.com", + }, + Client: &mockGetter{ + Response: chartGrafana, + }, + RegistryClient: &mockTagsGetter{ + tags: map[string][]string{ + "grafana": {"6.17.4"}, + }, + }, + } + } + + tests := []struct { + name string + baseDir string + path string + downloaders map[string]CleanDownloader + getChartDownloaderCallback GetChartDownloaderCallback + want int + wantChartFunc func(g *WithT, c *helmchart.Chart) + wantErr string + }{ + { + name: "build failure returns error", + baseDir: "./../testdata/charts", + path: "helmchartwithdeps", + wantErr: "failed to add remote dependency 'grafana': no chart repository for URL", + }, + { + name: "no dependencies returns zero", + baseDir: "./../testdata/charts", + path: "helmchart", + wantChartFunc: func(g *WithT, c *helmchart.Chart) { + g.Expect(c.Dependencies()).To(HaveLen(0)) + }, + want: 0, + }, + { + name: "no dependency returns zero - v1", + baseDir: "./../testdata/charts", + path: "helmchart-v1", + wantChartFunc: func(g *WithT, c *helmchart.Chart) { + g.Expect(c.Dependencies()).To(HaveLen(0)) + }, + want: 0, + }, + { + name: "build with dependencies using lock file", + baseDir: "./../testdata/charts", + path: "helmchartwithdeps", + downloaders: map[string]CleanDownloader{ + "https://grafana.github.io/helm-charts/": mockRepo(), + }, + getChartDownloaderCallback: func(url string) (CleanDownloader, error) { + return &repository.ChartRepository{URL: "https://grafana.github.io/helm-charts/"}, nil + }, + wantChartFunc: func(g *WithT, c *helmchart.Chart) { + g.Expect(c.Dependencies()).To(HaveLen(2)) + g.Expect(c.Lock.Dependencies).To(HaveLen(3)) + }, + want: 2, + }, + { + name: "build with dependencies - v1", + baseDir: "./../testdata/charts", + path: "helmchartwithdeps-v1", + wantChartFunc: func(g *WithT, c *helmchart.Chart) { + g.Expect(c.Dependencies()).To(HaveLen(1)) + }, + want: 1, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + chart, err := secureloader.Load(tt.baseDir, tt.path) + g.Expect(err).ToNot(HaveOccurred()) + + dm := NewDependencyManager( + WithRepositories(tt.downloaders), + WithDownloaderCallback(tt.getChartDownloaderCallback), ) absBaseDir, err := filepath.Abs(tt.baseDir) g.Expect(err).ToNot(HaveOccurred()) @@ -319,16 +465,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]CleanDownloader + 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]CleanDownloader{ + "https://example.com/": &repository.ChartRepository{ Client: &mockGetter{ Response: chartB, }, @@ -357,8 +503,8 @@ func TestDependencyManager_addRemoteDependency(t *testing.T) { }, }, { - name: "resolve repository error", - repositories: map[string]*repository.ChartRepository{}, + name: "resolve repository error", + downloaders: map[string]CleanDownloader{}, dep: &helmchart.Dependency{ Repository: "https://example.com", }, @@ -366,8 +512,8 @@ func TestDependencyManager_addRemoteDependency(t *testing.T) { }, { name: "strategic load error", - repositories: map[string]*repository.ChartRepository{ - "https://example.com/": { + downloaders: map[string]CleanDownloader{ + "https://example.com/": &repository.ChartRepository{ CachePath: "/invalid/cache/path/foo", RWMutex: &sync.RWMutex{}, }, @@ -379,8 +525,8 @@ func TestDependencyManager_addRemoteDependency(t *testing.T) { }, { name: "repository get error", - repositories: map[string]*repository.ChartRepository{ - "https://example.com/": { + downloaders: map[string]CleanDownloader{ + "https://example.com/": &repository.ChartRepository{ Index: &repo.IndexFile{}, RWMutex: &sync.RWMutex{}, }, @@ -392,8 +538,8 @@ func TestDependencyManager_addRemoteDependency(t *testing.T) { }, { name: "repository version constraint error", - repositories: map[string]*repository.ChartRepository{ - "https://example.com/": { + downloaders: map[string]CleanDownloader{ + "https://example.com/": &repository.ChartRepository{ Index: &repo.IndexFile{ Entries: map[string]repo.ChartVersions{ chartName: { @@ -418,8 +564,8 @@ func TestDependencyManager_addRemoteDependency(t *testing.T) { }, { name: "repository chart download error", - repositories: map[string]*repository.ChartRepository{ - "https://example.com/": { + downloaders: map[string]CleanDownloader{ + "https://example.com/": &repository.ChartRepository{ Index: &repo.IndexFile{ Entries: map[string]repo.ChartVersions{ chartName: { @@ -444,8 +590,8 @@ func TestDependencyManager_addRemoteDependency(t *testing.T) { }, { name: "chart load error", - repositories: map[string]*repository.ChartRepository{ - "https://example.com/": { + downloaders: map[string]CleanDownloader{ + "https://example.com/": &repository.ChartRepository{ Client: &mockGetter{}, Index: &repo.IndexFile{ Entries: map[string]repo.ChartVersions{ @@ -476,7 +622,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]CleanDownloader + dep *helmchart.Dependency + wantFunc func(g *WithT, c *helmchart.Chart) + wantErr string + }{ + { + name: "adds remote oci dependency", + downloaders: map[string]CleanDownloader{ + "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 get tags error", + downloaders: map[string]CleanDownloader{ + "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]CleanDownloader{ + "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]CleanDownloader{ + "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 +772,90 @@ 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]CleanDownloader + getChartDownloaderCallback GetChartDownloaderCallback url string - want *repository.ChartRepository - wantRepositories map[string]*repository.ChartRepository + want CleanDownloader + wantDownloaders map[string]CleanDownloader 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]CleanDownloader{ + "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) (CleanDownloader, 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]CleanDownloader{ + "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) (CleanDownloader, 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]CleanDownloader{}, }, { 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]CleanDownloader{ + "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) (CleanDownloader, 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]CleanDownloader{ + "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 +868,8 @@ func TestDependencyManager_resolveRepository(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) g.Expect(got).To(Equal(tt.want)) - if tt.wantRepositories != nil { - g.Expect(dm.repositories).To(Equal(tt.wantRepositories)) + if tt.wantDownloaders != nil { + g.Expect(dm.downloaders).To(Equal(tt.wantDownloaders)) } }) } diff --git a/internal/helm/registry/client.go b/internal/helm/registry/client.go index 9cb68a451..ea6a34433 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. @@ -35,9 +36,16 @@ func ClientGenerator(isLogin bool) (*registry.Client, string, error) { return nil, "", err } + var errs []error rClient, err := registry.NewClient(registry.ClientOptWriter(io.Discard), registry.ClientOptCredentialsFile(credentialFile.Name())) if err != nil { - return nil, "", err + errs = append(errs, err) + // attempt to delete the temporary file + err := os.Remove(credentialFile.Name()) + if err != nil { + errs = append(errs, err) + } + return nil, "", errors.NewAggregate(errs) } return rClient, credentialFile.Name(), nil } diff --git a/internal/helm/repository/chart_repository.go b/internal/helm/repository/chart_repository.go index 5ff8206c2..7fd324a9e 100644 --- a/internal/helm/repository/chart_repository.go +++ b/internal/helm/repository/chart_repository.go @@ -476,9 +476,9 @@ func (r *ChartRepository) Unload() { r.Index = nil } -// Clear cache the index in memory before unloading it. +// Clean 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) { +func (r *ChartRepository) Clean() (errs []error) { if err := r.CacheIndexInMemory(); err != nil { errs = append(errs, err) } diff --git a/internal/helm/repository/oci_chart_repository.go b/internal/helm/repository/oci_chart_repository.go index d05995e4d..f35aa7274 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" "sort" "strings" @@ -59,6 +60,8 @@ type OCIChartRepository struct { // RegistryClient is a client to use while downloading tags or charts from a registry. RegistryClient RegistryClient + // credentialFile is a temporary credential file to use while downloading tags or charts from a registry. + credentialFile string } // OCIChartRepositoryOption is a function that can be passed to NewOCIChartRepository @@ -93,6 +96,14 @@ func WithOCIGetterOptions(getterOpts []getter.Option) OCIChartRepositoryOption { } } +// WithCredentialFile returns a ChartRepositoryOption that will set the credential file +func WithCredentialFile(credentialFile string) OCIChartRepositoryOption { + return func(r *OCIChartRepository) error { + r.credentialFile = credentialFile + 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. @@ -123,7 +134,7 @@ func (r *OCIChartRepository) GetChartVersion(name, ver string) (*repo.ChartVersi // Either in an index file or from a registry. cvs, err := r.getTags(fmt.Sprintf("%s/%s", r.URL.String(), name)) if err != nil { - return nil, err + return nil, fmt.Errorf("could not get tags for %q: %s", name, err) } if len(cvs) == 0 { @@ -203,6 +214,18 @@ func (r *OCIChartRepository) Logout() error { return nil } +// Clean is a no-op for OCIChartRepository +func (r *OCIChartRepository) Clean() []error { + // clean the credential file if it exists + if r.credentialFile != "" { + if err := os.Remove(r.credentialFile); err != nil { + return []error{err} + } + } + + return []error{} +} + // 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) {