From c36dbfd4172752bbb3806f663a7f224133dab366 Mon Sep 17 00:00:00 2001 From: Samuel Archambault Date: Mon, 7 Sep 2026 04:50:25 -0400 Subject: [PATCH] fix: resolve OCI version constraints before deriving the shared chart cache path (#2768) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: resolve OCI version constraints before deriving the shared chart cache path When an OCI release uses a semver constraint (e.g. `~1`, `^2.0.0`, `*`), `getOCIChartPath` currently derives the on-disk cache directory from the raw constraint string via `safeVersionPath`, which substitutes constraint characters (`~`, `^`, `>`, `<`, `!`, `|`, `=`, ` `, `,`, `*`) with `_`. So `version: ~1` becomes `.../mychart/_1/` on disk. `acquireChartLock` then refuses to refresh anything under the shared cache dir to avoid race conditions between concurrent processes, so once the constraint is first resolved and written to `_1/`, every subsequent render on that machine (or that container replica) returns the pinned tarball regardless of newer matching tags being published. In multi-pod deployments like ArgoCD's argocd-repo-server this shows up as intermittent stale renders: different pods populate their caches at different moments and serve different snapshots of the same `~1` release forever. Fix: for OCI releases whose `version` looks like a constraint, run `helm show chart --version [flags]` and use the returned metadata.Version as the effective version for all downstream cache-key and path derivation. Helm already resolves the constraint against the registry and returns the concrete matching Chart.yaml. Callers get a content-addressable cache path (`.../mychart/1.0.1/`) that naturally invalidates when the constraint resolves to a new version. Exact-version releases and non-OCI releases skip the extra call. Adds an opt-out `resolveOCIVersions` field on `helmDefaults` and `ReleaseSpec` (both default true). If the resolution call fails transiently, the resolver logs a warning and falls back to the pre-fix behavior so a network hiccup doesn't break rendering. Adds `ShowChartWithFlags` to helmexec.Interface so the existing `ShowChart` API stays backwards compatible. Resolves #2766 Signed-off-by: Samuel Archambault * refactor: move ShowChartWithFlags to a ChartInspector capability interface Address Copilot review feedback on PR #2768: adding a method to the exported helmexec.Interface is a source-breaking change for every third-party implementation and mock of that interface, even though ShowChart itself stayed backward-compatible. Move ShowChartWithFlags off Interface and onto a new capability interface, helmexec.ChartInspector, following the same pattern used by the existing DependencyUpdater capability interface. The concrete execer and the exectest.Helm test stub still satisfy it (they already have the method); the OCI resolver in state.HelmState now type-asserts and falls back to the pre-fix caching behavior when the capability is absent, so downstream callers with their own helmexec.Interface implementations keep compiling untouched. Adds TestResolveOCIConstraintVersion_ChartInspectorFallback that exercises the type-assertion path with a helm value that satisfies Interface but deliberately does not satisfy ChartInspector. Reverts ShowChartWithFlags additions from testutil.noCallHelmExec and app_test.mockHelmExec since Interface no longer requires them. Signed-off-by: Samuel Archambault * fix: detect wildcard-segment semver constraints (1.x, 1.X) as constraints Address Copilot review feedback on PR #2768: the previous isVersionConstraint implementation scanned the input for operator characters (~, ^, >, <, !, |, =, space, comma, *). Masterminds/semver also accepts wildcard-segment constraints like "1.x", "1.X", "1.x.x", and "1.2.X" that contain no operator characters. Those would slip past the classifier, bypass OCI constraint resolution, and remain cached forever under the raw ".../mychart/1.x/" path — the same stale-cache bug the PR is meant to fix. Replace the character scan with a semver-parser-based check: a value is a constraint iff Masterminds/semver rejects it as a NewVersion but accepts it as a NewConstraint. This correctly: - Recognizes wildcard forms (1.x, 1.X, 1.x.x, 1.2.x, v1.x). - Preserves exact versions where "x" appears in prerelease metadata ("1.0.0-alpha.x") or build metadata ("1.0.0+x", "1.0.0+build.x.1") without misclassifying them, which a naive "add x to the scanned charset" fix would have gotten wrong. - Continues to classify values that are neither a version nor a constraint (empty string, "latest", junk) as non-constraints; helm handles those elsewhere. Removes the now-unused versionConstraintChars string constant. Expands TestIsVersionConstraint with 8 wildcard cases and 3 prerelease /build metadata cases containing "x", plus 2 non-parseable inputs. Adds a "wildcard segment constraint resolves to concrete version" subtest to TestResolveOCIConstraintVersion so the end-to-end pipeline is exercised for a version string that has no operator characters. Signed-off-by: Samuel Archambault * test: add getOCIChart integration test proving cache-path/pull-flag wiring Address Copilot review feedback on PR #2768. The existing unit test exercised resolveOCIConstraintVersion in isolation but did not prove that its output was propagated into the downstream cache key, cache path, and `helm chart pull --version` flag. Add a targeted integration test that: 1. Calls getOCIChart with a constraint release (`~1`) and a helm mock whose ShowChartWithFlags returns Chart.yaml version 1.0.1. 2. Asserts helm chart pull receives `--version 1.0.1`, not `~1`. 3. Asserts the destination path passed to helm chart pull contains the resolved-version segment (`/1.0.1/`) and does NOT contain the raw-constraint segment (`/_1/`). 4. Reads back the on-disk Chart.yaml under the cache path to confirm resolved version, path, and flag agree end to end. Add a second test that runs the same release twice with different resolver outputs (1.0.1, then 1.0.2 — simulating a newly published matching tag) and asserts the two resolutions land in distinct cache directories. This is the promise of the fix: once the raw constraint is out of the path, a new matching tag stops silently reusing the previously-resolved cache entry. The integration test flushed out a real correctness gap in the initial fix: getOCIChart resolved release.Version and chartVersion but did NOT recompute the qualified OCI ref that getOCIQualifiedChartName built pre-resolution. Helm was therefore receiving `oci:///:` alongside a `--version ` flag — at best redundant, at worst rejected by future Helm versions. Fixed by re-invoking getOCIQualifiedChartName on the mutated release copy so the embedded tag also carries the resolved value. Isolates the shared helmfile cache via `t.Setenv(HELMFILE_CACHE_HOME, t.TempDir())` so the OutputDirTemplate == "" code path (which writes into remote.CacheDir) does not touch the user's real `~/.cache/helmfile` during test runs. Signed-off-by: Samuel Archambault * refactor: flatten OCI constraint-resolution wiring in getOCIChart Address review feedback on PR #2768: - Extract the inline resolve/requalify block from getOCIChart into applyOCIConstraintResolution, keeping getOCIChart flat (guard-clause style) and making the resolution wiring independently testable. The helper returns the (possibly updated) release, qualified chart name, and chart version; every failure mode returns its inputs unchanged. - On a re-qualify failure after a successful resolution, fall back to the pre-fix behavior entirely (raw constraint in cache key, ref, AND --version flag) instead of the previous half-resolved mix (resolved version in the cache key, raw constraint in the path and flag), which could desynchronize the in-process cache key from the on-disk path. - Build the 'helm show chart' ref by reusing parseOCIChartRef instead of re-implementing its last-slash/last-colon tag-splitting inline. Same behavior for all realistic refs (registry ports preserved), and it also handles the digest suffix should one ever reach this point. - Drop --devel from the resolver flags: helm documents --devel as ignored whenever --version is set, and --version is always passed on this path. No behavior change intended beyond the requalify-failure fallback (which cannot realistically trigger) and the removal of the inert --devel flag. Signed-off-by: yxxhero * fix: classify partial semver versions (1, 1.2) as OCI constraints Address review feedback on PR #2768: Masterminds' lenient parser accepts partial versions like "1" or "1.2" as versions, so the previous classifier (NewVersion fails && NewConstraint succeeds) treated them as exact pins. But helm's OCI resolution — registry.GetTagMatchingVersionOrConstraint — honors a version string as an exact pin ONLY when a registry tag literally equals it; otherwise it parses the string as a constraint, and "1"/"1.2" float across 1.x.y/1.2.y tags. Caching those under their raw spelling reproduces the stale-cache bug of issue #2766, just with a narrower trigger. Replace the NewVersion probe with isFullSemver, which additionally requires the whole major.minor.patch triple to be spelled out (optional v prefix, prerelease, and build metadata all still count as exact when the core is fully qualified). When a registry does carry a literal tag equal to the version string, the resolver's metadata.Version == chartVersion path reports no change, so literal-tag pins keep today's behavior. TestIsVersionConstraint: "1"/"1.0" flip to constraints, joined by new v1.2/0/v1 cases and a 1.2.3 exact case. TestResolveOCIConstraintVersion gains a "partial version resolves" subtest. Docs updated to describe the parser-based classification instead of "constraint characters". Signed-off-by: yxxhero * fix: skip OCI constraint resolution under skipRefresh Address review feedback on PR #2768: the resolver ran even under --skip-refresh, so offline and cache-only workflows gained a 'helm show chart' registry attempt per constraint-versioned OCI release. It degraded gracefully (warn + fallback), but added registry-timeout latency and warning noise per release. skipOCIConstraintResolution now suppresses resolution when any of the skipRefresh levels is set — CLI --skip-refresh (forced), per-release skipRefresh, or helmDefaults.skipRefresh — with the same precedence the other skipRefresh consumers in prepareChartForRelease use. Skipped runs fall back to the constraint-keyed cache path, i.e. they reuse whatever a previous non-skipped run resolved, which is what 'skip checking for updates to cached charts' means for constraint versions. The existing issue #2766 integration tests flip their opts to SkipRefresh: false since they assert resolution happens. New coverage: TestSkipOCIConstraintResolution (tri-state precedence table) and TestGetOCIChart_SkipRefreshSkipsConstraintResolution (no inspector call, raw constraint in --version and cache path). Signed-off-by: yxxhero * perf: memoize OCI constraint resolution per chart+constraint Address review feedback on PR #2768: resolution ran before the in-process chart-cache fast path and was not memoized, so every constraint-versioned OCI release paid its own 'helm show chart' registry round-trip on every render — including N releases sharing the same chart+constraint, whose parallel workers could even resolve to different versions if the registry changed between their lookups. Memoize successful resolutions in resolvedOCIConstraints keyed by (chart ref, constraint), mirroring the downloadedCharts pattern: - Releases sharing a chart+constraint cost one round-trip per process and consistently use one resolved version per run. - Only successful resolutions are memoized; failures may be transient. - Flags are not part of the key: they govern TLS/verification/registry credentials, not which tag a constraint matches (--devel is already omitted as it is ignored whenever --version is set). - Concurrent misses may both hit the registry; last write wins, harmlessly. resetResolvedOCIConstraintsForTest is added alongside the existing resetChartCacheForTest and wired into the issue #2766 tests — notably ResolvesToDifferentVersionsPicksSeparateCachePaths, which reuses the same chart+constraint across its two runs and would otherwise be served the first resolution from the memo (which is exactly the intended per-process semantics). New coverage: TestResolveOCIConstraintVersion_Memoized (memo hit skips the registry, different constraint is a different key) and TestGetOCIChart_SharedConstraintResolvedOncePerProcess (two releases, one inspector call, one pull, same path). Signed-off-by: yxxhero * test: cover URL-embedded OCI constraint resolution Address review feedback on PR #2768: the existing integration tests only exercised the repo-aliased spelling (chart: myrepo/mychart, version: '~1') and the version-field spelling. The chart-URL spelling (chart: oci:///:~1) takes a different branch in getOCIQualifiedChartName — the URL version is deliberately NOT embedded into the qualified ref and flows through --version only — so its re-qualification after constraint resolution (release.Version mutated to the resolved value, versionInURL still the constraint) was untested. TestGetOCIChart_URLEmbeddedConstraintResolves asserts the resolver receives the URL-embedded constraint, helm chart pull receives the resolved version via --version with a tag-less ref, and the cache path carries the resolved version segment instead of the raw constraint. Also gofmt-aligns the test tables added in earlier commits and drops a redundant 1.2.3 test case that tripped goconst. Signed-off-by: yxxhero * docs: note empty-version OCI releases are unaffected by resolveOCIVersions Releases with no version: at all keep their pre-existing semantics: helm picks the latest tag at pull time and helmfile caches it under a version-less shared-cache path. Document the limitation alongside the other resolveOCIVersions scope notes. Signed-off-by: yxxhero --------- Signed-off-by: Samuel Archambault Signed-off-by: yxxhero Co-authored-by: Samuel Archambault Co-authored-by: yxxhero --- docs/configuration.md | 18 ++ pkg/exectest/helm.go | 16 ++ pkg/helmexec/exec.go | 23 +- pkg/helmexec/helmexec.go | 15 ++ pkg/state/issue_2766_test.go | 405 +++++++++++++++++++++++++++++++++++ pkg/state/state.go | 217 +++++++++++++++++++ pkg/state/state_test.go | 387 +++++++++++++++++++++++++++++++++ pkg/state/temp_test.go | 16 +- pkg/state/util.go | 74 +++++++ 9 files changed, 1158 insertions(+), 13 deletions(-) create mode 100644 pkg/state/issue_2766_test.go diff --git a/docs/configuration.md b/docs/configuration.md index 7532e734..70a20d81 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -129,6 +129,22 @@ helmDefaults: # When set to `true`, skips running `helm dep up` and `helm dep build` on this release's chart. # Useful when the chart is broken, like seen in https://github.com/roboll/helmfile/issues/1547 skipDeps: false + # When set to `true` (default), resolves an OCI chart's semver constraint + # (e.g. `~1`, `^2.0.0`, `*`, `1.x`, or any version that is not a + # fully-qualified `X.Y.Z` semver — including partial versions like `1` or + # `1.2`, which helm resolves as floating ranges) to a concrete registry tag + # before deriving the on-disk cache path under `$XDG_CACHE_HOME/helmfile`. + # This keeps the cache content-addressable so a newer matching tag is picked + # up on the next run instead of the previously-resolved (now stale) version. + # Set to `false` to preserve the pre-fix behavior of caching under the raw + # constraint string. Resolution is also skipped when skipRefresh is enabled + # (CLI `--skip-refresh`, per-release, or here): helmfile then reuses whatever + # a previous run resolved. Releases with no `version:` at all are also + # unaffected: helm picks the latest tag at pull time and helmfile caches it + # under a version-less path (same as before this setting existed). Exact + # `X.Y.Z` versions (e.g. `1.0.1`) and non-OCI releases are unaffected. See + # issue #2766. + resolveOCIVersions: true # If set to true, reuses the last release's values and merges them with ones provided in helmfile. # This attribute, can be overriden in CLI with --reset/reuse-values flag of apply/sync/diff subcommands reuseValues: false @@ -442,6 +458,7 @@ The following `helmDefaults` fields are also available but not shown in the exam | `enableDNS` | bool | false | Enable DNS lookups when rendering templates | | `skipCRDs` | bool | false | Skip CRDs during installation | | `skipRefresh` | bool | false | Skip running `helm dependency up` | +| `resolveOCIVersions` | bool | true | Resolve OCI semver constraints (e.g. `~1`, `^2.0.0`, `*`, `1.x`, and partial versions like `1.2` that helm treats as floating ranges) to concrete registry tags before deriving the shared cache path. Prevents stale cache hits after new matching tags are published. Set to `false` to keep the pre-fix behavior. Skipped when `skipRefresh` is enabled (CLI `--skip-refresh`, per-release, or `helmDefaults`). Only affects OCI releases with a non-exact version. See issue #2766 | | `atomic` | bool | false | Restore previous state on a failed install/upgrade. On Helm 4+ emits `--rollback-on-failure` (the successor to the deprecated `--atomic`); on older Helm emits `--atomic` | | `rollbackOnFailure` | bool | false | Restore previous state on a failed install/upgrade via the Helm 4 `--rollback-on-failure` flag. Requires Helm 4 or greater. Mutually exclusive with `atomic` | | `forceConflicts` | bool | false | Force server-side apply changes against conflicts (Helm 4 only) | @@ -475,6 +492,7 @@ The following per-release fields are also available: | `forceGoGetter` | bool | false | Force go-getter URL parsing for the chart field. Useful when go-getter URL parsing fails unexpectedly | | `forceNamespace` | string | | Force namespace on all K8s resources rendered by the chart, even when the template doesn't use `{{ .Namespace }}`. Use with caution | | `skipRefresh` | bool | false | Per-release skip for `helm dependency up` | +| `resolveOCIVersions` | bool | inherited | Per-release override of the `helmDefaults.resolveOCIVersions` setting | | `disableAutoDetectedKubeVersionForDiff` | bool | false | Disable auto-detected kubeVersion for helm diff on this release | | `takeOwnership` | bool | false | Take ownership of existing resources for this release | | `serverSide` | string | | Controls the helm 4 `--server-side` flag for this release. Must be `"true"`, `"false"`, or `"auto"` (Helm 4 only) | diff --git a/pkg/exectest/helm.go b/pkg/exectest/helm.go index 2c1ddeff..26a27bf3 100644 --- a/pkg/exectest/helm.go +++ b/pkg/exectest/helm.go @@ -50,6 +50,10 @@ type Helm struct { UpdateDepsCallbacks map[string]func(string) error + // ShowChartWithFlagsFunc lets tests stub `helm show chart --version `, + // which state.HelmState uses to resolve OCI constraint versions before caching. + ShowChartWithFlagsFunc func(chartPath string, flags ...string) (chart.Metadata, error) + DiffMutex *sync.Mutex ChartsMutex *sync.Mutex ReleasesMutex *sync.Mutex @@ -332,6 +336,18 @@ func (helm *Helm) ShowChart(chartPath string) (chart.Metadata, error) { } } +// ShowChartWithFlags mimics `helm show chart --version ` +// resolution used by state.HelmState to convert a version constraint to a +// concrete semver before caching. Test cases can inject a fake resolver via +// helm.ShowChartWithFlagsFunc; the default falls back to ShowChart to keep +// unrelated tests working. +func (helm *Helm) ShowChartWithFlags(chartPath string, flags ...string) (chart.Metadata, error) { + if helm.ShowChartWithFlagsFunc != nil { + return helm.ShowChartWithFlagsFunc(chartPath, flags...) + } + return helm.ShowChart(chartPath) +} + // IsHelm4Enabled detects the installed Helm version by executing the helm binary. // It returns true if Helm 4.x is installed, false for Helm 3.x or earlier. // Falls back to environment variable HELMFILE_HELM4 if helm binary is not available. diff --git a/pkg/helmexec/exec.go b/pkg/helmexec/exec.go index bbd0099c..e7995758 100644 --- a/pkg/helmexec/exec.go +++ b/pkg/helmexec/exec.go @@ -1348,15 +1348,24 @@ func resolveOciChart(ociChart string) (ociChartURL, ociChartTag string) { } func (helm *execer) ShowChart(chartPath string) (chart.Metadata, error) { - var helmArgs = []string{"show", "chart", chartPath} - out, error := helm.exec(helmArgs, map[string]string{}, nil) - if error != nil { - return chart.Metadata{}, error + return helm.ShowChartWithFlags(chartPath) +} + +// ShowChartWithFlags runs `helm show chart` and unmarshals the resulting +// Chart.yaml. Callers may pass additional helm flags (for example --version, +// --plain-http, --registry-config, --ca-file, --insecure-skip-tls-verify). +// When --version references a semver constraint, helm resolves it against the +// registry and returns the concrete matching Chart.yaml, so callers can read +// metadata.Version to obtain the resolved version. +func (helm *execer) ShowChartWithFlags(chartPath string, flags ...string) (chart.Metadata, error) { + helmArgs := append([]string{"show", "chart", chartPath}, flags...) + out, err := helm.exec(helmArgs, map[string]string{}, nil) + if err != nil { + return chart.Metadata{}, err } var metadata chart.Metadata - error = yaml.Unmarshal(out, &metadata) - if error != nil { - return chart.Metadata{}, error + if err := yaml.Unmarshal(out, &metadata); err != nil { + return chart.Metadata{}, err } return metadata, nil } diff --git a/pkg/helmexec/helmexec.go b/pkg/helmexec/helmexec.go index 2b1457c5..48d2b8a6 100644 --- a/pkg/helmexec/helmexec.go +++ b/pkg/helmexec/helmexec.go @@ -46,3 +46,18 @@ type DependencyUpdater interface { IsHelm3() bool IsHelm4() bool } + +// ChartInspector is a capability interface exposing `helm show chart` with +// arbitrary flags. It is implemented by helmexec's concrete execer and by +// stubs in tests. Callers should type-assert Interface to ChartInspector and +// fall back gracefully when the implementation does not satisfy it, so that +// third-party implementations of helmexec.Interface keep compiling. See +// state.HelmState.resolveOCIConstraintVersion for the usage pattern. +type ChartInspector interface { + // ShowChartWithFlags runs `helm show chart [flags...]` and returns + // the parsed Chart.yaml metadata. Unlike ShowChart, callers pass flags + // such as --version, --plain-http, or --registry-config so that + // constraint versions (e.g. "~1", "^2.0.0") can be resolved against an + // OCI registry to a concrete semver. + ShowChartWithFlags(chart string, flags ...string) (chart.Metadata, error) +} diff --git a/pkg/state/issue_2766_test.go b/pkg/state/issue_2766_test.go new file mode 100644 index 00000000..e569c307 --- /dev/null +++ b/pkg/state/issue_2766_test.go @@ -0,0 +1,405 @@ +package state + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "sync/atomic" + "testing" + + "github.com/stretchr/testify/require" + "go.uber.org/zap" + chart "helm.sh/helm/v4/pkg/chart/v2" + + "github.com/helmfile/helmfile/pkg/envvar" + "github.com/helmfile/helmfile/pkg/exectest" + "github.com/helmfile/helmfile/pkg/filesystem" +) + +// Test-local constants shared by the issue #2766 integration tests. +const ( + issue2766RepoName = "myrepo" + issue2766Chart = "myrepo/mychart" + issue2766Release = "app" + issue2766RepoURL = "registry.example.com/charts" + issue2766VersionArg = "--version" +) + +// mockOCIPullHelm wraps exectest.Helm so an integration test can observe the +// exact chart reference, --version flag, and destination path helmfile hands +// to `helm chart pull` after OCI constraint resolution. It also implements +// ShowChartWithFlags (the helmexec.ChartInspector capability) so +// resolveOCIConstraintVersion has a resolver to talk to. +type mockOCIPullHelm struct { + *exectest.Helm + // ResolveFn is invoked in place of a real registry round-trip; it + // receives the constraint version and returns the concrete resolved + // version. Tests can vary its behavior across successive invocations to + // simulate a newly published tag. + ResolveFn func(constraint string) (chart.Metadata, error) + + pullCount atomic.Int32 + pulledChart atomic.Value // string: last ref passed to ChartPull + pulledPath atomic.Value // string: last destination dir passed to ChartPull + pulledVersion atomic.Value // string: value of the --version flag on the last ChartPull + inspectorCalled atomic.Int32 +} + +func (m *mockOCIPullHelm) ShowChartWithFlags(chartPath string, flags ...string) (chart.Metadata, error) { + m.inspectorCalled.Add(1) + // Extract --version to prove the resolver passes the raw constraint down. + var constraint string + for i, f := range flags { + if f == issue2766VersionArg && i+1 < len(flags) { + constraint = flags[i+1] + break + } + } + if m.ResolveFn == nil { + return chart.Metadata{}, nil + } + return m.ResolveFn(constraint) +} + +func (m *mockOCIPullHelm) ChartPull(chartRef string, path string, flags ...string) error { + m.pullCount.Add(1) + m.pulledChart.Store(chartRef) + m.pulledPath.Store(path) + var version string + for i, f := range flags { + if f == issue2766VersionArg && i+1 < len(flags) { + version = flags[i+1] + break + } + } + m.pulledVersion.Store(version) + // Materialize a Chart.yaml so getOCIChart's findChartDirectory call + // downstream succeeds. exectest.Helm.ChartPull would do this too, but we + // hard-code the resolved version into the Chart.yaml here so that any + // future assertion on Chart.yaml contents can distinguish the versions. + chartDir := filepath.Join(path, "test-chart") + if err := os.MkdirAll(chartDir, 0755); err != nil { + return err + } + chartYaml := "apiVersion: v2\nname: test-chart\ndescription: A test chart\ntype: application\nversion: \"" + version + "\"\nappVersion: \"" + version + "\"\n" + return os.WriteFile(filepath.Join(chartDir, "Chart.yaml"), []byte(chartYaml), 0644) +} + +// TestGetOCIChart_ResolvesConstraintIntoCachePathAndPullFlag verifies the end +// to end wiring for issue #2766: given an OCI release with a constraint +// version like `~1`, getOCIChart must (a) call the ChartInspector to resolve +// the constraint, (b) use the resolved concrete version as the on-disk cache +// path segment (so the cache is content-addressable), and (c) pass the same +// resolved version to `helm chart pull --version`. The isolated resolver +// unit test only proves the helper works; this test proves the caller wires +// its output into every downstream consumer. +func TestGetOCIChart_ResolvesConstraintIntoCachePathAndPullFlag(t *testing.T) { + resetChartCacheForTest() + resetResolvedOCIConstraintsForTest() + + // Sandbox the shared helmfile cache dir so this test can safely exercise + // the `opts.OutputDirTemplate == ""` branch that writes into remote.CacheDir() + // without touching the user's real ~/.cache/helmfile. + t.Setenv(envvar.CacheHome, t.TempDir()) + + logger := zap.NewExample().Sugar() + st := &HelmState{ + ReleaseSetSpec: ReleaseSetSpec{ + Repositories: []RepositorySpec{ + {Name: issue2766RepoName, URL: issue2766RepoURL, OCI: true}, + }, + }, + logger: logger, + valsRuntime: valsRuntime, + fs: filesystem.DefaultFileSystem(), + } + + release := &ReleaseSpec{ + Name: issue2766Release, + Chart: issue2766Chart, + Version: "~1", + } + + const resolved = "1.0.1" + helm := &mockOCIPullHelm{ + Helm: &exectest.Helm{Helm3: true}, + ResolveFn: func(constraint string) (chart.Metadata, error) { + require.Equal(t, "~1", constraint, "resolver must receive the raw constraint from the release spec") + return chart.Metadata{Version: resolved}, nil + }, + } + + _, err := st.getOCIChart(release, "", helm, ChartPrepareOptions{ + SkipDeps: true, + }) + require.NoError(t, err) + + // 1. Resolver was invoked exactly once for the constraint. + require.Equal(t, int32(1), helm.inspectorCalled.Load(), + "ChartInspector.ShowChartWithFlags must be called once for a constraint version") + + // 2. helm chart pull was invoked once (no double-download regression). + require.Equal(t, int32(1), helm.pullCount.Load()) + + // 3. The --version flag on `helm chart pull` carries the RESOLVED value, + // not the raw constraint. Without the resolution wiring, this would + // still be "~1". + require.Equal(t, resolved, helm.pulledVersion.Load(), + "helm chart pull --version must receive the resolved version, not the raw constraint") + + // 4. The chart ref keeps its `:` suffix aligned with the resolved + // value (helmfile builds it as `/:`), so helm has + // no way to observe the raw constraint at any downstream layer. + pulledChart, _ := helm.pulledChart.Load().(string) + require.Contains(t, pulledChart, ":"+resolved, + "the OCI ref passed to helm chart pull must include the resolved version tag") + require.NotContains(t, pulledChart, ":~1", + "the OCI ref passed to helm chart pull must not include the raw constraint") + + // 5. The on-disk destination path is derived from the RESOLVED version. + // Prior to the fix this would be `.../mychart/_1/`; after the fix it + // should be `.../mychart/1.0.1/`. That path shape is what makes the + // shared cache content-addressable and self-invalidating. + pulledPath, _ := helm.pulledPath.Load().(string) + require.Contains(t, pulledPath, string(filepath.Separator)+resolved, + "cache destination path must contain the resolved version segment") + require.NotContains(t, pulledPath, string(filepath.Separator)+"_1", + "cache destination path must NOT contain the raw-constraint segment (_1)") + + // 6. The chart directory helmfile created for this release actually lives + // under the resolved-version cache path, and its Chart.yaml holds the + // resolved version — belt-and-suspenders against a path/flag mismatch. + writtenChartYaml := filepath.Join(pulledPath, "test-chart", "Chart.yaml") + body, err := os.ReadFile(writtenChartYaml) + require.NoError(t, err) + require.Contains(t, string(body), "version: \""+resolved+"\"") +} + +// TestGetOCIChart_ResolvesToDifferentVersionsPicksSeparateCachePaths verifies +// that when a constraint resolves to two different concrete versions across +// runs (e.g. `~1` first resolves to 1.0.1, then a newer 1.0.2 tag is +// published and matches the same constraint), each resolution writes to its +// own cache path. This is the core promise of the fix: once the raw +// constraint is out of the path, the shared cache stops silently serving +// stale content when the registry gets a new matching tag. +func TestGetOCIChart_ResolvesToDifferentVersionsPicksSeparateCachePaths(t *testing.T) { + resetChartCacheForTest() + resetResolvedOCIConstraintsForTest() + t.Setenv(envvar.CacheHome, t.TempDir()) + + logger := zap.NewExample().Sugar() + st := &HelmState{ + ReleaseSetSpec: ReleaseSetSpec{ + Repositories: []RepositorySpec{ + {Name: issue2766RepoName, URL: issue2766RepoURL, OCI: true}, + }, + }, + logger: logger, + valsRuntime: valsRuntime, + fs: filesystem.DefaultFileSystem(), + } + + // First resolution: `~1` -> 1.0.1 + release := &ReleaseSpec{Name: issue2766Release, Chart: issue2766Chart, Version: "~1"} + first := &mockOCIPullHelm{ + Helm: &exectest.Helm{Helm3: true}, + ResolveFn: func(_ string) (chart.Metadata, error) { + return chart.Metadata{Version: "1.0.1"}, nil + }, + } + _, err := st.getOCIChart(release, "", first, ChartPrepareOptions{SkipDeps: true}) + require.NoError(t, err) + firstPath, _ := first.pulledPath.Load().(string) + require.Contains(t, firstPath, string(filepath.Separator)+"1.0.1") + + // Simulate the registry publishing 1.0.2 and start over with a fresh + // process-local cache (resetChartCacheForTest + the resolution memo). The + // constraint is unchanged; only what it resolves to has moved. Second + // resolution: `~1` -> 1.0.2 must land in a distinct cache dir, not reuse + // `1.0.1`. + resetChartCacheForTest() + resetResolvedOCIConstraintsForTest() + release = &ReleaseSpec{Name: issue2766Release, Chart: issue2766Chart, Version: "~1"} + second := &mockOCIPullHelm{ + Helm: &exectest.Helm{Helm3: true}, + ResolveFn: func(_ string) (chart.Metadata, error) { + return chart.Metadata{Version: "1.0.2"}, nil + }, + } + _, err = st.getOCIChart(release, "", second, ChartPrepareOptions{SkipDeps: true}) + require.NoError(t, err) + secondPath, _ := second.pulledPath.Load().(string) + require.Contains(t, secondPath, string(filepath.Separator)+"1.0.2", + "the second resolution must use its own resolved-version cache path") + require.NotEqual(t, firstPath, secondPath, + "newly-resolved version must NOT reuse the previously-resolved cache directory") + // Neither run should have polluted the other with a raw-constraint segment. + require.False(t, strings.Contains(firstPath, "_1") || strings.Contains(secondPath, "_1"), + "no cache path should carry the raw `_1` constraint segment after resolution") +} + +// TestGetOCIChart_SkipRefreshSkipsConstraintResolution verifies that +// --skip-refresh (or its per-release / helmDefaults equivalents) suppresses +// the `helm show chart` resolution round-trip: no network attempt is made, and +// the chart is cached under the raw constraint path (pre-fix behavior), which +// reuses whatever a previous, non-skipped run resolved. +func TestGetOCIChart_SkipRefreshSkipsConstraintResolution(t *testing.T) { + resetChartCacheForTest() + resetResolvedOCIConstraintsForTest() + + logger := zap.NewExample().Sugar() + st := &HelmState{ + ReleaseSetSpec: ReleaseSetSpec{ + Repositories: []RepositorySpec{ + {Name: issue2766RepoName, URL: issue2766RepoURL, OCI: true}, + }, + }, + logger: logger, + valsRuntime: valsRuntime, + fs: filesystem.DefaultFileSystem(), + } + + // release-level skipRefresh unset / false / true — the CLI flag forces + // skipping in all three cases. + falseVal, trueVal := false, true + releases := []*ReleaseSpec{ + {Name: issue2766Release, Chart: issue2766Chart, Version: "~1"}, + {Name: issue2766Release, Chart: issue2766Chart, Version: "~1", SkipRefresh: &falseVal}, + {Name: issue2766Release, Chart: issue2766Chart, Version: "~1", SkipRefresh: &trueVal}, + } + for i, release := range releases { + t.Run(fmt.Sprintf("release %d", i), func(t *testing.T) { + resetChartCacheForTest() + // Fresh cache dir per subtest: a leftover `_1` directory from a + // previous subtest would satisfy the shared-cache path and skip the + // pull entirely. + t.Setenv(envvar.CacheHome, t.TempDir()) + helm := &mockOCIPullHelm{ + Helm: &exectest.Helm{Helm3: true}, + ResolveFn: func(constraint string) (chart.Metadata, error) { + t.Error("resolver must not be called under skipRefresh") + return chart.Metadata{}, nil + }, + } + _, err := st.getOCIChart(release, "", helm, ChartPrepareOptions{ + SkipRefresh: true, // CLI --skip-refresh + SkipDeps: true, + }) + require.NoError(t, err) + require.Equal(t, int32(0), helm.inspectorCalled.Load(), + "no `helm show chart` call may happen under skipRefresh") + require.Equal(t, int32(1), helm.pullCount.Load()) + require.Equal(t, "~1", helm.pulledVersion.Load(), + "helm chart pull must receive the raw constraint when resolution is skipped") + path, _ := helm.pulledPath.Load().(string) + require.Contains(t, path, string(filepath.Separator)+"_1", + "cache path falls back to the raw-constraint segment under skipRefresh") + }) + } +} + +// TestGetOCIChart_SharedConstraintResolvedOncePerProcess verifies that two +// releases sharing the same OCI chart+constraint trigger exactly one +// `helm show chart` resolution and one `helm chart pull` per process, and both +// see the same resolved chart — even if the registry would answer the two +// lookups differently (a tag published between them). Without the memo, each +// release resolved independently and paid its own registry round-trip. +func TestGetOCIChart_SharedConstraintResolvedOncePerProcess(t *testing.T) { + resetChartCacheForTest() + resetResolvedOCIConstraintsForTest() + t.Setenv(envvar.CacheHome, t.TempDir()) + + logger := zap.NewExample().Sugar() + st := &HelmState{ + ReleaseSetSpec: ReleaseSetSpec{ + Repositories: []RepositorySpec{ + {Name: issue2766RepoName, URL: issue2766RepoURL, OCI: true}, + }, + }, + logger: logger, + valsRuntime: valsRuntime, + fs: filesystem.DefaultFileSystem(), + } + + const resolved = "1.0.1" + helm := &mockOCIPullHelm{ + Helm: &exectest.Helm{Helm3: true}, + ResolveFn: func(_ string) (chart.Metadata, error) { + return chart.Metadata{Version: resolved}, nil + }, + } + + path1, err := st.getOCIChart(&ReleaseSpec{Name: "app-a", Chart: issue2766Chart, Version: "~1"}, "", helm, ChartPrepareOptions{SkipDeps: true}) + require.NoError(t, err) + path2, err := st.getOCIChart(&ReleaseSpec{Name: "app-b", Chart: issue2766Chart, Version: "~1"}, "", helm, ChartPrepareOptions{SkipDeps: true}) + require.NoError(t, err) + + require.Equal(t, int32(1), helm.inspectorCalled.Load(), + "one `helm show chart` per chart+constraint per process") + require.Equal(t, int32(1), helm.pullCount.Load(), + "the second release must reuse the first release's downloaded chart") + require.NotNil(t, path1) + require.NotNil(t, path2) + require.Equal(t, *path1, *path2, "both releases must see the same resolved chart") + path, _ := helm.pulledPath.Load().(string) + require.Contains(t, path, string(filepath.Separator)+resolved, + "the single download must land under the resolved-version cache path") +} + +// TestGetOCIChart_URLEmbeddedConstraintResolves covers the `chart: +// oci:///:` spelling, where the constraint lives +// in the chart URL instead of the version field. getOCIQualifiedChartName +// deliberately does NOT embed that URL version into the qualified ref (it +// flows through --version only), so after resolution the ref must stay +// tag-less while --version and the cache path carry the resolved version — +// the third branch of the re-qualification in applyOCIConstraintResolution. +func TestGetOCIChart_URLEmbeddedConstraintResolves(t *testing.T) { + resetChartCacheForTest() + resetResolvedOCIConstraintsForTest() + t.Setenv(envvar.CacheHome, t.TempDir()) + + logger := zap.NewExample().Sugar() + st := &HelmState{ + logger: logger, + valsRuntime: valsRuntime, + fs: filesystem.DefaultFileSystem(), + } + + const ( + base = "registry.example.com/charts/mychart" + chartURL = "oci://" + base + ":~1" + resolved = "1.0.1" + ) + release := &ReleaseSpec{Name: issue2766Release, Chart: chartURL} + + helm := &mockOCIPullHelm{ + Helm: &exectest.Helm{Helm3: true}, + ResolveFn: func(constraint string) (chart.Metadata, error) { + require.Equal(t, "~1", constraint, "resolver must receive the URL-embedded constraint") + return chart.Metadata{Version: resolved}, nil + }, + } + + _, err := st.getOCIChart(release, "", helm, ChartPrepareOptions{SkipDeps: true}) + require.NoError(t, err) + + require.Equal(t, int32(1), helm.inspectorCalled.Load()) + require.Equal(t, int32(1), helm.pullCount.Load()) + require.Equal(t, resolved, helm.pulledVersion.Load(), + "helm chart pull --version must receive the resolved version") + + // The URL-embedded constraint must NOT reappear as a ref tag; the + // resolved version flows through --version alone. + pulledChart, _ := helm.pulledChart.Load().(string) + require.Equal(t, base, pulledChart, + "the qualified ref stays tag-less when the constraint came from the chart URL") + require.NotContains(t, pulledChart, ":~1") + require.NotContains(t, pulledChart, ":"+resolved) + + path, _ := helm.pulledPath.Load().(string) + require.Contains(t, path, string(filepath.Separator)+resolved, + "cache destination path must contain the resolved version segment") + require.NotContains(t, path, string(filepath.Separator)+"_1", + "cache destination path must NOT contain the raw-constraint segment") +} diff --git a/pkg/state/state.go b/pkg/state/state.go index 36d34afc..85a9c4ec 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -260,6 +260,16 @@ type HelmSpec struct { SkipDeps bool `yaml:"skipDeps"` // SkipRefresh disables running `helm dependency up` SkipRefresh bool `yaml:"skipRefresh"` + // ResolveOCIVersions, when set (default true), resolves an OCI chart's semver + // constraint version (e.g. "~1", "^2.0.0", "*") to a concrete registry tag + // before helmfile derives the on-disk cache path. This makes the shared chart + // cache under $XDG_CACHE_HOME/helmfile content-addressable by resolved version + // so that a newer tag matching the same constraint is picked up on the next + // invocation instead of returning a stale, previously-resolved version. Set + // to false to preserve the pre-fix behavior of caching under the raw + // constraint string. Exact-version releases (no constraint characters in + // `version:`) are unaffected. Ignored for non-OCI releases. + ResolveOCIVersions *bool `yaml:"resolveOCIVersions,omitempty"` // on helm upgrade/diff, reuse values currently set in the release and merge them with the ones defined within helmfile ReuseValues bool `yaml:"reuseValues"` // Propagate '--post-renderer' to helmv3 template and helm install @@ -495,6 +505,10 @@ type ReleaseSpec struct { // SkipRefresh disables running `helm dependency up` SkipRefresh *bool `yaml:"skipRefresh,omitempty"` + // ResolveOCIVersions overrides helmDefaults.resolveOCIVersions for this release. + // See HelmSpec.ResolveOCIVersions for details. + ResolveOCIVersions *bool `yaml:"resolveOCIVersions,omitempty"` + // Propagate '--post-renderer' to helmv3 template and helm install PostRenderer *string `yaml:"postRenderer,omitempty"` @@ -5822,6 +5836,31 @@ func resetChartCacheForTest() { downloadedCharts = make(map[ChartCacheKey]string) } +// ociConstraintKey identifies one OCI constraint-resolution lookup: the +// registry-qualified chart reference (without oci:// prefix, version tag, or +// digest) plus the constraint string. +type ociConstraintKey struct { + chartRef string + constraint string +} + +// resolvedOCIConstraints memoizes constraint -> concrete-version resolutions +// for the lifetime of the process so that releases sharing the same OCI +// chart+constraint trigger at most one `helm show chart` registry round-trip +// per run, and consistently use one resolved version per chart+constraint +// even if the registry would answer two concurrent workers differently. Only +// successful resolutions are memoized — failures may be transient. Mirrors the +// downloadedCharts pattern above. +var resolvedOCIConstraints = make(map[ociConstraintKey]string) +var resolvedOCIConstraintsMutex sync.RWMutex + +// resetResolvedOCIConstraintsForTest clears the resolution memo. For testing only. +func resetResolvedOCIConstraintsForTest() { + resolvedOCIConstraintsMutex.Lock() + defer resolvedOCIConstraintsMutex.Unlock() + resolvedOCIConstraints = make(map[ociConstraintKey]string) +} + // isSharedCachePath returns true if the chartPath is within the shared cache directory. // Charts in the shared cache should not be deleted during refresh to prevent race conditions // when multiple processes are using the same cached chart. @@ -6093,6 +6132,13 @@ func (st *HelmState) getOCIChart(release *ReleaseSpec, tempDir string, helm helm return nil, nil } + // Resolve a semver constraint (e.g. "~1", "^2.0.0") to a concrete registry + // tag BEFORE deriving the on-disk cache key. Without this step the cache + // path is a function of the raw constraint string (safeVersionPath("~1") + // => "_1"), so a stale first-resolution is served indefinitely even after + // the registry publishes a newer matching tag. See issue #2766. + release, qualifiedChartName, chartVersion = st.applyOCIConstraintResolution(release, qualifiedChartName, chartVersion, helm, opts) + cacheKey := st.getChartCacheKey(release) // Fast path: check in-process cache without acquiring any lock. @@ -6386,3 +6432,174 @@ func (st *HelmState) getOCIChartPath(tempDir string, release *ReleaseSpec, chart pathElems = append(pathElems, safeVersionPath(chartVersion)) return filepath.Join(pathElems...), nil } + +// applyOCIConstraintResolution resolves a semver constraint version (e.g. +// "~1", "^2.0.0") for an OCI release to a concrete registry tag and returns +// the (possibly updated) release, qualified chart name, and chart version for +// the downstream cache-key, cache-path, and `--version` derivation in +// getOCIChart. +// +// It is a no-op — returning its inputs unchanged — when resolution is skipped +// (see skipOCIConstraintResolution), opted out, when the version is not a +// constraint, or when resolution fails. Every failure mode therefore degrades +// to the pre-fix constraint-keyed caching behavior instead of failing the +// render. See issue #2766. +func (st *HelmState) applyOCIConstraintResolution(release *ReleaseSpec, qualifiedChartName, chartVersion string, helm helmexec.Interface, opts ChartPrepareOptions) (*ReleaseSpec, string, string) { + if st.skipOCIConstraintResolution(release, opts) { + return release, qualifiedChartName, chartVersion + } + resolved, changed, err := st.resolveOCIConstraintVersion(release, helm, qualifiedChartName, chartVersion) + if err != nil { + st.logger.Warnf("resolving OCI version constraint %q for release %q failed: %v; falling back to unresolved constraint for cache key (a stale cache may be served)", chartVersion, release.Name, err) + return release, qualifiedChartName, chartVersion + } + if !changed { + return release, qualifiedChartName, chartVersion + } + st.logger.Debugf("resolved OCI version constraint %q for release %q to %q", chartVersion, release.Name, resolved) + + // Rewrite the release copy so the downstream cache key, path template, + // and --version flag all agree on the resolved value. + releaseCopy := *release + releaseCopy.Version = resolved + resolvedRelease := &releaseCopy + + // Recompute the qualified chart ref so its embedded `:` tag + // (added by getOCIQualifiedChartName when version came from the + // `version:` field) also carries the resolved value; otherwise + // `helm chart pull` would receive `/:` alongside + // a `--version ` flag, which is at best redundant and at worst + // rejected by future Helm versions. + requalified, _, requalifiedVersion, requalifyErr := st.getOCIQualifiedChartName(resolvedRelease) + if requalifyErr != nil { + // Should not happen: the release already parsed once above with the + // raw constraint. Fall back to the pre-fix behavior entirely (raw + // constraint in the cache key, ref, and --version flag) rather than + // a half-resolved mix of the two. + st.logger.Warnf("re-qualifying OCI chart name for release %q after version resolution failed: %v; using pre-resolution values", release.Name, requalifyErr) + return release, qualifiedChartName, chartVersion + } + return resolvedRelease, requalified, requalifiedVersion +} + +// resolveOCIConstraintVersion resolves a semver constraint version (e.g. "~1", +// "^2.0", "*") for an OCI release to the concrete registry tag that helm would +// download. It runs `helm show chart --version [flags]` +// which returns the resolved chart's Chart.yaml; the returned Version is the +// concrete tag helm picked. The returned bool indicates whether the effective +// version actually changed (false when the input was already a pinned semver, +// resolution is opted out, the release is not OCI-backed, or the helm +// implementation does not expose the ShowChartWithFlags capability). +// Successful resolutions are memoized per chart+constraint for the lifetime +// of the process, so repeated lookups cost no additional registry round-trips. +// +// This is a helper for getOCIChart. Callers should tolerate errors: a failed +// resolution shouldn't break rendering; it just falls back to the pre-fix +// caching behavior (cache path derived from the raw constraint). +func (st *HelmState) resolveOCIConstraintVersion(release *ReleaseSpec, helm helmexec.Interface, qualifiedChartName, chartVersion string) (string, bool, error) { + if !st.resolveOCIVersionsEnabled(release) { + return chartVersion, false, nil + } + // Nothing to resolve for empty version (helm treats it as "latest") or + // pinned semver — safeVersionPath is a no-op on those and the cache key is + // already unambiguous. + if chartVersion == "" || !isVersionConstraint(chartVersion) { + return chartVersion, false, nil + } + // Only OCI releases hit this code path via getOCIChart, but double-check + // so this helper is safe to call from other contexts too. + if !st.IsOCIChart(release.Chart) { + return chartVersion, false, nil + } + // Digest-pinned references bypass version resolution: the digest is the + // authoritative content identifier and helm ignores --version in that case. + if strings.Contains(qualifiedChartName, "@") { + return chartVersion, false, nil + } + // Type-assert to the optional ChartInspector capability so third-party + // implementations of helmexec.Interface that predate this feature keep + // compiling and simply fall back to the pre-fix caching behavior. + inspector, ok := helm.(helmexec.ChartInspector) + if !ok { + return chartVersion, false, nil + } + + // Strip any : tag that getOCIQualifiedChartName may have embedded + // in the ref, so `helm show chart` uses --version alone to resolve the + // constraint. `helm show chart oci://...:X --version Y` resolves Y against + // the registry regardless of X, but the ref without a tag matches the shape + // of a normal `helm pull` invocation and is what helm's own docs recommend + // for constraint resolution. parseOCIChartRef splits off any digest first, + // then the last tag colon after the last slash, preserving registry ports. + base, _, _ := parseOCIChartRef(qualifiedChartName) + ref := "oci://" + base + + // One registry round-trip per chart+constraint per process. The flags + // above do not influence WHICH tag a constraint matches (they only govern + // TLS/verification/registry credentials), so the memo key can ignore them. + // Concurrent misses may still race and both hit the registry; last write + // wins, which is harmless. + memoKey := ociConstraintKey{chartRef: base, constraint: chartVersion} + resolvedOCIConstraintsMutex.RLock() + memoized, hit := resolvedOCIConstraints[memoKey] + resolvedOCIConstraintsMutex.RUnlock() + if hit { + return memoized, memoized != chartVersion, nil + } + + flags := st.chartOCIFlags(release) + flags = st.appendVerifyFlags(flags, release) + flags = st.appendKeyringFlags(flags, release) + flags = st.appendChartDownloadFlags(flags, release) + // --devel is deliberately omitted: helm ignores it whenever --version is + // set, and --version is always passed here. + flags = append(flags, "--version", chartVersion) + + metadata, err := inspector.ShowChartWithFlags(ref, flags...) + if err != nil { + return chartVersion, false, err + } + if metadata.Version == "" { + return chartVersion, false, fmt.Errorf("helm show chart %s --version %s returned an empty Chart.yaml version", ref, chartVersion) + } + resolvedOCIConstraintsMutex.Lock() + resolvedOCIConstraints[memoKey] = metadata.Version + resolvedOCIConstraintsMutex.Unlock() + if metadata.Version == chartVersion { + return chartVersion, false, nil + } + return metadata.Version, true, nil +} + +// skipOCIConstraintResolution reports whether OCI constraint resolution +// should be skipped for this release, falling back to the constraint-keyed +// cache path (the pre-fix behavior, which reuses whatever the previous +// resolution cached). Resolution costs one `helm show chart` registry +// round-trip per constraint-versioned OCI release; honoring --skip-refresh +// (CLI flag, per-release skipRefresh, or helmDefaults.skipRefresh) keeps +// offline and cache-only workflows free of network attempts. Precedence +// mirrors the other skipRefresh consumers in prepareChartForRelease: the CLI +// flag forces skipping, then an explicit per-release value, then +// helmDefaults. +func (st *HelmState) skipOCIConstraintResolution(release *ReleaseSpec, opts ChartPrepareOptions) bool { + if opts.SkipRefresh { + return true + } + if release.SkipRefresh != nil { + return *release.SkipRefresh + } + return st.HelmDefaults.SkipRefresh +} + +// resolveOCIVersionsEnabled reports whether OCI constraint resolution is +// enabled for this release. Per-release setting wins over helmDefaults; both +// default to true (the fix is on unless explicitly opted out). +func (st *HelmState) resolveOCIVersionsEnabled(release *ReleaseSpec) bool { + if release.ResolveOCIVersions != nil { + return *release.ResolveOCIVersions + } + if st.HelmDefaults.ResolveOCIVersions != nil { + return *st.HelmDefaults.ResolveOCIVersions + } + return true +} diff --git a/pkg/state/state_test.go b/pkg/state/state_test.go index 40bbd51b..f7058019 100644 --- a/pkg/state/state_test.go +++ b/pkg/state/state_test.go @@ -12,6 +12,7 @@ import ( "github.com/helmfile/vals" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + chart "helm.sh/helm/v4/pkg/chart/v2" "github.com/helmfile/helmfile/pkg/environment" "github.com/helmfile/helmfile/pkg/exectest" @@ -5951,6 +5952,392 @@ func TestGetOCIChartPath(t *testing.T) { } } +// TestIsVersionConstraint checks the semver-parser-based constraint detector. +// Exact semvers (including with a "v" prefix, prerelease metadata that may +// contain "x", or build metadata that may contain "x") are not constraints. +// Anything the Masterminds/semver parser accepts as a constraint — operator +// forms AND wildcard segment forms (1.x, 1.X) — is a constraint. Values that +// are neither (empty, "latest", junk) return false: helm handles those +// separately elsewhere. +func TestIsVersionConstraint(t *testing.T) { + tests := []struct { + version string + want bool + }{ + // exact versions — never constraints + {"1.0.1", false}, + {"v1.0.1", false}, + {"1.0.0-rc.1", false}, + {"1.0.0+build.1", false}, + // exact versions where "x" appears in prerelease or build metadata: + // must NOT be misclassified as wildcard constraints + {"1.0.0-alpha.x", false}, + {"1.0.0+x", false}, + {"1.0.0+build.x.1", false}, + // operator constraints + {"~1", true}, + {"~1.0", true}, + {"^1", true}, + {"^2.0.0", true}, + {"*", true}, + {">=1.0.0", true}, + {">=1.0.0 <2.0.0", true}, // whitespace-separated range + {">1.0", true}, + {"<2.0", true}, + {"!=1.0.0", true}, + {"1.0.0 || 2.0.0", true}, + {"1.0.0,2.0.0", true}, + // wildcard-segment constraints — the case that a character scan missed + {"1.x", true}, + {"1.X", true}, + {"1.x.x", true}, + {"1.X.X", true}, + {"1.2.x", true}, + {"1.2.X", true}, + {"v1.x", true}, + // partial semvers — Masterminds' parser accepts them as versions, but + // helm's OCI resolution (registry.GetTagMatchingVersionOrConstraint) + // only treats a version string as an exact pin when a registry tag + // literally equals it; otherwise "1"/"1.2" float as ranges. They must + // therefore be classified as constraints and resolved before caching. + {"1", true}, + {"1.2", true}, + {"v1.2", true}, + {"0", true}, + {"v1", true}, + // neither a valid version nor a valid constraint — handled elsewhere, + // resolver skips them so the raw string keeps flowing to helm. + {"", false}, + {"latest", false}, + {"not-a-version", false}, + } + for _, tt := range tests { + t.Run(tt.version, func(t *testing.T) { + require.Equal(t, tt.want, isVersionConstraint(tt.version)) + }) + } +} + +// TestResolveOCIConstraintVersion exercises the pre-cache constraint resolver. +// The stubbed helm implementation stands in for `helm show chart ... --version +// ` and returns whatever Chart.yaml version the test wants; the +// resolver must forward the returned version back to the caller so downstream +// cache-key derivation uses the concrete tag rather than the raw constraint. +func TestResolveOCIConstraintVersion(t *testing.T) { + const ( + releaseName = "app" + chartRef = "myrepo/app" + qualified = "registry.example.com/charts/app" + resolvedVersion = "1.0.1" + ) + baseRepositories := []RepositorySpec{ + {Name: "myrepo", URL: "registry.example.com/charts", OCI: true}, + } + + newState := func(defaults HelmSpec) *HelmState { + return &HelmState{ + ReleaseSetSpec: ReleaseSetSpec{ + HelmDefaults: defaults, + Repositories: baseRepositories, + }, + logger: logger, + valsRuntime: valsRuntime, + } + } + + trueVal := true + falseVal := false + + tests := []struct { + name string + defaults HelmSpec + release ReleaseSpec + qualifiedRef string + version string + stubbedResolved string + stubbedErr error + expectHelmCalled bool + expectVersion string + expectChanged bool + expectErr bool + }{ + { + name: "constraint resolves to concrete version", + release: ReleaseSpec{Name: releaseName, Chart: chartRef, Version: "~1"}, + qualifiedRef: qualified, + version: "~1", + stubbedResolved: resolvedVersion, + expectHelmCalled: true, + expectVersion: resolvedVersion, + expectChanged: true, + }, + { + // Wildcard-segment constraint has no operator character but must + // still be detected as a constraint and resolved (regression + // coverage for the semver-parser-based isVersionConstraint fix). + name: "wildcard segment constraint resolves to concrete version", + release: ReleaseSpec{Name: releaseName, Chart: chartRef, Version: "1.x"}, + qualifiedRef: qualified, + version: "1.x", + stubbedResolved: resolvedVersion, + expectHelmCalled: true, + expectVersion: resolvedVersion, + expectChanged: true, + }, + { + // Partial semver ("1.2") parses as a version but floats as a range + // in helm's OCI tag matching, so it must also be resolved. + name: "partial version resolves to concrete version", + release: ReleaseSpec{Name: releaseName, Chart: chartRef, Version: "1.2"}, + qualifiedRef: qualified + ":1.2", + version: "1.2", + stubbedResolved: resolvedVersion, + expectHelmCalled: true, + expectVersion: resolvedVersion, + expectChanged: true, + }, + { + name: "exact version bypasses resolver", + release: ReleaseSpec{Name: releaseName, Chart: chartRef, Version: resolvedVersion}, + qualifiedRef: qualified + ":" + resolvedVersion, + version: resolvedVersion, + expectHelmCalled: false, + expectVersion: resolvedVersion, + expectChanged: false, + }, + { + name: "empty version bypasses resolver", + release: ReleaseSpec{Name: releaseName, Chart: chartRef}, + qualifiedRef: qualified, + version: "", + expectHelmCalled: false, + expectChanged: false, + }, + { + name: "digest-pinned ref bypasses resolver even with constraint version", + release: ReleaseSpec{Name: releaseName, Chart: chartRef, Version: "~1"}, + qualifiedRef: qualified + "@sha256:deadbeef", + version: "~1", + expectHelmCalled: false, + expectVersion: "~1", + expectChanged: false, + }, + { + name: "opt-out via HelmDefaults keeps raw constraint", + defaults: HelmSpec{ResolveOCIVersions: &falseVal}, + release: ReleaseSpec{Name: releaseName, Chart: chartRef, Version: "~1"}, + qualifiedRef: qualified, + version: "~1", + expectHelmCalled: false, + expectVersion: "~1", + expectChanged: false, + }, + { + name: "opt-out at release level wins over helmDefaults", + defaults: HelmSpec{ResolveOCIVersions: &trueVal}, + release: ReleaseSpec{Name: releaseName, Chart: chartRef, Version: "~1", ResolveOCIVersions: &falseVal}, + qualifiedRef: qualified, + version: "~1", + expectHelmCalled: false, + expectChanged: false, + }, + { + name: "resolver returns empty version is an error", + release: ReleaseSpec{Name: releaseName, Chart: chartRef, Version: "~1"}, + qualifiedRef: qualified, + version: "~1", + stubbedResolved: "", + expectHelmCalled: true, + expectErr: true, + }, + { + name: "resolver returns same version as constraint reports no change", + release: ReleaseSpec{Name: releaseName, Chart: chartRef, Version: "~1"}, + qualifiedRef: qualified, + version: "~1", + stubbedResolved: "~1", + expectHelmCalled: true, + expectVersion: "~1", + expectChanged: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Several subtests share the same chart+constraint; keep the + // in-process resolution memo out of the picture. + resetResolvedOCIConstraintsForTest() + called := false + var gotFlags []string + helm := &exectest.Helm{ + ShowChartWithFlagsFunc: func(chartPath string, flags ...string) (chart.Metadata, error) { + called = true + gotFlags = flags + if tt.stubbedErr != nil { + return chart.Metadata{}, tt.stubbedErr + } + return chart.Metadata{Version: tt.stubbedResolved}, nil + }, + } + st := newState(tt.defaults) + resolved, changed, err := st.resolveOCIConstraintVersion(&tt.release, helm, tt.qualifiedRef, tt.version) + + require.Equalf(t, tt.expectHelmCalled, called, "helm.ShowChartWithFlags call expectation mismatch") + if tt.expectHelmCalled { + // The resolver must pass --version so helm can + // resolve against the registry rather than a cached index. + require.Contains(t, gotFlags, "--version") + require.Contains(t, gotFlags, tt.version) + } + if tt.expectErr { + require.Error(t, err) + return + } + require.NoError(t, err) + require.Equal(t, tt.expectChanged, changed) + if tt.expectVersion != "" { + require.Equal(t, tt.expectVersion, resolved) + } + }) + } +} + +// TestSkipOCIConstraintResolution checks the tri-state skip logic: the CLI +// --skip-refresh flag forces skipping; otherwise an explicit per-release +// skipRefresh wins; otherwise helmDefaults.skipRefresh decides. +func TestSkipOCIConstraintResolution(t *testing.T) { + falseVal, trueVal := false, true + tests := []struct { + name string + opts ChartPrepareOptions + release ReleaseSpec + defaults HelmSpec + want bool + }{ + { + name: "CLI flag forces skip", + opts: ChartPrepareOptions{SkipRefresh: true}, + want: true, + }, + { + name: "no flags set resolves", + want: false, + }, + { + name: "release-level skipRefresh skips", + release: ReleaseSpec{SkipRefresh: &trueVal}, + want: true, + }, + { + name: "release-level skipRefresh=false beats helmDefaults=true", + release: ReleaseSpec{SkipRefresh: &falseVal}, + defaults: HelmSpec{SkipRefresh: true}, + want: false, + }, + { + name: "helmDefaults.skipRefresh applies when release is unset", + defaults: HelmSpec{SkipRefresh: true}, + want: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + st := &HelmState{ReleaseSetSpec: ReleaseSetSpec{HelmDefaults: tt.defaults}} + require.Equal(t, tt.want, st.skipOCIConstraintResolution(&tt.release, tt.opts)) + }) + } +} + +// TestResolveOCIConstraintVersion_Memoized verifies the in-process resolution +// memo: the second lookup of the same chart+constraint must not hit the +// registry again, while a different constraint on the same chart is a distinct +// key and does. +func TestResolveOCIConstraintVersion_Memoized(t *testing.T) { + resetResolvedOCIConstraintsForTest() + + const ( + repoURL = "registry.example.com/charts" + chartRef = "myrepo/memo" + qualified = repoURL + "/memo" + ) + calls := 0 + helm := &exectest.Helm{ + ShowChartWithFlagsFunc: func(_ string, flags ...string) (chart.Metadata, error) { + calls++ + return chart.Metadata{Version: "1.0.1"}, nil + }, + } + st := &HelmState{ + ReleaseSetSpec: ReleaseSetSpec{ + Repositories: []RepositorySpec{{Name: "myrepo", URL: repoURL, OCI: true}}, + }, + logger: logger, + valsRuntime: valsRuntime, + } + + first, changed, err := st.resolveOCIConstraintVersion(&ReleaseSpec{Name: "app", Chart: chartRef, Version: "~1"}, helm, qualified, "~1") + require.NoError(t, err) + require.True(t, changed) + require.Equal(t, "1.0.1", first) + require.Equal(t, 1, calls) + + // Same chart+constraint: served from the memo, no second registry call. + second, changed, err := st.resolveOCIConstraintVersion(&ReleaseSpec{Name: "app2", Chart: chartRef, Version: "~1"}, helm, qualified, "~1") + require.NoError(t, err) + require.True(t, changed) + require.Equal(t, "1.0.1", second) + require.Equal(t, 1, calls, "the second lookup must be served from the in-process memo") + + // A different constraint on the same chart is a different memo key. + _, _, err = st.resolveOCIConstraintVersion(&ReleaseSpec{Name: "app3", Chart: chartRef, Version: "^1"}, helm, qualified, "^1") + require.NoError(t, err) + require.Equal(t, 2, calls) +} + +// noOpChartInspector is a helmexec.Interface implementation that intentionally +// does NOT satisfy helmexec.ChartInspector. It exists to prove that +// resolveOCIConstraintVersion degrades gracefully when a third-party helm +// implementation predates the ShowChartWithFlags capability, instead of +// requiring every downstream mock to grow the new method. +type noOpChartInspector struct { + helmexec.Interface +} + +// TestResolveOCIConstraintVersion_ChartInspectorFallback confirms that a helm +// implementation lacking the ChartInspector capability causes the resolver to +// return the raw constraint unchanged with no error, keeping backward +// compatibility for third-party helmexec.Interface implementations. +func TestResolveOCIConstraintVersion_ChartInspectorFallback(t *testing.T) { + resetResolvedOCIConstraintsForTest() + const ( + repoName = "myrepo" + repoURL = "registry.example.com/charts" + chartRef = "myrepo/fallbackchart" + qualified = "registry.example.com/charts/fallbackchart" + ) + st := &HelmState{ + ReleaseSetSpec: ReleaseSetSpec{ + Repositories: []RepositorySpec{ + {Name: repoName, URL: repoURL, OCI: true}, + }, + }, + logger: logger, + valsRuntime: valsRuntime, + } + release := &ReleaseSpec{Name: "fallback", Chart: chartRef, Version: "~1"} + helm := &noOpChartInspector{} + + // Sanity: noOpChartInspector satisfies Interface but not ChartInspector. + var _ helmexec.Interface = helm + _, isInspector := any(helm).(helmexec.ChartInspector) + require.False(t, isInspector, "test setup: noOpChartInspector must NOT implement ChartInspector") + + resolved, changed, err := st.resolveOCIConstraintVersion(release, helm, qualified, "~1") + require.NoError(t, err) + require.False(t, changed, "no ChartInspector capability => must not change version") + require.Equal(t, "~1", resolved, "no ChartInspector capability => must return raw constraint") +} + func TestHelmState_chartOCIFlags(t *testing.T) { tests := []struct { name string diff --git a/pkg/state/temp_test.go b/pkg/state/temp_test.go index fb562383..b4809e60 100644 --- a/pkg/state/temp_test.go +++ b/pkg/state/temp_test.go @@ -35,42 +35,46 @@ func TestGenerateID(t *testing.T) { }) } + // NOTE: These hashes are derived from the full ReleaseSpec struct via + // generateValuesID -> HashObject. Adding or renaming fields on ReleaseSpec + // (even nil-defaulted pointer fields like ResolveOCIVersions) shifts every + // expected hash below. Regenerate the values whenever the struct changes. run(testcase{ subject: "baseline", release: ReleaseSpec{Name: "foo", Chart: "incubator/raw"}, - want: "foo-values-5bcd864488", + want: "foo-values-577699c466", }) run(testcase{ subject: "different bytes content", release: ReleaseSpec{Name: "foo", Chart: "incubator/raw"}, data: []byte(`{"k":"v"}`), - want: "foo-values-59cd566bbc", + want: "foo-values-c7dc8bf7", }) run(testcase{ subject: "different map content", release: ReleaseSpec{Name: "foo", Chart: "incubator/raw"}, data: map[string]any{"k": "v"}, - want: "foo-values-58bcc85765", + want: "foo-values-84744c8675", }) run(testcase{ subject: "different chart", release: ReleaseSpec{Name: "foo", Chart: "stable/envoy"}, - want: "foo-values-7796c46c49", + want: "foo-values-76c9d7ccff", }) run(testcase{ subject: "different name", release: ReleaseSpec{Name: "bar", Chart: "incubator/raw"}, - want: "bar-values-5b5f6f54cc", + want: "bar-values-55d5975ccf", }) run(testcase{ subject: "specific ns", release: ReleaseSpec{Name: "foo", Chart: "incubator/raw", Namespace: "myns"}, - want: "myns-foo-values-547578788f", + want: "myns-foo-values-7956fd86dc", }) for id, n := range ids { diff --git a/pkg/state/util.go b/pkg/state/util.go index 59cdb448..fe14e12e 100644 --- a/pkg/state/util.go +++ b/pkg/state/util.go @@ -6,6 +6,8 @@ import ( "path/filepath" "regexp" "strings" + + "github.com/Masterminds/semver/v3" ) var ( @@ -79,3 +81,75 @@ func safeVersionPath(version string) string { sp := c.ReplaceAll([]byte(version), []byte("_")) return string(sp) } + +// isVersionConstraint reports whether v is a Masterminds/semver constraint +// (e.g. "~1", "^2.0", ">=1.0.0 <2.0.0", "*", "1.x", "1.X.x") rather than an +// exact pinned version (e.g. "1.0.1", "v1.0.0-rc.1", "1.0.0+build.1"). Uses +// the semver parser instead of a character scan so that wildcard-segment +// constraints ("1.x", "1.X", "1.x.x") — which contain no operator characters +// — are correctly classified as constraints and reach the OCI resolver. +// +// Only fully-qualified semvers (see isFullSemver) count as exact pins. Helm +// resolves partial versions ("1", "1.2") as floating ranges against OCI +// registries — see helm's registry.GetTagMatchingVersionOrConstraint, which +// honors a version string as an exact pin only when a registry tag literally +// equals it — so partial versions must be resolved before deriving a cache +// path, too, or they reproduce the stale-cache bug of issue #2766. +// +// Values that are neither a valid semver nor a valid constraint (empty +// string, "latest", junk) return false: helm handles those separately (empty +// means "let helm pick latest"; "latest" is rejected earlier by +// getOCIQualifiedChartName). +func isVersionConstraint(v string) bool { + if v == "" { + return false + } + // Fully-qualified semver — with or without a "v" prefix, with prerelease + // and build metadata (which may legitimately contain "x") — is an exact + // pin, not a constraint. + if isFullSemver(v) { + return false + } + // Everything Masterminds accepts as a constraint is a constraint. This + // covers operator forms (~, ^, >=, ...), wildcard segment forms (1.x, + // 1.X, 1.x.x), and partial versions (1, 1.2) that float as ranges in + // helm's OCI tag matching. + _, err := semver.NewConstraint(v) + return err == nil +} + +// isFullSemver reports whether v is a fully-qualified semantic version that +// spells out the whole major.minor.patch triple (e.g. "1.2.3", "v1.2.3-rc.1", +// "1.2.3+build.5"). Masterminds' lenient parser also accepts partial versions +// ("1", "1.2", "v1.2"), but helm's OCI resolution treats those as floating +// ranges rather than exact pins, so they cannot be cached under their raw +// spelling either. +func isFullSemver(v string) bool { + if _, err := semver.NewVersion(v); err != nil { + return false + } + // Drop build metadata first, then the prerelease segment, leaving the + // numeric release core ("v1.2.3-rc.1+b" -> "v1.2.3"). + if i := strings.IndexByte(v, '+'); i >= 0 { + v = v[:i] + } + if i := strings.IndexByte(v, '-'); i >= 0 { + v = v[:i] + } + v = strings.TrimPrefix(v, "v") + parts := strings.Split(v, ".") + if len(parts) != 3 { + return false + } + for _, p := range parts { + if p == "" { + return false + } + for _, r := range p { + if r < '0' || r > '9' { + return false + } + } + } + return true +}