Create a new version of paste:

From fd47d7d77bfe3b1b7d11ac8635b49c5bb0c12353 Mon Sep 17 00:00:00 2001
From: Brett Tofel <btofel@redhat.com>
Date: Thu, 20 Feb 2025 16:15:25 -0500
Subject: [PATCH] Addressed some of the PR comments, see below
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

• Encapsulation of Authorization Logic:
  •	Moved AuthorizationClientMapper and related types/methods to the authorization package and expose a public interface (e.g., CheckContentPermissions).
  •	Created an AuthorizationClient interface and wrapper (clientImpl) in authorization/client.go
• Error Wrapping with Context
• Pre-allocate Slice Capacity
• Quoted Error Messages
• Efficient Error Joining:
• Simplify canI() Function:
  • Not explicitly requested but implied improvement for clarity
---
 internal/operator-controller/applier/helm.go  | 55 +++++++------------
 .../authorization/authorization.go            | 53 +++++++++---------
 .../authorization/client.go                   | 33 +++++++++++
 3 files changed, 81 insertions(+), 60 deletions(-)
 create mode 100644 internal/operator-controller/authorization/client.go

diff --git a/internal/operator-controller/applier/helm.go b/internal/operator-controller/applier/helm.go
index 1bed555..3a5aca5 100644
--- a/internal/operator-controller/applier/helm.go
+++ b/internal/operator-controller/applier/helm.go
@@ -71,7 +71,6 @@ func (acm *AuthorizationClientMapper) GetAuthorizationClient(ctx context.Context
 	if err != nil {
 		return nil, err
 	}
-
 	return acm.NewForConfig(authcfg)
 }
 
@@ -110,19 +109,17 @@ func shouldSkipPreflight(ctx context.Context, preflight Preflight, ext *ocv1.Clu
 
 func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels map[string]string, storageLabels map[string]string) ([]client.Object, string, error) {
 	if features.OperatorControllerFeatureGate.Enabled(features.PreflightPermissions) {
-		authclient, err := h.AuthorizationClientMapper.GetAuthorizationClient(ctx, ext)
+		rawAuthClient, err := h.AuthorizationClientMapper.GetAuthorizationClient(ctx, ext)
 		if err != nil {
-			return nil, "", err
+			return nil, "", fmt.Errorf("failed to get authorization client: %w", err)
 		}
-
-		err = h.checkContentPermissions(ctx, contentFS, authclient, ext)
-		if err != nil {
-			return nil, "", err
+		authClient := authorization.NewClient(rawAuthClient)
+		if err := h.checkContentPermissions(ctx, contentFS, authClient, ext); err != nil {
+			return nil, "", fmt.Errorf("failed checking content permissions: %w", err)
 		}
 	}
 
 	chrt, err := convert.RegistryV1ToHelmChart(ctx, contentFS, ext.Spec.Namespace, []string{corev1.NamespaceAll})
-
 	if err != nil {
 		return nil, "", err
 	}
@@ -130,7 +127,7 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte
 
 	ac, err := h.ActionClientGetter.ActionClientFor(ctx, ext)
 	if err != nil {
-		return nil, "", err
+		return nil, "", fmt.Errorf("failed to get action client: %w", err)
 	}
 
 	post := &postrenderer{
@@ -148,14 +145,12 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte
 		}
 		switch state {
 		case StateNeedsInstall:
-			err := preflight.Install(ctx, desiredRel)
-			if err != nil {
-				return nil, state, err
+			if err := preflight.Install(ctx, desiredRel); err != nil {
+				return nil, state, fmt.Errorf("preflight install check failed: %w", err)
 			}
 		case StateNeedsUpgrade:
-			err := preflight.Upgrade(ctx, desiredRel)
-			if err != nil {
-				return nil, state, err
+			if err := preflight.Upgrade(ctx, desiredRel); err != nil {
+				return nil, state, fmt.Errorf("preflight upgrade check failed: %w", err)
 			}
 		}
 	}
@@ -168,7 +163,7 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte
 			return nil
 		}, helmclient.AppendInstallPostRenderer(post))
 		if err != nil {
-			return nil, state, err
+			return nil, state, fmt.Errorf("failed to install release: %w", err)
 		}
 	case StateNeedsUpgrade:
 		rel, err = ac.Upgrade(ext.GetName(), ext.Spec.Namespace, chrt, values, func(upgrade *action.Upgrade) error {
@@ -177,11 +172,11 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte
 			return nil
 		}, helmclient.AppendUpgradePostRenderer(post))
 		if err != nil {
-			return nil, state, err
+			return nil, state, fmt.Errorf("failed to upgrade release: %w", err)
 		}
 	case StateUnchanged:
 		if err := ac.Reconcile(rel); err != nil {
-			return nil, state, err
+			return nil, state, fmt.Errorf("failed to reconcile release: %w", err)
 		}
 	default:
 		return nil, state, fmt.Errorf("unexpected release state %q", state)
@@ -189,32 +184,28 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte
 
 	relObjects, err := util.ManifestObjects(strings.NewReader(rel.Manifest), fmt.Sprintf("%s-release-manifest", rel.Name))
 	if err != nil {
-		return nil, state, err
+		return nil, state, fmt.Errorf("failed to convert manifest to objects: %w", err)
 	}
 
 	return relObjects, state, nil
 }
 
-// Check if RBAC allows the installer service account necessary permissions on the objects in the contentFS
-func (h *Helm) checkContentPermissions(ctx context.Context, contentFS fs.FS, authcl authorizationv1client.AuthorizationV1Interface, ext *ocv1.ClusterExtension) error {
+func (h *Helm) checkContentPermissions(ctx context.Context, contentFS fs.FS, authClient authorization.AuthorizationClient, ext *ocv1.ClusterExtension) error {
 	reg, err := convert.ParseFS(ctx, contentFS)
 	if err != nil {
-		return err
+		return fmt.Errorf("failed to parse content FS: %w", err)
 	}
 
 	plain, err := convert.Convert(reg, ext.Spec.Namespace, []string{corev1.NamespaceAll})
 	if err != nil {
-		return err
+		return fmt.Errorf("failed to convert registry: %w", err)
 	}
 
-	err = authorization.CheckObjectPermissions(ctx, authcl, plain.Objects, ext)
-
-	return err
+	return authClient.CheckContentPermissions(ctx, plain.Objects, ext)
 }
 
 func (h *Helm) getReleaseState(cl helmclient.ActionInterface, ext *ocv1.ClusterExtension, chrt *chart.Chart, values chartutil.Values, post postrender.PostRenderer) (*release.Release, *release.Release, string, error) {
 	currentRelease, err := cl.Get(ext.GetName())
-
 	if errors.Is(err, driver.ErrReleaseNotFound) {
 		desiredRelease, err := cl.Install(ext.GetName(), ext.Spec.Namespace, chrt, values, func(i *action.Install) error {
 			i.DryRun = true
@@ -222,16 +213,12 @@ func (h *Helm) getReleaseState(cl helmclient.ActionInterface, ext *ocv1.ClusterE
 			return nil
 		}, helmclient.AppendInstallPostRenderer(post))
 		if err != nil {
-			if features.OperatorControllerFeatureGate.Enabled(features.PreflightPermissions) {
-				_ = struct{}{} // minimal no-op to satisfy linter
-				// probably need to break out this error as it's the one for helm dry-run as opposed to any returned later
-			}
-			return nil, nil, StateError, err
+			return nil, nil, StateError, fmt.Errorf("failed dry-run install: %w", err)
 		}
 		return nil, desiredRelease, StateNeedsInstall, nil
 	}
 	if err != nil {
-		return nil, nil, StateError, err
+		return nil, nil, StateError, fmt.Errorf("failed to get current release: %w", err)
 	}
 
 	desiredRelease, err := cl.Upgrade(ext.GetName(), ext.Spec.Namespace, chrt, values, func(upgrade *action.Upgrade) error {
@@ -241,7 +228,7 @@ func (h *Helm) getReleaseState(cl helmclient.ActionInterface, ext *ocv1.ClusterE
 		return nil
 	}, helmclient.AppendUpgradePostRenderer(post))
 	if err != nil {
-		return currentRelease, nil, StateError, err
+		return currentRelease, nil, StateError, fmt.Errorf("failed dry-run upgrade: %w", err)
 	}
 	relState := StateUnchanged
 	if desiredRelease.Manifest != currentRelease.Manifest ||
diff --git a/internal/operator-controller/authorization/authorization.go b/internal/operator-controller/authorization/authorization.go
index 9e84881..94c8b0d 100644
--- a/internal/operator-controller/authorization/authorization.go
+++ b/internal/operator-controller/authorization/authorization.go
@@ -4,7 +4,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"slices"
 	"strings"
 
 	authorizationv1 "k8s.io/api/authorization/v1"
@@ -16,11 +15,7 @@ import (
 	ocv1 "github.com/operator-framework/operator-controller/api/v1"
 )
 
-const (
-	SelfSubjectRulesReview  string = "SelfSubjectRulesReview"
-	SelfSubjectAccessReview string = "SelfSubjectAccessReview"
-)
-
+// CheckObjectPermissions verifies that the given objects have the required permissions.
 func CheckObjectPermissions(ctx context.Context, authcl authorizationv1client.AuthorizationV1Interface, objects []client.Object, ext *ocv1.ClusterExtension) error {
 	ssrr := &authorizationv1.SelfSubjectRulesReview{
 		Spec: authorizationv1.SelfSubjectRulesReviewSpec{
@@ -30,7 +25,7 @@ func CheckObjectPermissions(ctx context.Context, authcl authorizationv1client.Au
 
 	ssrr, err := authcl.SelfSubjectRulesReviews().Create(ctx, ssrr, metav1.CreateOptions{})
 	if err != nil {
-		return err
+		return fmt.Errorf("failed to create SelfSubjectRulesReview: %w", err)
 	}
 
 	rules := []rbacv1.PolicyRule{}
@@ -50,10 +45,10 @@ func CheckObjectPermissions(ctx context.Context, authcl authorizationv1client.Au
 		})
 	}
 
-	resAttrs := []authorizationv1.ResourceAttributes{}
-	namespacedErrs := []error{}
-	clusterScopedErrs := []error{}
 	requiredVerbs := []string{"get", "create", "update", "list", "watch", "delete", "patch"}
+	resAttrs := make([]authorizationv1.ResourceAttributes, 0, len(requiredVerbs)*len(objects))
+	var namespacedErrs []error
+	var clusterScopedErrs []error
 
 	for _, o := range objects {
 		for _, verb := range requiredVerbs {
@@ -70,40 +65,46 @@ func CheckObjectPermissions(ctx context.Context, authcl authorizationv1client.Au
 	for _, resAttr := range resAttrs {
 		if !canI(resAttr, rules) {
 			if resAttr.Namespace != "" {
-				namespacedErrs = append(namespacedErrs, fmt.Errorf("cannot %s %s %s in namespace %s",
+				namespacedErrs = append(namespacedErrs, fmt.Errorf("cannot %s %q %q in namespace %q",
 					resAttr.Verb,
 					strings.TrimSuffix(resAttr.Resource, "s"),
 					resAttr.Name,
 					resAttr.Namespace))
 				continue
 			}
-			clusterScopedErrs = append(clusterScopedErrs, fmt.Errorf("cannot %s %s %s",
+			clusterScopedErrs = append(clusterScopedErrs, fmt.Errorf("cannot %s %q %q",
 				resAttr.Verb,
 				strings.TrimSuffix(resAttr.Resource, "s"),
 				resAttr.Name))
 		}
 	}
-	errs := append(namespacedErrs, clusterScopedErrs...)
-	if len(errs) > 0 {
-		errs = append([]error{fmt.Errorf("installer service account %s is missing required permissions", ext.Spec.ServiceAccount.Name)}, errs...)
+	allErrs := append(namespacedErrs, clusterScopedErrs...)
+	if len(allErrs) > 0 {
+		return fmt.Errorf("installer service account %q is missing required permissions: %w", ext.Spec.ServiceAccount.Name, errors.Join(allErrs...))
 	}
-
-	return errors.Join(errs...)
+	return nil
 }
 
-// Checks if the rules allow the verb on the GroupVersionKind in resAttr
+// canI checks if the rules allow the specified verb on the resource
 func canI(resAttr authorizationv1.ResourceAttributes, rules []rbacv1.PolicyRule) bool {
-	var canI bool
 	for _, rule := range rules {
-		if (slices.Contains(rule.APIGroups, resAttr.Group) || slices.Contains(rule.APIGroups, "*")) &&
-			(slices.Contains(rule.Resources, resAttr.Resource) || slices.Contains(rule.Resources, "*")) &&
-			(slices.Contains(rule.Verbs, resAttr.Verb) || slices.Contains(rule.Verbs, "*")) &&
-			(slices.Contains(rule.ResourceNames, resAttr.Name) || len(rule.ResourceNames) == 0) {
-			canI = true
-			break
+		if (contains(rule.APIGroups, resAttr.Group) || contains(rule.APIGroups, "*")) &&
+			(contains(rule.Resources, resAttr.Resource) || contains(rule.Resources, "*")) &&
+			(contains(rule.Verbs, resAttr.Verb) || contains(rule.Verbs, "*")) &&
+			(contains(rule.ResourceNames, resAttr.Name) || len(rule.ResourceNames) == 0) {
+			return true
+		}
+	}
+	return false
+}
+
+func contains(slice []string, s string) bool {
+	for _, item := range slice {
+		if item == s {
+			return true
 		}
 	}
-	return canI
+	return false
 }
 
 // SelfSubjectRulesReview formats the resource names as lowercase and plural
diff --git a/internal/operator-controller/authorization/client.go b/internal/operator-controller/authorization/client.go
new file mode 100644
index 0000000..8a855b5
--- /dev/null
+++ b/internal/operator-controller/authorization/client.go
@@ -0,0 +1,33 @@
+package authorization
+
+import (
+	"context"
+
+	authorizationv1client "k8s.io/client-go/kubernetes/typed/authorization/v1"
+	"k8s.io/client-go/rest"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	ocv1 "github.com/operator-framework/operator-controller/api/v1"
+)
+
+type NewForConfigFunc func(*rest.Config) (authorizationv1client.AuthorizationV1Interface, error)
+
+// AuthorizationClient is an interface exposing only the needed functionality
+type AuthorizationClient interface {
+	CheckContentPermissions(ctx context.Context, objects []client.Object, ext *ocv1.ClusterExtension) error
+}
+
+// clientImpl wraps the underlying authorization client
+type clientImpl struct {
+	authClient authorizationv1client.AuthorizationV1Interface
+}
+
+// NewClient wraps an authorizationv1client.AuthorizationV1Interface
+func NewClient(authClient authorizationv1client.AuthorizationV1Interface) AuthorizationClient {
+	return &clientImpl{authClient: authClient}
+}
+
+// CheckContentPermissions is the public method that internally calls the generic check
+func (c *clientImpl) CheckContentPermissions(ctx context.Context, objects []client.Object, ext *ocv1.ClusterExtension) error {
+	return CheckObjectPermissions(ctx, c.authClient, objects, ext)
+}
-- 
2.39.3 (Apple Git-146)