-
Notifications
You must be signed in to change notification settings - Fork 38.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix pointer receiver handling in queryparams marshaler #43346
Conversation
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 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. |
@k8s-bot ok to test Is this happening outside of a test case? |
@smarterclayton Yes, but this is outside of Kubernetes. I'm trying to copy certain fields from one |
021e7b2
to
0ef087d
Compare
I've removed the /cc @smarterclayton |
@@ -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() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
5fa390e
to
a84fce3
Compare
I've just rebased onto master and it's all green. Could someone take a look? @kubernetes/sig-api-machinery-misc |
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. |
/unassign @smarterclayton |
/lgtm Thanks for your patience with this one... |
/approve no-issue |
/retest |
@ash2k: you can't request testing unless you are a kubernetes member. In response to this:
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. |
/retest |
@deads2k would you approve this? |
/retest |
@ash2k: you can't request testing unless you are a kubernetes member. In response to this:
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. |
/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
/retest |
@ash2k: you can't request testing unless you are a kubernetes member. In response to this:
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. |
Half a year later, this is still open :) Can someone take a look please? @ironcladlou |
@ash2k, I already gave this a lgtm, but I don't think that's enough... |
@deads2k PTAL. p.s. Those builds failed because of something else and I cannot do |
/retest |
[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.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
/test all Tests are more than 96 hours old. Re-running tests. |
/test all [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here. |
What this PR does / why we need it:
Time.MarshalQueryParameter()
andTime.MarshalJSON()
try to handle nil pointer object (they callt.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.