Skip to content

Commit 24928e4

Browse files
trgeigertrgeiger
committed
RBAC preflight: handle nil errs, add explicit RBAC examples to tests
Signed-off-by: Tayler Geiger <[email protected]>
1 parent e529653 commit 24928e4

File tree

3 files changed

+201
-11
lines changed

3 files changed

+201
-11
lines changed

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ func (h *Helm) runPreAuthorizationChecks(ctx context.Context, ext *ocv1.ClusterE
109109
preAuthErrors = append(preAuthErrors, fmt.Errorf("authorization evaluation error: %w", authErr))
110110
}
111111
if len(preAuthErrors) > 0 {
112-
return fmt.Errorf("pre-authorization failed: %v", preAuthErrors)
112+
return fmt.Errorf("pre-authorization failed: %v", errors.Join(preAuthErrors...))
113113
}
114114
return nil
115115
}

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

+2-2
Original file line numberDiff line numberDiff line change
@@ -141,8 +141,8 @@ func (a *rbacPreAuthorizer) PreAuthorize(ctx context.Context, ext *ocv1.ClusterE
141141
if parseErr := errors.Join(parseErrors...); parseErr != nil {
142142
errs = append(errs, fmt.Errorf("failed to parse escalation check error strings: %v", parseErr))
143143
}
144-
if len(preAuthEvaluationErrors) > 0 {
145-
errs = append(errs, fmt.Errorf("failed to resolve or evaluate permissions: %v", errors.Join(preAuthEvaluationErrors...)))
144+
if preAuthEvaluationErrors := errors.Join(preAuthEvaluationErrors...); preAuthEvaluationErrors != nil {
145+
errs = append(errs, fmt.Errorf("failed to resolve or evaluate permissions: %v", preAuthEvaluationErrors))
146146
}
147147
if len(errs) > 0 {
148148
return allMissingPolicyRules, fmt.Errorf("missing rules may be incomplete: %w", errors.Join(errs...))

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

+198-8
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"github.com/stretchr/testify/require"
1111
corev1 "k8s.io/api/core/v1"
1212
rbacv1 "k8s.io/api/rbac/v1"
13+
v1 "k8s.io/api/rbac/v1"
1314
"k8s.io/apimachinery/pkg/api/meta/testrestmapper"
1415
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1516
"k8s.io/apimachinery/pkg/runtime"
@@ -214,6 +215,195 @@ subjects:
214215
},
215216
},
216217
}
218+
219+
expectedSingleNamespaceMissingRules = []ScopedPolicyRules{
220+
{
221+
Namespace: "",
222+
MissingRules: []v1.PolicyRule{
223+
{
224+
Verbs: []string{"list", "watch"},
225+
APIGroups: []string{""},
226+
Resources: []string{"services"},
227+
ResourceNames: []string(nil),
228+
NonResourceURLs: []string(nil)},
229+
{
230+
Verbs: []string{"list", "watch"},
231+
APIGroups: []string{"rbac.authorization.k8s.io"},
232+
Resources: []string{"rolebindings"},
233+
ResourceNames: []string(nil),
234+
NonResourceURLs: []string(nil)},
235+
{
236+
Verbs: []string{"list", "watch"},
237+
APIGroups: []string{"rbac.authorization.k8s.io"},
238+
Resources: []string{"roles"},
239+
ResourceNames: []string(nil),
240+
NonResourceURLs: []string(nil)},
241+
{
242+
Verbs: []string{"update"},
243+
APIGroups: []string{""},
244+
Resources: []string{"clusterextensions/finalizers"},
245+
ResourceNames: []string{"test-cluster-extension"},
246+
NonResourceURLs: []string(nil),
247+
},
248+
},
249+
},
250+
{
251+
Namespace: "test-namespace",
252+
MissingRules: []v1.PolicyRule{
253+
{
254+
Verbs: []string{"create"},
255+
APIGroups: []string{"*"},
256+
Resources: []string{"certificates"}},
257+
{
258+
Verbs: []string{"create"},
259+
APIGroups: []string{""},
260+
Resources: []string{"services"}},
261+
{
262+
Verbs: []string{"create"},
263+
APIGroups: []string{"rbac.authorization.k8s.io"},
264+
Resources: []string{"rolebindings"}},
265+
{
266+
Verbs: []string{"create"},
267+
APIGroups: []string{"rbac.authorization.k8s.io"},
268+
Resources: []string{"roles"}},
269+
{
270+
Verbs: []string{"delete", "get", "patch", "update"},
271+
APIGroups: []string{""},
272+
Resources: []string{"services"},
273+
ResourceNames: []string{"test-service"}},
274+
{
275+
Verbs: []string{"delete", "get", "patch", "update"},
276+
APIGroups: []string{"rbac.authorization.k8s.io"},
277+
Resources: []string{"rolebindings"},
278+
ResourceNames: []string{"test-extension-binding"}},
279+
{
280+
Verbs: []string{"delete", "get", "patch", "update"},
281+
APIGroups: []string{"rbac.authorization.k8s.io"},
282+
Resources: []string{"roles"},
283+
ResourceNames: []string{"test-extension-role"}},
284+
{
285+
Verbs: []string{"watch"},
286+
APIGroups: []string{"*"},
287+
Resources: []string{"serviceaccounts"},
288+
},
289+
},
290+
},
291+
}
292+
293+
expectedMultiNamespaceMissingRules = []ScopedPolicyRules{
294+
{
295+
Namespace: "",
296+
MissingRules: []v1.PolicyRule{
297+
{
298+
Verbs: []string{"list", "watch"},
299+
APIGroups: []string{""},
300+
Resources: []string{"services"},
301+
ResourceNames: []string(nil),
302+
NonResourceURLs: []string(nil)},
303+
{
304+
Verbs: []string{"list", "watch"},
305+
APIGroups: []string{"rbac.authorization.k8s.io"},
306+
Resources: []string{"rolebindings"},
307+
ResourceNames: []string(nil),
308+
NonResourceURLs: []string(nil)},
309+
{
310+
Verbs: []string{"list", "watch"},
311+
APIGroups: []string{"rbac.authorization.k8s.io"},
312+
Resources: []string{"roles"},
313+
ResourceNames: []string(nil),
314+
NonResourceURLs: []string(nil)},
315+
{
316+
Verbs: []string{"update"},
317+
APIGroups: []string{""},
318+
Resources: []string{"clusterextensions/finalizers"},
319+
ResourceNames: []string{"test-cluster-extension"},
320+
NonResourceURLs: []string(nil),
321+
},
322+
},
323+
},
324+
{
325+
Namespace: "a-test-namespace",
326+
MissingRules: []v1.PolicyRule{
327+
{
328+
Verbs: []string{"create"},
329+
APIGroups: []string{"*"},
330+
Resources: []string{"certificates"}},
331+
{
332+
Verbs: []string{"create"},
333+
APIGroups: []string{""},
334+
Resources: []string{"services"}},
335+
{
336+
Verbs: []string{"create"},
337+
APIGroups: []string{"rbac.authorization.k8s.io"},
338+
Resources: []string{"rolebindings"}},
339+
{
340+
Verbs: []string{"create"},
341+
APIGroups: []string{"rbac.authorization.k8s.io"},
342+
Resources: []string{"roles"}},
343+
{
344+
Verbs: []string{"delete", "get", "patch", "update"},
345+
APIGroups: []string{""},
346+
Resources: []string{"services"},
347+
ResourceNames: []string{"test-service"}},
348+
{
349+
Verbs: []string{"delete", "get", "patch", "update"},
350+
APIGroups: []string{"rbac.authorization.k8s.io"},
351+
Resources: []string{"rolebindings"},
352+
ResourceNames: []string{"test-extension-binding"}},
353+
{
354+
Verbs: []string{"delete", "get", "patch", "update"},
355+
APIGroups: []string{"rbac.authorization.k8s.io"},
356+
Resources: []string{"roles"},
357+
ResourceNames: []string{"test-extension-role"}},
358+
{
359+
Verbs: []string{"watch"},
360+
APIGroups: []string{"*"},
361+
Resources: []string{"serviceaccounts"},
362+
},
363+
},
364+
},
365+
{
366+
Namespace: "test-namespace",
367+
MissingRules: []v1.PolicyRule{
368+
{
369+
Verbs: []string{"create"},
370+
APIGroups: []string{"*"},
371+
Resources: []string{"certificates"}},
372+
{
373+
Verbs: []string{"create"},
374+
APIGroups: []string{""},
375+
Resources: []string{"services"}},
376+
{
377+
Verbs: []string{"create"},
378+
APIGroups: []string{"rbac.authorization.k8s.io"},
379+
Resources: []string{"rolebindings"}},
380+
{
381+
Verbs: []string{"create"},
382+
APIGroups: []string{"rbac.authorization.k8s.io"},
383+
Resources: []string{"roles"}},
384+
{
385+
Verbs: []string{"delete", "get", "patch", "update"},
386+
APIGroups: []string{""},
387+
Resources: []string{"services"},
388+
ResourceNames: []string{"test-service"}},
389+
{
390+
Verbs: []string{"delete", "get", "patch", "update"},
391+
APIGroups: []string{"rbac.authorization.k8s.io"},
392+
Resources: []string{"rolebindings"},
393+
ResourceNames: []string{"test-extension-binding"}},
394+
{
395+
Verbs: []string{"delete", "get", "patch", "update"},
396+
APIGroups: []string{"rbac.authorization.k8s.io"},
397+
Resources: []string{"roles"},
398+
ResourceNames: []string{"test-extension-role"}},
399+
{
400+
Verbs: []string{"watch"},
401+
APIGroups: []string{"*"},
402+
Resources: []string{"serviceaccounts"},
403+
},
404+
},
405+
},
406+
}
217407
)
218408

219409
func setupFakeClient(role client.Object) client.Client {
@@ -236,23 +426,23 @@ func TestPreAuthorize_Success(t *testing.T) {
236426
})
237427
}
238428

239-
func TestPreAuthorize_Failure(t *testing.T) {
240-
t.Run("preauthorize fails with missing rbac rules", func(t *testing.T) {
429+
func TestPreAuthorize_MissingRBAC(t *testing.T) {
430+
t.Run("preauthorize fails and finds missing rbac rules", func(t *testing.T) {
241431
fakeClient := setupFakeClient(limitedClusterRole)
242432
preAuth := NewRBACPreAuthorizer(fakeClient)
243433
missingRules, err := preAuth.PreAuthorize(context.TODO(), &exampleClusterExtension, strings.NewReader(testManifest))
244-
require.Error(t, err)
245-
require.NotEqual(t, []ScopedPolicyRules{}, missingRules)
434+
require.NoError(t, err)
435+
require.Equal(t, expectedSingleNamespaceMissingRules, missingRules)
246436
})
247437
}
248438

249-
func TestPreAuthorizeMultiNamespace_Failure(t *testing.T) {
250-
t.Run("preauthorize fails with missing rbac rules in multiple namespaces", func(t *testing.T) {
439+
func TestPreAuthorizeMultiNamespace_MissingRBAC(t *testing.T) {
440+
t.Run("preauthorize fails and finds missing rbac rules in multiple namespaces", func(t *testing.T) {
251441
fakeClient := setupFakeClient(limitedClusterRole)
252442
preAuth := NewRBACPreAuthorizer(fakeClient)
253443
missingRules, err := preAuth.PreAuthorize(context.TODO(), &exampleClusterExtension, strings.NewReader(testManifestMultiNamespace))
254-
require.Error(t, err)
255-
require.NotEqual(t, []ScopedPolicyRules{}, missingRules)
444+
require.NoError(t, err)
445+
require.Equal(t, expectedMultiNamespaceMissingRules, missingRules)
256446
})
257447
}
258448

0 commit comments

Comments
 (0)