Use Artifact.Path for HelmRepository index cache

Resolving it to a local path does not make it more unique, while resulting in longer keys and a lot of safejoin calls. Signed-off-by: Hidde Beydals <[email protected]>
fluxcd · Feb 10, 2023 · e40c7f6 · e40c7f6
1 parent 8252ce1
commit e40c7f6
Show file tree

Hide file tree

Showing 4 changed files with 16 additions and 19 deletions.
diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go
@@ -660,16 +660,16 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 
 		// Attempt to load the index from the cache.
 		if r.Cache != nil {
-			if index, ok := r.Cache.Get(httpChartRepo.Path); ok {
+			if index, ok := r.Cache.Get(repo.GetArtifact().Path); ok {
 				r.IncCacheEvents(cache.CacheEventTypeHit, repo.Name, repo.Namespace)
-				r.Cache.SetExpiration(httpChartRepo.Path, r.TTL)
+				r.Cache.SetExpiration(repo.GetArtifact().Path, r.TTL)
 				httpChartRepo.Index = index.(*helmrepo.IndexFile)
 			} else {
 				r.IncCacheEvents(cache.CacheEventTypeMiss, repo.Name, repo.Namespace)
 				defer func() {
 					// If we succeed in loading the index, cache it.
 					if httpChartRepo.Index != nil {
-						if err = r.Cache.Set(httpChartRepo.Path, httpChartRepo.Index, r.TTL); err != nil {
+						if err = r.Cache.Set(repo.GetArtifact().Path, httpChartRepo.Index, r.TTL); err != nil {
 							r.eventLogf(ctx, obj, eventv1.EventTypeTrace, sourcev1.CacheOperationFailedReason, "failed to cache index: %s", err)
 						}
 					}
@@ -1125,21 +1125,21 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont
 				return nil, err
 			}
 
-			if obj.Status.Artifact != nil {
+			if artifact := obj.GetArtifact(); artifact != nil {
+				httpChartRepo.Path = r.Storage.LocalPath(*artifact)
+
 				// Attempt to load the index from the cache.
-				httpChartRepo.Path = r.Storage.LocalPath(*obj.GetArtifact())
 				if r.Cache != nil {
-					if index, ok := r.Cache.Get(httpChartRepo.Path); ok {
+					if index, ok := r.Cache.Get(artifact.Path); ok {
 						r.IncCacheEvents(cache.CacheEventTypeHit, name, namespace)
-						r.Cache.SetExpiration(httpChartRepo.Path, r.TTL)
-
+						r.Cache.SetExpiration(artifact.Path, r.TTL)
 						httpChartRepo.Index = index.(*helmrepo.IndexFile)
 					} else {
 						r.IncCacheEvents(cache.CacheEventTypeMiss, name, namespace)
 						if err := httpChartRepo.LoadFromPath(); err != nil {
 							return nil, err
 						}
-						r.Cache.Set(httpChartRepo.Path, httpChartRepo.Index, r.TTL)
+						r.Cache.Set(artifact.Path, httpChartRepo.Index, r.TTL)
 					}
 				}
 			}

diff --git a/controllers/helmchart_controller_test.go b/controllers/helmchart_controller_test.go
@@ -137,8 +137,7 @@ func TestHelmChartReconciler_Reconcile(t *testing.T) {
 				repoKey := client.ObjectKey{Name: repository.Name, Namespace: repository.Namespace}
 				err = testEnv.Get(ctx, repoKey, repository)
 				g.Expect(err).ToNot(HaveOccurred())
-				localPath := testStorage.LocalPath(*repository.GetArtifact())
-				_, found := testCache.Get(localPath)
+				_, found := testCache.Get(repository.GetArtifact().Path)
 				g.Expect(found).To(BeTrue())
 
 				g.Expect(testEnv.Delete(ctx, obj)).To(Succeed())

diff --git a/controllers/helmrepository_controller.go b/controllers/helmrepository_controller.go
@@ -563,7 +563,7 @@ func (r *HelmRepositoryReconciler) reconcileArtifact(ctx context.Context, sp *pa
 	if obj.GetArtifact().HasRevision(artifact.Revision) && obj.GetArtifact().HasChecksum(artifact.Checksum) {
 		// Extend TTL of the Index in the cache (if present).
 		if r.Cache != nil {
-			r.Cache.SetExpiration(r.Storage.LocalPath(*artifact), r.TTL)
+			r.Cache.SetExpiration(artifact.Path, r.TTL)
 		}
 
 		r.eventLogf(ctx, obj, eventv1.EventTypeTrace, sourcev1.ArtifactUpToDateReason, "artifact up-to-date with remote revision: '%s'", artifact.Revision)
@@ -607,10 +607,9 @@ func (r *HelmRepositoryReconciler) reconcileArtifact(ctx context.Context, sp *pa
 	if r.Cache != nil && chartRepo.Index != nil {
 		// The cache keys have to be safe in multi-tenancy environments, as
 		// otherwise it could be used as a vector to bypass the repository's
-		// authentication. Using r.Storage.LocalPath(*repo.GetArtifact())
-		// is safe as the path is in the format of:
-		// /<repository-name>/<chart-name>/<filename>.
-		if err := r.Cache.Set(r.Storage.LocalPath(*artifact), chartRepo.Index, r.TTL); err != nil {
+		// authentication. Using the Artifact.Path is safe as the path is in
+		// the format of: /<repository-name>/<chart-name>/<filename>.
+		if err := r.Cache.Set(artifact.Path, chartRepo.Index, r.TTL); err != nil {
 			r.eventLogf(ctx, obj, eventv1.EventTypeTrace, sourcev1.CacheOperationFailedReason, "failed to cache index: %s", err)
 		}
 	}

diff --git a/controllers/helmrepository_controller_test.go b/controllers/helmrepository_controller_test.go
@@ -848,7 +848,7 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) {
 			},
 			want: sreconcile.ResultSuccess,
 			afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, cache *cache.Cache) {
-				i, ok := cache.Get(testStorage.LocalPath(*obj.GetArtifact()))
+				i, ok := cache.Get(obj.GetArtifact().Path)
 				t.Expect(ok).To(BeTrue())
 				t.Expect(i).To(BeAssignableToTypeOf(&repo.IndexFile{}))
 			},
@@ -1581,7 +1581,6 @@ func TestHelmRepositoryReconciler_InMemoryCaching(t *testing.T) {
 
 	err = testEnv.Get(ctx, key, helmRepo)
 	g.Expect(err).ToNot(HaveOccurred())
-	localPath := testStorage.LocalPath(*helmRepo.GetArtifact())
-	_, cacheHit := testCache.Get(localPath)
+	_, cacheHit := testCache.Get(helmRepo.GetArtifact().Path)
 	g.Expect(cacheHit).To(BeTrue())
 }