Skip to content

Commit

Permalink
chore: SyncStatusClient return status.Errors (#1476)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@karlkfi
karlkfi authored Nov 5, 2024
1 parent d3026c7 commit 22ca048
Show file tree
Hide file tree
Showing 6 changed files with 70 additions and 63 deletions.
12 changes: 6 additions & 6 deletions e2e/testcases/oci_signature_verification_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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)
Expand Down Expand Up @@ -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")
Expand All @@ -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)
Expand Down
38 changes: 21 additions & 17 deletions pkg/parse/repo_sync_client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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)
Expand All @@ -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 {
Expand All @@ -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
}
Expand All @@ -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

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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

Expand Down
42 changes: 23 additions & 19 deletions pkg/parse/root_sync_client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand All @@ -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 {
Expand All @@ -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
}
Expand All @@ -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

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions pkg/parse/root_sync_client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
}
Expand Down
17 changes: 7 additions & 10 deletions pkg/parse/run.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
}
}

Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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)
Expand Down
20 changes: 11 additions & 9 deletions pkg/parse/sync_status_client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

0 comments on commit 22ca048

Please sign in to comment.