Skip to content
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

KEP-2896: OpenAPI V3 #2898

Merged
merged 2 commits into from
Sep 9, 2021
Merged

KEP-2896: OpenAPI V3 #2898

merged 2 commits into from
Sep 9, 2021

Conversation

Jefftree
Copy link
Member

@Jefftree Jefftree commented Aug 25, 2021

  • One-line PR description: Initial commit for KEP-2896 OpenAPI V3

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 25, 2021
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Aug 25, 2021

- Support publishing and aggregating OpenAPI v3 for all kubernetes types
- Published v3 spec should be a lossless representation of the data and no fields should be stripped for both built-in types and CRDs
- Instead of serving the entire OpenAPI spec at one endpoint, separate the spec by group and only serve the resources required by each group
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this will come further down: by group is unfortunate as this means that the aggregator has to call out to each APIService and merge specs again. By group-version would remove this need.

Copy link
Member Author

@Jefftree Jefftree Aug 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any strong opinions about this, and will just list the benefits of each. It does seem like group/version reduces the complexity on the server side while potentially trading some performance on the client side.

Group:

  • Less calls needed to download a subset of the schema
  • Total size of schema downloaded is smaller if the entire schema needs to be aggregated client side

Group/version:

  • APIService does not need any additional aggregation
  • CRDs schemas do not need to be merged

@apelisse did a size analysis for group vs group/version for built-in resources.

Split Cardinality Avg Size (stddev) Aggregate Size
No split 1 131kB 131kB
Group 22 14kB (12kB) 308kB
Group/Version 44 11kB (10kB) 484kB

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which clients do you have in mind that care about more or less calls? Is there a client we see that needs the whole spec? I could see some client generators that would, but they don't care about 22 or 44 requests as they are batch-like jobs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know any latency sensitive clients that would care about the actual number of calls, it seems more of a nice to have. Different specs also have overlap that would be reduced by splitting only per group (total aggregate size is 30% smaller), but given the advantages of a omitting an aggregation step with group-version, let's adopt splitting by group-version.

@Jefftree Jefftree changed the title [WIP] KEP-2896: OpenAPI V3 KEP-2896: OpenAPI V3 Aug 31, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 31, 2021
@tengqm
Copy link

tengqm commented Sep 4, 2021

Maybe late to the party, maybe not... I have been studying the API specification for several years and I want to dump my thoughts here for consideration.

While I understand the benefits of generating API spec from source code, maybe we can/should switch to a different working model when we are migrating to OpenAPI v3. There are some OpenAPI spec features that are difficult to generate from source code. For example:

  • The structs like 'oneOf', 'allOf'. We tried this before, but the solution is not elegant. Maybe we are attacking the problem from the wrong end.
  • The different requirements for create, update and get operations. For example, 'metadata.name' may be required for a create operation, but it is almost always optional during an update. We cannot cover this difference in the generated spec.
  • The 'examples' section is always missing from the generated API.
  • There are other efforts on supporting "default", "enum", "minLength", "minimum" and even the linkage between a field and a feature-gate. All these efforts are trying to fix/improve the API generator ... but, are we investing on the correct problems to solve?

What I'd propose is a standalone API specification that is OpenAPI v3 format, SEPARATELY MAINTAINED rather than generated from the source code. We can use the 1.22 spec as a starting point, for example. The API validator logic validates API objects based on this specification. If we have some requirements that are difficult to express using OpenAPI spec, we either add some extensions to the spec, or we at least mention such constraints in the 'description'. API specification is the single source of truth. We do conformance tests based on the spec.

Source code comments are for developers. They can put whatever comments they need. They don't need to worry about the fact that the comments are "user-facing". They can put 'TODO's there when needed. We can remove those special tags in the comments rather than pushing hard on adding more tags.

I'm not quite aware of the drawbacks of this working model, so I'm open to all thoughts.

@sttts
Copy link
Contributor

sttts commented Sep 5, 2021

What I'd propose is a standalone API specification that is OpenAPI v3 format, SEPARATELY MAINTAINED rather than generated from the source code.

I don't think this is realistic to turn around the complete development model of Kube APIs and center it around OpenAPI, at least for native APIs, not without giant effort. It's definitely out of scope of this KEP.

Also note that for CRDs we basically follow what you suggest in the schema definition, technically. In practice though, nobody defines CRD schemas via OpenAPI first, and compliance of the Golang types via testing, although CRDs are totally capable of this way of working. For me this is a strong sign that this is not more viable or elegant than the approach we have.

We can use the 1.22 spec as a starting point, for example. The API validator logic validates API objects based on this specification. If we have some requirements that are difficult to express using OpenAPI spec, we either add some extensions to the spec, or we at least mention such constraints in the 'description'.

As we do today in CRD schemas, same approach.

Source code comments are for developers. They can put whatever comments they need. They don't need to worry about the fact that the comments are "user-facing". They can put 'TODO's there when needed.

They can today. We have two blocks of comments, separated by an empty line. If you see TODOs in API docs, it's a bug in the source code. Am not sure whether kubebuilder supports that, it is used everywhere in k/k types.

We can remove those special tags in the comments rather than pushing hard on adding more tags.

This sounds more like a comment about kubebuilder than about the mechanisms to publish OpenAPI v3 in this KEP.

I'm not quite aware of the drawbacks of this working model, so I'm open to all thoughts.

The core drawback is that the proposal has no proof that it works better. I would suggest to build a prototype of this model for CRDs, i.e. basically an alternative tool chain to kubebuilder that is centered around OpenAPI. I feel like this is a very othorgonal problem to solve than this KEP.

About the more specific questions from the top of the comment:

  • The different requirements for create, update and get operations. For example, 'metadata.name' may be required for a create operation, but it is almost always optional during an update. We cannot cover this difference in the generated spec.

Why can't we?

  • The 'examples' section is always missing from the generated API.

I don't see how this is a problem of the approach of generating from code. We admittedly don't use example, but could. I don't think there is a deep philosophical reason that we don't in our specs.

@tengqm
Copy link

tengqm commented Sep 8, 2021

would you be willing to (as exhaustively as possible) name the shortcomings of the spec and turn them into an actionable list of steps we can use as pre-req for a beta of OpenAPI v3 in Kube?

I'd try find some time slots for this, although I'm not confident the "shortcomings" I mentioned can be fixed if we don't switch the development model.

@deads2k
Copy link
Contributor

deads2k commented Sep 8, 2021

The PRR looks good. I am concerned about telling clients that they need to look at openapi/v3 and openapi/v2 to have a complete view of content. I think doing that means that we will be unable to remove openapi/v2.

if instead we find a way to show all the data (even in a slightly lossy way) via openapi/v3, then we have the ability to deprecate and eventually remove the openapi/v2 endpoint because we'll be able to direct clients to consume all the schema information via openapi/v3.

@sttts
Copy link
Contributor

sttts commented Sep 8, 2021

v2->v3 is fine as a beta requirement. We will attempt that. I think it is ugly, but feasible.

@Jefftree
Copy link
Member Author

Jefftree commented Sep 8, 2021

v2->v3 is fine as a beta requirement. We will attempt that. I think it is ugly, but feasible.

Updated v2->v3 as a beta requirement.

@kevindelgado kevindelgado mentioned this pull request Sep 8, 2021
13 tasks
@lavalamp
Copy link
Member

lavalamp commented Sep 8, 2021

@tengqm previous ideas to switch to some sort of IDL instead of go structs haven't gotten very far (e.g. see this one). I'm not sure how we could align on a specific IDL at this point. I am pretty sure, however, that there's little chance people would agree to hand-write OpenAPI and make that the source of truth.

@deads2k
Copy link
Contributor

deads2k commented Sep 9, 2021

/label tide/merge-method-squash
/lgtm
/approve
/hold

holding to prevent instant merge. I think we've got all comments in, but I'll quickly solicit to be sure.

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. labels Sep 9, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 9, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, Jefftree

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 9, 2021
@deads2k
Copy link
Contributor

deads2k commented Sep 9, 2021

holding to prevent instant merge. I think we've got all comments in, but I'll quickly solicit to be sure.

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 9, 2021
@k8s-ci-robot k8s-ci-robot merged commit 3e6261a into kubernetes:master Sep 9, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Sep 9, 2021
@apelisse
Copy link
Member

apelisse commented Sep 9, 2021

Thanks everyone for getting this merged, I'm very excited about it!

ravisantoshgudimetla pushed a commit to ravisantoshgudimetla/enhancements that referenced this pull request Sep 9, 2021
* OpenAPI v3

* Add heuristics note

Based on the provided group, clients can then request `openapi/v3/apis/apps/v1`,
`/openapi/v3/apis/networking.k8s.io/v1` and etc. These leaf node specs are self
contained OpenAPI v3 specs and include all referenced types.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it is late, but why don't we publish under /apis/networking.k8s.io/v1/openapi? It will remove all wiring into the aggregator.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @liggitt @deads2k for opinions.

This feels like a natural step and considerably simplifies the wiring and architecture within kube-apiserver. Basically apiextensions-apiserver will serve the spec directly. kube-aggregator will stop internally downloading the json blob and caching it in the aggregation layer. In the same way, kube-aggregator will not need special handling for openapi because /apis/foo/v1 is proxied to the aggregated apiserver already. That was the original reason why we switch to per-group-version specs here in the enhancement.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/apis/networking.k8s.io/v1/openapi conflicts with the endpoints for an openapi resource

Copy link
Member

@liggitt liggitt Nov 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(and matches the pattern for a resource URL instead of a non-resource URL... it would get authorized as verb=list,resource=openapi,group=networking.k8s.io,scope=cluster-wide)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can still proxy the requests to the apiextensions server even if the URL is different?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can proxy them, yes. Am fine with that approach. In the implementation PR, please make sure we avoid coupling apiextensions-apiserver with kube-aggregator (the downloader cames to mind). Rather serve the per-gv specs directly from the crd handler.

rikatz pushed a commit to rikatz/enhancements that referenced this pull request Feb 1, 2022
* OpenAPI v3

* Add heuristics note
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants