Skip to content

Commit b632003

Browse files
trgeigerbentito
trgeiger
and
bentito
committed
Add authorization client
Adds authorization client to authorization package which helps with testing. Also makes errors more descriptive. Signed-off-by: Tayler Geiger <[email protected]> Co-authored-by: Brett Tofel <[email protected]>
1 parent 7c9a05e commit b632003

File tree

4 files changed

+76
-59
lines changed

4 files changed

+76
-59
lines changed

‎internal/operator-controller/applier/helm.go‎

Lines changed: 31 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -82,13 +82,13 @@ func shouldSkipPreflight(ctx context.Context, preflight Preflight, ext *ocv1.Clu
8282

8383
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) {
8484
if features.OperatorControllerFeatureGate.Enabled(features.PreflightPermissions) {
85-
authclient, err := h.AuthorizationClientMapper.GetAuthorizationClient(ctx, ext)
85+
rawAuthClient, err := h.AuthorizationClientMapper.GetAuthorizationClient(ctx, ext)
8686
if err != nil {
87-
return nil, "", err
87+
return nil, "", fmt.Errorf("failed to get authorization client: %w", err)
8888
}
8989

90-
err = h.AuthorizationClientMapper.CheckContentPermissions(ctx, contentFS, authclient, ext)
91-
if err != nil {
90+
authClient := authorization.NewClient(rawAuthClient)
91+
if err := h.checkContentPermissions(ctx, contentFS, authClient, ext); err != nil {
9292
return nil, "", fmt.Errorf("failed checking content permissions: %w", err)
9393
}
9494
}
@@ -102,7 +102,7 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte
102102

103103
ac, err := h.ActionClientGetter.ActionClientFor(ctx, ext)
104104
if err != nil {
105-
return nil, "", err
105+
return nil, "", fmt.Errorf("failed to get action client: %w", err)
106106
}
107107

108108
post := &postrenderer{
@@ -120,14 +120,12 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte
120120
}
121121
switch state {
122122
case StateNeedsInstall:
123-
err := preflight.Install(ctx, desiredRel)
124-
if err != nil {
125-
return nil, state, err
123+
if err := preflight.Install(ctx, desiredRel); err != nil {
124+
return nil, state, fmt.Errorf("preflight install check failed: %w", err)
126125
}
127126
case StateNeedsUpgrade:
128-
err := preflight.Upgrade(ctx, desiredRel)
129-
if err != nil {
130-
return nil, state, err
127+
if err := preflight.Upgrade(ctx, desiredRel); err != nil {
128+
return nil, state, fmt.Errorf("preflight upgrade check failed: %w", err)
131129
}
132130
}
133131
}
@@ -140,7 +138,7 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte
140138
return nil
141139
}, helmclient.AppendInstallPostRenderer(post))
142140
if err != nil {
143-
return nil, state, err
141+
return nil, state, fmt.Errorf("failed to install release: %w", err)
144142
}
145143
case StateNeedsUpgrade:
146144
rel, err = ac.Upgrade(ext.GetName(), ext.Spec.Namespace, chrt, values, func(upgrade *action.Upgrade) error {
@@ -149,24 +147,39 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte
149147
return nil
150148
}, helmclient.AppendUpgradePostRenderer(post))
151149
if err != nil {
152-
return nil, state, err
150+
return nil, state, fmt.Errorf("failed to upgrade release: %w", err)
153151
}
154152
case StateUnchanged:
155153
if err := ac.Reconcile(rel); err != nil {
156-
return nil, state, err
154+
return nil, state, fmt.Errorf("failed to reconcile release: %w", err)
157155
}
158156
default:
159157
return nil, state, fmt.Errorf("unexpected release state %q", state)
160158
}
161159

162160
relObjects, err := util.ManifestObjects(strings.NewReader(rel.Manifest), fmt.Sprintf("%s-release-manifest", rel.Name))
163161
if err != nil {
164-
return nil, state, err
162+
return nil, state, fmt.Errorf("failed to convert manifest to objects: %w", err)
165163
}
166164

167165
return relObjects, state, nil
168166
}
169167

