Allow administrator to disable logging of ping requests

[johanfleury]

Description

Allow administrator to disable logging of ping requests on /api/v1/ping.

This is disabled by default, but can be enabled by setting log.log_ping_requests to true (or the PEERTUBE_LOG_PING_REQUESTS environment variable in Docker).

Related issues

Implements #3544

Has this been tested?

• 👍 yes, I added tests to the test suite
• 👍 yes, light tests as follows are enough
• 💭 no, because this PR is a draft and still needs work
• 🙅 no, because they aren't needed
• 🙋 no, because I need help

Not sure how I can implement unit test for that.

[johanfleury]

Ok, so I’ve opened this PR as a draft because I’m not sure if this is the correct way to do this. It should work, but it feels a bit messy (like what if we add a new path?)

Also, should we let the administrator configure this feature?

[kontrollanten]

I think it looks like a good start! If we want to add a new path it can be /api/v1/ping/email and then just check for the beginning of the path to determine.

But maybe some people use the request logging to monitor potential attacks, then I guess this can be a bad solution.

[Chocobozzz]

Hello,

It should not be the responsibility of the client to decide whether or not to log the request. So please instead of using a query parameter, add a config key below https://github.com/Chocobozzz/PeerTube/blob/develop/config/production.yaml.example#L102 logPingRequest: true

[rigelk]

Why not just whitelist the IP emitting the health check?

[johanfleury]

@Chocobozzz:

So please instead of using a query parameter, add a config key below https://github.com/Chocobozzz/PeerTube/blob/develop/config/production.yaml.example#L102 logPingRequest: true

I chose to use snake case (i.e. log_ping_requests) instead of camel case to be more consistent with other configuration options.

@kontrollanten:

But maybe some people use the request logging to monitor potential attacks, then I guess this can be a bad solution.

I don’t see how having logs disabled on that specific endpoint allows any attack. I also doubt many PeerTube instance in production are facing the internet directly. Most of them are probably deployed behind a reverse proxy with access logs enabled.

Anyway this is now at the administrator discretion.

@rigelk

Why not just whitelist the IP emitting the health check?

I feel like this would be a bit too much for just disabling logs from this endpoint.

Also, in some environments (e.g. Docker and Kubernetes) the source IP address is dynamic so you would end up allowing a subnet for the whole cluster which would have the same effect as just disabling logging on that endpoint

[johanfleury]

The failing test doesn’t seem to be related with my changes.

[Chocobozzz]

Thanks @johanfleury