add BoxcutterInstalledBundleGetter, plumb bundle metadata into revision annotations

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

Author: joelanford

cmd/operator-controller/main.go

@@ -441,8 +441,11 @@ func run() error {
 	}
 
 	// create applier
-	var ctrlBuilderOpts []controllers.ControllerBuilderOption
-	var extApplier controllers.Applier
+	var (
+		ctrlBuilderOpts       []controllers.ControllerBuilderOption
+		extApplier            controllers.Applier
+		installedBundleGetter controllers.InstalledBundleGetter
+	)
 	certProvider := getCertificateProvider()
 	if features.OperatorControllerFeatureGate.Enabled(features.BoxcutterRuntime) {
 		// TODO: add support for preflight checks
@@ -460,6 +463,7 @@ func run() error {
 			},
 			Preflights: preflights,
 		}
+		installedBundleGetter = &controllers.BoxcutterInstalledBundleGetter{Reader: mgr.GetClient()}
 		ctrlBuilderOpts = append(ctrlBuilderOpts, controllers.WithOwns(&ocv1.ClusterExtensionRevision{}))
 	} else {
 		// now initialize the helmApplier, assigning the potentially nil preAuth
@@ -474,6 +478,7 @@ func run() error {
 			PreAuthorizer:                 preAuth,
 			HelmReleaseToObjectsConverter: &applier.HelmReleaseToObjectsConverter{},
 		}
+		installedBundleGetter = &controllers.HelmInstalledBundleGetter{ActionClientGetter: acg}
 	}
 
 	cm := contentmanager.NewManager(clientRestConfigMapper, mgr.GetConfig(), mgr.GetRESTMapper())
