Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add applications to act as tenant owner #276

Closed
bsctl opened this issue Jun 3, 2021 · 9 comments · Fixed by #330
Closed

Add applications to act as tenant owner #276

bsctl opened this issue Jun 3, 2021 · 9 comments · Fixed by #330
Assignees
Labels
enhancement New feature or request needs-discussion No outline on the feature, discussion is welcome

Comments

@bsctl
Copy link
Member

bsctl commented Jun 3, 2021

Describe the feature

We should consider to have a more general approach for the Tenant Ownership. Currently ownership of a tenant is assigned by the cluster admin to a single subject: User or Group. However, there are cases where an application needs to act as tenant owner, for example, to provision namespaces or getting list of nodes and storage/ingress class assigned to tenant (through the capsule-proxy or simply asking the APIs server).

This enhancement proposal tries to address the above requirement. We already considered this topic in the past, see this comment, but for some reason, we excluded ServiceAccounts from tenant ownership.

The current proposal is inspired by the Harbor Registry with its RobotAccount feature.

What would the new user story look like?

  1. The Cluster Admin creates a new tenant as for this manifest
apiVersion: capsule.clastix.io/v1alpha1
kind: Tenant
metadata:
  name: oil
spec:
  owners:
  - kind: Group
    name: engineering
  - kind: User
    name: alice
  - kind: ServiceAccount
     token:
       aud: robot
       iss: kubernetes/serviceaccount
       # other stuff here that can make the jwt token
       # unique and only known by the tenant owner

we should make sure the tenant owner can create one or more Service Account by his own, in a self-provisioning way, in any namespace of the tenant, and Capsule recognises only such kind of Service Accounts as tenant owners.

  1. The tenant owner creates one or more namespaces in the tenant
  2. The tenant owner creates one or more Service Accounts with grants of tenant owner
  3. The tenant owner creates applications using one of above Service Account

Expected behavior

  1. Capsule Operator and Capsule Proxy recognise the Service Accounts above as tenant owners.
  2. The applications using such Service Accounts, for example an operator, act on behalf of the tenant owner, so they can create namespaces or discover namespaces, discover nodes and ingress/storage classes through the capsule-proxy.
  3. The tenant owner can grant permissions to other users but the Service Accounts created by these users cannot act on behalf of the tenant owner since Capsule and Capsule-proxy do not recognise them as tenant owners.
@bsctl bsctl added enhancement New feature or request blocked-needs-validation Issue need triage and validation needs-discussion No outline on the feature, discussion is welcome labels Jun 3, 2021
@prometherion
Copy link
Member

Thanks for sharing this feature proposal, @bsctl: really appreciated!

I'm not so concerned regarding the Owner Reference mutating webhook: I think we could add a sort of check, when the request is made by a ServiceAccount we could skip the User and Group traversal, simplifying the identity process.

Something similar to capsule-proxy too: a new case and there we go.

What is absolutely breaking is the CRD specification, since v1alpha accepts just a single entry and that would require a lot of gluing in the annotations to make this backward compatible.

However, I know @MaxFedotov is really interested in this topic and I'd like to know his point of view regarding the proposal and if this solves their problems.

@prometherion prometherion removed the blocked-needs-validation Issue need triage and validation label Jun 3, 2021
@bsctl
Copy link
Member Author

bsctl commented Jun 3, 2021

What is absolutely breaking is the CRD specification, since v1alpha accepts just a single entry and that would require a lot of gluing in the annotations to make this backward compatible.

yep, I'm aware of that. We should go with annotations in v1alpha and then migrate to a new CRD as we're doing with other enhancements.

@MaxFedotov
Copy link
Collaborator

MaxFedotov commented Jun 3, 2021

Hi @bsctl, thanks for a great proposal! That's really should work.
Regarding

  # other stuff here that can make the jwt token
  # unique and only known by the tenant owner

we can use sub field of jwt spec to specify which service accounts we would allow to act as tenant owners.

{
  "iss": "kubernetes/serviceaccount",
  "kubernetes.io/serviceaccount/namespace": "capsule-system",
  "kubernetes.io/serviceaccount/secret.name": "default-token-46p6j",
  "kubernetes.io/serviceaccount/service-account.name": "default",
  "kubernetes.io/serviceaccount/service-account.uid": "255ddde8-f2fe-4440-822f-2bcba1433a74",
  "sub": "system:serviceaccount:capsule-system:default"
}

So the spec will look like:

apiVersion: capsule.clastix.io/v1alpha1
kind: Tenant
metadata:
  name: oil
spec:
  owners:
  - kind: Group
    name: engineering
  - kind: User
    name: alice
  - kind: ServiceAccount
     token:
       iss: kubernetes/serviceaccount
       sub:
         allowedRegex: "^.*capsule-.*$"
         allowed: ["system:serviceaccount:capsule-system:default"]

