use stable hash for labels and name suffixes (aligning with OLMv0 cha…

…nges)

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

go.mod

@@ -3,7 +3,6 @@ module github.com/operator-framework/rukpak
 go 1.20
 
 require (
-	github.com/davecgh/go-spew v1.1.1
 	github.com/go-git/go-billy/v5 v5.5.0
 	github.com/go-git/go-git/v5 v5.9.0
 	github.com/go-logr/logr v1.2.4
@@ -61,6 +60,7 @@ require (
 	github.com/containerd/continuity v0.3.0 // indirect
 	github.com/containerd/ttrpc v1.1.0 // indirect
 	github.com/cyphar/filepath-securejoin v0.2.4 // indirect
+	github.com/davecgh/go-spew v1.1.1 // indirect
 	github.com/docker/cli v20.10.21+incompatible // indirect
 	github.com/docker/distribution v2.8.2+incompatible // indirect
 	github.com/docker/docker v20.10.24+incompatible // indirect

internal/convert/registryv1.go

@@ -4,7 +4,6 @@ import (
 	"bytes"
 	"errors"
 	"fmt"
-	"hash/fnv"
 	"io"
 	"io/fs"
 	"path/filepath"
@@ -13,21 +12,19 @@ import (
 	"time"
 
 	"github.com/operator-framework/api/pkg/operators/v1alpha1"
+	registrybundle "github.com/operator-framework/operator-registry/pkg/lib/bundle"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apimachinery/pkg/util/rand"
 	"k8s.io/apimachinery/pkg/util/sets"
 	apimachyaml "k8s.io/apimachinery/pkg/util/yaml"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/yaml"
 
-	registrybundle "github.com/operator-framework/operator-registry/pkg/lib/bundle"
-
 	registry "github.com/operator-framework/rukpak/internal/operator-registry"
 	"github.com/operator-framework/rukpak/internal/util"
 )
@@ -279,13 +276,19 @@ func Convert(in RegistryV1, installNamespace string, targetNamespaces []string)
 
 	for _, permission := range permissions {
 		saName := saNameOrDefault(permission.ServiceAccountName)
-		name := generateName(fmt.Sprintf("%s-%s", in.CSV.Name, saName), []interface{}{in.CSV.Name, permission})
+		name, err := generateName(fmt.Sprintf("%s-%s", in.CSV.Name, saName), permission)
+		if err != nil {
+			return nil, err
+		}
 		roles = append(roles, newRole(installNamespace, name, permission.Rules))
 		roleBindings = append(roleBindings, newRoleBinding(installNamespace, name, name, installNamespace, saName))
 	}
 	for _, permission := range clusterPermissions {
 		saName := saNameOrDefault(permission.ServiceAccountName)
-		name := generateName(fmt.Sprintf("%s-%s", in.CSV.Name, saName), []interface{}{in.CSV.GetName(), permission})
+		name, err := generateName(fmt.Sprintf("%s-%s", in.CSV.Name, saName), permission)
+		if err != nil {
+			return nil, err
+		}
 		clusterRoles = append(clusterRoles, newClusterRole(name, permission.Rules))
 		clusterRoleBindings = append(clusterRoleBindings, newClusterRoleBinding(name, name, installNamespace, saName))
 	}
@@ -341,16 +344,16 @@ func Convert(in RegistryV1, installNamespace string, targetNamespaces []string)
 
 const maxNameLength = 63
 
-func generateName(base string, o interface{}) string {
-	hasher := fnv.New32a()
-
-	util.DeepHashObject(hasher, o)
-	hashStr := rand.SafeEncodeString(fmt.Sprint(hasher.Sum32()))
+func generateName(base string, o interface{}) (string, error) {
+	hashStr, err := util.DeepHashObject(o)
+	if err != nil {
+		return "", err
+	}
 	if len(base)+len(hashStr) > maxNameLength {
 		base = base[:maxNameLength-len(hashStr)-1]
 	}
 
-	return fmt.Sprintf("%s-%s", base, hashStr)
+	return fmt.Sprintf("%s-%s", base, hashStr), nil
 }
 
 func newServiceAccount(namespace, name string) corev1.ServiceAccount {

internal/util/hash.go

@@ -1,22 +1,39 @@
 package util
 
 import (
-	"hash"
-
-	"github.com/davecgh/go-spew/spew"
+	"crypto/sha256"
+	"encoding/json"
+	"fmt"
+	"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.
-// From https://github.com/operator-framework/operator-lifecycle-manager/blob/master/pkg/lib/kubernetes/pkg/util/hash/hash.go
-func DeepHashObject(hasher hash.Hash, objectToWrite interface{}) {
+func DeepHashObject(obj interface{}) (string, error) {
+	// 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,
+	// we can see that the serializers are a thin wrapper over json.Marshal for encoding:
+	// https://github.com/kubernetes/kubernetes/blob/8509ab82b96caa2365552efa08c8ba8baf11c5ec/staging/src/k8s.io/apimachinery/pkg/runtime/serializer/json/json.go#L216-L247
+	// Therefore, we can be confident that using json.Marshal() here will:
+	//  1. be stable & idempotent - the library sorts keys, etc.
+	//  2. be germane to our needs - only fields that serialize and are sent to the server
+	//     will be encoded
+
+	hasher := sha256.New224()
 	hasher.Reset()
-	printer := spew.ConfigState{
-		Indent:         " ",
-		SortKeys:       true,
-		DisableMethods: true,
-		SpewKeys:       true,
+	encoder := json.NewEncoder(hasher)
+	if err := encoder.Encode(obj); err != nil {
+		return "", fmt.Errorf("couldn't encode object: %w", err)
 	}
-	printer.Fprintf(hasher, "%#v", objectToWrite)
+
+	// base62(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
 }

internal/util/util.go

@@ -6,7 +6,6 @@ import (
 	"encoding/pem"
 	"errors"
 	"fmt"
-	"hash/fnv"
 	"io"
 	"os"
 	"sort"
@@ -21,7 +20,6 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/selection"
 	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/apimachinery/pkg/util/rand"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/cli-runtime/pkg/resource"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -67,10 +65,16 @@ func ReconcileDesiredBundle(ctx context.Context, c client.Client, bd *rukpakv1al
 
 	// check whether there's an existing Bundle that matches the desired Bundle template
 	// specified in the BI resource, and if not, generate a new Bundle that matches the template.
-	b := CheckExistingBundlesMatchesTemplate(existingBundles, bd.Spec.Template)
+	b, err := CheckExistingBundlesMatchesTemplate(existingBundles, bd.Spec.Template)
+	if err != nil {
+		return nil, nil, err
+	}
 	if b == nil {
 		controllerRef := metav1.NewControllerRef(bd, bd.GroupVersionKind())
-		hash := GenerateTemplateHash(bd.Spec.Template)
+		hash, err := DeepHashObject(bd.Spec.Template)
+		if err != nil {
+			return nil, nil, err
+		}
 
 		labels := bd.Spec.Template.Labels
 		if len(labels) == 0 {
@@ -270,44 +274,64 @@ func GetBundlesForBundleDeploymentSelector(ctx context.Context, c client.Client,
 // CheckExistingBundlesMatchesTemplate evaluates whether the existing list of Bundle objects
 // match the desired Bundle template that's specified in a BundleDeployment object. If a match
 // is found, that Bundle object is returned, so callers are responsible for nil checking the result.
-func CheckExistingBundlesMatchesTemplate(existingBundles *rukpakv1alpha1.BundleList, desiredBundleTemplate rukpakv1alpha1.BundleTemplate) *rukpakv1alpha1.Bundle {
+func CheckExistingBundlesMatchesTemplate(existingBundles *rukpakv1alpha1.BundleList, desiredBundleTemplate rukpakv1alpha1.BundleTemplate) (*rukpakv1alpha1.Bundle, error) {
 	for i := range existingBundles.Items {
-		if !CheckDesiredBundleTemplate(&existingBundles.Items[i], desiredBundleTemplate) {
+		ok, err := CheckDesiredBundleTemplate(&existingBundles.Items[i], desiredBundleTemplate)
+		if err != nil {
+			return nil, err
+		}
+		if !ok {
 			continue
 		}
-		return existingBundles.Items[i].DeepCopy()
+		return existingBundles.Items[i].DeepCopy(), nil
 	}
-	return nil
+	return nil, nil
 }
 
 // CheckDesiredBundleTemplate is responsible for determining whether the existingBundle
 // hash is equal to the desiredBundle Bundle template hash.
-func CheckDesiredBundleTemplate(existingBundle *rukpakv1alpha1.Bundle, desiredBundle rukpakv1alpha1.BundleTemplate) bool {
+func CheckDesiredBundleTemplate(existingBundle *rukpakv1alpha1.Bundle, desiredBundle rukpakv1alpha1.BundleTemplate) (bool, error) {
 	if len(existingBundle.Labels) == 0 {
 		// Existing Bundle has no labels set, which should never be the case.
 		// Return false so that the Bundle is forced to be recreated with the expected labels.
-		return false
+		return false, nil
 	}
 
 	existingHash, ok := existingBundle.Labels[CoreBundleTemplateHashKey]
 	if !ok {
 		// Existing Bundle has no template hash associated with it.
 		// Return false so that the Bundle is forced to be recreated with the template hash label.
-		return false
+		return false, nil
 	}
 
 	// Check whether the hash of the desired bundle template matches the existing bundle on-cluster.
-	desiredHash := GenerateTemplateHash(desiredBundle)
-	return existingHash == desiredHash
+	desiredHash, err := DeepHashObject(desiredBundle)
+	if err != nil {
+		return false, err
+	}
+	return existingHash == desiredHash, nil
 }
 
-func GenerateTemplateHash(template rukpakv1alpha1.BundleTemplate) string {
-	hasher := fnv.New32a()
-	DeepHashObject(hasher, template)
-	return rand.SafeEncodeString(fmt.Sprintf("%x", hasher.Sum32())[:6])
-}
+const (
+	// maxBundleNameLength must be aligned with the Bundle CRD metadata.name length validation, defined in:
+	// <repoRoot>/manifests/base/apis/crds/patches/bundle_validation.yaml
+	maxBundleNameLength = 52
+
+	// maxBundleDeploymentNameLength must be aligned with the BundleDeployment CRD metadata.name length validation,
+	// defined in: <repoRoot>/manifests/base/apis/crds/patches/bundledeployment_validation.yaml
+	maxBundleDeploymentNameLength = 45
+)
 
 func GenerateBundleName(bdName, hash string) string {
+	if len(bdName) > maxBundleDeploymentNameLength {
+		// This should never happen because we have validation on the BundleDeployment CRD to ensure
+		// that the name is no more than 45 characters. But just to be safe...
+		bdName = bdName[:maxBundleDeploymentNameLength]
+	}
+
+	if len(hash) > maxBundleNameLength-len(bdName)-1 {
+		hash = hash[:maxBundleNameLength-len(bdName)-1]
+	}
 	return fmt.Sprintf("%s-%s", bdName, hash)
 }
 

internal/util/util_test.go

@@ -163,8 +163,12 @@ func TestCheckDesiredBundleTemplate(t *testing.T) {
 			// Dynamically inject the bundle template hash at runtime into the tests.
 			// This is due to the nature of the objects being passed in (pointers to BundleTemplates) being represented
 			// differently on different platforms, so hardcoding the hash values produces inconsistent results.
-			injectTemplateHashLabel(tt.args.existingBundle, tt.args.desiredBundle, tt.want)
-			if got := CheckDesiredBundleTemplate(tt.args.existingBundle, tt.args.desiredBundle); got != tt.want {
+			injectTemplateHashLabel(t, tt.args.existingBundle, tt.args.desiredBundle, tt.want)
+			got, err := CheckDesiredBundleTemplate(tt.args.existingBundle, tt.args.desiredBundle)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if got != tt.want {
 				t.Errorf("CheckDesiredBundleTemplate() = %v, want %v", got, tt.want)
 			}
 		})
@@ -180,10 +184,14 @@ func injectCoreLabels(bundle *rukpakv1alpha1.Bundle) {
 	labels[CoreOwnerNameKey] = ""
 }
 
-func injectTemplateHashLabel(bundle *rukpakv1alpha1.Bundle, template rukpakv1alpha1.BundleTemplate, want bool) {
+func injectTemplateHashLabel(t *testing.T, bundle *rukpakv1alpha1.Bundle, template rukpakv1alpha1.BundleTemplate, want bool) {
 	labels := bundle.GetLabels()
 	if want {
-		labels[CoreBundleTemplateHashKey] = GenerateTemplateHash(template)
+		hash, err := DeepHashObject(template)
+		if err != nil {
+			t.Fatal(err)
+		}
+		labels[CoreBundleTemplateHashKey] = hash
 	} else {
 		labels[CoreBundleTemplateHashKey] = "00000000"
 	}

test/e2e/plain_provisioner_test.go

@@ -1522,7 +1522,11 @@ var _ = Describe("plain provisioner bundledeployment", func() {
 					util.SortBundlesByCreation(existingBundles)
 					// Note: existing bundles are sorted by metadata.CreationTimestamp, so select
 					// the Bundle that was generated second to compare to the desired Bundle template.
-					return util.CheckDesiredBundleTemplate(&existingBundles.Items[1], bd.Spec.Template)
+					ok, err := util.CheckDesiredBundleTemplate(&existingBundles.Items[1], bd.Spec.Template)
+					if err != nil {
+						return false
+					}
+					return ok
 				}).Should(BeTrue())
 			})
 
@@ -1594,7 +1598,11 @@ var _ = Describe("plain provisioner bundledeployment", func() {
 					if err := c.Get(ctx, types.NamespacedName{Name: bd.Status.ActiveBundle}, currBundle); err != nil {
 						return false
 					}
-					return util.CheckDesiredBundleTemplate(currBundle, bd.Spec.Template)
+					ok, err := util.CheckDesiredBundleTemplate(currBundle, bd.Spec.Template)
+					if err != nil {
+						return false
+					}
+					return ok
 				}).Should(BeTrue())
 			})
 			It("should delete the old Bundle once the newly generated Bundle reports a successful installation state", func() {