Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions api/v1alpha1/clusterpermission_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@ type ClusterPermissionSpec struct {
// +kubebuilder:validation:XValidation:rule="has(self.subject) || has(self.subjects)",message="Either subject or subjects has to exist in clusterRoleBinding"
ClusterRoleBinding *ClusterRoleBinding `json:"clusterRoleBinding,omitempty"`

// ClusterRoleBindings represents multiple ClusterRoleBindings that are being created on the managed cluster
// +optional
// +kubebuilder:validation:XValidation:rule="self.all(i, has(i.subject) || has(i.subjects))",message="Either subject or subjects has to exist in every clusterRoleBinding"
ClusterRoleBindings *[]ClusterRoleBinding `json:"clusterRoleBindings,omitempty"`

// Roles represents roles that are being created on the managed cluster
// +optional
Roles *[]Role `json:"roles,omitempty"`
Expand Down
11 changes: 11 additions & 0 deletions api/v1alpha1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

101 changes: 101 additions & 0 deletions config/crds/rbac.open-cluster-management.io_clusterpermissions.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,107 @@ spec:
x-kubernetes-validations:
- message: Either subject or subjects has to exist in clusterRoleBinding
rule: has(self.subject) || has(self.subjects)
clusterRoleBindings:
description: ClusterRoleBindings represents multiple ClusterRoleBindings
that are being created on the managed cluster
items:
description: ClusterRoleBinding represents the ClusterRoleBinding
that is being created on the managed cluster
properties:
name:
description: Name of the ClusterRoleBinding if a name different
than the ClusterPermission name is used
type: string
roleRef:
description: RoleRef contains information that points to the
ClusterRole being used
properties:
apiGroup:
description: APIGroup is the group for the resource being
referenced
type: string
kind:
description: Kind is the type of resource being referenced
type: string
name:
description: Name is the name of resource being referenced
type: string
required:
- apiGroup
- kind
- name
type: object
x-kubernetes-map-type: atomic
subject:
description: |-
Subject contains a reference to the object or user identities a ClusterPermission binding applies to.
Besides the typical subject for a binding, a ManagedServiceAccount can be used as a subject as well.
If both subject and subjects exist then only subjects will be used.
properties:
apiGroup:
description: |-
APIGroup holds the API group of the referenced subject.
Defaults to "" for ServiceAccount subjects.
Defaults to "rbac.authorization.k8s.io" for User and Group subjects.
type: string
kind:
description: |-
Kind of object being referenced. Values defined by this API group are "User", "Group", and "ServiceAccount".
If the Authorizer does not recognized the kind value, the Authorizer should report an error.
type: string
name:
description: Name of the object being referenced.
type: string
namespace:
description: |-
Namespace of the referenced object. If the object kind is non-namespace, such as "User" or "Group", and this value is not empty
the Authorizer should report an error.
type: string
required:
- kind
- name
type: object
x-kubernetes-map-type: atomic
subjects:
description: |-
Subjects contains an array of references to objects or user identities a ClusterPermission binding applies to.
Besides the typical subject for a binding, a ManagedServiceAccount can be used as a subject as well.
If both subject and subjects exist then only subjects will be used.
items:
description: |-
Subject contains a reference to the object or user identities a role binding applies to. This can either hold a direct API object reference,
or a value for non-objects such as user and group names.
properties:
apiGroup:
description: |-
APIGroup holds the API group of the referenced subject.
Defaults to "" for ServiceAccount subjects.
Defaults to "rbac.authorization.k8s.io" for User and Group subjects.
type: string
kind:
description: |-
Kind of object being referenced. Values defined by this API group are "User", "Group", and "ServiceAccount".
If the Authorizer does not recognized the kind value, the Authorizer should report an error.
type: string
name:
description: Name of the object being referenced.
type: string
namespace:
description: |-
Namespace of the referenced object. If the object kind is non-namespace, such as "User" or "Group", and this value is not empty
the Authorizer should report an error.
type: string
required:
- kind
- name
type: object
x-kubernetes-map-type: atomic
type: array
type: object
type: array
x-kubernetes-validations:
- message: Either subject or subjects has to exist in every clusterRoleBinding
rule: self.all(i, has(i.subject) || has(i.subjects))
roleBindings:
description: RoleBindings represents RoleBindings that are being created
on the managed cluster
Expand Down
24 changes: 24 additions & 0 deletions config/samples/clusterpermission_multiple_clusterrolebindings.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
apiVersion: rbac.open-cluster-management.io/v1alpha1
kind: ClusterPermission
metadata:
name: clusterpermission-multiple-clusterrolebindings
spec:
clusterRoleBindings:
- name: multi-crb-binding1
roleRef:
apiGroup: rbac.authorization.k8s.io
kind: ClusterRole
name: argocd-application-controller-1
subject:
kind: User
name: user1
- name: multi-crb-binding2
roleRef:
apiGroup: rbac.authorization.k8s.io
kind: ClusterRole
name: argocd-application-controller-3
subjects:
- kind: User
name: user2
- kind: Group
name: group1
81 changes: 65 additions & 16 deletions controllers/clusterpermission_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,9 @@ func (r *ClusterPermissionReconciler) Reconcile(ctx context.Context, req ctrl.Re
log.Info("validating ClusterPermission")

/* Validations */
if clusterPermission.Spec.ClusterRoleBinding == nil && clusterPermission.Spec.RoleBindings == nil {
if clusterPermission.Spec.ClusterRoleBinding == nil &&
(clusterPermission.Spec.ClusterRoleBindings == nil || len(*clusterPermission.Spec.ClusterRoleBindings) == 0) &&
(clusterPermission.Spec.RoleBindings == nil || len(*clusterPermission.Spec.RoleBindings) == 0) {
log.Info("no bindings defined for ClusterPermission")

err := r.updateStatus(ctx, &clusterPermission, &metav1.Condition{
Expand Down Expand Up @@ -117,7 +119,7 @@ func (r *ClusterPermissionReconciler) Reconcile(ctx context.Context, req ctrl.Re

log.Info("preparing ManifestWork payload")

clusterRole, clusterRoleBinding, roles, roleBindings, err := r.generateManifestWorkPayload(
clusterRole, clusterRoleBindings, roles, roleBindings, err := r.generateManifestWorkPayload(
ctx, &clusterPermission)
if err != nil {
log.Error(err, "failed to generate payload")
Expand All @@ -134,7 +136,7 @@ func (r *ClusterPermissionReconciler) Reconcile(ctx context.Context, req ctrl.Re

mwName := generateManifestWorkName(clusterPermission)
manifestWork := buildManifestWork(clusterPermission, mwName,
clusterRole, clusterRoleBinding, roles, roleBindings)
clusterRole, clusterRoleBindings, roles, roleBindings)

var mw workv1.ManifestWork
err = r.Get(ctx, types.NamespacedName{Name: mwName, Namespace: clusterPermission.Namespace}, &mw)
Expand Down Expand Up @@ -255,9 +257,9 @@ func (r *ClusterPermissionReconciler) generateSubjects(ctx context.Context,

// generateManifestWorkPayload creates the payload for the ManifestWork based on the ClusterPermission spec
func (r *ClusterPermissionReconciler) generateManifestWorkPayload(ctx context.Context, clusterPermission *cpv1alpha1.ClusterPermission) (
*rbacv1.ClusterRole, *rbacv1.ClusterRoleBinding, []rbacv1.Role, []rbacv1.RoleBinding, error) {
*rbacv1.ClusterRole, []rbacv1.ClusterRoleBinding, []rbacv1.Role, []rbacv1.RoleBinding, error) {
var clusterRole *rbacv1.ClusterRole
var clusterRoleBinding *rbacv1.ClusterRoleBinding
var clusterRoleBindings []rbacv1.ClusterRoleBinding
var roles []rbacv1.Role
var roleBindings []rbacv1.RoleBinding

Expand Down Expand Up @@ -304,16 +306,63 @@ func (r *ClusterPermissionReconciler) generateManifestWorkPayload(ctx context.Co
clusterRoleBindingRoleRef = *clusterPermission.Spec.ClusterRoleBinding.RoleRef
}

clusterRoleBinding = &rbacv1.ClusterRoleBinding{
TypeMeta: metav1.TypeMeta{
APIVersion: rbacv1.SchemeGroupVersion.String(),
Kind: "ClusterRoleBinding",
},
ObjectMeta: metav1.ObjectMeta{
Name: clusterRoleBindingName,
},
RoleRef: clusterRoleBindingRoleRef,
Subjects: subjects,
// We make the behaviour consistent with how subject and subjects are handled in ClusterRoleBinding
// subjects will just override subject if both are provided
if clusterPermission.Spec.ClusterRoleBindings == nil || len(*clusterPermission.Spec.ClusterRoleBindings) == 0 {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if block is confusing and I think unnecessary. To deal with the spec crbs and backward compatible spec crb, we should add the handling spec crbs logic first. Like:

If crbs exists then do crbs stuffs, so now the return var "clusterRoleBindings" should not be empty anymore.

Only if return var "clusterRoleBindings" is empty and spec crb exist then do crb stuffs

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok how about the latest change? I tried a different AI prompt.

clusterRoleBindings = append(clusterRoleBindings, rbacv1.ClusterRoleBinding{
TypeMeta: metav1.TypeMeta{
APIVersion: rbacv1.SchemeGroupVersion.String(),
Kind: "ClusterRoleBinding",
},
ObjectMeta: metav1.ObjectMeta{
Name: clusterRoleBindingName,
},
RoleRef: clusterRoleBindingRoleRef,
Subjects: subjects,
})
}
}

// ClusterRoleBindings payload (plural)
if clusterPermission.Spec.ClusterRoleBindings != nil && len(*clusterPermission.Spec.ClusterRoleBindings) > 0 {
for _, clusterRoleBinding := range *clusterPermission.Spec.ClusterRoleBindings {
crbSubjects := getSubjects(clusterRoleBinding.Subject, clusterRoleBinding.Subjects)
if err := r.validateSubject(ctx, crbSubjects, clusterPermission.Namespace); err != nil {
return nil, nil, nil, nil, err
}

subjects, err := r.generateSubjects(ctx, crbSubjects, clusterPermission.Namespace)
if err != nil {
return nil, nil, nil, nil, err
}

// default to ClusterPermission name unless using custom name
clusterRoleBindingName := clusterPermission.Name
if clusterRoleBinding.Name != "" {
clusterRoleBindingName = clusterRoleBinding.Name
}

// default to creating a ClusterRole unless using existing ClusterRole
clusterRoleBindingRoleRef := rbacv1.RoleRef{
APIGroup: rbacv1.GroupName,
Kind: "ClusterRole",
Name: clusterPermission.Name,
}
if clusterRoleBinding.RoleRef != nil {
clusterRoleBindingRoleRef = *clusterRoleBinding.RoleRef
}

clusterRoleBindings = append(clusterRoleBindings, rbacv1.ClusterRoleBinding{
TypeMeta: metav1.TypeMeta{
APIVersion: rbacv1.SchemeGroupVersion.String(),
Kind: "ClusterRoleBinding",
},
ObjectMeta: metav1.ObjectMeta{
Name: clusterRoleBindingName,
},
RoleRef: clusterRoleBindingRoleRef,
Subjects: subjects,
})
}
}

Expand Down Expand Up @@ -479,5 +528,5 @@ func (r *ClusterPermissionReconciler) generateManifestWorkPayload(ctx context.Co
}
}

return clusterRole, clusterRoleBinding, roles, roleBindings, nil
return clusterRole, clusterRoleBindings, roles, roleBindings, nil
}
53 changes: 53 additions & 0 deletions controllers/clusterpermission_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,59 @@ var _ = Describe("ClusterPermission controller", func() {
}
return true
}).Should(BeTrue())

By("Creating a ClusterPermission that has multiple ClusterRoleBindings")
clusterPermissionMultipleCRBs := cpv1alpha1.ClusterPermission{
ObjectMeta: metav1.ObjectMeta{
Name: "clusterpermission-multiple-crbs",
Namespace: clusterName,
},
Spec: cpv1alpha1.ClusterPermissionSpec{
ClusterRoleBindings: &[]cpv1alpha1.ClusterRoleBinding{
{
Name: "crb-1",
RoleRef: &rbacv1.RoleRef{
APIGroup: "rbac.authorization.k8s.io",
Kind: "ClusterRole",
Name: "argocd-application-controller-1",
},
Subjects: []rbacv1.Subject{},
Subject: rbacv1.Subject{
Kind: "User",
Name: "user1",
},
},
{
Name: "crb-2",
RoleRef: &rbacv1.RoleRef{
APIGroup: "rbac.authorization.k8s.io",
Kind: "ClusterRole",
Name: "argocd-application-controller-2",
},
Subjects: []rbacv1.Subject{
{
Kind: "User",
Name: "user2",
},
{
Kind: "Group",
Name: "group1",
},
},
},
},
},
}

Expect(k8sClient.Create(ctx, &clusterPermissionMultipleCRBs)).Should(Succeed())
mwKeyMultipleCRBs := types.NamespacedName{Name: generateManifestWorkName(clusterPermissionMultipleCRBs), Namespace: clusterName}
mwMultipleCRBs := workv1.ManifestWork{}
Eventually(func() bool {
if err := k8sClient.Get(ctx, mwKeyMultipleCRBs, &mwMultipleCRBs); err != nil {
return false
}
return true
}).Should(BeTrue())
})
})

Expand Down
8 changes: 5 additions & 3 deletions controllers/helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ func generateManifestWorkName(clusterPermission cpv1alpha1.ClusterPermission) st
// buildManifestWork wraps the payloads in a ManifestWork
func buildManifestWork(clusterPermission cpv1alpha1.ClusterPermission, manifestWorkName string,
clusterRole *rbacv1.ClusterRole,
clusterRoleBinding *rbacv1.ClusterRoleBinding,
clusterRoleBindings []rbacv1.ClusterRoleBinding,
roles []rbacv1.Role,
roleBindings []rbacv1.RoleBinding) *workv1.ManifestWork {
var manifests []workv1.Manifest
Expand All @@ -42,8 +42,10 @@ func buildManifestWork(clusterPermission cpv1alpha1.ClusterPermission, manifestW
manifests = append(manifests, workv1.Manifest{RawExtension: runtime.RawExtension{Object: clusterRole}})
}

if clusterRoleBinding != nil {
manifests = append(manifests, workv1.Manifest{RawExtension: runtime.RawExtension{Object: clusterRoleBinding}})
if len(clusterRoleBindings) > 0 {
for i := range clusterRoleBindings {
manifests = append(manifests, workv1.Manifest{RawExtension: runtime.RawExtension{Object: &clusterRoleBindings[i]}})
}
}

if len(roles) > 0 {
Expand Down
2 changes: 1 addition & 1 deletion controllers/helper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ func Test_buildManifestWork(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := buildManifestWork(tt.args.clusterPermission, tt.args.manifestWorkName, tt.args.clusterRole, tt.args.clusterRoleBinding,
got := buildManifestWork(tt.args.clusterPermission, tt.args.manifestWorkName, tt.args.clusterRole, []rbacv1.ClusterRoleBinding{*tt.args.clusterRoleBinding},
tt.args.roles, tt.args.roleBindings)
// check work name
if got.Name != tt.wants.manifestWorkName {
Expand Down
Loading