Empty docker creds file errors out on Helm 4

Signed-off-by: Matheus Pimenta <matheuscscp@gmail.com>

Author: matheuscscp

internal/controller/helmchart_controller.go

@@ -70,6 +70,7 @@ import (
 	serror "github.com/fluxcd/source-controller/internal/error"
 	"github.com/fluxcd/source-controller/internal/helm/chart"
 	"github.com/fluxcd/source-controller/internal/helm/getter"
+	"github.com/fluxcd/source-controller/internal/helm/registry"
 	"github.com/fluxcd/source-controller/internal/helm/repository"
 	soci "github.com/fluxcd/source-controller/internal/oci"
 	scosign "github.com/fluxcd/source-controller/internal/oci/cosign"
@@ -132,10 +133,9 @@ type HelmChartReconciler struct {
 	kuberecorder.EventRecorder
 	helper.Metrics
 
-	RegistryClientGenerator RegistryClientGeneratorFunc
-	Storage                 *storage.Storage
-	Getters                 helmgetter.Providers
-	ControllerName          string
+	Storage        *storage.Storage
+	Getters        helmgetter.Providers
+	ControllerName string
 
 	Cache *cache.Cache
 	TTL   time.Duration
@@ -148,7 +148,7 @@ type HelmChartReconciler struct {
 // and an optional file name.
 // The file is used to store the registry client credentials.
 // The caller is responsible for deleting the file.
-type RegistryClientGeneratorFunc func(tlsConfig *tls.Config, isLogin, insecure bool) (*helmreg.Client, string, error)
+type RegistryClientGeneratorFunc func(tlsConfig *tls.Config, isLogin, insecure bool) (*helmreg.Client, error)
 
 func (r *HelmChartReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager) error {
 	return r.SetupWithManagerAndOptions(ctx, mgr, HelmChartReconcilerOptions{})
@@ -557,25 +557,16 @@ 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(clientOpts.TlsConfig, clientOpts.MustLoginToRegistry(), repo.Spec.Insecure)
+		registryClient, err := registry.NewClient(clientOpts.TlsConfig, repo.Spec.Insecure)
 		if err != nil {
 			e := serror.NewGeneric(
-				fmt.Errorf("failed to construct Helm client: %w", err),
+				fmt.Errorf("failed to construct Helm clientzzzzz: %w", err),
 				meta.FailedReason,
 			)
 			conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e)
 			return sreconcile.ResultEmpty, e
 		}
 
-		if credentialsFile != "" {
-			defer func() {
-				if err := os.Remove(credentialsFile); err != nil {
-					r.eventLogf(ctx, obj, corev1.EventTypeWarning, meta.FailedReason,
-						"failed to delete temporary credentials file: %s", err)
-				}
-			}()
-		}
-
 		var verifiers []soci.Verifier
 		if obj.Spec.Verify != nil {
 			provider := obj.Spec.Verify.Provider
@@ -1027,7 +1018,7 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont
 
 		var chartRepo repository.Downloader
 		if helmreg.IsOCI(normalizedURL) {
-			registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.TlsConfig, clientOpts.MustLoginToRegistry(), obj.Spec.Insecure)
+			registryClient, err := registry.NewClient(clientOpts.TlsConfig, obj.Spec.Insecure)
 			if err != nil {
 				return nil, fmt.Errorf("failed to create registry client: %w", err)
 			}
@@ -1038,17 +1029,9 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont
 			ociChartRepo, err := repository.NewOCIChartRepository(normalizedURL, repository.WithOCIGetter(r.Getters),
 				repository.WithOCIGetterOptions(getterOpts),
 				repository.WithOCIRegistryClient(registryClient),
-				repository.WithCertificatesStore(certsTmpDir),
-				repository.WithCredentialsFile(credentialsFile))
+				repository.WithCertificatesStore(certsTmpDir))
 			if err != nil {
-				errs = append(errs, fmt.Errorf("failed to create OCI chart repository: %w", err))
-				// clean up the credentialsFile
-				if credentialsFile != "" {
-					if err := os.Remove(credentialsFile); err != nil {
-						errs = append(errs, err)
-					}
-				}
-				return nil, kerrors.NewAggregate(errs)
+				return nil, fmt.Errorf("failed to create OCI chart repository: %w", err)
 			}
 
 			// If login options are configured, use them to login to the registry
@@ -1310,7 +1293,7 @@ func chartRepoConfigErrorReturn(err error, obj *sourcev1.HelmChart) (sreconcile.
 		return sreconcile.ResultEmpty, e
 	default:
 		e := serror.NewStalling(
-			fmt.Errorf("failed to construct Helm client: %w", err),
+			fmt.Errorf("failed to construct Helm clienttttt: %w", err),
 			meta.FailedReason,
 		)
 		conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e)

internal/controller/helmchart_controller_test.go

@@ -75,7 +75,6 @@ import (
 	serror "github.com/fluxcd/source-controller/internal/error"
 	"github.com/fluxcd/source-controller/internal/helm/chart"
 	"github.com/fluxcd/source-controller/internal/helm/chart/secureloader"
-	"github.com/fluxcd/source-controller/internal/helm/registry"
 	"github.com/fluxcd/source-controller/internal/oci"
 	snotation "github.com/fluxcd/source-controller/internal/oci/notation"
 	sreconcile "github.com/fluxcd/source-controller/internal/reconcile"
@@ -1371,12 +1370,11 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) {
 			}
 
 			r := &HelmChartReconciler{
-				Client:                  clientBuilder.Build(),
-				EventRecorder:           record.NewFakeRecorder(32),
-				Getters:                 testGetters,
-				Storage:                 st,
-				RegistryClientGenerator: registry.ClientGenerator,
-				patchOptions:            getPatchOptions(helmChartReadyCondition.Owned, "sc"),
+				Client:        clientBuilder.Build(),
+				EventRecorder: record.NewFakeRecorder(32),
+				Getters:       testGetters,
+				Storage:       st,
+				patchOptions:  getPatchOptions(helmChartReadyCondition.Owned, "sc"),
 			}
 
 			repository := &sourcev1.HelmRepository{
@@ -1615,11 +1613,10 @@ func TestHelmChartReconciler_buildFromTarballArtifact(t *testing.T) {
 					WithScheme(testEnv.Scheme()).
 					WithStatusSubresource(&sourcev1.HelmChart{}).
 					Build(),
-				EventRecorder:           record.NewFakeRecorder(32),
-				Storage:                 st,
-				Getters:                 testGetters,
-				RegistryClientGenerator: registry.ClientGenerator,
-				patchOptions:            getPatchOptions(helmChartReadyCondition.Owned, "sc"),
+				EventRecorder: record.NewFakeRecorder(32),
+				Storage:       st,
+				Getters:       testGetters,
+				patchOptions:  getPatchOptions(helmChartReadyCondition.Owned, "sc"),
 			}
 
 			obj := &sourcev1.HelmChart{
@@ -2687,11 +2684,10 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
 			}
 
 			r := &HelmChartReconciler{
-				Client:                  clientBuilder.Build(),
-				EventRecorder:           record.NewFakeRecorder(32),
-				Getters:                 testGetters,
-				RegistryClientGenerator: registry.ClientGenerator,
-				patchOptions:            getPatchOptions(helmChartReadyCondition.Owned, "sc"),
+				Client:        clientBuilder.Build(),
+				EventRecorder: record.NewFakeRecorder(32),
+				Getters:       testGetters,
+				patchOptions:  getPatchOptions(helmChartReadyCondition.Owned, "sc"),
 			}
 
 			var b chart.Build
@@ -2844,12 +2840,11 @@ func TestHelmChartRepository_reconcileSource_verifyOCISourceSignature_keyless(t
 			clientBuilder.WithObjects(repository)
 
 			r := &HelmChartReconciler{
-				Client:                  clientBuilder.Build(),
-				EventRecorder:           record.NewFakeRecorder(32),
-				Getters:                 testGetters,
-				Storage:                 testStorage,
-				RegistryClientGenerator: registry.ClientGenerator,
-				patchOptions:            getPatchOptions(helmChartReadyCondition.Owned, "sc"),
+				Client:        clientBuilder.Build(),
+				EventRecorder: record.NewFakeRecorder(32),
+				Getters:       testGetters,
+				Storage:       testStorage,
+				patchOptions:  getPatchOptions(helmChartReadyCondition.Owned, "sc"),
 			}
 
 			obj := &sourcev1.HelmChart{
@@ -3150,12 +3145,11 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignatureNotation(t *t
 			clientBuilder.WithObjects(repository, secret, caSecret)
 
 			r := &HelmChartReconciler{
-				Client:                  clientBuilder.Build(),
-				EventRecorder:           record.NewFakeRecorder(32),
-				Getters:                 testGetters,
-				Storage:                 st,
-				RegistryClientGenerator: registry.ClientGenerator,
-				patchOptions:            getPatchOptions(helmChartReadyCondition.Owned, "sc"),
+				Client:        clientBuilder.Build(),
+				EventRecorder: record.NewFakeRecorder(32),
+				Getters:       testGetters,
+				Storage:       st,
+				patchOptions:  getPatchOptions(helmChartReadyCondition.Owned, "sc"),
 			}
 
 			obj := &sourcev1.HelmChart{
@@ -3402,12 +3396,11 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignatureCosign(t *tes
 			clientBuilder.WithObjects(repository, secret)
 
 			r := &HelmChartReconciler{
-				Client:                  clientBuilder.Build(),
-				EventRecorder:           record.NewFakeRecorder(32),
-				Getters:                 testGetters,
-				Storage:                 st,
-				RegistryClientGenerator: registry.ClientGenerator,
-				patchOptions:            getPatchOptions(helmChartReadyCondition.Owned, "sc"),
+				Client:        clientBuilder.Build(),
+				EventRecorder: record.NewFakeRecorder(32),
+				Getters:       testGetters,
+				Storage:       st,
+				patchOptions:  getPatchOptions(helmChartReadyCondition.Owned, "sc"),
 			}
 
 			obj := &sourcev1.HelmChart{

internal/helm/chart/dependency_manager_test.go

@@ -76,9 +76,7 @@ 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()))
+	ociRepoWithCreds, err := repository.NewOCIChartRepository("oci://example.com")
 	g.Expect(err).ToNot(HaveOccurred())
 
 	downloaders := map[string]repository.Downloader{
@@ -99,14 +97,9 @@ func TestDependencyManager_Clear(t *testing.T) {
 		case *repository.ChartRepository:
 			g.Expect(v.Index).To(BeNil())
 		case *repository.OCIChartRepository:
-			g.Expect(v.HasCredentials()).To(BeFalse())
+			// nothing to check
 		}
 	}
-
-	if _, err := os.Stat(file.Name()); !errors.Is(err, os.ErrNotExist) {
-		err = os.Remove(file.Name())
-		g.Expect(err).ToNot(HaveOccurred())
-	}
 }
 
 func TestDependencyManager_Build(t *testing.T) {

internal/helm/registry/client.go

@@ -20,48 +20,11 @@ import (
 	"crypto/tls"
 	"io"
 	"net/http"
-	"os"
 
 	"helm.sh/helm/v4/pkg/registry"
-	"k8s.io/apimachinery/pkg/util/errors"
 )
 
-// ClientGenerator generates a registry client and a temporary credential file.
-// The client is meant to be used for a single reconciliation.
-// The file is meant to be used for a single reconciliation and deleted after.
-func ClientGenerator(tlsConfig *tls.Config, isLogin, insecureHTTP 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")
-		if err != nil {
-			return nil, "", err
-		}
-
-		var errs []error
-		rClient, err := newClient(credentialsFile.Name(), tlsConfig, insecureHTTP)
-		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 rClient, credentialsFile.Name(), nil
-	}
-
-	rClient, err := newClient("", tlsConfig, insecureHTTP)
-	if err != nil {
-		return nil, "", err
-	}
-	return rClient, "", nil
-}
-
-func newClient(credentialsFile string, tlsConfig *tls.Config, insecureHTTP bool) (*registry.Client, error) {
+func NewClient(tlsConfig *tls.Config, insecureHTTP bool) (*registry.Client, error) {
 	opts := []registry.ClientOption{
 		registry.ClientOptWriter(io.Discard),
 	}
@@ -75,9 +38,5 @@ func newClient(credentialsFile string, tlsConfig *tls.Config, insecureHTTP bool)
 			Transport: t,
 		}))
 	}
-	if credentialsFile != "" {
-		opts = append(opts, registry.ClientOptCredentialsFile(credentialsFile))
-	}
-
 	return registry.NewClient(opts...)
 }

internal/helm/repository/oci_chart_repository.go

@@ -20,7 +20,6 @@ import (
 	"bytes"
 	"context"
 	"crypto/tls"
-	"errors"
 	"fmt"
 	"net/url"
 	"os"
@@ -67,9 +66,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
-
 	// certificatesStore is a temporary store to use while downloading tags or charts from a registry.
 	certificatesStore string
 
@@ -127,14 +123,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
-	}
-}
-
 // WithCertificatesStore returns a ChartRepositoryOption that will set the certificates store
 func WithCertificatesStore(store string) OCIChartRepositoryOption {
 	return func(r *OCIChartRepository) error {
@@ -281,31 +269,13 @@ 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.
+// Clear deletes the OCI registry certificates store.
 func (r *OCIChartRepository) Clear() error {
-	var errs error
-	// clean the credentials file if it exists
-	if r.credentialsFile != "" {
-		if err := os.Remove(r.credentialsFile); err != nil {
-			errs = errors.Join(errs, err)
-		}
+	if s := r.certificatesStore; s != "" {
+		r.certificatesStore = ""
+		return os.RemoveAll(s)
 	}
-	r.credentialsFile = ""
-
-	// clean the certificates store if it exists
-	if r.certificatesStore != "" {
-		if err := os.RemoveAll(r.certificatesStore); err != nil {
-			errs = errors.Join(errs, err)
-		}
-	}
-	r.certificatesStore = ""
-
-	return errs
+	return nil
 }
 
 // getLastMatchingVersionOrConstraint returns the last version that matches the given version string.

main.go

@@ -63,7 +63,6 @@ import (
 	"github.com/fluxcd/source-controller/internal/controller"
 	"github.com/fluxcd/source-controller/internal/features"
 	"github.com/fluxcd/source-controller/internal/helm"
-	"github.com/fluxcd/source-controller/internal/helm/registry"
 )
 
 const controllerName = "source-controller"
@@ -259,16 +258,15 @@ func main() {
 	}
 
 	if err := (&controller.HelmChartReconciler{
-		Client:                  mgr.GetClient(),
-		RegistryClientGenerator: registry.ClientGenerator,
-		Storage:                 storage,
-		Getters:                 getters,
-		EventRecorder:           eventRecorder,
-		Metrics:                 metrics,
-		ControllerName:          controllerName,
-		Cache:                   helmIndexCache,
-		TTL:                     helmIndexCacheItemTTL,
-		CacheRecorder:           cacheRecorder,
+		Client:         mgr.GetClient(),
+		Storage:        storage,
+		Getters:        getters,
+		EventRecorder:  eventRecorder,
+		Metrics:        metrics,
+		ControllerName: controllerName,
+		Cache:          helmIndexCache,
+		TTL:            helmIndexCacheItemTTL,
+		CacheRecorder:  cacheRecorder,
 	}).SetupWithManagerAndOptions(ctx, mgr, controller.HelmChartReconcilerOptions{
 		RateLimiter: helper.GetRateLimiter(rateLimiterOptions),
 	}); err != nil {