From eac9da061ddf3b2d71ffcab124af8b0e3fb71cae Mon Sep 17 00:00:00 2001 From: Owen Strain Date: Fri, 1 Aug 2025 00:21:54 +0000 Subject: [PATCH] Bugfix: DeleteOptions decode errors should return 400 instead of 500 --- .../pkg/endpoints/handlers/delete.go | 8 +- .../pkg/endpoints/handlers/delete_test.go | 129 ++++++++++++++++++ 2 files changed, 135 insertions(+), 2 deletions(-) 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 e8b74c8a913..2efd08d25a8 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/delete.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/delete.go @@ -103,11 +103,13 @@ func DeleteResource(r rest.GracefulDeleter, allowsOptions bool, scope *RequestSc defaultGVK := scope.MetaGroupVersion.WithKind("DeleteOptions") obj, gvk, err := apihelpers.GetMetaInternalVersionCodecs().DecoderToVersion(s.Serializer, defaultGVK.GroupVersion()).Decode(body, &defaultGVK, options) if err != nil { + err = errors.NewBadRequest(err.Error()) scope.err(err, w, req) return } if obj != options { - scope.err(fmt.Errorf("decoded object cannot be converted to DeleteOptions"), w, req) + err = errors.NewBadRequest("decoded object cannot be converted to DeleteOptions") + scope.err(err, w, req) return } span.AddEvent("Decoded delete options") @@ -278,11 +280,13 @@ func DeleteCollection(r rest.CollectionDeleter, checkBody bool, scope *RequestSc defaultGVK := scope.MetaGroupVersion.WithKind("DeleteOptions") obj, gvk, err := apihelpers.GetMetaInternalVersionCodecs().DecoderToVersion(s.Serializer, defaultGVK.GroupVersion()).Decode(body, &defaultGVK, options) if err != nil { + err = errors.NewBadRequest(err.Error()) scope.err(err, w, req) return } if obj != options { - scope.err(fmt.Errorf("decoded object cannot be converted to DeleteOptions"), w, req) + err = errors.NewBadRequest("decoded object cannot be converted to DeleteOptions") + scope.err(err, w, req) return } diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/delete_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/delete_test.go index c0396ee4aec..3fbeef7f63e 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/delete_test.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/delete_test.go @@ -33,6 +33,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/runtime/serializer" + jsonserializer "k8s.io/apimachinery/pkg/runtime/serializer/json" "k8s.io/apiserver/pkg/admission" auditinternal "k8s.io/apiserver/pkg/apis/audit" "k8s.io/apiserver/pkg/audit" @@ -147,6 +148,73 @@ func TestDeleteResourceAuditLogRequestObject(t *testing.T) { } } +// For issue https://github.com/kubernetes/kubernetes/issues/132359 +func TestDeleteResourceDeleteOptions(t *testing.T) { + ctx := t.Context() + fakeDeleterFn := func(ctx context.Context, _ rest.ValidateObjectFunc, _ *metav1.DeleteOptions) (runtime.Object, bool, error) { + return nil, false, nil + } + js := jsonserializer.NewSerializerWithOptions(jsonserializer.DefaultMetaFactory, nil, nil, jsonserializer.SerializerOptions{}) + scope := &RequestScope{ + Namer: &mockNamer{}, + Serializer: &fakeSerializer{ + serializer: runtime.NewCodec(js, js), + }, + } + handler := DeleteResource(fakeDeleterFunc(fakeDeleterFn), true, scope, nil) + + tests := []struct { + name string + body string + expectCode int + }{ + { + name: "valid delete options", + body: "{}", + expectCode: 200, + }, + { + name: "orphanDependents invalid Go type", + body: `{"orphanDependents": "randomString"}`, + expectCode: 400, + }, + { + name: "apiVersion/kind mismatch", + body: `{ "apiVersion": "v1", "kind": "APIResourceList" }`, + expectCode: 400, + }, + { + name: "apiVersion invalid type", + body: `{ "apiVersion": false, "kind": "DeleteOptions" }`, + expectCode: 400, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + req, err := http.NewRequestWithContext(ctx, request.MethodDelete, "/api/v1/namespaces/default/pods/testpod", strings.NewReader(test.body)) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "*/*") + + recorder := httptest.NewRecorder() + handler.ServeHTTP(recorder, req) + gotCode := recorder.Code + if gotCode != test.expectCode { + t.Fatalf("expected status %v but got %v", test.expectCode, gotCode) + } + }) + } +} + +type fakeDeleterFunc func(ctx context.Context, deleteValidation rest.ValidateObjectFunc, options *metav1.DeleteOptions) (runtime.Object, bool, error) + +func (f fakeDeleterFunc) Delete(ctx context.Context, name string, deleteValidation rest.ValidateObjectFunc, options *metav1.DeleteOptions) (runtime.Object, bool, error) { + return f(ctx, deleteValidation, options) +} + func TestDeleteCollection(t *testing.T) { req := &http.Request{ Header: http.Header{}, @@ -216,6 +284,67 @@ func TestDeleteCollection(t *testing.T) { } } +// For issue https://github.com/kubernetes/kubernetes/issues/132359 +func TestDeleteCollectionDeleteOptions(t *testing.T) { + ctx := t.Context() + fakeDeleterFn := func(ctx context.Context, _ rest.ValidateObjectFunc, _ *metav1.DeleteOptions, _ *metainternalversion.ListOptions) (runtime.Object, error) { + return nil, nil + } + js := jsonserializer.NewSerializerWithOptions(jsonserializer.DefaultMetaFactory, nil, nil, jsonserializer.SerializerOptions{}) + scope := &RequestScope{ + Namer: &mockNamer{}, + Serializer: &fakeSerializer{ + serializer: runtime.NewCodec(js, js), + }, + } + handler := DeleteCollection(fakeCollectionDeleterFunc(fakeDeleterFn), true, scope, nil) + + tests := []struct { + name string + body string + expectCode int + }{ + { + name: "valid delete options", + body: "{}", + expectCode: 200, + }, + { + name: "orphanDependents invalid Go type", + body: `{"orphanDependents": "randomString"}`, + expectCode: 400, + }, + { + name: "apiVersion/kind mismatch", + body: `{ "apiVersion": "v1", "kind": "APIResourceList" }`, + expectCode: 400, + }, + { + name: "apiVersion invalid type", + body: `{ "apiVersion": false, "kind": "DeleteOptions" }`, + expectCode: 400, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + req, err := http.NewRequestWithContext(ctx, request.MethodDelete, "/api/v1/namespaces/default/pods/testpod", strings.NewReader(test.body)) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "*/*") + + recorder := httptest.NewRecorder() + handler.ServeHTTP(recorder, req) + gotCode := recorder.Code + if gotCode != test.expectCode { + t.Fatalf("expected status %v but got %v", test.expectCode, gotCode) + } + }) + } +} + func TestDeleteCollectionWithNoContextDeadlineEnforced(t *testing.T) { ctx := t.Context() var invokedGot, hasDeadlineGot int32