Skip to content
This repository has been archived by the owner on Feb 19, 2024. It is now read-only.

Implement OrganizationRole #68

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Implement OrganizationRole #68

wants to merge 6 commits into from

Conversation

jiachengxu
Copy link
Contributor

@jiachengxu jiachengxu commented Aug 4, 2020

What this PR does / why we need it:
This PR implements the OrganizationRole concept in proposal #66. The changes are as following:

  1. OrganizationRoleTemplate will create OrganizationRole not rbacv1.Role directly.
  2. rbacv1.Role will be created in OrganizationRole controller.
  3. The permission degradation is implemented for OrganizationRole.

PS: user still can create an OrganizationRole which escalates the user's privilege, currently, we will still let the OrganizationRole be created, but OrganizationRole controller will perform an intersection checking between max level permission of the Organization and the OrganizationRole, and only the intersection rules will be accepted.
In the next PR, I will implement a webhook to forbidden that, in the webhook, we first check the enabled rules for user by issuing SelfSubjectRulesReview, check if rules in OrganizationRole is a subset of that, if it is not, forbidden user to create OrganizationRole.
Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Documentation:

Does this PR introduce a user-facing change?:

NONE

@kubermatic-bot kubermatic-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Aug 4, 2020
@kubermatic-bot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jiachengxu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubermatic-bot kubermatic-bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 4, 2020
@jiachengxu jiachengxu changed the title [WIP] OrganizationRole Implement OrganizationRole Aug 6, 2020
@kubermatic-bot kubermatic-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Aug 6, 2020
func (r *OrganizationRoleReconciler) SetupWithManager(mgr ctrl.Manager) error {
enqueueAllRoles := &handler.EnqueueRequestsFromMapFunc{
ToRequests: handler.ToRequestsFunc(func(mapObject handler.MapObject) (out []ctrl.Request) {
organizationRoles := &corev1alpha1.OrganizationRoleList{}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to have scaling issues after a while, and it's wasteful, instead

Suggested change
organizationRoles := &corev1alpha1.OrganizationRoleList{}
ToRequests: handler.ToRequestsFunc(func(mapObject handler.MapObject) (out []ctrl.Request) {
obj := mapObject.Object.(*corev1alpha1.OrganizationRoleTemplate)
for _, organizationRole := range obj.Status.Targets {
out = append(out, ctrl.Request{
NamespacedName: types.NamespacedName{
Name: organizationRole.Name,
Namespace: mapObject.Meta.GetNamespace(),
},
})
}
return

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, this cannot work because the Targets in the Status are Organizations and Projects, not OrganiationRole.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmmm ok. But you can reconcile only a limited subset for each OrganizationRoleTemplate, and not every OrganizationRole? Because you know which OrganizationRole's are created for each OrganizationRoleTemplate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the reason that I reconcile all OrganizationRole is because OrganizationRoleTemplate doens't have a selector, which means OrganizationRoleTemplate applies to all Organizations in the system, once we have a selector for OrganizationRoleTemplate, we don't need to reconcile every OrganizationRole

pkg/utils/intersection/intersection.go Outdated Show resolved Hide resolved
if err = r.List(ctx, organizationRoleTemplates); err != nil {
return
}
var maxRules []rbacv1.PolicyRule
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd disagree with the rule cap being implemented like this. Instead, it should probably be defined in the Organization CRD itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I did't get it :)
What do you mean by defining in Organization?

Copy link
Contributor

Choose a reason for hiding this comment

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

By defining it in the Organization I mean we have a field .spec.maximumPermissions or something like it with rule lists which are the maximum allowed policy rules allowed within this organization.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We cannot pre-define the maximum permission of the Organization in the Organization itself. On one knows which permission will be needed when they create the Organization, for example, when we integrate Bulward with KubeCarrier, everything is dynamic, if couchdb objects are created, Organization should grant with couchdb related permissions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Where do we define it then? @thetechnick

It needs to be somewhere nicely defined and specified for ease of use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we define it in the OrganizationRoleTemplate, and then give the operator/admin for example, could be KubeCarrier operator to update the OrganizationRoleTemplate to grant/revoke permissions, and every time organization admin create the new OrganizationRole we check it with the Max Rule, like what is implemented now, and accepted rule will be shown in the status of OrganizationRole

Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't the obvious way. Where can I find out what's the maximum role scope for this organization? It's hidden away in OrganizationRoleTemplates.

Also, nothing about this is mentioned in the proposal IIRC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was mentioned a bit here:https://github.com/kubermatic/bulward/pull/66/files#diff-da75ad503782278090a6e6ef7b58c9d7R9-R10

I am not sure if user should care about the maximum permission at all, at least Organization Owner will have maximum permission.

Copy link
Contributor

Choose a reason for hiding this comment

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

ohh,, now I see it; it's a bit like a footnote.

This maximum permission level results from all roles that are assigned to the organization unit and thus to the Organization/Project Owner.

How bout we talk it briefly in tomorrow's daily?


we should at least aggregate maximum permission cap somewhere, like in the Organization status, to have a clear picture what it is.

@nmiculinic nmiculinic removed their assignment Oct 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants