review changes

Author: camilamacedo86

cmd/operator-controller/main.go

@@ -656,7 +656,7 @@ func (c *boxcutterReconcilerConfigurator) Configure(ceReconciler *controllers.Cl
 	}
 	cerTokenGetter := authentication.NewTokenGetter(cerCoreClient, authentication.WithExpirationDuration(1*time.Hour))
 
-	revisionEngineFactory := controllers.NewDefaultRevisionEngineFactory(
+	revisionEngineFactory, err := controllers.NewDefaultRevisionEngineFactory(
 		c.mgr.GetScheme(),
 		trackingCache,
 		discoveryClient,
@@ -665,6 +665,9 @@ func (c *boxcutterReconcilerConfigurator) Configure(ceReconciler *controllers.Cl
 		c.mgr.GetConfig(),
 		cerTokenGetter,
 	)
+	if err != nil {
+		return fmt.Errorf("unable to create revision engine factory: %w", err)
+	}
 
 	if err = (&controllers.ClusterExtensionRevisionReconciler{
 		Client:                c.mgr.GetClient(),

internal/operator-controller/controllers/clusterextensionrevision_controller.go

@@ -21,7 +21,6 @@ import (
 	"k8s.io/apimachinery/pkg/util/sets"
 	"pkg.package-operator.run/boxcutter"
 	"pkg.package-operator.run/boxcutter/machinery"
-	machinerytypes "pkg.package-operator.run/boxcutter/machinery/types"
 	"pkg.package-operator.run/boxcutter/probing"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
@@ -55,16 +54,6 @@ type trackingCache interface {
 	Free(ctx context.Context, user client.Object) error
 }
 
-type RevisionEngine interface {
-	Teardown(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionTeardownOption) (machinery.RevisionTeardownResult, error)
-	Reconcile(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionReconcileOption) (machinery.RevisionResult, error)
-}
-
-// RevisionEngineFactory creates a RevisionEngine for a ClusterExtensionRevision.
-type RevisionEngineFactory interface {
-	CreateRevisionEngine(ctx context.Context, rev *ocv1.ClusterExtensionRevision) (RevisionEngine, error)
-}
-
 //+kubebuilder:rbac:groups=olm.operatorframework.io,resources=clusterextensionrevisions,verbs=get;list;watch;update;patch;create;delete
 //+kubebuilder:rbac:groups=olm.operatorframework.io,resources=clusterextensionrevisions/status,verbs=update;patch
 //+kubebuilder:rbac:groups=olm.operatorframework.io,resources=clusterextensionrevisions/finalizers,verbs=update

internal/operator-controller/controllers/revision_engine_factory.go

@@ -12,13 +12,15 @@ import (
 	"fmt"
 	"net/http"
 	"strings"
+	"sync"
 
 	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/discovery"
 	"k8s.io/client-go/rest"
 	"pkg.package-operator.run/boxcutter/machinery"
+	machinerytypes "pkg.package-operator.run/boxcutter/machinery/types"
 	"pkg.package-operator.run/boxcutter/managedcache"
 	"pkg.package-operator.run/boxcutter/ownerhandling"
 	"pkg.package-operator.run/boxcutter/validation"
@@ -29,26 +31,50 @@ import (
 	"github.com/operator-framework/operator-controller/internal/operator-controller/labels"
 )
 
-// DefaultRevisionEngineFactory creates boxcutter RevisionEngines with serviceAccount-scoped clients.
-type DefaultRevisionEngineFactory struct {
+// RevisionEngine defines the interface for reconciling and tearing down revisions.
+type RevisionEngine interface {
+	Teardown(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionTeardownOption) (machinery.RevisionTeardownResult, error)
+	Reconcile(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionReconcileOption) (machinery.RevisionResult, error)
+}
+
+// RevisionEngineFactory creates a RevisionEngine for a ClusterExtensionRevision.
+type RevisionEngineFactory interface {
+	CreateRevisionEngine(ctx context.Context, rev *ocv1.ClusterExtensionRevision) (RevisionEngine, error)
+}
+
+// defaultRevisionEngineFactory creates boxcutter RevisionEngines with serviceAccount-scoped clients.
+type defaultRevisionEngineFactory struct {
 	Scheme           *runtime.Scheme
 	TrackingCache    managedcache.TrackingCache
 	DiscoveryClient  discovery.CachedDiscoveryInterface
 	RESTMapper       meta.RESTMapper
 	FieldOwnerPrefix string
 	BaseConfig       *rest.Config
 	TokenGetter      *authentication.TokenGetter
+
+	// engineCache caches RevisionEngines keyed by "namespace/serviceAccountName"
+	engineCache sync.Map
 }
 
 // CreateRevisionEngine constructs a boxcutter RevisionEngine for the given ClusterExtensionRevision.
-// It reads the ServiceAccount from annotations and creates a scoped client.
-func (f *DefaultRevisionEngineFactory) CreateRevisionEngine(_ context.Context, rev *ocv1.ClusterExtensionRevision) (RevisionEngine, error) {
-	scopedClient, err := f.getScopedClient(rev)
+// It reads the ServiceAccount from annotations and returns a cached or new engine.
+func (f *defaultRevisionEngineFactory) CreateRevisionEngine(_ context.Context, rev *ocv1.ClusterExtensionRevision) (RevisionEngine, error) {
+	cacheKey, err := f.getServiceAccountKey(rev)
 	if err != nil {
 		return nil, err
 	}
 
-	return machinery.NewRevisionEngine(
+	// Check cache first
+	if cached, ok := f.engineCache.Load(cacheKey); ok {
+		return cached.(RevisionEngine), nil
+	}
+
+	scopedClient, err := f.createScopedClient(cacheKey)
+	if err != nil {
+		return nil, err
+	}
+
+	engine := machinery.NewRevisionEngine(
 		machinery.NewPhaseEngine(
 			machinery.NewObjectEngine(
 				f.Scheme, f.TrackingCache, scopedClient,
@@ -59,35 +85,36 @@ func (f *DefaultRevisionEngineFactory) CreateRevisionEngine(_ context.Context, r
 			validation.NewClusterPhaseValidator(f.RESTMapper, scopedClient),
 		),
 		validation.NewRevisionValidator(), scopedClient,
-	), nil
+	)
+
+	// Store in cache (LoadOrStore handles race conditions)
+	actual, _ := f.engineCache.LoadOrStore(cacheKey, engine)
+	return actual.(RevisionEngine), nil
 }
 
-func (f *DefaultRevisionEngineFactory) getScopedClient(rev *ocv1.ClusterExtensionRevision) (client.Client, error) {
+func (f *defaultRevisionEngineFactory) getServiceAccountKey(rev *ocv1.ClusterExtensionRevision) (string, error) {
 	annotations := rev.GetAnnotations()
 	if annotations == nil {
-		return nil, fmt.Errorf("revision %q is missing required annotations", rev.Name)
+		return "", fmt.Errorf("revision %q is missing required annotations", rev.Name)
 	}
 
 	saName := strings.TrimSpace(annotations[labels.ServiceAccountNameKey])
 	saNamespace := strings.TrimSpace(annotations[labels.ServiceAccountNamespaceKey])
 
 	if len(saName) == 0 {
-		return nil, fmt.Errorf("revision %q is missing ServiceAccount name annotation", rev.Name)
+		return "", fmt.Errorf("revision %q is missing ServiceAccount name annotation", rev.Name)
 	}
 	if len(saNamespace) == 0 {
-		return nil, fmt.Errorf("revision %q is missing ServiceAccount namespace annotation", rev.Name)
+		return "", fmt.Errorf("revision %q is missing ServiceAccount namespace annotation", rev.Name)
 	}
 
-	return f.createScopedClient(saNamespace, saName)
+	return saNamespace + "/" + saName, nil
 }
 
-func (f *DefaultRevisionEngineFactory) createScopedClient(namespace, serviceAccountName string) (client.Client, error) {
-	if f.TokenGetter == nil {
-		return nil, fmt.Errorf("TokenGetter is required but not configured")
-	}
-	if f.BaseConfig == nil {
-		return nil, fmt.Errorf("BaseConfig is required but not configured")
-	}
+func (f *defaultRevisionEngineFactory) createScopedClient(serviceAccountKey string) (client.Client, error) {
+	// Parse the key back to namespace/name
+	parts := strings.SplitN(serviceAccountKey, "/", 2)
+	namespace, serviceAccountName := parts[0], parts[1]
 
 	saConfig := rest.AnonymousClientConfig(f.BaseConfig)
 	saConfig.Wrap(func(rt http.RoundTripper) http.RoundTripper {
@@ -105,13 +132,13 @@ func (f *DefaultRevisionEngineFactory) createScopedClient(namespace, serviceAcco
 		Scheme: f.Scheme,
 	})
 	if err != nil {
-		return nil, fmt.Errorf("failed to create client for ServiceAccount %s/%s: %w", namespace, serviceAccountName, err)
+		return nil, fmt.Errorf("failed to create client for ServiceAccount %s: %w", serviceAccountKey, err)
 	}
 
 	return scopedClient, nil
 }
 
-// NewDefaultRevisionEngineFactory creates a new DefaultRevisionEngineFactory.
+// NewDefaultRevisionEngineFactory creates a new defaultRevisionEngineFactory.
 func NewDefaultRevisionEngineFactory(
 	scheme *runtime.Scheme,
 	trackingCache managedcache.TrackingCache,
@@ -120,14 +147,20 @@ func NewDefaultRevisionEngineFactory(
 	fieldOwnerPrefix string,
 	baseConfig *rest.Config,
 	tokenGetter *authentication.TokenGetter,
-) *DefaultRevisionEngineFactory {
-	return &DefaultRevisionEngineFactory{
+) (RevisionEngineFactory, error) {
+	if baseConfig == nil {
+		return nil, fmt.Errorf("baseConfig is required but not provided")
+	}
+	if tokenGetter == nil {
+		return nil, fmt.Errorf("tokenGetter is required but not provided")
+	}
+	return &defaultRevisionEngineFactory{
 		Scheme:           scheme,
 		TrackingCache:    trackingCache,
 		DiscoveryClient:  discoveryClient,
 		RESTMapper:       restMapper,
 		FieldOwnerPrefix: fieldOwnerPrefix,
 		BaseConfig:       baseConfig,
 		TokenGetter:      tokenGetter,
-	}
+	}, nil
 }