API changes for Cascading deletion #23928
API changes for Cascading deletion #23928k8s-github-robot merged 2 commits intokubernetes:masterfrom
Conversation
pkg/api/v1/types.go
Outdated
|
@caesarxuchao I'm new to kubernetes, but I've read the log on all the issues (also in the one you point out in the PR) and seems fine to me. I'm not sure I follow when will be Finalizers added and how an entry is removed (i.e. when/how does it know the component. Is always a component in OwnerReferences/OrphanReferences that is there?). But it's fine, I don't need to fully understand it now and someone else can look at it :) |
|
@caesarxuchao oh, silly me. I haven't actually seen the document and only the discussion! Now I've read it. It seems fine to me :) |
pkg/api/types.go
Outdated
There was a problem hiding this comment.
We don't need both an orphan setting AND the finalizer list. lemme go over your proposal, we should pick the design.
Additionally, orphan should have a default value that depends on the object and api version-- default false for new objects, false for existing ones.
pkg/api/types.go
Outdated
There was a problem hiding this comment.
Does this mean the API server deletes owned objects on the caller's behalf? If so, need to make sure this doesn't allow escalation (would an update to add an owner reference allow a user to delete an object they wouldn't otherwise be allowed to delete?)
There was a problem hiding this comment.
Is cross namespace ownership allowed? I'd like to start with disallowing that…
There was a problem hiding this comment.
@liggitt as proposed in #23656, deleting a parent object (e.g., a replicaset) may result in the deletion of a child object (e.g., a pod). I'd imagine If a user has the right to delete a replicaset, he should also has the right to delete a pod, is there any counterexample?
Is cross namespace ownership allowed? I'd like to start with disallowing that…
We'll start with disallowing that. Do you mean I need to explicitly say this in the comment?
There was a problem hiding this comment.
@derekwaynecarr: "I wonder if this should be GuardianReferences so it aligns with dependent/guardian relationships. Maybe it's too close to tax season." (135fb5e#commitcomment-17170872)
There was a problem hiding this comment.
Here's @bgrant0607's reason for naming it "owner": #23656 (comment)
I think "owner" is more proper term in case of garbage collection.
There was a problem hiding this comment.
ReliesOn
DependsOn
LockedBy
KeptAliveBy
ReservedBy
ExistsWhilePresent
Just brainstorming. These names have issues.
If we don't allow cross-namespace references, I think there's a better type to use than ObjectReference.
There was a problem hiding this comment.
OrdainedBy
PermittedBy
DeleteWhenAllGone
ChainDeletionOf
GCReferences
GCLockedBy
GCLockReferences
I'm not sure any of these beat OwnerReferences, maybe it'll give someone a good idea.
There was a problem hiding this comment.
This is a standard concept in databases: existence dependency:
http://www.cs.sfu.ca/CourseCentral/354/zaiane/material/slides/Chapter2/tsld021.htm
https://www.dlsweb.rmit.edu.au/toolbox/ecommerce/tbn_respak/tbn_e2/html/tbn_e2_devsol/er_model_relnshps.htm#ExistenceDependency
62328cc to
359a17f
Compare
pkg/api/types.go
Outdated
There was a problem hiding this comment.
@derekwaynecarr: "I suspect we will need to require qualified names for both keys and values? How else will we prevent clobbering or allow for extensions?" (135fb5e#commitcomment-17170861)
There was a problem hiding this comment.
That's a valid concern. Let's consider what common qualities we should put in the names, maybe a provider field?
There was a problem hiding this comment.
I'll switch the meaning of the key and the value, because a map key needs to be unique, while an "explanatory name" doesn't have to be.
pkg/api/v1/types.go
Outdated
There was a problem hiding this comment.
Please don't use ObjectReference. Create a type that contains exactly the fields we plan to use.
There was a problem hiding this comment.
Also, we'll want to specify the patch merge key.
d5feb94 to
93286f0
Compare
|
@lavalamp it's updated according to comments. PTAL. Thanks. |
357bbac to
2eacc2c
Compare
2eacc2c to
e00932a
Compare
e00932a to
4562a26
Compare
|
@eparis The merge-bot keeps removing the LGTM label, could you take a look? Thanks. |
|
going to have to kick the bot :-( he did that to another PR this morning! |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e build/test passed for commit 4562a26. |
|
Automatic merge from submit-queue |
Automatic merge from submit-queue plumb Update resthandler to allow old/new comparisons in admission Rework how updated objects are passed to rest storage Update methods (first pass at #23928 (comment)) * allows centralizing precondition checks (uid and resourceVersion) * allows admission to have the old and new objects on patch/update operations (sets us up for field level authorization, differential quota updates, etc) * allows patch operations to avoid double-GETting the object to apply the patch Overview of important changes: * pkg/api/rest/rest.go * changes `rest.Update` interface to give rest storage an `UpdatedObjectInfo` interface instead of the object directly. To get the updated object, the storage must call `UpdatedObject()`, passing in the current object * pkg/api/rest/update.go * provides a default `UpdatedObjectInfo` impl * passes a copy of the updated object through any provided transforming functions and returns it when asked * builds UID preconditions from the updated object if they can be extracted * pkg/apiserver/resthandler.go * Reworks update and patch operations to give old objects to admission * pkg/registry/generic/registry/store.go * Calls `UpdatedObject()` inside `GuaranteedUpdate` so it can provide the old object Todo: - [x] Update rest.Update interface: * Given the name of the object being updated * To get the updated object data, the rest storage must pass the current object (fetched using the name) to an `UpdatedObject(ctx, oldObject) (newObject, error)` func. This is typically done inside a `GuaranteedUpdate` call. - [x] Add old object to admission attributes interface - [x] Update resthandler Update to move admission into the UpdatedObject() call - [x] Update resthandler Patch to move the patch application and admission into the UpdatedObject() call - [x] Add resttest tests to make sure oldObj is correctly passed to UpdatedObject(), and errors propagate back up Follow-up: * populate oldObject in admission for delete operations? * update quota plugin to use `GetOldObject()` in admission attributes * admission plugin to gate ownerReference modification on delete permission * Decide how to handle preconditions (does that belong in the storage layer or in the resthander layer?)
This PR includes the necessary API changes to implement cascading deletion with finalizers as proposed is in #23656. Comments are welcome.
@lavalamp @derekwaynecarr @bgrant0607 @rata @hongchaodeng