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-3515: add new kep kubectl explain openapi v3 upgrade #3516

Merged
merged 23 commits into from
Oct 5, 2022

Conversation

alexzielenski
Copy link
Contributor

@alexzielenski alexzielenski commented Sep 14, 2022

  • One-line PR description: adding new draft KEP
  • Other comments:

@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. labels Sep 14, 2022
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 14, 2022
@alexzielenski alexzielenski changed the title [wip] KEP-3515: add new kep kubectl explai openapin v3 upgrade kep [wip] KEP-3515: add new kep kubectl explain openapin v3 upgrade Sep 14, 2022
@alexzielenski alexzielenski changed the title [wip] KEP-3515: add new kep kubectl explain openapin v3 upgrade [wip] KEP-3515: add new kep kubectl explain openapi v3 upgrade Sep 14, 2022

##### Mitigation

If the initial request for OpenAPI V3 fails, the old implementation using
Copy link
Member

Choose a reason for hiding this comment

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

I think considering the number of requests that kubectl can make on an invocation (especially including discovery), an extra request after a 404 is perfectly fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right but 404 is a positive HTTP server response. i.e. it is able to respond you with an error. If there is no response from the server due to gateway issues or it being too busy you need to timeout the request from the client-side

keps/sig-cli/3515-kubectl-explain-openapiv3/README.md Outdated Show resolved Hide resolved
keps/sig-cli/3515-kubectl-explain-openapiv3/README.md Outdated Show resolved Hide resolved
keps/sig-cli/3515-kubectl-explain-openapiv3/README.md Outdated Show resolved Hide resolved
@alexzielenski alexzielenski changed the title [wip] KEP-3515: add new kep kubectl explain openapi v3 upgrade KEP-3515: add new kep kubectl explain openapi v3 upgrade Sep 28, 2022
@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 Sep 28, 2022
keps/sig-cli/3515-kubectl-explain-openapiv3/README.md Outdated Show resolved Hide resolved
* markdown
* maybe others
5. (Optional?) Allow users to specify their own templates for use with kubectl
explain (there may be interesting use cases for this)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not saying this should be in scope for this KEP, but I think there's an interesting tie-in here to the ideas in kubernetes/kubectl#914. That issue proposes embedding templates that can be used to generate sample resources directly in the openAPI, so that instead of bespoke (and therefore unmaintainable) kubectl create commands, we could have fully extensible kubectl generate commands driven by this data. If kubectl explain is used to drive documentation in an even richer way thanks to this KEP, it would be pretty awesome if it could include generated examples in its output, perhaps through that mechanism.

@alexzielenski
Copy link
Contributor Author

/assign @johnbelamaric

for prr review

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 3, 2022
@alexzielenski
Copy link
Contributor Author

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Oct 3, 2022
keps/sig-cli/3515-kubectl-explain-openapiv3/README.md Outdated Show resolved Hide resolved
keps/sig-cli/3515-kubectl-explain-openapiv3/README.md Outdated Show resolved Hide resolved
keps/sig-cli/3515-kubectl-explain-openapiv3/README.md Outdated Show resolved Hide resolved
keps/sig-cli/3515-kubectl-explain-openapiv3/README.md Outdated Show resolved Hide resolved
keps/sig-cli/3515-kubectl-explain-openapiv3/README.md Outdated Show resolved Hide resolved
keps/sig-cli/3515-kubectl-explain-openapiv3/README.md Outdated Show resolved Hide resolved
keps/sig-cli/3515-kubectl-explain-openapiv3/README.md Outdated Show resolved Hide resolved

