diff --git a/go.mod b/go.mod index 2a25f5f78..83f3d2acd 100644 --- a/go.mod +++ b/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 diff --git a/internal/operator-controller/applier/boxcutter.go b/internal/operator-controller/applier/boxcutter.go index c47846498..e56304d98 100644 --- a/internal/operator-controller/applier/boxcutter.go +++ b/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 diff --git a/internal/operator-controller/applier/boxcutter_test.go b/internal/operator-controller/applier/boxcutter_test.go index 0b3b1ea70..4dccb4487 100644 --- a/internal/operator-controller/applier/boxcutter_test.go +++ b/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) diff --git a/internal/operator-controller/rukpak/render/registryv1/generators/generators.go b/internal/operator-controller/rukpak/render/registryv1/generators/generators.go index 233793308..7d5d435ea 100644 --- a/internal/operator-controller/rukpak/render/registryv1/generators/generators.go +++ b/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( diff --git a/internal/operator-controller/rukpak/render/registryv1/generators/generators_test.go b/internal/operator-controller/rukpak/render/registryv1/generators/generators_test.go index 1bdf85232..8176795db 100644 --- a/internal/operator-controller/rukpak/render/registryv1/generators/generators_test.go +++ b/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 { diff --git a/internal/operator-controller/rukpak/render/render.go b/internal/operator-controller/rukpak/render/render.go index 384b16796..a279b5c1e 100644 --- a/internal/operator-controller/rukpak/render/render.go +++ b/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 { diff --git a/internal/operator-controller/rukpak/render/render_test.go b/internal/operator-controller/rukpak/render/render_test.go index 0461ea3be..004c3edb6 100644 --- a/internal/operator-controller/rukpak/render/render_test.go +++ b/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) } diff --git a/internal/operator-controller/rukpak/util/hash.go b/internal/shared/util/hash/hash.go similarity index 62% rename from internal/operator-controller/rukpak/util/hash.go rename to internal/shared/util/hash/hash.go index a46bb3434..c1fac21bf 100644 --- a/internal/operator-controller/rukpak/util/hash.go +++ b/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) } diff --git a/internal/operator-controller/rukpak/util/hash_test.go b/internal/shared/util/hash/hash_test.go similarity index 75% rename from internal/operator-controller/rukpak/util/hash_test.go rename to internal/shared/util/hash/hash_test.go index 9d16dfd64..9e94ffc3e 100644 --- a/internal/operator-controller/rukpak/util/hash_test.go +++ b/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) + } }) } }