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 get permissions to get all the resources it will need to
inspect for a server-connected dry-run and returns errors detailing all
the missing get permissions. This prevents a hung server-connected
dry-run getting caught on individual missing get permissions.

Signed-off-by: Tayler Geiger <[email protected]>
  • Loading branch information
trgeiger committed Feb 13, 2025
1 parent becde51 commit 678687c
Show file tree
Hide file tree
Showing 5 changed files with 188 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 @@ -380,9 +380,12 @@ func main() {
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
2 changes: 2 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ require (
k8s.io/client-go v0.32.0
k8s.io/component-base v0.32.0
k8s.io/klog/v2 v2.130.1
k8s.io/kubernetes v1.31.2
k8s.io/utils v0.0.0-20241210054802-24370beab758
sigs.k8s.io/controller-runtime v0.19.4
sigs.k8s.io/yaml v1.4.0
Expand Down Expand Up @@ -244,6 +245,7 @@ require (
gopkg.in/inf.v0 v0.9.1 // indirect
gopkg.in/warnings.v0 v0.1.2 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
k8s.io/component-helpers v0.32.0 // indirect
k8s.io/kube-openapi v0.0.0-20241105132330-32ad38e42d3f // indirect
k8s.io/kubectl v0.32.0 // indirect
oras.land/oras-go v1.2.5 // indirect
Expand Down
4 changes: 4 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -1007,12 +1007,16 @@ k8s.io/client-go v0.32.0 h1:DimtMcnN/JIKZcrSrstiwvvZvLjG0aSxy8PxN8IChp8=
k8s.io/client-go v0.32.0/go.mod h1:boDWvdM1Drk4NJj/VddSLnx59X3OPgwrOo0vGbtq9+8=
k8s.io/component-base v0.32.0 h1:d6cWHZkCiiep41ObYQS6IcgzOUQUNpywm39KVYaUqzU=
k8s.io/component-base v0.32.0/go.mod h1:JLG2W5TUxUu5uDyKiH2R/7NnxJo1HlPoRIIbVLkK5eM=
k8s.io/component-helpers v0.32.0 h1:pQEEBmRt3pDJJX98cQvZshDgJFeKRM4YtYkMmfOlczw=
k8s.io/component-helpers v0.32.0/go.mod h1:9RuClQatbClcokXOcDWSzFKQm1huIf0FzQlPRpizlMc=
k8s.io/klog/v2 v2.130.1 h1:n9Xl7H1Xvksem4KFG4PYbdQCQxqc/tTUyrgXaOhHSzk=
k8s.io/klog/v2 v2.130.1/go.mod h1:3Jpz1GvMt720eyJH1ckRHK1EDfpxISzJ7I9OYgaDtPE=
k8s.io/kube-openapi v0.0.0-20241105132330-32ad38e42d3f h1:GA7//TjRY9yWGy1poLzYYJJ4JRdzg3+O6e8I+e+8T5Y=
k8s.io/kube-openapi v0.0.0-20241105132330-32ad38e42d3f/go.mod h1:R/HEjbvWI0qdfb8viZUeVZm0X6IZnxAydC7YU42CMw4=
k8s.io/kubectl v0.32.0 h1:rpxl+ng9qeG79YA4Em9tLSfX0G8W0vfaiPVrc/WR7Xw=
k8s.io/kubectl v0.32.0/go.mod h1:qIjSX+QgPQUgdy8ps6eKsYNF+YmFOAO3WygfucIqFiE=
k8s.io/kubernetes v1.31.2 h1:VNSu4O7Xn5FFRsh9ePXyEPg6ucR21fOftarSdi053Gs=
k8s.io/kubernetes v1.31.2/go.mod h1:9xmT2buyTYj8TRKwRae7FcuY8k5+xlxv7VivvO0KKfs=
k8s.io/utils v0.0.0-20241210054802-24370beab758 h1:sdbE21q2nlQtFh65saZY+rRM6x6aJJI8IUa1AmH/qa0=
k8s.io/utils v0.0.0-20241210054802-24370beab758/go.mod h1:OLgZIPagt7ERELqWJFomSt595RzquPNLL48iOWgYOg0=
oras.land/oras-go v1.2.5 h1:XpYuAwAb0DfQsunIyMfeET92emK8km3W4yEzZvUbsTo=
Expand Down
68 changes: 68 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,25 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte
return relObjects, state, nil
}

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 Expand Up @@ -191,6 +254,11 @@ func (h *Helm) getReleaseState(cl helmclient.ActionInterface, ext *ocv1.ClusterE
return currentRelease, desiredRelease, relState, nil
}

// RulesAllow() checks expects resource names to be lowercase and plural, there's probably a better way to do this
func sanitizeResourceName(resourceName string) string {

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

View workflow job for this annotation

GitHub Actions / lint

func `sanitizeResourceName` is unused (unused)
return strings.ToLower(resourceName) + "s"
}

type postrenderer struct {
labels map[string]string
cascade postrender.PostRenderer
Expand Down
111 changes: 111 additions & 0 deletions internal/operator-controller/authorization/authorization.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
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)

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
}

// RulesAllow() checks expects resource names to be lowercase and plural, there's probably a better way to do this
func sanitizeResourceName(resourceName string) string {
return strings.ToLower(resourceName) + "s"
}

0 comments on commit 678687c

Please sign in to comment.