From e90c2bd7dcf506dee91f30ea35b3c72f83a38fba Mon Sep 17 00:00:00 2001 From: deads2k Date: Mon, 11 Jan 2016 12:54:26 -0500 Subject: [PATCH] make patch call update admission chain after applying the patch --- pkg/apiserver/resthandler.go | 35 +++++++----- pkg/apiserver/resthandler_test.go | 94 ++++++++++++++++++++++++++++++- 2 files changed, 114 insertions(+), 15 deletions(-) diff --git a/pkg/apiserver/resthandler.go b/pkg/apiserver/resthandler.go index 15a637c6b9..41032066af 100644 --- a/pkg/apiserver/resthandler.go +++ b/pkg/apiserver/resthandler.go @@ -466,21 +466,9 @@ func PatchResource(r rest.Patcher, scope RequestScope, typer runtime.ObjectTyper return } - obj := r.New() ctx := scope.ContextFunc(req) ctx = api.WithNamespace(ctx, namespace) - // PATCH requires same permission as UPDATE - if admit.Handles(admission.Update) { - userInfo, _ := api.UserFrom(ctx) - - err = admit.Admit(admission.NewAttributesRecord(obj, scope.Kind.GroupKind(), namespace, name, scope.Resource.GroupResource(), scope.Subresource, admission.Update, userInfo)) - if err != nil { - errorJSON(err, scope.Codec, w) - return - } - } - versionedObj, err := converter.ConvertToVersion(r.New(), scope.Kind.GroupVersion().String()) if err != nil { errorJSON(err, scope.Codec, w) @@ -500,7 +488,16 @@ func PatchResource(r rest.Patcher, scope RequestScope, typer runtime.ObjectTyper return } - result, err := patchResource(ctx, timeout, versionedObj, r, name, patchType, patchJS, scope.Namer, scope.Codec) + updateAdmit := func(updatedObject runtime.Object) error { + if admit != nil && admit.Handles(admission.Update) { + userInfo, _ := api.UserFrom(ctx) + return admit.Admit(admission.NewAttributesRecord(updatedObject, scope.Kind.GroupKind(), namespace, name, scope.Resource.GroupResource(), scope.Subresource, admission.Update, userInfo)) + } + + return nil + } + + result, err := patchResource(ctx, updateAdmit, timeout, versionedObj, r, name, patchType, patchJS, scope.Namer, scope.Codec) if err != nil { errorJSON(err, scope.Codec, w) return @@ -516,8 +513,10 @@ func PatchResource(r rest.Patcher, scope RequestScope, typer runtime.ObjectTyper } +type updateAdmissionFunc func(updatedObject runtime.Object) error + // patchResource divides PatchResource for easier unit testing -func patchResource(ctx api.Context, timeout time.Duration, versionedObj runtime.Object, patcher rest.Patcher, name string, patchType api.PatchType, patchJS []byte, namer ScopeNamer, codec runtime.Codec) (runtime.Object, error) { +func patchResource(ctx api.Context, admit updateAdmissionFunc, timeout time.Duration, versionedObj runtime.Object, patcher rest.Patcher, name string, patchType api.PatchType, patchJS []byte, namer ScopeNamer, codec runtime.Codec) (runtime.Object, error) { namespace := api.NamespaceValue(ctx) original, err := patcher.Get(ctx, name) @@ -543,6 +542,10 @@ func patchResource(ctx api.Context, timeout time.Duration, versionedObj runtime. } return finishRequest(timeout, func() (runtime.Object, error) { + if err := admit(objToUpdate); err != nil { + return nil, err + } + // update should never create as previous get would fail updateObject, _, updateErr := patcher.Update(ctx, objToUpdate) for i := 0; i < MaxPatchConflicts && (errors.IsConflict(updateErr)); i++ { @@ -597,6 +600,10 @@ func patchResource(ctx api.Context, timeout time.Duration, versionedObj runtime. return nil, err } + if err := admit(objToUpdate); err != nil { + return nil, err + } + updateObject, _, updateErr = patcher.Update(ctx, objToUpdate) } diff --git a/pkg/apiserver/resthandler_test.go b/pkg/apiserver/resthandler_test.go index 1870ed389d..ee9545aaca 100644 --- a/pkg/apiserver/resthandler_test.go +++ b/pkg/apiserver/resthandler_test.go @@ -133,6 +133,9 @@ func (p *testNamer) GenerateListLink(req *restful.Request) (path, query string, type patchTestCase struct { name string + // admission chain to use, nil is fine + admit updateAdmissionFunc + // startingPod is used for the first Get startingPod *api.Pod // changedPod is the "destination" pod for the patch. The test will create a patch from the startingPod to the changedPod @@ -153,6 +156,12 @@ func (tc *patchTestCase) Run(t *testing.T) { name := tc.startingPod.Name codec := latest.GroupOrDie(api.GroupName).Codec + admit := tc.admit + if admit == nil { + admit = func(updatedObject runtime.Object) error { + return nil + } + } testPatcher := &testPatcher{} testPatcher.startingPod = tc.startingPod @@ -208,7 +217,7 @@ func (tc *patchTestCase) Run(t *testing.T) { } - resultObj, err := patchResource(ctx, 1*time.Second, versionedObj, testPatcher, name, patchType, patch, namer, codec) + resultObj, err := patchResource(ctx, admit, 1*time.Second, versionedObj, testPatcher, name, patchType, patch, namer, codec) if len(tc.expectedError) != 0 { if err == nil || err.Error() != tc.expectedError { t.Errorf("%s: expected error %v, but got %v", tc.name, tc.expectedError, err) @@ -329,3 +338,86 @@ func TestPatchResourceWithConflict(t *testing.T) { tc.Run(t) } + +func TestPatchWithAdmissionRejection(t *testing.T) { + namespace := "bar" + name := "foo" + fifteen := int64(15) + thirty := int64(30) + + tc := &patchTestCase{ + name: "TestPatchWithAdmissionRejection", + + admit: func(updatedObject runtime.Object) error { + return errors.New("admission failure") + }, + + startingPod: &api.Pod{}, + changedPod: &api.Pod{}, + updatePod: &api.Pod{}, + + expectedError: "admission failure", + } + + tc.startingPod.Name = name + tc.startingPod.Namespace = namespace + tc.startingPod.ResourceVersion = "1" + tc.startingPod.APIVersion = "v1" + tc.startingPod.Spec.ActiveDeadlineSeconds = &fifteen + + tc.changedPod.Name = name + tc.changedPod.Namespace = namespace + tc.changedPod.ResourceVersion = "1" + tc.changedPod.APIVersion = "v1" + tc.changedPod.Spec.ActiveDeadlineSeconds = &thirty + + tc.Run(t) +} + +func TestPatchWithVersionConflictThenAdmissionFailure(t *testing.T) { + namespace := "bar" + name := "foo" + fifteen := int64(15) + thirty := int64(30) + seen := false + + tc := &patchTestCase{ + name: "TestPatchWithVersionConflictThenAdmissionFailure", + + admit: func(updatedObject runtime.Object) error { + if seen { + return errors.New("admission failure") + } + + seen = true + return nil + }, + + startingPod: &api.Pod{}, + changedPod: &api.Pod{}, + updatePod: &api.Pod{}, + + expectedError: "admission failure", + } + + tc.startingPod.Name = name + tc.startingPod.Namespace = namespace + tc.startingPod.ResourceVersion = "1" + tc.startingPod.APIVersion = "v1" + tc.startingPod.Spec.ActiveDeadlineSeconds = &fifteen + + tc.changedPod.Name = name + tc.changedPod.Namespace = namespace + tc.changedPod.ResourceVersion = "1" + tc.changedPod.APIVersion = "v1" + tc.changedPod.Spec.ActiveDeadlineSeconds = &thirty + + tc.updatePod.Name = name + tc.updatePod.Namespace = namespace + tc.updatePod.ResourceVersion = "2" + tc.updatePod.APIVersion = "v1" + tc.updatePod.Spec.ActiveDeadlineSeconds = &fifteen + tc.updatePod.Spec.NodeName = "anywhere" + + tc.Run(t) +}