Skip to content

Commit c01f824

Browse files
committed
permissions preflight: enable implementation behind feature gate
Signed-off-by: Joe Lanford <[email protected]>
1 parent f2adc68 commit c01f824

File tree

5 files changed

+108
-78
lines changed

5 files changed

+108
-78
lines changed

Diff for: cmd/operator-controller/main.go

+9-2
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ import (
3232
"github.com/sirupsen/logrus"
3333
"github.com/spf13/cobra"
3434
corev1 "k8s.io/api/core/v1"
35+
rbacv1 "k8s.io/api/rbac/v1"
3536
apiextensionsv1client "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/typed/apiextensions/v1"
3637
"k8s.io/apimachinery/pkg/fields"
3738
k8slabels "k8s.io/apimachinery/pkg/labels"
@@ -58,6 +59,7 @@ import (
5859
"github.com/operator-framework/operator-controller/internal/operator-controller/action"
5960
"github.com/operator-framework/operator-controller/internal/operator-controller/applier"
6061
"github.com/operator-framework/operator-controller/internal/operator-controller/authentication"
62+
"github.com/operator-framework/operator-controller/internal/operator-controller/authorization"
6163
"github.com/operator-framework/operator-controller/internal/operator-controller/catalogmetadata/cache"
6264
catalogclient "github.com/operator-framework/operator-controller/internal/operator-controller/catalogmetadata/client"
6365
"github.com/operator-framework/operator-controller/internal/operator-controller/contentmanager"
@@ -201,8 +203,12 @@ func run() error {
201203
setupLog.Info("set up manager")
202204
cacheOptions := crcache.Options{
203205
ByObject: map[client.Object]crcache.ByObject{
204-
&ocv1.ClusterExtension{}: {Label: k8slabels.Everything()},
205-
&ocv1.ClusterCatalog{}: {Label: k8slabels.Everything()},
206+
&ocv1.ClusterExtension{}: {Label: k8slabels.Everything()},
207+
&ocv1.ClusterCatalog{}: {Label: k8slabels.Everything()},
208+
&rbacv1.ClusterRole{}: {Label: k8slabels.Everything()},
209+
&rbacv1.ClusterRoleBinding{}: {Label: k8slabels.Everything()},
210+
&rbacv1.Role{}: {Namespaces: map[string]crcache.Config{}, Label: k8slabels.Everything()},
211+
&rbacv1.RoleBinding{}: {Namespaces: map[string]crcache.Config{}, Label: k8slabels.Everything()},
206212
},
207213
DefaultNamespaces: map[string]crcache.Config{
208214
cfg.systemNamespace: {LabelSelector: k8slabels.Everything()},
@@ -409,6 +415,7 @@ func run() error {
409415
helmApplier := &applier.Helm{
410416
ActionClientGetter: acg,
411417
Preflights: preflights,
418+
PreAuthorizer: authorization.NewRBACPreAuthorizer(mgr.GetClient()),
412419
}
413420

414421
cm := contentmanager.NewManager(clientRestConfigMapper, mgr.GetConfig(), mgr.GetRESTMapper())

Diff for: config/base/operator-controller/rbac/role.yaml

+10
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,16 @@ rules:
4747
verbs:
4848
- patch
4949
- update
50+
- apiGroups:
51+
- rbac.authorization.k8s.io
52+
resources:
53+
- clusterrolebindings
54+
- clusterroles
55+
- rolebindings
56+
- roles
57+
verbs:
58+
- list
59+
- watch
5060
---
5161
apiVersion: rbac.authorization.k8s.io/v1
5262
kind: Role

Diff for: internal/operator-controller/applier/helm.go

+88-9
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"fmt"
88
"io"
99
"io/fs"
10+
"slices"
1011
"strings"
1112

1213
"helm.sh/helm/v3/pkg/action"
@@ -16,14 +17,17 @@ import (
1617
"helm.sh/helm/v3/pkg/release"
1718
"helm.sh/helm/v3/pkg/storage/driver"
1819
corev1 "k8s.io/api/core/v1"
20+
rbacv1 "k8s.io/api/rbac/v1"
1921
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
2022
apimachyaml "k8s.io/apimachinery/pkg/util/yaml"
23+
"k8s.io/apiserver/pkg/authentication/user"
2124
"sigs.k8s.io/controller-runtime/pkg/client"
2225
"sigs.k8s.io/controller-runtime/pkg/log"
2326

2427
helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client"
2528

2629
ocv1 "github.com/operator-framework/operator-controller/api/v1"
30+
"github.com/operator-framework/operator-controller/internal/operator-controller/authorization"
2731
"github.com/operator-framework/operator-controller/internal/operator-controller/features"
2832
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/convert"
2933
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/preflights/crdupgradesafety"
@@ -56,6 +60,7 @@ type Preflight interface {
5660
type Helm struct {
5761
ActionClientGetter helmclient.ActionClientGetter
5862
Preflights []Preflight
63+
PreAuthorizer authorization.PreAuthorizer
5964
}
6065

6166
// shouldSkipPreflight is a helper to determine if the preflight check is CRDUpgradeSafety AND
@@ -85,18 +90,46 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte
8590
}
8691
values := chartutil.Values{}
8792

93+
post := &postrenderer{
94+
labels: objectLabels,
95+
}
96+
97+
if features.OperatorControllerFeatureGate.Enabled(features.PreflightPermissions) {
98+
tmplRel, err := h.template(ctx, ext, chrt, values, post)
99+
if err != nil {
100+
return nil, "", fmt.Errorf("failed to get release state using client-only dry-run: %w", err)
101+
}
102+
103+
ceServiceAccount := user.DefaultInfo{Name: fmt.Sprintf("system:serviceaccount:%s:%s", ext.Spec.Namespace, ext.Spec.ServiceAccount.Name)}
104+
missingRules, err := h.PreAuthorizer.PreAuthorize(ctx, &ceServiceAccount, strings.NewReader(tmplRel.Manifest))
105+
106+
var preAuthErrors []error
107+
if len(missingRules) > 0 {
108+
var missingRuleDescriptions []string
109+
for ns, policyRules := range missingRules {
110+
for _, rule := range policyRules {
111+
missingRuleDescriptions = append(missingRuleDescriptions, ruleDescription(ns, rule))
112+
}
113+
}
114+
slices.Sort(missingRuleDescriptions)
115+
preAuthErrors = append(preAuthErrors, fmt.Errorf("service account lacks permission to manage cluster extension:\n %s", strings.Join(missingRuleDescriptions, "\n ")))
116+
}
117+
if err != nil {
118+
preAuthErrors = append(preAuthErrors, fmt.Errorf("authorization evaluation error: %w", err))
119+
}
120+
if len(preAuthErrors) > 0 {
121+
return nil, "", fmt.Errorf("pre-authorization failed: %v", preAuthErrors)
122+
}
123+
}
124+
88125
ac, err := h.ActionClientGetter.ActionClientFor(ctx, ext)
89126
if err != nil {
90127
return nil, "", err
91128
}
92129

93-
post := &postrenderer{
94-
labels: objectLabels,
95-
}
96-
97130
rel, desiredRel, state, err := h.getReleaseState(ac, ext, chrt, values, post)
98131
if err != nil {
99-
return nil, "", err
132+
return nil, "", fmt.Errorf("failed to get release state using server-side dry-run: %w", err)
100133
}
101134

102135
for _, preflight := range h.Preflights {
@@ -152,6 +185,34 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte
152185
return relObjects, state, nil
153186
}
154187

188+
func (h *Helm) template(ctx context.Context, ext *ocv1.ClusterExtension, chrt *chart.Chart, values chartutil.Values, post postrender.PostRenderer) (*release.Release, error) {
189+
// We need to get a separate action client because our template call below
190+
// permanently modifies the underlying action.Configuration for ClientOnly mode.
191+
ac, err := h.ActionClientGetter.ActionClientFor(ctx, ext)
192+
if err != nil {
193+
return nil, err
194+
}
195+
196+
isUpgrade := false
197+
currentRelease, err := ac.Get(ext.GetName())
198+
if err != nil && !errors.Is(err, driver.ErrReleaseNotFound) {
199+
return nil, err
200+
}
201+
if currentRelease != nil {
202+
isUpgrade = true
203+
}
204+
205+
return ac.Install(ext.GetName(), ext.Spec.Namespace, chrt, values, func(i *action.Install) error {
206+
i.DryRun = true
207+
i.ReleaseName = ext.GetName()
208+
i.Replace = true
209+
i.ClientOnly = true
210+
i.IncludeCRDs = true
211+
i.IsUpgrade = isUpgrade
212+
return nil
213+
}, helmclient.AppendInstallPostRenderer(post))
214+
}
215+
155216
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) {
156217
currentRelease, err := cl.Get(ext.GetName())
157218
if errors.Is(err, driver.ErrReleaseNotFound) {
@@ -161,10 +222,6 @@ func (h *Helm) getReleaseState(cl helmclient.ActionInterface, ext *ocv1.ClusterE
161222
return nil
162223
}, helmclient.AppendInstallPostRenderer(post))
163224
if err != nil {
164-
if features.OperatorControllerFeatureGate.Enabled(features.PreflightPermissions) {
165-
_ = struct{}{} // minimal no-op to satisfy linter
166-
// probably need to break out this error as it's the one for helm dry-run as opposed to any returned later
167-
}
168225
return nil, nil, StateError, err
169226
}
170227
return nil, desiredRelease, StateNeedsInstall, nil
@@ -220,3 +277,25 @@ func (p *postrenderer) Run(renderedManifests *bytes.Buffer) (*bytes.Buffer, erro
220277
}
221278
return &buf, nil
222279
}
280+
281+
func ruleDescription(ns string, rule rbacv1.PolicyRule) string {
282+
var sb strings.Builder
283+
sb.WriteString(fmt.Sprintf("Namespace:%q", ns))
284+
285+
if len(rule.APIGroups) > 0 {
286+
sb.WriteString(fmt.Sprintf(" APIGroups:[%s]", strings.Join(rule.APIGroups, ",")))
287+
}
288+
if len(rule.Resources) > 0 {
289+
sb.WriteString(fmt.Sprintf(" Resources:[%s]", strings.Join(rule.Resources, ",")))
290+
}
291+
if len(rule.ResourceNames) > 0 {
292+
sb.WriteString(fmt.Sprintf(" ResourceNames:[%s]", strings.Join(rule.ResourceNames, ",")))
293+
}
294+
if len(rule.Verbs) > 0 {
295+
sb.WriteString(fmt.Sprintf(" Verbs:[%s]", strings.Join(rule.Verbs, ",")))
296+
}
297+
if len(rule.NonResourceURLs) > 0 {
298+
sb.WriteString(fmt.Sprintf(" NonResourceURLs:[%s]", strings.Join(rule.NonResourceURLs, ",")))
299+
}
300+
return sb.String()
301+
}

Diff for: internal/operator-controller/applier/helm_test.go

-67
Original file line numberDiff line numberDiff line change
@@ -13,14 +13,12 @@ import (
1313
"helm.sh/helm/v3/pkg/chart"
1414
"helm.sh/helm/v3/pkg/release"
1515
"helm.sh/helm/v3/pkg/storage/driver"
16-
featuregatetesting "k8s.io/component-base/featuregate/testing"
1716
"sigs.k8s.io/controller-runtime/pkg/client"
1817

1918
helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client"
2019

2120
ocv1 "github.com/operator-framework/operator-controller/api/v1"
2221
"github.com/operator-framework/operator-controller/internal/operator-controller/applier"
23-
"github.com/operator-framework/operator-controller/internal/operator-controller/features"
2422
)
2523

2624
type mockPreflight struct {
@@ -228,71 +226,6 @@ func TestApply_Installation(t *testing.T) {
228226
})
229227
}
230228

231-
func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) {
232-
featuregatetesting.SetFeatureGateDuringTest(t, features.OperatorControllerFeatureGate, features.PreflightPermissions, true)
233-
234-
t.Run("fails during dry-run installation", func(t *testing.T) {
235-
mockAcg := &mockActionGetter{
236-
getClientErr: driver.ErrReleaseNotFound,
237-
dryRunInstallErr: errors.New("failed attempting to dry-run install chart"),
238-
}
239-
helmApplier := applier.Helm{ActionClientGetter: mockAcg}
240-
241-
objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
242-
require.Error(t, err)
243-
require.ErrorContains(t, err, "attempting to dry-run install chart")
244-
require.Nil(t, objs)
245-
require.Empty(t, state)
246-
})
247-
248-
t.Run("fails during pre-flight installation", func(t *testing.T) {
249-
mockAcg := &mockActionGetter{
250-
getClientErr: driver.ErrReleaseNotFound,
251-
installErr: errors.New("failed installing chart"),
252-
}
253-
mockPf := &mockPreflight{installErr: errors.New("failed during install pre-flight check")}
254-
helmApplier := applier.Helm{ActionClientGetter: mockAcg, Preflights: []applier.Preflight{mockPf}}
255-
256-
objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
257-
require.Error(t, err)
258-
require.ErrorContains(t, err, "install pre-flight check")
259-
require.Equal(t, applier.StateNeedsInstall, state)
260-
require.Nil(t, objs)
261-
})
262-
263-
t.Run("fails during installation", func(t *testing.T) {
264-
mockAcg := &mockActionGetter{
265-
getClientErr: driver.ErrReleaseNotFound,
266-
installErr: errors.New("failed installing chart"),
267-
}
268-
helmApplier := applier.Helm{ActionClientGetter: mockAcg}
269-
270-
objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
271-
require.Error(t, err)
272-
require.ErrorContains(t, err, "installing chart")
273-
require.Equal(t, applier.StateNeedsInstall, state)
274-
require.Nil(t, objs)
275-
})
276-
277-
t.Run("successful installation", func(t *testing.T) {
278-
mockAcg := &mockActionGetter{
279-
getClientErr: driver.ErrReleaseNotFound,
280-
desiredRel: &release.Release{
281-
Info: &release.Info{Status: release.StatusDeployed},
282-
Manifest: validManifest,
283-
},
284-
}
285-
helmApplier := applier.Helm{ActionClientGetter: mockAcg}
286-
287-
objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
288-
require.NoError(t, err)
289-
require.Equal(t, applier.StateNeedsInstall, state)
290-
require.NotNil(t, objs)
291-
assert.Equal(t, "service-a", objs[0].GetName())
292-
assert.Equal(t, "service-b", objs[1].GetName())
293-
})
294-
}
295-
296229
func TestApply_Upgrade(t *testing.T) {
297230
testCurrentRelease := &release.Release{
298231
Info: &release.Info{Status: release.StatusDeployed},

Diff for: internal/operator-controller/controllers/clusterextension_controller.go

+1
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,7 @@ type InstalledBundleGetter interface {
9696
//+kubebuilder:rbac:namespace=system,groups=core,resources=secrets,verbs=create;update;patch;delete;deletecollection;get;list;watch
9797
//+kubebuilder:rbac:groups=core,resources=serviceaccounts/token,verbs=create
9898
//+kubebuilder:rbac:groups=apiextensions.k8s.io,resources=customresourcedefinitions,verbs=get
99+
//+kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterroles;clusterrolebindings;roles;rolebindings,verbs=list;watch
99100

100101
//+kubebuilder:rbac:groups=olm.operatorframework.io,resources=clustercatalogs,verbs=list;watch
101102

0 commit comments

Comments
 (0)