Bump kube-openapi with v3 marshal and requestBody required marking #120735
Conversation
k8s-ci-robot
added
release-note
|
/triage accepted |
k8s-ci-robot
added
the
triage/accepted
k8s-ci-robot
removed
the
needs-triage
k8s-ci-robot
added
area/apiserver
area/cloudprovider
area/code-generation
area/dependency
|
/priority important-longterm |
k8s-ci-robot
added
priority/important-longterm
|
/retest |
k8s-ci-robot
added
the
needs-rebase
Jefftree
force-pushed
the
request-body
branch
from
ca0acba
to
92fe88d
Compare
k8s-ci-robot
removed
the
lgtm
k8s-ci-robot
removed
the
needs-rebase
|
/assign @liggitt |
| @@ -26610,7 +26673,8 @@ | |||
| "$ref": "#/components/schemas/io.k8s.apimachinery.pkg.apis.meta.v1.Patch" | |||
| } | |||
| } | |||
| } | |||
| }, | |||
| "required": true | |||
There was a problem hiding this comment.
what is this used for? the schema for patch doesn't look right for apply (can be yaml, not required to be json) or application/json-patch+json (is a list, not an object)
There was a problem hiding this comment.
communicating to the user that a requestbody is required for patch operations, this is something that we have in the openapi v2 schema. It's currently not used for anything other than for informational purposes.
As for the incorrect schema, cc @apelisse
Is the list here incorrect? https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/types/patch.go#L28
I see we only accept one format (json or yaml) for each of the patches, do we need to expand this list?
There was a problem hiding this comment.
We don't take json for apply, so that's correct, but the schema type is wrong indeed, it should be the type of the object (same as returned type). I think that's already true about v2 though. I'll open an issue for that.
There was a problem hiding this comment.
After looking into the code a bit more, the idea of consuming a "Patch" object instead of the return object schema seems intentional.
Per https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go#L1200-L1201
// Patch is provided to give a concrete name and type to the Kubernetes PATCH request body.
Due to limitations with go-restful, we're not able to specify different return types for different patch types. I'm testing out a PR to change the patch return type, though not sure if the OpenAPI is more accurate.
There was a problem hiding this comment.
yeah... I don't think that's more accurate. merge and apply patches take a sparse schema, meaning they would not actually require fields marked required in the specific type schemas.
strategic-merge-patch takes other directive properties that aren't present in the specific type schemas
jsonpatch takes an array of json patch items, which don't resemble the specific type schema at all
There was a problem hiding this comment.
How do we want to proceed with this PR in the meantime? @liggitt's comments describe the ideal state, I can create an issue to track this but it'll take some time to land because a go-restful update is probably needed. For this PR, are we okay with adding the "required" field? It just serves for informational purposes for the user. The goal was to address kubernetes/kube-openapi#427 and be consistent with OpenAPI V2. I can revert this for now if it's not desirable to add the field without the fixes described above.
There was a problem hiding this comment.
I didn't realize openapi v2 was already publishing body parameters with required: true
if that's been the case for multiple releases already, bringing this to parity seems ok. I checked and v2 has indicated body param is required (including the patch one) since at least 1.25
|
lgtm overall, the existing schema types don't look correct for a couple patch types... I'm not sure what the implications of marking the requestBody as required would be if that's the case |
Jefftree
force-pushed
the
request-body
branch
from
92fe88d
to
555c1b8
Compare
|
(latest push is purely a rebase) |
|
/lgtm based on #120735 (comment) please open a follow up issue to fix the incorrect patch schemas |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 9210ed8400f1cd6e272346bddb80f82507f9f96d
|
k8s-ci-robot
added
the
approved
What type of PR is this?
/kind feature
What this PR does / why we need it:
Includes
Updates the OpenAPI V3 to mark requestBody as required.
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.: