From 837f5fd4df7f0c83f2acf8376ceedcb7309917a7 Mon Sep 17 00:00:00 2001 From: Oleksandr Piskun Date: Fri, 18 Sep 2026 10:51:10 +0000 Subject: [PATCH 01/10] fix(k8s): apply daemon registry mappings Signed-off-by: Oleksandr Piskun --- lib/DeployActions/DockerActions.php | 26 +-- lib/DeployActions/KubernetesActions.php | 22 ++- lib/Service/DaemonConfigService.php | 21 ++ tests/php/DeployActions/DockerActionsTest.php | 65 +++++++ .../DeployActions/KubernetesActionsTest.php | 180 ++++++++++++++++++ tests/php/Service/DaemonConfigServiceTest.php | 61 ++++++ 6 files changed, 344 insertions(+), 31 deletions(-) create mode 100644 tests/php/DeployActions/KubernetesActionsTest.php create mode 100644 tests/php/Service/DaemonConfigServiceTest.php diff --git a/lib/DeployActions/DockerActions.php b/lib/DeployActions/DockerActions.php index 61fc0e088..d169f2770 100644 --- a/lib/DeployActions/DockerActions.php +++ b/lib/DeployActions/DockerActions.php @@ -16,6 +16,7 @@ use OCA\AppAPI\Db\DaemonConfig; use OCA\AppAPI\Db\ExApp; use OCA\AppAPI\Service\AppAPICommonService; +use OCA\AppAPI\Service\DaemonConfigService; use OCA\AppAPI\Service\ExAppDeployOptionsService; use OCA\AppAPI\Service\ExAppService; use OCA\AppAPI\Service\HarpService; @@ -431,16 +432,7 @@ public function buildApiUrl(string $dockerUrl, string $route): string { } public function buildBaseImageName(array $imageParams, DaemonConfig $daemonConfig): string { - $deployConfig = $daemonConfig->getDeployConfig(); - if (isset($deployConfig['registries'])) { // custom Docker registry, overrides ExApp's image_src - foreach ($deployConfig['registries'] as $registry) { - if ($registry['from'] === $imageParams['image_src'] && $registry['to'] !== 'local') { // local target skips image pull, imageId should be unchanged - $imageParams['image_src'] = rtrim($registry['to'], '/'); - break; - } - } - } - return $imageParams['image_src'] . '/' + return DaemonConfigService::resolveImageRegistry($daemonConfig->getDeployConfig(), $imageParams['image_src']) . '/' . $imageParams['image_name'] . ':' . $imageParams['image_tag']; } @@ -449,23 +441,15 @@ private function buildExtendedImageName(array $imageParams, DaemonConfig $daemon if (empty($deployConfig['computeDevice']['id'])) { return null; } - if (isset($deployConfig['registries'])) { // custom Docker registry, overrides ExApp's image_src - foreach ($deployConfig['registries'] as $registry) { - if ($registry['from'] === $imageParams['image_src'] && $registry['to'] !== 'local') { // local target skips image pull, imageId should be unchanged - $imageParams['image_src'] = rtrim($registry['to'], '/'); - break; - } - } - } - return $imageParams['image_src'] . '/' - . $imageParams['image_name'] . ':' . $imageParams['image_tag'] . '-' . $daemonConfig->getDeployConfig()['computeDevice']['id']; + return DaemonConfigService::resolveImageRegistry($deployConfig, $imageParams['image_src']) . '/' + . $imageParams['image_name'] . ':' . $imageParams['image_tag'] . '-' . $deployConfig['computeDevice']['id']; } private function shouldPullImage(array $imageParams, DaemonConfig $daemonConfig): bool { $deployConfig = $daemonConfig->getDeployConfig(); if (isset($deployConfig['registries'])) { // custom Docker registry, overrides ExApp's image_src foreach ($deployConfig['registries'] as $registry) { - if ($registry['from'] === $imageParams['image_src'] && $registry['to'] === 'local') { // local target skips image pull, imageId should be unchanged + if (($registry['from'] ?? null) === $imageParams['image_src'] && ($registry['to'] ?? null) === 'local') { // local target skips image pull, imageId should be unchanged return false; } } diff --git a/lib/DeployActions/KubernetesActions.php b/lib/DeployActions/KubernetesActions.php index bf7c6eb21..72201162d 100644 --- a/lib/DeployActions/KubernetesActions.php +++ b/lib/DeployActions/KubernetesActions.php @@ -16,6 +16,7 @@ use OCA\AppAPI\Db\DaemonConfig; use OCA\AppAPI\Db\ExApp; use OCA\AppAPI\Service\AppAPICommonService; +use OCA\AppAPI\Service\DaemonConfigService; use OCA\AppAPI\Service\ExAppDeployOptionsService; use OCA\AppAPI\Service\ExAppService; use OCP\App\IAppManager; @@ -85,13 +86,13 @@ public function deployExApp(ExApp $exApp, DaemonConfig $daemonConfig, array $par $roles = $params['k8s_service_roles'] ?? []; if (empty($roles)) { - return $this->deploySingleExApp($exApp, $harpUrl, $params); + return $this->deploySingleExApp($exApp, $daemonConfig, $harpUrl, $params); } - return $this->deployMultiRoleExApp($exApp, $harpUrl, $params, $roles); + return $this->deployMultiRoleExApp($exApp, $daemonConfig, $harpUrl, $params, $roles); } - private function deploySingleExApp(ExApp $exApp, string $harpUrl, array $params): string { + private function deploySingleExApp(ExApp $exApp, DaemonConfig $daemonConfig, string $harpUrl, array $params): string { $exAppName = $params['container_params']['name']; $instanceId = ''; @@ -112,7 +113,7 @@ private function deploySingleExApp(ExApp $exApp, string $harpUrl, array $params) $this->exAppService->setAppDeployProgress($exApp, 50); - $error = $this->createExApp($harpUrl, $exAppName, $instanceId, $params); + $error = $this->createExApp($daemonConfig, $harpUrl, $exAppName, $instanceId, $params); if ($error) { return $error; } @@ -146,7 +147,7 @@ private function deploySingleExApp(ExApp $exApp, string $harpUrl, array $params) * * @param array $roles Array of role definitions from k8s-service-roles */ - private function deployMultiRoleExApp(ExApp $exApp, string $harpUrl, array $params, array $roles): string { + private function deployMultiRoleExApp(ExApp $exApp, DaemonConfig $daemonConfig, string $harpUrl, array $params, array $roles): string { $exAppName = $params['container_params']['name']; $instanceId = ''; $totalRoles = count($roles); @@ -184,7 +185,7 @@ private function deployMultiRoleExApp(ExApp $exApp, string $harpUrl, array $para $this->logger->info(sprintf('Creating K8s deployment for ExApp "%s" role "%s" (%d/%d).', $exAppName, $roleSuffix, $roleIndex + 1, $totalRoles)); - $error = $this->createExApp($harpUrl, $exAppName, $instanceId, $roleParams, $roleSuffix); + $error = $this->createExApp($daemonConfig, $harpUrl, $exAppName, $instanceId, $roleParams, $roleSuffix); if ($error) { $this->rollbackDeployedRoles($harpUrl, $exAppName, $deployedRoles); return $error; @@ -342,14 +343,14 @@ private function checkExists(string $harpUrl, string $exAppName, string $instanc } } - private function createExApp(string $harpUrl, string $exAppName, string $instanceId, array $params, string $roleSuffix = ''): string { + private function createExApp(DaemonConfig $daemonConfig, string $harpUrl, string $exAppName, string $instanceId, array $params, string $roleSuffix = ''): string { $computeDevice = 'cpu'; if (isset($params['container_params']['computeDevice']['id'])) { $computeDevice = $params['container_params']['computeDevice']['id']; } $createPayload = $this->buildNamePayload($exAppName, $instanceId, $roleSuffix); - $createPayload['image'] = $this->buildImageName($params['image_params']); + $createPayload['image'] = $this->buildImageName($params['image_params'], $daemonConfig); $createPayload['environment_variables'] = $params['container_params']['env'] ?? []; $createPayload['compute_device'] = $computeDevice; @@ -767,8 +768,9 @@ public function buildHarpK8sUrl(DaemonConfig $daemonConfig): string { return rtrim($url, '/') . '/exapps/app_api/k8s'; } - private function buildImageName(array $imageParams): string { - return $imageParams['image_src'] . '/' . $imageParams['image_name'] . ':' . $imageParams['image_tag']; + public function buildImageName(array $imageParams, DaemonConfig $daemonConfig): string { + return DaemonConfigService::resolveImageRegistry($daemonConfig->getDeployConfig(), $imageParams['image_src']) . '/' + . $imageParams['image_name'] . ':' . $imageParams['image_tag']; } public function initGuzzleClient(DaemonConfig $daemonConfig): void { diff --git a/lib/Service/DaemonConfigService.php b/lib/Service/DaemonConfigService.php index c61a011db..d182d1b52 100644 --- a/lib/Service/DaemonConfigService.php +++ b/lib/Service/DaemonConfigService.php @@ -224,4 +224,25 @@ public function removeDockerRegistry(DaemonConfig $daemonConfig, array $registry return null; } } + + /** + * Registry to take an ExApp image from, after the registry mappings of the daemon are applied. + * + * The first mapping of the registry with a target other than "local" wins, entries without a usable target + * are ignored. "local" never renames the image: Docker daemons skip the pull for it, on Kubernetes it changes + * nothing and the kubelet pulls the image if the node does not have it. + */ + public static function resolveImageRegistry(array $deployConfig, string $imageRegistry): string { + foreach ($deployConfig['registries'] ?? [] as $registry) { + $target = $registry['to'] ?? null; + if (($registry['from'] ?? null) !== $imageRegistry || !is_string($target) || $target === 'local') { + continue; + } + $target = rtrim($target, '/'); + if ($target !== '') { + return $target; + } + } + return $imageRegistry; + } } diff --git a/tests/php/DeployActions/DockerActionsTest.php b/tests/php/DeployActions/DockerActionsTest.php index 0f81aaeb6..2e2c0cbb8 100644 --- a/tests/php/DeployActions/DockerActionsTest.php +++ b/tests/php/DeployActions/DockerActionsTest.php @@ -10,6 +10,7 @@ namespace OCA\AppAPI\Tests\php\DeployActions; use OCA\AppAPI\AppInfo\Application; +use OCA\AppAPI\Db\DaemonConfig; use OCA\AppAPI\DeployActions\DockerActions; use OCA\AppAPI\Service\AppAPICommonService; use OCA\AppAPI\Service\ExAppDeployOptionsService; @@ -21,9 +22,11 @@ use OCP\ITempManager; use OCP\IURLGenerator; use OCP\Security\ICrypto; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Psr\Log\LoggerInterface; +use ReflectionMethod; class DockerActionsTest extends TestCase { private DockerActions $dockerActions; @@ -99,4 +102,66 @@ public function testBuildApiUrlWithContainerRoute(): void { self::assertSame('http://localhost:8780/v1.41/containers/nc_app_test/json', $url); } + + public static function imageNameProvider(): array { + return [ + 'no registry mappings' => [[], 'ghcr.io'], + 'mapped registry' => [[['from' => 'ghcr.io', 'to' => 'registry.example.com/']], 'registry.example.com'], + 'mapping of another registry' => [[['from' => 'docker.io', 'to' => 'registry.example.com']], 'ghcr.io'], + 'local keeps the image name' => [[['from' => 'ghcr.io', 'to' => 'local']], 'ghcr.io'], + ]; + } + + #[DataProvider('imageNameProvider')] + public function testBuildBaseImageName(array $registries, string $expectedRegistry): void { + $imageParams = ['image_src' => 'ghcr.io', 'image_name' => 'nextcloud/test-deploy', 'image_tag' => 'release']; + $daemonConfig = new DaemonConfig(['deploy_config' => ['registries' => $registries]]); + + self::assertSame( + $expectedRegistry . '/nextcloud/test-deploy:release', + $this->dockerActions->buildBaseImageName($imageParams, $daemonConfig), + ); + } + + #[DataProvider('imageNameProvider')] + public function testBuildExtendedImageName(array $registries, string $expectedRegistry): void { + $imageParams = ['image_src' => 'ghcr.io', 'image_name' => 'nextcloud/test-deploy', 'image_tag' => 'release']; + $daemonConfig = new DaemonConfig([ + 'deploy_config' => ['registries' => $registries, 'computeDevice' => ['id' => 'cuda']], + ]); + + $buildExtendedImageName = new ReflectionMethod($this->dockerActions, 'buildExtendedImageName'); + self::assertSame( + $expectedRegistry . '/nextcloud/test-deploy:release-cuda', + $buildExtendedImageName->invoke($this->dockerActions, $imageParams, $daemonConfig), + ); + } + + public function testBuildExtendedImageNameWithoutComputeDevice(): void { + $imageParams = ['image_src' => 'ghcr.io', 'image_name' => 'nextcloud/test-deploy', 'image_tag' => 'release']; + $daemonConfig = new DaemonConfig(['deploy_config' => ['registries' => []]]); + + $buildExtendedImageName = new ReflectionMethod($this->dockerActions, 'buildExtendedImageName'); + self::assertNull($buildExtendedImageName->invoke($this->dockerActions, $imageParams, $daemonConfig)); + } + + public static function shouldPullImageProvider(): array { + $local = ['from' => 'ghcr.io', 'to' => 'local']; + return [ + 'no registry mappings' => [[], true], + 'mapped to a mirror' => [[['from' => 'ghcr.io', 'to' => 'registry.example.com']], true], + 'mapped to local' => [[$local], false], + 'local mapping of another registry' => [[['from' => 'docker.io', 'to' => 'local']], true], + 'malformed entries are ignored' => [['ghcr.io', ['from' => 'ghcr.io'], ['to' => 'local'], $local], false], + ]; + } + + #[DataProvider('shouldPullImageProvider')] + public function testShouldPullImage(array $registries, bool $expected): void { + $imageParams = ['image_src' => 'ghcr.io', 'image_name' => 'nextcloud/test-deploy', 'image_tag' => 'release']; + $daemonConfig = new DaemonConfig(['deploy_config' => ['registries' => $registries]]); + + $shouldPullImage = new ReflectionMethod($this->dockerActions, 'shouldPullImage'); + self::assertSame($expected, $shouldPullImage->invoke($this->dockerActions, $imageParams, $daemonConfig)); + } } diff --git a/tests/php/DeployActions/KubernetesActionsTest.php b/tests/php/DeployActions/KubernetesActionsTest.php new file mode 100644 index 000000000..60cb57d0f --- /dev/null +++ b/tests/php/DeployActions/KubernetesActionsTest.php @@ -0,0 +1,180 @@ + 'ghcr.io', + 'image_name' => 'nextcloud/test-deploy', + 'image_tag' => 'release', + ]; + + private KubernetesActions $kubernetesActions; + + protected function setUp(): void { + parent::setUp(); + + $this->kubernetesActions = new KubernetesActions( + $this->createMock(LoggerInterface::class), + $this->createMock(IConfig::class), + $this->createMock(ICertificateManager::class), + $this->createMock(IAppManager::class), + $this->createMock(IURLGenerator::class), + $this->createMock(AppAPICommonService::class), + $this->createMock(ExAppService::class), + $this->createMock(ICrypto::class), + $this->createMock(ExAppDeployOptionsService::class), + ); + } + + public static function buildImageNameProvider(): array { + return [ + 'no registry mappings' => [[], 'ghcr.io/nextcloud/test-deploy:release'], + 'mapped registry' => [ + [['from' => 'ghcr.io', 'to' => 'registry.example.com']], + 'registry.example.com/nextcloud/test-deploy:release', + ], + 'mapping of another registry' => [ + [['from' => 'docker.io', 'to' => 'registry.example.com']], + 'ghcr.io/nextcloud/test-deploy:release', + ], + 'local keeps the image name' => [ + [['from' => 'ghcr.io', 'to' => 'local']], + 'ghcr.io/nextcloud/test-deploy:release', + ], + ]; + } + + #[DataProvider('buildImageNameProvider')] + public function testBuildImageName(array $registries, string $expected): void { + $daemonConfig = new DaemonConfig([ + 'accepts_deploy_id' => KubernetesActions::DEPLOY_ID, + 'deploy_config' => ['registries' => $registries], + ]); + + self::assertSame($expected, $this->kubernetesActions->buildImageName(self::IMAGE_PARAMS, $daemonConfig)); + } + + public function testBuildImageNameWithoutRegistriesInDeployConfig(): void { + $daemonConfig = new DaemonConfig(['deploy_config' => ['kubernetes' => ['expose_type' => 'clusterip']]]); + + self::assertSame( + 'ghcr.io/nextcloud/test-deploy:release', + $this->kubernetesActions->buildImageName(self::IMAGE_PARAMS, $daemonConfig), + ); + } + + public function testDeployExAppSendsTheMappedImageToHarp(): void { + $createPayloads = $this->deployWithMappedRegistry([]); + + self::assertCount(1, $createPayloads); + self::assertSame('registry.example.com/nextcloud/test-deploy:release', $createPayloads[0]['image']); + self::assertArrayNotHasKey('role_suffix', $createPayloads[0]); + } + + public function testDeployExAppSendsTheMappedImageForEveryRole(): void { + $createPayloads = $this->deployWithMappedRegistry([ + ['name' => 'web', 'env' => 'ROLE=web', 'expose' => true], + ['name' => 'worker', 'env' => 'ROLE=worker', 'expose' => false], + ]); + + self::assertSame(['web', 'worker'], array_column($createPayloads, 'role_suffix')); + foreach ($createPayloads as $payload) { + self::assertSame('registry.example.com/nextcloud/test-deploy:release', $payload['image']); + } + } + + /** + * Runs deployExApp() against queued HaRP responses and returns the payloads of the /exapp/create requests. + */ + private function deployWithMappedRegistry(array $roles): array { + $deployments = max(1, count($roles)); + $responses = [new Response(200, [], json_encode(['kubernetes' => ['enabled' => true, 'reachable' => true]]))]; + for ($i = 0; $i < $deployments; $i++) { + $responses[] = new Response(200, [], json_encode(['exists' => false])); + } + for ($i = 0; $i < $deployments; $i++) { + $responses[] = new Response(201, [], json_encode(['name' => 'nc-app-test-deploy'])); + $responses[] = new Response(204); + $responses[] = new Response(204); + } + for ($i = 0; $i < $deployments; $i++) { + $responses[] = new Response(200, [], json_encode(['started' => true])); + } + + $requests = []; + $mockHandler = new MockHandler($responses); + $handlerStack = HandlerStack::create($mockHandler); + $handlerStack->push(Middleware::history($requests)); + + $kubernetesActions = $this->getMockBuilder(KubernetesActions::class) + ->setConstructorArgs([ + $this->createMock(LoggerInterface::class), + $this->createMock(IConfig::class), + $this->createMock(ICertificateManager::class), + $this->createMock(IAppManager::class), + $this->createMock(IURLGenerator::class), + $this->createMock(AppAPICommonService::class), + $this->createMock(ExAppService::class), + $this->createMock(ICrypto::class), + $this->createMock(ExAppDeployOptionsService::class), + ]) + ->onlyMethods(['initGuzzleClient']) + ->getMock(); + (new ReflectionProperty(KubernetesActions::class, 'guzzleClient')) + ->setValue($kubernetesActions, new Client(['handler' => $handlerStack])); + + $daemonConfig = new DaemonConfig([ + 'accepts_deploy_id' => KubernetesActions::DEPLOY_ID, + 'protocol' => 'http', + 'host' => 'harp:8780', + 'deploy_config' => ['registries' => [['from' => 'ghcr.io', 'to' => 'registry.example.com']]], + ]); + $params = [ + 'image_params' => self::IMAGE_PARAMS, + 'container_params' => ['name' => 'test-deploy', 'env' => ['APP_ID=test-deploy']], + ]; + if ($roles !== []) { + $params['k8s_service_roles'] = $roles; + } + + self::assertSame('', $kubernetesActions->deployExApp(new ExApp(['appid' => 'test-deploy']), $daemonConfig, $params)); + self::assertSame(0, $mockHandler->count()); + + $createPayloads = []; + foreach ($requests as $transaction) { + if (str_ends_with($transaction['request']->getUri()->getPath(), '/exapp/create')) { + $createPayloads[] = json_decode((string)$transaction['request']->getBody(), true); + } + } + return $createPayloads; + } +} diff --git a/tests/php/Service/DaemonConfigServiceTest.php b/tests/php/Service/DaemonConfigServiceTest.php new file mode 100644 index 000000000..ba60b9adc --- /dev/null +++ b/tests/php/Service/DaemonConfigServiceTest.php @@ -0,0 +1,61 @@ + 'ghcr.io', 'to' => 'registry.example.com']; + return [ + 'no registries key' => [[], 'ghcr.io', 'ghcr.io'], + 'empty registries' => [['registries' => []], 'ghcr.io', 'ghcr.io'], + 'matching mapping' => [['registries' => [$mirror]], 'ghcr.io', 'registry.example.com'], + 'other registry is left alone' => [['registries' => [$mirror]], 'docker.io', 'docker.io'], + 'the matching mapping is picked among several' => [ + ['registries' => [['from' => 'docker.io', 'to' => 'hub.example.com'], $mirror]], + 'ghcr.io', + 'registry.example.com', + ], + 'trailing slashes of the target are dropped' => [ + ['registries' => [['from' => 'ghcr.io', 'to' => 'registry.example.com//']]], + 'ghcr.io', + 'registry.example.com', + ], + 'target with port and path' => [ + ['registries' => [['from' => 'ghcr.io', 'to' => 'registry.example.com:5000/mirror/ghcr']]], + 'ghcr.io', + 'registry.example.com:5000/mirror/ghcr', + ], + 'local keeps the registry' => [['registries' => [['from' => 'ghcr.io', 'to' => 'local']]], 'ghcr.io', 'ghcr.io'], + 'legacy duplicate source: the local entry is skipped' => [ + ['registries' => [['from' => 'ghcr.io', 'to' => 'local'], $mirror]], + 'ghcr.io', + 'registry.example.com', + ], + 'malformed entries are ignored' => [ + ['registries' => ['ghcr.io', ['from' => 'ghcr.io'], ['from' => 'ghcr.io', 'to' => 5000], ['from' => 'ghcr.io', 'to' => '/'], $mirror]], + 'ghcr.io', + 'registry.example.com', + ], + 'only an exact match counts' => [['registries' => [$mirror]], 'my.ghcr.io', 'my.ghcr.io'], + 'match is case sensitive' => [['registries' => [$mirror]], 'GHCR.IO', 'GHCR.IO'], + 'registries with gaps in their keys' => [['registries' => [2 => $mirror]], 'ghcr.io', 'registry.example.com'], + ]; + } + + #[DataProvider('resolveImageRegistryProvider')] + public function testResolveImageRegistry(array $deployConfig, string $imageRegistry, string $expected): void { + self::assertSame($expected, DaemonConfigService::resolveImageRegistry($deployConfig, $imageRegistry)); + } +} From 5fc7fefac2b4d4ba7dcfd90324cc82a289a97956 Mon Sep 17 00:00:00 2001 From: Oleksandr Piskun Date: Fri, 18 Sep 2026 10:51:10 +0000 Subject: [PATCH 02/10] fix: keep registry mappings a list Signed-off-by: Oleksandr Piskun --- lib/Service/DaemonConfigService.php | 6 +-- tests/php/Service/DaemonConfigServiceTest.php | 39 +++++++++++++++++++ 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/lib/Service/DaemonConfigService.php b/lib/Service/DaemonConfigService.php index d182d1b52..b07b8f07d 100644 --- a/lib/Service/DaemonConfigService.php +++ b/lib/Service/DaemonConfigService.php @@ -196,7 +196,7 @@ public function addDockerRegistry(DaemonConfig $daemonConfig, array $registryMap return ['error' => 'The source and target registry cannot be empty']; } - $deployConfig['registries'][] = $registryMap; + $deployConfig['registries'] = [...array_values($deployConfig['registries']), $registryMap]; $daemonConfig->setDeployConfig($deployConfig); return $this->mapper->update($daemonConfig); @@ -213,9 +213,9 @@ public function removeDockerRegistry(DaemonConfig $daemonConfig, array $registry if (!in_array($registryMap, $deployConfig['registries'])) { return ['error' => 'This Docker registry map does not exist']; } - $deployConfig['registries'] = array_filter($deployConfig['registries'], function ($registry) use ($registryMap) { + $deployConfig['registries'] = array_values(array_filter($deployConfig['registries'], function ($registry) use ($registryMap) { return !($registry['from'] === $registryMap['from'] && $registry['to'] === $registryMap['to']); - }); + })); $daemonConfig->setDeployConfig($deployConfig); return $this->mapper->update($daemonConfig); diff --git a/tests/php/Service/DaemonConfigServiceTest.php b/tests/php/Service/DaemonConfigServiceTest.php index ba60b9adc..e79347017 100644 --- a/tests/php/Service/DaemonConfigServiceTest.php +++ b/tests/php/Service/DaemonConfigServiceTest.php @@ -9,9 +9,14 @@ namespace OCA\AppAPI\Tests\php\Service; +use OCA\AppAPI\Db\DaemonConfig; +use OCA\AppAPI\Db\DaemonConfigMapper; use OCA\AppAPI\Service\DaemonConfigService; +use OCA\AppAPI\Service\ExAppService; +use OCP\Security\ICrypto; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; +use Psr\Log\LoggerInterface; class DaemonConfigServiceTest extends TestCase { @@ -58,4 +63,38 @@ public static function resolveImageRegistryProvider(): array { public function testResolveImageRegistry(array $deployConfig, string $imageRegistry, string $expected): void { self::assertSame($expected, DaemonConfigService::resolveImageRegistry($deployConfig, $imageRegistry)); } + + private function createService(): DaemonConfigService { + $mapper = $this->createMock(DaemonConfigMapper::class); + $mapper->method('update')->willReturnArgument(0); + return new DaemonConfigService( + $this->createMock(LoggerInterface::class), + $mapper, + $this->createMock(ExAppService::class), + $this->createMock(ICrypto::class), + ); + } + + public function testAddDockerRegistryKeepsAList(): void { + $stored = ['from' => 'docker.io', 'to' => 'hub.example.com']; + $added = ['from' => 'ghcr.io', 'to' => 'registry.example.com']; + $daemonConfig = new DaemonConfig(['deploy_config' => ['registries' => [1 => $stored]]]); + + $result = $this->createService()->addDockerRegistry($daemonConfig, $added); + + self::assertInstanceOf(DaemonConfig::class, $result); + self::assertSame([$stored, $added], $result->getDeployConfig()['registries']); + } + + public function testRemoveDockerRegistryKeepsAList(): void { + $service = $this->createService(); + $first = ['from' => 'docker.io', 'to' => 'hub.example.com']; + $second = ['from' => 'ghcr.io', 'to' => 'registry.example.com']; + $daemonConfig = new DaemonConfig(['deploy_config' => ['registries' => [$first, $second]]]); + + $result = $service->removeDockerRegistry($daemonConfig, $first); + + self::assertInstanceOf(DaemonConfig::class, $result); + self::assertSame([$second], $result->getDeployConfig()['registries']); + } } From 3f9af0a8b2065a88464bf73341b0894b0815af33 Mon Sep 17 00:00:00 2001 From: Oleksandr Piskun Date: Mon, 21 Sep 2026 08:12:58 +0000 Subject: [PATCH 03/10] fix: reject unusable registry mappings Signed-off-by: Oleksandr Piskun --- lib/Service/DaemonConfigService.php | 17 ++++--- tests/php/Service/DaemonConfigServiceTest.php | 45 ++++++++++++++++++- 2 files changed, 53 insertions(+), 9 deletions(-) diff --git a/lib/Service/DaemonConfigService.php b/lib/Service/DaemonConfigService.php index b07b8f07d..d565aeb10 100644 --- a/lib/Service/DaemonConfigService.php +++ b/lib/Service/DaemonConfigService.php @@ -173,6 +173,12 @@ public function updateDaemonConfig(DaemonConfig $daemonConfig): ?DaemonConfig { public function addDockerRegistry(DaemonConfig $daemonConfig, array $registryMap): DaemonConfig|array|null { try { + $from = $registryMap['from'] ?? null; + $to = $registryMap['to'] ?? null; + if (!is_string($from) || !is_string($to) || $from === '' || rtrim($to, '/') === '') { + return ['error' => 'The source and target registry cannot be empty']; + } + $deployConfig = $daemonConfig->getDeployConfig(); if (!isset($deployConfig['registries'])) { @@ -181,22 +187,19 @@ public function addDockerRegistry(DaemonConfig $daemonConfig, array $registryMap $fromExists = false; foreach ($deployConfig['registries'] as $registry) { - if ($registry['from'] === $registryMap['from']) { + if (($registry['from'] ?? null) === $from) { $fromExists = true; break; } } if ($fromExists) { - return ['error' => sprintf('This Docker registry map from "%s" already exists', $registryMap['from'])]; + return ['error' => sprintf('This Docker registry map from "%s" already exists', $from)]; } - if ($registryMap['from'] === $registryMap['to']) { + if ($from === $to) { return ['error' => 'The source and target registry cannot be the same']; } - if (empty($registryMap['from']) || empty($registryMap['to'])) { - return ['error' => 'The source and target registry cannot be empty']; - } - $deployConfig['registries'] = [...array_values($deployConfig['registries']), $registryMap]; + $deployConfig['registries'] = [...array_values($deployConfig['registries']), ['from' => $from, 'to' => $to]]; $daemonConfig->setDeployConfig($deployConfig); return $this->mapper->update($daemonConfig); diff --git a/tests/php/Service/DaemonConfigServiceTest.php b/tests/php/Service/DaemonConfigServiceTest.php index e79347017..245d263ba 100644 --- a/tests/php/Service/DaemonConfigServiceTest.php +++ b/tests/php/Service/DaemonConfigServiceTest.php @@ -64,9 +64,9 @@ public function testResolveImageRegistry(array $deployConfig, string $imageRegis self::assertSame($expected, DaemonConfigService::resolveImageRegistry($deployConfig, $imageRegistry)); } - private function createService(): DaemonConfigService { + private function createService(bool $expectUpdate = true): DaemonConfigService { $mapper = $this->createMock(DaemonConfigMapper::class); - $mapper->method('update')->willReturnArgument(0); + $mapper->expects($expectUpdate ? self::once() : self::never())->method('update')->willReturnArgument(0); return new DaemonConfigService( $this->createMock(LoggerInterface::class), $mapper, @@ -86,6 +86,47 @@ public function testAddDockerRegistryKeepsAList(): void { self::assertSame([$stored, $added], $result->getDeployConfig()['registries']); } + public static function unusableRegistryMapProvider(): array { + return [ + 'no source' => [['to' => 'registry.example.com']], + 'no target' => [['from' => 'ghcr.io']], + 'empty source' => [['from' => '', 'to' => 'registry.example.com']], + 'empty target' => [['from' => 'ghcr.io', 'to' => '']], + 'target of slashes only' => [['from' => 'ghcr.io', 'to' => '//']], + 'target is not a string' => [['from' => 'ghcr.io', 'to' => 5000]], + 'source is not a string' => [['from' => ['ghcr.io'], 'to' => 'registry.example.com']], + ]; + } + + #[DataProvider('unusableRegistryMapProvider')] + public function testAddDockerRegistryRejectsAnUnusableMap(array $registryMap): void { + $daemonConfig = new DaemonConfig(['deploy_config' => ['registries' => []]]); + + $result = $this->createService(expectUpdate: false)->addDockerRegistry($daemonConfig, $registryMap); + + self::assertSame(['error' => 'The source and target registry cannot be empty'], $result); + self::assertSame([], $daemonConfig->getDeployConfig()['registries']); + } + + public function testAddDockerRegistryRejectsADuplicateSource(): void { + $daemonConfig = new DaemonConfig(['deploy_config' => ['registries' => ['junk', ['from' => 'ghcr.io', 'to' => 'local']]]]); + + $result = $this->createService(expectUpdate: false) + ->addDockerRegistry($daemonConfig, ['from' => 'ghcr.io', 'to' => 'registry.example.com']); + + self::assertSame(['error' => 'This Docker registry map from "ghcr.io" already exists'], $result); + } + + public function testAddDockerRegistryStoresOnlySourceAndTarget(): void { + $daemonConfig = new DaemonConfig(['deploy_config' => []]); + + $result = $this->createService() + ->addDockerRegistry($daemonConfig, ['from' => 'ghcr.io', 'to' => 'registry.example.com/', 'extra' => 'x']); + + self::assertInstanceOf(DaemonConfig::class, $result); + self::assertSame([['from' => 'ghcr.io', 'to' => 'registry.example.com/']], $result->getDeployConfig()['registries']); + } + public function testRemoveDockerRegistryKeepsAList(): void { $service = $this->createService(); $first = ['from' => 'docker.io', 'to' => 'hub.example.com']; From 4cd6e679832535844fd38a8a49ab99b8019421d4 Mon Sep 17 00:00:00 2001 From: Oleksandr Piskun Date: Mon, 21 Sep 2026 13:06:20 +0000 Subject: [PATCH 04/10] fix: pull the image when a mirror mapping applies Signed-off-by: Oleksandr Piskun --- lib/DeployActions/DockerActions.php | 3 +++ tests/php/DeployActions/DockerActionsTest.php | 4 ++++ 2 files changed, 7 insertions(+) diff --git a/lib/DeployActions/DockerActions.php b/lib/DeployActions/DockerActions.php index d169f2770..ebe3459cf 100644 --- a/lib/DeployActions/DockerActions.php +++ b/lib/DeployActions/DockerActions.php @@ -447,6 +447,9 @@ private function buildExtendedImageName(array $imageParams, DaemonConfig $daemon private function shouldPullImage(array $imageParams, DaemonConfig $daemonConfig): bool { $deployConfig = $daemonConfig->getDeployConfig(); + if (DaemonConfigService::resolveImageRegistry($deployConfig, $imageParams['image_src']) !== $imageParams['image_src']) { + return true; // the image is taken from a mapped registry + } if (isset($deployConfig['registries'])) { // custom Docker registry, overrides ExApp's image_src foreach ($deployConfig['registries'] as $registry) { if (($registry['from'] ?? null) === $imageParams['image_src'] && ($registry['to'] ?? null) === 'local') { // local target skips image pull, imageId should be unchanged diff --git a/tests/php/DeployActions/DockerActionsTest.php b/tests/php/DeployActions/DockerActionsTest.php index 2e2c0cbb8..b395621f7 100644 --- a/tests/php/DeployActions/DockerActionsTest.php +++ b/tests/php/DeployActions/DockerActionsTest.php @@ -153,6 +153,10 @@ public static function shouldPullImageProvider(): array { 'mapped to local' => [[$local], false], 'local mapping of another registry' => [[['from' => 'docker.io', 'to' => 'local']], true], 'malformed entries are ignored' => [['ghcr.io', ['from' => 'ghcr.io'], ['to' => 'local'], $local], false], + 'legacy duplicate source: the mirror is used, so it is pulled' => [ + [$local, ['from' => 'ghcr.io', 'to' => 'registry.example.com']], + true, + ], ]; } From 4db381f51c2236d59ed02779b652498da49e9d71 Mon Sep 17 00:00:00 2001 From: Oleksandr Piskun Date: Mon, 21 Sep 2026 13:06:35 +0000 Subject: [PATCH 05/10] fix: tolerate malformed entries when removing a mapping Signed-off-by: Oleksandr Piskun --- lib/Service/DaemonConfigService.php | 2 +- tests/php/Service/DaemonConfigServiceTest.php | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/lib/Service/DaemonConfigService.php b/lib/Service/DaemonConfigService.php index d565aeb10..43becf9f3 100644 --- a/lib/Service/DaemonConfigService.php +++ b/lib/Service/DaemonConfigService.php @@ -217,7 +217,7 @@ public function removeDockerRegistry(DaemonConfig $daemonConfig, array $registry return ['error' => 'This Docker registry map does not exist']; } $deployConfig['registries'] = array_values(array_filter($deployConfig['registries'], function ($registry) use ($registryMap) { - return !($registry['from'] === $registryMap['from'] && $registry['to'] === $registryMap['to']); + return !(($registry['from'] ?? null) === $registryMap['from'] && ($registry['to'] ?? null) === $registryMap['to']); })); $daemonConfig->setDeployConfig($deployConfig); diff --git a/tests/php/Service/DaemonConfigServiceTest.php b/tests/php/Service/DaemonConfigServiceTest.php index 245d263ba..1a39d955e 100644 --- a/tests/php/Service/DaemonConfigServiceTest.php +++ b/tests/php/Service/DaemonConfigServiceTest.php @@ -138,4 +138,15 @@ public function testRemoveDockerRegistryKeepsAList(): void { self::assertInstanceOf(DaemonConfig::class, $result); self::assertSame([$second], $result->getDeployConfig()['registries']); } + + public function testRemoveDockerRegistryToleratesMalformedEntries(): void { + $first = ['from' => 'docker.io', 'to' => 'hub.example.com']; + $second = ['from' => 'ghcr.io', 'to' => 'registry.example.com']; + $daemonConfig = new DaemonConfig(['deploy_config' => ['registries' => ['junk', ['from' => 'quay.io'], $first, $second]]]); + + $result = $this->createService()->removeDockerRegistry($daemonConfig, $first); + + self::assertInstanceOf(DaemonConfig::class, $result); + self::assertSame(['junk', ['from' => 'quay.io'], $second], $result->getDeployConfig()['registries']); + } } From 72ed9f188bd2444599051346b9d65b7dc06e62a2 Mon Sep 17 00:00:00 2001 From: Oleksandr Piskun Date: Fri, 25 Sep 2026 07:27:01 +0000 Subject: [PATCH 06/10] refactor: resolve the effective registry mapping once Signed-off-by: Oleksandr Piskun --- lib/DeployActions/DockerActions.php | 14 ++------ lib/Service/DaemonConfigService.php | 25 ++++++++++---- tests/php/DeployActions/DockerActionsTest.php | 5 +-- tests/php/Service/DaemonConfigServiceTest.php | 33 +++++++++++++++++-- 4 files changed, 54 insertions(+), 23 deletions(-) diff --git a/lib/DeployActions/DockerActions.php b/lib/DeployActions/DockerActions.php index ebe3459cf..ae17b8d76 100644 --- a/lib/DeployActions/DockerActions.php +++ b/lib/DeployActions/DockerActions.php @@ -446,18 +446,8 @@ private function buildExtendedImageName(array $imageParams, DaemonConfig $daemon } private function shouldPullImage(array $imageParams, DaemonConfig $daemonConfig): bool { - $deployConfig = $daemonConfig->getDeployConfig(); - if (DaemonConfigService::resolveImageRegistry($deployConfig, $imageParams['image_src']) !== $imageParams['image_src']) { - return true; // the image is taken from a mapped registry - } - if (isset($deployConfig['registries'])) { // custom Docker registry, overrides ExApp's image_src - foreach ($deployConfig['registries'] as $registry) { - if (($registry['from'] ?? null) === $imageParams['image_src'] && ($registry['to'] ?? null) === 'local') { // local target skips image pull, imageId should be unchanged - return false; - } - } - } - return true; + return DaemonConfigService::resolveRegistryTarget($daemonConfig->getDeployConfig(), $imageParams['image_src']) + !== DaemonConfigService::LOCAL_REGISTRY; } public function imageExists(string $dockerUrl, string $imageId): bool { diff --git a/lib/Service/DaemonConfigService.php b/lib/Service/DaemonConfigService.php index 43becf9f3..d30e75f32 100644 --- a/lib/Service/DaemonConfigService.php +++ b/lib/Service/DaemonConfigService.php @@ -24,6 +24,9 @@ * Daemon configuration (daemons) */ readonly class DaemonConfigService { + /** Registry mapping target that keeps the image name and tells the daemon not to pull the image. */ + public const LOCAL_REGISTRY = 'local'; + public function __construct( private LoggerInterface $logger, private DaemonConfigMapper $mapper, @@ -229,16 +232,16 @@ public function removeDockerRegistry(DaemonConfig $daemonConfig, array $registry } /** - * Registry to take an ExApp image from, after the registry mappings of the daemon are applied. + * Effective target of the daemon's registry mappings for the registry an ExApp image comes from: + * the registry to take the image from instead, LOCAL_REGISTRY, or null when no usable mapping exists. * - * The first mapping of the registry with a target other than "local" wins, entries without a usable target - * are ignored. "local" never renames the image: Docker daemons skip the pull for it, on Kubernetes it changes - * nothing and the kubelet pulls the image if the node does not have it. + * The first mapping of the registry with a usable target wins. A target is usable when it is a non-empty + * string once trailing slashes are dropped, so "local/" is LOCAL_REGISTRY as well. */ - public static function resolveImageRegistry(array $deployConfig, string $imageRegistry): string { + public static function resolveRegistryTarget(array $deployConfig, string $imageRegistry): ?string { foreach ($deployConfig['registries'] ?? [] as $registry) { $target = $registry['to'] ?? null; - if (($registry['from'] ?? null) !== $imageRegistry || !is_string($target) || $target === 'local') { + if (($registry['from'] ?? null) !== $imageRegistry || !is_string($target)) { continue; } $target = rtrim($target, '/'); @@ -246,6 +249,14 @@ public static function resolveImageRegistry(array $deployConfig, string $imageRe return $target; } } - return $imageRegistry; + return null; + } + + /** + * Registry to take an ExApp image from, after the registry mappings of the daemon are applied. + */ + public static function resolveImageRegistry(array $deployConfig, string $imageRegistry): string { + $target = self::resolveRegistryTarget($deployConfig, $imageRegistry); + return $target === null || $target === self::LOCAL_REGISTRY ? $imageRegistry : $target; } } diff --git a/tests/php/DeployActions/DockerActionsTest.php b/tests/php/DeployActions/DockerActionsTest.php index b395621f7..1780e6243 100644 --- a/tests/php/DeployActions/DockerActionsTest.php +++ b/tests/php/DeployActions/DockerActionsTest.php @@ -153,10 +153,11 @@ public static function shouldPullImageProvider(): array { 'mapped to local' => [[$local], false], 'local mapping of another registry' => [[['from' => 'docker.io', 'to' => 'local']], true], 'malformed entries are ignored' => [['ghcr.io', ['from' => 'ghcr.io'], ['to' => 'local'], $local], false], - 'legacy duplicate source: the mirror is used, so it is pulled' => [ + 'legacy duplicate source: the first entry wins' => [ [$local, ['from' => 'ghcr.io', 'to' => 'registry.example.com']], - true, + false, ], + 'local with a trailing slash' => [[['from' => 'ghcr.io', 'to' => 'local/']], false], ]; } diff --git a/tests/php/Service/DaemonConfigServiceTest.php b/tests/php/Service/DaemonConfigServiceTest.php index 1a39d955e..c6c3365e8 100644 --- a/tests/php/Service/DaemonConfigServiceTest.php +++ b/tests/php/Service/DaemonConfigServiceTest.php @@ -43,10 +43,15 @@ public static function resolveImageRegistryProvider(): array { 'registry.example.com:5000/mirror/ghcr', ], 'local keeps the registry' => [['registries' => [['from' => 'ghcr.io', 'to' => 'local']]], 'ghcr.io', 'ghcr.io'], - 'legacy duplicate source: the local entry is skipped' => [ + 'legacy duplicate source: the first entry wins' => [ ['registries' => [['from' => 'ghcr.io', 'to' => 'local'], $mirror]], 'ghcr.io', - 'registry.example.com', + 'ghcr.io', + ], + 'local with a trailing slash keeps the registry' => [ + ['registries' => [['from' => 'ghcr.io', 'to' => 'local/']]], + 'ghcr.io', + 'ghcr.io', ], 'malformed entries are ignored' => [ ['registries' => ['ghcr.io', ['from' => 'ghcr.io'], ['from' => 'ghcr.io', 'to' => 5000], ['from' => 'ghcr.io', 'to' => '/'], $mirror]], @@ -64,6 +69,30 @@ public function testResolveImageRegistry(array $deployConfig, string $imageRegis self::assertSame($expected, DaemonConfigService::resolveImageRegistry($deployConfig, $imageRegistry)); } + public static function resolveRegistryTargetProvider(): array { + $mirror = ['from' => 'ghcr.io', 'to' => 'registry.example.com']; + $local = ['from' => 'ghcr.io', 'to' => 'local']; + return [ + 'no registries key' => [[], null], + 'no mapping of the registry' => [['registries' => [['from' => 'docker.io', 'to' => 'local']]], null], + 'mirror' => [['registries' => [$mirror]], 'registry.example.com'], + 'mirror with trailing slashes' => [['registries' => [['from' => 'ghcr.io', 'to' => 'registry.example.com//']]], 'registry.example.com'], + 'local' => [['registries' => [$local]], 'local'], + 'local with a trailing slash' => [['registries' => [['from' => 'ghcr.io', 'to' => 'local/']]], 'local'], + 'first usable entry wins' => [['registries' => [$local, $mirror]], 'local'], + 'unusable entries are skipped' => [ + ['registries' => ['ghcr.io', ['from' => 'ghcr.io'], ['from' => 'ghcr.io', 'to' => 5000], ['from' => 'ghcr.io', 'to' => '/'], $mirror]], + 'registry.example.com', + ], + 'only unusable entries' => [['registries' => [['from' => 'ghcr.io', 'to' => '//']]], null], + ]; + } + + #[DataProvider('resolveRegistryTargetProvider')] + public function testResolveRegistryTarget(array $deployConfig, ?string $expected): void { + self::assertSame($expected, DaemonConfigService::resolveRegistryTarget($deployConfig, 'ghcr.io')); + } + private function createService(bool $expectUpdate = true): DaemonConfigService { $mapper = $this->createMock(DaemonConfigMapper::class); $mapper->expects($expectUpdate ? self::once() : self::never())->method('update')->willReturnArgument(0); From 0b6b9f6e66dde33f1054a1e693881f6e7eea1ee2 Mon Sep 17 00:00:00 2001 From: Oleksandr Piskun Date: Fri, 25 Sep 2026 07:27:01 +0000 Subject: [PATCH 07/10] feat(k8s): skip the image pull for local mappings Signed-off-by: Oleksandr Piskun --- lib/DeployActions/KubernetesActions.php | 3 +++ .../DeployActions/KubernetesActionsTest.php | 19 +++++++++++++++++-- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/lib/DeployActions/KubernetesActions.php b/lib/DeployActions/KubernetesActions.php index 72201162d..e278d5ada 100644 --- a/lib/DeployActions/KubernetesActions.php +++ b/lib/DeployActions/KubernetesActions.php @@ -351,6 +351,9 @@ private function createExApp(DaemonConfig $daemonConfig, string $harpUrl, string $createPayload = $this->buildNamePayload($exAppName, $instanceId, $roleSuffix); $createPayload['image'] = $this->buildImageName($params['image_params'], $daemonConfig); + if (DaemonConfigService::resolveRegistryTarget($daemonConfig->getDeployConfig(), $params['image_params']['image_src']) === DaemonConfigService::LOCAL_REGISTRY) { + $createPayload['image_pull_policy'] = 'Never'; // HaRP without support for this field keeps IfNotPresent + } $createPayload['environment_variables'] = $params['container_params']['env'] ?? []; $createPayload['compute_device'] = $computeDevice; diff --git a/tests/php/DeployActions/KubernetesActionsTest.php b/tests/php/DeployActions/KubernetesActionsTest.php index 60cb57d0f..0cf46725c 100644 --- a/tests/php/DeployActions/KubernetesActionsTest.php +++ b/tests/php/DeployActions/KubernetesActionsTest.php @@ -100,6 +100,21 @@ public function testDeployExAppSendsTheMappedImageToHarp(): void { self::assertArrayNotHasKey('role_suffix', $createPayloads[0]); } + public function testDeployExAppNeverPullsForALocalMapping(): void { + $createPayloads = $this->deployWithMappedRegistry([], [['from' => 'ghcr.io', 'to' => 'local']]); + + self::assertSame('ghcr.io/nextcloud/test-deploy:release', $createPayloads[0]['image']); + self::assertSame('Never', $createPayloads[0]['image_pull_policy']); + } + + public function testDeployExAppLeavesThePullPolicyToHarpWithoutALocalMapping(): void { + foreach ([[], [['from' => 'ghcr.io', 'to' => 'registry.example.com']]] as $registries) { + $createPayloads = $this->deployWithMappedRegistry([], $registries); + + self::assertArrayNotHasKey('image_pull_policy', $createPayloads[0]); + } + } + public function testDeployExAppSendsTheMappedImageForEveryRole(): void { $createPayloads = $this->deployWithMappedRegistry([ ['name' => 'web', 'env' => 'ROLE=web', 'expose' => true], @@ -115,7 +130,7 @@ public function testDeployExAppSendsTheMappedImageForEveryRole(): void { /** * Runs deployExApp() against queued HaRP responses and returns the payloads of the /exapp/create requests. */ - private function deployWithMappedRegistry(array $roles): array { + private function deployWithMappedRegistry(array $roles, ?array $registries = null): array { $deployments = max(1, count($roles)); $responses = [new Response(200, [], json_encode(['kubernetes' => ['enabled' => true, 'reachable' => true]]))]; for ($i = 0; $i < $deployments; $i++) { @@ -156,7 +171,7 @@ private function deployWithMappedRegistry(array $roles): array { 'accepts_deploy_id' => KubernetesActions::DEPLOY_ID, 'protocol' => 'http', 'host' => 'harp:8780', - 'deploy_config' => ['registries' => [['from' => 'ghcr.io', 'to' => 'registry.example.com']]], + 'deploy_config' => ['registries' => $registries ?? [['from' => 'ghcr.io', 'to' => 'registry.example.com']]], ]); $params = [ 'image_params' => self::IMAGE_PARAMS, From ef6a948f90840cb2cc0f865ca160d1135f3afa72 Mon Sep 17 00:00:00 2001 From: Oleksandr Piskun Date: Fri, 25 Sep 2026 08:07:01 +0000 Subject: [PATCH 08/10] fix: normalise registry mappings on add Signed-off-by: Oleksandr Piskun --- lib/Service/DaemonConfigService.php | 20 +++++----- tests/php/Service/DaemonConfigServiceTest.php | 38 +++++++++++++++++-- 2 files changed, 44 insertions(+), 14 deletions(-) diff --git a/lib/Service/DaemonConfigService.php b/lib/Service/DaemonConfigService.php index d30e75f32..2e58da382 100644 --- a/lib/Service/DaemonConfigService.php +++ b/lib/Service/DaemonConfigService.php @@ -178,7 +178,12 @@ public function addDockerRegistry(DaemonConfig $daemonConfig, array $registryMap try { $from = $registryMap['from'] ?? null; $to = $registryMap['to'] ?? null; - if (!is_string($from) || !is_string($to) || $from === '' || rtrim($to, '/') === '') { + if (!is_string($from) || !is_string($to)) { + return ['error' => 'The source and target registry cannot be empty']; + } + $from = rtrim(trim($from), '/'); + $to = rtrim(trim($to), '/'); + if ($from === '' || $to === '') { return ['error' => 'The source and target registry cannot be empty']; } @@ -188,14 +193,7 @@ public function addDockerRegistry(DaemonConfig $daemonConfig, array $registryMap $deployConfig['registries'] = []; } - $fromExists = false; - foreach ($deployConfig['registries'] as $registry) { - if (($registry['from'] ?? null) === $from) { - $fromExists = true; - break; - } - } - if ($fromExists) { + if (self::resolveRegistryTarget($deployConfig, $from) !== null) { return ['error' => sprintf('This Docker registry map from "%s" already exists', $from)]; } if ($from === $to) { @@ -216,11 +214,11 @@ public function removeDockerRegistry(DaemonConfig $daemonConfig, array $registry try { $deployConfig = $daemonConfig->getDeployConfig(); - if (!in_array($registryMap, $deployConfig['registries'])) { + if (!in_array($registryMap, $deployConfig['registries'] ?? [])) { return ['error' => 'This Docker registry map does not exist']; } $deployConfig['registries'] = array_values(array_filter($deployConfig['registries'], function ($registry) use ($registryMap) { - return !(($registry['from'] ?? null) === $registryMap['from'] && ($registry['to'] ?? null) === $registryMap['to']); + return !(($registry['from'] ?? null) === ($registryMap['from'] ?? null) && ($registry['to'] ?? null) === ($registryMap['to'] ?? null)); })); $daemonConfig->setDeployConfig($deployConfig); diff --git a/tests/php/Service/DaemonConfigServiceTest.php b/tests/php/Service/DaemonConfigServiceTest.php index c6c3365e8..8c8c567c7 100644 --- a/tests/php/Service/DaemonConfigServiceTest.php +++ b/tests/php/Service/DaemonConfigServiceTest.php @@ -80,6 +80,7 @@ public static function resolveRegistryTargetProvider(): array { 'local' => [['registries' => [$local]], 'local'], 'local with a trailing slash' => [['registries' => [['from' => 'ghcr.io', 'to' => 'local/']]], 'local'], 'first usable entry wins' => [['registries' => [$local, $mirror]], 'local'], + 'first usable entry wins, mirror first' => [['registries' => [$mirror, $local]], 'registry.example.com'], 'unusable entries are skipped' => [ ['registries' => ['ghcr.io', ['from' => 'ghcr.io'], ['from' => 'ghcr.io', 'to' => 5000], ['from' => 'ghcr.io', 'to' => '/'], $mirror]], 'registry.example.com', @@ -122,6 +123,7 @@ public static function unusableRegistryMapProvider(): array { 'empty source' => [['from' => '', 'to' => 'registry.example.com']], 'empty target' => [['from' => 'ghcr.io', 'to' => '']], 'target of slashes only' => [['from' => 'ghcr.io', 'to' => '//']], + 'whitespace only' => [['from' => ' ', 'to' => "\t"]], 'target is not a string' => [['from' => 'ghcr.io', 'to' => 5000]], 'source is not a string' => [['from' => ['ghcr.io'], 'to' => 'registry.example.com']], ]; @@ -146,14 +148,44 @@ public function testAddDockerRegistryRejectsADuplicateSource(): void { self::assertSame(['error' => 'This Docker registry map from "ghcr.io" already exists'], $result); } - public function testAddDockerRegistryStoresOnlySourceAndTarget(): void { + public function testAddDockerRegistryIgnoresAnUnusableStoredEntryOfTheSameSource(): void { + $unusable = ['from' => 'ghcr.io', 'to' => '/']; + $added = ['from' => 'ghcr.io', 'to' => 'registry.example.com']; + $daemonConfig = new DaemonConfig(['deploy_config' => ['registries' => [$unusable]]]); + + $result = $this->createService()->addDockerRegistry($daemonConfig, $added); + + self::assertInstanceOf(DaemonConfig::class, $result); + self::assertSame([$unusable, $added], $result->getDeployConfig()['registries']); + self::assertSame('registry.example.com', DaemonConfigService::resolveImageRegistry($result->getDeployConfig(), 'ghcr.io')); + } + + public function testAddDockerRegistryStoresNormalisedSourceAndTarget(): void { $daemonConfig = new DaemonConfig(['deploy_config' => []]); $result = $this->createService() - ->addDockerRegistry($daemonConfig, ['from' => 'ghcr.io', 'to' => 'registry.example.com/', 'extra' => 'x']); + ->addDockerRegistry($daemonConfig, ['from' => ' ghcr.io/ ', 'to' => 'registry.example.com//', 'extra' => 'x']); + + self::assertInstanceOf(DaemonConfig::class, $result); + self::assertSame([['from' => 'ghcr.io', 'to' => 'registry.example.com']], $result->getDeployConfig()['registries']); + } + + public function testAddDockerRegistryStoresLocalWithoutATrailingSlash(): void { + $daemonConfig = new DaemonConfig(['deploy_config' => []]); + + $result = $this->createService()->addDockerRegistry($daemonConfig, ['from' => 'ghcr.io', 'to' => 'local/']); self::assertInstanceOf(DaemonConfig::class, $result); - self::assertSame([['from' => 'ghcr.io', 'to' => 'registry.example.com/']], $result->getDeployConfig()['registries']); + self::assertSame([['from' => 'ghcr.io', 'to' => 'local']], $result->getDeployConfig()['registries']); + } + + public function testRemoveDockerRegistryWithoutAnyMapping(): void { + $daemonConfig = new DaemonConfig(['deploy_config' => ['net' => 'host']]); + + $result = $this->createService(expectUpdate: false) + ->removeDockerRegistry($daemonConfig, ['from' => 'ghcr.io', 'to' => 'registry.example.com']); + + self::assertSame(['error' => 'This Docker registry map does not exist'], $result); } public function testRemoveDockerRegistryKeepsAList(): void { From 8269a881107f467db189827eea1b7b0837b5e382 Mon Sep 17 00:00:00 2001 From: Oleksandr Piskun Date: Fri, 25 Sep 2026 08:07:01 +0000 Subject: [PATCH 09/10] test: pin the mapping order and the K8s pull policy cases Signed-off-by: Oleksandr Piskun --- tests/php/DeployActions/DockerActionsTest.php | 4 ++++ .../php/DeployActions/KubernetesActionsTest.php | 17 ++++++++++++----- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/tests/php/DeployActions/DockerActionsTest.php b/tests/php/DeployActions/DockerActionsTest.php index 1780e6243..1aa3d6665 100644 --- a/tests/php/DeployActions/DockerActionsTest.php +++ b/tests/php/DeployActions/DockerActionsTest.php @@ -158,6 +158,10 @@ public static function shouldPullImageProvider(): array { false, ], 'local with a trailing slash' => [[['from' => 'ghcr.io', 'to' => 'local/']], false], + 'legacy duplicate source, mirror first: the mirror is pulled' => [ + [['from' => 'ghcr.io', 'to' => 'registry.example.com'], $local], + true, + ], ]; } diff --git a/tests/php/DeployActions/KubernetesActionsTest.php b/tests/php/DeployActions/KubernetesActionsTest.php index 0cf46725c..ed5d5d70c 100644 --- a/tests/php/DeployActions/KubernetesActionsTest.php +++ b/tests/php/DeployActions/KubernetesActionsTest.php @@ -107,12 +107,18 @@ public function testDeployExAppNeverPullsForALocalMapping(): void { self::assertSame('Never', $createPayloads[0]['image_pull_policy']); } - public function testDeployExAppLeavesThePullPolicyToHarpWithoutALocalMapping(): void { - foreach ([[], [['from' => 'ghcr.io', 'to' => 'registry.example.com']]] as $registries) { - $createPayloads = $this->deployWithMappedRegistry([], $registries); + public static function nonLocalRegistriesProvider(): array { + return [ + 'no mapping' => [[]], + 'mirror mapping' => [[['from' => 'ghcr.io', 'to' => 'registry.example.com']]], + ]; + } - self::assertArrayNotHasKey('image_pull_policy', $createPayloads[0]); - } + #[DataProvider('nonLocalRegistriesProvider')] + public function testDeployExAppLeavesThePullPolicyToHarpWithoutALocalMapping(array $registries): void { + $createPayloads = $this->deployWithMappedRegistry([], $registries); + + self::assertArrayNotHasKey('image_pull_policy', $createPayloads[0]); } public function testDeployExAppSendsTheMappedImageForEveryRole(): void { @@ -190,6 +196,7 @@ private function deployWithMappedRegistry(array $roles, ?array $registries = nul $createPayloads[] = json_decode((string)$transaction['request']->getBody(), true); } } + self::assertCount($deployments, $createPayloads); return $createPayloads; } } From 4a29c4b9dae5c97ec35aa612fadf8e2e64926af8 Mon Sep 17 00:00:00 2001 From: Oleksandr Piskun Date: Fri, 25 Sep 2026 08:27:01 +0000 Subject: [PATCH 10/10] fix: ignore whitespace-only registry targets Signed-off-by: Oleksandr Piskun --- lib/Service/DaemonConfigService.php | 4 ++-- tests/php/Service/DaemonConfigServiceTest.php | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/Service/DaemonConfigService.php b/lib/Service/DaemonConfigService.php index 2e58da382..b9d9ef645 100644 --- a/lib/Service/DaemonConfigService.php +++ b/lib/Service/DaemonConfigService.php @@ -234,7 +234,7 @@ public function removeDockerRegistry(DaemonConfig $daemonConfig, array $registry * the registry to take the image from instead, LOCAL_REGISTRY, or null when no usable mapping exists. * * The first mapping of the registry with a usable target wins. A target is usable when it is a non-empty - * string once trailing slashes are dropped, so "local/" is LOCAL_REGISTRY as well. + * string once surrounding whitespace and trailing slashes are dropped, so "local/" is LOCAL_REGISTRY as well. */ public static function resolveRegistryTarget(array $deployConfig, string $imageRegistry): ?string { foreach ($deployConfig['registries'] ?? [] as $registry) { @@ -242,7 +242,7 @@ public static function resolveRegistryTarget(array $deployConfig, string $imageR if (($registry['from'] ?? null) !== $imageRegistry || !is_string($target)) { continue; } - $target = rtrim($target, '/'); + $target = rtrim(trim($target), '/'); if ($target !== '') { return $target; } diff --git a/tests/php/Service/DaemonConfigServiceTest.php b/tests/php/Service/DaemonConfigServiceTest.php index 8c8c567c7..5d0f4361e 100644 --- a/tests/php/Service/DaemonConfigServiceTest.php +++ b/tests/php/Service/DaemonConfigServiceTest.php @@ -86,6 +86,8 @@ public static function resolveRegistryTargetProvider(): array { 'registry.example.com', ], 'only unusable entries' => [['registries' => [['from' => 'ghcr.io', 'to' => '//']]], null], + 'whitespace-only target is unusable' => [['registries' => [['from' => 'ghcr.io', 'to' => ' ']]], null], + 'stored target with surrounding whitespace' => [['registries' => [['from' => 'ghcr.io', 'to' => ' registry.example.com ']]], 'registry.example.com'], ]; }