diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index 73060f44e..227c5a072 100644 --- a/controllers/helmchart_controller.go +++ b/controllers/helmchart_controller.go @@ -21,8 +21,7 @@ import ( "crypto/tls" "errors" "fmt" - "github.com/fluxcd/pkg/git" - "github.com/opencontainers/go-digest" + "net/url" "os" "path/filepath" @@ -30,12 +29,12 @@ import ( "strings" "time" - eventv1 "github.com/fluxcd/pkg/apis/event/v1beta1" - soci "github.com/fluxcd/source-controller/internal/oci" "github.com/google/go-containerregistry/pkg/authn" "github.com/google/go-containerregistry/pkg/v1/remote" + "github.com/opencontainers/go-digest" helmgetter "helm.sh/helm/v3/pkg/getter" helmreg "helm.sh/helm/v3/pkg/registry" + helmrepo "helm.sh/helm/v3/pkg/repo" corev1 "k8s.io/api/core/v1" apierrs "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -54,7 +53,9 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/controller-runtime/pkg/source" + eventv1 "github.com/fluxcd/pkg/apis/event/v1beta1" "github.com/fluxcd/pkg/apis/meta" + "github.com/fluxcd/pkg/git" "github.com/fluxcd/pkg/oci" "github.com/fluxcd/pkg/runtime/conditions" helper "github.com/fluxcd/pkg/runtime/controller" @@ -70,6 +71,7 @@ import ( "github.com/fluxcd/source-controller/internal/helm/getter" "github.com/fluxcd/source-controller/internal/helm/registry" "github.com/fluxcd/source-controller/internal/helm/repository" + soci "github.com/fluxcd/source-controller/internal/oci" sreconcile "github.com/fluxcd/source-controller/internal/reconcile" "github.com/fluxcd/source-controller/internal/reconcile/summarize" "github.com/fluxcd/source-controller/internal/util" @@ -527,7 +529,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * } // Build client options from secret - opts, tls, err := r.clientOptionsFromSecret(secret, normalizedURL) + opts, tlsCfg, err := r.clientOptionsFromSecret(secret, normalizedURL) if err != nil { e := &serror.Event{ Err: err, @@ -538,7 +540,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * return sreconcile.ResultEmpty, e } clientOpts = append(clientOpts, opts...) - tlsConfig = tls + tlsConfig = tlsCfg // Build registryClient options from secret keychain, err = registry.LoginOptionFromSecret(normalizedURL, *secret) @@ -651,34 +653,26 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * } } default: - httpChartRepo, err := repository.NewChartRepository(normalizedURL, r.Storage.LocalPath(*repo.GetArtifact()), r.Getters, tlsConfig, clientOpts, - repository.WithMemoryCache(r.Storage.LocalPath(*repo.GetArtifact()), r.Cache, r.TTL, func(event string) { - r.IncCacheEvents(event, obj.Name, obj.Namespace) - })) + httpChartRepo, err := repository.NewChartRepository(normalizedURL, r.Storage.LocalPath(*repo.GetArtifact()), r.Getters, tlsConfig, clientOpts...) if err != nil { return chartRepoConfigErrorReturn(err, obj) } chartRepo = httpChartRepo defer func() { - if httpChartRepo == nil { - return - } // Cache the index if it was successfully retrieved - // and the chart was successfully built + // and the chart was successfully built. if r.Cache != nil && httpChartRepo.Index != nil { - // The cache key have to be safe in multi-tenancy environments, - // as otherwise it could be used as a vector to bypass the helm repository's authentication. - // Using r.Storage.LocalPath(*repo.GetArtifact() is safe as the path is in the format ///. - err := httpChartRepo.CacheIndexInMemory() - if err != nil { + // The cache key has to be safe in multi-tenancy environments, + // as otherwise it could be used as a vector to bypass + // authentication to the repository. + // Using r.Storage.LocalPath(*repo.GetArtifact()) is safe as the + // path is in the format of: ///. + if err = r.Cache.Set(r.Storage.LocalPath(*repo.GetArtifact()), httpChartRepo.Index, r.TTL); err != nil { r.eventLogf(ctx, obj, eventv1.EventTypeTrace, sourcev1.CacheOperationFailedReason, "failed to cache index: %s", err) } } - // Delete the index reference - if httpChartRepo.Index != nil { - httpChartRepo.Unload() - } + httpChartRepo.Clear() }() } @@ -845,7 +839,7 @@ func (r *HelmChartReconciler) buildFromTarballArtifact(ctx context.Context, obj // early. // On a successful archive, the Artifact in the Status of the object is set, // and the symlink in the Storage is updated to its path. -func (r *HelmChartReconciler) reconcileArtifact(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.HelmChart, b *chart.Build) (sreconcile.Result, error) { +func (r *HelmChartReconciler) reconcileArtifact(ctx context.Context, _ *patch.SerialPatcher, obj *sourcev1.HelmChart, b *chart.Build) (sreconcile.Result, error) { // Without a complete chart build, there is little to reconcile if !b.Complete() { return sreconcile.ResultRequeue, nil @@ -1016,14 +1010,15 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont authenticator authn.Authenticator keychain authn.Keychain ) + normalizedURL := repository.NormalizeURL(url) - repo, err := r.resolveDependencyRepository(ctx, url, namespace) + obj, err := r.resolveDependencyRepository(ctx, url, namespace) if err != nil { // Return Kubernetes client errors, but ignore others if apierrs.ReasonForError(err) != metav1.StatusReasonUnknown { return nil, err } - repo = &sourcev1.HelmRepository{ + obj = &sourcev1.HelmRepository{ Spec: sourcev1.HelmRepositorySpec{ URL: url, Timeout: &metav1.Duration{Duration: 60 * time.Second}, @@ -1032,37 +1027,37 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont } // Used to login with the repository declared provider - ctxTimeout, cancel := context.WithTimeout(ctx, repo.Spec.Timeout.Duration) + ctxTimeout, cancel := context.WithTimeout(ctx, obj.Spec.Timeout.Duration) defer cancel() clientOpts := []helmgetter.Option{ helmgetter.WithURL(normalizedURL), - helmgetter.WithTimeout(repo.Spec.Timeout.Duration), - helmgetter.WithPassCredentialsAll(repo.Spec.PassCredentials), + helmgetter.WithTimeout(obj.Spec.Timeout.Duration), + helmgetter.WithPassCredentialsAll(obj.Spec.PassCredentials), } - if secret, err := r.getHelmRepositorySecret(ctx, repo); secret != nil || err != nil { + if secret, err := r.getHelmRepositorySecret(ctx, obj); secret != nil || err != nil { if err != nil { return nil, err } // Build client options from secret - opts, tls, err := r.clientOptionsFromSecret(secret, normalizedURL) + opts, tlsCfg, err := r.clientOptionsFromSecret(secret, normalizedURL) if err != nil { return nil, err } clientOpts = append(clientOpts, opts...) - tlsConfig = tls + tlsConfig = tlsCfg // Build registryClient options from secret keychain, err = registry.LoginOptionFromSecret(normalizedURL, *secret) if err != nil { - return nil, fmt.Errorf("failed to create login options for HelmRepository '%s': %w", repo.Name, err) + return nil, fmt.Errorf("failed to create login options for HelmRepository '%s': %w", obj.Name, err) } - } else if repo.Spec.Provider != sourcev1.GenericOCIProvider && repo.Spec.Type == sourcev1.HelmRepositoryTypeOCI { - auth, authErr := oidcAuth(ctxTimeout, repo.Spec.URL, repo.Spec.Provider) + } else if obj.Spec.Provider != sourcev1.GenericOCIProvider && obj.Spec.Type == sourcev1.HelmRepositoryTypeOCI { + auth, authErr := oidcAuth(ctxTimeout, obj.Spec.URL, obj.Spec.Provider) if authErr != nil && !errors.Is(authErr, oci.ErrUnconfiguredProvider) { - return nil, fmt.Errorf("failed to get credential from %s: %w", repo.Spec.Provider, authErr) + return nil, fmt.Errorf("failed to get credential from %s: %w", obj.Spec.Provider, authErr) } if auth != nil { authenticator = auth @@ -1078,7 +1073,7 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont if helmreg.IsOCI(normalizedURL) { registryClient, credentialsFile, err := r.RegistryClientGenerator(loginOpt != nil) if err != nil { - return nil, fmt.Errorf("failed to create registry client for HelmRepository '%s': %w", repo.Name, err) + return nil, fmt.Errorf("failed to create registry client for HelmRepository '%s': %w", obj.Name, err) } var errs []error @@ -1089,7 +1084,7 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont repository.WithOCIRegistryClient(registryClient), repository.WithCredentialsFile(credentialsFile)) if err != nil { - errs = append(errs, fmt.Errorf("failed to create OCI chart repository for HelmRepository '%s': %w", repo.Name, err)) + errs = append(errs, fmt.Errorf("failed to create OCI chart repository for HelmRepository '%s': %w", obj.Name, err)) // clean up the credentialsFile if credentialsFile != "" { if err := os.Remove(credentialsFile); err != nil { @@ -1104,7 +1099,7 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont if loginOpt != nil { err = ociChartRepo.Login(loginOpt) if err != nil { - errs = append(errs, fmt.Errorf("failed to login to OCI chart repository for HelmRepository '%s': %w", repo.Name, err)) + errs = append(errs, fmt.Errorf("failed to login to OCI chart repository for HelmRepository '%s': %w", obj.Name, err)) // clean up the credentialsFile errs = append(errs, ociChartRepo.Clear()) return nil, kerrors.NewAggregate(errs) @@ -1113,19 +1108,28 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont chartRepo = ociChartRepo } else { - httpChartRepo, err := repository.NewChartRepository(normalizedURL, "", r.Getters, tlsConfig, clientOpts) + httpChartRepo, err := repository.NewChartRepository(normalizedURL, "", r.Getters, tlsConfig, clientOpts...) if err != nil { return nil, err } - // Ensure that the cache key is the same as the artifact path - // otherwise don't enable caching. We don't want to cache indexes - // for repositories that are not reconciled by the source controller. - if repo.Status.Artifact != nil { - httpChartRepo.CachePath = r.Storage.LocalPath(*repo.GetArtifact()) - httpChartRepo.SetMemCache(r.Storage.LocalPath(*repo.GetArtifact()), r.Cache, r.TTL, func(event string) { - r.IncCacheEvents(event, name, namespace) - }) + if obj.Status.Artifact != nil { + // 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 { + r.IncCacheEvents(cache.CacheEventTypeHit, name, namespace) + r.Cache.SetExpiration(httpChartRepo.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) + } + } } chartRepo = httpChartRepo diff --git a/controllers/helmrepository_controller.go b/controllers/helmrepository_controller.go index 343a9f883..37c918e5c 100644 --- a/controllers/helmrepository_controller.go +++ b/controllers/helmrepository_controller.go @@ -21,10 +21,12 @@ import ( "crypto/tls" "errors" "fmt" + "github.com/fluxcd/source-controller/internal/digest" "net/url" "time" "github.com/docker/go-units" + digestlib "github.com/opencontainers/go-digest" helmgetter "helm.sh/helm/v3/pkg/getter" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" @@ -277,13 +279,13 @@ func (r *HelmRepositoryReconciler) reconcile(ctx context.Context, sp *patch.Seri res = sreconcile.LowestRequeuingResult(res, recResult) } - r.notify(ctx, oldObj, obj, chartRepo, res, resErr) + r.notify(ctx, oldObj, obj, &chartRepo, res, resErr) return res, resErr } // notify emits notification related to the reconciliation. -func (r *HelmRepositoryReconciler) notify(ctx context.Context, oldObj, newObj *sourcev1.HelmRepository, chartRepo repository.ChartRepository, res sreconcile.Result, resErr error) { +func (r *HelmRepositoryReconciler) notify(ctx context.Context, oldObj, newObj *sourcev1.HelmRepository, chartRepo *repository.ChartRepository, res sreconcile.Result, resErr error) { // Notify successful reconciliation for new artifact and recovery from any // failure. if resErr == nil && res == sreconcile.ResultSuccess && newObj.Status.Artifact != nil { @@ -433,7 +435,7 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patc } // Construct Helm chart repository with options and download index - newChartRepo, err := repository.NewChartRepository(obj.Spec.URL, "", r.Getters, tlsConfig, clientOpts) + newChartRepo, err := repository.NewChartRepository(obj.Spec.URL, "", r.Getters, tlsConfig, clientOpts...) if err != nil { switch err.(type) { case *url.Error: @@ -454,8 +456,7 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patc } // Fetch the repository index from remote. - checksum, err := newChartRepo.CacheIndex() - if err != nil { + if err := newChartRepo.CacheIndex(); err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to fetch Helm repository index: %w", err), Reason: meta.FailedReason, @@ -466,20 +467,48 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patc } *chartRepo = *newChartRepo - // 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. - if obj.GetArtifact().HasChecksum(checksum) { - *artifact = *obj.GetArtifact() - conditions.Delete(obj, sourcev1.FetchFailedCondition) - return sreconcile.ResultSuccess, nil + // Early comparison to current Artifact. + if curArtifact := obj.GetArtifact(); curArtifact != nil { + curDig := digestlib.Digest(curArtifact.Digest) + if curDig == "" { + curDig = digestlib.Digest(sourcev1.TransformLegacyRevision(curArtifact.Checksum)) + } + 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. + if newDig := chartRepo.Digest(curDig.Algorithm()); newDig.Validate() == nil && (newDig == curDig) { + *artifact = *curArtifact + conditions.Delete(obj, sourcev1.FetchFailedCondition) + return sreconcile.ResultSuccess, nil + } + } } - // Load the cached repository index to ensure it passes validation. This - // also populates chartRepo.Checksum. - if err := chartRepo.LoadFromCache(); err != nil { + // Load the cached repository index to ensure it passes validation. + if err := chartRepo.LoadFromPath(); err != nil { e := &serror.Event{ - Err: fmt.Errorf("failed to load Helm repository from cache: %w", err), + Err: fmt.Errorf("failed to load Helm repository from index YAML: %w", err), + Reason: sourcev1.IndexationFailedReason, + } + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) + return sreconcile.ResultEmpty, e + } + // 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 := digestlib.Digest(sourcev1.TransformLegacyRevision(artifact.Revision)) + changed = curRev.Validate() != nil || curRev != chartRepo.Revision(curRev.Algorithm()) + } + + // Calculate revision. + revision := chartRepo.Revision(digest.Canonical) + if revision.Validate() != nil { + e := &serror.Event{ + Err: fmt.Errorf("failed to calculate revision: %w", err), Reason: sourcev1.IndexationFailedReason, } conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) @@ -487,8 +516,8 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patc } // Mark observations about the revision on the object. - if !obj.GetArtifact().HasRevision(chartRepo.Checksum) { - message := fmt.Sprintf("new index revision '%s'", checksum) + 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) } @@ -500,15 +529,11 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patc } // Create potential new artifact. - // Note: Since this is a potential artifact, artifact.Checksum is empty at - // this stage. It's populated when the artifact is written in storage. *artifact = r.Storage.NewArtifactFor(obj.Kind, obj.ObjectMeta.GetObjectMeta(), - chartRepo.Checksum, - fmt.Sprintf("index-%s.yaml", checksum)) - - // Delete any stale failure observation - conditions.Delete(obj, sourcev1.FetchFailedCondition) + revision.String(), + fmt.Sprintf("index-%s.yaml", revision.Hex()), + ) return sreconcile.ResultSuccess, nil } @@ -530,15 +555,17 @@ func (r *HelmRepositoryReconciler) reconcileArtifact(ctx context.Context, sp *pa conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact: revision '%s'", artifact.Revision) } - - chartRepo.Unload() - - if err := chartRepo.RemoveCache(); err != nil { + if err := chartRepo.Clear(); err != nil { ctrl.LoggerFrom(ctx).Error(err, "failed to remove temporary cached index file") } }() 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.eventLogf(ctx, obj, eventv1.EventTypeTrace, sourcev1.ArtifactUpToDateReason, "artifact up-to-date with remote revision: '%s'", artifact.Revision) return sreconcile.ResultSuccess, nil } @@ -564,7 +591,7 @@ func (r *HelmRepositoryReconciler) reconcileArtifact(ctx context.Context, sp *pa defer unlock() // Save artifact to storage. - if err = r.Storage.CopyFromPath(artifact, chartRepo.CachePath); err != nil { + if err = r.Storage.CopyFromPath(artifact, chartRepo.Path); err != nil { e := &serror.Event{ Err: fmt.Errorf("unable to save artifact to storage: %w", err), Reason: sourcev1.ArchiveOperationFailedReason, @@ -576,6 +603,18 @@ func (r *HelmRepositoryReconciler) reconcileArtifact(ctx context.Context, sp *pa // Record it on the object. obj.Status.Artifact = artifact.DeepCopy() + // Cache the index if it was successfully retrieved. + 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 { + r.eventLogf(ctx, obj, eventv1.EventTypeTrace, sourcev1.CacheOperationFailedReason, "failed to cache index: %s", err) + } + } + // Update index symlink. indexURL, err := r.Storage.Symlink(*artifact, "index.yaml") if err != nil { @@ -586,26 +625,6 @@ func (r *HelmRepositoryReconciler) reconcileArtifact(ctx context.Context, sp *pa obj.Status.URL = indexURL } conditions.Delete(obj, sourcev1.StorageOperationFailedCondition) - - // enable cache if applicable - if r.Cache != nil && chartRepo.IndexCache == nil { - chartRepo.SetMemCache(r.Storage.LocalPath(*artifact), r.Cache, r.TTL, func(event string) { - r.IncCacheEvents(event, obj.GetName(), obj.GetNamespace()) - }) - } - - // Cache the index if it was successfully retrieved - // and the chart was successfully built - if r.Cache != nil && chartRepo.Index != nil { - // The cache key have to be safe in multi-tenancy environments, - // as otherwise it could be used as a vector to bypass the helm repository's authentication. - // Using r.Storage.LocalPath(*repo.GetArtifact() is safe as the path is in the format ///. - err := chartRepo.CacheIndexInMemory() - if err != nil { - r.eventLogf(ctx, obj, eventv1.EventTypeTrace, sourcev1.CacheOperationFailedReason, "failed to cache index: %s", err) - } - } - return sreconcile.ResultSuccess, nil } diff --git a/controllers/helmrepository_controller_test.go b/controllers/helmrepository_controller_test.go index 40b106509..807fef045 100644 --- a/controllers/helmrepository_controller_test.go +++ b/controllers/helmrepository_controller_test.go @@ -21,6 +21,10 @@ import ( "crypto/tls" "errors" "fmt" + "github.com/fluxcd/source-controller/internal/cache" + "github.com/fluxcd/source-controller/internal/digest" + digestlib "github.com/opencontainers/go-digest" + "helm.sh/helm/v3/pkg/repo" "net/http" "os" "path/filepath" @@ -312,8 +316,8 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { server options url string secret *corev1.Secret - beforeFunc func(t *WithT, obj *sourcev1.HelmRepository, checksum string) - afterFunc func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo repository.ChartRepository) + beforeFunc func(t *WithT, obj *sourcev1.HelmRepository, revision, digest digestlib.Digest) + afterFunc func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo *repository.ChartRepository) want sreconcile.Result wantErr bool assertConditions []metav1.Condition @@ -344,9 +348,9 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { *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 *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo repository.ChartRepository) { - t.Expect(chartRepo.Checksum).ToNot(BeEmpty()) - t.Expect(chartRepo.CachePath).ToNot(BeEmpty()) + afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo *repository.ChartRepository) { + t.Expect(chartRepo.Path).ToNot(BeEmpty()) + t.Expect(chartRepo.Index).ToNot(BeNil()) t.Expect(artifact.Checksum).To(BeEmpty()) t.Expect(artifact.Revision).ToNot(BeEmpty()) }, @@ -367,7 +371,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { "password": []byte("1234"), }, }, - beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, checksum string) { + beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, revision, checksum digestlib.Digest) { obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "basic-auth"} }, want: sreconcile.ResultSuccess, @@ -375,9 +379,9 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { *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 *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo repository.ChartRepository) { - t.Expect(chartRepo.Checksum).ToNot(BeEmpty()) - t.Expect(chartRepo.CachePath).ToNot(BeEmpty()) + afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo *repository.ChartRepository) { + t.Expect(chartRepo.Path).ToNot(BeEmpty()) + t.Expect(chartRepo.Index).ToNot(BeNil()) t.Expect(artifact.Checksum).To(BeEmpty()) t.Expect(artifact.Revision).ToNot(BeEmpty()) }, @@ -398,7 +402,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { "caFile": tlsCA, }, }, - beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, checksum string) { + beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, revision, checksum digestlib.Digest) { obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "ca-file"} }, want: sreconcile.ResultSuccess, @@ -406,9 +410,9 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { *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 *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo repository.ChartRepository) { - t.Expect(chartRepo.Checksum).ToNot(BeEmpty()) - t.Expect(chartRepo.CachePath).ToNot(BeEmpty()) + afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo *repository.ChartRepository) { + t.Expect(chartRepo.Path).ToNot(BeEmpty()) + t.Expect(chartRepo.Index).ToNot(BeNil()) t.Expect(artifact.Checksum).To(BeEmpty()) t.Expect(artifact.Revision).ToNot(BeEmpty()) }, @@ -429,7 +433,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { "caFile": []byte("invalid"), }, }, - beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, checksum string) { + beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, revision, checksum digestlib.Digest) { obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "invalid-ca"} conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") @@ -440,10 +444,10 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, - afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo repository.ChartRepository) { + afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo *repository.ChartRepository) { // No repo index due to fetch fail. - t.Expect(chartRepo.Checksum).To(BeEmpty()) - t.Expect(chartRepo.CachePath).To(BeEmpty()) + t.Expect(chartRepo.Path).To(BeEmpty()) + t.Expect(chartRepo.Index).To(BeNil()) t.Expect(artifact.Checksum).To(BeEmpty()) t.Expect(artifact.Revision).To(BeEmpty()) }, @@ -451,7 +455,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { { name: "Invalid URL makes FetchFailed=True and returns stalling error", protocol: "http", - beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, checksum string) { + beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, revision, checksum digestlib.Digest) { obj.Spec.URL = strings.ReplaceAll(obj.Spec.URL, "http://", "") conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") @@ -463,10 +467,10 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, - afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo repository.ChartRepository) { + afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo *repository.ChartRepository) { // No repo index due to fetch fail. - t.Expect(chartRepo.Checksum).To(BeEmpty()) - t.Expect(chartRepo.CachePath).To(BeEmpty()) + t.Expect(chartRepo.Path).To(BeEmpty()) + t.Expect(chartRepo.Index).To(BeNil()) t.Expect(artifact.Checksum).To(BeEmpty()) t.Expect(artifact.Revision).To(BeEmpty()) }, @@ -474,7 +478,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { { name: "Unsupported scheme makes FetchFailed=True and returns stalling error", protocol: "http", - beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, checksum string) { + beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, revision, checksum digestlib.Digest) { obj.Spec.URL = strings.ReplaceAll(obj.Spec.URL, "http://", "ftp://") conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") @@ -486,10 +490,10 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, - afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo repository.ChartRepository) { + afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo *repository.ChartRepository) { // No repo index due to fetch fail. - t.Expect(chartRepo.Checksum).To(BeEmpty()) - t.Expect(chartRepo.CachePath).To(BeEmpty()) + t.Expect(chartRepo.Path).To(BeEmpty()) + t.Expect(chartRepo.Index).To(BeNil()) t.Expect(artifact.Checksum).To(BeEmpty()) t.Expect(artifact.Revision).To(BeEmpty()) }, @@ -497,7 +501,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { { name: "Missing secret returns FetchFailed=True and returns error", protocol: "http", - beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, checksum string) { + beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, revision, checksum digestlib.Digest) { obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "non-existing"} conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") @@ -508,10 +512,10 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, - afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo repository.ChartRepository) { + afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo *repository.ChartRepository) { // No repo index due to fetch fail. - t.Expect(chartRepo.Checksum).To(BeEmpty()) - t.Expect(chartRepo.CachePath).To(BeEmpty()) + t.Expect(chartRepo.Path).To(BeEmpty()) + t.Expect(chartRepo.Index).To(BeNil()) t.Expect(artifact.Checksum).To(BeEmpty()) t.Expect(artifact.Revision).To(BeEmpty()) }, @@ -527,7 +531,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { "username": []byte("git"), }, }, - beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, checksum string) { + beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, revision, checksum digestlib.Digest) { obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "malformed-basic-auth"} conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") @@ -538,66 +542,125 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, - afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo repository.ChartRepository) { + afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo *repository.ChartRepository) { // No repo index due to fetch fail. - t.Expect(chartRepo.Checksum).To(BeEmpty()) - t.Expect(chartRepo.CachePath).To(BeEmpty()) + t.Expect(chartRepo.Path).To(BeEmpty()) + t.Expect(chartRepo.Index).To(BeNil()) t.Expect(artifact.Checksum).To(BeEmpty()) t.Expect(artifact.Revision).To(BeEmpty()) }, }, { - name: "cached index with same checksum", + name: "Stored index with same digest and revision", protocol: "http", - beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, checksum string) { + beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, revision, digest digestlib.Digest) { obj.Status.Artifact = &sourcev1.Artifact{ - Revision: checksum, - Checksum: checksum, + Revision: revision.String(), + Digest: digest.String(), + Checksum: digest.Hex(), } + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, "foo", "bar") }, assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, - afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo repository.ChartRepository) { - // chartRepo.Checksum isn't populated, artifact.Checksum is - // populated from the cached repo index data. - t.Expect(chartRepo.Checksum).To(BeEmpty()) - t.Expect(chartRepo.CachePath).ToNot(BeEmpty()) - t.Expect(artifact.Checksum).To(Equal(obj.Status.Artifact.Checksum)) - t.Expect(artifact.Revision).To(Equal(obj.Status.Artifact.Revision)) + afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo *repository.ChartRepository) { + t.Expect(chartRepo.Path).ToNot(BeEmpty()) + t.Expect(chartRepo.Index).To(BeNil()) + + t.Expect(&artifact).To(BeEquivalentTo(obj.Status.Artifact)) }, want: sreconcile.ResultSuccess, }, { - name: "cached index with different checksum", + name: "Stored index with same checksum and (legacy) revision", protocol: "http", - beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, checksum string) { + beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, revision, digest digestlib.Digest) { obj.Status.Artifact = &sourcev1.Artifact{ - Revision: checksum, - Checksum: "foo", + Revision: revision.Hex(), + Checksum: digest.Hex(), } + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, "foo", "bar") }, assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, - afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo repository.ChartRepository) { - t.Expect(chartRepo.Checksum).ToNot(BeEmpty()) - t.Expect(chartRepo.CachePath).ToNot(BeEmpty()) - t.Expect(artifact.Checksum).To(BeEmpty()) + afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo *repository.ChartRepository) { + t.Expect(chartRepo.Path).ToNot(BeEmpty()) + t.Expect(chartRepo.Index).To(BeNil()) + + t.Expect(&artifact).To(BeEquivalentTo(obj.Status.Artifact)) + }, + want: sreconcile.ResultSuccess, + }, + { + name: "Stored index with different digest and same revision", + protocol: "http", + beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, revision, digest digestlib.Digest) { + obj.Status.Artifact = &sourcev1.Artifact{ + Revision: revision.String(), + Digest: "sha256:80bb3dd67c63095d985850459834ea727603727a370079de90d221191d375a86", + Checksum: "80bb3dd67c63095d985850459834ea727603727a370079de90d221191d375a86", + } + + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, "foo", "bar") + }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), + }, + afterFunc: func(t *WithT, obj *sourcev1.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.Checksum).ToNot(Equal(obj.Status.Artifact.Checksum)) + }, + want: sreconcile.ResultSuccess, + }, + { + name: "Stored index with different revision and digest", + protocol: "http", + beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, revision, checksum digestlib.Digest) { + obj.Status.Artifact = &sourcev1.Artifact{ + Revision: "80bb3dd67c63095d985850459834ea727603727a370079de90d221191d375a86", + Checksum: "80bb3dd67c63095d985850459834ea727603727a370079de90d221191d375a86", + Digest: "sha256:80bb3dd67c63095d985850459834ea727603727a370079de90d221191d375a86", + } + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") + }, + assertConditions: []metav1.Condition{ + *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 *sourcev1.HelmRepository, artifact sourcev1.Artifact, chartRepo *repository.ChartRepository) { + t.Expect(chartRepo.Path).ToNot(BeEmpty()) + t.Expect(chartRepo.Index).ToNot(BeNil()) + + t.Expect(artifact.Path).To(Not(BeEmpty())) + t.Expect(artifact.Revision).ToNot(Equal(obj.Status.Artifact.Revision)) + t.Expect(artifact.Digest).ToNot(Equal(obj.Status.Artifact.Digest)) + t.Expect(artifact.Checksum).ToNot(Equal(obj.Status.Artifact.Checksum)) }, want: sreconcile.ResultSuccess, }, { name: "Existing artifact makes ArtifactOutdated=True", protocol: "http", - beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, checksum string) { + beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, revision, checksum digestlib.Digest) { obj.Status.Artifact = &sourcev1.Artifact{ Path: "some-path", Revision: "some-rev", @@ -698,22 +761,24 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { if serr != nil { validSecret = false } - newChartRepo, err = repository.NewChartRepository(obj.Spec.URL, "", testGetters, tOpts, clientOpts) + newChartRepo, err = repository.NewChartRepository(obj.Spec.URL, "", testGetters, tOpts, clientOpts...) } else { - newChartRepo, err = repository.NewChartRepository(obj.Spec.URL, "", testGetters, nil, nil) + newChartRepo, err = repository.NewChartRepository(obj.Spec.URL, "", testGetters, nil) } g.Expect(err).ToNot(HaveOccurred()) // NOTE: checksum will be empty in beforeFunc for invalid repo // configurations as the client can't get the repo. - var indexChecksum string + var revision, checksum digestlib.Digest if validSecret { - indexChecksum, err = newChartRepo.CacheIndex() - g.Expect(err).ToNot(HaveOccurred()) - } + g.Expect(newChartRepo.CacheIndex()).To(Succeed()) + checksum = newChartRepo.Digest(digest.Canonical) + g.Expect(newChartRepo.LoadFromPath()).To(Succeed()) + revision = newChartRepo.Revision(digest.Canonical) + } if tt.beforeFunc != nil { - tt.beforeFunc(g, obj, indexChecksum) + tt.beforeFunc(g, obj, revision, checksum) } r := &HelmRepositoryReconciler{ @@ -734,14 +799,14 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { sp := patch.NewSerialPatcher(obj, r.Client) got, err := r.reconcileSource(context.TODO(), sp, obj, &artifact, &chartRepo) - defer os.Remove(chartRepo.CachePath) + defer os.Remove(chartRepo.Path) g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(tt.assertConditions)) g.Expect(err != nil).To(Equal(tt.wantErr)) g.Expect(got).To(Equal(tt.want)) if tt.afterFunc != nil { - tt.afterFunc(g, obj, artifact, chartRepo) + tt.afterFunc(g, obj, artifact, &chartRepo) } // In-progress status condition validity. @@ -754,8 +819,9 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) { tests := []struct { name string + cache *cache.Cache beforeFunc func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, index *repository.ChartRepository) - afterFunc func(t *WithT, obj *sourcev1.HelmRepository) + afterFunc func(t *WithT, obj *sourcev1.HelmRepository, cache *cache.Cache) want sreconcile.Result wantErr bool assertConditions []metav1.Condition @@ -770,13 +836,33 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) { *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact: revision 'existing'"), }, }, + { + name: "Archiving (loaded) artifact to storage adds to cache", + cache: cache.New(10, time.Minute), + beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, index *repository.ChartRepository) { + index.Index = &repo.IndexFile{ + APIVersion: "v1", + Generated: time.Now(), + } + obj.Spec.Interval = metav1.Duration{Duration: interval} + }, + want: sreconcile.ResultSuccess, + afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, cache *cache.Cache) { + i, ok := cache.Get(testStorage.LocalPath(*obj.GetArtifact())) + t.Expect(ok).To(BeTrue()) + t.Expect(i).To(BeAssignableToTypeOf(&repo.IndexFile{})) + }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact: revision 'existing'"), + }, + }, { name: "Up-to-date artifact should not update status", beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, index *repository.ChartRepository) { obj.Spec.Interval = metav1.Duration{Duration: interval} obj.Status.Artifact = artifact.DeepCopy() }, - afterFunc: func(t *WithT, obj *sourcev1.HelmRepository) { + afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, _ *cache.Cache) { t.Expect(obj.Status.URL).To(BeEmpty()) }, want: sreconcile.ResultSuccess, @@ -800,7 +886,7 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) { beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, index *repository.ChartRepository) { obj.Spec.Interval = metav1.Duration{Duration: interval} }, - afterFunc: func(t *WithT, obj *sourcev1.HelmRepository) { + afterFunc: func(t *WithT, obj *sourcev1.HelmRepository, _ *cache.Cache) { localPath := testStorage.LocalPath(*obj.GetArtifact()) symlinkPath := filepath.Join(filepath.Dir(localPath), "index.yaml") targetFile, err := os.Readlink(symlinkPath) @@ -822,6 +908,8 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) { Client: fakeclient.NewClientBuilder().WithScheme(testEnv.GetScheme()).Build(), EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, + Cache: tt.cache, + TTL: 1 * time.Minute, patchOptions: getPatchOptions(helmRepositoryReadyCondition.Owned, "sc"), } @@ -848,9 +936,9 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) g.Expect(cacheFile.Close()).ToNot(HaveOccurred()) - chartRepo, err := repository.NewChartRepository(obj.Spec.URL, "", testGetters, nil, nil) + chartRepo, err := repository.NewChartRepository(obj.Spec.URL, "", testGetters, nil) g.Expect(err).ToNot(HaveOccurred()) - chartRepo.CachePath = cachePath + chartRepo.Path = cachePath artifact := testStorage.NewArtifactFor(obj.Kind, obj, "existing", "foo.tar.gz") // Checksum of the index file calculated by the ChartRepository. @@ -873,7 +961,7 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) { g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(tt.assertConditions)) if tt.afterFunc != nil { - tt.afterFunc(g, obj) + tt.afterFunc(g, obj, tt.cache) } }) } @@ -1209,7 +1297,7 @@ func TestHelmRepositoryReconciler_notify(t *testing.T) { chartRepo := repository.ChartRepository{ URL: "some-address", } - reconciler.notify(ctx, oldObj, newObj, chartRepo, tt.res, tt.resErr) + reconciler.notify(ctx, oldObj, newObj, &chartRepo, tt.res, tt.resErr) select { case x, ok := <-recorder.Events: diff --git a/internal/helm/chart/builder_remote_test.go b/internal/helm/chart/builder_remote_test.go index 31e6235c5..fa4fcf3ef 100644 --- a/internal/helm/chart/builder_remote_test.go +++ b/internal/helm/chart/builder_remote_test.go @@ -193,10 +193,9 @@ entries: targetPath := filepath.Join(tmpDir, "chart.tgz") if tt.repository != nil { - _, err := tt.repository.CacheIndex() - g.Expect(err).ToNot(HaveOccurred()) + g.Expect(tt.repository.CacheIndex()).ToNot(HaveOccurred()) // Cleanup the cache index path. - defer os.Remove(tt.repository.CachePath) + defer os.Remove(tt.repository.Path) } b := NewRemoteBuilder(tt.repository) @@ -411,10 +410,10 @@ entries: reference := RemoteReference{Name: "helmchart"} repository := mockRepo() - _, err = repository.CacheIndex() + err = repository.CacheIndex() g.Expect(err).ToNot(HaveOccurred()) // Cleanup the cache index path. - defer os.Remove(repository.CachePath) + defer os.Remove(repository.Path) b := NewRemoteBuilder(repository) diff --git a/internal/helm/chart/dependency_manager_test.go b/internal/helm/chart/dependency_manager_test.go index d63e5f153..fcd7015a7 100644 --- a/internal/helm/chart/dependency_manager_test.go +++ b/internal/helm/chart/dependency_manager_test.go @@ -86,11 +86,6 @@ func TestDependencyManager_Clear(t *testing.T) { Index: repo.NewIndexFile(), RWMutex: &sync.RWMutex{}, }, - "cached cache path": &repository.ChartRepository{ - CachePath: "/invalid/path/resets", - Cached: true, - RWMutex: &sync.RWMutex{}, - }, "with credentials": ociRepoWithCreds, "without credentials": &repository.OCIChartRepository{}, "nil downloader": nil, @@ -103,8 +98,6 @@ func TestDependencyManager_Clear(t *testing.T) { switch v := v.(type) { case *repository.ChartRepository: g.Expect(v.Index).To(BeNil()) - g.Expect(v.CachePath).To(BeEmpty()) - g.Expect(v.Cached).To(BeFalse()) case *repository.OCIChartRepository: g.Expect(v.HasCredentials()).To(BeFalse()) } @@ -441,14 +434,14 @@ func TestDependencyManager_addRemoteDependency(t *testing.T) { name: "strategic load error", downloaders: map[string]repository.Downloader{ "https://example.com/": &repository.ChartRepository{ - CachePath: "/invalid/cache/path/foo", - RWMutex: &sync.RWMutex{}, + Client: &mockGetter{}, + RWMutex: &sync.RWMutex{}, }, }, dep: &helmchart.Dependency{ Repository: "https://example.com", }, - wantErr: "failed to strategically load index", + wantErr: "failed to load index", }, { name: "repository get error", diff --git a/internal/helm/repository/chart_repository.go b/internal/helm/repository/chart_repository.go index 0b1e9332a..34781e9ac 100644 --- a/internal/helm/repository/chart_repository.go +++ b/internal/helm/repository/chart_repository.go @@ -19,12 +19,9 @@ package repository import ( "bytes" "context" - "crypto/sha256" "crypto/tls" - "encoding/hex" "errors" "fmt" - "github.com/opencontainers/go-digest" "io" "net/url" "os" @@ -32,22 +29,79 @@ import ( "sort" "strings" "sync" - "time" "github.com/Masterminds/semver/v3" + "github.com/opencontainers/go-digest" + "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/getter" "helm.sh/helm/v3/pkg/repo" - kerrors "k8s.io/apimachinery/pkg/util/errors" "sigs.k8s.io/yaml" "github.com/fluxcd/pkg/version" - "github.com/fluxcd/source-controller/internal/cache" "github.com/fluxcd/source-controller/internal/helm" "github.com/fluxcd/source-controller/internal/transport" ) -var ErrNoChartIndex = errors.New("no chart index") +var ( + ErrNoChartIndex = errors.New("no chart index") +) + +// IndexFromFile loads a repo.IndexFile from the given path. It returns an +// error if the file does not exist, is not a regular file, exceeds the +// maximum index file size, or if the file cannot be parsed. +func IndexFromFile(path string) (*repo.IndexFile, error) { + st, err := os.Lstat(path) + if err != nil { + return nil, err + } + if !st.Mode().IsRegular() { + return nil, fmt.Errorf("%s is not a regular file", path) + } + if st.Size() > helm.MaxIndexSize { + return nil, fmt.Errorf("%s exceeds the maximum index file size of %d bytes", path, helm.MaxIndexSize) + } + b, err := os.ReadFile(path) + if err != nil { + return nil, err + } + return IndexFromBytes(b) +} + +// IndexFromBytes loads a repo.IndexFile from the given bytes. It returns an +// error if the bytes cannot be parsed, or if the API version is not set. +// The entries are sorted before the index is returned. +func IndexFromBytes(b []byte) (*repo.IndexFile, error) { + if len(b) == 0 { + return nil, repo.ErrEmptyIndexYaml + } + + i := &repo.IndexFile{} + if err := yaml.UnmarshalStrict(b, i); err != nil { + return nil, err + } + + if i.APIVersion == "" { + return nil, repo.ErrNoAPIVersion + } + + for _, cvs := range i.Entries { + for idx := len(cvs) - 1; idx >= 0; idx-- { + if cvs[idx] == nil { + continue + } + if cvs[idx].APIVersion == "" { + cvs[idx].APIVersion = chart.APIVersionV1 + } + if err := cvs[idx].Validate(); err != nil { + cvs = append(cvs[:idx], cvs[idx+1:]...) + } + } + } + + i.SortEntries() + return i, nil +} // ChartRepository represents a Helm chart repository, and the configuration // required to download the chart index and charts from the repository. @@ -56,73 +110,32 @@ type ChartRepository struct { // URL the ChartRepository's index.yaml can be found at, // without the index.yaml suffix. URL string + // Path is the absolute path to the Index file. + Path string + // Index of the ChartRepository. + Index *repo.IndexFile + // Client to use while downloading the Index or a chart from the URL. Client getter.Getter // Options to configure the Client with while downloading the Index // or a chart from the URL. Options []getter.Option - // CachePath is the path of a cached index.yaml for read-only operations. - CachePath string - // Cached indicates if the ChartRepository index.yaml has been cached - // to CachePath. - Cached bool - // Index contains a loaded chart repository index if not nil. - Index *repo.IndexFile - // Checksum contains the SHA256 checksum of the loaded chart repository - // index bytes. This is different from the checksum of the CachePath, which - // may contain unordered entries. - Checksum string tlsConfig *tls.Config - *sync.RWMutex + cached bool + revisions map[digest.Algorithm]digest.Digest + digests map[digest.Algorithm]digest.Digest - cacheInfo -} - -type cacheInfo struct { - // In memory cache of the index.yaml file. - IndexCache *cache.Cache - // IndexKey is the cache key for the index.yaml file. - IndexKey string - // IndexTTL is the cache TTL for the index.yaml file. - IndexTTL time.Duration - // RecordIndexCacheMetric records the cache hit/miss metrics for the index.yaml file. - RecordIndexCacheMetric RecordMetricsFunc -} - -// ChartRepositoryOption is a function that can be passed to NewChartRepository -// to configure a ChartRepository. -type ChartRepositoryOption func(*ChartRepository) error - -// RecordMetricsFunc is a function that records metrics. -type RecordMetricsFunc func(event string) - -// WithMemoryCache returns a ChartRepositoryOptions that will enable the -// ChartRepository to cache the index.yaml file in memory. -// The cache key have to be safe in multi-tenancy environments, -// as otherwise it could be used as a vector to bypass the helm repository's authentication. -func WithMemoryCache(key string, c *cache.Cache, ttl time.Duration, rec RecordMetricsFunc) ChartRepositoryOption { - return func(r *ChartRepository) error { - if c != nil { - if key == "" { - return errors.New("cache key cannot be empty") - } - } - r.IndexCache = c - r.IndexKey = key - r.IndexTTL = ttl - r.RecordIndexCacheMetric = rec - return nil - } + *sync.RWMutex } // NewChartRepository constructs and returns a new ChartRepository with // the ChartRepository.Client configured to the getter.Getter for the // repository URL scheme. It returns an error on URL parsing failures, // or if there is no getter available for the scheme. -func NewChartRepository(repositoryURL, cachePath string, providers getter.Providers, tlsConfig *tls.Config, getterOpts []getter.Option, chartRepoOpts ...ChartRepositoryOption) (*ChartRepository, error) { - u, err := url.Parse(repositoryURL) +func NewChartRepository(URL, path string, providers getter.Providers, tlsConfig *tls.Config, getterOpts ...getter.Option) (*ChartRepository, error) { + u, err := url.Parse(URL) if err != nil { return nil, err } @@ -132,24 +145,20 @@ func NewChartRepository(repositoryURL, cachePath string, providers getter.Provid } r := newChartRepository() - r.URL = repositoryURL - r.CachePath = cachePath + r.URL = URL + r.Path = path r.Client = c r.Options = getterOpts r.tlsConfig = tlsConfig - for _, opt := range chartRepoOpts { - if err := opt(r); err != nil { - return nil, err - } - } - return r, nil } func newChartRepository() *ChartRepository { return &ChartRepository{ - RWMutex: &sync.RWMutex{}, + revisions: make(map[digest.Algorithm]digest.Digest, 0), + digests: make(map[digest.Algorithm]digest.Digest, 0), + RWMutex: &sync.RWMutex{}, } } @@ -206,10 +215,10 @@ func (r *ChartRepository) getChartVersion(name, ver string) (*repo.ChartVersion, } } - // Filter out chart versions that doesn't satisfy constraints if any, + // Filter out chart versions that don't satisfy constraints if any, // parse semver and build a lookup table var matchedVersions semver.Collection - lookup := make(map[*semver.Version]*repo.ChartVersion) + lookup := make(map[*semver.Version]*repo.ChartVersion, 0) for _, cv := range cvs { v, err := version.ParseVersion(cv.Version) if err != nil { @@ -289,155 +298,86 @@ func (r *ChartRepository) DownloadChart(chart *repo.ChartVersion) (*bytes.Buffer return r.Client.Get(u.String(), clientOpts...) } -// LoadIndexFromBytes loads Index from the given bytes. -// It returns a repo.ErrNoAPIVersion error if the API version is not set -func (r *ChartRepository) LoadIndexFromBytes(b []byte) error { - i := &repo.IndexFile{} - if err := yaml.UnmarshalStrict(b, i); err != nil { - return err - } - if i.APIVersion == "" { - return repo.ErrNoAPIVersion - } - i.SortEntries() - - r.Lock() - r.Index = i - r.Checksum = digest.SHA256.FromBytes(b).Hex() - r.Unlock() - return nil -} - -// LoadFromFile reads the file at the given path and loads it into Index. -func (r *ChartRepository) LoadFromFile(path string) error { - stat, err := os.Stat(path) - if err != nil || stat.IsDir() { - if err == nil { - err = fmt.Errorf("'%s' is a directory", path) - } - return err - } - if stat.Size() > helm.MaxIndexSize { - return fmt.Errorf("size of index '%s' exceeds '%d' bytes limit", stat.Name(), helm.MaxIndexSize) - } - b, err := os.ReadFile(path) - if err != nil { - return err - } - return r.LoadIndexFromBytes(b) -} - // CacheIndex attempts to write the index from the remote into a new temporary file -// using DownloadIndex, and sets CachePath and Cached. +// using DownloadIndex, and sets Path and cached. // It returns the SHA256 checksum of the downloaded index bytes, or an error. -// The caller is expected to handle the garbage collection of CachePath, and to -// load the Index separately using LoadFromCache if required. -func (r *ChartRepository) CacheIndex() (string, error) { +// The caller is expected to handle the garbage collection of Path, and to +// load the Index separately using LoadFromPath if required. +func (r *ChartRepository) CacheIndex() error { f, err := os.CreateTemp("", "chart-index-*.yaml") if err != nil { - return "", fmt.Errorf("failed to create temp file to cache index to: %w", err) + return fmt.Errorf("failed to create temp file to cache index to: %w", err) } - h := sha256.New() - mw := io.MultiWriter(f, h) - if err = r.DownloadIndex(mw); err != nil { + if err = r.DownloadIndex(f); err != nil { f.Close() - os.RemoveAll(f.Name()) - return "", fmt.Errorf("failed to cache index to temporary file: %w", err) + os.Remove(f.Name()) + return fmt.Errorf("failed to cache index to temporary file: %w", err) } if err = f.Close(); err != nil { - os.RemoveAll(f.Name()) - return "", fmt.Errorf("failed to close cached index file '%s': %w", f.Name(), err) + os.Remove(f.Name()) + return fmt.Errorf("failed to close cached index file '%s': %w", f.Name(), err) } r.Lock() - r.CachePath = f.Name() - r.Cached = true + r.Path = f.Name() + r.Index = nil + r.cached = true + r.invalidate() r.Unlock() - return hex.EncodeToString(h.Sum(nil)), nil -} - -// CacheIndexInMemory attempts to cache the index in memory. -// It returns an error if it fails. -// The cache key have to be safe in multi-tenancy environments, -// as otherwise it could be used as a vector to bypass the helm repository's authentication. -func (r *ChartRepository) CacheIndexInMemory() error { - // Cache the index if it was successfully retrieved - // and the chart was successfully built - if r.IndexCache != nil && r.Index != nil { - err := r.IndexCache.Set(r.IndexKey, r.Index, r.IndexTTL) - if err != nil { - return err - } - } return nil } -// StrategicallyLoadIndex lazy-loads the Index -// first from Indexcache, -// then from CachePath using oadFromCache if it does not HasIndex. -// If not HasCacheFile, a cache attempt is made using CacheIndex -// before continuing to load. +// StrategicallyLoadIndex lazy-loads the Index if required, first +// attempting to load it from Path if the file exists, before falling +// back to caching it. func (r *ChartRepository) StrategicallyLoadIndex() (err error) { if r.HasIndex() { return } - if r.IndexCache != nil { - if found := r.LoadFromMemCache(); found { + if !r.HasFile() { + if err = r.CacheIndex(); err != nil { + err = fmt.Errorf("failed to cache index: %w", err) return } } - if !r.HasCacheFile() { - if _, err = r.CacheIndex(); err != nil { - err = fmt.Errorf("failed to strategically load index: %w", err) - return - } - } - if err = r.LoadFromCache(); err != nil { - err = fmt.Errorf("failed to strategically load index: %w", err) + if err = r.LoadFromPath(); err != nil { + err = fmt.Errorf("failed to load index: %w", err) return } return } -// LoadFromMemCache attempts to load the Index from the provided cache. -// It returns true if the Index was found in the cache, and false otherwise. -func (r *ChartRepository) LoadFromMemCache() bool { - if index, found := r.IndexCache.Get(r.IndexKey); found { - r.Lock() - r.Index = index.(*repo.IndexFile) - r.Unlock() - - // record the cache hit - if r.RecordIndexCacheMetric != nil { - r.RecordIndexCacheMetric(cache.CacheEventTypeHit) - } - return true - } +// LoadFromPath attempts to load the Index from the configured Path. +// It returns an error if no Path is set, or if the load failed. +func (r *ChartRepository) LoadFromPath() error { + r.Lock() + defer r.Unlock() - // record the cache miss - if r.RecordIndexCacheMetric != nil { - r.RecordIndexCacheMetric(cache.CacheEventTypeMiss) + if len(r.Path) == 0 { + return fmt.Errorf("no cache path") } - return false -} -// LoadFromCache attempts to load the Index from the configured CachePath. -// It returns an error if no CachePath is set, or if the load failed. -func (r *ChartRepository) LoadFromCache() error { - if cachePath := r.CachePath; cachePath != "" { - return r.LoadFromFile(cachePath) + i, err := IndexFromFile(r.Path) + if err != nil { + return fmt.Errorf("failed to load index: %w", err) } - return fmt.Errorf("no cache path set") + + r.Index = i + r.revisions = make(map[digest.Algorithm]digest.Digest, 0) + return nil } // DownloadIndex attempts to download the chart repository index using // the Client and set Options, and writes the index to the given io.Writer. // It returns an url.Error if the URL failed to parse. func (r *ChartRepository) DownloadIndex(w io.Writer) (err error) { + r.RLock() + defer r.RUnlock() + u, err := url.Parse(r.URL) if err != nil { return err @@ -460,75 +400,96 @@ 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() { + return "" + } + + r.Lock() + defer r.Unlock() + + if _, ok := r.digests[algorithm]; !ok { + if f, err := os.Open(r.Path); err == nil { + defer f.Close() + rd := io.LimitReader(f, helm.MaxIndexSize) + if d, err := algorithm.FromReader(rd); err == nil { + r.digests[algorithm] = d + } + } + } + return r.digests[algorithm] +} + // HasIndex returns true if the Index is not nil. func (r *ChartRepository) HasIndex() bool { r.RLock() defer r.RUnlock() + return r.Index != nil } -// HasCacheFile returns true if CachePath is not empty. -func (r *ChartRepository) HasCacheFile() bool { +// HasFile returns true if Path exists and is a regular file. +func (r *ChartRepository) HasFile() bool { r.RLock() defer r.RUnlock() - return r.CachePath != "" -} -// Unload can be used to signal the Go garbage collector the Index can -// be freed from memory if the ChartRepository object is expected to -// continue to exist in the stack for some time. -func (r *ChartRepository) Unload() { - if r == nil { - return + if r.Path != "" { + if stat, err := os.Lstat(r.Path); err == nil { + return stat.Mode().IsRegular() + } } - - r.Lock() - defer r.Unlock() - r.Index = nil + return false } -// Clear caches the index in memory before unloading it. -// It cleans up temporary files and directories created by the repository. +// Clear clears the Index and removes the file at Path, if cached. func (r *ChartRepository) Clear() error { - var errs []error - if err := r.CacheIndexInMemory(); err != nil { - errs = append(errs, err) - } + r.Lock() + defer r.Unlock() - r.Unload() + r.Index = nil - if err := r.RemoveCache(); err != nil { - errs = append(errs, err) + if r.cached { + if err := os.Remove(r.Path); err != nil { + return fmt.Errorf("failed to remove cached index: %w", err) + } + r.Path = "" + r.cached = false } - return kerrors.NewAggregate(errs) -} - -// SetMemCache sets the cache to use for this repository. -func (r *ChartRepository) SetMemCache(key string, c *cache.Cache, ttl time.Duration, rec RecordMetricsFunc) { - r.IndexKey = key - r.IndexCache = c - r.IndexTTL = ttl - r.RecordIndexCacheMetric = rec + r.invalidate() + return nil } -// RemoveCache removes the CachePath if Cached. -func (r *ChartRepository) RemoveCache() error { - if r == nil { - return nil - } - +// Invalidate clears any cached digests and revisions. +func (r *ChartRepository) Invalidate() { r.Lock() defer r.Unlock() - if r.Cached { - if err := os.Remove(r.CachePath); err != nil && !os.IsNotExist(err) { - return err - } - r.CachePath = "" - r.Cached = false - } - return nil + r.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. diff --git a/internal/helm/repository/chart_repository_test.go b/internal/helm/repository/chart_repository_test.go index 4023345bd..c0947a69a 100644 --- a/internal/helm/repository/chart_repository_test.go +++ b/internal/helm/repository/chart_repository_test.go @@ -18,20 +18,22 @@ package repository import ( "bytes" - "crypto/sha256" + "errors" "fmt" "net/url" "os" "path/filepath" + "sync" "testing" "time" - "github.com/fluxcd/source-controller/internal/cache" - "github.com/fluxcd/source-controller/internal/helm" . "github.com/onsi/gomega" + digestlib "github.com/opencontainers/go-digest" "helm.sh/helm/v3/pkg/chart" helmgetter "helm.sh/helm/v3/pkg/getter" "helm.sh/helm/v3/pkg/repo" + + "github.com/fluxcd/source-controller/internal/helm" ) var now = time.Now() @@ -55,6 +57,136 @@ func (g *mockGetter) Get(u string, _ ...helmgetter.Option) (*bytes.Buffer, error return bytes.NewBuffer(r), nil } +// Index load tests are derived from https://github.com/helm/helm/blob/v3.3.4/pkg/repo/index_test.go#L108 +// to ensure parity with Helm behaviour. +func TestIndexFromFile(t *testing.T) { + g := NewWithT(t) + + // Create an index file that exceeds the max index size. + tmpDir := t.TempDir() + bigIndexFile := filepath.Join(tmpDir, "index.yaml") + data := make([]byte, helm.MaxIndexSize+10) + g.Expect(os.WriteFile(bigIndexFile, data, 0o640)).ToNot(HaveOccurred()) + + tests := []struct { + name string + filename string + wantErr string + }{ + { + name: "regular index file", + filename: testFile, + }, + { + name: "chartmuseum index file", + filename: chartmuseumTestFile, + }, + { + name: "error if index size exceeds max size", + filename: bigIndexFile, + wantErr: "exceeds the maximum index file size", + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + i, err := IndexFromFile(tt.filename) + if tt.wantErr != "" { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.wantErr)) + return + } + + g.Expect(err).ToNot(HaveOccurred()) + + verifyLocalIndex(t, i) + }) + } +} + +func TestIndexFromBytes(t *testing.T) { + tests := []struct { + name string + b []byte + wantName string + wantVersion string + wantDigest string + wantErr string + }{ + { + name: "index", + b: []byte(` +apiVersion: v1 +entries: + nginx: + - urls: + - https://kubernetes-charts.storage.googleapis.com/nginx-0.2.0.tgz + name: nginx + description: string + version: 0.2.0 + home: https://github.com/something/else + digest: "sha256:1234567890abcdef" +`), + wantName: "nginx", + wantVersion: "0.2.0", + wantDigest: "sha256:1234567890abcdef", + }, + { + name: "index without API version", + b: []byte(`entries: + nginx: + - name: nginx`), + wantErr: "no API version specified", + }, + { + name: "index with duplicate entry", + b: []byte(`apiVersion: v1 +entries: + nginx: + - name: nginx" + nginx: + - name: nginx`), + wantErr: "key \"nginx\" already set in map", + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + t.Parallel() + + i, err := IndexFromBytes(tt.b) + if tt.wantErr != "" { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.wantErr)) + g.Expect(i).To(BeNil()) + return + } + + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(i).ToNot(BeNil()) + got, err := i.Get(tt.wantName, tt.wantVersion) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(got.Digest).To(Equal(tt.wantDigest)) + }) + } +} + +func TestIndexFromBytes_Unordered(t *testing.T) { + b, err := os.ReadFile(unorderedTestFile) + if err != nil { + t.Fatal(err) + } + i, err := IndexFromBytes(b) + if err != nil { + t.Fatal(err) + } + verifyLocalIndex(t, i) +} + func TestNewChartRepository(t *testing.T) { repositoryURL := "https://example.com" providers := helmgetter.Providers{ @@ -68,7 +200,7 @@ func TestNewChartRepository(t *testing.T) { t.Run("should construct chart repository", func(t *testing.T) { g := NewWithT(t) - r, err := NewChartRepository(repositoryURL, "", providers, nil, options) + r, err := NewChartRepository(repositoryURL, "", providers, nil, options...) g.Expect(err).ToNot(HaveOccurred()) g.Expect(r).ToNot(BeNil()) g.Expect(r.URL).To(Equal(repositoryURL)) @@ -95,7 +227,7 @@ func TestNewChartRepository(t *testing.T) { }) } -func TestChartRepository_Get(t *testing.T) { +func TestChartRepository_GetChartVersion(t *testing.T) { g := NewWithT(t) r := newChartRepository() @@ -252,6 +384,31 @@ func TestChartRepository_DownloadChart(t *testing.T) { } } +func TestChartRepository_CacheIndex(t *testing.T) { + g := NewWithT(t) + + mg := mockGetter{Response: []byte("foo")} + + r := newChartRepository() + r.URL = "https://example.com" + r.Client = &mg + r.revisions["key"] = "value" + r.digests["key"] = "value" + + err := r.CacheIndex() + g.Expect(err).To(Not(HaveOccurred())) + + g.Expect(r.Path).ToNot(BeEmpty()) + t.Cleanup(func() { _ = os.Remove(r.Path) }) + + g.Expect(r.Path).To(BeARegularFile()) + b, _ := os.ReadFile(r.Path) + g.Expect(b).To(Equal(mg.Response)) + + g.Expect(r.revisions).To(BeEmpty()) + g.Expect(r.digests).To(BeEmpty()) +} + func TestChartRepository_DownloadIndex(t *testing.T) { g := NewWithT(t) @@ -260,8 +417,9 @@ func TestChartRepository_DownloadIndex(t *testing.T) { mg := mockGetter{Response: b} r := &ChartRepository{ - URL: "https://example.com", - Client: &mg, + URL: "https://example.com", + Client: &mg, + RWMutex: &sync.RWMutex{}, } buf := bytes.NewBuffer([]byte{}) @@ -271,258 +429,166 @@ func TestChartRepository_DownloadIndex(t *testing.T) { g.Expect(err).To(BeNil()) } -func TestChartRepository_LoadIndexFromBytes(t *testing.T) { - tests := []struct { - name string - b []byte - wantName string - wantVersion string - wantDigest string - wantErr string - }{ - { - name: "index", - b: []byte(` -apiVersion: v1 -entries: - nginx: - - urls: - - https://kubernetes-charts.storage.googleapis.com/nginx-0.2.0.tgz - name: nginx - description: string - version: 0.2.0 - home: https://github.com/something/else - digest: "sha256:1234567890abcdef" -`), - wantName: "nginx", - wantVersion: "0.2.0", - wantDigest: "sha256:1234567890abcdef", - }, - { - name: "index without API version", - b: []byte(`entries: - nginx: - - name: nginx`), - wantErr: "no API version specified", - }, - { - name: "index with duplicate entry", - b: []byte(`apiVersion: v1 -entries: - nginx: - - name: nginx" - nginx: - - name: nginx`), - wantErr: "key \"nginx\" already set in map", - }, - } - for _, tt := range tests { - tt := tt - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - t.Parallel() +func TestChartRepository_StrategicallyLoadIndex(t *testing.T) { + t.Run("loads from path", func(t *testing.T) { + g := NewWithT(t) - r := newChartRepository() - err := r.LoadIndexFromBytes(tt.b) - if tt.wantErr != "" { - g.Expect(err).To(HaveOccurred()) - g.Expect(err.Error()).To(ContainSubstring(tt.wantErr)) - g.Expect(r.Index).To(BeNil()) - return - } + i := filepath.Join(t.TempDir(), "index.yaml") + g.Expect(os.WriteFile(i, []byte(`apiVersion: v1`), 0o644)).To(Succeed()) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(r.Index).ToNot(BeNil()) - got, err := r.Index.Get(tt.wantName, tt.wantVersion) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(got.Digest).To(Equal(tt.wantDigest)) + r := newChartRepository() + r.Path = i + + err := r.StrategicallyLoadIndex() + g.Expect(err).To(Succeed()) + g.Expect(r.Index).ToNot(BeNil()) + }) + + t.Run("loads from client", func(t *testing.T) { + g := NewWithT(t) + + r := newChartRepository() + r.Client = &mockGetter{ + Response: []byte(`apiVersion: v1`), + } + t.Cleanup(func() { + _ = os.Remove(r.Path) }) - } -} -func TestChartRepository_LoadIndexFromBytes_Unordered(t *testing.T) { - b, err := os.ReadFile(unorderedTestFile) - if err != nil { - t.Fatal(err) - } - r := newChartRepository() - err = r.LoadIndexFromBytes(b) - if err != nil { - t.Fatal(err) - } - verifyLocalIndex(t, r.Index) + err := r.StrategicallyLoadIndex() + g.Expect(err).To(Succeed()) + g.Expect(r.Path).ToNot(BeEmpty()) + g.Expect(r.Index).ToNot(BeNil()) + }) + + t.Run("skips if index is already loaded", func(t *testing.T) { + g := NewWithT(t) + + r := newChartRepository() + r.Index = repo.NewIndexFile() + + g.Expect(r.StrategicallyLoadIndex()).To(Succeed()) + }) } -// Index load tests are derived from https://github.com/helm/helm/blob/v3.3.4/pkg/repo/index_test.go#L108 -// to ensure parity with Helm behaviour. -func TestChartRepository_LoadIndexFromFile(t *testing.T) { - g := NewWithT(t) +func TestChartRepository_LoadFromPath(t *testing.T) { + t.Run("loads index", func(t *testing.T) { + g := NewWithT(t) - // Create an index file that exceeds the max index size. - tmpDir := t.TempDir() - bigIndexFile := filepath.Join(tmpDir, "index.yaml") - data := make([]byte, helm.MaxIndexSize+10) - g.Expect(os.WriteFile(bigIndexFile, data, 0o640)).ToNot(HaveOccurred()) + i := filepath.Join(t.TempDir(), "index.yaml") + g.Expect(os.WriteFile(i, []byte(`apiVersion: v1`), 0o644)).To(Succeed()) - tests := []struct { - name string - filename string - wantErr string - }{ - { - name: "regular index file", - filename: testFile, - }, - { - name: "chartmuseum index file", - filename: chartmuseumTestFile, - }, - { - name: "error if index size exceeds max size", - filename: bigIndexFile, - wantErr: "size of index 'index.yaml' exceeds", - }, - } + r := newChartRepository() + r.Path = i + r.revisions["key"] = "value" - for _, tt := range tests { - tt := tt - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) + g.Expect(r.LoadFromPath()).To(Succeed()) + g.Expect(r.Index).ToNot(BeNil()) + g.Expect(r.revisions).To(BeEmpty()) + }) - r := newChartRepository() - err := r.LoadFromFile(tt.filename) - if tt.wantErr != "" { - g.Expect(err).To(HaveOccurred()) - g.Expect(err.Error()).To(ContainSubstring(tt.wantErr)) - return - } + t.Run("no cache path", func(t *testing.T) { + g := NewWithT(t) - g.Expect(err).ToNot(HaveOccurred()) + err := newChartRepository().LoadFromPath() + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("no cache path")) + }) - verifyLocalIndex(t, r.Index) - }) - } + t.Run("index load error", func(t *testing.T) { + g := NewWithT(t) + + r := newChartRepository() + r.Path = filepath.Join(t.TempDir(), "index.yaml") + + err := r.LoadFromPath() + g.Expect(err).To(HaveOccurred()) + g.Expect(errors.Is(err, os.ErrNotExist)).To(BeTrue()) + }) } -func TestChartRepository_CacheIndex(t *testing.T) { - g := NewWithT(t) +func TestChartRepository_Revision(t *testing.T) { + t.Run("with algorithm", func(t *testing.T) { + r := newChartRepository() + r.Index = repo.NewIndexFile() - mg := mockGetter{Response: []byte("foo")} - expectSum := fmt.Sprintf("%x", sha256.Sum256(mg.Response)) + for _, algo := range []digestlib.Algorithm{digestlib.SHA256, digestlib.SHA512} { + t.Run(algo.String(), func(t *testing.T) { + g := NewWithT(t) - r := newChartRepository() - r.URL = "https://example.com" - r.Client = &mg + d := r.Revision(algo) + g.Expect(d).ToNot(BeEmpty()) + g.Expect(d.Algorithm()).To(Equal(algo)) + g.Expect(r.revisions[algo]).To(Equal(d)) + }) + } + }) - sum, err := r.CacheIndex() - g.Expect(err).To(Not(HaveOccurred())) + t.Run("without index", func(t *testing.T) { + g := NewWithT(t) - g.Expect(r.CachePath).ToNot(BeEmpty()) - defer os.RemoveAll(r.CachePath) - g.Expect(r.CachePath).To(BeARegularFile()) - b, _ := os.ReadFile(r.CachePath) + r := newChartRepository() + g.Expect(r.Revision(digestlib.SHA256)).To(BeEmpty()) + }) - g.Expect(b).To(Equal(mg.Response)) - g.Expect(sum).To(BeEquivalentTo(expectSum)) -} + t.Run("from cache", func(t *testing.T) { + g := NewWithT(t) -func TestChartRepository_StrategicallyLoadIndex(t *testing.T) { - g := NewWithT(t) + algo := digestlib.SHA256 + expect := digestlib.Digest("sha256:fake") - r := newChartRepository() - r.Index = repo.NewIndexFile() - g.Expect(r.StrategicallyLoadIndex()).To(Succeed()) - g.Expect(r.CachePath).To(BeEmpty()) - g.Expect(r.Cached).To(BeFalse()) - - r.Index = nil - r.CachePath = "/invalid/cache/index/path.yaml" - err := r.StrategicallyLoadIndex() - g.Expect(err).To(HaveOccurred()) - g.Expect(err.Error()).To(ContainSubstring("/invalid/cache/index/path.yaml: no such file or directory")) - g.Expect(r.Cached).To(BeFalse()) - - r.CachePath = "" - r.Client = &mockGetter{} - err = r.StrategicallyLoadIndex() - g.Expect(err).To(HaveOccurred()) - g.Expect(err.Error()).To(ContainSubstring("no API version specified")) - g.Expect(r.Cached).To(BeTrue()) - g.Expect(r.RemoveCache()).To(Succeed()) + r := newChartRepository() + r.Index = repo.NewIndexFile() + r.revisions[algo] = expect + + g.Expect(r.Revision(algo)).To(Equal(expect)) + }) } -func TestChartRepository_CacheIndexInMemory(t *testing.T) { - g := NewWithT(t) +func TestChartRepository_Digest(t *testing.T) { + t.Run("with algorithm", func(t *testing.T) { + g := NewWithT(t) - interval, _ := time.ParseDuration("5s") - memCache := cache.New(1, interval) - indexPath := "/multi-tenent-safe/mock/index.yaml" - r := newChartRepository() - r.Index = repo.NewIndexFile() - indexFile := *r.Index - g.Expect( - indexFile.MustAdd( - &chart.Metadata{ - Name: "grafana", - Version: "6.17.4", - }, - "grafana-6.17.4.tgz", - "http://example.com/charts", - "sha256:1234567890abc", - )).To(Succeed()) - indexFile.WriteFile(indexPath, 0o640) - ttl, _ := time.ParseDuration("1m") - r.SetMemCache(indexPath, memCache, ttl, func(event string) { - fmt.Println(event) + p := filepath.Join(t.TempDir(), "index.yaml") + g.Expect(repo.NewIndexFile().WriteFile(p, 0o644)).To(Succeed()) + + r := newChartRepository() + r.Path = p + + for _, algo := range []digestlib.Algorithm{digestlib.SHA256, digestlib.SHA512} { + t.Run(algo.String(), func(t *testing.T) { + g := NewWithT(t) + + d := r.Digest(algo) + g.Expect(d).ToNot(BeEmpty()) + g.Expect(d.Algorithm()).To(Equal(algo)) + g.Expect(r.digests[algo]).To(Equal(d)) + }) + } }) - r.CacheIndexInMemory() - _, cacheHit := r.IndexCache.Get(indexPath) - g.Expect(cacheHit).To(Equal(true)) - r.Unload() - g.Expect(r.Index).To(BeNil()) - g.Expect(r.StrategicallyLoadIndex()).To(Succeed()) - g.Expect(r.Index.Entries["grafana"][0].Digest).To(Equal("sha256:1234567890abc")) -} -func TestChartRepository_LoadFromCache(t *testing.T) { - tests := []struct { - name string - cachePath string - wantErr string - }{ - { - name: "cache path", - cachePath: chartmuseumTestFile, - }, - { - name: "invalid cache path", - cachePath: "invalid", - wantErr: "stat invalid: no such file", - }, - { - name: "no cache path", - cachePath: "", - wantErr: "no cache path set", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) + t.Run("without path", func(t *testing.T) { + g := NewWithT(t) - r := newChartRepository() - r.CachePath = tt.cachePath - err := r.LoadFromCache() - if tt.wantErr != "" { - g.Expect(err).To(HaveOccurred()) - g.Expect(err.Error()).To(ContainSubstring(tt.wantErr)) - g.Expect(r.Index).To(BeNil()) - return - } + r := newChartRepository() + g.Expect(r.Digest(digestlib.SHA256)).To(BeEmpty()) + }) - g.Expect(err).ToNot(HaveOccurred()) - verifyLocalIndex(t, r.Index) - }) - } + t.Run("from cache", func(t *testing.T) { + g := NewWithT(t) + + algo := digestlib.SHA256 + expect := digestlib.Digest("sha256:fake") + + i := filepath.Join(t.TempDir(), "index.yaml") + g.Expect(os.WriteFile(i, []byte(`apiVersion: v1`), 0o644)).To(Succeed()) + + r := newChartRepository() + r.Path = i + r.digests[algo] = expect + + g.Expect(r.Digest(algo)).To(Equal(expect)) + }) } func TestChartRepository_HasIndex(t *testing.T) { @@ -534,23 +600,88 @@ func TestChartRepository_HasIndex(t *testing.T) { g.Expect(r.HasIndex()).To(BeTrue()) } -func TestChartRepository_HasCacheFile(t *testing.T) { +func TestChartRepository_HasFile(t *testing.T) { g := NewWithT(t) r := newChartRepository() - g.Expect(r.HasCacheFile()).To(BeFalse()) - r.CachePath = "foo" - g.Expect(r.HasCacheFile()).To(BeTrue()) + g.Expect(r.HasFile()).To(BeFalse()) + + i := filepath.Join(t.TempDir(), "index.yaml") + g.Expect(os.WriteFile(i, []byte(`apiVersion: v1`), 0o644)).To(Succeed()) + r.Path = i + g.Expect(r.HasFile()).To(BeTrue()) } -func TestChartRepository_UnloadIndex(t *testing.T) { +func TestChartRepository_Clear(t *testing.T) { + t.Run("without index", func(t *testing.T) { + g := NewWithT(t) + + r := newChartRepository() + g.Expect(r.Clear()).To(Succeed()) + }) + + t.Run("with index", func(t *testing.T) { + g := NewWithT(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) { + g := NewWithT(t) + + f, err := os.CreateTemp(t.TempDir(), "index-*.yaml") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(f.Close()).To(Succeed()) + + r := newChartRepository() + 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()) + }) + + t.Run("with path", func(t *testing.T) { + g := NewWithT(t) + + f, err := os.CreateTemp(t.TempDir(), "index-*.yaml") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(f.Close()).To(Succeed()) + + r := newChartRepository() + r.Path = f.Name() + r.digests["key"] = "value" + + g.Expect(r.Clear()).To(Succeed()) + g.Expect(r.Path).ToNot(BeEmpty()) + g.Expect(r.Path).To(BeARegularFile()) + g.Expect(r.digests).To(BeEmpty()) + }) +} + +func TestChartRepository_Invalidate(t *testing.T) { g := NewWithT(t) r := newChartRepository() - g.Expect(r.HasIndex()).To(BeFalse()) - r.Index = repo.NewIndexFile() - r.Unload() - g.Expect(r.Index).To(BeNil()) + 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) { @@ -622,27 +753,3 @@ func verifyLocalIndex(t *testing.T, i *repo.IndexFile) { g.Expect(tt.Keywords).To(ContainElements(expect.Keywords)) } } - -func TestChartRepository_RemoveCache(t *testing.T) { - g := NewWithT(t) - - tmpFile, err := os.CreateTemp("", "remove-cache-") - g.Expect(err).ToNot(HaveOccurred()) - defer os.Remove(tmpFile.Name()) - - r := newChartRepository() - r.CachePath = tmpFile.Name() - r.Cached = true - - g.Expect(r.RemoveCache()).To(Succeed()) - g.Expect(r.CachePath).To(BeEmpty()) - g.Expect(r.Cached).To(BeFalse()) - g.Expect(tmpFile.Name()).ToNot(BeAnExistingFile()) - - r.CachePath = tmpFile.Name() - r.Cached = true - - g.Expect(r.RemoveCache()).To(Succeed()) - g.Expect(r.CachePath).To(BeEmpty()) - g.Expect(r.Cached).To(BeFalse()) -}