Improve server logs for administration #9105

Berbe · 2021-04-25T14:18:07Z

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')
- newgrf_debug_data.h may need updating.
- PR must be added to API tracker

TrueBrain · 2021-04-25T14:26:06Z

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 · 2021-04-25T14:38:09Z

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 · 2021-04-25T14:40:14Z

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 · 2021-04-25T14:44:46Z

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).

TrueBrain · 2021-04-25T15:16:20Z

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 · 2021-04-25T15:27:06Z

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 · 2021-04-25T15:49:30Z

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.

TrueBrain · 2021-04-25T16:44:49Z

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

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 · 2021-04-25T16:48:21Z

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 · 2021-04-25T16:48:44Z

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

TrueBrain · 2021-06-27T23:15:24Z

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 · 2021-09-12T19:22:44Z

src/network/network.cpp

@@ -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. */


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

TrueBrain · 2021-09-12T19:23:15Z

src/network/network_server.cpp

@@ -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)


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

TrueBrain · 2021-09-12T19:33:32Z

src/lang/english.txt

@@ -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})


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

TrueBrain · 2021-09-18T10:09:43Z

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 :)

Berbe marked this pull request as draft April 25, 2021 14:59

Berbe force-pushed the logs branch from ca3bffe to c3fa447 Compare April 25, 2021 15:47

Berbe force-pushed the logs branch from c3fa447 to 958938b Compare April 25, 2021 16:28

Berbe marked this pull request as ready for review April 25, 2021 16:30

TrueBrain reviewed Apr 25, 2021

View reviewed changes

src/network/network_server.cpp Outdated Show resolved Hide resolved

src/network/network_server.cpp Outdated Show resolved Hide resolved

TrueBrain added the work: minor details label Jul 2, 2021

TrueBrain added this to the 1.12 milestone Aug 14, 2021

Change: Use member initialisers

Verified

This commit was signed with the committer’s verified signature. The key has expired.

Berbe

GPG key ID: 17B4F3B42338B299
Expired

Verified
Learn about vigilant mode

b52d611

Berbe force-pushed the logs branch from 958938b to c647ee8 Compare September 5, 2021 17:33

Berbe added 2 commits September 5, 2021 19:47

Change: Link client ID with IP address on connection (close)

Verified

This commit was signed with the committer’s verified signature. The key has expired.

Berbe

GPG key ID: 17B4F3B42338B299
Expired

Verified
Learn about vigilant mode

19fc598

Add: IP address in server logs on client join/leave

Verified

This commit was signed with the committer’s verified signature. The key has expired.

Berbe

GPG key ID: 17B4F3B42338B299
Expired

Verified
Learn about vigilant mode

f7911ad

Berbe force-pushed the logs branch from c647ee8 to f7911ad Compare September 5, 2021 17:48

TrueBrain reviewed Sep 12, 2021

View reviewed changes

TrueBrain mentioned this pull request Sep 18, 2021

Add: [Network] On join, log the ClientID + IP + Name clearly #9558

Merged

LordAro closed this in #9558 Sep 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve server logs for administration #9105

Improve server logs for administration #9105

Berbe commented Apr 25, 2021 •

edited

Loading

TrueBrain commented Apr 25, 2021

TrueBrain commented Apr 25, 2021 •

edited

Loading

Berbe commented Apr 25, 2021

Berbe commented Apr 25, 2021 •

edited

Loading

TrueBrain commented Apr 25, 2021 •

edited

Loading

TrueBrain commented Apr 25, 2021

Berbe commented Apr 25, 2021

TrueBrain commented Apr 25, 2021 •

edited

Loading

James103 commented Apr 25, 2021

TrueBrain commented Apr 25, 2021

TrueBrain commented Jun 27, 2021

TrueBrain Sep 12, 2021

TrueBrain Sep 12, 2021

TrueBrain Sep 12, 2021

TrueBrain commented Sep 18, 2021 •

edited

Loading

Improve server logs for administration #9105

Improve server logs for administration #9105

Conversation

Berbe commented Apr 25, 2021 • edited Loading

Motivation / Problem

Description

Limitations

Checklist for review

TrueBrain commented Apr 25, 2021

TrueBrain commented Apr 25, 2021 • edited Loading

Berbe commented Apr 25, 2021

Berbe commented Apr 25, 2021 • edited Loading

TrueBrain commented Apr 25, 2021 • edited Loading

TrueBrain commented Apr 25, 2021

Berbe commented Apr 25, 2021

TrueBrain commented Apr 25, 2021 • edited Loading

James103 commented Apr 25, 2021

TrueBrain commented Apr 25, 2021

TrueBrain commented Jun 27, 2021

TrueBrain Sep 12, 2021

Choose a reason for hiding this comment

TrueBrain Sep 12, 2021

Choose a reason for hiding this comment

TrueBrain Sep 12, 2021

Choose a reason for hiding this comment

TrueBrain commented Sep 18, 2021 • edited Loading

Berbe commented Apr 25, 2021 •

edited

Loading

TrueBrain commented Apr 25, 2021 •

edited

Loading

Berbe commented Apr 25, 2021 •

edited

Loading

TrueBrain commented Apr 25, 2021 •

edited

Loading

TrueBrain commented Apr 25, 2021 •

edited

Loading

TrueBrain commented Sep 18, 2021 •

edited

Loading