From 6fe7c3caf17a3ed2469374a8916e56ebc9513a94 Mon Sep 17 00:00:00 2001 From: Camila Macedo <7708031+camilamacedo86@users.noreply.github.com> Date: Tue, 6 Jan 2026 16:56:27 +0000 Subject: [PATCH 1/5] (feat): [Boxcutter] Use ClusterExtension ServiceAccount for revision operations Implement serviceAccount-scoped token-based authentication for ClusterExtensionRevision controller using annotation-based configuration. - Add RevisionEngineFactory with CreateRevisionEngine(ctx, rev) interface - Read ServiceAccount from annotations (no ClusterExtension dependency) - Token-based auth using TokenInjectingRoundTripper - ServiceAccount name and namespace in annotations for observability - TrackingCache uses global client for caching/cleanup - Comprehensive error path tests ClusterExtensionRevision can exist independently. Easy mode impersonation deferred until API is finalized. Assisted-by: Cursor --- cmd/operator-controller/main.go | 36 +-- .../operator-controller/applier/boxcutter.go | 6 + .../applier/boxcutter_test.go | 36 ++- .../clusterextensionrevision_controller.go | 27 ++- ...lusterextensionrevision_controller_test.go | 213 +++++++++++++++--- .../controllers/revision_engine_factory.go | 133 +++++++++++ internal/operator-controller/labels/labels.go | 14 +- 7 files changed, 405 insertions(+), 60 deletions(-) create mode 100644 internal/operator-controller/controllers/revision_engine_factory.go diff --git a/cmd/operator-controller/main.go b/cmd/operator-controller/main.go index a1d03feee4..5b9042f710 100644 --- a/cmd/operator-controller/main.go +++ b/cmd/operator-controller/main.go @@ -42,10 +42,7 @@ import ( _ "k8s.io/client-go/plugin/pkg/client/auth" "k8s.io/klog/v2" "k8s.io/utils/ptr" - "pkg.package-operator.run/boxcutter/machinery" "pkg.package-operator.run/boxcutter/managedcache" - "pkg.package-operator.run/boxcutter/ownerhandling" - "pkg.package-operator.run/boxcutter/validation" ctrl "sigs.k8s.io/controller-runtime" crcache "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/certwatcher" @@ -653,21 +650,26 @@ func (c *boxcutterReconcilerConfigurator) Configure(ceReconciler *controllers.Cl return fmt.Errorf("unable to add tracking cache to manager: %v", err) } + cerCoreClient, err := corev1client.NewForConfig(c.mgr.GetConfig()) + if err != nil { + return fmt.Errorf("unable to create client for ClusterExtensionRevision controller: %w", err) + } + cerTokenGetter := authentication.NewTokenGetter(cerCoreClient, authentication.WithExpirationDuration(1*time.Hour)) + + revisionEngineFactory := controllers.NewDefaultRevisionEngineFactory( + c.mgr.GetScheme(), + trackingCache, + discoveryClient, + c.mgr.GetRESTMapper(), + fieldOwnerPrefix, + c.mgr.GetConfig(), + cerTokenGetter, + ) + if err = (&controllers.ClusterExtensionRevisionReconciler{ - Client: c.mgr.GetClient(), - RevisionEngine: machinery.NewRevisionEngine( - machinery.NewPhaseEngine( - machinery.NewObjectEngine( - c.mgr.GetScheme(), trackingCache, c.mgr.GetClient(), - ownerhandling.NewNative(c.mgr.GetScheme()), - machinery.NewComparator(ownerhandling.NewNative(c.mgr.GetScheme()), discoveryClient, c.mgr.GetScheme(), fieldOwnerPrefix), - fieldOwnerPrefix, fieldOwnerPrefix, - ), - validation.NewClusterPhaseValidator(c.mgr.GetRESTMapper(), c.mgr.GetClient()), - ), - validation.NewRevisionValidator(), c.mgr.GetClient(), - ), - TrackingCache: trackingCache, + Client: c.mgr.GetClient(), + RevisionEngineFactory: revisionEngineFactory, + TrackingCache: trackingCache, }).SetupWithManager(c.mgr); err != nil { return fmt.Errorf("unable to setup ClusterExtensionRevision controller: %w", err) } diff --git a/internal/operator-controller/applier/boxcutter.go b/internal/operator-controller/applier/boxcutter.go index c69ab5a1a0..04991fe0db 100644 --- a/internal/operator-controller/applier/boxcutter.go +++ b/internal/operator-controller/applier/boxcutter.go @@ -186,6 +186,12 @@ func (r *SimpleRevisionGenerator) buildClusterExtensionRevision( ext *ocv1.ClusterExtension, annotations map[string]string, ) *ocv1.ClusterExtensionRevision { + if annotations == nil { + annotations = make(map[string]string) + } + annotations[labels.ServiceAccountNameKey] = ext.Spec.ServiceAccount.Name + annotations[labels.ServiceAccountNamespaceKey] = ext.Spec.Namespace + return &ocv1.ClusterExtensionRevision{ ObjectMeta: metav1.ObjectMeta{ Annotations: annotations, diff --git a/internal/operator-controller/applier/boxcutter_test.go b/internal/operator-controller/applier/boxcutter_test.go index 8cae359f51..9cc1d0a51d 100644 --- a/internal/operator-controller/applier/boxcutter_test.go +++ b/internal/operator-controller/applier/boxcutter_test.go @@ -66,6 +66,12 @@ func Test_SimpleRevisionGenerator_GenerateRevisionFromHelmRelease(t *testing.T) ObjectMeta: metav1.ObjectMeta{ Name: "test-123", }, + Spec: ocv1.ClusterExtensionSpec{ + Namespace: "test-namespace", + ServiceAccount: ocv1.ServiceAccountReference{ + Name: "test-sa", + }, + }, } objectLabels := map[string]string{ @@ -79,10 +85,12 @@ func Test_SimpleRevisionGenerator_GenerateRevisionFromHelmRelease(t *testing.T) ObjectMeta: metav1.ObjectMeta{ Name: "test-123-1", Annotations: map[string]string{ - "olm.operatorframework.io/bundle-name": "my-bundle", - "olm.operatorframework.io/bundle-reference": "bundle-ref", - "olm.operatorframework.io/bundle-version": "1.2.0", - "olm.operatorframework.io/package-name": "my-package", + "olm.operatorframework.io/bundle-name": "my-bundle", + "olm.operatorframework.io/bundle-reference": "bundle-ref", + "olm.operatorframework.io/bundle-version": "1.2.0", + "olm.operatorframework.io/package-name": "my-package", + "olm.operatorframework.io/service-account-name": "test-sa", + "olm.operatorframework.io/service-account-namespace": "test-namespace", }, Labels: map[string]string{ labels.OwnerNameKey: "test-123", @@ -172,6 +180,12 @@ func Test_SimpleRevisionGenerator_GenerateRevision(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "test-extension", }, + Spec: ocv1.ClusterExtensionSpec{ + Namespace: "test-namespace", + ServiceAccount: ocv1.ServiceAccountReference{ + Name: "test-sa", + }, + }, } rev, err := b.GenerateRevision(t.Context(), fstest.MapFS{}, ext, map[string]string{}, map[string]string{}) @@ -291,7 +305,12 @@ func Test_SimpleRevisionGenerator_AppliesObjectLabelsAndRevisionAnnotations(t *t "other": "value", } - rev, err := b.GenerateRevision(t.Context(), fstest.MapFS{}, &ocv1.ClusterExtension{}, map[string]string{ + rev, err := b.GenerateRevision(t.Context(), fstest.MapFS{}, &ocv1.ClusterExtension{ + Spec: ocv1.ClusterExtensionSpec{ + Namespace: "test-namespace", + ServiceAccount: ocv1.ServiceAccountReference{Name: "test-sa"}, + }, + }, map[string]string{ "some": "value", }, revAnnotations) require.NoError(t, err) @@ -319,7 +338,12 @@ func Test_SimpleRevisionGenerator_Failure(t *testing.T) { ManifestProvider: r, } - rev, err := b.GenerateRevision(t.Context(), fstest.MapFS{}, &ocv1.ClusterExtension{}, map[string]string{}, map[string]string{}) + rev, err := b.GenerateRevision(t.Context(), fstest.MapFS{}, &ocv1.ClusterExtension{ + Spec: ocv1.ClusterExtensionSpec{ + Namespace: "test-namespace", + ServiceAccount: ocv1.ServiceAccountReference{Name: "test-sa"}, + }, + }, map[string]string{}, map[string]string{}) require.Nil(t, rev) t.Log("by checking rendering errors are propagated") require.Error(t, err) diff --git a/internal/operator-controller/controllers/clusterextensionrevision_controller.go b/internal/operator-controller/controllers/clusterextensionrevision_controller.go index a7da574d61..18cde19b25 100644 --- a/internal/operator-controller/controllers/clusterextensionrevision_controller.go +++ b/internal/operator-controller/controllers/clusterextensionrevision_controller.go @@ -43,9 +43,9 @@ const ( // ClusterExtensionRevisionReconciler actions individual snapshots of ClusterExtensions, // as part of the boxcutter integration. type ClusterExtensionRevisionReconciler struct { - Client client.Client - RevisionEngine RevisionEngine - TrackingCache trackingCache + Client client.Client + RevisionEngineFactory RevisionEngineFactory + TrackingCache trackingCache } type trackingCache interface { @@ -60,6 +60,11 @@ type RevisionEngine interface { 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 @@ -139,7 +144,13 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev return ctrl.Result{}, werr } - rres, err := c.RevisionEngine.Reconcile(ctx, *revision, opts...) + revisionEngine, err := c.RevisionEngineFactory.CreateRevisionEngine(ctx, rev) + if err != nil { + setRetryingConditions(rev, err.Error()) + return ctrl.Result{}, fmt.Errorf("failed to create revision engine: %v", err) + } + + rres, err := revisionEngine.Reconcile(ctx, *revision, opts...) if err != nil { if rres != nil { // Log detailed reconcile reports only in debug mode (V(1)) to reduce verbosity. @@ -253,7 +264,13 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev func (c *ClusterExtensionRevisionReconciler) teardown(ctx context.Context, rev *ocv1.ClusterExtensionRevision, revision *boxcutter.Revision) (ctrl.Result, error) { l := log.FromContext(ctx) - tres, err := c.RevisionEngine.Teardown(ctx, *revision) + revisionEngine, err := c.RevisionEngineFactory.CreateRevisionEngine(ctx, rev) + if err != nil { + markAsAvailableUnknown(rev, ocv1.ClusterExtensionRevisionReasonReconciling, err.Error()) + return ctrl.Result{}, fmt.Errorf("failed to create revision engine for teardown: %v", err) + } + + tres, err := revisionEngine.Teardown(ctx, *revision) if err != nil { if tres != nil { l.V(1).Info("teardown failure report", "report", tres.String()) diff --git a/internal/operator-controller/controllers/clusterextensionrevision_controller_test.go b/internal/operator-controller/controllers/clusterextensionrevision_controller_test.go index 8a1c52513a..d613da1376 100644 --- a/internal/operator-controller/controllers/clusterextensionrevision_controller_test.go +++ b/internal/operator-controller/controllers/clusterextensionrevision_controller_test.go @@ -484,14 +484,15 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_RevisionReconciliation(t Build() // reconcile cluster extension revision - result, err := (&controllers.ClusterExtensionRevisionReconciler{ - Client: testClient, - RevisionEngine: &mockRevisionEngine{ - reconcile: func(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionReconcileOption) (machinery.RevisionResult, error) { - return tc.revisionResult, tc.revisionReconcileErr - }, + mockEngine := &mockRevisionEngine{ + reconcile: func(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionReconcileOption) (machinery.RevisionResult, error) { + return tc.revisionResult, tc.revisionReconcileErr }, - TrackingCache: &mockTrackingCache{client: testClient}, + } + result, err := (&controllers.ClusterExtensionRevisionReconciler{ + Client: testClient, + RevisionEngineFactory: &mockRevisionEngineFactory{engine: mockEngine}, + TrackingCache: &mockTrackingCache{client: testClient}, }).Reconcile(t.Context(), ctrl.Request{ NamespacedName: types.NamespacedName{ Name: tc.reconcilingRevisionName, @@ -603,14 +604,15 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_ValidationError_Retries(t Build() // reconcile cluster extension revision - result, err := (&controllers.ClusterExtensionRevisionReconciler{ - Client: testClient, - RevisionEngine: &mockRevisionEngine{ - reconcile: func(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionReconcileOption) (machinery.RevisionResult, error) { - return tc.revisionResult, nil - }, + mockEngine := &mockRevisionEngine{ + reconcile: func(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionReconcileOption) (machinery.RevisionResult, error) { + return tc.revisionResult, nil }, - TrackingCache: &mockTrackingCache{client: testClient}, + } + result, err := (&controllers.ClusterExtensionRevisionReconciler{ + Client: testClient, + RevisionEngineFactory: &mockRevisionEngineFactory{engine: mockEngine}, + TrackingCache: &mockTrackingCache{client: testClient}, }).Reconcile(t.Context(), ctrl.Request{ NamespacedName: types.NamespacedName{ Name: clusterExtensionRevisionName, @@ -652,7 +654,7 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_Deletion(t *testing.T) { rev1.Finalizers = []string{ "olm.operatorframework.io/teardown", } - return []client.Object{rev1} + return []client.Object{ext, rev1} }, validate: func(t *testing.T, c client.Client) { rev := &ocv1.ClusterExtensionRevision{} @@ -887,14 +889,15 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_Deletion(t *testing.T) { Build() // reconcile cluster extension revision - result, err := (&controllers.ClusterExtensionRevisionReconciler{ - Client: testClient, - RevisionEngine: &mockRevisionEngine{ - reconcile: func(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionReconcileOption) (machinery.RevisionResult, error) { - return tc.revisionResult, nil - }, - teardown: tc.revisionEngineTeardownFn(t), + mockEngine := &mockRevisionEngine{ + reconcile: func(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionReconcileOption) (machinery.RevisionResult, error) { + return tc.revisionResult, nil }, + teardown: tc.revisionEngineTeardownFn(t), + } + result, err := (&controllers.ClusterExtensionRevisionReconciler{ + Client: testClient, + RevisionEngineFactory: &mockRevisionEngineFactory{engine: mockEngine}, TrackingCache: &mockTrackingCache{ client: testClient, freeFn: tc.trackingCacheFreeFn, @@ -952,10 +955,12 @@ func newTestClusterExtensionRevision(t *testing.T, revisionName string, ext *ocv UID: types.UID(revisionName), Generation: int64(1), Annotations: map[string]string{ - labels.PackageNameKey: "some-package", - labels.BundleNameKey: "some-package.v1.0.0", - labels.BundleReferenceKey: "registry.io/some-repo/some-package:v1.0.0", - labels.BundleVersionKey: "1.0.0", + labels.PackageNameKey: "some-package", + labels.BundleNameKey: "some-package.v1.0.0", + labels.BundleReferenceKey: "registry.io/some-repo/some-package:v1.0.0", + labels.BundleVersionKey: "1.0.0", + labels.ServiceAccountNameKey: ext.Spec.ServiceAccount.Name, + labels.ServiceAccountNamespaceKey: ext.Spec.Namespace, }, Labels: map[string]string{ labels.OwnerNameKey: "test-ext", @@ -1001,6 +1006,19 @@ func (m mockRevisionEngine) Reconcile(ctx context.Context, rev machinerytypes.Re return m.reconcile(ctx, rev) } +// mockRevisionEngineFactory creates mock RevisionEngines for testing +type mockRevisionEngineFactory struct { + engine controllers.RevisionEngine + createErr error +} + +func (f *mockRevisionEngineFactory) CreateRevisionEngine(ctx context.Context, rev *ocv1.ClusterExtensionRevision) (controllers.RevisionEngine, error) { + if f.createErr != nil { + return nil, f.createErr + } + return f.engine, nil +} + type mockRevisionResult struct { validationError *validation.RevisionValidationError phases []machinery.PhaseResult @@ -1177,3 +1195,146 @@ func (m *mockTrackingCache) Free(ctx context.Context, user client.Object) error } return nil } + +func Test_ClusterExtensionRevisionReconciler_getScopedClient_Errors(t *testing.T) { + testScheme := newScheme(t) + + t.Run("works with serviceAccount annotation and without owner label", func(t *testing.T) { + rev := &ocv1.ClusterExtensionRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-rev-1", + Labels: map[string]string{}, + Annotations: map[string]string{ + labels.ServiceAccountNameKey: "test-sa", + labels.ServiceAccountNamespaceKey: "test-ns", + labels.BundleVersionKey: "1.0.0", + }, + }, + Spec: ocv1.ClusterExtensionRevisionSpec{ + LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateActive, + Revision: 1, + Phases: []ocv1.ClusterExtensionRevisionPhase{}, + }, + } + + testClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithStatusSubresource(&ocv1.ClusterExtensionRevision{}). + WithObjects(rev). + Build() + + mockEngine := &mockRevisionEngine{ + reconcile: func(ctx context.Context, r machinerytypes.Revision, opts ...machinerytypes.RevisionReconcileOption) (machinery.RevisionResult, error) { + return &mockRevisionResult{}, nil + }, + } + + reconciler := &controllers.ClusterExtensionRevisionReconciler{ + Client: testClient, + RevisionEngineFactory: &mockRevisionEngineFactory{engine: mockEngine}, + TrackingCache: &mockTrackingCache{client: testClient}, + } + + _, err := reconciler.Reconcile(t.Context(), ctrl.Request{ + NamespacedName: types.NamespacedName{Name: "test-rev-1"}, + }) + + require.NoError(t, err) + }) + + t.Run("missing serviceAccount annotation", func(t *testing.T) { + rev := &ocv1.ClusterExtensionRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-rev-1", + Annotations: map[string]string{ + labels.BundleVersionKey: "1.0.0", + }, + }, + Spec: ocv1.ClusterExtensionRevisionSpec{ + LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateActive, + Revision: 1, + Phases: []ocv1.ClusterExtensionRevisionPhase{}, + }, + } + + testClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(rev). + Build() + + failingFactory := &mockRevisionEngineFactory{ + createErr: errors.New("missing serviceAccount name annotation"), + } + + reconciler := &controllers.ClusterExtensionRevisionReconciler{ + Client: testClient, + RevisionEngineFactory: failingFactory, + TrackingCache: &mockTrackingCache{client: testClient}, + } + + _, err := reconciler.Reconcile(t.Context(), ctrl.Request{ + NamespacedName: types.NamespacedName{Name: "test-rev-1"}, + }) + + require.Error(t, err) + require.Contains(t, err.Error(), "serviceAccount") + }) + + t.Run("factory fails to create engine", func(t *testing.T) { + ext := newTestClusterExtension() + rev := newTestClusterExtensionRevision(t, "test-rev", ext, testScheme) + + testClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(ext, rev). + Build() + + failingFactory := &mockRevisionEngineFactory{ + createErr: errors.New("token getter failed"), + } + + reconciler := &controllers.ClusterExtensionRevisionReconciler{ + Client: testClient, + RevisionEngineFactory: failingFactory, + TrackingCache: &mockTrackingCache{client: testClient}, + } + + _, err := reconciler.Reconcile(t.Context(), ctrl.Request{ + NamespacedName: types.NamespacedName{Name: "test-rev"}, + }) + + require.Error(t, err) + require.Contains(t, err.Error(), "failed to create revision engine") + require.Contains(t, err.Error(), "token getter failed") + }) + + t.Run("factory fails during teardown", func(t *testing.T) { + ext := newTestClusterExtension() + rev := newTestClusterExtensionRevision(t, "test-rev", ext, testScheme) + rev.DeletionTimestamp = &metav1.Time{Time: time.Now()} + rev.Finalizers = []string{"olm.operatorframework.io/teardown"} + + testClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(ext, rev). + Build() + + failingFactory := &mockRevisionEngineFactory{ + createErr: errors.New("serviceaccount not found"), + } + + reconciler := &controllers.ClusterExtensionRevisionReconciler{ + Client: testClient, + RevisionEngineFactory: failingFactory, + TrackingCache: &mockTrackingCache{client: testClient}, + } + + _, err := reconciler.Reconcile(t.Context(), ctrl.Request{ + NamespacedName: types.NamespacedName{Name: "test-rev"}, + }) + + require.Error(t, err) + require.Contains(t, err.Error(), "failed to create revision engine for teardown") + require.Contains(t, err.Error(), "serviceaccount not found") + }) +} diff --git a/internal/operator-controller/controllers/revision_engine_factory.go b/internal/operator-controller/controllers/revision_engine_factory.go new file mode 100644 index 0000000000..c677ed102b --- /dev/null +++ b/internal/operator-controller/controllers/revision_engine_factory.go @@ -0,0 +1,133 @@ +//go:build !standard + +// This file is excluded from standard builds because ClusterExtensionRevision +// is an experimental feature. Standard builds use Helm-based applier only. +// The experimental build includes BoxcutterRuntime which requires these factories +// for serviceAccount-scoped client creation and RevisionEngine instantiation. + +package controllers + +import ( + "context" + "fmt" + "net/http" + "strings" + + "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" + "pkg.package-operator.run/boxcutter/managedcache" + "pkg.package-operator.run/boxcutter/ownerhandling" + "pkg.package-operator.run/boxcutter/validation" + "sigs.k8s.io/controller-runtime/pkg/client" + + ocv1 "github.com/operator-framework/operator-controller/api/v1" + "github.com/operator-framework/operator-controller/internal/operator-controller/authentication" + "github.com/operator-framework/operator-controller/internal/operator-controller/labels" +) + +// 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 +} + +// CreateRevisionEngine constructs a boxcutter RevisionEngine for the given ClusterExtensionRevision. +// It reads the ServiceAccount from annotations and creates a scoped client. +func (f *DefaultRevisionEngineFactory) CreateRevisionEngine(ctx context.Context, rev *ocv1.ClusterExtensionRevision) (RevisionEngine, error) { + scopedClient, err := f.getScopedClient(rev) + if err != nil { + return nil, err + } + + return machinery.NewRevisionEngine( + machinery.NewPhaseEngine( + machinery.NewObjectEngine( + f.Scheme, f.TrackingCache, scopedClient, + ownerhandling.NewNative(f.Scheme), + machinery.NewComparator(ownerhandling.NewNative(f.Scheme), f.DiscoveryClient, f.Scheme, f.FieldOwnerPrefix), + f.FieldOwnerPrefix, f.FieldOwnerPrefix, + ), + validation.NewClusterPhaseValidator(f.RESTMapper, scopedClient), + ), + validation.NewRevisionValidator(), scopedClient, + ), nil +} + +func (f *DefaultRevisionEngineFactory) getScopedClient(rev *ocv1.ClusterExtensionRevision) (client.Client, error) { + annotations := rev.GetAnnotations() + if annotations == nil { + return nil, 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) + } + if len(saNamespace) == 0 { + return nil, fmt.Errorf("revision %q is missing ServiceAccount namespace annotation", rev.Name) + } + + return f.createScopedClient(saNamespace, saName) +} + +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") + } + + saConfig := rest.AnonymousClientConfig(f.BaseConfig) + saConfig.Wrap(func(rt http.RoundTripper) http.RoundTripper { + return &authentication.TokenInjectingRoundTripper{ + Tripper: rt, + TokenGetter: f.TokenGetter, + Key: types.NamespacedName{ + Name: serviceAccountName, + Namespace: namespace, + }, + } + }) + + scopedClient, err := client.New(saConfig, client.Options{ + Scheme: f.Scheme, + }) + if err != nil { + return nil, fmt.Errorf("failed to create client for ServiceAccount %s/%s: %w", namespace, serviceAccountName, err) + } + + return scopedClient, nil +} + +// NewDefaultRevisionEngineFactory creates a new DefaultRevisionEngineFactory. +func NewDefaultRevisionEngineFactory( + scheme *runtime.Scheme, + trackingCache managedcache.TrackingCache, + discoveryClient discovery.CachedDiscoveryInterface, + restMapper meta.RESTMapper, + fieldOwnerPrefix string, + baseConfig *rest.Config, + tokenGetter *authentication.TokenGetter, +) *DefaultRevisionEngineFactory { + return &DefaultRevisionEngineFactory{ + Scheme: scheme, + TrackingCache: trackingCache, + DiscoveryClient: discoveryClient, + RESTMapper: restMapper, + FieldOwnerPrefix: fieldOwnerPrefix, + BaseConfig: baseConfig, + TokenGetter: tokenGetter, + } +} diff --git a/internal/operator-controller/labels/labels.go b/internal/operator-controller/labels/labels.go index 63bf0c4931..1deb73822e 100644 --- a/internal/operator-controller/labels/labels.go +++ b/internal/operator-controller/labels/labels.go @@ -1,10 +1,12 @@ package labels const ( - OwnerKindKey = "olm.operatorframework.io/owner-kind" - OwnerNameKey = "olm.operatorframework.io/owner-name" - PackageNameKey = "olm.operatorframework.io/package-name" - BundleNameKey = "olm.operatorframework.io/bundle-name" - BundleVersionKey = "olm.operatorframework.io/bundle-version" - BundleReferenceKey = "olm.operatorframework.io/bundle-reference" + OwnerKindKey = "olm.operatorframework.io/owner-kind" + OwnerNameKey = "olm.operatorframework.io/owner-name" + PackageNameKey = "olm.operatorframework.io/package-name" + BundleNameKey = "olm.operatorframework.io/bundle-name" + BundleVersionKey = "olm.operatorframework.io/bundle-version" + BundleReferenceKey = "olm.operatorframework.io/bundle-reference" + ServiceAccountNameKey = "olm.operatorframework.io/service-account-name" + ServiceAccountNamespaceKey = "olm.operatorframework.io/service-account-namespace" ) From e5d8e947e94c6ee1544e1f57df23485cdcd13fce Mon Sep 17 00:00:00 2001 From: Camila Macedo <7708031+camilamacedo86@users.noreply.github.com> Date: Wed, 7 Jan 2026 18:27:28 +0000 Subject: [PATCH 2/5] (doc) Add godoc comments to label constants Adds documentation comments to all label/annotation constants explaining: - What each constant represents - Where they are applied (labels vs annotations) - ServiceAccount constants document their relationship to ClusterExtension spec Addresses code review feedback for improved maintainability. --- .../controllers/revision_engine_factory.go | 2 +- internal/operator-controller/labels/labels.go | 44 ++++++++++++++++--- 2 files changed, 38 insertions(+), 8 deletions(-) diff --git a/internal/operator-controller/controllers/revision_engine_factory.go b/internal/operator-controller/controllers/revision_engine_factory.go index c677ed102b..599b27338e 100644 --- a/internal/operator-controller/controllers/revision_engine_factory.go +++ b/internal/operator-controller/controllers/revision_engine_factory.go @@ -42,7 +42,7 @@ type DefaultRevisionEngineFactory struct { // CreateRevisionEngine constructs a boxcutter RevisionEngine for the given ClusterExtensionRevision. // It reads the ServiceAccount from annotations and creates a scoped client. -func (f *DefaultRevisionEngineFactory) CreateRevisionEngine(ctx context.Context, rev *ocv1.ClusterExtensionRevision) (RevisionEngine, error) { +func (f *DefaultRevisionEngineFactory) CreateRevisionEngine(_ context.Context, rev *ocv1.ClusterExtensionRevision) (RevisionEngine, error) { scopedClient, err := f.getScopedClient(rev) if err != nil { return nil, err diff --git a/internal/operator-controller/labels/labels.go b/internal/operator-controller/labels/labels.go index 1deb73822e..52fa6e2b15 100644 --- a/internal/operator-controller/labels/labels.go +++ b/internal/operator-controller/labels/labels.go @@ -1,12 +1,42 @@ package labels const ( - OwnerKindKey = "olm.operatorframework.io/owner-kind" - OwnerNameKey = "olm.operatorframework.io/owner-name" - PackageNameKey = "olm.operatorframework.io/package-name" - BundleNameKey = "olm.operatorframework.io/bundle-name" - BundleVersionKey = "olm.operatorframework.io/bundle-version" - BundleReferenceKey = "olm.operatorframework.io/bundle-reference" - ServiceAccountNameKey = "olm.operatorframework.io/service-account-name" + // OwnerKindKey is the label key used to record the kind of the owner + // resource responsible for creating or managing a ClusterExtensionRevision. + OwnerKindKey = "olm.operatorframework.io/owner-kind" + + // OwnerNameKey is the label key used to record the name of the owner + // resource responsible for creating or managing a ClusterExtensionRevision. + OwnerNameKey = "olm.operatorframework.io/owner-name" + + // PackageNameKey is the label key used to record the package name + // associated with a ClusterExtensionRevision. + PackageNameKey = "olm.operatorframework.io/package-name" + + // BundleNameKey is the label key used to record the bundle name + // associated with a ClusterExtensionRevision. + BundleNameKey = "olm.operatorframework.io/bundle-name" + + // BundleVersionKey is the label key used to record the bundle version + // associated with a ClusterExtensionRevision. + BundleVersionKey = "olm.operatorframework.io/bundle-version" + + // BundleReferenceKey is the label key used to record an external reference + // (such as an image or catalog reference) to the bundle for a + // ClusterExtensionRevision. + BundleReferenceKey = "olm.operatorframework.io/bundle-reference" + + // ServiceAccountNameKey is the annotation key used to record the name of + // the ServiceAccount configured on the owning ClusterExtension. It is + // applied as an annotation on ClusterExtensionRevision resources to + // capture which ServiceAccount was used for their lifecycle operations. + ServiceAccountNameKey = "olm.operatorframework.io/service-account-name" + + // ServiceAccountNamespaceKey is the annotation key used to record the + // namespace of the ServiceAccount configured on the owning + // ClusterExtension. It is applied as an annotation on + // ClusterExtensionRevision resources together with ServiceAccountNameKey + // so that the effective ServiceAccount identity used for + // ClusterExtensionRevision operations is preserved. ServiceAccountNamespaceKey = "olm.operatorframework.io/service-account-namespace" ) From b38c39191a181f47899241f9cfa8a8f14714e62b Mon Sep 17 00:00:00 2001 From: Camila Macedo <7708031+camilamacedo86@users.noreply.github.com> Date: Wed, 7 Jan 2026 19:03:36 +0000 Subject: [PATCH 3/5] (fix) Add ClusterExtensionRevision permissions to upgrade test RBAC The upgrade test ServiceAccount needs permissions to manage ClusterExtensionRevisions when BoxcutterRuntime is enabled. Without these permissions, the upgraded controller cannot create or update ClusterExtensionRevision resources, causing the ClusterExtension to fail reconciliation after upgrade. --- hack/test/pre-upgrade-setup.sh | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/hack/test/pre-upgrade-setup.sh b/hack/test/pre-upgrade-setup.sh index 669f9da37f..f2dc28761c 100755 --- a/hack/test/pre-upgrade-setup.sh +++ b/hack/test/pre-upgrade-setup.sh @@ -122,6 +122,19 @@ rules: - "update" resourceNames: - "${TEST_CLUSTER_EXTENSION_NAME}" + - apiGroups: + - "olm.operatorframework.io" + resources: + - "clusterextensionrevisions" + - "clusterextensionrevisions/finalizers" + verbs: + - "create" + - "update" + - "patch" + - "delete" + - "get" + - "list" + - "watch" EOF kubectl apply -f - < Date: Thu, 8 Jan 2026 17:34:23 +0000 Subject: [PATCH 4/5] review changes --- cmd/operator-controller/main.go | 5 +- .../clusterextensionrevision_controller.go | 10 ---- .../controllers/revision_engine_factory.go | 58 ++++++++++++------- 3 files changed, 41 insertions(+), 32 deletions(-) diff --git a/cmd/operator-controller/main.go b/cmd/operator-controller/main.go index 5b9042f710..9da4007665 100644 --- a/cmd/operator-controller/main.go +++ b/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(), diff --git a/internal/operator-controller/controllers/clusterextensionrevision_controller.go b/internal/operator-controller/controllers/clusterextensionrevision_controller.go index 18cde19b25..a2173a2fcf 100644 --- a/internal/operator-controller/controllers/clusterextensionrevision_controller.go +++ b/internal/operator-controller/controllers/clusterextensionrevision_controller.go @@ -55,16 +55,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 diff --git a/internal/operator-controller/controllers/revision_engine_factory.go b/internal/operator-controller/controllers/revision_engine_factory.go index 599b27338e..aa8a9b9ffc 100644 --- a/internal/operator-controller/controllers/revision_engine_factory.go +++ b/internal/operator-controller/controllers/revision_engine_factory.go @@ -19,6 +19,7 @@ import ( "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,8 +30,19 @@ 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 @@ -42,8 +54,13 @@ type DefaultRevisionEngineFactory struct { // 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) +func (f *defaultRevisionEngineFactory) CreateRevisionEngine(_ context.Context, rev *ocv1.ClusterExtensionRevision) (RevisionEngine, error) { + saNamespace, saName, err := f.getServiceAccount(rev) + if err != nil { + return nil, err + } + + scopedClient, err := f.createScopedClient(saNamespace, saName) if err != nil { return nil, err } @@ -62,33 +79,26 @@ func (f *DefaultRevisionEngineFactory) CreateRevisionEngine(_ context.Context, r ), nil } -func (f *DefaultRevisionEngineFactory) getScopedClient(rev *ocv1.ClusterExtensionRevision) (client.Client, error) { +func (f *defaultRevisionEngineFactory) getServiceAccount(rev *ocv1.ClusterExtensionRevision) (string, 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(namespace, serviceAccountName string) (client.Client, error) { saConfig := rest.AnonymousClientConfig(f.BaseConfig) saConfig.Wrap(func(rt http.RoundTripper) http.RoundTripper { return &authentication.TokenInjectingRoundTripper{ @@ -111,7 +121,7 @@ func (f *DefaultRevisionEngineFactory) createScopedClient(namespace, serviceAcco return scopedClient, nil } -// NewDefaultRevisionEngineFactory creates a new DefaultRevisionEngineFactory. +// NewDefaultRevisionEngineFactory creates a new defaultRevisionEngineFactory. func NewDefaultRevisionEngineFactory( scheme *runtime.Scheme, trackingCache managedcache.TrackingCache, @@ -120,8 +130,14 @@ 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, @@ -129,5 +145,5 @@ func NewDefaultRevisionEngineFactory( FieldOwnerPrefix: fieldOwnerPrefix, BaseConfig: baseConfig, TokenGetter: tokenGetter, - } + }, nil } From 092099cb95b7cf130db0f237e5575ea4e5c0d990 Mon Sep 17 00:00:00 2001 From: Camila Macedo <7708031+camilamacedo86@users.noreply.github.com> Date: Mon, 12 Jan 2026 16:17:37 +0000 Subject: [PATCH 5/5] (fix): e2e: add bind/escalate verbs for Boxcutter Server-Side Apply MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add `bind` and `escalate` RBAC verbs to e2e test ServiceAccount to support Boxcutter applier's use of Kubernetes Server-Side Apply (SSA). Experimental e2e tests fail when Boxcutter uses ServiceAccount-scoped clients to apply bundle RBAC resources (ClusterRoles and ClusterRoleBindings): ``` clusterrolebindings.rbac.authorization.k8s.io is forbidden: User "system:serviceaccount:olmv1-e2e:olm-sa" cannot bind ClusterRole: RBAC: attempting to grant RBAC permissions not currently held ``` - Uses helm.sh/helm/v3 library - Applies resources with traditional CREATE/UPDATE operations - Kubernetes RBAC allows ClusterRoleBinding creation when the ServiceAccount already has all the permissions being granted (permission matching) - **Works WITHOUT `bind`/`escalate` verbs** ✅ - Uses pkg.package-operator.run/boxcutter machinery - Applies resources with **Server-Side Apply (SSA)** (`client.Apply`) - SSA enforces field-level ownership and **stricter RBAC enforcement** - Kubernetes API server **requires explicit `bind` verb** for ClusterRoleBindings - Permission matching fallback does NOT work reliably with SSA - **REQUIRES `bind`/`escalate` verbs** ❌ Validated by running actual tests: **Test 1: Main branch standard-e2e (Helm, NO bind/escalate)** ```bash make test-e2e ``` Result: ✅ PASS (21 scenarios passed) **Test 2: PR branch experimental-e2e (Boxcutter, NO bind/escalate)** ```bash make test-experimental-e2e ``` Result: ❌ FAIL (cannot bind ClusterRole error) **Test 3: PR branch experimental-e2e (Boxcutter, WITH bind/escalate)** Result: ✅ PASS (all tests pass) Add `bind` and `escalate` verbs to the e2e test RBAC template: ```yaml - apiGroups: ["rbac.authorization.k8s.io"] resources: [clusterroles, roles, clusterrolebindings, rolebindings] verbs: [ update, create, list, watch, get, delete, patch, bind, escalate ] ``` These verbs allow the ServiceAccount to: - `bind`: Create ClusterRoleBindings that reference roles with permissions the ServiceAccount doesn't have - `escalate`: Create ClusterRoles with permissions the ServiceAccount doesn't have This is the documented requirement in `docs/concepts/permission-model.md` for extension installers and aligns with Kubernetes RBAC best practices. 1. **Required for SSA**: Server-Side Apply has stricter RBAC enforcement 2. **Documented requirement**: OLMv1 docs specify bind/escalate as proper approach 3. **Industry best practice**: Operator installers should have these verbs 4. **Supports all operators**: Not just test-operator with matching permissions 5. **Maintains SSA benefits**: Field ownership, conflict resolution, GitOps support - Kubernetes RBAC: https://kubernetes.io/docs/reference/access-authn-authz/rbac/#privilege-escalation-prevention-and-bootstrapping - OLMv1 Permission Model: docs/concepts/permission-model.md - Boxcutter machinery: pkg.package-operator.run/boxcutter/machinery (uses client.Apply) - Testing evidence: FINAL_TESTED_ANSWER.md, SERVER_SIDE_APPLY_ANSWER.md Tested-by: Actual e2e test runs comparing Helm vs Boxcutter behavior Signed-off-by: Camila --- test/e2e/steps/testdata/rbac-template.yaml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/e2e/steps/testdata/rbac-template.yaml b/test/e2e/steps/testdata/rbac-template.yaml index d975d76988..0d44f9d7bd 100644 --- a/test/e2e/steps/testdata/rbac-template.yaml +++ b/test/e2e/steps/testdata/rbac-template.yaml @@ -50,7 +50,11 @@ rules: - roles - clusterrolebindings - rolebindings - verbs: [ update, create, list, watch, get, delete, patch ] + # The bind and escalate verbs allow the ServiceAccount to create role bindings + # for roles it doesn't have and grant permissions beyond its own. This is required + # because extension bundles contain their own RBAC that must be created. + # See docs/concepts/permission-model.md for details on these requirements. + verbs: [ update, create, list, watch, get, delete, patch, bind, escalate ] - apiGroups: ["coordination.k8s.io"] resources: ["leases"] verbs: [ update, create, list, watch, get, delete, patch ]