Fix pointer receiver handling in queryparams marshaler by ash2k · Pull Request #43346 · kubernetes/kubernetes

ash2k · 2017-03-18T12:57:35Z

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.

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

k8s-reviewable · 2017-03-18T12:57:40Z

This change is

k8s-ci-robot · 2017-03-18T12:57:42Z

Hi @ash2k. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

smarterclayton · 2017-03-18T21:51:31Z

@k8s-bot ok to test

Is this happening outside of a test case?

ash2k · 2017-03-18T23:07:35Z

@smarterclayton Yes, but this is outside of Kubernetes. I'm trying to copy certain fields from one Unstructured to another one.
Wow, so many tests have failed - is it my fault?

ash2k · 2017-03-31T01:55:58Z

I've removed the MarshalJSON() change - want to do it incrementally. I think I've fixed an issue where converter did not check if a type has a pointer receiver for the method when it is not a pointer-based type (i.e. when type is Time and it does not implement Marshaller interface but *Time does). See the diff.
One of the builds is failing however yesterday it was failing in a different way and I was not able to figure out what the issue was. And same now - I don't understand what is failing and why. Not familiar with the build system (yet). If someone could take a look...

/cc @smarterclayton

ash2k · 2017-03-31T01:57:41Z

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

 	marshaler, ok := value.Interface().(Marshaler)
 	if !ok {
-		return reflect.Value{}, false
+		if !isPointerKind(value.Kind()) && value.CanAddr() {


I implemented this and then realized json package must be doing the same check. And it does https://github.com/golang/go/blob/9073af247d602dff4633710adf90c8b3c1869c45/src/encoding/json/encode.go#L392

ash2k · 2017-04-23T04:38:05Z

I've just rebased onto master and it's all green. Could someone take a look? @kubernetes/sig-api-machinery-misc

ash2k · 2017-04-25T23:33:28Z

39 days since I opened this PR. Is anybody reading my comments? :) I understand that this is not very important but it is clearly a bug and the fix seems to not be controversial.

ash2k · 2017-05-11T10:40:22Z

/unassign @smarterclayton

ironcladlou · 2017-08-30T13:23:07Z

/lgtm

Thanks for your patience with this one...

janetkuo · 2017-08-30T18:29:04Z

/approve no-issue
/retest

ash2k · 2017-08-30T22:52:33Z

/retest

k8s-ci-robot · 2017-08-30T22:52:47Z

@ash2k: you can't request testing unless you are a kubernetes member.

Details

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

janetkuo · 2017-08-31T22:01:31Z

/retest

janetkuo · 2017-08-31T22:01:48Z

@deads2k would you approve this?

ash2k · 2017-09-05T08:44:37Z

/retest

k8s-ci-robot · 2017-09-05T08:44:51Z

@ash2k: you can't request testing unless you are a kubernetes member.

Details

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

dims · 2017-09-05T15:36:11Z

/retest

Automatic merge from submit-queue Fix pointer receivers handling in unstructured converter **What this PR does / why we need it**: Fixes unstructured converter to properly handle types that have `MarshalJSON()` implemented with a pointer receiver. In particular the converter now can roundtrip `*Unstructured` type. **Which issue this PR fixes**: Fixes #47889. Similar to #43346. **Special notes for your reviewer**: Without the fix the tests are failing: ```console make test WHAT=./vendor/k8s.io/apimachinery/pkg/conversion/unstructured Running tests for APIVersion: v1,admissionregistration.k8s.io/v1alpha1,admission.k8s.io/v1alpha1,apps/v1beta1,apps/v1beta2,authentication.k8s.io/v1,authentication.k8s.io/v1beta1,authorization.k8s.io/v1,authorization.k8s.io/v1beta1,autoscaling/v1,autoscaling/v2alpha1,batch/v1,batch/v1beta1,batch/v2alpha1,certificates.k8s.io/v1beta1,extensions/v1beta1,imagepolicy.k8s.io/v1alpha1,networking.k8s.io/v1,policy/v1beta1,rbac.authorization.k8s.io/v1,rbac.authorization.k8s.io/v1beta1,rbac.authorization.k8s.io/v1alpha1,scheduling.k8s.io/v1alpha1,settings.k8s.io/v1alpha1,storage.k8s.io/v1beta1,storage.k8s.io/v1,,federation/v1beta1 +++ [0829 16:39:36] Running tests without code coverage --- FAIL: TestCustomToUnstructured (0.00s) --- FAIL: TestCustomToUnstructured/false (0.00s) Error Trace: converter_test.go:500 Error: Not equal: expected: bool(false) actual: map[string]interface {}(map[string]interface {}{"data":"ZmFsc2U="}) Messages: customPointer1 --- FAIL: TestCustomToUnstructured/[1] (0.00s) Error Trace: converter_test.go:500 Error: Not equal: expected: []interface {}([]interface {}{1}) actual: map[string]interface {}(map[string]interface {}{"data":"WzFd"}) Messages: customPointer1 --- FAIL: TestCustomToUnstructured/true (0.00s) Error Trace: converter_test.go:500 Error: Not equal: expected: bool(true) actual: map[string]interface {}(map[string]interface {}{"data":"dHJ1ZQ=="}) Messages: customPointer1 --- FAIL: TestCustomToUnstructured/0 (0.00s) Error Trace: converter_test.go:500 Error: Not equal: expected: int64(0) actual: map[string]interface {}(map[string]interface {}{"data":"MA=="}) Messages: customPointer1 --- FAIL: TestCustomToUnstructured/null (0.00s) Error Trace: converter_test.go:500 Error: Not equal: expected: <nil>(<nil>) actual: map[string]interface {}(map[string]interface {}{"data":"bnVsbA=="}) Messages: customPointer1 --- FAIL: TestCustomToUnstructured/[] (0.00s) Error Trace: converter_test.go:500 Error: Not equal: expected: []interface {}([]interface {}{}) actual: map[string]interface {}(map[string]interface {}{"data":"W10="}) Messages: customPointer1 --- FAIL: TestCustomToUnstructured/{"a":1} (0.00s) Error Trace: converter_test.go:500 Error: Not equal: expected: map[string]interface {}{"a":1} actual: map[string]interface {}{"data":"eyJhIjoxfQ=="} Diff: --- Expected +++ Actual @@ -1,3 +1,3 @@ (map[string]interface {}) (len=1) { - (string) (len=1) "a": (int64) 1 + (string) (len=4) "data": (string) (len=12) "eyJhIjoxfQ==" } Messages: customPointer1 --- FAIL: TestCustomToUnstructured/0.0 (0.00s) Error Trace: converter_test.go:500 Error: Not equal: expected: float64(0) actual: map[string]interface {}(map[string]interface {}{"data":"MC4w"}) Messages: customPointer1 --- FAIL: TestCustomToUnstructured/{} (0.00s) Error Trace: converter_test.go:500 Error: Not equal: expected: map[string]interface {}{} actual: map[string]interface {}{"data":"e30="} Diff: --- Expected +++ Actual @@ -1,2 +1,3 @@ -(map[string]interface {}) { +(map[string]interface {}) (len=1) { + (string) (len=4) "data": (string) (len=4) "e30=" } Messages: customPointer1 --- FAIL: TestCustomToUnstructuredTopLevel (0.00s) --- FAIL: TestCustomToUnstructuredTopLevel/1 (0.00s) Error Trace: converter_test.go:519 Error: Not equal: expected: map[string]interface {}{"a":1} actual: map[string]interface {}{"data":"eyJhIjoxfQ=="} Diff: --- Expected +++ Actual @@ -1,3 +1,3 @@ (map[string]interface {}) (len=1) { - (string) (len=1) "a": (int64) 1 + (string) (len=4) "data": (string) (len=12) "eyJhIjoxfQ==" } FAIL FAIL k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/conversion/unstructured 0.047s make: *** [test] Error 1 ``` ```console make test WHAT=./vendor/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured Running tests for APIVersion: v1,admissionregistration.k8s.io/v1alpha1,admission.k8s.io/v1alpha1,apps/v1beta1,apps/v1beta2,authentication.k8s.io/v1,authentication.k8s.io/v1beta1,authorization.k8s.io/v1,authorization.k8s.io/v1beta1,autoscaling/v1,autoscaling/v2alpha1,batch/v1,batch/v1beta1,batch/v2alpha1,certificates.k8s.io/v1beta1,extensions/v1beta1,imagepolicy.k8s.io/v1alpha1,networking.k8s.io/v1,policy/v1beta1,rbac.authorization.k8s.io/v1,rbac.authorization.k8s.io/v1beta1,rbac.authorization.k8s.io/v1alpha1,scheduling.k8s.io/v1alpha1,settings.k8s.io/v1alpha1,storage.k8s.io/v1beta1,storage.k8s.io/v1,,federation/v1beta1 +++ [0829 16:40:38] Running tests without code coverage --- FAIL: TestConversionRoundtrip (0.00s) unstructured_test.go:111: FromUnstructured failed: Object 'Kind' is missing in '{"object":{"apiVersion":"v1","kind":"Foo","metadata":{"name":"foo1"}}}' FAIL FAIL k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured 0.046s make: *** [test] Error 1 ``` **Release note**: ```release-note NONE ``` /kind bug /sig api-machinery

ash2k · 2017-09-23T02:23:32Z

/retest

k8s-ci-robot · 2017-09-23T02:23:47Z

@ash2k: you can't request testing unless you are a kubernetes member.

Details

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ash2k · 2017-09-25T12:19:10Z

Half a year later, this is still open :) Can someone take a look please? @ironcladlou

