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

kubectl should apply OpenAPI schema defaults before OpenAPI schema validation #108768

Closed
sbueringer opened this issue Mar 17, 2022 · 14 comments
Closed
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. sig/cli Categorizes an issue or PR as relevant to SIG CLI. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@sbueringer
Copy link
Member

sbueringer commented Mar 17, 2022

What would you like to be added?

Today kubectl runs OpenAPI schema validation on e.g. kubectl apply without applying OpenAPI schema defaults before. (although validation can be disabled with --validate=false).

It would be great if kubectl could apply defaults before validation like the apiserver.

Why is this needed?

Example:

Let's assume we have the following required port field in a CRD:

      port:
        description: The port on which the API server is serving.
        format: int32
        default: 6443
        type: integer

In this case a user shouldn't have to set the port field because the APIserver first sets the default (6443) before validating if the required field exists.

With kubectl apply users get an error that the required field is not set.

A workaround is to use kubectl apply --validate=false instead.

Because we don't want to confuse our users, in ClusterAPI we made fields like this optional to avoid forcing users to set required fields on the client-side when they are not actually required because they have a default value.

@sbueringer sbueringer added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 17, 2022
@k8s-ci-robot k8s-ci-robot added needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 17, 2022
@dims
Copy link
Member

dims commented Mar 17, 2022

cc @nikhita @palnabarun

@neolit123
Copy link
Member

/sig api-machinery cli

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 17, 2022
@fedebongio
Copy link
Contributor

/assign @seans3
/remove-sig api-machinery

@k8s-ci-robot k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Mar 22, 2022
@seans3
Copy link
Contributor

seans3 commented Apr 27, 2022

/triage accept

@k8s-ci-robot
Copy link
Contributor

@seans3: The label(s) triage/accept cannot be applied, because the repository doesn't have them.

In response to this:

/triage accept

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/test-infra repository.

@seans3
Copy link
Contributor

seans3 commented Apr 27, 2022

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Apr 27, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 26, 2022
@vaibhav2107
Copy link
Member

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 2, 2022
@apelisse
Copy link
Member

I think there's a misunderstanding.

There is an absolute key rule here though, it's that kubectl will NEVER apply the defaults.

A field can not both have a default and be required, that makes no sense. You can't say that a field MUST be present, but if it's not present it will be given a default value, that's impossible. I'm going to give kubernetes-sigs/cluster-api#6398 a look to make sure I understand the purpose correctly, but that seems wrong, and we should be able to close this right now since that's never going to happen.

@sbueringer
Copy link
Member Author

sbueringer commented Aug 15, 2022

I think the corresponding CAPI issue is not necessarily relevant. What mainly confused us was that on the client-side we run OpenAPI validation while on the server-side we first run OpenAPI defaulting and then OpenAPI validation.

This means of course that the OpenAPI schema validation on client-side will have different results then the one on server-side.

@apelisse
Copy link
Member

apelisse commented Aug 15, 2022

Yeah, sorry, I realized later too that the api-server runs defaulting before validation. Without having spent too much time thinking about that, that seems like a design problem. I'm still pretty convinced we should mark these fields as non-required.

@apelisse
Copy link
Member

And we can't get kubectl to do that to be honest, but we're deprecating client-side validation anyway so this should go away soon hopefully.

@sbueringer
Copy link
Member Author

And we can't get kubectl to do that to be honest, but we're deprecating client-side validation anyway so this should go away soon hopefully.

Ah perfect, that's just as good for me, probably even better :)

@apelisse
Copy link
Member

Awesome, I'm going to close that for now then if you don't mind ...
And I'll leave this KEP here for prosperity: kubernetes/enhancements#2885

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. sig/cli Categorizes an issue or PR as relevant to SIG CLI. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

9 participants