From 22ca0484461a946ffe005f95de721b3d4246c722 Mon Sep 17 00:00:00 2001 From: Karl Isenberg Date: Tue, 5 Nov 2024 11:41:12 -0800 Subject: [PATCH] chore: SyncStatusClient return status.Errors (#1476) Returning status.Error instead of just error simplifies usage of the return value, which is usually appended to a MultiError or returned directly to state.invalidate(err). The caller doesn't need to append, if there's no other error that needs to be reported to the RSync status. This change also fixes SetImageToSyncAnnotation to return APIServer errors, instead of wrapping all errors as Source errors. This is better because most error types from client.Patch are APIServer errors, and we have no explicit interface contract to detect OCI image signature validation errors from a webhook. --- .../oci_signature_verification_test.go | 12 +++--- pkg/parse/repo_sync_client.go | 38 +++++++++-------- pkg/parse/root_sync_client.go | 42 ++++++++++--------- pkg/parse/root_sync_client_test.go | 4 +- pkg/parse/run.go | 17 ++++---- pkg/parse/sync_status_client.go | 20 +++++---- 6 files changed, 70 insertions(+), 63 deletions(-) diff --git a/e2e/testcases/oci_signature_verification_test.go b/e2e/testcases/oci_signature_verification_test.go index 024525762..dd1095066 100644 --- a/e2e/testcases/oci_signature_verification_test.go +++ b/e2e/testcases/oci_signature_verification_test.go @@ -63,7 +63,7 @@ func TestAddPreSyncAnnotationRepoSync(t *testing.T) { nt.Must(rootSyncGitRepo.Add(nomostest.StructuredNSPath(repoSyncID.Namespace, repoSyncID.Name), repoSyncOCI)) nt.Must(rootSyncGitRepo.CommitAndPush("Set the RepoSync to sync from OCI")) nt.Must(nt.Watcher.WatchObject(kinds.RepoSyncV1Beta1(), repoSyncID.Name, namespaceRepo, - testwatcher.WatchPredicates(testpredicates.RepoSyncHasSourceError(status.SourceErrorCode, "no signatures found")))) + testwatcher.WatchPredicates(testpredicates.RepoSyncHasSourceError(status.APIServerErrorCode, "no signatures found")))) nt.T.Log("Create second image with latest tag.") bookstoreSA := k8sobjects.ServiceAccountObject("bookstore-sa") @@ -81,8 +81,8 @@ func TestAddPreSyncAnnotationRepoSync(t *testing.T) { nt.Must( nt.Watcher.WatchObject(kinds.RepoSyncV1Beta1(), repoSyncID.Name, namespaceRepo, testwatcher.WatchPredicates( - testpredicates.RepoSyncHasSourceError(status.SourceErrorCode, "no signatures found"), - testpredicates.RepoSyncHasSourceError(status.SourceErrorCode, image1.Digest)))) + testpredicates.RepoSyncHasSourceError(status.APIServerErrorCode, "no signatures found"), + testpredicates.RepoSyncHasSourceError(status.APIServerErrorCode, image1.Digest)))) if err = signImage(nt, image1); err != nil { nt.T.Fatalf("Failed to sign second test image %s", err) @@ -139,7 +139,7 @@ func TestAddPreSyncAnnotationRootSync(t *testing.T) { rootSyncOCI := nt.RootSyncObjectOCI(rootSyncKey.Name, image.OCIImageID().WithoutDigest(), "", image.Digest) nt.Must(nt.KubeClient.Apply(rootSyncOCI)) nt.Must(nt.Watcher.WatchObject(kinds.RootSyncV1Beta1(), rootSyncID.Name, configsync.ControllerNamespace, - testwatcher.WatchPredicates(testpredicates.RootSyncHasSourceError(status.SourceErrorCode, "no signatures found")))) + testwatcher.WatchPredicates(testpredicates.RootSyncHasSourceError(status.APIServerErrorCode, "no signatures found")))) nt.T.Log("Create second image with latest tag.") bookstoreSA := k8sobjects.ServiceAccountObject("bookstore-sa") @@ -157,8 +157,8 @@ func TestAddPreSyncAnnotationRootSync(t *testing.T) { nt.Must( nt.Watcher.WatchObject(kinds.RootSyncV1Beta1(), rootSyncID.Name, configsync.ControllerNamespace, testwatcher.WatchPredicates( - testpredicates.RootSyncHasSourceError(status.SourceErrorCode, "no signatures found"), - testpredicates.RootSyncHasSourceError(status.SourceErrorCode, image1.Digest)))) + testpredicates.RootSyncHasSourceError(status.APIServerErrorCode, "no signatures found"), + testpredicates.RootSyncHasSourceError(status.APIServerErrorCode, image1.Digest)))) if err = signImage(nt, image1); err != nil { nt.T.Fatalf("Failed to sign second test image %s", err) diff --git a/pkg/parse/repo_sync_client.go b/pkg/parse/repo_sync_client.go index b79c7d8ae..75eef5f7c 100644 --- a/pkg/parse/repo_sync_client.go +++ b/pkg/parse/repo_sync_client.go @@ -48,15 +48,15 @@ type repoSyncStatusClient struct { // // SetSourceStatus sets the source status with a given source state and set of errors. If errs is empty, all errors // will be removed from the status. -func (p *repoSyncStatusClient) SetSourceStatus(ctx context.Context, newStatus *SourceStatus) error { +func (p *repoSyncStatusClient) SetSourceStatus(ctx context.Context, newStatus *SourceStatus) status.Error { p.mux.Lock() defer p.mux.Unlock() return p.setSourceStatusWithRetries(ctx, newStatus, defaultDenominator) } -func (p *repoSyncStatusClient) setSourceStatusWithRetries(ctx context.Context, newStatus *SourceStatus, denominator int) error { +func (p *repoSyncStatusClient) setSourceStatusWithRetries(ctx context.Context, newStatus *SourceStatus, denominator int) status.Error { if denominator <= 0 { - return fmt.Errorf("The denominator must be a positive number") + return status.InternalErrorf("denominator must be positive: %d", denominator) } opts := p.options @@ -111,7 +111,7 @@ func (p *repoSyncStatusClient) setSourceStatusWithRetries(ctx context.Context, n return nil } -func (p *repoSyncStatusClient) SetSourceAnnotations(ctx context.Context, commit string) error { +func (p *repoSyncStatusClient) SetImageToSyncAnnotation(ctx context.Context, commit string) status.Error { opts := p.options rs := &v1beta1.RepoSync{} rs.Namespace = string(opts.Scope) @@ -132,15 +132,13 @@ func (p *repoSyncStatusClient) SetSourceAnnotations(ctx context.Context, commit err := opts.Client.Patch(ctx, rs, client.RawPatch(types.MergePatchType, []byte(patch)), client.FieldOwner(configsync.FieldManager)) - if err != nil { - return fmt.Errorf("failed to patch RepoSync annotations: %w\nPatch content: %s", err, patch) + return status.APIServerErrorf(err, "failed to patch RepoSync annotation: %s", metadata.ImageToSyncAnnotationKey) } - return nil } -func (p *repoSyncStatusClient) SetRequiresRendering(ctx context.Context, renderingRequired bool) error { +func (p *repoSyncStatusClient) SetRequiresRenderingAnnotation(ctx context.Context, renderingRequired bool) status.Error { opts := p.options rs := &v1beta1.RepoSync{} if err := opts.Client.Get(ctx, reposync.ObjectKey(opts.Scope, opts.SyncName), rs); err != nil { @@ -153,11 +151,17 @@ func (p *repoSyncStatusClient) SetRequiresRendering(ctx context.Context, renderi } existing := rs.DeepCopy() core.SetAnnotation(rs, metadata.RequiresRenderingAnnotationKey, newVal) - return opts.Client.Patch(ctx, rs, client.MergeFrom(existing), client.FieldOwner(configsync.FieldManager)) + err := opts.Client.Patch(ctx, rs, + client.MergeFrom(existing), + client.FieldOwner(configsync.FieldManager)) + if err != nil { + return status.APIServerErrorf(err, "failed to patch RepoSync annotation: %s", metadata.RequiresRenderingAnnotationKey) + } + return nil } // SetRenderingStatus implements the Parser interface -func (p *repoSyncStatusClient) SetRenderingStatus(ctx context.Context, oldStatus, newStatus *RenderingStatus) error { +func (p *repoSyncStatusClient) SetRenderingStatus(ctx context.Context, oldStatus, newStatus *RenderingStatus) status.Error { if oldStatus.Equals(newStatus) { return nil } @@ -167,9 +171,9 @@ func (p *repoSyncStatusClient) SetRenderingStatus(ctx context.Context, oldStatus return p.setRenderingStatusWithRetries(ctx, newStatus, defaultDenominator) } -func (p *repoSyncStatusClient) setRenderingStatusWithRetries(ctx context.Context, newStatus *RenderingStatus, denominator int) error { +func (p *repoSyncStatusClient) setRenderingStatusWithRetries(ctx context.Context, newStatus *RenderingStatus, denominator int) status.Error { if denominator <= 0 { - return fmt.Errorf("The denominator must be a positive number") + return status.InternalErrorf("denominator must be positive: %d", denominator) } opts := p.options @@ -219,8 +223,8 @@ func (p *repoSyncStatusClient) setRenderingStatusWithRetries(ctx context.Context return nil } -// ReconcilerStatusFromCluster gets the RepoSync sync status from the cluster. -func (p *repoSyncStatusClient) ReconcilerStatusFromCluster(ctx context.Context) (*ReconcilerStatus, error) { +// GetReconcilerStatus gets the RepoSync sync status from the cluster. +func (p *repoSyncStatusClient) GetReconcilerStatus(ctx context.Context) (*ReconcilerStatus, status.Error) { opts := p.options rs := &v1beta1.RepoSync{} if err := opts.Client.Get(ctx, reposync.ObjectKey(opts.Scope, opts.SyncName), rs); err != nil { @@ -249,15 +253,15 @@ func (p *repoSyncStatusClient) ReconcilerStatusFromCluster(ctx context.Context) // SetSyncStatus implements the Parser interface // SetSyncStatus sets the RepoSync sync status. // `errs` includes the errors encountered during the apply step; -func (p *repoSyncStatusClient) SetSyncStatus(ctx context.Context, newStatus *SyncStatus) error { +func (p *repoSyncStatusClient) SetSyncStatus(ctx context.Context, newStatus *SyncStatus) status.Error { p.mux.Lock() defer p.mux.Unlock() return p.setSyncStatusWithRetries(ctx, newStatus, defaultDenominator) } -func (p *repoSyncStatusClient) setSyncStatusWithRetries(ctx context.Context, newStatus *SyncStatus, denominator int) error { +func (p *repoSyncStatusClient) setSyncStatusWithRetries(ctx context.Context, newStatus *SyncStatus, denominator int) status.Error { if denominator <= 0 { - return fmt.Errorf("The denominator must be a positive number") + return status.InternalErrorf("denominator must be positive: %d", denominator) } opts := p.options diff --git a/pkg/parse/root_sync_client.go b/pkg/parse/root_sync_client.go index 753dba983..10a343391 100644 --- a/pkg/parse/root_sync_client.go +++ b/pkg/parse/root_sync_client.go @@ -75,15 +75,15 @@ type rootSyncStatusClient struct { } // SetSourceStatus implements the Parser interface -func (p *rootSyncStatusClient) SetSourceStatus(ctx context.Context, newStatus *SourceStatus) error { +func (p *rootSyncStatusClient) SetSourceStatus(ctx context.Context, newStatus *SourceStatus) status.Error { p.mux.Lock() defer p.mux.Unlock() return p.setSourceStatusWithRetries(ctx, newStatus, defaultDenominator) } -func (p *rootSyncStatusClient) setSourceStatusWithRetries(ctx context.Context, newStatus *SourceStatus, denominator int) error { +func (p *rootSyncStatusClient) setSourceStatusWithRetries(ctx context.Context, newStatus *SourceStatus, denominator int) status.Error { if denominator <= 0 { - return fmt.Errorf("The denominator must be a positive number") + return status.InternalErrorf("denominator must be positive: %d", denominator) } opts := p.options @@ -176,7 +176,7 @@ func setSourceStatusFields(source *v1beta1.SourceStatus, newStatus *SourceStatus source.LastUpdate = newStatus.LastUpdate } -func (p *rootSyncStatusClient) SetSourceAnnotations(ctx context.Context, commit string) error { +func (p *rootSyncStatusClient) SetImageToSyncAnnotation(ctx context.Context, commit string) status.Error { opts := p.options rs := &v1beta1.RootSync{} rs.Namespace = configsync.ControllerNamespace @@ -197,15 +197,13 @@ func (p *rootSyncStatusClient) SetSourceAnnotations(ctx context.Context, commit err := opts.Client.Patch(ctx, rs, client.RawPatch(types.MergePatchType, []byte(patch)), client.FieldOwner(configsync.FieldManager)) - if err != nil { - return fmt.Errorf("failed to patch RootSync annotations: %w\nPatch content: %s", err, patch) + return status.APIServerErrorf(err, "failed to patch RootSync annotation: %s", metadata.ImageToSyncAnnotationKey) } - return nil } -func (p *rootSyncStatusClient) SetRequiresRendering(ctx context.Context, renderingRequired bool) error { +func (p *rootSyncStatusClient) SetRequiresRenderingAnnotation(ctx context.Context, renderingRequired bool) status.Error { opts := p.options rs := &v1beta1.RootSync{} if err := opts.Client.Get(ctx, rootsync.ObjectKey(opts.SyncName), rs); err != nil { @@ -218,11 +216,17 @@ func (p *rootSyncStatusClient) SetRequiresRendering(ctx context.Context, renderi } existing := rs.DeepCopy() core.SetAnnotation(rs, metadata.RequiresRenderingAnnotationKey, newVal) - return opts.Client.Patch(ctx, rs, client.MergeFrom(existing), client.FieldOwner(configsync.FieldManager)) + err := opts.Client.Patch(ctx, rs, + client.MergeFrom(existing), + client.FieldOwner(configsync.FieldManager)) + if err != nil { + return status.APIServerErrorf(err, "failed to patch RootSync annotation: %s", metadata.RequiresRenderingAnnotationKey) + } + return nil } // SetRenderingStatus implements the Parser interface -func (p *rootSyncStatusClient) SetRenderingStatus(ctx context.Context, oldStatus, newStatus *RenderingStatus) error { +func (p *rootSyncStatusClient) SetRenderingStatus(ctx context.Context, oldStatus, newStatus *RenderingStatus) status.Error { if oldStatus.Equals(newStatus) { return nil } @@ -232,9 +236,9 @@ func (p *rootSyncStatusClient) SetRenderingStatus(ctx context.Context, oldStatus return p.setRenderingStatusWithRetries(ctx, newStatus, defaultDenominator) } -func (p *rootSyncStatusClient) setRenderingStatusWithRetries(ctx context.Context, newStatus *RenderingStatus, denominator int) error { +func (p *rootSyncStatusClient) setRenderingStatusWithRetries(ctx context.Context, newStatus *RenderingStatus, denominator int) status.Error { if denominator <= 0 { - return fmt.Errorf("The denominator must be a positive number") + return status.InternalErrorf("denominator must be positive: %d", denominator) } opts := p.options @@ -327,8 +331,8 @@ func setRenderingStatusFields(rendering *v1beta1.RenderingStatus, newStatus *Ren rendering.LastUpdate = newStatus.LastUpdate } -// ReconcilerStatusFromCluster gets the RootSync sync status from the cluster. -func (p *rootSyncStatusClient) ReconcilerStatusFromCluster(ctx context.Context) (*ReconcilerStatus, error) { +// GetReconcilerStatus gets the RootSync sync status from the cluster. +func (p *rootSyncStatusClient) GetReconcilerStatus(ctx context.Context) (*ReconcilerStatus, status.Error) { opts := p.options rs := &v1beta1.RootSync{} if err := opts.Client.Get(ctx, rootsync.ObjectKey(opts.SyncName), rs); err != nil { @@ -461,15 +465,15 @@ func reconcilerStatusFromRSyncStatus(rsyncStatus v1beta1.Status, sourceType conf // SetSyncStatus implements the Parser interface // SetSyncStatus sets the RootSync sync status. // `errs` includes the errors encountered during the apply step; -func (p *rootSyncStatusClient) SetSyncStatus(ctx context.Context, newStatus *SyncStatus) error { +func (p *rootSyncStatusClient) SetSyncStatus(ctx context.Context, newStatus *SyncStatus) status.Error { p.mux.Lock() defer p.mux.Unlock() return p.setSyncStatusWithRetries(ctx, newStatus, defaultDenominator) } -func (p *rootSyncStatusClient) setSyncStatusWithRetries(ctx context.Context, newStatus *SyncStatus, denominator int) error { +func (p *rootSyncStatusClient) setSyncStatusWithRetries(ctx context.Context, newStatus *SyncStatus, denominator int) status.Error { if denominator <= 0 { - return fmt.Errorf("The denominator must be a positive number") + return status.InternalErrorf("denominator must be positive: %d", denominator) } opts := p.options @@ -589,9 +593,9 @@ func summarizeErrorsForCommit(sourceStatus v1beta1.SourceStatus, renderingStatus } // prependRootSyncRemediatorStatus adds the conflict error detected by the remediator to the front of the sync errors. -func prependRootSyncRemediatorStatus(ctx context.Context, c client.Client, syncName string, conflictErrs []status.ManagementConflictError, denominator int) error { +func prependRootSyncRemediatorStatus(ctx context.Context, c client.Client, syncName string, conflictErrs []status.ManagementConflictError, denominator int) status.Error { if denominator <= 0 { - return fmt.Errorf("The denominator must be a positive number") + return status.InternalErrorf("denominator must be positive: %d", denominator) } var rs v1beta1.RootSync diff --git a/pkg/parse/root_sync_client_test.go b/pkg/parse/root_sync_client_test.go index 6fc31ed9b..82cf077db 100644 --- a/pkg/parse/root_sync_client_test.go +++ b/pkg/parse/root_sync_client_test.go @@ -569,8 +569,8 @@ For more information, see https://g.co/cloud/acm-errors#knv1060` []status.ManagementConflictError{conflictAB}, defaultDenominator) require.NoError(t, err) var updatedRootSync v1beta1.RootSync - err = fakeClient.Get(ctx, rootsync.ObjectKey(rootSyncName), &updatedRootSync) - require.NoError(t, err) + statuserr := fakeClient.Get(ctx, rootsync.ObjectKey(rootSyncName), &updatedRootSync) + require.NoError(t, statuserr) testutil.AssertEqual(t, tc.expectedErrors, updatedRootSync.Status.Sync.Errors) }) } diff --git a/pkg/parse/run.go b/pkg/parse/run.go index c588684af..342762d09 100644 --- a/pkg/parse/run.go +++ b/pkg/parse/run.go @@ -93,9 +93,9 @@ func DefaultRunFunc(ctx context.Context, r Reconciler, trigger string) RunResult // Initialize status // TODO: Populate status from RSync status if state.status == nil { - reconcilerStatus, err := r.SyncStatusClient().ReconcilerStatusFromCluster(ctx) + reconcilerStatus, err := r.SyncStatusClient().GetReconcilerStatus(ctx) if err != nil { - state.invalidate(status.Append(nil, err)) + state.invalidate(err) return result } state.status = reconcilerStatus @@ -119,9 +119,8 @@ func DefaultRunFunc(ctx context.Context, r Reconciler, trigger string) RunResult // If updating the object fails, it's likely due to a signature verification error // from the webhook. In this case, add the error as a source error. if gs.Errs == nil { - err := r.SyncStatusClient().SetSourceAnnotations(ctx, gs.Commit) - if err != nil { - gs.Errs = status.Append(gs.Errs, status.SourceError.Wrap(err).Build()) + if err := r.SyncStatusClient().SetImageToSyncAnnotation(ctx, gs.Commit); err != nil { + gs.Errs = status.Append(gs.Errs, err) } } @@ -177,8 +176,7 @@ func DefaultRunFunc(ctx context.Context, r Reconciler, trigger string) RunResult state.status.RenderingStatus = rs state.status.SyncingConditionLastUpdate = rs.LastUpdate } else { - var m status.MultiError - state.invalidate(status.Append(m, setRenderingStatusErr)) + state.invalidate(setRenderingStatusErr) } return result } @@ -252,9 +250,8 @@ func (r *reconciler) Read(ctx context.Context, trigger string, sourceState *sour if opts.RenderingEnabled != hydrationStatus.RequiresRendering { // the reconciler is misconfigured. set the annotation so that the reconciler-manager // will recreate this reconciler with the correct configuration. - if err := r.SyncStatusClient().SetRequiresRendering(ctx, hydrationStatus.RequiresRendering); err != nil { - hydrationStatus.Errs = status.Append(hydrationStatus.Errs, - status.InternalHydrationError(err, "error setting %s annotation", metadata.RequiresRenderingAnnotationKey)) + if err := r.SyncStatusClient().SetRequiresRenderingAnnotation(ctx, hydrationStatus.RequiresRendering); err != nil { + hydrationStatus.Errs = status.Append(hydrationStatus.Errs, err) } } hydrationStatus.LastUpdate = nowMeta(opts) diff --git a/pkg/parse/sync_status_client.go b/pkg/parse/sync_status_client.go index 48b5216d5..b84f0a60b 100644 --- a/pkg/parse/sync_status_client.go +++ b/pkg/parse/sync_status_client.go @@ -16,20 +16,22 @@ package parse import ( "context" + + "kpt.dev/configsync/pkg/status" ) // SyncStatusClient provides methods to read and write RSync object status. type SyncStatusClient interface { - // ReconcilerStatusFromCluster reads the status of the reconciler from the RSync status. - ReconcilerStatusFromCluster(ctx context.Context) (*ReconcilerStatus, error) + // GetReconcilerStatus reads the status of the reconciler from the RSync status. + GetReconcilerStatus(ctx context.Context) (*ReconcilerStatus, status.Error) // SetSourceStatus sets the source status and syncing condition on the RSync. - SetSourceStatus(ctx context.Context, newStatus *SourceStatus) error + SetSourceStatus(ctx context.Context, newStatus *SourceStatus) status.Error // SetRenderingStatus sets the rendering status and syncing condition on the RSync. - SetRenderingStatus(ctx context.Context, oldStatus, newStatus *RenderingStatus) error + SetRenderingStatus(ctx context.Context, oldStatus, newStatus *RenderingStatus) status.Error // SetSyncStatus sets the sync status and syncing condition on the RSync. - SetSyncStatus(ctx context.Context, newStatus *SyncStatus) error - // SetRequiresRendering sets the requires-rendering annotation on the RSync. - SetRequiresRendering(ctx context.Context, renderingRequired bool) error - // SetSourceAnnotations sets the source annotations on the RSync. - SetSourceAnnotations(ctx context.Context, commit string) error + SetSyncStatus(ctx context.Context, newStatus *SyncStatus) status.Error + // SetRequiresRenderingAnnotation sets the requires-rendering annotation on the RSync. + SetRequiresRenderingAnnotation(ctx context.Context, renderingRequired bool) status.Error + // SetImageToSyncAnnotation sets the source annotations on the RSync. + SetImageToSyncAnnotation(ctx context.Context, commit string) status.Error }