human-readable plaintext form:
```shell
kubectl explain pods --template /path/to/template.tmpl
Copy link
Contributor

Choose a reason for hiding this comment

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

After the recent change to the planned flag and having reflected more on the html/md options, I wanted to revisit this to make sure the flag set we've chosen isn't going to paint us into a corner. Even if we do add a default markdown implementation, it's easy to imagine that some users will prefer things to be displayed differently than we've chosen, leading to demand for this feature.

So, the new thought that has occurred to me is that it will technically be necessary for the user to be able to tell us which version of openAPI their template expects, similar to the problem we had with the long-term strategy around "raw". What would that look like? --output=openapiv3 --template /path/to/template.tmpl doesn't quite feel right... Our other commands that support file-based templating use --output=go-template-file --template=/path/to/template, so I think we'd want that for consistency. But it isn't sufficient, since as I was saying, the schema version isn't already inherent in the command, unlike in every case that currently exists.

So, maybe we need (at least) two flags after all, but not the ones we had this morning? I feel like I've talked myself in a circle. 😅

  • --output=plaintext|openapi-json|md|html|go-template-file --> during alpha and beta, requires a compatibility check with the openapi version flag; "plaintext" would be compatible with v2 or v3, all others require v3. But the next migration to the theoretical v4 wouldn't require that, since all templates would be required to support it.
  • --openapi-version=v2|v3--> This could be used to provide an opt-out via a "v2" value during the beta rollout period. At GA it becomes useless, so could be removed after deprecation (then re-introduced if we eventually need it again).
  • And if we do add templating: --template=/path/to/template.tmpl

(all would presumably need the experimental prefix during alpha)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think our built-in templates can hardcode their supported openapi versions. If in the future we add custom templates and --template, then I think it would be appropriate to think about how associated metadata is included, and possibly then add the --openapi-version flag. I think the current proposal does not preclude that possibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

What would it look like to hard code supported API versions? Do you mean in some convention surrounding how kubectl accesses them (not viable for custom templates), or within the template proper? I would think we couldn't embed the version in the template itself, since we'd need to parse the template to access it, which we couldn't do without knowing the version.

The implication I was seeing for the currently planned part of this proposal (sorry, it's kinda buried within the comment) is the value for the "raw" output format, because --output=openapiv3 --openapi-version=[whatever] seems problematic. If we change to something like --output=openapi-json then we end up requiring the separate flag to handle a version upgrade, but this thread supposes we may already have that requirement for other reasons.

Copy link
Member

Choose a reason for hiding this comment

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

We already have the following in kubectl:

  -o, --output='': Output format. One of:
json|yaml|wide|name|custom-columns=...|custom-columns-file=...|go-template=...|go-template-file=...|jsonpath=...|jsonpath-file=...
See custom columns [http://kubernetes.io/docs/user-guide/kubectl-overview/#custom-columns], golang template
[http://golang.org/pkg/text/template/#pkg-overview] and jsonpath template
[http://kubernetes.io/docs/user-guide/jsonpath].

How about:

kubectl explain pods -o v3-template=/path/to/template.tmpl

Copy link
Contributor Author

@alexzielenski alexzielenski Oct 4, 2022

Choose a reason for hiding this comment

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

What would it look like to hard code supported API versions? Do you mean in some convention surrounding how kubectl accesses them (not viable for custom templates), or within the template proper?

It could be a map in the Go code that uses them. Or a naming convention from filename. Or a yaml that lives next to the templates with metadata. I am not attached to a specified idea. But the idea is that it is internal and does not need to extend to custom templates, as long as it possible to change in the future to allow custom templates.

How about:

kubectl explain pods -o v3-template=/path/to/template.tmpl

If we were ever to add templates in the future, I like this option since it is consistent with the existing -o flag as you pointed out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KnVerey does your initial concern about leaving door open for future version-pinned templates still stand after hearing the above? Personally I am satisfied that we have not backed ourselves into a corner in terms of implementation or interface on this issue

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, you're right: I missed that the filepath could also be specified inline. I'm not sold on the specific option name because v3 refers to an openapi version, and openapi isn't anywhere in the command, so it sounds like it might instead refer to v3 of a templating system, or maybe of pod. Although part of me feels like we're mashing two concepts into one option, the main practical drawback of fully specifying --output=openapiv3-go-template= is verbosity. In any case, I won't block on this. The KEP as it stands now is simpler for the features certainly planned. Plus we have an opportunity to revisit naming if need be when we promote the flag out of experimental, at which point we should have a more solid stance on the custom template possibility.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, I wasn't trying to find the best name, but I think the mechanism leaves the door open for the future. Thanks!


human-readable plaintext form:
```shell
kubectl explain pods --template /path/to/template.tmpl
Copy link
Contributor

Choose a reason for hiding this comment

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

What would it look like to hard code supported API versions? Do you mean in some convention surrounding how kubectl accesses them (not viable for custom templates), or within the template proper? I would think we couldn't embed the version in the template itself, since we'd need to parse the template to access it, which we couldn't do without knowing the version.

The implication I was seeing for the currently planned part of this proposal (sorry, it's kinda buried within the comment) is the value for the "raw" output format, because --output=openapiv3 --openapi-version=[whatever] seems problematic. If we change to something like --output=openapi-json then we end up requiring the separate flag to handle a version upgrade, but this thread supposes we may already have that requirement for other reasons.

@KnVerey
Copy link
Contributor

KnVerey commented Oct 4, 2022

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 4, 2022
@alexzielenski
Copy link
Contributor Author

/assign @johnbelamaric

This feature only requires the target cluster has enabled The OpenAPIV3 feature.

OpenAPIV3 is Beta as of Kubernetes 1.24. This feature should not be on-by-default
until it is GA.
Copy link
Contributor

Choose a reason for hiding this comment

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

If I remember correctly, OpenAPIV3 is Beta in 1.24 and has been on by default

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

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexzielenski, johnbelamaric, KnVerey, seans3

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 Oct 5, 2022
@seans3
Copy link
Contributor

seans3 commented Oct 5, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 5, 2022
@k8s-ci-robot k8s-ci-robot merged commit 11a70d5 into kubernetes:master Oct 5, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.26 milestone Oct 5, 2022
ahmedtd pushed a commit to ahmedtd/enhancements that referenced this pull request Feb 2, 2023
…3516)

* add kubectl explain: openapi v3 upgrade kep

* spruce up wording

* reduce scope and take feedback into account

* fix typos

* fixx out kep yaml

* remove prnumber

what is this used for

* fill in testing plan a bit more

* rename raw to openapiv3

* update with feedback

* fix typo

* fill in version skew strategy

* update toc

* formatting fixes

* remove TODOs

* address feedback

* address feedback

* clarify --output fallback

* spelling

* address feedback about rollout

* add explicit not alpha 2 may be dropped

* add note about openapi v3 data not available

* add GA criterion

* changed feature gate to use environment variable
Copy link

@tscswcn tscswcn left a comment

Choose a reason for hiding this comment

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

view

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/cli Categorizes an issue or PR as relevant to SIG CLI. size/XXL Denotes a PR that changes 1000+ 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