fix(k8s): apply daemon registry mappings - #1046
Conversation
Signed-off-by: Oleksandr Piskun <oleksandr2088@icloud.com>
Signed-off-by: Oleksandr Piskun <oleksandr2088@icloud.com>
Signed-off-by: Oleksandr Piskun <oleksandr2088@icloud.com>
|
/backport to stable35 |
|
/backport to stable34 |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughThe change adds shared registry target resolution and validation to Fixed issue severity: Medium. <fixed_issue_severity>Medium</fixed_issue_severity> Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to A daemon with a whitespace-only stored registry target can reject a valid replacement and fail to deploy images. Normalize stored targets before merging. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Registry mappings now affect images deployed to the cluster. The shared resolution logic reduces disagreement between deployment backends, but a local-image mapping can leave an app undeployed if the image is not available after its existing deployment is removed. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: b53d5b1f-5d41-49fb-bda0-ff700cb724e3
📒 Files selected for processing (6)
lib/DeployActions/DockerActions.phplib/DeployActions/KubernetesActions.phplib/Service/DaemonConfigService.phptests/php/DeployActions/DockerActionsTest.phptests/php/DeployActions/KubernetesActionsTest.phptests/php/Service/DaemonConfigServiceTest.php
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.
| 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) { |
There was a problem hiding this comment.
🩺 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/phpRepository: 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 260Repository: 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
doneRepository: 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']);
}));Signed-off-by: Oleksandr Piskun <oleksandr2088@icloud.com>
Signed-off-by: Oleksandr Piskun <oleksandr2088@icloud.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Normalize local before checking the special target. · DaemonConfigService.php:241
lib/Service/DaemonConfigService.php:241
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNormalize
localbefore checking the special target.
addDockerRegistry()acceptsto: "local/". This condition does not recognize that value aslocal, thenrtrim()returnslocaland the resolver returns it as a mirror registry. Docker then pullslocal/..., and Kubernetes does not preserve the required unchanged-image behavior.Trim the target before the
localcomparison. Add a resolver test forto: "local/".Proposed fix
- if (($registry['from'] ?? null) !== $imageRegistry || !is_string($target) || $target === 'local') { + if (($registry['from'] ?? null) !== $imageRegistry || !is_string($target)) { continue; } $target = rtrim($target, '/'); - if ($target !== '') { + if ($target !== '' && $target !== 'local') { return $target; }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 519481f6-4d41-426c-8b71-be779d00baa4
📒 Files selected for processing (4)
lib/DeployActions/DockerActions.phplib/Service/DaemonConfigService.phptests/php/DeployActions/DockerActionsTest.phptests/php/Service/DaemonConfigServiceTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/php/DeployActions/DockerActionsTest.php
- lib/DeployActions/DockerActions.php
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review.
kyteinsky
left a comment
kyteinsky
left a comment
There was a problem hiding this comment.
- The
localtarget has no effect on Kubernetes. AppAPI never pulls there. HaRP creates the pods withimagePullPolicy: IfNotPresent, so the image name is kept and the kubelet pulls it if the node does not have it. Docker is unchanged: the name is kept and the pull is skipped.
maybe we can keep the same behaviour as docker here, set to no-pull when this registry mapping is present.
There was a problem hiding this comment.
not sure but seems like the added lines 450-452 do the same thing which the rest of the function is doing
Signed-off-by: Oleksandr Piskun <oleksandr2088@icloud.com>
Signed-off-by: Oleksandr Piskun <oleksandr2088@icloud.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Ignore unusable stored targets during the duplicate-source check. · DaemonConfigService.php:193
lib/Service/DaemonConfigService.php:193
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winIgnore unusable stored targets during the duplicate-source check.
If a stored mapping has
from: ghcr.ioandto: /,resolveRegistryTargetignores it. This check still rejects a new, usableghcr.iomapping. The administrator cannot apply a mirror until the unusable entry is removed separately. Apply the resolver’s target-usability check before treating a source as occupied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 7758e80f-47eb-485c-b238-27791ef5a2f1
📒 Files selected for processing (6)
lib/DeployActions/DockerActions.phplib/DeployActions/KubernetesActions.phplib/Service/DaemonConfigService.phptests/php/DeployActions/DockerActionsTest.phptests/php/DeployActions/KubernetesActionsTest.phptests/php/Service/DaemonConfigServiceTest.php
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.
good idea - done! 👍 |
Signed-off-by: Oleksandr Piskun <oleksandr2088@icloud.com>
Signed-off-by: Oleksandr Piskun <oleksandr2088@icloud.com>
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 9a754fcf-76b3-4863-8413-2dcd4b782dd2
📒 Files selected for processing (4)
lib/Service/DaemonConfigService.phptests/php/DeployActions/DockerActionsTest.phptests/php/DeployActions/KubernetesActionsTest.phptests/php/Service/DaemonConfigServiceTest.php
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review.
Signed-off-by: Oleksandr Piskun <oleksandr2088@icloud.com>
2 participants
Fixes #1039
Registry mappings of a deploy daemon (
occ app_api:daemon:registry:add, or the "Docker registries" dialog) were only applied by the Docker backend. On a Kubernetes daemon the mapping was stored and listed, butKubernetesActionsbuilt the image name straight frominfo.xml, so the cluster kept pulling from the upstream registry. That breaks air-gapped and mirror-only setups.Changes
DaemonConfigService::resolveRegistryTarget()resolves the effective target of the daemon's mappings for the registry an ExApp image comes from: a mirror registry,local, or none. The first usable mapping of a registry wins. The image name (resolveImageRegistry(), used by both Docker image name builders andKubernetesActions::buildImageName()) and the Docker pull decision (shouldPullImage()) both derive from it, so the two backends and the two decisions cannot disagree. On Kubernetes the daemon config is passed down to where the HaRP/exapp/createpayload is built, for single and multi-role deployments.fromortois missing or not a string, or when the target is empty once surrounding whitespace and trailing slashes are dropped. Before, such an entry raised aTypeErrorduring a Docker deploy or produced an image name with an empty registry. Tolerating them matters more now that Kubernetes resolves mappings too: it does so after the previous Deployment has been removed, so a throwing resolver would leave the ExApp undeployed.addDockerRegistry()andremoveDockerRegistry()keepregistriesa list. Removing an entry that was not the last one left a JSON object with gaps ({"1": {...}}), which breaks the add form of the registries dialog. Data that already has that shape is repaired on the next add or remove.addDockerRegistry()rejects unusable mappings (same cases as above), trims both values, drops trailing slashes and stores onlyfromandto. A stored unusable entry no longer blocks adding a usable mapping for the same source.removeDockerRegistry()no longer fails with aTypeErrorwhen an unusable entry is stored next to the mapping that is removed, or when the daemon has no mappings at all.localmapping the Kubernetes create payload carriesimage_pull_policy: Never, so the node uses only an image it already has, the same as Docker skipping the pull. This needs a HaRP that supports the field (feat(k8s): accept image_pull_policy in the create payload HaRP#122, onmain, not in a release yet); older HaRP versions ignore the field and keepIfNotPresent.local/(with a trailing slash) counts aslocal; before it was treated as a registry namedlocal.Behaviour
localmapping keeps the image name on both backends. Docker skips the pull. Kubernetes getsimagePullPolicy: Neverwith a HaRP that supportsimage_pull_policy, and fails within seconds when the image is not on the node; with an older HaRP the policy staysIfNotPresentand the kubelet pulls the image if the node does not have it. A Kubernetes daemon that already had alocalmapping stored therefore needs the image on every node that can schedule the pod once HaRP supports the field.addDockerRegistry()rejects a duplicate source), the first usable entry wins on both backends. Before, the image name took the first non-localentry while the Docker pull decision looked at anylocalentry.imagePullSecrets, so a mirror that needs authentication needs pull credentials configured in the cluster.<tag>-cuda,<tag>-rocm) are still not used on Kubernetes.