Reactivate OpenAPI v3 JSON+Proto roundtrip integration test #126731
Conversation
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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-sigs/prow repository. |
| // In the JSON/YAML OpenAPI definitions, however, there are cases with "default": {}, deserializing into JSON as map[string]any. | ||
| // From protobuf, however, these deserialize naturally into nil, as they are not expressed. | ||
| // We should probably patch the OpenAPI v3 spec to not include any empty struct defaults at all in JSON form, as proto doesn't support it. | ||
| // For now, however, we can ignore these differences. |
There was a problem hiding this comment.
Actually, every JSON value could show up as default. How do we ensure these never hit these (outside of tests) ?
There was a problem hiding this comment.
If we are sure, these can never show up in the proto spec, we can restrict the fuzzing here (equivalent to your ignoreEmptyStructDefaults below).
Actually, we are in an integration test here. So the arbitrary defaults show up?
There was a problem hiding this comment.
You mean that someone would put "default": {"foo": "bar"} into the OpenAPI?
The other alternative is changing proto to support struct defaults I guess. Does that sound feasible?
There was a problem hiding this comment.
Tested this now with CRDs as well. The findings:
a) native types have at least "simple" defaults that proto support, e.g. string, number, bool. These show up in OpenAPI
b) if one specified defaults in a CRD OpenAPI schema declaratively, these show up in server's OpenAPI schema as well as you'd expect
c) however CRD declarative defaulting can be non-simple types, sth like {"cronSpec": "5 0 * * *", "image": "foo"}. This shows up correctly in JSON OpenAPI v3, but NOT at all in proto, here's the diff from this test in that case:
json is -
proto is +
"spec": {
VendorExtensible: {},
SchemaProps: spec.SchemaProps{
... // 6 identical fields
Format: "",
Title: "",
- Default: map[string]any{"cronSpec": string("5 0 * * *"), "image": string("foo")},
+ Default: nil,
Maximum: nil,
ExclusiveMaximum: false,
... // 24 identical fields
},
SwaggerSchemaProps: {},
ExtraProps: nil,
There was a problem hiding this comment.
Have double checks that we expose defaults of CRDs. So how do these show up in a proto serialization?
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: luxas The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
xref: google/gnostic-models#11 |
|
PR needs rebase. 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-sigs/prow repository. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Hi folks, reactivating the integration test which makes sure JSON and Proto encodings of OpenAPI v3 are equal.
This can be done now that the earlier issue #106387 is solved.
As can be seen, they are in fact not exactly similar, so maybe we should fix the JSON encoding of the OpenAPI v3 spec to not include
"default": {}?(FYI: Discovered this from digging into supporting OpenAPI v3 "natively" using the client-go discovery client)
cc recent kube-openapi contributors @sttts @Jefftree @apelisse @alexzielenski @thockin
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: