Rest hooks for Create and Update #5970

thockin · 2015-03-26T00:19:00Z

Rename pre-create hooks and add pre-validate hooks. I need this for multi-port services.

@smarterclayton in general
@derekwaynecarr for advice on namespace updates
@bgrant0607 or @yujuhong for advice on pod updates

smarterclayton · 2015-03-26T00:37:03Z

pkg/api/rest/update.go

@@ -33,6 +33,10 @@ type RESTUpdateStrategy interface {
 	NamespaceScoped() bool
 	// AllowCreateOnUpdate returns true if the object can be created by a PUT.
 	AllowCreateOnUpdate() bool
+	// PrepareForUpdate is invoked on update before validation to normalize
+	// the object.  For example: remove fields that are not to be persisted,
+	// sort order-insensitive list fields, etc.


Gofmt wrong?

What about it is wrong? gofmt runs on every save...

It's fixed now for some reason.

smarterclayton · 2015-03-26T00:38:06Z

Don't see anything obviously wrong

yujuhong · 2015-03-26T01:23:16Z

pkg/registry/pod/rest.go

+	newPod := obj.(*api.Pod)
+	oldPod := old.(*api.Pod)
+	newPod.Status = oldPod.Status
+	//FIXME: can Spec.Host be updated?


No, ValidatePodUpdate allows only container.image to be updated. That said, I think it's more clear for the request to fail at the validation than simply overwriting Spec.Host here.

That's a good point. @smarterclayton should we validate first? Or should we allow Prepare to return errors? Or should we intentionally copy Host over and let it fail in validation?

Intent was for prepare to do things that involve stripping invalid user input based on the semantics of the call. Status, things that are not intended to be allowed to change. Then validate runs and should fail if the user provides something.

Currently ValidateObjectMetaUpdate is doing some checks (creationTimestamp, etc) that should be handled before PrepareForUpdate and Validation are called:

Create

Object meta is normalized

PrepareForCreate

ValidateCreate

Update

Object meta is normalized

PrepareForUpdate

ValidateUpdate

----- Original Message -----

@@ -59,6 +59,14 @@ func (podStrategy) ResetBeforeCreate(obj runtime.Object)
{
}
}

+// PrepareForUpdate clears fields that are not allowed to be set by end
users on update.
+func (podStrategy) PrepareForUpdate(obj, old runtime.Object) {

newPod := obj.(*api.Pod)

oldPod := old.(*api.Pod)

newPod.Status = oldPod.Status

//FIXME: can Spec.Host be updated?

That's a good point. @smarterclayton should we validate first? Or should we
allow Prepare to return errors? Or should we intentionally copy Host over
and let it fail in validation?

Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/5970/files#r27182480

thockin · 2015-03-26T02:31:57Z

And now integration does not pass.

thockin · 2015-03-26T03:18:55Z

pkg/registry/controller/rest.go

+func (rcStrategy) PrepareForUpdate(obj, old runtime.Object) {
+	newController := obj.(*api.ReplicationController)
+	oldController := old.(*api.ReplicationController)
+	newController.Status = oldController.Status


Humm, clearly I do not fully comprehend this logic. The existence of this line causes integration tests to fail.

E0325 20:15:57.728446 10801 replication_controller.go:175] unexpected sync error: replicationControllers "nginx-controller" cannot be updated: the resource was updated to 1306

Inspecting old and new I see new has Replicas = 2, and old has Replicas = 0.

I am not sure how to track this, and my lack of understanding makes me scared to use it.

I think replication controller does not have a subresource endpoint for status update (#4909), which means it relies on this update to update the status as well. Your change prevents that from happening.

ha, great observation. Given this framework, what is the fix? Just don't?

Yep, don't. Not until we add the status endpoint.

thockin · 2015-03-26T04:37:35Z

I pushed a new snapshot with everything fixed except the question of errors on invalid input.

derekwaynecarr · 2015-03-26T04:57:34Z

I will review this in the morning. Had fell off my radar

ncdc · 2015-03-26T14:22:36Z

@thockin could you please pass the ctx to PrepareForCreate/Update? My use case is I need to ensure the current user is authorized to reference another cluster resource.

smarterclayton · 2015-03-26T14:25:17Z

Vs us going now and adding CheckObjectIntegrity or something... which we could do. I'm a bit torn. If we add context to Create (which we should) then I think we're ok for now.

----- Original Message -----

@thockin could you please pass the ctx to PrepareForCreate/Update? My use
case is I need to ensure the current user is authorized to reference another
cluster resource.

Reply to this email directly or view it on GitHub:
#5970 (comment)

derekwaynecarr · 2015-03-26T14:35:13Z

pkg/registry/namespace/rest.go

+func (namespaceStrategy) PrepareForUpdate(obj, old runtime.Object) {
+	_ = obj.(*api.Namespace)
+	_ = old.(*api.Namespace)
+	// TODO(derekwaynecarr): help to implement?


Need to add:

namespace.Spec.Finalizers = oldNamespace.Spec.Finalizers

done in next push

thockin · 2015-03-26T18:54:37Z

regarding context, can I punt that to another PR (or back to you Andy) - I am trying to finish multi-port services, and can't afford any more distraction :)

thockin · 2015-03-26T18:55:52Z

@smarterclayton I did not parse your last comment. Was that in regards to returning errors if the user tries to change immutable fields? What do you want me to do in this PR vs what can we file a bug on and come back to?

ncdc · 2015-03-26T18:55:53Z

That's fine, thanks
On Thu, Mar 26, 2015 at 2:55 PM Tim Hockin notifications@github.com wrote:

regarding context, can I punt that to another PR (or back to you Andy) - I
am trying to finish multi-port services, and can't afford any more
distraction :)

—
Reply to this email directly or view it on GitHub
#5970 (comment)
.

smarterclayton · 2015-03-26T18:59:01Z

Prepare* for should set up the object so that Validate does not need to know about fields the user can't change. I.e., update clears status because some status updates are invalid, but for the normal path, a user cannot change status, and therefore validation should not have to return an immutable error.

smarterclayton · 2015-03-26T19:00:54Z

Put another way, Prepare* handles taking the context of an update (status, regular post, resize, binding) and ensures the object that will be passed to Validate is a true, properly formed resource object. Validate decides whether the changes are allowed at a fundamental level (is creationtimestamp allowed to change ever). Prepare* might ensure that creationtimestamp gets set properly, so validation doesn't have to make that distinction.

thockin · 2015-03-26T20:18:33Z

new push is up with (I think) all comments addressed except passing context - @ncdc

PTAL

As per discussion with @snmarterclayton. I implemented this for most types in the "obvious" way. I am not sure how to implement this for a couple types, though.

thockin · 2015-03-26T22:37:46Z

Shippable is a flake, but travis passed.

smarterclayton · 2015-03-26T22:41:57Z

LGTM

Rest hooks for Create and Update

googlebot added the cla: yes label Mar 26, 2015

smarterclayton reviewed Mar 26, 2015
View reviewed changes

saad-ali assigned smarterclayton Mar 26, 2015

yujuhong reviewed Mar 26, 2015
View reviewed changes

thockin force-pushed the rest_cleanups branch from de39fa2 to af94e18 Compare March 26, 2015 02:12

thockin reviewed Mar 26, 2015
View reviewed changes

thockin force-pushed the rest_cleanups branch from af94e18 to 8cd1f54 Compare March 26, 2015 04:36

derekwaynecarr reviewed Mar 26, 2015
View reviewed changes

thockin force-pushed the rest_cleanups branch from 8cd1f54 to 2524cc6 Compare March 26, 2015 20:17

thockin added 2 commits March 26, 2015 13:48

rename ResetBeforeCreate to PrepareForCreate

b2687c1

Add REST PrepareForUpdate() hook

0f36c68

As per discussion with @snmarterclayton. I implemented this for most types in the "obvious" way. I am not sure how to implement this for a couple types, though.

thockin force-pushed the rest_cleanups branch from 2524cc6 to 0f36c68 Compare March 26, 2015 20:48

smarterclayton added a commit that referenced this pull request Mar 26, 2015

Merge pull request #5970 from thockin/rest_cleanups

f9aa0d5

Rest hooks for Create and Update

smarterclayton merged commit f9aa0d5 into kubernetes:master Mar 26, 2015

thockin deleted the rest_cleanups branch March 27, 2015 20:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rest hooks for Create and Update #5970

Rest hooks for Create and Update #5970

thockin commented Mar 26, 2015

smarterclayton Mar 26, 2015

thockin Mar 26, 2015

smarterclayton Mar 26, 2015

smarterclayton commented Mar 26, 2015

yujuhong Mar 26, 2015

thockin Mar 26, 2015

smarterclayton Mar 26, 2015

thockin commented Mar 26, 2015

thockin Mar 26, 2015

yujuhong Mar 26, 2015

thockin Mar 26, 2015

yujuhong Mar 26, 2015

thockin commented Mar 26, 2015

derekwaynecarr commented Mar 26, 2015

ncdc commented Mar 26, 2015

smarterclayton commented Mar 26, 2015

derekwaynecarr Mar 26, 2015

thockin Mar 26, 2015

thockin commented Mar 26, 2015

thockin commented Mar 26, 2015

ncdc commented Mar 26, 2015

smarterclayton commented Mar 26, 2015

smarterclayton commented Mar 26, 2015

thockin commented Mar 26, 2015

thockin commented Mar 26, 2015

smarterclayton commented Mar 26, 2015

Rest hooks for Create and Update #5970

Rest hooks for Create and Update #5970

Conversation

thockin commented Mar 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Mar 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Mar 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Mar 26, 2015

derekwaynecarr commented Mar 26, 2015

ncdc commented Mar 26, 2015

smarterclayton commented Mar 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Mar 26, 2015

thockin commented Mar 26, 2015

ncdc commented Mar 26, 2015

smarterclayton commented Mar 26, 2015

smarterclayton commented Mar 26, 2015

thockin commented Mar 26, 2015

thockin commented Mar 26, 2015

smarterclayton commented Mar 26, 2015