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

core: add a KEP for modeling entitlements to cross-workspace concerns #4

Closed

Conversation

stevekuznetsov
Copy link

No description provided.

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
Use of the entitlement review API might look like

```
> POST /services/entitlementreview/clusters/<entitlement-provider-cluster>/apis/core.kcp.io/v1alpha1/entitlementreviews
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this surprises. Would have expected to have the review as a local request, not against the provider cluster. Also having this under /services is odd.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is related to our private discussion in Slack about inversion of the mechanism:

  • make entitlements properties of the acting workspace.
  • allow a provider workspace to control entitlements.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's against the (local) view server, which is as local as we can get, right? It's not "against" the provider cluster, but we need to indicate somehow that we're reviewing an entitlement from somewhere.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not /clusters/<workspace-checked-for-entitlement>/...?

Copy link
Member

@sttts sttts Apr 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented somewhere else already: every service URL must be announced somewhere in another API object the caller has access to. /services as a prefix is not a convention and can vary by kcp installation and be completely different or on another host.

However, to entitle tenants, a per-tenant endpoint is used:

```
> POST /services/entitlements/clusters/<entitlement-consumer-cluster>/apis/entitlements.tenancy.kcp.io/v1alpha1/entitlementpolicybindings
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe intentional: this means that this POST goes to the same shard as the APIExport workspace? And that the VW is clever enough and has permissions to route the request to the right shard where the tenant workspace lives? This might be possible to do, but need deeper design.

Also: where does an entitler know this URL from? It must be set somewhere, e.g. on an APIExport.

Alternative might be that we have an ThingEntitlementClass object in the provider workspace, and we put the URL there into status.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe intentional: this means that this POST goes to the same shard as the APIExport workspace?

I expect this to be unrelated from APIExports, actually - we already have three distinct use-cases in our current code-base and I imagine Edge MC will have ones as well.

And that the VW is clever enough and has permissions to route the request to the right shard where the tenant workspace lives? This might be possible to do, but need deeper design.

Orthogonally from that, though, yes, if we expect a user to interact with one endpoint and we need the output objects to go to the right shard, we need that routing logic. Doesn't that problem exist already today with any other view?

Also: where does an entitler know this URL from? It must be set somewhere, e.g. on an APIExport.

I was expecting something like your ThingEntitlementClass idea, good call-out that this is underspecified.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't that problem exist already today with any other view?

Am not aware of any such case. It's not trivial I fear, basically depends on the workspace index.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thought: would add the provider cluster and policy name in here:

POST /services/entitlements/clusters/<provider>/<policy-name>/clusters/<entitlement-consumer-cluster>/apis/entitlements.tenancy.kcp.io/v1alpha1/entitlementpolicybindings

That way we could authz the access easily against the policy object.

```

Entitlement policy bindings will be stored in tenant clusters; however, the tenant will have
no access to read or change them. This mechanism is used to distribute this data, reduce its
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not read?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hoping to strike a middle ground between providing a useful base implementation while not exposing the APIs anywhere they don't need to be exposed, so it's less likely that when a deployment of kcp changes the implementation mechanism, people have their workflows broken. Perhaps useful to sketch out what it looks like when a custom entitlement review is added, though - no real reason to remove this one from the system at that time, it could still function, as a fallback?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that thought 👍 If you cannot read them, nobody will ask about listing either, and with that nobody will ask for an exhaustive list of entitlement including external ones.

@ncdc ncdc changed the title core: add a KEP for modeling entitlements to cross-worksapce concerns core: add a KEP for modeling entitlements to cross-workspace concerns Apr 4, 2023
keps/core/model-cross-worksapce-entitlements.md Outdated Show resolved Hide resolved
keps/core/model-cross-worksapce-entitlements.md Outdated Show resolved Hide resolved
keps/core/model-cross-worksapce-entitlements.md Outdated Show resolved Hide resolved
keps/core/model-cross-worksapce-entitlements.md Outdated Show resolved Hide resolved
Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
@sttts
Copy link
Member

sttts commented Apr 5, 2023

Looks pretty good. Great work! 👍

Some open questions:

@sttts
Copy link
Member

sttts commented Apr 5, 2023

And as mentioned before: an API exploration for a quota API might help to drive direction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants