Skip to content
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

Improve server logs for administration #9105

Closed
wants to merge 3 commits into from
Closed

Conversation

Berbe
Copy link
Contributor

@Berbe Berbe commented Apr 25, 2021

Motivation / Problem

Server administration requires quickly being able to act on entities when needed, for instance behaving wrongly.
In the case of OpenTTD, this is done either with the help of entity's client ID or IP address.

However, in its current state, the game never directy logs a direct relation between an entity's IP address and its client ID, which needs time-based guessing to discover it.
Once done, the client ID can be used to match with the entity's player name, but only with the help of the STR_NETWORK_MESSAGE_CLIENT_JOINED_ID, which is only available on entity's join.

This 2-step process, involving guessing

Description

This proposal enhances the servers logs by adding entity's IP address:

  • on client connection & connection close events in case the entity never fully joins
  • on client join & leave to allow for an easier & quicker linking between entity's name & IP address

On a side job, some constructors have been rewritten to make us of member initialiser rather than in-constructor property definition.

Limitations

net debug facility is busy from levels 1 to 6 (default level for dedicated servers), which makes it difficult to decide which level is the most appropriate for specific events.

Also, level 2 is already quite heavily used already, as queried events are very often triggered on a server.
Would it be worth burying those messages in a higher debug level, freeing space on level 2 for some other messages, hence allowing different level of server administrationg logging while avoiding pollution?

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@TrueBrain
Copy link
Member

Haven't really looked into this yet, but with upcoming network changes, "IP" is becoming a more vague value in the network code, as it could be of a relay (and not of the true client). Without really looking into what this PR is solving (sorry, bit busy atm :D), please keep in mind that showing IP might be a short-lived solution. Will look into this PR in more detail later :)

@TrueBrain
Copy link
Member

TrueBrain commented Apr 25, 2021

Ugh, as I already made the context-switch anyway, I had a better look (could have done it first try :P). Sorry about the double-comment :)

I understand the first commit and the last two commits, but the second feels really odd to me. This not so much logs it, but it shows on the server the IP in chat. That feels like the wrong thing to do to me. Can you elaborate why you went for this solution?

Additionally, isn't it sufficient to see on connect and close-connection what the clientid <-> ip relation was? What is the added value of the 2nd commit, is basically what I am asking :D

The 3th commit is fine, also towards the future. In my network-rewrite PR this already shows other texts depending how the connection was established, so that works fine.

(sorry, had the commits wrong .. I meant Add: IP address in logs on client join/leave)

@Berbe
Copy link
Contributor Author

Berbe commented Apr 25, 2021

IP address is the way machines are talking to each other in the foreseable future, hence if clients are gooing to connecte to a server, there will be those.

If you talking about proxying traffic (very bad idea), the proxy will indeed face doom id any of the entities going through it is misbehaving.

If you are talking about NAT-traversal hacks, such as STUN, those are there because IPv6 is not deemed important by corporations owning large IPv4 blocks, hence blocking de facto its adoption.
STUN, however, only operates IIRC as an initial handshake method; real traffic will be direct between client & server.

@Berbe
Copy link
Contributor Author

Berbe commented Apr 25, 2021

I understand the first commit and the last two commits, but the second feels really odd to me. This not so much logs it, but it shows on the server the IP in chat. That feels like the wrong thing to do to me. Can you elaborate why you went for this solution?

Whoops. Not the intent.
Will fix that obvious & stupid mistake.

Additionally, isn't it sufficient to see on connect and close-connection what the clientid <-> ip relation was? What is the added value of the 2nd commit, is basically what I am asking :D

Player name <-> IP address relation, as the client ID is an internal, behind-the-scene, value, which will only be useful in de-duplication (for instance when several entities share looking-alike names).

@Berbe Berbe marked this pull request as draft April 25, 2021 14:59
@TrueBrain
Copy link
Member

TrueBrain commented Apr 25, 2021

If you talking about proxying traffic (very bad idea), the proxy will indeed face doom id any of the entities going through it is misbehaving.

You are of course free of your opinion, but make no mistake, something like Steam Relay (SDR) is on the table. It would improve the gaming experience for a lot of people by a lot. Besides, the downside of IP-banning for example is that someone who wants to grief, changes IP in seconds. So this needs a more robust solution sooner or later, if we want to take server administration seriously.
Doesn't mean your PR is invalid or anything, it is just something we have to keep in mind. That we don't create more work for ourselves on the short term :)

Whoops. Not the intent.
Will fix that obvious & stupid mistake.

Personally, I would just add some DEBUG() statements in those places. Also saves you from concatenating these strings like this :)

Player name <-> IP address relation, as the client ID is an internal, behind-the-scene, value, which will only be useful in de-duplication (for instance when several entities share looking-alike names).

Tnx! I am slowly collecting what people run into when running a server; the new ingame network GUI already resulted in a ton of requests. I am looking into making this a lot easier to server admins, as the current system is just a bit weird and outdated :) So tnx, this helps!

@TrueBrain
Copy link
Member

Owh, completely forgot your question about log-levels:

The current levels always feel random to me. Mainly, I think, as there is nowhere (that I know of) written down what should be on level 1, level 2, etc. Writing this down, from a server-admin perspective, would already really really help. So if you are up for that, please.

For example, the UDP queries .. that is just weird where they are now. It really is not that interested, that information. You cannot stop it, and unless someone really wants to, doesn't even hurt your server.

