enhancements

enhancements copied to clipboard

• One-line PR description: subresource finalizers

• Issue link: https://github.com/kubernetes/enhancements/issues/3285

• Other comments:

/assign @lavalamp @deads2k

@lavalamp @deads2k Could you give feedback on this KEP?

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mkimuram To complete the pull request process, please ask for approval from deads2k after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• keps/prod-readiness/OWNERS
• keps/sig-api-machinery/OWNERS

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

@thockin Thank you for your review. I've update this KEP to address your comments. I don't think that we can make it for v1.25, so please review it when you have enough bandwidth.

(I would like to make this KEP detailed enough not to make this KEP a blocker for lien KEP).

As we think about this, another good subresource could be /status/conditions

It almost looks like we're creeping into per-field RBAC. If we step back and think about this in that frame, does it change how we implement it? Would you still want to implement this as a bunch of subresources, or is there a better way?

It almost looks like we're creeping into per-field RBAC. If we step back and think about this in that frame, does it change how we implement it? Would you still want to implement this as a bunch of subresources, or is there a better way?

I guess that would only get us permissions but not independent PUTs and DELETEs, so maybe it's insufficient anyway.

I think subresources is the path to more granular access, but obviously not every field. This proposal is really about providing better control of permissions for things that are commonly used to implement controllers, especially non-core things.

For comparison, I would probably NOT endorse a sub-resource for individual service fields.

One could argue that we could have a single /metadata subresource which covers all of these, but it seems valuable to go for granularity since we know that there's a difference between "I can add a finalizer" and "I can change your labels". It would be nice to go EVEN finer - e.g. a given pricipal can add/remove finalizers with a specific prefix, but that seems like it would require even more thinking and deeper changes. Anyone feel up for it? :D

/lgtm

PRR is fine, needs SIG approval.

I think we can't approve this now, this was @deads2k's ask and I'm not sure this is what he was hoping to get. Since he hasn't already approved, I'll say what I think:

If it were just this in isolation, then a subresource seems right, but it's not, it's N metadata fields, certainly we need this for labels, annotations, ownerReferences, liens if we do that... that turns into a lot of subresources and a lot of custom ~verbs~permissions, and suddenly it's no longer true that you only have to know a few things to be a k8s api client.

I would like to see a comparison with another approach, e.g. extending RBAC to limit to specific fields only, before settling on this.

Really sorry for the late time in the cycle at which I was able to review.

What do you mean "custom verbs" ? Obviously it has client-lib expansion, but the verbs are common?

Meant permissions (RBAC verb), sorry, edited.

I would like to see a comparison with another approach, e.g. extending RBAC to limit to specific fields only, before settling on this.

It seems an interesting idea. If we can set a separate RBAC without adding endpoints, it would be nice. I agree with continuing discussion by comparing another approaches like it (And thanks to @thockin and @apelisse, subresource approach become a bit more detailed, which seems a good big step to solve the issue in this release).

When/where can we continue this discussion? I'm eager to see us do better on controller permissions and this (abstract field-level control) seems like a big step forward.

Should we put it on sig-auth or sig-api-machinery sometime in the not-too-distant future? If so, please tell me when and I will be there :)

My vague idea is to add to roles/cluster roles optional allow and deny field lists, so you can give permissions specifically to a given fieldset, or on all fields NOT in that fieldset. The system would find all roles you match an union the permitted fields to figure out what you're allowed to set. This would really help some people trying to enforce certain policies, since many policies can be enforced by a special permission being needed for a certain field.

SSA has a relatively compact field specification format we could use...

I don't have time right now to expand this into a proper design, so hopefully this vague paragraph inspires someone.

SIG Auth & SIG API Machinery are the places to debate this. I suspect field-level control is considered either very hard or an anti-goal right now so it may be difficult, but I think it would help every field all at once instead of us making a KEP + subresource per important field.

Also I'm really sorry again I didn't find time to read this 3 weeks ago.

I would like to see a comparison with another approach, e.g. extending RBAC to limit to specific fields only, before settling on this.

This was my initial take too. But it's more than just field access. We need to be able to independently append to a list and remove from the list. That is, multiple actors need to be able to modify the same field without clobbering each other. Being able to do PUT and DELETE independently on specific values in the list (as if they were real resources) enables that naturally. Just allowing write access to the field doesn't.

Being able to do PUT and DELETE independently on specific values in the list (as if they were real resources)

