refactor datagateway into proxy middleware - #3289
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 34 |
| Duplication | -15 |
🟢 Coverage 1.56% diff coverage · 0.00% coverage variation
Metric Results Coverage variation ✅ 0.00% coverage variation (-1.00%) Diff coverage ✅ 1.56% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (573844a) 84012 19245 22.91% Head commit (0fdf3ac) 84138 (+126) 19271 (+26) 22.90% (0.00%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#3289) 128 2 1.56% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
910dabd to
d785470
Compare
|
tests fail because the fake office receives an internal connection to localhost that it cannot route: {
"time": "2026-08-11T19:57:29.417",
"host": "9c3b943e11a3",
"level": "DEBUG",
"process": "wopiserver",
"module": "cs3iface",
"msg": "readfile: InitiateFileDownloadRes returned",
"trace": "752d0776bf00d82386eb32abe80a10a7",
"protocols": "[protocol: \"spaces\"\ndownload_endpoint: \"http://localhost:9158/data/spaces/2d695f7d-e3df-43e7-9746-70daf064e404$943f892c-1009-4912-b2b6-e21d8bdeb0b7%21943f892c-1009-4912-b2b6-e21d8bdeb0b7/test.wopitest\"\n]"
}I think this can be fixed by setting the STORAGE_USERS_DATA_SERVER_URL to |
bebf568 to
c7f932a
Compare
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
c7f932a to
0fdf3ac
Compare
|
This change makes a lot of sense. |
rhafer
left a comment
There was a problem hiding this comment.
@butonic I think the change itself is good. But do we really still need STORAGE_USERS_EXPOSE_DATA_SERVER config knob. Is there any good reason to keep it? If not, would you mind creating tickets to clean that up?
I guess the reva gateway won't need TRANSFER_SECRET and related stuff anymore as well.
We moved the datagateway download url token generation and unpacking into a proxy middleware and enabled
STORAGE_USERS_EXPOSE_DATA_SERVER=trueby default. This allows all internal requests do directly access the file using the dataprovider url (default isSTORAGE_USERS_DATA_SERVER_URL="http://localhost:9158/data", this needs to be routable by internal services, so set this when opencloud services are spread on multiple hosts if you see problems).This has two benefits:
OC_URL;GATEWAY_FRONTEND_PUBLIC_URLwhenSTORAGE_USERS_EXPOSE_DATA_SERVER=falseNow only the proxy is responsible for wrapping (the TUS 201 Created location header, unwrapping upload urls and forwarding them to the correct dataprovider. AFAICT this saves two reverseproxy trips (datagateway and proxy). Barely noticeable with small files, but very noticeable with another reverse proxy infront of opencloud that adds latency.
Regarding CS3 compatability: we do not expose the CS3 API. And imo if we do we can add a grpc interceprot that does the same thing when requests are made by external services.
The only question I have is if we want to delete all the datagateway code in the reva storage provider, gateway. I don't see the usecase. At cern they split the storageprovider into several instances to spread to load by username initials. That is not what the datagateway solves. It should just be a middleware when internal Up or download urls leave the instance. Which AFAICT only happens in the TUS POST 201 Created responses Location header.
Opinions?
Related: owncloud/ocis#6296
Related: #2860
Related: #1124