fixes operation for "create on update" #65572

yue9944882 · 2018-06-28T08:40:23Z

What this PR does / why we need it:

Set operation to admission.Create for create-on-update requests.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #65553

Special notes for your reviewer:

Release note:

Checks CREATE admission for create-on-update requests instead of UPDATE admission

yue9944882 · 2018-06-28T08:40:47Z

/kind bug
/sig api-machinery

yue9944882 · 2018-06-28T08:41:06Z

/cc @jennybuckley

jennybuckley

Thanks for fixing this!

Just a couple comments but otherwise it looks good to me. Also I think it should have a release note since it will fix (change) the behavior of update.

jennybuckley · 2018-06-28T17:20:21Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go

 		var transformers []rest.TransformFunc
 		if mutatingAdmission, ok := admit.(admission.MutationInterface); ok && mutatingAdmission.Handles(admission.Update) {
 			transformers = append(transformers, func(ctx context.Context, newObj, oldObj runtime.Object) (runtime.Object, error) {
+				if accessor, err := meta.Accessor(oldObj); err == nil && accessor.GetResourceVersion() == "" {


I think we also need to take the && mutatingAdmission.Handles(admission.Update) part out of line 101 and move it after this check. because the operation is currently hard coded as admission.Update there, and It should also be dependent on whether the object exists.

Also, have you looked into how the patcher checks if it is looking at a persisted object? It uses UID since that is guaranteed not to be specified until the object exists. Resource Version might work the same way, but maybe for consistency we should do it the same as in the patcher. https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go#L319-L323

@jennybuckley Nice catch. Just sync the change to PATCH. PTAL again. Thx :)

yue9944882 · 2018-06-29T04:46:38Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go

+				if mutatingAdmission.Handles(admission.Update) {
+					return newObj, mutatingAdmission.Admit(admission.NewAttributesRecord(newObj, oldObj, scope.Kind, namespace, name, scope.Resource, scope.Subresource, admission.Update, userInfo))
+				}
+				return newObj, nil


For reviewers, this line makes the transformer function dummy.

yue9944882 · 2018-06-29T07:12:02Z

/test pull-kubernetes-e2e-gce

yue9944882 · 2018-06-29T14:08:34Z

/cc @adohe

adohe-zz · 2018-06-29T14:56:15Z

Overall LGTM. I think @lavalamp could help approve this.

adohe-zz · 2018-06-29T14:56:26Z

/assign @lavalamp

jennybuckley

Thanks for fixing this!

Just a couple comments but otherwise it looks good to me. Also I think it should have a release note since it will fix (change) the behavior of update.

jennybuckley · 2018-06-29T19:50:19Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go

 		admissionCheck := func(updatedObject runtime.Object, currentObject runtime.Object) error {
-			if mutatingAdmission, ok := admit.(admission.MutationInterface); ok && admit.Handles(admission.Update) {
-				return mutatingAdmission.Admit(admission.NewAttributesRecord(updatedObject, currentObject, scope.Kind, namespace, name, scope.Resource, scope.Subresource, admission.Update, userInfo))
+			if mutatingAdmission, ok := admit.(admission.MutationInterface); ok {


We shouldn't change the patcher, because it doesn't currently allow create on update anyway.

@jennybuckley Thx for reviewing. I thought that patcher would finally call store.Update here. So the object would also be created when its AllowCreateOnUpdate is true. Am I missing sth?😃

kubernetes/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go

Line 359 in e49e3ba

updateObject, _, updateErr := p.restPatcher.Update(ctx, p.name, p.updatedObjectInfo, p.createValidation, p.updateValidation)

Thats true, but that the patcher also checks if the object already exists in it's applyPatch function and returns an error if the object doesn't exist yet.

@jennybuckley Make sense 👍

jennybuckley · 2018-06-29T19:55:13Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go

 			transformers = append(transformers, func(ctx context.Context, newObj, oldObj runtime.Object) (runtime.Object, error) {
-				return newObj, mutatingAdmission.Admit(admission.NewAttributesRecord(newObj, oldObj, scope.Kind, namespace, name, scope.Resource, scope.Subresource, admission.Update, userInfo))
+				if mutatingAdmission.Handles(admission.Create) {
+					if accessor, err := meta.Accessor(oldObj); err == nil && accessor.GetResourceVersion() == "" {


I think this check should be the outer one, and only one of the checks mutatingAdmission.Handles(admission.Create)/mutatingAdmission.Handles(admission.Update) should be done, depending on the result of this accessor.GetResourceVersion() == "" check.

The way you have it now doesn't work in the case where mutatingAdmission.Handles(admission.Create) is false, accessor.GetResourceVersion() == "" is true, and mutatingAdmission.Handles(admission.Update) is true

In that case it would call Update admission, but it should be calling nothing.

Nice catch :) Thx

CaoShuFeng · 2018-07-02T03:17:41Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go

@@ -122,8 +129,8 @@ func PatchResource(r rest.Patcher, scope RequestScope, admit admission.Interface
 			kind:            scope.Kind,
 			resource:        scope.Resource,

-			createValidation: rest.AdmissionToValidateObjectFunc(admit, staticAdmissionAttributes),
-			updateValidation: rest.AdmissionToValidateObjectUpdateFunc(admit, staticAdmissionAttributes),
+			createValidation: rest.AdmissionToValidateObjectFunc(admit, admission.NewAttributesRecord(nil, nil, scope.Kind, namespace, name, scope.Resource, scope.Subresource, admission.Create, userInfo)),


If I try to create a pod on update, which is not allowed. Will this still have effect? For example, will this increase quota usage?

@CaoShuFeng Thanks for reviewing. I do find some codes need to be trimed in resourcequota controller as u can see in my following commit. And AFAICT resourcequotas would still be fine after these changes WDYT?

Sorry that I have no suggestions about how to fix this issue. But I'd be happy to point it out when I see something doesn't work as expected.

Maybe we can ask for good suggestions from more experienced developers. :)

@CaoShuFeng Thx all the same. I'm all ears😃

Actually, I don't see any invocation of NewObjectCountEvaluator with allowCreateOnUpdate=true, not in kube and not in OpenShift. Maybe @derekwaynecarr can shed some light on that.

In other words: did we miss creations on update in the quota mechanism before this PR?

@sttts I don't see either even search through the whole project. It should have been pruned before 😅

deads2k · 2018-07-02T12:05:36Z

At a high level I like the idea of making the admission calls more correct. @sttts ptal

/assign @sttts

sttts · 2018-07-04T19:55:36Z

It looks pretty good. As a last thing I would like to see a "predetermined breaking point" in the patch handler, i.e.:

if len(oldObj.GetUID()) {
  // if we allow this some day, we have this TODO: call the mutating admission chain with the CREATE verb instead of UPDATE
  panic("a PATCH cannot create objects")
}

yue9944882 · 2018-07-05T03:07:28Z

// if we allow this some day, we have this TODO

@sttts #65723 I believe it's coming very soon :)

sttts · 2018-07-06T12:37:39Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go

@@ -108,6 +108,14 @@ func PatchResource(r rest.Patcher, scope RequestScope, admit admission.Interface
 		userInfo, _ := request.UserFrom(ctx)
 		staticAdmissionAttributes := admission.NewAttributesRecord(nil, nil, scope.Kind, namespace, name, scope.Resource, scope.Subresource, admission.Update, userInfo)
 		admissionCheck := func(updatedObject runtime.Object, currentObject runtime.Object) error {
+			isNotZeroObject, err := hasUID(currentObject)
+			if err != nil {
+				return fmt.Errorf("unexpected error when extracting UID from oldObj: %s", err.Error())


we usually do %v and err

if isNotZeroObject, err := hasUID(currentObject); err != nil { ... } else if !isNotZeroObject { ... }

sttts · 2018-07-06T12:40:26Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go

@@ -108,6 +108,14 @@ func PatchResource(r rest.Patcher, scope RequestScope, admit admission.Interface
 		userInfo, _ := request.UserFrom(ctx)
 		staticAdmissionAttributes := admission.NewAttributesRecord(nil, nil, scope.Kind, namespace, name, scope.Resource, scope.Subresource, admission.Update, userInfo)
 		admissionCheck := func(updatedObject runtime.Object, currentObject runtime.Object) error {
+			isNotZeroObject, err := hasUID(currentObject)


isZeroObject := !hasUID(...) makes the logic simpler

@sttts I'm afraid we cannot do this because unary operator ! can't be applied to a function returning 2 values.🧐

add another helper. Or even simpler: isPreExistingObject instead of isNotZeroObject. Just to get rid of the double negation.

jennybuckley · 2018-07-06T20:23:47Z

@sttts
Do we really need to change the patcher here? It doesn't allow create at this point. Before any admission gets called, the check in applyPatch would fail for objects which aren't in storage yet.

https://github.com/kubernetes/kubernetes/pull/64892/files#diff-15bdfc59ec78ff09b509d4a347560a1bR361 to the serverside apply feature branch which adds that functionality, also makes sure that the admission controllers are called correctly. By trying to change it here too we aren't actually changing any behavior, it would just make it harder to merge later.

sttts · 2018-07-09T09:00:41Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go

+			} else if !isNotZeroObject {
+				// if we allow this some day, we have this TODO: call the mutating admission chain with the CREATE verb instead of UPDATE
+				panic("a PATCH cannot create objects")
+			}


all this is kind of ugly, agreeing with @jennybuckley. Let's condense the change for the patch here into the simple TODO, no panic. Lgtm then.

@sttts Done

sttts · 2018-07-10T08:12:15Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go

+			isNotZeroObject, err := hasUID(currentObject)
+			if err != nil {
+				return fmt.Errorf("unexpected error when extracting UID from oldObj: %v", err.Error())
+			} else if !isNotZeroObject {


misunderstanding I think. Even remove the upper logic. Just a comment: // if we allow create-on-patch, TODO: ...

@sttts Oops.. Indeed

sttts · 2018-07-10T09:18:02Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go

@@ -108,6 +108,7 @@ func PatchResource(r rest.Patcher, scope RequestScope, admit admission.Interface
 		userInfo, _ := request.UserFrom(ctx)
 		staticAdmissionAttributes := admission.NewAttributesRecord(nil, nil, scope.Kind, namespace, name, scope.Resource, scope.Subresource, admission.Update, userInfo)
 		admissionCheck := func(updatedObject runtime.Object, currentObject runtime.Object) error {
+			// if we allow this some day, we have this TODO: call the mutating admission chain with the CREATE verb instead of UPDATE


what is "this"? 🙂 needs some context, like "if we allow create-on-patch, we have this TODO: ..."

remove create-on-update logic for quota controller review: add more error check remove unused args revert changes in patch.go use hasUID to judge if it's a create-on-update

yue9944882 · 2018-07-10T12:58:56Z

/test pull-kubernetes-e2e-kops-aws

yue9944882 · 2018-07-12T09:22:07Z

@sttts PTAL

sttts · 2018-07-13T11:21:28Z

lgtm from my side.

/assign @deads2k

for final review and approval.

jennybuckley · 2018-07-16T21:44:32Z

It all looks good to me too, but I'll hold off on giving lgtm since @deads2k is planning to review this.

deads2k · 2018-07-19T14:38:33Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go

 			transformers = append(transformers, func(ctx context.Context, newObj, oldObj runtime.Object) (runtime.Object, error) {
-				return newObj, mutatingAdmission.Admit(admission.NewAttributesRecord(newObj, oldObj, scope.Kind, namespace, name, scope.Resource, scope.Subresource, admission.Update, userInfo))
+				isNotZeroObject, err := hasUID(oldObj)


The old admission code (at least for quota) used to compare against nil. Am I to understand the old code was wrong?

The old admission code (at least for quota) used to compare against nil. Am I to understand the old code was wrong?

Ah, it probably was for create on update, but not for create. That kind of makes sense. I'm glad this is getting fixed.

deads2k · 2018-07-19T14:43:40Z

/lgtm

k8s-github-robot · 2018-07-19T14:43:49Z

/test all

Tests are more than 96 hours old. Re-running tests.

k8s-ci-robot · 2018-07-19T14:44:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, yue9944882

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/controller/OWNERS~~ [deads2k]
~~pkg/quota/OWNERS~~ [deads2k]
~~plugin/pkg/admission/OWNERS~~ [deads2k]
~~staging/src/k8s.io/apiserver/OWNERS~~ [deads2k]

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

k8s-github-robot · 2018-07-19T16:29:39Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-07-19T17:42:54Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 28, 2018

k8s-ci-robot requested review from hzxuzhonghu and lavalamp June 28, 2018 08:40

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 28, 2018

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Jun 28, 2018

k8s-ci-robot requested a review from jennybuckley June 28, 2018 08:41

yue9944882 force-pushed the fixes-admission-operation-mismatch-for-create-on-update branch 3 times, most recently from 3c40cf1 to 043aec7 Compare June 28, 2018 10:45

jennybuckley reviewed Jun 28, 2018

View reviewed changes

yue9944882 force-pushed the fixes-admission-operation-mismatch-for-create-on-update branch from 043aec7 to 83dcd7d Compare June 29, 2018 04:08

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 29, 2018

yue9944882 force-pushed the fixes-admission-operation-mismatch-for-create-on-update branch from 83dcd7d to 4de1837 Compare June 29, 2018 04:43

yue9944882 commented Jun 29, 2018

View reviewed changes

k8s-ci-robot requested a review from adohe-zz June 29, 2018 14:08

k8s-ci-robot assigned lavalamp Jun 29, 2018

jennybuckley reviewed Jun 29, 2018

View reviewed changes

yue9944882 force-pushed the fixes-admission-operation-mismatch-for-create-on-update branch from 4de1837 to 7bccd3f Compare July 1, 2018 15:51

CaoShuFeng reviewed Jul 2, 2018

View reviewed changes

k8s-ci-robot assigned sttts Jul 2, 2018

yue9944882 mentioned this pull request Jul 5, 2018

[FEATURE BRANCH] Force allow create on patch if content type is apply #65723

Merged

yue9944882 force-pushed the fixes-admission-operation-mismatch-for-create-on-update branch from e1aa7aa to 1b10960 Compare July 6, 2018 12:33

sttts reviewed Jul 6, 2018

View reviewed changes

yue9944882 force-pushed the fixes-admission-operation-mismatch-for-create-on-update branch from 1b10960 to 7c8a9e8 Compare July 6, 2018 12:58

sttts reviewed Jul 9, 2018

View reviewed changes

yue9944882 force-pushed the fixes-admission-operation-mismatch-for-create-on-update branch from 7c8a9e8 to 4dc5807 Compare July 10, 2018 02:34

sttts reviewed Jul 10, 2018

View reviewed changes

yue9944882 force-pushed the fixes-admission-operation-mismatch-for-create-on-update branch from 4dc5807 to c6c66ea Compare July 10, 2018 08:20

sttts reviewed Jul 10, 2018

View reviewed changes

fixes operation for "create on update"

ccb1ec7

remove create-on-update logic for quota controller review: add more error check remove unused args revert changes in patch.go use hasUID to judge if it's a create-on-update

yue9944882 force-pushed the fixes-admission-operation-mismatch-for-create-on-update branch from c6c66ea to ccb1ec7 Compare July 10, 2018 09:22

k8s-ci-robot assigned deads2k Jul 13, 2018

deads2k reviewed Jul 19, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 19, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 19, 2018

k8s-github-robot merged commit 8770d12 into kubernetes:master Jul 19, 2018

liggitt mentioned this pull request Aug 10, 2018

split scc admission into mutating and validating openshift/origin#20605

Merged

fixes operation for "create on update" #65572

fixes operation for "create on update" #65572

Conversation

yue9944882 commented Jun 28, 2018 • edited Loading

yue9944882 commented Jun 28, 2018

yue9944882 commented Jun 28, 2018

jennybuckley left a comment

Choose a reason for hiding this comment

jennybuckley Jun 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yue9944882 Jun 29, 2018 • edited Loading

Choose a reason for hiding this comment

yue9944882 commented Jun 29, 2018

yue9944882 commented Jun 29, 2018

adohe-zz commented Jun 29, 2018

adohe-zz commented Jun 29, 2018

jennybuckley left a comment

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

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

yue9944882 Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Jul 2, 2018

sttts commented Jul 4, 2018

yue9944882 commented Jul 5, 2018

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

Choose a reason for hiding this comment

jennybuckley commented Jul 6, 2018 • edited Loading

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

sttts Jul 10, 2018 • edited Loading

Choose a reason for hiding this comment

yue9944882 commented Jul 10, 2018

yue9944882 commented Jul 12, 2018

sttts commented Jul 13, 2018

jennybuckley commented Jul 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Jul 19, 2018

k8s-github-robot commented Jul 19, 2018

k8s-ci-robot commented Jul 19, 2018

k8s-github-robot commented Jul 19, 2018

k8s-github-robot commented Jul 19, 2018

yue9944882 commented Jun 28, 2018 •

edited

Loading

jennybuckley Jun 28, 2018 •

edited

Loading

yue9944882 Jun 29, 2018 •

edited

Loading

yue9944882 Jul 2, 2018 •

edited

Loading

sttts Jul 2, 2018 •

edited

Loading

jennybuckley commented Jul 6, 2018 •

edited

Loading

sttts Jul 10, 2018 •

edited

Loading

jennybuckley commented Jul 16, 2018 •

edited

Loading