ironcladlou · 2017-09-26T12:41:27Z

@ash2k, I already gave this a lgtm, but I don't think that's enough...

ash2k · 2017-09-27T09:27:26Z

@deads2k PTAL.

p.s. Those builds failed because of something else and I cannot do /retest.

deads2k · 2017-09-27T12:13:38Z

/retest
/approve no-issue

k8s-github-robot · 2017-09-27T12:13:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ash2k, deads2k, ironcladlou, janetkuo

Associated issue requirement bypassed by: deads2k

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these OWNERS Files:

~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [deads2k]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-09-27T13:05:44Z

/test all

Tests are more than 96 hours old. Re-running tests.

k8s-github-robot · 2017-09-27T14:24:24Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-09-27T15:14:04Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 18, 2017

k8s-github-robot assigned krousey Mar 18, 2017

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Mar 18, 2017

smarterclayton assigned smarterclayton and unassigned krousey Mar 18, 2017

smarterclayton added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Mar 18, 2017

ash2k force-pushed the fix-time-npe branch 3 times, most recently from 021e7b2 to 0ef087d Compare March 30, 2017 11:46

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 30, 2017

ash2k force-pushed the fix-time-npe branch from 90311eb to 7177127 Compare March 30, 2017 22:49

ash2k commented Mar 31, 2017

