search

kubernetes / kube-openapi

Kubernetes OpenAPI spec generation & serving
Apache License 2.0
317 stars 205 forks source link

feature: Allow validation comment tags to be merged through aliases #449

Closed alexzielenski closed 2 months ago

alexzielenski commented 7 months ago

We have a number of shared Go types used in k8s API. It would be useful if we could use aliases to override validation of a base type without creating a new type (so it is transparent to Go).

One example is NodeSelectorTerm:

https://github.com/kubernetes/kubernetes/blob/2ce04fc04bf2cbbbacf2f184fd9ebd4e99d65430/pkg/apis/core/types.go#L2830-L2847

There are separate validations for MatchExpressions vs MatchField both with type NodeSelectorRequirement despite the type being the same.

https://github.com/kubernetes/kubernetes/blob/7972f0309ce8bad3292f3291718361367b2e58fe/pkg/apis/core/validation/validation.go#L4384-L4396

It wouldn't be good to write complex CEL .all quantification expressions due to the fact that the error messages wouldnt be reported in the correct locations.

This PR makes it so we can define separate types overlaying a common type through aliases, and our validation comment tags still function. The tags specified closest to the the field take precedence, and we follow the type and aliases thereafter.

k8s-ci-robot commented 7 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexzielenski

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: - ~~[OWNERS](https://github.com/kubernetes/kube-openapi/blob/master/OWNERS)~~ [alexzielenski] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
alexzielenski commented 7 months ago

/cc @Jefftree curious what you think

Jefftree commented 7 months ago

This has the same idea as why we have a separate "description" field that we override for referenced types. I like the merging order, Aliases closest to t should take precedence is quite important. I haven't looked at the implementation too closely but overall +1 on the idea.

I'm trying to understand the schema implications a bit more. How does the OpenAPI look like for a type alias field? Does the validation field for an upstream type T propagate down to the alias? I understand that's the end behavior but what does the schema for that look like?

jpbetz commented 7 months ago

Putting validation on an alias type helps a lot here.

Given how hard it is to expose the validation data in OpenAPI, I'd be okay with "server side only" declarative validation for these cases, at least until we have more time to look into ways to either merge or inject validation into shared types in OpenAPI while still keeping the changes non-disruptive to OpenAPI generated clients.

EDIT: I could imagine a couple possible ways to implement "server side only" declarative validation. One would be to use the aliases like proposed in this PR, but don't change OpenAPI. Another might be to have a declarative way to call out to hand written validation as a stop-gap that would allow us to make incremental forward progress on a migration.

k8s-ci-robot commented 7 months ago

PR needs rebase.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
k8s-triage-robot commented 4 months ago

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

This bot triages PRs according to the following rules:

You can:

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

/lifecycle stale

k8s-triage-robot commented 3 months ago

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

This bot triages PRs according to the following rules:

You can:

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

/lifecycle rotten

k8s-triage-robot commented 2 months ago

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

This bot triages PRs according to the following rules:

You can:

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

/close

k8s-ci-robot commented 2 months ago

@k8s-triage-robot: Closed this PR.

In response to [this](https://github.com/kubernetes/kube-openapi/pull/449#issuecomment-2241471808): >The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. > >This bot triages 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 PR is closed > >You can: >- Reopen this PR with `/reopen` >- Mark this PR as fresh with `/remove-lifecycle rotten` >- Offer to help out with [Issue Triage][1] > >Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community). > >/close > >[1]: https://www.kubernetes.dev/docs/guide/issue-triage/ Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.