Skip to content

Commit 5d08fa1

Browse files
gman0gman0
committed
code cleaning
1 parent f6d0978 commit 5d08fa1

3 files changed

Lines changed: 100 additions & 84 deletions

File tree

‎backend/controllers/serviceexportrbac/rbac_controller.go‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -82,9 +82,8 @@ func NewAPIServiceExportRBACReconciler(
8282
return &ns, nil
8383
},
8484
// ClusterRole.
85-
getClusterRole: func(ctx context.Context, cache cache.Cache, name string) (*rbacv1.ClusterRole, error) {
85+
getClusterRole: func(ctx context.Context, cache cache.Cache, key types.NamespacedName) (*rbacv1.ClusterRole, error) {
8686
var role rbacv1.ClusterRole
87-
key := types.NamespacedName{Name: name}
8887
if err := cache.Get(ctx, key, &role); err != nil {
8988
return nil, err
9089
}
@@ -97,9 +96,8 @@ func NewAPIServiceExportRBACReconciler(
9796
return client.Update(ctx, binding)
9897
},
9998
// ClusterRoleBinding.
100-
getClusterRoleBinding: func(ctx context.Context, cache cache.Cache, name string) (*rbacv1.ClusterRoleBinding, error) {
99+
getClusterRoleBinding: func(ctx context.Context, cache cache.Cache, key types.NamespacedName) (*rbacv1.ClusterRoleBinding, error) {
101100
var binding rbacv1.ClusterRoleBinding
102-
key := types.NamespacedName{Name: name}
103101
if err := cache.Get(ctx, key, &binding); err != nil {
104102
return nil, err
105103
}
@@ -112,9 +110,8 @@ func NewAPIServiceExportRBACReconciler(
112110
return client.Update(ctx, binding)
113111
},
114112
// Role.
115-
getRole: func(ctx context.Context, cache cache.Cache, namespace, name string) (*rbacv1.Role, error) {
113+
getRole: func(ctx context.Context, cache cache.Cache, key types.NamespacedName) (*rbacv1.Role, error) {
116114
var role rbacv1.Role
117-
key := types.NamespacedName{Namespace: namespace, Name: name}
118115
if err := cache.Get(ctx, key, &role); err != nil {
119116
return nil, err
120117
}
@@ -127,9 +124,8 @@ func NewAPIServiceExportRBACReconciler(
127124
return client.Update(ctx, role)
128125
},
129126
// RoleBinding.
130-
getRoleBinding: func(ctx context.Context, cache cache.Cache, ns, name string) (*rbacv1.RoleBinding, error) {
127+
getRoleBinding: func(ctx context.Context, cache cache.Cache, key types.NamespacedName) (*rbacv1.RoleBinding, error) {
131128
var binding rbacv1.RoleBinding
132-
key := types.NamespacedName{Namespace: ns, Name: name}
133129
if err := cache.Get(ctx, key, &binding); err != nil {
134130
return nil, err
135131
}

‎backend/controllers/serviceexportrbac/rbac_reconciler.go‎

Lines changed: 84 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ import (
2828
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
2929
apierrors "k8s.io/apimachinery/pkg/api/errors"
3030
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
31+
"k8s.io/apimachinery/pkg/types"
3132
utilerrors "k8s.io/apimachinery/pkg/util/errors"
3233
"sigs.k8s.io/controller-runtime/pkg/cache"
3334
"sigs.k8s.io/controller-runtime/pkg/client"
@@ -43,19 +44,19 @@ type reconciler struct {
4344
listServiceNamespaces func(ctx context.Context, cache cache.Cache, namespace string) ([]*kubebindv1alpha2.APIServiceNamespace, error)
4445
getNamespace func(ctx context.Context, cache cache.Cache, name string) (*corev1.Namespace, error)
4546

46-
getClusterRole func(ctx context.Context, cache cache.Cache, name string) (*rbacv1.ClusterRole, error)
47+
getClusterRole func(ctx context.Context, cache cache.Cache, key types.NamespacedName) (*rbacv1.ClusterRole, error)
4748
createClusterRole func(ctx context.Context, client client.Client, clusterRole *rbacv1.ClusterRole) error
4849
updateClusterRole func(ctx context.Context, client client.Client, clusterRole *rbacv1.ClusterRole) error
4950

50-
getClusterRoleBinding func(ctx context.Context, cache cache.Cache, name string) (*rbacv1.ClusterRoleBinding, error)
51+
getClusterRoleBinding func(ctx context.Context, cache cache.Cache, key types.NamespacedName) (*rbacv1.ClusterRoleBinding, error)
5152
createClusterRoleBinding func(ctx context.Context, client client.Client, clusterRoleBinding *rbacv1.ClusterRoleBinding) error
5253
updateClusterRoleBinding func(ctx context.Context, client client.Client, clusterRoleBinding *rbacv1.ClusterRoleBinding) error
5354

54-
getRole func(ctx context.Context, cache cache.Cache, namespace, name string) (*rbacv1.Role, error)
55+
getRole func(ctx context.Context, cache cache.Cache, key types.NamespacedName) (*rbacv1.Role, error)
5556
createRole func(ctx context.Context, client client.Client, role *rbacv1.Role) error
5657
updateRole func(ctx context.Context, client client.Client, role *rbacv1.Role) error
5758

58-
getRoleBinding func(ctx context.Context, cache cache.Cache, namespace, name string) (*rbacv1.RoleBinding, error)
59+
getRoleBinding func(ctx context.Context, cache cache.Cache, key types.NamespacedName) (*rbacv1.RoleBinding, error)
5960
createRoleBinding func(ctx context.Context, client client.Client, roleBinding *rbacv1.RoleBinding) error
6061
updateRoleBinding func(ctx context.Context, client client.Client, roleBinding *rbacv1.RoleBinding) error
6162
}
@@ -236,22 +237,21 @@ func (r *reconciler) ensureRoleAndRoleBinding(ctx context.Context, client client
236237
Rules: rules,
237238
}
238239

239-
role, err := r.getRole(ctx, cache, namespace, name)
240-
if err != nil && !apierrors.IsNotFound(err) {
240+
role, err := getOrCreate(ctx,
241+
cache,
242+
client,
243+
&expectedRole,
244+
r.getRole,
245+
r.createRole,
246+
)
247+
if err != nil {
241248
return err
242249
}
243-
if role == nil {
244-
err = r.createRole(ctx, client, &expectedRole)
245-
if err != nil && apierrors.IsAlreadyExists(err) {
246-
return err
247-
}
248-
role = &expectedRole
249-
}
250+
250251
if !reflect.DeepEqual(expectedRole.Rules, role.Rules) {
251252
copyRole := role.DeepCopy()
252253
copyRole.Rules = expectedRole.Rules
253-
err = r.updateRole(ctx, client, copyRole)
254-
if err != nil {
254+
if err := r.updateRole(ctx, client, copyRole); err != nil {
255255
return err
256256
}
257257
}
@@ -271,20 +271,17 @@ func (r *reconciler) ensureRoleAndRoleBinding(ctx context.Context, client client
271271
Subjects: subjects,
272272
}
273273

274-
roleBinding, err := r.getRoleBinding(ctx, cache, namespace, name)
275-
if err != nil && !apierrors.IsNotFound(err) {
274+
roleBinding, err := getOrCreate(
275+
ctx,
276+
cache,
277+
client,
278+
&expectedRoleBinding,
279+
r.getRoleBinding,
280+
r.createRoleBinding,
281+
)
282+
if err != nil {
276283
return err
277284
}
278-
if roleBinding == nil {
279-
err = r.createRoleBinding(ctx, client, &expectedRoleBinding)
280-
if err == nil {
281-
return nil
282-
}
283-
if !apierrors.IsAlreadyExists(err) {
284-
return err
285-
}
286-
roleBinding = &expectedRoleBinding
287-
}
288285

289286
if !reflect.DeepEqual(expectedRoleBinding.Subjects, roleBinding.Subjects) {
290287
// We don't check .RoleRef because it's immutable.
@@ -308,20 +305,16 @@ func (r *reconciler) ensureAggregatedClusterRole(ctx context.Context, client cli
308305
Rules: rules,
309306
}
310307

311-
clusterRole, err := r.getClusterRole(ctx, cache, name)
312-
if err != nil && !apierrors.IsNotFound(err) {
308+
clusterRole, err := getOrCreate(ctx,
309+
cache,
310+
client,
311+
&expectedClusterRole,
312+
r.getClusterRole,
313+
r.createClusterRole,
314+
)
315+
if err != nil {
313316
return err
314317
}
315-
if clusterRole == nil {
316-
err = r.createClusterRole(ctx, client, &expectedClusterRole)
317-
if err == nil {
318-
return nil
319-
}
320-
if !apierrors.IsAlreadyExists(err) {
321-
return err
322-
}
323-
clusterRole = &expectedClusterRole
324-
}
325318

326319
if clusterRole.Labels == nil || clusterRole.Labels[rbacAggregateLabelKey] != "true" ||
327320
!reflect.DeepEqual(expectedClusterRole.OwnerReferences, clusterRole.OwnerReferences) ||
@@ -360,23 +353,21 @@ func (r *reconciler) ensureAggregatingClusteRoleAndClusterRoleBinding(ctx contex
360353
},
361354
}
362355

363-
clusterRole, err := r.getClusterRole(ctx, cache, name)
364-
if err != nil && !apierrors.IsNotFound(err) {
356+
clusterRole, err := getOrCreate(ctx,
357+
cache,
358+
client,
359+
&expectedClusterRole,
360+
r.getClusterRole,
361+
r.createClusterRole,
362+
)
363+
if err != nil {
365364
return err
366365
}
367-
if clusterRole == nil {
368-
err = r.createClusterRole(ctx, client, &expectedClusterRole)
369-
if err != nil && apierrors.IsAlreadyExists(err) {
370-
return err
371-
}
372-
clusterRole = &expectedClusterRole
373-
}
374366

375367
if !reflect.DeepEqual(expectedClusterRole.AggregationRule, clusterRole.AggregationRule) {
376368
copyClusterRole := clusterRole.DeepCopy()
377369
copyClusterRole.AggregationRule = expectedClusterRole.AggregationRule
378-
err = r.updateClusterRole(ctx, client, copyClusterRole)
379-
if err != nil {
370+
if err := r.updateClusterRole(ctx, client, copyClusterRole); err != nil {
380371
return err
381372
}
382373
}
@@ -395,20 +386,16 @@ func (r *reconciler) ensureAggregatingClusteRoleAndClusterRoleBinding(ctx contex
395386
Subjects: subjects,
396387
}
397388

398-
clusterRoleBinding, err := r.getClusterRoleBinding(ctx, cache, name)
399-
if err != nil && !apierrors.IsNotFound(err) {
389+
clusterRoleBinding, err := getOrCreate(ctx,
390+
cache,
391+
client,
392+
&expectedClusterRoleBinding,
393+
r.getClusterRoleBinding,
394+
r.createClusterRoleBinding,
395+
)
396+
if err != nil {
400397
return err
401398
}
402-
if clusterRoleBinding == nil {
403-
err = r.createClusterRoleBinding(ctx, client, &expectedClusterRoleBinding)
404-
if err == nil {
405-
return nil
406-
}
407-
if !apierrors.IsAlreadyExists(err) {
408-
return err
409-
}
410-
clusterRoleBinding = &expectedClusterRoleBinding
411-
}
412399

413400
if !reflect.DeepEqual(expectedClusterRoleBinding.Subjects, clusterRoleBinding.Subjects) {
414401
// We don't check .RoleRef because it's immutable.
@@ -419,3 +406,38 @@ func (r *reconciler) ensureAggregatingClusteRoleAndClusterRoleBinding(ctx contex
419406

420407
return nil
421408
}
409+
410+
func getOrCreate[R client.Object](
411+
ctx context.Context,
412+
cache cache.Cache,
413+
client client.Client,
414+
expectedObj R,
415+
416+
get func(
417+
ctx context.Context,
418+
cache cache.Cache,
419+
key types.NamespacedName,
420+
) (R, error),
421+
422+
create func(
423+
ctx context.Context,
424+
client client.Client,
425+
obj R,
426+
) error,
427+
428+
) (R, error) {
429+
var empty R
430+
obj, err := get(ctx, cache, types.NamespacedName{Name: expectedObj.GetName(), Namespace: expectedObj.GetNamespace()})
431+
432+
if err != nil {
433+
if !apierrors.IsNotFound(err) {
434+
return empty, err
435+
}
436+
if err = create(ctx, client, expectedObj); err != nil && !apierrors.IsAlreadyExists(err) {
437+
return empty, err
438+
}
439+
return expectedObj, nil
440+
}
441+
442+
return obj, nil
443+
}

‎backend/controllers/serviceexportrbac/rbac_reconciler_test.go‎

Lines changed: 12 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -65,10 +65,10 @@ func Test_reconciler_reconcile(t *testing.T) {
6565
listServiceNamespaces: func(ctx context.Context, _ cache.Cache, namespace string) ([]*kubebindv1alpha2.APIServiceNamespace, error) {
6666
return slices.Collect(maps.Values(apiServiceNamespaces)), nil
6767
},
68-
getClusterRole: func(ctx context.Context, _ cache.Cache, name string) (*rbacv1.ClusterRole, error) {
69-
clusterRole, ok := st.clusterRoles[name]
68+
getClusterRole: func(ctx context.Context, _ cache.Cache, key types.NamespacedName) (*rbacv1.ClusterRole, error) {
69+
clusterRole, ok := st.clusterRoles[key.Name]
7070
if !ok {
71-
return nil, apierrors.NewNotFound(rbacv1.Resource("clusterroles"), name)
71+
return nil, apierrors.NewNotFound(rbacv1.Resource("clusterroles"), key.Name)
7272
}
7373
return clusterRole, nil
7474
},
@@ -88,11 +88,10 @@ func Test_reconciler_reconcile(t *testing.T) {
8888
st.clusterRoles[binding.Name] = binding
8989
return nil
9090
},
91-
getRole: func(ctx context.Context, _ cache.Cache, namespace, name string) (*rbacv1.Role, error) {
92-
key := types.NamespacedName{Namespace: namespace, Name: name}.String()
93-
role, ok := st.roles[key]
91+
getRole: func(ctx context.Context, _ cache.Cache, key types.NamespacedName) (*rbacv1.Role, error) {
92+
role, ok := st.roles[key.String()]
9493
if !ok {
95-
return nil, apierrors.NewNotFound(rbacv1.Resource("roles"), key)
94+
return nil, apierrors.NewNotFound(rbacv1.Resource("roles"), key.String())
9695
}
9796
return role, nil
9897
},
@@ -114,10 +113,10 @@ func Test_reconciler_reconcile(t *testing.T) {
114113
st.roles[key] = role
115114
return nil
116115
},
117-
getClusterRoleBinding: func(ctx context.Context, _ cache.Cache, name string) (*rbacv1.ClusterRoleBinding, error) {
118-
clusterRoleBinding, ok := st.clusterRoleBindings[name]
116+
getClusterRoleBinding: func(ctx context.Context, _ cache.Cache, key types.NamespacedName) (*rbacv1.ClusterRoleBinding, error) {
117+
clusterRoleBinding, ok := st.clusterRoleBindings[key.Name]
119118
if !ok {
120-
return nil, apierrors.NewNotFound(rbacv1.Resource("clusterrolebindings"), name)
119+
return nil, apierrors.NewNotFound(rbacv1.Resource("clusterrolebindings"), key.Name)
121120
}
122121
return clusterRoleBinding, nil
123122
},
@@ -137,11 +136,10 @@ func Test_reconciler_reconcile(t *testing.T) {
137136
st.clusterRoleBindings[binding.Name] = binding
138137
return nil
139138
},
140-
getRoleBinding: func(ctx context.Context, _ cache.Cache, ns, name string) (*rbacv1.RoleBinding, error) {
141-
key := types.NamespacedName{Namespace: ns, Name: name}.String()
142-
roleBinding, ok := st.roleBindings[key]
139+
getRoleBinding: func(ctx context.Context, _ cache.Cache, key types.NamespacedName) (*rbacv1.RoleBinding, error) {
140+
roleBinding, ok := st.roleBindings[key.String()]
143141
if !ok {
144-
return nil, apierrors.NewNotFound(rbacv1.Resource("rolebindings"), key)
142+
return nil, apierrors.NewNotFound(rbacv1.Resource("rolebindings"), key.String())
145143
}
146144
return roleBinding, nil
147145
},

0 commit comments

Comments
 (0)