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