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

Move validation to be per-API-version #8550

Closed
thockin opened this issue May 20, 2015 · 11 comments
Closed

Move validation to be per-API-version #8550

thockin opened this issue May 20, 2015 · 11 comments
Labels
area/apiserver kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/backlog Higher priority than priority/awaiting-more-evidence. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.

Comments

@thockin
Copy link
Member

thockin commented May 20, 2015

Validation as it exists today is pretty wrong - it tracks internal types. Once we have long-term support for v1 API and are deep into v2 rework, the validation errors that are produced for v1 will be less and less useful.

We've gone back and forth about this but have not yet come up with a good answer on how to do this generically. I propose we just move it to be per-version, same as defaulting.

Given that v1beta[12] are on the block right now and v1beta3 will be soon, this is the right time. The same day we delete v1beta3 we should move pkg/api/validation into v1 and make it use versioned types.

Yes, it will be tedious to make changes, but I think that is the cost of having multiple versioned APIs in support cycles.

@thockin thockin added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels May 20, 2015
@nikhiljindal
Copy link
Contributor

My impression was that we have decided that we want to go this path.
Its not part of 1.0 since that will have just one API version: #3084 (comment)

@hurf
Copy link
Contributor

hurf commented May 20, 2015

Agree with @thockin , put validation on internal structs also looks odd to me. I think better to validate template itself for each version. Though #3084 may not be an issue now, it will come out along with v2, sooner or later we had to resolve this problem. I guess @nikhiljindal 's concern is mainly about limited timeline?

@bgrant0607
Copy link
Member

I think we have agreed in principle that validation should be versioned. I don't see this happening before 1.0, however. We should do it before introducing the v2 api.

See also #3084, #5889, #8116, #6210, #1451

@lavalamp @smarterclayton

@bgrant0607 bgrant0607 changed the title RFC: Move validation to be per-API-version? Move validation to be per-API-version May 20, 2015
@thockin
Copy link
Member Author

thockin commented May 20, 2015

That's fine, my point was we should do it after v1b3 dies and before v2b1
is born :)

On Wed, May 20, 2015 at 11:09 AM, Brian Grant notifications@github.com
wrote:

I think we have agreed in principle that validation should be versioned. I
don't see this happening before 1.0, however. We should do it before
introducing the v2 api.

See also #3084
#3084, #5889
#5889, #8116
#8116, #6210
#6210, #1451
#1451

@lavalamp https://github.com/lavalamp @smarterclayton
https://github.com/smarterclayton


Reply to this email directly or view it on GitHub
#8550 (comment)
.

@nikhiljindal
Copy link
Contributor

cc @caesarxuchao

@bgrant0607 bgrant0607 added this to the v1.2-candidate milestone Sep 12, 2015
@bgrant0607 bgrant0607 removed this from the next-candidate milestone May 18, 2016
@bgrant0607 bgrant0607 added priority/backlog Higher priority than priority/awaiting-more-evidence. help-wanted and removed priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels May 18, 2016
@bgrant0607
Copy link
Member

Ref #8190 so that it's on my "v2" radar.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

Prevent issues from auto-closing with an /lifecycle frozen comment.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or @fejta.
/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 Jan 3, 2018
@bgrant0607
Copy link
Member

/remove-lifecycle stale
/lifecycle frozen

@k8s-ci-robot k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 23, 2018
@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed kind/technical-debt labels Aug 23, 2018
@sftim
Copy link
Contributor

sftim commented Dec 21, 2022

Can I check: is this issue still relevant? If it is, how does it manifest? (eg: as maintenance toil for contributors)

@liggitt
Copy link
Member

liggitt commented Dec 21, 2022

I think completely per-version validation will cause more bugs than it fixes. Keeping open-coded validation rules consistent between multiple versions sounds really difficult to get right (both to write and to test).

We have the ability to propagate the request version into validation when we need to, and have used that in a few places to do ratcheting or version-specific validation, but generally, we expect all versions of an API to pass the same validation rules.

@thockin
Copy link
Member Author

thockin commented Dec 21, 2022

What we have is "obviously" wrong but works in practice. We have very few places where the field-path differs between API versions, so special-casing those works "well enough". It's smelly but not the worst smelling hunk of code we have. Doing it per-version would have a LOT of duplicated-but-identical code.

If/when we have a v2 of something major, we may want to revisit this. Hopefully by then we will have declarative validation for all but the most bespoke logic. Then we have to keep the decalarations in sync across API versions, but hopefully we can automate most of that.

@thockin thockin closed this as completed Dec 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/apiserver kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/backlog Higher priority than priority/awaiting-more-evidence. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.
Projects
None yet
Development

No branches or pull requests

8 participants