From dd588c0aaca48f5bde5850f221afaabe91df0963 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Wed, 15 Jul 2026 17:48:14 +1200 Subject: [PATCH 1/4] fix: send propagationPolicy at the top level of DeleteOptions delete() nested propagationPolicy and gracePeriodSeconds inside the preconditions map, where the Kubernetes API ignores them. Worse, the refresh() call that followed replaced the resource attributes entirely, so the DeleteOptions body sent to the API was just the refreshed resource relabelled as kind DeleteOptions with no propagationPolicy anywhere. Job deletes therefore fell back to the API default (Orphan), leaving completed pods with no ownerRef and no TTL, which pins PVCs via pvc-protection and permanently wedges dedicated database resizes (DAT-1869). Build the DeleteOptions payload explicitly instead of mutating the resource: propagationPolicy and gracePeriodSeconds at the top level, and preconditions carrying only the resource uid. The resourceVersion precondition is dropped deliberately: it never took effect before, and enforcing it now would introduce conflict failures into every consumer's delete path whenever a controller bumps the resource between refresh and delete. The uid precondition still guards against deleting a recreated resource of the same name. Co-Authored-By: Claude Opus 4.8 --- src/Traits/RunsClusterOperations.php | 44 +++++++++---- tests/DeleteOptionsTest.php | 93 ++++++++++++++++++++++++++++ 2 files changed, 126 insertions(+), 11 deletions(-) create mode 100644 tests/DeleteOptionsTest.php diff --git a/src/Traits/RunsClusterOperations.php b/src/Traits/RunsClusterOperations.php index 89bd404..9ff354d 100644 --- a/src/Traits/RunsClusterOperations.php +++ b/src/Traits/RunsClusterOperations.php @@ -233,23 +233,14 @@ public function update(array $query = ['pretty' => 1]): bool /** * Delete the resource. * - * @param null|int $gracePeriod - * * @throws KubernetesAPIException */ - public function delete(array $query = ['pretty' => 1], $gracePeriod = null, string $propagationPolicy = 'Foreground'): bool + public function delete(array $query = ['pretty' => 1], ?int $gracePeriod = null, string $propagationPolicy = 'Foreground'): bool { if (! $this->isSynced()) { return true; } - $this->setAttribute('preconditions', [ - 'resourceVersion' => $this->getResourceVersion(), - 'uid' => $this->getResourceUid(), - 'propagationPolicy' => $propagationPolicy, - 'gracePeriodSeconds' => $gracePeriod, - ]); - $this->refresh(); $this->cluster @@ -257,7 +248,10 @@ public function delete(array $query = ['pretty' => 1], $gracePeriod = null, stri ->runOperation( Operation::DELETE, $this->resourcePath(), - $this->toJsonPayload('DeleteOptions'), + json_encode( + $this->deleteOptions($gracePeriod, $propagationPolicy), + JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES + ), $query ); @@ -266,6 +260,34 @@ public function delete(array $query = ['pretty' => 1], $gracePeriod = null, stri return true; } + /** + * Build the DeleteOptions payload for the delete operation. + * The propagationPolicy and gracePeriodSeconds fields are top-level + * DeleteOptions fields; only the resource uid belongs in preconditions. + * + * @return array + */ + public function deleteOptions(?int $gracePeriod = null, string $propagationPolicy = 'Foreground'): array + { + $options = [ + 'apiVersion' => 'v1', + 'kind' => 'DeleteOptions', + 'propagationPolicy' => $propagationPolicy, + ]; + + if (! is_null($gracePeriod)) { + $options['gracePeriodSeconds'] = $gracePeriod; + } + + if (! is_null($uid = $this->getResourceUid())) { + $options['preconditions'] = [ + 'uid' => $uid, + ]; + } + + return $options; + } + /** * Apply the resource using server-side apply. * diff --git a/tests/DeleteOptionsTest.php b/tests/DeleteOptionsTest.php new file mode 100644 index 0000000..25979c5 --- /dev/null +++ b/tests/DeleteOptionsTest.php @@ -0,0 +1,93 @@ +cluster->job()->setName('reap'); + + $job->syncWith([ + 'metadata' => [ + 'name' => 'reap', + 'uid' => 'aa11bb22-cc33-dd44-ee55-ff6677889900', + 'resourceVersion' => '12345', + ], + ]); + + $options = $job->deleteOptions(null, 'Foreground'); + + $this->assertSame('DeleteOptions', $options['kind']); + $this->assertSame('v1', $options['apiVersion']); + $this->assertSame('Foreground', $options['propagationPolicy']); + $this->assertSame(['uid' => 'aa11bb22-cc33-dd44-ee55-ff6677889900'], $options['preconditions']); + $this->assertArrayNotHasKey('propagationPolicy', $options['preconditions']); + $this->assertArrayNotHasKey('gracePeriodSeconds', $options['preconditions']); + $this->assertArrayNotHasKey('gracePeriodSeconds', $options); + } + + public function test_grace_period_is_a_top_level_delete_options_field() + { + $job = $this->cluster->job()->setName('reap'); + + $job->syncWith([ + 'metadata' => [ + 'name' => 'reap', + 'uid' => 'aa11bb22-cc33-dd44-ee55-ff6677889900', + ], + ]); + + $options = $job->deleteOptions(30, 'Background'); + + $this->assertSame('Background', $options['propagationPolicy']); + $this->assertSame(30, $options['gracePeriodSeconds']); + $this->assertSame(['uid' => 'aa11bb22-cc33-dd44-ee55-ff6677889900'], $options['preconditions']); + } + + public function test_preconditions_are_omitted_without_a_resource_uid() + { + $job = $this->cluster->job()->setName('reap'); + + $options = $job->deleteOptions(null, 'Foreground'); + + $this->assertSame('Foreground', $options['propagationPolicy']); + $this->assertArrayNotHasKey('preconditions', $options); + } + + public function test_delete_options_payload_shape_matches_the_kubernetes_api() + { + $job = $this->cluster->job()->setName('reap'); + + $job->syncWith([ + 'metadata' => [ + 'name' => 'reap', + 'uid' => 'aa11bb22-cc33-dd44-ee55-ff6677889900', + ], + 'spec' => [ + 'template' => [ + 'spec' => [ + 'containers' => [], + ], + ], + ], + ]); + + $payload = json_decode(json_encode($job->deleteOptions(null, 'Foreground')), true); + + $this->assertSame( + [ + 'apiVersion' => 'v1', + 'kind' => 'DeleteOptions', + 'propagationPolicy' => 'Foreground', + 'preconditions' => [ + 'uid' => 'aa11bb22-cc33-dd44-ee55-ff6677889900', + ], + ], + $payload + ); + + $this->assertArrayNotHasKey('spec', $payload); + $this->assertArrayNotHasKey('metadata', $payload); + } +} From 75fd8d0682861f422706b9a637c4574e78556eb8 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Tue, 22 Sep 2026 20:21:15 +1200 Subject: [PATCH 2/4] fix: accept the PropagationPolicy enum and wait out foreground deletes The propagation policy was an unchecked string, so a typo reached the API and came back as a server-side rejection. It is now a PropagationPolicy enum, accepted either as the case or as its string value, and validated before the request with the valid values in the message. Four tests asserted an immediate 404 after delete(), which stopped being true once the policy defaulted to Foreground: the resource stays readable until its dependents are gone. They now wait for it to disappear through a bounded helper that fails with the kind and name rather than hanging. DeleteOptionsTest no longer round-trips the payload through json_encode to assert its shape; deleteOptions() already returns a plain array. Co-Authored-By: Claude Fable 5.1 --- docs/guide/usage/crud-operations.md | 20 ++++++++++++----- src/Enums/PropagationPolicy.php | 13 +++++++++++ src/Traits/RunsClusterOperations.php | 32 ++++++++++++++++++++++------ tests/CSIDriverTest.php | 2 ++ tests/DeleteOptionsTest.php | 25 +++++++++++++++++++++- tests/IngressTest.php | 2 ++ tests/SecretTest.php | 2 ++ tests/StorageClassTest.php | 2 ++ tests/TestCase.php | 23 ++++++++++++++++++++ 9 files changed, 108 insertions(+), 13 deletions(-) create mode 100644 src/Enums/PropagationPolicy.php diff --git a/docs/guide/usage/crud-operations.md b/docs/guide/usage/crud-operations.md index 1fb2b4f..fd33baa 100644 --- a/docs/guide/usage/crud-operations.md +++ b/docs/guide/usage/crud-operations.md @@ -184,25 +184,35 @@ The `delete()` method accepts optional parameters for fine-grained control: public function delete( array $query = ['pretty' => 1], ?int $gracePeriod = null, - string $propagationPolicy = 'Foreground' + PropagationPolicy|string $propagationPolicy = PropagationPolicy::FOREGROUND ): bool ``` Example with options: ```php +use RenokiCo\PhpK8s\Enums\PropagationPolicy; + // Delete with 30-second grace period $pod->delete( query: ['pretty' => 1], gracePeriod: 30, - propagationPolicy: 'Foreground' + propagationPolicy: PropagationPolicy::FOREGROUND ); ``` **Propagation Policies:** -- `Foreground` - Waits for dependents to be deleted first -- `Background` - Deletes immediately, dependents deleted in background -- `Orphan` - Leaves dependents orphaned +- `PropagationPolicy::FOREGROUND` - Waits for dependents to be deleted first +- `PropagationPolicy::BACKGROUND` - Deletes immediately, dependents deleted in background +- `PropagationPolicy::ORPHAN` - Leaves dependents orphaned + +The equivalent strings (`'Foreground'`, `'Background'`, `'Orphan'`) are still +accepted; anything else raises an `InvalidArgumentException` before the request +is sent, rather than being rejected by the API. + +Under `Foreground`, the resource stays readable until its dependents are gone, +so a `get()` immediately after `delete()` returns the resource with a +`deletionTimestamp` instead of a 404. ## Creating or Updating Resources diff --git a/src/Enums/PropagationPolicy.php b/src/Enums/PropagationPolicy.php new file mode 100644 index 0000000..833ac98 --- /dev/null +++ b/src/Enums/PropagationPolicy.php @@ -0,0 +1,13 @@ + 1]): bool * * @throws KubernetesAPIException */ - public function delete(array $query = ['pretty' => 1], ?int $gracePeriod = null, string $propagationPolicy = 'Foreground'): bool + public function delete(array $query = ['pretty' => 1], ?int $gracePeriod = null, PropagationPolicy|string $propagationPolicy = PropagationPolicy::FOREGROUND): bool { if (! $this->isSynced()) { return true; @@ -243,15 +245,17 @@ public function delete(array $query = ['pretty' => 1], ?int $gracePeriod = null, $this->refresh(); + $payload = json_encode( + $this->deleteOptions($gracePeriod, $propagationPolicy), + JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES + ); + $this->cluster ->setResourceClass(get_class($this)) ->runOperation( Operation::DELETE, $this->resourcePath(), - json_encode( - $this->deleteOptions($gracePeriod, $propagationPolicy), - JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES - ), + $payload, $query ); @@ -266,13 +270,27 @@ public function delete(array $query = ['pretty' => 1], ?int $gracePeriod = null, * DeleteOptions fields; only the resource uid belongs in preconditions. * * @return array + * + * @throws InvalidArgumentException */ - public function deleteOptions(?int $gracePeriod = null, string $propagationPolicy = 'Foreground'): array + public function deleteOptions(?int $gracePeriod = null, PropagationPolicy|string $propagationPolicy = PropagationPolicy::FOREGROUND): array { + $policy = $propagationPolicy instanceof PropagationPolicy + ? $propagationPolicy + : PropagationPolicy::tryFrom($propagationPolicy); + + if (is_null($policy)) { + throw new InvalidArgumentException(sprintf( + 'Invalid propagation policy "%s"; expected one of %s', + $propagationPolicy, + implode(', ', array_column(PropagationPolicy::cases(), 'value')) + )); + } + $options = [ 'apiVersion' => 'v1', 'kind' => 'DeleteOptions', - 'propagationPolicy' => $propagationPolicy, + 'propagationPolicy' => $policy->value, ]; if (! is_null($gracePeriod)) { diff --git a/tests/CSIDriverTest.php b/tests/CSIDriverTest.php index fec5a16..50ed962 100644 --- a/tests/CSIDriverTest.php +++ b/tests/CSIDriverTest.php @@ -139,6 +139,8 @@ public function runDeletionTests() $this->assertTrue($csiDriver->delete()); + $this->waitUntilDeleted($csiDriver); + $this->expectException(KubernetesAPIException::class); $this->cluster->getCSIDriverByName('test-csi-driver.example.com'); diff --git a/tests/DeleteOptionsTest.php b/tests/DeleteOptionsTest.php index 25979c5..15b3e45 100644 --- a/tests/DeleteOptionsTest.php +++ b/tests/DeleteOptionsTest.php @@ -2,6 +2,9 @@ namespace RenokiCo\PhpK8s\Test; +use InvalidArgumentException; +use RenokiCo\PhpK8s\Enums\PropagationPolicy; + class DeleteOptionsTest extends TestCase { public function test_propagation_policy_is_a_top_level_delete_options_field() @@ -55,6 +58,26 @@ public function test_preconditions_are_omitted_without_a_resource_uid() $this->assertArrayNotHasKey('preconditions', $options); } + public function test_invalid_propagation_policy_is_rejected_before_the_request() + { + $job = $this->cluster->job()->setName('reap'); + + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('Invalid propagation policy "foreground"'); + + $job->deleteOptions(null, 'foreground'); + } + + public function test_propagation_policy_accepts_the_enum() + { + $job = $this->cluster->job()->setName('reap'); + + $this->assertSame( + 'Orphan', + $job->deleteOptions(null, PropagationPolicy::ORPHAN)['propagationPolicy'] + ); + } + public function test_delete_options_payload_shape_matches_the_kubernetes_api() { $job = $this->cluster->job()->setName('reap'); @@ -73,7 +96,7 @@ public function test_delete_options_payload_shape_matches_the_kubernetes_api() ], ]); - $payload = json_decode(json_encode($job->deleteOptions(null, 'Foreground')), true); + $payload = $job->deleteOptions(null, 'Foreground'); $this->assertSame( [ diff --git a/tests/IngressTest.php b/tests/IngressTest.php index 2ce0e4d..0c65aab 100644 --- a/tests/IngressTest.php +++ b/tests/IngressTest.php @@ -178,6 +178,8 @@ public function runDeletionTests() $this->assertTrue($ingress->delete()); + $this->waitUntilDeleted($ingress); + $this->expectException(KubernetesAPIException::class); $this->cluster->getIngressByName('nginx'); diff --git a/tests/SecretTest.php b/tests/SecretTest.php index 20e644c..6f5ab17 100644 --- a/tests/SecretTest.php +++ b/tests/SecretTest.php @@ -149,6 +149,8 @@ public function runDeletionTests() $this->assertTrue($secret->delete()); + $this->waitUntilDeleted($secret); + $this->expectException(KubernetesAPIException::class); $this->cluster->getSecretByName('passwords'); diff --git a/tests/StorageClassTest.php b/tests/StorageClassTest.php index ecd96bd..8dac68c 100644 --- a/tests/StorageClassTest.php +++ b/tests/StorageClassTest.php @@ -130,6 +130,8 @@ public function runDeletionTests() $this->assertTrue($sc->delete()); + $this->waitUntilDeleted($sc); + $this->expectException(KubernetesAPIException::class); $this->cluster->getStorageClassByName('io1'); diff --git a/tests/TestCase.php b/tests/TestCase.php index d15c88a..36f2375 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -7,6 +7,7 @@ use RenokiCo\PhpK8s\Instances\Container; use RenokiCo\PhpK8s\K8s; use RenokiCo\PhpK8s\Kinds\K8sPod; +use RenokiCo\PhpK8s\Kinds\K8sResource; use RenokiCo\PhpK8s\KubernetesCluster; use RenokiCo\PhpK8s\PhpK8sServiceProvider; @@ -65,6 +66,28 @@ public function getEnvironmentSetUp($app) // } + /** + * Foreground propagation keeps a deleted resource readable until its + * dependents are gone, so a 404 is not immediate after delete(). + */ + protected function waitUntilDeleted(K8sResource $resource, int $timeout = 60): void + { + $deadline = time() + $timeout; + + while ($resource->exists()) { + if (time() >= $deadline) { + self::fail(sprintf( + '%s "%s" still exists %d seconds after delete()', + $resource::getKind(), + $resource->getName(), + $timeout + )); + } + + sleep(1); + } + } + /** * Create a standard mariadb container with common configuration. * From 627db1a3708b4295ced182298e40d3942af25a4f Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Tue, 22 Sep 2026 20:44:31 +1200 Subject: [PATCH 3/4] test: assert the DeleteOptions that delete() puts on the wire The suite only called deleteOptions() and asserted the array it returns, so it could have passed while delete() sent something else entirely - which is the failure this fixes. A RecordingCluster now answers from canned responses and keeps the requests, and the new test drives delete() and reads propagationPolicy, gracePeriodSeconds and preconditions out of the DELETE body. Confirmed against the original bug: nesting the fields back under preconditions fails the new test. Co-Authored-By: Claude Fable 5.1 --- tests/DeleteOptionsTest.php | 37 +++++++++++++++++++++++++ tests/RecordingCluster.php | 54 +++++++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+) create mode 100644 tests/RecordingCluster.php diff --git a/tests/DeleteOptionsTest.php b/tests/DeleteOptionsTest.php index 15b3e45..0dd98a7 100644 --- a/tests/DeleteOptionsTest.php +++ b/tests/DeleteOptionsTest.php @@ -78,6 +78,43 @@ public function test_propagation_policy_accepts_the_enum() ); } + public function test_delete_sends_the_propagation_policy_at_the_top_level() + { + $cluster = (new RecordingCluster('http://127.0.0.1:8080')) + ->respondWith([ + ['kind' => 'Job', 'apiVersion' => 'batch/v1', 'metadata' => [ + 'name' => 'reap', + 'uid' => 'aa11bb22-cc33-dd44-ee55-ff6677889900', + ]], + ['kind' => 'Status', 'apiVersion' => 'v1', 'status' => 'Success'], + ]); + + $job = $cluster->job()->setName('reap'); + $job->syncWith([ + 'metadata' => [ + 'name' => 'reap', + 'uid' => 'aa11bb22-cc33-dd44-ee55-ff6677889900', + ], + ]); + + $this->assertTrue($job->delete(gracePeriod: 30, propagationPolicy: PropagationPolicy::BACKGROUND)); + + $requests = $cluster->requests(); + $delete = end($requests); + + $this->assertSame('DELETE', $delete->getMethod()); + + $sent = json_decode((string) $delete->getBody(), true); + + $this->assertSame('Background', $sent['propagationPolicy'] ?? null); + $this->assertSame(30, $sent['gracePeriodSeconds'] ?? null); + $this->assertSame( + ['uid' => 'aa11bb22-cc33-dd44-ee55-ff6677889900'], + $sent['preconditions'] ?? null, + 'only the uid belongs in preconditions' + ); + } + public function test_delete_options_payload_shape_matches_the_kubernetes_api() { $job = $this->cluster->job()->setName('reap'); diff --git a/tests/RecordingCluster.php b/tests/RecordingCluster.php new file mode 100644 index 0000000..adc9484 --- /dev/null +++ b/tests/RecordingCluster.php @@ -0,0 +1,54 @@ + */ + public array $transactions = []; + + /** @var array */ + protected array $responses = []; + + /** + * @param array> $bodies + */ + public function respondWith(array $bodies): static + { + foreach ($bodies as $body) { + $this->responses[] = new Response(200, ['Content-Type' => 'application/json'], json_encode($body)); + } + + return $this; + } + + #[Override] + public function getClient(): Client + { + $stack = HandlerStack::create(new MockHandler($this->responses)); + $stack->push(Middleware::history($this->transactions)); + + return new Client(['handler' => $stack]); + } + + public function requests(): array + { + return array_map( + fn (array $transaction): RequestInterface => $transaction['request'], + $this->transactions + ); + } +} From 2f269f4d0b9e93c4cf70e6f669df228826e65279 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Tue, 22 Sep 2026 20:59:36 +1200 Subject: [PATCH 4/4] test: keep one response queue per recording cluster getClient() is called once per request, so building a MockHandler from the full response list each time replayed the first response to every call: delete()'s DELETE was answered with the refresh GET's body and the queued Status response was never reached. The client and its handler are now built once, and the test asserts both requests in order. Co-Authored-By: Claude Fable 5.1 --- tests/DeleteOptionsTest.php | 5 ++++- tests/RecordingCluster.php | 31 +++++++++++++++++++++++++------ 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/tests/DeleteOptionsTest.php b/tests/DeleteOptionsTest.php index 0dd98a7..1343ca1 100644 --- a/tests/DeleteOptionsTest.php +++ b/tests/DeleteOptionsTest.php @@ -100,8 +100,11 @@ public function test_delete_sends_the_propagation_policy_at_the_top_level() $this->assertTrue($job->delete(gracePeriod: 30, propagationPolicy: PropagationPolicy::BACKGROUND)); $requests = $cluster->requests(); - $delete = end($requests); + $this->assertCount(2, $requests, 'delete() refreshes the resource, then deletes it'); + $this->assertSame('GET', $requests[0]->getMethod()); + + $delete = $requests[1]; $this->assertSame('DELETE', $delete->getMethod()); $sent = json_decode((string) $delete->getBody(), true); diff --git a/tests/RecordingCluster.php b/tests/RecordingCluster.php index adc9484..d4c5e64 100644 --- a/tests/RecordingCluster.php +++ b/tests/RecordingCluster.php @@ -20,30 +20,49 @@ class RecordingCluster extends KubernetesCluster /** @var array */ public array $transactions = []; - /** @var array */ - protected array $responses = []; + protected ?MockHandler $handler = null; + + protected ?Client $client = null; /** * @param array> $bodies */ public function respondWith(array $bodies): static { + $this->handler ??= new MockHandler; + foreach ($bodies as $body) { - $this->responses[] = new Response(200, ['Content-Type' => 'application/json'], json_encode($body)); + $this->handler->append( + new Response(200, ['Content-Type' => 'application/json'], json_encode($body)) + ); } return $this; } + /** + * One client, and therefore one response queue, for the life of the + * cluster: a fresh handler per call would replay the first response to + * every request and no multi-request flow could be modelled. + */ #[Override] public function getClient(): Client { - $stack = HandlerStack::create(new MockHandler($this->responses)); - $stack->push(Middleware::history($this->transactions)); + if ($this->client === null) { + $this->handler ??= new MockHandler; + + $stack = HandlerStack::create($this->handler); + $stack->push(Middleware::history($this->transactions)); - return new Client(['handler' => $stack]); + $this->client = new Client(['handler' => $stack]); + } + + return $this->client; } + /** + * @return array + */ public function requests(): array { return array_map(