openapiv2 experimental json unmarshaler #319

alexzielenski · 2022-09-27T17:48:25Z

Continuing work from #315

This PR adds integrates the work done by @dsnet in the above issue.

Adds fuzz tests and also tests for roundtripping.

Adds benchmark

jsonv2_via_jsonv1 uses the new experimental decoder but is triggered via json.Unmarshal rather than invoking jsonv2 directly. The legacy json wrapper does have a cost.

goos: darwin
goarch: arm64
pkg: k8s.io/kube-openapi/pkg/validation/spec
BenchmarkSwaggerSpec_ExperimentalUnmarshal/jsonv1-20         	           3	 447452972 ns/op	103245472 B/op	 1506417 allocs/op
BenchmarkSwaggerSpec_ExperimentalUnmarshal/jsonv2_via_jsonv1-20            24	  47524977 ns/op	26232751 B/op	  138894 allocs/op
BenchmarkSwaggerSpec_ExperimentalUnmarshal/jsonv2-20                       44	  25974345 ns/op	26232392 B/op	  138887 allocs/op
PASS
ok  	k8s.io/kube-openapi/pkg/validation/spec	5.320s

/cc @apelisse

pkg/validation/spec/swagger_test.go

apelisse · 2022-09-27T18:07:09Z

Since the wrapper has quite an impact on the performance, can we just provide a FromJSON() method on the Swagger type?

apelisse · 2022-09-27T19:49:51Z

That seems to be what keeps failing for some reason:

         											AdditionalProperties: &spec.SchemaOrBool{
        - 												Allows: true,
        + 												Allows: false,
          												Schema: &{},
          											},

pkg/validation/spec/swagger_test.go

apelisse · 2022-09-27T21:03:28Z

Went through it, looks good to me. Except for the bug I don't understand.

apelisse · 2022-09-28T20:13:16Z

OK feel free to rebase now that #320 has merged.

apelisse

/lgtm

I don't have approval on pkg/validation, but I would appreciate someone else's look at it. @dsnet?

apelisse · 2022-09-28T23:38:35Z

pkg/internal/flags.go

+limitations under the License.
+*/
+
+package internal


Note for other reviewers, this package is at the right level because we'll probably use the same variable for OpenAPI v3 that lives in pkg/spec3. Thanks!

Should this be protected by a build tag rather than a variable?

I've generally recommended against open source projects using go-json-experiment due to the following effect:

Let module M depend on modules A and B.

Module A depends on go-json-experiment/[email protected].

Module B depends on go-json-experiment/[email protected].

Let’s suppose a breaking change occurs between v0.5.0 and v0.8.0.

MVS dictates that v0.8.0 be selected to build module M.

However, the use of v0.8.0 can break package A since it is using the API for v0.5.0, which is not compatible.

I don't know enough about the kubernetes/kube-openapi whether this is a module that is only used to build a binary, or whether it is a module intended to be depended upon by many other modules. If the former, then guarding it with a variable sounds fine. If the latter, then we probably want to guard it with a build tag.

