fixup! Enable remote dependencies from OCI repositories

Author: souleb

controllers/helmchart_controller.go

@@ -461,7 +461,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 		loginOpts []helmreg.LoginOption
 	)
 
-	normalizedURL := strings.TrimSuffix(repo.Spec.URL, "/")
+	normalizedURL := repository.NormalizeURL(repo.Spec.URL)
 	// Construct the Getter options from the HelmRepository data
 	clientOpts := []helmgetter.Option{
 		helmgetter.WithURL(normalizedURL),
@@ -519,7 +519,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 	}
 
 	// Initialize the chart repository
-	var chartRepo chart.Downloader
+	var chartRepo repository.Downloader
 	switch repo.Spec.Type {
 	case sourcev1.HelmRepositoryTypeOCI:
 		if !helmreg.IsOCI(normalizedURL) {
@@ -687,7 +687,13 @@ func (r *HelmChartReconciler) buildFromTarballArtifact(ctx context.Context, obj
 	dm := chart.NewDependencyManager(
 		chart.WithDownloaderCallback(r.namespacedChartRepositoryCallback(ctx, obj.GetName(), obj.GetNamespace())),
 	)
-	defer dm.Clear()
+	defer func() {
+		err := dm.Clear()
+		if err != nil {
+			r.eventLogf(ctx, obj, corev1.EventTypeWarning, meta.FailedReason,
+				"dependency manager cleanup error: %s", err)
+		}
+	}()
 
 	// Configure builder options, including any previously cached chart
 	opts := chart.BuildOptions{
@@ -915,16 +921,16 @@ func (r *HelmChartReconciler) garbageCollect(ctx context.Context, obj *sourcev1.
 }
 
 // namespacedChartRepositoryCallback returns a chart.GetChartDownloaderCallback scoped to the given namespace.
-// The returned callback returns a repository.ChartRepository configured with the retrieved v1beta1.HelmRepository,
+// The returned callback returns a repository.Downloader 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) (chart.CleanableDownloader, error) {
+	return func(url string) (repository.Downloader, error) {
 		var (
 			tlsConfig *tls.Config
 			loginOpts []helmreg.LoginOption
 		)
-		normalizedURL := strings.TrimSuffix(url, "/")
+		normalizedURL := repository.NormalizeURL(url)
 		repo, err := r.resolveDependencyRepository(ctx, url, namespace)
 		if err != nil {
 			// Return Kubernetes client errors, but ignore others
@@ -967,7 +973,7 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont
 			loginOpts = append([]helmreg.LoginOption{}, loginOpt)
 		}
 
-		var chartRepo chart.CleanableDownloader
+		var chartRepo repository.Downloader
 		if helmreg.IsOCI(normalizedURL) {
 			registryClient, file, err := r.RegistryClientGenerator(loginOpts != nil)
 			if err != nil {
@@ -980,13 +986,15 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont
 			ociChartRepo, err := repository.NewOCIChartRepository(normalizedURL, repository.WithOCIGetter(r.Getters),
 				repository.WithOCIGetterOptions(clientOpts),
 				repository.WithOCIRegistryClient(registryClient),
-				repository.WithCredentialFile(file))
+				repository.WithCredentialsFile(file))
 			if err != nil {
+				errs = append(errs, fmt.Errorf("failed to create OCI chart repository for HelmRepository '%s': %w", repo.Name, err))
 				// clean up the file
-				if err := os.Remove(file); err != nil {
-					errs = append(errs, err)
+				if 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)
 			}
 
@@ -995,7 +1003,14 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont
 			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)
+					errs = append(errs, fmt.Errorf("failed to login to OCI chart repository for HelmRepository '%s': %w", repo.Name, err))
+					// clean up the file
+					if file != "" {
+						if err := os.Remove(file); err != nil {
+							errs = append(errs, err)
+						}
+					}
+					return nil, kerrors.NewAggregate(errs)
 				}
 			}
 

controllers/helmchart_controller_test.go

@@ -411,9 +411,6 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) {
 				}))
 			},
 		},
