From bd961781d7fa14f065b127001a98251ed27697fc Mon Sep 17 00:00:00 2001 From: Kevin Date: Sun, 7 Jun 2020 13:13:53 +0000 Subject: [PATCH] prevent update handler being called on disallowed CreateOnUpdate --- .../pkg/registry/generic/registry/store.go | 29 ++++++++-------- .../apiserver/apply/apply_crd_test.go | 33 +++++++++++++++++++ test/integration/auth/node_test.go | 4 ++- 3 files changed, 52 insertions(+), 14 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go index f50d1a0b32a..64a5ab3f3d8 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go @@ -473,6 +473,16 @@ func (e *Store) Update(ctx context.Context, name string, objInfo rest.UpdatedObj // deleteObj is only used in case a deletion is carried out var deleteObj runtime.Object err = e.Storage.GuaranteedUpdate(ctx, key, out, true, storagePreconditions, func(existing runtime.Object, res storage.ResponseMeta) (runtime.Object, *uint64, error) { + existingResourceVersion, err := e.Storage.Versioner().ObjectResourceVersion(existing) + if err != nil { + return nil, nil, err + } + if existingResourceVersion == 0 { + if !e.UpdateStrategy.AllowCreateOnUpdate() && !forceAllowCreate { + return nil, nil, apierrors.NewNotFound(qualifiedResource, name) + } + } + // Given the existing object, get the new object obj, err := objInfo.UpdatedObject(ctx, existing) if err != nil { @@ -483,20 +493,13 @@ func (e *Store) Update(ctx context.Context, name string, objInfo rest.UpdatedObj // the user does not have a resource version, then we populate it with // the latest version. Else, we check that the version specified by // the user matches the version of latest storage object. - resourceVersion, err := e.Storage.Versioner().ObjectResourceVersion(obj) + newResourceVersion, err := e.Storage.Versioner().ObjectResourceVersion(obj) if err != nil { return nil, nil, err } - doUnconditionalUpdate := resourceVersion == 0 && e.UpdateStrategy.AllowUnconditionalUpdate() + doUnconditionalUpdate := newResourceVersion == 0 && e.UpdateStrategy.AllowUnconditionalUpdate() - version, err := e.Storage.Versioner().ObjectResourceVersion(existing) - if err != nil { - return nil, nil, err - } - if version == 0 { - if !e.UpdateStrategy.AllowCreateOnUpdate() && !forceAllowCreate { - return nil, nil, apierrors.NewNotFound(qualifiedResource, name) - } + if existingResourceVersion == 0 { creating = true creatingObj = obj if err := rest.BeforeCreate(e.CreateStrategy, ctx, obj); err != nil { @@ -529,15 +532,15 @@ func (e *Store) Update(ctx context.Context, name string, objInfo rest.UpdatedObj } else { // Check if the object's resource version matches the latest // resource version. - if resourceVersion == 0 { + if newResourceVersion == 0 { // TODO: The Invalid error should have a field for Resource. // After that field is added, we should fill the Resource and // leave the Kind field empty. See the discussion in #18526. qualifiedKind := schema.GroupKind{Group: qualifiedResource.Group, Kind: qualifiedResource.Resource} - fieldErrList := field.ErrorList{field.Invalid(field.NewPath("metadata").Child("resourceVersion"), resourceVersion, "must be specified for an update")} + fieldErrList := field.ErrorList{field.Invalid(field.NewPath("metadata").Child("resourceVersion"), newResourceVersion, "must be specified for an update")} return nil, nil, apierrors.NewInvalid(qualifiedKind, name, fieldErrList) } - if resourceVersion != version { + if newResourceVersion != existingResourceVersion { return nil, nil, apierrors.NewConflict(qualifiedResource, name, fmt.Errorf(OptimisticLockErrorMsg)) } } diff --git a/test/integration/apiserver/apply/apply_crd_test.go b/test/integration/apiserver/apply/apply_crd_test.go index cb91612e0af..ee0d9cfce98 100644 --- a/test/integration/apiserver/apply/apply_crd_test.go +++ b/test/integration/apiserver/apply/apply_crd_test.go @@ -369,6 +369,39 @@ spec: t.Fatalf("failed to add a new list item to the object as a different applier: %v:\n%v", err, string(result)) } verifyNumPorts(t, result, 2) + + // UpdateOnCreate + notExistingYAMLBody := []byte(fmt.Sprintf(` + { + "apiVersion": "%s", + "kind": "%s", + "metadata": { + "name": "%s", + "finalizers": [ + "test-finalizer" + ] + }, + "spec": { + "cronSpec": "* * * * */5", + "replicas": 1, + "ports": [ + { + "name": "x", + "containerPort": 80 + } + ] + }, + "protocol": "TCP" + }`, apiVersion, kind, "should-not-exist")) + _, err = rest.Put(). + AbsPath("/apis", noxuDefinition.Spec.Group, noxuDefinition.Spec.Version, noxuDefinition.Spec.Names.Plural). + Name("should-not-exist"). + Param("fieldManager", "apply_test"). + Body(notExistingYAMLBody). + DoRaw(context.TODO()) + if !apierrors.IsNotFound(err) { + t.Fatalf("create on update should fail with notFound, got %v", err) + } } // TestApplyCRDNonStructuralSchema tests that when a CRD has a non-structural schema in its validation field, diff --git a/test/integration/auth/node_test.go b/test/integration/auth/node_test.go index d978c065a40..4527bf3d198 100644 --- a/test/integration/auth/node_test.go +++ b/test/integration/auth/node_test.go @@ -480,7 +480,7 @@ func TestNodeAuthorizer(t *testing.T) { expectNotFound(t, deleteNode2MirrorPod(node1Client)) expectNotFound(t, createNode2MirrorPodEviction(node1Client)) expectForbidden(t, createNode2(node1Client)) - expectForbidden(t, updateNode2Status(node1Client)) + expectNotFound(t, updateNode2Status(node1Client)) expectForbidden(t, deleteNode2(node1Client)) // related object requests from node2 fail @@ -500,6 +500,8 @@ func TestNodeAuthorizer(t *testing.T) { expectAllowed(t, updateNode2Status(node2Client)) // self deletion is not allowed expectForbidden(t, deleteNode2(node2Client)) + // modification of another node's status is not allowed + expectForbidden(t, updateNode2Status(node1Client)) // clean up node2 expectAllowed(t, deleteNode2(superuserClient))