-
Notifications
You must be signed in to change notification settings - Fork 189
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
feat: add WebSocket support to the existing outline-ss-server
#225
base: sbruens/udp-split-serving
Are you sure you want to change the base?
Conversation
- type: websocket | ||
address: "[::]:8000" | ||
options: | ||
# TCP over WebSocket | ||
- path: "/tcp" | ||
connection_type: "stream" | ||
# UDP over WebSocket | ||
- path: "/udp" | ||
connection_type: "packet" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think something like this would be cleaner:
- type: websocket | |
address: "[::]:8000" | |
options: | |
# TCP over WebSocket | |
- path: "/tcp" | |
connection_type: "stream" | |
# UDP over WebSocket | |
- path: "/udp" | |
connection_type: "packet" | |
- type: websocket-stream | |
web_server: main_web_server | |
path: "/tcp" | |
- type: websocket-packet | |
web_server: main_web_server | |
path: "/udp" | |
... | |
web_servers: | |
- id: main_web_server | |
listen: | |
- "[::]:8000" |
Also, this gives us a way to configure the web server as needed.
BTW, we can also add a way to configure the metrics to be exposed on one of the configured web_servers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was playing around with this after we brainstormed about this offline. I was going to add another layer here though, so that we leave room to add configuration that applies to all web servers.
web:
servers:
- id: my_web_server
listen: ...
@@ -339,6 +408,30 @@ func RunOutlineServer(filename string, natTimeout time.Duration, serverMetrics * | |||
return server, nil | |||
} | |||
|
|||
// wrappedConn overrides [websocket.Conn]'s remote address handling. | |||
type wrappedConn struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it will be beneficial to get rid of this kind of wrapping and create some sort of ClientConnection struct with ClientAddr and Conn. The handler could take that instead. The way we do wrapping is a pain and potentially remove ReadFrom/WriteTo optimizations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack. If it's ok with you I'd like to do that in a follow-up PR to keep the changes smaller. I've added a TODO here.
e0bcccf
to
21cae17
Compare
07984ed
to
d14ea20
Compare
listenerTypeWebsocketPacket ListenerType = "websocket-packet" | ||
) | ||
|
||
type WebServerConfig struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming these are always HTTP?
Perhaps we should add a comment that this should probably listen on localhost addresses, since it's not safe to expose HTTP publicly
type WebServerConfig struct { | ||
ID string `yaml:"id"` | ||
Listeners []string `yaml:"listen"` | ||
} | ||
|
||
type ListenerConfig struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add comments here?
Also, this needs to be redesigned. Each type needs different parameters, but we can't simply merge them all in one struct. They each need separate definitions.
Address string | ||
Type ListenerType `yaml:"type"` | ||
Address string `yaml:"address,omitempty"` | ||
WebServer string `yaml:"web_server,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found it best to avoid tags, since that ties you to a specific implementation. You can't use this for JSON or other libraries. Better to just name the fields like you want.
In this case, you could use Web_Server
to remove the tag.
return fmt.Errorf("listener type `%s` references unknown web server `%s`", lnConfig.Type, lnConfig.WebServer) | ||
} | ||
mux := webServers[lnConfig.WebServer] | ||
handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is ok for this PR, but it doesn't work with half-closed. We need to migrate to the Gorilla library to support that.
This PR enables Shadowsocks-over-WebSockets functionality in the existing v1 server, allowing clients to connect to Shadowsocks via WebSockets.
This is the counterpart to the Caddy-backed v2 version introduced in #216.