[keyserver] migration to publish prekeys to identity
ClosedPublic
Actions

Authored by kamil on Oct 9 2023, 7:08 AM.

Details

Reviewers

varun
marcin
ashoat

Commits

rCOMM3f2145c39b26: [keyserver] migration to publish prekeys to identity

Summary

Issue: ENG-3944.

Currently, prekeys on the prod keyserver are for sure marked as published, but yet published to Identity. To fix that uploading those via migration.

Keys are already marked as published, so not marking those again to keep the old publishing date, only uploading to the Identity service.

Depends on D9423

Test Plan

Run migration.
Check for migration 46 succeeded.
Check Identity logs.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kamil created this revision.Oct 9 2023, 7:08 AM

kamil held this revision as a draft.

Herald added subscribers: will, tomek. · View Herald TranscriptOct 9 2023, 7:08 AM

Harbormaster completed remote builds in B23103: Diff 31805.Oct 9 2023, 7:33 AM

kamil published this revision for review.Oct 9 2023, 8:20 AM

marcin added inline comments.Oct 10 2023, 7:33 AM

keyserver/src/scripts/publish-prekeys.js
15 ↗	(On Diff #31805)	Even though it is meant to be a one time script can we avoid code duplication? It looks like this function is identical to `publishNewPrekeys` implemented earlier in the stack.

Rather than a script, it would make my life a little easier if we could put this is in keyserver/src/database/migration-config.js – that way it should automatically run when I deploy the keyserver next

keyserver/src/scripts/publish-prekeys.js
41–49 ↗	(On Diff #31805)	Let's make this silently fail, but not in production

This revision is now accepted and ready to land.Oct 10 2023, 7:59 AM

script -> migration

Code changed significantly, I would like to get approval on this one

keyserver/src/database/migration-config.js
576 ↗	(On Diff #31927)	@ashoat are you sure we don't want silent fails also on production?
keyserver/src/scripts/publish-prekeys.js
15 ↗	(On Diff #31805)	addressed

kamil added a child revision: D9450: [keyserver] rename `retrieveAccountKeysSet` -> `retrieveSessionInitializationKeysSet`.Oct 11 2023, 6:09 AM

Harbormaster completed remote builds in B23183: Diff 31927.Oct 11 2023, 6:22 AM

@kamil are you planning to introduce a differential where account keys are published to identity service on database creation?

This revision is now accepted and ready to land.Oct 12 2023, 2:27 AM

In D9427#277565, @marcin wrote:

@kamil are you planning to introduce a differential where account keys are published to identity service on database creation?

There is no need to, after database creation keyserver will login/register to Identity and the keys are published, this diff is a fix only for the current prod keyserver.

rebase

Harbormaster completed remote builds in B23259: Diff 32025.Oct 16 2023, 1:59 AM

Closed by commit rCOMM3f2145c39b26: [keyserver] migration to publish prekeys to identity (authored by kamil). · Explain WhyOct 16 2023, 2:16 AM

This revision was automatically updated to reflect the committed changes.

kamil added a commit: rCOMM3f2145c39b26: [keyserver] migration to publish prekeys to identity.

@ashoat are you sure we don't want silent fails also on production?

I'm worried that if the migration silently fails, we won't be aware of the issue. I'm also worried that we'll increment the migration version, and will have to introduce a "duplicate" migration.

keyserver/src/database/migration-config.js
575	Are you sure this is the right check? I think you're using the Electron check instead of the Node.js check. I think this is supposed to be: process.env.NODE_ENV === 'production'