Allowing multiple service accounts to act as owner of a Tenant

[KristianTrifork]

Hello Capsule!

Me and a colleague have been talking to the awesome people at Capsule on Slack, about a challenge we encountered when implementing multi tenancy, and using Flux.

Context

We have a tenant called "demo-tenant". We follow Capsules guide and bootstrap a namespace and create a Serviceaccount as the owner.

apiVersion: capsule.clastix.io/v1beta2
kind: Tenant
metadata:
  name: demo-tenant
spec:
  owners:
    - name: system:serviceaccount:demo-tenant-bootstrap:tenant-gitops
      kind: ServiceAccount
      clusterRoles:
        - cluster-admin
  additionalRoleBindings:
    - clusterRoleName: cluster-admin
      subjects:
        - name: tenant-gitops
          kind: ServiceAccount

We add the service account to our HelmRelease:

manager.capsuleUserGroups:
      - system:serviceaccounts:demo-tenant-bootstrap

The Tenant creates their new namespace:

---
apiVersion: v1
kind: Namespace
metadata:
  name: demo-tenant-default

Now the Tenant deploys a HelmRelease which is creating new namespaces.

The Tenant is required to do two things:

• Supply a service account in the HelmRlease manifest. Under spec.serviceAccountName
• The service account has to be in the same namespace as the HelmRelease.

We make sure each namespace in the Tenant has a service account called tenant-gitops, by using GlobalTenantResource.

apiVersion: capsule.clastix.io/v1beta2
kind: GlobalTenantResource
metadata:
  name: tenant-base-resources
spec:
  resources:
    - namespaceSelector:
        matchExpressions:
          - key: capsule.clastix.io/tenant
            operator: Exists
      rawItems:
        - apiVersion: v1
          kind: ServiceAccount
          metadata:
            name: tenant-gitops
  resyncPeriod: 60s

This service account system:serviceaccount:demo-tenant-default:tenant-gitops is not TenantOwner, and is not part of CapsuleUserGroups so it is not allowed to create the namespace.

Feature request

This could be done in the tenant.spec.additionalRoleBindings.[index].actAsOwner. This would allow us to make all tenant-gitops service accounts tenant owners dynamically.

This would allow the Tenant to deploy HelmRelease in any namespace in the Tenant, and create new namespace from that HelmRelease.

This would also be GitOps compliant as we don't rely on mutating manifests after deployment.

We would love to hear your input on this feature!

[oliverbaehler]

I am open to that discussion, i have had multiple customers with the same demand. Do you feel comfy enough to draft a PR with your suggestion, so we can explore an effective implementation together.

we have to changed this, since these are the webhooks verifying tenant ownership wrapped around all the other admissions:
https://github.com/projectcapsule/capsule/blob/main/pkg/webhook/utils/in_capsule_groups.go

Now here is where we need to do changes:

capsule/pkg/webhook/utils/is_capsule_user.go

Line 28 in 49fb307

if sets.NewString(req.UserInfo.Groups...).Has("system:serviceaccounts") {

Here I am just a bit worried about a potential performance impact, but we would some indexers, then that should be fine. So we need to index serviceAccount owners with their corresponding. That should only be done, if none of the groups matched. I would not sync it to the capsuleConfiguration.

It makes sense to me, however i am not yet 100% convinced by the declaration with tenant.spec.additionalRoleBindings.[index].actAsOwner, not to say it's a bad idea.

WDYT @prometherion

[KristianTrifork]

Do you feel comfy enough to draft a PR with your suggestion, so we can explore an effective implementation together.

I'll discuss it with my team, I know this week is going to be hard, as we have a deadline on Friday, but we do start new sprint after that, and it have been brought up that we could try and formulate a PR with this feature.

It makes sense to me, however i am not yet 100% convinced by the declaration with tenant.spec.additionalRoleBindings.[index].actAsOwner, not to say it's a bad idea.

I wrote it there, because my initial understanding after toying around with Capsule and the RBAC, I saw it handled the RBAC by doing that, so I thought it would also let the new service accounts create namespaces, if the correct role was provided. I am very open to where the toggle could be placed.

Another place could be tenant.spec.owners.[index].allowImpersonation. Naming and placement of the field I do not have any strong feelings about.

[prometherion]

I already discussed this with @KristianTrifork on Slack, and I'm ok with the actAsOwner field, defaulted to false to keep it backwards compatible.

Adding to the discussion, @maxgio92 since everything started while testing the FluxCD addon he's maintaining.

[maxgio92]

Thank you all. I see the current limitations and the value this feature would bring.

I also don't like too much the API, I would probably expect to be a specification of spec.owners as those ServiceAccounts would be TenantOwners. I imagine that IsTenantOwner would consider also, when actAsOwner knob is enabled, ServiceAccounts in the Tenant's Namespaces (and possibly with a specific name/labels, if we want to filter futher).

[prometherion]

@oliverbaehler

Now here is where we need to do changes:

That's not correct. We already intercept HTTP calls made by ServiceAccount in the Tenant's Namespaces, and it has worked like a charm so far (famous last words).

[prometherion]

A pitfall we discovered with Massimiliano is about Kubernetes RBAC:

apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
  creationTimestamp: "2024-05-17T09:22:08Z"
  name: capsule-namespace-provisioner
  resourceVersion: "163131743"
  uid: ff156d60-0968-4cd0-a259-9aa4479c443d
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: ClusterRole
  name: capsule-namespace-provisioner
subjects:
- apiGroup: rbac.authorization.k8s.io
  kind: Group
  name: system:serviceaccounts:default

We need to know in advance the Namespaces name, otherwise, Kubernetes will block with RBAC the creation of Namespaces.

Since Capsule automatically generates this object, we could update the subjects with the entries of ServiceAccount marked to act as tenant owners in the proposed API change.

@maxgio92 @oliverbaehler @KristianTrifork

[KristianTrifork]

The way we got around the RBAC restriction from Kubernetes, was by adding our service account, tenant-gitops to the additionalRoleBindings spec of the Tenant resource. When we inspected this in Kubernetes, IIRC Capsule handled the creation of the rolebinding in each namespace, for the ServiceAccount.

That was my initial reason for putting the flag there, as it would fit in with the behavior of Capsule handling the RBAC.

If, we put this flag in the tenant.spec.owners.[index].allowImpersonation (for example), it will still require either the cluster admin creating the RBAC rules, either using GlobalTenantResources or manually. (Unless this flag should make the ServiceAccount viewed as Owner in every aspect.

But if the flag is set in the additionalRoleBindings and added actAsOwner field to Tenant.spec.additionalRoleBindings[index] it would be a expected behavior, that the RBAC, both needs to set deliberately, and we can make Capsule webhook, allow this ServiceAccount to create namespaces.

I think for me it comes down to exactly what actAsOwner should mean, my thought about it is:

• If a ServiceAccount should be an Owner it should be defined as an Owner in the Tenant spec.
• If multiple ServiceAccounts should be able to create namespaces, they can be allowed to act or impersonate. But only for the purpose of creating namespaces in the Tenant.

I'd very much like to hear your understanding of what the flag should imply @prometherion

Because I think we should try to keep it as close to the design philosophy of Capsule as possible, so we don't create magic flags, which does not fit with the general understanding of what Capsule handles.