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 <hello@hidde.co>

controllers/helmchart_controller.go

@@ -667,16 +667,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)
  }
  }
@@ -1123,21 +1123,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)
  }
  }
  }

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())

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)
  }
  }

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())
 }