From e40c7f6de49c923003c602ab6be4e8e5e803bef0 Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Fri, 10 Feb 2023 10:52:08 +0100 Subject: [PATCH] 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 --- controllers/helmchart_controller.go | 18 +++++++++--------- controllers/helmchart_controller_test.go | 3 +-- controllers/helmrepository_controller.go | 9 ++++----- controllers/helmrepository_controller_test.go | 5 ++--- 4 files changed, 16 insertions(+), 19 deletions(-) diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index 5593a5f17..9db51948e 100644 --- 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 index a7460e8c7..1a20bf4b5 100644 --- 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 index 6e1c599b1..328c908ff 100644 --- 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: - // ///. - 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: ///. + 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 index b205f35c2..4952effdd 100644 --- 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()) }