Reproducible chart archive builds#31323
Conversation
pull-request-size
bot
added
the
size/XL
|
Note, this is related to #10178. That PR has many commits and no longer cleanly applies. This updates for the changes that have happened to Helm and includes tests that were not previously updated. |
There was a problem hiding this comment.
Pull Request Overview
This PR modifies Helm chart archiving to enable reproducible builds by using consistent file modification times instead of the current timestamp. The change ensures that packaging the same chart multiple times produces archives with identical SHA256 hashes.
Key changes:
- File modification times are now captured when charts are loaded from the filesystem
- Archive creation uses these captured timestamps instead of
time.Now() - New test coverage validates reproducible archive creation
Reviewed Changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/chart/common/file.go | Added ModTime field to File struct |
| pkg/chart/v2/chart.go | Added ModTime and SchemaModTime fields to Chart struct |
| pkg/chart/v2/util/save.go | Modified writeToTar to accept modTime parameter |
| pkg/chart/v2/util/save_test.go | Added tests for reproducible archive generation |
| pkg/chart/loader/archive/archive.go | Updated BufferedFile to include ModTime |
| pkg/chart/v2/loader/load.go | Set ModTime fields during chart loading |
| pkg/chart/v2/loader/directory.go | Capture file ModTime from filesystem |
| Multiple test files | Updated test fixtures to include ModTime fields |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
With https://github.com/helm/helm/pull/31315/files merged, I get: Running what I'm pretty sure is in the v8 action |
|
Is this considered a breaking change? |
|
@TerryHowe The lint rule that was recently removed is used. We have several package that have 'meaningless' names which trigger this rule on change. I created #31330 to restore the rule and left a code comment for context. In the past we had considered this a breaking change but having written the change I wouldn't. It's been requested for a long time. |
mattfarina
force-pushed
the
reproducible-chart-builds
branch
from
d536250 to
7facf29
Compare
|
It'd be nice to respect |
|
@hanikesn that's an interesting idea. We would need to set the ModTime for each file in the archive to this date/time. This is something we can do. |
Building the same chart into an archive multiple times will have the same sha256 hash. Perviously, the time in the headers for a file was time.Now() which changed each time. The time is now collected from the operating system when the file is loaded and this time is used. Fixes: helm#3612 Signed-off-by: Matt Farina <[email protected]>
mattfarina
force-pushed
the
reproducible-chart-builds
branch
from
7facf29 to
ca8eae9
Compare
Note, when time is not available, the builds are not reproducible. This problem would only happen when an SDK user is using parts of the API to build their own tooling. Helm will consistently inject the dates through the higher level APIs. Signed-off-by: Matt Farina <[email protected]>
robertsirc
left a comment
robertsirc
left a comment
There was a problem hiding this comment.
LGTM, Walked through the unit test, and this passed my local test suite as well.
robertsirc
added
the
Has One Approval
|
Per our policy, this needs 2 maintainers to look at it. For community that's 2 maintainers. For maintainer developed changes, the developer counts as one of the two. With that, merging this PR. |
gjenkins8
removed
the
Has One Approval
|
I had reviewed privately with Matt, but didn't get back to formally give the LGTM this after the fixes. Just wanted to say NICE WORK on getting the fix in for this long standing and very frustrating issue! 👏👏 |
Building the same chart into an archive multiple times will have the same sha256 hash.
Perviously, the time in the headers for a file was time.Now() which changed each time. The time is now collected from the operating system when the file is loaded and this time is used.
Fixes: #3612
What this PR does / why we need it:
Special notes for your reviewer:
If applicable:
docs neededlabel should be applied if so)