@prometherion, @bsctl WDYT?

@prometherion
Copy link
Member

What about, instead using directly the sub key as name?

apiVersion: capsule.clastix.io/v1alpha1
kind: Tenant
metadata:
  name: oil
spec:
  owners:
  - kind: Group
    name: engineering
  - kind: User
    name: alice
  - kind: ServiceAccount
    name: system:serviceaccount:capsule-system:default

Honestly, I wouldn't put a regex in the token stuff:that would degrade further performances and, if you need to allow permissions to a set of Service Account, you could simply use the system:serviceaccount:capsule-system notation that would mean all the SA in the capsule-system Namespace.

@MaxFedotov
Copy link
Collaborator

MaxFedotov commented Jun 3, 2021

The problem is if I want to allow all service accounts located in all tenant's Namespaces to act as tenant owners (and as a cluster admin I don't know which namespaces and SA will be in tenant beforehand), but I can use force-tenant-prefix, and supply a regexp like "system:serviceaccount:${TenantName}-.* in tenant spec to achieve it

@bsctl
Copy link
Member Author

bsctl commented Jun 3, 2021

@prometherion @MaxFedotov
that's not exactly the intention of the proposal. If well understand, either the

  - kind: ServiceAccount
    name: system:serviceaccount:capsule-system:default

or

  - kind: ServiceAccount
     token:
       iss: kubernetes/serviceaccount
       sub:
         allowedRegex: "^.*capsule-.*$"
         allowed: ["system:serviceaccount:capsule-system:default"]

give the role of tenant owner to any SA in the capsule-system namespace. Also such SA will be tenant owner of multiple tenants at same time.

Imho, we should give that role only to SA under the control of the the tenant owner:

  # other stuff here that can make the jwt token
  # unique and only known by the tenant owner

I'm not sure how this can be achieved, honestly, need to figure out. May be scraping JWT docs?

@MaxFedotov yes, this would be better

  - kind: ServiceAccount
     token:
       iss: kubernetes/serviceaccount
       sub:
         allowed: ["system:serviceaccount:${TenantName}-.*"]

and --force-tenant-prefix can be a pre-condition to achieve a SA acting as tenant owner.

@prometherion
Copy link
Member

give the role of tenant owner to any SA in the capsule-system namespace

No, it isn't.

It seems could be a good idea using the allowed and allowedRegex fields also in the owner key, although this would require a lot of conversion stuff required for the upcoming CRD version.

To address this in the current schema, we could use the annotations meta-programming, but first let's introduce how the v1alpha2 could be.

apiVersion: capsule.clastix.io/v1alpha2
kind: Tenant
metadata:
  name: new-tnt
spec:
  owners:
  - kind: User
    allowed: ["alice"]
  - kind: Group
    allowed: ["engineering", "production"]
  - kind: ServiceGroup
    allowedRegex: "^.*capsule-.*$"

This would be converted to v1alpha1 as follows.

apiVersion: capsule.clastix.io/v1alpha1
kind: Tenant
metadata:
  name: new-tnt
  annotations:
    owners.capsule.clastix.io/1: "Group;engineering,production"    // 1 is the 0-based index of owners in v1alpha2
    owners.capsule.clastix.io/2: "ServiceAccount;;^.*capsule-.*$"
spec:
  owner:
    kind: User
    name: "alice"

We have to pay extra attention to the cases of converting a v1alpha2 back to v1alpha as follows:

apiVersion: capsule.clastix.io/v1alpha2
kind: Tenant
metadata:
  name: new-tnt
spec:
  owners:
  - kind: User
    allowed: ["alice", "bob"]

Because we would expect something as follows:

apiVersion: capsule.clastix.io/v1alpha1
kind: Tenant
metadata:
  name: new-tnt
  annotations:
    owners.capsule.clastix.io/0: "User;bob"    // adding bob to the allowed list of users
spec:
  owner:
    kind: User
    name: "alice"

Since these changes are going to be fully backed by a new CRD version, we can easily update the old CRD using the minor release to support a new enum value in the Kind property, adding ServiceAccount besides to User and Group.

@MaxFedotov
Copy link
Collaborator

@prometherion
So, what we would start with? Add support for annotations, update ownerReference webhook, update capsule controller ownerRoleBinding func, right?

@prometherion
Copy link
Member

We can start this but, first, we need to scaffold the v1alpha2 in order to start the migration and get a concrete type to use for testing.

We don't need to start gluing the new version into the current status, just having an API there that is going to be used during the development phase is good enough.

We can open a new issue to track the new CRD version so we can easily test the migration webhook without any problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs-discussion No outline on the feature, discussion is welcome
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants