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

Redis crypto store #1256

Closed
wants to merge 22 commits into from
Closed

Conversation

andybalaam
Copy link
Contributor

@andybalaam andybalaam commented Dec 5, 2022

Create a Redis crypto store to allow deploying e.g. in a Kubernetes environment with no disk.

Signed-off-by: Andy Balaam <[email protected]>

@andybalaam andybalaam marked this pull request as ready for review December 5, 2022 14:54
@codecov
Copy link

codecov bot commented Dec 5, 2022

Codecov Report

Base: 74.22% // Head: 74.43% // Increases project coverage by +0.21% 🎉

Coverage data is based on head (4344141) compared to base (a18919b).
Patch coverage: 79.26% of modified lines in pull request are covered.

❗ Current head 4344141 differs from pull request most recent head d7f1428. Consider uploading reports for the commit d7f1428 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1256      +/-   ##
==========================================
+ Coverage   74.22%   74.43%   +0.21%     
==========================================
  Files         114      119       +5     
  Lines       13007    13581     +574     
==========================================
+ Hits         9654    10109     +455     
- Misses       3353     3472     +119     
Impacted Files Coverage Δ
crates/matrix-sdk-redis/src/lib.rs 0.00% <0.00%> (ø)
crates/matrix-sdk-redis/src/real_redis.rs 0.00% <0.00%> (ø)
crates/matrix-sdk-redis/src/redis_shim.rs 0.00% <0.00%> (ø)
crates/matrix-sdk/src/client/builder.rs 71.20% <ø> (ø)
crates/matrix-sdk-redis/src/fake_redis.rs 84.29% <84.29%> (ø)
crates/matrix-sdk-redis/src/redis_crypto_store.rs 90.51% <90.51%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@gnunicorn gnunicorn requested a review from a team December 7, 2022 11:29
@andybalaam
Copy link
Contributor Author

I think this is ready for review when people have time.

Copy link
Collaborator

@jplatte jplatte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this at the end of last week and we're not sure how to best handle maintenance of this crate. Can we sync up on that on Matrix? In any case I've left some comments :)

crates/matrix-sdk-redis/src/lib.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-redis/src/real_redis.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-redis/src/real_redis.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-redis/src/redis_crypto_store.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-redis/src/redis_crypto_store.rs Outdated Show resolved Hide resolved
let mut connection = self.client.get_async_connection().await.unwrap();
let redis_key =
format!("{}secret_requests_by_info|{}", self.key_prefix, key_info.redis_key());
let id: Option<String> = connection.get(&redis_key).await.unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unwrap()?

Comment on lines +800 to +803
let mut connection = self.client.get_async_connection().await.unwrap();
let redis_key = format!("{}unsent_secret_requests", self.key_prefix);
let req_map: HashMap<String, Vec<u8>> = connection.hgetall(&redis_key).await.unwrap();
Ok(req_map.values().map(|req| self.deserialize_value(req).unwrap()).collect())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three unwrap()s here as well.

crates/matrix-sdk-redis/src/redis_crypto_store.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-redis/src/redis_crypto_store.rs Outdated Show resolved Hide resolved
Comment on lines +53 to +57
type Conn: RedisConnectionShim;

async fn get_async_connection(&self) -> RedisResultShim<Self::Conn>;
fn get_connection_info(&self) -> &redis::ConnectionInfo;
fn create_pipe(&self) -> Box<dyn RedisPipelineShim<Conn = Self::Conn>>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like type Conn, the pipeline type can probably be an associated type instead of being boxed and dynamically dispatched, right?

@andybalaam
Copy link
Contributor Author

We discussed this at the end of last week and we're not sure how to best handle maintenance of this crate. Can we sync up on that on Matrix? In any case I've left some comments :)

Over Matrix we agreed that this might turn up in a different repo, or in a "labs" section of this repo.

Both are fine by me - I didn't get anyone's approval before working on this and appreciate it could be hard work to maintain. Thanks for the review Jonas - I will work on the non-suggestion changes when I get a chance.

@jplatte
Copy link
Collaborator

jplatte commented Jan 31, 2023

I'm going to close this since it has a number of maintenance downsides to have this crate in the same repo as the SDK itself:

  • More CI usage for every PR (given we want all things in the repo to be well-tested),
  • we don't have the bandwidth to continually update this crate when things change in the cryptostore abstraction,
  • and issues and PRs here being about something we don't really want to maintain in the team could also be problematic.

That said, I am happy to be pinged about Ruma- and Rust SDK-related maintenance tasks in matrix-sdk-redis once it has its own repo.

@jplatte jplatte closed this Jan 31, 2023
@andybalaam
Copy link
Contributor Author

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.

2 participants