API tools faq
paste
Advertisement
bentito

Untitled

bentito
Feb 26th, 2025 (edited)
672
0
Never
Add comment
Not a member of Pastebin yet? Sign Up, it unlocks many cool features!
Diff 13.77 KB | None | 0 0
raw download clone embed print report
  1. From fd47d7d77bfe3b1b7d11ac8635b49c5bb0c12353 Mon Sep 17 00:00:00 2001
  2. From: Brett Tofel <[email protected]>
  3. Date: Thu, 20 Feb 2025 16:15:25 -0500
  4. Subject: [PATCH] Addressed some of the PR comments, see below
  5. MIME-Version: 1.0
  6. Content-Type: text/plain; charset=UTF-8
  7. Content-Transfer-Encoding: 8bit
  8.  
  9. • Encapsulation of Authorization Logic:
  10.   •   Moved AuthorizationClientMapper and related types/methods to the authorization package and expose a public interface (e.g., CheckContentPermissions).
  11.   •   Created an AuthorizationClient interface and wrapper (clientImpl) in authorization/client.go
  12. • Error Wrapping with Context
  13. • Pre-allocate Slice Capacity
  14. • Quoted Error Messages
  15. • Efficient Error Joining:
  16. • Simplify canI() Function:
  17.   • Not explicitly requested but implied improvement for clarity
  18. ---
  19. internal/operator-controller/applier/helm.go  | 55 +++++++------------
  20.  .../authorization/authorization.go            | 53 +++++++++---------
  21.  .../authorization/client.go                   | 33 +++++++++++
  22.  3 files changed, 81 insertions(+), 60 deletions(-)
  23.  create mode 100644 internal/operator-controller/authorization/client.go
  24.  
  25. diff --git a/internal/operator-controller/applier/helm.go b/internal/operator-controller/applier/helm.go
  26. index 1bed555..3a5aca5 100644
  27. --- a/internal/operator-controller/applier/helm.go
  28. +++ b/internal/operator-controller/applier/helm.go
  29. @@ -71,7 +71,6 @@ func (acm *AuthorizationClientMapper) GetAuthorizationClient(ctx context.Context
  30.     if err != nil {
  31.         return nil, err
  32.     }
  33. -
  34.     return acm.NewForConfig(authcfg)
  35.  }
  36.  
  37. @@ -110,19 +109,17 @@ func shouldSkipPreflight(ctx context.Context, preflight Preflight, ext *ocv1.Clu
  38.  
  39.  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) {
  40.     if features.OperatorControllerFeatureGate.Enabled(features.PreflightPermissions) {
  41. -       authclient, err := h.AuthorizationClientMapper.GetAuthorizationClient(ctx, ext)
  42. +       rawAuthClient, err := h.AuthorizationClientMapper.GetAuthorizationClient(ctx, ext)
  43.         if err != nil {
  44. -           return nil, "", err
  45. +           return nil, "", fmt.Errorf("failed to get authorization client: %w", err)
  46.         }
  47. -
  48. -       err = h.checkContentPermissions(ctx, contentFS, authclient, ext)
  49. -       if err != nil {
  50. -           return nil, "", err
  51. +       authClient := authorization.NewClient(rawAuthClient)
  52. +       if err := h.checkContentPermissions(ctx, contentFS, authClient, ext); err != nil {
  53. +           return nil, "", fmt.Errorf("failed checking content permissions: %w", err)
  54.         }
  55.     }
  56.  
  57.     chrt, err := convert.RegistryV1ToHelmChart(ctx, contentFS, ext.Spec.Namespace, []string{corev1.NamespaceAll})
  58. -
  59.     if err != nil {
  60.         return nil, "", err
  61.     }
  62. @@ -130,7 +127,7 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte
  63.  
  64.     ac, err := h.ActionClientGetter.ActionClientFor(ctx, ext)
  65.     if err != nil {
  66. -       return nil, "", err
  67. +       return nil, "", fmt.Errorf("failed to get action client: %w", err)
  68.     }
  69.  
  70.     post := &postrenderer{
  71. @@ -148,14 +145,12 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte
  72.         }
  73.         switch state {
  74.         case StateNeedsInstall:
  75. -           err := preflight.Install(ctx, desiredRel)
  76. -           if err != nil {
  77. -               return nil, state, err
  78. +           if err := preflight.Install(ctx, desiredRel); err != nil {
  79. +               return nil, state, fmt.Errorf("preflight install check failed: %w", err)
  80.             }
  81.         case StateNeedsUpgrade:
  82. -           err := preflight.Upgrade(ctx, desiredRel)
  83. -           if err != nil {
  84. -               return nil, state, err
  85. +           if err := preflight.Upgrade(ctx, desiredRel); err != nil {
  86. +               return nil, state, fmt.Errorf("preflight upgrade check failed: %w", err)
  87.             }
  88.         }
  89.     }
  90. @@ -168,7 +163,7 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte
  91.             return nil
  92.         }, helmclient.AppendInstallPostRenderer(post))
  93.         if err != nil {
  94. -           return nil, state, err
  95. +           return nil, state, fmt.Errorf("failed to install release: %w", err)
  96.         }
  97.     case StateNeedsUpgrade:
  98.         rel, err = ac.Upgrade(ext.GetName(), ext.Spec.Namespace, chrt, values, func(upgrade *action.Upgrade) error {
  99. @@ -177,11 +172,11 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte
  100.             return nil
  101.         }, helmclient.AppendUpgradePostRenderer(post))
  102.         if err != nil {
  103. -           return nil, state, err
  104. +           return nil, state, fmt.Errorf("failed to upgrade release: %w", err)
  105.         }
  106.     case StateUnchanged:
  107.         if err := ac.Reconcile(rel); err != nil {
  108. -           return nil, state, err
  109. +           return nil, state, fmt.Errorf("failed to reconcile release: %w", err)
  110.         }
  111.     default:
  112.         return nil, state, fmt.Errorf("unexpected release state %q", state)
  113. @@ -189,32 +184,28 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte
  114.  
  115.     relObjects, err := util.ManifestObjects(strings.NewReader(rel.Manifest), fmt.Sprintf("%s-release-manifest", rel.Name))
  116.     if err != nil {
  117. -       return nil, state, err
  118. +       return nil, state, fmt.Errorf("failed to convert manifest to objects: %w", err)
  119.     }
  120.  
  121.     return relObjects, state, nil
  122.  }
  123.  
  124. -// Check if RBAC allows the installer service account necessary permissions on the objects in the contentFS
  125. -func (h *Helm) checkContentPermissions(ctx context.Context, contentFS fs.FS, authcl authorizationv1client.AuthorizationV1Interface, ext *ocv1.ClusterExtension) error {
  126. +func (h *Helm) checkContentPermissions(ctx context.Context, contentFS fs.FS, authClient authorization.AuthorizationClient, ext *ocv1.ClusterExtension) error {
  127.     reg, err := convert.ParseFS(ctx, contentFS)
  128.     if err != nil {
  129. -       return err
  130. +       return fmt.Errorf("failed to parse content FS: %w", err)
  131.     }
  132.  
  133.     plain, err := convert.Convert(reg, ext.Spec.Namespace, []string{corev1.NamespaceAll})
  134.     if err != nil {
  135. -       return err
  136. +       return fmt.Errorf("failed to convert registry: %w", err)
  137.     }
  138.  
  139. -   err = authorization.CheckObjectPermissions(ctx, authcl, plain.Objects, ext)
  140. -
  141. -   return err
  142. +   return authClient.CheckContentPermissions(ctx, plain.Objects, ext)
  143.  }
  144.  
  145.  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) {
  146.     currentRelease, err := cl.Get(ext.GetName())
  147. -
  148.     if errors.Is(err, driver.ErrReleaseNotFound) {
  149.         desiredRelease, err := cl.Install(ext.GetName(), ext.Spec.Namespace, chrt, values, func(i *action.Install) error {
  150.             i.DryRun = true
  151. @@ -222,16 +213,12 @@ func (h *Helm) getReleaseState(cl helmclient.ActionInterface, ext *ocv1.ClusterE
  152.             return nil
  153.         }, helmclient.AppendInstallPostRenderer(post))
  154.         if err != nil {
  155. -           if features.OperatorControllerFeatureGate.Enabled(features.PreflightPermissions) {
  156. -               _ = struct{}{} // minimal no-op to satisfy linter
  157. -               // probably need to break out this error as it's the one for helm dry-run as opposed to any returned later
  158. -           }
  159. -           return nil, nil, StateError, err
  160. +           return nil, nil, StateError, fmt.Errorf("failed dry-run install: %w", err)
  161.         }
  162.         return nil, desiredRelease, StateNeedsInstall, nil
  163.     }
  164.     if err != nil {
  165. -       return nil, nil, StateError, err
  166. +       return nil, nil, StateError, fmt.Errorf("failed to get current release: %w", err)
  167.     }
  168.  
  169.     desiredRelease, err := cl.Upgrade(ext.GetName(), ext.Spec.Namespace, chrt, values, func(upgrade *action.Upgrade) error {
  170. @@ -241,7 +228,7 @@ func (h *Helm) getReleaseState(cl helmclient.ActionInterface, ext *ocv1.ClusterE
  171.         return nil
  172.     }, helmclient.AppendUpgradePostRenderer(post))
  173.     if err != nil {
  174. -       return currentRelease, nil, StateError, err
  175. +       return currentRelease, nil, StateError, fmt.Errorf("failed dry-run upgrade: %w", err)
  176.     }
  177.     relState := StateUnchanged
  178.     if desiredRelease.Manifest != currentRelease.Manifest ||
  179. diff --git a/internal/operator-controller/authorization/authorization.go b/internal/operator-controller/authorization/authorization.go
  180. index 9e84881..94c8b0d 100644
  181. --- a/internal/operator-controller/authorization/authorization.go
  182. +++ b/internal/operator-controller/authorization/authorization.go
  183. @@ -4,7 +4,6 @@ import (
  184.     "context"
  185.     "errors"
  186.     "fmt"
  187. -   "slices"
  188.     "strings"
  189.  
  190.     authorizationv1 "k8s.io/api/authorization/v1"
  191. @@ -16,11 +15,7 @@ import (
  192.     ocv1 "github.com/operator-framework/operator-controller/api/v1"
  193.  )
  194.  
  195. -const (
  196. -   SelfSubjectRulesReview  string = "SelfSubjectRulesReview"
  197. -   SelfSubjectAccessReview string = "SelfSubjectAccessReview"
  198. -)
  199. -
  200. +// CheckObjectPermissions verifies that the given objects have the required permissions.
  201.  func CheckObjectPermissions(ctx context.Context, authcl authorizationv1client.AuthorizationV1Interface, objects []client.Object, ext *ocv1.ClusterExtension) error {
  202.     ssrr := &authorizationv1.SelfSubjectRulesReview{
  203.         Spec: authorizationv1.SelfSubjectRulesReviewSpec{
  204. @@ -30,7 +25,7 @@ func CheckObjectPermissions(ctx context.Context, authcl authorizationv1client.Au
  205.  
  206.     ssrr, err := authcl.SelfSubjectRulesReviews().Create(ctx, ssrr, metav1.CreateOptions{})
  207.     if err != nil {
  208. -       return err
  209. +       return fmt.Errorf("failed to create SelfSubjectRulesReview: %w", err)
  210.     }
  211.  
  212.     rules := []rbacv1.PolicyRule{}
  213. @@ -50,10 +45,10 @@ func CheckObjectPermissions(ctx context.Context, authcl authorizationv1client.Au
  214.         })
  215.     }
  216.  
  217. -   resAttrs := []authorizationv1.ResourceAttributes{}
  218. -   namespacedErrs := []error{}
  219. -   clusterScopedErrs := []error{}
  220.     requiredVerbs := []string{"get", "create", "update", "list", "watch", "delete", "patch"}
  221. +   resAttrs := make([]authorizationv1.ResourceAttributes, 0, len(requiredVerbs)*len(objects))
  222. +   var namespacedErrs []error
  223. +   var clusterScopedErrs []error
  224.  
  225.     for _, o := range objects {
  226.         for _, verb := range requiredVerbs {
  227. @@ -70,40 +65,46 @@ func CheckObjectPermissions(ctx context.Context, authcl authorizationv1client.Au
  228.     for _, resAttr := range resAttrs {
  229.         if !canI(resAttr, rules) {
  230.             if resAttr.Namespace != "" {
  231. -               namespacedErrs = append(namespacedErrs, fmt.Errorf("cannot %s %s %s in namespace %s",
  232. +               namespacedErrs = append(namespacedErrs, fmt.Errorf("cannot %s %q %q in namespace %q",
  233.                     resAttr.Verb,
  234.                     strings.TrimSuffix(resAttr.Resource, "s"),
  235.                     resAttr.Name,
  236.                     resAttr.Namespace))
  237.                 continue
  238.             }
  239. -           clusterScopedErrs = append(clusterScopedErrs, fmt.Errorf("cannot %s %s %s",
  240. +           clusterScopedErrs = append(clusterScopedErrs, fmt.Errorf("cannot %s %q %q",
  241.                 resAttr.Verb,
  242.                 strings.TrimSuffix(resAttr.Resource, "s"),
  243.                 resAttr.Name))
  244.         }
  245.     }
  246. -   errs := append(namespacedErrs, clusterScopedErrs...)
  247. -   if len(errs) > 0 {
  248. -       errs = append([]error{fmt.Errorf("installer service account %s is missing required permissions", ext.Spec.ServiceAccount.Name)}, errs...)
  249. +   allErrs := append(namespacedErrs, clusterScopedErrs...)
  250. +   if len(allErrs) > 0 {
  251. +       return fmt.Errorf("installer service account %q is missing required permissions: %w", ext.Spec.ServiceAccount.Name, errors.Join(allErrs...))
  252.     }
  253. -
  254. -   return errors.Join(errs...)
  255. +   return nil
  256.  }
  257.  
  258. -// Checks if the rules allow the verb on the GroupVersionKind in resAttr
  259. +// canI checks if the rules allow the specified verb on the resource
  260.  func canI(resAttr authorizationv1.ResourceAttributes, rules []rbacv1.PolicyRule) bool {
  261. -   var canI bool
  262.     for _, rule := range rules {
  263. -       if (slices.Contains(rule.APIGroups, resAttr.Group) || slices.Contains(rule.APIGroups, "*")) &&
  264. -           (slices.Contains(rule.Resources, resAttr.Resource) || slices.Contains(rule.Resources, "*")) &&
  265. -           (slices.Contains(rule.Verbs, resAttr.Verb) || slices.Contains(rule.Verbs, "*")) &&
  266. -           (slices.Contains(rule.ResourceNames, resAttr.Name) || len(rule.ResourceNames) == 0) {
  267. -           canI = true
  268. -           break
  269. +       if (contains(rule.APIGroups, resAttr.Group) || contains(rule.APIGroups, "*")) &&
  270. +           (contains(rule.Resources, resAttr.Resource) || contains(rule.Resources, "*")) &&
  271. +           (contains(rule.Verbs, resAttr.Verb) || contains(rule.Verbs, "*")) &&
  272. +           (contains(rule.ResourceNames, resAttr.Name) || len(rule.ResourceNames) == 0) {
  273. +           return true
  274. +       }
  275. +   }
  276. +   return false
  277. +}
  278. +
  279. +func contains(slice []string, s string) bool {
  280. +   for _, item := range slice {
  281. +       if item == s {
  282. +           return true
  283.         }
  284.     }
  285. -   return canI
  286. +   return false
  287.  }
  288.  
  289.  // SelfSubjectRulesReview formats the resource names as lowercase and plural
  290. diff --git a/internal/operator-controller/authorization/client.go b/internal/operator-controller/authorization/client.go
  291. new file mode 100644
  292. index 0000000..8a855b5
  293. --- /dev/null
  294. +++ b/internal/operator-controller/authorization/client.go
  295. @@ -0,0 +1,33 @@
  296. +package authorization
  297. +
  298. +import (
  299. +   "context"
  300. +
  301. +   authorizationv1client "k8s.io/client-go/kubernetes/typed/authorization/v1"
  302. +   "k8s.io/client-go/rest"
  303. +   "sigs.k8s.io/controller-runtime/pkg/client"
  304. +
  305. +   ocv1 "github.com/operator-framework/operator-controller/api/v1"
  306. +)
  307. +
  308. +type NewForConfigFunc func(*rest.Config) (authorizationv1client.AuthorizationV1Interface, error)
  309. +
  310. +// AuthorizationClient is an interface exposing only the needed functionality
  311. +type AuthorizationClient interface {
  312. +   CheckContentPermissions(ctx context.Context, objects []client.Object, ext *ocv1.ClusterExtension) error
  313. +}
  314. +
  315. +// clientImpl wraps the underlying authorization client
  316. +type clientImpl struct {
  317. +   authClient authorizationv1client.AuthorizationV1Interface
  318. +}
  319. +
  320. +// NewClient wraps an authorizationv1client.AuthorizationV1Interface
  321. +func NewClient(authClient authorizationv1client.AuthorizationV1Interface) AuthorizationClient {
  322. +   return &clientImpl{authClient: authClient}
  323. +}
  324. +
  325. +// CheckContentPermissions is the public method that internally calls the generic check
  326. +func (c *clientImpl) CheckContentPermissions(ctx context.Context, objects []client.Object, ext *ocv1.ClusterExtension) error {
  327. +   return CheckObjectPermissions(ctx, c.authClient, objects, ext)
  328. +}
  329. --
  330. 2.39.3 (Apple Git-146)
  331.  
  332.  
Advertisement
Add Comment
Please, Sign In to add comment
Advertisement
Public Pastes
Advertisement
We use cookies for various purposes including analytics. By continuing to use Pastebin, you agree to our use of cookies as described in the Cookies Policy.  OK, I Understand
Not a member of Pastebin yet?
Sign Up, it unlocks many cool features!