[keyserver] implement socket and update code to interact with Tunnelbroker API
ClosedPublic
Actions

Authored by kamil on Oct 27 2023, 9:58 AM.

Details

Reviewers

michal
bartek
varun

Commits

rCOMMa639d04ed525: [keyserver] implement socket and update code to interact with Tunnelbroker API

Summary

Updates old code to new API
Implement Socket
Remove old types

There is some code duplication with what's in lib/. I'll try to refactor this later.

Looks like RefreshKeyRequest is defined not properly in Rust codebase, should not be a separate message but part of MessageToDevice. I'll fix that as followup - shouldn't affect this code.

Depends on D9596

Test Plan

Check if the keyserver can connect to Tunnelbroker
Check if the socket is properly closed when eg. Tunnelbroker is killed
Check if socket issues don't cause the keyserver to crash
Send RefreshKeyRequest and verify on keyserver and Identity logs that it is properly processed
Kill keyserver, send RefreshKeyRequest, launch keyserver and check if the message was delivered and processed.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

kamil created this revision.Oct 27 2023, 9:58 AM

kamil held this revision as a draft.

Herald added subscribers: will, tomek, ashoat. · View Herald TranscriptOct 27 2023, 9:58 AM

kamil published this revision for review.Oct 27 2023, 10:09 AM

kamil added inline comments.

keyserver/src/socket/tunnelbroker-socket.js
63 ↗	(On Diff #32468)	leftover from testing, I'll remove this
95 ↗	(On Diff #32468)	leftover from testing, I'll remove this

Harbormaster completed remote builds in B23543: Diff 32468.Oct 27 2023, 10:17 AM

Should we implement some kind of timeout for the promises?

So if we get a SerializationError or a bad request we don't just keep them forever and instead reject them after some time?

keyserver/src/socket/tunnelbroker-socket.js
61 ↗	(On Diff #32468)	What happens if this would fail? Should we add a `try` block?
66 ↗	(On Diff #32468)	Nit, also it might be good to log some more info about this message.
75–84 ↗	(On Diff #32468)	I think it might be worth to track the initialization state more precisely than just a connected boolean, but an enum with e.g. `waiting_for_initialization_response`. Then we could error here if we get an initialization message outside of the first message.

Do we need to handle socket reconnection?

keyserver/src/socket/tunnelbroker.js
46 ↗	(On Diff #32468)	How does this work? Doesn't this object get garbage collected?

address review

In D9623#282088, @michal wrote:

Do we need to handle socket reconnection?

ENG-4924

In D9623#282084, @michal wrote:

Should we implement some kind of timeout for the promises?

ENG-5601

(sorry for not linking those tasks in summary when creating diff)

keyserver/src/socket/tunnelbroker-socket.js
61 ↗	(On Diff #32468)	yes we should, thanks
75–84 ↗	(On Diff #32468)	I don't think there is a reason for it, we can error even with simple boolean, I want to avoid adding too much complexity, what do you think? We can add tracking state more precisely when there will be a serious reason to do it
keyserver/src/socket/tunnelbroker.js
46 ↗	(On Diff #32468)	I think this object could live as long as being "reachable". "Reachable" has multiple meanings, but in this case, this object creates a WebSocket connection, socket is event-driven so it should be reachable from an event loop, managed by Node.js. So this object could be removed form memory after closing the socket - but then auto-reconnection should start so it's probably okay, but I'll do some research about this to make sure it's okay.

Harbormaster completed remote builds in B23623: Diff 32566.Oct 31 2023, 11:28 AM

michal added inline comments.Nov 2 2023, 7:07 AM

keyserver/src/socket/tunnelbroker-socket.js
75–84 ↗	(On Diff #32468)	Can you add a check for an init message when `this.connected == true`?

add log for multiple ConnectionInitializationResponse

Harbormaster completed remote builds in B23676: Diff 32630.Nov 2 2023, 10:26 AM

michal accepted this revision.Nov 3 2023, 4:15 AM

michal added inline comments.

keyserver/src/socket/tunnelbroker-socket.js
117–122	Last small thing: could you add a console log if we get a `MESSAGE_TO_DEVICE_REQUEST_STATUS` for a message that we don't have in the `this.promises` object?