plumb Update resthandler to allow old/new comparisons in admission#25787
plumb Update resthandler to allow old/new comparisons in admission#25787k8s-github-robot merged 1 commit intokubernetes:masterfrom
Conversation
e718eb0 to
91b5e04
Compare
|
cc @deads2k @smarterclayton @lavalamp @caesarxuchao (related to discussion on plumbing for protecting ownerReferences) @derekwaynecarr PTAL at admission-related bits |
pkg/api/rest/update.go
Outdated
There was a problem hiding this comment.
This resolves a TODO inside the generic store that noted that all sorts of things in the updated object got mutated in BeforeCreate. It also exposed several tests that only passed because of side-effect modifications on the object passed to Update that made later DeepEqual comparisons pass.
434f058 to
3f6c11c
Compare
|
squashed |
|
This is a huge change which I'm just noticing now-- how important for 1.3 is this? @caesarxuchao some of your change is overlapping this change. Can you look over this so at least someone from here will have read this closely? |
|
This is what allows us to secure garbage collection.
|
|
Sure. I'm taking a look. |
|
The change looks a lot worse than it is. |
pkg/registry/controller/etcd/etcd.go
Outdated
There was a problem hiding this comment.
A unrelated question, does anyone know why the folder is called "controller" instead of "replicationcontroller"?
There was a problem hiding this comment.
Can you keep this logic out of the tryUpdate function? It doesn't need to be rerun in every retry. I think you can get the obj by objInfo.UpdatedObject(ctx, nil).
There was a problem hiding this comment.
no, this check is not expensive, and I definitely don't want to call UpdatedObject with a nil oldObj (or even call it multiple times unnecessarily). in Patch API calls, passing in the old object is how the new object is determined, and the admission chain now lives in that call as well.
There are unit tests in place in resttest.go to make sure the old object passed to UpdatedObject() is actually the old object from storage.
There was a problem hiding this comment.
Thanks for the explanation.
There was a problem hiding this comment.
@liggitt, I realized if obj is not available until calling into the storage layer, I don't know how to rebase my PR onto yours: https://github.com/kubernetes/kubernetes/pull/25599/files#diff-543310447bdc03ffceba5cf23a04be97R313
Do you have any suggestions?
Add OldObject to admission attributes Update resthandler Patch/Update admission plumbing
|
added requested godoc |
|
GCE e2e build/test passed for commit 29252ac. |
|
Automatic merge from submit-queue |
Automatic merge from submit-queue Add WrapUpdatedObjectInfo helper This makes it easier to attach checks/transformations to the updated object in storage Update functions, while still keeping the data flow intact (so admission, patch, and other injected checks continue to work as intended), without needing to do anything tricky to get the updated object out of the UpdatedObjectInfo introduced in #25787 This is especially useful when one storage is delegating to another, but wants its checks to be run in the heart of the eventual GuaranteedUpdate call.
Rework how updated objects are passed to rest storage Update methods (first pass at #23928 (comment))
Overview of important changes:
rest.Updateinterface to give rest storage anUpdatedObjectInfointerface instead of the object directly. To get the updated object, the storage must callUpdatedObject(), passing in the current objectUpdatedObjectInfoimplUpdatedObject()insideGuaranteedUpdateso it can provide the old objectTodo:
UpdatedObject(ctx, oldObject) (newObject, error)func. This is typically done inside aGuaranteedUpdatecall.Follow-up:
GetOldObject()in admission attributes