diff --git a/internal/serialize/keyvalues.go b/internal/serialize/keyvalues.go index e64dab36..193796f2 100644 --- a/internal/serialize/keyvalues.go +++ b/internal/serialize/keyvalues.go @@ -20,6 +20,8 @@ import ( "bytes" "fmt" "strconv" + + "github.com/go-logr/logr" ) // WithValues implements LogSink.WithValues. The old key/value pairs are @@ -176,6 +178,24 @@ func KVListFormat(b *bytes.Buffer, keysAndValues ...interface{}) { writeStringValue(b, true, v) case error: writeStringValue(b, true, ErrorToString(v)) + case logr.Marshaler: + value := MarshalerToValue(v) + // A marshaler that returns a string is useful for + // delayed formatting of complex values. We treat this + // case like a normal string. This is useful for + // multi-line support. + // + // We could do this by recursively formatting a value, + // but that comes with the risk of infinite recursion + // if a marshaler returns itself. Instead we call it + // only once and rely on it returning the intended + // value directly. + switch value := value.(type) { + case string: + writeStringValue(b, true, value) + default: + writeStringValue(b, false, fmt.Sprintf("%+v", v)) + } case []byte: // In https://github.com/kubernetes/klog/pull/237 it was decided // to format byte slices with "%+q". The advantages of that are: @@ -208,6 +228,18 @@ func StringerToString(s fmt.Stringer) (ret string) { return } +// MarshalerToValue invokes a marshaler and catches +// panics. +func MarshalerToValue(m logr.Marshaler) (ret interface{}) { + defer func() { + if err := recover(); err != nil { + ret = fmt.Sprintf("", err) + } + }() + ret = m.MarshalLog() + return +} + // ErrorToString converts an error to a string, // handling panics if they occur. func ErrorToString(err error) (ret string) { diff --git a/test/output.go b/test/output.go index b69286bd..673e40c5 100644 --- a/test/output.go +++ b/test/output.go @@ -358,7 +358,13 @@ I output.go:] "test" firstKey=1 secondKey=3 "MarshalLog() that panics": { text: "marshaler panic", values: []interface{}{"obj", faultyMarshaler{}}, - expectedOutput: `I output.go:] "marshaler panic" obj={} + expectedOutput: `I output.go:] "marshaler panic" obj="" +`, + }, + "MarshalLog() that returns itself": { + text: "marshaler recursion", + values: []interface{}{"obj", recursiveMarshaler{}}, + expectedOutput: `I output.go:] "marshaler recursion" obj={} `, }, } @@ -802,6 +808,15 @@ func (f faultyMarshaler) MarshalLog() interface{} { var _ logr.Marshaler = faultyMarshaler{} +type recursiveMarshaler struct{} + +// MarshalLog returns itself, which could cause the logger to recurse infinitely. +func (r recursiveMarshaler) MarshalLog() interface{} { + return r +} + +var _ logr.Marshaler = recursiveMarshaler{} + type faultyError struct{} // Error always panics. diff --git a/test/zapr.go b/test/zapr.go index 56e60e9f..32a82e68 100644 --- a/test/zapr.go +++ b/test/zapr.go @@ -161,8 +161,12 @@ I output.go:] "odd WithValues" keyWithoutValue="(MISSING)" `: `{"caller":"test/output.go:","msg":"error panic","errError":"PANIC=fake Error panic"} `, - `I output.go:] "marshaler panic" obj={} + `I output.go:] "marshaler panic" obj="" `: `{"caller":"test/output.go:","msg":"marshaler panic","v":0,"objError":"PANIC=fake MarshalLog panic"} +`, + + `I output.go:] "marshaler recursion" obj={} +`: `{"caller":"test/output.go:","msg":"marshaler recursion","v":0,"obj":{}} `, // klog.Info