Report parsing error in json serializer #63668

ash2k · 2018-05-10T12:04:13Z

What this PR does / why we need it:
Fixes missing error reporting in json parsing using the json-iterator library. Also introduces a private copy of the library config to partially shield from external mutations. json-iterator/go#265.

Special notes for your reviewer:
Found while working on refactoring in #63284.

Release note:

NONE

/kind bug
/sig api-machinery
/cc wojtek-t liggitt

ash2k · 2018-05-10T12:05:00Z

staging/src/k8s.io/apimachinery/pkg/runtime/serializer/json/json.go

+			*(*interface{})(ptr) = f64
+			return
+		}
+		iter.ReportError("DecodeNumber", err.Error())


This line is missing in the deleted snippet below. Everything else is exactly the same.

ash2k · 2018-05-10T12:05:52Z

staging/src/k8s.io/apimachinery/pkg/runtime/serializer/json/json.go

-			*(*interface{})(ptr) = iter.Read()
-		}
-	}
-	jsoniter.RegisterTypeDecoderFunc("interface {}", decodeNumberAsInt64IfPossible)


This mutates some package level field inside of the library. 😞

lavalamp · 2018-05-10T20:20:31Z

/assign @roycaihw

ash2k · 2018-05-17T09:41:43Z

@kubernetes/sig-api-machinery-bugs PTAL

roycaihw

Sorry I didn't get to this earlier. It lgtm in general. I left one comment.

roycaihw · 2018-05-17T21:39:24Z

staging/src/k8s.io/apimachinery/pkg/runtime/serializer/json/json.go

+	// Private copy of jsoniter.ConfigCompatibleWithStandardLibrary to try to shield against possible mutations
+	// from outside. Still does not protect from package level jsoniter.Register*() functions - someone calling them
+	// in some other library will mess with every usage of the jsoniter library in the whole program.
+	// See https://github.com/json-iterator/go/issues/265


Thanks for the pointer. Is there any thing we can do to prevent misusing package level jsoniter.Register*() functions? Did a quick search in our code base and currently this file seems to be the only place using it.

Since I see your effort changing use of encoding/json to github.com/json-iterator/go, does it worth adding some linter/script in CI/makerule that blocks any use of jsoniter.Register*() functions, in case we adopt more and more use of jsoniter?

Is there any thing we can do to prevent misusing package level jsoniter.Register*() functions? Did a quick search in our code base and currently this file seems to be the only place using it.

I think the right thing is to fix the library but the maintainer thinks it is not a problem so... We can have a linting script, as you pointed out. However, I just want to fix the bug and I don't have time to add a linter. Can create a ticket to track this work but I doubt anyone will pick it up :)

I agree with you and I see the maintainer's point. It would be great if you could open a ticket to track the linting work.

Sure #64010

roycaihw · 2018-05-18T18:25:32Z

Thanks @ash2k.

/lgtm

ash2k · 2018-05-18T22:16:58Z

/retest

ash2k · 2018-05-18T22:45:36Z

/assign deads2k
For approval please

deads2k · 2018-05-21T17:34:19Z

/assign deads2k
For approval please

I wasn't involved in jsoniter. I think @thockin worked on that.

/assign @thockin

ash2k · 2018-05-23T09:34:05Z

/retest

ash2k · 2018-05-28T10:07:45Z

/retest

ash2k · 2018-05-29T11:17:27Z

/retest

sttts · 2018-05-29T12:36:27Z

staging/src/k8s.io/apimachinery/pkg/runtime/serializer/json/json.go

+	// in some other library will mess with every usage of the jsoniter library in the whole program.
+	// See https://github.com/json-iterator/go/issues/265
+	configCompatibleWithStandardLibrary = jsoniter.Config{
+		EscapeHTML:             true,


this needs a test to detect drift.

This is my biggest concert with this PR. Can we start with their struct and make a deepcopy or something? Can we get them (upstream) to have an immutable baseline object and then a mutable one that derives from that. Then we can skip th emutable one, but stay in sync with the base?

@thockin

Can we start with their struct and make a deepcopy or something?

We cannot make a copy because jsoniter.ConfigCompatibleWithStandardLibrary is an interface.

Can we get them (upstream) to have an immutable baseline object and then a mutable one that derives from that. Then we can skip th emutable one, but stay in sync with the base?

I have tried to communicate the issue but failed. See json-iterator/go#265.

I don't want to boil the ocean here, just want to fix the bug. I can reduce the scope of this PR to just add the missing error handling line. WDYT?

I mean, it is sad, but... what else can I do?

Can we get them to expose the pre-Froze() struct so we can copy that and Froze() it ourself?

Rather than race to get this in 1.11, is it a big deal to try to get good resolution to this issue and then aim for 1.12 ? Is there any real issue being triggered?

I think this can be delayed until later.

thockin · 2018-05-29T17:54:31Z

staging/src/k8s.io/apimachinery/pkg/runtime/serializer/json/json.go

+	// in some other library will mess with every usage of the jsoniter library in the whole program.
+	// See https://github.com/json-iterator/go/issues/265
+	configCompatibleWithStandardLibrary = jsoniter.Config{
+		EscapeHTML:             true,


This is my biggest concert with this PR. Can we start with their struct and make a deepcopy or something? Can we get them (upstream) to have an immutable baseline object and then a mutable one that derives from that. Then we can skip th emutable one, but stay in sync with the base?

thockin · 2018-05-29T17:56:49Z

staging/src/k8s.io/apimachinery/pkg/runtime/serializer/json/json.go

+	// from outside. Still does not protect from package level jsoniter.Register*() functions - someone calling them
+	// in some other library will mess with every usage of the jsoniter library in the whole program.
+	// See https://github.com/json-iterator/go/issues/265
+	configCompatibleWithStandardLibrary = jsoniter.Config{


Do we use jsoniter elsewhere? I.e. do we need our ow pkg/util/json that exposes this?

My plan was to use k8s.io/apimachinery/pkg/util/json (which uses jsoniter) everywhere in k/k to get performance benefits across the board. See WIP #63284.

thockin · 2018-05-31T20:15:22Z

I'm going to approve this for now, but I I'm asking you to not drop this topic - I am sure we can get them to do something to make drift less likely. Loop me in if you need, please.

/approve

dims · 2018-06-04T14:35:20Z

/milestone v1.11

/hold

@thockin @ash2k please remove hold if you need this for 1.11

k8s-github-robot · 2018-06-04T14:36:13Z

[MILESTONENOTIFIER] Milestone Pull Request Labels Incomplete

@ash2k @deads2k @roycaihw @thockin

Action required: This pull request requires label changes. If the required changes are not made within 3 days, the pull request will be moved out of the v1.11 milestone.

priority: Must specify exactly one of priority/critical-urgent, priority/important-longterm or priority/important-soon.

Help

jberkus · 2018-06-04T20:51:34Z

@thockin @ash2k if you need this in 1.11, please add priority and approved-for-milestone labels. Thanks!

thockin · 2018-06-04T21:39:41Z

Rather than race to get this in 1.11, is it a big deal to try to get good resolution to this issue and then aim for 1.12 ? Is there any real issue being triggered?

dims · 2018-06-04T21:40:44Z

sounds like a plan @thockin

/milestone clear

ash2k · 2018-06-20T10:01:49Z

@roycaihw lost LGTM due to rebase. Thanks

roycaihw · 2018-06-20T21:24:11Z

/lgtm

k8s-ci-robot · 2018-06-20T21:24:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ash2k, roycaihw, thockin

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ash2k · 2018-06-21T01:05:01Z

@dims can you cancel the hold please

dims · 2018-06-21T01:42:29Z

/hold cancel

k8s-github-robot · 2018-06-21T03:04:14Z

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

k8s-github-robot · 2018-06-21T04:21:34Z

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 release-note-none Denotes a PR that doesn't merit a release note. label May 10, 2018

k8s-ci-robot requested review from liggitt and wojtek-t May 10, 2018 12:04

ash2k commented May 10, 2018

View reviewed changes

k8s-ci-robot assigned roycaihw May 10, 2018

roycaihw reviewed May 17, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 18, 2018

k8s-ci-robot assigned deads2k May 18, 2018

k8s-ci-robot assigned thockin May 21, 2018

sttts reviewed May 29, 2018

View reviewed changes

thockin reviewed May 29, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 31, 2018

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 4, 2018

k8s-ci-robot added this to the v1.11 milestone Jun 4, 2018

k8s-github-robot added the milestone/incomplete-labels label Jun 4, 2018

thockin added status/approved-for-milestone and removed status/approved-for-milestone labels Jun 4, 2018

k8s-ci-robot removed this from the v1.11 milestone Jun 4, 2018

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 16, 2018

Report parsing error in json serializer

b76e512

ash2k force-pushed the jsoniter-error-handling branch from 180529c to b76e512 Compare June 18, 2018 11:46

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 20, 2018

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 21, 2018

k8s-github-robot merged commit 1a75395 into kubernetes:master Jun 21, 2018

ash2k deleted the jsoniter-error-handling branch June 21, 2018 05:11

Report parsing error in json serializer #63668

Report parsing error in json serializer #63668

Conversation

ash2k commented May 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented May 10, 2018

ash2k commented May 17, 2018

roycaihw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roycaihw commented May 18, 2018

ash2k commented May 18, 2018

ash2k commented May 18, 2018

deads2k commented May 21, 2018

ash2k commented May 23, 2018

ash2k commented May 28, 2018

ash2k commented May 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented May 31, 2018

dims commented Jun 4, 2018

k8s-github-robot commented Jun 4, 2018

jberkus commented Jun 4, 2018

thockin commented Jun 4, 2018

dims commented Jun 4, 2018

ash2k commented Jun 20, 2018

roycaihw commented Jun 20, 2018

k8s-ci-robot commented Jun 20, 2018

ash2k commented Jun 21, 2018

dims commented Jun 21, 2018

k8s-github-robot commented Jun 21, 2018

k8s-github-robot commented Jun 21, 2018