Scrub URLs from log keys

What does this MR do?

Scrub URLs from log keys

Why was this MR needed?

SecretsCleanupHook removes secrets from log messages such as X-AMZ-Signature. It does not remove them for the keys, so when a URL with tokens are logged they are not scrubbed as expected.

Iterate over the keys and scrub them as as well, since this is quite a hot path (every log uses this hook) a benchmark was added to test performance to calculate the before and after and there is a 3x performance hit. After an attempt of adding concurrency the performance was even worse since a mutex had to be introduced.

Before

pkg: gitlab.com/gitlab-org/gitlab-runner/log BenchmarkSecretsCleanupHook_Fire-8 1000000 1326 ns/op PASS

After

pkg: gitlab.com/gitlab-org/gitlab-runner/log BenchmarkSecretsCleanupHook_Fire-8 300000 4813 ns/op PASS

Looking at a flame graph of the CPU this is because of the multiple calls to regex since is quite expensive. After trying to add concurrency the performance as even worse:

pkg: gitlab.com/gitlab-org/gitlab-runner/log
BenchmarkSecretsCleanupHook_Fire-8   	  200000	      9990 ns/op
PASS

Diff of the concurrenct change

diff --git a/log/secrets_cleanup.go b/log/secrets_cleanup.go
index d9929748f..e47be1709 100644
--- a/log/secrets_cleanup.go
+++ b/log/secrets_cleanup.go
@@ -1,12 +1,15 @@
 package log

 import (
+       "sync"
+
        "github.com/sirupsen/logrus"

-       "gitlab.com/gitlab-org/gitlab-runner/helpers/url"
+       url_helpers "gitlab.com/gitlab-org/gitlab-runner/helpers/url"
 )

-type SecretsCleanupHook struct{}
+type SecretsCleanupHook struct {
+}

 func (s *SecretsCleanupHook) Levels() []logrus.Level {
        return logrus.AllLevels
@@ -14,9 +17,38 @@ func (s *SecretsCleanupHook) Levels() []logrus.Level {

 func (s *SecretsCleanupHook) Fire(entry *logrus.Entry) error {
        entry.Message = url_helpers.ScrubSecrets(entry.Message)
+
+       wg := sync.WaitGroup{}
+       wg.Add(len(entry.Data))
+
+       for key, value := range entry.Data {
+               safeEntry := &entrySafe{
+                       Entry: entry,
+                       Mutex: sync.Mutex{},
+               }
+
+               go s.scrubKey(&wg, safeEntry, key, value)
+       }
+
+       wg.Wait()
+
        return nil
 }

+func (s *SecretsCleanupHook) scrubKey(wg *sync.WaitGroup, entry *entrySafe, key string, value interface{}) {
+       defer wg.Done()
+       entry.Lock()
+       defer entry.Unlock()
+
+       str, ok := value.(string)
+       if !ok {
+               // If we fail to parse it to remove it to be safe.
+               delete(entry.Data, key)
+       }
+
+       entry.Data[key] = url_helpers.ScrubSecrets(str)
+}
+
 func AddSecretsCleanupLogHook(logger *logrus.Logger) {
        if logger == nil {
                logger = logrus.StandardLogger()
@@ -24,3 +56,8 @@ func AddSecretsCleanupLogHook(logger *logrus.Logger) {

        logger.AddHook(new(SecretsCleanupHook))
 }
+
+type entrySafe struct {
+       *logrus.Entry
+       sync.Mutex
+}

Are there points in the code the reviewer needs to double check?

Does this MR meet the acceptance criteria?

• Documentation created/updated
• Added tests for this feature/bug
• In case of conflicts with master - branch was rebased

What are the relevant issue numbers?

Closes #4625 (closed)