perf(s3): Provide direct pre-signed download link #54436
Conversation
|
FYI I've been working on #53634 to enable previews to generate by passing a presigned URL to ffmpeg. |
|
very excited for this! it'll be a gamechanger for nextcloud users no longer being "choked" behind nextcloud proxying everything. can't wait! |
|
I am also super excited, as it would allow to distribute data geographically with several auto mirroring minio instances across continents. Web browsers are automatically getting the file from the shortest round trip servers. |
|
Oh, this would be amazing for something I've been cooking up on the side! |
CarlSchwan
force-pushed
the
s3-signed-url
branch
from
f9f2e9a to
3a04f90
Compare
CarlSchwan
added
3. to review
CarlSchwan
requested review from
Altahrim,
come-nc,
icewind1991 and
salmart-dev
and removed request for
a team
| * @return array|false | ||
| * @param string $path Either the path or the fileId | ||
| * @return array{url: ?string}|false | ||
| * @since 9.0.0 | ||
| */ | ||
| public function getDirectDownload(string $path); |
There was a problem hiding this comment.
Is this still being called with path anywhere? implementations having to support both is kinda awkward (and an issue in the (rare) case of a numeric filename in the root of the storage).
I would either just drop the path option from this method or create a new method (in an extension trait) for getDirectDownloadById.
Also, the return type array should mention expiration
There was a problem hiding this comment.
I added a getDirectDownloadById API and deprecated getDirectDownload as it is not really used anymore.
| } | ||
|
|
||
| try { | ||
| return (string)$this->getConnection()->createPresignedRequest($command, $expiration, [ |
There was a problem hiding this comment.
just to make sure, have we confirmed that creating presigned urls is cheap enough to do for every file in a directory?
There was a problem hiding this comment.
I just checked with blackfire, creating a pre-signed urls has a minimal impact on performance since it's only doing an hash.
The only thing expensive is creating the connection object, because we are doing a HEAD request to the s3 server all the time, which I am fixing here: https://github.com/nextcloud/server/pull/56395/files
There was a problem hiding this comment.
I would like to comment that efficiency is not the only important parameter, as there are cases where the nextcloud server itself is unable to serve files because of the company policies which are forcing S3 servers to face the customer instead
|
Is there any way we can get a basic guide to testing this PR? I'm looking to get some basic performance data so I can plan my setup ahead of the release (and also have a bit of fun with it) |
CarlSchwan
force-pushed
the
s3-signed-url
branch
2 times, most recently
from
09235cc to
fd292e4
Compare
Best way is to test this PR is with juliusknorr/nextcloud-docker-dev#431 just change in .env PRIMARY to minio and start it up with |
CarlSchwan
force-pushed
the
s3-signed-url
branch
3 times, most recently
from
a9ac87c to
9fa3576
Compare
artonge
left a comment
artonge
left a comment
There was a problem hiding this comment.
- How is this behaving for files without read or download permissions?
- What happens if the object store is not available publicly?
CarlSchwan
force-pushed
the
s3-signed-url
branch
2 times, most recently
from
93cca86 to
e903621
Compare
I added a check for read. For the download permissions, the downloadUrl dav properties is already completely disabled for shares.
This is opt-in an behavior. You need to mark 'use_presigned_url' to true in your config.php for the object store. I need to add it to the documentation |
This is faster than going back to nextcloud to download the files. This is an opt-in setting that can be enabled by setting use_presigned_url in the object store config. Additionally add support for the proxy config which is needed in a docker setup. See juliusknorr/nextcloud-docker-dev#431 Signed-off-by: Carl Schwan <[email protected]>
CarlSchwan
force-pushed
the
s3-signed-url
branch
from
e903621 to
d6bfac4
Compare
Should we add a setup check to hint to this new possibility? |
I would propose to specify the URL instead, as the URL of the front S3 server might be and often is different than the one set internally |
Summary
This is faster than going back to nextcloud and setting up the file system to download the files.
TODO
Currently in my dev docker setup, the minio service is not available from outside, so the url can't be accessed. There should be a way to configure whether the s3 service is available or not from outsideMinio + pre-signed urls juliusknorr/nextcloud-docker-dev#431Also expose the preview url like this(let's move that to a seperate PR)Checklist