Trace improvements
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)