allow for docker registry password to be a file #2419
Conversation
🦋 Changeset detectedLatest commit: b80c015 The changes in this PR will be included in the next version bump. This PR includes changesets to release 23 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
apps/supervisor/src/workloadManager/docker.ts (3)
11-11: Avoid synchronous disk I/O on the hot path (optional)Using readFileSync will block the event loop. Given this runs in the supervisor, impact is likely small, but consider either caching the password at startup or switching to async fs.promises.readFile with an async auth path.
280-307: Optional: cache password after first resolutionIf create() is called frequently, re-reading the same file each time is unnecessary. You can memoize the computed password (including the env string path case) to avoid repeated I/O.
51-61: Add basic tests for password resolution pathsTests to cover: plain env value, file:// happy path, file:// missing file (should log and skip auth), trimming behavior. I can scaffold these if helpful.
Also applies to: 280-307
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.changeset/red-moose-design.md(1 hunks)apps/supervisor/src/workloadManager/docker.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
apps/supervisor/src/workloadManager/docker.ts
🧬 Code Graph Analysis (1)
apps/supervisor/src/workloadManager/docker.ts (1)
apps/supervisor/src/env.ts (1)
env(105-105)
🔇 Additional comments (4)
.changeset/red-moose-design.md (1)
1-6: Changelog entry LGTMClear and scoped. Patch level is appropriate for this behavior change.
apps/supervisor/src/workloadManager/docker.ts (3)
41-49: Credential presence logging LGTMGood clarity on whether registry auth will be attempted.
169-175: Image pull refactor LGTM (uses auth() method)Swapping to this.auth() keeps behavior consistent while computing auth lazily. Once auth() is made safe against throws, this will be robust.
51-61: No lingeringthis.authproperty accesses detected
- Verified with
rg -n -C2 -g '!**/dist/**' -P '\bthis\.auth\b(?!\s*\()'and found no matches.All call sites correctly invoke
this.auth().
| private auth(): Docker.AuthConfig | undefined { | ||
| if (env.DOCKER_REGISTRY_USERNAME && env.DOCKER_REGISTRY_PASSWORD && env.DOCKER_REGISTRY_URL) { | ||
| return { | ||
| username: env.DOCKER_REGISTRY_USERNAME, | ||
| password: env.DOCKER_REGISTRY_PASSWORD, | ||
| password: getDockerPassword(), | ||
| serveraddress: env.DOCKER_REGISTRY_URL, | ||
| }; | ||
| } else { | ||
| this.logger.warn("🐋 No Docker registry credentials provided, skipping auth"); | ||
| } | ||
|
|
||
| return undefined; | ||
| } |
There was a problem hiding this comment.
Synchronous throw from getDockerPassword can crash create() before tryCatch; handle errors inside auth()
this.auth() calls getDockerPassword(), which may throw. Because arguments are evaluated before awaiting tryCatch(this.docker.createImage(...)), any synchronous throw here will bypass tryCatch and bubble out of create(), potentially crashing the supervisor. Wrap resolution and guard against empty/undefined passwords.
Apply this diff:
private auth(): Docker.AuthConfig | undefined {
- if (env.DOCKER_REGISTRY_USERNAME && env.DOCKER_REGISTRY_PASSWORD && env.DOCKER_REGISTRY_URL) {
- return {
- username: env.DOCKER_REGISTRY_USERNAME,
- password: getDockerPassword(),
- serveraddress: env.DOCKER_REGISTRY_URL,
- };
- }
-
- return undefined;
+ if (!(env.DOCKER_REGISTRY_USERNAME && env.DOCKER_REGISTRY_PASSWORD && env.DOCKER_REGISTRY_URL)) {
+ return undefined;
+ }
+
+ try {
+ const password = getDockerPassword();
+ if (!password) {
+ this.logger.warn("🐋 Docker registry password resolved to empty/undefined, skipping auth", {
+ username: env.DOCKER_REGISTRY_USERNAME,
+ url: env.DOCKER_REGISTRY_URL,
+ });
+ return undefined;
+ }
+
+ return {
+ username: env.DOCKER_REGISTRY_USERNAME,
+ password,
+ serveraddress: env.DOCKER_REGISTRY_URL,
+ };
+ } catch (error) {
+ this.logger.error("🐋 Failed to resolve Docker registry password, skipping auth", { error });
+ return undefined;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private auth(): Docker.AuthConfig | undefined { | |
| if (env.DOCKER_REGISTRY_USERNAME && env.DOCKER_REGISTRY_PASSWORD && env.DOCKER_REGISTRY_URL) { | |
| return { | |
| username: env.DOCKER_REGISTRY_USERNAME, | |
| password: env.DOCKER_REGISTRY_PASSWORD, | |
| password: getDockerPassword(), | |
| serveraddress: env.DOCKER_REGISTRY_URL, | |
| }; | |
| } else { | |
| this.logger.warn("🐋 No Docker registry credentials provided, skipping auth"); | |
| } | |
| return undefined; | |
| } | |
| private auth(): Docker.AuthConfig | undefined { | |
| if (!(env.DOCKER_REGISTRY_USERNAME && env.DOCKER_REGISTRY_PASSWORD && env.DOCKER_REGISTRY_URL)) { | |
| return undefined; | |
| } | |
| try { | |
| const password = getDockerPassword(); | |
| if (!password) { | |
| this.logger.warn("🐋 Docker registry password resolved to empty/undefined, skipping auth", { | |
| username: env.DOCKER_REGISTRY_USERNAME, | |
| url: env.DOCKER_REGISTRY_URL, | |
| }); | |
| return undefined; | |
| } | |
| return { | |
| username: env.DOCKER_REGISTRY_USERNAME, | |
| password, | |
| serveraddress: env.DOCKER_REGISTRY_URL, | |
| }; | |
| } catch (error) { | |
| this.logger.error("🐋 Failed to resolve Docker registry password, skipping auth", { error }); | |
| return undefined; | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/supervisor/src/workloadManager/docker.ts around lines 51 to 61, auth()
currently calls getDockerPassword() directly which can synchronously throw and
escape the outer tryCatch in create(); wrap the getDockerPassword() call in an
internal try/catch so auth() never throws (catch errors and return undefined or
omit the password field), and additionally guard against empty/undefined
password values (if password is falsy return undefined or build the auth object
without the password) so create() cannot be crashed by a synchronous exception
from getDockerPassword().
| function getDockerPassword(): string | undefined { | ||
| if (!env.DOCKER_REGISTRY_PASSWORD) { | ||
| return undefined | ||
| } | ||
| if (!env.DOCKER_REGISTRY_PASSWORD.startsWith("file://")) { | ||
| return env.DOCKER_REGISTRY_PASSWORD; | ||
| } | ||
|
|
||
| const passwordPath = env.DOCKER_REGISTRY_PASSWORD.replace("file://", ""); | ||
|
|
||
| console.debug( | ||
| JSON.stringify({ | ||
| message: "🔑 Reading docker password from file", | ||
| passwordPath, | ||
| }) | ||
| ); | ||
|
|
||
| try { | ||
| const password = readFileSync(passwordPath, "utf8").trim(); | ||
| return password; | ||
| } catch (error) { | ||
| console.error(`Failed to read docker password from file: ${passwordPath}`, error); | ||
| throw new Error( | ||
| `Unable to read docker password from file: ${error instanceof Error ? error.message : "Unknown error" | ||
| }` | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t throw on password file read failure; use structured logging and return undefined
Throwing here can crash the call site (see auth() comment). Prefer logging and returning undefined so the caller can skip auth. Also, use the project’s structured logger instead of console.* and fix minor style nits.
Apply this diff:
function getDockerPassword(): string | undefined {
if (!env.DOCKER_REGISTRY_PASSWORD) {
- return undefined
+ return undefined;
}
if (!env.DOCKER_REGISTRY_PASSWORD.startsWith("file://")) {
return env.DOCKER_REGISTRY_PASSWORD;
}
const passwordPath = env.DOCKER_REGISTRY_PASSWORD.replace("file://", "");
- console.debug(
- JSON.stringify({
- message: "🔑 Reading docker password from file",
- passwordPath,
- })
- );
+ moduleLogger.debug("🔑 Reading docker password from file", { passwordPath });
try {
const password = readFileSync(passwordPath, "utf8").trim();
return password;
} catch (error) {
- console.error(`Failed to read docker password from file: ${passwordPath}`, error);
- throw new Error(
- `Unable to read docker password from file: ${error instanceof Error ? error.message : "Unknown error"
- }`
- );
+ moduleLogger.error("Failed to read docker password from file", {
+ passwordPath,
+ error,
+ });
+ return undefined;
}
}Add this module-level logger once near the top of the file (outside the selected range):
// Module-level logger for helpers outside the class
const moduleLogger = new SimpleStructuredLogger("docker-workload-manager");🤖 Prompt for AI Agents
In apps/supervisor/src/workloadManager/docker.ts around lines 280 to 307, the
helper currently uses console.debug/console.error and throws on failing to read
a password file; change it to use the project’s module-level structured logger
and return undefined on failure instead of throwing so callers can skip auth.
Add the suggested moduleLogger declaration once near the top of the file
(outside this range): const moduleLogger = new
SimpleStructuredLogger("docker-workload-manager"); then replace console.debug
with moduleLogger.debug(...) including the same message and passwordPath, and in
the catch block call moduleLogger.warn or moduleLogger.error with a structured
object { message: "Failed to read docker password from file", passwordPath,
error: error instanceof Error ? error.message : String(error) } and return
undefined instead of throwing; keep the same behavior for non-file passwords and
undefined env values.
No description provided.