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 0ccca911c82..c164ca2ba08 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 @@ -53,6 +53,9 @@ import ( // FinishFunc is a function returned by Begin hooks to complete an operation. type FinishFunc func(ctx context.Context, success bool) +// AfterDeleteFunc is the type used for the Store.AfterDelete hook. +type AfterDeleteFunc func(obj runtime.Object, options *metav1.DeleteOptions) + // GenericStore interface can be used for type assertions when we need to access the underlying strategies. type GenericStore interface { GetCreateStrategy() rest.RESTCreateStrategy @@ -61,10 +64,11 @@ type GenericStore interface { GetExportStrategy() rest.RESTExportStrategy } -// Store implements pkg/api/rest.StandardStorage. It's intended to be -// embeddable and allows the consumer to implement any non-generic functions -// that are required. This object is intended to be copyable so that it can be -// used in different ways but share the same underlying behavior. +// Store implements k8s.io/apiserver/pkg/registry/rest.StandardStorage. It's +// intended to be embeddable and allows the consumer to implement any +// non-generic functions that are required. This object is intended to be +// copyable so that it can be used in different ways but share the same +// underlying behavior. // // All fields are required unless specified. // @@ -173,7 +177,7 @@ type Store struct { DeleteStrategy rest.RESTDeleteStrategy // AfterDelete implements a further operation to run after a resource is // deleted and before it is decorated, optional. - AfterDelete func(runtime.Object) + AfterDelete AfterDeleteFunc // ReturnDeletedObject determines whether the Store returns the object // that was deleted. Otherwise, return a generic success status response. ReturnDeletedObject bool @@ -453,16 +457,16 @@ func ShouldDeleteDuringUpdate(ctx context.Context, key string, obj, existing run // deleteWithoutFinalizers handles deleting an object ignoring its finalizer list. // Used for objects that are either been finalized or have never initialized. -func (e *Store) deleteWithoutFinalizers(ctx context.Context, name, key string, obj runtime.Object, preconditions *storage.Preconditions, dryRun bool) (runtime.Object, bool, error) { +func (e *Store) deleteWithoutFinalizers(ctx context.Context, name, key string, obj runtime.Object, preconditions *storage.Preconditions, options *metav1.DeleteOptions) (runtime.Object, bool, error) { out := e.NewFunc() klog.V(6).Infof("going to delete %s from registry, triggered by update", name) // 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, nil); err != nil { + if err := e.Storage.Delete(ctx, key, out, preconditions, rest.ValidateAllObjectFunc, dryrun.IsDryRun(options.DryRun), nil); 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. if storage.IsNotFound(err) { - _, err := e.finalizeDelete(ctx, obj, true) + _, err := e.finalizeDelete(ctx, obj, true, options) // clients are expecting an updated object if a PUT succeeded, // but finalizeDelete returns a metav1.Status, so return // the object in the request instead. @@ -470,7 +474,7 @@ func (e *Store) deleteWithoutFinalizers(ctx context.Context, name, key string, o } return nil, false, storeerr.InterpretDeleteError(err, e.qualifiedResourceFromContext(ctx), name) } - _, err := e.finalizeDelete(ctx, out, true) + _, err := e.finalizeDelete(ctx, out, true, options) // clients are expecting an updated object if a PUT succeeded, but // finalizeDelete returns a metav1.Status, so return the object in // the request instead. @@ -642,7 +646,7 @@ func (e *Store) Update(ctx context.Context, name string, objInfo rest.UpdatedObj if err != nil { // delete the object if err == errEmptiedFinalizers { - return e.deleteWithoutFinalizers(ctx, name, key, deleteObj, storagePreconditions, dryrun.IsDryRun(options.DryRun)) + return e.deleteWithoutFinalizers(ctx, name, key, deleteObj, storagePreconditions, newDeleteOptionsFromUpdateOptions(options)) } if creating { err = storeerr.InterpretCreateError(err, qualifiedResource, name) @@ -679,6 +683,16 @@ func newCreateOptionsFromUpdateOptions(in *metav1.UpdateOptions) *metav1.CreateO return co } +// This is a helper to convert UpdateOptions to DeleteOptions for the +// delete-on-update path. +func newDeleteOptionsFromUpdateOptions(in *metav1.UpdateOptions) *metav1.DeleteOptions { + do := &metav1.DeleteOptions{ + DryRun: in.DryRun, + } + do.TypeMeta.SetGroupVersionKind(metav1.SchemeGroupVersion.WithKind("DeleteOptions")) + return do +} + // Get retrieves the item from storage. func (e *Store) Get(ctx context.Context, name string, options *metav1.GetOptions) (runtime.Object, error) { obj := e.NewFunc() @@ -951,7 +965,7 @@ func (e *Store) updateForGracefulDeletionAndFinalizers(ctx context.Context, name // we should fall through and truly delete the object. return nil, false, true, out, lastExisting case errAlreadyDeleting: - out, err = e.finalizeDelete(ctx, in, true) + out, err = e.finalizeDelete(ctx, in, true, options) return err, false, false, out, lastExisting default: return storeerr.InterpretUpdateError(err, e.qualifiedResourceFromContext(ctx), name), false, false, out, lastExisting @@ -985,7 +999,7 @@ func (e *Store) Delete(ctx context.Context, name string, deleteValidation rest.V } // this means finalizers cannot be updated via DeleteOptions if a deletion is already pending if pendingGraceful { - out, err := e.finalizeDelete(ctx, obj, false) + out, err := e.finalizeDelete(ctx, obj, false, options) return out, false, err } // check if obj has pending finalizers @@ -1041,12 +1055,12 @@ func (e *Store) Delete(ctx context.Context, name string, deleteValidation rest.V if storage.IsNotFound(err) && ignoreNotFound && lastExisting != nil { // The lastExisting object may not be the last state of the object // before its deletion, but it's the best approximation. - out, err := e.finalizeDelete(ctx, lastExisting, true) + out, err := e.finalizeDelete(ctx, lastExisting, true, options) return out, true, err } return nil, false, storeerr.InterpretDeleteError(err, qualifiedResource, name) } - out, err = e.finalizeDelete(ctx, out, true) + out, err = e.finalizeDelete(ctx, out, true, options) return out, true, err } @@ -1144,9 +1158,9 @@ func (e *Store) DeleteCollection(ctx context.Context, deleteValidation rest.Vali // finalizeDelete runs the Store's AfterDelete hook if runHooks is set and // returns the decorated deleted object if appropriate. -func (e *Store) finalizeDelete(ctx context.Context, obj runtime.Object, runHooks bool) (runtime.Object, error) { +func (e *Store) finalizeDelete(ctx context.Context, obj runtime.Object, runHooks bool, options *metav1.DeleteOptions) (runtime.Object, error) { if runHooks && e.AfterDelete != nil { - e.AfterDelete(obj) + e.AfterDelete(obj, options) } if e.ReturnDeletedObject { if e.Decorator != nil { 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 83a59dfe8a7..a47d521107c 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 @@ -420,6 +420,66 @@ func TestNewCreateOptionsFromUpdateOptions(t *testing.T) { } } +func TestNewDeleteOptionsFromUpdateOptions(t *testing.T) { + f := fuzz.New().NilChance(0.0).NumElements(1, 1) + + // The goal here is to trigger when any changes are made to either + // DeleteOptions or UpdateOptions types, so we can update the converter. + for i := 0; i < 20; i++ { + in := &metav1.UpdateOptions{} + f.Fuzz(in) + in.TypeMeta.SetGroupVersionKind(metav1.SchemeGroupVersion.WithKind("DeleteOptions")) + + out := newDeleteOptionsFromUpdateOptions(in) + + // This sequence is intending to elide type information, but produce an + // intermediate structure (map) that can be manually patched up to make + // the comparison work as needed. + + // Convert both structs to maps of primitives. + inBytes, err := json.Marshal(in) + if err != nil { + t.Fatalf("failed to json.Marshal(in): %v", err) + } + outBytes, err := json.Marshal(out) + if err != nil { + t.Fatalf("failed to json.Marshal(out): %v", err) + } + inMap := map[string]interface{}{} + if err := json.Unmarshal(inBytes, &inMap); err != nil { + t.Fatalf("failed to json.Unmarshal(in): %v", err) + } + outMap := map[string]interface{}{} + if err := json.Unmarshal(outBytes, &outMap); err != nil { + t.Fatalf("failed to json.Unmarshal(out): %v", err) + } + + // Patch the maps to handle any expected differences before we compare. + + // DeleteOptions does not have these fields. + delete(inMap, "fieldManager") + + // UpdateOptions does not have these fields. + delete(outMap, "gracePeriodSeconds") + delete(outMap, "preconditions") + delete(outMap, "orphanDependents") + delete(outMap, "propagationPolicy") + + // Compare the results. + inBytes, err = json.Marshal(inMap) + if err != nil { + t.Fatalf("failed to json.Marshal(in): %v", err) + } + outBytes, err = json.Marshal(outMap) + if err != nil { + t.Fatalf("failed to json.Marshal(out): %v", err) + } + if i, o := string(inBytes), string(outBytes); i != o { + t.Fatalf("output != input:\n want: %s\n got: %s", i, o) + } + } +} + func TestStoreCreateHooks(t *testing.T) { // To track which hooks were called in what order. Not all hooks can // mutate the object. @@ -1231,11 +1291,19 @@ func TestStoreDelete(t *testing.T) { destroyFunc, registry := NewTestGenericStoreRegistry(t) defer destroyFunc() + afterWasCalled := false + registry.AfterDelete = func(obj runtime.Object, options *metav1.DeleteOptions) { + afterWasCalled = true + } + // test failure condition _, _, err := registry.Delete(testContext, podA.Name, rest.ValidateAllObjectFunc, nil) if !errors.IsNotFound(err) { t.Errorf("Unexpected error: %v", err) } + if afterWasCalled { + t.Errorf("Unexpected call to AfterDelete") + } // create pod _, err = registry.Create(testContext, podA, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) @@ -1251,6 +1319,9 @@ func TestStoreDelete(t *testing.T) { if !wasDeleted { t.Errorf("unexpected, pod %s should have been deleted immediately", podA.Name) } + if !afterWasCalled { + t.Errorf("Expected call to AfterDelete, but got none") + } // try to get a item which should be deleted _, err = registry.Get(testContext, podA.Name, &metav1.GetOptions{}) @@ -1366,11 +1437,18 @@ func TestGracefulStoreHandleFinalizers(t *testing.T) { registry.DeleteStrategy = testGracefulStrategy{defaultDeleteStrategy} defer destroyFunc() + afterWasCalled := false + registry.AfterDelete = func(obj runtime.Object, options *metav1.DeleteOptions) { + afterWasCalled = true + } + gcStates := []bool{true, false} for _, gcEnabled := range gcStates { t.Logf("garbage collection enabled: %t", gcEnabled) registry.EnableGarbageCollection = gcEnabled + afterWasCalled = false // reset + // create pod _, err := registry.Create(testContext, podWithFinalizer, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) if err != nil { @@ -1385,6 +1463,9 @@ func TestGracefulStoreHandleFinalizers(t *testing.T) { if wasDeleted { t.Errorf("unexpected, pod %s should not have been deleted immediately", podWithFinalizer.Name) } + if afterWasCalled { + t.Errorf("unexpected, AfterDelete() was called") + } _, err = registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{}) if err != nil { t.Fatalf("Unexpected error: %v", err) @@ -1398,6 +1479,9 @@ func TestGracefulStoreHandleFinalizers(t *testing.T) { if err != nil { t.Fatalf("Unexpected error: %v", err) } + if afterWasCalled { + t.Errorf("unexpected, AfterDelete() was called") + } // the object should still exist, because it still has a finalizer _, err = registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{}) @@ -1413,6 +1497,9 @@ func TestGracefulStoreHandleFinalizers(t *testing.T) { if err != nil { t.Fatalf("Unexpected error: %v", err) } + if !afterWasCalled { + t.Errorf("unexpected, AfterDelete() was not called") + } // the pod should be removed, because its finalizer is removed _, err = registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{}) if !errors.IsNotFound(err) { @@ -1430,13 +1517,20 @@ func TestNonGracefulStoreHandleFinalizers(t *testing.T) { testContext := genericapirequest.WithNamespace(genericapirequest.NewContext(), "test") destroyFunc, registry := NewTestGenericStoreRegistry(t) - defer destroyFunc() + + afterWasCalled := false + registry.AfterDelete = func(obj runtime.Object, options *metav1.DeleteOptions) { + afterWasCalled = true + } + gcStates := []bool{true, false} for _, gcEnabled := range gcStates { t.Logf("garbage collection enabled: %t", gcEnabled) registry.EnableGarbageCollection = gcEnabled + afterWasCalled = false // reset + // create pod _, err := registry.Create(testContext, podWithFinalizer, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) if err != nil { @@ -1451,6 +1545,9 @@ func TestNonGracefulStoreHandleFinalizers(t *testing.T) { if wasDeleted { t.Errorf("unexpected, pod %s should not have been deleted immediately", podWithFinalizer.Name) } + if afterWasCalled { + t.Errorf("unexpected, AfterDelete() was called") + } // the object should still exist obj, err := registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{}) @@ -1479,6 +1576,9 @@ func TestNonGracefulStoreHandleFinalizers(t *testing.T) { if err != nil { t.Errorf("Unexpected error: %v", err) } + if afterWasCalled { + t.Errorf("unexpected, AfterDelete() was called") + } // the object should still exist, because it still has a finalizer obj, err = registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{}) @@ -1498,6 +1598,9 @@ func TestNonGracefulStoreHandleFinalizers(t *testing.T) { if err != nil { t.Errorf("Unexpected error: %v", err) } + if !afterWasCalled { + t.Errorf("unexpected, AfterDelete() was not called") + } // the pod should be removed, because its finalizer is removed _, err = registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{}) if !errors.IsNotFound(err) { @@ -2248,7 +2351,7 @@ func TestFinalizeDelete(t *testing.T) { obj := &example.Pod{ ObjectMeta: metav1.ObjectMeta{Name: "foo", UID: "random-uid"}, } - result, err := s.finalizeDelete(genericapirequest.NewContext(), obj, false) + result, err := s.finalizeDelete(genericapirequest.NewContext(), obj, false, &metav1.DeleteOptions{}) if err != nil { t.Fatalf("unexpected err: %s", err) }