extend on-new-remote-conversation by the subconv ID #2997

stefanwire · 2023-01-17T16:35:12Z

When a subconversation is created, all backends involved in the main conversation need to be notified, so that they create a group ID → subconversation mapping. Otherwise, commits coming from clients on those backends cannot be routed.

Similarly, when a user from a new backend B joins a conversation containing non-empty subconversations, it needs to be informed of those subconversations, so that (external) commits for those subconversations coming from clients in B can be routed.

https://wearezeta.atlassian.net/browse/FS-1451

TODO

Split federation endpoint into on-new-remote-conversation and on-new-remote-subconversation
Call on-new-remote-subconversation when a new subconversation is created
Call on-new-remote-subconversation for all existing subconversations when a new backend gets involved
Call on-new-remote-subconversation when a subconversation is reset
Integration tests
Subconversation End-to-end tests
(optional) Split executeProposalAction into one function for conversations and one for subconversations

Checklist

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

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

elland · 2023-01-18T09:10:14Z

libs/wire-api-federation/src/Wire/API/Federation/API/Galley.hs

@@ -220,6 +220,8 @@ ccRemoteOrigUserId cc = qualifyAs (ccCnvId cc) (ccOrigUserId cc)
 data NewRemoteConversation = NewRemoteConversation
  { -- | The conversation ID, local to the backend invoking the RPC.
    nrcConvId :: ConvId,
+    -- | The subconversation, if set.
+    nrcSubConvId :: Maybe SubConvId,


Not a fan of encoding things with such ambiguity. It's a bit unintuitive that this maps to a sub-conversation if this value is present. Could we change this into a sum type instead?

It is a sum type (Maybe). I'm not sure where the ambiguity is. We are using the same representation for events.

The way we handled a need for a change like this before is to use the ConvOrSubConvId type instead of ConvId, and that type can carry either of the conv ID or the full subconv ID (maybe modulo the domain). This would be a change that affects the federation API beyond MLS, so I'd recommend a separate PR to develop just to change these types/API and then rebase the PR to mls on top of it.

I'm still not sure what we are looking to achieve. What is the problem with the current representation?

There's nothing wrong. It will get the job done. It's just not consistent with how it was done before, but maybe we shouldn't worry about it.

Another note: the way I did such a change before is to split the request type into two: one for Proteus (which doesn't feature subconversations) and one for MLS. It could be done here too.

It would be the first commit in develop and the second in mls. This would require SubConversationStore in develop which doesn't exist there yet.

It would be the first commit in develop and the second in mls. This would require SubConversationStore in develop which doesn't exist there yet.

Unfortunately, the links lead to nowhere now (I suppose due to a rebase). The split of the message sending types that I was referring to was in PR #2925. There I didn't need the SubConversationStore effect, but your mileage might vary.

smatting · 2023-01-31T17:23:34Z

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

+    pure (scMLSData sconv)
+
+  -- notify all backends that the subconversation has a new ID
+  let remotes = Set.fromList (map (void . rmId) (convRemoteMembers cnv))


nit-pick: in other places we've used bucketRemote instead of Set.fromList

smatting · 2023-01-31T17:24:14Z

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

+  -- TODO: only do this for new subconversations (i.e. Epoch 1)
+  -- call `on-new-remote-conversation` on all the remote backends involved in
+  -- the main conversation


still left to do?

services/galley/test/integration/API/MLS.hs

Co-authored-by: Stefan Matting <smatting@users.noreply.github.com>

stefanwire temporarily deployed to cachix January 17, 2023 16:35 — with GitHub Actions Inactive

stefanwire changed the base branch from develop to mls January 17, 2023 16:35

zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Jan 17, 2023

stefanwire marked this pull request as ready for review January 17, 2023 16:37

elland reviewed Jan 18, 2023

View reviewed changes

stefanwire force-pushed the sb/on-new-remote-conversation branch from a41c4cd to 7ba795d Compare January 18, 2023 10:01

stefanwire temporarily deployed to cachix January 18, 2023 10:01 — with GitHub Actions Inactive

pcapriotti marked this pull request as draft January 18, 2023 10:02

stefanwire force-pushed the sb/on-new-remote-conversation branch from 7ba795d to e581d6d Compare January 18, 2023 10:12

stefanwire temporarily deployed to cachix January 18, 2023 10:12 — with GitHub Actions Inactive

stefanwire force-pushed the sb/on-new-remote-conversation branch from e581d6d to e62b2a4 Compare January 18, 2023 14:00

stefanwire temporarily deployed to cachix January 18, 2023 14:00 — with GitHub Actions Inactive

stefanwire force-pushed the sb/on-new-remote-conversation branch from e62b2a4 to ba0b055 Compare January 18, 2023 14:10

stefanwire temporarily deployed to cachix January 18, 2023 14:10 — with GitHub Actions Inactive

stefanwire force-pushed the sb/on-new-remote-conversation branch from ba0b055 to 1f257a5 Compare January 19, 2023 16:42

stefanwire temporarily deployed to cachix January 19, 2023 16:42 — with GitHub Actions Inactive

stefanwire force-pushed the sb/on-new-remote-conversation branch from 1f257a5 to ddf3406 Compare January 20, 2023 10:31

stefanwire temporarily deployed to cachix January 20, 2023 10:31 — with GitHub Actions Inactive

pcapriotti temporarily deployed to cachix January 20, 2023 10:59 — with GitHub Actions Inactive

pcapriotti temporarily deployed to cachix January 20, 2023 14:32 — with GitHub Actions Inactive

stefanwire temporarily deployed to cachix January 20, 2023 14:41 — with GitHub Actions Inactive

stefanwire and others added 16 commits January 31, 2023 15:11

add end2end test for messages in a subconversation

2983e30

Notify new backends about existing subconvs

e18d32b

Add client field to federated MLS messages

6f0d66d

Use sender client for federated messages

581988b

Test remote user joining a local subconversation

a11d6a6

fix deserialisation of subgroups in e2e tests

6a6a79d

add client header to requests in e2e tests

b371129

fix comments in e2e test

1636112

abstract sendCommitBundle in e2e tests

c583532

Test joining remote subconversation

55dbfd7

Test onrc when a new backend joins a conversation

8328f15

Call onrc when a subconversation is reset

0aa072d

Test onrc call when resetting a subconversation

cbae292

Split on-new-remote-conversation into 2 endpoints

2c2c866

lint: remove redundant brackets

048bf48

Fix assertions of RPCs

4701626

stefanwire force-pushed the sb/on-new-remote-conversation branch from a1c7fc1 to 4701626 Compare January 31, 2023 15:12

stefanwire marked this pull request as ready for review January 31, 2023 15:54

smatting assigned smatting and unassigned smatting Jan 31, 2023

smatting self-requested a review January 31, 2023 16:38

add changelog entry

68c79d9

smatting approved these changes Jan 31, 2023

View reviewed changes

stefanwire and others added 3 commits February 1, 2023 09:41

Update services/galley/test/integration/API/MLS.hs

785edc1

Co-authored-by: Stefan Matting <smatting@users.noreply.github.com>

Do not call onrsc unless the subconv is new

b1f4fb2

Use bucketRemote

2f0e92d

pcapriotti merged commit dc5aeaa into mls Feb 1, 2023

pcapriotti deleted the sb/on-new-remote-conversation branch February 1, 2023 10:35

This was referenced Oct 19, 2023

Release 2023-10-19 - (expected chart version 4.39.0) #3661

Closed

Release 2023-10-23 - (expected chart version 4.39.0) #3668

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

extend on-new-remote-conversation by the subconv ID #2997

extend on-new-remote-conversation by the subconv ID #2997

stefanwire commented Jan 17, 2023 •

edited

Loading

elland Jan 18, 2023

pcapriotti Jan 18, 2023

mdimjasevic Jan 18, 2023 •

edited

Loading

pcapriotti Jan 18, 2023

mdimjasevic Jan 18, 2023

stefanwire Jan 18, 2023

mdimjasevic Jan 20, 2023 •

edited

Loading

smatting Jan 31, 2023

smatting Jan 31, 2023

extend on-new-remote-conversation by the subconv ID #2997

extend on-new-remote-conversation by the subconv ID #2997

Conversation

stefanwire commented Jan 17, 2023 • edited Loading

TODO

Checklist

elland Jan 18, 2023

Choose a reason for hiding this comment

pcapriotti Jan 18, 2023

Choose a reason for hiding this comment

mdimjasevic Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

pcapriotti Jan 18, 2023

Choose a reason for hiding this comment

mdimjasevic Jan 18, 2023

Choose a reason for hiding this comment

stefanwire Jan 18, 2023

Choose a reason for hiding this comment

mdimjasevic Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

smatting Jan 31, 2023

Choose a reason for hiding this comment

smatting Jan 31, 2023

Choose a reason for hiding this comment

stefanwire commented Jan 17, 2023 •

edited

Loading

mdimjasevic Jan 18, 2023 •

edited

Loading

mdimjasevic Jan 20, 2023 •

edited

Loading