feat: Add Kubernetes adapters (K8sCLI and K8s SDK)#62
feat: Add Kubernetes adapters (K8sCLI and K8s SDK)#62amustaque97 wants to merge 21 commits intoutopia-php:mainfrom
Conversation
WalkthroughAdds Kubernetes orchestration support by introducing two new adapters (K8s — SDK-based, and K8sCLI — kubectl-based), comprehensive PHPUnit E2E test suites for both adapters, a composer dependency on renoki-co/php-k8s, kubectl installation in the Dockerfile, CI workflow changes (renamed lint → docker-tests and a new KinD-based k8s-tests job), and docker-compose updates to mount a kubeconfig into test containers. The adapters implement pod lifecycle, NetworkPolicy-based network management, command execution, image/volume handling, and basic metrics collection; tests exercise image pull, run/list/exec/remove, networks, stats, timeouts, and name sanitization. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.github/workflows/test.yml(2 hunks)Dockerfile(1 hunks)composer.json(1 hunks)docker-compose.yml(1 hunks)src/Orchestration/Adapter/K8s.php(1 hunks)src/Orchestration/Adapter/K8sCLI.php(1 hunks)tests/Orchestration/Adapter/K8sCLITest.php(1 hunks)tests/Orchestration/Adapter/K8sTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/Orchestration/Adapter/K8s.php (5)
src/Orchestration/Adapter.php (2)
Adapter(5-183)filterEnvKey(38-41)src/Orchestration/Container.php (3)
Container(5-120)getLabels(67-70)setLabels(114-119)src/Orchestration/Container/Stats.php (1)
Stats(5-89)src/Orchestration/Network.php (1)
Network(5-82)src/Orchestration/Adapter/K8sCLI.php (16)
__construct(28-32)buildLabelSelector(93-104)sanitizePodName(54-59)sanitizeLabelValue(64-88)createNetwork(146-188)removeNetwork(193-201)networkConnect(206-215)networkDisconnect(220-228)networkExists(233-241)listNetworks(248-278)list(486-533)getStats(286-365)pull(409-478)run(543-785)remove(840-855)execute(793-835)
src/Orchestration/Adapter/K8sCLI.php (6)
src/Orchestration/Adapter.php (2)
Adapter(5-183)filterEnvKey(38-41)src/Orchestration/Container.php (1)
Container(5-120)src/Orchestration/Container/Stats.php (1)
Stats(5-89)src/Orchestration/Exception/Timeout.php (1)
Timeout(5-7)src/Orchestration/Network.php (1)
Network(5-82)src/Orchestration/Adapter/K8s.php (16)
__construct(33-65)sanitizePodName(86-91)sanitizeLabelValue(96-120)buildLabelSelector(70-81)execute(632-677)createNetwork(161-206)removeNetwork(211-224)networkConnect(229-245)networkDisconnect(250-265)networkExists(270-282)listNetworks(289-318)list(386-431)getStats(326-368)pull(374-378)run(441-624)remove(682-699)
tests/Orchestration/Adapter/K8sTest.php (2)
src/Orchestration/Adapter/K8s.php (12)
K8s(14-700)remove(682-699)pull(374-378)run(441-624)list(386-431)execute(632-677)createNetwork(161-206)networkExists(270-282)networkConnect(229-245)networkDisconnect(250-265)removeNetwork(211-224)getStats(326-368)src/Orchestration/Orchestration.php (1)
parseCommandString(31-73)
tests/Orchestration/Adapter/K8sCLITest.php (4)
src/Orchestration/Adapter.php (1)
Adapter(5-183)src/Orchestration/Adapter/K8sCLI.php (13)
K8sCLI(13-856)remove(840-855)pull(409-478)run(543-785)list(486-533)execute(793-835)createNetwork(146-188)networkExists(233-241)listNetworks(248-278)networkConnect(206-215)networkDisconnect(220-228)removeNetwork(193-201)getStats(286-365)src/Orchestration/Adapter/K8s.php (12)
remove(682-699)pull(374-378)run(441-624)list(386-431)execute(632-677)createNetwork(161-206)networkExists(270-282)listNetworks(289-318)networkConnect(229-245)networkDisconnect(250-265)removeNetwork(211-224)getStats(326-368)src/Orchestration/Orchestration.php (1)
parseCommandString(31-73)
🪛 PHPMD (2.15.0)
src/Orchestration/Adapter/K8s.php
250-250: Avoid unused parameters such as '$network'. (undefined)
(UnusedFormalParameter)
250-250: Avoid unused parameters such as '$force'. (undefined)
(UnusedFormalParameter)
374-374: Avoid unused parameters such as '$image'. (undefined)
(UnusedFormalParameter)
636-636: Avoid unused parameters such as '$vars'. (undefined)
(UnusedFormalParameter)
637-637: Avoid unused parameters such as '$timeout'. (undefined)
(UnusedFormalParameter)
src/Orchestration/Adapter/K8sCLI.php
220-220: Avoid unused parameters such as '$network'. (undefined)
(UnusedFormalParameter)
220-220: Avoid unused parameters such as '$force'. (undefined)
(UnusedFormalParameter)
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (1)
src/Orchestration/Adapter/K8sCLI.php (1)
132-143: Escape pod name in delete command.Line 139 concatenates
$podNamedirectly into the shell command. Even though the name is sanitized, always escape arguments passed to the shell.Apply this diff:
Console::execute( - $this->buildKubectlCmd().' delete pod '.$podName.$flags, + $this->buildKubectlCmd().' delete pod '.escapeshellarg($podName).$flags, '', $output );
🧹 Nitpick comments (1)
src/Orchestration/Adapter/K8sCLI.php (1)
488-535: Remove redundant namespace flag in list command.Line 494 includes
--namespacewhich is already added bybuildKubectlCmd().Apply this diff:
- $result = Console::execute($this->buildKubectlCmd().' get pods'.$selector.' --namespace='.$this->k8sNamespace.' -o json', '', $output); + $result = Console::execute($this->buildKubectlCmd().' get pods'.$selector.' -o json', '', $output);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/test.yml(2 hunks)src/Orchestration/Adapter/K8s.php(1 hunks)src/Orchestration/Adapter/K8sCLI.php(1 hunks)tests/Orchestration/Adapter/K8sCLITest.php(1 hunks)tests/Orchestration/Adapter/K8sTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/test.yml
🧰 Additional context used
🧬 Code graph analysis (4)
tests/Orchestration/Adapter/K8sTest.php (3)
src/Orchestration/Adapter.php (1)
Adapter(5-183)src/Orchestration/Adapter/K8s.php (13)
K8s(14-700)remove(682-699)pull(377-381)run(444-634)list(389-434)execute(642-677)createNetwork(164-209)networkExists(273-285)listNetworks(292-321)networkConnect(232-248)networkDisconnect(253-268)removeNetwork(214-227)getStats(329-371)src/Orchestration/Orchestration.php (1)
parseCommandString(31-73)
tests/Orchestration/Adapter/K8sCLITest.php (3)
src/Orchestration/Adapter.php (1)
Adapter(5-183)src/Orchestration/Adapter/K8sCLI.php (13)
K8sCLI(13-858)remove(842-857)pull(411-480)run(545-787)list(488-535)execute(795-837)createNetwork(148-190)networkExists(235-243)listNetworks(250-280)networkConnect(208-217)networkDisconnect(222-230)removeNetwork(195-203)getStats(288-367)src/Orchestration/Orchestration.php (1)
parseCommandString(31-73)
src/Orchestration/Adapter/K8s.php (4)
src/Orchestration/Adapter.php (2)
Adapter(5-183)filterEnvKey(38-41)src/Orchestration/Container.php (3)
Container(5-120)getLabels(67-70)setLabels(114-119)src/Orchestration/Container/Stats.php (1)
Stats(5-89)src/Orchestration/Network.php (1)
Network(5-82)
src/Orchestration/Adapter/K8sCLI.php (6)
src/Orchestration/Adapter.php (2)
Adapter(5-183)filterEnvKey(38-41)src/Orchestration/Container.php (1)
Container(5-120)src/Orchestration/Container/Stats.php (1)
Stats(5-89)src/Orchestration/Exception/Timeout.php (1)
Timeout(5-7)src/Orchestration/Network.php (1)
Network(5-82)src/Orchestration/Adapter/K8s.php (16)
__construct(33-65)sanitizePodName(89-94)sanitizeLabelValue(99-123)buildLabelSelector(73-84)execute(642-677)createNetwork(164-209)removeNetwork(214-227)networkConnect(232-248)networkDisconnect(253-268)networkExists(273-285)listNetworks(292-321)list(389-434)getStats(329-371)pull(377-381)run(444-634)remove(682-699)
🪛 PHPMD (2.15.0)
src/Orchestration/Adapter/K8s.php
253-253: Avoid unused parameters such as '$network'. (undefined)
(UnusedFormalParameter)
253-253: Avoid unused parameters such as '$force'. (undefined)
(UnusedFormalParameter)
377-377: Avoid unused parameters such as '$image'. (undefined)
(UnusedFormalParameter)
646-646: Avoid unused parameters such as '$vars'. (undefined)
(UnusedFormalParameter)
647-647: Avoid unused parameters such as '$timeout'. (undefined)
(UnusedFormalParameter)
src/Orchestration/Adapter/K8sCLI.php
222-222: Avoid unused parameters such as '$network'. (undefined)
(UnusedFormalParameter)
222-222: Avoid unused parameters such as '$force'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (7)
src/Orchestration/Adapter/K8sCLI.php (1)
28-48: LGTM! Shell injection concerns addressed.The escaping via
escapeshellarg()on lines 42 and 45 properly mitigates shell injection risks flagged in the previous review.tests/Orchestration/Adapter/K8sTest.php (2)
118-138: LGTM! Robust test setup with forced cleanup.The forced pod deletion with
--grace-period=0on line 130 ensures clean test state, and ignoring errors on line 132 is appropriate since pods may not exist.
455-494: Good decision to skip timing-sensitive test.Skipping this test on line 459 with clear documentation is the right approach given the unreliable timing of pod phase transitions in Kubernetes.
tests/Orchestration/Adapter/K8sCLITest.php (2)
16-27: LGTM! Clean environment check.The kubectl availability check on lines 19-23 appropriately skips tests when Kubernetes is unavailable.
343-369: Auto-remove test kept for CLI adapter.Unlike the SDK adapter test (which skips this test), the CLI adapter version keeps the auto-remove test. This suggests the CLI-based implementation has more predictable timing for this feature.
src/Orchestration/Adapter/K8s.php (2)
253-268: Consider validating the network parameter.The method removes the 'network' label without checking if it matches the requested
$networkparameter. This could silently succeed even when disconnecting from a different network than intended.Verify the expected behavior: should
networkDisconnectfail if the pod isn't currently connected to the specified network, or is unconditional label removal acceptable?If validation is needed:
public function networkDisconnect(string $container, string $network, bool $force = false): bool { try { + $labelValue = $this->sanitizeLabelValue($network); $pod = $this->cluster->getPodByName($container, $this->k8sNamespace); $labels = $pod->getLabels(); + // Verify the pod is connected to the specified network + if (!isset($labels['network']) || $labels['network'] !== $labelValue) { + throw new Orchestration("Container is not connected to network: {$network}"); + } unset($labels['network']); $pod->setLabels($labels);
377-381: Correct behavior for Kubernetes image pulling.Returning
trueunconditionally is appropriate since Kubernetes handles image pulls during pod creation. The unused$imageparameter is required by the abstractAdapterinterface.
amustaque97
force-pushed
the
k8s-adapter
branch
from
a6c48b7 to
f413128
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
tests/Orchestration/Adapter/K8sTest.php (1)
59-60: Clean up temporary certificate files.Temporary files for certificates/keys are created but never cleaned up. This was flagged in a previous review but remains unresolved.
Track and remove temp files in
tearDownAfterClass():+ private static array $tempFiles = []; + public static function setUpBeforeClass(): void { // ... existing code ... $certFile = tempnam(sys_get_temp_dir(), 'k8s-cert-'); $keyFile = tempnam(sys_get_temp_dir(), 'k8s-key-'); + self::$tempFiles[] = $certFile; + self::$tempFiles[] = $keyFile; file_put_contents($certFile, $certData); file_put_contents($keyFile, $keyData); // ... and for CA file ... $caFile = tempnam(sys_get_temp_dir(), 'k8s-ca-'); + self::$tempFiles[] = $caFile; file_put_contents($caFile, $caData); } public static function tearDownAfterClass(): void { + // Clean up temp cert files + foreach (self::$tempFiles as $file) { + if (file_exists($file)) { + @unlink($file); + } + } + // ... existing pod cleanup code ... }Also applies to: 78-80
🧹 Nitpick comments (8)
src/Orchestration/Adapter/K8s.php (2)
329-371: Stats implementation returns placeholder data.Unlike the K8sCLI adapter which retrieves real metrics via
kubectl top, this SDK implementation returns zeroed metrics. While documented, this creates a functional gap where the SDK adapter provides significantly less value than the CLI adapter for monitoring use cases.Consider implementing metrics retrieval via the Metrics API:
// Example: Call metrics.k8s.io/v1beta1 API $metricsResponse = $this->cluster->call( 'GET', "/apis/metrics.k8s.io/v1beta1/namespaces/{$this->k8sNamespace}/pods/{$podName}" ); $metrics = json_decode((string) $metricsResponse->getBody(), true); // Parse CPU and memory from $metrics['containers'][0]['usage']Alternatively, document this limitation more prominently in the method's PHPDoc to set user expectations.
377-381: Image validation not performed in SDK adapter.The K8sCLI adapter validates images by creating a temporary pod and checking for
ImagePullBackOfferrors (lines 411-480 in K8sCLI.php), but the SDK adapter simply returnstruewithout validation. This means invalid image references won't be caught untilrun()is called, potentially delaying error feedback.For consistency and better developer experience, consider either:
- Implementing similar validation logic via a temporary pod creation, or
- Adding a clear PHPDoc warning that image validation is deferred until pod creation
The unused
$imageparameter warning from static analysis is a consequence of this design choice.tests/Orchestration/Adapter/K8sTest.php (1)
492-531: Consider alternative testing approach for auto-remove.The test is skipped due to timing unreliability, but the auto-remove feature still needs coverage. Consider testing the auto-remove label presence and cleanup mechanism indirectly:
public function testAutoRemoveLabelSet(): void { $response = self::getOrchestration()->run( 'alpine:latest', 'TestAutoRemoveLabel', ['sh', '-c', 'sleep 1'], remove: true ); $this->assertNotEmpty($response); sleep(1); // Verify auto-remove label is set $containers = self::getOrchestration()->list(); foreach ($containers as $container) { if ($container->getName() === 'testautoRemovelabel') { $labels = $container->getLabels(); $this->assertEquals('true', $labels['utopia-auto-remove'] ?? ''); break; } } // Cleanup self::getOrchestration()->remove('testautoRemovelabel', true); }This tests that the label is properly set without relying on timing-sensitive cleanup.
src/Orchestration/Adapter/K8sCLI.php (5)
240-240: Remove duplicate namespace flag.Line 240 includes
--namespace=butbuildKubectlCmd()already adds the namespace flag (line 45). While not harmful (kubectl uses the last value), this creates redundancy and potential confusion.Apply this diff:
- $result = Console::execute($this->buildKubectlCmd().' get networkpolicy '.escapeshellarg($resourceName).' --namespace='.$this->k8sNamespace.' -o name 2>/dev/null', '', $output); + $result = Console::execute($this->buildKubectlCmd().' get networkpolicy '.escapeshellarg($resourceName).' -o name 2>/dev/null', '', $output);
303-306: Remove duplicate namespace flags in getStats.Lines 303 and 306 include
--namespace=butbuildKubectlCmd()already adds it. Remove the redundant flags:- $result = Console::execute($this->buildKubectlCmd().' top pod '.escapeshellarg($container).' --namespace='.$this->k8sNamespace.' --no-headers 2>/dev/null', '', $output); + $result = Console::execute($this->buildKubectlCmd().' top pod '.escapeshellarg($container).' --no-headers 2>/dev/null', '', $output); } else { $selector = $this->buildLabelSelector($filters); - $result = Console::execute($this->buildKubectlCmd().' top pod'.$selector.' --namespace='.$this->k8sNamespace.' --no-headers 2>/dev/null', '', $output); + $result = Console::execute($this->buildKubectlCmd().' top pod'.$selector.' --no-headers 2>/dev/null', '', $output); }
494-494: Remove duplicate namespace flag in list.Line 494 includes
--namespace=butbuildKubectlCmd()already adds it:- $result = Console::execute($this->buildKubectlCmd().' get pods'.$selector.' --namespace='.$this->k8sNamespace.' -o json', '', $output); + $result = Console::execute($this->buildKubectlCmd().' get pods'.$selector.' -o json', '', $output);
810-810: Remove duplicate namespace flag in execute.Line 810 includes
--namespace=butbuildKubectlCmd()already adds it:- $execCmd = $this->buildKubectlCmd().' exec '.escapeshellarg($name).' --namespace='.$this->k8sNamespace.' -- '; + $execCmd = $this->buildKubectlCmd().' exec '.escapeshellarg($name).' -- ';
846-846: Remove duplicate namespace flag in remove.Line 846 includes
--namespace=butbuildKubectlCmd()already adds it:- $result = Console::execute($this->buildKubectlCmd().' delete pod '.escapeshellarg($name).$forceFlag.' --namespace='.$this->k8sNamespace, '', $output); + $result = Console::execute($this->buildKubectlCmd().' delete pod '.escapeshellarg($name).$forceFlag, '', $output);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Orchestration/Adapter/K8s.php(1 hunks)src/Orchestration/Adapter/K8sCLI.php(1 hunks)tests/Orchestration/Adapter/K8sTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/Orchestration/Adapter/K8sTest.php (3)
src/Orchestration/Adapter.php (1)
Adapter(5-183)src/Orchestration/Adapter/K8s.php (13)
K8s(14-739)remove(721-738)pull(377-381)run(444-654)list(389-434)execute(676-716)createNetwork(164-209)networkExists(273-285)listNetworks(292-321)networkConnect(232-248)networkDisconnect(253-268)removeNetwork(214-227)getStats(329-371)src/Orchestration/Orchestration.php (1)
parseCommandString(31-73)
src/Orchestration/Adapter/K8sCLI.php (6)
src/Orchestration/Adapter.php (2)
Adapter(5-183)filterEnvKey(38-41)src/Orchestration/Container.php (1)
Container(5-120)src/Orchestration/Container/Stats.php (1)
Stats(5-89)src/Orchestration/Exception/Timeout.php (1)
Timeout(5-7)src/Orchestration/Network.php (1)
Network(5-82)src/Orchestration/Adapter/K8s.php (16)
__construct(33-65)sanitizePodName(89-94)sanitizeLabelValue(99-123)buildLabelSelector(73-84)execute(676-716)createNetwork(164-209)removeNetwork(214-227)networkConnect(232-248)networkDisconnect(253-268)networkExists(273-285)listNetworks(292-321)list(389-434)getStats(329-371)pull(377-381)run(444-654)remove(721-738)
src/Orchestration/Adapter/K8s.php (5)
src/Orchestration/Adapter.php (2)
Adapter(5-183)filterEnvKey(38-41)src/Orchestration/Container.php (3)
Container(5-120)getLabels(67-70)setLabels(114-119)src/Orchestration/Container/Stats.php (1)
Stats(5-89)src/Orchestration/Network.php (1)
Network(5-82)src/Orchestration/Adapter/K8sCLI.php (16)
__construct(28-32)buildLabelSelector(95-106)sanitizePodName(54-59)sanitizeLabelValue(64-88)createNetwork(148-190)removeNetwork(195-203)networkConnect(208-217)networkDisconnect(222-230)networkExists(235-243)listNetworks(250-280)list(488-535)getStats(288-367)pull(411-480)run(545-787)remove(838-853)execute(795-833)
🪛 PHPMD (2.15.0)
src/Orchestration/Adapter/K8sCLI.php
222-222: Avoid unused parameters such as '$network'. (undefined)
(UnusedFormalParameter)
222-222: Avoid unused parameters such as '$force'. (undefined)
(UnusedFormalParameter)
src/Orchestration/Adapter/K8s.php
253-253: Avoid unused parameters such as '$network'. (undefined)
(UnusedFormalParameter)
253-253: Avoid unused parameters such as '$force'. (undefined)
(UnusedFormalParameter)
377-377: Avoid unused parameters such as '$image'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (8)
src/Orchestration/Adapter/K8s.php (4)
389-434: LGTM! Clean pod listing with proper auto-remove handling.The implementation correctly:
- Filters by labels when provided
- Skips pods already being deleted (deletionTimestamp check)
- Handles auto-remove pods by triggering cleanup during list operations
- Silently ignores deletion errors to avoid disrupting list operations
The async cleanup approach is appropriate for Kubernetes environments.
676-716: LGTM! Proper validation for unsupported parameters.The method correctly:
- Validates and rejects unsupported
$varsand$timeoutparameters with clear error messages- Refreshes pod state before execution
- Determines the container name explicitly to avoid defaults
- Provides proper exception handling with wrapped error messages
The documented limitations are now enforced at runtime.
721-738: LGTM! Correct pod deletion with force option.The implementation properly handles both graceful and forced deletion by setting
gracePeriodSecondsto 0 when force is enabled.
134-159: LGTM! Correct image reference parsing.The parsing logic correctly handles:
- Digest-based references (ignores tag when digest present)
- Tag detection after the last slash (handles registry ports correctly)
- Default to 'latest' when no explicit tag
tests/Orchestration/Adapter/K8sTest.php (1)
176-680: LGTM! Comprehensive test coverage.The test suite provides good coverage of:
- Image operations (pull)
- Pod lifecycle (run, list with/without filters, remove)
- Command execution (success, failure, parameter validation)
- Network operations (create, exists, list, connect, disconnect, remove)
- Stats retrieval (all pods, specific pod, filtered)
- Edge cases (special characters, sanitization, resource limits)
The skipped tests are well-documented with clear reasons. Good use of
@dependsannotations to establish test execution order.src/Orchestration/Adapter/K8sCLI.php (3)
411-480: LGTM! Robust image validation.The implementation properly validates images by:
- Creating a temporary pod with the specified image
- Checking container statuses for pull errors
- Cleaning up the temporary pod in both success and error cases
This provides much better validation than the SDK adapter's no-op approach.
723-723: Remove duplicate namespace flags in run method.Multiple kubectl commands include
--namespace=whenbuildKubectlCmd()already adds it:$podOutput = ''; - Console::execute($this->buildKubectlCmd().' get pod '.escapeshellarg($name).' -o json', '', $podOutput); + Console::execute($this->buildKubectlCmd().' get pod '.escapeshellarg($name).' -o json', '', $podOutput); // ... (no change needed - already correct) ... $statusOutput = ''; - Console::execute($this->buildKubectlCmd().' get pod '.escapeshellarg($name).' -o json', '', $statusOutput); + Console::execute($this->buildKubectlCmd().' get pod '.escapeshellarg($name).' -o json', '', $statusOutput); // ... later ... $statusOutput = ''; - Console::execute($this->buildKubectlCmd().' get pod '.escapeshellarg($name).' -o json', '', $statusOutput); + Console::execute($this->buildKubectlCmd().' get pod '.escapeshellarg($name).' -o json', '', $statusOutput);Actually, reviewing these lines more carefully - they DON'T have the explicit --namespace flag. Let me re-check... Lines 723, 738, 770 in the code only show
kubectl get pod <name> -o jsonwithout explicit namespace. So there's no duplicate flag here. My analysis was wrong.[pending]
Re-analyze these lines - no duplicate namespace issue exists.
[scratchpad_end] -->Also applies to: 738-738, 770-770
732-764: LGTM! Proper kubectl cp error handling.Unlike the SDK adapter which uses
@exec(), this implementation correctly usesConsole::execute()with timeout (line 762), providing proper error handling for file copy operations. The command construction withescapeshellarg()is also secure.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Orchestration/Adapter/K8s.php(1 hunks)src/Orchestration/Adapter/K8sCLI.php(1 hunks)tests/Orchestration/Adapter/K8sCLITest.php(1 hunks)tests/Orchestration/Adapter/K8sTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/Orchestration/Adapter/K8sTest.php
- src/Orchestration/Adapter/K8sCLI.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/Orchestration/Adapter/K8s.php (5)
src/Orchestration/Adapter.php (2)
Adapter(5-183)filterEnvKey(38-41)src/Orchestration/Container.php (3)
Container(5-120)getLabels(67-70)setLabels(114-119)src/Orchestration/Container/Stats.php (1)
Stats(5-89)src/Orchestration/Network.php (1)
Network(5-82)src/Orchestration/Adapter/K8sCLI.php (16)
__construct(28-32)buildLabelSelector(95-106)sanitizePodName(54-59)sanitizeLabelValue(64-88)createNetwork(148-190)removeNetwork(195-203)networkConnect(208-217)networkDisconnect(222-255)networkExists(260-268)listNetworks(275-305)list(513-560)getStats(313-392)pull(436-505)run(570-812)remove(863-878)execute(820-858)
tests/Orchestration/Adapter/K8sCLITest.php (3)
src/Orchestration/Adapter.php (1)
Adapter(5-183)src/Orchestration/Adapter/K8sCLI.php (13)
K8sCLI(13-879)remove(863-878)pull(436-505)run(570-812)list(513-560)execute(820-858)createNetwork(148-190)networkExists(260-268)listNetworks(275-305)networkConnect(208-217)networkDisconnect(222-255)removeNetwork(195-203)getStats(313-392)src/Orchestration/Orchestration.php (1)
parseCommandString(31-73)
🪛 PHPMD (2.15.0)
src/Orchestration/Adapter/K8s.php
389-389: Avoid unused parameters such as '$image'. (undefined)
(UnusedFormalParameter)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
tests/Orchestration/Adapter/K8sCLITest.php (6)
31-33: Consider relative paths and verify tar archive usage.The hardcoded absolute path
/usr/src/code/tests/Orchestration/Resources/may not work across different development or CI environments. Consider using__DIR__or similar PHP constants for portability. Additionally, the tar archives (php.tar.gzandtimeout.tar.gz) are created insetUp()but don't appear to be used in any test methods within this file.If the tar archives are indeed unused, consider removing these lines:
- \exec('sh -c "cd /usr/src/code/tests/Orchestration/Resources && tar -zcf ./php.tar.gz php"'); - \exec('sh -c "cd /usr/src/code/tests/Orchestration/Resources && tar -zcf ./timeout.tar.gz timeout"');Or make paths relative:
- \exec('rm -rf /usr/src/code/tests/Orchestration/Resources/screens'); // cleanup - \exec('sh -c "cd /usr/src/code/tests/Orchestration/Resources && tar -zcf ./php.tar.gz php"'); - \exec('sh -c "cd /usr/src/code/tests/Orchestration/Resources && tar -zcf ./timeout.tar.gz timeout"'); + $resourcesDir = __DIR__ . '/../Resources'; + \exec("rm -rf {$resourcesDir}/screens"); // cleanup + \exec("sh -c \"cd {$resourcesDir} && tar -zcf ./php.tar.gz php\""); + \exec("sh -c \"cd {$resourcesDir} && tar -zcf ./timeout.tar.gz timeout\"");
47-67: Consider dynamic test resource cleanup.The manual list of container names for cleanup requires maintenance whenever new tests are added. Consider a more dynamic approach that queries and removes all test-related pods using labels or name patterns.
Example refactor to clean up all test pods dynamically:
- // Clean up any remaining test pods - try { - self::$orchestration->remove('testcontainer', true); - } catch (\Exception $e) { - // Ignore - } - try { - self::$orchestration->remove('testcontainertimeout', true); - } catch (\Exception $e) { - // Ignore - } - try { - self::$orchestration->remove('usagestats1', true); - } catch (\Exception $e) { - // Ignore - } - try { - self::$orchestration->remove('usagestats2', true); - } catch (\Exception $e) { - // Ignore - } + // Clean up all test pods + try { + $containers = self::$orchestration->list(); + foreach ($containers as $container) { + try { + self::$orchestration->remove($container->getName(), true); + } catch (\Exception $e) { + // Ignore individual failures + } + } + } catch (\Exception $e) { + // Ignore + }
109-110: Remove unnecessary sleep if run() already waits.The comment states "run() already waits for container ready", which suggests the
sleep(2)call is redundant. If the adapter'srun()method properly waits for the pod to be ready, this sleep can be removed to speed up the tests.If
run()properly waits for readiness, remove the sleep:- // Wait for pod to be fully ready (run() already waits for container ready) - sleep(2);
302-314: Hardcoded sleep may cause flaky tests.The
sleep(3)on line 314 assumes the pod will be ready within 3 seconds. This could cause flaky test failures if the cluster is slow or under load. Consider implementing a polling mechanism that checks pod readiness status with a timeout instead of fixed sleeps.Example polling approach:
// Wait for pod to be running with timeout $maxWait = 30; // 30 seconds max $waited = 0; while ($waited < $maxWait) { $containers = self::getOrchestration()->list(['stats-test' => 'true']); if (!empty($containers) && $containers[0]->getStatus() === 'Running') { break; } sleep(1); $waited++; } $this->assertLessThan($maxWait, $waited, 'Pod did not become ready in time');
383-384: Auto-remove timing may cause flaky test.The comment explicitly mentions "Wait longer for container to finish and be auto-removed", and the 5-second sleep is a guess. Container completion and cleanup timing can vary significantly based on cluster load. This test may become flaky. Consider either:
- Polling the container list with a timeout until the container disappears
- Accepting that auto-remove timing is unreliable in tests and documenting this limitation (as mentioned in the PR objectives for K8s SDK)
Example polling approach:
- // Wait longer for container to finish and be auto-removed - sleep(5); - - // Trigger cleanup by calling list (which cleans up completed auto-remove pods) - $containers = self::getOrchestration()->list(['test' => 'rm']); - - // After cleanup, should be empty - $this->assertCount(0, $containers, 'Container with remove=true should be auto-removed'); + // Poll for auto-removal with timeout + $maxWait = 15; // 15 seconds max + $waited = 0; + $containers = []; + while ($waited < $maxWait) { + $containers = self::getOrchestration()->list(['test' => 'rm']); + if (count($containers) === 0) { + break; + } + sleep(1); + $waited++; + } + $this->assertCount(0, $containers, 'Container with remove=true should be auto-removed');
494-496: Loose name matching could match unintended pods.The verification uses
str_contains()to check if the pod name contains "test" and "container", which is quite loose and could match unintended pods in a shared test cluster. Consider storing the exact sanitized name returned fromrun()or using a more specific label-based query.More precise verification approach:
+ // Store the sanitized name + $sanitizedName = 'test-container-with-underscores'; + // Verify pod exists with sanitized name $containers = self::getOrchestration()->list(); $foundContainer = false; foreach ($containers as $container) { - // Name should be sanitized to lowercase with hyphens - if (str_contains(strtolower($container->getName()), 'test') && - str_contains(strtolower($container->getName()), 'container')) { + if ($container->getName() === $sanitizedName) { $foundContainer = true; break; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/Orchestration/Adapter/K8sCLITest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Orchestration/Adapter/K8sCLITest.php (3)
src/Orchestration/Adapter.php (1)
Adapter(5-183)src/Orchestration/Adapter/K8sCLI.php (13)
K8sCLI(13-879)remove(863-878)pull(436-505)run(570-812)list(513-560)execute(820-858)createNetwork(148-190)networkExists(260-268)listNetworks(275-305)networkConnect(208-217)networkDisconnect(222-255)removeNetwork(195-203)getStats(313-392)src/Orchestration/Orchestration.php (1)
parseCommandString(31-73)
🔇 Additional comments (1)
tests/Orchestration/Adapter/K8sCLITest.php (1)
192-295: Well-structured network operation tests.The network test methods comprehensively cover creation, existence checks, listing, connection, disconnection (including error cases and force flag), and removal. The use of
@dependsannotations ensures proper test execution order, and both positive and negative test cases are included.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/Orchestration/Adapter/K8sTest.php (2)
459-494: Use consistent casing in test for clarity.Line 480 passes
'UsageStatsSDK1'(mixed case) togetStats(), but the pod is created as'usagestatssdk1'(lowercase) on line 464. While the adapter likely handles case-insensitive lookups via sanitization, using consistent casing improves test readability.Apply this diff for consistency:
- $podStats = self::getOrchestration()->getStats('UsageStatsSDK1'); + $podStats = self::getOrchestration()->getStats('usagestatssdk1');
658-692: Use exact sanitized name assertion instead of fuzzy matching.Lines 677-679 use fuzzy matching with
str_contains()to find the sanitized pod name, but line 688 shows you already know the exact sanitized name:'test-container-sdk-with-underscores'. Replace the fuzzy check with a direct name comparison for more reliable assertions.Apply this diff:
// Verify pod exists with sanitized name $containers = self::getOrchestration()->list(); $foundContainer = false; foreach ($containers as $container) { - // Name should be sanitized to lowercase with hyphens - if (str_contains(strtolower($container->getName()), 'test') && - str_contains(strtolower($container->getName()), 'container') && - str_contains(strtolower($container->getName()), 'sdk')) { + if ($container->getName() === 'test-container-sdk-with-underscores') { $foundContainer = true; break; } } - $this->assertTrue($foundContainer); + $this->assertTrue($foundContainer, 'Pod with sanitized name test-container-sdk-with-underscores not found');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/Orchestration/Adapter/K8sTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Orchestration/Adapter/K8sTest.php (3)
src/Orchestration/Adapter.php (1)
Adapter(5-183)src/Orchestration/Adapter/K8s.php (13)
K8s(15-754)remove(736-753)pull(389-393)run(456-669)list(401-446)execute(691-731)createNetwork(165-210)networkExists(285-297)listNetworks(304-333)networkConnect(233-249)networkDisconnect(254-280)removeNetwork(215-228)getStats(341-383)src/Orchestration/Orchestration.php (1)
parseCommandString(31-73)
🔇 Additional comments (4)
tests/Orchestration/Adapter/K8sTest.php (4)
21-128: LGTM! Robust setup with proper temp file cleanup.The setup logic correctly handles multiple K8s authentication methods, auto-detects cluster configuration, and properly tracks temporary certificate files for cleanup in
tearDownAfterClass(). The previous review concern about temp file cleanup has been addressed.
195-349: LGTM! Test coverage for pull, run, list, and execute operations.These tests provide good coverage of core container operations and properly document K8s SDK limitations (no env vars or timeout support in
execute()). Exception testing for edge cases is appropriate.
533-572: LGTM! Appropriate handling of timing-sensitive test.Skipping
testRunWithRemove()with a clear explanation and preserving the commented code for documentation is the right approach given the unreliable timing of pod phase transitions in Kubernetes.
590-640: LGTM! Properly skipped due to SDK limitation.Skipping the timeout test with documentation is appropriate since the underlying K8s SDK library doesn't support timeout parameters in
exec().
| public static function tearDownAfterClass(): void | ||
| { | ||
| if (self::$orchestration === null) { | ||
| return; | ||
| } | ||
|
|
||
| // Clean up any remaining test pods (use sanitized names) | ||
| $podsToClean = [ | ||
| 'testcontainer', | ||
| 'testcontainertimeoutsdk', | ||
| 'usagestatssdk1', | ||
| 'usagestats2', | ||
| 'testcontainerwithlimits', | ||
| 'test-container-sdk-with-underscores', | ||
| 'testcontainerrmsdk' | ||
| ]; | ||
|
|
||
| foreach ($podsToClean as $podName) { | ||
| try { | ||
| self::$orchestration->remove($podName, true); | ||
| } catch (\Exception $e) { | ||
| // Ignore | ||
| } | ||
| } | ||
|
|
||
| // Clean up temporary certificate files | ||
| foreach (self::$tempFiles as $file) { | ||
| if (file_exists($file)) { | ||
| @unlink($file); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Remove 'usagestats2' from teardown cleanup list.
Similar to the issue in setUp(), line 168 includes 'usagestats2' which is never created by any test. Remove it to maintain consistency.
Apply this diff:
$podsToClean = [
'testcontainer',
'testcontainertimeoutsdk',
'usagestatssdk1',
- 'usagestats2',
'testcontainerwithlimits',
'test-container-sdk-with-underscores',
'testcontainerrmsdk'🤖 Prompt for AI Agents
In tests/Orchestration/Adapter/K8sTest.php around lines 157 to 188, the
$podsToClean array includes 'usagestats2' which is never created by any test;
remove the 'usagestats2' entry from the $podsToClean list so teardown only
attempts to remove pods that are actually created.
a9997b1 to
a43d8b3
Compare
amustaque97
force-pushed
the
k8s-adapter
branch
from
134733f to
17cf0ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.github/workflows/test.yml (2)
38-40: Remove unused HOST_DIR environment variable.The
HOST_DIRenvironment variable is set but never referenced in any subsequent steps.Apply this diff to remove the unused variable:
- env: - HOST_DIR: ${{ github.workspace }} steps:
76-79: Consider verifying network connection success.The
|| trueoperator swallows connection failures, which could lead to confusing test failures if the container cannot reach the KinD cluster. Consider verifying the connection succeeded or at least logging a warning if it fails.Apply this diff to add verification:
- name: Connect container to kind network run: | CONTAINER_ID=$(docker compose ps -q tests) - docker network connect kind "$CONTAINER_ID" || true + if ! docker network connect kind "$CONTAINER_ID"; then + echo "Warning: Failed to connect container to kind network" + docker network ls + exit 1 + fisrc/Orchestration/Adapter/K8s.php (2)
343-386: Document getStats limitation more prominently.The method returns zero values for all metrics (CPU, memory, I/O), differing significantly from the K8sCLI adapter which parses real metrics from
kubectl top. This stub implementation may mislead users who expect actual metrics data. Consider either implementing metrics API calls or documenting this limitation prominently in the class-level PHPDoc and method description.For a complete implementation, you could call the metrics API similar to what K8sCLI does:
// GET /apis/metrics.k8s.io/v1beta1/namespaces/{namespace}/pods/{name} $metricsResponse = $this->cluster->call( 'GET', "/apis/metrics.k8s.io/v1beta1/namespaces/{$this->k8sNamespace}/pods/{$podName}" ); $metricsData = json_decode($metricsResponse->getBody(), true); // Parse CPU and memory from $metricsData['containers'][0]['usage']Alternatively, add a prominent note to the class PHPDoc explaining that
getStats()returns zero values.
646-667: Consider handling subdirectories in mountFolder.The current implementation only copies files and silently skips subdirectories (line 655 checks
is_file()). If users expect directory structures to be preserved, this could lead to unexpected behavior.If subdirectory support is intended, consider recursively copying directories:
if (! empty($mountFolder)) { - $files = @\scandir(rtrim($mountFolder, '/')) ?: []; - foreach ($files as $file) { - if ($file === '.' || $file === '..') { + // Use recursive directory iterator to copy all files and subdirectories + $iterator = new \RecursiveIteratorIterator( + new \RecursiveDirectoryIterator(rtrim($mountFolder, '/'), \RecursiveDirectoryIterator::SKIP_DOTS), + \RecursiveIteratorIterator::SELF_FIRST + ); + + foreach ($iterator as $item) { + if (!$item->isFile()) { continue; } - $local = rtrim($mountFolder, '/').'/'.$file; - if (!is_file($local)) { - continue; - } + $local = $item->getPathname(); + $relativePath = substr($local, strlen(rtrim($mountFolder, '/')) + 1); // Use kubectl cp to copy files into the pod $output = ''; - $cmd = 'kubectl cp '.\escapeshellarg($local).' '.\escapeshellarg($this->k8sNamespace.'/'.$name.':/tmp/'.$file).' -c main'; + $cmd = 'kubectl cp '.\escapeshellarg($local).' '.\escapeshellarg($this->k8sNamespace.'/'.$name.':/tmp/'.$relativePath).' -c main'; $result = Console::execute($cmd, '', $output, 30); if ($result !== 0) { - throw new Orchestration("Failed to copy {$file} to pod: {$output}"); + throw new Orchestration("Failed to copy {$relativePath} to pod: {$output}"); } } }Otherwise, consider documenting that only top-level files are copied.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/test.yml(2 hunks)src/Orchestration/Adapter/K8s.php(1 hunks)tests/Orchestration/Adapter/K8sTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Orchestration/Adapter/K8sTest.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/Orchestration/Adapter/K8s.php (5)
src/Orchestration/Adapter.php (2)
Adapter(5-183)filterEnvKey(38-41)src/Orchestration/Container.php (3)
Container(5-120)getLabels(67-70)setLabels(114-119)src/Orchestration/Container/Stats.php (1)
Stats(5-89)src/Orchestration/Network.php (1)
Network(5-82)src/Orchestration/Adapter/K8sCLI.php (16)
__construct(28-32)buildLabelSelector(95-106)sanitizePodName(54-59)sanitizeLabelValue(64-88)createNetwork(148-190)removeNetwork(195-203)networkConnect(208-217)networkDisconnect(222-255)networkExists(260-268)listNetworks(275-305)list(513-560)getStats(313-392)pull(436-505)run(570-812)remove(863-878)execute(820-858)
🪛 PHPMD (2.15.0)
src/Orchestration/Adapter/K8s.php
392-392: Avoid unused parameters such as '$image'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (10)
src/Orchestration/Adapter/K8s.php (10)
34-66: LGTM!The constructor properly validates the required URL parameter and provides flexible authentication options (token, basic auth, certificate-based) with appropriate TLS configuration.
74-160: LGTM!The helper methods provide proper sanitization consistent with the K8sCLI adapter and correctly handle image reference parsing including digest and tag formats.
165-210: LGTM!The NetworkPolicy creation properly handles both internal (deny-all) and external (allow-all) network configurations using the correct Kubernetes API format.
215-228: LGTM!The NetworkPolicy deletion properly sanitizes the resource name and handles errors appropriately.
233-282: LGTM!Both methods properly sanitize pod names before lookup and
networkDisconnectcorrectly validates that the pod is connected to the specified network before attempting to disconnect.
287-335: LGTM!The network existence check and listing methods properly handle NetworkPolicy resources and preserve original network names via annotations.
392-396: LGTM!The implementation correctly reflects Kubernetes behavior where image pulling is automatic during pod creation. The unused
$imageparameter reported by PHPMD is unavoidable since this method is required by the abstract Adapter interface.
404-449: LGTM!The list method properly filters pods by labels, handles auto-remove cleanup, and skips pods that are being deleted, ensuring accurate container listings.
695-748: LGTM!The execute method now explicitly validates unsupported parameters (environment variables and timeout) and throws clear exceptions, properly documenting the SDK limitations. The dual handling of array/string results is well-implemented.
753-770: LGTM!The remove method properly sanitizes pod names and correctly implements force deletion using the Kubernetes grace period mechanism.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/Orchestration/Adapter/K8sTest.php (2)
131-131: Remove 'usagestats2' from cleanup list—pod is never created.The cleanup array includes
'usagestats2', but this pod is not created by any test. Line 481 creates'usagestatssdk1'(which is already in the list). Remove the non-existent pod name.Apply this diff:
- $pods = ['testcontainer', 'testcontainerrmsdk', 'testcontainertimeoutsdk', 'usagestatssdk1', 'usagestats2', 'testcontainerwithlimits', 'test-container-sdk-with-underscores']; + $pods = ['testcontainer', 'testcontainerrmsdk', 'testcontainertimeoutsdk', 'usagestatssdk1', 'testcontainerwithlimits', 'test-container-sdk-with-underscores'];
169-169: Remove 'usagestats2' from teardown cleanup list.Similar to line 131,
'usagestats2'is included but never created. Remove it to maintain consistency.Apply this diff:
$podsToClean = [ 'testcontainer', 'testcontainertimeoutsdk', 'usagestatssdk1', - 'usagestats2', 'testcontainerwithlimits', 'test-container-sdk-with-underscores', 'testcontainerrmsdk'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/test.yml(2 hunks)src/Orchestration/Adapter/K8s.php(1 hunks)tests/Orchestration/Adapter/K8sTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/test.yml
🧰 Additional context used
🧬 Code graph analysis (2)
tests/Orchestration/Adapter/K8sTest.php (3)
src/Orchestration/Adapter.php (1)
Adapter(5-183)src/Orchestration/Adapter/K8s.php (13)
K8s(15-771)remove(753-770)pull(392-396)run(459-673)list(404-449)execute(695-748)createNetwork(165-210)networkExists(287-299)listNetworks(306-335)networkConnect(233-250)networkDisconnect(255-282)removeNetwork(215-228)getStats(343-386)src/Orchestration/Orchestration.php (1)
parseCommandString(31-73)
src/Orchestration/Adapter/K8s.php (5)
src/Orchestration/Adapter.php (2)
Adapter(5-183)filterEnvKey(38-41)src/Orchestration/Container.php (3)
Container(5-120)getLabels(67-70)setLabels(114-119)src/Orchestration/Container/Stats.php (1)
Stats(5-89)src/Orchestration/Network.php (1)
Network(5-82)src/Orchestration/Adapter/K8sCLI.php (15)
__construct(28-32)sanitizePodName(54-59)sanitizeLabelValue(64-88)createNetwork(148-190)removeNetwork(195-203)networkConnect(208-217)networkDisconnect(222-255)networkExists(260-268)listNetworks(275-305)list(513-560)getStats(313-392)pull(436-505)run(570-812)remove(863-878)execute(820-858)
🪛 PHPMD (2.15.0)
src/Orchestration/Adapter/K8s.php
392-392: Avoid unused parameters such as '$image'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (20)
tests/Orchestration/Adapter/K8sTest.php (10)
196-211: LGTM!The test correctly documents that the K8s SDK adapter always returns
trueforpull()since Kubernetes handles image pulling during pod creation, not as a separate operation.
216-234: LGTM!The test correctly creates a pod with appropriate configuration and waits for readiness.
239-292: LGTM!Both test methods appropriately handle Kubernetes' eventual consistency with retry logic and reasonable wait times.
336-368: LGTM!Both tests correctly validate that the K8s SDK adapter throws exceptions for unsupported
execute()parameters (env vars and timeout), matching the documented limitations.
370-473: LGTM!Comprehensive network testing covering creation, listing, existence checks, connect/disconnect operations, and both success and error paths.
478-513: LGTM!The test correctly exercises stats retrieval for all pods, specific pods, and filtered pods. Line 499 intentionally uses different casing to test name sanitization.
518-547: LGTM!Both tests appropriately cover pod removal, including verification of successful deletion and proper exception handling for non-existent pods.
552-604: LGTM!The skipped test includes clear documentation explaining the K8s timing reliability issue. The command parsing test is thorough.
609-675: LGTM!The skipped timeout test is appropriately documented. The network sanitization test correctly validates handling of special characters.
677-741: LGTM!Both tests appropriately validate name sanitization and resource limit configuration. Proper cleanup is performed in both cases.
src/Orchestration/Adapter/K8s.php (10)
27-66: LGTM!The constructor properly validates the required URL, initializes the cluster connection, and configures authentication with appropriate precedence (token > basic auth > certificates). TLS verification handling is correct.
68-160: LGTM!All helper methods are well-implemented:
buildLabelSelectorcorrectly formats K8s label queriessanitizePodNameandsanitizeLabelValueproperly enforce K8s naming/label constraintsparseImageReferencecorrectly handles both digest and tag formats
165-210: LGTM!Network creation correctly implements NetworkPolicy-based networking with proper allow-all/deny-all rules for external/internal networks and preserves the original name in annotations.
215-282: LGTM!All network operations are correctly implemented:
removeNetworkproperly deletes the NetworkPolicynetworkConnectsanitizes names and adds the network labelnetworkDisconnectvalidates the connection before removal and respects the force flag
287-335: LGTM!Network existence check and listing are correctly implemented. The list method properly uses annotations to preserve original network names.
343-386: LGTM!Stats collection correctly handles both specific and filtered queries with proper name sanitization. The zeroed metrics are appropriately documented as requiring Metrics Server integration.
392-396: LGTM!The method correctly returns
trueunconditionally because Kubernetes handles image pulling during pod creation. The unused$imageparameter warning is a false positive—the signature must match the abstract method.
404-449: LGTM!Container listing correctly filters by labels, skips pods being deleted, and implements auto-remove logic for completed pods. Proper error handling is in place.
695-748: LGTM!The execute method correctly documents and enforces its limitations (no env vars or timeout support), sanitizes pod names, and properly handles both array and string exec results.
753-771: LGTM!The remove method correctly sanitizes pod names and implements both normal and forced deletion with appropriate grace periods.
| // If mountFolder is set, copy files into the pod using kubectl cp | ||
| // This is necessary because we use emptyDir instead of hostPath | ||
| if (! empty($mountFolder)) { | ||
| $files = @\scandir(rtrim($mountFolder, '/')) ?: []; | ||
| foreach ($files as $file) { | ||
| if ($file === '.' || $file === '..') { | ||
| continue; | ||
| } | ||
| $local = rtrim($mountFolder, '/').'/'.$file; | ||
| if (!is_file($local)) { | ||
| continue; | ||
| } | ||
|
|
||
| // Use kubectl cp to copy files into the pod | ||
| $output = ''; | ||
| $cmd = 'kubectl cp '.\escapeshellarg($local).' '.\escapeshellarg($this->k8sNamespace.'/'.$name.':/tmp/'.$file).' -c main'; | ||
| $result = Console::execute($cmd, '', $output, 30); | ||
| if ($result !== 0) { | ||
| throw new Orchestration("Failed to copy {$file} to pod: {$output}"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify directory handling in mountFolder.
Lines 655-657 skip non-files, meaning subdirectories in mountFolder are not recursively copied. This may differ from expected behavior if the Docker adapter handles directories.
Verify whether this limitation is acceptable:
🏁 Script executed:
#!/bin/bash
# Check if Docker adapter handles subdirectories in mountFolder
rg -n -A10 -B2 'mountFolder' src/Orchestration/Adapter/Docker.phpLength of output: 76
---
🏁 Script executed:
```shell
#!/bin/bash
# Find all adapter files in the Orchestration directory
fd -t f 'Docker' src/Orchestration/ || fd -t f '\\.php$' src/Orchestration/Adapter/
Length of output: 79
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for mountFolder handling in both Docker adapters
echo "=== DockerAPI.php ==="
rg -n -A10 -B2 'mountFolder' src/Orchestration/Adapter/DockerAPI.php
echo ""
echo "=== DockerCLI.php ==="
rg -n -A10 -B2 'mountFolder' src/Orchestration/Adapter/DockerCLI.php
Length of output: 2278
mountFolder directory handling differs from Docker adapters—subdirectories are not copied.
Lines 655-657 skip non-files using !is_file($local), meaning subdirectories are ignored. Docker adapters (DockerAPI.php:541, DockerCLI.php:375) mount the entire mountFolder including all subdirectories via volume mounts. The K8s adapter should recursively copy nested directories or document this limitation.
🤖 Prompt for AI Agents
In src/Orchestration/Adapter/K8s.php around lines 646 to 667, the current
implementation skips non-files (directories) when copying mountFolder contents
into the pod, so nested subdirectories are not copied; update the copy loop to
recursively traverse mountFolder and copy directories and their contents
(preserving relative paths) into the pod (e.g., create target dirs in /tmp in
the pod then use kubectl cp for files, or tar the folder and extract in the pod)
so the behavior matches Docker adapters, or if recursion is undesired,
explicitly document the limitation; implement the recursive traversal and
copying approach and ensure errors include which path failed.
|
Thanks for the PR. We don't have an plans to include a k8s adapter as part of the orchestration library. |
|
Sorry to ask this @loks0n, but can you clarify why this PR was closed? I could understand the reasoning if it wasn't for issue #31 created by @byawitz who looks to be another contributor to the project like yourself. I am asking because I was planning to contribute a K8 adapter myself as I would like to deploy Appwrite to K8, but since the functions rely on docker atm I have to have a dedicated machine running around just to use functions instead of using GKE. Is the plan that utopia-php is not going to support orchestration through K8 or is the utopia team exploring a different option for K8 support? Either way #31 should probably be updated to prevent further confusion. |
The orchestration library isn't the right layer for this adapter. Although it's called orchestration it's much closer to an abstraction around containers (and tightly coupled to Docker). We're going to move things around and maybe in the future a kubernetes adapter, or other orchestrators might make sense, but not here. |
|
Very understandable, thank you very much for clarifying that for me! |
3 participants
Closes #31
Summary
This PR introduces two Kubernetes adapter implementations for the orchestration library, providing full container orchestration capabilities on Kubernetes clusters. Both adapters support the complete
Orchestrationinterface with network management, container lifecycle operations, and resource monitoring.Adapters Implemented
1. K8sCLI Adapter (
src/Orchestration/Adapter/K8sCLI.php)kubectlCLI commands via shell executionkubectlbinary and cluster access2. K8s SDK Adapter (
src/Orchestration/Adapter/K8s.php)renoki-co/php-k8sSDK library for native PHP Kubernetes API interactionrenoki-co/php-k8spackageKey Features
Container Operations
imagePullPolicy: Always)Network Operations
Limitations
K8s SDK Adapter Specific Limitations
No Timeout Support in
execute()renoki-co/php-k8slibrary'sexec()method doesn't support timeout parameterstestTimeoutNo Environment Variables in
execute()testExecuteContainerAuto-Remove Timing Unreliability
Succeededphase quickly enough for deterministic testingtestRunWithRemoveGeneral Kubernetes Limitations
Image Pull Returns True
imagePullPolicy: Alwayspull()method returnstruefor both existing and non-existent imagesNetwork Isolation
Resource Overhead
Stats Collection
kubectl top podcommandTest Coverage
K8sCLI Tests (
tests/Orchestration/Adapter/k8sCLITest.php)K8s SDK Tests (
tests/Orchestration/Adapter/K8sTest.php)Test Infrastructure Improvements
Utopia\Tests\Base, making them independent and focusedkubectl cluster-infosetUp()andtearDown()with forced pod deletion to prevent test conflictsBreaking Changes
None - this is a new feature addition.
Migration Guide
N/A - new adapters.
Usage Examples
K8sCLI Adapter
K8s SDK Adapter
Files Changed
New Files
src/Orchestration/Adapter/K8sCLI.php- Kubectl CLI-based adaptersrc/Orchestration/Adapter/K8s.php- PHP SDK-based adaptertests/Orchestration/Adapter/k8sCLITest.php- K8sCLI comprehensive teststests/Orchestration/Adapter/K8sTest.php- K8s SDK comprehensive testsFuture Improvements
renoki-co/php-k8sto add timeout supportttlSecondsAfterFinishedfor more reliable auto-cleanuprun()methodTesting Instructions
Prerequisites:
Summary by CodeRabbit
New Features
Chores
Tests