mapstr: M.Format and M.MarshalLogObject and mutate nested fields

[efd6]

Currently, when M.Format or MarshalLogObject are called, they result in the mutation of the receiver. This leads to the unexpected mutation of values when printing or logging mapstr.M.

This is demonstrated by running the following program.

package main

import (
	"fmt"

	"github.com/elastic/elastic-agent-libs/mapstr"
)

func main() {
	m := mapstr.M{
		"events": []any{
			map[string]any{
				"url": "",
			},
		},
	}
	fmt.Println(map[string]any(m))
	fmt.Sprint(m)
	fmt.Println(map[string]any(m))
}

output:

map[events:[map[url:]]]
map[events:[map[url:xxxxx]]]

https://go.dev/play/p/rviU45kYeOk

The underlying cause of this is that mapstr.M.Clone, while claiming to deep copy the receiver, does not when it comes to slices or array; only shallow copies are made when any value that is not assignable to a mapstr.M is encountered.

[andrewkroh]

This seems important or severe because it has the potential to "corrupt" data being sent to Elasticsearch, particularly when debug is enabled. I did a very narrow, non-exhaustive search in Beats, and there are cases where this could occur such as

• https://github.com/elastic/beats/blob/13d71a6a2d0ad0217a8aae6ae0ff20a68fef34f4/libbeat/processors/actions/decode_json_fields.go#L109
• https://github.com/elastic/beats/blob/13d71a6a2d0ad0217a8aae6ae0ff20a68fef34f4/libbeat/processors/ratelimit/rate_limit.go#L113

My immediate concern is to ensure data cannot be mutated as a side-effect of logging. Secondarily, I think separation of concerns should be reconsidered for mapstr.M. I don't think a map data structure should be concerned with logging redaction. That IMO, most likely, should be a concern of the logger.

[efd6]

For redaction, I would suggest that one of the shim layers in logp makes a conditional call based on whether the value implements a (bikeshed) Redact() any/Replace() any method. This method would return a log-safe version of the value. Having this be an interface would allow any value to be redacted in logs by providing a wrapper utility type that would perform redaction on the value that it holds. Notably, mapstr.M should not implement this by default since a global logger cannot know the significance of fields purely based on the name of the field.

[cmacknz]

Redaction makes sense to me in the logger, see elastic/elastic-agent#5675 for a place where we'd immediately use this. I think the key would be making it impossible to forget to redact, without mandatory redaction imposing significant performance overhead.