So yeah .. some more consideration behind what should go where: I would love it :)

@Berbe
Copy link
Contributor Author

Berbe commented Apr 25, 2021

I was initially reluctant on modifying language files... but there is no choice, other than using the DEBUG macro on the side of the advertised message, which would make code & server output redundant.

@Berbe Berbe marked this pull request as ready for review April 25, 2021 16:30
@TrueBrain
Copy link
Member

TrueBrain commented Apr 25, 2021

What I am aiming at, and what is not addressed by your change, is this:

image

I can fully understand you want this information for your (most likely dedicated) server, but I do not think it is useful to show this graphically in-game to the host. Mostly because if you have a GUI, banning / kicking people can be done via the GUI, without ever really needed to know the IP. So this is "noise" for a server host like this, I would think?

Do you see a good cause to also show this information in the GUI (instead of only in logs)?

@James103
Copy link
Contributor

IP addresses can be considered to be personal information, for which some people may object to having their IP publicly broadcasted to the other clients as he/she joins a server.

@TrueBrain
Copy link
Member

IP addresses can be considered to be personal information, for which some people may object to having their IP publicly broadcasted to the other clients as he/she joins a server.

That is not what is going on here. Only the server sees this.

src/network/network_server.cpp Outdated Show resolved Hide resolved
src/network/network_server.cpp Outdated Show resolved Hide resolved
@TrueBrain
Copy link
Member

Oof, I lost sight of this PR. We did revamp all the DEBUG levels to be more sane, as we talked about in the Discussion. But if I remember correctly there were some parts that this PR added extra info, right?

Mind rebasing and cleaning it up? I think it is worth having :)

@TrueBrain TrueBrain added the work: minor details This Pull Request has some minor details left to do label Jul 2, 2021
@TrueBrain TrueBrain added this to the 1.12 milestone Aug 14, 2021
@@ -242,7 +242,10 @@ void NetworkTextMessage(NetworkAction action, TextColour colour, bool self_send,
/* Show the Client ID for the server but not for the client. */
strid = _network_server ? STR_NETWORK_MESSAGE_CLIENT_JOINED_ID : STR_NETWORK_MESSAGE_CLIENT_JOINED;
break;
case NETWORK_ACTION_LEAVE: strid = STR_NETWORK_MESSAGE_CLIENT_LEFT; break;
case NETWORK_ACTION_LEAVE:
/* Show client identification for the server but not for the client. */
Copy link
Member

Choose a reason for hiding this comment

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

the sentence 4 lines higher says exactly the same with different words. Could you please sync those up?

@@ -205,16 +205,14 @@ struct PacketWriter : SaveFilter {
* Create a new socket for the server side of the game connection.
* @param s The socket to connect with.
*/
ServerNetworkGameSocketHandler::ServerNetworkGameSocketHandler(SOCKET s) : NetworkGameSocketHandler(s)
ServerNetworkGameSocketHandler::ServerNetworkGameSocketHandler(SOCKET s, NetworkAddress client_address) : NetworkGameSocketHandler(s, _network_client_id++), status(STATUS_INACTIVE), receive_limit(_settings_client.network.bytes_per_frame_burst), client_address(client_address)
Copy link
Member

Choose a reason for hiding this comment

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

This _network_client_id++ is going to kick us in the ass sooner or later. Please move this to the body of this function.

src/network/network_server.cpp Show resolved Hide resolved
@@ -2484,11 +2484,12 @@ STR_NETWORK_SERVER_MESSAGE_GAME_REASON_LINK_GRAPH :waiting for lin

STR_NETWORK_MESSAGE_CLIENT_LEAVING :leaving
STR_NETWORK_MESSAGE_CLIENT_JOINED :*** {RAW_STRING} has joined the game
STR_NETWORK_MESSAGE_CLIENT_JOINED_ID :*** {RAW_STRING} has joined the game (Client #{2:NUM})
STR_NETWORK_MESSAGE_CLIENT_JOINED_ID :*** {RAW_STRING} ({1:RAW_STRING}) has joined the game (Client #{2:NUM})
Copy link
Member

Choose a reason for hiding this comment

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

you can now remove the 1, 2 (for both strings)

@TrueBrain
Copy link
Member

TrueBrain commented Sep 18, 2021

Every time I look at this PR, it just doesn't sit right, and I can't put my finger on it. So I spend some time toying around, to see why exactly :)

This PR seems to be mostly aimed for non-GUI servers. In that context, I can understand why it does what it does. But for GUI servers, I have some issues with it.

As I mentioned earlier in this PR, but I haven't seen a response to it in this PR, I do not think we should add the IP in the chat message for server. This is information that is not useful to GUI servers. Banning they do via the Online Players window. And if they really want to know, they could always use clients in the console. But in general, the IP of a user is not useful information for GUI servers; they shouldn't need know.

Similar for the ClientID btw, and I also think that should never been added to the chat message. In fact, I am somewhat tempted to remove it :P But one thing at the time :)

But that leaves your usecase, where you want to match username + IP + clientid. And I understand that usecase, and I think we should support it. But I do think chat-messages are not the way to go about that. So I made a small PR, which is similar to your PR in function, but with these things addressed. Can you take a look and tell me what you think about it? To be clear, the PR is fully meant as a discussion point; I am just trying to balance "being verbose" vs "having the right information", and I notice it isn't trivial :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work: minor details This Pull Request has some minor details left to do
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants