Console over websocket #124
Conversation
itzg
left a comment
itzg
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
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 🫡 |
You don't. And you can't. Thats my point. But you can give a specific "feedback" for each command send to the server stdin. You can not do that over WebSocket, at least not without an proper protocol like JsonRPC 2.0 |
I guess "feedback" is confusing me. What do you imagine this feedback to be? Just a 2xx response code, or? |
For example, yes. And with that you can build on top of that. For example posting to But if you cram all of that in just this one WebSocket connection, you quickly would hit limitations. Just to make it clear: You can absolutely build it that way and say: "Yes I only want to be able to stream the stdout, and send commands to the server". And that's completely fine. But If you want to be more open (implementation wise) and expand the functionality in the future, I would not build it that way... |
|
For example Discord API works the same. To send messages to a channel you use the normal HTTP POST API and to receive live message feeds from the API you can subscribe to the WebSocket API for a channel. |
|
I think your ideas are good, but I still think having a fully websocket console is the best solution for that part of it. Maybe the websocket console should be put behind a path (eg. /console), to leave space for other endpoints. Then there could be another endpoint (/console/send) that you could use to send stdin lines directly. |
That's at least what I would do 😅 but yea |
|
All of my opinions are just suggestions, because I have build systems and concepts like this in the past 😅 |
|
But I am not familiar with go-lang that's why I am not telling you how to do it and instead offering solutions how you could build it and make it (in my opinion) better 😊 |
These are all functional considerations so the language doesn't matter :) I do like the idea of making room for more features in the future, but I am worried about scope creep, and whether those features are even possible. I'm not even sure how many other endpoints are possible. Different functions/endpoints might also need to be handled by different parts of the whole docker-minecraft-server stack, and that introduces quite a bit of complexity. |
|
I agree with @EmilyxFox 's concerns. Your feature scope @Mqxx seems to be expanding and changing quite a bit. As with all open source, you're welcome to keep using your mod implementation since that seems to do what you want more precisely. I do like the "/console" subpath just in case there are other things and self-documents the intent. I also caution against reinventing a few APIs that are already present in the ecosytem:
|
|
(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
4 participants
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 :)