Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Propagate Range header to underlying storage #527

Closed
wants to merge 0 commits into from

Conversation

ochaton
Copy link
Contributor

@ochaton ochaton commented Feb 3, 2023

I've made an attempt to support downloads with passed Range header and propagate Range to underlying storage (except GDrive, I didn't find any clue in the gdrive doc how to pass Range header to it).

Only range with single slice is supported, and I didn't implement suffix and prefix ranges. If Range is not satisfied then header is ignored (it seems legit by RFC, but maybe it is better to respond with HTTP 416).

This patch does not fix the problem with downloads limiting :( this is really hard. IMO, it is better to not allow range downloads for limited content.

realtes to #490

server/storage/gdrive.go Outdated Show resolved Hide resolved
@aspacca
Copy link
Collaborator

aspacca commented Feb 4, 2023

thank you @ochaton , very impressive!

I can test on local a gdrive storage, but I'd need someone else to test for s3 and storj

maybe for s3 I can try with minio

cc @stefanbenten

@ochaton
Copy link
Contributor Author

ochaton commented Feb 4, 2023

My general setup is transfer.sh in front of S3 storage. But it would be better if you test patch yourself :)

@stefanbenten
Copy link
Collaborator

I can test both S3 and Storj early next week.

@ochaton
Copy link
Contributor Author

ochaton commented Feb 10, 2023

@stefanbenten @aspacca Any updates?

@aspacca
Copy link
Collaborator

aspacca commented Feb 10, 2023

@ochaton do you mind if i push on your branch to check to test range in gdrive?

@aspacca aspacca mentioned this pull request Feb 11, 2023
@aspacca
Copy link
Collaborator

aspacca commented Feb 11, 2023

@ochaton please, check ochaton#1 :)

@aspacca
Copy link
Collaborator

aspacca commented Feb 25, 2023

@stefanbenten , @ochaton where you able to test storj and s3 with my commit added?

I will think about bumping google.golang.org/api version than required to drop 1.15 go build support
see my reasons at #505 (comment) for avoiding that if possible

@aspacca
Copy link
Collaborator

aspacca commented Mar 1, 2023

This patch does not fix the problem with downloads limiting :( this is really hard. IMO, it is better to not allow range downloads for limited content.

do you mean in terms of respecting Max-Download?

not sure it will make a huge difference in the end: we could prevent range to be fulfilled if Max-Download is set, this way we will prevent partial download in the case the number of "chunked" requests will me higher than the Max-Download value. but there's nothing we can do in hitting the Max-Download limit even if we ignore the range: I would not avoid to decrease the counter, since it's an open door to abuse of the fact that the value was set

@aspacca
Copy link
Collaborator

aspacca commented Mar 1, 2023

tested with s3 on localhost

@stefanbenten did you have the chance to test with storj?

@ochaton
Copy link
Contributor Author

ochaton commented Mar 3, 2023

This patch does not fix the problem with downloads limiting :( this is really hard. IMO, it is better to not allow range downloads for limited content.

do you mean in terms of respecting Max-Download?

not sure it will make a huge difference in the end: we could prevent range to be fulfilled if Max-Download is set, this way we will prevent partial download in the case the number of "chunked" requests will me higher than the Max-Download value. but there's nothing we can do in hitting the Max-Download limit even if we ignore the range: I would not avoid to decrease the counter, since it's an open door to abuse of the fact that the value was set

I didn't get the idea of Max-Downloads. Does anyone use it to limit "only-once" downloads or it can be used in different way? Maybe you have some production statistics?

Nevertheless it seems that ignoring Range header for objects with defined 'Max-Downloads' in metadata file is the only option to keep compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants