feat(nodes): add Docker API backend#394
Conversation
runleveldev
left a comment
There was a problem hiding this comment.
The UI displays "Creds missing" for Docker nodes
Running our default templates fail since they use SystemD. Similar to how Proxmox checks for "application containers" and make the nessecary adjustments, this needs to check for "system containers" and make the nessacary adjustments (such as setting privileged: true)
7a542e3b9e12 ghcr.io/mieweb/opensource-server/base:latest "/sbin/init" 19 seconds ago Exited (255) 17 seconds ago hollow-urban-otter
| @@ -12,23 +12,24 @@ import { | |||
| Spinner, | |||
| Switch, | |||
| useToast, | |||
| } from '@mieweb/ui'; | |||
| import { Server } from 'lucide-react'; | |||
| import { api, ApiError } from '@/lib/api'; | |||
| import { keys, queries } from '@/lib/queries'; | |||
| import { FormPageLayout } from '@/components/FormPageLayout'; | |||
| import type { Node } from '@/lib/types'; | |||
| } from "@mieweb/ui"; | |||
| import { Server } from "lucide-react"; | |||
| import { api, ApiError } from "@/lib/api"; | |||
| import { keys, queries } from "@/lib/queries"; | |||
| import { FormPageLayout } from "@/components/FormPageLayout"; | |||
| import type { Node } from "@/lib/types"; | |||
There was a problem hiding this comment.
I'm assuming these changes were from an auto-formatter. Please avoid making formatting-only changes since we don't have a consistent formatting standard. I'm open to a PR to enforce formatting standards, but it should be a dedicated PR to avoid expanding feature diffs.
| class DockerApi { | ||
| constructor(node = {}) { | ||
| this.node = node; | ||
| const dockerHost = node.apiUrl || process.env.DOCKER_HOST || 'unix:///var/run/docker.sock'; |
There was a problem hiding this comment.
This should only be node.apiUrl. The fallbacks seem unreachable, since a provisionable node is guarded on the existence of node.apiUrl. If somehow this code is reached wthout the node having an apiUrl, this should throw due to violated preconditions.
| const MANAGER_CONTAINER_LABEL = 'manager-os.container-id'; | ||
|
|
||
| function parseDockerHost(host) { | ||
| const raw = host || process.env.DOCKER_HOST || 'unix:///var/run/docker.sock'; |
There was a problem hiding this comment.
This fallback is the same as the one below and has the same issue: the fallbacks should be unreachable. I'll allow that the function should validate it's inputs (since this isn't typescript), but it should throw with precondition failed if typeof host !== 'string' (or similar).
| if (raw.startsWith('unix://')) { | ||
| const url = new URL(raw); | ||
| return { | ||
| baseURL: 'http://docker', | ||
| socketPath: decodeURIComponent(url.pathname), | ||
| }; | ||
| } | ||
|
|
||
| if (raw.startsWith('tcp://')) { | ||
| const url = new URL(raw); | ||
| return { | ||
| baseURL: `http://${url.host}`, | ||
| }; | ||
| } | ||
|
|
||
| if (raw.startsWith('http://') || raw.startsWith('https://')) { | ||
| return { | ||
| baseURL: raw.replace(/\/$/, ''), | ||
| }; | ||
| } |
There was a problem hiding this comment.
Similar to my earlier comments: this chunk of code allows for "bad" URLs and corrects them, such as by stripping path components. Rather than adjusting that here, the API server should validate that the URL is valid before it's ever saved into the database throwing a 4xx error if the user provided an invalid URL. The same validation code could be used here as a safetynet i.e.: if (!isValid(host)) throw.
| const MANAGER_NODE_LABEL = 'manager-os.node-id'; | ||
| const MANAGER_CONTAINER_LABEL = 'manager-os.container-id'; |
There was a problem hiding this comment.
We've been using the namespace of org.mieweb.opensource-server, these should follow that pattern, such as org.mieweb.opensource-server.node-id.
|
|
||
| async storageContents() { | ||
| return []; | ||
| } |
There was a problem hiding this comment.
This is used by the create-container.js job to check for cached images. It might be sufficient to rely on pullOciImage below to handle caching since docker is better at it. If so, please note that in a comment near this function.
| async createLxc(node, options = {}) { | ||
| if (!this.lastPulledImage) { | ||
| throw new Error('No Docker image has been pulled for this create operation'); | ||
| } |
There was a problem hiding this comment.
This is an unnecessary check. Images might be cached for example.
| async waitForTask() { | ||
| return { status: 'stopped', exitstatus: 'OK' }; | ||
| } |
There was a problem hiding this comment.
Is this a no-op because all calls to the Docker API are synchronous? If so, please note that in a comment near this function.
| macAddress: await this.getLxcMacAddress(node, vmid), | ||
| ipv4Address: await this.getLxcIpAddress(node, vmid), |
There was a problem hiding this comment.
Doing this this way creates 2 round-trips to the API. Fine for local, could get painful if remote. You should be able to query the API once for both these pieces of information.
| return !!(this.apiUrl && this.tokenId && this.secret); | ||
| } | ||
|
|
||
| static provisionableWhere(Sequelize) { |
There was a problem hiding this comment.
Can we import Sequelize into this file? Doesn't make sense to me to pass this as an argument.
Summary
Closes #375.
This PR adds support for Docker-backed nodes by introducing a new
DockerApiimplementation that follows the existing Node API contract used byProxmoxApiandDummyApi.Changes include:
DockerApiusing Docker Engine's native HTTP API throughaxiosunix:///var/run/docker.socktcp://host:2375http://...https://...Node.api()to dispatch toDockerApiwhennodeType === "docker"node.hasApiAccess()Node.provisionableWhere(...)nodeTypesupport to node API serialization, create, and update routesunix:///var/run/docker.sockin the frontend formRelated Issue
Closes #375
Testing
Tested locally with the following checks:
Ran backend syntax checks successfully:
Ran frontend type-check successfully:
Ran whitespace checks successfully:
Tested
DockerApidirectly against local Docker using:Verified direct Docker lifecycle flow:
nginx:latestVerified the UI can create a Docker node with:
Attempted full UI container creation with
nginx:latest. The job selected the Docker node and Docker image path correctly, but failed with:This appears to be a local runtime/socket-mount limitation because the Manager runtime does not currently have the host Docker socket mounted. The
DockerApiitself was verified successfully through the direct lifecycle test above.Screenshots
Docker node form
Docker node created
Breaking Changes
None known.
Notes
Full end-to-end Docker provisioning from the UI requires the Manager runtime to have access to the configured Docker host. For local socket usage,
/var/run/docker.sockmust be mounted or otherwise reachable from the process running Manager.