-
-
Notifications
You must be signed in to change notification settings - Fork 18
Console over websocket #124
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
Conversation
itzg
left a comment
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.
Sorry for the delay reviewing this. Overall, I really like this, especially the new "multi" abstractions.
|
Actually sending stderr works now (tested). I think I should remove the prefix option since its useless with the json output. |
|
@itzg I'd be interested to hear your take on the potential features bit. Especially which ones are kinda doable and which ones are less so. |
It seems like the ones you noted, available commands, auto completions, and various server stats (cpu, ram), are not particularly doable without deeper hooks into with a plugin/mod at which point that plugin/mod should be the one exposing a websocket interface. My expectations for this websocket support are covered since it can do the equivalent of rcon's sending commands and returning the response. The prior and current logging retrieval was even a bonus/optional capability in my mind. Reporting server up/down status does seem doable and helpful via a call to mc-monitor. |
|
I think authentication might still be a good idea before its merged. It could just be a header or query param or something like that, I think, and then just taken from the rcon password. |
|
Ah, great point about authentication, but yes keeping it simple like leveraging the rcon password as a bearer token, etc seems good. |
websocket_shell_service.go
Outdated
| password := r.Header.Get("X-WS-Auth") | ||
| if password != getWebsocketPassword() { | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusUnauthorized) | ||
|
|
||
| errMsg := AuthErrorMessage{ | ||
| Type: MessageTypeAuthError, | ||
| Reason: "invalid password", | ||
| } | ||
| json.NewEncoder(w).Encode(errMsg) | ||
| return | ||
| } |
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 would like to be able to skip this with WEBSOCKET_DISABLE_AUTHENTICATION=true but I would have to coerce "true" "TRUE" and maybe "1" to bools and idk you might have a fancy way of doing that.
Otherwise this works. lmk if you don't like the header name or whatnot
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'd recommend adding it to Args like you did the other new field:
but adding in a WithEnv(""), documented here, in the flags parse call at
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 didn't have to use WithEnv(). That created env vars for every setting and I'm not sure that's what you wanted. lmk!
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 forgot that I have a selective way to enable env vars for flags
|
(I can only reference changes in comments) |
Yes but even better is to hook into the context that gets cancelled there. Only quirk is that would mean moving the context creation here further up and passing that into the websocket server object setup. I'm likely over simplifying the suggestion so I don't mind pushing a change to show what I mean. |
|
Feel free to push your suggestion. Its quite late here, so no rush, I probably wont write any more code today 🫡 |
|
(ugh, the UI switched focus on me just before pressing enter) |
|
I thought more about a edit: i guess needlessness isn't really an argument for why not to do it 🙃 |
|
Now you've thrown a fun and interesting twist into the discussion: rcon over websocket. Maybe that turns out to be the distinct, portable, and just general enough aspect to focus. The downside is, that means the docker attach/logs endpoint is probably the better one to shift over for general log consumption. |
|
...with that said, that Rcon approach can be iterated later in a separate PR. Don't feel compelled to have this one PR be the end all, last chance 😄 |
Maybe not RCON over WebSocket but rather RCON over HTTP. Like |
No abstraction can be worst than RCON itself 😅, also I don't think it would be a "needless abstraction" |
|
I would suggest, to not get to much out of scope to implement the sending and receiving using only the WebSocket for now with a subpath ( |
apparently failure is more descriptive that its the client's fault and not an error in the auth process
|
@EmilyxFox @itzg I cooked something in the meantime: https://github.com/MinecraftPlayground/server-management-mod/wiki Maybe you can get some inspiration from that. |
|
I'm sorry I basically forgot about this PR. @Mqxx what endpoint are those operations sending to? The ones being added in this PR? |
I used my own:
I also linked the documentation to the wiki. Basically the Mod hosts its own HTTP/WS server and you can send/receive to/from it. |
|
Sorry, I remember now that yours is a mod based solution. @EmilyxFox let me know if/how you want to proceed. |
No problem. I figured a mod would be more easy for my use-case. But I also developed a Unix-Socket wrapper for TypeScript: https://jsr.io/@typescriptplayground/socket But since all of your projects are developed using go or shell, this is probably not that useful for you 😅 |
@itzg Probably just a final evaluation and merge if you're happy with it 😁 |
itzg
left a comment
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.
Sorry for losing this in the pile. Continues to look great.
|
This is now released in https://github.com/itzg/mc-server-runner/releases/tag/1.14.0 |
What
I'm trying to add a websocket console similar to the ssh console that's already there.
This was also requested in itzg/docker-minecraft-server#3152.
Things that currently work:
Things that don't work
sending the last x amount of lines to newly connected clientswebsocket isn't gracefully closed by the server before the program exitscloses with 1006Potential features
itzg/docker-minecraft-server#3152 mentions being able to request things like available commands, auto completions, and various server stats (cpu, ram). I don't know the feasibility of this at all but I thought I'd include it here anyways just to track.
I think being able to query server status (starting, running, stopping) would be cool, or having it sent in a heartbeat type thing.
Why
Feature could be useful for making web panel consoles which I think would be cool as a sidecar.
I think it would also be a good idea to have two formats (json & text) to either use json formatted messages with extra stuff in, or just send plain stdout/err and stdin through the websocket.
I haven't tested a lot of edge cases at all since this code is an absolute mess. I did test the really long line bug though, and it seems to handle that fine. I make no promises that I'll finish this, but I've put it here to gather some feedback :)