feat(supervisor): add ecr support to docker workloads #2424
Conversation
|
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
apps/supervisor/src/workloadManager/ecrAuth.ts (2)
18-24: Region logging may print an unresolved provider function.ecrClient.config.region can be a string or a provider function. Logging it directly will either omit the field (JSON.stringify drops functions) or log a noisy function body. Resolve it to a string-friendly value.
Apply this diff:
- this.logger.info("🔐 ECR Auth Service initialized", { - region: this.ecrClient.config.region, - }); + const region = + typeof this.ecrClient.config.region === "string" + ? this.ecrClient.config.region + : "<provider>"; + this.logger.info("🔐 ECR Auth Service initialized", { region });
6-11: Clarify naming: token vs password.The cache field token actually holds the decoded password. Rename for clarity to avoid confusion in future refactors.
-interface ECRTokenCache { - token: string; +interface ECRTokenCache { + password: string; username: string; serverAddress: string; expiresAt: Date; } @@ - const tokenCache: ECRTokenCache = { - token: password, + const tokenCache: ECRTokenCache = { + password, username, serverAddress: authData.proxyEndpoint, expiresAt, }; @@ return { username: this.tokenCache!.username, - password: this.tokenCache!.token, + password: this.tokenCache!.password, serveraddress: this.tokenCache!.serverAddress, }; @@ return { username: newToken.username, - password: newToken.token, + password: newToken.password, serveraddress: newToken.serverAddress, };Also applies to: 88-93, 113-116, 130-134
apps/supervisor/src/workloadManager/docker.ts (1)
170-172: Log chosen auth path for debuggability (optional).Consider logging which auth path was used (static vs ECR) at pull time for faster diagnostics.
- const authConfig = await this.getAuthConfig(); + const authConfig = await this.getAuthConfig(); + if (authConfig) { + logger.debug("Using authenticated pull", { + authType: this.staticAuth ? "static" : "ecr", + serveraddress: authConfig.serveraddress, + }); + } else { + logger.debug("Pulling without registry auth"); + }
📜 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 ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
apps/supervisor/package.json(1 hunks)apps/supervisor/src/workloadManager/docker.ts(4 hunks)apps/supervisor/src/workloadManager/ecrAuth.ts(1 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/ecrAuth.tsapps/supervisor/src/workloadManager/docker.ts
🧬 Code Graph Analysis (2)
apps/supervisor/src/workloadManager/ecrAuth.ts (1)
packages/core/src/v3/utils/structuredLogger.ts (1)
SimpleStructuredLogger(21-102)
apps/supervisor/src/workloadManager/docker.ts (3)
apps/supervisor/src/workloadManager/types.ts (1)
WorkloadManager(16-18)apps/supervisor/src/workloadManager/ecrAuth.ts (1)
ECRAuthService(13-144)packages/core/src/utils.ts (1)
tryCatch(5-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
apps/supervisor/package.json (1)
16-16: Dependency addition looks good; confirm compatibility matrix.Adding @aws-sdk/client-ecr is appropriate for ECR auth. Ensure the Node runtime used by supervisor matches AWS SDK v3 requirements (Node 16+), which the repo likely already satisfies.
If you want, I can scan the repo for engines/node declarations and CI matrix to confirm alignment.
apps/supervisor/src/workloadManager/ecrAuth.ts (1)
61-65: Good error handling with tryCatch.Wrapping the AWS SDK call with tryCatch keeps the code tidy and prevents unhandled rejections. Logging avoids leaking secrets.
apps/supervisor/src/workloadManager/docker.ts (2)
229-247: Helper is solid and side-effect free.Prefers static creds and falls back to ECR dynamically, returning undefined when not available. Works well with the corrected createImage options.
18-21: Private fields restructuring LGTM.Separating staticAuth from ecrAuthService clarifies the auth paths and keeps state minimal.
Adds support for AWS ECR authentication to the Docker workload manager, enabling automatic token-based authentication and refresh when AWS credentials are available and falling back to static credentials when configured.
Environment Variables
No new environment variables are required. The implementation automatically detects existing AWS credentials through standard AWS SDK mechanisms:
AWS_ACCESS_KEY_IDandAWS_SECRET_ACCESS_KEY(if using access keys)AWS_PROFILE(if using AWS profiles - requires mounting.aws)AWS_ROLE_ARNandAWS_WEB_IDENTITY_TOKEN_FILE(if using IAM roles)Backwards Compatibility
Fully backwards compatible - existing Docker registry configurations continue to work unchanged, with ECR authentication only activating when AWS credentials are detected.