168+
// Check if RBAC allows the installer service account necessary permissions on the objects in the contentFS
169+
func (h *Helm) checkContentPermissions(ctx context.Context, contentFS fs.FS, authClient authorization.AuthorizationClient, ext *ocv1.ClusterExtension) error {
170+
reg, err := convert.ParseFS(ctx, contentFS)
171+
if err != nil {
172+
return fmt.Errorf("failed to parse content FS: %w", err)
173+
}
174+
175+
plain, err := convert.Convert(reg, ext.Spec.Namespace, []string{corev1.NamespaceAll})
176+
if err != nil {
177+
return fmt.Errorf("failed to convert registry: %w", err)
178+
}
179+
180+
return authClient.CheckContentPermissions(ctx, plain.Objects, ext)
181+
}
182+
170183
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) {
171184
currentRelease, err := cl.Get(ext.GetName())
172185

@@ -177,16 +190,12 @@ func (h *Helm) getReleaseState(cl helmclient.ActionInterface, ext *ocv1.ClusterE
177190
return nil
178191
}, helmclient.AppendInstallPostRenderer(post))
179192
if err != nil {
180-
if features.OperatorControllerFeatureGate.Enabled(features.PreflightPermissions) {
181-
_ = struct{}{} // minimal no-op to satisfy linter
182-
// probably need to break out this error as it's the one for helm dry-run as opposed to any returned later
183-
}
184-
return nil, nil, StateError, err
193+
return nil, nil, StateError, fmt.Errorf("failed dry-run install: %w", err)
185194
}
186195
return nil, desiredRelease, StateNeedsInstall, nil
187196
}
188197
if err != nil {
189-
return nil, nil, StateError, err
198+
return nil, nil, StateError, fmt.Errorf("failed to get current release: %w", err)
190199
}
191200

