Skip to content

fix(k8s): apply daemon registry mappings #1046

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
oleksandr-nc merged 10 commits into from
Sep 25, 2026
Merged

fix(k8s): apply daemon registry mappings #1046

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6a7858b
fix(k8s): apply daemon registry mappings
oleksandr-nc Sep 18, 2026
5435e02
fix: keep registry mappings a list
oleksandr-nc Sep 18, 2026
af4a36b
fix: reject unusable registry mappings
oleksandr-nc Sep 21, 2026
e4f9a6d
fix: pull the image when a mirror mapping applies
oleksandr-nc Sep 21, 2026
cb6817d
fix: tolerate malformed entries when removing a mapping
oleksandr-nc Sep 21, 2026
fc14a86
refactor: resolve the effective registry mapping once
oleksandr-nc Sep 25, 2026
80688ef
feat(k8s): skip the image pull for local mappings
oleksandr-nc Sep 25, 2026
c08c5a5
fix: normalise registry mappings on add
oleksandr-nc Sep 25, 2026
1b66957
test: pin the mapping order and the K8s pull policy cases
oleksandr-nc Sep 25, 2026
04955fc
fix: ignore whitespace-only registry targets
oleksandr-nc Sep 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 6 additions & 29 deletions lib/DeployActions/DockerActions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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'];
}

Expand All @@ -449,28 +441,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 {
Expand Down
25 changes: 15 additions & 10 deletions lib/DeployActions/KubernetesActions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 = '';

Expand All @@ -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;
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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 {
Expand Down
69 changes: 51 additions & 18 deletions lib/Service/DaemonConfigService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -173,30 +176,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) {
Comment thread
coderabbitai[bot] marked this conversation as resolved.
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);
Expand All @@ -210,12 +214,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) {

@coderabbitai coderabbitai Bot Sep 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🔎 Supported by static analysis

🏁 Script executed:

sed -n '150,270p' lib/Service/DaemonConfigService.php
rg -n 'removeDockerRegistry|addDockerRegistry|registries' lib tests/php

Repository: nextcloud/app_api

Length of output: 11740

🏁 Script executed:

sed -n '1,155p' tests/php/Service/DaemonConfigServiceTest.php
printf '\n--- DaemonConfig references and storage/config handling ---\n'
rg -n -C 3 'class DaemonConfig|function getDeployConfig|function setDeployConfig|deploy_config|json_decode|json_encode|registr' lib tests/php migrations appinfo README.md 2>/dev/null | head -n 260

Repository: nextcloud/app_api

Length of output: 28299

🏁 Script executed:

fd -i 'DaemonConfig*' .
printf '\n--- model and mapper candidates ---\n'
for f in $(fd -i 'DaemonConfig*' . | head -n 12); do
  printf '\n### %s\n' "$f"
  rg -n -C 3 'class DaemonConfig|deploy_config|getDeployConfig|setDeployConfig|json|serialize|column' "$f" 2>/dev/null | head -n 100
done

Repository: nextcloud/app_api

Length of output: 31750

Handle malformed stored entries during removal.

If registries contains a string and the requested valid mapping, the callback accesses $registry['from'] on the string. PHP raises a TypeError, which catch (Exception) does not catch. Registry removal then fails with an uncaught error.

Guard non-array entries and use null-coalescing for missing fields.

Proposed fix 
 			$deployConfig['registries'] = array_values(array_filter($deployConfig['registries'], function ($registry) use ($registryMap) {
-				return !($registry['from'] === $registryMap['from'] && $registry['to'] === $registryMap['to']);
+				if (!is_array($registry)) {
+					return true;
+				}
+				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);

return $this->mapper->update($daemonConfig);
Expand All @@ -224,4 +228,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;
}
}
74 changes: 74 additions & 0 deletions tests/php/DeployActions/DockerActionsTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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));
}
}
Loading
Loading