[backup] Introduce upload endpoint
ClosedPublic
Actions

Authored by michal on Aug 28 2023, 1:33 AM.

Details

Reviewers

bartek
• patryk
kamil
• jon

Commits

rCOMM82ad20c82fd8: [backup] Introduce upload endpoint

Summary

ENG-4501

Endpoint for uploading the backups.

Depends on D8962

Test Plan

Make a request:

POST http://127.0.0.1:50052/backups
Content-Type: multipart/form-data; boundary=bound
Authorization: Bearer eyJ1c2VySUQiOiAiMSIsICJhY2Nlc3NUb2tlbiI6ICIyIiwgImRldmljZUlEIjogIjMifQ==

--bound
Content-Disposition: form-data; name="backup_id"

id7
--bound
Content-Disposition: form-data; name="user_keys_hash"

keys_hash7
--bound
Content-Disposition: form-data; name="user_keys"
Content-Type: application/octet-stream

keys7
--bound
Content-Disposition: form-data; name="user_data_hash"

data_hash7
--bound
Content-Disposition: form-data; name="user_data"
Content-Type: application/octet-stream

data7
--bound
Content-Disposition: form-data; name="attachments"

a7-1
a7-2
a7-3
--bound--

checked that there is a new backup row in dynamodb
checked that there are 2 blobs in dynamodb

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

michal created this revision.Aug 28 2023, 1:33 AM

Herald added a reviewer: • jon. · View Herald TranscriptAug 28 2023, 1:33 AM

Herald added subscribers: tomek, ashoat. · View Herald Transcript

michal added a child revision: D8964: [backup] Introduce download endpoints.Aug 28 2023, 1:49 AM

Harbormaster completed remote builds in B22155: Diff 30421.Aug 28 2023, 1:52 AM

michal requested review of this revision.Aug 28 2023, 1:52 AM

Note that blob cleanup (if error occurs later during upload) will be added in another diff.

bartek added inline comments.Aug 28 2023, 5:17 AM

services/backup/src/http/handlers/backup.rs
34–37	This part is repeating frequently. Wondering if worth doing a macro: https://github.com/CommE2E/comm/blob/master/services/blob/src/http/utils.rs
120
135	typo
160	Do logs from `receive_promise` and `send_promise` appear inside the `forward_to_blob` span? You might need to wrap them in `.in_current_span()` from `tracing-futures` crate

michal added inline comments.Aug 28 2023, 6:45 AM

services/backup/src/http/handlers/backup.rs

34–37

If anything I think we could add a wrapper around get_text_field like get_named_text_field that would do the check and return an error if it's not correct. I'm not really a fan of the macro approach.

160

Seem to work fine:

2023-08-28T13:39:33.689100Z  INFO upload_backup: backup::http::handlers::backup: Upload backup request
2023-08-28T13:39:33.689312Z TRACE upload_backup{backup_id="id7"}:forward_to_blob: backup::http::handlers::backup: Reading blob fields: "user_keys_hash", "user_keys"
2023-08-28T13:39:33.689398Z TRACE upload_backup{backup_id="id7"}:forward_to_blob: backup::http::handlers::backup: Receiving blob data
2023-08-28T13:39:33.689425Z TRACE upload_backup{backup_id="id7"}:forward_to_blob: backup::http::handlers::backup: Finished receiving blob data
2023-08-28T13:39:33.735435Z TRACE upload_backup{backup_id="id7"}:forward_to_blob: backup::http::handlers::backup: Reading blob fields: "user_data_hash", "user_data"
2023-08-28T13:39:33.735567Z TRACE upload_backup{backup_id="id7"}:forward_to_blob: backup::http::handlers::backup: Receiving blob data
2023-08-28T13:39:33.735589Z TRACE upload_backup{backup_id="id7"}:forward_to_blob: backup::http::handlers::backup: Finished receiving blob data

it might be because they are not executed on separate tasks, but they are run on the current task.

bartek accepted this revision.Aug 28 2023, 6:49 AM

bartek added inline comments.

services/backup/src/http/handlers/backup.rs
34–37	ok, up to you. I suggested a macro because I like complicating things 😉
160	it might be because they are not executed on separate tasks, but they are run on the current task. Yeah, that's likely. I wasn't sure what `tokio::try_join` is doing underneath