diff --git a/internal/controller/helmrepository_controller.go b/internal/controller/helmrepository_controller.go index d5175fdf1..0b630344b 100644 --- a/internal/controller/helmrepository_controller.go +++ b/internal/controller/helmrepository_controller.go @@ -17,8 +17,10 @@ limitations under the License. package controller import ( + "bytes" "context" "crypto/tls" + "encoding/json" "errors" "fmt" "net/url" @@ -476,11 +478,11 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patc // Early comparison to current Artifact. if curArtifact := obj.GetArtifact(); curArtifact != nil { - curDig := digest.Digest(curArtifact.Digest) - if curDig.Validate() == nil { + curRev := digest.Digest(curArtifact.Revision) + if curRev.Validate() == nil { // Short-circuit based on the fetched index being an exact match to the // stored Artifact. - if newDig := chartRepo.Digest(curDig.Algorithm()); newDig.Validate() == nil && (newDig == curDig) { + if newRev := chartRepo.Digest(curRev.Algorithm()); newRev.Validate() == nil && (newRev == curRev) { *artifact = *curArtifact conditions.Delete(obj, sourcev1.FetchFailedCondition) return sreconcile.ResultSuccess, nil @@ -500,13 +502,6 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patc // Delete any stale failure observation conditions.Delete(obj, sourcev1.FetchFailedCondition) - // Check if index has changed compared to current Artifact revision. - var changed bool - if artifact := obj.Status.Artifact; artifact != nil { - curRev := digest.Digest(artifact.Revision) - changed = curRev.Validate() != nil || curRev != chartRepo.Digest(curRev.Algorithm()) - } - // Calculate revision. revision := chartRepo.Digest(intdigest.Canonical) if revision.Validate() != nil { @@ -519,16 +514,14 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patc } // Mark observations about the revision on the object. - if obj.Status.Artifact == nil || changed { - message := fmt.Sprintf("new index revision '%s'", revision) - 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 sreconcile.ResultEmpty, err - } + message := fmt.Sprintf("new index revision '%s'", revision) + 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 sreconcile.ResultEmpty, err } // Create potential new artifact. @@ -593,8 +586,18 @@ func (r *HelmRepositoryReconciler) reconcileArtifact(ctx context.Context, sp *pa } defer unlock() - // Save artifact to storage. - if err = r.Storage.CopyFromPath(artifact, chartRepo.Path); err != nil { + // Save artifact to storage in JSON format. + b := &bytes.Buffer{} + if err := json.NewEncoder(b).Encode(chartRepo.Index); err != nil { + e := &serror.Event{ + Err: fmt.Errorf("unable to encode index to JSON: %w", err), + Reason: sourcev1.ArchiveOperationFailedReason, + } + conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) + return sreconcile.ResultEmpty, e + } + + if err = r.Storage.Copy(artifact, b); err != nil { e := &serror.Event{ Err: fmt.Errorf("unable to save artifact to storage: %w", err), Reason: sourcev1.ArchiveOperationFailedReason, diff --git a/internal/controller/helmrepository_controller_test.go b/internal/controller/helmrepository_controller_test.go index d6f56920c..359df6434 100644 --- a/internal/controller/helmrepository_controller_test.go +++ b/internal/controller/helmrepository_controller_test.go @@ -19,6 +19,7 @@ package controller import ( "context" "crypto/tls" + "encoding/json" "errors" "fmt" "net/http" @@ -633,12 +634,12 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { want: sreconcile.ResultSuccess, }, { - name: "Stored index with different digest and same revision", + name: "Stored index with different revision and same digest", protocol: "http", beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev, dig digest.Digest) { obj.Status.Artifact = &sourcev1.Artifact{ - Revision: rev.String(), - Digest: "sha256:80bb3dd67c63095d985850459834ea727603727a370079de90d221191d375a86", + Revision: "sha256:80bb3dd67c63095d985850459834ea727603727a370079de90d221191d375a86", + Digest: dig.String(), } conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") @@ -646,15 +647,16 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, "foo", "bar") }, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), - *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), + *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new index revision"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new index revision"), }, afterFunc: func(t *WithT, obj *helmv1.HelmRepository, artifact sourcev1.Artifact, chartRepo *repository.ChartRepository) { t.Expect(chartRepo.Path).ToNot(BeEmpty()) t.Expect(chartRepo.Index).ToNot(BeNil()) - t.Expect(artifact.Revision).To(Equal(obj.Status.Artifact.Revision)) - t.Expect(artifact.Digest).ToNot(Equal(obj.Status.Artifact.Digest)) + t.Expect(artifact.Revision).NotTo(Equal(obj.Status.Artifact.Revision)) + t.Expect(artifact.Digest).To(BeEmpty()) }, want: sreconcile.ResultSuccess, }, @@ -802,10 +804,12 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { var rev, dig digest.Digest if validSecret { g.Expect(newChartRepo.CacheIndex()).To(Succeed()) - dig = newChartRepo.Digest(intdigest.Canonical) + rev = newChartRepo.Digest(intdigest.Canonical) g.Expect(newChartRepo.LoadFromPath()).To(Succeed()) - rev = newChartRepo.Digest(intdigest.Canonical) + d := intdigest.Canonical.Digester() + g.Expect(json.NewEncoder(d.Hash()).Encode(newChartRepo.Index)).To(Succeed()) + dig = d.Digest() } if tt.beforeFunc != nil { tt.beforeFunc(g, obj, rev, dig) @@ -857,11 +861,17 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) { assertConditions []metav1.Condition }{ { - name: "Archiving artifact to storage makes ArtifactInStorage=True", + name: "Archiving artifact to storage makes ArtifactInStorage=True and artifact is stored as JSON", beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, artifact sourcev1.Artifact, index *repository.ChartRepository) { obj.Spec.Interval = metav1.Duration{Duration: interval} }, want: sreconcile.ResultSuccess, + afterFunc: func(t *WithT, obj *helmv1.HelmRepository, cache *cache.Cache) { + localPath := testStorage.LocalPath(*obj.GetArtifact()) + b, err := os.ReadFile(localPath) + t.Expect(err).To(Not(HaveOccurred())) + t.Expect(json.Valid(b)).To(BeTrue()) + }, assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact: revision 'existing'"), },