Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove JSON Transport #7435

Draft
wants to merge 123 commits into
base: dev
Choose a base branch
from
Draft

Remove JSON Transport #7435

wants to merge 123 commits into from

Conversation

sardination
Copy link
Contributor

@sardination sardination commented Oct 11, 2022

NOTE: this PR must be merged in conjunction with https://github.com/whisthq/brave-core/pull/637 and @MauAraujo's upcoming JSON transport removal PR

Ticket(s) Closed

Description
This PR removes the references to and dependencies on JSON transport from the client extension and the mandelbox scripts. Additionally, it assumes a modified version of the mandelbox_assign endpoint that returns the mandelbox ports and secret as well. Finally, certain dev options(kiosk mode, load extension, local client) that are currently being set via JSON transport on dev have been converted to flags on run.sh for the mandelbox. NOTE: I have included a rough update to the json_transport endpoint in the host service to support this, but the appropriate change will need to be made in the updated version of the host service the corresponds to this PR.

The following features that are currently (in dev) set via JSON transport have been updated to be shared after client connection in this PR:

  • geolocation - a geolocation request from the server is transmitted to the client and returns the live location from the client
  • timezone - this is read and transmitted to the server every time a tab is opened. This is not a fully "live" implementation, but is better than just setting it once at the beginning.
  • keyboard repeat rate and initial delay - we only send this once at the socket connection since we don't have a way of monitoring for changes (yet)
  • browser language - whenever the browser language is updated on the client, a language update message is sent to the server and the server's browser language is updated.
  • dark mode - whenever the dark mode setting is updated on the client, an update message is sent to the server and the server's dark mode is updated. This required a change to Whistium, which is why the Dockerfile has a change to the Whistium build file name.

The following features that are currently (in dev) set via JSON transport have been removed because I couldn't figure out how to hot-switch them on the server or just didn't want them anymore:

  • system language (couldn't figure out)
  • user locale (didn't want - browser language seems to do the job perfectly well)

Implementation
JSON Transport Conversion
These are handled via updates to the client extension, server extension, and server teleport extension.

  • Geolocation is handled via an initial message from server to client requesting geolocation and then a subsequent exchange of messages with a unique identifier to return the geolocation value from the client.
  • Timezone, keyboard repeat rate, and keyboard initial delay are sent from client to server on socket connection (and, for timezone, every time a tab is opened).
  • Browser language and dark mode are sent from client to server on socket connection and every time they are updated on the client.

JSON Transport Removal from Client
All references to the json_transport endpoint have been removed and merged with the mandelbox_assign endpoint.

JSON Transport Removal from Mandelboxes
All references to the config.json written by the host service have been removed and all environment variables that are now being set by the extension have been removed. Dev options have been kept temporarily to allow setting via options in when using run.sh but are handled in the host service in the same way that the AES key and Sentry environments since they are set on container start instead of after container start.

Documentation & Tests Added
N/A

Testing Instructions
This can't be properly tested until the corresponding host service/scaling service update PR is made by @MauAraujo. Stay tuned for updates.

PR Checklist

  • Did the PR author fully test this PR end-to-end?
  • Did one PR reviewer fully test this PR end-to-end?
  • Did one PR reviewer conduct a thorough code design review?

@github-actions github-actions bot added 📁 Repo: hybrid This PR/Issue modifies /hybrid code 📁 Repo: mandelboxes This PR/Issue modifies /mandelboxes code 📁 Repo: browser This PR/Issue modifies /browser code labels Oct 11, 2022
@github-actions github-actions bot added 📁 Repo: services This PR/Issue modifies /services code 📁 Repo: backend This PR/Issue modifies /backend code labels Oct 26, 2022
@github-actions
Copy link

github-actions bot commented Oct 26, 2022

Schema is unchanged, no database migration needed.

Carry on!

''

Suriya Kandaswamy and others added 25 commits December 5, 2022 17:57
* Remove JSON transport from host service

* Add mandelbox info request

* Remove JSON transport from scaling service

* Update run.py, updatee mandelboxDieHandler

* Remove transportMap and transportMapLock

* Use path instead of query params

* Add case for localdev

* Add error handling to run script, use strings Cut instead of Trim

* Rewrite httpserver tests

* Remove JSON transport from tests

* Remove config.json from tests

* Remove unused import

* Formatting

* Add kiosk mode, local client and load extension arguments

* Handle empty query params

* Linting

* Fix deploy script, add optional config db setup

* Formatting, move config specific steps after compose

* Remove unused import

* Remove session id from main service

* Remove session id from audio service

* Always wait for mandelbox to be ready

* Finish getMandelboxInfo function in scaling service server

* Set mandelbox status to running

* Change localdev arg defaults

* Change session id field to server-side session id

* Add missing field

* Remove session id field from assign request

* Properly write session id to db
@github-actions github-actions bot added the 📁 Repo: protocol This PR/Issue modifies /protocol code label Dec 5, 2022
@codecov
Copy link

codecov bot commented Dec 5, 2022

Codecov Report

Merging #7435 (b499a1d) into dev (c104c46) will increase coverage by 1.29%.
The diff coverage is n/a.

❗ Current head b499a1d differs from pull request most recent head 6da4ef9. Consider uploading reports for the commit 6da4ef9 to get more accurate results

@@            Coverage Diff             @@
##              dev    #7435      +/-   ##
==========================================
+ Coverage   56.13%   57.43%   +1.29%     
==========================================
  Files         158      106      -52     
  Lines       32519    26947    -5572     
==========================================
- Hits        18255    15476    -2779     
+ Misses      14010    11471    -2539     
+ Partials      254        0     -254     
Flag Coverage Δ
backend-services ?
protocol 57.43% <ø> (-0.12%) ⬇️
Impacted Files Coverage Δ
protocol/whist/fec/wirehair/WirehairCodec.cpp 89.29% <0.00%> (-1.48%) ⬇️
protocol/whist/network/tcp.c 62.19% <0.00%> (-1.35%) ⬇️
protocol/whist/fec/wirehair/WirehairTools.cpp 77.54% <0.00%> (ø)
backend/services/host-service/host-service.go
backend/services/host-service/httpserver.go
...ckend/services/host-service/mandelbox/mandelbox.go
...ervices/host-service/mandelbox/mandelbox_params.go
backend/services/host-service/spinup.go
backend/services/httputils/types.go
backend/services/scaling-service/httpserver.go
... and 46 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 674545d...6da4ef9. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📁 Repo: backend This PR/Issue modifies /backend code 📁 Repo: browser This PR/Issue modifies /browser code 📁 Repo: hybrid This PR/Issue modifies /hybrid code 📁 Repo: mandelboxes This PR/Issue modifies /mandelboxes code 📁 Repo: protocol This PR/Issue modifies /protocol code 📁 Repo: services This PR/Issue modifies /services code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant