Option to omit transfer on same storage volume #147
Conversation
📝 WalkthroughWalkthroughAdds a StoragePoolConfiguration and per-node StoragePool setting; sets X-Storage-Pool on outbound transfer requests; short-circuits transfer handling when the incoming pool header matches; and prevents async server file cleanup during deletion when the storage pool is active and matching. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Router
participant Config
participant Env
Client->>Router: POST /transfer (may include X-Storage-Pool)
Router->>Config: Read Transfers.StoragePool (Enabled, PoolName)
Config-->>Router: Return storage pool config
alt StoragePool enabled AND header matches PoolName
Router->>Env: Create server environment
Env-->>Router: Env created
Router->>Client: HTTP 200 (early exit)
else Standard transfer flow
Router->>Client: Process multipart transfer as before
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (1)
✏️ Tip: You can disable this entire section by setting 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 |
hUwUtao
changed the title
config/config.go
Outdated
| // with other nodes in the cluster. When this value is set and matches the value on a target node, | ||
| // Wings will assume the server data already exists on the target and will skip copying and cleanup. | ||
| // When empty, transfers behave normally. | ||
| StoragePool string `yaml:"storage_pool"` |
There was a problem hiding this comment.
I would change this to a struct with an Enabled bool and PoolName stringor something similar to you are not using "" as a value to match to mean enabled.
It would look like this.
StoragePool struct {
Enabled bool `default:"false" yaml:"enabled"`
PoolName string `yaml:"pool_name"`
} `yaml:"storage_pool"`There was a problem hiding this comment.
I know this would change all the other code you wrote but is arguably a better choice.
# Conflicts: # router/router_server.go # server/transfer/source.go
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@router/router_server.go`:
- Around line 278-288: The current check only tests
config.Get().System.Transfers.StoragePool.Enabled and therefore skips file
removal for every deletion when the pool is enabled; change the condition in the
goroutine that runs on server deletion (the block using s *server.Server,
s.Filesystem(), fs.Path()) to skip cleanup only when a storage pool is both
enabled AND named (check StoragePool.Enabled && StoragePool.PoolName != "") and
the specific server is actually being transferred; i.e., require the server
transfer flag/field (use the Server struct's transfer indicator or add/ call a
helper like s.IsTransferring() or check s.Transfer/ s.TransferTarget / transfer
ID field) in the same conditional before returning early — otherwise proceed to
close UnixFS and call os.RemoveAll(p) as before.
In `@router/router_transfer.go`:
- Line 135: The comparison using remotePool.PoolName vs the unauthenticated
header value sourcePool (strings.EqualFold(remotePool.PoolName, sourcePool)) is
unsafe because sourcePool is attacker-controlled; change the check to validate
the source pool from an authenticated, signed source (e.g., the transfer JWT
claims) instead of the header. Update the transfer handling logic to parse and
verify the transfer token's claims (the signed source_pool claim) and replace
the strings.EqualFold(remotePool.PoolName, sourcePool) check with a comparison
against that verified claim (reject if the token's source_pool is missing or
does not match remotePool.PoolName), or alternatively require a strong identity
proof (client cert or panel-signed value) rather than the X-Storage-Pool header.
Ensure you still keep the remotePool.Enabled and remotePool.PoolName != ""
guards and add explicit rejection/logging when the authenticated claim does not
match.
🧹 Nitpick comments (2)
config/config.go (1)
300-311: Configuration structure looks good.The new
StoragePoolConfigurationfollows existing patterns. One minor observation: whenEnabledistruebutPoolNameis empty, the feature silently becomes a no-op because all call sites checkPoolName != "". Consider logging a warning at startup ifEnabledistrueandPoolNameis empty, to help operators catch misconfigurations early.server/transfer/source.go (1)
26-28: Misleading comment: says "Always include" but header is conditionally set.The comment on line 26 states "Always include the configured storage pool identifier" but the header is only set when
sp.Enabled && sp.PoolName != ""(line 70). Consider rewording to match the conditional logic.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
config/config.gorouter/router_server.gorouter/router_transfer.goserver/transfer/source.go
🧰 Additional context used
🧬 Code graph analysis (3)
router/router_server.go (1)
config/config.go (2)
Get(451-459)Transfers(292-303)
router/router_transfer.go (2)
config/config.go (2)
Get(451-459)Transfers(292-303)router/middleware/middleware.go (1)
CaptureAndAbort(60-63)
server/transfer/source.go (1)
config/config.go (2)
Get(451-459)Transfers(292-303)
🔇 Additional comments (2)
server/transfer/source.go (1)
70-72: Header setting logic is correct.The conditional matches the receiver-side check in
router_transfer.go. No issues here.router/router_transfer.go (1)
132-144: The early-exit path with empty response body is correct and requires no changes.The early-exit returns HTTP 200 with no body. When
PushArchiveToTargetexecutesio.ReadAll(res.Body)on line 190, it safely returns an empty[]byte{}with no error. The caller atrouter/router_server_transfer.go:84explicitly ignores this return value with_, so there is no downstream parsing or processing of the bytes. No nil or empty-parse error will occur.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| { | ||
| remotePool := config.Get().System.Transfers.StoragePool | ||
| sourcePool := c.GetHeader("X-Storage-Pool") | ||
| if remotePool.Enabled && remotePool.PoolName != "" && sourcePool != "" && strings.EqualFold(remotePool.PoolName, sourcePool) { |
There was a problem hiding this comment.
Pool name comparison relies on an unauthenticated header value for a security-sensitive decision.
While the request itself is authenticated via Bearer token, the X-Storage-Pool header value is entirely attacker-controlled. A compromised or malicious source node could set this header to match the destination's pool name, causing the transfer to succeed without any files being copied—resulting in an empty server directory on the destination.
Consider whether a stricter trust model is needed (e.g., the Panel could include the source pool name in the signed JWT transfer token instead of relying on a plain header).
🤖 Prompt for AI Agents
In `@router/router_transfer.go` at line 135, The comparison using
remotePool.PoolName vs the unauthenticated header value sourcePool
(strings.EqualFold(remotePool.PoolName, sourcePool)) is unsafe because
sourcePool is attacker-controlled; change the check to validate the source pool
from an authenticated, signed source (e.g., the transfer JWT claims) instead of
the header. Update the transfer handling logic to parse and verify the transfer
token's claims (the signed source_pool claim) and replace the
strings.EqualFold(remotePool.PoolName, sourcePool) check with a comparison
against that verified claim (reject if the token's source_pool is missing or
does not match remotePool.PoolName), or alternatively require a strong identity
proof (client cert or panel-signed value) rather than the X-Storage-Pool header.
Ensure you still keep the remotePool.Enabled and remotePool.PoolName != ""
guards and add explicit rejection/logging when the authenticated claim does not
match.
2 participants
Hi, I'm building a Ceph based infrastructure that syncs the volume (ex: /var/lib/pelican/volumes) across nodes in cluster. A roadblock is that wings cares about transfer and cleaning up, the stack will not work as expected.
A configuration option is added for the purpose to circumvent this behavior. When configured:
Wings will not attempt to transfer files or cleanup.
It will still move server to
storage_poolthat is not match, but the cleanup will not work at the moment.At the moment, it works with the stack if simply configured with a common name across nodes with the following setup:
If the configuration left empty, wings will behave as just normal.
Summary by CodeRabbit
New Features
Bug Fixes / Behavior Changes