Merge pull request #43346 from atlassian/fix-time-npe

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Fix pointer receiver handling in queryparams marshaler

**What this PR does / why we need it**:
`Time.MarshalQueryParameter()` and `Time.MarshalJSON()` try to handle nil pointer object (they call `t.IsZero()` which checks if t == nil) but fail because receiver is not a pointer so the dereference needed to pass it as receiver to these methods fails with npe.
In practice this happens with `Unstructured.SetDeletionTimestamp(Unstructured.GetDeletionTimestamp())`.

Here is the stacktrace of the failing test if receiver is not a pointer.
```go
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x1407969]

goroutine 22 [running]:
testing.tRunner.func1(0xc4204f0680)
	/usr/local/Cellar/go/1.8/libexec/src/testing/testing.go:622 +0x29d
panic(0x1485a80, 0x1782bc0)
	/usr/local/Cellar/go/1.8/libexec/src/runtime/panic.go:489 +0x2cf
k8s.io/kubernetes/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured.(*Unstructured).SetDeletionTimestamp(0xc420030790, 0x0)
	/Users/ash2k/gopath/src/k8s.io/kubernetes/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured.go:387 +0x29
k8s.io/kubernetes/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured.TestNilDeletionTimestamp(0xc4204f0680)
	/Users/ash2k/gopath/src/k8s.io/kubernetes/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_test.go:40 +0x4c
```

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_test.go

@@ -58,3 +58,21 @@ func TestUnstructuredList(t *testing.T) {
 		t.Fatalf("unexpected fields: %#v", items[0])
 	}
 }
+
+func TestNilDeletionTimestamp(t *testing.T) {
+	var u Unstructured
+	del := u.GetDeletionTimestamp()
+	if del != nil {
+		t.Errorf("unexpected non-nil deletion timestamp: %v", del)
+	}
+	u.SetDeletionTimestamp(u.GetDeletionTimestamp())
+	del = u.GetDeletionTimestamp()
+	if del != nil {
+		t.Errorf("unexpected non-nil deletion timestamp: %v", del)
+	}
+	metadata := u.Object["metadata"].(map[string]interface{})
+	deletionTimestamp := metadata["deletionTimestamp"]
+	if deletionTimestamp != nil {
+		t.Errorf("unexpected deletion timestamp field: %q", deletionTimestamp)
+	}
+}

staging/src/k8s.io/apimachinery/pkg/conversion/queryparams/convert.go

@@ -90,7 +90,14 @@ func customMarshalValue(value reflect.Value) (reflect.Value, bool) {
 
 	marshaler, ok := value.Interface().(Marshaler)
 	if !ok {
-		return reflect.Value{}, false
+		if !isPointerKind(value.Kind()) && value.CanAddr() {
+			marshaler, ok = value.Addr().Interface().(Marshaler)
+			if !ok {
+				return reflect.Value{}, false
+			}
+		} else {
+			return reflect.Value{}, false
+		}
 	}
 
 	// Don't invoke functions on nil pointers

staging/src/k8s.io/apimachinery/pkg/conversion/queryparams/convert_test.go

@@ -66,12 +66,13 @@ func (obj *baz) GetObjectKind() schema.ObjectKind { return schema.EmptyObjectKin
 // childStructs tests some of the types we serialize to query params for log API calls
 // notably, the nested time struct
 type childStructs struct {
-	Container    string       `json:"container,omitempty"`
+	Container      string       `json:"container,omitempty"`
-	Follow       bool         `json:"follow,omitempty"`
+	Follow         bool         `json:"follow,omitempty"`
-	Previous     bool         `json:"previous,omitempty"`
+	Previous       bool         `json:"previous,omitempty"`
-	SinceSeconds *int64       `json:"sinceSeconds,omitempty"`
+	SinceSeconds   *int64       `json:"sinceSeconds,omitempty"`
-	SinceTime    *metav1.Time `json:"sinceTime,omitempty"`
+	SinceTime      *metav1.Time `json:"sinceTime,omitempty"`
-	EmptyTime    *metav1.Time `json:"emptyTime"`
+	EmptyTime      *metav1.Time `json:"emptyTime"`
+	NonPointerTime metav1.Time  `json:"nonPointerTime"`
 }
 
 func (obj *childStructs) GetObjectKind() schema.ObjectKind { return schema.EmptyObjectKind }
@@ -177,24 +178,26 @@ func TestConvert(t *testing.T) {
 		},
 		{
 			input: &childStructs{
-				Container:    "mycontainer",
+				Container:      "mycontainer",
-				Follow:       true,
+				Follow:         true,
-				Previous:     true,
+				Previous:       true,
-				SinceSeconds: &sinceSeconds,
+				SinceSeconds:   &sinceSeconds,
-				SinceTime:    &sinceTime, // test a custom marshaller
+				SinceTime:      &sinceTime, // test a custom marshaller
-				EmptyTime:    nil,        // test a nil custom marshaller without omitempty
+				EmptyTime:      nil,        // test a nil custom marshaller without omitempty
+				NonPointerTime: sinceTime,
 			},
-			expected: url.Values{"container": {"mycontainer"}, "follow": {"true"}, "previous": {"true"}, "sinceSeconds": {"123"}, "sinceTime": {"2000-01-01T12:34:56Z"}, "emptyTime": {""}},
+			expected: url.Values{"container": {"mycontainer"}, "follow": {"true"}, "previous": {"true"}, "sinceSeconds": {"123"}, "sinceTime": {"2000-01-01T12:34:56Z"}, "emptyTime": {""}, "nonPointerTime": {"2000-01-01T12:34:56Z"}},
 		},
 		{
 			input: &childStructs{
-				Container:    "mycontainer",
+				Container:      "mycontainer",
-				Follow:       true,
+				Follow:         true,
-				Previous:     true,
+				Previous:       true,
-				SinceSeconds: &sinceSeconds,
+				SinceSeconds:   &sinceSeconds,
-				SinceTime:    nil, // test a nil custom marshaller with omitempty
+				SinceTime:      nil, // test a nil custom marshaller with omitempty
+				NonPointerTime: sinceTime,
 			},
-			expected: url.Values{"container": {"mycontainer"}, "follow": {"true"}, "previous": {"true"}, "sinceSeconds": {"123"}, "emptyTime": {""}},
+			expected: url.Values{"container": {"mycontainer"}, "follow": {"true"}, "previous": {"true"}, "sinceSeconds": {"123"}, "emptyTime": {""}, "nonPointerTime": {"2000-01-01T12:34:56Z"}},
 		},
 	}