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

S3 Pre-signed URLs for upload endpoints #705

Closed
tsa96 opened this issue Feb 25, 2023 · 8 comments · Fixed by #964
Closed

S3 Pre-signed URLs for upload endpoints #705

tsa96 opened this issue Feb 25, 2023 · 8 comments · Fixed by #964
Labels
Blocked: Needs more work This needs more work to be able to be accepted For: Backend This is something to do for the backend (server folder) of the website. For: Frontend This is something to do for the front end (client folder) of the website. Type: Enhancement Something that builds on top of what already exists

Comments

@tsa96
Copy link
Member

tsa96 commented Feb 25, 2023

We want to keep traffic through the actual API to a minimum. There shouldn't be any need to stream data like map uploads through the API itself, instead, just generate pre-signed S3 URLs and pass those to the client.

We should go through every endpoint that does anything upload/download related and ensure that where possible, we avoid file streams. The game seems like it's already using it for map downloads, not sure about images and runs though - I'll look at that myself.

@tsa96 tsa96 added Type: Enhancement Something that builds on top of what already exists For: Backend This is something to do for the backend (server folder) of the website. For: Frontend This is something to do for the front end (client folder) of the website. labels Feb 25, 2023
@Gocnak
Copy link
Member

Gocnak commented Feb 25, 2023

Images will be fun because they need to be resized to ensure they're good. We could honestly use something like Cloudflare Workers to resize it and upload to the S3 bucket instead.

Runs will be a separate pipeline in the future, due to the extra processing required for AC purposes.

@tsa96
Copy link
Member Author

tsa96 commented Feb 26, 2023

Yeah, good point re images. Though the case of map image upload is rare enough that probably we can just let the API handle it? If total size of images for a map is like 20MB of images (rough estimate, for 4k). CF Workers sounds like quite a lot of fun though.

@tsa96
Copy link
Member Author

tsa96 commented Jul 8, 2023

Blocking for now; we're going to see how much bandwidth we get through after 0.10.0. S3 presigned gives less control, nice to avoid if poss.

@tsa96 tsa96 added the Blocked: Needs more work This needs more work to be able to be accepted label Jul 8, 2023
@tsa96
Copy link
Member Author

tsa96 commented Jun 16, 2024

Closing, we're going down this route #318

@tsa96 tsa96 closed this as not planned Won't fix, can't repro, duplicate, stale Jun 16, 2024
@GordiNoki
Copy link
Member

After a little conversation with Tom, we decided to yet again use pre-signed urls, since you can actually control an object being put in bucket and bandwidth isn't really an issue there.

@GordiNoki GordiNoki reopened this Jun 18, 2024
@tsa96
Copy link
Member Author

tsa96 commented Jun 18, 2024

Main reason against chunking being we don't have a way to guarantee requests with chunked data hit the same server, so using local disk doesn't work.

@Gocnak
Copy link
Member

Gocnak commented Jun 19, 2024

Note that nginx (and others) support session stickiness, something like ip_hash defined with our multiple instances would direct the same IP address to the same server: https://nginx.org/en/docs/http/load_balancing.html#nginx_load_balancing_with_ip_hash

@tsa96
Copy link
Member Author

tsa96 commented Jun 19, 2024

Note that nginx (and others) support session stickiness, something like ip_hash defined with our multiple instances would direct the same IP address to the same server: https://nginx.org/en/docs/http/load_balancing.html#nginx_load_balancing_with_ip_hash

Right, but if devops stuff gets more complicated in the future we have to do the work to ensure we keep that, plus we have to rely on writing to disk (which we don't do currently), seems like a lot of extra headache for quite a small feature, vs just using S3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocked: Needs more work This needs more work to be able to be accepted For: Backend This is something to do for the backend (server folder) of the website. For: Frontend This is something to do for the front end (client folder) of the website. Type: Enhancement Something that builds on top of what already exists
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants