Commit Graph

5 Commits

Author SHA1 Message Date
Shane Starcher 23802e7a95
fix: refresh Chart.lock after rewriting file:// dependencies (#2587)
* fix: refresh Chart.lock after rewriting file:// dependencies

`rewriteChartDependencies` rewrites relative `file://` repository URLs in
Chart.yaml to absolute paths so chartify can resolve them from a temp
directory. That mutates the Chart.yaml dependencies block, which
invalidates the Chart.lock digest (helm computes it as
`sha256(json.Marshal([2][]Dependency{req, lock}))` over the dependencies).

Once the lock is out of sync, downstream `helm dependency build` errors
with "the lock file (Chart.lock) is out of sync with the dependencies
file (Chart.yaml)" and chartify falls back to `helm dependency update`.
`dep update` then re-resolves Chart.yaml's version constraints against
the chart repo, so any constraint that admits newer versions
(e.g. `version: "*"`, `~1.0`) silently picks up a newer dependency on
every render — even though Chart.lock pins a specific version.

Repro:
  - Chart.yaml has `version: "*"` for some-dep, Chart.lock pins 4.1.0,
    upstream now publishes 4.2.0.
  - `helm template .` honors the cached `charts/some-dep-4.1.0.tgz`.
  - `helmfile template` produces 4.2.0, because it triggered chartify
    (via jsonPatches/strategic-merge/kustomize/etc), which copied the
    chart, ran `dep build` against an out-of-sync lock, fell back to
    `dep up`, and re-resolved the wildcard.

This commit refreshes Chart.lock alongside Chart.yaml in the temp copy:

- Mirror the rewritten file:// repository URLs onto matching entries in
  Chart.lock's dependencies. Without this, `helm dep build` would resolve
  the lock's relative `file://` paths against the temp chart directory
  and fail with "directory ... not found".
- Recompute the digest using helm's resolver.HashReq algorithm
  (`sha256(json.Marshal([2][]chart.Dependency{req, lock}))`). The
  algorithm is small and stable; resolver.HashReq itself lives in an
  internal package, so it's inlined here.
- Locked versions are preserved verbatim — only the repository URL is
  updated and the digest recomputed. Chart.lock remains the source of
  truth for which versions get installed.
- The original Chart.lock on disk is never modified; only the temp copy
  is rewritten.

Adds TestRewriteChartDependencies_RefreshesChartLock covering digest
recomputation, file:// URL mirroring, version preservation, untouched
non-file:// deps, and original-on-disk integrity.


Signed-off-by: Shane Starcher <shane.starcher@gmail.com>

* fix: address Copilot review issues for Chart.lock refresh

- Map all helm Dependency fields (alias, condition, tags, import-values,
  enabled) when building the request slice for digest computation, not
  just name/version/repository. This ensures the recomputed digest
  matches Helm's resolver.HashReq for all dependency shapes.
- Match lock entries by Name + Alias (not Name alone) to correctly
  handle charts with duplicate dependency names distinguished by alias.
- Log a warning when reading Chart.lock fails with a non-NotExist error,
  while still treating a missing Chart.lock as expected.
- Add test case exercising dependencies with alias, condition, tags, and
  import-values fields, including same-name deps disambiguated by alias.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Signed-off-by: Shane Starcher <shane.starcher@gmail.com>

* build(deps): bump github.com/helmfile/chartify to v0.26.4

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Signed-off-by: Shane Starcher <shane.starcher@gmail.com>

* fix: normalize import-values for JSON marshaling and improve test coverage

- Normalize import-values using maputil.RecursivelyStringifyMapKey before
  assigning to helmchart.Dependency.ImportValues. When go-yaml v2 decodes
  nested maps (e.g. import-values entries with child/parent keys), they
  become map[interface{}]interface{} which json.Marshal cannot encode.
  This would silently prevent Chart.lock rewriting. The normalization
  converts all map keys to strings, making the value JSON-safe.
- Improve TestRewriteChartDependencies_RefreshesChartLockWithExtraFields
  to prove that extra fields (condition, tags, import-values) actually
  affect the computed digest by comparing digests with and without those
  fields and asserting they differ.

Signed-off-by: Shane Starcher <shane.starcher@gmail.com>

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: normalize lock ImportValues and fix digest test isolation

- Normalize lock.Dependencies ImportValues via RecursivelyStringifyMapKey
  before json.Marshal, preventing failures when go-yaml v2 decodes nested
  maps as map[interface{}]interface{}.
- Fix TestRewriteChartDependencies_RefreshesChartLockWithExtraFields to use
  a shared root directory so both chart variants resolve file:// paths to
  the same absolute location, isolating digest differences to field content.
- Add TestRewriteChartDependencies_GoYamlV2ImportValues exercising the
  HELMFILE_GO_YAML_V3=false path with import-values containing nested maps.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Shane Starcher <shane.starcher@gmail.com>

* fix: add exact digest verification test against Helm's HashReq

Add TestRewriteChartDependencies_DigestMatchesHelmHashReq which computes
the expected digest independently using the same algorithm as Helm's
resolver.HashReq and asserts the rewritten Chart.lock matches exactly.
This guards against producing a digest that is "different" yet still
rejected by `helm dependency build`.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Shane Starcher <shane.starcher@gmail.com>

---------

Signed-off-by: Shane Starcher <shane.starcher@gmail.com>
Co-authored-by: Shane Starcher <shane.starcher@gmail.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
2026-05-19 05:41:41 +08:00
yxxhero fc31dbfc5e
fix: eliminate race condition in rewriteChartDependencies (#2541)
* fix: eliminate race condition in rewriteChartDependencies by copying chart before modifying

Instead of modifying the original Chart.yaml in-place (which causes race
conditions when multiple releases reference the same local chart), copy the
chart to a temporary directory and rewrite the copy's dependencies. This
eliminates the need for per-chart mutex locks and prevents file corruption
when concurrent goroutines process releases sharing the same local chart.

Fixes #2502

Signed-off-by: yxxhero <aiopsclub@163.com>

* fix: address PR review comments for rewriteChartDependencies

- Handle non-NotExist errors from st.fs.Stat to surface permission/IO failures
- Reword function doc to clarify temp copy is conditional on rewrite being needed
- Assert rewrittenPath vs tempDir based on expectModified in test table

Signed-off-by: yxxhero <aiopsclub@163.com>

* test: add integration test for issue #2502 race condition with shared local chart

Signed-off-by: yxxhero <aiopsclub@163.com>

* fix: separate environments and releases with --- in helmfile.yaml

Signed-off-by: yxxhero <aiopsclub@163.com>

* fix: correct file:// path and remove --skip-deps for dependency build

Signed-off-by: yxxhero <aiopsclub@163.com>

* fix: correct file:// dependency path (5 levels up to test/integration/)

Signed-off-by: yxxhero <aiopsclub@163.com>

* fix: remove output validation from race condition test

Signed-off-by: yxxhero <aiopsclub@163.com>

* fix: assert WriteFile/MkdirTemp/RemoveAll/CopyDir in DefaultFileSystem test

Signed-off-by: yxxhero <aiopsclub@163.com>

* fix: add strategicMergePatches to trigger chartify in race condition test

Signed-off-by: yxxhero <aiopsclub@163.com>

* fix: scope test values under raw subchart and align ConfigMap name with strategic merge patches

The race condition test values.yaml had templates at the top level instead
of scoped under the raw subchart key, causing helm template to produce no
output and chartify's ReplaceWithRendered to fail with an empty
helmx.1.rendered directory. Also align the ConfigMap name to match the
strategicMergePatches target.

Signed-off-by: yxxhero <aiopsclub@163.com>

---------

Signed-off-by: yxxhero <aiopsclub@163.com>
2026-04-20 10:15:47 +08:00
yxxhero acb7ce36fc
fix: add mutex lock for concurrent rewriteChartDependencies access (#2509)
* fix: add mutex lock for concurrent rewriteChartDependencies access

Prevent race conditions when multiple goroutines concurrently access
the same Chart.yaml by introducing a per-chart-path mutex via sync.Map.

Signed-off-by: yxxhero <yxxhero@users.noreply.github.com>
Signed-off-by: yxxhero <aiopsclub@163.com>

* fix: unlock mutex on all error paths and improve race condition test validation

Agent-Logs-Url: https://github.com/helmfile/helmfile/sessions/135e06b8-99e8-42cb-859d-524b2d2b1907

Co-authored-by: yxxhero <11087727+yxxhero@users.noreply.github.com>

* fix: remove duplicate mutex, reuse getNamedRWMutex, restore on write failure, add test barrier

Agent-Logs-Url: https://github.com/helmfile/helmfile/sessions/f93746bf-82fa-46b1-b8f0-b34bc9aa749c

Co-authored-by: yxxhero <11087727+yxxhero@users.noreply.github.com>

* fix: early unlock when unmodified, assert exact restore in race test

Agent-Logs-Url: https://github.com/helmfile/helmfile/sessions/5981aea4-2799-4560-9272-315d28d4b7d7

Co-authored-by: yxxhero <11087727+yxxhero@users.noreply.github.com>

* fix: improve comment wording and strengthen race test start barrier

Agent-Logs-Url: https://github.com/helmfile/helmfile/sessions/9e4d6f4f-cdc1-4e9e-bdc6-81061ebc1dcc

Co-authored-by: yxxhero <11087727+yxxhero@users.noreply.github.com>

---------

Signed-off-by: yxxhero <yxxhero@users.noreply.github.com>
Signed-off-by: yxxhero <aiopsclub@163.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
2026-04-13 11:59:52 +08:00
Etienne Champetier 4bdb6f097c
fix: keep all chart dependencies key / values (#2501)
* feat: Refactor TestRewriteChartDependencies

Signed-off-by: Etienne Champetier <e.champetier@ateme.com>

* fix: keep all chart dependencies key / values

In rewriteChartDependencies we were only parsing name / repository / version,
thus dropping keys like condition / import-values.

This at least fixes the use of condition.

Signed-off-by: Etienne Champetier <e.champetier@ateme.com>

---------

Signed-off-by: Etienne Champetier <e.champetier@ateme.com>
2026-03-26 13:16:54 +08:00
Shane Starcher 61f4a316a6
fix: rewrite relative file:// chart dependencies to absolute paths (#2334)
Fixes an issue where Chart.yaml dependencies with relative file:// paths
fail during chartification because the paths become invalid when the chart
is copied to chartify's temporary directory.

The rewriteChartDependencies function now converts relative file://
dependencies to absolute paths before chartification, then restores the
original Chart.yaml afterwards. Absolute file:// and other repository
types (https, oci) are left unchanged.

Includes comprehensive test coverage for various dependency scenarios.

Signed-off-by: Shane Starcher <shane.starcher@gmail.com>
Co-authored-by: Shane Starcher <shane.starcher@gmail.com>
2025-12-20 09:08:39 +08:00