Skip to content

Commit d3edc3e

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 d3edc3e

File tree

3 files changed

+200
-12
lines changed

3 files changed

+200
-12
lines changed

Diff for: ‎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
}

Diff for: ‎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...))

Diff for: ‎internal/operator-controller/authorization/rbac_test.go

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

219408
func setupFakeClient(role client.Object) client.Client {
220409
s := runtime.NewScheme()
221410
_ = corev1.AddToScheme(s)
222411
_ = rbacv1.AddToScheme(s)
223412
restMapper := testrestmapper.TestOnlyStaticRESTMapper(s)
224-
// restMapper := meta.NewDefaultRESTMapper(nil)
225413
fakeClientBuilder := fake.NewClientBuilder().WithObjects(append(objects, role)...).WithRESTMapper(restMapper)
226414
return fakeClientBuilder.Build()
227415
}
@@ -236,23 +424,23 @@ func TestPreAuthorize_Success(t *testing.T) {
236424
})
237425
}
238426

239-
func TestPreAuthorize_Failure(t *testing.T) {
240-
t.Run("preauthorize fails with missing rbac rules", func(t *testing.T) {
427+
func TestPreAuthorize_MissingRBAC(t *testing.T) {
428+
t.Run("preauthorize fails and finds missing rbac rules", func(t *testing.T) {
241429
fakeClient := setupFakeClient(limitedClusterRole)
242430
preAuth := NewRBACPreAuthorizer(fakeClient)
243431
missingRules, err := preAuth.PreAuthorize(context.TODO(), &exampleClusterExtension, strings.NewReader(testManifest))
244-
require.Error(t, err)
245-
require.NotEqual(t, []ScopedPolicyRules{}, missingRules)
432+
require.NoError(t, err)
433+
require.Equal(t, expectedSingleNamespaceMissingRules, missingRules)
246434
})
247435
}
248436

249-
func TestPreAuthorizeMultiNamespace_Failure(t *testing.T) {
250-
t.Run("preauthorize fails with missing rbac rules in multiple namespaces", func(t *testing.T) {
437+
func TestPreAuthorizeMultiNamespace_MissingRBAC(t *testing.T) {
438+
t.Run("preauthorize fails and finds missing rbac rules in multiple namespaces", func(t *testing.T) {
251439
fakeClient := setupFakeClient(limitedClusterRole)
252440
preAuth := NewRBACPreAuthorizer(fakeClient)
253441
missingRules, err := preAuth.PreAuthorize(context.TODO(), &exampleClusterExtension, strings.NewReader(testManifestMultiNamespace))
254-
require.Error(t, err)
255-
require.NotEqual(t, []ScopedPolicyRules{}, missingRules)
442+
require.NoError(t, err)
443+
require.Equal(t, expectedMultiNamespaceMissingRules, missingRules)
256444
})
257445
}
258446

0 commit comments

Comments
 (0)