🐛 Fix deep hash object #11
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## poc-boxcutter #11 +/- ##
================================================
Coverage ? 71.96%
================================================
Files ? 85
Lines ? 8128
Branches ? 0
================================================
Hits ? 5849
Misses ? 1890
Partials ? 389
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
joelanford
force-pushed
the
fix-deep-hash-object
branch
from
f89e037 to
a3cc2be
Compare
| // which follows pointers and prints actual values of the nested objects | ||
| // ensuring the hash does not change when a pointer changes. | ||
| func DeepHashObject(obj interface{}) (string, error) { | ||
| func DeepHashObject(obj interface{}) string { |
There was a problem hiding this comment.
I would keep keep the previous signature, returning error is safer/less surprising to the caller than raising panic. If the panic is not caught, it kills the process, which might be problematic.
There was a problem hiding this comment.
My thought process here is if we (the programmers) manage to send in an un-marshalable object, it seems like the error would be essentially unrecoverable for a user.
We have metrics collected/alerted in our e2e if anything in our e2e causes panics, so as long as we have decent e2e test coverage, we should catch any panic possibility in pre-merge testing.
@thetechnick's original hash function also panic-ed on an unexpected error, so I followed that lead. I don't have strong opinions on this though.
internal/shared/util/hash/hash.go
Outdated
| // DeepHashObject writes specified object to hash using the spew library | ||
| // which follows pointers and prints actual values of the nested objects | ||
| // ensuring the hash does not change when a pointer changes. |
There was a problem hiding this comment.
It looks like the docs should be update as well, given that we do not use spew library anymore?
| encoder := json.NewEncoder(hasher) | ||
| if err := encoder.Encode(obj); err != nil { | ||
| return "", fmt.Errorf("couldn't encode object: %w", err) | ||
| panic(fmt.Sprintf("couldn't encode object: %v", err)) |
| } | ||
|
|
||
| // base62(sha224(bytes)) is a useful hash and encoding for adding the contents of this | ||
| // base36(sha224(bytes)) is a useful hash and encoding for adding the contents of this |
There was a problem hiding this comment.
why not going with base62 instead, i.e. returning i.Text(62)? It is more compact, hashes are shorter.
There was a problem hiding this comment.
Somehow, something seems like it was lost in translation when we ported this from OLM to rukpak to here.
In OLMv0 it looks like this: https://github.com/operator-framework/operator-lifecycle-manager/blob/b9e6cce9f3f08aaffa305f3b3add0e71a34cee20/pkg/lib/kubernetes/pkg/util/hash/hash.go#L47-L54
In rukpak it looks like this: https://github.com/operator-framework/rukpak/blob/5ffcfff615566f3a1a482a5f6649cd3e07f2427f/pkg/util/hash.go#L31-L38
I'm up for changing this back to base62, but I think that may change the names of a decent chunk of objects that the registry+v1 converter produces.
My vote, leave as is for now. But I'll open a separate issue to focus on this question.
There was a problem hiding this comment.
It also may be moot based on your dry-run suggestion last week.
internal/shared/util/hash/hash.go
Outdated
| var ( | ||
| i big.Int | ||
| hash = make([]byte, 0, hasher.Size()) | ||
| ) | ||
| i.SetBytes(hasher.Sum(hash)) |
There was a problem hiding this comment.
what is the benefit of this change vs previous code, or could we do this like instead?
var i big.Int
i.SetBytes(hasher.Sum(nil))
return i.Text(62)There was a problem hiding this comment.
Good call. I was thinking "pre-allocate the slice capacity", but the Sum method directly writes all the bytes in one go, so no need to pre-allocate.
Changing to your suggestion.
thetechnick
force-pushed
the
poc-boxcutter
branch
3 times, most recently
from
a0adcc0 to
c175077
Compare
Bumps [github.com/prometheus/client_golang](https://github.com/prometheus/client_golang) from 1.23.1 to 1.23.2. - [Release notes](https://github.com/prometheus/client_golang/releases) - [Changelog](https://github.com/prometheus/client_golang/blob/v1.23.2/CHANGELOG.md) - [Commits](prometheus/client_golang@v1.23.1...v1.23.2) --- updated-dependencies: - dependency-name: github.com/prometheus/client_golang dependency-version: 1.23.2 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Update boxcutter library to branch with latest k8s and controller-runtime libs Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com> * Update go.mod Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com> * Add ClusterExtensionRevisionAPI Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com> * Add BoxcutterRuntime featuregate Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com> * Add Boxcutter applier Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com> * Add ClusterExtensionRevision controller Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com> * Add Boxcutter runtime to main Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com> * Remove ClusterExtensionRevision from crd-docs Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com> * Update hack/tools/update-crds.sh for ClusterExtensionRevision API Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com> * Generate manifests Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com> * Remove access manager and dynamic cache Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com> * Update boxcutter to v0.3.0, add TrackingCache to Runnables * boxcutter webhook support Signed-off-by: Joe Lanford <joe.lanford@gmail.com> * add BoxcutterRuntime feature gate to experimental release Signed-off-by: Joe Lanford <joe.lanford@gmail.com> * add boxcutter cluster-admin cluster role binding in boxcutter's feature component Signed-off-by: Joe Lanford <joe.lanford@gmail.com> * Boxcutter Preflight Signed-off-by: Todd Short <tshort@redhat.com> * Boxcutter Preflight mock cleanup Signed-off-by: Todd Short <tshort@redhat.com> * Use new TrackingCache Watch/Free. Ensure informers are started before reconciling and stopped before removing the finalizer. * add BoxcutterInstalledBundleGetter, plumb bundle metadata into revision annotations Signed-off-by: Joe Lanford <joe.lanford@gmail.com> * InstalledBundleGetter -> RevisionStatesGetter This change accommodates the possibility of a revision that is currently rolling out, which is possible for appliers that perform rollouts asynchronously. Signed-off-by: Joe Lanford <joe.lanford@gmail.com> * refactor Applier interface and improve status reporting Signed-off-by: Joe Lanford <joe.lanford@gmail.com> * fixup tests for applier and installedbundlegetter changes Signed-off-by: Joe Lanford <joe.lanford@gmail.com> * resolve linter issues Signed-off-by: Joe Lanford <joe.lanford@gmail.com> * set status for other failure modes during ClusterExtensionRevision reconciliation Signed-off-by: Joe Lanford <joe.lanford@gmail.com> * TODO: fail upgrade-e2e if revision storage is unmigrated Signed-off-by: Joe Lanford <joe.lanford@gmail.com> * fixing broken tests after rebase Signed-off-by: Joe Lanford <joe.lanford@gmail.com> * Boxcutter Phases Defines a set of phases which facilitate a smoother installation vs applying every resource in the bundle all at once. Signed-off-by: Daniel Franz <dfranz@redhat.com> * Const Cleanup Captures conditions and reasons used by ClusterExtensionRevision into consts. Signed-off-by: Daniel Franz <dfranz@redhat.com> * Add migration from helm to boxcutter revision --------- Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com> Signed-off-by: Joe Lanford <joe.lanford@gmail.com> Signed-off-by: Todd Short <tshort@redhat.com> Signed-off-by: Daniel Franz <dfranz@redhat.com> Co-authored-by: Per Goncalves da Silva <pegoncal@redhat.com> Co-authored-by: Joe Lanford <joe.lanford@gmail.com> Co-authored-by: Todd Short <tshort@redhat.com> Co-authored-by: Daniel Franz <dfranz@redhat.com>
…or-framework#2196) Bumps [pkg.package-operator.run/boxcutter](https://github.com/package-operator/boxcutter) from 0.5.1 to 0.6.0. - [Release notes](https://github.com/package-operator/boxcutter/releases) - [Commits](package-operator/boxcutter@v0.5.1...v0.6.0) --- updated-dependencies: - dependency-name: pkg.package-operator.run/boxcutter dependency-version: 0.6.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [golang.org/x/mod](https://github.com/golang/mod) from 0.27.0 to 0.28.0. - [Commits](golang/mod@v0.27.0...v0.28.0) --- updated-dependencies: - dependency-name: golang.org/x/mod dependency-version: 0.28.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [mkdocs-material](https://github.com/squidfunk/mkdocs-material) from 9.6.18 to 9.6.19. - [Release notes](https://github.com/squidfunk/mkdocs-material/releases) - [Changelog](https://github.com/squidfunk/mkdocs-material/blob/master/CHANGELOG) - [Commits](squidfunk/mkdocs-material@9.6.18...9.6.19) --- updated-dependencies: - dependency-name: mkdocs-material dependency-version: 9.6.19 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [golang.org/x/sync](https://github.com/golang/sync) from 0.16.0 to 0.17.0. - [Commits](golang/sync@v0.16.0...v0.17.0) --- updated-dependencies: - dependency-name: golang.org/x/sync dependency-version: 0.17.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com> Co-authored-by: Per Goncalves da Silva <pegoncal@redhat.com>
Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
joelanford
force-pushed
the
fix-deep-hash-object
branch
from
a3cc2be to
66293ac
Compare
|
Re-opened here: operator-framework#2201 |
Description
Reviewer Checklist