feat: Permission API block

[niuxe]

trafficstars

Working from the conversation in this PR: https://github.com/projectcapsule/capsule/pull/1332

Original feature was to implement a flag for the webhook to allow creation of namespace, by letting some users impersinate the owner.

After feedback from @oliverbaehler this has changed to be a new API block in the Tenant resource, for controlling the RBAC for the Capsule Tenant.

Features:

• Keeping the old functionality of the AdditionalRoleBindings field.
• Providing an option for Capsule to create ClusterRoleBindings (Addressing this issue #1386 )
• Providing a way of specifying the namespaces for the RoleBindings with the use of a namespace selector.

[niuxe]

@oliverbaehler

In the other PR you asked if I needed help. And I do, at least pointers (not a Go joke).

I'll try to list them here:

• Is the idea to deprecate the AddionalRoleBindings such that the controllers/tenant/manager.go should no longer call syncRoleBindings?
  • Currently I've copied the functionality, to be backwards compatiable. But I'd like your input here.
• Since you have a plan for making the Permissions a CRD, should it be somewhere other than where it's currently implemented?
  • I'm not 100% sure when to put the struct in v1beta2, vs the pkg/api. But I read that a common pattern is to create a shared folder and embed the struct in both places, if the plan is to use it both in the Tenant CRD and as a stand-alone CRD.
• General feed back on where I've written the code. Still new to both Go and writing kubernetes operators, so feedback is very much appricated.

[oliverbaehler]

Is the idea to deprecate the AddionalRoleBindings such that the controllers/tenant/manager.go should no longer call syncRoleBindings

You have done the correct thing. We mainly deprecate in the docs and create an issue. Once we are satisfied with all new functions, we are going to roll over to v1.

Since you have a plan for making the Permissions a CRD, should it be somewhere other than where it's currently implemented?

Correct, or with other structs, which are not directly bound to CRD use only. eg ExtendedSubject might be better placed in the pkg/api folder. Note that methods should not cross from these two packets.

[oliverbaehler]

I am still thinking about, if there's more need to extend this case. With this we are adding these things:

• ClusterRoleBindings are synced or can be targeted
• serivceaccounts can be promoted as owners
• ClusterRoles

So what if we abstracted even more regarding the clusterrolebinding, where on a tenant perspective, we mainly define, which clusterRoleBindings can be bound on bases of labelSelectors (maybe also considering namespace). Eg.

  permissions:
     allowedClusterBindings:
        - matchExpressions:
           - key: target
             operator: In
             values: ["customer"]

Then we offload the entire permissions to it's on CR, essentially allowing tenant owners initially to provision them:

apiVersion: capsule.clastix.io/v1beta2
kind: TenantPermission
metadata:
 name: solar
 namespace: solar-system
spec:
   bindings: 
      - read-only
   subjects:
       - name: operators
         kind: Group
    actAsOwner: false

And then we leverage GlobalTenantResources or TenantResources to distribute TenantPermissions. Not yet done thinking, WDYT?

WDYT

[niuxe]

I like the idea of keeping the Tenant CR as sort of a "Cluster config", in a lack of a better word. But what I view it as, is a resource cluster admins can use to define the scope of a tenant, and then inside the tenant, the owner essentially act as cluster admins.

A lot of the functionality is already there with options to limit labels or image registries.

To me it would make sense with this new feature to aim for a direction of a Tenant resource should be touched as little as possible. Creating is "once", and set the scope, then let the owner create permissions, resources, policies etc.

What I'm thinking though, should this PR create the permission block, with the new CR in mind, or should we add the fields directly on the Tenant CRD?

If you, and I agree, wanna go down the road of keeping the Tenant more as a config CRD, and the off load the whole permission logic to the new CRD. I think it would make more sense implementing it that way in the first place.

A side thought, in our cluster we're trying to make it easy for tenants, and we're adding the tenant-gitops-reconciler to each tenant, as we want that service account to have the same RBAC for all tenants.

If we change the permissions block to only scope clusterRoles allowed to be bound, it feel we as cluster admins lose the "templating" of a tenant. So right now that is my main worry, to strike a balance between letting the owner be as much in charge as possible, and also letting the cluster admins have a good time templating tenants.

Unless the idea is for cluster admins to also use TenantResource og GlobalTenantResource.

[oliverbaehler]

Great Thinking.

Most definitly, i would also prefer going with the dedicated CR. Implementing this goes you core knowhow on how to write a golang based kubernetes controller in no time, i will help you with the bootstrapping.

In the case of TenantResource and GlobalTenantResource. The first is meant for tenant owners, while the other one is meant to distribute amongst the collection of tenants, for cluster admins. Imagine this, we are also reflecting the separation of responsibility we are creating with namespaces (plattform-user) and tenant (plattform-admins) in these resources. **So yes, my solution leads to admins having to use GlobalTenantResource, which further lightens the Tenant CRD.

"If we change the permissions block to only scope clusterRoles allowed to be bound" This one i don't understand completely tbh.

[niuxe]

"If we change the permissions block to only scope clusterRoles allowed to be bound" This one i don't understand completely tbh.

What I mean here was, right now the Tenant resource heavily implies that the cluster admins will use that resource to set up things like rolebindings in namespaces in the tenant, which is a feature we're using right now.

When we bootstrap a tenant, we use GlobalTenantResource and AdditionalRolebindings to ensure a service account with a name we choose is present in every namespace in the tenant, and that is has the correct rolebinding. This is done by us to ensure deployments can default to this SA, to make it easier for the tenant.

If we remove this functionality from the tenant, so it's only possible to specify AllowedClusterRoles, I was wondering if that change would also make it so we as cluster admins has to greate the GlobalTenantResource to keep our current practice.

But you answered that. :)

I am a fan of keep the Tenant resource as close to pure configuration as possible, and off loading everything else, e.g. creating rolebindings and other resource to the other CRDs of Capsule.

[niuxe]

Things to achieve in this PR:

• [x] Create new CRD: TenantPermission
• [ ] Add configuration to Tenant CRD for controlling allowed bindings to clusterroles in the Tenant
• [ ] Update Webhook to validate if the Tenant is alllowed to create the clusterrole
• [ ] Update webhook check for create / update / delete, to no only verify is the requesters is owner, but if they have correct permissions. (Need to discuss if the validation should be based on RBAC alone, or if the Owner of a tenant should be treated differently from other subjects, even if they have sufficient RBAC e.g. the talk about the ActAsOwner).

[github-actions[bot]]

This pull request has been marked as stale because it has been inactive for more than 30 days. Please update this pull request or it will be automatically closed in 30 days.

[github-actions[bot]]

This pull request has been marked as stale because it has been inactive for more than 30 days. Please update this pull request or it will be automatically closed in 30 days.