192201
desiredRelease, err := cl.Upgrade(ext.GetName(), ext.Spec.Namespace, chrt, values, func(upgrade *action.Upgrade) error {
@@ -196,7 +205,7 @@ func (h *Helm) getReleaseState(cl helmclient.ActionInterface, ext *ocv1.ClusterE
196205
return nil
197206
}, helmclient.AppendUpgradePostRenderer(post))
198207
if err != nil {
199-
return currentRelease, nil, StateError, err
208+
return currentRelease, nil, StateError, fmt.Errorf("failed dry-run upgrade: %w", err)
200209
}
201210
relState := StateUnchanged
202211
if desiredRelease.Manifest != currentRelease.Manifest ||

‎internal/operator-controller/applier/helm_test.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import (
2424

2525
v1 "github.com/operator-framework/operator-controller/api/v1"
2626
"github.com/operator-framework/operator-controller/internal/operator-controller/applier"
27+
"github.com/operator-framework/operator-controller/internal/operator-controller/authorization"
2728
"github.com/operator-framework/operator-controller/internal/operator-controller/features"
2829
)
2930

@@ -121,7 +122,7 @@ func newPassingAuthorizationClientMapper() applier.AuthorizationClientMapper {
121122
mockRCM := func(ctx context.Context, obj client.Object, cfg *rest.Config) (*rest.Config, error) {
122123
return cfg, nil
123124
}
124-
acm := applier.NewAuthorizationClientMapper(mockRCM, fakeRestConfig)
125+
acm := authorization.NewAuthorizationClientMapper(mockRCM, fakeRestConfig)
125126
acm.NewForConfig = func(*rest.Config) (authorizationv1client.AuthorizationV1Interface, error) {
126127
return newPassingSSRRAuthClient(), nil
127128
}

‎internal/operator-controller/authorization/authorization.go‎

Lines changed: 10 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,11 @@ import (
44
"context"
55
"errors"
66
"fmt"
7-
"io/fs"
87
"slices"
98
"strings"
109

1110
ocv1 "github.com/operator-framework/operator-controller/api/v1"
12-
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/convert"
1311
authv1 "k8s.io/api/authorization/v1"
14-
corev1 "k8s.io/api/core/v1"
1512
rbacv1 "k8s.io/api/rbac/v1"
1613

1714
v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -20,15 +17,8 @@ import (
2017
"sigs.k8s.io/controller-runtime/pkg/client"
2118
)
2219

23-
const (
24-
SelfSubjectRulesReview string = "SelfSubjectRulesReview"
25-
SelfSubjectAccessReview string = "SelfSubjectAccessReview"
26-
)
27-
2820
type RestConfigMapper func(context.Context, client.Object, *rest.Config) (*rest.Config, error)
2921

30-
type NewForConfigFunc func(*rest.Config) (authorizationv1client.AuthorizationV1Interface, error)
31-
3222
type AuthorizationClientMapper struct {
3323
rcm RestConfigMapper
3424
baseCfg *rest.Config
@@ -47,28 +37,11 @@ func (acm *AuthorizationClientMapper) GetAuthorizationClient(ctx context.Context
4737
if err != nil {
4838
return nil, err
4939
}
50-
5140
return acm.NewForConfig(authcfg)
5241
}
5342

54-
// Check if RBAC allows the installer service account necessary permissions on the objects in the contentFS
55-
func (acm *AuthorizationClientMapper) CheckContentPermissions(ctx context.Context, contentFS fs.FS, authcl authorizationv1client.AuthorizationV1Interface, ext *ocv1.ClusterExtension) error {
56-
reg, err := convert.ParseFS(ctx, contentFS)
57-
if err != nil {
58-
return err
59-
}
60-
61-
plain, err := convert.Convert(reg, ext.Spec.Namespace, []string{corev1.NamespaceAll})
62-
if err != nil {
63-
return err
64-
}
65-
66-
err = checkObjectPermissions(ctx, authcl, plain.Objects, ext)
67-
68-
return err
69-
}
70-
71-
func checkObjectPermissions(ctx context.Context, authcl authorizationv1client.AuthorizationV1Interface, objects []client.Object, ext *ocv1.ClusterExtension) error {
43+
// CheckObjectPermissions verifies that the given objects have the required permissions.
44+
func CheckObjectPermissions(ctx context.Context, authcl authorizationv1client.AuthorizationV1Interface, objects []client.Object, ext *ocv1.ClusterExtension) error {
7245
ssrr := &authv1.SelfSubjectRulesReview{
7346
Spec: authv1.SelfSubjectRulesReviewSpec{
7447
Namespace: ext.Spec.Namespace,
@@ -78,7 +51,7 @@ func checkObjectPermissions(ctx context.Context, authcl authorizationv1client.Au
7851
opts := v1.CreateOptions{}
7952
ssrr, err := authcl.SelfSubjectRulesReviews().Create(ctx, ssrr, opts)
8053
if err != nil {
81-
return err
54+
return fmt.Errorf("failed to create SelfSubjectRulesReview: %w", err)
8255
}
8356

8457
rules := []rbacv1.PolicyRule{}
@@ -118,25 +91,26 @@ func checkObjectPermissions(ctx context.Context, authcl authorizationv1client.Au
11891
for _, resAttr := range resAttrs {
11992
if !canI(resAttr, rules) {
12093
if resAttr.Namespace != "" {
121-
namespacedErrs = append(namespacedErrs, fmt.Errorf("cannot %q %q %q in namespace %q",
94+
namespacedErrs = append(namespacedErrs, fmt.Errorf("cannot %s %q %q in namespace %q",
12295
resAttr.Verb,
12396
strings.TrimSuffix(resAttr.Resource, "s"),
12497
resAttr.Name,
12598
resAttr.Namespace))
12699
continue
127100
}
128-
clusterScopedErrs = append(clusterScopedErrs, fmt.Errorf("cannot %s %s %s",
101+
clusterScopedErrs = append(clusterScopedErrs, fmt.Errorf("cannot %s %q %q",
129102
resAttr.Verb,
130103
strings.TrimSuffix(resAttr.Resource, "s"),
131104
resAttr.Name))
132105
}
133106
}
134-
errs := append(namespacedErrs, clusterScopedErrs...)
135-
if len(errs) > 0 {
136-
errs = append([]error{fmt.Errorf("installer service account %s is missing required permissions", ext.Spec.ServiceAccount.Name)}, errs...)
107+
108+
allErrs := append(namespacedErrs, clusterScopedErrs...)
109+
if len(allErrs) > 0 {
110+
return fmt.Errorf("installer service account %q is missing required permissions: %w", ext.Spec.ServiceAccount.Name, errors.Join(allErrs...))
137111
}
138112

139-
return errors.Join(errs...)
113+
return nil
140114
}
141115

142116
// Checks if the rules allow the verb on the GroupVersionKind in resAttr

‎internal/operator-controller/authorization/client.go‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
package authorization
2+
3+
import (
4+
"context"
5+
6+
authorizationv1client "k8s.io/client-go/kubernetes/typed/authorization/v1"
7+
"k8s.io/client-go/rest"
8+
"sigs.k8s.io/controller-runtime/pkg/client"
9+
10+
ocv1 "github.com/operator-framework/operator-controller/api/v1"
11+
)
12+
13+
type NewForConfigFunc func(*rest.Config) (authorizationv1client.AuthorizationV1Interface, error)
14+
15+
// AuthorizationClient is an interface exposing only the needed functionality
16+
type AuthorizationClient interface {
17+
CheckContentPermissions(ctx context.Context, objects []client.Object, ext *ocv1.ClusterExtension) error
18+
}
19+
20+
// clientImpl wraps the underlying authorization client
21+
type clientImpl struct {
22+
authClient authorizationv1client.AuthorizationV1Interface
23+
}
24+
25+
// NewClient wraps an authorizationv1client.AuthorizationV1Interface
26+
func NewClient(authClient authorizationv1client.AuthorizationV1Interface) AuthorizationClient {
27+
return &clientImpl{authClient: authClient}
28+
}
29+
30+
// CheckContentPermissions is the public method that internally calls the generic check
31+
func (c *clientImpl) CheckContentPermissions(ctx context.Context, objects []client.Object, ext *ocv1.ClusterExtension) error {
32+
return CheckObjectPermissions(ctx, c.authClient, objects, ext)
33+
}

0 commit comments

Comments
 (0)