-		//{
-		//	name: "Error on transient build error",
-		//},
 		{
 			name: "Stalling on persistent build error",
 			source: &sourcev1.GitRepository{

controllers/helmrepository_controller_oci.go

@@ -326,7 +326,7 @@ func (r *HelmRepositoryOCIReconciler) reconcile(ctx context.Context, obj *v1beta
 	if loginOpts != nil {
 		err = chartRepo.Login(loginOpts...)
 		if err != nil {
-			e := fmt.Errorf("failed to log into registry '%s': %w", obj.Spec.URL, err)
+			e := fmt.Errorf("failed to login to registry '%s': %w", obj.Spec.URL, err)
 			conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.AuthenticationFailedReason, e.Error())
 			result, retErr = ctrl.Result{}, e
 			return

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]CleanableDownloader
+		repositories        map[string]repository.Downloader
 		dependentChartPaths []string
 		wantValues          chartutil.Values
 		wantVersion         string
@@ -146,7 +146,7 @@ fullnameOverride: "full-foo-name-override"`),
 		{
 			name:      "chart with dependencies",
 			reference: LocalReference{Path: "../testdata/charts/helmchartwithdeps"},
-			repositories: map[string]CleanableDownloader{
+			repositories: map[string]repository.Downloader{
 				"https://grafana.github.io/helm-charts/": mockRepo(),
 			},
 			dependentChartPaths: []string{"./../testdata/charts/helmchart"},
@@ -165,7 +165,7 @@ fullnameOverride: "full-foo-name-override"`),
 		{
 			name:      "v1 chart with dependencies",
 			reference: LocalReference{Path: "../testdata/charts/helmchartwithdeps-v1"},
-			repositories: map[string]CleanableDownloader{
+			repositories: map[string]repository.Downloader{
 				"https://grafana.github.io/helm-charts/": mockRepo(),
 			},
 			dependentChartPaths: []string{"../testdata/charts/helmchart-v1"},

internal/helm/chart/builder_remote.go

@@ -34,24 +34,16 @@ import (
 
 	"github.com/fluxcd/source-controller/internal/fs"
 	"github.com/fluxcd/source-controller/internal/helm/chart/secureloader"
+	"github.com/fluxcd/source-controller/internal/helm/repository"
 )
 
-// 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)
-	// DownloadChart downloads a chart from the remote Helm repository or OCI registry.
-	DownloadChart(chart *repo.ChartVersion) (*bytes.Buffer, error)
-}
-
 type remoteChartBuilder struct {
-	remote Downloader
+	remote repository.Downloader
 }
 
 // NewRemoteBuilder returns a Builder capable of building a Helm
-// chart with a RemoteReference in the given Downloader.
-func NewRemoteBuilder(repository Downloader) Builder {
+// chart with a RemoteReference in the given repository.Downloader.
+func NewRemoteBuilder(repository repository.Downloader) Builder {
 	return &remoteChartBuilder{
 		remote: repository,
 	}
@@ -132,7 +124,7 @@ func (b *remoteChartBuilder) Build(_ context.Context, ref Reference, p string, o
 	return result, nil
 }
 
-func (b *remoteChartBuilder) downloadFromRepository(remote Downloader, remoteRef RemoteReference, opts BuildOptions) (*bytes.Buffer, *Build, error) {
+func (b *remoteChartBuilder) downloadFromRepository(remote repository.Downloader, remoteRef RemoteReference, opts BuildOptions) (*bytes.Buffer, *Build, error) {
 	// Get the current version for the RemoteReference
 	cv, err := remote.GetChartVersion(remoteRef.Name, remoteRef.Version)
 	if err != nil {

internal/helm/chart/dependency_manager.go

@@ -30,30 +30,23 @@ 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"
 )
 
-// CleanableDownloader 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 CleanableDownloader 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) (CleanableDownloader, error)
+type GetChartDownloaderCallback func(url string) (repository.Downloader, error)
 
 // DependencyManager manages dependencies for a Helm chart.
 type DependencyManager struct {
 	// 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.
-	downloaders map[string]CleanableDownloader
+	downloaders map[string]repository.Downloader
 
 	// getChartDownloaderCallback can be set to an on-demand GetChartDownloaderCallback
 	// whose returned result is cached to downloaders.
@@ -72,7 +65,7 @@ type DependencyManagerOption interface {
 	applyToDependencyManager(dm *DependencyManager)
 }
 
-type WithRepositories map[string]CleanableDownloader
+type WithRepositories map[string]repository.Downloader
 
 func (o WithRepositories) applyToDependencyManager(dm *DependencyManager) {
 	dm.downloaders = o
@@ -103,12 +96,12 @@ func NewDependencyManager(opts ...DependencyManagerOption) *DependencyManager {
 // Clear iterates over the downloaders, calling Clean on all
 // items.
 // It returns a collection of errors.
-func (dm *DependencyManager) Clear() []error {
+func (dm *DependencyManager) Clear() error {
 	var errs []error
 	for _, v := range dm.downloaders {
 		errs = append(errs, v.Clean()...)
 	}
-	return errs
+	return errors.NewAggregate(errs)
 }
 
 // Build compiles a set of missing dependencies from chart.Chart, and attempts to
@@ -241,7 +234,7 @@ func (dm *DependencyManager) addRemoteDependency(chart *chartWithLock, dep *helm
 
 	ver, err := repo.GetChartVersion(dep.Name, dep.Version)
 	if err != nil {
-		return fmt.Errorf("failed to get chart version '%s' from '%s': %w", dep.Version, dep.Repository, err)
+		return fmt.Errorf("failed to get chart '%s' version '%s' from '%s': %w", dep.Name, dep.Version, dep.Repository, err)
 	}
 	res, err := repo.DownloadChart(ver)
 	if err != nil {
@@ -260,7 +253,7 @@ func (dm *DependencyManager) addRemoteDependency(chart *chartWithLock, dep *helm
 
 // 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) {
+func (dm *DependencyManager) resolveRepository(url string) (repo repository.Downloader, err error) {
 	dm.mu.Lock()
 	defer dm.mu.Unlock()
 
@@ -272,7 +265,7 @@ func (dm *DependencyManager) resolveRepository(url string) (_ Downloader, err er
 		}
 
 		if dm.downloaders == nil {
-			dm.downloaders = map[string]CleanableDownloader{}
+			dm.downloaders = map[string]repository.Downloader{}
 		}
 
 		if dm.downloaders[nUrl], err = dm.getChartDownloaderCallback(nUrl); err != nil {