search

progrium / qmux

wire protocol for multiplexing connections or streams into a single connection, based on a subset of the SSH Connection Protocol
MIT License
230 stars 48 forks source link

golang/session: initial implementation and basic test #2

Closed progrium closed 3 years ago

progrium commented 3 years ago

This is the meat of the implementation. Although I'd like better test coverage and comments, I'm particularly concerned about the use of contexts. I don't believe they are used where given or appropriate where given so I'd like to think about them more.

Also, I don't often centralize errors into predefined variables, but it might be a good idea to here?

Double check min-max limits ... there are packet sizes and data packet sizes and I'm not sure they're correctly used?

After this there is simply a transport subpackage with various transport protocol dialers and such.

If synchronous review works better, schedule a time with me!

progrium commented 3 years ago

Also is auth going to be part of qmux at all, or are we assuming a pre-authed io stream?

Auth is out of scope. Either it's handled in the underlying transport (WebSocket, for example), preceding the session (TLS client certs maybe), or in a protocol above this (maybe you're muxing HTTP, or maybe you have an RPC auth mechanism).

One other scenario that's worth keeping in mind is somekind of graceful reload support when possible, without losing state/connections. That's one thing that's particularly difficult with ssh.

That's hard in general right? What are some examples?

shazow commented 3 years ago

That's hard in general right? What are some examples?

Yea. There's lots of implementations for this in HTTP, but HTTP is fairly stateless so it's less hard. Here's a fairly general TCP implementation: https://github.com/facebookarchive/grace/blob/75cf19382434/gracenet/net.go#L206

This is probably another transport-level thing but it helps when the protocol/API doesn't get in the way.

progrium commented 3 years ago

@mgood @shazow besides removing stored contexts, and adding context to Open, I've also removed the Listener interface (for now) and dropped the LocalAddr/RemoteAddr methods from Session, since if Session is on a net.Conn, you should be able to type assert as net.Conn to get those. They don't make sense if over stdio for example.

shazow commented 3 years ago

That does look better.

progrium commented 3 years ago

I'll take that as a pass for now!