Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive remote node management system with file-based persistent storage, encryption, REST API endpoints, and UI integration. It adds cryptographic utilities, domain models, a persistence layer, resolver logic, API handlers, and frontend components to manage remote nodes alongside existing configuration-sourced nodes. Changes
Sequence DiagramssequenceDiagram
participant Client
participant API as API Handler
participant Resolver
participant Store as File Store
participant Config as Config Source
participant Crypto as Encryptor
Client->>API: POST /remote-nodes (create)
activate API
API->>Resolver: Check existing nodes
activate Resolver
Resolver->>Store: List store nodes
activate Store
Store-->>Resolver: Nodes
deactivate Store
Resolver->>Config: Check config nodes
Config-->>Resolver: Nodes
Resolver-->>API: All nodes
deactivate Resolver
API->>Crypto: Encrypt credentials
activate Crypto
Crypto-->>API: Encrypted credentials
deactivate Crypto
API->>Store: Create node
activate Store
Store->>Store: Write JSON file atomically
Store-->>API: Success
deactivate Store
API-->>Client: 201 Created
deactivate API
sequenceDiagram
participant Client
participant SSEProxy as SSE Proxy Handler
participant Resolver
participant RemoteNode as Remote Node
participant HTTPClient as HTTP Client
participant RemoteAPI as Remote API
Client->>SSEProxy: WebSocket connection + remote node name
activate SSEProxy
SSEProxy->>Resolver: GetByName(nodeName)
activate Resolver
Resolver-->>SSEProxy: RemoteNode
deactivate Resolver
SSEProxy->>RemoteNode: ApplyAuth(request)
activate RemoteNode
RemoteNode-->>SSEProxy: Request with credentials
deactivate RemoteNode
SSEProxy->>HTTPClient: NewRequest with auth headers
activate HTTPClient
HTTPClient->>RemoteAPI: GET /path
activate RemoteAPI
RemoteAPI-->>HTTPClient: Response stream
deactivate RemoteAPI
HTTPClient-->>SSEProxy: Response
deactivate HTTPClient
SSEProxy-->>Client: Stream events via WebSocket
deactivate SSEProxy
sequenceDiagram
participant Server
participant FileSystem
participant EnvVar as Environment
participant Crypto as Encryptor
participant Store as FileRemoteNode Store
participant Resolver
Server->>EnvVar: Check DAGU_ENCRYPTION_KEY
activate EnvVar
alt Key exists in env
EnvVar-->>Server: Key
else No env key
EnvVar-->>Server: None
Server->>FileSystem: Read dataDir/auth/encryption_key
activate FileSystem
alt File exists
FileSystem-->>Server: Key
else No file
FileSystem-->>Server: None
Server->>Server: Generate 32-byte random key
Server->>FileSystem: Write to dataDir/auth/encryption_key
FileSystem-->>Server: Success
end
deactivate FileSystem
end
deactivate EnvVar
Server->>Crypto: NewEncryptor(key)
activate Crypto
Crypto-->>Server: Encryptor ready
deactivate Crypto
Server->>Store: New(dataDir/remote-nodes, encryptor)
activate Store
Store->>FileSystem: Scan directory
activate FileSystem
FileSystem-->>Store: File list
deactivate FileSystem
Store->>Crypto: Decrypt each node's credentials
activate Crypto
Crypto-->>Store: Plaintext credentials
deactivate Crypto
Store-->>Server: Store initialized
deactivate Store
Server->>Resolver: NewResolver(configNodes, store)
activate Resolver
Resolver-->>Server: Resolver ready
deactivate Resolver
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (6)
internal/service/frontend/sse/proxy.go (1)
16-20: Consider propagating the actual error for non-not-found cases.The error from
GetByNameis not included in the response. While returning a generic message is fine forErrRemoteNodeNotFound, other errors (e.g., store connection failures) would be silently converted to "unknown remote node" which may hinder debugging.♻️ Proposed improvement to differentiate error types
node, err := h.nodeResolver.GetByName(r.Context(), nodeName) if err != nil { + if errors.Is(err, remotenode.ErrRemoteNodeNotFound) { + http.Error(w, fmt.Sprintf("unknown remote node: %s", nodeName), http.StatusBadRequest) + return + } + http.Error(w, "failed to resolve remote node", http.StatusInternalServerError) - http.Error(w, fmt.Sprintf("unknown remote node: %s", nodeName), http.StatusBadRequest) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/frontend/sse/proxy.go` around lines 16 - 20, When handling the error returned by h.nodeResolver.GetByName in the SSE proxy, distinguish ErrRemoteNodeNotFound from other errors: if errors.Is(err, ErrRemoteNodeNotFound) keep the existing http.Error with "unknown remote node" and StatusBadRequest; for any other error include the actual err message (and/or log it) and return an appropriate server error (e.g., http.StatusInternalServerError) so store/connection failures are visible; reference h.nodeResolver.GetByName, ErrRemoteNodeNotFound, and the handler’s http.Error/w to locate where to implement the conditional handling.ui/src/App.tsx (1)
153-179: Use typed API access (client.GET/ SWR query hook) instead of ad-hocfetchhereThis mount fetch duplicates request plumbing and bypasses the typed API layer expected in UI code. Please switch this to the existing typed query/client path and keep
remoteNodein query params.As per coding guidelines
ui/src/**/*.{ts,tsx}: “Frontend code must use ... SWR for data fetching, andopenapi-fetchfor typed API calls.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/App.tsx` around lines 153 - 179, Replace the ad-hoc fetch inside the React.useEffect (the fetchRemoteNodeNames helper that calls fetch and calls setRemoteNodes) with the project's typed API/SWR approach: use the existing openapi-fetch client GET method or the SWR query hook that wraps it to request the endpoint with the remoteNode=local query param, pass the same Authorization bearer token from localStorage into the client's headers if required, and update setRemoteNodes with the returned remoteNodes (including 'local' and deduping as before); ensure you remove the manual try/catch/fetch plumbing and keep the dependency on config.apiURL (or the SWR key) so the hook re-runs when the API URL changes.ui/src/pages/remote-nodes/index.tsx (1)
63-196: Consolidate remote-node API calls behind typed client/query hooksThe page currently repeats manual
fetchflows for list/delete/test/refresh. Please switch these to the typed API client/query pattern to keep request typing, error handling, and cache behavior consistent across UI code.Based on learnings: “Applies to ui/**/*.{ts,tsx} : Use
client.GETandclient.POSTcalls with remoteNode in query parameters for API interactions.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/remote-nodes/index.tsx` around lines 63 - 196, The component repeats raw fetch logic in fetchRemoteNodes, refreshRemoteNodeNames, handleDeleteNode, and handleTestConnection; replace these manual fetch calls with the typed API client/query hooks (use client.GET and client.POST) passing remoteNode as a query parameter and the Authorization token via the client or hook config, and update callers to use the typed response shapes (e.g., RemoteNodeResponse, test result type) so you retain typing, consistent error handling, and cache behavior; ensure you remove duplicate JSON parsing and response.ok checks and instead rely on the client’s error handling, then call fetchRemoteNodes and refreshRemoteNodeNames (or invalidate the query cache) after deletions to refresh state, and update imports to bring in the typed client/query utilities.internal/cmn/crypto/key.go (1)
35-41: Consider validating the key format when reading from file.When reading an existing key from the file, there's no validation that the key is actually valid (e.g., proper base64, appropriate length). A corrupted or manually-edited key file could cause downstream encryption failures with less informative errors.
Additionally, there's a potential TOCTOU race between checking if the file exists (line 35-41) and writing a new key (line 54). If two processes start simultaneously with no key file, both could generate keys and one would overwrite the other. This is generally acceptable since both keys are valid, but worth documenting.
♻️ Optional: Add key format validation
data, err := os.ReadFile(keyPath) //nolint:gosec // path is constructed from trusted dataDir if err == nil { key := string(data) - if key != "" { + if key != "" && len(key) >= 32 { // Basic sanity check return key, nil } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmn/crypto/key.go` around lines 35 - 41, When reading the key in key.go (the block that calls os.ReadFile and sets key := string(data)), validate the key's format before returning: check it decodes as base64 (or the expected encoding) and meets required length/entropy constraints and return a clear error if validation fails; if invalid, fall through to generate a new key as the existing logic does. Also add a short comment near the ReadFile/generate key logic documenting the TOCTOU race (two processes may concurrently generate keys and one may overwrite the other) so future maintainers are aware. Ensure these checks are applied where key is returned from the function and reference the ReadFile/key variable and the generate-new-key branch so you modify the correct control flow.api/v1/api.yaml (1)
5831-5838: Consider clarifying acceptedremoteNodeIdformats in the parameter description.Documenting expected forms (e.g., UUID and config-derived IDs) would reduce client ambiguity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1/api.yaml` around lines 5831 - 5838, Update the RemoteNodeId parameter description (parameter name: remoteNodeId, schema: type string) to explicitly list accepted formats — for example: UUID (v4) pattern, legacy/config-derived numeric or hyphenated IDs, or any length constraints — and, where applicable, include a regex example or sample values; keep the required/ in: path settings unchanged and ensure the description clearly states which formats are preferred or deprecated.internal/service/frontend/api/v1/remote_nodes.go (1)
294-295: Normalize/healthURL construction.When
APIBaseURLends with/, current concatenation yields//health. It usually works, but normalizing keeps behavior strict.🔧 Proposed tweak
- healthURL := fmt.Sprintf("%s/health", node.APIBaseURL) + healthURL := strings.TrimSuffix(node.APIBaseURL, "/") + "/health"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/frontend/api/v1/remote_nodes.go` around lines 294 - 295, The health URL is built by concatenating node.APIBaseURL and "/health" which can produce "//health" if node.APIBaseURL ends with "/", so normalize construction by trimming any trailing slash from node.APIBaseURL before appending "/health" (use strings.TrimRight or TrimSuffix) when building the healthURL variable in remote_nodes.go; ensure the code still produces a single "/health" segment for both cases where node.APIBaseURL ends with or without a trailing slash.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v1/api.yaml`:
- Around line 5469-5636: The remote-node operations (operationId:
listRemoteNodes, createRemoteNode, getRemoteNode, updateRemoteNode,
deleteRemoteNode, testRemoteNodeConnection) are missing consistent
authentication/authorization responses; for each of these operations add
standardized 401 (Unauthorized) and 403 (Forbidden) responses with
application/json bodies referencing the existing Error schema so auth failures
are documented uniformly across all remote-node endpoints.
In `@internal/cmn/config/key_hints.go`:
- Around line 98-101: The hint mapping that maps "remotenodes.isbasicauth" /
"remotenodes.isauthtoken" directly to "remote_nodes.auth_type" must be
value-aware: update the migration/hint logic that handles these keys so it
converts legacy boolean values into the enum strings ("none"|"basic"|"token")
instead of just renaming keys. Specifically, when encountering
remotenodes.isbasicauth=true set remote_nodes.auth_type="basic" (false => no-op
or "none"), when encountering remotenodes.isauthtoken=true set
remote_nodes.auth_type="token", handle cases where both booleans exist (define
precedence or emit an explicit conflict hint), and preserve existing
remote_nodes.auth_type if already set; adjust the code that builds key_hints
(the mapping entries for "remotenodes.isbasicauth" and
"remotenodes.isauthtoken") to perform this value-aware migration rather than a
blind key rename.
In `@internal/cmn/schema/config.schema.json`:
- Around line 440-443: The schema added "remote_nodes_dir" is not bound because
PathsDef in internal/cmn/config/definition.go lacks a corresponding field with
the mapstructure tag; add a new string field (e.g., RemoteNodesDir string
`mapstructure:"remote_nodes_dir" json:"remote_nodes_dir"`) to the PathsDef
struct so the config decoder binds the value, and run tests to ensure loading
picks up remote_nodes_dir.
In `@internal/persis/fileremotenode/store.go`:
- Around line 346-349: The delete path currently aborts if
s.loadStoredFromFile(filePath) returns a parse error; change the error handling
in the deletion flow (around the call to loadStoredFromFile in store.go) so that
JSON parse/decoding errors are treated like os.ErrNotExist: log or note the
corrupt metadata (but do not return the error) and proceed with deletion as if
stored == nil; ensure only unexpected non-parse I/O errors still return (keep
the existing errors.Is(err, os.ErrNotExist) logic and add a check for
JSON/unmarshal/parse errors to fallback to continuing the delete).
- Around line 128-147: The current code silently logs decryption errors from
s.encryptor.Decrypt and leaves node.BasicAuthPassword/node.AuthToken empty which
may later overwrite stored secrets; instead, on decryption failure for
stored.BasicAuthPasswordEnc or stored.AuthTokenEnc return an error (wrap with
context including stored.ID and which field failed) from the containing function
so the load fails visibly and the caller can handle it, and only set
node.BasicAuthPassword/node.AuthToken when Decrypt succeeds.
In `@internal/remotenode/resolver.go`:
- Around line 84-87: In ListAll, stop treating r.store.List failures as
non-fatal: when storeNodes, err := r.store.List(ctx) returns an error, return
that error (or a wrapped/contextualized error) to the caller instead of logging
and continuing to return config-only results; update the branch in ListAll that
currently logs the error to return nil, err (or fmt.Errorf("listing store nodes:
%w", err)) so callers receive a proper failure signal.
In `@internal/service/frontend/api/v1/remote_nodes.go`:
- Around line 278-283: The code currently treats IDs with
remotenode.ConfigNodeIDPrefix the same as other names and calls
a.remoteNodeResolver.GetByName, which returns store-first results and can return
the wrong node on name collisions; change the branch that detects
remotenode.ConfigNodeIDPrefix to perform a source-aware lookup instead — e.g.,
call a.remoteNodeResolver.GetByNameFromSource(ctx, name,
remotenode.NodeSourceConfig) or add/use a dedicated method like
a.remoteNodeResolver.GetConfigByName(ctx, name) (or fetch and verify node.Source
== remotenode.NodeSourceConfig) so that config-prefixed IDs always resolve to
the config-backed node rather than a store node.
In `@internal/service/frontend/api/v1/remote.go`:
- Around line 128-131: The proxy handler in remote.go writes the proxied body
(respData) but never forwards resp.StatusCode, causing all proxied successes to
appear as 200; update the success path in the handler that uses w, resp,
respData to call w.WriteHeader(resp.StatusCode) (after copying headers like
Content-Type and before w.Write(respData)) so the original upstream status code
is preserved when proxying responses.
- Around line 33-34: In WithRemoteNode middleware, guard resolver before calling
resolver.GetByName: check if resolver == nil before dereferencing (the call at
node, err := resolver.GetByName(r.Context(), remoteNodeName)); if nil and
remoteNodeName is present, handle the case explicitly (e.g., return an HTTP
500/appropriate error response or skip lookup and proceed) and log a clear
message; ensure you still handle the existing err path from GetByName when
resolver is non-nil.
In `@rfcs/draft/027-remote-node-store.md`:
- Line 76: The fenced code blocks in rfcs/draft/027-remote-node-store.md are
missing language identifiers and trigger MD040; update each triple-backtick
block (the ones containing the RemoteNodeStore interface diagram, the <data_dir>
filesystem example, the POST /api/v1/remote-nodes request, and the GET
/api/v1/remote-nodes response) to include appropriate languages (e.g., ```text,
```text, ```http, ```json respectively) — also apply the same fix to the other
blocks noted (lines 92, 140, 157) so all fences include language identifiers.
In `@ui/src/pages/remote-nodes/index.tsx`:
- Around line 330-332: The icon-only actions Button (the Button wrapping
MoreHorizontal) lacks an accessible name; add an aria-label prop to that Button
(for example aria-label="Row actions" or "More actions") so screen readers can
announce its purpose, e.g., update the Button component that contains
MoreHorizontal to include aria-label="More actions" (and optionally
aria-haspopup="menu" if it opens a menu).
- Around line 265-280: The TableCell rendering the node name/description and the
TableCell showing node.apiBaseUrl can overflow; add wrapping utility classes to
prevent layout breakage by applying "whitespace-normal break-words" to the
relevant elements (e.g., the TableCell for the name/description and/or the inner
description div that renders node.description, and the TableCell that renders
node.apiBaseUrl) so long hostnames/URLs/descriptions wrap instead of
overflowing; update the className on those TableCell(s) / inner div(s) near the
JSX that references node.description and node.apiBaseUrl accordingly.
In `@ui/src/pages/remote-nodes/RemoteNodeFormModal.tsx`:
- Around line 99-170: Replace raw fetch usage in RemoteNodeFormModal.tsx with
the typed openapi-fetch client (import client from '@/api/v1/client') and call
client.PATCH('/remote-nodes/{id}') when isEditing and
client.POST('/remote-nodes') when creating, passing path params ({ id: node.id
}) and query params ({ remoteNode: appBarContext.selectedRemoteNode || 'local'
}) and the request body (use CreateRemoteNodeRequestAuthType for authType and
conditionally include basicAuthUsername/basicAuthPassword/authToken only when
present). Also remove the unconditional token throw (the localStorage TOKEN_KEY
check in the try block) and instead handle missing token gracefully when
auth.mode === 'none' (do not require Authorization header) and include
Authorization: `Bearer ${token}` only when token exists; surface client errors
by checking the returned { error } and throwing new Error(error.message) if
present.
---
Nitpick comments:
In `@api/v1/api.yaml`:
- Around line 5831-5838: Update the RemoteNodeId parameter description
(parameter name: remoteNodeId, schema: type string) to explicitly list accepted
formats — for example: UUID (v4) pattern, legacy/config-derived numeric or
hyphenated IDs, or any length constraints — and, where applicable, include a
regex example or sample values; keep the required/ in: path settings unchanged
and ensure the description clearly states which formats are preferred or
deprecated.
In `@internal/cmn/crypto/key.go`:
- Around line 35-41: When reading the key in key.go (the block that calls
os.ReadFile and sets key := string(data)), validate the key's format before
returning: check it decodes as base64 (or the expected encoding) and meets
required length/entropy constraints and return a clear error if validation
fails; if invalid, fall through to generate a new key as the existing logic
does. Also add a short comment near the ReadFile/generate key logic documenting
the TOCTOU race (two processes may concurrently generate keys and one may
overwrite the other) so future maintainers are aware. Ensure these checks are
applied where key is returned from the function and reference the ReadFile/key
variable and the generate-new-key branch so you modify the correct control flow.
In `@internal/service/frontend/api/v1/remote_nodes.go`:
- Around line 294-295: The health URL is built by concatenating node.APIBaseURL
and "/health" which can produce "//health" if node.APIBaseURL ends with "/", so
normalize construction by trimming any trailing slash from node.APIBaseURL
before appending "/health" (use strings.TrimRight or TrimSuffix) when building
the healthURL variable in remote_nodes.go; ensure the code still produces a
single "/health" segment for both cases where node.APIBaseURL ends with or
without a trailing slash.
In `@internal/service/frontend/sse/proxy.go`:
- Around line 16-20: When handling the error returned by
h.nodeResolver.GetByName in the SSE proxy, distinguish ErrRemoteNodeNotFound
from other errors: if errors.Is(err, ErrRemoteNodeNotFound) keep the existing
http.Error with "unknown remote node" and StatusBadRequest; for any other error
include the actual err message (and/or log it) and return an appropriate server
error (e.g., http.StatusInternalServerError) so store/connection failures are
visible; reference h.nodeResolver.GetByName, ErrRemoteNodeNotFound, and the
handler’s http.Error/w to locate where to implement the conditional handling.
In `@ui/src/App.tsx`:
- Around line 153-179: Replace the ad-hoc fetch inside the React.useEffect (the
fetchRemoteNodeNames helper that calls fetch and calls setRemoteNodes) with the
project's typed API/SWR approach: use the existing openapi-fetch client GET
method or the SWR query hook that wraps it to request the endpoint with the
remoteNode=local query param, pass the same Authorization bearer token from
localStorage into the client's headers if required, and update setRemoteNodes
with the returned remoteNodes (including 'local' and deduping as before); ensure
you remove the manual try/catch/fetch plumbing and keep the dependency on
config.apiURL (or the SWR key) so the hook re-runs when the API URL changes.
In `@ui/src/pages/remote-nodes/index.tsx`:
- Around line 63-196: The component repeats raw fetch logic in fetchRemoteNodes,
refreshRemoteNodeNames, handleDeleteNode, and handleTestConnection; replace
these manual fetch calls with the typed API client/query hooks (use client.GET
and client.POST) passing remoteNode as a query parameter and the Authorization
token via the client or hook config, and update callers to use the typed
response shapes (e.g., RemoteNodeResponse, test result type) so you retain
typing, consistent error handling, and cache behavior; ensure you remove
duplicate JSON parsing and response.ok checks and instead rely on the client’s
error handling, then call fetchRemoteNodes and refreshRemoteNodeNames (or
invalidate the query cache) after deletions to refresh state, and update imports
to bring in the typed client/query utilities.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
api/v1/api.gen.goapi/v1/api.yamlinternal/cmn/config/config.gointernal/cmn/config/config_test.gointernal/cmn/config/definition.gointernal/cmn/config/key_hints.gointernal/cmn/config/loader.gointernal/cmn/config/loader_test.gointernal/cmn/crypto/aes.gointernal/cmn/crypto/key.gointernal/cmn/schema/config.schema.jsoninternal/persis/fileremotenode/store.gointernal/remotenode/remotenode.gointernal/remotenode/resolver.gointernal/remotenode/resolver_test.gointernal/remotenode/store.gointernal/service/audit/entry.gointernal/service/frontend/api/v1/api.gointernal/service/frontend/api/v1/remote.gointernal/service/frontend/api/v1/remote_nodes.gointernal/service/frontend/server.gointernal/service/frontend/sse/handler.gointernal/service/frontend/sse/handler_test.gointernal/service/frontend/sse/proxy.gointernal/service/frontend/sse/proxy_test.gorfcs/draft/027-remote-node-store.mdui/src/App.tsxui/src/api/v1/schema.tsui/src/contexts/AppBarContext.tsui/src/lib/fetchJson.tsui/src/menu.tsxui/src/pages/remote-nodes/RemoteNodeFormModal.tsxui/src/pages/remote-nodes/index.tsx
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1704 +/- ##
==========================================
- Coverage 69.97% 69.34% -0.64%
==========================================
Files 385 390 +5
Lines 42679 43064 +385
==========================================
- Hits 29866 29863 -3
- Misses 10418 10655 +237
- Partials 2395 2546 +151
... and 11 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Summary by CodeRabbit
Release Notes