Nothing in our API works like that. Clients to do that would be very unlike existing clients and that is bad for supporting N languages.

multiple actors need to be able to modify the same field without clobbering each other

SSA fixes this! (...we just need a syntax for representing removing an item you don't own. but you can do that with one of the existing patch formats in a pinch.)

We do not currently enforce certain users only touch "their" values in certain fields in the API, that's the domain of policy e.g. gatekeeper. We would not start doing that with this proposal or any other similar proposal.

A minor comment: shouldn't this be titled "finalizers subresource" instead of "subresource finalizers"? I always thought this refers to finalizing subresources (which doesn't make sense though) before reading the KEP.

I would like to see a comparison with another approach, e.g. extending RBAC to limit to specific fields only, before settling on this.

BTW, a discussion item is now on the SIG API Machinery agenda for next meeting (next Wednesday) and also SIG Auth's agenda (the following Wednesday). Hopefully we can get a clear answer.

I have retitled this.

What are next steps on the scoped-authz doc?

What are next steps on the scoped-authz doc?

SIG Auth road show next week. Then bigger write up of (probably) design 4 as a KEP

I am not sure I agree about nouns. I always read "scale" as a verb. I think of subresources as either literally a sub-section of the type (status) or an abstract polymorphic method. "Scale this resource".

So my lean is towards "finalize". But I could be happy either way. Since scale was ambiguous, this becomes our precedent setter :)

On Sat, Jun 11, 2022, 4:53 AM Tim Bannister @.***> wrote:

@.**** requested changes on this pull request.

In keps/sig-api-machinery/3285-subresource-finalizers/README.md https://github.com/kubernetes/enhancements/pull/3286#discussion_r895015506 :

+Currently, finalizers field isn't subresourced, therefore RBAC for the field can only be set through the one for its entire resource. +On the other hand, some controllers only need to update finalizers field and some of its subresources, and don't need to update its entire resource. +To implement least privilege for such controllers, there should be a way to only allow updating finalizers.

+### Goals + +- Provide a way to set a different RBAC to finalizers from its entire resource + +### Non-Goals + +- Provide a way to set a different RBAC to other than finalizers from its entire resource +- Provide a way to allow updating its entire resource but disallow updating finalizers field + +## Proposal + +A new endpoint finalize is added as an alternative way to access to finalizers field and allow setting a separate RBAC to the endpoint.

Resources and subresources are usually nouns or noun-like. This should be finalizers not finalize.

Compare status, tokenrequest, scale: all noun-like.

— Reply to this email directly, view it on GitHub https://github.com/kubernetes/enhancements/pull/3286#pullrequestreview-1003454053, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABKWAVHHAVWBU2NIIU4MM43VOR4Z7ANCNFSM5UW7NHQA . You are receiving this because you were assigned.Message ID: @.***>

If status is a subresource, then it's surely a noun subresource? Other examples are tokens for ServiceAccount (not createtoken). You can ask the API server to create, update or delete a subresource. That ought to include a (list of) finalizers.

I'm sure that the likes of Tim Berners-Lee and Roy Fielding would be thinking of nouns here, and I think we should too.

Since it returns a Scale object, I thought of it as a noun. Surveying what we currently have:

verbs:

• */proxy
• namespaces/finalize
• pods/attach
• pods/exec
• pods/portforward

nouns:

• */status
• pods/log
• pods/ephemeralcontainers
• pods/eviction (not pods/evict)
• pods/binding (not pods/bind)
• serviceaccounts/token (not createtoken)
• certificatesigningrequests/approval (not certificatesigningrequests/approve)

ambiguous:

• */scale

Framing resources/subresources as nouns which are acted on by verbs makes more sense to me and matches how the authz layer treats requests.

The model conformant subresources are supposed to be nouns. namespaces/finalize was named incorrectly.

@lavalamp - do I understand correctly, that the current plan of record is something along the lines with https://github.com/kubernetes/enhancements/pull/3617 and we decided that going with subresources is not the right way to go (as also mentioned in that KEP)?

If so, should we close this KEP then?

@wojtek-t Yeah I think I have directional approval from both sig auth and api machinery for that. But I don't have people signed up to finish & implement it.

Yeah I think I have directional approval from both sig auth and api machinery for that. But I don't have people signed up to finish & implement it.

I understand that, but that also means we won't go with this proposal because we believe that subresources aren't the right path no matter what. So I will just go ahead and close this KEP.