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

Use selectors instead of AllowedList for IngressClasses, StorageClasses, PriorityClasses #436

Closed
MaxFedotov opened this issue Sep 27, 2021 · 6 comments · Fixed by #673
Closed
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@MaxFedotov
Copy link
Collaborator

We are using a specific struct, AllowedListSpec:

type AllowedListSpec struct {
	Exact []string `json:"allowed,omitempty"`
	Regex string   `json:"allowedRegex,omitempty"`
}

to allow tenant users to use only specific IngressClasses, StorageClasses or PriorityClasses.

While it allows additional fine-grained configuration, that is not a kubernetes way of specifying resources.
Much better will be to use label selectors (the same way, as it is in nodeSelector option for tenant).

With this, tenant spec will look like:

apiVersion: capsule.clastix.io/v1beta1
kind: Tenant
metadata:
  name: oil
spec:
  owners:
  - name: alice
    kind: User
  storageClasses:
    selector:
       foo: bar  

This change will simplify webhook code, make it works much faster (no regexp check), and will simplify the code of a capsule-proxy (as we won't need to get all resources first and then perform regexp validation or array probing).

The downside of this change is that it is completely backward incompatible and there will be no way to automatically upgrade tenant spec to this new version.

@bsctl, @prometherion need your opinion on this.

@MaxFedotov MaxFedotov added enhancement New feature or request blocked-needs-validation Issue need triage and validation labels Sep 27, 2021
@bsctl
Copy link
Member

bsctl commented Sep 28, 2021

@MaxFedotov sounds good to me since more Kubernetes like. What about labelSelector ?

For backward compatibility same concerns. Let’s see @prometherion point of view

@MaxFedotov
Copy link
Collaborator Author

@bsctl

What about labelSelector ?

Can you please explain in more details?

@bsctl
Copy link
Member

bsctl commented Sep 28, 2021

Can you please explain in more details?

just this:

apiVersion: capsule.clastix.io/v1beta1
kind: Tenant
metadata:
  name: oil
spec:
  owners:
  - name: alice
    kind: User
  storageClasses:
    labelSelector:
       foo: bar  

@prometherion
Copy link
Member

@MaxFedotov sounds good to me since more Kubernetes like. What about labelSelector ?

Couldn't agree more on this.

For backward compatibility same concerns. Let’s see @prometherion point of view

This change is absolutely breaking and not portable using a conversion. The unique way to use it would be adding labelSelector a third option in the AllowedList spec.

https://github.com/clastix/capsule/blob/d3e3b8a881b4ce25332e8d1a6904ff178b11242d/api/v1beta1/allowed_list.go#L12-L15

However, this would make things even more complicated on capsule-proxy side since we would need to add additional checks in the selection.

I got a plan:

  1. introducing this feature for v1beta1 as annotation
  2. plan landing on v1beta2
  3. switch entirely towards selection for v1 API version that will be Capsule v1.0.0

Do you think this could be feasible?

@bsctl
Copy link
Member

bsctl commented Oct 7, 2022

@prometherion as we're approaching new API version for Tenant resource, what about to introduce this enhancement? To avoid breaking changes, we can just add another method of specifying Classes to make the transition smooth:

apiVersion: capsule.clastix.io/v1beta1
kind: Tenant
metadata:
  name: oil
spec:
  owners:
  - name: alice
    kind: User
  storageClasses:
    labelSelector: # <<< new way in v1beta2
       foo: bar
    allowed: # <<< old way deprecated but still supported in v1beta2, removed in v1
    - default
    allowedRegex: "^tier-.*$"  

@prometherion prometherion added this to the v0.2.0 milestone Oct 13, 2022
@prometherion
Copy link
Member

@bsctl you're right, @MaxFedotov agreed on working on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
3 participants