[identity] remove OTKs in chronological order to make sure there are never more than 100 keys per olm account
ClosedPublic
Actions

Authored by varun on Apr 18 2024, 4:51 PM.

Details

Reviewers

bartek

Commits

Summary

this diff is pretty big. high level summary:

reduce the number of olm keys per account per upload to 24. We might need to delete as many keys as we are uploading, so 24 keys * 2 accounts * 2 operations (put + delete) = 96. we also need to do one update call per upload to update the otk count for each olm account. that's 97 total operations, which is under the 100 operation limit per transaction.
add a new helper to create the delete operations
update helper responsible for creating update operation. now responsible for update and delete operations.
integration tests to make sure that excess keys are removed correctly and that the upload size limit is also enforced

Test Plan

new integration tests, some manual testing

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

services/identity/src/database/one_time_keys.rs
162 ↗	(On Diff #39264)	might be worth adding a comment here explaining that DDB expects `limit` to be >=1, so we shouldn't call it with 0
249 ↗	(On Diff #39264)	if `num_content_keys_to_append + current_content_otk_count < MAX_ONE_TIME_KEYS`, we shouldn't delete any keys
services/identity/src/ddb_utils.rs
123 ↗	(On Diff #39264)	this should never overflow because we only call this function when adding new OTKs, and we should never delete more keys than we're adding.

varun requested review of this revision.Apr 18 2024, 5:07 PM

Nice!

services/commtest/tests/identity_one_time_key_tests.rs
86–87 ↗	(On Diff #39264)	This is `get(5)` because later you upload 5 more keys, so keys 1..4 get dropped and these (5th) become the first keys to be retrieved in the second `get_keyserver_keys` call, right?
services/identity/src/ddb_utils.rs
162 ↗	(On Diff #39264)	I'd see this as part of `impl OTKRow` impl OTKRow { pub fn as_delete_request(&self) -> TransactWriteItem { ... } // or consuming self version pub fn into_delete_request(self) -> TransactWriteItem { ... } } The reasoning (besides syntax preferences 😜 ) is that we have many functions called `into_something()` and it's easy to get lost among them