From 7200880083e68f5a7ce629ee6bc644595b15105d Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Fri, 25 Sep 2026 14:53:39 +1200 Subject: [PATCH 1/2] fix: keep the HTTP status on API errors without a JSON Status body KubernetesAPIException took its code only from the `code` field of the decoded response body. When the API server answers a 4xx with something other than a JSON Status, the code became 0 and the HTTP status was lost. That is what it does for an API group or resource it does not serve: a GET under snapshot.storage.k8s.io on a cluster without the VolumeSnapshot CRDs answers a plain-text "404 page not found". Every `getCode() === 404` check, K8sResource::exists() included, then missed the 404. appwrite-labs/edge hit this on kind clusters without the snapshot CRDs. Branch cleanup read the missing VolumeSnapshot as a hard failure rather than as absent, left orphan branch namespaces behind, and failed cloud's CI. The Status code still wins when the body carries one (a Status code of 0 means unset). Otherwise the exception takes the HTTP status. JSON Status payloads pass through unchanged. The Guzzle exception is now chained as the previous exception, so the raw response stays reachable, and the cluster version check builds its exception the same way. PhpK8sException gains an optional trailing `$previous` argument, which keeps existing callers compatible, and it stops passing a null message to Exception, which PHP 8.1+ deprecates. Co-Authored-By: Claude Opus 5.5 --- src/Exceptions/KubernetesAPIException.php | 17 ++++- src/Exceptions/PhpK8sException.php | 5 +- src/Traits/Cluster/ChecksClusterVersion.php | 10 +-- src/Traits/Cluster/MakesHttpCalls.php | 10 +-- tests/ChecksClusterVersionTest.php | 20 +++++ tests/MakesHttpCallsTest.php | 84 +++++++++++++++++++++ tests/TestCase.php | 21 ++++++ 7 files changed, 148 insertions(+), 19 deletions(-) create mode 100644 tests/MakesHttpCallsTest.php diff --git a/src/Exceptions/KubernetesAPIException.php b/src/Exceptions/KubernetesAPIException.php index 5f23f24..4010ad8 100644 --- a/src/Exceptions/KubernetesAPIException.php +++ b/src/Exceptions/KubernetesAPIException.php @@ -2,7 +2,22 @@ namespace RenokiCo\PhpK8s\Exceptions; +use GuzzleHttp\Exception\BadResponseException; + class KubernetesAPIException extends PhpK8sException { - // + public static function from(BadResponseException $exception): self + { + $response = $exception->getResponse(); + $body = json_decode((string) $response->getBody(), true); + $payload = is_array($body) ? $body : null; + $code = $payload['code'] ?? null; + + return new self( + $exception->getMessage(), + is_int($code) && $code > 0 ? $code : $response->getStatusCode(), + $payload, + $exception, + ); + } } diff --git a/src/Exceptions/PhpK8sException.php b/src/Exceptions/PhpK8sException.php index e65a94a..952f0b6 100644 --- a/src/Exceptions/PhpK8sException.php +++ b/src/Exceptions/PhpK8sException.php @@ -3,15 +3,16 @@ namespace RenokiCo\PhpK8s\Exceptions; use Exception; +use Throwable; class PhpK8sException extends Exception { /** * Initialize the exception. */ - public function __construct(?string $message = null, int $code = 0, protected ?array $payload = null) + public function __construct(?string $message = null, int $code = 0, protected ?array $payload = null, ?Throwable $previous = null) { - parent::__construct($message, $code); + parent::__construct($message ?? '', $code, $previous); } /** diff --git a/src/Traits/Cluster/ChecksClusterVersion.php b/src/Traits/Cluster/ChecksClusterVersion.php index 8514b17..aa8c618 100644 --- a/src/Traits/Cluster/ChecksClusterVersion.php +++ b/src/Traits/Cluster/ChecksClusterVersion.php @@ -32,14 +32,8 @@ protected function loadClusterVersion(): void try { $response = $this->getClient()->request('GET', $callableUrl); - } catch (ClientException $e) { - $payload = json_decode((string) $e->getResponse()->getBody(), true); - - throw new KubernetesAPIException( - $e->getMessage(), - $payload['code'] ?? 0, - $payload - ); + } catch (ClientException $exception) { + throw KubernetesAPIException::from($exception); } $json = json_decode($response->getBody(), true, 512, JSON_THROW_ON_ERROR); diff --git a/src/Traits/Cluster/MakesHttpCalls.php b/src/Traits/Cluster/MakesHttpCalls.php index 675cd45..5b69a67 100644 --- a/src/Traits/Cluster/MakesHttpCalls.php +++ b/src/Traits/Cluster/MakesHttpCalls.php @@ -128,14 +128,8 @@ public function call(string $method, string $path, string $payload = '', array $ } $response = $this->getClient()->request($method, $this->getCallableUrl($path, $query), $requestOptions); - } catch (ClientException $e) { - $errorPayload = json_decode((string) $e->getResponse()->getBody(), true); - - throw new KubernetesAPIException( - $e->getMessage(), - $errorPayload['code'] ?? 0, - $errorPayload - ); + } catch (ClientException $exception) { + throw KubernetesAPIException::from($exception); } return $response; diff --git a/tests/ChecksClusterVersionTest.php b/tests/ChecksClusterVersionTest.php index a31d23c..2f03083 100644 --- a/tests/ChecksClusterVersionTest.php +++ b/tests/ChecksClusterVersionTest.php @@ -2,6 +2,10 @@ namespace RenokiCo\PhpK8s\Test; +use GuzzleHttp\Exception\ClientException; +use GuzzleHttp\Psr7\Response; +use RenokiCo\PhpK8s\Exceptions\KubernetesAPIException; + class ChecksClusterVersionTest extends TestCase { public function test_check_cluster_version(): void @@ -11,4 +15,20 @@ public function test_check_cluster_version(): void $this->assertFalse($this->cluster->newerThan('2.0.0')); $this->assertTrue($this->cluster->olderThan('2.0.0')); } + + public function test_plain_text_version_error_keeps_http_status_and_chains_guzzle_exception(): void + { + $cluster = $this->clusterRespondingWith( + new Response(403, ['Content-Type' => 'text/plain; charset=utf-8'], "Forbidden\n") + ); + + try { + $cluster->newerThan('1.18.0'); + $this->fail('Expected a KubernetesAPIException.'); + } catch (KubernetesAPIException $exception) { + $this->assertSame(403, $exception->getCode()); + $this->assertNull($exception->getPayload()); + $this->assertInstanceOf(ClientException::class, $exception->getPrevious()); + } + } } diff --git a/tests/MakesHttpCallsTest.php b/tests/MakesHttpCallsTest.php new file mode 100644 index 0000000..9056987 --- /dev/null +++ b/tests/MakesHttpCallsTest.php @@ -0,0 +1,84 @@ +failedCall( + new Response(404, ['Content-Type' => 'text/plain; charset=utf-8'], "404 page not found\n") + ); + + $this->assertSame(404, $exception->getCode()); + $this->assertNull($exception->getPayload()); + $this->assertInstanceOf(ClientException::class, $exception->getPrevious()); + } + + #[DataProvider('statuses')] + public function test_json_status_error_keeps_payload_code(array $status): void + { + $exception = $this->failedCall( + new Response($status['code'], ['Content-Type' => 'application/json'], json_encode($status)) + ); + + $this->assertSame($status['code'], $exception->getCode()); + $this->assertSame($status, $exception->getPayload()); + } + + public static function statuses(): iterable + { + yield 'not found' => [[ + 'kind' => 'Status', + 'apiVersion' => 'v1', + 'metadata' => [], + 'status' => 'Failure', + 'message' => 'volumesnapshots.snapshot.storage.k8s.io "branch" not found', + 'reason' => 'NotFound', + 'details' => ['name' => 'branch', 'group' => 'snapshot.storage.k8s.io', 'kind' => 'volumesnapshots'], + 'code' => 404, + ]]; + + yield 'conflict' => [[ + 'kind' => 'Status', + 'apiVersion' => 'v1', + 'metadata' => [], + 'status' => 'Failure', + 'message' => 'volumesnapshots.snapshot.storage.k8s.io "branch" already exists', + 'reason' => 'AlreadyExists', + 'details' => ['name' => 'branch', 'group' => 'snapshot.storage.k8s.io', 'kind' => 'volumesnapshots'], + 'code' => 409, + ]]; + } + + public function test_unserved_api_group_answers_not_found(): void + { + try { + $this->cluster->call('GET', self::UNSERVED_PATH); + $this->fail('Expected a KubernetesAPIException for an API group the cluster does not serve.'); + } catch (KubernetesAPIException $exception) { + $this->assertSame(404, $exception->getCode()); + } + } + + private function failedCall(ResponseInterface $response): KubernetesAPIException + { + try { + $this->clusterRespondingWith($response)->call('GET', self::SNAPSHOT_PATH); + } catch (KubernetesAPIException $exception) { + return $exception; + } + + $this->fail('Expected a KubernetesAPIException.'); + } +} diff --git a/tests/TestCase.php b/tests/TestCase.php index d15c88a..4e2f123 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -2,7 +2,11 @@ namespace RenokiCo\PhpK8s\Test; +use GuzzleHttp\Client; +use GuzzleHttp\Handler\MockHandler; +use GuzzleHttp\HandlerStack; use Orchestra\Testbench\TestCase as Orchestra; +use Psr\Http\Message\ResponseInterface; use RenokiCo\PhpK8s\Exceptions\PhpK8sException; use RenokiCo\PhpK8s\Instances\Container; use RenokiCo\PhpK8s\K8s; @@ -191,4 +195,21 @@ protected function createPerlPod(array $options = []): K8sPod return $pod; } + + protected function clusterRespondingWith(ResponseInterface $response): KubernetesCluster + { + return new class(HandlerStack::create(new MockHandler([$response]))) extends KubernetesCluster + { + public function __construct(private readonly HandlerStack $handler) + { + parent::__construct('https://kubernetes.test'); + } + + #[\Override] + public function getClient(): Client + { + return new Client(['handler' => $this->handler]); + } + }; + } } From 83eeb15192d7dd1306a34d47729c402d30a94177 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Fri, 25 Sep 2026 15:10:05 +1200 Subject: [PATCH 2/2] test: pin exception code precedence and skip the live check without a cluster The JSON Status cases sent the same value as the HTTP status and the body `code`, so they passed whichever source the exception read its code from. They could not catch a regression to "always use the HTTP status" or to "always use the body". Each case now sends distinct values: a positive body code beats the HTTP status, a body code of 0 (unset in a Kubernetes Status) falls back to it, and a plain-text body takes it. Swapping from() to always read the HTTP status, or dropping the zero check, now turns a test red. The live check for an unserved API group raised a connection error instead of skipping when no API server is reachable. It now sits in its own integration test class with the same setUp guard as the other live tests, so CI always runs it and local runs without a proxy skip it. The probe catches Exception, as ExecCredentialIntegrationTest does. The variants that catch only KubernetesAPIException never see the ConnectException raised by a refused connection, so they error instead of skipping. Co-Authored-By: Claude Opus 5.5 --- tests/MakesHttpCallsIntegrationTest.php | 40 +++++++++++++++++ tests/MakesHttpCallsTest.php | 59 +++++++------------------ 2 files changed, 57 insertions(+), 42 deletions(-) create mode 100644 tests/MakesHttpCallsIntegrationTest.php diff --git a/tests/MakesHttpCallsIntegrationTest.php b/tests/MakesHttpCallsIntegrationTest.php new file mode 100644 index 0000000..5a2c18e --- /dev/null +++ b/tests/MakesHttpCallsIntegrationTest.php @@ -0,0 +1,40 @@ +isClusterAvailable()) { + $this->markTestSkipped('Integration tests require a live Kubernetes cluster'); + } + } + + private function isClusterAvailable(): bool + { + try { + $this->cluster->getAllNamespaces(); + + return true; + } catch (Exception) { + return false; + } + } + + public function test_unserved_api_group_answers_not_found(): void + { + try { + $this->cluster->call('GET', '/apis/unserved.php-k8s.invalid/v1/namespaces/default/widgets/missing'); + $this->fail('Expected a KubernetesAPIException for an API group the cluster does not serve.'); + } catch (KubernetesAPIException $exception) { + $this->assertSame(404, $exception->getCode()); + } + } +} diff --git a/tests/MakesHttpCallsTest.php b/tests/MakesHttpCallsTest.php index 9056987..3c88409 100644 --- a/tests/MakesHttpCallsTest.php +++ b/tests/MakesHttpCallsTest.php @@ -4,16 +4,11 @@ use GuzzleHttp\Exception\ClientException; use GuzzleHttp\Psr7\Response; -use PHPUnit\Framework\Attributes\DataProvider; use Psr\Http\Message\ResponseInterface; use RenokiCo\PhpK8s\Exceptions\KubernetesAPIException; class MakesHttpCallsTest extends TestCase { - private const string SNAPSHOT_PATH = '/apis/snapshot.storage.k8s.io/v1/namespaces/default/volumesnapshots/branch'; - - private const string UNSERVED_PATH = '/apis/unserved.php-k8s.invalid/v1/namespaces/default/widgets/missing'; - public function test_plain_text_error_keeps_http_status_and_chains_guzzle_exception(): void { $exception = $this->failedCall( @@ -25,56 +20,36 @@ public function test_plain_text_error_keeps_http_status_and_chains_guzzle_except $this->assertInstanceOf(ClientException::class, $exception->getPrevious()); } - #[DataProvider('statuses')] - public function test_json_status_error_keeps_payload_code(array $status): void + public function test_status_body_code_wins_over_http_status(): void { $exception = $this->failedCall( - new Response($status['code'], ['Content-Type' => 'application/json'], json_encode($status)) + new Response(400, ['Content-Type' => 'application/json'], '{"kind":"Status","apiVersion":"v1","status":"Failure","reason":"Conflict","code":409}') ); - $this->assertSame($status['code'], $exception->getCode()); - $this->assertSame($status, $exception->getPayload()); + $this->assertSame(409, $exception->getCode()); + $this->assertSame( + ['kind' => 'Status', 'apiVersion' => 'v1', 'status' => 'Failure', 'reason' => 'Conflict', 'code' => 409], + $exception->getPayload() + ); } - public static function statuses(): iterable + public function test_zero_status_body_code_falls_back_to_http_status(): void { - yield 'not found' => [[ - 'kind' => 'Status', - 'apiVersion' => 'v1', - 'metadata' => [], - 'status' => 'Failure', - 'message' => 'volumesnapshots.snapshot.storage.k8s.io "branch" not found', - 'reason' => 'NotFound', - 'details' => ['name' => 'branch', 'group' => 'snapshot.storage.k8s.io', 'kind' => 'volumesnapshots'], - 'code' => 404, - ]]; - - yield 'conflict' => [[ - 'kind' => 'Status', - 'apiVersion' => 'v1', - 'metadata' => [], - 'status' => 'Failure', - 'message' => 'volumesnapshots.snapshot.storage.k8s.io "branch" already exists', - 'reason' => 'AlreadyExists', - 'details' => ['name' => 'branch', 'group' => 'snapshot.storage.k8s.io', 'kind' => 'volumesnapshots'], - 'code' => 409, - ]]; - } + $exception = $this->failedCall( + new Response(404, ['Content-Type' => 'application/json'], '{"kind":"Status","apiVersion":"v1","status":"Failure","reason":"NotFound","code":0}') + ); - public function test_unserved_api_group_answers_not_found(): void - { - try { - $this->cluster->call('GET', self::UNSERVED_PATH); - $this->fail('Expected a KubernetesAPIException for an API group the cluster does not serve.'); - } catch (KubernetesAPIException $exception) { - $this->assertSame(404, $exception->getCode()); - } + $this->assertSame(404, $exception->getCode()); + $this->assertSame( + ['kind' => 'Status', 'apiVersion' => 'v1', 'status' => 'Failure', 'reason' => 'NotFound', 'code' => 0], + $exception->getPayload() + ); } private function failedCall(ResponseInterface $response): KubernetesAPIException { try { - $this->clusterRespondingWith($response)->call('GET', self::SNAPSHOT_PATH); + $this->clusterRespondingWith($response)->call('GET', '/apis/snapshot.storage.k8s.io/v1/namespaces/default/volumesnapshots/branch'); } catch (KubernetesAPIException $exception) { return $exception; }