Skip to content

Trace improvements

Arran Walker requested to merge ajwalker/trace-improvements into master

What does this MR do?

Improves the trace implementation, especially around secret hiding.

This uses the golang.org/x/text/transform library for efficient text transforms that can be chained.

Why was this MR needed?

  • The trace implementation used an odd Pipe() / goroutine setup.

  • Hiding secrets was difficult.

    The trie replace implementation required the secret to be known. This has proved difficult to extend.

    For example, we have a "Scrub URL" function that removes sensitive information from URLs. This worked in some contexts, but not in others (#4625 (closed)). It didn't work at the trace level, which is where it should be handled. By using text/transform, we can more easily extend how we scrub secrets in the future, without having to add static secrets.

  • We found that the performance was degraded when using regexp to scrub URLs (!1541 (comment 364168496)), especially if we had to run it multiple times to cover different contexts.

  • There was a bug with the existing implementation: #16705 (closed)

Benchmarks

BenchmarkBuffer is the old trace implementation

BenchmarkBuffer10kWithURLScrub is the old implementating using the URL Scrub regex function

BenchmarkTrace10kWithURLScrub is the implementation this MR provides, with built-in URL scrubbing.

BenchmarkBuffer10k-16                          4     331473228 ns/op      10.50 MB/s     1851978 B/op     230047 allocs/op
BenchmarkBuffer10kWithURLScrub-16              2     529119926 ns/op       6.58 MB/s    19836228 B/op     330116 allocs/op
BenchmarkTrace10kWithURLScrub-16              16      68736431 ns/op      50.63 MB/s       13777 B/op         27 allocs/op

What's the best way to test this MR?

There's no doubt that the Transform parts of this MR are difficult to follow. It's low level byte handling.

This MR has been split across two commits: One contains both implementations and includes benchmarks. The last commit removes the older implementation.

All previous tests pass (in trace_test and mask_test). It also contains a TestVariablesMaskingBoundary test to perform masking at arbitrary boundaries.

There's another MR currently in-flight for performing Fuzz testing on the existing trace masking (!2347 (merged)), this will later add confidence to both the static secret masking and URL scrubbing from this MR.

What are the relevant issue numbers?

Closes #4625 (closed)

Closes #16705 (closed)

!1541 (closed)

Edited by Arran Walker

Merge request reports