Merge pull request #1035 from fluxcd/helm-index-digest-rev

helm: only use Digest to calculcate index revision

Author: hiddeco

controllers/helmrepository_controller.go

@@ -318,8 +318,8 @@ func (r *HelmRepositoryReconciler) notify(ctx context.Context, oldObj, newObj *s
 			if sreconcile.FailureRecovery(oldObj, newObj, helmRepositoryFailConditions) {
 				r.AnnotatedEventf(newObj, annotations, corev1.EventTypeNormal,
 					meta.SucceededReason, message)
+				ctrl.LoggerFrom(ctx).Info(message)
 			}
-			ctrl.LoggerFrom(ctx).Info(message)
 		}
 	}
 }
@@ -475,8 +475,7 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patc
 		}
 		if curDig.Validate() == nil {
 			// Short-circuit based on the fetched index being an exact match to the
-			// stored Artifact. This prevents having to unmarshal the YAML to calculate
-			// the (stable) revision, which is a memory expensive operation.
+			// stored Artifact.
 			if newDig := chartRepo.Digest(curDig.Algorithm()); newDig.Validate() == nil && (newDig == curDig) {
 				*artifact = *curArtifact
 				conditions.Delete(obj, sourcev1.FetchFailedCondition)
@@ -501,11 +500,11 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patc
 	var changed bool
 	if artifact := obj.Status.Artifact; artifact != nil {
 		curRev := digest.Digest(sourcev1.TransformLegacyRevision(artifact.Revision))
-		changed = curRev.Validate() != nil || curRev != chartRepo.Revision(curRev.Algorithm())
+		changed = curRev.Validate() != nil || curRev != chartRepo.Digest(curRev.Algorithm())
 	}
 
 	// Calculate revision.
-	revision := chartRepo.Revision(intdigest.Canonical)
+	revision := chartRepo.Digest(intdigest.Canonical)
 	if revision.Validate() != nil {
 		e := &serror.Event{
 			Err:    fmt.Errorf("failed to calculate revision: %w", err),

controllers/helmrepository_controller_test.go

@@ -775,7 +775,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) {
 				checksum = newChartRepo.Digest(intdigest.Canonical)
 
 				g.Expect(newChartRepo.LoadFromPath()).To(Succeed())
-				revision = newChartRepo.Revision(intdigest.Canonical)
+				revision = newChartRepo.Digest(intdigest.Canonical)
 			}
 			if tt.beforeFunc != nil {
 				tt.beforeFunc(g, obj, revision, checksum)

internal/helm/repository/chart_repository.go

@@ -122,9 +122,8 @@ type ChartRepository struct {
 
 	tlsConfig *tls.Config
 
-	cached    bool
-	revisions map[digest.Algorithm]digest.Digest
-	digests   map[digest.Algorithm]digest.Digest
+	cached  bool
+	digests map[digest.Algorithm]digest.Digest
 
 	*sync.RWMutex
 }
@@ -155,9 +154,8 @@ func NewChartRepository(URL, path string, providers getter.Providers, tlsConfig
 
 func newChartRepository() *ChartRepository {
 	return &ChartRepository{
-		revisions: make(map[digest.Algorithm]digest.Digest, 0),
-		digests:   make(map[digest.Algorithm]digest.Digest, 0),
-		RWMutex:   &sync.RWMutex{},
+		digests: make(map[digest.Algorithm]digest.Digest, 0),
+		RWMutex: &sync.RWMutex{},
 	}
 }
 
@@ -351,7 +349,6 @@ func (r *ChartRepository) LoadFromPath() error {
 	}
 
 	r.Index = i
-	r.revisions = make(map[digest.Algorithm]digest.Digest, 0)
 	return nil
 }
 
@@ -384,24 +381,6 @@ func (r *ChartRepository) DownloadIndex(w io.Writer) (err error) {
 	return nil
 }
 
-// Revision returns the revision of the ChartRepository's Index. It assumes
-// the Index is stable sorted.
-func (r *ChartRepository) Revision(algorithm digest.Algorithm) digest.Digest {
-	if !r.HasIndex() {
-		return ""
-	}
-
-	r.Lock()
-	defer r.Unlock()
-
-	if _, ok := r.revisions[algorithm]; !ok {
-		if b, _ := yaml.Marshal(r.Index); len(b) > 0 {
-			r.revisions[algorithm] = algorithm.FromBytes(b)
-		}
-	}
-	return r.revisions[algorithm]
-}
-
 // Digest returns the digest of the file at the ChartRepository's Path.
 func (r *ChartRepository) Digest(algorithm digest.Algorithm) digest.Digest {
 	if !r.HasFile() {
@@ -463,7 +442,7 @@ func (r *ChartRepository) Clear() error {
 	return nil
 }
 
-// Invalidate clears any cached digests and revisions.
+// Invalidate clears any cached digests.
 func (r *ChartRepository) Invalidate() {
 	r.Lock()
 	defer r.Unlock()
@@ -473,7 +452,6 @@ func (r *ChartRepository) Invalidate() {
 
 func (r *ChartRepository) invalidate() {
 	r.digests = make(map[digest.Algorithm]digest.Digest, 0)
-	r.revisions = make(map[digest.Algorithm]digest.Digest, 0)
 }
 
 // VerifyChart verifies the chart against a signature.

internal/helm/repository/chart_repository_test.go

@@ -392,7 +392,6 @@ func TestChartRepository_CacheIndex(t *testing.T) {
 	r := newChartRepository()
 	r.URL = "https://example.com"
 	r.Client = &mg
-	r.revisions["key"] = "value"
 	r.digests["key"] = "value"
 
 	err := r.CacheIndex()
@@ -405,7 +404,6 @@ func TestChartRepository_CacheIndex(t *testing.T) {
 	b, _ := os.ReadFile(r.Path)
 	g.Expect(b).To(Equal(mg.Response))
 
-	g.Expect(r.revisions).To(BeEmpty())
 	g.Expect(r.digests).To(BeEmpty())
 }
 
@@ -480,11 +478,9 @@ func TestChartRepository_LoadFromPath(t *testing.T) {
 
 		r := newChartRepository()
 		r.Path = i
-		r.revisions["key"] = "value"
 
 		g.Expect(r.LoadFromPath()).To(Succeed())
 		g.Expect(r.Index).ToNot(BeNil())
-		g.Expect(r.revisions).To(BeEmpty())
 	})
 
 	t.Run("no cache path", func(t *testing.T) {
@@ -507,44 +503,6 @@ func TestChartRepository_LoadFromPath(t *testing.T) {
 	})
 }
 
-func TestChartRepository_Revision(t *testing.T) {
-	t.Run("with algorithm", func(t *testing.T) {
-		r := newChartRepository()
-		r.Index = repo.NewIndexFile()
-
-		for _, algo := range []digest.Algorithm{digest.SHA256, digest.SHA512} {
-			t.Run(algo.String(), func(t *testing.T) {
-				g := NewWithT(t)
-
-				d := r.Revision(algo)
-				g.Expect(d).ToNot(BeEmpty())
-				g.Expect(d.Algorithm()).To(Equal(algo))
-				g.Expect(r.revisions[algo]).To(Equal(d))
-			})
-		}
-	})
-
-	t.Run("without index", func(t *testing.T) {
-		g := NewWithT(t)
-
-		r := newChartRepository()
-		g.Expect(r.Revision(digest.SHA256)).To(BeEmpty())
-	})
-
-	t.Run("from cache", func(t *testing.T) {
-		g := NewWithT(t)
-
-		algo := digest.SHA256
-		expect := digest.Digest("sha256:fake")
-
-		r := newChartRepository()
-		r.Index = repo.NewIndexFile()
-		r.revisions[algo] = expect
-
-		g.Expect(r.Revision(algo)).To(Equal(expect))
-	})
-}
-
 func TestChartRepository_Digest(t *testing.T) {
 	t.Run("with algorithm", func(t *testing.T) {
 		g := NewWithT(t)
@@ -625,11 +583,9 @@ func TestChartRepository_Clear(t *testing.T) {
 
 		r := newChartRepository()
 		r.Index = repo.NewIndexFile()
-		r.revisions["key"] = "value"
 
 		g.Expect(r.Clear()).To(Succeed())
 		g.Expect(r.Index).To(BeNil())
-		g.Expect(r.revisions).To(BeEmpty())
 	})
 
 	t.Run("with index and cached path", func(t *testing.T) {
@@ -643,14 +599,12 @@ func TestChartRepository_Clear(t *testing.T) {
 		r.Path = f.Name()
 		r.Index = repo.NewIndexFile()
 		r.digests["key"] = "value"
-		r.revisions["key"] = "value"
 		r.cached = true
 
 		g.Expect(r.Clear()).To(Succeed())
 		g.Expect(r.Index).To(BeNil())
 		g.Expect(r.Path).To(BeEmpty())
 		g.Expect(r.digests).To(BeEmpty())
-		g.Expect(r.revisions).To(BeEmpty())
 		g.Expect(r.cached).To(BeFalse())
 	})
 
@@ -677,11 +631,9 @@ func TestChartRepository_Invalidate(t *testing.T) {
 
 	r := newChartRepository()
 	r.digests["key"] = "value"
-	r.revisions["key"] = "value"
 
 	r.Invalidate()
 	g.Expect(r.digests).To(BeEmpty())
-	g.Expect(r.revisions).To(BeEmpty())
 }
 
 func verifyLocalIndex(t *testing.T, i *repo.IndexFile) {