diff --git a/internal/serialize/keyvalues.go b/internal/serialize/keyvalues.go index 4f583912f..d89731368 100644 --- a/internal/serialize/keyvalues.go +++ b/internal/serialize/keyvalues.go @@ -126,11 +126,11 @@ func KVListFormat(b *bytes.Buffer, keysAndValues ...interface{}) { // (https://github.com/kubernetes/kubernetes/pull/106594#issuecomment-975526235). switch v := v.(type) { case fmt.Stringer: - writeStringValue(b, true, stringerToString(v)) + writeStringValue(b, true, StringerToString(v)) case string: writeStringValue(b, true, v) case error: - writeStringValue(b, true, v.Error()) + writeStringValue(b, true, ErrorToString(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: @@ -151,16 +151,30 @@ func KVListFormat(b *bytes.Buffer, keysAndValues ...interface{}) { } } -func stringerToString(s fmt.Stringer) (ret string) { +// StringerToString converts a Stringer to a string, +// handling panics if they occur. +func StringerToString(s fmt.Stringer) (ret string) { defer func() { if err := recover(); err != nil { - ret = "nil" + ret = fmt.Sprintf("", err) } }() ret = s.String() return } +// ErrorToString converts an error to a string, +// handling panics if they occur. +func ErrorToString(err error) (ret string) { + defer func() { + if err := recover(); err != nil { + ret = fmt.Sprintf("", err) + } + }() + ret = err.Error() + return +} + func writeStringValue(b *bytes.Buffer, quote bool, v string) { data := []byte(v) index := bytes.IndexByte(data, '\n') diff --git a/internal/serialize/keyvalues_test.go b/internal/serialize/keyvalues_test.go index f7fdaba36..affb887ec 100644 --- a/internal/serialize/keyvalues_test.go +++ b/internal/serialize/keyvalues_test.go @@ -137,7 +137,7 @@ No whitespace.`, }, { keysValues: []interface{}{"point-1", point{100, 200}, "point-2", emptyPoint}, - want: " point-1=\"x=100, y=200\" point-2=\"nil\"", + want: " point-1=\"x=100, y=200\" point-2=\"\"", }, } diff --git a/klogr/klogr.go b/klogr/klogr.go index 8206c10d4..48cb65815 100644 --- a/klogr/klogr.go +++ b/klogr/klogr.go @@ -140,7 +140,7 @@ func (l klogger) Error(err error, msg string, kvList ...interface{}) { msgStr := flatten("msg", msg) var loggableErr interface{} if err != nil { - loggableErr = err.Error() + loggableErr = serialize.ErrorToString(err) } switch l.format { case FormatSerialize: diff --git a/test/output.go b/test/output.go index 0c447a827..6a6cee9ea 100644 --- a/test/output.go +++ b/test/output.go @@ -286,6 +286,24 @@ I output.go:] "test" firstKey=1 secondKey=3 text: "test", err: errors.New("whoops"), expectedOutput: `E output.go:] "test" err="whoops" +`, + }, + "Error() that panics": { + text: "error panic", + err: (*customErrorJSON)(nil), + expectedOutput: `E output.go:] "error panic" err="" +`, + }, + "String() that panics": { + text: "stringer panic", + values: []interface{}{"stringer", (*stringer)(nil)}, + expectedOutput: `I output.go:] "stringer panic" stringer="" +`, + }, + "MarshalLog that panics": { + text: "marshaler panic", + values: []interface{}{"obj", (*klog.ObjectRef)(nil)}, + expectedOutput: `I output.go:] "marshaler panic" obj="" `, }, } @@ -699,3 +717,14 @@ func (e *customErrorJSON) Error() string { func (e *customErrorJSON) MarshalJSON() ([]byte, error) { return json.Marshal(strings.ToUpper(e.s)) } + +type stringer struct { + s string +} + +// String crashes when called for nil. +func (s *stringer) String() string { + return s.s +} + +var _ fmt.Stringer = &stringer{}