fix: pass registry credentials to environments#603
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds container registry credential support across DTOs, handlers, services, and jobs. Updates PullImage and image update methods to accept external credentials. Introduces an EnvironmentMiddleware that proxies requests and injects credentials. Adjusts router/bootstrap wiring and image polling job to load credentials. Minor .env change removes AGENT_MODE default. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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. 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/internal/services/image_service.go (1)
166-235: Use URL-safe base64 for RegistryAuth (StdEncoding is incorrect).Dockerd expects X-Registry-Auth / RegistryAuth to be base64-url-safe (base64url); using base64.StdEncoding can cause the header to be ignored. Replace base64.StdEncoding.EncodeToString(authBytes) with base64.URLEncoding.EncodeToString(authBytes) (include padding). (docs.docker.com)
Locations: backend/internal/services/image_service.go — lines ~190 and ~226.
♻️ Duplicate comments (1)
backend/internal/job/image_polling_job.go (1)
101-133: Code duplication flagged earlier inenvironment_service.go.This method duplicates credential loading logic. See the review comment on
backend/internal/services/environment_service.golines 332-361.
🧹 Nitpick comments (3)
.env.dev (1)
27-27: Fix the AGENT_MODE example in.env.dev.The commented line shows
# AGENT_MODE=f, which appears truncated—please update it to:-# AGENT_MODE=f +# AGENT_MODE=falseThe application already defaults to
falsewhenAGENT_MODEis unset (seegetBoolEnvOrDefault("AGENT_MODE", false)inbackend/internal/config/config.go).backend/internal/api/environment_handler.go (1)
101-116: Approve refactoring; recommend refining HTTP status codes
- PairAgentWithBootstrap treats every non-200 response and network error as a generic error, leading the handler to always return 502. To improve API semantics, consider refactoring so the handler can distinguish cases:
- Return 401/403 when the agent responds with an authentication error
- Return 503 for timeouts or agent unavailability
- Fallback to 502 for other gateway/network failures
backend/internal/services/environment_service.go (1)
328-330: Consider whether exposing the database connection is necessary.Exposing
GetDB()breaks encapsulation and may allow callers to bypass service-layer business logic. If this is only needed for specific operations, consider adding targeted methods instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.env.dev(1 hunks)backend/internal/api/container_handler.go(1 hunks)backend/internal/api/environment_handler.go(2 hunks)backend/internal/api/image_handler.go(1 hunks)backend/internal/api/image_update_handler.go(2 hunks)backend/internal/bootstrap/jobs_bootstrap.go(1 hunks)backend/internal/bootstrap/router_bootstrap.go(1 hunks)backend/internal/dto/container_registry_dto.go(1 hunks)backend/internal/dto/image_dto.go(1 hunks)backend/internal/dto/image_update_dto.go(1 hunks)backend/internal/job/image_polling_job.go(5 hunks)backend/internal/middleware/environment_middleware.go(5 hunks)backend/internal/services/environment_service.go(3 hunks)backend/internal/services/image_service.go(3 hunks)backend/internal/services/image_update_service.go(4 hunks)backend/internal/services/project_service.go(1 hunks)backend/internal/services/updater_service.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-23T20:31:03.836Z
Learnt from: CR
PR: ofkm/arcane#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-23T20:31:03.836Z
Learning: Applies to internal/middleware/**/*.go : Implement authentication middleware (JWT/OIDC) and correct CORS handling
Applied to files:
backend/internal/middleware/environment_middleware.go
🪛 GitHub Actions: Run Backend Linter
backend/internal/middleware/environment_middleware.go
[error] 139-139: cognitive complexity 31 of func (*EnvironmentMiddleware).proxyHTTP is high (> 30) (gocognit)
🪛 GitHub Check: Run Golangci-lint
backend/internal/middleware/environment_middleware.go
[failure] 253-253:
error is not nil (line 251) but it returns nil (nilerr)
[failure] 139-139:
cognitive complexity 31 of func (*EnvironmentMiddleware).proxyHTTP is high (> 30) (gocognit)
⏰ 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). (3)
- GitHub Check: test
- GitHub Check: build
- GitHub Check: Analyze (go)
🔇 Additional comments (12)
backend/internal/api/environment_handler.go (1)
4-4: LGTM: Structured logging import.The addition of
log/slogis appropriate for the structured logging introduced in the error handling below.backend/internal/dto/image_dto.go (1)
12-13: LGTM! Credentials field enables registry authentication.The addition of the
Credentialsfield toImagePullDtoaligns with the PR objective to pass registry credentials for private image pulls. Theomitemptytag correctly makes this field optional.backend/internal/dto/image_update_dto.go (1)
46-47: LGTM! Credentials field supports batch update authentication.The addition of the
Credentialsfield toBatchImageUpdateRequestenables passing registry credentials for batch image update operations. Theomitemptytag correctly makes this field optional.backend/internal/dto/container_registry_dto.go (1)
16-21: LGTM! New credential type supports registry authentication.The
ContainerRegistryCredentialtype provides the necessary fields for passing registry credentials. Thebinding:"required"tags on URL, Username, and Token ensure these fields are validated during request binding.backend/internal/bootstrap/jobs_bootstrap.go (1)
26-26: Constructor signature matches. The call toNewImagePollingJobcorrectly passesappServices.ContainerRegistryas a*services.ContainerRegistryService, aligning with the constructor’s fourth parameter.backend/internal/api/image_handler.go (1)
127-127: LGTM! Credentials are now propagated to the image pull operation.The updated call correctly passes
req.Credentialsto the service layer, enabling external registry credentials for image pulls.backend/internal/api/image_update_handler.go (1)
101-101: LGTM! Credentials are now passed to batch image update checks.The update correctly propagates credentials from the request to the service layer.
backend/internal/job/image_polling_job.go (2)
21-27: LGTM! Registry service dependency added to support credential loading.The constructor now accepts
registryServiceto enable loading of registry credentials for image update checks.
64-75: LGTM! Credentials are now loaded and passed to image update checks.The job now loads registry credentials before checking images, with appropriate error handling that logs failures but continues operation.
backend/internal/services/image_service.go (3)
100-100: LGTM! Image pull now accepts external credentials.The signature update enables passing registry credentials from callers, supporting the credential propagation flow introduced in this PR.
107-116: Good observability for credential handling.The debug logging helps trace credential usage without exposing sensitive tokens, and the warning on auth failures provides visibility into configuration issues.
171-197: LGTM! External credentials take precedence with proper fallback.The logic correctly:
- Iterates through external credentials first
- Matches credentials by normalized registry host
- Returns early on match
- Falls back to database credentials if no match
The host normalization ensures consistent matching across different URL formats.
|
I pulled release 1.3 and still seeing the broken behavior. Does this need some configuration? |
2 participants
Fixes: #562
Summary by CodeRabbit
New Features
Improvements