diff --git a/.bingo/Variables.mk b/.bingo/Variables.mk index 3f44c355..c9a19313 100644 --- a/.bingo/Variables.mk +++ b/.bingo/Variables.mk @@ -29,11 +29,11 @@ $(CONTROLLER_GEN): $(BINGO_DIR)/controller-gen.mod @echo "(re)installing $(GOBIN)/controller-gen-v0.12.0" @cd $(BINGO_DIR) && GOWORK=off $(GO) build -mod=mod -modfile=controller-gen.mod -o=$(GOBIN)/controller-gen-v0.12.0 "sigs.k8s.io/controller-tools/cmd/controller-gen" -GINKGO := $(GOBIN)/ginkgo-v2.12.0 +GINKGO := $(GOBIN)/ginkgo-v2.13.0 $(GINKGO): $(BINGO_DIR)/ginkgo.mod @# Install binary/ries using Go 1.14+ build command. This is using bwplotka/bingo-controlled, separate go module with pinned dependencies. - @echo "(re)installing $(GOBIN)/ginkgo-v2.12.0" - @cd $(BINGO_DIR) && GOWORK=off $(GO) build -mod=mod -modfile=ginkgo.mod -o=$(GOBIN)/ginkgo-v2.12.0 "github.com/onsi/ginkgo/v2/ginkgo" + @echo "(re)installing $(GOBIN)/ginkgo-v2.13.0" + @cd $(BINGO_DIR) && GOWORK=off $(GO) build -mod=mod -modfile=ginkgo.mod -o=$(GOBIN)/ginkgo-v2.13.0 "github.com/onsi/ginkgo/v2/ginkgo" GOLANGCI_LINT := $(GOBIN)/golangci-lint-v1.53.2 $(GOLANGCI_LINT): $(BINGO_DIR)/golangci-lint.mod diff --git a/.bingo/ginkgo.mod b/.bingo/ginkgo.mod index 3f1454c5..cad695f3 100644 --- a/.bingo/ginkgo.mod +++ b/.bingo/ginkgo.mod @@ -2,4 +2,4 @@ module _ // Auto generated by https://github.com/bwplotka/bingo. DO NOT EDIT go 1.20 -require github.com/onsi/ginkgo/v2 v2.12.0 // ginkgo +require github.com/onsi/ginkgo/v2 v2.13.0 // ginkgo diff --git a/.bingo/ginkgo.sum b/.bingo/ginkgo.sum index c84b10b1..9112eeab 100644 --- a/.bingo/ginkgo.sum +++ b/.bingo/ginkgo.sum @@ -14,6 +14,8 @@ github.com/onsi/ginkgo/v2 v2.8.3 h1:RpbK1G8nWPNaCVFBWsOGnEQQGgASi6b8fxcWBvDYjxQ= github.com/onsi/ginkgo/v2 v2.8.3/go.mod h1:6OaUA8BCi0aZfmzYT/q9AacwTzDpNbxILUT+TlBq6MY= github.com/onsi/ginkgo/v2 v2.12.0 h1:UIVDowFPwpg6yMUpPjGkYvf06K3RAiJXUhCxEwQVHRI= github.com/onsi/ginkgo/v2 v2.12.0/go.mod h1:ZNEzXISYlqpb8S36iN71ifqLi3vVD1rVJGvWRCJOUpQ= +github.com/onsi/ginkgo/v2 v2.13.0 h1:0jY9lJquiL8fcf3M4LAXN5aMlS/b2BV86HFFPCPMgE4= +github.com/onsi/ginkgo/v2 v2.13.0/go.mod h1:TE309ZR8s5FsKKpuB1YAQYBzCaAfUgatB/xlT/ETL/o= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/testify v1.5.1/go.mod h1:5W2xD1RspED5o8YsWQXVCued0rvSQ+mT+I5cxcmMvtA= diff --git a/.bingo/variables.env b/.bingo/variables.env index 35fb704b..9f82ff64 100644 --- a/.bingo/variables.env +++ b/.bingo/variables.env @@ -12,7 +12,7 @@ BINGO="${GOBIN}/bingo-v0.8.0" CONTROLLER_GEN="${GOBIN}/controller-gen-v0.12.0" -GINKGO="${GOBIN}/ginkgo-v2.12.0" +GINKGO="${GOBIN}/ginkgo-v2.13.0" GOLANGCI_LINT="${GOBIN}/golangci-lint-v1.53.2" diff --git a/go.mod b/go.mod index 9f8573dd..546fa8e3 100644 --- a/go.mod +++ b/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 diff --git a/internal/convert/registryv1.go b/internal/convert/registryv1.go index 936433ea..c9ef7095 100644 --- a/internal/convert/registryv1.go +++ b/internal/convert/registryv1.go @@ -4,7 +4,6 @@ import ( "bytes" "errors" "fmt" - "hash/fnv" "io" "io/fs" "path/filepath" @@ -13,6 +12,7 @@ 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" @@ -20,14 +20,11 @@ import ( 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 { diff --git a/internal/util/hash.go b/internal/util/hash.go index 61e54b1f..a46bb343 100644 --- a/internal/util/hash.go +++ b/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 } diff --git a/internal/util/util.go b/internal/util/util.go index 3648d74d..67c5d2f3 100644 --- a/internal/util/util.go +++ b/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: + // /manifests/base/apis/crds/patches/bundle_validation.yaml + maxBundleNameLength = 52 + + // maxBundleDeploymentNameLength must be aligned with the BundleDeployment CRD metadata.name length validation, + // defined in: /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) } diff --git a/internal/util/util_test.go b/internal/util/util_test.go index 5c832a87..5c840af6 100644 --- a/internal/util/util_test.go +++ b/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" } diff --git a/test/e2e/plain_provisioner_test.go b/test/e2e/plain_provisioner_test.go index 04aed04f..289f05f1 100644 --- a/test/e2e/plain_provisioner_test.go +++ b/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() {