-
-
Notifications
You must be signed in to change notification settings - Fork 827
Support staged rollout of migration to Rust Crypto #12184
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
/* | ||
Copyright 2024 The Matrix.org Foundation C.I.C. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
import { test, expect } from "../../element-web-test"; | ||
import { logIntoElement } from "./utils"; | ||
|
||
test.describe("Migration of existing logins", () => { | ||
test("Test migration of existing logins when rollout is 100%", async ({ | ||
page, | ||
context, | ||
app, | ||
credentials, | ||
homeserver, | ||
}, workerInfo) => { | ||
test.skip(workerInfo.project.name === "Rust Crypto", "This test only works with Rust crypto."); | ||
await page.goto("/#/login"); | ||
|
||
let featureRustCrypto = false; | ||
let stagedRolloutPercent = 0; | ||
|
||
await context.route(`http://localhost:8080/config.json*`, async (route) => { | ||
const json = {}; | ||
json["features"] = { | ||
feature_rust_crypto: featureRustCrypto, | ||
}; | ||
json["setting_defaults"] = { | ||
"RustCrypto.staged_rollout_percent": stagedRolloutPercent, | ||
}; | ||
await route.fulfill({ json }); | ||
}); | ||
|
||
await logIntoElement(page, homeserver, credentials); | ||
|
||
await app.settings.openUserSettings("Help & About"); | ||
await expect(page.getByText("Crypto version: Olm")).toBeVisible(); | ||
|
||
featureRustCrypto = true; | ||
|
||
await page.reload(); | ||
|
||
await app.settings.openUserSettings("Help & About"); | ||
await expect(page.getByText("Crypto version: Olm")).toBeVisible(); | ||
|
||
stagedRolloutPercent = 100; | ||
|
||
await page.reload(); | ||
|
||
await app.settings.openUserSettings("Help & About"); | ||
await expect(page.getByText("Crypto version: Rust SDK")).toBeVisible(); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,15 +18,15 @@ limitations under the License. | |
*/ | ||
|
||
import { | ||
ICreateClientOpts, | ||
PendingEventOrdering, | ||
RoomNameState, | ||
RoomNameType, | ||
EventTimeline, | ||
EventTimelineSet, | ||
ICreateClientOpts, | ||
IStartClientOpts, | ||
MatrixClient, | ||
MemoryStore, | ||
PendingEventOrdering, | ||
RoomNameState, | ||
RoomNameType, | ||
TokenRefreshFunction, | ||
} from "matrix-js-sdk/src/matrix"; | ||
import * as utils from "matrix-js-sdk/src/utils"; | ||
|
@@ -53,6 +53,7 @@ import PlatformPeg from "./PlatformPeg"; | |
import { formatList } from "./utils/FormattingUtils"; | ||
import SdkConfig from "./SdkConfig"; | ||
import { Features } from "./settings/Settings"; | ||
import { PhasedRolloutFeature } from "./utils/PhasedRolloutFeature"; | ||
|
||
export interface IMatrixClientCreds { | ||
homeserverUrl: string; | ||
|
@@ -302,13 +303,34 @@ class MatrixClientPegClass implements IMatrixClientPeg { | |
throw new Error("createClient must be called first"); | ||
} | ||
|
||
const useRustCrypto = SettingsStore.getValue(Features.RustCrypto); | ||
let useRustCrypto = SettingsStore.getValue(Features.RustCrypto); | ||
|
||
// We want the value that is set in the config.json for that web instance | ||
const defaultUseRustCrypto = SettingsStore.getValueAt(SettingLevel.CONFIG, Features.RustCrypto); | ||
const migrationPercent = SettingsStore.getValueAt(SettingLevel.CONFIG, "RustCrypto.staged_rollout_percent"); | ||
|
||
// If the default config is to use rust crypto, and the user is on legacy crypto, | ||
// we want to check if we should migrate the current user. | ||
if (!useRustCrypto && defaultUseRustCrypto && Number.isInteger(migrationPercent)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wouldn't bother with the check on defaultUseRustCrypto. It's fiddly to implement and I don't think it's hugely helpful. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't it be strange that if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Counterpoint: it's strange that setting staged_rollout_percent does nothing unless you also remember to set feature_rust_crypto. But you make a good point. I have no strong feelings. |
||
// The user is not on rust crypto, but the default stack is now rust; Let's check if we should migrate | ||
// the current user to rust crypto. | ||
try { | ||
const stagedRollout = new PhasedRolloutFeature("RustCrypto.staged_rollout_percent", migrationPercent); | ||
// Device id should not be null at that point, or init crypto will fail anyhow | ||
const deviceId = this.matrixClient.getDeviceId()!; | ||
// we use deviceId rather than userId because we don't particularly want all devices | ||
// of a user to be migrated at the same time. | ||
useRustCrypto = stagedRollout.isFeatureEnabled(deviceId); | ||
} catch (e) { | ||
logger.warn("Failed to create staged rollout feature for rust crypto migration", e); | ||
} | ||
} | ||
|
||
// we want to make sure that the same crypto implementation is used throughout the lifetime of a device, | ||
// so persist the setting at the device layer | ||
// (At some point, we'll allow the user to *enable* the setting via labs, which will migrate their existing | ||
// device to the rust-sdk implementation, but that won't change anything here). | ||
await SettingsStore.setValue("feature_rust_crypto", null, SettingLevel.DEVICE, useRustCrypto); | ||
await SettingsStore.setValue(Features.RustCrypto, null, SettingLevel.DEVICE, useRustCrypto); | ||
|
||
// Now we can initialise the right crypto impl. | ||
if (useRustCrypto) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
/* | ||
Copyright 2024 New Vector Ltd | ||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
http://www.apache.org/licenses/LICENSE-2.0 | ||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
import { xxHash32 } from "js-xxhash"; | ||
|
||
/** | ||
* The PhasedRolloutFeature class is used to manage the phased rollout of a new feature. | ||
* | ||
* It uses a hash of the user's identifier and the feature name to determine if a feature is enabled for a specific user. | ||
* The rollout percentage determines the probability that a user will be enabled for the feature. | ||
* The feature will be enabled for all users if the rollout percentage is 100, and for no users if the percentage is 0. | ||
* If a user is enabled for a feature at x% rollout, it will also be for any greater than x percent. | ||
* | ||
* The process ensures a uniform distribution of enabled features across users. | ||
* | ||
* @property featureName - The name of the feature to be rolled out. | ||
* @property rolloutPercentage - The int percentage (0..100) of users for whom the feature should be enabled. | ||
*/ | ||
export class PhasedRolloutFeature { | ||
public readonly featureName: string; | ||
private readonly rolloutPercentage: number; | ||
private readonly seed: number; | ||
|
||
public constructor(featureName: string, rolloutPercentage: number) { | ||
this.featureName = featureName; | ||
if (!Number.isInteger(rolloutPercentage) || rolloutPercentage < 0 || rolloutPercentage > 100) { | ||
throw new Error("Rollout percentage must be an integer between 0 and 100"); | ||
} | ||
this.rolloutPercentage = rolloutPercentage; | ||
// We add the feature name for the seed to ensure that the hash is different for each feature | ||
this.seed = Array.from(featureName).reduce((sum, char) => sum + char.charCodeAt(0), 0); | ||
} | ||
|
||
/** | ||
* Returns true if the feature should be enabled for the given user. | ||
* @param userIdentifier - Some unique identifier for the user, e.g. their user ID or device ID. | ||
*/ | ||
public isFeatureEnabled(userIdentifier: string): boolean { | ||
/* | ||
* We use a hash function to convert the unique user ID string into an integer. | ||
* This integer can then be used as a basis for deciding whether the user should have access to the new feature. | ||
* We need some hash with good uniform distribution properties, security is not a concern here. | ||
* We use xxHash32, which is fast and has good distribution properties. | ||
*/ | ||
const hash = xxHash32(userIdentifier, this.seed); | ||
// We use the hash modulo 100 to get a number between 0 and 99. | ||
// Modulo is simple and effective and the distribution should be uniform enough for our purposes. | ||
return hash % 100 < this.rolloutPercentage; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be
!==
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to test legacy to rust migration (feature_rust_crypto from
false
totrue
).In the project
Rust Crypto
, the app is started withfeature_rust_crypto: true
But anyhow it doesn't matter much as we anyhow override the
cryptoBackend
by interceptinghttp://localhost:8080/config.json*
, so it would test twice the same thing.