From 5caa076753ce3311a4bbb192c6adb81b709b0770 Mon Sep 17 00:00:00 2001 From: Sunny Date: Mon, 28 Aug 2023 20:15:09 +0000 Subject: [PATCH] Remove event error Remove deprecated Event error. Event error was used for scenarios where an error should result in an event/notification. It was introduced as a contextual error along with Stalling and Waiting errors but was later replaced with Generic error which doesn't have any contextual meaning. The Generic error provided error configuration which allowed defining how the error should be handled. This replaced the contextual error handling with error action handlers which behaved on the error configuration of the errors. The Generic error was first introduced to be used in GitRepository reconciler and was used by new reconcilers like the OCIRepository reconcilers. The old reconcilers bucket, helmrepository and helmchart reconcilers were still using the deprecated Event error. This change replaces the Event errors in these reconcilers with a Generic error. It also fixes a bug in the Generic error constructor which configured the error to be logged by default. This resulted in an error to be logged by the result processor and the runtime, double logging. This behavior has been changed to not log explicitly and allow the runtime to log the error. Since the Generic error is based on defining the error handling behavior in the error configuration, a generic error that needs to be ignored (not returned to the runtime), but logged can enable the logging behavior explicitly on the Generic error instance. This is done in GitRepository reconciler for no-op reconciliations where an ignore error is returned. Signed-off-by: Sunny --- internal/controller/bucket_controller.go | 84 ++++----- .../controller/gitrepository_controller.go | 10 +- internal/controller/helmchart_controller.go | 162 +++++++++--------- .../controller/helmchart_controller_test.go | 10 +- .../controller/helmrepository_controller.go | 106 ++++++------ internal/error/error.go | 26 +-- internal/reconcile/summarize/processor.go | 21 --- internal/reconcile/summarize/summary_test.go | 2 +- 8 files changed, 190 insertions(+), 231 deletions(-) diff --git a/internal/controller/bucket_controller.go b/internal/controller/bucket_controller.go index 521fb2546..d6598aea8 100644 --- a/internal/controller/bucket_controller.go +++ b/internal/controller/bucket_controller.go @@ -200,7 +200,7 @@ func (r *BucketReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res summarize.WithReconcileError(retErr), summarize.WithIgnoreNotFound(), summarize.WithProcessors( - summarize.RecordContextualError, + summarize.ErrorActionHandler, summarize.RecordReconcileReq, ), summarize.WithResultBuilder(sreconcile.AlwaysRequeueResultBuilder{ @@ -279,10 +279,10 @@ func (r *BucketReconciler) reconcile(ctx context.Context, sp *patch.SerialPatche // Create temp working dir tmpDir, err := os.MkdirTemp("", fmt.Sprintf("%s-%s-%s-", obj.Kind, obj.Namespace, obj.Name)) if err != nil { - e := &serror.Event{ - Err: fmt.Errorf("failed to create temporary working directory: %w", err), - Reason: sourcev1.DirCreationFailedReason, - } + e := serror.NewGeneric( + fmt.Errorf("failed to create temporary working directory: %w", err), + sourcev1.DirCreationFailedReason, + ) conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } @@ -423,7 +423,7 @@ func (r *BucketReconciler) reconcileStorage(ctx context.Context, sp *patch.Seria func (r *BucketReconciler) reconcileSource(ctx context.Context, sp *patch.SerialPatcher, obj *bucketv1.Bucket, index *index.Digester, dir string) (sreconcile.Result, error) { secret, err := r.getBucketSecret(ctx, obj) if err != nil { - e := &serror.Event{Err: err, Reason: sourcev1.AuthenticationFailedReason} + e := serror.NewGeneric(err, sourcev1.AuthenticationFailedReason) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Error()) // Return error as the world as observed may change return sreconcile.ResultEmpty, e @@ -434,34 +434,34 @@ func (r *BucketReconciler) reconcileSource(ctx context.Context, sp *patch.Serial switch obj.Spec.Provider { case bucketv1.GoogleBucketProvider: if err = gcp.ValidateSecret(secret); err != nil { - e := &serror.Event{Err: err, Reason: sourcev1.AuthenticationFailedReason} + e := serror.NewGeneric(err, sourcev1.AuthenticationFailedReason) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Error()) return sreconcile.ResultEmpty, e } if provider, err = gcp.NewClient(ctx, secret); err != nil { - e := &serror.Event{Err: err, Reason: "ClientError"} + e := serror.NewGeneric(err, "ClientError") conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Error()) return sreconcile.ResultEmpty, e } case bucketv1.AzureBucketProvider: if err = azure.ValidateSecret(secret); err != nil { - e := &serror.Event{Err: err, Reason: sourcev1.AuthenticationFailedReason} + e := serror.NewGeneric(err, sourcev1.AuthenticationFailedReason) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Error()) return sreconcile.ResultEmpty, e } if provider, err = azure.NewClient(obj, secret); err != nil { - e := &serror.Event{Err: err, Reason: "ClientError"} + e := serror.NewGeneric(err, "ClientError") conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Error()) return sreconcile.ResultEmpty, e } default: if err = minio.ValidateSecret(secret); err != nil { - e := &serror.Event{Err: err, Reason: sourcev1.AuthenticationFailedReason} + e := serror.NewGeneric(err, sourcev1.AuthenticationFailedReason) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Error()) return sreconcile.ResultEmpty, e } if provider, err = minio.NewClient(obj, secret); err != nil { - e := &serror.Event{Err: err, Reason: "ClientError"} + e := serror.NewGeneric(err, "ClientError") conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Error()) return sreconcile.ResultEmpty, e } @@ -469,7 +469,7 @@ func (r *BucketReconciler) reconcileSource(ctx context.Context, sp *patch.Serial // Fetch etag index if err = fetchEtagIndex(ctx, provider, obj, index, dir); err != nil { - e := &serror.Event{Err: err, Reason: bucketv1.BucketOperationFailedReason} + e := serror.NewGeneric(err, bucketv1.BucketOperationFailedReason) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Error()) return sreconcile.ResultEmpty, e } @@ -501,7 +501,7 @@ func (r *BucketReconciler) reconcileSource(ctx context.Context, sp *patch.Serial }() if err = fetchIndexFiles(ctx, provider, obj, index, dir); err != nil { - e := &serror.Event{Err: err, Reason: bucketv1.BucketOperationFailedReason} + e := serror.NewGeneric(err, bucketv1.BucketOperationFailedReason) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Error()) return sreconcile.ResultEmpty, e } @@ -550,45 +550,45 @@ func (r *BucketReconciler) reconcileArtifact(ctx context.Context, sp *patch.Seri // Ensure target path exists and is a directory if f, err := os.Stat(dir); err != nil { - e := &serror.Event{ - Err: fmt.Errorf("failed to stat source path: %w", err), - Reason: sourcev1.StatOperationFailedReason, - } + e := serror.NewGeneric( + fmt.Errorf("failed to stat source path: %w", err), + sourcev1.StatOperationFailedReason, + ) conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } else if !f.IsDir() { - e := &serror.Event{ - Err: fmt.Errorf("source path '%s' is not a directory", dir), - Reason: sourcev1.InvalidPathReason, - } + e := serror.NewGeneric( + fmt.Errorf("source path '%s' is not a directory", dir), + sourcev1.InvalidPathReason, + ) conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } // Ensure artifact directory exists and acquire lock if err := r.Storage.MkdirAll(artifact); err != nil { - e := &serror.Event{ - Err: fmt.Errorf("failed to create artifact directory: %w", err), - Reason: sourcev1.DirCreationFailedReason, - } + e := serror.NewGeneric( + fmt.Errorf("failed to create artifact directory: %w", err), + sourcev1.DirCreationFailedReason, + ) conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } unlock, err := r.Storage.Lock(artifact) if err != nil { - return sreconcile.ResultEmpty, &serror.Event{ - Err: fmt.Errorf("failed to acquire lock for artifact: %w", err), - Reason: meta.FailedReason, - } + return sreconcile.ResultEmpty, serror.NewGeneric( + fmt.Errorf("failed to acquire lock for artifact: %w", err), + meta.FailedReason, + ) } defer unlock() // Archive directory to storage if err := r.Storage.Archive(&artifact, dir, nil); err != nil { - e := &serror.Event{ - Err: fmt.Errorf("unable to archive artifact to storage: %s", err), - Reason: sourcev1.ArchiveOperationFailedReason, - } + e := serror.NewGeneric( + fmt.Errorf("unable to archive artifact to storage: %s", err), + sourcev1.ArchiveOperationFailedReason, + ) conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } @@ -635,10 +635,10 @@ func (r *BucketReconciler) reconcileDelete(ctx context.Context, obj *bucketv1.Bu func (r *BucketReconciler) garbageCollect(ctx context.Context, obj *bucketv1.Bucket) error { if !obj.DeletionTimestamp.IsZero() { if deleted, err := r.Storage.RemoveAll(r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), "", "*")); err != nil { - return &serror.Event{ - Err: fmt.Errorf("garbage collection for deleted resource failed: %s", err), - Reason: "GarbageCollectionFailed", - } + return serror.NewGeneric( + fmt.Errorf("garbage collection for deleted resource failed: %s", err), + "GarbageCollectionFailed", + ) } else if deleted != "" { r.eventLogf(ctx, obj, eventv1.EventTypeTrace, "GarbageCollectionSucceeded", "garbage collected artifacts for deleted resource") @@ -649,10 +649,10 @@ func (r *BucketReconciler) garbageCollect(ctx context.Context, obj *bucketv1.Buc if obj.GetArtifact() != nil { delFiles, err := r.Storage.GarbageCollect(ctx, *obj.GetArtifact(), time.Second*5) if err != nil { - return &serror.Event{ - Err: fmt.Errorf("garbage collection of artifacts failed: %w", err), - Reason: "GarbageCollectionFailed", - } + return serror.NewGeneric( + fmt.Errorf("garbage collection of artifacts failed: %w", err), + "GarbageCollectionFailed", + ) } if len(delFiles) > 0 { r.eventLogf(ctx, obj, eventv1.EventTypeTrace, "GarbageCollectionSucceeded", diff --git a/internal/controller/gitrepository_controller.go b/internal/controller/gitrepository_controller.go index 3dfa9c91e..cd6062dfd 100644 --- a/internal/controller/gitrepository_controller.go +++ b/internal/controller/gitrepository_controller.go @@ -561,6 +561,8 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, sp *patch ) ge.Notification = false ge.Ignore = true + // Log it as this will not be passed to the runtime. + ge.Log = true ge.Event = corev1.EventTypeNormal // Remove any stale fetch failed condition. conditions.Delete(obj, sourcev1.FetchFailedCondition) @@ -815,10 +817,10 @@ func (r *GitRepositoryReconciler) reconcileInclude(ctx context.Context, sp *patc // Copy artifact (sub)contents to configured directory. if err := r.Storage.CopyToPath(artifact, incl.GetFromPath(), toPath); err != nil { - e := &serror.Event{ - Err: fmt.Errorf("failed to copy '%s' include from %s to %s: %w", incl.GitRepositoryRef.Name, incl.GetFromPath(), incl.GetToPath(), err), - Reason: "CopyFailure", - } + e := serror.NewGeneric( + fmt.Errorf("failed to copy '%s' include from %s to %s: %w", incl.GitRepositoryRef.Name, incl.GetFromPath(), incl.GetToPath(), err), + "CopyFailure", + ) conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } diff --git a/internal/controller/helmchart_controller.go b/internal/controller/helmchart_controller.go index 35a896f92..7ef584384 100644 --- a/internal/controller/helmchart_controller.go +++ b/internal/controller/helmchart_controller.go @@ -215,7 +215,7 @@ func (r *HelmChartReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( summarize.WithReconcileError(retErr), summarize.WithIgnoreNotFound(), summarize.WithProcessors( - summarize.RecordContextualError, + summarize.ErrorActionHandler, summarize.RecordReconcileReq, ), summarize.WithResultBuilder(sreconcile.AlwaysRequeueResultBuilder{ @@ -420,19 +420,19 @@ func (r *HelmChartReconciler) reconcileSource(ctx context.Context, sp *patch.Ser // Retrieve the source s, err := r.getSource(ctx, obj) if err != nil { - e := &serror.Event{ - Err: fmt.Errorf("failed to get source: %w", err), - Reason: "SourceUnavailable", - } + e := serror.NewGeneric( + fmt.Errorf("failed to get source: %w", err), + "SourceUnavailable", + ) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) // Return Kubernetes client errors, but ignore others which can only be // solved by a change in generation if apierrs.ReasonForError(err) == metav1.StatusReasonUnknown { - return sreconcile.ResultEmpty, &serror.Stalling{ - Err: fmt.Errorf("failed to get source: %w", err), - Reason: "UnsupportedSourceKind", - } + return sreconcile.ResultEmpty, serror.NewStalling( + fmt.Errorf("failed to get source: %w", err), + "UnsupportedSourceKind", + ) } return sreconcile.ResultEmpty, e } @@ -471,15 +471,15 @@ func (r *HelmChartReconciler) reconcileSource(ctx context.Context, sp *patch.Ser // Handle any build error if retErr != nil { if buildErr := new(chart.BuildError); errors.As(retErr, &buildErr) { - retErr = &serror.Event{ - Err: buildErr, - Reason: buildErr.Reason.Reason, - } + retErr = serror.NewGeneric( + buildErr, + buildErr.Reason.Reason, + ) if chart.IsPersistentBuildErrorReason(buildErr.Reason) { - retErr = &serror.Stalling{ - Err: buildErr, - Reason: buildErr.Reason.Reason, - } + retErr = serror.NewStalling( + buildErr, + buildErr.Reason.Reason, + ) } } } @@ -516,10 +516,10 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * clientOpts, certsTmpDir, err := getter.GetClientOpts(ctxTimeout, r.Client, repo, normalizedURL) if err != nil && !errors.Is(err, getter.ErrDeprecatedTLSConfig) { - e := &serror.Event{ - Err: err, - Reason: sourcev1.AuthenticationFailedReason, - } + e := serror.NewGeneric( + err, + sourcev1.AuthenticationFailedReason, + ) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } @@ -549,10 +549,10 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * // or rework to enable reusing credentials to avoid the unneccessary handshake operations registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.TlsConfig, clientOpts.MustLoginToRegistry()) if err != nil { - e := &serror.Event{ - Err: fmt.Errorf("failed to construct Helm client: %w", err), - Reason: meta.FailedReason, - } + e := serror.NewGeneric( + fmt.Errorf("failed to construct Helm client: %w", err), + meta.FailedReason, + ) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } @@ -574,10 +574,10 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * if obj.Spec.Verify.SecretRef == nil { provider = fmt.Sprintf("%s keyless", provider) } - e := &serror.Event{ - Err: fmt.Errorf("failed to verify the signature using provider '%s': %w", provider, err), - Reason: sourcev1.VerificationError, - } + e := serror.NewGeneric( + fmt.Errorf("failed to verify the signature using provider '%s': %w", provider, err), + sourcev1.VerificationError, + ) conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } @@ -599,10 +599,10 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * if clientOpts.MustLoginToRegistry() { err = ociChartRepo.Login(clientOpts.RegLoginOpts...) if err != nil { - e := &serror.Event{ - Err: fmt.Errorf("failed to login to OCI registry: %w", err), - Reason: sourcev1.AuthenticationFailedReason, - } + e := serror.NewGeneric( + fmt.Errorf("failed to login to OCI registry: %w", err), + sourcev1.AuthenticationFailedReason, + ) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } @@ -683,10 +683,10 @@ func (r *HelmChartReconciler) buildFromTarballArtifact(ctx context.Context, obj // Create temporary working directory tmpDir, err := util.TempDirForObj("", obj) if err != nil { - e := &serror.Event{ - Err: fmt.Errorf("failed to create temporary working directory: %w", err), - Reason: sourcev1.DirCreationFailedReason, - } + e := serror.NewGeneric( + fmt.Errorf("failed to create temporary working directory: %w", err), + sourcev1.DirCreationFailedReason, + ) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } @@ -695,10 +695,10 @@ func (r *HelmChartReconciler) buildFromTarballArtifact(ctx context.Context, obj // Create directory to untar source into sourceDir := filepath.Join(tmpDir, "source") if err := os.Mkdir(sourceDir, 0o700); err != nil { - e := &serror.Event{ - Err: fmt.Errorf("failed to create directory to untar source into: %w", err), - Reason: sourcev1.DirCreationFailedReason, - } + e := serror.NewGeneric( + fmt.Errorf("failed to create directory to untar source into: %w", err), + sourcev1.DirCreationFailedReason, + ) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } @@ -706,25 +706,25 @@ func (r *HelmChartReconciler) buildFromTarballArtifact(ctx context.Context, obj // Open the tarball artifact file and untar files into working directory f, err := os.Open(r.Storage.LocalPath(source)) if err != nil { - e := &serror.Event{ - Err: fmt.Errorf("failed to open source artifact: %w", err), - Reason: sourcev1.ReadOperationFailedReason, - } + e := serror.NewGeneric( + fmt.Errorf("failed to open source artifact: %w", err), + sourcev1.ReadOperationFailedReason, + ) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } if err = tar.Untar(f, sourceDir, tar.WithMaxUntarSize(-1)); err != nil { _ = f.Close() - return sreconcile.ResultEmpty, &serror.Event{ - Err: fmt.Errorf("artifact untar error: %w", err), - Reason: meta.FailedReason, - } + return sreconcile.ResultEmpty, serror.NewGeneric( + fmt.Errorf("artifact untar error: %w", err), + meta.FailedReason, + ) } if err = f.Close(); err != nil { - return sreconcile.ResultEmpty, &serror.Event{ - Err: fmt.Errorf("artifact close error: %w", err), - Reason: meta.FailedReason, - } + return sreconcile.ResultEmpty, serror.NewGeneric( + fmt.Errorf("artifact close error: %w", err), + meta.FailedReason, + ) } // Setup dependency manager @@ -834,19 +834,19 @@ func (r *HelmChartReconciler) reconcileArtifact(ctx context.Context, _ *patch.Se // Ensure artifact directory exists and acquire lock if err := r.Storage.MkdirAll(artifact); err != nil { - e := &serror.Event{ - Err: fmt.Errorf("failed to create artifact directory: %w", err), - Reason: sourcev1.DirCreationFailedReason, - } + e := serror.NewGeneric( + fmt.Errorf("failed to create artifact directory: %w", err), + sourcev1.DirCreationFailedReason, + ) conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } unlock, err := r.Storage.Lock(artifact) if err != nil { - e := &serror.Event{ - Err: fmt.Errorf("failed to acquire lock for artifact: %w", err), - Reason: sourcev1.AcquireLockFailedReason, - } + e := serror.NewGeneric( + fmt.Errorf("failed to acquire lock for artifact: %w", err), + sourcev1.AcquireLockFailedReason, + ) conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } @@ -854,10 +854,10 @@ func (r *HelmChartReconciler) reconcileArtifact(ctx context.Context, _ *patch.Se // Copy the packaged chart to the artifact path if err = r.Storage.CopyFromPath(&artifact, b.Path); err != nil { - e := &serror.Event{ - Err: fmt.Errorf("unable to copy Helm chart to storage: %w", err), - Reason: sourcev1.ArchiveOperationFailedReason, - } + e := serror.NewGeneric( + fmt.Errorf("unable to copy Helm chart to storage: %w", err), + sourcev1.ArchiveOperationFailedReason, + ) conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } @@ -938,10 +938,10 @@ func (r *HelmChartReconciler) reconcileDelete(ctx context.Context, obj *helmv1.H func (r *HelmChartReconciler) garbageCollect(ctx context.Context, obj *helmv1.HelmChart) error { if !obj.DeletionTimestamp.IsZero() { if deleted, err := r.Storage.RemoveAll(r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), "", "*")); err != nil { - return &serror.Event{ - Err: fmt.Errorf("garbage collection for deleted resource failed: %w", err), - Reason: "GarbageCollectionFailed", - } + return serror.NewGeneric( + fmt.Errorf("garbage collection for deleted resource failed: %w", err), + "GarbageCollectionFailed", + ) } else if deleted != "" { r.eventLogf(ctx, obj, eventv1.EventTypeTrace, "GarbageCollectionSucceeded", "garbage collected artifacts for deleted resource") @@ -952,10 +952,10 @@ func (r *HelmChartReconciler) garbageCollect(ctx context.Context, obj *helmv1.He if obj.GetArtifact() != nil { delFiles, err := r.Storage.GarbageCollect(ctx, *obj.GetArtifact(), time.Second*5) if err != nil { - return &serror.Event{ - Err: fmt.Errorf("garbage collection of artifacts failed: %w", err), - Reason: "GarbageCollectionFailed", - } + return serror.NewGeneric( + fmt.Errorf("garbage collection of artifacts failed: %w", err), + "GarbageCollectionFailed", + ) } if len(delFiles) > 0 { r.eventLogf(ctx, obj, eventv1.EventTypeTrace, "GarbageCollectionSucceeded", @@ -1275,17 +1275,17 @@ func reasonForBuild(build *chart.Build) string { func chartRepoConfigErrorReturn(err error, obj *helmv1.HelmChart) (sreconcile.Result, error) { switch err.(type) { case *url.Error: - e := &serror.Stalling{ - Err: fmt.Errorf("invalid Helm repository URL: %w", err), - Reason: sourcev1.URLInvalidReason, - } + e := serror.NewStalling( + fmt.Errorf("invalid Helm repository URL: %w", err), + sourcev1.URLInvalidReason, + ) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e default: - e := &serror.Stalling{ - Err: fmt.Errorf("failed to construct Helm client: %w", err), - Reason: meta.FailedReason, - } + e := serror.NewStalling( + fmt.Errorf("failed to construct Helm client: %w", err), + meta.FailedReason, + ) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } diff --git a/internal/controller/helmchart_controller_test.go b/internal/controller/helmchart_controller_test.go index 9d45271dc..3d5fc5c7d 100644 --- a/internal/controller/helmchart_controller_test.go +++ b/internal/controller/helmchart_controller_test.go @@ -659,7 +659,7 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) { conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") }, want: sreconcile.ResultEmpty, - wantErr: &serror.Event{Err: errors.New("gitrepositories.source.toolkit.fluxcd.io \"unavailable\" not found")}, + wantErr: &serror.Generic{Err: errors.New("gitrepositories.source.toolkit.fluxcd.io \"unavailable\" not found")}, assertFunc: func(g *WithT, build chart.Build, obj helmv1.HelmChart) { g.Expect(build.Complete()).To(BeFalse()) @@ -963,7 +963,7 @@ func TestHelmChartReconciler_buildFromHelmRepository(t *testing.T) { } }, want: sreconcile.ResultEmpty, - wantErr: &serror.Event{Err: errors.New("failed to get authentication secret '/invalid'")}, + wantErr: &serror.Generic{Err: errors.New("failed to get authentication secret '/invalid'")}, assertFunc: func(g *WithT, obj *helmv1.HelmChart, build chart.Build) { g.Expect(build.Complete()).To(BeFalse()) @@ -1231,7 +1231,7 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) { } }, want: sreconcile.ResultEmpty, - wantErr: &serror.Event{Err: errors.New("failed to get authentication secret '/invalid'")}, + wantErr: &serror.Generic{Err: errors.New("failed to get authentication secret '/invalid'")}, assertFunc: func(g *WithT, obj *helmv1.HelmChart, build chart.Build) { g.Expect(build.Complete()).To(BeFalse()) @@ -1463,7 +1463,7 @@ func TestHelmChartReconciler_buildFromTarballArtifact(t *testing.T) { name: "Empty source artifact", source: sourcev1.Artifact{}, want: sreconcile.ResultEmpty, - wantErr: &serror.Event{Err: errors.New("no such file or directory")}, + wantErr: &serror.Generic{Err: errors.New("no such file or directory")}, assertFunc: func(g *WithT, build chart.Build) { g.Expect(build.Complete()).To(BeFalse()) }, @@ -1472,7 +1472,7 @@ func TestHelmChartReconciler_buildFromTarballArtifact(t *testing.T) { name: "Invalid artifact type", source: *yamlArtifact, want: sreconcile.ResultEmpty, - wantErr: &serror.Event{Err: errors.New("artifact untar error: requires gzip-compressed body")}, + wantErr: &serror.Generic{Err: errors.New("artifact untar error: requires gzip-compressed body")}, assertFunc: func(g *WithT, build chart.Build) { g.Expect(build.Complete()).To(BeFalse()) }, diff --git a/internal/controller/helmrepository_controller.go b/internal/controller/helmrepository_controller.go index eb871a1f1..c8462bea6 100644 --- a/internal/controller/helmrepository_controller.go +++ b/internal/controller/helmrepository_controller.go @@ -177,7 +177,7 @@ func (r *HelmRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reque summarize.WithReconcileError(retErr), summarize.WithIgnoreNotFound(), summarize.WithProcessors( - summarize.RecordContextualError, + summarize.ErrorActionHandler, summarize.RecordReconcileReq, ), summarize.WithResultBuilder(sreconcile.AlwaysRequeueResultBuilder{ @@ -393,10 +393,10 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patc obj *helmv1.HelmRepository, artifact *sourcev1.Artifact, chartRepo *repository.ChartRepository) (sreconcile.Result, error) { normalizedURL, err := repository.NormalizeURL(obj.Spec.URL) if err != nil { - e := &serror.Stalling{ - Err: fmt.Errorf("invalid Helm repository URL: %w", err), - Reason: sourcev1.URLInvalidReason, - } + e := serror.NewStalling( + fmt.Errorf("invalid Helm repository URL: %w", err), + sourcev1.URLInvalidReason, + ) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } @@ -407,10 +407,10 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patc ctrl.LoggerFrom(ctx). Info("warning: specifying TLS authentication data via `.spec.secretRef` is deprecated, please use `.spec.certSecretRef` instead") } else { - e := &serror.Event{ - Err: err, - Reason: sourcev1.AuthenticationFailedReason, - } + e := serror.NewGeneric( + err, + sourcev1.AuthenticationFailedReason, + ) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } @@ -421,17 +421,17 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patc if err != nil { switch err.(type) { case *url.Error: - e := &serror.Stalling{ - Err: fmt.Errorf("invalid Helm repository URL: %w", err), - Reason: sourcev1.URLInvalidReason, - } + e := serror.NewStalling( + fmt.Errorf("invalid Helm repository URL: %w", err), + sourcev1.URLInvalidReason, + ) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e default: - e := &serror.Stalling{ - Err: fmt.Errorf("failed to construct Helm client: %w", err), - Reason: meta.FailedReason, - } + e := serror.NewStalling( + fmt.Errorf("failed to construct Helm client: %w", err), + meta.FailedReason, + ) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } @@ -439,10 +439,10 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patc // Fetch the repository index from remote. if err := newChartRepo.CacheIndex(); err != nil { - e := &serror.Event{ - Err: fmt.Errorf("failed to fetch Helm repository index: %w", err), - Reason: meta.FailedReason, - } + e := serror.NewGeneric( + fmt.Errorf("failed to fetch Helm repository index: %w", err), + meta.FailedReason, + ) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) // Coin flip on transient or persistent error, return error and hope for the best return sreconcile.ResultEmpty, e @@ -465,10 +465,10 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patc // Load the cached repository index to ensure it passes validation. if err := chartRepo.LoadFromPath(); err != nil { - e := &serror.Event{ - Err: fmt.Errorf("failed to load Helm repository from index YAML: %w", err), - Reason: helmv1.IndexationFailedReason, - } + e := serror.NewGeneric( + fmt.Errorf("failed to load Helm repository from index YAML: %w", err), + helmv1.IndexationFailedReason, + ) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } @@ -478,10 +478,10 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patc // Calculate revision. revision := chartRepo.Digest(intdigest.Canonical) if revision.Validate() != nil { - e := &serror.Event{ - Err: fmt.Errorf("failed to calculate revision: %w", err), - Reason: helmv1.IndexationFailedReason, - } + e := serror.NewGeneric( + fmt.Errorf("failed to calculate revision: %w", err), + helmv1.IndexationFailedReason, + ) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } @@ -541,10 +541,10 @@ func (r *HelmRepositoryReconciler) reconcileArtifact(ctx context.Context, sp *pa // Create artifact dir if err := r.Storage.MkdirAll(*artifact); err != nil { - e := &serror.Event{ - Err: fmt.Errorf("failed to create artifact directory: %w", err), - Reason: sourcev1.DirCreationFailedReason, - } + e := serror.NewGeneric( + fmt.Errorf("failed to create artifact directory: %w", err), + sourcev1.DirCreationFailedReason, + ) conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } @@ -552,28 +552,28 @@ func (r *HelmRepositoryReconciler) reconcileArtifact(ctx context.Context, sp *pa // Acquire lock. unlock, err := r.Storage.Lock(*artifact) if err != nil { - return sreconcile.ResultEmpty, &serror.Event{ - Err: fmt.Errorf("failed to acquire lock for artifact: %w", err), - Reason: meta.FailedReason, - } + return sreconcile.ResultEmpty, serror.NewGeneric( + fmt.Errorf("failed to acquire lock for artifact: %w", err), + meta.FailedReason, + ) } defer unlock() // Save artifact to storage in JSON format. b, err := chartRepo.ToJSON() if err != nil { - e := &serror.Event{ - Err: fmt.Errorf("unable to get JSON index from chart repo: %w", err), - Reason: sourcev1.ArchiveOperationFailedReason, - } + e := serror.NewGeneric( + fmt.Errorf("unable to get JSON index from chart repo: %w", err), + sourcev1.ArchiveOperationFailedReason, + ) conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } if err = r.Storage.Copy(artifact, bytes.NewBuffer(b)); err != nil { - e := &serror.Event{ - Err: fmt.Errorf("unable to save artifact to storage: %w", err), - Reason: sourcev1.ArchiveOperationFailedReason, - } + e := serror.NewGeneric( + fmt.Errorf("unable to save artifact to storage: %w", err), + sourcev1.ArchiveOperationFailedReason, + ) conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } @@ -639,10 +639,10 @@ func (r *HelmRepositoryReconciler) reconcileDelete(ctx context.Context, obj *hel func (r *HelmRepositoryReconciler) garbageCollect(ctx context.Context, obj *helmv1.HelmRepository) error { if !obj.DeletionTimestamp.IsZero() || (obj.Spec.Type != "" && obj.Spec.Type != helmv1.HelmRepositoryTypeDefault) { if deleted, err := r.Storage.RemoveAll(r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), "", "*")); err != nil { - return &serror.Event{ - Err: fmt.Errorf("garbage collection for deleted resource failed: %w", err), - Reason: "GarbageCollectionFailed", - } + return serror.NewGeneric( + fmt.Errorf("garbage collection for deleted resource failed: %w", err), + "GarbageCollectionFailed", + ) } else if deleted != "" { r.eventLogf(ctx, obj, eventv1.EventTypeTrace, "GarbageCollectionSucceeded", "garbage collected artifacts for deleted resource") @@ -657,10 +657,10 @@ func (r *HelmRepositoryReconciler) garbageCollect(ctx context.Context, obj *helm if obj.GetArtifact() != nil { delFiles, err := r.Storage.GarbageCollect(ctx, *obj.GetArtifact(), time.Second*5) if err != nil { - return &serror.Event{ - Err: fmt.Errorf("garbage collection of artifacts failed: %w", err), - Reason: "GarbageCollectionFailed", - } + return serror.NewGeneric( + fmt.Errorf("garbage collection of artifacts failed: %w", err), + "GarbageCollectionFailed", + ) } if len(delFiles) > 0 { r.eventLogf(ctx, obj, eventv1.EventTypeTrace, "GarbageCollectionSucceeded", diff --git a/internal/error/error.go b/internal/error/error.go index 0852ba412..cb3a8cd78 100644 --- a/internal/error/error.go +++ b/internal/error/error.go @@ -90,28 +90,6 @@ func NewStalling(err error, reason string) *Stalling { } } -// Event is an error event. It can be used to construct an event to be -// recorded. -// Deprecated: use Generic error with NewGeneric() for the same behavior and -// replace the RecordContextualError with ErrorActionHandler for result -// processing. -type Event struct { - // Reason is the reason for the event error. - Reason string - // Error is the actual error for the event. - Err error -} - -// Error implements error interface. -func (ee *Event) Error() string { - return ee.Err.Error() -} - -// Unwrap returns the underlying error. -func (ee *Event) Unwrap() error { - return ee.Err -} - // Waiting is the reconciliation wait state error. It contains an error, wait // duration and a reason for the wait. It is a contextual error, used to express // the scenario which contributed to the reconciliation result. @@ -176,13 +154,13 @@ func (g *Generic) Unwrap() error { // NewGeneric constructs a new Generic error with default configuration. func NewGeneric(err error, reason string) *Generic { - // Since it's a error, ensure to log and send failure notification. + // Since it's a generic error, it'll be returned to the runtime and logged + // automatically, do not log it. Send failure notification. return &Generic{ Reason: reason, Err: err, Config: Config{ Event: corev1.EventTypeWarning, - Log: true, Notification: true, }, } diff --git a/internal/reconcile/summarize/processor.go b/internal/reconcile/summarize/processor.go index dcee87360..746ca7c8e 100644 --- a/internal/reconcile/summarize/processor.go +++ b/internal/reconcile/summarize/processor.go @@ -36,27 +36,6 @@ import ( // reconciliation failure. The errors can be recorded as logs and events. type ResultProcessor func(context.Context, kuberecorder.EventRecorder, client.Object, reconcile.Result, error) -// RecordContextualError is a ResultProcessor that records the contextual errors -// based on their types. -// An event is recorded for the errors that are returned to the runtime. The -// runtime handles the logging of the error. -// An event is recorded and an error is logged for errors that are known to be -// swallowed, not returned to the runtime. -func RecordContextualError(ctx context.Context, recorder kuberecorder.EventRecorder, obj client.Object, _ reconcile.Result, err error) { - switch e := err.(type) { - case *serror.Event: - recorder.Eventf(obj, corev1.EventTypeWarning, e.Reason, e.Error()) - case *serror.Waiting: - // Waiting errors are not returned to the runtime. Log it explicitly. - ctrl.LoggerFrom(ctx).Info("reconciliation waiting", "reason", e.Err, "duration", e.RequeueAfter) - recorder.Event(obj, corev1.EventTypeNormal, e.Reason, e.Error()) - case *serror.Stalling: - // Stalling errors are not returned to the runtime. Log it explicitly. - ctrl.LoggerFrom(ctx).Error(e, "reconciliation stalled") - recorder.Eventf(obj, corev1.EventTypeWarning, e.Reason, e.Error()) - } -} - // RecordReconcileReq is a ResultProcessor that checks the reconcile // annotation value and sets it in the object status as // status.lastHandledReconcileAt. diff --git a/internal/reconcile/summarize/summary_test.go b/internal/reconcile/summarize/summary_test.go index 6064fcbd9..c7703a940 100644 --- a/internal/reconcile/summarize/summary_test.go +++ b/internal/reconcile/summarize/summary_test.go @@ -357,7 +357,7 @@ func TestSummarizeAndPatch(t *testing.T) { WithReconcileError(tt.reconcileErr), WithConditions(tt.conditions...), WithIgnoreNotFound(), - WithProcessors(RecordContextualError, RecordReconcileReq), + WithProcessors(ErrorActionHandler, RecordReconcileReq), WithResultBuilder(reconcile.AlwaysRequeueResultBuilder{RequeueAfter: obj.Spec.Interval.Duration}), } if tt.bipolarConditions != nil {