[keyserver] Tunnelbroker integration - Adding Tunnelbroker publisher and connector
Needs RevisionPublic
Actions

Authored by • max on Mar 1 2023, 6:43 AM.

Details

Reviewers

ashoat
atul

Summary

This diff adds a singleton class for a publisher in the Tunnelbroker client integration into the Tunnelbroker. The publisher class uses connect() method to make a connection to the Tunnelbroker for the publisher. This approach is inspired by our Redis approach with the changes that we are connecting to the Tunnelbroker ahead of time (on server start) instead of on publishing like in the Redis implementation. I think it's a good way to make a connection ahead of time, but this can be changed if reviewers think otherwise.

Linear task: ENG-2613

Test Plan

Start the tunnelbroker instance by running cd services/tunnelbroker && cargo run.
Connect to the tunnelbroker messages stream by the gRPC client with the deviceID A.
Add a simple publish to the keyserver.js:

import { tunnelbrokerPublisher } from './socket/tunnelbroker.js';
...
tunnelbrokerPublisher.publish('A','Some test data');

Start the keyserver using the cd keyserver && yarn dev.
The expected result is that the message is published and delivered to the gRPC client which is connected as a deviceID A.

Diff Detail

Repository

rCOMM Comm

Branch

tb-add-publisher

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• max created this revision.Mar 1 2023, 6:43 AM

• max held this revision as a draft.

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptMar 1 2023, 6:43 AM

• max published this revision for review.Mar 1 2023, 6:55 AM

• max edited the summary of this revision. (Show Details)

• max edited the test plan for this revision. (Show Details)

• max added reviewers: ashoat, atul.

Harbormaster completed remote builds in B17044: Diff 23339.Mar 1 2023, 6:59 AM

ashoat requested changes to this revision.Mar 1 2023, 8:29 PM

ashoat added inline comments.

keyserver/src/keyserver.js
174	Why aren't we `await`ing this? Is this call necessary here if lazily initialize and apply my suggestion on line 27 of `tunnelbroker.js` below?
keyserver/src/socket/tunnelbroker.js
11	Exit early please
17	What does this `return;` statement do?
27	Wouldn't we want to `await` this and skip the return on the next line?

This revision now requires changes to proceed.Mar 1 2023, 8:29 PM

Thought about this a bit more. We need to make sure that we don't accidentally initialize two tbClientBinding, but TunnelbrokerPublisher.connect is an async function, which makes this difficult to guarantee.

@atul had a similar problem recently with initializing Olm, which also is an async function. I think there are two ways forward here:

What @atul did in D6901: force every thread to wait for connect to complete before starting up. This requires an await to make sure the connect completes before anything attempts to call publish (which could trigger a race with a second connect).
What I suggested in this comment on D6897: you could assign the promise to a variable, and then whenever a second connect attempt comes in, see if the promise variable is already set, and if it is just return that.