refactor Applier interface and improve status reporting

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

Author: joelanford

api/v1/common_types.go

@@ -20,12 +20,18 @@ const (
 	TypeInstalled   = "Installed"
 	TypeProgressing = "Progressing"
 
+	// Installed reasons
+	ReasonAbsent = "Absent"
+
 	// Progressing reasons
-	ReasonSucceeded = "Succeeded"
-	ReasonRetrying  = "Retrying"
-	ReasonBlocked   = "Blocked"
+	ReasonRolloutInProgress = "RolloutInProgress"
+	ReasonRetrying          = "Retrying"
+	ReasonBlocked           = "Blocked"
 
-	// Terminal reasons
+	// Deprecation reasons
 	ReasonDeprecated = "Deprecated"
-	ReasonFailed     = "Failed"
+
+	// Common reasons
+	ReasonSucceeded = "Succeeded"
+	ReasonFailed    = "Failed"
 )

cmd/operator-controller/main.go

@@ -317,38 +317,6 @@ func run() error {
 		return err
 	}
 
-	coreClient, err := corev1client.NewForConfig(mgr.GetConfig())
-	if err != nil {
-		setupLog.Error(err, "unable to create core client")
-		return err
-	}
-	tokenGetter := authentication.NewTokenGetter(coreClient, authentication.WithExpirationDuration(1*time.Hour))
-	clientRestConfigMapper := action.ServiceAccountRestConfigMapper(tokenGetter)
-	if features.OperatorControllerFeatureGate.Enabled(features.SyntheticPermissions) {
-		clientRestConfigMapper = action.SyntheticUserRestConfigMapper(clientRestConfigMapper)
-	}
-
-	cfgGetter, err := helmclient.NewActionConfigGetter(mgr.GetConfig(), mgr.GetRESTMapper(),
-		helmclient.StorageDriverMapper(action.ChunkedStorageDriverMapper(coreClient, mgr.GetAPIReader(), cfg.systemNamespace)),
-		helmclient.ClientNamespaceMapper(func(obj client.Object) (string, error) {
-			ext := obj.(*ocv1.ClusterExtension)
-			return ext.Spec.Namespace, nil
-		}),
-		helmclient.ClientRestConfigMapper(clientRestConfigMapper),
-	)
-	if err != nil {
-		setupLog.Error(err, "unable to config for creating helm client")
-		return err
-	}
-
-	acg, err := action.NewWrappedActionClientGetter(cfgGetter,
-		helmclient.WithFailureRollbacks(false),
-	)
-	if err != nil {
-		setupLog.Error(err, "unable to create helm client")
-		return err
-	}
-
 	certPoolWatcher, err := httputil.NewCertPoolWatcher(cfg.catalogdCasDir, ctrl.Log.WithName("cert-pool"))
 	if err != nil {
 		setupLog.Error(err, "unable to create CA certificate pool")
@@ -434,24 +402,30 @@ func run() error {
 		crdupgradesafety.NewPreflight(aeClient.CustomResourceDefinitions()),
 	}
 
-	// determine if PreAuthorizer should be enabled based on feature gate
-	var preAuth authorization.PreAuthorizer
-	if features.OperatorControllerFeatureGate.Enabled(features.PreflightPermissions) {
-		preAuth = authorization.NewRBACPreAuthorizer(mgr.GetClient())
+	var ctrlBuilderOpts []controllers.ControllerBuilderOption
+	if features.OperatorControllerFeatureGate.Enabled(features.BoxcutterRuntime) {
+		ctrlBuilderOpts = append(ctrlBuilderOpts, controllers.WithOwns(&ocv1.ClusterExtensionRevision{}))
+	}
+
+	ceReconciler := &controllers.ClusterExtensionReconciler{
+		Client:      cl,
+		Resolver:    resolver,
+		ImageCache:  imageCache,
+		ImagePuller: imagePuller,
+		Finalizers:  clusterExtensionFinalizers,
+	}
+	ceController, err := ceReconciler.SetupWithManager(mgr, ctrlBuilderOpts...)
+	if err != nil {
+		setupLog.Error(err, "unable to create controller", "controller", "ClusterExtension")
+		return err
 	}
 
-	// create applier
-	var (
-		ctrlBuilderOpts      []controllers.ControllerBuilderOption
-		extApplier           controllers.Applier
-		revisionStatesGetter controllers.RevisionStatesGetter
-	)
 	certProvider := getCertificateProvider()
 	if features.OperatorControllerFeatureGate.Enabled(features.BoxcutterRuntime) {
 		// TODO: add support for preflight checks
 		// TODO: better scheme handling - which types do we want to support?
 		_ = apiextensionsv1.AddToScheme(mgr.GetScheme())
-		extApplier = &applier.Boxcutter{
+		ceReconciler.Applier = &applier.Boxcutter{
 			Client: mgr.GetClient(),
 			Scheme: mgr.GetScheme(),
 			RevisionGenerator: &applier.SimpleRevisionGenerator{
@@ -463,50 +437,8 @@ func run() error {
 			},
 			Preflights: preflights,
 		}
-		revisionStatesGetter = &controllers.BoxcutterRevisionStatesGetter{Reader: mgr.GetClient()}
-		ctrlBuilderOpts = append(ctrlBuilderOpts, controllers.WithOwns(&ocv1.ClusterExtensionRevision{}))
-	} else {
-		// now initialize the helmApplier, assigning the potentially nil preAuth
-		extApplier = &applier.Helm{
-			ActionClientGetter: acg,
-			Preflights:         preflights,
-			BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{
-				BundleRenderer:          registryv1.Renderer,
-				CertificateProvider:     certProvider,
-				IsWebhookSupportEnabled: certProvider != nil,
-			},
-			PreAuthorizer:                 preAuth,
-			HelmReleaseToObjectsConverter: &applier.HelmReleaseToObjectsConverter{},
-		}
-		revisionStatesGetter = &controllers.HelmRevisionStatesGetter{ActionClientGetter: acg}
-	}
-
-	cm := contentmanager.NewManager(clientRestConfigMapper, mgr.GetConfig(), mgr.GetRESTMapper())
-	err = clusterExtensionFinalizers.Register(controllers.ClusterExtensionCleanupContentManagerCacheFinalizer, finalizers.FinalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) {
-		ext := obj.(*ocv1.ClusterExtension)
-		err := cm.Delete(ext)
-		return crfinalizer.Result{}, err
-	}))
-	if err != nil {
-		setupLog.Error(err, "unable to register content manager cleanup finalizer")
-		return err
-	}
-
-	if err = (&controllers.ClusterExtensionReconciler{
-		Client:               cl,
-		Resolver:             resolver,
-		ImageCache:           imageCache,
-		ImagePuller:          imagePuller,
-		Applier:              extApplier,
-		RevisionStatesGetter: revisionStatesGetter,
-		Finalizers:           clusterExtensionFinalizers,
-		Manager:              cm,
-	}).SetupWithManager(mgr, ctrlBuilderOpts...); err != nil {
-		setupLog.Error(err, "unable to create controller", "controller", "ClusterExtension")
-		return err
-	}
+		ceReconciler.RevisionStatesGetter = &controllers.BoxcutterRevisionStatesGetter{Reader: mgr.GetClient()}
 
-	if features.OperatorControllerFeatureGate.Enabled(features.BoxcutterRuntime) {
 		// Boxcutter
 		const (
 			boxcutterSystemPrefixFieldOwner = "olm.operatorframework.io"
@@ -551,6 +483,70 @@ func run() error {
 			setupLog.Error(err, "unable to create controller", "controller", "ClusterExtensionRevision")
 			return err
 		}
+	} else {
+		coreClient, err := corev1client.NewForConfig(mgr.GetConfig())
+		if err != nil {
+			setupLog.Error(err, "unable to create core client")
+			return err
+		}
+		tokenGetter := authentication.NewTokenGetter(coreClient, authentication.WithExpirationDuration(1*time.Hour))
+		clientRestConfigMapper := action.ServiceAccountRestConfigMapper(tokenGetter)
+		if features.OperatorControllerFeatureGate.Enabled(features.SyntheticPermissions) {
+			clientRestConfigMapper = action.SyntheticUserRestConfigMapper(clientRestConfigMapper)
+		}
+
+		cfgGetter, err := helmclient.NewActionConfigGetter(mgr.GetConfig(), mgr.GetRESTMapper(),
+			helmclient.StorageDriverMapper(action.ChunkedStorageDriverMapper(coreClient, mgr.GetAPIReader(), cfg.systemNamespace)),
+			helmclient.ClientNamespaceMapper(func(obj client.Object) (string, error) {
+				ext := obj.(*ocv1.ClusterExtension)
+				return ext.Spec.Namespace, nil
+			}),
+			helmclient.ClientRestConfigMapper(clientRestConfigMapper),
+		)
+		if err != nil {
+			setupLog.Error(err, "unable to config for creating helm client")
+			return err
+		}
+
+		acg, err := action.NewWrappedActionClientGetter(cfgGetter,
+			helmclient.WithFailureRollbacks(false),
+		)
+		if err != nil {
+			setupLog.Error(err, "unable to create helm client")
+			return err
+		}
+
+		// determine if PreAuthorizer should be enabled based on feature gate
+		var preAuth authorization.PreAuthorizer
+		if features.OperatorControllerFeatureGate.Enabled(features.PreflightPermissions) {
+			preAuth = authorization.NewRBACPreAuthorizer(mgr.GetClient())
+		}
+
+		cm := contentmanager.NewManager(clientRestConfigMapper, mgr.GetConfig(), mgr.GetRESTMapper())
+		err = clusterExtensionFinalizers.Register(controllers.ClusterExtensionCleanupContentManagerCacheFinalizer, finalizers.FinalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) {
+			ext := obj.(*ocv1.ClusterExtension)
+			err := cm.Delete(ext)
+			return crfinalizer.Result{}, err
+		}))
+		if err != nil {
+			setupLog.Error(err, "unable to register content manager cleanup finalizer")
+			return err
+		}
+		// now initialize the helmApplier, assigning the potentially nil preAuth
+		ceReconciler.Applier = &applier.Helm{
+			ActionClientGetter: acg,
+			Preflights:         preflights,
+			BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{
+				BundleRenderer:          registryv1.Renderer,
+				CertificateProvider:     certProvider,
+				IsWebhookSupportEnabled: certProvider != nil,
+			},
+			HelmReleaseToObjectsConverter: &applier.HelmReleaseToObjectsConverter{},
+			PreAuthorizer:                 preAuth,
+			Watcher:                       ceController,
+			Manager:                       cm,
+		}
+		ceReconciler.RevisionStatesGetter = &controllers.HelmRevisionStatesGetter{ActionClientGetter: acg}
 	}
 
 	if err = (&controllers.ClusterCatalogReconciler{

internal/operator-controller/applier/boxcutter.go

@@ -12,6 +12,7 @@ import (
 	"slices"
 
 	"github.com/davecgh/go-spew/spew"
+	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -104,9 +105,8 @@ type Boxcutter struct {
 	Preflights        []Preflight
 }
 
-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
+func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (bool, string, error) {
+	return bc.apply(ctx, contentFS, ext, objectLabels, revisionAnnotations)
 }
 
 func (bc *Boxcutter) getObjects(rev *ocv1.ClusterExtensionRevision) []client.Object {
@@ -119,17 +119,17 @@ 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, revisionAnnotations map[string]string) ([]client.Object, error) {
+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 nil, err
+		return false, "", err
 	}
 
 	// List all existing revisions
 	existingRevisions, err := bc.getExistingRevisions(ctx, ext.GetName())
 	if err != nil {
-		return nil, err
+		return false, "", err
 	}
 	desiredHash := computeSHA256Hash(desiredRevision.Spec.Phases)
 
@@ -159,12 +159,16 @@ func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust
 		case StateNeedsInstall:
 			err := preflight.Install(ctx, plainObjs)
 			if err != nil {
-				return nil, err
+				return false, "", err
 			}
+		// TODO: jlanford's IDE says that "StateNeedsUpgrade" condition is always true, but
+		//   it isn't immediately obvious why that is. Perhaps len(existingRevisions) is
+		//   always greater than 0 (seems unlikely), or shouldSkipPreflight always returns
+		//   true (and we continue) when state == StateNeedsInstall?
 		case StateNeedsUpgrade:
 			err := preflight.Upgrade(ctx, plainObjs)
 			if err != nil {
-				return nil, err
+				return false, "", err
 			}
 		}
 	}
@@ -189,19 +193,24 @@ func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust
 		}
 
 		if err := controllerutil.SetControllerReference(ext, newRevision, bc.Scheme); err != nil {
-			return nil, fmt.Errorf("set ownerref: %w", err)
+			return false, "", fmt.Errorf("set ownerref: %w", err)
 		}
 		if err := bc.Client.Create(ctx, newRevision); err != nil {
-			return nil, fmt.Errorf("creating new Revision: %w", err)
+			return false, "", fmt.Errorf("creating new Revision: %w", err)
 		}
+		currentRevision = newRevision
 	}
 
 	// TODO: Delete archived previous revisions over a certain revision limit
 
-	// TODO: Read status from revision.
-
-	// Collect objects
-	return bc.getObjects(desiredRevision), nil
+	// TODO: Define constants for the ClusterExtensionRevision condition types.
+	installedCondition := meta.FindStatusCondition(currentRevision.Status.Conditions, "Succeeded")
+	if installedCondition == nil {
+		return false, "New revision created", nil
+	} else if installedCondition.Status != metav1.ConditionTrue {
+		return false, installedCondition.Message, nil
+	}
+	return true, "", nil
 }
 
 // getExistingRevisions returns the list of ClusterExtensionRevisions for a ClusterExtension with name extName in revision order (oldest to newest)