Fix package consistency - retain shasum for the package#10178
Fix package consistency - retain shasum for the package#10178
Conversation
helm-bot
added
the
size/M
Signed-off-by: frenchben <[email protected]>
Signed-off-by: frenchben <[email protected]>
FrenchBen
force-pushed
the
fix-packages
branch
from
f933eaa to
0ac5a21
Compare
FrenchBen
changed the title
Signed-off-by: frenchben <[email protected]>
Signed-off-by: frenchben <[email protected]>
helm-bot
added
size/L
pkg/chart/loader/load_test.go
Outdated
| home: http://example.com | ||
| icon: https://example.com/64x64.png | ||
| `), | ||
| ModTime: time.Now(), |
There was a problem hiding this comment.
Think we need a c.ModTime check in TestLoadFiles since other fields in c were also checked.
|
Don't think the provided test case will reproduce the original issue, since the issue does not happen all the time (70% of the time sha is identical). Also as pointed out in #3612 (comment) and #9674 (comment), the potential fix is to enforce the deterministic file ordering within tarballs. It would be great if you could provide test cases to reproduce the original issue (#9674 (review)). |
Signed-off-by: frenchben <[email protected]>
|
@zonggen I don't believe that file ordering is the key here, as walking the dir to add files into the array, as currently done, should be a repeatable process. Golang Walk pkg: https://pkg.go.dev/path/filepath#Walk
I went ahead and added the exact same test found in this PR, without the mod-time, to the main branch. Repeatedly got wrong SHA: (not 70%) |
|
Thanks for the explanation! I reproduced your I used a modified chart which has three subcharts. |
|
Great use case @zonggen - let me reproduce on my end, and figure out why this isn't fixed as I'd expect it to. |
Signed-off-by: frenchben <[email protected]>
|
@zonggen Fixed! Since subcharts were using |
Signed-off-by: frenchben <[email protected]>
|
Tested with latest change (ModTime + subchart load order), worked as expected: same hash for each run. Then I was curious to see if only changing the load order will fix this issue (without ModTime). Turned out that without ModTime, it is less likely to have different hash, but still happens occasionally: $ for i in {1..20}; do helm package druid/ > /dev/null && sha256sum druid-0.3.0.tgz && rm druid-0.3.0.tgz; done
4d8cc0bb5e1e11d3b938b679e62eec9eb4a796e606ef5c9d84f3b408e1535f64 druid-0.3.0.tgz
4d8cc0bb5e1e11d3b938b679e62eec9eb4a796e606ef5c9d84f3b408e1535f64 druid-0.3.0.tgz
4d8cc0bb5e1e11d3b938b679e62eec9eb4a796e606ef5c9d84f3b408e1535f64 druid-0.3.0.tgz
4d8cc0bb5e1e11d3b938b679e62eec9eb4a796e606ef5c9d84f3b408e1535f64 druid-0.3.0.tgz
4d8cc0bb5e1e11d3b938b679e62eec9eb4a796e606ef5c9d84f3b408e1535f64 druid-0.3.0.tgz
4d8cc0bb5e1e11d3b938b679e62eec9eb4a796e606ef5c9d84f3b408e1535f64 druid-0.3.0.tgz
4d8cc0bb5e1e11d3b938b679e62eec9eb4a796e606ef5c9d84f3b408e1535f64 druid-0.3.0.tgz
4d8cc0bb5e1e11d3b938b679e62eec9eb4a796e606ef5c9d84f3b408e1535f64 druid-0.3.0.tgz
4d8cc0bb5e1e11d3b938b679e62eec9eb4a796e606ef5c9d84f3b408e1535f64 druid-0.3.0.tgz
4d8cc0bb5e1e11d3b938b679e62eec9eb4a796e606ef5c9d84f3b408e1535f64 druid-0.3.0.tgz
4d8cc0bb5e1e11d3b938b679e62eec9eb4a796e606ef5c9d84f3b408e1535f64 druid-0.3.0.tgz
4d8cc0bb5e1e11d3b938b679e62eec9eb4a796e606ef5c9d84f3b408e1535f64 druid-0.3.0.tgz
4d8cc0bb5e1e11d3b938b679e62eec9eb4a796e606ef5c9d84f3b408e1535f64 druid-0.3.0.tgz
4d8cc0bb5e1e11d3b938b679e62eec9eb4a796e606ef5c9d84f3b408e1535f64 druid-0.3.0.tgz
66f5e13dd451f60807e6d7b75e7a3b128e49592eda384ed37d28ebabbf48169f druid-0.3.0.tgz
66f5e13dd451f60807e6d7b75e7a3b128e49592eda384ed37d28ebabbf48169f druid-0.3.0.tgz
66f5e13dd451f60807e6d7b75e7a3b128e49592eda384ed37d28ebabbf48169f druid-0.3.0.tgz
66f5e13dd451f60807e6d7b75e7a3b128e49592eda384ed37d28ebabbf48169f druid-0.3.0.tgz
66f5e13dd451f60807e6d7b75e7a3b128e49592eda384ed37d28ebabbf48169f druid-0.3.0.tgz
66f5e13dd451f60807e6d7b75e7a3b128e49592eda384ed37d28ebabbf48169f druid-0.3.0.tgzSo I think both changes are needed and this PR seems to fix the original issue. Waiting for core maintainers to chime in :) |
Signed-off-by: frenchben <[email protected]>
FrenchBen
force-pushed
the
fix-packages
branch
from
6ca73c5 to
e4d4f96
Compare
|
bump @bacongobbler :) |
|
So... could this one go in? It looks like a useful improvement I'd like to use ;-) |
@Mathiasdm how do you tar it? I close to giving up as the tgz-files from helm always create a different checksum after unpacking and repacking (If I run helm package on the same chart content in between) So far I've tried: |
|
Thanks for doing this @FrenchBen, today I needed this and I ended up here 😄 |
|
@h0tbird it's technically an existing command, that provided inconsistent shasum when building a package. |
|
Back to the top, and hoping that we can get this merged :) |
|
@FrenchBen TODO: Add idempotent flag to the CLI to prevent breaking Helm 3. |
|
Any update on the contribution? @gjenkins8 @mattfarina? We are also facing an issue where the sums are different for every build (of the same chart version) which makes it seem like the OCI chart package is always changing. The issue is now almost 7 years old 😐 |
|
Closing in favor of #31323 reopen if I missed something |
14 participants
What this PR does / why we need it: Fixes #3612
Special notes for your reviewer:
We also have this PR pending for it: #9674
I believe this is the better approach, by leveraging the file modtime, instead of creating a new one in the tar headers.
If applicable:
Added test for package SHA showing a repeatable process:
Validation