[stable34] fix(k8s): apply daemon registry mappings

lib/DeployActions/DockerActions.php

@@ -17,6 +17,7 @@
 
 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;
@@ -433,16 +434,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'];
 	}
 
@@ -451,28 +443,13 @@ 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
-					return false;
-				}
-			}
-		}
-		return true;
+		return DaemonConfigService::resolveRegistryTarget($daemonConfig->getDeployConfig(), $imageParams['image_src'])
+			!== DaemonConfigService::LOCAL_REGISTRY;
 	}
 
 	public function imageExists(string $dockerUrl, string $imageId): bool {

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,17 @@ 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);
+		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;
 
@@ -767,8 +771,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 {

lib/Service/DaemonConfigService.php

@@ -25,6 +25,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,
@@ -174,30 +177,31 @@ 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)) {
+				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'];
+			}
+
 			$deployConfig = $daemonConfig->getDeployConfig();
 
 			if (!isset($deployConfig['registries'])) {
 				$deployConfig['registries'] = [];
 			}
 
-			$fromExists = false;
-			foreach ($deployConfig['registries'] as $registry) {
-				if ($registry['from'] === $registryMap['from']) {
-					$fromExists = true;
-					break;
-				}
-			}
-			if ($fromExists) {
-				return ['error' => sprintf('This Docker registry map from "%s" already exists', $registryMap['from'])];
+			if (self::resolveRegistryTarget($deployConfig, $from) !== null) {
+				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'][] = $registryMap;
+			$deployConfig['registries'] = [...array_values($deployConfig['registries']), ['from' => $from, 'to' => $to]];
 			$daemonConfig->setDeployConfig($deployConfig);
 
 			return $this->mapper->update($daemonConfig);
@@ -211,12 +215,12 @@ 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_filter($deployConfig['registries'], function ($registry) use ($registryMap) {
-				return !($registry['from'] === $registryMap['from'] && $registry['to'] === $registryMap['to']);
-			});
+			$deployConfig['registries'] = array_values(array_filter($deployConfig['registries'], function ($registry) use ($registryMap) {
+				return !(($registry['from'] ?? null) === ($registryMap['from'] ?? null) && ($registry['to'] ?? null) === ($registryMap['to'] ?? null));
+			}));
 			$daemonConfig->setDeployConfig($deployConfig);
 
 			return $this->mapper->update($daemonConfig);
@@ -225,4 +229,33 @@ public function removeDockerRegistry(DaemonConfig $daemonConfig, array $registry
 			return null;
 		}
 	}
+
+	/**
+	 * 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 usable target wins. A target is usable when it is a non-empty
+	 * 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) {
+			$target = $registry['to'] ?? null;
+			if (($registry['from'] ?? null) !== $imageRegistry || !is_string($target)) {
+				continue;
+			}
+			$target = rtrim(trim($target), '/');
+			if ($target !== '') {
+				return $target;
+			}
+		}
+		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;
+	}
 }

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,75 @@ 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],
+			'legacy duplicate source: the first entry wins' => [
+				[$local, ['from' => 'ghcr.io', 'to' => 'registry.example.com']],
+				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,
+			],
+		];
+	}
+
+	#[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));
+	}
 }