Thanks for the explanation. One quick (and ugly, but we're used to it here) solution would be to just fork the go-json-experiment into this repo so that we only depend internally on it, no more problem with MVS?

@liggitt Do you have an opinion about the suggestion above? Thanks.

Forking SGTM.

The license is BSD and the listed authors are the Go authors since I had originally started the project when I was a Google employee. It is currently developed by all non-Googlers, but every contributor is also a regular contributor to the Go toolchain, so have each signed the Google CLA.

forking into an internal third_party package with correct attribution as an internal implementation detail until the go-json-experiment stabilizes sgtm

I've forked it into pkg/internal/third_party/go-json-experiment/json. I added a readme with the commit hash and a note about when it should be removed.

To fork I executed git clone and removed .git, .github, go.mod, go.sum, and all .png files. I am unsure if there is a more standard way to "fork" the package into our tree. Please let me know if the current way I've done it is OK

pkg/validation/spec/gnostic.go

dsnet

Implementation generally LGTM.

dsnet · 2022-09-29T07:28:35Z

pkg/internal/flags.go

+limitations under the License.
+*/
+
+package internal


Should this be protected by a build tag rather than a variable?

I've generally recommended against open source projects using go-json-experiment due to the following effect:

Let module M depend on modules A and B.

Module A depends on go-json-experiment/[email protected].

Module B depends on go-json-experiment/[email protected].

Let’s suppose a breaking change occurs between v0.5.0 and v0.8.0.

MVS dictates that v0.8.0 be selected to build module M.

However, the use of v0.8.0 can break package A since it is using the API for v0.5.0, which is not compatible.

I don't know enough about the kubernetes/kube-openapi whether this is a module that is only used to build a binary, or whether it is a module intended to be depended upon by many other modules. If the former, then guarding it with a variable sounds fine. If the latter, then we probably want to guard it with a build tag.

pkg/validation/spec/header.go

pkg/validation/spec/responses.go

pkg/validation/spec/paths.go

apelisse · 2022-10-03T20:45:34Z

I'm good on this, just waiting for Jordan to confirm if he's alright with the fork.

pkg/validation/spec/fuzz.go

pkg/validation/spec/header.go

liggitt · 2022-10-04T02:56:39Z

pkg/validation/spec/swagger.go

+func (s *StringOrArray) UnmarshalNextJSON(opts jsonv2.UnmarshalOptions, dec *jsonv2.Decoder) error {
+	switch k := dec.PeekKind(); k {
+	case '[':
+		return opts.UnmarshalNext(dec, (*[]string)(s))


does this switch StringOrArray from overwriting on multiple unmarshals to appending?

no, but ive added a line to reset s so it is explicit

Question since I'm curious, what does the spec requires when it comes to multiple unmarshaling? I was experimenting the other day and it felt inconsistent to me.

pkg/validation/spec/swagger.go

pkg/validation/spec/responses.go

liggitt · 2022-10-04T03:17:20Z

pkg/internal/flags.go

+limitations under the License.
+*/
+
+package internal


forking into an internal third_party package with correct attribution as an internal implementation detail until the go-json-experiment stabilizes sgtm

pkg/validation/spec/response.go

pkg/validation/spec/swagger.go

pkg/validation/spec/parameter.go

pkg/util/testing/json_roundtrip.go

apelisse · 2022-10-06T23:53:35Z

pkg/validation/spec/swagger.go

+func (s *StringOrArray) UnmarshalNextJSON(opts jsonv2.UnmarshalOptions, dec *jsonv2.Decoder) error {
+	switch k := dec.PeekKind(); k {
+	case '[':
+		return opts.UnmarshalNext(dec, (*[]string)(s))


Question since I'm curious, what does the spec requires when it comes to multiple unmarshaling? I was experimenting the other day and it felt inconsistent to me.

pkg/validation/spec/swagger.go

pkg/util/testing/json_roundtrip.go

apelisse · 2022-10-11T17:07:25Z

@dsnet The internal/third_party test is failing, is this something expected?!

dsnet · 2022-10-11T17:09:47Z

The minimum support version of go-json-experiment is Go1.19 since we're relying on updated behavior in the reflect package.

It seems that we should either run tests on Go1.19 or patching a revert for go-json-experiment/json@ef074b6.

liggitt · 2022-10-11T19:51:44Z

/approve
for validation package changes

/hold for green CI on WIP PR in k/k

apelisse · 2022-10-11T23:48:09Z

tests are looking good, https://prow.k8s.io/pr-history/?org=kubernetes&repo=kubernetes&pr=112988

/hold cancel

apelisse · 2022-10-11T23:48:32Z

/lgtm

liggitt · 2022-10-12T01:18:45Z

looks like the testdata and tests get dropped when importing into kube, which is nice, but the png images referenced from the readme get kept, which is less nice... can we drop those from our import of the dependency?

revert json experiment to 4987ed27d44794668c752d227a2d3a7be54570bc correct commit hash correct package path remove pngs

refactor name

…ype which needs it adds subtest `UnmarshalEmbedded` that positively shows that there is at least one property per embedded field also does not use field labels, so compiler errors are thrown in event any are missing whoops

apelisse · 2022-10-12T15:31:45Z

So did you just delete the files from the repo and that's it?

liggitt · 2022-10-12T15:33:27Z

yes, according to https://github.com/kubernetes/kube-openapi/compare/e07a7982065fe6884a76047b719ece144a5cf26a..8d4943971e80ccf385fe182cbeae58a37a3b3819

/lgtm

apelisse · 2022-10-12T15:36:10Z

/approve

Thanks!

k8s-ci-robot · 2022-10-12T15:36:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexzielenski, apelisse, dsnet, liggitt

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:

~~OWNERS~~ [apelisse]
~~pkg/validation/OWNERS~~ [liggitt]

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

apelisse · 2022-10-12T16:33:03Z

What's next?

We probably have the same bug unmarshalling OpenAPI v3
There's probably room for improvement with marshalling OpenAPI v2/v3
Investigate the new trade-offs between json and protobuf, possibly encourage json moving forward

k8s-ci-robot requested a review from apelisse September 27, 2022 17:48

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 27, 2022

alexzielenski force-pushed the json_next branch from 2394259 to 94f2c15 Compare September 27, 2022 17:53

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 27, 2022

apelisse reviewed Sep 27, 2022

View reviewed changes

pkg/validation/spec/swagger_test.go Show resolved Hide resolved

alexzielenski force-pushed the json_next branch from 5952e45 to 283749e Compare September 27, 2022 19:00

apelisse reviewed Sep 27, 2022

View reviewed changes

pkg/validation/spec/swagger_test.go Outdated Show resolved Hide resolved

alexzielenski mentioned this pull request Sep 27, 2022

fix json unmarshal bug when extension used with Responses #320

Merged

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

alexzielenski force-pushed the json_next branch from 6987bd4 to 52157ce Compare September 28, 2022 20:47

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 28, 2022

apelisse reviewed Sep 28, 2022

View reviewed changes

k8s-ci-robot assigned apelisse Sep 28, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 28, 2022

dsnet approved these changes Sep 29, 2022

View reviewed changes

liggitt reviewed Oct 4, 2022

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 5, 2022

apelisse reviewed Oct 6, 2022

View reviewed changes

pkg/util/testing/json_roundtrip.go Outdated Show resolved Hide resolved

apelisse reviewed Oct 6, 2022

View reviewed changes

apelisse reviewed Oct 7, 2022

View reviewed changes

pkg/util/testing/json_roundtrip.go Outdated Show resolved Hide resolved

pkg/util/testing/json_roundtrip.go Outdated Show resolved Hide resolved

pkg/util/testing/json_roundtrip.go Outdated Show resolved Hide resolved

alexzielenski requested a review from apelisse October 10, 2022 22:58

gofmt changes

5ca72f5

alexzielenski force-pushed the json_next branch from c02624d to e07a798 Compare October 11, 2022 18:58

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

alexzielenski mentioned this pull request Oct 11, 2022

update kube-openapi kubernetes/kubernetes#112988

Merged

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

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 11, 2022

alexzielenski added 4 commits October 11, 2022 21:21

add go-json-experiment/json to pkg/internal/third_party

c601e50

revert json experiment to 4987ed27d44794668c752d227a2d3a7be54570bc correct commit hash correct package path remove pngs

add jsonv2 implementation

395774f

refactor name

add jsonv2 roundtrip fuzz tests and benchmarks

3b3bb66

refactor name

alexzielenski force-pushed the json_next branch from e07a798 to 8d49439 Compare October 12, 2022 04:22

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 12, 2022

k8s-ci-robot assigned liggitt Oct 12, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 12, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 12, 2022

k8s-ci-robot merged commit 172d655 into kubernetes:master Oct 12, 2022

apelisse mentioned this pull request Oct 27, 2022

json.Marshal allocates a lot of unnecessary memory #324

Closed

liggitt mentioned this pull request Nov 23, 2022

Add apelisse as approver to pkg/validation #330

Merged

liggitt mentioned this pull request Jan 4, 2023

OpenAPI-based CEL type library kubernetes/kubernetes#113312

Merged

apelisse mentioned this pull request May 19, 2023

[UMBRELLA] OpenAPI is very expensive kubernetes/kubernetes#118136

Closed

haydentherapper mentioned this pull request Nov 6, 2023

Investigate using custom JSON parser sigstore/rekor#1812

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

openapiv2 experimental json unmarshaler #319

openapiv2 experimental json unmarshaler #319

alexzielenski commented Sep 27, 2022 •

edited

Loading

apelisse commented Sep 27, 2022 •

edited

Loading

apelisse commented Sep 27, 2022

apelisse commented Sep 27, 2022

apelisse commented Sep 28, 2022

apelisse left a comment

apelisse Sep 28, 2022

dsnet Sep 29, 2022

apelisse Oct 3, 2022

apelisse Oct 3, 2022

dsnet Oct 3, 2022

liggitt Oct 4, 2022

alexzielenski Oct 5, 2022 •

edited

Loading

dsnet left a comment

dsnet Sep 29, 2022

apelisse commented Oct 3, 2022

liggitt Oct 4, 2022

alexzielenski Oct 6, 2022

apelisse Oct 6, 2022

liggitt Oct 4, 2022

apelisse Oct 6, 2022

apelisse commented Oct 11, 2022

dsnet commented Oct 11, 2022

liggitt commented Oct 11, 2022

apelisse commented Oct 11, 2022

apelisse commented Oct 11, 2022

liggitt commented Oct 12, 2022

apelisse commented Oct 12, 2022

liggitt commented Oct 12, 2022

apelisse commented Oct 12, 2022

k8s-ci-robot commented Oct 12, 2022

apelisse commented Oct 12, 2022

openapiv2 experimental json unmarshaler #319

openapiv2 experimental json unmarshaler #319

Conversation

alexzielenski commented Sep 27, 2022 • edited Loading

apelisse commented Sep 27, 2022 • edited Loading

apelisse commented Sep 27, 2022

apelisse commented Sep 27, 2022

apelisse commented Sep 28, 2022

apelisse 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexzielenski Oct 5, 2022 • edited Loading

Choose a reason for hiding this comment

dsnet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apelisse commented Oct 3, 2022

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

apelisse commented Oct 11, 2022

dsnet commented Oct 11, 2022

liggitt commented Oct 11, 2022

apelisse commented Oct 11, 2022

apelisse commented Oct 11, 2022

liggitt commented Oct 12, 2022

apelisse commented Oct 12, 2022

liggitt commented Oct 12, 2022

apelisse commented Oct 12, 2022

k8s-ci-robot commented Oct 12, 2022

apelisse commented Oct 12, 2022

alexzielenski commented Sep 27, 2022 •

edited

Loading

apelisse commented Sep 27, 2022 •

edited

Loading

alexzielenski Oct 5, 2022 •

edited

Loading