🌱 OPRUN-4122 Drop hash computation of ClusterExtensionRevision phases (#2245)

* Drop hash computation of `ClusterExtensionRevision` phases

Applier can decide if a new `ClusterExtensionRevision` needs to be created
without computing the digest all objects in phases:

* try to patch the current revision
* if the operation fails due to invalid payload, it is a signal that we tried
to update an immutable field (phases included)
* in that case, create a new revision

Benefits:
* No need to keep the computed digest attached to `ClusterExtensionRevision` as annotation
* Revisions are created using SSA, passing the right field owner
* Simpler applier logic

Changes:
* Unit tests updated, rephrasing their names to better reflect the use case scenario under test
* Added test-operator 1.2.0 bundle to be able to assert creation of new revision in added e2e `TestClusterExtensionForceInstallNonSuccessorVersion` test
* Helper function previously living in `test/e2e/cluster_extension_install_test.go` extracted into `test/helpers/helpers.go` so that it could be used in
  `test/experimental-e2e/experimental_e2e_test.go` as well

* Address reviewer comments

Author: pedjak

cmd/operator-controller/main.go

@@ -107,7 +107,10 @@ type config struct {
 	globalPullSecret     string
 }
 
-const authFilePrefix = "operator-controller-global-pull-secrets"
+const (
+	authFilePrefix   = "operator-controller-global-pull-secrets"
+	fieldOwnerPrefix = "olm.operatorframework.io"
+)
 
 // podNamespace checks whether the controller is running in a Pod vs.
 // being run locally by inspecting the namespace file that gets mounted
@@ -560,6 +563,7 @@ func setupBoxcutter(
 		Scheme:            mgr.GetScheme(),
 		RevisionGenerator: rg,
 		Preflights:        preflights,
+		FieldOwner:        fmt.Sprintf("%s/clusterextension-controller", fieldOwnerPrefix),
 	}
 	ceReconciler.RevisionStatesGetter = &controllers.BoxcutterRevisionStatesGetter{Reader: mgr.GetClient()}
 	ceReconciler.StorageMigrator = &applier.BoxcutterStorageMigrator{
@@ -568,11 +572,6 @@ func setupBoxcutter(
 		RevisionGenerator:  rg,
 	}
 
-	// Boxcutter
-	const (
-		boxcutterSystemPrefixFieldOwner = "olm.operatorframework.io"
-	)
-
 	discoveryClient, err := discovery.NewDiscoveryClientForConfig(mgr.GetConfig())
 	if err != nil {
 		return fmt.Errorf("unable to create discovery client: %w", err)
@@ -599,8 +598,8 @@ func setupBoxcutter(
 				machinery.NewObjectEngine(
 					mgr.GetScheme(), trackingCache, mgr.GetClient(),
 					ownerhandling.NewNative(mgr.GetScheme()),
-					machinery.NewComparator(ownerhandling.NewNative(mgr.GetScheme()), discoveryClient, mgr.GetScheme(), boxcutterSystemPrefixFieldOwner),
-					boxcutterSystemPrefixFieldOwner, boxcutterSystemPrefixFieldOwner,
+					machinery.NewComparator(ownerhandling.NewNative(mgr.GetScheme()), discoveryClient, mgr.GetScheme(), fieldOwnerPrefix),
+					fieldOwnerPrefix, fieldOwnerPrefix,
 				),
 				validation.NewClusterPhaseValidator(mgr.GetRESTMapper(), mgr.GetClient()),
 			),

internal/operator-controller/applier/boxcutter.go

@@ -27,11 +27,9 @@ import (
 	ocv1 "github.com/operator-framework/operator-controller/api/v1"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/controllers"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/labels"
-	hashutil "github.com/operator-framework/operator-controller/internal/shared/util/hash"
 )
 
 const (
-	RevisionHashAnnotation                = "olm.operatorframework.io/hash"
 	ClusterExtensionRevisionPreviousLimit = 5
 )
 
@@ -200,6 +198,7 @@ type Boxcutter struct {
 	Scheme            *runtime.Scheme
 	RevisionGenerator ClusterExtensionRevisionGenerator
 	Preflights        []Preflight
+	FieldOwner        string
 }
 
 func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (bool, string, error) {
@@ -216,34 +215,56 @@ func (bc *Boxcutter) getObjects(rev *ocv1.ClusterExtensionRevision) []client.Obj
 	return objs
 }
 
+func (bc *Boxcutter) createOrUpdate(ctx context.Context, obj client.Object) error {
+	if obj.GetObjectKind().GroupVersionKind().Empty() {
+		gvk, err := apiutil.GVKForObject(obj, bc.Scheme)
+		if err != nil {
+			return err
+		}
+		obj.GetObjectKind().SetGroupVersionKind(gvk)
+	}
+	return bc.Client.Patch(ctx, obj, client.Apply, client.FieldOwner(bc.FieldOwner), client.ForceOwnership)
+}
+
 func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (bool, string, error) {
 	// Generate desired revision
 	desiredRevision, err := bc.RevisionGenerator.GenerateRevision(contentFS, ext, objectLabels, revisionAnnotations)
 	if err != nil {
 		return false, "", err
 	}
 
+	if err := controllerutil.SetControllerReference(ext, desiredRevision, bc.Scheme); err != nil {
+		return false, "", fmt.Errorf("set ownerref: %w", err)
+	}
+
 	// List all existing revisions
 	existingRevisions, err := bc.getExistingRevisions(ctx, ext.GetName())
 	if err != nil {
 		return false, "", err
 	}
-	desiredHash := hashutil.DeepHashObject(desiredRevision.Spec.Phases)
 
-	// Sort into current and previous revisions.
-	var (
-		currentRevision *ocv1.ClusterExtensionRevision
-	)
+	currentRevision := &ocv1.ClusterExtensionRevision{}
 	state := StateNeedsInstall
+	// check if we can update the current revision.
 	if len(existingRevisions) > 0 {
-		maybeCurrentRevision := existingRevisions[len(existingRevisions)-1]
-		annotations := maybeCurrentRevision.GetAnnotations()
-		if annotations != nil {
-			if revisionHash, ok := annotations[RevisionHashAnnotation]; ok && revisionHash == desiredHash {
-				currentRevision = &maybeCurrentRevision
-			}
+		// try first to update the current revision.
+		currentRevision = &existingRevisions[len(existingRevisions)-1]
+		desiredRevision.Spec.Previous = currentRevision.Spec.Previous
+		desiredRevision.Spec.Revision = currentRevision.Spec.Revision
+		desiredRevision.Name = currentRevision.Name
+
+		err := bc.createOrUpdate(ctx, desiredRevision)
+		switch {
+		case apierrors.IsInvalid(err):
+			// We could not update the current revision due to trying to update an immutable field.
+			// Therefore, we need to create a new revision.
+			state = StateNeedsUpgrade
+		case err == nil:
+			// inplace patch was successful, no changes in phases
+			state = StateUnchanged
+		default:
+			return false, "", fmt.Errorf("patching %s Revision: %w", desiredRevision.Name, err)
 		}
-		state = StateNeedsUpgrade
 	}
 
 	// Preflights
@@ -270,30 +291,22 @@ func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust
 		}
 	}
 
-	if currentRevision == nil {
-		// all Revisions are outdated => create a new one.
+	if state != StateUnchanged {
+		// need to create new revision
 		prevRevisions := existingRevisions
 		revisionNumber := latestRevisionNumber(prevRevisions) + 1
 
-		newRevision := desiredRevision
-		newRevision.Name = fmt.Sprintf("%s-%d", ext.Name, revisionNumber)
-		if newRevision.GetAnnotations() == nil {
-			newRevision.Annotations = map[string]string{}
-		}
-		newRevision.Annotations[RevisionHashAnnotation] = desiredHash
-		newRevision.Spec.Revision = revisionNumber
+		desiredRevision.Name = fmt.Sprintf("%s-%d", ext.Name, revisionNumber)
+		desiredRevision.Spec.Revision = revisionNumber
 
-		if err = bc.setPreviousRevisions(ctx, newRevision, prevRevisions); err != nil {
+		if err = bc.setPreviousRevisions(ctx, desiredRevision, prevRevisions); err != nil {
 			return false, "", fmt.Errorf("garbage collecting old Revisions: %w", err)
 		}
 
-		if err := controllerutil.SetControllerReference(ext, newRevision, bc.Scheme); err != nil {
-			return false, "", fmt.Errorf("set ownerref: %w", err)
-		}
-		if err := bc.Client.Create(ctx, newRevision); err != nil {
+		if err := bc.createOrUpdate(ctx, desiredRevision); err != nil {
 			return false, "", fmt.Errorf("creating new Revision: %w", err)
 		}
-		currentRevision = newRevision
+		currentRevision = desiredRevision
 	}
 
 	progressingCondition := meta.FindStatusCondition(currentRevision.Status.Conditions, ocv1.TypeProgressing)