Repository: kubernetes/klog (go)

Issue:
logr.Marshaler: does not handle non-string results
/kind bug

**What steps did you take and what happened:**

I wrote a MarshalLog implementation which returns a slice. klog called the function, but then logged the original value instead of the one returned by MarshaLog.

**What did you expect to happen:**

https://github.com/kubernetes/klog/blob/0990e81f1a8f011181187da59ef32356bb836330/internal/serialize/keyvalues.go#L148 should log `value`, not `v`.


Pull request:
kvlistformat: fix the issue with display marshalled value for non string type
**What this PR does / why we need it**:
Current implementation of `KVListFormat` helper has a bug with how it handles the values for the struct that implements `logr.Marshler` interfaces. 

https://github.com/kubernetes/klog/blob/0990e81f1a8f011181187da59ef32356bb836330/internal/serialize/keyvalues.go#L132-L148

It ends up printing the original value itself and ignores the value returned from the `MarshalLog` call. This happens only if the `MarshalLog` returns a non string type of values.  This PR fixes that issue. 

It also adds a few additional test cases to simulate situations where the struct can implement `logr.Marshler` interface to return a string value or a non string type to avoid future regressions into this path. 

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #343

**Special notes for your reviewer**:

NA

**Release note**:
<!--  Write your release note:
1. Enter your extended release note in the below block. If the PR requires additional action from users switching to the new release, include the string "action required".
2. If no release note is required, just write "NONE".
-->
```release-note
kvlistformat: fix the issue with display of marshalled value for non string type
```

Patch:
diff --git a/internal/serialize/keyvalues.go b/internal/serialize/keyvalues.go
index f85d7cc..ad6bf11 100644
--- a/internal/serialize/keyvalues.go
+++ b/internal/serialize/keyvalues.go
@@ -145,7 +145,7 @@ func KVListFormat(b *bytes.Buffer, keysAndValues ...interface{}) {
 			case string:
 				writeStringValue(b, true, value)
 			default:
-				writeStringValue(b, false, fmt.Sprintf("%+v", v))
+				writeStringValue(b, false, fmt.Sprintf("%+v", value))
 			}
 		case []byte:
 			// In https://github.com/kubernetes/klog/pull/237 it was decided


Test patch:
diff --git a/internal/serialize/keyvalues_test.go b/internal/serialize/keyvalues_test.go
index d4b3385..1bf4842 100644
--- a/internal/serialize/keyvalues_test.go
+++ b/internal/serialize/keyvalues_test.go
@@ -39,6 +39,27 @@ func (p point) String() string {
 	return fmt.Sprintf("x=%d, y=%d", p.x, p.y)
 }
 
+type dummyStruct struct {
+	key   string
+	value string
+}
+
+func (d *dummyStruct) MarshalLog() interface{} {
+	return map[string]string{
+		"key-data":   d.key,
+		"value-data": d.value,
+	}
+}
+
+type dummyStructWithStringMarshal struct {
+	key   string
+	value string
+}
+
+func (d *dummyStructWithStringMarshal) MarshalLog() interface{} {
+	return fmt.Sprintf("%s=%s", d.key, d.value)
+}
+
 // Test that kvListFormat works as advertised.
 func TestKvListFormat(t *testing.T) {
 	var emptyPoint *point
@@ -46,6 +67,14 @@ func TestKvListFormat(t *testing.T) {
 		keysValues []interface{}
 		want       string
 	}{
+		{
+			keysValues: []interface{}{"data", &dummyStruct{key: "test", value: "info"}},
+			want:       " data=map[key-data:test value-data:info]",
+		},
+		{
+			keysValues: []interface{}{"data", &dummyStructWithStringMarshal{key: "test", value: "info"}},
+			want:       ` data="test=info"`,
+		},
 		{
 			keysValues: []interface{}{"pod", "kubedns"},
 			want:       " pod=\"kubedns\"",