Skip to content

Commit

Permalink
Add client-only helm dry-run to helm applier
Browse files Browse the repository at this point in the history
Adds a check that makes sure the installer service account has the
necessary permissions to manage all the resources in the
ClusterExtension payload and returns errors detailing all
the missing permissions. This prevents a hung server-connected
dry-run getting caught on individual missing get permissions as well
returns many other missing required permissions needed for the actual
installation or upgrade.

Signed-off-by: Tayler Geiger <tayler@redhat.com>
  • Loading branch information
trgeiger committed Feb 14, 2025
1 parent 7f00b13 commit 74c93c7
Show file tree
Hide file tree
Showing 3 changed files with 179 additions and 0 deletions.
3 changes: 3 additions & 0 deletions cmd/operator-controller/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -407,9 +407,12 @@ func run() error {
crdupgradesafety.NewPreflight(aeClient.CustomResourceDefinitions()),
}

acm := applier.NewAuthClientMapper(clientRestConfigMapper, mgr.GetConfig())

helmApplier := &applier.Helm{
ActionClientGetter: acg,
Preflights: preflights,
AuthClientMapper: acm,
}

cm := contentmanager.NewManager(clientRestConfigMapper, mgr.GetConfig(), mgr.GetRESTMapper())
Expand Down
64 changes: 64 additions & 0 deletions internal/operator-controller/applier/helm.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,15 @@ import (
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
apimachyaml "k8s.io/apimachinery/pkg/util/yaml"

Check failure on line 20 in internal/operator-controller/applier/helm.go

View workflow job for this annotation

GitHub Actions / lint

File is not properly formatted (gci)
"k8s.io/client-go/rest"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/log"

helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client"
authorizationv1client "k8s.io/client-go/kubernetes/typed/authorization/v1"

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/features"
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/convert"
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/preflights/crdupgradesafety"
Expand Down Expand Up @@ -53,9 +56,38 @@ type Preflight interface {
Upgrade(context.Context, *release.Release) error
}

type RestConfigMapper func(context.Context, client.Object, *rest.Config) (*rest.Config, error)

type AuthClientMapper struct {
rcm RestConfigMapper
baseCfg *rest.Config
}

func (acm *AuthClientMapper) GetAuthenticationClient(ctx context.Context, ext *ocv1.ClusterExtension) (*authorizationv1client.AuthorizationV1Client, error) {
authcfg, err := acm.rcm(ctx, ext, acm.baseCfg)
if err != nil {
return nil, err
}

authclient, err := authorizationv1client.NewForConfig(authcfg)
if err != nil {
return nil, err
}

return authclient, nil
}

type Helm struct {
ActionClientGetter helmclient.ActionClientGetter
Preflights []Preflight
AuthClientMapper AuthClientMapper
}

func NewAuthClientMapper(rcm RestConfigMapper, baseCfg *rest.Config) AuthClientMapper {
return AuthClientMapper{
rcm: rcm,
baseCfg: baseCfg,
}
}

// shouldSkipPreflight is a helper to determine if the preflight check is CRDUpgradeSafety AND
Expand All @@ -79,7 +111,21 @@ 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) {

Check failure on line 113 in internal/operator-controller/applier/helm.go

View workflow job for this annotation

GitHub Actions / lint

unnecessary leading newline (whitespace)

if features.OperatorControllerFeatureGate.Enabled(features.PreflightPermissions) {
authclient, err := h.AuthClientMapper.GetAuthenticationClient(ctx, ext)
if err != nil {
return nil, "", err
}

err = h.checkContentPermissions(ctx, contentFS, authclient, ext)
if err != nil {
return nil, "", err
}
}

chrt, err := convert.RegistryV1ToHelmChart(ctx, contentFS, ext.Spec.Namespace, []string{corev1.NamespaceAll})

if err != nil {
return nil, "", err
}
Expand Down Expand Up @@ -152,8 +198,26 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte
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.AuthorizationV1Client, ext *ocv1.ClusterExtension) error {
reg, err := convert.ParseFS(ctx, contentFS)
if err != nil {
return err
}

plain, err := convert.Convert(reg, ext.Spec.Namespace, []string{corev1.NamespaceAll})
if err != nil {
return err
}

err = authorization.CheckObjectPermissions(ctx, authcl, plain.Objects, ext)

return err
}

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
Expand Down
112 changes: 112 additions & 0 deletions internal/operator-controller/authorization/authorization.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
package authorization

import (
"context"
"errors"
"fmt"
"slices"
"strings"

ocv1 "github.com/operator-framework/operator-controller/api/v1"

Check failure on line 10 in internal/operator-controller/authorization/authorization.go

View workflow job for this annotation

GitHub Actions / lint

File is not properly formatted (gci)
authv1 "k8s.io/api/authorization/v1"

Check failure on line 11 in internal/operator-controller/authorization/authorization.go

View workflow job for this annotation

GitHub Actions / lint

import "k8s.io/api/authorization/v1" imported as "authv1" but must be "authorizationv1" according to config (importas)
rbacv1 "k8s.io/api/rbac/v1"
v1 "k8s.io/apimachinery/pkg/apis/meta/v1"

Check failure on line 13 in internal/operator-controller/authorization/authorization.go

View workflow job for this annotation

GitHub Actions / lint

import "k8s.io/apimachinery/pkg/apis/meta/v1" imported as "v1" but must be "metav1" according to config (importas)
authorizationv1client "k8s.io/client-go/kubernetes/typed/authorization/v1"
"sigs.k8s.io/controller-runtime/pkg/client"

Check failure on line 15 in internal/operator-controller/authorization/authorization.go

View workflow job for this annotation

GitHub Actions / lint

File is not properly formatted (gci)
)

const (
SelfSubjectRulesReview string = "SelfSubjectRulesReview"
SelfSubjectAccessReview string = "SelfSubjectAccessReview"
)

func CheckObjectPermissions(ctx context.Context, authcl *authorizationv1client.AuthorizationV1Client, objects []client.Object, ext *ocv1.ClusterExtension) error {
ssrr := &authv1.SelfSubjectRulesReview{
Spec: authv1.SelfSubjectRulesReviewSpec{
Namespace: ext.Spec.Namespace,
},
}

ssrr, err := authcl.SelfSubjectRulesReviews().Create(ctx, ssrr, v1.CreateOptions{})
if err != nil {
return err
}

rules := []rbacv1.PolicyRule{}
for _, rule := range ssrr.Status.ResourceRules {
rules = append(rules, rbacv1.PolicyRule{
Verbs: rule.Verbs,
APIGroups: rule.APIGroups,
Resources: rule.Resources,
ResourceNames: rule.ResourceNames,
})
}

for _, rule := range ssrr.Status.NonResourceRules {
rules = append(rules, rbacv1.PolicyRule{
Verbs: rule.Verbs,
NonResourceURLs: rule.NonResourceURLs,
})
}

resAttrs := []authv1.ResourceAttributes{}
namespacedErrs := []error{}
clusterScopedErrs := []error{}
requiredVerbs := []string{"get", "create", "update", "list", "watch", "delete", "patch"}

for _, o := range objects {
for _, verb := range requiredVerbs {
resAttrs = append(resAttrs, authv1.ResourceAttributes{
Namespace: o.GetNamespace(),
Verb: verb,
Resource: sanitizeResourceName(o.GetObjectKind().GroupVersionKind().Kind),
Group: o.GetObjectKind().GroupVersionKind().Group,
Name: o.GetName(),
})
}
}

for _, resAttr := range resAttrs {
if !canI(resAttr, rules) {
if resAttr.Namespace != "" {
namespacedErrs = append(namespacedErrs, fmt.Errorf("cannot %s %s %s in namespace %s",
resAttr.Verb,
strings.TrimSuffix(resAttr.Resource, "s"),
resAttr.Name,
resAttr.Namespace))
continue
}
clusterScopedErrs = append(clusterScopedErrs, fmt.Errorf("cannot %s %s %s",
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...)
}

return errors.Join(errs...)

}

Check failure on line 92 in internal/operator-controller/authorization/authorization.go

View workflow job for this annotation

GitHub Actions / lint

unnecessary trailing newline (whitespace)

// Checks if the rules allow the verb on the GroupVersionKind in resAttr
func canI(resAttr authv1.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
}
}
return canI
}

// SelfSubjectRulesReview formats the resource names as lowercase and plural
func sanitizeResourceName(resourceName string) string {
return strings.ToLower(resourceName) + "s"
}

0 comments on commit 74c93c7

Please sign in to comment.