From f14a053f0ac98af7f0d331a38fe935f44d55ea19 Mon Sep 17 00:00:00 2001 From: Sunny Date: Thu, 16 Dec 2021 14:44:34 +0530 Subject: [PATCH] helmrepo: Add more reconciler design improvements - Remove ArtifactUnavailable condition and use Reconciling condition to convey the same. - Make Reconciling condition affect the ready condition. - Introduce summarizeAndPatch() to calculate the final status conditions and patch them. - Introduce reconcile() to iterate through the sub-reconcilers and execute them. - Simplify logging and event recording - Introduce controller-check/status checks to assert that the status conditions are valid at the end of the tests. - Create variables for various condition groups: owned conditions, ready dependencies and ready dependencies negative. Signed-off-by: Sunny --- controllers/helmrepository_controller.go | 401 ++++++++++-------- controllers/helmrepository_controller_test.go | 377 ++++++++++++++-- 2 files changed, 569 insertions(+), 209 deletions(-) diff --git a/controllers/helmrepository_controller.go b/controllers/helmrepository_controller.go index d3944ff7..b870797f 100644 --- a/controllers/helmrepository_controller.go +++ b/controllers/helmrepository_controller.go @@ -18,6 +18,7 @@ package controllers import ( "context" + "errors" "fmt" "net/url" "os" @@ -26,7 +27,7 @@ import ( helmgetter "helm.sh/helm/v3/pkg/getter" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" - 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" kuberecorder "k8s.io/client-go/tools/record" @@ -43,10 +44,38 @@ import ( "github.com/fluxcd/pkg/runtime/predicates" sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" + serror "github.com/fluxcd/source-controller/internal/error" "github.com/fluxcd/source-controller/internal/helm/getter" "github.com/fluxcd/source-controller/internal/helm/repository" + sreconcile "github.com/fluxcd/source-controller/internal/reconcile" ) +// Status conditions owned by HelmRepository reconciler. +var helmRepoOwnedConditions = []string{ + sourcev1.FetchFailedCondition, + sourcev1.ArtifactOutdatedCondition, + meta.ReadyCondition, + meta.ReconcilingCondition, + meta.StalledCondition, +} + +// Conditions that Ready condition is influenced by in descending order of their +// priority. +var helmRepoReadyDeps = []string{ + sourcev1.FetchFailedCondition, + sourcev1.ArtifactOutdatedCondition, + meta.StalledCondition, + meta.ReconcilingCondition, +} + +// Negative conditions that Ready condition is influenced by. +var helmRepoReadyDepsNegative = []string{ + sourcev1.FetchFailedCondition, + sourcev1.ArtifactOutdatedCondition, + meta.StalledCondition, + meta.ReconcilingCondition, +} + // +kubebuilder:rbac:groups=source.toolkit.fluxcd.io,resources=helmrepositories,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=source.toolkit.fluxcd.io,resources=helmrepositories/status,verbs=get;update;patch // +kubebuilder:rbac:groups=source.toolkit.fluxcd.io,resources=helmrepositories/finalizers,verbs=get;create;update;patch;delete @@ -66,6 +95,11 @@ type HelmRepositoryReconcilerOptions struct { MaxConcurrentReconciles int } +// helmRepoReconcilerFunc is the function type for all the helm repository +// reconciler functions. The reconciler functions are grouped together and +// executed serially to perform the main operation of the reconciler. +type helmRepoReconcilerFunc func(ctx context.Context, obj *sourcev1.HelmRepository, artifact *sourcev1.Artifact, repo *repository.ChartRepository) (sreconcile.Result, error) + func (r *HelmRepositoryReconciler) SetupWithManager(mgr ctrl.Manager) error { return r.SetupWithManagerAndOptions(mgr, HelmRepositoryReconcilerOptions{}) } @@ -103,71 +137,15 @@ func (r *HelmRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reque return ctrl.Result{}, err } - // Always attempt to patch the object and status after each reconciliation + // Result of the sub-reconciliation. + var recResult sreconcile.Result + + // Always attempt to patch the object after each reconciliation. + // NOTE: This deferred block only modifies the named return error. The + // result from the reconciliation remains the same. Any requeue attributes + // set in the result will continue to be effective. defer func() { - // Record the value of the reconciliation request, if any - if v, ok := meta.ReconcileAnnotationValue(obj.GetAnnotations()); ok { - obj.Status.SetLastHandledReconcileRequest(v) - } - - // Summarize Ready condition - conditions.SetSummary(obj, - meta.ReadyCondition, - conditions.WithConditions( - sourcev1.FetchFailedCondition, - sourcev1.ArtifactOutdatedCondition, - sourcev1.ArtifactUnavailableCondition, - ), - conditions.WithNegativePolarityConditions( - sourcev1.FetchFailedCondition, - sourcev1.ArtifactOutdatedCondition, - sourcev1.ArtifactUnavailableCondition, - ), - ) - - // Patch the object, ignoring conflicts on the conditions owned by this controller - patchOpts := []patch.Option{ - patch.WithOwnedConditions{ - Conditions: []string{ - sourcev1.FetchFailedCondition, - sourcev1.ArtifactOutdatedCondition, - sourcev1.ArtifactUnavailableCondition, - meta.ReadyCondition, - meta.ReconcilingCondition, - meta.StalledCondition, - }, - }, - } - - // Determine if the resource is still being reconciled, or if it has stalled, and record this observation - if retErr == nil && (result.IsZero() || !result.Requeue) { - // We are no longer reconciling - conditions.Delete(obj, meta.ReconcilingCondition) - - // We have now observed this generation - patchOpts = append(patchOpts, patch.WithStatusObservedGeneration{}) - - readyCondition := conditions.Get(obj, meta.ReadyCondition) - switch readyCondition.Status { - case metav1.ConditionFalse: - // As we are no longer reconciling and the end-state - // is not ready, the reconciliation has stalled - conditions.MarkStalled(obj, readyCondition.Reason, readyCondition.Message) - case metav1.ConditionTrue: - // As we are no longer reconciling and the end-state - // is ready, the reconciliation is no longer stalled - conditions.Delete(obj, meta.StalledCondition) - } - } - - // Finally, patch the resource - if err := patchHelper.Patch(ctx, obj, patchOpts...); err != nil { - // Ignore patch error "not found" when the object is being deleted. - if !obj.ObjectMeta.DeletionTimestamp.IsZero() { - err = kerrors.FilterOut(err, func(e error) bool { return apierrors.IsNotFound(e) }) - } - retErr = kerrors.NewAggregate([]error{retErr, err}) - } + retErr = r.summarizeAndPatch(ctx, obj, patchHelper, recResult, retErr) // Always record readiness and duration metrics r.Metrics.RecordReadiness(ctx, obj) @@ -178,53 +156,104 @@ func (r *HelmRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reque // between init and delete if !controllerutil.ContainsFinalizer(obj, sourcev1.SourceFinalizer) { controllerutil.AddFinalizer(obj, sourcev1.SourceFinalizer) + recResult = sreconcile.ResultRequeue return ctrl.Result{Requeue: true}, nil } // Examine if the object is under deletion if !obj.ObjectMeta.DeletionTimestamp.IsZero() { - return r.reconcileDelete(ctx, obj) + res, err := r.reconcileDelete(ctx, obj) + return sreconcile.BuildRuntimeResult(ctx, r.EventRecorder, obj, res, err) } // Reconcile actual object - return r.reconcile(ctx, obj) + reconcilers := []helmRepoReconcilerFunc{ + r.reconcileStorage, + r.reconcileSource, + r.reconcileArtifact, + } + recResult, err = r.reconcile(ctx, obj, reconcilers) + return sreconcile.BuildRuntimeResult(ctx, r.EventRecorder, obj, recResult, err) } -// reconcile steps through the actual reconciliation tasks for the object, it returns early on the first step that -// produces an error. -func (r *HelmRepositoryReconciler) reconcile(ctx context.Context, obj *sourcev1.HelmRepository) (ctrl.Result, error) { - // Mark the resource as under reconciliation - conditions.MarkReconciling(obj, meta.ProgressingReason, "") +// summarizeAndPatch analyzes the object conditions to create a summary of the +// status conditions and patches the object with the calculated summary. The +// reconciler error type is also used to determine the conditions and the +// returned error. +func (r *HelmRepositoryReconciler) summarizeAndPatch(ctx context.Context, obj *sourcev1.HelmRepository, patchHelper *patch.Helper, res sreconcile.Result, recErr error) error { + // Record the value of the reconciliation request, if any. + if v, ok := meta.ReconcileAnnotationValue(obj.GetAnnotations()); ok { + obj.Status.SetLastHandledReconcileRequest(v) + } - // Reconcile the storage data - if result, err := r.reconcileStorage(ctx, obj); err != nil { - return result, err + // Compute the reconcile results, obtain patch options and reconcile error. + var patchOpts []patch.Option + patchOpts, recErr = sreconcile.ComputeReconcileResult(obj, res, recErr, helmRepoOwnedConditions) + + // Summarize Ready condition. + conditions.SetSummary(obj, + meta.ReadyCondition, + conditions.WithConditions( + helmRepoReadyDeps..., + ), + conditions.WithNegativePolarityConditions( + helmRepoReadyDepsNegative..., + ), + ) + + // Finally, patch the resource. + if err := patchHelper.Patch(ctx, obj, patchOpts...); err != nil { + // Ignore patch error "not found" when the object is being deleted. + if !obj.ObjectMeta.DeletionTimestamp.IsZero() { + err = kerrors.FilterOut(err, func(e error) bool { return apierrors.IsNotFound(e) }) + } + recErr = kerrors.NewAggregate([]error{recErr, err}) + } + + return recErr +} + +// reconcile iterates through the sub-reconcilers and processes the source +// object. The sub-reconcilers are run sequentially. The result and error of +// the sub-reconciliation are collected and returned. For multiple results +// from different sub-reconcilers, the results are combined to return the +// result with the shortest requeue period. +func (r *HelmRepositoryReconciler) reconcile(ctx context.Context, obj *sourcev1.HelmRepository, reconcilers []helmRepoReconcilerFunc) (sreconcile.Result, error) { + if obj.Generation != obj.Status.ObservedGeneration { + conditions.MarkReconciling(obj, "NewGeneration", "reconciling new generation %d", obj.Generation) } var chartRepo repository.ChartRepository var artifact sourcev1.Artifact - // Reconcile the source from upstream - if result, err := r.reconcileSource(ctx, obj, &artifact, &chartRepo); err != nil || result.IsZero() { - return result, err - } - // Reconcile the artifact. - if result, err := r.reconcileArtifact(ctx, obj, artifact, &chartRepo); err != nil || result.IsZero() { - return result, err + // Run the sub-reconcilers and build the result of reconciliation. + var res sreconcile.Result + var resErr error + for _, rec := range reconcilers { + recResult, err := rec(ctx, obj, &artifact, &chartRepo) + // Exit immediately on ResultRequeue. + if recResult == sreconcile.ResultRequeue { + return sreconcile.ResultRequeue, nil + } + // If an error is received, prioritize the returned results because an + // error also means immediate requeue. + if err != nil { + resErr = err + res = recResult + break + } + // Prioritize requeue request in the result for successful results. + res = sreconcile.LowestRequeuingResult(res, recResult) } - - return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil + return res, resErr } // reconcileStorage ensures the current state of the storage matches the desired and previously observed state. // // All artifacts for the resource except for the current one are garbage collected from the storage. // If the artifact in the Status object of the resource disappeared from storage, it is removed from the object. -// If the object does not have an artifact in its Status object, a v1beta1.ArtifactUnavailableCondition is set. // If the hostname of any of the URLs on the object do not match the current storage server hostname, they are updated. -// -// The caller should assume a failure if an error is returned, or the Result is zero. -func (r *HelmRepositoryReconciler) reconcileStorage(ctx context.Context, obj *sourcev1.HelmRepository) (ctrl.Result, error) { +func (r *HelmRepositoryReconciler) reconcileStorage(ctx context.Context, obj *sourcev1.HelmRepository, artifact *sourcev1.Artifact, chartRepo *repository.ChartRepository) (sreconcile.Result, error) { // Garbage collect previous advertised artifact(s) from storage _ = r.garbageCollect(ctx, obj) @@ -236,17 +265,16 @@ func (r *HelmRepositoryReconciler) reconcileStorage(ctx context.Context, obj *so // Record that we do not have an artifact if obj.GetArtifact() == nil { - conditions.MarkTrue(obj, sourcev1.ArtifactUnavailableCondition, "NoArtifact", "No artifact for resource in storage") - return ctrl.Result{Requeue: true}, nil + conditions.MarkReconciling(obj, "NoArtifact", "no artifact for resource in storage") + return sreconcile.ResultSuccess, nil } - conditions.Delete(obj, sourcev1.ArtifactUnavailableCondition) // Always update URLs to ensure hostname is up-to-date // TODO(hidde): we may want to send out an event only if we notice the URL has changed r.Storage.SetArtifactURL(obj.GetArtifact()) obj.Status.URL = r.Storage.SetHostname(obj.Status.URL) - return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil + return sreconcile.ResultSuccess, nil } // reconcileSource ensures the upstream Helm repository can be reached and downloaded out using the declared @@ -254,11 +282,9 @@ func (r *HelmRepositoryReconciler) reconcileStorage(ctx context.Context, obj *so // // The Helm repository index is downloaded using the defined configuration, and in case of an error during this process // (including transient errors), it records v1beta1.FetchFailedCondition=True and returns early. -// On a successful write of a new artifact, the artifact in the status of the given object is set, and the symlink in -// the storage is updated to its path. -// -// The caller should assume a failure if an error is returned, or the Result is zero. -func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, obj *sourcev1.HelmRepository, artifact *sourcev1.Artifact, chartRepo *repository.ChartRepository) (ctrl.Result, error) { +// If the download is successful, the given artifact pointer is set to a new artifact with the available metadata, and +// the index pointer is set to the newly downloaded index. +func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, obj *sourcev1.HelmRepository, artifact *sourcev1.Artifact, chartRepo *repository.ChartRepository) (sreconcile.Result, error) { // Configure Helm client to access repository clientOpts := []helmgetter.Option{ helmgetter.WithTimeout(obj.Spec.Timeout.Duration), @@ -275,34 +301,34 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, obj *sou } var secret corev1.Secret if err := r.Client.Get(ctx, name, &secret); err != nil { - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, - "Failed to get secret '%s': %s", name.String(), err.Error()) - r.Eventf(obj, corev1.EventTypeWarning, sourcev1.AuthenticationFailedReason, - "Failed to get secret '%s': %s", name.String(), err.Error()) - // Return error as the world as observed may change - return ctrl.Result{}, err + e := &serror.Event{ + Err: fmt.Errorf("failed to get secret '%s': %w", name.String(), err), + Reason: sourcev1.AuthenticationFailedReason, + } + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, e.Err.Error()) + return sreconcile.ResultEmpty, e } // Get client options from secret tmpDir, err := os.MkdirTemp("", fmt.Sprintf("%s-%s-auth-", obj.Name, obj.Namespace)) if err != nil { - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.StorageOperationFailedReason, - "Failed to create temporary directory for credentials: %s", err.Error()) - r.Eventf(obj, corev1.EventTypeWarning, sourcev1.StorageOperationFailedReason, - "Failed to create temporary directory for credentials: %s", err.Error()) - return ctrl.Result{}, err + return sreconcile.ResultEmpty, &serror.Event{ + Err: fmt.Errorf("failed to create temporary directory for credentials: %w", err), + Reason: sourcev1.StorageOperationFailedReason, + } } defer os.RemoveAll(tmpDir) // Construct actual options opts, err := getter.ClientOptionsFromSecret(tmpDir, secret) if err != nil { - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, - "Failed to configure Helm client with secret data: %s", err) - r.Eventf(obj, corev1.EventTypeWarning, sourcev1.AuthenticationFailedReason, - "Failed to configure Helm client with secret data: %s", err) - // Return err as the content of the secret may change - return ctrl.Result{}, err + e := &serror.Event{ + Err: fmt.Errorf("failed to configure Helm client with secret data: %w", err), + Reason: sourcev1.AuthenticationFailedReason, + } + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, e.Err.Error()) + // Return err as the content of the secret may change. + return sreconcile.ResultEmpty, e } clientOpts = append(clientOpts, opts...) } @@ -312,42 +338,49 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, obj *sou if err != nil { switch err.(type) { case *url.Error: - ctrl.LoggerFrom(ctx).Error(err, "invalid Helm repository URL") - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.URLInvalidReason, - "Invalid Helm repository URL: %s", err.Error()) - return ctrl.Result{}, nil + e := &serror.Stalling{ + Err: fmt.Errorf("invalid Helm repository URL: %w", err), + Reason: sourcev1.URLInvalidReason, + } + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.URLInvalidReason, e.Err.Error()) + return sreconcile.ResultEmpty, e default: - ctrl.LoggerFrom(ctx).Error(err, "failed to construct Helm client") - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, meta.FailedReason, - "Failed to construct Helm client: %s", err.Error()) - return ctrl.Result{}, nil + e := &serror.Stalling{ + Err: fmt.Errorf("failed to construct Helm client: %w", err), + Reason: meta.FailedReason, + } + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, meta.FailedReason, e.Err.Error()) + return sreconcile.ResultEmpty, e } } checksum, err := newChartRepo.CacheIndex() if err != nil { - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, meta.FailedReason, - "Failed to download Helm repository index: %s", err.Error()) - r.Eventf(obj, corev1.EventTypeWarning, sourcev1.FetchFailedCondition, - "Failed to download Helm repository index: %s", err.Error()) + e := &serror.Event{ + Err: fmt.Errorf("failed to download Helm repository index: %w", err), + Reason: meta.FailedReason, + } + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, meta.FailedReason, e.Err.Error()) // Coin flip on transient or persistent error, return error and hope for the best - return ctrl.Result{}, err + return sreconcile.ResultEmpty, e } *chartRepo = *newChartRepo // Load the cached repository index to ensure it passes validation. if err := chartRepo.LoadFromCache(); err != nil { - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.IndexationFailedReason, - "Failed to load Helm repository from cache: %s", err.Error()) - r.Eventf(obj, corev1.EventTypeWarning, sourcev1.FetchFailedCondition, - "Failed to load Helm repository from cache: %s", err.Error()) - return ctrl.Result{}, err + e := &serror.Event{ + Err: fmt.Errorf("failed to load Helm repository from cache: %w", err), + Reason: sourcev1.FetchFailedCondition, + } + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.IndexationFailedReason, e.Err.Error()) + return sreconcile.ResultEmpty, e } defer chartRepo.Unload() // Mark observations about the revision on the object. if !obj.GetArtifact().HasRevision(checksum) { - conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", - "New index revision '%s'", checksum) + message := fmt.Sprintf("new index revision '%s'", checksum) + conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", message) + conditions.MarkReconciling(obj, "NewRevision", message) } conditions.Delete(obj, sourcev1.FetchFailedCondition) @@ -358,81 +391,95 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, obj *sou chartRepo.Checksum, fmt.Sprintf("index-%s.yaml", checksum)) - return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil + return sreconcile.ResultSuccess, nil } -func (r *HelmRepositoryReconciler) reconcileArtifact(ctx context.Context, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo *repository.ChartRepository) (ctrl.Result, error) { +// reconcileArtifact stores a new artifact in the storage, if the current observation on the object does not match the +// given data. +// +// The inspection of the given data to the object is differed, ensuring any stale observations as +// v1beta1.ArtifactUnavailableCondition and v1beta1.ArtifactOutdatedCondition are always deleted. +// If the given artifact does not differ from the object's current, it returns early. +// On a successful write of a new artifact, the artifact in the status of the given object is set, and the symlink in +// the storage is updated to its path. +func (r *HelmRepositoryReconciler) reconcileArtifact(ctx context.Context, obj *sourcev1.HelmRepository, artifact *sourcev1.Artifact, chartRepo *repository.ChartRepository) (sreconcile.Result, error) { // Always restore the Ready condition in case it got removed due to a transient error. defer func() { - if obj.GetArtifact() != nil { - conditions.Delete(obj, sourcev1.ArtifactUnavailableCondition) - } if obj.GetArtifact().HasRevision(artifact.Revision) { conditions.Delete(obj, sourcev1.ArtifactOutdatedCondition) conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, - "Stored artifact for revision '%s'", artifact.Revision) + "stored artifact for revision '%s'", artifact.Revision) } }() if obj.GetArtifact().HasRevision(artifact.Revision) { - ctrl.LoggerFrom(ctx).Info(fmt.Sprintf("Already up to date, current revision '%s'", artifact.Revision)) - return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil + r.eventLogf(ctx, obj, corev1.EventTypeNormal, meta.SucceededReason, "already up to date, current revision '%s'", artifact.Revision) + return sreconcile.ResultSuccess, nil } + // Mark reconciling because the artifact and remote source are different. + // and they have to be reconciled. + conditions.MarkReconciling(obj, "NewRevision", "new index revision '%s'", artifact.Revision) + // Clear cache at the very end. defer chartRepo.RemoveCache() // Create artifact dir. - if err := r.Storage.MkdirAll(artifact); err != nil { - ctrl.LoggerFrom(ctx).Error(err, "failed to create artifact directory") - return ctrl.Result{}, err + if err := r.Storage.MkdirAll(*artifact); err != nil { + return sreconcile.ResultEmpty, &serror.Event{ + Err: fmt.Errorf("failed to create artifact directory: %w", err), + Reason: sourcev1.StorageOperationFailedReason, + } } // Acquire lock. - unlock, err := r.Storage.Lock(artifact) + unlock, err := r.Storage.Lock(*artifact) if err != nil { - ctrl.LoggerFrom(ctx).Error(err, "failed to acquire lock for artifact") - return ctrl.Result{}, err + return sreconcile.ResultEmpty, &serror.Event{ + Err: fmt.Errorf("failed to acquire lock for artifact: %w", err), + Reason: meta.FailedReason, + } } defer unlock() // Save artifact to storage. - if err = r.Storage.CopyFromPath(&artifact, chartRepo.CachePath); err != nil { - r.Eventf(obj, corev1.EventTypeWarning, sourcev1.StorageOperationFailedReason, - "Unable to save artifact to storage: %s", err) - return ctrl.Result{}, err + if err = r.Storage.CopyFromPath(artifact, chartRepo.CachePath); err != nil { + return sreconcile.ResultEmpty, &serror.Event{ + Err: fmt.Errorf("unable to save artifact to storage: %w", err), + Reason: sourcev1.StorageOperationFailedReason, + } } // Record it on the object. obj.Status.Artifact = artifact.DeepCopy() // Update index symlink. - indexURL, err := r.Storage.Symlink(artifact, "index.yaml") + indexURL, err := r.Storage.Symlink(*artifact, "index.yaml") if err != nil { - r.Eventf(obj, corev1.EventTypeWarning, sourcev1.StorageOperationFailedReason, - "Failed to update status URL symlink: %s", err) + r.eventLogf(ctx, obj, corev1.EventTypeWarning, sourcev1.StorageOperationFailedReason, + "failed to update status URL symlink: %s", err) } if indexURL != "" { obj.Status.URL = indexURL } - return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil + return sreconcile.ResultSuccess, nil } // reconcileDelete handles the delete of an object. It first garbage collects all artifacts for the object from the // artifact storage, if successful, the finalizer is removed from the object. -func (r *HelmRepositoryReconciler) reconcileDelete(ctx context.Context, obj *sourcev1.HelmRepository) (ctrl.Result, error) { +func (r *HelmRepositoryReconciler) reconcileDelete(ctx context.Context, obj *sourcev1.HelmRepository) (sreconcile.Result, error) { // Garbage collect the resource's artifacts if err := r.garbageCollect(ctx, obj); err != nil { // Return the error so we retry the failed garbage collection - return ctrl.Result{}, err + return sreconcile.ResultEmpty, err } // Remove our finalizer from the list controllerutil.RemoveFinalizer(obj, sourcev1.SourceFinalizer) // Stop reconciliation as the object is being deleted - return ctrl.Result{}, nil + return sreconcile.ResultEmpty, nil } // garbageCollect performs a garbage collection for the given v1beta1.HelmRepository. It removes all but the current @@ -441,25 +488,41 @@ func (r *HelmRepositoryReconciler) reconcileDelete(ctx context.Context, obj *sou func (r *HelmRepositoryReconciler) garbageCollect(ctx context.Context, obj *sourcev1.HelmRepository) error { if !obj.DeletionTimestamp.IsZero() { if err := r.Storage.RemoveAll(r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), "", "*")); err != nil { - r.Eventf(obj, corev1.EventTypeWarning, "GarbageCollectionFailed", - "Garbage collection for deleted resource failed: %s", err) - return err + return &serror.Event{ + Err: fmt.Errorf("garbage collection for deleted resource failed: %w", err), + Reason: "GarbageCollectionFailed", + } } obj.Status.Artifact = nil // TODO(hidde): we should only push this event if we actually garbage collected something - r.Eventf(obj, corev1.EventTypeNormal, "GarbageCollectionSucceeded", - "Garbage collected artifacts for deleted resource") + r.eventLogf(ctx, obj, corev1.EventTypeNormal, "GarbageCollectionSucceeded", + "garbage collected artifacts for deleted resource") return nil } if obj.GetArtifact() != nil { if err := r.Storage.RemoveAllButCurrent(*obj.GetArtifact()); err != nil { - r.Eventf(obj, corev1.EventTypeWarning, "GarbageCollectionFailed", - "Garbage collection of old artifacts failed: %s", err) - return err + return &serror.Event{ + Err: fmt.Errorf("garbage collection of old artifacts failed: %w", err), + Reason: "GarbageCollectionFailed", + } } // TODO(hidde): we should only push this event if we actually garbage collected something - r.Eventf(obj, corev1.EventTypeNormal, "GarbageCollectionSucceeded", - "Garbage collected old artifacts") + r.eventLogf(ctx, obj, corev1.EventTypeNormal, "GarbageCollectionSucceeded", + "garbage collected old artifacts") } return nil } + +// eventLog records event and logs at the same time. This log is different from +// the debug log in the event recorder in the sense that this is a simple log, +// the event recorder debug log contains complete details about the event. +func (r *HelmRepositoryReconciler) eventLogf(ctx context.Context, obj runtime.Object, eventType string, reason string, messageFmt string, args ...interface{}) { + msg := fmt.Sprintf(messageFmt, args...) + // Log and emit event. + if eventType == corev1.EventTypeWarning { + ctrl.LoggerFrom(ctx).Error(errors.New(reason), msg) + } else { + ctrl.LoggerFrom(ctx).Info(msg) + } + r.Eventf(obj, eventType, reason, msg) +} diff --git a/controllers/helmrepository_controller_test.go b/controllers/helmrepository_controller_test.go index 55bbe557..f397a3d3 100644 --- a/controllers/helmrepository_controller_test.go +++ b/controllers/helmrepository_controller_test.go @@ -24,7 +24,9 @@ import ( "path/filepath" "strings" "testing" + "time" + "github.com/darkowlzz/controller-check/status" "github.com/go-logr/logr" . "github.com/onsi/gomega" "helm.sh/helm/v3/pkg/getter" @@ -32,7 +34,6 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/tools/record" - ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/log" @@ -40,9 +41,12 @@ import ( "github.com/fluxcd/pkg/apis/meta" "github.com/fluxcd/pkg/helmtestserver" "github.com/fluxcd/pkg/runtime/conditions" + "github.com/fluxcd/pkg/runtime/patch" sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" + serror "github.com/fluxcd/source-controller/internal/error" "github.com/fluxcd/source-controller/internal/helm/repository" + sreconcile "github.com/fluxcd/source-controller/internal/reconcile" ) var ( @@ -99,9 +103,15 @@ func TestHelmRepositoryReconciler_Reconcile(t *testing.T) { } readyCondition := conditions.Get(obj, meta.ReadyCondition) return readyCondition.Status == metav1.ConditionTrue && - obj.Generation == readyCondition.ObservedGeneration + obj.Generation == readyCondition.ObservedGeneration && + obj.Generation == obj.Status.ObservedGeneration }, timeout).Should(BeTrue()) + // Check if the object status is valid. + condns := &status.Conditions{NegativePolarity: helmRepoReadyDepsNegative} + checker := status.NewChecker(testEnv.Client, testEnv.GetScheme(), condns) + checker.CheckErr(ctx, obj) + g.Expect(testEnv.Delete(ctx, obj)).To(Succeed()) // Wait for HelmRepository to be deleted @@ -117,7 +127,7 @@ func TestHelmRepositoryReconciler_reconcileStorage(t *testing.T) { tests := []struct { name string beforeFunc func(obj *sourcev1.HelmRepository, storage *Storage) error - want ctrl.Result + want sreconcile.Result wantErr bool assertArtifact *sourcev1.Artifact assertConditions []metav1.Condition @@ -154,6 +164,7 @@ func TestHelmRepositoryReconciler_reconcileStorage(t *testing.T) { "!/reconcile-storage/b.txt", "!/reconcile-storage/a.txt", }, + want: sreconcile.ResultSuccess, }, { name: "notices missing artifact in storage", @@ -165,12 +176,12 @@ func TestHelmRepositoryReconciler_reconcileStorage(t *testing.T) { testStorage.SetArtifactURL(obj.Status.Artifact) return nil }, - want: ctrl.Result{Requeue: true}, + want: sreconcile.ResultSuccess, assertPaths: []string{ "!/reconcile-storage/invalid.txt", }, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactUnavailableCondition, "NoArtifact", "No artifact for resource in storage"), + *conditions.TrueCondition(meta.ReconcilingCondition, "NoArtifact", "no artifact for resource in storage"), }, }, { @@ -190,6 +201,7 @@ func TestHelmRepositoryReconciler_reconcileStorage(t *testing.T) { } return nil }, + want: sreconcile.ResultSuccess, assertPaths: []string{ "/reconcile-storage/hostname.txt", }, @@ -219,7 +231,10 @@ func TestHelmRepositoryReconciler_reconcileStorage(t *testing.T) { g.Expect(tt.beforeFunc(obj, testStorage)).To(Succeed()) } - got, err := r.reconcileStorage(context.TODO(), obj) + var chartRepo repository.ChartRepository + var artifact sourcev1.Artifact + + got, err := r.reconcileStorage(context.TODO(), obj, &artifact, &chartRepo) g.Expect(err != nil).To(Equal(tt.wantErr)) g.Expect(got).To(Equal(tt.want)) @@ -257,16 +272,17 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { secret *corev1.Secret beforeFunc func(t *WithT, obj *sourcev1.HelmRepository) afterFunc func(t *WithT, obj *sourcev1.HelmRepository) - want ctrl.Result + want sreconcile.Result wantErr bool assertConditions []metav1.Condition }{ { name: "HTTP without secretRef makes ArtifactOutdated=True", protocol: "http", - want: ctrl.Result{RequeueAfter: interval}, + want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "New index revision"), + *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision"), + *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new index revision"), }, }, { @@ -288,9 +304,10 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository) { obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "basic-auth"} }, - want: ctrl.Result{RequeueAfter: interval}, + want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "New index revision"), + *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision"), + *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new index revision"), }, }, { @@ -312,9 +329,10 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository) { obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "ca-file"} }, - want: ctrl.Result{RequeueAfter: interval}, + want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "New index revision"), + *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision"), + *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new index revision"), }, }, { @@ -342,25 +360,25 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { }, }, { - name: "Invalid URL makes FetchFailed=True and returns zero Result", + name: "Invalid URL makes FetchFailed=True and returns stalling error", protocol: "http", beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository) { obj.Spec.URL = strings.ReplaceAll(obj.Spec.URL, "http://", "") }, - want: ctrl.Result{}, - wantErr: false, + want: sreconcile.ResultEmpty, + wantErr: true, assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.URLInvalidReason, "first path segment in URL cannot contain colon"), }, }, { - name: "Unsupported scheme makes FetchFailed=True and returns zero Result", + name: "Unsupported scheme makes FetchFailed=True and returns stalling error", protocol: "http", beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository) { obj.Spec.URL = strings.ReplaceAll(obj.Spec.URL, "http://", "ftp://") }, - want: ctrl.Result{}, - wantErr: false, + want: sreconcile.ResultEmpty, + wantErr: true, assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.FetchFailedCondition, meta.FailedReason, "scheme \"ftp\" not supported"), }, @@ -482,7 +500,7 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) { name string beforeFunc func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, index *repository.ChartRepository) afterFunc func(t *WithT, obj *sourcev1.HelmRepository) - want ctrl.Result + want sreconcile.Result wantErr bool assertConditions []metav1.Condition }{ @@ -491,9 +509,10 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) { beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, index *repository.ChartRepository) { obj.Spec.Interval = metav1.Duration{Duration: interval} }, - want: ctrl.Result{RequeueAfter: interval}, + want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "Stored artifact for revision 'existing'"), + *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'existing'"), + *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new index revision 'existing'"), }, }, { @@ -505,20 +524,9 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) { afterFunc: func(t *WithT, obj *sourcev1.HelmRepository) { t.Expect(obj.Status.URL).To(BeEmpty()) }, - want: ctrl.Result{RequeueAfter: interval}, + want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "Stored artifact for revision 'existing'"), - }, - }, - { - name: "Removes ArtifactUnavailableCondition after creating artifact", - beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, index *repository.ChartRepository) { - obj.Spec.Interval = metav1.Duration{Duration: interval} - conditions.MarkTrue(obj, sourcev1.ArtifactUnavailableCondition, "Foo", "") - }, - want: ctrl.Result{RequeueAfter: interval}, - assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "Stored artifact for revision 'existing'"), + *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'existing'"), }, }, { @@ -527,9 +535,10 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) { obj.Spec.Interval = metav1.Duration{Duration: interval} conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "Foo", "") }, - want: ctrl.Result{RequeueAfter: interval}, + want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "Stored artifact for revision 'existing'"), + *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'existing'"), + *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new index revision 'existing'"), }, }, { @@ -544,9 +553,10 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) { t.Expect(err).NotTo(HaveOccurred()) t.Expect(localPath).To(Equal(targetFile)) }, - want: ctrl.Result{RequeueAfter: interval}, + want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "Stored artifact for revision 'existing'"), + *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'existing'"), + *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new index revision 'existing'"), }, }, } @@ -598,7 +608,7 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) { } dlog := log.NewDelegatingLogSink(log.NullLogSink{}) nullLogger := logr.New(dlog) - got, err := r.reconcileArtifact(logr.NewContext(ctx, nullLogger), obj, artifact, chartRepo) + got, err := r.reconcileArtifact(logr.NewContext(ctx, nullLogger), obj, &artifact, chartRepo) g.Expect(err != nil).To(Equal(tt.wantErr)) g.Expect(got).To(Equal(tt.want)) @@ -615,3 +625,290 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) { }) } } + +func TestHelmRepositoryReconciler_summarizeAndPatch(t *testing.T) { + tests := []struct { + name string + generation int64 + beforeFunc func(obj *sourcev1.HelmRepository) + result sreconcile.Result + reconcileErr error + wantErr bool + afterFunc func(t *WithT, obj *sourcev1.HelmRepository) + assertConditions []metav1.Condition + }{ + // Success/Fail indicates if a reconciliation succeeded or failed. On + // a successful reconciliation, the object generation is expected to + // match the observed generation in the object status. + // All the cases have some Ready condition set, even if a test case is + // unrelated to the conditions, because it's neseccary for a valid + // status. + { + name: "Success, no extra conditions", + generation: 4, + beforeFunc: func(obj *sourcev1.HelmRepository) { + conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "test-msg") + }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "test-msg"), + }, + afterFunc: func(t *WithT, obj *sourcev1.HelmRepository) { + t.Expect(obj.Status.ObservedGeneration).To(Equal(int64(4))) + }, + }, + { + name: "Success, Ready=True", + generation: 5, + beforeFunc: func(obj *sourcev1.HelmRepository) { + conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "created") + }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "created"), + }, + afterFunc: func(t *WithT, obj *sourcev1.HelmRepository) { + t.Expect(obj.Status.ObservedGeneration).To(Equal(int64(5))) + }, + }, + { + name: "Success, removes reconciling for successful result", + generation: 2, + beforeFunc: func(obj *sourcev1.HelmRepository) { + conditions.MarkReconciling(obj, "NewRevision", "new index version") + conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "stored artifact") + }, + result: sreconcile.ResultSuccess, + wantErr: false, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact"), + }, + afterFunc: func(t *WithT, obj *sourcev1.HelmRepository) { + t.Expect(obj.Status.ObservedGeneration).To(Equal(int64(2))) + }, + }, + { + name: "Success, record reconciliation request", + beforeFunc: func(obj *sourcev1.HelmRepository) { + annotations := map[string]string{ + meta.ReconcileRequestAnnotation: "now", + } + obj.SetAnnotations(annotations) + conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "test-msg") + }, + generation: 3, + result: sreconcile.ResultSuccess, + wantErr: false, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "test-msg"), + }, + afterFunc: func(t *WithT, obj *sourcev1.HelmRepository) { + t.Expect(obj.Status.LastHandledReconcileAt).To(Equal("now")) + t.Expect(obj.Status.ObservedGeneration).To(Equal(int64(3))) + }, + }, + { + name: "Fail, with multiple conditions ArtifactOutdated=True,Reconciling=True", + generation: 7, + beforeFunc: func(obj *sourcev1.HelmRepository) { + conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision") + conditions.MarkReconciling(obj, "NewRevision", "new index revision") + }, + reconcileErr: fmt.Errorf("failed to create dir"), + wantErr: true, + assertConditions: []metav1.Condition{ + *conditions.FalseCondition(meta.ReadyCondition, "NewRevision", "new index revision"), + *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision"), + *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new index revision"), + }, + afterFunc: func(t *WithT, obj *sourcev1.HelmRepository) { + t.Expect(obj.Status.ObservedGeneration).ToNot(Equal(int64(7))) + }, + }, + { + name: "Success, with subreconciler stalled error", + generation: 9, + beforeFunc: func(obj *sourcev1.HelmRepository) { + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.FetchFailedCondition, "failed to construct helm client") + }, + reconcileErr: &serror.Stalling{Err: fmt.Errorf("some error"), Reason: "some reason"}, + wantErr: false, + assertConditions: []metav1.Condition{ + *conditions.FalseCondition(meta.ReadyCondition, sourcev1.FetchFailedCondition, "failed to construct helm client"), + *conditions.TrueCondition(meta.StalledCondition, "some reason", "some error"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.FetchFailedCondition, "failed to construct helm client"), + }, + afterFunc: func(t *WithT, obj *sourcev1.HelmRepository) { + t.Expect(obj.Status.ObservedGeneration).To(Equal(int64(9))) + }, + }, + { + name: "Fail, no error but requeue requested", + generation: 3, + beforeFunc: func(obj *sourcev1.HelmRepository) { + conditions.MarkFalse(obj, meta.ReadyCondition, meta.FailedReason, "test-msg") + }, + result: sreconcile.ResultRequeue, + assertConditions: []metav1.Condition{ + *conditions.FalseCondition(meta.ReadyCondition, meta.FailedReason, "test-msg"), + }, + afterFunc: func(t *WithT, obj *sourcev1.HelmRepository) { + t.Expect(obj.Status.ObservedGeneration).ToNot(Equal(int64(3))) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + builder := fakeclient.NewClientBuilder().WithScheme(testEnv.GetScheme()) + r := &HelmRepositoryReconciler{ + Client: builder.Build(), + } + obj := &sourcev1.HelmRepository{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "test-", + Generation: tt.generation, + }, + Spec: sourcev1.HelmRepositorySpec{ + Interval: metav1.Duration{Duration: 5 * time.Second}, + }, + } + + if tt.beforeFunc != nil { + tt.beforeFunc(obj) + } + + ctx := context.TODO() + g.Expect(r.Create(ctx, obj)).To(Succeed()) + patchHelper, err := patch.NewHelper(obj, r.Client) + g.Expect(err).ToNot(HaveOccurred()) + + gotErr := r.summarizeAndPatch(ctx, obj, patchHelper, tt.result, tt.reconcileErr) + g.Expect(gotErr != nil).To(Equal(tt.wantErr)) + + g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(tt.assertConditions)) + + if tt.afterFunc != nil { + tt.afterFunc(g, obj) + } + + // Check if the object status is valid. + condns := &status.Conditions{NegativePolarity: helmRepoReadyDepsNegative} + checker := status.NewChecker(r.Client, testEnv.GetScheme(), condns) + checker.CheckErr(ctx, obj) + }) + } +} + +func TestHelmRepositoryReconciler_reconcileSubRecs(t *testing.T) { + // Helper to build simple helmRepoReconcilerFunc with result and error. + buildReconcileFuncs := func(r sreconcile.Result, e error) helmRepoReconcilerFunc { + return func(ctx context.Context, obj *sourcev1.HelmRepository, artifact *sourcev1.Artifact, repo *repository.ChartRepository) (sreconcile.Result, error) { + return r, e + } + } + + tests := []struct { + name string + generation int64 + observedGeneration int64 + reconcileFuncs []helmRepoReconcilerFunc + wantResult sreconcile.Result + wantErr bool + assertConditions []metav1.Condition + }{ + { + name: "successful reconciliations", + reconcileFuncs: []helmRepoReconcilerFunc{ + buildReconcileFuncs(sreconcile.ResultSuccess, nil), + }, + wantResult: sreconcile.ResultSuccess, + wantErr: false, + }, + { + name: "successful reconciliation with generation difference", + generation: 3, + observedGeneration: 2, + reconcileFuncs: []helmRepoReconcilerFunc{ + buildReconcileFuncs(sreconcile.ResultSuccess, nil), + }, + wantResult: sreconcile.ResultSuccess, + wantErr: false, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, "NewGeneration", "reconciling new generation 3"), + }, + }, + { + name: "failed reconciliation", + reconcileFuncs: []helmRepoReconcilerFunc{ + buildReconcileFuncs(sreconcile.ResultEmpty, fmt.Errorf("some error")), + }, + wantResult: sreconcile.ResultEmpty, + wantErr: true, + }, + { + name: "multiple object status conditions mutations", + reconcileFuncs: []helmRepoReconcilerFunc{ + func(ctx context.Context, obj *sourcev1.HelmRepository, artifact *sourcev1.Artifact, repo *repository.ChartRepository) (sreconcile.Result, error) { + conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision") + return sreconcile.ResultSuccess, nil + }, + func(ctx context.Context, obj *sourcev1.HelmRepository, artifact *sourcev1.Artifact, repo *repository.ChartRepository) (sreconcile.Result, error) { + conditions.MarkTrue(obj, meta.ReconcilingCondition, "Progressing", "creating artifact") + return sreconcile.ResultSuccess, nil + }, + }, + wantResult: sreconcile.ResultSuccess, + wantErr: false, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision"), + *conditions.TrueCondition(meta.ReconcilingCondition, "Progressing", "creating artifact"), + }, + }, + { + name: "subrecs with one result=Requeue, no error", + reconcileFuncs: []helmRepoReconcilerFunc{ + buildReconcileFuncs(sreconcile.ResultSuccess, nil), + buildReconcileFuncs(sreconcile.ResultRequeue, nil), + buildReconcileFuncs(sreconcile.ResultSuccess, nil), + }, + wantResult: sreconcile.ResultRequeue, + wantErr: false, + }, + { + name: "subrecs with error before result=Requeue", + reconcileFuncs: []helmRepoReconcilerFunc{ + buildReconcileFuncs(sreconcile.ResultSuccess, nil), + buildReconcileFuncs(sreconcile.ResultEmpty, fmt.Errorf("some error")), + buildReconcileFuncs(sreconcile.ResultRequeue, nil), + }, + wantResult: sreconcile.ResultEmpty, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + r := &HelmRepositoryReconciler{} + obj := &sourcev1.HelmRepository{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "test-", + Generation: tt.generation, + }, + Status: sourcev1.HelmRepositoryStatus{ + ObservedGeneration: tt.observedGeneration, + }, + } + + ctx := context.TODO() + + gotRes, gotErr := r.reconcile(ctx, obj, tt.reconcileFuncs) + g.Expect(gotErr != nil).To(Equal(tt.wantErr)) + g.Expect(gotRes).To(Equal(tt.wantResult)) + + g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(tt.assertConditions)) + }) + } +}