@@ -493,7 +498,7 @@ func run() error {
 		ImageCache:            imageCache,
 		ImagePuller:           imagePuller,
 		Applier:               extApplier,
-		InstalledBundleGetter: &controllers.DefaultInstalledBundleGetter{ActionClientGetter: acg},
+		InstalledBundleGetter: installedBundleGetter,
 		Finalizers:            clusterExtensionFinalizers,
 		Manager:               cm,
 	}).SetupWithManager(mgr, ctrlBuilderOpts...); err != nil {

internal/operator-controller/applier/boxcutter.go

@@ -30,15 +30,15 @@ const (
 )
 
 type ClusterExtensionRevisionGenerator interface {
-	GenerateRevision(bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels map[string]string) (*ocv1.ClusterExtensionRevision, error)
+	GenerateRevision(bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error)
 }
 
 type SimpleRevisionGenerator struct {
 	Scheme         *runtime.Scheme
 	BundleRenderer BundleRenderer
 }
 
-func (r *SimpleRevisionGenerator) GenerateRevision(bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels map[string]string) (*ocv1.ClusterExtensionRevision, error) {
+func (r *SimpleRevisionGenerator) GenerateRevision(bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
 	// extract plain manifests
 	plain, err := r.BundleRenderer.Render(bundleFS, ext)
 	if err != nil {
@@ -74,10 +74,14 @@ func (r *SimpleRevisionGenerator) GenerateRevision(bundleFS fs.FS, ext *ocv1.Clu
 		})
 	}
 
+	if revisionAnnotations == nil {
+		revisionAnnotations = map[string]string{}
+	}
+
 	// Build desired revision
 	return &ocv1.ClusterExtensionRevision{
 		ObjectMeta: metav1.ObjectMeta{
-			Annotations: map[string]string{},
+			Annotations: revisionAnnotations,
 			Labels: map[string]string{
 				controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
 			},
@@ -100,8 +104,8 @@ type Boxcutter struct {
 	Preflights        []Preflight
 }
 
-func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, storageLabels map[string]string) ([]client.Object, string, error) {
-	objs, err := bc.apply(ctx, contentFS, ext, objectLabels, storageLabels)
+func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) ([]client.Object, string, error) {
+	objs, err := bc.apply(ctx, contentFS, ext, objectLabels, revisionAnnotations)
 	return objs, "", err
 }
 
@@ -115,9 +119,9 @@ func (bc *Boxcutter) getObjects(rev *ocv1.ClusterExtensionRevision) []client.Obj
 	return objs
 }
 
-func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, _ map[string]string) ([]client.Object, error) {
+func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) ([]client.Object, error) {
 	// Generate desired revision
-	desiredRevision, err := bc.RevisionGenerator.GenerateRevision(contentFS, ext, objectLabels)
+	desiredRevision, err := bc.RevisionGenerator.GenerateRevision(contentFS, ext, objectLabels, revisionAnnotations)
 	if err != nil {
 		return nil, err
 	}

internal/operator-controller/controllers/clusterextension_controller.go

@@ -17,10 +17,12 @@ limitations under the License.
 package controllers
 
 import (
+	"cmp"
 	"context"
 	"errors"
 	"fmt"
 	"io/fs"
+	"slices"
 	"strings"
 	"time"
 
@@ -488,7 +490,7 @@ func clusterExtensionRequestsForCatalog(c client.Reader, logger logr.Logger) crh
 	}
 }
 
-type DefaultInstalledBundleGetter struct {
+type HelmInstalledBundleGetter struct {
 	helmclient.ActionClientGetter
 }
 
@@ -497,7 +499,7 @@ type InstalledBundle struct {
 	Image string
 }
 
-func (d *DefaultInstalledBundleGetter) GetInstalledBundle(ctx context.Context, ext *ocv1.ClusterExtension) (*InstalledBundle, error) {
+func (d *HelmInstalledBundleGetter) GetInstalledBundle(ctx context.Context, ext *ocv1.ClusterExtension) (*InstalledBundle, error) {
 	cl, err := d.ActionClientFor(ctx, ext)
 	if err != nil {
 		return nil, err
@@ -526,3 +528,47 @@ func (d *DefaultInstalledBundleGetter) GetInstalledBundle(ctx context.Context, e
 	}
 	return nil, nil
 }
+
+type BoxcutterInstalledBundleGetter struct {
+	client.Reader
+}
+
+func (d *BoxcutterInstalledBundleGetter) GetInstalledBundle(ctx context.Context, ext *ocv1.ClusterExtension) (*InstalledBundle, error) {
+	// TODO: boxcutter applier has a nearly identical bit of code for listing and sorting revisions
+	//   only difference here is that it sorts in reverse order to start iterating with the most
+	//   recent revisions. We should consolidate to avoid code duplication.
+	existingRevisionList := &ocv1.ClusterExtensionRevisionList{}
+	if err := d.Reader.List(ctx, existingRevisionList, client.MatchingLabels{
+		ClusterExtensionRevisionOwnerLabel: ext.Name,
+	}); err != nil {
+		return nil, fmt.Errorf("listing revisions: %w", err)
+	}
+	slices.SortFunc(existingRevisionList.Items, func(a, b ocv1.ClusterExtensionRevision) int {
+		return cmp.Compare(b.Spec.Revision, a.Spec.Revision)
+	})
+
+	for _, rev := range existingRevisionList.Items {
+		if rev.Spec.LifecycleState == ocv1.ClusterExtensionRevisionLifecycleStateActive {
+			// TODO: we should make constants for the ClusterExtensionRevision condition types.
+			if installedCondition := apimeta.FindStatusCondition(rev.Status.Conditions, "Succeeded"); installedCondition == nil || installedCondition.Status != metav1.ConditionTrue {
+				// TODO: It's not great to return this error in a completely normal situation. This currently cascades
+				//   into Installed=False, Failed and Progressing=True, Retrying conditions that give the impression
+				//   that something is wrong. We should probably refactor the InstalledBundleGetter interface to better
+				//   handle the async nature of ClusterExtensionRevision rollouts.
+				return nil, fmt.Errorf("most recent active revision %s is still rolling out", rev.Name)
+			}
+
+			// TODO: the setting of these annotations (happens in boxcutter applier when we pass in "storageLabels")
+			//   is fairly decoupled from this code where we get the annotations back out. We may want to co-locate
+			//   the set/get logic a bit better to make it more maintainable and less likely to get out of sync.
+			return &InstalledBundle{
+				BundleMetadata: ocv1.BundleMetadata{
+					Name:    rev.Annotations[labels.BundleNameKey],
+					Version: rev.Annotations[labels.BundleVersionKey],
+				},
+				Image: rev.Annotations[labels.BundleReferenceKey],
+			}, nil
+		}
+	}
+	return nil, nil
+}

internal/operator-controller/controllers/clusterextension_controller_test.go

@@ -1485,7 +1485,7 @@ func (mag *MockActionGetter) Reconcile(rel *release.Release) error {
 }
 
 func TestGetInstalledBundleHistory(t *testing.T) {
-	getter := controllers.DefaultInstalledBundleGetter{}
+	getter := controllers.HelmInstalledBundleGetter{}
 
 	ext := ocv1.ClusterExtension{
 		ObjectMeta: metav1.ObjectMeta{