Outdated version of the `zap` logger, with multiple wrappers within Gorouter, used in Routing Release

peanball commented 11 months ago

No. I couldn't find CVEs for zap.

Issue

The version of the zap logger used in routing-release and thus Gorouter is pinned to a version from 2016. The API has since changed significantly in newer releases. Performance characteristics may have as well.

Additionally, and potentially worse, there are two wrappers (lager + logger) that ultimately write into zap. The major issue with that is how the wrappers handle auxiliary data to be logged. The data is pre-processed without taking the active log level into consideration.

Each debug statement will actually pre-process and wrap data into zap.Field, just to never be logged in 99% of cases. There are means for lazy evaluation for such data, but they need to be handled explicitly, e.g. in such wrappers.

Affected Versions

Probably all version from 2016 onwards.

Context

As stated above, the zap library is significantly outdated. Furthermore, the wrappers are inefficiently handling auxiliary data when it ultimately will not be logged. ## Traffic Diagram N/A ## Steps to Reproduce

N/A

Expected result

We have an up to date version of zap to get functional, performance and security improvements.

We don't waste additional cycles on preparing data that will ultimately not be logged.

Current result

See above.

Possible Fix

Remove the version pin for zap. Adapt, rewrite or remove the later + logger wrappers. Alternatively, one wrapper could remain if we don't want to tie ourselves to zap. A lot of the code is tied to zap however.

Very alternatively, we could use golang 1.21's slog feature. That also has a zap backend.

Considering that the rift between 2016 zap and current zap is huge, a lot of the logging code would need to be rewritten anyway. If we tackle this larger undertaking, we might also do it the best way available at the current time. Opinions are welcome.

Additional Context

maxmoehl commented 11 months ago

Very alternatively, we could use golang 1.21's slog feature. That also has a zap backend. [...] Alternatively, one wrapper could remain if we don't want to tie ourselves to zap. A lot of the code is tied to zap however.

➕

If we touch this it would probably make sense to use log/slog as the frontend to allow us to change the backend more easily. I'd imagine that most logging libraries will (eventually) support some interoperability with slog.

code.cloudfoundry.org/lager: supported
go.uber.org/zap: experimental support

maxmoehl commented 7 months ago

We recently noticed that our current usage of zap is (periodically) flushing logs when writing a log. This results in syscalls that prevent the calling goroutine from doing its work.

Most prominently we have some larger environments in which we see (almost) constant nats message buffering. Upon closer inspection we noticed that one of the contributors for messages which take a long time to process are write syscalls caused by the zap logger. But I'm sure this causes delays in other parts of gorouter (and other packages) as well.

More recent versions of zap offer a BufferedWriteSyncer. Calls to write on that buffer operate purely on the in-memory buffer and a dedicated goroutine periodically flushes to disk without causing delays in the hot paths.

With our current version of zap this is not supported and as such we can't fix this issue.

@dsabeti since you are assigned: how do you think we should proceed here?

maxmoehl commented 4 months ago

We are planning to start on this soon and plan to use log/slog as the fronted. The gorouter coding would be adjusted to call that and we centrally define which backend is used. Are there any opinions on which backend we should use? Available options (from the top of my head) would be zap or the builtin JSON handler, but there's probably more. Keeping the format stable is our top priority either way.

plowin commented 2 months ago

Proposal in

cloudfoundry / routing-release