From ab17d4910da3f9b900afa509e51e2504d280f17e Mon Sep 17 00:00:00 2001 From: Joe Betz Date: Fri, 15 May 2026 01:59:49 +0000 Subject: [PATCH] Add API definition test to enforce ReturnDeletedObject: false --- .../apidefinitions/default_behaviors_test.go | 113 ++++++++++++++++++ 1 file changed, 113 insertions(+) diff --git a/test/integration/apiserver/apidefinitions/default_behaviors_test.go b/test/integration/apiserver/apidefinitions/default_behaviors_test.go index 35ceb276adf..3bb73026b00 100644 --- a/test/integration/apiserver/apidefinitions/default_behaviors_test.go +++ b/test/integration/apiserver/apidefinitions/default_behaviors_test.go @@ -24,10 +24,13 @@ import ( "testing" apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/dynamic" + "k8s.io/client-go/rest" "k8s.io/client-go/util/retry" ) @@ -337,6 +340,116 @@ func TestCheckGracefulDelete(t *testing.T) { }) } +// TestReturnDeletedObject verifies that APIs prefer ReturnDeletedObject = false. +// +// Rationale: Returning the entire object on deletion is expensive, almost never needed +// by clients, and the status returned when ReturnDeletedObject = false is preferred. +func TestReturnDeletedObject(t *testing.T) { + exempt := sets.New( + // Grandfathered legacy APIs that historically returned the full object on deletion: + "namespaces.v1", + "persistentvolumeclaims.v1", + "persistentvolumes.v1", + "pods.v1", + "podtemplates.v1", + "resourcequotas.v1", + "serviceaccounts.v1", + "services.v1", + "csidrivers.v1.storage.k8s.io", + "csinodes.v1.storage.k8s.io", + "storageclasses.v1.storage.k8s.io", + "volumeattachments.v1.storage.k8s.io", + "volumeattributesclasses.v1.storage.k8s.io", + "volumeattributesclasses.v1beta1.storage.k8s.io", + "deviceclasses.v1.resource.k8s.io", + "deviceclasses.v1beta1.resource.k8s.io", + "deviceclasses.v1beta2.resource.k8s.io", + "devicetaintrules.v1alpha3.resource.k8s.io", + "devicetaintrules.v1beta2.resource.k8s.io", + "resourceclaims.v1.resource.k8s.io", + "resourceclaims.v1beta1.resource.k8s.io", + "resourceclaims.v1beta2.resource.k8s.io", + "resourceclaimtemplates.v1.resource.k8s.io", + "resourceclaimtemplates.v1beta1.resource.k8s.io", + "resourceclaimtemplates.v1beta2.resource.k8s.io", + "resourcepoolstatusrequests.v1alpha3.resource.k8s.io", + "resourceslices.v1.resource.k8s.io", + "resourceslices.v1beta1.resource.k8s.io", + "resourceslices.v1beta2.resource.k8s.io", + + // APIs with mandatory asynchronous cleanup finalizers that cannot be stripped: + "customresourcedefinitions.v1.apiextensions.k8s.io", + ) + TestAllDefinitions(t, "return-deleted-object", func(t *testing.T, api Definition) { + if !api.HasVerb("create") || !api.HasVerb("delete") { + t.Skip("Resource does not support create and delete") + } + + client := api.ResourceClient() + obj := TestObj(t, api.StorageData.Stub, "{}", api.Mapping.GroupVersionKind) + created, err := client.Create(context.TODO(), obj, metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Failed to create: %v", err) + } + name := created.GetName() + + if len(created.GetFinalizers()) > 0 && api.HasVerb("update") { + created.SetFinalizers(nil) + _, err := client.Update(context.TODO(), created, metav1.UpdateOptions{}) + if err != nil { + t.Fatalf("Failed to strip finalizers: %v", err) + } + } + + gv := api.Mapping.GroupVersionKind.GroupVersion() + config := dynamic.ConfigFor(api.Config) + config.GroupVersion = &gv + config.APIPath = dynamic.LegacyAPIPathResolverFunc(api.Mapping.GroupVersionKind) + restClient, err := rest.RESTClientFor(config) + if err != nil { + t.Fatalf("Failed to create rest client: %v", err) + } + + zero := int64(0) + bg := metav1.DeletePropagationBackground + delOpts := metav1.DeleteOptions{ + GracePeriodSeconds: &zero, + PropagationPolicy: &bg, + } + + req := restClient.Delete(). + Resource(api.Mapping.Resource.Resource). + Name(name). + Body(&delOpts) + + if api.Mapping.Scope == meta.RESTScopeNamespace { + req.Namespace(api.Namespace) + } + + result := req.Do(context.TODO()) + if err := result.Error(); err != nil { + t.Fatalf("Failed to delete: %v", err) + } + + resObj, err := result.Get() + if err != nil { + t.Fatalf("Failed to get result object: %v", err) + } + + var kind string + if _, ok := resObj.(*metav1.Status); ok { + kind = "Status" + } else if unstr, ok := resObj.(*unstructured.Unstructured); ok { + kind = unstr.GetKind() + } else { + kind = resObj.GetObjectKind().GroupVersionKind().Kind + } + + returnsDeleted := kind != "Status" + assertDefault(t, api.Mapping.Resource, "ReturnDeletedObject must be false", !returnsDeleted, exempt) + }) +} + // addBlockingFinalizer appends a test finalizer to name, retrying on conflict. func addBlockingFinalizer(client dynamic.ResourceInterface, name string) error { return retry.RetryOnConflict(retry.DefaultRetry, func() error {