Page MenuHomePhabricator

[Tunnelbroker] Route refreshkeys request to websocket connection
ClosedPublic

Authored by jon on May 1 2023, 7:14 AM.
Tags
None
Referenced Files
F1268937: D7693.id.diff
Sun, Mar 3, 11:22 AM
Unknown Object (File)
Sat, Feb 10, 3:32 PM
Unknown Object (File)
Sat, Feb 10, 8:23 AM
Unknown Object (File)
Sat, Feb 10, 8:23 AM
Unknown Object (File)
Sat, Feb 10, 8:22 AM
Unknown Object (File)
Sat, Feb 10, 8:22 AM
Unknown Object (File)
Sat, Feb 10, 8:22 AM
Unknown Object (File)
Sat, Feb 10, 8:22 AM
Subscribers

Details

Summary

Ensure that messages issued to the gRPC endpoint end up
being routed to the correct websocket connection.

https://linear.app/comm/issue/ENG-3681
https://linear.app/comm/issue/ENG-3745

Test Plan
# Launch tunnelbroker in other window or in the background
(cd services/tunnelbroker && RUST_LOG=debug cargo run &)
#
cd services/commtest && cargo test --test tunnelbroker_integration_test

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.May 1 2023, 7:29 AM
Harbormaster failed remote builds in B18953: Diff 25963!
varun requested changes to this revision.May 4 2023, 3:13 PM
varun added inline comments.
services/commtest/tests/tunnelbroker_integration_test.rs
14 ↗(On Diff #26030)

session

services/tunnelbroker/src/grpc/mod.rs
30 ↗(On Diff #26030)

is unavailable the right code? how about not found instead?

services/tunnelbroker/src/websockets/mod.rs
71–79 ↗(On Diff #26030)

how does this work exactly? a comment might be helpful since the select macro seems to be doing a lot here

81 ↗(On Diff #26030)

handle_incoming_message

97 ↗(On Diff #26030)

how about handle_outgoing_message?

This revision now requires changes to proceed.May 4 2023, 3:13 PM
jon marked 5 inline comments as done.

Address feedback

services/tunnelbroker/src/grpc/mod.rs
30 ↗(On Diff #26030)

Depends if you see it as "is the device available for a message", or "does a connection to this device exist?". I don't feel strongly either way.

services/tunnelbroker/src/websockets/mod.rs
81 ↗(On Diff #26030)

ambiguous if this is incoming from something like identity service or the device. I'll use handle_message_from_device

97 ↗(On Diff #26030)

It's not outgoing, it would be something like identity service asking this specific device to refresh keys. So the origin of the message is external to tunnelbroker or the device.

I'll do handle_message_from_service to keep it consistent.

Minor fixups, rebase on master

This revision is now accepted and ready to land.May 15 2023, 4:24 AM