🌱 promote rukpak hash util to shared package, use in boxcutter applier

go.mod

@@ -9,7 +9,6 @@ require (
 	github.com/cert-manager/cert-manager v1.18.2
 	github.com/containerd/containerd v1.7.28
 	github.com/containers/image/v5 v5.36.2
-	github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc
 	github.com/fsnotify/fsnotify v1.9.0
 	github.com/go-logr/logr v1.4.3
 	github.com/golang-jwt/jwt/v5 v5.3.0
@@ -89,6 +88,7 @@ require (
 	github.com/containers/storage v1.59.1 // indirect
 	github.com/cyberphone/json-canonicalization v0.0.0-20241213102144-19d51d7fe467 // indirect
 	github.com/cyphar/filepath-securejoin v0.4.1 // indirect
+	github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect
 	github.com/distribution/reference v0.6.0 // indirect
 	github.com/docker/cli v28.4.0+incompatible // indirect
 	github.com/docker/distribution v2.8.3+incompatible // indirect

internal/operator-controller/applier/boxcutter.go

@@ -3,17 +3,13 @@ package applier
 import (
 	"cmp"
 	"context"
-	"crypto/sha256"
-	"encoding/hex"
 	"errors"
 	"fmt"
-	"hash"
 	"io/fs"
 	"maps"
 	"slices"
 	"strings"
 
-	"github.com/davecgh/go-spew/spew"
 	"helm.sh/helm/v3/pkg/release"
 	"helm.sh/helm/v3/pkg/storage/driver"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -33,6 +29,7 @@ import (
 	"github.com/operator-framework/operator-controller/internal/operator-controller/labels"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle/source"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/render"
+	hashutil "github.com/operator-framework/operator-controller/internal/shared/util/hash"
 )
 
 const (
@@ -233,7 +230,7 @@ func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust
 	if err != nil {
  return false, "", err
 	}
-	desiredHash := computeSHA256Hash(desiredRevision.Spec.Phases)
+	desiredHash := hashutil.DeepHashObject(desiredRevision.Spec.Phases)
 
 	// Sort into current and previous revisions.
 	var (
@@ -355,33 +352,6 @@ func (bc *Boxcutter) getExistingRevisions(ctx context.Context, extName string) (
 	return existingRevisionList.Items, nil
 }
 
-// computeSHA256Hash returns a sha236 hash value calculated from object.
-func computeSHA256Hash(obj any) string {
-	hasher := sha256.New()
-	deepHashObject(hasher, obj)
-	return hex.EncodeToString(hasher.Sum(nil))
-}
-
-// 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.
-func deepHashObject(hasher hash.Hash, objectToWrite any) {
-	hasher.Reset()
-
-	// TODO: change this out to `json.Marshal`. Pretty sure we found issues in the past where
-	// spew would produce different output when internal structures changed without the
-	// external public API changing.
-	printer := spew.ConfigState{
- Indent: " ",
- SortKeys: true,
- DisableMethods: true,
- SpewKeys: true,
-	}
-	if _, err := printer.Fprintf(hasher, "%#v", objectToWrite); err != nil {
- panic(err)
-	}
-}
-
 func latestRevisionNumber(prevRevisions []ocv1.ClusterExtensionRevision) int64 {
 	if len(prevRevisions) == 0 {
  return 0

internal/operator-controller/applier/boxcutter_test.go

@@ -354,7 +354,7 @@ func TestBoxcutter_Apply(t *testing.T) {
  UID: "test-uid",
  },
 	}
-	defaultDesiredHash := "705ada5296ab26f74d94bfa497295a0cbccdb140623bbe704a3506cd1dfba4eb"
+	defaultDesiredHash := "gvvp8nzq5sbila80hkiv69am8hdr7o68qkk8n084gdn"
 	defaultDesiredRevision := &ocv1.ClusterExtensionRevision{
  ObjectMeta: metav1.ObjectMeta{
  Name: "test-ext-1",
@@ -546,7 +546,7 @@ func TestBoxcutter_Apply(t *testing.T) {
 
  assert.Equal(t, "test-ext-2", newRev.Name)
  assert.Equal(t, int64(2), newRev.Spec.Revision)
- assert.Equal(t, "9d0e48f6830fce1be5f510eb996f2876719fdb8bcffcfe1dfd3fd60e56316424", newRev.Annotations[applier.RevisionHashAnnotation])
+ assert.Equal(t, "1fqrim12vefkogp3pwxwhcs7c0pi1z1t2fw4roxu81sv", newRev.Annotations[applier.RevisionHashAnnotation])
  require.Len(t, newRev.Spec.Previous, 1)
  assert.Equal(t, "test-ext-1", newRev.Spec.Previous[0].Name)
  assert.Equal(t, types.UID("rev-uid-1"), newRev.Spec.Previous[0].UID)

internal/operator-controller/rukpak/render/registryv1/generators/generators.go

@@ -122,10 +122,7 @@ func BundleCSVPermissionsGenerator(rv1 *bundle.RegistryV1, opts render.Options)
 	for _, ns := range opts.TargetNamespaces {
  for _, permission := range permissions {
  saName := saNameOrDefault(permission.ServiceAccountName)
- name, err := opts.UniqueNameGenerator(fmt.Sprintf("%s-%s", rv1.CSV.Name, saName), permission)
- if err != nil {
- return nil, err
- }
+ name := opts.UniqueNameGenerator(fmt.Sprintf("%s-%s", rv1.CSV.Name, saName), permission)
 
  objs = append(objs,
  CreateRoleResource(name, ns, WithRules(permission.Rules...)),
@@ -167,10 +164,7 @@ func BundleCSVClusterPermissionsGenerator(rv1 *bundle.RegistryV1, opts render.Op
 	objs := make([]client.Object, 0, 2*len(clusterPermissions))
 	for _, permission := range clusterPermissions {
  saName := saNameOrDefault(permission.ServiceAccountName)
- name, err := opts.UniqueNameGenerator(fmt.Sprintf("%s-%s", rv1.CSV.Name, saName), permission)
- if err != nil {
- return nil, err
- }
+ name := opts.UniqueNameGenerator(fmt.Sprintf("%s-%s", rv1.CSV.Name, saName), permission)
  objs = append(objs,
  CreateClusterRoleResource(name, WithRules(permission.Rules...)),
  CreateClusterRoleBindingResource(

internal/operator-controller/rukpak/render/registryv1/generators/generators_test.go

@@ -352,8 +352,8 @@ func Test_BundleCSVDeploymentGenerator_FailsOnNil(t *testing.T) {
 }
 
 func Test_BundleCSVPermissionsGenerator_Succeeds(t *testing.T) {
-	fakeUniqueNameGenerator := func(base string, _ interface{}) (string, error) {
- return base, nil
+	fakeUniqueNameGenerator := func(base string, _ interface{}) string {
+ return base
 	}
 
 	for _, tc := range []struct {
@@ -786,8 +786,8 @@ func Test_BundleCSVPermissionGenerator_FailsOnNil(t *testing.T) {
 }
 
 func Test_BundleCSVClusterPermissionsGenerator_Succeeds(t *testing.T) {
-	fakeUniqueNameGenerator := func(base string, _ interface{}) (string, error) {
- return base, nil
+	fakeUniqueNameGenerator := func(base string, _ interface{}) string {
+ return base
 	}
 
 	for _, tc := range []struct {

internal/operator-controller/rukpak/render/render.go

@@ -12,6 +12,7 @@ import (
 
 	"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/util"
+	hashutil "github.com/operator-framework/operator-controller/internal/shared/util/hash"
 )
 
 // BundleValidator validates a RegistryV1 bundle by executing a series of
@@ -54,7 +55,7 @@ func (r ResourceGenerators) ResourceGenerator() ResourceGenerator {
 	return r.GenerateResources
 }
 
-type UniqueNameGenerator func(string, interface{}) (string, error)
+type UniqueNameGenerator func(string, interface{}) string
 
 type Options struct {
 	InstallNamespace string
@@ -140,12 +141,9 @@ func (r BundleRenderer) Render(rv1 bundle.RegistryV1, installNamespace string, o
 	return objs, nil
 }
 
-func DefaultUniqueNameGenerator(base string, o interface{}) (string, error) {
-	hashStr, err := util.DeepHashObject(o)
-	if err != nil {
- return "", err
-	}
-	return util.ObjectNameForBaseAndSuffix(base, hashStr), nil
+func DefaultUniqueNameGenerator(base string, o interface{}) string {
+	hashStr := hashutil.DeepHashObject(o)
+	return util.ObjectNameForBaseAndSuffix(base, hashStr)
 }
 
 func validateTargetNamespaces(rv1 *bundle.RegistryV1, installNamespace string, targetNamespaces []string) error {

internal/operator-controller/rukpak/render/render_test.go

@@ -206,10 +206,10 @@ func Test_WithUniqueNameGenerator(t *testing.T) {
 	opts := &render.Options{
  UniqueNameGenerator: render.DefaultUniqueNameGenerator,
 	}
-	render.WithUniqueNameGenerator(func(s string, i interface{}) (string, error) {
- return "a man needs a name", nil
+	render.WithUniqueNameGenerator(func(s string, i interface{}) string {
+ return "a man needs a name"
 	})(opts)
-	generatedName, _ := opts.UniqueNameGenerator("", nil)
+	generatedName := opts.UniqueNameGenerator("", nil)
 	require.Equal(t, "a man needs a name", generatedName)
 }
 

internal/operator-controller/rukpak/util/hash.go → internal/shared/util/hash/hash.go

@@ -1,4 +1,4 @@
-package util
+package hash
 
 import (
 	"crypto/sha256"
@@ -7,10 +7,10 @@ import (
 	"math/big"
 )
 
-// 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.
-func DeepHashObject(obj interface{}) (string, error) {
+// DeepHashObject writes specified object to hash based on the object's
+// JSON representation. If the object fails JSON marshaling, DeepHashObject
+// panics.
+func DeepHashObject(obj interface{}) string {
 	// While the most accurate encoding we could do for Kubernetes objects (runtime.Object)
 	// would use the API machinery serializers, those operate over entire objects - and
 	// we often need to operate on snippets. Checking with the experts and the implementation,
@@ -22,18 +22,19 @@ func DeepHashObject(obj interface{}) (string, error) {
 	// will be encoded
 
 	hasher := sha256.New224()
-	hasher.Reset()
 	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
+	// TODO: Investigate whether we can change this to base62(sha224(bytes))
+	// The main concern with changing the base is that it will cause the hash
+	// function output to change, which may cause issues with consumers expecting
+	// a consistent hash.
+	//
+	// base36(sha224(bytes)) is a useful hash and encoding for adding the contents of this
 	// to a Kubernetes identifier or other field which has length and character set requirements
-	var hash []byte
-	hash = hasher.Sum(hash)
-
 	var i big.Int
-	i.SetBytes(hash[:])
-	return i.Text(36), nil
+	i.SetBytes(hasher.Sum(nil))
+	return i.Text(36)
 }

internal/operator-controller/rukpak/util/hash_test.go → internal/shared/util/hash/hash_test.go

@@ -1,24 +1,30 @@
-package util_test
+package hash_test
 
 import (
+	"errors"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
-	"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/util"
+hashutil "github.com/operator-framework/operator-controller/internal/shared/util/hash"
 )
 
+type unmarshalable struct{}
+
+func (u *unmarshalable) MarshalJSON() ([]byte, error) {
+	return nil, errors.New("unmarshalable")
+}
+
 func TestDeepHashObject(t *testing.T) {
 	tests := []struct {
  name string
-wantErr  bool
+wantPanic bool
  obj interface{}
  expectedHash string
 	}{
  {
- name: "populated obj with exported fields",
- wantErr: false,
+ name: "populated obj with exported fields",
  obj: struct {
  Str string
  Num int
@@ -37,8 +43,7 @@ func TestDeepHashObject(t *testing.T) {
  expectedHash: "gta1bt5sybll5qjqxdiekmjm7py93glrinmnrfb31fj",
  },
  {
- name: "modified populated obj with exported fields",
- wantErr: false,
+ name: "modified populated obj with exported fields",
  obj: struct {
  Str string
  Num int
@@ -57,8 +62,7 @@ func TestDeepHashObject(t *testing.T) {
  expectedHash: "1ftn1z2ieih8hsmi2a8c6mkoef6uodrtn4wtt1qapioh",
  },
  {
- name: "populated obj with unexported fields",
- wantErr: false,
+ name: "populated obj with unexported fields",
  obj: struct {
  str string
  num int
@@ -80,38 +84,42 @@ func TestDeepHashObject(t *testing.T) {
  // The JSON encoder requires exported fields or it will generate
  // the same hash as a completely empty object
  name: "empty obj",
- wantErr: false,
  obj: struct{}{},
  expectedHash: "16jfjhihxbzhfhs1k5mimq740kvioi98pfbea9q6qtf9",
  },
  {
  name: "string a",
- wantErr: false,
  obj: "a",
  expectedHash: "1lu1qv1451mq7gv9upu1cx8ffffi07rel5xvbvvc44dh",
  },
  {
  name: "string b",
- wantErr: false,
  obj: "b",
  expectedHash: "1ija85ah4gd0beltpfhszipkxfyqqxhp94tf2mjfgq61",
  },
  {
  name: "nil obj",
- wantErr: false,
  obj: nil,
  expectedHash: "2im0kl1kwvzn46sr4cdtkvmdzrlurvj51xdzhwdht8l0",
  },
+ {
+ name: "unmarshalable obj",
+ obj: &unmarshalable{},
+ wantPanic: true,
+ },
 	}
 	for _, tc := range tests {
  t.Run(tc.name, func(t *testing.T) {
- hash, err := util.DeepHashObject(tc.obj)
- if tc.wantErr {
- require.Error(t, err)
- } else {
- require.NoError(t, err)
+ test := func() {
+ hash := hashutil.DeepHashObject(tc.obj)
  assert.Equal(t, tc.expectedHash, hash)
  }
+
+ if tc.wantPanic {
+ require.Panics(t, test)
+ } else {
+ require.NotPanics(t, test)
+ }
  })
 	}
 }