Support Groups as Subject Kind for Tenant Namespace RoleBindings created by Capsule

[MaxFedotov]

This PR closes #70 and adds additional configuration parameter use-groups-as-tenant-owner which allows Tenant Namespace RoleBindings to be created with "Kind": "Group"

It's completely backward-compatible, as default is set to false.

[bsctl]

@MaxFedotov thanks for your interest in Capsule and for pushing this PR.

Should we consider to have a more general approach for the Tenant Ownership.
What about to leave the cluster admin to set the Subject.Kind of the owner by tenant instead of a general setting?

I mean something like this:

apiVersion: capsule.clastix.io/v1alpha1
kind: Tenant
metadata:
  name: oil
spec:
...
 subjectOwner:
   kind: Group
   name: foo

Where the kind can be User, Group, and ServiceAccount too. The latter case can be useful if the tenant owner needs be an application.

Not sure if technically feasible. @prometherion @MaxFedotov what do you think?

[prometherion]

The main concern I have is upon Namespace creation, since we're using an indexer to speed up the Owner lookup at webhook level.

I need to investigate and check if it's feasible or not, since we have to lookup also per Kind rather than just the name.

[MaxFedotov]

@prometherion
You are talking about this code - https://github.com/clastix/capsule/blob/0f935d53b7acb17379e749a2d64e2f631adb6779/pkg/webhook/owner_reference/patching.go#L84-L93?

[prometherion]

Exactly, that's a field indexer provided by the related struct

[MaxFedotov]

@prometherion

I need to investigate and check if it's feasible or not, since we have to lookup also per Kind rather than just the name.

There is a way to do it - kubernetes-sigs/controller-runtime#612 (comment)

in our case we can use function like

func getValue(tenant) string {
   return tenant.Spec.SubjectOwner.Kind+":"+tenant.Spec.SubjectOwner.Name
}

But there is another problem, when https://github.com/clastix/capsule/blob/0f935d53b7acb17379e749a2d64e2f631adb6779/pkg/webhook/owner_reference/patching.go#L84-L93
is called, there is no way to get from req admission.Requestan information about a Kind of TenantOwner.
So first we need get TenantList for User:req.UserInfo.Groups and then if len(req.UserInfo.Groups) > 0 loop through it and for each group try to find TenantList which match Group:group_name and append them.

Btw, according to https://github.com/clastix/capsule/blob/0f935d53b7acb17379e749a2d64e2f631adb6779/pkg/webhook/owner_reference/patching.go#L92

we do not event need to find all tenants, just first one returned (which seems like a bit strange logic, so the namespace is actually assigned to a random tenant, within an owner)

Another way, is to enable by default force-tenant-prefix, which will make looking for TenantList match easier, as we can just get tenant name from Namespace name :)

Or we can combine these two approaches:

• if force-tenant-prefix is enabled, extract tenantName from Namespace name and do simple lookup. But in this case we need to apply pattern for TenantName, so it doesn't allow -, as it is a separator between TenantName-NamespaceName. And as it is impossible to do via kubebuilder annotations, as Name goes from metav1.ObjectMeta imported struct, so may be we should create a validating webhook which will enforce pattern: ^[a-z0-9]([a-z0-9]*[a-z0-9])?$ for Tenant Name field.
• if not - find first matching tenant either from User or from Group kind

But my personal vote is for enabling force-tenant-prefix by default, as it will make code more simple and also enforcing naming conventions is obviously a good thing :)

[MaxFedotov]

@prometherion
So what i've done in last commit:

• Updated Capsule CRD to add support for new struct

type OwnerSpec struct {
	Name string `json:"name"`
	Kind Kind   `json:"kind"`
}

• Rewritten indexer to support complex Owner field like

tenant.Spec.Owner.Kind.String() + ":" + tenant.Spec.Owner.Name

• Updated owner_reference webhook with a logic, that I proposed in previous comment

if force-tenant-prefix is enabled, extract tenantName from Namespace name and do simple lookup.
if not - find first matching tenant either from User or from Group kind

• Added tenant name validation webhook in order to restrict tenant names to only alphanumeric characters, so we will be able to extract tenant from namespace name in owner_reference webhook
• Created new tests and updated existing to support these changes
• Updated docs

[prometherion]

Thanks @MaxFedotov, can't wait for take a look to it.

@bsctl do you think we should version a new API since this breaking change in the contract?

IMO, no need since we haven't yet released a stable version :)

[prometherion]

Some typos and more details requests

[MaxFedotov]

@prometherion
I moved logic of getting Tenant for Namespace from owner_reference webhook to utils package to simplify it's refactoring with #76

[prometherion]

@MaxFedotov this rebase will be painful, I know :(
If you don't mind, we could try to work on this together: eager to get it merged.

[prometherion]

LGTM, just check why tests are not green: could be flaky