Skip to content
This repository has been archived by the owner on Dec 20, 2023. It is now read-only.

Ns 2 http websocket #6

Merged
merged 29 commits into from
Aug 14, 2023
Merged

Ns 2 http websocket #6

merged 29 commits into from
Aug 14, 2023

Conversation

yuroitaki
Copy link
Member

@yuroitaki yuroitaki commented Aug 4, 2023

#3.

src/axum_websocket.rs Outdated Show resolved Hide resolved
src/axum_websocket.rs Outdated Show resolved Hide resolved
Copy link
Member

@sinui0 sinui0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! 🚀

Although, I'm not sure I'm convinced about the "upgrade" approach for the TCP case. It seems that it introduces unnecessary complexity and I'm not sure how clients are going to behave with it.

I'm now wondering if it would be better to split these services up to different ports. Having to establish a new connection isn't too big of a deal, given we now know that axum/hyper is not very good at supporting what we need. We can put the websocket endpoint on something like ws.tlsnotary.org.

What do you think about the following:

  1. New route called /new_session which is used for configuration negotation and issues the prover a session ID as it does now.
  2. TCP clients get to reuse the connection, but now hit /notarize to intiiate notarization sending an HTTP Close. If the session id is good, we set a flag indicating the connection info in the Notary global state. We use the without_shutdown provided by axum and when a connection closes we check if the flag is set and act accordingly.
  3. Websocket clients hit a different port which we run a separate service on which only provides the websocket handler you've written.

This shouldn't be that much of a backtrack, lmk what you think.

src/config/config.yaml Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/domain/notary.rs Outdated Show resolved Hide resolved
src/server.rs Outdated Show resolved Hide resolved
src/server.rs Outdated Show resolved Hide resolved
src/server.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/service/tcp.rs Show resolved Hide resolved
@sinui0
Copy link
Member

sinui0 commented Aug 9, 2023

Mulling this over further, I'm not completely against the "upgrade" approach for TCP clients. However, I do think we should decouple the configuration negotiation/session id issuance from the route for starting notarization.

What do you think about combining the websocket and tcp extractors into 1?

@yuroitaki
Copy link
Member Author

yuroitaki commented Aug 9, 2023

Mulling this over further, I'm not completely against the "upgrade" approach for TCP clients. However, I do think we should decouple the configuration negotiation/session id issuance from the route for starting notarization.

What do you think about combining the websocket and tcp extractors into 1?

Agreed on separating configuration from notarization — it was coupled together because originally I didn't know Axum can reuse the same TCP connection for consecutive HTTP requests — separating will make configuration endpoint much simpler and straightforward to understand and interact with.

For combining websocket and tcp extractor into one, if I understand correctly, it means we only have one handler (hence one endpoint with either https:// or wss://) for both websocket and tcp notarization — I can definitely see more pros than cons here where

(1) from prover developer perspective, they only have one notarization endpoint to deal with regardless of client type and the request format is very similar where it only differs in the value of Upgrade header (and the other necessary headers for websocket), and using wss:// versus using https:// basically.

(2) from notary developer perspective, the pro is that the logic of retrieving session_id and fetching configuration data from the store is centralised in the single handler now. Though one con is that we might need to modify more of Axum's websocket code to achieve this — but I think it can be done minimally by using a 'wrapper-extractor' that contains Axum's websocket extractor (hence not modifying any of its internal code) and our custom tcp extractor — need to experiment on this more to tell.

Please let me know if I understand your statement of combining the extractors correctly, and also if you have any other reasons that I did not think of when you suggested this :)

@sinui0
Copy link
Member

sinui0 commented Aug 10, 2023

Yes, that is essentially what I have in mind for composing the extractors. You would first strip the session ID off, then you would check which protocol is present in the Upgrade header and pass it to the appropriate extractor (TCP or WS)

src/domain/notary.rs Outdated Show resolved Hide resolved
@yuroitaki yuroitaki requested a review from sinui0 August 11, 2023 11:17
Copy link
Member

@sinui0 sinui0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work!

@yuroitaki yuroitaki merged commit 3fcd517 into main Aug 14, 2023
3 checks passed
@sinui0 sinui0 deleted the NS-2_http_websocket branch August 14, 2023 17:56
yuroitaki added a commit that referenced this pull request Aug 30, 2023
* Add logic to promote to http and then downgrade to tcp for notarization.

* Fix client hang issue

* Change channel message type.

* Fix response parsing from notary.

* Fix websocket implementation and use upgrade protocol for raw tcp.

* Modify test to mimick browser extension for websocket test.

* Refactor tcp client handling.

* Add global store for persistent data.

* Finish websocket handler and test.

* Add comments.

* Add more comments and documentation.

* Add openapi.yaml.

* Modify README.

* Add architecture explanation.

* Modify README.

* Fix PR based on comments.

* Combine tcp and websocket extractors.

* Refactor and fix documentations.
yuroitaki added a commit that referenced this pull request Aug 30, 2023
* Add logic to promote to http and then downgrade to tcp for notarization.

* Fix client hang issueno

* Change channel message type.

* Fix response parsing from notary.

* Fix websocket implementation and use upgrade protocol for raw tcp.

* Modify test to mimick browser extension for websocket test.

* Refactor tcp client handling.

* Add global store for persistent data.

* Finish websocket handler and test.

* Add comments.

* Add more comments and documentation.

* Add openapi.yaml.

* Modify README.

* Add architecture explanation.

* Modify README.

* Fix PR based on comments.

* Combine tcp and websocket extractors.

* Refactor and fix documentations.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants