Use MLS member table indexed by group id #2859

smatting · 2022-11-21T18:08:47Z

This PR adds a table galley.mls_group_member_client (indexed by group id) which replaces galley.member_client (index by conv). Motivation: This table may be used by MLS subconversations. This PR also includes a data migration from galley.member_client to galley.mls_group_member_client (tested locally).

Checklist

Add a new entry in an appropriate subdirectory of changelog.d
Read and follow the PR guidelines

cassandra-schema.cql

services/galley/schema/src/Main.hs

services/galley/schema/src/V77_MLSSubconversation.hs

mdimjasevic

Looks good! There are some questions I have though; see them inlined. In particular, if possible it'd be great to avoid conditional execution of various MLS actions if we're in a function that deals with MLS conversations only.

mdimjasevic · 2022-11-24T10:56:52Z

services/galley/src/Galley/API/Action.hs

+      for_ (Data.mlsMetadata (tUnqualified lconv')) $ \meta ->
+        traverse_ (removeUser lconv' (cnvmlsGroupId meta)) victims


You said the SConversationLeaveTag case applies to MLS only, hence this protocol-constrained removal is fine?

No, the removeUser function is from the MLS modules. It only applies to MLS conversations, that's why it's okay to require the MLS metadata as argument

mdimjasevic · 2022-11-24T12:25:19Z

services/galley/src/Galley/API/MLS/Message.hs

+  let curEpoch = cnvmlsEpoch mlsMeta
+      groupId = cnvmlsGroupId mlsMeta
+      suite = cnvmlsCipherSuite mlsMeta


This now allows to drop the Member (ErrorS 'ConvNotFound) r constraint from this function: it is not thrown directly here anymore, nor applyProposalRef needs to throw it because it does have all the MLS metadata needed.

I could remove the ConvNotFound constraint from checkEpoch, but checkGroup still throws it. I refactored both functions to not used the converation object anymore but rather the ConversationMLSData in
8e92789

mdimjasevic · 2022-11-24T12:27:09Z

services/galley/src/Galley/API/MLS/Message.hs

  ProposalAction ->
  Maybe UpdatePath ->
  Sem r ()
-processExternalCommit qusr mSenderClient lconv cm epoch groupId action updatePath = withCommitLock groupId epoch $ do
+processExternalCommit qusr mSenderClient lconv mlsMeta cm epoch action updatePath = withCommitLock (cnvmlsGroupId mlsMeta) epoch $ do


The ConvNotFound error constraint likely isn't needed here either.

Unfortunately because of checkGroup it can't be removed

mdimjasevic · 2022-11-24T12:28:09Z

services/galley/src/Galley/API/MLS/Message.hs

  ProposalAction ->
  Sender 'MLSPlainText ->
  Commit ->
  Sem r [LocalConversationUpdate]
-processCommitWithAction qusr senderClient con lconv cm epoch groupId action sender commit =
+processCommitWithAction qusr senderClient con lconv mlsMeta cm epoch action sender commit =


Can you check if ConvNotFound can be removed from this one too?

Unfortunately it can't because of downstream checkGroup.

mdimjasevic · 2022-11-24T12:30:54Z

services/galley/src/Galley/API/MLS/Removal.hs

@@ -110,9 +110,10 @@ removeClient ::
  ClientId ->
  Sem r ()
 removeClient lc qusr cid = do


Can you see if you can change the signature of the function such that you don't need to do for_ in the function? You know you do this only for MLS clients, so it feels a bit awkward to have to check if this is an MLS conversation.

I think you could pass in ConversationMLSData and as little as needed from Local Data.Conversation instead of passing in the whole conversation.

Downstream of removeClient is propagateMessage and runMessagePush which require the full conversation object. Maybe we could refactor them as well, but I woudn't want to mix it into this PR

Fair point about inlining the for_ part into removeUser itself. Did this is acf5d67bb0fc9e61753e927bb8f7751377a33c26

smatting temporarily deployed to cachix November 21, 2022 18:08 Inactive

zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Nov 21, 2022

smatting commented Nov 21, 2022

View reviewed changes

cassandra-schema.cql Outdated Show resolved Hide resolved

smatting commented Nov 21, 2022

View reviewed changes

services/galley/schema/src/Main.hs Outdated Show resolved Hide resolved

smatting commented Nov 21, 2022

View reviewed changes

services/galley/schema/src/Main.hs Show resolved Hide resolved

mdimjasevic reviewed Nov 22, 2022

View reviewed changes

services/galley/schema/src/V77_MLSSubconversation.hs Outdated Show resolved Hide resolved

smatting force-pushed the fs-901/preparation-member-clients branch from 15704bd to 5f562b9 Compare November 23, 2022 13:04

smatting temporarily deployed to cachix November 23, 2022 13:04 Inactive

smatting temporarily deployed to cachix November 23, 2022 13:05 Inactive

smatting force-pushed the fs-901/preparation-member-clients branch 2 times, most recently from 87a11d6 to 2c1c036 Compare November 23, 2022 17:00

smatting temporarily deployed to cachix November 23, 2022 17:00 Inactive

smatting temporarily deployed to cachix November 23, 2022 17:08 Inactive

smatting marked this pull request as ready for review November 23, 2022 17:09

smatting requested a review from mdimjasevic November 23, 2022 17:09

smatting temporarily deployed to cachix November 24, 2022 09:30 Inactive

smatting temporarily deployed to cachix November 24, 2022 10:40 Inactive

smatting force-pushed the fs-901/preparation-member-clients branch from 7a89f33 to e2f47c8 Compare November 24, 2022 11:08

smatting temporarily deployed to cachix November 24, 2022 11:08 Inactive

smatting temporarily deployed to cachix November 24, 2022 11:09 Inactive

mdimjasevic approved these changes Nov 24, 2022

View reviewed changes

mdimjasevic and others added 4 commits November 24, 2022 18:32

Update schema for MLS subconversations

a05a30d

Add changelog entry

90a97ee

Add data migration

b8be6a6

Add warning logs to migration script

53a91a9

smatting added 2 commits November 24, 2022 18:32

Update changelog

51e4085

apply hlint hints

e87f8d5

smatting force-pushed the fs-901/preparation-member-clients branch from e2f47c8 to 8e92789 Compare November 24, 2022 17:48

smatting temporarily deployed to cachix November 24, 2022 17:48 Inactive

smatting temporarily deployed to cachix November 24, 2022 17:59 Inactive

smatting added 3 commits November 24, 2022 19:02

ormolu

d08423e

user ConversationMLSData instead of Conversation

94ecac0

Factor check for MLS metadata into removeUser

d9b8425

smatting force-pushed the fs-901/preparation-member-clients branch from acf5d67 to d9b8425 Compare November 24, 2022 18:02

smatting temporarily deployed to cachix November 24, 2022 18:02 Inactive

Fix unused imports

4a13be5

smatting temporarily deployed to cachix November 25, 2022 09:51 Inactive

smatting merged commit 54b2cae into develop Nov 25, 2022

smatting deleted the fs-901/preparation-member-clients branch November 25, 2022 11:45

zebot mentioned this pull request Jan 12, 2023

Release 2023-01-12 - (expected chart version 4.30.0) #2977

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use MLS member table indexed by group id #2859

Use MLS member table indexed by group id #2859

smatting commented Nov 21, 2022 •

edited

Loading

mdimjasevic left a comment

mdimjasevic Nov 24, 2022

smatting Nov 24, 2022

mdimjasevic Nov 24, 2022

smatting Nov 24, 2022

mdimjasevic Nov 24, 2022

smatting Nov 24, 2022

mdimjasevic Nov 24, 2022

smatting Nov 24, 2022

mdimjasevic Nov 24, 2022

smatting Nov 24, 2022

smatting Nov 24, 2022 •

edited

Loading

		for_ (Data.mlsMetadata (tUnqualified lconv')) $ \meta ->
		traverse_ (removeUser lconv' (cnvmlsGroupId meta)) victims

Use MLS member table indexed by group id #2859

Use MLS member table indexed by group id #2859

Conversation

smatting commented Nov 21, 2022 • edited Loading

Checklist

mdimjasevic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smatting Nov 24, 2022 • edited Loading

Choose a reason for hiding this comment

smatting commented Nov 21, 2022 •

edited

Loading

smatting Nov 24, 2022 •

edited

Loading