Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion cmd/operator-controller/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -598,7 +598,12 @@ func (c *boxcutterReconcilerConfigurator) Configure(ceReconciler *controllers.Cl
return err
}

// TODO: add support for preflight checks
// determine if PreAuthorizer should be enabled based on feature gate
var preAuth authorization.PreAuthorizer
if features.OperatorControllerFeatureGate.Enabled(features.PreflightPermissions) {
preAuth = authorization.NewRBACPreAuthorizer(c.mgr.GetClient())
}

// TODO: better scheme handling - which types do we want to support?
_ = apiextensionsv1.AddToScheme(c.mgr.GetScheme())
rg := &applier.SimpleRevisionGenerator{
Expand All @@ -610,6 +615,7 @@ func (c *boxcutterReconcilerConfigurator) Configure(ceReconciler *controllers.Cl
Scheme: c.mgr.GetScheme(),
RevisionGenerator: rg,
Preflights: c.preflights,
PreAuthorizer: preAuth,
FieldOwner: fmt.Sprintf("%s/clusterextension-controller", fieldOwnerPrefix),
}
revisionStatesGetter := &controllers.BoxcutterRevisionStatesGetter{Reader: c.mgr.GetClient()}
Expand Down
98 changes: 80 additions & 18 deletions internal/operator-controller/applier/boxcutter.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"context"
"errors"
"fmt"
"io"
"io/fs"
"maps"
"slices"
Expand All @@ -16,6 +17,8 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apiserver/pkg/authentication/user"
"k8s.io/apiserver/pkg/authorization/authorizer"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
Expand All @@ -25,6 +28,7 @@ import (
helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client"

ocv1 "github.com/operator-framework/operator-controller/api/v1"
"github.com/operator-framework/operator-controller/internal/operator-controller/authorization"
"github.com/operator-framework/operator-controller/internal/operator-controller/labels"
"github.com/operator-framework/operator-controller/internal/shared/util/cache"
)
Expand Down Expand Up @@ -279,28 +283,27 @@ type Boxcutter struct {
Scheme *runtime.Scheme
RevisionGenerator ClusterExtensionRevisionGenerator
Preflights []Preflight
PreAuthorizer authorization.PreAuthorizer
FieldOwner string
}

func (bc *Boxcutter) getObjects(rev *ocv1.ClusterExtensionRevision) []client.Object {
var objs []client.Object
for _, phase := range rev.Spec.Phases {
for _, phaseObject := range phase.Objects {
objs = append(objs, &phaseObject.Object)
}
}
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)
// createOrUpdate creates or updates the revision object. PreAuthorization checks are performed to ensure the
// manifestManager has sufficient permissions to manage the revision and its resources
func (bc *Boxcutter) createOrUpdate(ctx context.Context, manifestManager user.Info, rev *ocv1.ClusterExtensionRevision) error {
if rev.GetObjectKind().GroupVersionKind().Empty() {
gvk, err := apiutil.GVKForObject(rev, bc.Scheme)
if err != nil {
return err
}
obj.GetObjectKind().SetGroupVersionKind(gvk)
rev.GetObjectKind().SetGroupVersionKind(gvk)
}
return bc.Client.Patch(ctx, obj, client.Apply, client.FieldOwner(bc.FieldOwner), client.ForceOwnership)

// Run auth preflight checks
if err := bc.runPreAuthorizationChecks(ctx, manifestManager, rev); err != nil {
return err
}

return bc.Client.Patch(ctx, rev, 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) error {
Expand Down Expand Up @@ -329,7 +332,7 @@ func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust
desiredRevision.Spec.Revision = currentRevision.Spec.Revision
desiredRevision.Name = currentRevision.Name

err := bc.createOrUpdate(ctx, desiredRevision)
err := bc.createOrUpdate(ctx, getUserInfo(ext), desiredRevision)
switch {
case apierrors.IsInvalid(err):
// We could not update the current revision due to trying to update an immutable field.
Expand All @@ -344,7 +347,7 @@ func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust
}

// Preflights
plainObjs := bc.getObjects(desiredRevision)
plainObjs := getObjects(desiredRevision)
for _, preflight := range bc.Preflights {
if shouldSkipPreflight(ctx, preflight, ext, state) {
continue
Expand Down Expand Up @@ -379,14 +382,31 @@ func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust
return fmt.Errorf("garbage collecting old revisions: %w", err)
}

if err := bc.createOrUpdate(ctx, desiredRevision); err != nil {
if err := bc.createOrUpdate(ctx, getUserInfo(ext), desiredRevision); err != nil {
return fmt.Errorf("creating new Revision: %w", err)
}
}

return nil
}

// runPreAuthorizationChecks runs PreAuthorization checks if the PreAuthorizer is set. An error will be returned if
// the ClusterExtension service account does not have the necessary permissions to manage the revision's resources
func (bc *Boxcutter) runPreAuthorizationChecks(ctx context.Context, manifestManager user.Info, rev *ocv1.ClusterExtensionRevision) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that you're taking this variable name from the example already set in rbac.go, but to me the term "manifestManager" implies a more complex object than just user.Info, especially given we have stuff like the manager.Manager in our codebase.

This is purely a nitpick, but I wonder if a different variable name would help future legibility of the code. Maybe manifestUser? or manifestOwner? The latter might not be entirely accurate though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took the previous variable name, I think. Though, I'm happy to updated it to anything. The reason I thought manifestManager was reasonable is that it fits with the mental model of "we're checking permissions against the user that will manage the lifecycle of these manifests". Even just, user would be ok, imo, tho.

if bc.PreAuthorizer == nil {
return nil
}

// collect the revision manifests
manifestReader, err := revisionManifestReader(rev)
if err != nil {
return err
}

// run preauthorization check
return formatPreAuthorizerOutput(bc.PreAuthorizer.PreAuthorize(ctx, manifestManager, manifestReader, revisionManagementPerms(rev)))
}

// garbageCollectOldRevisions deletes archived revisions beyond ClusterExtensionRevisionRetentionLimit.
// Active revisions are never deleted. revisionList must be sorted oldest to newest.
func (bc *Boxcutter) garbageCollectOldRevisions(ctx context.Context, revisionList []ocv1.ClusterExtensionRevision) error {
Expand Down Expand Up @@ -445,3 +465,45 @@ func splitManifestDocuments(file string) []string {
}
return docs
}

// getObjects returns a slice of all objects in the revision
func getObjects(rev *ocv1.ClusterExtensionRevision) []client.Object {
var objs []client.Object
for _, phase := range rev.Spec.Phases {
for _, phaseObject := range phase.Objects {
objs = append(objs, &phaseObject.Object)
}
}
return objs
}

// revisionManifestReader returns an io.Reader containing all manifests in the revision
func revisionManifestReader(rev *ocv1.ClusterExtensionRevision) (io.Reader, error) {
var manifestBuilder strings.Builder
for _, obj := range getObjects(rev) {
objBytes, err := yaml.Marshal(obj)
if err != nil {
return nil, fmt.Errorf("error generating revision manifest: %w", err)
}
manifestBuilder.WriteString("---\n")
manifestBuilder.WriteString(string(objBytes))
manifestBuilder.WriteString("\n")
}
return strings.NewReader(manifestBuilder.String()), nil
}

func revisionManagementPerms(rev *ocv1.ClusterExtensionRevision) func(user.Info) []authorizer.AttributesRecord {
return func(user user.Info) []authorizer.AttributesRecord {
return []authorizer.AttributesRecord{
{
User: user,
Name: rev.Name,
APIGroup: ocv1.GroupVersion.Group,
APIVersion: ocv1.GroupVersion.Version,
Resource: "clusterextensionrevisions/finalizers",
ResourceRequest: true,
Verb: "update",
},
}
}
}
Loading
Loading