diff --git a/pkg/master/reconcilers/lease.go b/pkg/master/reconcilers/lease.go index 7a0d54080c7..25afeb72262 100644 --- a/pkg/master/reconcilers/lease.go +++ b/pkg/master/reconcilers/lease.go @@ -35,6 +35,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" kruntime "k8s.io/apimachinery/pkg/runtime" apirequest "k8s.io/apiserver/pkg/endpoints/request" + "k8s.io/apiserver/pkg/registry/rest" "k8s.io/apiserver/pkg/storage" corev1client "k8s.io/client-go/kubernetes/typed/core/v1" endpointsv1 "k8s.io/kubernetes/pkg/api/v1/endpoints" @@ -106,7 +107,7 @@ func (s *storageLeases) UpdateLease(ip string) error { // RemoveLease removes the lease on a master IP in storage func (s *storageLeases) RemoveLease(ip string) error { - return s.storage.Delete(apirequest.NewDefaultContext(), s.baseKey+"/"+ip, &corev1.Endpoints{}, nil) + return s.storage.Delete(apirequest.NewDefaultContext(), s.baseKey+"/"+ip, &corev1.Endpoints{}, nil, rest.ValidateAllObjectFunc) } // NewLeases creates a new etcd-based Leases implementation. diff --git a/pkg/registry/core/namespace/storage/storage.go b/pkg/registry/core/namespace/storage/storage.go index b76e48f01dd..7a7b3a646fe 100644 --- a/pkg/registry/core/namespace/storage/storage.go +++ b/pkg/registry/core/namespace/storage/storage.go @@ -162,10 +162,6 @@ func (r *REST) Delete(ctx context.Context, name string, deleteValidation rest.Va // upon first request to delete, we switch the phase to start namespace termination // TODO: enhance graceful deletion's calls to DeleteStrategy to allow phase change and finalizer patterns if namespace.DeletionTimestamp.IsZero() { - if err := deleteValidation(nsObj); err != nil { - return nil, false, err - } - key, err := r.store.KeyFunc(ctx, name) if err != nil { return nil, false, err @@ -182,6 +178,9 @@ func (r *REST) Delete(ctx context.Context, name string, deleteValidation rest.Va // wrong type return nil, fmt.Errorf("expected *api.Namespace, got %v", existing) } + if err := deleteValidation(existingNamespace); err != nil { + return nil, err + } // Set the deletion timestamp if needed if existingNamespace.DeletionTimestamp.IsZero() { now := metav1.Now() diff --git a/pkg/registry/core/namespace/storage/storage_test.go b/pkg/registry/core/namespace/storage/storage_test.go index f36b645ce39..0f2560be473 100644 --- a/pkg/registry/core/namespace/storage/storage_test.go +++ b/pkg/registry/core/namespace/storage/storage_test.go @@ -163,7 +163,7 @@ func TestDeleteNamespaceWithIncompleteFinalizers(t *testing.T) { t.Fatalf("unexpected error: %v", err) } if _, _, err := storage.Delete(ctx, "foo", rest.ValidateAllObjectFunc, nil); err == nil { - t.Errorf("unexpected error: %v", err) + t.Errorf("unexpected no error") } // should still exist _, err := storage.Get(ctx, "foo", &metav1.GetOptions{}) @@ -578,7 +578,7 @@ func TestDeleteWithGCFinalizers(t *testing.T) { } var obj runtime.Object var err error - if obj, _, err = storage.Delete(ctx, test.name, test.deleteOptions); err != nil { + if obj, _, err = storage.Delete(ctx, test.name, rest.ValidateAllObjectFunc, test.deleteOptions); err != nil { t.Fatalf("unexpected error: %v", err) } ns, ok := obj.(*api.Namespace) diff --git a/pkg/registry/core/pod/storage/storage_test.go b/pkg/registry/core/pod/storage/storage_test.go index 57d2804ffb5..f0f03aa33af 100644 --- a/pkg/registry/core/pod/storage/storage_test.go +++ b/pkg/registry/core/pod/storage/storage_test.go @@ -156,7 +156,7 @@ type FailDeletionStorage struct { Called *bool } -func (f FailDeletionStorage) Delete(ctx context.Context, key string, out runtime.Object, precondition *storage.Preconditions) error { +func (f FailDeletionStorage) Delete(ctx context.Context, key string, out runtime.Object, precondition *storage.Preconditions, _ storage.ValidateObjectFunc) error { *f.Called = true return storage.NewKeyNotFoundError(key, 0) } diff --git a/pkg/registry/core/service/storage/rest_test.go b/pkg/registry/core/service/storage/rest_test.go index db7f90e4aab..b2a995d718e 100644 --- a/pkg/registry/core/service/storage/rest_test.go +++ b/pkg/registry/core/service/storage/rest_test.go @@ -979,7 +979,7 @@ func TestServiceRegistryDeleteDryRun(t *testing.T) { if err != nil { t.Fatalf("Expected no error: %v", err) } - _, _, err = storage.Delete(ctx, svc.Name, &metav1.DeleteOptions{DryRun: []string{metav1.DryRunAll}}) + _, _, err = storage.Delete(ctx, svc.Name, rest.ValidateAllObjectFunc, &metav1.DeleteOptions{DryRun: []string{metav1.DryRunAll}}) if err != nil { t.Fatalf("Expected no error: %v", err) } @@ -1009,7 +1009,7 @@ func TestServiceRegistryDeleteDryRun(t *testing.T) { if err != nil { t.Fatalf("Expected no error: %v", err) } - _, _, err = storage.Delete(ctx, svc.Name, &metav1.DeleteOptions{DryRun: []string{metav1.DryRunAll}}) + _, _, err = storage.Delete(ctx, svc.Name, rest.ValidateAllObjectFunc, &metav1.DeleteOptions{DryRun: []string{metav1.DryRunAll}}) if err != nil { t.Fatalf("Expected no error: %v", err) } diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition/etcd.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition/etcd.go index 9d020cab437..6d36d37bddc 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition/etcd.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition/etcd.go @@ -103,10 +103,6 @@ func (r *REST) Delete(ctx context.Context, name string, deleteValidation rest.Va // upon first request to delete, add our finalizer and then delegate if crd.DeletionTimestamp.IsZero() { - if err := deleteValidation(obj); err != nil { - return nil, false, err - } - key, err := r.Store.KeyFunc(ctx, name) if err != nil { return nil, false, err @@ -123,6 +119,9 @@ func (r *REST) Delete(ctx context.Context, name string, deleteValidation rest.Va // wrong type return nil, fmt.Errorf("expected *apiextensions.CustomResourceDefinition, got %v", existing) } + if err := deleteValidation(existingCRD); err != nil { + return nil, err + } // Set the deletion timestamp if needed if existingCRD.DeletionTimestamp.IsZero() { diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go index a0cd62fb435..084987092ee 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go @@ -447,7 +447,7 @@ func (storage *SimpleRESTStorage) Delete(ctx context.Context, id string, deleteV if err := storage.errors["delete"]; err != nil { return nil, false, err } - if err := deleteValidation(nil); err != nil { + if err := deleteValidation(&storage.item); err != nil { return nil, false, err } var obj runtime.Object = &metav1.Status{Status: metav1.StatusSuccess} @@ -572,18 +572,6 @@ func (s *ConnecterRESTStorage) NewConnectOptions() (runtime.Object, bool, string return s.emptyConnectOptions, false, "" } -<<<<<<< HEAD -======= -type LegacyRESTStorage struct { - *SimpleRESTStorage -} - -func (storage LegacyRESTStorage) Delete(ctx context.Context, id string) (runtime.Object, error) { - obj, _, err := storage.SimpleRESTStorage.Delete(ctx, id, nil, nil) - return obj, err -} - ->>>>>>> ab07482ecb... validate deletion admission object type MetadataRESTStorage struct { *SimpleRESTStorage types []string diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/delete.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/delete.go index eef4b1f62b1..2ac2cc3f131 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/delete.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/delete.go @@ -180,7 +180,6 @@ func DeleteCollection(r rest.CollectionDeleter, checkBody bool, scope *RequestSc ctx := req.Context() ctx = request.WithNamespace(ctx, namespace) ae := request.AuditEventFrom(ctx) - admit = admission.WithAudit(admit, ae) outputMediaType, _, err := negotiation.NegotiateOutputMediaType(req, scope.Serializer, scope) if err != nil { @@ -252,6 +251,7 @@ func DeleteCollection(r rest.CollectionDeleter, checkBody bool, scope *RequestSc } options.TypeMeta.SetGroupVersionKind(metav1.SchemeGroupVersion.WithKind("DeleteOptions")) + admit = admission.WithAudit(admit, ae) userInfo, _ := request.UserFrom(ctx) staticAdmissionAttrs := admission.NewAttributesRecord(nil, nil, scope.Kind, namespace, "", scope.Resource, scope.Subresource, admission.Delete, options, dryrun.IsDryRun(options.DryRun), userInfo) result, err := finishRequest(timeout, func() (runtime.Object, error) { diff --git a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/dryrun.go b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/dryrun.go index 08046fa22cc..24a1b1ec288 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/dryrun.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/dryrun.go @@ -44,14 +44,17 @@ func (s *DryRunnableStorage) Create(ctx context.Context, key string, obj, out ru return s.Storage.Create(ctx, key, obj, out, ttl) } -func (s *DryRunnableStorage) Delete(ctx context.Context, key string, out runtime.Object, preconditions *storage.Preconditions, dryRun bool) error { +func (s *DryRunnableStorage) Delete(ctx context.Context, key string, out runtime.Object, preconditions *storage.Preconditions, deleteValidation storage.ValidateObjectFunc, dryRun bool) error { if dryRun { if err := s.Storage.Get(ctx, key, "", out, false); err != nil { return err } - return preconditions.Check(key, out) + if err := preconditions.Check(key, out); err != nil { + return err + } + return deleteValidation(out) } - return s.Storage.Delete(ctx, key, out, preconditions) + return s.Storage.Delete(ctx, key, out, preconditions, deleteValidation) } func (s *DryRunnableStorage) Watch(ctx context.Context, key string, resourceVersion string, p storage.SelectionPredicate) (watch.Interface, error) { diff --git a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/dryrun_test.go b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/dryrun_test.go index 8fbf219cfdb..9b1fbb18691 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/dryrun_test.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/dryrun_test.go @@ -29,6 +29,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" examplev1 "k8s.io/apiserver/pkg/apis/example/v1" + "k8s.io/apiserver/pkg/registry/rest" "k8s.io/apiserver/pkg/storage" etcdtesting "k8s.io/apiserver/pkg/storage/etcd/testing" "k8s.io/apiserver/pkg/storage/storagebackend/factory" @@ -233,7 +234,7 @@ func TestDryRunDeleteDoesntDelete(t *testing.T) { t.Fatalf("Failed to create new object: %v", err) } - err = s.Delete(context.Background(), "key", out, nil, true) + err = s.Delete(context.Background(), "key", out, nil, rest.ValidateAllObjectFunc, true) if err != nil { t.Fatalf("Failed to dry-run delete the object: %v", err) } @@ -249,7 +250,7 @@ func TestDryRunDeleteMissingObjectFails(t *testing.T) { defer destroy() out := UnstructuredOrDie(`{}`) - err := s.Delete(context.Background(), "key", out, nil, true) + err := s.Delete(context.Background(), "key", out, nil, rest.ValidateAllObjectFunc, true) if e, ok := err.(*storage.StorageError); !ok || e.Code != storage.ErrCodeKeyNotFound { t.Errorf("Expected key to be not found, error: %v", err) } @@ -269,7 +270,7 @@ func TestDryRunDeleteReturnsObject(t *testing.T) { out = UnstructuredOrDie(`{}`) expected := UnstructuredOrDie(`{"kind": "Pod", "metadata": {"resourceVersion": "2"}}`) - err = s.Delete(context.Background(), "key", out, nil, true) + err = s.Delete(context.Background(), "key", out, nil, rest.ValidateAllObjectFunc, true) if err != nil { t.Fatalf("Failed to delete with valid precondition: %v", err) } @@ -292,12 +293,12 @@ func TestDryRunDeletePreconditions(t *testing.T) { wrongID := types.UID("wrong-uid") myID := types.UID("my-uid") - err = s.Delete(context.Background(), "key", out, &storage.Preconditions{UID: &wrongID}, true) + err = s.Delete(context.Background(), "key", out, &storage.Preconditions{UID: &wrongID}, rest.ValidateAllObjectFunc, true) if e, ok := err.(*storage.StorageError); !ok || e.Code != storage.ErrCodeInvalidObj { t.Errorf("Expected invalid object, error: %v", err) } - err = s.Delete(context.Background(), "key", out, &storage.Preconditions{UID: &myID}, true) + err = s.Delete(context.Background(), "key", out, &storage.Preconditions{UID: &myID}, rest.ValidateAllObjectFunc, true) if err != nil { t.Fatalf("Failed to delete with valid precondition: %v", err) } 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 c8bbfe6316d..ff44081a7d7 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 @@ -24,7 +24,6 @@ import ( "sync" "time" - "k8s.io/apimachinery/pkg/api/errors" kubeerr "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/api/validation/path" @@ -427,7 +426,8 @@ func ShouldDeleteDuringUpdate(ctx context.Context, key string, obj, existing run func (e *Store) deleteWithoutFinalizers(ctx context.Context, name, key string, obj runtime.Object, preconditions *storage.Preconditions, dryRun bool) (runtime.Object, bool, error) { out := e.NewFunc() klog.V(6).Infof("going to delete %s from registry, triggered by update", name) - if err := e.Storage.Delete(ctx, key, out, preconditions, dryRun); err != nil { + // Using the rest.ValidateAllObjectFunc because the request is an UPDATE request and has already passed the admission for the UPDATE verb. + if err := e.Storage.Delete(ctx, key, out, preconditions, rest.ValidateAllObjectFunc, dryRun); err != nil { // Deletion is racy, i.e., there could be multiple update // requests to remove all finalizers from the object, so we // ignore the NotFound error. @@ -801,7 +801,7 @@ func markAsDeleting(obj runtime.Object, now time.Time) (err error) { // should be deleted immediately // 4. a new output object with the state that was updated // 5. a copy of the last existing state of the object -func (e *Store) updateForGracefulDeletionAndFinalizers(ctx context.Context, name, key string, options *metav1.DeleteOptions, preconditions storage.Preconditions, in runtime.Object) (err error, ignoreNotFound, deleteImmediately bool, out, lastExisting runtime.Object) { +func (e *Store) updateForGracefulDeletionAndFinalizers(ctx context.Context, name, key string, options *metav1.DeleteOptions, preconditions storage.Preconditions, deleteValidation rest.ValidateObjectFunc, in runtime.Object) (err error, ignoreNotFound, deleteImmediately bool, out, lastExisting runtime.Object) { lastGraceful := int64(0) var pendingFinalizers bool out = e.NewFunc() @@ -812,6 +812,9 @@ func (e *Store) updateForGracefulDeletionAndFinalizers(ctx context.Context, name false, /* ignoreNotFound */ &preconditions, storage.SimpleUpdate(func(existing runtime.Object) (runtime.Object, error) { + if err := deleteValidation(existing); err != nil { + return nil, err + } graceful, pendingGraceful, err := rest.BeforeDelete(e.DeleteStrategy, ctx, existing, options) if err != nil { return nil, err @@ -889,19 +892,8 @@ func (e *Store) Delete(ctx context.Context, name string, deleteValidation rest.V } obj := e.NewFunc() qualifiedResource := e.qualifiedResourceFromContext(ctx) - err = e.Storage.Get(ctx, key, "", obj, false) - if err != nil { - // Note that we continue the admission check on "Not Found" error is for the compatibility - // with the NodeRestriction admission controller. - if interpretedErr := storeerr.InterpretDeleteError(err, qualifiedResource, name); errors.IsNotFound(interpretedErr) { - if err := deleteValidation(obj); err != nil { - return nil, false, err - } - return nil, false, interpretedErr - } - } - if err := deleteValidation(obj.DeepCopyObject()); err != nil { - return nil, false, err + if err = e.Storage.Get(ctx, key, "", obj, false); err != nil { + return nil, false, storeerr.InterpretDeleteError(err, qualifiedResource, name) } // support older consumers of delete by treating "nil" as delete immediately @@ -937,7 +929,7 @@ func (e *Store) Delete(ctx context.Context, name string, deleteValidation rest.V shouldUpdateFinalizers, _ := deletionFinalizersForGarbageCollection(ctx, e, accessor, options) // TODO: remove the check, because we support no-op updates now. if graceful || pendingFinalizers || shouldUpdateFinalizers { - err, ignoreNotFound, deleteImmediately, out, lastExisting = e.updateForGracefulDeletionAndFinalizers(ctx, name, key, options, preconditions, obj) + err, ignoreNotFound, deleteImmediately, out, lastExisting = e.updateForGracefulDeletionAndFinalizers(ctx, name, key, options, preconditions, deleteValidation, obj) } // !deleteImmediately covers all cases where err != nil. We keep both to be future-proof. @@ -960,7 +952,7 @@ func (e *Store) Delete(ctx context.Context, name string, deleteValidation rest.V // delete immediately, or no graceful deletion supported klog.V(6).Infof("going to delete %s from registry: ", name) out = e.NewFunc() - if err := e.Storage.Delete(ctx, key, out, &preconditions, dryrun.IsDryRun(options.DryRun)); err != nil { + if err := e.Storage.Delete(ctx, key, out, &preconditions, storage.ValidateObjectFunc(deleteValidation), dryrun.IsDryRun(options.DryRun)); err != nil { // Please refer to the place where we set ignoreNotFound for the reason // why we ignore the NotFound error . if storage.IsNotFound(err) && ignoreNotFound && lastExisting != nil { @@ -992,12 +984,6 @@ func (e *Store) DeleteCollection(ctx context.Context, deleteValidation rest.Vali listOptions = listOptions.DeepCopy() } - if deleteValidation != nil { - if err := deleteValidation(nil); err != nil { - return nil, err - } - } - listObj, err := e.List(ctx, listOptions) if err != nil { return nil, err @@ -1044,7 +1030,7 @@ func (e *Store) DeleteCollection(ctx context.Context, deleteValidation rest.Vali errs <- err return } - if _, _, err := e.Delete(ctx, accessor.GetName(), rest.ValidateAllObjectFunc, options); err != nil && !kubeerr.IsNotFound(err) { + if _, _, err := e.Delete(ctx, accessor.GetName(), deleteValidation, options); err != nil && !kubeerr.IsNotFound(err) { klog.V(4).Infof("Delete %s in DeleteCollection failed: %v", accessor.GetName(), err) errs <- err return diff --git a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go index a919f193763..e8a46425e08 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go @@ -1885,7 +1885,7 @@ func TestDeleteWithCachedObject(t *testing.T) { t.Fatal(err) } // The object shouldn't be deleted, because the persisted object has pending finalizers. - _, _, err = registry.Delete(ctx, podName, nil) + _, _, err = registry.Delete(ctx, podName, rest.ValidateAllObjectFunc, nil) if err != nil { t.Fatal(err) } @@ -1895,3 +1895,87 @@ func TestDeleteWithCachedObject(t *testing.T) { t.Fatal(err) } } + +// TestRetryDeleteValidation checks if the deleteValidation is called again if +// the GuaranteedUpdate in the Delete handler conflicts with a simultaneous +// Update. +func TestRetryDeleteValidation(t *testing.T) { + testContext := genericapirequest.WithNamespace(genericapirequest.NewContext(), "test") + destroyFunc, registry := NewTestGenericStoreRegistry(t) + defer destroyFunc() + + tests := []struct { + pod *example.Pod + deleted bool + }{ + { + pod: &example.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "test", Finalizers: []string{"pending"}}, + Spec: example.PodSpec{NodeName: "machine"}, + }, + deleted: false, + }, + + { + pod: &example.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "bar", Namespace: "test"}, + Spec: example.PodSpec{NodeName: "machine"}, + }, + deleted: true, + }, + } + + for _, test := range tests { + ready := make(chan struct{}) + updated := make(chan struct{}) + var readyOnce, updatedOnce sync.Once + var called int + deleteValidation := func(runtime.Object) error { + readyOnce.Do(func() { + close(ready) + }) + // wait for the update completes + <-updated + called++ + return nil + } + + if _, err := registry.Create(testContext, test.pod, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + transformer := func(ctx context.Context, newObj runtime.Object, oldObj runtime.Object) (transformedNewObj runtime.Object, err error) { + <-ready + pod, ok := newObj.(*example.Pod) + if !ok { + t.Fatalf("unexpected object %v", newObj) + } + pod.Labels = map[string]string{ + "modified": "true", + } + return pod, nil + } + + go func() { + // This update will cause the Delete to retry due to conflict. + _, _, err := registry.Update(testContext, test.pod.Name, rest.DefaultUpdatedObjectInfo(test.pod, transformer), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{}) + if err != nil { + t.Fatal(err) + } + updatedOnce.Do(func() { + close(updated) + }) + }() + + _, deleted, err := registry.Delete(testContext, test.pod.Name, deleteValidation, &metav1.DeleteOptions{}) + if err != nil { + t.Fatal(err) + } + if a, e := deleted, test.deleted; a != e { + t.Fatalf("expected deleted to be %v, got %v", e, a) + } + if called != 2 { + t.Fatalf("expected deleteValidation to be called twice") + } + } +} diff --git a/staging/src/k8s.io/apiserver/pkg/registry/rest/delete.go b/staging/src/k8s.io/apiserver/pkg/registry/rest/delete.go index 661db668bf8..2b5038b210d 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/rest/delete.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/rest/delete.go @@ -147,27 +147,33 @@ func AdmissionToValidateObjectDeleteFunc(admit admission.Interface, staticAttrib mutatingAdmission, isMutatingAdmission := admit.(admission.MutationInterface) validatingAdmission, isValidatingAdmission := admit.(admission.ValidationInterface) + mutating := isMutatingAdmission && mutatingAdmission.Handles(staticAttributes.GetOperation()) + validating := isValidatingAdmission && validatingAdmission.Handles(staticAttributes.GetOperation()) + return func(old runtime.Object) error { - if !isMutatingAdmission && !isValidatingAdmission { + if !mutating && !validating { return nil } finalAttributes := admission.NewAttributesRecord( nil, - old, + // Deep copy the object to avoid accidentally changing the object. + old.DeepCopyObject(), staticAttributes.GetKind(), staticAttributes.GetNamespace(), staticAttributes.GetName(), staticAttributes.GetResource(), staticAttributes.GetSubresource(), staticAttributes.GetOperation(), + staticAttributes.GetOperationOptions(), + staticAttributes.IsDryRun(), staticAttributes.GetUserInfo(), ) - if isMutatingAdmission && mutatingAdmission.Handles(finalAttributes.GetOperation()) { + if mutating { if err := mutatingAdmission.Admit(finalAttributes, objInterfaces); err != nil { return err } } - if isValidatingAdmission && validatingAdmission.Handles(finalAttributes.GetOperation()) { + if validating { if err := validatingAdmission.Validate(finalAttributes, objInterfaces); err != nil { return err } diff --git a/staging/src/k8s.io/apiserver/pkg/registry/rest/rest.go b/staging/src/k8s.io/apiserver/pkg/registry/rest/rest.go index e4336fb52d6..08c7cafc6b7 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/rest/rest.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/rest/rest.go @@ -148,6 +148,7 @@ type TableConvertor interface { // RESTful object. type GracefulDeleter interface { // Delete finds a resource in the storage and deletes it. + // The delete attempt is validated by the deleteValidation first. // If options are provided, the resource will attempt to honor them or return an invalid // request error. // Although it can return an arbitrary error value, IsNotFound(err) is true for the @@ -163,8 +164,9 @@ type GracefulDeleter interface { // of RESTful resources. type CollectionDeleter interface { // DeleteCollection selects all resources in the storage matching given 'listOptions' - // and deletes them. If 'options' are provided, the resource will attempt to honor - // them or return an invalid request error. + // and deletes them. The delete attempt is validated by the deleteValidation first. + // If 'options' are provided, the resource will attempt to honor them or return an + // invalid request error. // DeleteCollection may not be atomic - i.e. it may delete some objects and still // return an error after it. On success, returns a list of deleted objects. DeleteCollection(ctx context.Context, deleteValidation ValidateObjectFunc, options *metav1.DeleteOptions, listOptions *metainternalversion.ListOptions) (runtime.Object, error) diff --git a/staging/src/k8s.io/apiserver/pkg/registry/rest/resttest/resttest.go b/staging/src/k8s.io/apiserver/pkg/registry/rest/resttest/resttest.go index 7e8f9f2a20b..ccf6c74b91d 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/rest/resttest/resttest.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/rest/resttest/resttest.go @@ -923,14 +923,14 @@ func (t *Tester) testDeleteWithResourceVersion(obj runtime.Object, createFn Crea t.Errorf("unexpected error: %v", err) } opts.Preconditions = metav1.NewRVDeletionPrecondition("RV1111").Preconditions - obj, wasDeleted, err := t.storage.(rest.GracefulDeleter).Delete(ctx, objectMeta.GetName(), &opts) + obj, wasDeleted, err := t.storage.(rest.GracefulDeleter).Delete(ctx, objectMeta.GetName(), rest.ValidateAllObjectFunc, &opts) if err == nil || !errors.IsConflict(err) { t.Errorf("unexpected error: %v", err) } if wasDeleted { t.Errorf("unexpected, object %s should not have been deleted immediately", objectMeta.GetName()) } - obj, _, err = t.storage.(rest.GracefulDeleter).Delete(ctx, objectMeta.GetName(), metav1.NewRVDeletionPrecondition("RV0000")) + obj, _, err = t.storage.(rest.GracefulDeleter).Delete(ctx, objectMeta.GetName(), rest.ValidateAllObjectFunc, metav1.NewRVDeletionPrecondition("RV0000")) if err != nil { t.Errorf("unexpected error: %v", err) } @@ -962,7 +962,7 @@ func (t *Tester) testDeleteDryRunGracefulHasdefault(obj runtime.Object, createFn t.Errorf("unexpected error: %v", err) } objectMeta := t.getObjectMetaOrFail(foo) - object, wasDeleted, err := t.storage.(rest.GracefulDeleter).Delete(ctx, objectMeta.GetName(), &metav1.DeleteOptions{DryRun: []string{metav1.DryRunAll}}) + object, wasDeleted, err := t.storage.(rest.GracefulDeleter).Delete(ctx, objectMeta.GetName(), rest.ValidateAllObjectFunc, &metav1.DeleteOptions{DryRun: []string{metav1.DryRunAll}}) if err != nil { t.Errorf("unexpected error: %v", err) } @@ -973,7 +973,7 @@ func (t *Tester) testDeleteDryRunGracefulHasdefault(obj runtime.Object, createFn if objectMeta.GetDeletionTimestamp() == nil || objectMeta.GetDeletionGracePeriodSeconds() == nil || *objectMeta.GetDeletionGracePeriodSeconds() != expectedGrace { t.Errorf("unexpected deleted meta: %#v", objectMeta) } - _, _, err = t.storage.(rest.GracefulDeleter).Delete(ctx, objectMeta.GetName(), &metav1.DeleteOptions{}) + _, _, err = t.storage.(rest.GracefulDeleter).Delete(ctx, objectMeta.GetName(), rest.ValidateAllObjectFunc, &metav1.DeleteOptions{}) if err != nil { t.Errorf("unexpected error: %v", err) } diff --git a/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher.go b/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher.go index 51c3d36ca7b..c51c4b86404 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher.go @@ -402,8 +402,8 @@ func (c *Cacher) Create(ctx context.Context, key string, obj, out runtime.Object } // Delete implements storage.Interface. -func (c *Cacher) Delete(ctx context.Context, key string, out runtime.Object, preconditions *storage.Preconditions) error { - return c.storage.Delete(ctx, key, out, preconditions) +func (c *Cacher) Delete(ctx context.Context, key string, out runtime.Object, preconditions *storage.Preconditions, validateDeletion storage.ValidateObjectFunc) error { + return c.storage.Delete(ctx, key, out, preconditions, validateDeletion) } // Watch implements storage.Interface. diff --git a/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher_whitebox_test.go b/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher_whitebox_test.go index f2a2326f3f6..b305c96920a 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher_whitebox_test.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher_whitebox_test.go @@ -299,7 +299,7 @@ func (d *dummyStorage) Versioner() storage.Versioner { return nil } func (d *dummyStorage) Create(_ context.Context, _ string, _, _ runtime.Object, _ uint64) error { return fmt.Errorf("unimplemented") } -func (d *dummyStorage) Delete(_ context.Context, _ string, _ runtime.Object, _ *storage.Preconditions) error { +func (d *dummyStorage) Delete(_ context.Context, _ string, _ runtime.Object, _ *storage.Preconditions, _ storage.ValidateObjectFunc) error { return fmt.Errorf("unimplemented") } func (d *dummyStorage) Watch(_ context.Context, _ string, _ string, _ storage.SelectionPredicate) (watch.Interface, error) { diff --git a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go index 5a349c7a3eb..349c0a5e4dc 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go @@ -181,44 +181,16 @@ func (s *store) Create(ctx context.Context, key string, obj, out runtime.Object, } // Delete implements storage.Interface.Delete. -func (s *store) Delete(ctx context.Context, key string, out runtime.Object, preconditions *storage.Preconditions) error { +func (s *store) Delete(ctx context.Context, key string, out runtime.Object, preconditions *storage.Preconditions, validateDeletion storage.ValidateObjectFunc) error { v, err := conversion.EnforcePtr(out) if err != nil { panic("unable to convert output object to pointer") } key = path.Join(s.pathPrefix, key) - if preconditions == nil { - return s.unconditionalDelete(ctx, key, out) - } - return s.conditionalDelete(ctx, key, out, v, preconditions) + return s.conditionalDelete(ctx, key, out, v, preconditions, validateDeletion) } -func (s *store) unconditionalDelete(ctx context.Context, key string, out runtime.Object) error { - // We need to do get and delete in single transaction in order to - // know the value and revision before deleting it. - startTime := time.Now() - txnResp, err := s.client.KV.Txn(ctx).If().Then( - clientv3.OpGet(key), - clientv3.OpDelete(key), - ).Commit() - metrics.RecordEtcdRequestLatency("delete", getTypeName(out), startTime) - if err != nil { - return err - } - getResp := txnResp.Responses[0].GetResponseRange() - if len(getResp.Kvs) == 0 { - return storage.NewKeyNotFoundError(key, 0) - } - - kv := getResp.Kvs[0] - data, _, err := s.transformer.TransformFromStorage(kv.Value, authenticatedDataString(key)) - if err != nil { - return storage.NewInternalError(err.Error()) - } - return decode(s.codec, s.versioner, data, out, kv.ModRevision) -} - -func (s *store) conditionalDelete(ctx context.Context, key string, out runtime.Object, v reflect.Value, preconditions *storage.Preconditions) error { +func (s *store) conditionalDelete(ctx context.Context, key string, out runtime.Object, v reflect.Value, preconditions *storage.Preconditions, validateDeletion storage.ValidateObjectFunc) error { startTime := time.Now() getResp, err := s.client.KV.Get(ctx, key) metrics.RecordEtcdRequestLatency("get", getTypeName(out), startTime) @@ -230,7 +202,12 @@ func (s *store) conditionalDelete(ctx context.Context, key string, out runtime.O if err != nil { return err } - if err := preconditions.Check(key, origState.obj); err != nil { + if preconditions != nil { + if err := preconditions.Check(key, origState.obj); err != nil { + return err + } + } + if err := validateDeletion(origState.obj); err != nil { return err } startTime := time.Now() diff --git a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store_test.go b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store_test.go index 4d4c1e158ca..65627fbbe41 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store_test.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store_test.go @@ -74,7 +74,7 @@ func (p prefixTransformer) TransformFromStorage(b []byte, ctx value.Context) ([] panic("no context provided") } if !bytes.HasPrefix(b, p.prefix) { - return nil, false, fmt.Errorf("value does not have expected prefix: %s", string(b)) + return nil, false, fmt.Errorf("value does not have expected prefix %q: %s,", p.prefix, string(b)) } return bytes.TrimPrefix(b, p.prefix), p.stale, p.err } @@ -241,7 +241,7 @@ func TestUnconditionalDelete(t *testing.T) { for i, tt := range tests { out := &example.Pod{} // reset - err := store.Delete(ctx, tt.key, out, nil) + err := store.Delete(ctx, tt.key, out, nil, storage.ValidateAllObjectFunc) if tt.expectNotFoundErr { if err == nil || !storage.IsNotFound(err) { t.Errorf("#%d: expecting not found error, but get: %s", i, err) @@ -275,7 +275,7 @@ func TestConditionalDelete(t *testing.T) { for i, tt := range tests { out := &example.Pod{} - err := store.Delete(ctx, key, out, tt.precondition) + err := store.Delete(ctx, key, out, tt.precondition, storage.ValidateAllObjectFunc) if tt.expectInvalidObjErr { if err == nil || !storage.IsInvalidObj(err) { t.Errorf("#%d: expecting invalid UID error, but get: %s", i, err) @@ -740,12 +740,11 @@ func TestTransformationFailure(t *testing.T) { t.Errorf("Unexpected error: %v", err) } - // Delete succeeds but reports an error because we cannot access the body - if err := store.Delete(ctx, preset[1].key, &example.Pod{}, nil); !storage.IsInternalError(err) { + // Delete fails with internal error. + if err := store.Delete(ctx, preset[1].key, &example.Pod{}, nil, storage.ValidateAllObjectFunc); !storage.IsInternalError(err) { t.Errorf("Unexpected error: %v", err) } - - if err := store.Get(ctx, preset[1].key, "", &example.Pod{}, false); !storage.IsNotFound(err) { + if err := store.Get(ctx, preset[1].key, "", &example.Pod{}, false); !storage.IsInternalError(err) { t.Errorf("Unexpected error: %v", err) } } @@ -1349,7 +1348,7 @@ func testSetup(t *testing.T) (context.Context, *store, *integration.ClusterV3) { func testPropogateStore(ctx context.Context, t *testing.T, store *store, obj *example.Pod) (string, *example.Pod) { // Setup store with a key and grab the output for returning. key := "/testkey" - err := store.unconditionalDelete(ctx, key, &example.Pod{}) + err := store.conditionalDelete(ctx, key, &example.Pod{}, reflect.ValueOf(example.Pod{}), nil, storage.ValidateAllObjectFunc) if err != nil && !storage.IsNotFound(err) { t.Fatalf("Cleanup failed: %v", err) } diff --git a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/watcher_test.go b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/watcher_test.go index a8438038850..243eebc9b9a 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/watcher_test.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/watcher_test.go @@ -135,7 +135,7 @@ func TestDeleteTriggerWatch(t *testing.T) { if err != nil { t.Fatalf("Watch failed: %v", err) } - if err := store.Delete(ctx, key, &example.Pod{}, nil); err != nil { + if err := store.Delete(ctx, key, &example.Pod{}, nil, storage.ValidateAllObjectFunc); err != nil { t.Fatalf("Delete failed: %v", err) } testCheckEventType(t, watch.Deleted, w) @@ -295,7 +295,7 @@ func TestWatchDeleteEventObjectHaveLatestRV(t *testing.T) { } etcdW := cluster.RandClient().Watch(ctx, "/", clientv3.WithPrefix()) - if err := store.Delete(ctx, key, &example.Pod{}, &storage.Preconditions{}); err != nil { + if err := store.Delete(ctx, key, &example.Pod{}, &storage.Preconditions{}, storage.ValidateAllObjectFunc); err != nil { t.Fatalf("Delete failed: %v", err) } diff --git a/staging/src/k8s.io/apiserver/pkg/storage/interfaces.go b/staging/src/k8s.io/apiserver/pkg/storage/interfaces.go index f2a1f105373..1e78be8da5f 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/interfaces.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/interfaces.go @@ -95,6 +95,16 @@ var Everything = SelectionPredicate{ // See the comment for GuaranteedUpdate for more details. type UpdateFunc func(input runtime.Object, res ResponseMeta) (output runtime.Object, ttl *uint64, err error) +// ValidateObjectFunc is a function to act on a given object. An error may be returned +// if the hook cannot be completed. The function may NOT transform the provided +// object. +type ValidateObjectFunc func(obj runtime.Object) error + +// ValidateAllObjectFunc is a "admit everything" instance of ValidateObjectFunc. +func ValidateAllObjectFunc(obj runtime.Object) error { + return nil +} + // Preconditions must be fulfilled before an operation (update, delete, etc.) is carried out. type Preconditions struct { // Specifies the target UID. @@ -153,7 +163,7 @@ type Interface interface { // Delete removes the specified key and returns the value that existed at that spot. // If key didn't exist, it will return NotFound storage error. - Delete(ctx context.Context, key string, out runtime.Object, preconditions *Preconditions) error + Delete(ctx context.Context, key string, out runtime.Object, preconditions *Preconditions, validateDeletion ValidateObjectFunc) error // Watch begins watching the specified key. Events are decoded into API objects, // and any items selected by 'p' are sent down to returned watch.Interface. diff --git a/staging/src/k8s.io/apiserver/pkg/storage/tests/cacher_test.go b/staging/src/k8s.io/apiserver/pkg/storage/tests/cacher_test.go index 0f151688b68..f50def6315c 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/tests/cacher_test.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/tests/cacher_test.go @@ -259,7 +259,7 @@ func TestList(t *testing.T) { updatePod(t, etcdStorage, podFooNS2, nil) deleted := example.Pod{} - if err := etcdStorage.Delete(context.TODO(), "pods/ns/bar", &deleted, nil); err != nil { + if err := etcdStorage.Delete(context.TODO(), "pods/ns/bar", &deleted, nil, storage.ValidateAllObjectFunc); err != nil { t.Errorf("Unexpected error: %v", err) } @@ -521,7 +521,7 @@ func TestFiltering(t *testing.T) { _ = updatePod(t, etcdStorage, podFooPrime, fooUnfiltered) deleted := example.Pod{} - if err := etcdStorage.Delete(context.TODO(), "pods/ns/foo", &deleted, nil); err != nil { + if err := etcdStorage.Delete(context.TODO(), "pods/ns/foo", &deleted, nil, storage.ValidateAllObjectFunc); err != nil { t.Errorf("Unexpected error: %v", err) } diff --git a/test/e2e/apimachinery/webhook.go b/test/e2e/apimachinery/webhook.go index fa05c07cd01..ed25afe80f4 100644 --- a/test/e2e/apimachinery/webhook.go +++ b/test/e2e/apimachinery/webhook.go @@ -73,19 +73,20 @@ const ( crdWebhookConfigName = "e2e-test-webhook-config-crd" slowWebhookConfigName = "e2e-test-webhook-config-slow" - skipNamespaceLabelKey = "skip-webhook-admission" - skipNamespaceLabelValue = "yes" - skippedNamespaceName = "exempted-namesapce" - disallowedPodName = "disallowed-pod" - toBeAttachedPodName = "to-be-attached-pod" - hangingPodName = "hanging-pod" - disallowedConfigMapName = "disallowed-configmap" - allowedConfigMapName = "allowed-configmap" - failNamespaceLabelKey = "fail-closed-webhook" - failNamespaceLabelValue = "yes" - failNamespaceName = "fail-closed-namesapce" - addedLabelKey = "added-label" - addedLabelValue = "yes" + skipNamespaceLabelKey = "skip-webhook-admission" + skipNamespaceLabelValue = "yes" + skippedNamespaceName = "exempted-namesapce" + disallowedPodName = "disallowed-pod" + toBeAttachedPodName = "to-be-attached-pod" + hangingPodName = "hanging-pod" + disallowedConfigMapName = "disallowed-configmap" + nonDeletableConfigmapName = "nondeletable-configmap" + allowedConfigMapName = "allowed-configmap" + failNamespaceLabelKey = "fail-closed-webhook" + failNamespaceLabelValue = "yes" + failNamespaceName = "fail-closed-namesapce" + addedLabelKey = "added-label" + addedLabelValue = "yes" ) var serverWebhookVersion = utilversion.MustParseSemantic("v1.8.0") @@ -136,7 +137,7 @@ var _ = SIGDescribe("AdmissionWebhook", func() { testAttachingPodWebhook(f) }) - ginkgo.It("Should be able to deny custom resource creation", func() { + ginkgo.It("Should be able to deny custom resource creation and deletion", func() { testcrd, err := crd.CreateTestCRD(f) if err != nil { return @@ -145,6 +146,7 @@ var _ = SIGDescribe("AdmissionWebhook", func() { webhookCleanup := registerWebhookForCustomResource(f, context, testcrd) defer webhookCleanup() testCustomResourceWebhook(f, testcrd.Crd, testcrd.DynamicClients["v1"]) + testBlockingCustomResourceDeletion(f, testcrd.Crd, testcrd.DynamicClients["v1"]) }) ginkgo.It("Should unconditionally reject operations on fail closed webhook", func() { @@ -458,7 +460,7 @@ func registerWebhook(f *framework.Framework, context *certContext) func() { { Name: "deny-unwanted-configmap-data.k8s.io", Rules: []v1beta1.RuleWithOperations{{ - Operations: []v1beta1.OperationType{v1beta1.Create, v1beta1.Update}, + Operations: []v1beta1.OperationType{v1beta1.Create, v1beta1.Update, v1beta1.Delete}, Rule: v1beta1.Rule{ APIGroups: []string{""}, APIVersions: []string{"v1"}, @@ -788,6 +790,36 @@ func testWebhook(f *framework.Framework) { framework.ExpectNoError(err, "failed to create configmap %s in namespace: %s", configmap.Name, skippedNamespaceName) } +func testBlockingConfigmapDeletion(f *framework.Framework) { + ginkgo.By("create a configmap that should be denied by the webhook when deleting") + client := f.ClientSet + configmap := nonDeletableConfigmap(f) + _, err := client.CoreV1().ConfigMaps(f.Namespace.Name).Create(configmap) + gomega.Expect(err).NotTo(gomega.HaveOccurred(), "failed to create configmap %s in namespace: %s", configmap.Name, f.Namespace.Name) + + ginkgo.By("deleting the configmap should be denied by the webhook") + err = client.CoreV1().ConfigMaps(f.Namespace.Name).Delete(configmap.Name, &metav1.DeleteOptions{}) + gomega.Expect(err).To(gomega.HaveOccurred(), "deleting configmap %s in namespace: %s should be denied", configmap.Name, f.Namespace.Name) + expectedErrMsg1 := "the configmap cannot be deleted because it contains unwanted key and value" + if !strings.Contains(err.Error(), expectedErrMsg1) { + framework.Failf("expect error contains %q, got %q", expectedErrMsg1, err.Error()) + } + + ginkgo.By("remove the offending key and value from the configmap data") + toCompliantFn := func(cm *v1.ConfigMap) { + if cm.Data == nil { + cm.Data = map[string]string{} + } + cm.Data["webhook-e2e-test"] = "webhook-allow" + } + _, err = updateConfigMap(client, f.Namespace.Name, configmap.Name, toCompliantFn) + gomega.Expect(err).NotTo(gomega.HaveOccurred(), "failed to update configmap %s in namespace: %s", configmap.Name, f.Namespace.Name) + + ginkgo.By("deleting the updated configmap should be successful") + err = client.CoreV1().ConfigMaps(f.Namespace.Name).Delete(configmap.Name, &metav1.DeleteOptions{}) + gomega.Expect(err).NotTo(gomega.HaveOccurred(), "failed to delete configmap %s in namespace: %s", configmap.Name, f.Namespace.Name) +} + func testAttachingPodWebhook(f *framework.Framework) { ginkgo.By("create a pod") client := f.ClientSet @@ -1187,6 +1219,17 @@ func nonCompliantConfigMap(f *framework.Framework) *v1.ConfigMap { } } +func nonDeletableConfigmap(f *framework.Framework) *v1.ConfigMap { + return &v1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: nonDeletableConfigmapName, + }, + Data: map[string]string{ + "webhook-e2e-test": "webhook-nondeletable", + }, + } +} + func toBeMutatedConfigMap(f *framework.Framework) *v1.ConfigMap { return &v1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ @@ -1224,6 +1267,28 @@ func updateConfigMap(c clientset.Interface, ns, name string, update updateConfig return cm, pollErr } +type updateCustomResourceFn func(cm *unstructured.Unstructured) + +func updateCustomResource(c dynamic.ResourceInterface, ns, name string, update updateCustomResourceFn) (*unstructured.Unstructured, error) { + var cr *unstructured.Unstructured + pollErr := wait.PollImmediate(2*time.Second, 1*time.Minute, func() (bool, error) { + var err error + if cr, err = c.Get(name, metav1.GetOptions{}); err != nil { + return false, err + } + update(cr) + if cr, err = c.Update(cr, metav1.UpdateOptions{}); err == nil { + return true, nil + } + // Only retry update on conflict + if !errors.IsConflict(err) { + return false, err + } + return false, nil + }) + return cr, pollErr +} + func cleanWebhookTest(client clientset.Interface, namespaceName string) { _ = client.CoreV1().Services(namespaceName).Delete(serviceName, nil) _ = client.AppsV1().Deployments(namespaceName).Delete(deploymentName, nil) @@ -1245,7 +1310,7 @@ func registerWebhookForCustomResource(f *framework.Framework, context *certConte { Name: "deny-unwanted-custom-resource-data.k8s.io", Rules: []v1beta1.RuleWithOperations{{ - Operations: []v1beta1.OperationType{v1beta1.Create, v1beta1.Update}, + Operations: []v1beta1.OperationType{v1beta1.Create, v1beta1.Update, v1beta1.Delete}, Rule: v1beta1.Rule{ APIGroups: []string{testcrd.Crd.Spec.Group}, APIVersions: servedAPIVersions(testcrd.Crd), @@ -1358,6 +1423,50 @@ func testCustomResourceWebhook(f *framework.Framework, crd *apiextensionsv1beta1 } } +func testBlockingCustomResourceDeletion(f *framework.Framework, crd *apiextensionsv1beta1.CustomResourceDefinition, customResourceClient dynamic.ResourceInterface) { + ginkgo.By("Creating a custom resource whose deletion would be denied by the webhook") + crInstanceName := "cr-instance-2" + crInstance := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "kind": crd.Spec.Names.Kind, + "apiVersion": crd.Spec.Group + "/" + crd.Spec.Version, + "metadata": map[string]interface{}{ + "name": crInstanceName, + "namespace": f.Namespace.Name, + }, + "data": map[string]interface{}{ + "webhook-e2e-test": "webhook-nondeletable", + }, + }, + } + _, err := customResourceClient.Create(crInstance, metav1.CreateOptions{}) + gomega.Expect(err).NotTo(gomega.HaveOccurred(), "failed to create custom resource %s in namespace: %s", crInstanceName, f.Namespace.Name) + + ginkgo.By("Deleting the custom resource should be denied") + err = customResourceClient.Delete(crInstanceName, &metav1.DeleteOptions{}) + gomega.Expect(err).To(gomega.HaveOccurred(), "deleting custom resource %s in namespace: %s should be denied", crInstanceName, f.Namespace.Name) + expectedErrMsg1 := "the custom resource cannot be deleted because it contains unwanted key and value" + if !strings.Contains(err.Error(), expectedErrMsg1) { + framework.Failf("expect error contains %q, got %q", expectedErrMsg1, err.Error()) + } + + ginkgo.By("Remove the offending key and value from the custom resource data") + toCompliantFn := func(cr *unstructured.Unstructured) { + if _, ok := cr.Object["data"]; !ok { + cr.Object["data"] = map[string]interface{}{} + } + data := cr.Object["data"].(map[string]interface{}) + data["webhook-e2e-test"] = "webhook-allow" + } + _, err = updateCustomResource(customResourceClient, f.Namespace.Name, crInstanceName, toCompliantFn) + gomega.Expect(err).NotTo(gomega.HaveOccurred(), "failed to update custom resource %s in namespace: %s", crInstanceName, f.Namespace.Name) + + ginkgo.By("Deleting the updated custom resource should be successful") + err = customResourceClient.Delete(crInstanceName, &metav1.DeleteOptions{}) + gomega.Expect(err).NotTo(gomega.HaveOccurred(), "failed to delete custom resource %s in namespace: %s", crInstanceName, f.Namespace.Name) + +} + func testMutatingCustomResourceWebhook(f *framework.Framework, crd *apiextensionsv1beta1.CustomResourceDefinition, customResourceClient dynamic.ResourceInterface, prune bool) { ginkgo.By("Creating a custom resource that should be mutated by the webhook") crName := "cr-instance-1" diff --git a/test/images/webhook/VERSION b/test/images/webhook/VERSION index 42e1b6f2255..951143a81a2 100644 --- a/test/images/webhook/VERSION +++ b/test/images/webhook/VERSION @@ -1 +1 @@ -1.14v1 +1.15v1 diff --git a/test/images/webhook/configmap.go b/test/images/webhook/configmap.go index 25ba7f4efa3..0a700446d5b 100644 --- a/test/images/webhook/configmap.go +++ b/test/images/webhook/configmap.go @@ -41,7 +41,12 @@ func admitConfigMaps(ar v1beta1.AdmissionReview) *v1beta1.AdmissionResponse { return nil } - raw := ar.Request.Object.Raw + var raw []byte + if ar.Request.Operation == v1beta1.Delete { + raw = ar.Request.OldObject.Raw + } else { + raw = ar.Request.Object.Raw + } configmap := corev1.ConfigMap{} deserializer := codecs.UniversalDeserializer() if _, _, err := deserializer.Decode(raw, nil, &configmap); err != nil { @@ -51,12 +56,19 @@ func admitConfigMaps(ar v1beta1.AdmissionReview) *v1beta1.AdmissionResponse { reviewResponse := v1beta1.AdmissionResponse{} reviewResponse.Allowed = true for k, v := range configmap.Data { - if k == "webhook-e2e-test" && v == "webhook-disallow" { + if k == "webhook-e2e-test" && v == "webhook-disallow" && + (ar.Request.Operation == v1beta1.Create || ar.Request.Operation == v1beta1.Update) { reviewResponse.Allowed = false reviewResponse.Result = &metav1.Status{ Reason: "the configmap contains unwanted key and value", } } + if k == "webhook-e2e-test" && v == "webhook-nondeletable" && ar.Request.Operation == v1beta1.Delete { + reviewResponse.Allowed = false + reviewResponse.Result = &metav1.Status{ + Reason: "the configmap cannot be deleted because it contains unwanted key and value", + } + } } return &reviewResponse } diff --git a/test/images/webhook/customresource.go b/test/images/webhook/customresource.go index 53e235f3fa1..d14b036a0f3 100644 --- a/test/images/webhook/customresource.go +++ b/test/images/webhook/customresource.go @@ -69,7 +69,12 @@ func admitCustomResource(ar v1beta1.AdmissionReview) *v1beta1.AdmissionResponse Data map[string]string }{} - raw := ar.Request.Object.Raw + var raw []byte + if ar.Request.Operation == v1beta1.Delete { + raw = ar.Request.OldObject.Raw + } else { + raw = ar.Request.Object.Raw + } err := json.Unmarshal(raw, &cr) if err != nil { klog.Error(err) @@ -79,12 +84,19 @@ func admitCustomResource(ar v1beta1.AdmissionReview) *v1beta1.AdmissionResponse reviewResponse := v1beta1.AdmissionResponse{} reviewResponse.Allowed = true for k, v := range cr.Data { - if k == "webhook-e2e-test" && v == "webhook-disallow" { + if k == "webhook-e2e-test" && v == "webhook-disallow" && + (ar.Request.Operation == v1beta1.Create || ar.Request.Operation == v1beta1.Update) { reviewResponse.Allowed = false reviewResponse.Result = &metav1.Status{ Reason: "the custom resource contains unwanted data", } } + if k == "webhook-e2e-test" && v == "webhook-nondeletable" && ar.Request.Operation == v1beta1.Delete { + reviewResponse.Allowed = false + reviewResponse.Result = &metav1.Status{ + Reason: "the custom resource cannot be deleted because it contains unwanted key and value", + } + } } return &reviewResponse } diff --git a/test/integration/apiserver/admissionwebhook/admission_test.go b/test/integration/apiserver/admissionwebhook/admission_test.go index f9069eef522..4d47adcad7b 100644 --- a/test/integration/apiserver/admissionwebhook/admission_test.go +++ b/test/integration/apiserver/admissionwebhook/admission_test.go @@ -527,6 +527,7 @@ func testResourcePatch(c *testContext) { } func testResourceDelete(c *testContext) { + // Verify that an immediate delete triggers the webhook and populates the admisssionRequest.oldObject. obj, err := createOrGetResource(c.client, c.gvr, c.resource) if err != nil { c.t.Error(err) @@ -534,12 +535,13 @@ func testResourceDelete(c *testContext) { } background := metav1.DeletePropagationBackground zero := int64(0) - c.admissionHolder.expect(c.gvr, gvk(c.resource.Group, c.resource.Version, c.resource.Kind), gvkDeleteOptions, v1beta1.Delete, obj.GetName(), obj.GetNamespace(), false, false, true) + c.admissionHolder.expect(c.gvr, gvk(c.resource.Group, c.resource.Version, c.resource.Kind), gvkDeleteOptions, v1beta1.Delete, obj.GetName(), obj.GetNamespace(), false, true, true) err = c.client.Resource(c.gvr).Namespace(obj.GetNamespace()).Delete(obj.GetName(), &metav1.DeleteOptions{GracePeriodSeconds: &zero, PropagationPolicy: &background}) if err != nil { c.t.Error(err) return } + c.admissionHolder.verify(c.t) // wait for the item to be gone err = wait.PollImmediate(100*time.Millisecond, 10*time.Second, func() (bool, error) { @@ -557,6 +559,77 @@ func testResourceDelete(c *testContext) { c.t.Error(err) return } + + // Verify that an update-on-delete triggers the webhook and populates the admisssionRequest.oldObject. + obj, err = createOrGetResource(c.client, c.gvr, c.resource) + if err != nil { + c.t.Error(err) + return + } + // Adding finalizer to the object, then deleting it. + // We don't add finalizers by setting DeleteOptions.PropagationPolicy + // because some resource (e.g., events) do not support garbage + // collector finalizers. + _, err = c.client.Resource(c.gvr).Namespace(obj.GetNamespace()).Patch( + obj.GetName(), + types.MergePatchType, + []byte(`{"metadata":{"finalizers":["test/k8s.io"]}}`), + metav1.PatchOptions{}) + if err != nil { + c.t.Error(err) + return + } + c.admissionHolder.expect(c.gvr, gvk(c.resource.Group, c.resource.Version, c.resource.Kind), gvkDeleteOptions, v1beta1.Delete, obj.GetName(), obj.GetNamespace(), false, true, true) + err = c.client.Resource(c.gvr).Namespace(obj.GetNamespace()).Delete(obj.GetName(), &metav1.DeleteOptions{GracePeriodSeconds: &zero, PropagationPolicy: &background}) + if err != nil { + c.t.Error(err) + return + } + c.admissionHolder.verify(c.t) + + // wait other finalizers (e.g., crd's customresourcecleanup finalizer) to be removed. + err = wait.PollImmediate(100*time.Millisecond, 10*time.Second, func() (bool, error) { + obj, err := c.client.Resource(c.gvr).Namespace(obj.GetNamespace()).Get(obj.GetName(), metav1.GetOptions{}) + if err != nil { + return false, err + } + finalizers := obj.GetFinalizers() + if len(finalizers) != 1 { + c.t.Logf("waiting for other finalizers on %#v %s to be removed, existing finalizers are %v", c.gvr, obj.GetName(), obj.GetFinalizers()) + return false, nil + } + if finalizers[0] != "test/k8s.io" { + return false, fmt.Errorf("expected the single finalizer on %#v %s to be test/k8s.io, got %v", c.gvr, obj.GetName(), obj.GetFinalizers()) + } + return true, nil + }) + + // remove the finalizer + _, err = c.client.Resource(c.gvr).Namespace(obj.GetNamespace()).Patch( + obj.GetName(), + types.MergePatchType, + []byte(`{"metadata":{"finalizers":[]}}`), + metav1.PatchOptions{}) + if err != nil { + c.t.Error(err) + return + } + // wait for the item to be gone + err = wait.PollImmediate(100*time.Millisecond, 10*time.Second, func() (bool, error) { + obj, err := c.client.Resource(c.gvr).Namespace(obj.GetNamespace()).Get(obj.GetName(), metav1.GetOptions{}) + if errors.IsNotFound(err) { + return true, nil + } + if err == nil { + c.t.Logf("waiting for %#v to be deleted (name: %s, finalizers: %v)...\n", c.gvr, obj.GetName(), obj.GetFinalizers()) + return false, nil + } + return false, err + }) + if err != nil { + c.t.Error(err) + return + } } func testResourceDeletecollection(c *testContext) { @@ -580,7 +653,7 @@ func testResourceDeletecollection(c *testContext) { } // set expectations - c.admissionHolder.expect(c.gvr, gvk(c.resource.Group, c.resource.Version, c.resource.Kind), gvkDeleteOptions, v1beta1.Delete, "", obj.GetNamespace(), false, false, true) + c.admissionHolder.expect(c.gvr, gvk(c.resource.Group, c.resource.Version, c.resource.Kind), gvkDeleteOptions, v1beta1.Delete, "", obj.GetNamespace(), false, true, true) // delete err = c.client.Resource(c.gvr).Namespace(obj.GetNamespace()).DeleteCollection(&metav1.DeleteOptions{GracePeriodSeconds: &zero, PropagationPolicy: &background}, metav1.ListOptions{LabelSelector: "webhooktest=true"}) @@ -708,7 +781,7 @@ func testNamespaceDelete(c *testContext) { background := metav1.DeletePropagationBackground zero := int64(0) - c.admissionHolder.expect(c.gvr, gvk(c.resource.Group, c.resource.Version, c.resource.Kind), gvkDeleteOptions, v1beta1.Delete, obj.GetName(), obj.GetNamespace(), false, false, true) + c.admissionHolder.expect(c.gvr, gvk(c.resource.Group, c.resource.Version, c.resource.Kind), gvkDeleteOptions, v1beta1.Delete, obj.GetName(), obj.GetNamespace(), false, true, true) err = c.client.Resource(c.gvr).Namespace(obj.GetNamespace()).Delete(obj.GetName(), &metav1.DeleteOptions{GracePeriodSeconds: &zero, PropagationPolicy: &background}) if err != nil { c.t.Error(err) diff --git a/test/utils/image/manifest.go b/test/utils/image/manifest.go index e1eea1a98b8..2e547833d87 100644 --- a/test/utils/image/manifest.go +++ b/test/utils/image/manifest.go @@ -196,7 +196,7 @@ const ( func initImageConfigs() map[int]Config { configs := map[int]Config{} configs[CRDConversionWebhook] = Config{e2eRegistry, "crd-conversion-webhook", "1.13rev2"} - configs[AdmissionWebhook] = Config{e2eRegistry, "webhook", "1.14v1"} + configs[AdmissionWebhook] = Config{e2eRegistry, "webhook", "1.15v1"} configs[Agnhost] = Config{e2eRegistry, "agnhost", "1.0"} configs[APIServer] = Config{e2eRegistry, "sample-apiserver", "1.10"} configs[AppArmorLoader] = Config{e2eRegistry, "apparmor-loader", "1.0"}