log when the OCI temp credentials file can't be deleted

Signed-off-by: Max Jonas Werner <mail@makk.es>
fluxcd · May 23, 2022 · 2db9585 · 2db9585
1 parent 9dd8b2f
commit 2db9585
Show file tree

Hide file tree

Showing 2 changed files with 64 additions and 22 deletions.
diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go
@@ -54,6 +54,7 @@ import (
 	"github.com/fluxcd/pkg/runtime/patch"
 	"github.com/fluxcd/pkg/runtime/predicates"
 	"github.com/fluxcd/pkg/untar"
+	"github.com/go-logr/logr"
 
 	sourcev1 "github.com/fluxcd/source-controller/api/v1beta2"
 	"github.com/fluxcd/source-controller/internal/cache"
@@ -140,7 +141,15 @@ type HelmChartReconcilerOptions struct {
 // helmChartReconcileFunc is the function type for all the v1beta2.HelmChart
 // (sub)reconcile functions. The type implementations are grouped and
 // executed serially to perform the complete reconcile of the object.
-type helmChartReconcileFunc func(ctx context.Context, obj *sourcev1.HelmChart, build *chart.Build) (sreconcile.Result, error)
+type helmChartReconcileFunc func(
+	ctx context.Context,
+	obj *sourcev1.HelmChart,
+	log logr.Logger,
+	build *chart.Build,
+) (
+	sreconcile.Result,
+	error,
+)
 
 func (r *HelmChartReconciler) SetupWithManagerAndOptions(mgr ctrl.Manager, opts HelmChartReconcilerOptions) error {
 	if err := mgr.GetCache().IndexField(context.TODO(), &sourcev1.HelmRepository{}, sourcev1.HelmRepositoryURLIndexKey,
@@ -249,14 +258,15 @@ func (r *HelmChartReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 		r.reconcileSource,
 		r.reconcileArtifact,
 	}
-	recResult, retErr = r.reconcile(ctx, obj, reconcilers)
+	recResult, retErr = r.reconcile(ctx, obj, log, reconcilers)
 	return
 }
 
 // reconcile iterates through the helmChartReconcileFunc tasks for the
 // object. It returns early on the first call that returns
 // reconcile.ResultRequeue, or produces an error.
-func (r *HelmChartReconciler) reconcile(ctx context.Context, obj *sourcev1.HelmChart, reconcilers []helmChartReconcileFunc) (sreconcile.Result, error) {
+func (r *HelmChartReconciler) reconcile(ctx context.Context, obj *sourcev1.HelmChart, log logr.Logger,
+	reconcilers []helmChartReconcileFunc) (sreconcile.Result, error) {
 	oldObj := obj.DeepCopy()
 
 	// Mark as reconciling if generation differs.
@@ -271,7 +281,7 @@ func (r *HelmChartReconciler) reconcile(ctx context.Context, obj *sourcev1.HelmC
 		resErr error
 	)
 	for _, rec := range reconcilers {
-		recResult, err := rec(ctx, obj, &build)
+		recResult, err := rec(ctx, obj, log, &build)
 		// Exit immediately on ResultRequeue.
 		if recResult == sreconcile.ResultRequeue {
 			return sreconcile.ResultRequeue, nil
@@ -331,7 +341,8 @@ func (r *HelmChartReconciler) notify(oldObj, newObj *sourcev1.HelmChart, build *
 // condition is added.
 // The hostname of any URL in the Status of the object are updated, to ensure
 // they match the Storage server hostname of current runtime.
-func (r *HelmChartReconciler) reconcileStorage(ctx context.Context, obj *sourcev1.HelmChart, build *chart.Build) (sreconcile.Result, error) {
+func (r *HelmChartReconciler) reconcileStorage(ctx context.Context, obj *sourcev1.HelmChart, log logr.Logger,
+	build *chart.Build) (sreconcile.Result, error) {
 	// Garbage collect previous advertised artifact(s) from storage
 	_ = r.garbageCollect(ctx, obj)
 
@@ -358,7 +369,8 @@ func (r *HelmChartReconciler) reconcileStorage(ctx context.Context, obj *sourcev
 	return sreconcile.ResultSuccess, nil
 }
 
-func (r *HelmChartReconciler) reconcileSource(ctx context.Context, obj *sourcev1.HelmChart, build *chart.Build) (_ sreconcile.Result, retErr error) {
+func (r *HelmChartReconciler) reconcileSource(ctx context.Context, obj *sourcev1.HelmChart,
+	log logr.Logger, build *chart.Build) (_ sreconcile.Result, retErr error) {
 	// Retrieve the source
 	s, err := r.getSource(ctx, obj)
 	if err != nil {
@@ -429,7 +441,7 @@ func (r *HelmChartReconciler) reconcileSource(ctx context.Context, obj *sourcev1
 	// Perform the build for the chart source type
 	switch typedSource := s.(type) {
 	case *sourcev1.HelmRepository:
-		return r.buildFromHelmRepository(ctx, obj, typedSource, build)
+		return r.buildFromHelmRepository(ctx, obj, log, typedSource, build)
 	case *sourcev1.GitRepository, *sourcev1.Bucket:
 		return r.buildFromTarballArtifact(ctx, obj, *typedSource.GetArtifact(), build)
 	default:
@@ -445,7 +457,7 @@ func (r *HelmChartReconciler) reconcileSource(ctx context.Context, obj *sourcev1
 // In case of a failure it records v1beta2.FetchFailedCondition on the chart
 // object, and returns early.
 func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *sourcev1.HelmChart,
-	repo *sourcev1.HelmRepository, b *chart.Build) (sreconcile.Result, error) {
+	log logr.Logger, repo *sourcev1.HelmRepository, b *chart.Build) (sreconcile.Result, error) {
 	var (
 		tlsConfig *tls.Config
 		loginOpts []helmreg.LoginOption
@@ -527,7 +539,16 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 
 		if file != "" {
 			defer func() {
-				os.Remove(file)
+				if err := os.Remove(file); err != nil {
+					log.Error(err, "failed to delete temporary credentials file")
+					r.Eventf(
+						obj,
+						corev1.EventTypeWarning,
+						meta.FailedReason,
+						"failed to delete temporary credentials file: %s",
+						err,
+					)
+				}
 			}()
 		}
 
@@ -730,7 +751,8 @@ func (r *HelmChartReconciler) buildFromTarballArtifact(ctx context.Context, obj
 // early.
 // On a successful archive, the Artifact in the Status of the object is set,
 // and the symlink in the Storage is updated to its path.
-func (r *HelmChartReconciler) reconcileArtifact(ctx context.Context, obj *sourcev1.HelmChart, b *chart.Build) (sreconcile.Result, error) {
+func (r *HelmChartReconciler) reconcileArtifact(ctx context.Context, obj *sourcev1.HelmChart, log logr.Logger,
+	b *chart.Build) (sreconcile.Result, error) {
 	// Without a complete chart build, there is little to reconcile
 	if !b.Complete() {
 		return sreconcile.ResultRequeue, nil

diff --git a/controllers/helmrepository_controller_oci.go b/controllers/helmrepository_controller_oci.go
@@ -35,6 +35,7 @@ import (
 	intpredicates "github.com/fluxcd/source-controller/internal/predicates"
 	sreconcile "github.com/fluxcd/source-controller/internal/reconcile"
 	"github.com/fluxcd/source-controller/internal/reconcile/summarize"
+	"github.com/go-logr/logr"
 	helmgetter "helm.sh/helm/v3/pkg/getter"
 	helmreg "helm.sh/helm/v3/pkg/registry"
 	corev1 "k8s.io/api/core/v1"
@@ -98,7 +99,11 @@ type RegistryClientGeneratorFunc func(isLogin bool) (*helmreg.Client, string, er
 // v1beta2.HelmRepository (sub)reconcile functions for OCI type. The type implementations
 // are grouped and executed serially to perform the complete reconcile of the
 // object.
-type helmRepositoryOCIReconcileFunc func(ctx context.Context, obj *sourcev1.HelmRepository) (sreconcile.Result, error)
+type helmRepositoryOCIReconcileFunc func(
+	ctx context.Context,
+	obj *sourcev1.HelmRepository,
+	log logr.Logger,
+) (sreconcile.Result, error)
 
 func (r *HelmRepositoryOCIReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	return r.SetupWithManagerAndOptions(mgr, HelmRepositoryReconcilerOptions{})
@@ -171,7 +176,7 @@ func (r *HelmRepositoryOCIReconciler) Reconcile(ctx context.Context, req ctrl.Re
 		r.Metrics.RecordDuration(ctx, obj, start)
 	}()
 
-	// Add finalizer first if not exist to avoid the race condition
+	// Add finalizer first if it doesn't exist to avoid the race condition
 	// between init and delete
 	if !controllerutil.ContainsFinalizer(obj, sourcev1.SourceFinalizer) {
 		controllerutil.AddFinalizer(obj, sourcev1.SourceFinalizer)
@@ -196,13 +201,14 @@ func (r *HelmRepositoryOCIReconciler) Reconcile(ctx context.Context, req ctrl.Re
 	reconcilers := []helmRepositoryOCIReconcileFunc{
 		r.reconcileSource,
 	}
-	recResult, retErr = r.reconcile(ctx, obj, reconcilers)
+	recResult, retErr = r.reconcile(ctx, obj, log, reconcilers)
 	return
 }
 
 // reconcileDelete handles the deletion of the object.
 // Removing the finalizer from the object if successful.
-func (r *HelmRepositoryOCIReconciler) reconcileDelete(ctx context.Context, obj *sourcev1.HelmRepository) (sreconcile.Result, error) {
+func (r *HelmRepositoryOCIReconciler) reconcileDelete(ctx context.Context, obj *sourcev1.HelmRepository,
+) (sreconcile.Result, error) {
 	// Remove our finalizer from the list
 	controllerutil.RemoveFinalizer(obj, sourcev1.SourceFinalizer)
 
@@ -211,7 +217,8 @@ func (r *HelmRepositoryOCIReconciler) reconcileDelete(ctx context.Context, obj *
 }
 
 // notify emits notification related to the reconciliation.
-func (r *HelmRepositoryOCIReconciler) notify(oldObj, newObj *sourcev1.HelmRepository, res sreconcile.Result, resErr error) {
+func (r *HelmRepositoryOCIReconciler) notify(oldObj, newObj *sourcev1.HelmRepository, res sreconcile.Result,
+	resErr error) {
 	// Notify successful recovery from any failure.
 	if resErr == nil && res == sreconcile.ResultSuccess {
 		if sreconcile.FailureRecovery(oldObj, newObj, helmRepositoryOCIFailConditions) {
@@ -221,7 +228,9 @@ func (r *HelmRepositoryOCIReconciler) notify(oldObj, newObj *sourcev1.HelmReposi
 	}
 }
 
-func (r *HelmRepositoryOCIReconciler) reconcile(ctx context.Context, obj *sourcev1.HelmRepository, reconcilers []helmRepositoryOCIReconcileFunc) (sreconcile.Result, error) {
+func (r *HelmRepositoryOCIReconciler) reconcile(ctx context.Context, obj *sourcev1.HelmRepository, log logr.Logger,
+	reconcilers []helmRepositoryOCIReconcileFunc) (sreconcile.Result, error) {
+
 	oldObj := obj.DeepCopy()
 
 	// Mark as reconciling if generation differs.
@@ -233,7 +242,7 @@ func (r *HelmRepositoryOCIReconciler) reconcile(ctx context.Context, obj *source
 	var res sreconcile.Result
 	var resErr error
 	for _, rec := range reconcilers {
-		recResult, err := rec(ctx, obj)
+		recResult, err := rec(ctx, obj, log)
 		// Exit immediately on ResultRequeue.
 		if recResult == sreconcile.ResultRequeue {
 			return sreconcile.ResultRequeue, nil
@@ -254,7 +263,8 @@ func (r *HelmRepositoryOCIReconciler) reconcile(ctx context.Context, obj *source
 	return res, resErr
 }
 
-func (r *HelmRepositoryOCIReconciler) reconcileSource(ctx context.Context, obj *sourcev1.HelmRepository) (sreconcile.Result, error) {
+func (r *HelmRepositoryOCIReconciler) reconcileSource(ctx context.Context, obj *sourcev1.HelmRepository,
+	log logr.Logger) (sreconcile.Result, error) {
 	var loginOpts []helmreg.LoginOption
 	// Configure any authentication related options
 	if obj.Spec.SecretRef != nil {
@@ -290,7 +300,7 @@ func (r *HelmRepositoryOCIReconciler) reconcileSource(ctx context.Context, obj *
 		}
 	}
 
-	if result, err := r.validateSource(ctx, obj, loginOpts...); err != nil || result == sreconcile.ResultEmpty {
+	if result, err := r.validateSource(ctx, obj, log, loginOpts...); err != nil || result == sreconcile.ResultEmpty {
 		return result, err
 	}
 
@@ -299,11 +309,12 @@ func (r *HelmRepositoryOCIReconciler) reconcileSource(ctx context.Context, obj *
 
 // validateSource the HelmRepository object by checking the url and connecting to the underlying registry
 // with he provided credentials.
-func (r *HelmRepositoryOCIReconciler) validateSource(ctx context.Context, obj *sourcev1.HelmRepository, logOpts ...helmreg.LoginOption) (sreconcile.Result, error) {
+func (r *HelmRepositoryOCIReconciler) validateSource(ctx context.Context, obj *sourcev1.HelmRepository, log logr.Logger,
+	logOpts ...helmreg.LoginOption) (sreconcile.Result, error) {
 	registryClient, file, err := r.RegistryClientGenerator(logOpts != nil)
 	if err != nil {
 		e := &serror.Stalling{
-			Err:    fmt.Errorf("failed to create registry client:: %w", err),
+			Err:    fmt.Errorf("failed to create registry client: %w", err),
 			Reason: meta.FailedReason,
 		}
 		conditions.MarkFalse(obj, meta.ReadyCondition, e.Reason, e.Err.Error())
@@ -312,7 +323,16 @@ func (r *HelmRepositoryOCIReconciler) validateSource(ctx context.Context, obj *s
 
 	if file != "" {
 		defer func() {
-			os.Remove(file)
+			if err := os.Remove(file); err != nil {
+				log.Error(err, "failed to delete temporary credentials file")
+				r.Eventf(
+					obj,
+					corev1.EventTypeWarning,
+					meta.FailedReason,
+					"failed to delete temporary credentials file: %s",
+					err,
+				)
+			}
 		}()
 	}