Page MenuHomePhabricator

[commtest] Move backup client to shared
ClosedPublic

Authored by michal on Dec 8 2023, 7:13 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 12, 11:45 AM
Unknown Object (File)
Thu, Nov 7, 12:00 AM
Unknown Object (File)
Oct 18 2024, 2:43 AM
Unknown Object (File)
Oct 18 2024, 2:43 AM
Unknown Object (File)
Oct 18 2024, 2:43 AM
Unknown Object (File)
Oct 18 2024, 2:42 AM
Unknown Object (File)
Oct 18 2024, 2:42 AM
Unknown Object (File)
Oct 2 2024, 10:45 AM
Subscribers

Details

Summary

ENG-5555 and ENG-5554

Move the backup client to shared so that it can be used on native. There are a few changes that I will annotate in inline comments. The total amount of moved logic is pretty small but if it's hard to review, let me know and I will split this into more diffs.

Test Plan
  • cargo run in backup and blob
  • yarn run-integration-tests backup
  • yarn run-performance-tests blob

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

shared/backup_client/Cargo.toml
6–17

All dependencies with these specific versions are already being used in our codebase.

19–22

This will be required for native. On Android we have to use rustls-tls because the native-tls bundles openssl which conflicts with our openssl setup during linking.

I added it in this diff to make sure that the commtest works with default features (native-tls).

shared/backup_client/src/lib.rs
10–20

Instead of passing url to each function call I added a struct that will hold the configuration.

37–40

Previously the user of the backup client passed in the blob hash. Now we just calculate it here.
If later we will need to be able to specify the hash value again (e.g. streaming the data?) we can just add e.g. user_data_hash: Option<String> to BackupData struct and do the hashing conditionally. But I prefer to keep it simple for now.

41

Previously it was:

Part::stream(Body::wrap_stream(
        stream! { yield Ok::<Vec<u8>, Infallible>(user_keys);  },
      )),

I changed it because I don't think it gave us anything -> we were still sending one big chunk (it wasn't really "streaming").

56

Previously this was converted to commtest-specific error. Now we just return reqwest::Error if something went wrong.

63

Previously this took ownership to BackupDescriptor. I changed it to take a reference as we don't need the complete ownership of the value inside the function.

123–130

Added an error struct.

michal requested review of this revision.Dec 8 2023, 8:54 AM

It's been several days that this diff is up – ping to reviewers to take a look!

sorry for the delay. accepting, but with a question: how are we setting the CA Certificate against which to verify the server’s TLS certificate?

shared/backup_client/src/lib.rs
47

can we make the newline character ("\n") a named constant?

This revision is now accepted and ready to land.Dec 13 2023, 5:53 PM

Makes sense to me

shared/backup_client/src/lib.rs
123–130

Do you think it's a good time and place to introduce thiserror? 😉

sorry for the delay. accepting, but with a question: how are we setting the CA Certificate against which to verify the server’s TLS certificate?

From reqwest docs we can see that rustls-tls feature is equivalent to rustls-tls-webpki-roots, which uses the wepki-roots crate. There is an alternative crate rustls-native-certs (that can be activated with a feature) but the docs don't say anything about Android/ iOS compatibility and I haven't tested it.

shared/backup_client/src/lib.rs
123–130

To be honest I think derive_more get's us to about 95% percent of what thiserror gives us, with a slightly worse syntax. I would prefer to switch to thiserror but it would be better to change it throughout the whole codebase.

Add a constant, fix lockfile

This revision was landed with ongoing or failed builds.Dec 19 2023, 7:30 AM
This revision was automatically updated to reflect the committed changes.