Relax update validation of uninitialized pod #51733

caesarxuchao · 2017-08-31T18:57:39Z

Split from #50344

Let the podStrategy to only call validation.ValidatePod() if the old pod is not initialized, so fields are mutable.
Let the podStatusStrategy refuse updates if the old pod is not initialized.

Pod spec is mutable when the pod is uninitialized. The apiserver requires the pod spec to be valid even if it's uninitialized. Updating the status field of uninitialized pods is invalid.

caesarxuchao · 2017-08-31T18:57:55Z

/release-note-none
/sig api-machinery

liggitt · 2017-08-31T19:00:46Z

pkg/registry/core/pod/strategy.go

@@ -92,6 +94,14 @@ func (podStrategy) AllowCreateOnUpdate() bool {
 // ValidateUpdate is the default update validation for an end user.
 func (podStrategy) ValidateUpdate(ctx genericapirequest.Context, obj, old runtime.Object) field.ErrorList {
 	errorList := validation.ValidatePod(obj.(*api.Pod))
+	oldMeta, err := meta.Accessor(old)


wrap all of this with a check of the initializers feature gate

or actually, maybe just a helper func:

initialized, err := IsInitializedObject(old) switch { case err != nil: return append(errorList, field.InternalError(field.NewPath("metadata"), err)) case !initialized: return errorList default: return append(errorList, validation.ValidatePodUpdate(obj.(*api.Pod), old.(*api.Pod))...) }

liggitt · 2017-08-31T19:01:52Z

test/e2e/resource_quota.go

@@ -148,71 +149,71 @@ var _ = framework.KubeDescribe("ResourceQuota", func() {
 	})

 	It("should create a ResourceQuota and capture the life of an uninitialized pod.", func() {


depends on initializer feature, right?

marked as [Feature: Initializers]

deads2k · 2017-08-31T19:04:26Z

pkg/registry/core/pod/strategy.go

@@ -92,6 +94,14 @@ func (podStrategy) AllowCreateOnUpdate() bool {
 // ValidateUpdate is the default update validation for an end user.
 func (podStrategy) ValidateUpdate(ctx genericapirequest.Context, obj, old runtime.Object) field.ErrorList {
 	errorList := validation.ValidatePod(obj.(*api.Pod))


Why shouldn't a pod be allowed to be invalid during the process of initialization? It is possible to do with admission today and given a "my special defaulter that can take an invalid user object and choose a good/valid default" initializer, seems even more useful after.

It is possible to do with admission today

not in a persisted, visible state that someone getting an object from the API has to deal with, or that migration has to take into account. bad data in etcd is not something I'm excited about.

not in a persisted, visible state that someone getting an object from the API has to deal with, or that migration has to take into account. bad data in etcd is not something I'm excited about.

It's uninitialized data. Not allowing someone to initialize that data to a shape of their choosing seems excessively restrictive. I know that I would respond to an API not letting me do it by pushing garbage into fields that fail validation and using magic values.

Our API already has problems dealing with new fields and I don't see forcing this limitation as helping.

if we want to further relax this in the future, I think we can, if there are compelling use cases that outweigh the downsides of persisting bad data.

if we want to further relax this in the future, I think we can, if there are compelling use cases that outweigh the downsides of persisting bad data.

I suppose its true that we can't go the other way around. If we do it, I reserve the right to say "I told you so" twice. :)

seems even more useful after.

It might be useful. However, user can GET uninitialized objects. I think it's more important to never persist invalid objects.

I suppose its true that we can't go the other way around. If we do it, I reserve the right to say "I told you so" twice. :)

let the record show that @deads2k and @liggitt do not always agree :)

caesarxuchao · 2017-08-31T20:30:19Z

pkg/registry/core/pod/strategy.go

 // ValidateUpdate is the default update validation for an end user.
 func (podStrategy) ValidateUpdate(ctx genericapirequest.Context, obj, old runtime.Object) field.ErrorList {
 	errorList := validation.ValidatePod(obj.(*api.Pod))
+	uninitializedUpdate, err := isUpdatingUninitializedPod(old)


@liggitt I just saw the code you posted. Do you have a strong preference using the switch statement?

I find it easier to read, but don't feel strongly

caesarxuchao · 2017-08-31T20:32:11Z

pkg/registry/core/pod/strategy.go

@@ -89,9 +93,31 @@ func (podStrategy) AllowCreateOnUpdate() bool {
 	return false
 }

+func isUpdatingUninitializedPod(old runtime.Object) (bool, error) {
+	if !utilfeature.DefaultFeatureGate.Enabled(features.Initializers) {


The generic registry will delete the initiailizers if the feature gate is disabled, so the check here is just to be future-proof.

liggitt · 2017-08-31T20:35:51Z

test/e2e/resource_quota.go

-
-		// TODO: uncomment the test when the replenishment_controller uses the
-		// sharedInformer that list/watches uninitialized objects.
+	It("[Feature: Initializers] should create a ResourceQuota and capture the life of an uninitialized pod.", func() {


[Feature:Initializers]

liggitt · 2017-08-31T20:57:40Z

@derekwaynecarr is there an issue tracking the quota admission updates that need to be done?

caesarxuchao · 2017-08-31T21:07:14Z

@liggitt @derekwaynecarr my sharedInformer PR (#51247) isn't going to get merged before the deadline, so the quota replenishment controller won't capture the deletion of uninitialized objects. I will send a follow-up PR to let the quota admission only charge the quota when the object is initialized.

caesarxuchao · 2017-08-31T23:31:35Z

@liggitt I sent out #51749, which makes the quota admission only charges the final update that removes the last pending finalizer.

caesarxuchao · 2017-09-01T00:34:22Z

@LiGgit this PR doesn't need to wait for #51749. The quota leak issue existed when we first introduced initializers.

caesarxuchao · 2017-09-01T06:34:50Z

@liggitt do you have other concerns for this PR?

The quota problem is addressed in #51749, where we charge the quota when pod is initialized, so no need to worry about if the quota system can handle updates of uninitialized pods here.

liggitt · 2017-09-01T16:06:12Z

/lgtm
needs release note

caesarxuchao · 2017-09-01T17:05:20Z

Release notes added.

@deads2k could you approve? Thanks.

deads2k · 2017-09-01T17:18:36Z

/approve

k8s-github-robot · 2017-09-01T17:19:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, deads2k, liggitt

Associated issue: 47837

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

Needs approval from an approver in each of these OWNERS Files:

~~pkg/registry/OWNERS~~ [deads2k]
~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [caesarxuchao,deads2k,liggitt]
~~test/OWNERS~~ [deads2k,liggitt]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

liggitt · 2017-09-01T17:21:12Z

added an item to do this generically and remove the pod special case in #49038

caesarxuchao · 2017-09-01T17:33:44Z

Added the item to the umbrella issue.

fejta-bot · 2017-09-04T12:27:50Z