feature: Allow validation comment tags to be merged through aliases

[alexzielenski]

trafficstars

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]

[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~~ [alexzielenski]

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

[alexzielenski]

/cc @Jefftree curious what you think

[Jefftree]

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]

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]

PR needs rebase.

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.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all 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:

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

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

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all 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:

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

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

/lifecycle rotten

[k8s-triage-robot]

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

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

/close

[k8s-ci-robot]

@k8s-triage-robot: Closed this PR.

In response to this:

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

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

/close

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-sigs/prow repository.