From 55573f5eb6364f799aefe51e026f344b59cc1f2a Mon Sep 17 00:00:00 2001 From: Sunny Date: Fri, 2 Dec 2022 12:46:26 +0000 Subject: [PATCH] Add progressive status in ocirepo reconciler Signed-off-by: Sunny --- controllers/ocirepository_controller.go | 76 ++++++--- controllers/ocirepository_controller_test.go | 165 ++++++++++++++----- 2 files changed, 181 insertions(+), 60 deletions(-) diff --git a/controllers/ocirepository_controller.go b/controllers/ocirepository_controller.go index c0160a31..79c091ba 100644 --- a/controllers/ocirepository_controller.go +++ b/controllers/ocirepository_controller.go @@ -31,7 +31,6 @@ import ( "time" "github.com/Masterminds/semver/v3" - eventv1 "github.com/fluxcd/pkg/apis/event/v1beta1" soci "github.com/fluxcd/source-controller/internal/oci" "github.com/google/go-containerregistry/pkg/authn" "github.com/google/go-containerregistry/pkg/authn/k8schain" @@ -54,6 +53,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/ratelimiter" + eventv1 "github.com/fluxcd/pkg/apis/event/v1beta1" "github.com/fluxcd/pkg/apis/meta" "github.com/fluxcd/pkg/oci" "github.com/fluxcd/pkg/oci/auth/login" @@ -61,9 +61,11 @@ import ( helper "github.com/fluxcd/pkg/runtime/controller" "github.com/fluxcd/pkg/runtime/patch" "github.com/fluxcd/pkg/runtime/predicates" + rreconcile "github.com/fluxcd/pkg/runtime/reconcile" "github.com/fluxcd/pkg/sourceignore" "github.com/fluxcd/pkg/untar" "github.com/fluxcd/pkg/version" + sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" serror "github.com/fluxcd/source-controller/internal/error" sreconcile "github.com/fluxcd/source-controller/internal/reconcile" @@ -120,7 +122,7 @@ func (e invalidOCIURLError) Error() string { // ociRepositoryReconcileFunc is the function type for all the v1beta2.OCIRepository // (sub)reconcile functions. The type implementations are grouped and // executed serially to perform the complete reconcile of the object. -type ociRepositoryReconcileFunc func(ctx context.Context, obj *sourcev1.OCIRepository, metadata *sourcev1.Artifact, dir string) (sreconcile.Result, error) +type ociRepositoryReconcileFunc func(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.OCIRepository, metadata *sourcev1.Artifact, dir string) (sreconcile.Result, error) // OCIRepositoryReconciler reconciles a v1beta2.OCIRepository object type OCIRepositoryReconciler struct { @@ -131,6 +133,8 @@ type OCIRepositoryReconciler struct { Storage *Storage ControllerName string requeueDependency time.Duration + + patchOptions []patch.Option } type OCIRepositoryReconcilerOptions struct { @@ -145,6 +149,8 @@ func (r *OCIRepositoryReconciler) SetupWithManager(mgr ctrl.Manager) error { } func (r *OCIRepositoryReconciler) SetupWithManagerAndOptions(mgr ctrl.Manager, opts OCIRepositoryReconcilerOptions) error { + r.patchOptions = getPatchOptions(ociRepositoryReadyCondition.Owned, r.ControllerName) + r.requeueDependency = opts.DependencyRequeueInterval return ctrl.NewControllerManagedBy(mgr). @@ -178,10 +184,7 @@ func (r *OCIRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques r.RecordSuspend(ctx, obj, obj.Spec.Suspend) // Initialize the patch helper with the current version of the object. - patchHelper, err := patch.NewHelper(obj, r.Client) - if err != nil { - return ctrl.Result{}, err - } + serialPatcher := patch.NewSerialPatcher(obj, r.Client) // recResult stores the abstracted reconcile result. var recResult sreconcile.Result @@ -189,7 +192,7 @@ func (r *OCIRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques // Always attempt to patch the object and status after each reconciliation // NOTE: The final runtime result and error are set in this block. defer func() { - summarizeHelper := summarize.NewHelper(r.EventRecorder, patchHelper) + summarizeHelper := summarize.NewHelper(r.EventRecorder, serialPatcher) summarizeOpts := []summarize.Option{ summarize.WithConditions(ociRepositoryReadyCondition), summarize.WithBiPolarityConditionTypes(sourcev1.SourceVerifiedCondition), @@ -236,19 +239,36 @@ func (r *OCIRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques r.reconcileSource, r.reconcileArtifact, } - recResult, retErr = r.reconcile(ctx, obj, reconcilers) + recResult, retErr = r.reconcile(ctx, serialPatcher, obj, reconcilers) return } // reconcile iterates through the ociRepositoryReconcileFunc tasks for the // object. It returns early on the first call that returns // reconcile.ResultRequeue, or produces an error. -func (r *OCIRepositoryReconciler) reconcile(ctx context.Context, obj *sourcev1.OCIRepository, reconcilers []ociRepositoryReconcileFunc) (sreconcile.Result, error) { +func (r *OCIRepositoryReconciler) reconcile(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.OCIRepository, reconcilers []ociRepositoryReconcileFunc) (sreconcile.Result, error) { oldObj := obj.DeepCopy() - // Mark as reconciling if generation differs. - if obj.Generation != obj.Status.ObservedGeneration { - conditions.MarkReconciling(obj, "NewGeneration", "reconciling new object generation (%d)", obj.Generation) + rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, "reconciliation in progress") + + var reconcileAtVal string + if v, ok := meta.ReconcileAnnotationValue(obj.GetAnnotations()); ok { + reconcileAtVal = v + } + + // Persist reconciling status if generation differs or reconciliation is + // requested. + switch { + case obj.Generation != obj.Status.ObservedGeneration: + rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, + "processing object: new generation %d -> %d", obj.Status.ObservedGeneration, obj.Generation) + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + return sreconcile.ResultEmpty, err + } + case reconcileAtVal != obj.Status.GetLastHandledReconcileRequest(): + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + return sreconcile.ResultEmpty, err + } } // Create temp working dir @@ -276,7 +296,7 @@ func (r *OCIRepositoryReconciler) reconcile(ctx context.Context, obj *sourcev1.O // Run the sub-reconcilers and build the result of reconciliation. for _, rec := range reconcilers { - recResult, err := rec(ctx, obj, &metadata, tmpDir) + recResult, err := rec(ctx, sp, obj, &metadata, tmpDir) // Exit immediately on ResultRequeue. if recResult == sreconcile.ResultRequeue { return sreconcile.ResultRequeue, nil @@ -299,7 +319,8 @@ func (r *OCIRepositoryReconciler) reconcile(ctx context.Context, obj *sourcev1.O // reconcileSource fetches the upstream OCI artifact metadata and content. // If this fails, it records v1beta2.FetchFailedCondition=True on the object and returns early. -func (r *OCIRepositoryReconciler) reconcileSource(ctx context.Context, obj *sourcev1.OCIRepository, metadata *sourcev1.Artifact, dir string) (sreconcile.Result, error) { +func (r *OCIRepositoryReconciler) reconcileSource(ctx context.Context, sp *patch.SerialPatcher, + obj *sourcev1.OCIRepository, metadata *sourcev1.Artifact, dir string) (sreconcile.Result, error) { var auth authn.Authenticator ctxTimeout, cancel := context.WithTimeout(ctx, obj.Spec.Timeout.Duration) @@ -385,8 +406,14 @@ func (r *OCIRepositoryReconciler) reconcileSource(ctx context.Context, obj *sour defer func() { if !obj.GetArtifact().HasRevision(revision) { message := fmt.Sprintf("new revision '%s' for '%s'", revision, url) - conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", message) - conditions.MarkReconciling(obj, "NewRevision", message) + if obj.GetArtifact() != nil { + conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", message) + } + rreconcile.ProgressiveStatus(true, obj, meta.ProgressingReason, "building artifact: %s", message) + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + ctrl.LoggerFrom(ctx).Error(err, "failed to patch") + return + } } }() @@ -876,22 +903,32 @@ func oidcAuth(ctx context.Context, url, provider string) (authn.Authenticator, e // condition is added. // The hostname of any URL in the Status of the object are updated, to ensure // they match the Storage server hostname of current runtime. -func (r *OCIRepositoryReconciler) reconcileStorage(ctx context.Context, obj *sourcev1.OCIRepository, _ *sourcev1.Artifact, _ string) (sreconcile.Result, error) { +func (r *OCIRepositoryReconciler) reconcileStorage(ctx context.Context, sp *patch.SerialPatcher, + obj *sourcev1.OCIRepository, _ *sourcev1.Artifact, _ string) (sreconcile.Result, error) { // Garbage collect previous advertised artifact(s) from storage _ = r.garbageCollect(ctx, obj) // Determine if the advertised artifact is still in storage + var artifactMissing bool if artifact := obj.GetArtifact(); artifact != nil && !r.Storage.ArtifactExist(*artifact) { obj.Status.Artifact = nil obj.Status.URL = "" + artifactMissing = true // Remove the condition as the artifact doesn't exist. conditions.Delete(obj, sourcev1.ArtifactInStorageCondition) } // Record that we do not have an artifact if obj.GetArtifact() == nil { - conditions.MarkReconciling(obj, "NoArtifact", "no artifact for resource in storage") + msg := "building artifact" + if artifactMissing { + msg += ": disappeared from storage" + } + rreconcile.ProgressiveStatus(true, obj, meta.ProgressingReason, msg) conditions.Delete(obj, sourcev1.ArtifactInStorageCondition) + if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + return sreconcile.ResultEmpty, err + } return sreconcile.ResultSuccess, nil } @@ -911,7 +948,8 @@ func (r *OCIRepositoryReconciler) reconcileStorage(ctx context.Context, obj *sou // early. // On a successful archive, the Artifact in the Status of the object is set, // and the symlink in the Storage is updated to its path. -func (r *OCIRepositoryReconciler) reconcileArtifact(ctx context.Context, obj *sourcev1.OCIRepository, metadata *sourcev1.Artifact, dir string) (sreconcile.Result, error) { +func (r *OCIRepositoryReconciler) reconcileArtifact(ctx context.Context, sp *patch.SerialPatcher, + obj *sourcev1.OCIRepository, metadata *sourcev1.Artifact, dir string) (sreconcile.Result, error) { revision := metadata.Revision // Create artifact diff --git a/controllers/ocirepository_controller_test.go b/controllers/ocirepository_controller_test.go index 061978a5..0abd1648 100644 --- a/controllers/ocirepository_controller_test.go +++ b/controllers/ocirepository_controller_test.go @@ -335,7 +335,7 @@ func TestOCIRepository_Reconcile_MediaType(t *testing.T) { return false } readyCondition := conditions.Get(obj, meta.ReadyCondition) - return readyCondition != nil + return readyCondition != nil && !conditions.IsUnknown(obj, meta.ReadyCondition) }, timeout).Should(BeTrue()) g.Expect(conditions.IsReady(obj)).To(BeIdenticalTo(!tt.wantErr)) @@ -383,8 +383,8 @@ func TestOCIRepository_reconcileSource_authStrategy(t *testing.T) { name: "HTTP without basic auth", want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new revision '' for ''"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new revision '' for ''"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), }, }, { @@ -404,8 +404,8 @@ func TestOCIRepository_reconcileSource_authStrategy(t *testing.T) { includeSecret: true, }, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new revision '' for ''"), - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new revision '' for ''"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), }, }, { @@ -425,8 +425,8 @@ func TestOCIRepository_reconcileSource_authStrategy(t *testing.T) { includeSA: true, }, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new revision '' for ''"), - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new revision '' for ''"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), }, }, { @@ -508,8 +508,8 @@ func TestOCIRepository_reconcileSource_authStrategy(t *testing.T) { }, }, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new revision '' for ''"), - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new revision '' for ''"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), }, }, { @@ -580,8 +580,8 @@ func TestOCIRepository_reconcileSource_authStrategy(t *testing.T) { }, provider: "azure", assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new revision '' for ''"), - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new revision '' for ''"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), }, }, } @@ -595,6 +595,7 @@ func TestOCIRepository_reconcileSource_authStrategy(t *testing.T) { obj := &sourcev1.OCIRepository{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "auth-strategy-", + Generation: 1, }, Spec: sourcev1.OCIRepositorySpec{ Interval: metav1.Duration{Duration: interval}, @@ -667,6 +668,7 @@ func TestOCIRepository_reconcileSource_authStrategy(t *testing.T) { Client: builder.Build(), EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, + patchOptions: getPatchOptions(ociRepositoryReadyCondition.Owned, "sc"), } opts := craneOptions(ctx, true) @@ -680,8 +682,15 @@ func TestOCIRepository_reconcileSource_authStrategy(t *testing.T) { assertConditions[k].Message = strings.ReplaceAll(assertConditions[k].Message, "", repoURL) } + g.Expect(r.Client.Create(ctx, obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(ctx, obj)).ToNot(HaveOccurred()) + }() + + sp := patch.NewSerialPatcher(obj, r.Client) + tmpDir := t.TempDir() - got, err := r.reconcileSource(ctx, obj, &sourcev1.Artifact{}, tmpDir) + got, err := r.reconcileSource(ctx, sp, obj, &sourcev1.Artifact{}, tmpDir) if tt.wantErr { g.Expect(err).ToNot(BeNil()) } else { @@ -778,6 +787,7 @@ func TestOCIRepository_CertSecret(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ GenerateName: "ocirepository-test-resource", Namespace: ns.Name, + Generation: 1, }, Spec: sourcev1.OCIRepositorySpec{ URL: tt.url, @@ -818,7 +828,7 @@ func TestOCIRepository_CertSecret(t *testing.T) { return false } readyCondition := conditions.Get(&resultobj, meta.ReadyCondition) - if readyCondition == nil { + if readyCondition == nil || conditions.IsUnknown(&resultobj, meta.ReadyCondition) { return false } return obj.Generation == readyCondition.ObservedGeneration && @@ -866,8 +876,8 @@ func TestOCIRepository_reconcileSource_remoteReference(t *testing.T) { want: sreconcile.ResultSuccess, wantRevision: fmt.Sprintf("latest/%s", img6.digest.Hex), assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new revision"), - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new revision"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision"), }, }, { @@ -878,8 +888,8 @@ func TestOCIRepository_reconcileSource_remoteReference(t *testing.T) { want: sreconcile.ResultSuccess, wantRevision: fmt.Sprintf("%s/%s", img6.tag, img6.digest.Hex), assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new revision"), - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new revision"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision"), }, }, { @@ -890,8 +900,8 @@ func TestOCIRepository_reconcileSource_remoteReference(t *testing.T) { want: sreconcile.ResultSuccess, wantRevision: fmt.Sprintf("%s/%s", img6.tag, img6.digest.Hex), assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new revision"), - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new revision"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision"), }, }, { @@ -902,8 +912,8 @@ func TestOCIRepository_reconcileSource_remoteReference(t *testing.T) { wantRevision: img6.digest.Hex, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new revision"), - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new revision"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision"), }, }, { @@ -948,8 +958,8 @@ func TestOCIRepository_reconcileSource_remoteReference(t *testing.T) { want: sreconcile.ResultSuccess, wantRevision: fmt.Sprintf("%s/%s", img6.tag, img6.digest.Hex), assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new revision"), - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new revision"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision"), }, }, { @@ -962,8 +972,8 @@ func TestOCIRepository_reconcileSource_remoteReference(t *testing.T) { want: sreconcile.ResultSuccess, wantRevision: img5.digest.Hex, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new revision"), - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new revision"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision"), }, }, } @@ -974,6 +984,7 @@ func TestOCIRepository_reconcileSource_remoteReference(t *testing.T) { Client: builder.Build(), EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, + patchOptions: getPatchOptions(ociRepositoryReadyCondition.Owned, "sc"), } for _, tt := range tests { @@ -981,6 +992,7 @@ func TestOCIRepository_reconcileSource_remoteReference(t *testing.T) { obj := &sourcev1.OCIRepository{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "checkout-strategy-", + Generation: 1, }, Spec: sourcev1.OCIRepositorySpec{ URL: fmt.Sprintf("oci://%s/podinfo", server.registryHost), @@ -993,9 +1005,16 @@ func TestOCIRepository_reconcileSource_remoteReference(t *testing.T) { obj.Spec.Reference = tt.reference } + g.Expect(r.Client.Create(ctx, obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(ctx, obj)).ToNot(HaveOccurred()) + }() + + sp := patch.NewSerialPatcher(obj, r.Client) + artifact := &sourcev1.Artifact{} tmpDir := t.TempDir() - got, err := r.reconcileSource(ctx, obj, artifact, tmpDir) + got, err := r.reconcileSource(ctx, sp, obj, artifact, tmpDir) if tt.wantErr { g.Expect(err).To(HaveOccurred()) } else { @@ -1043,8 +1062,8 @@ func TestOCIRepository_reconcileSource_verifyOCISourceSignature(t *testing.T) { shouldSign: true, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new revision '' for ''"), - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new revision '' for ''"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), *conditions.TrueCondition(sourcev1.SourceVerifiedCondition, meta.SucceededReason, "verified signature of revision "), }, }, @@ -1058,8 +1077,8 @@ func TestOCIRepository_reconcileSource_verifyOCISourceSignature(t *testing.T) { wantErrMsg: "failed to verify the signature using provider 'cosign': no matching signatures were found for ''", want: sreconcile.ResultEmpty, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new revision '' for ''"), - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new revision '' for ''"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), *conditions.FalseCondition(sourcev1.SourceVerifiedCondition, sourcev1.VerificationError, "failed to verify the signature using provider '': no matching signatures were found for ''"), }, }, @@ -1073,8 +1092,8 @@ func TestOCIRepository_reconcileSource_verifyOCISourceSignature(t *testing.T) { want: sreconcile.ResultEmpty, keyless: true, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new revision '' for ''"), - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new revision '' for ''"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), *conditions.FalseCondition(sourcev1.SourceVerifiedCondition, sourcev1.VerificationError, "failed to verify the signature using provider ' keyless': no matching signatures"), }, }, @@ -1132,8 +1151,8 @@ func TestOCIRepository_reconcileSource_verifyOCISourceSignature(t *testing.T) { wantErr: true, want: sreconcile.ResultEmpty, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new revision '' for ''"), - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new revision '' for ''"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), *conditions.FalseCondition(sourcev1.SourceVerifiedCondition, sourcev1.VerificationError, "cosign does not support insecure registries"), }, }, @@ -1145,6 +1164,7 @@ func TestOCIRepository_reconcileSource_verifyOCISourceSignature(t *testing.T) { Client: builder.Build(), EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, + patchOptions: getPatchOptions(ociRepositoryReadyCondition.Owned, "sc"), } pf := func(b bool) ([]byte, error) { @@ -1175,6 +1195,7 @@ func TestOCIRepository_reconcileSource_verifyOCISourceSignature(t *testing.T) { obj := &sourcev1.OCIRepository{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "verify-oci-source-signature-", + Generation: 1, }, Spec: sourcev1.OCIRepositorySpec{ URL: fmt.Sprintf("oci://%s/podinfo", server.registryHost), @@ -1236,8 +1257,15 @@ func TestOCIRepository_reconcileSource_verifyOCISourceSignature(t *testing.T) { tt.beforeFunc(obj) } + g.Expect(r.Client.Create(ctx, obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(ctx, obj)).ToNot(HaveOccurred()) + }() + + sp := patch.NewSerialPatcher(obj, r.Client) + artifact := &sourcev1.Artifact{} - got, err := r.reconcileSource(ctx, obj, artifact, tmpDir) + got, err := r.reconcileSource(ctx, sp, obj, artifact, tmpDir) if tt.wantErr { tt.wantErrMsg = strings.ReplaceAll(tt.wantErrMsg, "", artifactURL) g.Expect(err).ToNot(BeNil()) @@ -1373,6 +1401,7 @@ func TestOCIRepository_reconcileSource_noop(t *testing.T) { Client: builder.Build(), EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, + patchOptions: getPatchOptions(ociRepositoryReadyCondition.Owned, "sc"), } for _, tt := range tests { @@ -1382,6 +1411,7 @@ func TestOCIRepository_reconcileSource_noop(t *testing.T) { obj := &sourcev1.OCIRepository{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "noop-", + Generation: 1, }, Spec: sourcev1.OCIRepositorySpec{ URL: fmt.Sprintf("oci://%s/podinfo", server.registryHost), @@ -1395,9 +1425,16 @@ func TestOCIRepository_reconcileSource_noop(t *testing.T) { tt.beforeFunc(obj) } + g.Expect(r.Client.Create(ctx, obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(ctx, obj)).ToNot(HaveOccurred()) + }() + + sp := patch.NewSerialPatcher(obj, r.Client) + artifact := &sourcev1.Artifact{} tmpDir := t.TempDir() - got, err := r.reconcileSource(ctx, obj, artifact, tmpDir) + got, err := r.reconcileSource(ctx, sp, obj, artifact, tmpDir) g.Expect(err).ToNot(HaveOccurred()) g.Expect(got).To(Equal(sreconcile.ResultSuccess)) @@ -1593,6 +1630,7 @@ func TestOCIRepository_reconcileArtifact(t *testing.T) { Client: builder.Build(), EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, + patchOptions: getPatchOptions(ociRepositoryReadyCondition.Owned, "sc"), } for _, tt := range tests { @@ -1602,6 +1640,7 @@ func TestOCIRepository_reconcileArtifact(t *testing.T) { obj := &sourcev1.OCIRepository{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "reconcile-artifact-", + Generation: 1, }, } if tt.beforeFunc != nil { @@ -1612,7 +1651,15 @@ func TestOCIRepository_reconcileArtifact(t *testing.T) { if tt.artifact != nil { artifact = tt.artifact } - got, err := r.reconcileArtifact(ctx, obj, artifact, tt.targetPath) + + g.Expect(r.Client.Create(ctx, obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(ctx, obj)).ToNot(HaveOccurred()) + }() + + sp := patch.NewSerialPatcher(obj, r.Client) + + got, err := r.reconcileArtifact(ctx, sp, obj, artifact, tt.targetPath) if tt.wantErr { g.Expect(err).To(HaveOccurred()) } else { @@ -1698,6 +1745,7 @@ func TestOCIRepository_getArtifactURL(t *testing.T) { Client: builder.Build(), EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, + patchOptions: getPatchOptions(ociRepositoryReadyCondition.Owned, "sc"), } for _, tt := range tests { @@ -1763,7 +1811,7 @@ func TestOCIRepository_stalled(t *testing.T) { return false } return obj.Generation == readyCondition.ObservedGeneration && - !conditions.IsReady(&resultobj) + !conditions.IsUnknown(&resultobj, meta.ReadyCondition) }, timeout).Should(BeTrue()) // Verify that stalled condition is present in status @@ -1809,6 +1857,7 @@ func TestOCIRepository_reconcileStorage(t *testing.T) { } testStorage.SetArtifactURL(obj.Status.Artifact) + conditions.MarkTrue(obj, meta.ReadyCondition, "foo", "bar") return nil }, assertArtifact: &sourcev1.Artifact{ @@ -1825,6 +1874,17 @@ func TestOCIRepository_reconcileStorage(t *testing.T) { "!/oci-reconcile-storage/a.txt", }, want: sreconcile.ResultSuccess, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReadyCondition, "foo", "bar"), + }, + }, + { + name: "build artifact first time", + want: sreconcile.ResultSuccess, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact"), + }, }, { name: "notices missing artifact in storage", @@ -1841,7 +1901,8 @@ func TestOCIRepository_reconcileStorage(t *testing.T) { "!/oci-reconcile-storage/invalid.txt", }, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, "NoArtifact", "no artifact for resource in storage"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), }, }, { @@ -1859,6 +1920,7 @@ func TestOCIRepository_reconcileStorage(t *testing.T) { if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader("file"), 0o640); err != nil { return err } + conditions.MarkTrue(obj, meta.ReadyCondition, "foo", "bar") return nil }, want: sreconcile.ResultSuccess, @@ -1872,6 +1934,9 @@ func TestOCIRepository_reconcileStorage(t *testing.T) { URL: testStorage.Hostname + "/oci-reconcile-storage/hostname.txt", Size: int64p(int64(len("file"))), }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReadyCondition, "foo", "bar"), + }, }, } @@ -1880,6 +1945,7 @@ func TestOCIRepository_reconcileStorage(t *testing.T) { Client: builder.Build(), EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, + patchOptions: getPatchOptions(ociRepositoryReadyCondition.Owned, "sc"), } for _, tt := range tests { @@ -1888,11 +1954,22 @@ func TestOCIRepository_reconcileStorage(t *testing.T) { obj := &sourcev1.OCIRepository{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "test-", + Generation: 1, }, } - g.Expect(tt.beforeFunc(obj)).To(Succeed()) - got, err := r.reconcileStorage(ctx, obj, &sourcev1.Artifact{}, "") + if tt.beforeFunc != nil { + g.Expect(tt.beforeFunc(obj)).To(Succeed()) + } + + g.Expect(r.Client.Create(ctx, obj)).ToNot(HaveOccurred()) + defer func() { + g.Expect(r.Client.Delete(ctx, obj)).ToNot(HaveOccurred()) + }() + + sp := patch.NewSerialPatcher(obj, r.Client) + + got, err := r.reconcileStorage(ctx, sp, obj, &sourcev1.Artifact{}, "") if tt.wantErr { g.Expect(err).To(HaveOccurred()) } else { @@ -1916,6 +1993,10 @@ func TestOCIRepository_reconcileStorage(t *testing.T) { g.Expect(absoluteP).ToNot(BeAnExistingFile()) } + + // In-progress status condition validity. + checker := conditionscheck.NewInProgressChecker(r.Client) + checker.CheckErr(ctx, obj) }) } } @@ -1926,6 +2007,7 @@ func TestOCIRepository_ReconcileDelete(t *testing.T) { r := &OCIRepositoryReconciler{ EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, + patchOptions: getPatchOptions(ociRepositoryReadyCondition.Owned, "sc"), } obj := &sourcev1.OCIRepository{ @@ -2058,6 +2140,7 @@ func TestOCIRepositoryReconciler_notify(t *testing.T) { reconciler := &OCIRepositoryReconciler{ EventRecorder: recorder, + patchOptions: getPatchOptions(ociRepositoryReadyCondition.Owned, "sc"), } reconciler.notify(ctx, oldObj, newObj, tt.res, tt.resErr)