promote rukpak hash util to shared package, use in boxcutter applier (operator-framework#2201)

Signed-off-by: Joe Lanford <[email protected]>

Author: joelanford

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 renamed to 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 renamed to 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)
+			}
 		})
 	}
 }