View reviewed changes

grodrigues3 added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 5, 2017

ash2k force-pushed the fix-time-npe branch 2 times, most recently from 5fa390e to a84fce3 Compare April 23, 2017 00:21

ash2k force-pushed the fix-time-npe branch from a84fce3 to 6efdcca Compare May 10, 2017 12:56

k8s-ci-robot unassigned smarterclayton May 11, 2017

k8s-github-robot assigned errordeveloper May 11, 2017

ash2k mentioned this pull request Aug 29, 2017

Fix pointer receivers handling in unstructured converter #51505

Merged

Pointer receiver support for MarshalQueryParameter()

0fe4911

ash2k force-pushed the fix-time-npe branch from 6ec5365 to 0fe4911 Compare August 30, 2017 06:18

k8s-ci-robot assigned ironcladlou Aug 30, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2017

janetkuo approved these changes Aug 30, 2017

View reviewed changes

ash2k changed the title ~~Fix NPE in Time type~~ Fix pointer receiver handling in queryparams marshaler Sep 27, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 27, 2017

k8s-github-robot merged commit 1ccdc5c into kubernetes:master Sep 27, 2017

ash2k deleted the fix-time-npe branch September 28, 2017 00:23

Conversation

ash2k commented Mar 18, 2017

Uh oh!

k8s-reviewable commented Mar 18, 2017

Uh oh!

k8s-ci-robot commented Mar 18, 2017

Uh oh!

smarterclayton commented Mar 18, 2017

Uh oh!

ash2k commented Mar 18, 2017

Uh oh!

ash2k commented Mar 31, 2017

Uh oh!

ash2k Mar 31, 2017

Choose a reason for hiding this comment

Uh oh!

ash2k commented Apr 23, 2017

Uh oh!

ash2k commented Apr 25, 2017

Uh oh!

ash2k commented May 11, 2017

Uh oh!

ironcladlou commented Aug 30, 2017

Uh oh!

janetkuo commented Aug 30, 2017

Uh oh!

ash2k commented Aug 30, 2017

Uh oh!

k8s-ci-robot commented Aug 30, 2017

Uh oh!

janetkuo commented Aug 31, 2017

Uh oh!

janetkuo commented Aug 31, 2017

Uh oh!

ash2k commented Sep 5, 2017

Uh oh!

k8s-ci-robot commented Sep 5, 2017

Uh oh!

dims commented Sep 5, 2017

Uh oh!

ash2k commented Sep 23, 2017

Uh oh!

k8s-ci-robot commented Sep 23, 2017

Uh oh!

ash2k commented Sep 25, 2017

Uh oh!

ironcladlou commented Sep 26, 2017

Uh oh!

ash2k commented Sep 27, 2017

Uh oh!

deads2k commented Sep 27, 2017

Uh oh!

k8s-github-robot commented Sep 27, 2017

Uh oh!

k8s-github-robot commented Sep 27, 2017

Uh oh!

k8s-github-robot commented Sep 27, 2017

Uh oh!

k8s-github-robot commented Sep 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants